Feature Request - Method to verify @NonNull fields are not null

[Andrey]

Since the @NoArgsConstructor leaves the @NonNull fields uninitialized, I would like to verify that the @NonNull fields are indeed not null before I start using the object (e.g. in the init method of a SpringBean).  The alternative is to write notNull(field) checks for all of fields that are already marked as @NonNull (aka, add more boiler plate) and cringe.

[Reinier Zwitserloot]

So what would the recipe be then? You're going to have to explain it like I'm 5, I'm not a user of SpringBean etc. a before / after or some similar treatment.

 --Reinier Zwitserloot

On Wed, Jul 31, 2013 at 9:35 PM, Andrey <skya...@gmail.com> wrote:

Since the @NoArgsConstructor leaves the @NonNull fields uninitialized, I would like to verify that the @NonNull fields are indeed not null before I start using the object (e.g. in the init method of a SpringBean).  The alternative is to write notNull(field) checks for all of fields that are already marked as @NonNull (aka, add more boiler plate) and cringe.

--
You received this message because you are subscribed to the Google Groups "Project Lombok" group.
To unsubscribe from this group and stop receiving emails from it, send an email to project-lombo...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
 
 

[Andrey]

Lets keep Spring out of this - the functionality I described is in no way specific to it.  Here's a concrete example:

Annotated
============================
@NoArgsConstructor
public class Example
{
    @Setter @NonNull Long val;
    @Setter @NonNull private List< Long > list;
   
    // Desired annotation addition
    @VerifyNonNull
    public void init() {   
        assert( false == list.isEmpty() );
    }
}

Plain Java
============================
public class Example
{
    Long val;
    private List< Long > list;

    // No args constructor
    // ...

    // Setters with null checks, etc.
    // ...

    public void init() {
        // Start of desired addition
        // One null check for every @NonNull labeled field
        if ( val == null ){
            throw new NullPointerException("val");
        }
       
        if ( list == null ) {
            throw new NullPointerException("list");
        }
        // End of desired addition
       
        assert( false == list.isEmpty() );
    }
}
============================

[Reinier Zwitserloot]

Ah, I get it. The annotation would instruct lombok to inject a null-check statement for each field annotated with @NonNull at the  beginning of the so annotated method, if I understand your example correctly.

That doesn't sound particularly hard to do, and it is indeed more or less 'generic', in that it could feasibly be useful for any number of frameworks, and it is not inherently interwoven with springbeans.

A few afterthoughts:

Should it be possible to stick @VerifyNonNull on a class? This would mean that an init() method is generated with the assorted null checks if 'init()' doesn't exist yet, and if it does exist, that init() method gets the @VerifyNonNull treatment. Should it be possible to configure the method name (with the caveat that trying to set the method name if using @VerifyNonNull in annotated-method mode is an illegal use and gets you a compiler error)?

Is there anything else that might have to be configurable? Should it be possible to configure excludes/of the way you can for i.e. @EqualsAndHashCode?

 --Reinier Zwitserloot

[Fabrizio Giudici]

On Sat, 03 Aug 2013 07:03:42 +0200, Reinier Zwitserloot
<rei...@zwitserloot.com> wrote:

> Is there anything else that might have to be configurable? Should it be
> possible to configure excludes/of the way you can for i.e.
> @EqualsAndHashCode?

The proposal sounds good to me, but why don't we make it a bit more
generic? Sure @NonNull is one of the most important preconditions, but
there's also @Nonnegative, for instance, and eventually other annotations
from the validation API. Rather a @VerifyNonNull it would be good to see a
@ValidationEnforcer or such.

Of course I reckon that enlarging the scope makes it longer the
definitions of the specs and the implementation - perfect is the enemy of
good. But let's think about it.


--
Fabrizio Giudici - Java Architect @ Tidalwave s.a.s.
"We make Java work. Everywhere."
http://tidalwave.it/fabrizio/blog - fabrizio...@tidalwave.it

[Andrey]

Should it be possible to stick @VerifyNonNull on a class? This would mean that an init() method is generated with the assorted null checks if 'init()' doesn't exist yet, and if it does exist, that init() method gets the @VerifyNonNull treatment. Should it be possible to configure the method name (with the caveat that trying to set the method name if using @VerifyNonNull in annotated-method mode is an illegal use and gets you a compiler error)?

