Add possibility to add custom annotations to Builder

[lazystone]

In particular edu.umd.cs.findbugs.annotations.SuppressFBWarnings, otherweise SpotBugs detects violations which normally would be ignored in generated code.

It's not possible to make SpotBugs process @Generated annotation since SpotBugs process byte-code and @Generated has source retention type.

P.S. Nice library and nice work! I use Immutables a lot so it's really nice to have similar funtionality for java records.

[Randgalt]

Thanks!

How would you see this working? Would it be simpler to not add the @Generated annotations (or at least make it optional)?

[lazystone]

My guess that the easiest would be to pass annotations to the @RecordBuilder itself. Something like

@RecordBuilder(passAnnotations={SuppressFBWarnings.class}). At least Immutables has similar approach: https://github.com/immutables/immutables/blob/master/value-annotations/src/org/immutables/value/Value.java#L870

The plug&play approach would be to detect presence of edu.umd.cs.findbugs.annotations.SuppressFBWarnings class on classpath and add it auto-magically - that's how Immutables works. In my case it auto-magically adds

@org.immutables.value.Generated(from = "AbstractAxis", generator = "Immutables")
@javax.annotation.processing.Generated("org.immutables.processor.ProxyProcessor")
@java.lang.SuppressWarnings({"all"})
@edu.umd.cs.findbugs.annotations.SuppressFBWarnings

Immutables seems to support both ways which is nice, but sometimes too much magic is too much :)

[Randgalt]

too much magic

That's something I'd like to avoid. It would be nice to handle this in a way that if the Immutables library could implement it anyway they want now with Java 16 what would they do?

[Randgalt]

@lazystone any more interest in this or should I close this?

[lazystone]

Not at the moment - I switched back to Immutables. At the moment it can generate builders, so works for me. But thanks for alternative anyway!

[blalasaadri]

While I'm not the original poster, this would be interesting to us. The use case is slightly different though: I'd like to pass @Nonnull and @Nullable annotations from JSR 305. For example, I have the record:

@RecordBuilder
public record Address(
        @Nonnull String streetName,
        @Nullable String houseNumber,
        @Nonnull String city,
        @Nonnull String zipCode,
        @Nonnull String country
) implements AddressBuilder.With {
}

Currently, the following is generated:

/**
 * Set a new value for the {@code streetName} record component in the builder
 */
@Generated("io.soabase.recordbuilder.core.RecordBuilder")
public AddressBuilder streetName(String streetName) {
    this.streetName = streetName;
    return this;
}

/**
 * Return the current value for the {@code streetName} record component in the builder
 */
@Generated("io.soabase.recordbuilder.core.RecordBuilder")
public String streetName() {
    return streetName;
}

/**
 * Set a new value for the {@code houseNumber} record component in the builder
 */
@Generated("io.soabase.recordbuilder.core.RecordBuilder")
public AddressBuilder houseNumber(String houseNumber) {
    this.houseNumber = houseNumber;
    return this;
}

/**
 * Return the current value for the {@code houseNumber} record component in the builder
 */
@Generated("io.soabase.recordbuilder.core.RecordBuilder")
public String houseNumber() {
    return houseNumber;
}

// ...

What I would want is something like this though:

/**
 * Set a new value for the {@code streetName} record component in the builder
 */
@Generated("io.soabase.recordbuilder.core.RecordBuilder")
public AddressBuilder streetName(@Nonnull String streetName) { // <- @Nonnull as a parameter annotation
    this.streetName = streetName;
    return this;
}

/**
 * Return the current value for the {@code streetName} record component in the builder
 */
@Generated("io.soabase.recordbuilder.core.RecordBuilder")
@Nonnull // <- @Nonnull as a method annotation
public String streetName() {
    return streetName;
}

/**
 * Set a new value for the {@code houseNumber} record component in the builder
 */
@Generated("io.soabase.recordbuilder.core.RecordBuilder")
public AddressBuilder houseNumber(@Nullable String houseNumber) { // <- @Nullable as a parameter annotation
    this.houseNumber = houseNumber;
    return this;
}

/**
 * Return the current value for the {@code houseNumber} record component in the builder
 */
@Generated("io.soabase.recordbuilder.core.RecordBuilder")
@Nullable // <- @Nullable as a method annotation
public String houseNumber() {
    return houseNumber;
}

Note also, that there are of course a few different implementations of @Nonnull / @NonNull / @NotNull so a generic solution in which field, parameter and method annotations are / can be passed on is probably the easiest solution right now.

A check in the setters to ensure that values aren't null when annotated as such would also be nice, but that has a much lower priority (and would involve much more magic, given the various annotations).

[Randgalt]

I know why the Immutables library needs to copy down these annotations - their builder is used by Jackson and other libraries to construct the ultimate object. But, in this case, Jackson will use the Record directly. So, is it necessary to duplicate the annotations on the builder? When/where would they be used?

[blalasaadri]

