Upgrading project to 2.0.2 and ran into an unusual issue. The project has a ConfigurationPropertiesBinding that checks String properties for the text 'encoded:' and base64 decodes the contents if found. This works in 1.5.13, but after upgrading to 2.0.2, this stopped working.
I've reviewed the migration notes as well as the docs around Properties. I notice a number of changes around Binding, but don't see anything that implies that this should not work anymore.
I've created a minimal repo. Simple configuration with a Service that outputs the property value.
The only different is the version of Spring Boot.
Please let me know if I should be configuring the converter in a different manner than I am, or if it's actually an issue in Boot 2.0.2.
Example repo: https://gitlab.com/mikezx6r/boot-property-issue
tag boot-1.5.13 works tag boot-2.0.2 does not. Only change is upgrading Spring Boot in build.gradle
Comment From: philwebb
This looks like an issue with the new BindConverter class introduced in 2.0. It contains a CompositeConversionService with the TypeConverterConversionService being used first and the user defined ConversionService second. In this case the TypeConverterConversionService can convert String -> String as a NO_OP so it never bothers trying any further conversions.
Comment From: snicoll
@mikezx6r regardless of the regression @philwebb described, does your actual use case involve a conversion from String to String? Using the ConversionService for this looks weird to me (we can't guarantee that a future optimization may ignore your converter in the future).
Comment From: mikezx6r
First, thank you for such a quick response. Greatly appreciated!
@snicoll It's a convenience. In a properties file, I can put either
value: some string
OR
value: encoded:base64encodedstring
and in the @ConfigurationProperties class that consumes it, it's just a String.
The converter looks for the encoded: prefix and decodes the value.
I can fix this by creating an explicit type (Decoded or something) that's effectively a String. Then the converter would be String -> Decoded and the @ConfigurationProperties class would have a Decoded rather than a String. Not as 'slick', but definitely less likely to get broken either by another String->String converter or an optimization.
If it's decided that a Converter from T -> T will be ignored, perhaps a warning/error can be issued?
Comment From: snicoll
If it's decided that a Converter from T -> T will be ignored, perhaps a warning/error can be issued?
What I am saying is that a Converter<T,T> does not make much sense to me and, as such, optimizing to NO_OP is fine and doesn't need to issue a warning. With your setup, the value in the raw Environment is different from the one you effectively use in code (there is a Sanitizer to hide that from /env endpoints).
In any case, we should restore the behaviour that user-defined converters are invoked first, just sharing some thoughts on your setup.
Comment From: mikezx6r
Bottom-line you're recommending I make it clear that the property in the file is potentially different once in the code, and therefore use an explicit type.
I agree and have moved the code base to that. Also means I don't need to wait for the fix if it's decided to restore the behaviour for user-defined converters even if it's Converter<T,T>
Comment From: adam-mccormick
@snicoll I'd just like to give some context on the use case and why we have a Converter<T, T>. Some of our properties injected by the cloud platform come base 64 encoded. Some of these are strings and some are resources.
I felt that this was a configuration issue and wanted a solution that would enable us to sort this out in the properties file itself rather than using declared types (such as marking a configuration class property an EncodedString or EncodedResource). Using a custom type only works if I have control over the configuration properties class myself. If one of these base 64 encoded values need to be used as a spring or other lib property value I can't do this (since it will just assume it's a string).
The solution was to add these custom converters which were able to determine how a property value should be read and set on the configuration (in this case looking for a custom prefix indicating that the string value of the property is actually a base64 encoded and must be decoded first). We have another converter that handles String -> Resource conversions where the resource needs to be constructed by some base64 encoded data.
I mention this because I think the use case is valid I like the ability to take control of the way a property should be converted, even if the java types match the conversion might need to be specialized.
Just want to make this use case clear so that it might be kept in mind in case there are any future changes that might impact this behavior.
Comment From: snicoll
I mention this because I think the use case is valid I like the ability to take control of the way a property should be converted, even if the java types match the conversion might need to be specialized.
No question asked about that but the ConversionService contract is clear: the converter class itself makes it clear that it’s intended for type conversion. When the source and target types are the same, there’s obviously no type conversion happening so it’s entirely reasonable for no conversion to be performed.
Given that this optimization is entirely reasonable, I think it is wrong to rely on a converter for your use case. There is a concept of Formatter in Spring land that allows you to customize how a given value is "rendered". IMO, it's a much better approach and perhaps there is a hook point we need to add in binding for this.
I am also puzzled by the fact environment.getProperty("my.encoded.prop") returns a different value than the one the @ConfigurationProperties hold ultimately. If you want to chat more about this arrangement, please join us on Gitter as this issue isn't directly related to that.
Comment From: snicoll
@philwebb I had a quick look to this one and it is quite tricky as user-defined converters are buried behind a generic ConversionService type that can either be a user-defined ConversionService or a ConversionService we build ourselves with the @ConfigurationPropertiesBinding annotated one if they exist (see ConversionServiceDeducer).
I wonder how this is different from any use of the ConversionService. If I want my converter to be invoked first, there must be some way to do that (expressing an ordering priority or something). This looks more consistent than us putting them manually in front of what we create.
According to @jhoeller
we don't really have a first-class mechanism for registering an 'override' converter... you'd have to decorate the ConversionService and make custom converters apply before you delegate to the default ConversionService
So I guess we need to keep track of those custom converters and add them in front manually.
Comment From: magdel
With upgrade from 1.5 to 2.1 converters with @ConfigurationPropertiesBinding are just ignored. Is it going to be fixed or we have to forget typed params and switch to basic types?
Comment From: snicoll
I don’t think that the “are just ignored” part is legit. There is an history in this issue that shows only very specific converters are affected. If you believe the problem is more global, please open a separate issue with a sample we can run.
Comment From: magdel
Yes, thank you. I found it has something with changes in validation routine. Converters are just fine itself.