Skip to content

ConstructorBinding default detection is considered with a private constructor #32639

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

Closed
snicoll opened this issue Oct 8, 2022 · 2 comments
Closed
Assignees
Labels
type: regression A regression from a previous release
Milestone

Comments

@snicoll
Copy link
Member

snicoll commented Oct 8, 2022

I am upgrading a project to Spring Boot 3 and I am facing an issue that is summarized in this sample project: https://github.com/snicoll-scratches/demo-binder

I have a configuration properties with a nested object that has a constructor. The constructor is for convenience within the class as it allows me to set configurable default values. The nested object is used to signal that it is a @NestedConfigurationPropery. There's no setter.

If I run the annotation processor on this project, I can now see (after @mbhave told me!) that it defaults to constructor binding and the other POJO properties aren't detected.

I've added two tests. The first test sets such property (description). Binding works and the value is set using the setter. The second test try to set the value within the constructor. In that case, the context fails as follows:

 Caused by: org.springframework.boot.context.properties.bind.BindException: Failed to bind properties under 'test' to com.example.demobinder.TestProperties
 	at org.springframework.boot.context.properties.bind.Binder.handleBindError(Binder.java:387)
 	at org.springframework.boot.context.properties.bind.Binder.bind(Binder.java:347)
 	at org.springframework.boot.context.properties.bind.Binder.bind(Binder.java:332)
 	at org.springframework.boot.context.properties.bind.Binder.bind(Binder.java:262)
 	at org.springframework.boot.context.properties.bind.Binder.bind(Binder.java:249)
 	at org.springframework.boot.context.properties.ConfigurationPropertiesBinder.bind(ConfigurationPropertiesBinder.java:94)
 	at org.springframework.boot.context.properties.ConfigurationPropertiesBindingPostProcessor.bind(ConfigurationPropertiesBindingPostProcessor.java:89)
 	... 98 more
 Caused by: java.lang.IllegalStateException: No setter found for property: one
 	at org.springframework.boot.context.properties.bind.JavaBeanBinder.bind(JavaBeanBinder.java:104)
 	at org.springframework.boot.context.properties.bind.JavaBeanBinder.bind(JavaBeanBinder.java:83)
 	at org.springframework.boot.context.properties.bind.JavaBeanBinder.bind(JavaBeanBinder.java:59)
 	at org.springframework.boot.context.properties.bind.Binder.lambda$bindDataObject$5(Binder.java:476)
 	at org.springframework.boot.context.properties.bind.Binder$Context.withIncreasedDepth(Binder.java:590)
 	at org.springframework.boot.context.properties.bind.Binder$Context.withDataObject(Binder.java:576)
 	at org.springframework.boot.context.properties.bind.Binder.bindDataObject(Binder.java:474)
 	at org.springframework.boot.context.properties.bind.Binder.bindObject(Binder.java:414)
 	at org.springframework.boot.context.properties.bind.Binder.bind(Binder.java:343)
 	... 103 more

If the constructor is switched to private, the binder still tries to use it.

Now I know that this object is managed by constructor binding, the error message is helping. But without all that context, I find the upgrade path really confusing. It doesn't feel consistent that description is injected either (except if we have some sort of fallback handling I am not aware of?).

@snicoll snicoll added the status: waiting-for-triage An issue we've not yet triaged label Oct 8, 2022
@philwebb philwebb added this to the 3.0.x milestone Oct 17, 2022
@philwebb philwebb added type: bug A general bug type: regression A regression from a previous release and removed status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Oct 17, 2022
@philwebb philwebb self-assigned this Oct 17, 2022
@philwebb
Copy link
Member

It's a bit of a tricky one to know how to fix. We need some signal that the constructor for TestProperties.SimpleElement should not be used. We could introduce a @NoConstructorBinding annotation (with a better name), but I wonder if we can use a private constructor as a signal instead.

@philwebb
Copy link
Member

I've gone with the private constructor for now. The sample app works if I change the SimpleElement. I suspect we might find another use-case in the future where a private constructor can't be used, but we should probably wait and see.

@philwebb philwebb modified the milestones: 3.0.x, 3.0.0-RC1 Oct 18, 2022
@snicoll snicoll changed the title ConstructorBinding detection is not consistent ConstructorBinding default detection is considered with a private constructor Oct 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: regression A regression from a previous release
Projects
None yet
Development

No branches or pull requests

2 participants