FindBugs detecter for NonNull Lombok builder attributes

Question

I have a lot of classes with @NonNull fields using Lombok builders.

@Builder
class SomeObject {
    @NonNull String mandatoryField1;
    @NonNull String mandatoryField2;
    Integer optionalField;
    ...
}

However, this gives the caller the option to create the object without setting a mandatoryField, which when used, would lead to a runtime failure.

SomeObject.builder()
          .mandatoryField1("...")
          // Not setting mandatoryField2
          .build();

I'm looking for ways to catch these errors at build-time.

There are non-Lombok ways like StepBuilders or even a constructor to ensure mandatory fields are always set, but I'm interested in ways to achieve this using the Lombok builder.

Additionally, I understand that designing classes (like a step-builder, or an @AllArgsConstructor) in order to have compile-time checks would create a lot of clumsy code - which is why I'm motivated to build a post-compile FindBugs step that detects these.

Now, FindBugs does fail when I explicitly set a @NonNull field to null:

FindBugs detects this failure,

new SomeObject().setMandatoryField1(null);

but it doesn't detect this:

SomeObject.builder()
          .mandatoryField1(null)
          .build();

Nor does it detect this:

SomeObject.builder()
          .mandatoryField1("...")
          //.mandatoryField2("...") Not setting it at all.
          .build();

This seems to be happening because the Delomboked builder looks something like,

public static class SomeObjectBuilder {
    private String mandatoryField1;
    private String mandatoryField2;
    private Integer optionalField;

    SomeObjectBuilder() {}

    public SomeObjectBuilder mandatoryField1(final String mandatoryField1) {
        this.mandatoryField1 = mandatoryField1;
        return this;
    }

    // ... other chained setters.

    public SomeObject build() {
        return new SomeObject(mandatoryField1, mandatoryField2, optionalField);
    }
}

I observe that:

• Lombok doesn't add any @NonNull to its internal fields, nor does it add any null-checks to the non-null fields.
• It doesn't call any SomeObject.set* methods, for FindBugs to catch these failures.

I have the following questions:

• Is there any way to use Lombok builders in a way that causes build-time failures (while running FindBugs, or otherwise), if @NonNull attributes are set?
• Is there any custom FindBugs detector that detects these failures?

Derryberry

Do you really need compile time checks? Is that code not covered by unit tests? – Dally

No, not all code is. Mere ensuring the high coverage required to check this flow is not a realistic task for me. Plus there's risks of developers missing this code-flow during their tests. The point of this FindBugs check is to fail the compilation before those tests, perhaps when someone is in the process of writing the code itself, and for all the objects. – Derryberry

Would this proposal help you? I guess, having only a couple of mandatory fields is pretty common. – Woodrowwoodruff

@Woodrowwoodruff Yes, this is exactly I'm looking for! Although my question asks for a FindBugs detecter, my motivation for adding that detecter was a lack of this feature in Lombok. I too, just want that somebody explicitly called .mandatoryField1(null) and not simply forget this attribute. I understand the concerns mentioned here, and I would want the option to have that readability vs. have compile-time hints on mandatory fields. – Derryberry

@JohnBupit, I think they might have fixed this in 1.18.4 github.com/rzwitserloot/lombok/issues/1634 – Fraise

Answer 1

Lombok takes these @NonNull annotations into account when generating @AllArgsConstructor. This also holds for the constructor that is generated by @Builder. This is the delomboked code of the constructor in your example:

SomeObject(@NonNull final String mandatoryField1, @NonNull final String mandatoryField2, final Integer optionalField) {
    if (mandatoryField1 == null) {
        throw new java.lang.NullPointerException("mandatoryField1 is marked @NonNull but is null");
    }
    if (mandatoryField2 == null) {
        throw new java.lang.NullPointerException("mandatoryField2 is marked @NonNull but is null");
    }
    this.mandatoryField1 = mandatoryField1;
    this.mandatoryField2 = mandatoryField2;
    this.optionalField = optionalField;
}

Thus, FindBugs could in theory find the problem, because the null check is present in the constructor, which is called later with a null value in your example. However, FindBugs is probably not powerful enough to do so (yet?), and I don't know of any custom detector that is capable of that.

