@ConfigurationPropertiesBinding converters are not invoked for target types that can be converted by ObjectToObjectConverter

[adase11]

I noticed for an application that I was working on that a Converter that I registered via @ConfigurationPropertiesBinding for a class that was a simple wrapper around a String ( i.e. record SimpleWrapper(String value) {}) was not actually invoked. (In my actual application I needed to do some logic to transform the String).

While the Converter for the class when it was a simple wrapper around a String was not invoked, if I made the target class more complex (i.e. record ComplexWrapper(String value, BigDecimal other) {} ) than the converter was invoked. I believe that no matter the type or number of members of the target class that the Converter should always be invoked.

I created a quick demo application to demonstrate the issue (trying to stay to as true to the way the application I am working on is structured while also being as generic as possible). See https://github.com/adase11/property-converter-demo

Specifically the test PropertyConverterDemoApplicationTest
should demonstrate the behavior.

Interestingly, as demonstrated in the test, the behavior is only reproducible by starting up a full application context. My attempts to recreate the behavior with org.springframework.boot.test.context.runner.ApplicationContextRunner were unsucessful.

[wilkinsona]

Thanks for the sample, @adase11. Unfortunately, there's quite a lot of complexity in it and I'm not sure if it's required to reproduce the problem. For example:

• the interfaces with default @Bean methods that are implemented by @AutoConfiguration classes are very unusual.
• there are @AutoConfiguration classes that are not registered in META-INF/spring/org.springframework.boot.autoconfigure.AutoConfiguration.imports but are covered by component scanning. This will cause ordering problems and may cause @ConditionalOnBean to not behave as expected
• You're using a custom properties file and factory

Can you please rework the sample to remove as much of this complexity as possible? To aid problem diagnosis, we'd like something that contains the bare minimum that's required to reproduce the problem.

[adase11]

No problem, I'll leave an update here once I've made the simplification changes.

[adase11]

@wilkinsona - I've simplified the sample in a way that I hope eliminates much of the complexity while still demonstrating that the issue exists (both a positive and negative test case).

The changes are in a new branch (simplify-property-converter-demo) here - https://github.com/adase11/property-converter-demo/tree/simplify-property-converter-demo

I did the following:

• Removed any AutoConfiguration and simplified to regular @Configuration
• Moved the properties to standard application.properties file
• Removed interface, class hierarchy, and @Conditional
• Updated the PropertyConverterDemoApplicationTest in a way that I think more clearly demonstrates the problem
• Updated the PropertyConverterDemoApplication main class to also log out the details of what I hope more clearly explains the expected value after the configuration properties converter is applied vs the actual value.

At a high level, I'm looking to demonstrate that the converter SimpleStringWrapperConverter is not working for SimpleStringWrapper properties while the converter ComplexStringWrapperConverter is working for ComplexStringWrapper where the only difference between the two is that ComplexStringWrapper has 2 String properties vs SimpleStringWrapper has just one.

Both converters should be appending the string -addition to the property value when they are applied. So given the two properties:

demo.complexStringWrapper=complex-value
demo.simpleStringWrapper=simple-value

The values they resolve to should be complex-value-addition and simple-value-addition

Hopes this helps but I'm happy to make any further updates that you find useful!

[wilkinsona]

Thanks very much, @adase11. That's much clearer now. I've changed the tests a little bit to separate them:

diff --git a/src/test/java/com/example/propertyconverterdemo/PropertyConverterDemoApplicationTest.java b/src/test/java/com/example/propertyconverterdemo/PropertyConverterDemoApplicationTest.java
index 8ba0219..6cd8a35 100644
--- a/src/test/java/com/example/propertyconverterdemo/PropertyConverterDemoApplicationTest.java
+++ b/src/test/java/com/example/propertyconverterdemo/PropertyConverterDemoApplicationTest.java
@@ -14,9 +14,13 @@ import org.junit.jupiter.api.DisplayName;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.function.Executable;
 import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.boot.SpringApplication;
 import org.springframework.boot.context.annotation.UserConfigurations;
+import org.springframework.boot.convert.ApplicationConversionService;
 import org.springframework.boot.test.context.SpringBootTest;
 import org.springframework.boot.test.context.runner.ApplicationContextRunner;
