Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

onX support for constructor argument annotations #1335

Closed
jhovell opened this issue Mar 14, 2017 · 18 comments
Closed

onX support for constructor argument annotations #1335

jhovell opened this issue Mar 14, 2017 · 18 comments

Comments

@jhovell
Copy link

jhovell commented Mar 14, 2017

Feature request to be able to include annotations on generated constructors for @RequiredArgsConstructor and/or @AllAgsConstructor

Right now it is possible to add an annotation to the constructor itself via:

@AllArgsConstructor(onConstructor=@__(@Inject))

but to add annotations to the arguments does not seem to be possible. This is supported via @Setter annotation, at least I think, though this related issue hinting maybe the documentation is not quite accurate
#1178

I think this could work either by adding an annotation to the fields themselves or some new syntax added to the AllArgsConstructor / RequiredArgsConstructor annotation.

The use case is being able to generate constructors for DI-managed beans that need the arguments to contain additional qualifier annotations to inject the proper bean.

@Maaartinus
Copy link
Contributor

FTR: Same request on SO.

@rzwitserloot
Copy link
Collaborator

The problem with this, is twofold:

  • We can't really hardcode @Named. For starters, it feels a bit dirty to hardcode something, even something from javax.inject. More to the point, you're doing it wrong. @Named is a codesmell. Make your own annotation, mark it a binding annotation, and use that. @Named is stringly typed, and stringly typed is dirty (says the guy who wrote stringly typed field exclusion/inclusion via of and exclude.. just do what I say and not what I do... 😉). We can't find any binding annotation (that would require resolution), and having you list them in lombok.config is more trouble than it's worth.
  • We could just move or copy all annotations on the field to the constructor parameters, possibly instigated by some other annotation, but, this does not work for annotations that are allowed only on parameters and not on fields. Javac would find a known annotation on a field that can't go there, error out, and never even call lombok. We won't get a chance to fix it.

A solution, then, would take one of the following two forms:

  • An @OnConstructor(@__(@Named("foo"))) (JDK6/7) / @OnConstructor(ann_=@Named("foo")) (JDK8+) construct. This is already hacky and hard to support, I don't really want to go any deeper down that rabbit hole. But it's an option. There are some limits as to the complexity of the annotations you're copying here. The other onX stuff moves, this would have to copy (multiple constructors might be generated), hence the limits on how complex they can get. No args or string constants will be fine.
  • You write the entire constructor signature, but we generate the list of this.fieldName = argName; statements for you.

Both are really hard to 'discover' and not very natural. No nice way to do this, I'm afraid.

@jhovell
Copy link
Author

jhovell commented Mar 25, 2017

@rzwitserloot I agree the very general case might be hard to support, though I think there is value in supporting more specific, common use cases. I think the use case I'm about to describe is quite typical, but if it's code smell or there's a better way to do it, I'm all ears. My use case is to use Lombok/code generation to lower the barrier to using constructor-based dependency injection (rather than field based). In this use case, for any relevant annotation the annotation is allowed on a field as well as a constructor parameter. Typically the only annotation added is ConfigProperty or a custom binding annotation.

https://deltaspike.apache.org/documentation/configuration.html#Injectionofconfiguredvaluesintobeansusing@ConfigProperty

I'd see value in being able to use onX to support this, though even if it was supported the next hurdle is to support automatic generation of interfaces from classes as otherwise typically DI doesn't work as the classes are not proxyable. I've seen a few other libraries out there that support interface generation from classes (though they are very old) and as far as I know Lombok does not support this either.

So in short you don't think the second bullet "We could just move or copy all annotations..." is worth implementing for this common use case?

@Maaartinus
Copy link
Contributor

@Named is a codesmell.... stringly typed is dirty

Sure, but for mapping a property file with dozens of lines, it's the only sane way. Anyway, I agree that hardcoding any annotation feels wrong.

list them in lombok.config is more trouble than it's worth

To me, it looks like the simplest workable solution. Having to add all binding annotations to the config file sounds like boilerplate, but it allows to save much more boilerplate. It's error-prone as it's easy to forget one annotation, but it's easy to write a test for it (finding all binding annotations is the hard part, but Guava ClassPath helps).

