[BUG] Validation Annotation on Setter Method after upgrading from 1.18.22 to edge Version

[c-koell]

Describe the bug
I'm not clear if it is a bug but it can break existing code.

To Reproduce
Lets look at following example.

import javax.validation.constraints.NotNull;
import lombok.Setter;

@Setter
public class A implements B {
	@NotNull
	private String value;
}

public interface B {

	public void setValue(String val);
}

With lombok 1.18.22 the generated Setter looks like

a.setValue(String value)

but with the current lombok edge it looks like

a.setValue(@NotNull String value)

Expected behavior
No change on generated Methods

Version info (please complete the following information):

• Lombok 1.18.22 or edge
• Eclipse 2022.03

Additional context
If Class A is a JPA Entity Bean you will see following Exception if Lombok creates the Setter with the Validation Annotation.

[EXCEPTION] class javax.validation.ConstraintDeclarationException - HV000151: A method overriding another method must not redefine the parameter constraint configuration, but method A#setValue(String) redefines the configuration of B#setValue(String). 
    at org.hibernate.validator.internal.metadata.aggregated.rule.OverridingMethodMustNotAlterParameterConstraints.apply(OverridingMethodMustNotAlterParameterConstraints.java:24) ~[com.ibm.ws.org.hibernate.validator_1.0.62.jar:6.1.7.Final]

[rzwitserloot]

It is a change, yes, but I think a welcome one - you have a fundamental problem here. The interface you inherit from doesn't treat it as non-null, therefore, callers are told that they can pass null to this method. However, the rest of your code clearly indicates this makes no sense.

The specific solution is to write your own setter which explicitly nullchecks - it is no longer simple boilerplate.

It would be kinda neat if there was a way to let lombok bridge the gap here (hey, lombok could generate that nullcheck), but I see no understandable way to accomplish this short of dipping into looneybin territory: @Setter @NonNull @DoNotCopyNullityAnnotationsAndNullCheckInstead.

TL;DR: Lombok's update uncovered a bug in your code. Lombok policy is that we will freely break backwards compatibility if the code we break was already broken anyway.

[c-koell]

Hi @rzwitserloot !

Thank you for your quick response. As i have written i'm not sure if it is a bug or a feature :-) I agree with you, theoretically your arguments are right.

But if you read the specification of the hibernate validator which evaluates the annotation you will see that it is possible to use either field level constraints or property level constraints
https://docs.jboss.org/hibernate/stable/validator/reference/en-US/html_single/#section-declaring-bean-constraints

Normally we are using field level constraints on jpa entities. The specification say also following:

It is recommended to stick either to field or property annotations within one class. It is not recommended to annotate a field and the accompanying getter method as this would cause the field to be validated twice.

And one hint more says following:

The property’s getter method has to be annotated, not its setter. That way also read-only properties can be constrained which have no setter method.

In our case the next problem is that we use a Interface to define a API for some JPA Entities. The generated code breaks the Constraint HV000151 from Hibernate which says:

A method overriding another method must not redefine the parameter constraint configuration

So i think the feature of lombok to copy Constraint Annotations makes no sense if you use Hibernate.

Is it possible to deactivate that feature ?
Or do you also think now that it could be problematic to copy the Annotation, based on my arguments ?

thanks
claus

[rzwitserloot]

So i think the feature of lombok to copy Constraint Annotations makes no sense if you use Hibernate.

I'm all in favour of a unified, official notion of exactly how simple java POJOs with obvious annotations are supposed to be transformed into JPA-ready types instead. However, every time I delve into this (note that I don't personally use hibernate nor any other JPA implementation or JPA-esque project, nor does @rspilker ), it seems like everybody, notably including the hibernate authors, are hopelessly confused and completely clueless that there's even an issue here in the first place. There where the official docs appear to make a suggestion (which is: The class is a representation of a database row first, and not really a representation of the thing that DB row represents, i.e. equality is defined by 'does it have the same unid or not' and not 'does it have the same extrinsic properties or not) - most of the users of hibernate do it the other way.

Given that it's this confusing, and then no core contributor I know of either wants to contribute on this, or has the requisite experience to try to come up with something - we cannot add this.

The route forward is to promise to maintain this, or at least to write a treatise on all these details, which covers how hibernate actually functions, what hibernate wants you to do, how it operates best, and what the alternatives are and why you shouldn't do it that way - and it needs to be a consistent whole that we as non-hibernate users can follow.