The questions remains why lombok doesn't add those checks to the builder's setter methods (which would make it easier for FindBugs to spot the problem). This is because it is perfectly legit to work with a builder instance that still has @NonNull fields set to null. Consider the following use case:

You may, e.g., create a new builder from an instance using the toBuilder() method, and then remove one of its mandatory fields by calling mandatoryField1(null) (maybe because you want to avoid leaking an instance value). Then you could pass it to some other method to let it re-fill the mandatory field. Thus, lombok does not and should not add those null checks to the different setter methods of the generated builder. (Of course, lombok could be extended such that users could "opt-in" to generating more null checks; see this discussion at GitHub. However, that decision is up to the lombok maintainers.)

TLDR: The problem could be found theoretically, but FindBugs is not powerful enough. On the other hand, lombok should not add further null checks, because it would break legitimate use cases.

Artamas

It is perfectly fine for Lombok to remain as-is. I am simply looking for a compile-time (or post-compile for that matter) warning, instead of failing with an NPE. Moreover, in the use-case you mention, is mandatoryField1 really @NonNull then? – Derryberry

IMHO, a @NonNull on a class variable should not say something about the builder (at least not by default), because the builder is just representing an intermediate, preliminary state of the future object to be constructed, not the object itself. Therefore: Yes, mandatoryField1 is still @NonNull. Concerning extending lombok: I think (optionally) generating further null checks combined with a builder() method that takes parameters would be a nice addition. – Artamas

I would disagree. IMO, if a field is @NonNull, then it is mustn't be null - in an intermediate state or otherwise. I believe, there shouldn't be any such intermediate state (... or at least that's what I would want to ensure in my code). – Derryberry

But there will never be an instance with that field == null, so it is @NonNull. It can only be null in the builder. One of the core ideas of the builder pattern is that it provides control over the steps of the construction process. So most users of this pattern want step-wise construction, an therefore they also kind of expect having temporary constraint violations in the builder. Thus, lombok should not add further restrictions by default, IMHO. If you want to ensure constraints even in the builder, then the proposed lombok extension would be a solution. – Artamas

Answer 2

it might seem like a nit pick...

... but please keep in mind that none of these:

• Findbugs
• Bean Validation (JSR303)
• Bean Validation 2.0 (JSR380)

happen at compile time, which very much matters in this discussion.

Bean Validation happens at runtime and as such requires either explicit invocation in the code or the managed environment implicitly does it (like Spring or JavaEE) by creating and invoking validators.

FindBugs is a static bytecode analyser and therefore happens post-compilation. It uses clever heuristics, but it does not execute the code, and therefore is not 100% watertight. In your case it followed nullability check only in shallow case and missed the builder.

Please also note that by manually creating the builder and adding necessary @NotNull annotations, FindBugs would not kick in, if you did not assign any value, as oppose to assigning null. Another gap is reflection and deserialisation.

I understand that you would like the contract expressed in validation annotations (like @NotNull) to be verified as soon as possible.

There is a way to do it on SomeClassBuilder.build() (still runtime!), but it's a bit involved and requires creating custom builder:

^{perhaps it could be made generic to accommodate many classes - somoeone please edit!}

@Builder
class SomeObject {
  @NonNull String mandatoryField1;
  @NonNull String mandatoryField2;
  Integer optionalField;
  ...

  public static SomeObjectBuilder builder() { //class name convention by Lombok
    return new CustomBuilder();
  }

  public static class CustomBuilder extends SomeObjectBuilder {
    private static ValidationFactory vf = Validation.buildDefaultValidationFactory();
    private Validator validator = vf.getValidator();

    @Overrride
    public SomeObject build() {
      SomeObject result = super.build();
      validateObject(result);
      return result;
    }

    private void validateObject(Object object) {
      //if object is null throw new IllegalArgException or ValidationException
      Set<ConstraintVioletion<Object>> violations = validator.validate(object);

      if (violations.size() > 0) { 
        //iterate through violations and each one has getMessage(), getPropertyPath() 
        // - to build up detailed exception message listing all violations
        [...]
        throw new ValidationException(messageWithAllViolations) }

    }        
}

Penang

Thanks for the input. However, my code already fails with an NPE (due to @NonNull) when .build() executes. The only benefit with this CustomBuilder seems to be adding a checked ValidationException, which, perhaps, fails the compilation if not properly handled. – Derryberry

nice tip about @NonNull! – Penang