The other onX stuff moves, this would have to copy (multiple constructors might be generated)

Right, but DI can't use multiple constructors (there must be either a single constructor or a single constructor annotated with @Inject). Copying the annotation to all constructors is not necessary (all my constructor-injected classes have a single constructor generated by @RequiredArgsConstructor(onConstructor=@__(@Inject))).

Javac would find a known annotation on a field that can't go there

Sure, this could happen, but binding annotations make sense on constructor arguments, setters and fields, so I don't think anyone ever writes a binding annotation disallowed on fields.

For me, either @OnConstructor (supporting a single constructor) or configuration (moving all listed annotations) would do perfectly.

@ghost
Copy link

ghost commented Mar 27, 2017

Literally ran into this problem at work on Friday. I thought the problem was something else, but as it turns out, Lombok doesn't copy @nAmed for RequiredArgsConstructor.

This is very common, especially at my company (a very well known and big company).. so I would like to +1 the hell out of this issue. Even if it's added to experimental, I totally plan on using something like this.

@Gustu
Copy link

Gustu commented Jun 9, 2017

👍

@rmozone
Copy link

rmozone commented Feb 27, 2018

+1 for such a feature. It's not only handy but also promotes constructor DI which promotes clean unit testing.

@allemattio
Copy link

I posted about this in Lombok google group.This would be nice, I was thinking on an annotation for the parameters, something like this:
@NonNull @ConstructorAnnotation(@onArgument=@__({@Lazy})) Object field;

@Sam-Kruglov
Copy link

Using Spring Boot, sometimes I need to inject a @Value field from properties file. And that may only be one field while there are 10 fields in the class and then I must add constructor MYSELF which is OBVIOUSLY VERY HARD

@ayusun
Copy link

ayusun commented Apr 14, 2018

+1,
For me, I wanted to use @Qualifiers to specify the bean to be injected for this use case

@rspilker
Copy link
Collaborator

@solongtony
Copy link

@rspilker The use cases you linked to are for including and excluding, and a few other specific actions. They don't cover arbitrary annotations, which is the original request (and what people have been +1) Specifically, the linked use case does not cover annotations required by dependency injection frameworks, or serialization / deserialization frameworks.

Like some others, I'm specifically wanting to be able to add com.fasterxml.jackson.annotation.JsonProperty to each arg of the AllArgsConstructor. Copying the annotation from the member fields would work fine for this case. An AllArgsConstructor property that allows specifying a annotation to add to each arg would be even better.

@janrieke
Copy link
Contributor

Doesn't lombok.copyableAnnotations work for this?

@bannmann
Copy link

bannmann commented Jan 3, 2019

The problem with this, is twofold:
(...)

A solution, then, would take one of the following two forms:

  • An @OnConstructor(@__(@Named("foo"))) (JDK6/7) / @OnConstructor(ann_=@Named("foo")) (JDK8+) construct. (...)

  • You write the entire constructor signature, but we generate the list of this.fieldName = argName; statements for you.

Both are really hard to 'discover' and not very natural. No nice way to do this, I'm afraid.

I also missed the @Named annotation today, but given the constraints @rzwitserloot outlines above, I agree that there's no nice solution. I don't like writing constructors, but in this case it's the cleanest approach.

@solongtony
Copy link

solongtony commented Jan 18, 2019

@janrieke It seems like copyableAnnotations will work. I'll give it a try sometime.
https://projectlombok.org/changelog
https://projectlombok.org/features/constructor

@mwisnicki
Copy link

copyableAnnotations does not help if the annotation does not list field among valid targets. E.g. Spring's @DefaultValue.

@pelepelin
Copy link

pelepelin commented Aug 18, 2021

I've found this trying to use Spring @DefaultValue as well, which is required for constructor injection with defaults for @ConfigurationProperties https://docs.spring.io/spring-boot/docs/current/reference/htmlsingle/#features.external-config.typesafe-configuration-properties.constructor-binding

@JacksonBaileyHM
Copy link

Hello, I noticed the feature proposal mentioned here on the wiki was removed. Can this be reopened? Thank you for your time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests