[FEATURE] Narrowing lombok.copyableAnnotations target

[el-dot]

Problem

Right now, annotations in lombok.copyableAnnotations are copied to all eligible places generated by lombok. The problem is that some annotations does not make sense in all places.

Example

In spring one can configure lombok Autowired and Value annotations like this (to support required=false in generated constructor injection):

lombok.copyableAnnotations += org.springframework.beans.factory.annotation.Autowired

The Problem is that @Autowired is then copied to getters and causes Spring runtime warnings:

Autowired annotation should only be used on methods with parameters:

Feature

Allow to specify copyableAnnotations per generated method/lombok annotation.

I would like to configure:
"Copy annotation foo.Bar to getters" instead of "Copy annotation foo.Bar to every possible place"

Proposal

To stick with current conventions, annotation specific config could be following:

lombok.getter.copyableAnnotations += foo.Bar

NOTE: I think lombok.getter.copyableAnnotations list should be merged with lombok.copyableAnnotations to not break current configs

[rzwitserloot]

I've spoken about this before but can't find the issue. It's a combinatory explosion; I could come up with at least 15 different unique 'takes' on precisely where lombok should and should not copy annotations to, and under which conditions - half of which would also be hard to implement due to lack of resolution.

That's way too complicated - the tests and docs alone'd be a nightmare. So, that's just not a solution that actually solves the problem.

Instead I think the right move is to continue to expand on lombok's baked in support - whilst we can't ask a random lombokuser to consider precisely which of the 15+ different variants of copy behaviour they'd like, we can ask the core developers of whatever library is fielding said annotation.

Unfortunately, we don't have that either - we do have an easy way to add new annotations to lombok's baked in list of 'annotations that should by default be copied', but not (yet) a way to specify more specific copy behaviour. A real problem there is veracity: Someone is going to send a PR adding some annotation we don't know about along with an exact specification of the required copy behaviour. But how do we know it is correct? We don't use it and this stuff is complicated; it may well take us many hours to figure out what the library even does and determine if the PR is correct.

We think there's a solution to that too: Only a project owner can send it. If they want to contribute a broken definition, they're ruining their own project's utility just as much as they're ruining lombok's with it. Also, any issues that come up about incorrect copy behaviour can be trivially closed with 'well, the project maintainers who wrote that annotation thinks this is how to do it, so go file a ticket with those folks. And please explain to them we will also be sending them the angry hordes because a fix to the copy behaviour is backwards incompatible'.

But that does mean I bet it'll be used a lot less - not every project maintainer would presumably be interested or motivated to write such contributions.

Point is: We think it's the right way forward, but it's low priority. That means a PR that updates lombok to let you easily add PRs that match annotation type names to copy behaviour would be accepted, but note that we'll document that only project maintainers get to submit PRs. And it also means we're likely never going to get around to it ourselves.

[c-koell]

As written in #3166 (comment) i think also it makes no sense on some types of Annotations.
Especially the Annotation javax.validation.constraints.NotNull makes no sense from my point of view.

Is it possible to disable the whole feature or can we remove the Annotation javax.validation.constraints.NotNull ?

[el-dot]

@c-koell

Is it possible to disable the whole feature or can we remove the Annotation javax.validation.constraints.NotNull ?

I'm not sure if I understand you correctly, but I think if you don't specify lombok.getter.copyableAnnotations the feature would be "disabled"?

@rzwitserloot

Instead I think the right move is to continue to expand on lombok's baked in support .

Baked-in support and suggested solutions sounds a lot more complicated and way harder to maintain proper, up-to-date state, than allowing people to just specify which annotations should be copied per generated place.

[c-koell]

@el-dot i think with lombok.getter.copyableAnnotations you can define additional Annotations to copy.
We want to exclude the javax.validation.constraints.NotNull or to disable the whole feature.

[el-dot]

@c-koell Maybe something like lombok.getter.copyableAnnotations = [] would work? (Not sure and have no time to check this, but adding elements by += suggests that = may override the list)

