[BUG] Lombok incorrectly inherits constraints on setter

[Hayo-Baan]

Describe the bug
The latest Lombok incorrectly inherits field constraints in the setter (and getter). The setter inherits the constraints even though these are limited to validation groups that are not always active.

To Reproduce

public interface BaseEntityDto {

    /**
     * Returns the ID of the entity.
     *
     * @return the ID of the entity.
     */
    Long getId();

    /**
     * Sets the ID of the entity.
     *
     * @param id the ID of the entity.
     */
    void setId(Long id);

    /**
     * Returns the timestamp of the last update of the entity.
     *
     * @return the timestamp of the last update.
     */
    Instant getUpdateTimestamp();

    /**
     * Sets the timestamp of the last update of the entity.
     *
     * @param updateTimestamp the timestamp of the last update.
     */
    void setUpdateTimestamp(Instant updateTimestamp);

    /**
     * Returns the version of the entity.
     *
     * @return the version of the entity.
     */
    Long getVersion();

    /**
     * Sets the version of the entity.
     *
     * @param version the version of the entity.
     */
    void setVersion(Long version);

}

@Data
public class AccountDto implements BaseEntityDto {

    /**
     * Interface for annotating the validation group for account creation.
     */
    public interface CreateValidation {}

    @Null(groups = CreateValidation.class)
    @NotNull
    @Positive
    private Long id;

    @NotBlank(groups = {Default.class, CreateValidation.class})
    private String name;

    private Instant updateTimestamp;

    @NotNull
    @Null(groups = CreateValidation.class)
    private Long version;
}

Here e.g. the ID setter of account dto constrains the value to non-null, but that is a perfectly valid value when validating under CreateValidation.

Expected behavior
The constraints should not be added.

Version info (please complete the following information):

• Version used in Spring Boot 2.6.7 (version in Spring Boot 2.6.6. works fine)
• Platform Java

[rzwitserloot]

@HayoBaanAtHand You messed up the form I think. I'll give you an hour to correct it.

Or perhaps you thought the title is enough of a bug report and you decided that reading the template and doing what it says was far too much effort. In which case, oof. That's rude.

[Hayo-Baan]

Yeah, sorry, accidentally pressed enter which saved the form I was editing. Completed it now..

[Hayo-Baan]

Note: the getters also incorrectly inherit the constraints.
This might actually be is related to issue #3166 and perhaps #3150

[Hayo-Baan]

Thinking about this some more, I actually think you should not (at least not by default) inherit the annotations, not only because of above use case, but more in general: the validation is for the end-state of the object, this does not necessarily say anything about the intermediate states. So the individual setters (and getters) should not be validated (again) as well.
There are use case where inheriting the annotations may be desired, of course, so adding an option that would do so would be nice still, just not by default and in general.

[rzwitserloot]

In all the hullabaloo I misunderstood a comment. I thought us copying validation annotations has been part of lombok for quite a while already. I conflated the date of a PR with '... and it has been in lombok since that date' (whereas actually we accepted that PR only relatively recently).

In other words, I wasn't aware that 1.18.24 is the version that introduces this change.

Given the complexity / confused takes on what validation annotations even mean, "we wash our hands and do not do anything - we do not copy them, we do not add nullchecks" is probably better.

I'll talk to @rspilker about doing a quick 1.18.26 release to reduce the chance folks are going to start relying on the current copy behaviour. And I need to discuss this as a principle too, we may have to change a few things.

I'm going to make this the umbrella issue. The others are hopelessly bogged down and/or put the focus on an undesired way of addressing this problem.

[kflorian]

Any timeline for a "quick 1.18.26", as mentioned above?

[rPraml]

For search engine: Hibernate throws

javax.validation.ConstraintDeclarationException: HV000151:
A method overriding another method must not redefine the parameter constraint configuration,
but method MyClass#setFoo(String) redefines the configuration of MyInterface#setFoo(String).

example code:

public interface MyInterface {
    void setFoo(String foo);
}

public class MyClass implements MyInterface {
    @Setter
    @javax.validation.constraints.NotNull
    private String foo;
}

As workaround, you may need to change the interface to void setFoo(@NotNull String foo) which is not always possible.
Waiting for 1.18.26...

[gitmotte]

Why copy annotations automatically without users knowlegde ... as far i've seen there is a solution for this already if someone likes to copy it ...

@Getter(onMethod_ = { @NotNull })
@Setter(onParam_ = { @NotNull })

I don't think that Validation-Annotations should be copied at all - neither @nonnull nor other validations from the javax.validation namespace. The specification is clear and unambiguous - invalid data may be included in the bean - but not persisted by the JPA framework.

Apart from that, it is also possible to implement user-specific validations or to use an alternative extending validation framework, which would then not be copied as well.

I have to go back to version 1.8.22 until the change is rolled back!

[rzwitserloot]

Why copy annotations automatically without users knowlegde

Because of the concept of type-associated annotations vs. field-associated ones.