For these annotations specifically, IDEs will warn developers if they pass null to a setter where the parameter is annotated with @Nonnull. So with the example above, if the annotations were copied I would get the following from my IDE (IntelliJ IDEA):

Address.builder()
        .country(null) // <- Warning: Passing 'null' argument to parameter annotated as @NotNull
        .build()

This can be VERY useful to identify whether you're using a record as it is intended. It can also be reported during the build process (and potentially force a build to fail) with plugins such as SpotBugs:

[INFO] --- spotbugs-maven-plugin:4.2.0:check (default-cli) @ my-project ---
[INFO] BugInstance size is 1
[INFO] Error size is 0
[INFO] Total bugs: 1
[ERROR] High: Null passed for non-null parameter of my.package.Address$AddressBuilder.country(String) in my.package.App.createAddress() [my.package.App] At App.java:[line 54] NP_NONNULL_PARAM_VIOLATION

[Randgalt]

I see. So, what about always copying record component annotations down to the builder? That seems simple enough.

[Randgalt]

Note - see the discussion here: https://github.com/Randgalt/record-builder/discussions/32

[Randgalt]

I have a proposal. RecordBuilder will add 3 new options:

Option	Default	Behavior
inheritComponentAnnotations	true	Any annotations on record components are copied to the corresponding builder method and the builder static constructor
interpretNullableValidations	false	Behave in a manner similar to the Immutables library where any annotation named some form of nullable makes that component optional. When the record is built an exception is thrown for non-optional components that haven't been specified. Additionally, as discussed here an additional builder constructor is created with only non-optional compontents.
applyValidator	false	Pass created record instances through the JSR-303 validator
addSuppressWarningsAnnotations	false	Try to emulate Immutables library's behavior with the various warning suppression annotations

Note: the validation options will default to false as I really want to keep this library simple. Any behavior not expected in a future version of Java should be opt-in IMO.

Thoughts?

[aowss]

in my opinion, you could just keep things simple and copy all annotations as you mentioned earlier.

I don't think you should act on them: that's not really the job of this library. This library's goal is to add a builder pattern to the Java record construct, IMHO.

Regarding the annotations, if the original record was acting on them, then this will still happen. if not, then how are you going to handle those annotations. for JSR-303 for example, are you going to throw an exception ?

[blalasaadri]

I see. So, what about always copying record component annotations down to the builder? That seems simple enough.

Close, but that won't always work - it'll have to check, whether the field annotations are valid for parameters as well. So, effectively it would be:

1. Check, whether the annotation to be copied itself has the @Target annotation.
2. If not, copy it to setters and getters.
3. If it does, check whether PARAMETER is a valid target. If so, copy it to setters.
4. If it does, check whether METHOD is a valid target. If so, copy it to the getters.

I have a proposal. RecordBuilder will add 3 new options:

Option	Default	Behavior
inheritComponentAnnotations	true	Any annotations on record components are copied to the corresponding builder method and the builder static constructor
interpretNullableValidations	false	Behave in a manner similar to the Immutables library where any annotation named some form of nullable makes that component optional. When the record is built an exception is thrown for non-optional components that haven't been specified. Additionally, as discussed here an additional builder constructor is created with only non-optional compontents.
applyValidator	false	Pass created record instances through the JSR-303 validator
addSuppressWarningsAnnotations	false	Try to emulate Immutables library's behavior with the various warning suppression annotations

Note: the validation options will default to false as I really want to keep this library simple. Any behavior not expected in a future version of Java should be opt-in IMO.

Thoughts?

I like the inheritComponentAnnotations annotation, since that is something people may want to disable. The others however... I do like the Immutables library, but I feel like this lib has a much smaller scope and I wouldn't try to change that. Users can always build custom constructors in records to check for null values or JSR 303 compatibility for example.

[Randgalt]

Thanks for the feedback everyone. So, at minimum copy the annotations (respecting Target, etc.). We can decide later if we can/should do more.

[Randgalt]

I did some testing and found some interesting things. Given something like this:

record Foo(@Ann String bar){}

That @Ann annotation gets applied in two places. First, the record class's constructor gets it. But, also the "accessor" gets it (that's the generated getter method: String bar()). So, I imagine the accessor version could be used for RecordBuilder's getter and the constructor version could be used for RecordBuilder's setter. It's not exactly obvious how to find the default constructor though - I guess it's the one where the arguments match the component list (update: it's described here https://github.com/openjdk/jdk/pull/3556/files#diff-a6270f4b50989abe733607c69038b2036306d13f77276af005d023b7fc57f1a2R2368)

[Randgalt]

Here's a PR that implements what seems to me to be the correct support. I'd appreciate testing and comments: https://github.com/Randgalt/record-builder/pull/33

[Randgalt]

@blalasaadri or @aowss any chance you can review the PR?

[blalasaadri]

@blalasaadri or @aowss any chance you can review the PR?

There, finally got around to it. 🙂