Tested on: Spring Boot 3.2.2
Hello. Sorry for the long title...
What I found is this: if I use constructor binding on a @ConfigurationProperties class and the class has, somewhere, a field of a custom type that implements Map or Collection or extends some Map or Collection class and doesn't have the no-args constructor, even if I provide a Converter for this custom type, the binding fails because the org.springframework.boot.context.properties.bind.Binder class tries to instantiate it before checking to see if there's a converter for it (or at least, that's my guess for why).
***************************
APPLICATION FAILED TO START
***************************
Description:
Failed to bind properties under 'test-project.my-property' to com.example.MyCustomClass:
Reason: java.lang.NoSuchMethodException: com.example.MyCustomClass.<init>()
Action:
Update your application's configuration
This doesn't happen when not using constructor binding! In that case the custom converter (which is annotated with @Component and @ConfigurationPropertiesBinding) is found and used.
Now, I don't know if, when using constructor binding, this binding happens sooner and with less Beans available (including my converter?), compared to "normal" binding. But the documentation states that using @ConfigurationPropertiesBinding on the converter should be sufficient to have it found by Spring. Isn't that right?
I made a small project with 2 different configurations separated by different packages, the "main" one (under the package "com.example") uses constructor binding and its test fails. The other one (under "com.example2") uses "classic" properties binding, with the same custom class and converter for it, and its test passes.
Sorry for the somewhat messy structure and copy-paste of a few classes, but at least it works. Also, I had to mimic the structure of source and test directories of the actual project I'm working on, which is why the folders aren't "src/main/java" and "src/test/java" but just "src" and "test" in the root folder... I don't like it either and it's late, please pardon me if I leave them like that...
Also I hope it's okay to provide you the test project as a zip like that. I won't be able to change this before tomorrow.
Comment From: wilkinsona
Thanks for the sample. I've reproduced the problem.
If I update MyCustomClass so that it does not implement Map, things work as I think you'd like them to in both cases. Does MyCustomClass have to implement Map? That's rather unusual for configuration properties, particularly when you're not binding properties into the map but rather trying to create an instance from a single property. If a consumer of MyCustomClass needs to work with a Map, we generally recommend a toMap() method or similar, rather than the class to which properties are bound being a Map itself.
Comment From: pietro-saccani
Thanks for looking into it.
I was expecting a reply of that kind. Can I play the card of "it's an old-ish, mandatory, external dependency"? 😅
I really wanted to be able to map a property directly to that class for simplicity (I already have a workaround but it's a bit ugly), especially since it works properly without constructor binding. But I do need to have immutable configuration properties unfortunately.
Regardless, in my very humble opinion, the behavior between the two types of property binding should be the same, especially if you do provide a proper converter for the specific class.
Comment From: wilkinsona
We'll see what we can do but we may decide that any fix is too risky to make at all or that we'll only want to make it in 3.3.x.
In both cases, the code in question is this:
https://github.com/spring-projects/spring-boot/blob/550651f88fdb69378ea650946c2507fa05cf34fa/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/MapBinder.java#L54-L72
The difference in behavior arises on lines 56-57 due to the difference in type information that's available during JavaBean binding and constructor binding. With the former, target.getValue() is not null so createMap is called with Map.class. With the latter, target.getValue() is null so createMap is called with MyCustomClass and creation fails due to the lack of a default constructor.
Interestingly, this map creation is a waste of time as we reach line 65 and return the result of the property conversion anyway. It may be possible to rework things so that the map creation is deferred until it's needed. That would make things more efficient in the JavaBean binding case that's already working and may, as a side-effect, get the constructor binding case working as well.
Comment From: pietro-saccani
Yeah, conceptually it's what I thought. It tries to do "generic" Map/Collection stuff before even checking if there's a converter available.
I wasn't expecting a fix anytime soon anyway. But I'm glad that this issue of mine could potentially lead to optimizing the code too :V
Thank you again.
Comment From: mhalbritter
I have something in https://github.com/mhalbritter/spring-boot/tree/39375-configurationproperties-binding-inconsistency-in-edge-case-between-constructor-binding-and-normal-binding-with-custom-maps-or-collections which passes all tests. Should we target that for 3.1.x or do we consider that too risky?
Comment From: philwebb
That looks pretty safe to me. I'd vote 3.1
Comment From: wilkinsona
Given that it was reported against 3.2 and hasn't had any reactions or comments suggesting it's affecting multiple users, I'd vote 3.2.x. It seems to be a niche problem so the overall benefit is low and pretty narrow while there's a small risk of regression for most users.