The problem is, when annotations launched (softlaunch in JDK5, proper launch in JDK6), annotations were things you could stick on fields, methods, parameters, and types. Almost immediately people decided to use the feature to annotate type usages. The point of the annotation was that it was a modifier of the type, e.g. if the annotation was on a method, that really meant it was intended to be modifying the nature of the return type of that method.

In a later release (I believe JDK8), this was enshrined by way of now allowing it everywhere, (i.e. the TYPE_USE target): Now, starting with jdk8, you can e.g. write List<@NonNull String>, which wasn't legal before.

The problem is, we still have a ton of legacy cruft:

• Annotations where the authors know perfectly well that it's properly targeted as TYPE_USE, but they launched them before TYPE_USE was out and do not consider it worth the backwards breakage to change them.
• Authors who don't understand the difference, even to this day,

and as a final kick in the behind, lombok's own limitations:

• Lombok can't tell - we need resolution to know what the targets of an annotation are, and the JDK annotation spec is intentionally unclear about what an annotation means if you don't have the annotation definition: @Foo String bar; can mean one of 2 completely different things (@Foo is an annotation on the type String, or @Foo is an annotation on the field declaration bar). There's no way to know without checking the @Target annotation on the definition of @Foo.

For TYPE_USE annotations, it's modifying the type, therefore it stands to reason that if we say we copy the type to e.g. the getter's return value, we copy that annotation just the same. But, when the annotation is supposed to apply to the field, it would be much more of a stretch if we copy them.

Point is: It is impossible to use the guidance of 'if on type, copy it, if on field, don't' for many reasons, but at the same time the semantic meaning of annotations means a blanket 'never ever copy it' is also entirely wrong and it's not a fair solution to require from any lombok user to re-list all annotations in quadruplicate, telling lombok separately to copy them to the getter, the setter, the wither, the builder 'setter', the constructor parameter, etc.

So, we decided to auto-copy them.

Now, message is clear, this wasn't the right choice either. We've got alternate plans, which primarily involve:

• If it's type-level nullity associated, we know about it and copy appropriately and silently.
• Otherwise, if we recognize it in our list of known annotations, and that list says that copying MIGHT be warranted, we will generate a warning and demand that you state what you want us to do in your lombok.config. You can then choose to either have us apply the logical behaviour (presumably, copy it to various places), or not to (and once you have that setting, we stop generating a warning for it).
• It's backwards incompatible once, for a small set of annotations, namely the ones that aren't nullity related that we copy now. But they'd result in warnings that explain what you have to do, so that's not too bad.

I have to go back to version 1.8.22 until the change is rolled back!

That is correct.

I don't think that Validation-Annotations should be copied at all

Note that this was a contribution. We didn't spend the appropriate time to evaluate it (a mistake we rarely make, almost uniformly the complaint is that we don't accept enough PRs, not that we accept them too cavalierly!). We apologize for this.

[Hayo-Baan]

Any news on a fix release for this?

[Indifer]

@HayoBaanAtHand You can go back to 1.8.22 first

[Hayo-Baan]

@HayoBaanAtHand You can go back to 1.8.22 first

Sure, but I'd rather have prevented that as it means I have to add a version to the lombok dependency instead of being able to simply inherit the version as used by Spring Boot. Since I maintain 20+ microservices, I'd rather have prevented this and simply wait until this was fixed (and the fix assimilated in a Spring Boot update).

[kflorian]

You can go back to 1.8.22 first

No, I can't, because 1.8.22 doesn't support JDK 18.

[mjaggard]

lombok.copyableAnnotations -= javax.validation.constraints.NotNull
I've done this ^ which seems to have solved the problem for me (although I'm not totally clear whether that's the correct syntax or the problem has been solved by something else)

[mjaggard]

Looks that that syntax is correct (although I can't see it documented anywhere)
https://github.com/projectlombok/lombok/blob/master/src/core/lombok/core/configuration/ConfigurationParser.java#L101

[mjaggard]

Sorry, my mistake I'd removed the annotation from the field - it doesn't work.

[lildadou]

Indeed, it doesn't work

[lildadou]

I encountered a problem with input validation in my Spring REST controllers. As the getters also have the @NotNull annotation then they are also checked during the object mapping. As a result, I get 2 constraint violations (one for the attribute and one for the method) and 2 strictly identical error messages are returned to my users.

[mjaggard]

More annoying than the bug in Lombok here is the fact that JPA doesn't check this until you actually use an entity - it's just asking for a bug in production 🤦‍♂️

[jamfor352]

Are there any updates on this?

[mjaggard]

@rzwitserloot do you think we can have a release version that contains this change? I see the v1.18.24 tag still has the incorrect copyable annotation, as does the source in my maven repo.

[dstango]

Wondering if this issue can be closed due to 1473389 ...

[Hayo-Baan]

Certainly looks like it. If I read the comments in the change correctly, all validation annotations are now excluded. And this is indeed the correct approach here.

I don't see 1.18.26 as available yet, so I can't test to confirm, but will do as soon as it is and report back here.

[rfelgent]

@Hayo-Baan v1.18.26 is available since 3rd February 23