I like the idea of having a class-level annotation for a default init method that just does verification.  I would also advocate for a method-level annotation in the case that there is an init method that does something.  I would prefer this over adding an attribute to the class-level annotation because it makes it impossible to mistype the init method's name.  However, allowing the class-level annotation to override the default validation method name makes sense.  Having both the class-level and the method level annotation should be a compile time error.

@Validated

class SomeClass {

    @NonNull Long val;

};

Generates a default init method (named "init") that just does validation.

@Validated( methodName="validate" )

class SomeClass {

    @NonNull Long val;

};

Generates a default init method (named "validate") that just does validation.

class SomeClass {

    @NonNull Long val;

    long square;

    @Validated

    public void init() {

        square = val * val;

    }

};

Adds the validation logic to the beginning of the annotated init method.

@Validated
class SomeClass {

    @NonNull Long val;

    long square;

    @Validated

    public void init() {

        square = val * val;

    }

};

Compile time error.

> Is there anything else that might have to be configurable? Should it be
> possible to configure excludes/of the way you can for i.e.
> @EqualsAndHashCode?

> but there's also @Nonnegative, for instance, and eventually other annotations from the validation API

Short version - yes, but let's put it as an attribute of the @NonNull annotation to be more explicit and avoid potential typos in the field names.  I think this could work for @NonNegative and friends as well.

@NonNull( validate=false ) private List<Long> uncheckedList;

Long version.  Here are my thoughts on the special cases that came to mind.

Currently, the @RequiredArgsConstructor doesn't generate a null check for @NonNull fields that have been explicitly initialized to null.  This makes perfect sense if the way you create your objects is to create an empty object and then call a bunch of setters on it (better known as setter injection).

Once the injection finishes, though, all of your assertions should hold.  The code that the user writes in the "init" method should assume all of the assertions hold/have been verified.  If, for some reason, you don't want to include the @NonNull field in these checks, then say it explicitly with the @NonNull( validate=false ) construct above.

@NonNull private List<Long> list = null;

Don't check in @RequiredArgsConstructor (if there is one), but do check in @ValidateNonNull.  Hope that the user initializes this variable in some way before the init method is called.

@NonNull( validate=false ) private List<Long> list = null;

Don't check in either the @RequiredArgsConstructor nor in the @ValidateNonNull.  This should generate a warning, because it's semantically equivalent to the much simpler
private List<Long> list;

@Setter @NonNull( validate=false ) private List<Long> list = null;

Here, the @NonNull adds a null check only in the @Setter, but prevents the @RequiredArgsConstructor and @ValidateNonNull from checking the field.

[Fabrizio Giudici]

On Sat, 03 Aug 2013 19:51:59 +0200, Andrey <skya...@gmail.com> wrote:

...

Keep in mind @PostConstruct too. I'd add to the picture that if a method
annotated with @PostConstruct exists, then all the checks should be done
at the beginning of it.

[Reinier Zwitserloot]

@Verify sounds like a better 'word' - while in 95% of all cases you'd be verifying post construction, I can imagine running verify checks at other times, such as just before exporting an object (via JPA or otherwise).

