https://beam.apache.org/contribute/ptransform-style-guide/#validation now includes the new guidance.
It also includes updated guidance on what to put in expand() vs. validate() (TL;DR: validate() is almost always unnecessary. Put almost all validation in expand()) On Fri, Jul 28, 2017 at 11:56 AM Kenneth Knowles <k...@google.com.invalid> wrote: > So here's an easy solution: our own checkNotNull that throws > InvalidArgumentException with a good error message. The error message can > then be templated to allow terse invocations with just the method and > parameter. > > Unsure why I didn't go for this straightaway. > > On Fri, Jul 28, 2017 at 11:43 AM, Reuven Lax <re...@google.com.invalid> > wrote: > > > Yuck. I think that checkNotNull throwing a NPE is a very poor design > choice > > from the author of checkNotNull. > > > > On Fri, Jul 28, 2017 at 11:29 AM, Kenneth Knowles <k...@google.com.invalid > > > > wrote: > > > > > As to the choice of check method: > > > > > > - checkArgument throws an InvalidArgumentException, which is clearly > in > > > "your fault" category, a la HTTP 4xx > > > - checkNotNull throws an NPE, which is usually a "my fault" > exception, a > > > la HTTP 5xx. > > > > > > The docs on NPE are not clear on blame, and this is a bug in the docs. > > But > > > almost all the time, an NPE indicates that the line where it is thrown > is > > > incorrect. InvalidArgumentException is unambiguous. This could also be > > > called a bug in checkNotNull. It throws the same exception as if you > > > _forgot_ to check if it was not null. So it sort of doesn't do one of > > the > > > most important things it should be doing. > > > > > > As to verbosity: All error messages should be actionable. We have a > > chronic > > > problem with terrible or nonexistent error messages. > > > > > > NPE is uninformative and this feeds into the prior two bullets: If I > see > > > "NPE on line XYZ of file ABC" I am _always_ going to file a bug against > > the > > > author of file ABC because they dereferenced null. Their fix might be > to > > > simply protect themselves with a checkArgument to clearly blame their > > > caller, but that is a totally acceptable bug/fix pair. > > > > > > We should really get an analysis in place based on @Nullable > annotations > > to > > > mitigate this a bit, too. > > > > > > Kenn > > > > > > On Fri, Jul 28, 2017 at 11:17 AM, Eugene Kirpichov < > > > kirpic...@google.com.invalid> wrote: > > > > > > > Hey all, > > > > > > > > I think this has been discussed before on a JIRA issue but I can't > find > > > it, > > > > so raising again on the mailing list. > > > > > > > > Various IO (and non-IO) transforms validate their builder parameters > > > using > > > > Preconditions.checkArgument/checkNotNull, and use different styles > for > > > > error messages. There are 2 major styles: > > > > > > > > 1) style such as: > > > > checkNotNull(username, "username"), or checkArgument(username != > null, > > > > "username can not be null") or checkArgument(username != null, > > > > "username must be set"); > > > > checkArgument(batchSize > 0, "batchSize must be non-negative, but > was: > > > %s", > > > > batchSize) > > > > > > > > 2) style such as: > > > > checkArgument( > > > > username != null, > > > > "ConnectionConfiguration.create().withBasicCredentials( > > > > username, > > > > password) " > > > > + "called with null username"); > > > > checkArgument( > > > > !username.isEmpty(), > > > > "ConnectionConfiguration.create().withBasicCredentials( > > > > username, > > > > password) " > > > > + "called with empty username"); > > > > > > > > Style 2 is recommended by the PTransform Style Guide > > > > > https://beam.apache.org/contribute/ptransform-style-guide/#transform- > > > > configuration-errors > > > > > > > > However: > > > > 1) The usage of these two styles is not consistent - both are used in > > > about > > > > the same amounts in Beam IOs. > > > > 2) Style 2 seems unnecessarily verbose to me. The exception thrown > > from a > > > > checkArgument or checkNotNull already includes the method being > called > > > into > > > > the stack trace, so I don't think the message needs to include the > > > method. > > > > 3) Beam is not the first Java project to have validation of > > configuration > > > > parameters of something or another, and I don't think I've seen > > something > > > > as verbose as style 2 used anywhere else in my experience of writing > > > Java. > > > > > > > > What do people think about changing the guidance in favor of style 1? > > > > > > > > Specifically change the following example: > > > > > > > > public Twiddle withMoo(int moo) { > > > > checkArgument(moo >= 0 && moo < 100, > > > > "Thumbs.Twiddle.withMoo() called with an invalid moo of %s. " > > > > + "Valid values are 0 (inclusive) to 100 (exclusive)", > > > > moo); > > > > return toBuilder().setMoo(moo).build();} > > > > > > > > into the following: > > > > > > > > public Twiddle withMoo(int moo) { > > > > checkArgument(moo >= 0 && moo < 100, > > > > "Valid values for moo are 0 (inclusive) to 100 (exclusive), " > > > > + "but was: %s", > > > > moo); > > > > return toBuilder().setMoo(moo).build();} > > > > > > > > > > > > And in simpler cases such as non-null checks: > > > > public Twiddle withUsername(String username) { > > > > checkNotNull(username, "username"); > > > > checkArgument(!username.isEmpty(), "username can not be empty"); > > > > ... > > > > } > > > > > > > > > >