And even then this won't be going anywhere until we're completely convinced the document is correct. This is probably impossible unless you get the core hibernate dev team to sign off on it, in which case we'll jump right in and get this feature rolled out ASAP. After all, now I can insta-deny any open feature requests or tickets that want to do it another way (because I know we'll be getting a boatload of em), and simply answer: "The core authors of hibernate say you're wrong and you should be doing it our way instead, so, no, we won't implement what you want / you are doing it wrong". Without such absolutist approaches to ticket management any attempt to try to cater to hibernate needs is going to result in a combinatorial explosion of options which we don't want for multiple reasons, or in endless debate smeared out over lots of tickets.

When/if thinking about how it should all work, something like @lombok.extern.jpa.JpaValue or something similar is one way we can get there, and probably the preferred way: The extern package exists precisely for this purpose: It is a place where we can put lombok features that generate code that makes absolutely no sense at all unless you place it in context of some external project (such as hibernate or JPA - the package may have to be extern.hibernate. instead), and then it makes perfect sense. You get a lot of leeway once you're in an extern package, if we're sure that the code we generate is clearly how the project we're generating it for wants you to do it.

I'm going to close this for now; any such documentation should probably show up as a new feature. Probably not worth making until you've got that document along with enough 'proof of buy-in' from the community that we can make a reasonable assumption it covers all defensible ways of using JPA: From the core hibernate or JPA team, or perhaps from multiple well known authors in the space, something we can easily confirm represents informed opinion by someone(s) well versed in all the many ways hibernate and JPA are used.

[c-koell]

Hi @rzwitserloot . I understand your discontent here but you must also see my point of view. I'm no JPA Committer nor a Hibernate Validation Committer. I use it as many other out there .. thats all and we have to live with it ;-)
We have to maintain about 60 different projects that all use lombok over years with happiness !

The new copy annotations feature will break a lot of valid code ! This i s a no go for us. A lot of hard coded annotations will not impact any of running code, but the javax.validation.constraints.NotNull annotation will change the runtime behavoiur of valid code.

So please is it possible to exclude the Annotation ? or can we exclude it manually ? The worst case for us is to fork your code and exclude it by ourselve.

Thanks for any help !

[Rawi01]

The copy feature is not new, it is there for years and it is really usefull in some situations. PR #2904 added javax.validation.constraints.NotNull to the BASE_COPYABLE_ANNOTATIONS list which leads to this issue.

@rzwitserloot Unlike most null annotations javax.validation.constraints.NotNull does not indicate that it is not allowed to assign a null value. It is a marker annotation that gets used if you trigger bean validation for the field. This is the reason why it is not excluded from the NONNULL_ANNOTATIONS list.

lombok/src/core/lombok/core/handlers/HandlerUtil.java

Line 102 in 2c892a4

// "javax.validation.constraints.NotNull", // The field might contain a null value until it is persisted.

I think we should sort out what exactly is a 'copyable' annotation that should be part of BASE_COPYABLE_ANNOTATIONS.

[rzwitserloot]

@c-koell wrote:

I understand your discontent

discontent? I have no idea where you are getting that from. I've merely set out why we cannot do what you want - we can't just take random internet user's word for it, as we've heard of conflicting reports about exactly how JPA annotations are meant to be used, and the core maintenance team lacks the JPA experience requires for us to ascertain the appropriate answer ourselves.

Perhaps you're only thinking of your own needs and this nuance isn't coming across; you just interpret it as us hating on your needs. That's, I assure you, not at all what's happening here.

but you must also see my point of view.

That makes no sense. We do not have any contract stipulating such a thing. It appears you wish for lombok to act differently than it is, which means you'd have to convince a core maintainer of your plight. In other words, you must see my point of view. Your attitude is coming across as entitled.

I'm no JPA Committer nor a Hibernate Validation Committer.

Yes, I understand this. However, you can do two things:

• Write a PR for lombok that makes it easy for those committers to make a very simple PR, or at least some sort of form that lets the JPA committers describe precisely how one should copy the various JPA annotations in existence.
• Ask the JPA committers to then use this feature. If they don't want to, then perhaps you can think of a creative way to convince us you are nevertheless correct. Perhaps get a famous JPA book author on board, for example.

It's quite a bit of work, but you can do these things. If you don't want to, well, I don't want to either. You understand the dilemma, now, surely.

I use it as many other out there

Well, that's great! Perhaps if you do not want to do the above work and lack the skills to do this, if there are many others in the same boat, perhaps you can find someone that can!

The new copy annotations feature will break a lot of valid code !

That's not good. Can you show me a specific example, and add to this sufficient, verifiable proof that you're actually correct (because I've read so many conflicting views of how one should use JPA annotations, I can't just take some random internet user's word for it, surely you understand this. If not, well, your words: You must also see my point of view, or this isn't going to go anywhere).

[rzwitserloot]

@Rawi01 wrote:

The copy feature is not new, it is there for years and it is really usefull in some situations. PR #2904 added javax.validation.constraints.NotNull to the BASE_COPYABLE_ANNOTATIONS list which leads to this issue.

Do you think it should be kicked from the BASE_COPYABLE_ANNOTATIONS list? Semantically, one would think a validation constraint on a field should therefore also exist on the setter parameter, as well as a getter (if a validation service calls the getters, it would want to state that if a @NotNull annotated getter returns null, that the object is not in a valid state, right?)

So semantically this seems right to me. Maybe I misunderstand the semantics of javax.validation.constraints.NotNull.

[c-koell]

@rzwitserloot thanks for your feedback.

As written before i don't know what you need more as the official specification from hibernate itself ?
Under https://docs.jboss.org/hibernate/stable/validator/reference/en-US/html_single/#section-declaring-bean-constraints
you find following

It is recommended to stick either to field or property annotations within one class. It is not recommended to annotate a field and the accompanying getter method as this would cause the field to be validated twice

Can you show me a specific example, and add to this sufficient, verifiable proof that you're actually correct

Of course .... we get following exception on working code if we switch to a newer lombok version

javax.validation.ConstraintDeclarationException - HV000151

as we use a base interface for a lot of entity beans. If the implementation now adds a Annotation on the Setter Method the code will break.

I see it like described here
https://stackoverflow.com/questions/4963300/which-notnull-java-annotation-should-i-use

-> javax.validation.constraints.NotNull
Created for runtime validation, not static analysis.

On the other hand as example
-> javax.annotation.Nonnull
are compile time hints and will be used from Findbugs or something similar and has no impact on the runtime behavior.

And for compile time annotations i see it like @rzwitserloot .. semantically it must/should be on the setter and also on the member field. That's okay...

After searching around i found in the official Beanvalidation Specification following hint !
https://beanvalidation.org/1.1/spec/#constraintsdefinitionimplementation

4.1.2. Field and property validation
Constraint declarations can be applied on both fields and properties for the same object type. The same constraint should however not be duplicated between a field and its associated property (the constraint validation would be applied twice). It is recommended for objects holding constraint declarations to adhere to a single state access strategy (either annotated fields or properties).

I can't just take some random internet user's word for it, surely you understand this

I hope you see now my concerns come not only from a random internet user ;-)

@Rawi01 i hope you see it like i do. The Annotations from javax.validation.constraints are described in the beans validation specification and should not be duplicated between a field and its associated property

[rzwitserloot]

@c-koell You show me documentation that says 'if you do this, the problem is that validation occurs twice', and then you tell me that an exception occurs, which is inconsistent with what the documentation says is going to happen.

The bean spec is not a clear document and not followed by most tools that say they do, but you couldn't know that. It does show that 'here, I will quote from official docs' is not usually good enough.

Note that if one labours under the model that validation annotations should be either on the field or on the getter, that I bet most want it on the getter, i.e. that lombok should remove that annotation from the field, which gets us back to the whole 'there are a million different ways to define copy behaviour', and "just let me write in lombok.config a list of annotation types that should not be copied" or "just take validation.NotNull off of the standard copy list" both are incorrect solutions to this problem. The 'correct' solution would be to either just let it go if actual real world use of this annotation is all over the place, or to "move" the annotation to the getter (and setter, presumably). Which makes this a very large feature request that we're not going to get to anytime soon.

[dstango]

@c-koell : Just a thought about the original request (not considering any potential "double check") -- wouldn't the solution be to amend the interface B, so the @NotNull-annotation would be present in the interface? It actually feels more in line with the contract an interface is providing. If you add the requirement in "through the back-door" of solely declaring it on the field, such a @NotNull-constraint actually would break the contract of the interface IMHO.

[rzwitserloot]

@dstango Yup. The problem here is that c-koell had a bug in their code, but wasn't aware of this. A lombok update made it easier to find, and c-koell's original request was evidently to shoot the messenger instead of fixing things.

I'm going to tune this issue out for a while, it doesn't feel like it's going anywhere useful right now.

[c-koell]

@rzwitserloot as a user that has nothing to do with jpa and hibernate you think you know what others should do ? :-)

The bean spec is not a clear document and not followed by most tools that say they do, but you couldn't know that. It does show that 'here, I will quote from official docs' is not usually good enough.