The big hurdle to cross now is: Which validations do we support? Do we make our own and create a bajillion annotations to represent preconditions (@NonEmpty for strings, @NonNull of course, @NonNegative, @LargerThan, @LargerOrEqual, @NotNaN - and i'm just spitballing here) - that's quite a bit of scope creep, and as neither I nor Roel use this much, we might not be the best guys to make this API. I do like the concept; we both are rather enamoured of the 'annotations-as-compiler-checked-documentation' concept.

[Martin Grajcar]

I'm afraid, I'll make this even more complicated....

I guess there are the following kinds of methods w.r.t. the validation:

- such that may be used before the object gets properly initialized (this might be a single method or multiple ones, e.g., setters if setter injection gets used)

- such that guarantee that the object is properly initialized when they finish (some init() method)

- such that need properly initialized object when they start (the "normal" methods)

- and finally the method doing the verification

This looks like @PreValidate, @PostValidate, and @Validator. Combining the first two may makes sense in order to check that a method starts with an object in a proper state and doesn't invalidate it (this is the fifth kind and probably useful mainly for debugging).

The first two annotations make sense on a class level and then apply to all methods where this make sense. Placing a nonsensical annotation on a method is a compile-time error, while a class level annotation nonsensical for a method gets ignored. Fortunately, it's rather obvious:

- No above annotation applies to any static method.

- Only @PostValidate applies to constructors.

- @Validator is incompatible with both other annotations.

The annotations @PreValidate and @PostValidate on a method level should accept false as an argument, so that the effect of the class level annotation can be turned off.

There can be a single method (its name doesn't matter) annotated with @Validator, which is responsible for all the checks. By default it gets extended by all the checks Lombok can generate (this can be switched off by @Validator(extend=false) or whatever).

If there's no such method, it gets generated with some unspecified, strange dollar-containing name. Anybody who cares about the name can write

@Validator void verify() {}

By using no strings, it's refactoring-proof. Apologies for the long things-complicating post.

[Fabrizio Giudici]

On Sun, 04 Aug 2013 16:12:21 +0200, Reinier Zwitserloot
<rein...@gmail.com> wrote:

> The big hurdle to cross now is: Which validations do we support? Do we
> make
> our own and create a bajillion annotations to represent preconditions

-1

As I said, I'd go with existing JSR 303 annotations. Since I believe
there's a reference implementation of the validator, I'd say:

* @NonNull, @NonNegative etc... should be enforced with generated code
only (it's simple), without using an external library
* Any JSR 303 annotation would generate a call to an external library (as
much as @Slf4J does).

There are for sure some corner case to deal with - but this is a good
starting point IMHO.

[Lenny Primak]

I agree. I think that re-inventing additional annotations is a no-no.
Re-use JSR 303 annotations all the way

[Reinier Zwitserloot]

There are lots of aspects to JSR 303 that aren't very compatible with lombok.

* Making your own validation annotations with their own validator class: Lombok does not have resolution (or at least, I'd rather not try and glue resolution into this feature). We simply wouldn't know that @ZipCode is a custom validation annotation, which results in the very very bad end result of the verifier method simply not checking this, without any visual feedback that it isn't happening.

* Validation restrictions are inherited. Again, without resolution, we'd be silently omitting required calls.

* In general it looks like the idea of JSR 303 is that you call into some sort of validation framework to do your validating for you; it will scan the class for the annotations and apply these to the fields directly using reflection. That seems like a MUCH more sensible way to handle validation.

Wouldn't it be a lot easier all around that those who need validation use a library to do so? Perhaps lombok can help you out and generate an init() method with the call to the validation framework inside it?

The showstopper issue I have with re-using the JSR 303 annotations is that this would get two things wrong:

* It creates the impression that lombok is an implementation (of sorts) of a JSR 303 validator, but we can't do most of the spec, so actually we're just using a few of their annotations but 'This is a JSR-303 style validator' is entirely the wrong mindset. This is a pretty serious violation of the principle of least surprise.

* Where-ever there is a mismatch between what the programmer expects would happen, and what lombok actually does, it is likely that nobody knows things are awry; errors in validation are usually not spotted unless you have an explicit test for every violation, and we don't live in the ponies and rainbow world where everyone has 100% test coverage for all code, all the time. Violating the principle of least surprise is one thing. Failing silently when things go wrong? Unacceptable.

At the same time, I really don't think we should reinvent the wheel on this, which means I'm now strongly considering this feature to not be a feasible plan. If you need validation, use a framework to do that; lombok cannot help in a way that wouldn't be highly confusing.

So, is there something we can generate to call into a standard framework or two? We have the extern package just for this: To help reduce boilerplate involved in writing against third party libraries.

 --Reinier Zwitserloot

[Lenny Primak]

I don't think you need to implement the whole JSR303 in lombok.

I just don't want having >=2 annotations that do the same thing.

for example, @NonNull vs. @NotNull.  There should just be @NotNull (JSR303)

that lombok supports.  It doesn't have to fully support it, just support it enough.

Think of it like JSR303 light.  

Right now, I have @NonNull @NotNull in my code (which sucks)

[Fabrizio Giudici]

On Sun, 04 Aug 2013 17:17:53 +0200, Lenny Primak <lpr...@hope.nyc.ny.us>
wrote:

> I don't think you need to implement the whole JSR303 in lombok.
> I just don't want having >=2 annotations that do the same thing.
> for example, @NonNull vs. @NotNull. There should just be @NotNull
> (JSR303)
> that lombok supports. It doesn't have to fully support it, just support
> it enough.

This is a technical solution; still I understand Reinier's point about the
fact that if you use stuff from a JSR, then people expect that you fully
comply with that JSR. We need to think about it.

[Lenny Primak]

Yea I get that, but I think it's more evil to reinvent the annotations still
Clear cut for me.

[Reinier Zwitserloot]

The part where reusing some of these results in both (A) violation of principle of least surprise (the 'least surprise' is that lombok is JSR-303 compatible, but it wouldn't be), and (B) misunderstanding the problem in A results in no obvious bugs or compiler errors unless you have a complete test suite result inevitably in (C): This isn't going to happen.

 --Reinier Zwitserloot

[Lenny Primak]

I think 90% people that are using these annotations never heard of JSR303 or what its supposed to do.

[Reinier Zwitserloot]

If we're going to re-use the JSR-303 annotations, they would have to at least download it, no?

 --Reinier Zwitserloot

[Lenny Primak]

Since they are part of the JEE spec people usually get them "for free" with the app server or as a transitive dependency. I've never heard anybody specifically downloading these by themselves. 

[Andrey]

So lets reduce the scope to only validation-type annotations provided by Lombok.  Right now that's @NonNull.  Incorporating JSR 303 is a separate project that seems scope at this point.  But Lombok is already generating validators for @NonNull in @Setters and @RequiredArgsConstructors, so adding another one in @Verify keeps all provided functionality in Lombok and satisfies the principal of least surprise.

[Fabrizio Giudici]

On Sun, 04 Aug 2013 21:15:04 +0200, Andrey <skya...@gmail.com> wrote:

> So lets reduce the scope to only validation-type annotations provided by
> Lombok. Right now that's @NonNull. Incorporating JSR 303 is a separate
> project that seems scope at this point. But Lombok is already generating
> validators for @NonNull in @Setters and @RequiredArgsConstructors, so
> adding another one in @Verify keeps all provided functionality in Lombok
> and satisfies the principal of least surprise.

I agree on splitting. I think we should only think a bit about backward
compatibility when JSR 303 support will be eventually added. I mean, if
you just implement reduced validation, tomorrow using @Verify with JSR 303
annotations in place won't enforce them. The day after tomorrow, when JSR
303 support will be added, the code won't be backward-compatible, because
all of a sudden @Verify enforces JSR 303. I suggest that @Verify accept an
argument, which is an array of enums defining the target library it is
enforcing. Thus, tomorrow we will just have @Verify which means
javax.annotation.*; the day after tomorrow @Verify(target=JSR303) will
enforce also JSR 303; and eventually new stuff could be added by means of
@Verify(target={JSR303,SOMETHINGELSE}).

[Andrey]

I'm not sure it makes sense to marry these two things right now.  On the surface they seem similar - enforce constraints specified by annotations.  But I think the deeper analysis that we've done in this thread shows that the task of enforcing non-lombok annotations is a completely different problem with a potentially different implementation (e.g. doing the verifications at runtime) and a much larger scope.  Rather than thinking about this as a planned future iteration of the @Verify semantics, I would suggest we think of it as a completely separate feature and decouple their destinies.  Maybe we should start a separate thread for the larger scope discussion?  I would like to refocus this one the narrower and more clearly defined features and scope that was originally proposed.

[Fabrizio Giudici]

The implementation is separate, but the semantics are pretty similar. I
don't see how meaningful could be to have two separate @VerifyXXX
annotations for two separate sets of annotations.

[Lenny Primak]

I agree. It's all about the DRY principle for me.
Perhaps we,should just leave this whole deal to JEE.