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

Allow @ConstructorBinding to be optional #23216

Closed
philwebb opened this issue Sep 8, 2020 · 5 comments
Closed

Allow @ConstructorBinding to be optional #23216

philwebb opened this issue Sep 8, 2020 · 5 comments
Assignees
Labels
type: enhancement A general enhancement
Milestone

Comments

@philwebb
Copy link
Member

philwebb commented Sep 8, 2020

Currently ConfigurationPropertiesBindConstructorProvider requires that @ConstructorBinding be used to indicate that a @ConfigurationProperties instance should use the ValueObjectBinder. I think we could skip the need for this if there is a single constructor with one or more arguments.

Somewhat related to #23172

@philwebb philwebb modified the milestones: 2.2.x, 2.x Sep 8, 2020
@philwebb philwebb added for: team-attention An issue we'd like other members of the team to review type: enhancement A general enhancement labels Sep 8, 2020
@Davio
Copy link

Davio commented Sep 8, 2020

Would this mean that my @ConfigurationProperties classes which are currently annotated by Lombok's @Data (which creates getters and setters for my non-final fields) can be annotated by @Value instead (which creates final fields with an all args constructor)?

If so, I think that would be a reasonable improvement.

@snicoll
Copy link
Member

snicoll commented Sep 8, 2020

I think we could skip the need for this if there is a single constructor with one or more arguments.

I don't think we can/should. I remember us having a very lengthy conversation that concluded with the current arrangement. One problem with this approach is that you can't opt-out if you happen to have a single constructor and you don't want it to be used that way (can be surprising for folks having an existing class that uses javabean conventions and a constructor for whatever reason).

@Davio
Copy link

Davio commented Sep 8, 2020

Maybe it's an idea to consolidate @ConfigurationProperties and @ConstructorBinding by adding the binding mode as an optional property to @ConfigurationProperties or something like useConstructor = true which would work if there is a single constructor?

Would it work if the constructor only sets some of the properties, but others should still be set by setters?

@philwebb
Copy link
Member Author

philwebb commented Sep 8, 2020

@snicoll I remember some discussions, but I couldn't remember exactly why we landed on the current setup.

One problem with this approach is that you can't opt-out if you happen to have a single constructor and you don't want it to be used that way

Ah yes, I remember now. The issue is that you may have a @ConfigurationProperties class that is either a @Component or is loaded via a @Import. In those situations you want Spring to inject beans into the constructor and getter/setter binding to be applied afterwards.

@philwebb philwebb closed this as completed Sep 8, 2020
@philwebb philwebb added status: declined A suggestion or change that we don't feel we should currently apply and removed for: team-attention An issue we'd like other members of the team to review type: enhancement A general enhancement labels Sep 8, 2020
@philwebb philwebb removed this from the 2.x milestone Sep 8, 2020
@philwebb philwebb added this to the 3.0.x milestone Nov 29, 2021
@philwebb philwebb added type: enhancement A general enhancement and removed status: declined A suggestion or change that we don't feel we should currently apply labels Nov 29, 2021
@philwebb philwebb reopened this Nov 29, 2021
@mbhave mbhave self-assigned this Dec 2, 2021
@mbhave mbhave closed this as completed in 44b88cc Feb 16, 2022
@mbhave mbhave modified the milestones: 3.0.x, 3.0.0-M2 Feb 16, 2022
mbhave added a commit that referenced this issue Feb 16, 2022
@philwebb philwebb reopened this Feb 17, 2022
@philwebb

This comment was marked as outdated.

@snicoll snicoll added the for: team-attention An issue we'd like other members of the team to review label Mar 13, 2022
@philwebb philwebb removed the for: team-attention An issue we'd like other members of the team to review label Mar 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

4 participants