... sorry but thats really funny ...

@dstango i repeat myself but in jpa and with validation annotations you should use them either on field or on property level not both ! So we have not defined it on the interface because normally we define it on field level.

But i must accept that a normal discussion is not easy here .. unfortunately

[dstango]

@c-koell I understand you strive to have consistency with defining it on the field or method level, yet lombok doesn't support this scenario, unless you manually define the setter, which results in manual coding of boilerplate.
Yet I can also see the point of duplicating the annotation, as tools might depend on the annotations of the setters/getters only, despite the bean spec, so lombok's behavior makes field/method annotations consistent.
I don't know if any performance penalties actually incur in practice, yet I'd guess for null checks that's negligible.
The real trouble is with the interface -- and in this case I'd say a subtype should rather not narrow a contract. Otherwise you'll not be able to trust the contract, like with UnsupportedOperationExceptions in the collection framework, that were originally considered a clever idea, but not any more if I get it correctly.

I propose to include a note in the release notes about the possibility of a breaking change in cases like yours, if that's not already the case.

[c-koell]

@dstango yes i think there is no really performance penaltie ... but as the beanvalidation spec and also the implementor (hibernate) note that you should not annotate both i think if we can prevent it we should.

You are also talking abou the contract between field and getter/setter. But once again i think and also defined in the spec there is no contract that should be fullfilled.

[dstango]

@c-koell Just for clarification of what I meant with contract: I don't mean between field/methods but -- your interface B defines a contract (e.g. a setValue()-method that accepts all kinds of values including null). Your implementation A then imposes stricter rules on what is allowed for setValue() -- namely "null not allowed".
Thus (leaning on your example)

B anObject = new A();
anObject.setValue(null);

would impose stricter rules than defined by B and cause a validator to reject the object.
Of course we could discuss if you consider validation-annotations to be part of the contract of a class, or simply ... ehm ... "validator-hints" (?!?) -- but as you guess I'd go for contract ... ;-)

(BTW: I have no say in lombok, as I'm simply a grateful user of the library, and not a maintainer ... so these are just my 2 cents. Personally I'd be happy to have some sort of configuration option, if it doesn't exist already.)

[dstango]

@c-koell having re-read your quote from the hibernate-doc I see that they'd rather have you put the annotations to the getter or the field, but not the setter. Though I wonder if it would actually do any harm to have the @NotNull also on the setter?!? I see that the interface is generated code in your case -- do you have a say in what exactly is generated?

[rzwitserloot]

I'm putting a stop to this. c-koell doesn't (want to) understand and is sucking all the air out of the room, and I'm calling time of death on this line of reasoning ever getting to new conclusions.

I'll likely delete any further traffic on this issue (or any filed duplicates) for a few months.

[quotidian-ennui]

While I know I'm commenting on something where tempers have become a little frayed (I live in the UK, so I have inherited their capacity for understatement); I'd just like to throw a couple of pennies.

In our case (adaptris/interlok#941) we have an interface that doesn't specify whether or not the underlying getter/setter will be non-null. The interface is fine, it's an interface that indicates availability of methods and nothing else from a behavioural perspective (what calls it is a configuration tool, and there's historical baggage that we're all dealing with)

As it so happens some implementations of this interface don't care that it's null, some implementations do. The implementations that do care, and are using lombok, will have annotated with @Getter @Setter @NotNull on their respective fields. Prior to 1.8.24, the @NotNull would not be cascaded to the getters & setters, and now it is; so effectively this change has broken backwards compatibility.

As I see it, there are effectively 3 choices.

• Change to add @NotNull to the interface is likely to break all existing implementations at runtime where null is happily passed in potentially non-obvious ways.
  • I'm not necesesarily a fan of this, I'm dictating behavioural aspects in the interface definition where I don't have to.
• Downgrade the specific projects that are broken to 1.8.22 leaves them limited ability to use future lombok features they might want to use.
• Removing the @Getter/@Setter annotations in the affected implementations so they aren't generated by lombok means a curious mix of boilerplate and no boilerplate for a non-obvious-reason (unless you document it, but who reads documentation)

Whether or not this change is technically correct or not, the expectation now is that breaking changes from features need to be marked in such a way that's more obvious to users. It's sad but it's true, but a point release isn't where I'd have expected a breaking change of this nature to occur.

[rzwitserloot]

Further discussion in #3180.

[Indifer]

@c-koell You can go back to 1.8.22 first

[timguy]

Should be fixed since 1.18.26 with this commit: 1473389