[c-koell]

After a short look at the code i think it is not possible to clear the hardcoded list of annotations :-(

[rzwitserloot]

@el-dot your suggestion is denied; it boils down to having lombok actively act incorrectly (copying annotations whose semantics do not imply that copying them is correct), and then handwave the problem away by saying 'eh, it does not matter, users can look up how to fix lombok on their own by adding to the 'negative' copyable list'.

No.

There are only two available options:

• You, @c-koell and everybody else using this annotation is misunderstanding their purpose is abusing them. That's on you.
• The inclusion of this annotation on the list is incorrect and it should be kicked. That's backwards incompatible, so that's a big deal, but, if it shouldn't be on there, it shouldn't be on there.

I'm mystified as to what, exactly, a third option would look like.

Regardless of which of the above 2 explains this, 'just let users remove the annotation from the copy list' is incorrect.

[bkoziak]

Hi @rzwitserloot,

second options seems to be more valid, at least in my case. By adding javax.validation.constraints.NotNull to the BASE_COPYABLE_ANNOTATIONS list a behavior for a below pojo changed in a backward incompatible way. Now, instead of a single exception raised during validation we have two of them (due to annotation present on the field and getter). It has its consequences in further processing.

@Data
public class TestRequestObject {
    private String fieldA;

    @NotNull
    private String fieldB;
}

It's not clear to me what should be done to at the same time use the latest lombok version and have this code working as in previous versions. What I think could be done is to replace @NotNull with @Getter(onMethod=@__({@NotNull})) but it looks like a workaround not a solution.
I will be greatful for support in understanding why the code I had before was incorrect and how I should refactor it. Thanks!

[el-dot]

@rzwitserloot

Regardless of which of the above 2 explains this, 'just let users remove the annotation from the copy list' is incorrect

It's not about is it correct, but should I be able turn off lombok parts which I don't want.

I don't believe lombok, as boilerplate generation library, should be generating code I don't want it to (for any reason).

[rzwitserloot]

@bkoziak As I said in issue #3166, in the scenario where javax.validation's @NotNull is on a field and lombok makes getters and setters for it, it fairly clear from various documentation that it should either be on the field, or be on the getter/setter and not both. However, it is not at all clear to me that the only correct answer is: "It should remain on the field only". It feels more correct that the right answer is: Move it to the getter/setter, delete it from the field.

Given that the validation project cannot be bothered to fix this on their end (how hard is it to see this and just.. not double-validate? You're proposing we bend over backwards to attempt to write smarter code that tries to figure out what's going wrong, when they can trivially do so), my patience for trying to fix this is really, really low. Why don't you go bother this silly validation project that decides to apply the exact same validation rule twice in a row because they couldn't be bothered to realize that the semantic definition of the annotation they decided to use is nowhere near as mechanically clearly defined to warrant dogged insistence on doing a silly thing (namely, same validation twice in a row)?

It's not about is it correct, but should I be able turn off lombok parts which I don't want.

That's not how lombok works. Specifically, we don't want to give you a twiddling knob for every imaginable thing, as that would result in an untestable, extremely hard to document combinatory explosion of options, and a needlessly steep learning curve, or, 999 twiddly bits that are all documented as: "This knob exists solely to work around bizarre (wrong) takes on semantic meanings and buggy / badly designed libraries", and that seems overly hostile.

[bkoziak]

@rzwitserloot I get your point and in general agree. What I don't understand is why @NotNull was added to the list. Up until 1.18.24 this annotation was not copied to accessors automatically what didn't cause any issues (I'm aware of) but now it is and it clearly is troublesome for many people (validation is quite common thing). Wouldn't be better just to keep status quo until behavior of the hibernate validator library (and probably others) allow to safely copy it? What's the point of adding a change that instead of improving breaks stuff?

[rzwitserloot]

Issue closed - further discussion about specifically validation.NonNull copying in issue #3180. An opt-out mechanism is not going to happen, which is what this issue is asking for as a solution.