Re: Style of messages for checkArgument/checkNotNull in IOs

[Robert Bradshaw]

Huge +1 to the

checkArgument(username != null, ...)

style. A note on validate(), aren't we trying to remove pipeline
options from PTransforms altogether (and, in addition, how does this
even work with the Runner API and cross-language transforms).

On Thu, Aug 10, 2017 at 4:59 PM, Eugene Kirpichov
 wrote:
> 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 
> 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 
>> 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 > >
>> > 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 

Re: Style of messages for checkArgument/checkNotNull in IOs

[Eugene Kirpichov]

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 
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 
> 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  >
> > 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 

Re: Style of messages for checkArgument/checkNotNull in IOs

[Kenneth Knowles]

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 
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 
> 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");
> > >   ...
> > > }
> > >
> >
>

Re: Style of messages for checkArgument/checkNotNull in IOs

[Reuven Lax]

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 
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");
> >   ...
> > }
> >
>

Re: Style of messages for checkArgument/checkNotNull in IOs

[Eugene Kirpichov]

(I don't think changing messages in existing IOs is a high priority, btw -
just a nice-to-have, as long as we have guidance for future IOs)

On Fri, Jul 28, 2017 at 11:37 AM Eugene Kirpichov 
wrote:

> Okay, so then let's change guidance to:
> - Always use checkArgument to check stuff that the user supplied
> - Use style 1 for messages on the checkArgument
> ?
>
> On Fri, Jul 28, 2017 at 11:31 AM Jean-Baptiste Onofré 
> wrote:
>
>> Agree
>>
>> Regards
>> JB
>>
>> On Jul 28, 2017, 20:22, at 20:22, Thomas Groh 
>> wrote:
>> >I'm in favor of the wording in the style of the first: it's an
>> >immediately
>> >actionable message that will have an associated stack trace, but will
>> >provide the parameter in plaintext so the caller doesn't have to dig
>> >through the invoked code, they can just look at the documentation.
>> >
>> >I've recently been convinced that all input validation should go
>> >through
>> >`checkArgument` (including for nulls) rather than 'checkNotNull', due
>> >to
>> >the type of exception thrown, so I'd usually prefer using that as the
>> >`Preconditions` method. Beyond that, +1
>> >
>> >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");
>> >>   ...
>> >> }
>> >>
>>
>

Re: Style of messages for checkArgument/checkNotNull in IOs

[Eugene Kirpichov]

Okay, so then let's change guidance to:
- Always use checkArgument to check stuff that the user supplied
- Use style 1 for messages on the checkArgument
?

On Fri, Jul 28, 2017 at 11:31 AM Jean-Baptiste Onofré 
wrote:

> Agree
>
> Regards
> JB
>
> On Jul 28, 2017, 20:22, at 20:22, Thomas Groh 
> wrote:
> >I'm in favor of the wording in the style of the first: it's an
> >immediately
> >actionable message that will have an associated stack trace, but will
> >provide the parameter in plaintext so the caller doesn't have to dig
> >through the invoked code, they can just look at the documentation.
> >
> >I've recently been convinced that all input validation should go
> >through
> >`checkArgument` (including for nulls) rather than 'checkNotNull', due
> >to
> >the type of exception thrown, so I'd usually prefer using that as the
> >`Preconditions` method. Beyond that, +1
> >
> >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");
> >>   ...
> >> }
> >>
>

Re: Style of messages for checkArgument/checkNotNull in IOs

[Jean-Baptiste Onofré]

Agree

Regards
JB

On Jul 28, 2017, 20:22, at 20:22, Thomas Groh  wrote:
>I'm in favor of the wording in the style of the first: it's an
>immediately
>actionable message that will have an associated stack trace, but will
>provide the parameter in plaintext so the caller doesn't have to dig
>through the invoked code, they can just look at the documentation.
>
>I've recently been convinced that all input validation should go
>through
>`checkArgument` (including for nulls) rather than 'checkNotNull', due
>to
>the type of exception thrown, so I'd usually prefer using that as the
>`Preconditions` method. Beyond that, +1
>
>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");
>>   ...
>> }
>>

Re: Style of messages for checkArgument/checkNotNull in IOs

[Jean-Baptiste Onofré]

Hi

I'm a bit concerned because it has been discussed in a Jira and we discussed 
especially about this with Kenn and I. It dates from the incubation period.

I changes most of the IOs I did for style 2 whereas I started with style 1.

I'm ready to change again on the IOs but we agreed that checkArgument and 
checkState are better to explain cause to the users more than checkNotNull.

For me it's important to keep a coherence in the project.
Either way is fine as far as it's consistent and it's not a single person 
decision.

Regards
JB


On Jul 28, 2017, 20:18, at 20:18, Eugene Kirpichov 
 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");
>  ...
>}

Re: Style of messages for checkArgument/checkNotNull in IOs

[Raghu Angadi]

On Fri, Jul 28, 2017 at 11:21 AM, Thomas Groh 
wrote:

> I'm in favor of the wording in the style of the first: it's an immediately
> actionable message that will have an associated stack trace, but will
> provide the parameter in plaintext so the caller doesn't have to dig
> through the invoked code, they can just look at the documentation.
>
> I've recently been convinced that all input validation should go through
> `checkArgument` (including for nulls) rather than 'checkNotNull', due to
> the type of exception thrown, so I'd usually prefer using that as the
> `Preconditions` method. Beyond that, +1
>

+1. For me, main requirement is that it should be aimed at end user and
should be actionable without having to look at the code. It is always a
delight to receive message useful error as early as possible. This also
applies when checks are made in validate().


>
> 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");
> >   ...
> > }
> >
>

Re: Style of messages for checkArgument/checkNotNull in IOs

[Kenneth Knowles]

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");
>   ...
> }
>

Re: Style of messages for checkArgument/checkNotNull in IOs

[Thomas Groh]

I'm in favor of the wording in the style of the first: it's an immediately
actionable message that will have an associated stack trace, but will
provide the parameter in plaintext so the caller doesn't have to dig
through the invoked code, they can just look at the documentation.

I've recently been convinced that all input validation should go through
`checkArgument` (including for nulls) rather than 'checkNotNull', due to
the type of exception thrown, so I'd usually prefer using that as the
`Preconditions` method. Beyond that, +1

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");
>   ...
> }
>

Style of messages for checkArgument/checkNotNull in IOs

[Eugene Kirpichov]

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");
  ...
}