+import org.springframework.context.ApplicationContext;
+import org.springframework.core.convert.support.ConfigurableConversionService;
 import org.springframework.core.convert.support.GenericConversionService;
 
 import com.example.propertyconverterdemo.configuration.ConfigurationClass;
@@ -24,12 +28,7 @@ import com.example.propertyconverterdemo.converters.SimpleStringWrapperConverter
 import com.example.propertyconverterdemo.properties.StringWrapperProperties;
 import com.example.propertyconverterdemo.propertywrappers.SimpleStringWrapper;
 
-@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.NONE)
 class PropertyConverterDemoApplicationTest {
-       private final ApplicationContextRunner contextRunner = new ApplicationContextRunner().withUserConfiguration(PropertyConverterDemoApplication.class);
-
-       @Autowired
-       private StringWrapperProperties stringWrapperProperties;
 
        List<Executable> buildAssertions(StringWrapperProperties props) {
                return List.of(
@@ -39,22 +38,18 @@ class PropertyConverterDemoApplicationTest {
        }
 
        @Test
-       @DisplayName("Using @SpringBootTest. Demonstrate that for the 'simple' case the converter does take effect.")
+       @DisplayName("Using SpringApplication. Demonstrate that for the 'simple' case the converter does take effect.")
        void testFullAppSimple_doesNotWork() {
-               assertAll(
-                               buildAssertions(stringWrapperProperties)
-               );
-
+               ApplicationContext context = SpringApplication.run(PropertyConverterDemoApplication.class);
+               assertAll(buildAssertions(context.getBean(StringWrapperProperties.class)));
        }
 
        @Test
        @DisplayName("Using ApplicationContextRunner. Demonstrates that the issue is only reproducible using @SpringBootTest.")
        void testBindingSimple_worksAsExpected() {
-               contextRunner.withConfiguration(UserConfigurations.of(ConfigurationClass.class))
-                               .withPropertyValues(
-                                               "demo.complexStringWrapper=complex-value",
-                                               "demo.simpleStringWrapper=simple-value"
-                               )
+               ApplicationContextRunner contextRunner = new ApplicationContextRunner()
+                               .withUserConfiguration(PropertyConverterDemoApplication.class)
+                               .withPropertyValues("demo.complexStringWrapper=complex-value", "demo.simpleStringWrapper=simple-value")
                                .run(ctx -> {
                                        assertThat(ctx).hasNotFailed();
                                        final StringWrapperProperties props = ctx.getBean(StringWrapperProperties.class);

My goal was to allow the ApplicationContextRunner test to run without the noise of @SpringBootTest also bootstrapping the application.

The ApplicationContextRunner-based test can be made to fail by configuring its BeanFactory to use ApplicationConversionService:

@Test
@DisplayName("Using ApplicationContextRunner. Demonstrates that the issue is only reproducible using @SpringBootTest.")
void testBindingSimple_worksAsExpected() {
	ApplicationContextRunner contextRunner = new ApplicationContextRunner()
			.withUserConfiguration(PropertyConverterDemoApplication.class)
			.withPropertyValues("demo.complexStringWrapper=complex-value", "demo.simpleStringWrapper=simple-value")
			.withInitializer(context -> context.getBeanFactory().setConversionService(ApplicationConversionService.getSharedInstance()))
			.run(ctx -> {
				assertThat(ctx).hasNotFailed();
				final StringWrapperProperties props = ctx.getBean(StringWrapperProperties.class);
				assertAll(
						buildAssertions(props)
				);
			});
}

This mimics some of the setup that's performed by SpringApplication.

I think the problem lies in ConversionServiceDeducer. When the bean factory has a conversion service, any converter beans are added to a second, separate ConversionService. If the first conversion service that comes from the bean factory can perform the requested conversion, the second conversion service isn't used. That's what's happening here. The ObjectToObjectConverter in the first conversion service can convert a String into a SimpleStringWrapper due to its SimpleStringWrapper(String) constructor.

We could fix your problem by reversing the order of the two conversion services, but this would potentially break things for someone relying on the bean factory's conversion service being called first. I'll need to discuss our options with the rest of the team.

In the meantime, you may be able to work around the problem by making some changes to prevent the ObjectToObjectConverter from getting involved. For example, if your SimpleStringWrapper(String) constructor was package-private ObjectToObjectConverter will not use it. I realise that may not be possible in your real app if you can't change the wrapper and/or can't place the converter in the same package.

[adase11]

Thanks for looking into the changes @wilkinsona and I appreciate the suggestion. What I did as a workaround in the meantime was to use essentially the ComplexStringWrappper instead of SimpleStringWrapper and just ignored the other property (as you suspected for my real use case the package-private constructor wasn't the best way to move forward). That is working ok for my use case but I thought it would still be valuable to bring this up.

[wilkinsona]

https://github.com/wilkinsona/spring-boot/tree/gh-34631 contains some updates to the existing unit tests to reproduce the problem.

Summary of the problem for team discussion:

The problem lies in ConversionServiceDeducer. When the bean factory has a conversion service, any converter beans are added to a second, separate ConversionService. If the first conversion service that comes from the bean factory can perform the requested conversion, the second conversion service isn't used. That's what's happening here. The ObjectToObjectConverter in the first conversion service can convert a String into a SimpleStringWrapper due to its SimpleStringWrapper(String) constructor.

We could fix your problem by reversing the order of the two conversion services, but this would potentially break things for someone relying on the bean factory's conversion service being called first.

[philwebb]

We discussed this today and we think that the ConverterBeans could be applied first, but to an empty FormattingConversionService. We'd then add the applicationContext.getBeanFactory().getConversionService() and possibly an ApplicationConversionService.sharedInstance().

[philwebb]

This is a risky change, so we think it's a 3.2 issue.

[asarkar]

@wilkinsona I came across a StackOverflow question that upon investigation, can be attributed to the problem described in this ticket. It appears the fix here was merged in 3.2.0-M1, but even after using 3.2.0-M3, I'm still able to reproduce the same error. I have posted my analysis as an answer to the question, although I admit that doesn't actually solve the issue OP is facing.

Please advise if a separate ticket should be created, or if this ticket can be used for further discussion.

[wilkinsona]

I don't think it's the same problem. The following works for me:

package com.example.gh34631;

import java.util.ArrayList;

import org.springframework.boot.SpringApplication;
import org.springframework.boot.autoconfigure.SpringBootApplication;
import org.springframework.boot.context.properties.ConfigurationProperties;
import org.springframework.boot.context.properties.ConfigurationPropertiesBinding;
import org.springframework.boot.context.properties.EnableConfigurationProperties;
import org.springframework.context.annotation.Bean;
import org.springframework.core.convert.converter.Converter;

import com.google.common.collect.ImmutableList;

@SpringBootApplication
@EnableConfigurationProperties(com.example.gh34631.Gh34631Application.GuavaImmutableListProperties.class)
public class Gh34631Application {

	public static void main(String[] args) {
		GuavaImmutableListProperties properties = SpringApplication
				.run(Gh34631Application.class, "--guava.immutable.values[0]=zero", "--guava.immutable.values[1]=one")
				.getBean(GuavaImmutableListProperties.class);
		System.out.println(properties.getValues());
	}
	
	@Bean
	@ConfigurationPropertiesBinding
	static Converter<ArrayList<?>, ImmutableList<?>> arrayListToImmutableList() {
		return new Converter<>() {

			@Override
			public ImmutableList<?> convert(ArrayList<?> source) {
				return ImmutableList.copyOf(source);
			}
			
		};
		
	}
	
	@ConfigurationProperties("guava.immutable")
	static class GuavaImmutableListProperties {
		
		private ImmutableList<String> values;

		public ImmutableList<String> getValues() {
			return values;
		}

		public void setValues(ImmutableList<String> values) {
			this.values = values;
		}

	}


}

When run, it outputs [zero, one]. This is the case with both 3.1.4 and 3.2.0-M3.

It may be that the use of Kotlin in the question on SO is causing the problem, but it does not appear to be a general conversion service and converter ordering problem like this issue addressed. I'll follow up on Stack Overflow as and when I have something more.