From f4e05c91c74ee65e242febd4d46543e8f74c26c7 Mon Sep 17 00:00:00 2001 From: Andy Wilkinson Date: Thu, 6 Jul 2023 20:17:10 +0100 Subject: [PATCH] Use converter beans in preference to ObjectToObjectConverter Previously, with the converter beans in a conversion service that appears after the bean factory's conversion service, they would not be called for a conversion that could be handled by the ObjectToObjectConverter in the bean factory's conversion service. This commit creates a new FormattingConversionService that is empty except for the converter beans and places it first in the list. It's followed by the bean factory's conversion service. The shared application conversion service is added to the end of the list to pick up any conversions that the previous two services could not handle. This should maintain backwards compatibility with the previous arrangement where the converter beans were added to an application conversion service that went after the bean factory's conversion service. Fixes gh-34631 --- .../properties/ConversionServiceDeducer.java | 16 ++++-- .../ConfigurationPropertiesTests.java | 57 ++++++++++++------- .../ConversionServiceDeducerTests.java | 10 ++-- 3 files changed, 54 insertions(+), 29 deletions(-) diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/ConversionServiceDeducer.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/ConversionServiceDeducer.java index 3593faf4230..775680c794a 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/ConversionServiceDeducer.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/ConversionServiceDeducer.java @@ -31,6 +31,7 @@ import org.springframework.core.convert.converter.Converter; import org.springframework.core.convert.converter.GenericConverter; import org.springframework.format.Formatter; import org.springframework.format.FormatterRegistry; +import org.springframework.format.support.FormattingConversionService; /** * Utility to deduce the {@link ConversionService} to use for configuration properties @@ -59,14 +60,21 @@ class ConversionServiceDeducer { private List getConversionServices(ConfigurableApplicationContext applicationContext) { List conversionServices = new ArrayList<>(); + ConverterBeans converterBeans = new ConverterBeans(applicationContext); + if (!converterBeans.isEmpty()) { + FormattingConversionService beansConverterService = new FormattingConversionService(); + converterBeans.addTo(beansConverterService); + conversionServices.add(beansConverterService); + } if (applicationContext.getBeanFactory().getConversionService() != null) { conversionServices.add(applicationContext.getBeanFactory().getConversionService()); } - ConverterBeans converterBeans = new ConverterBeans(applicationContext); if (!converterBeans.isEmpty()) { - ApplicationConversionService beansConverterService = new ApplicationConversionService(); - converterBeans.addTo(beansConverterService); - conversionServices.add(beansConverterService); + // Converters beans used to be added to a custom ApplicationConversionService + // after the BeanFactory's ConversionService. For backwards compatibility, we + // add an ApplicationConversationService as a fallback in the same place in + // the list. + conversionServices.add(ApplicationConversionService.getSharedInstance()); } return conversionServices; } diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/ConfigurationPropertiesTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/ConfigurationPropertiesTests.java index a0683316bbc..0e7a8fe02a0 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/ConfigurationPropertiesTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/ConfigurationPropertiesTests.java @@ -644,24 +644,36 @@ class ConfigurationPropertiesTests { @Test void loadShouldUseConverterBean() { - prepareConverterContext(ConverterConfiguration.class, PersonProperties.class); + prepareConverterContext(PersonConverterConfiguration.class, PersonProperties.class); Person person = this.context.getBean(PersonProperties.class).getPerson(); assertThat(person.firstName).isEqualTo("John"); assertThat(person.lastName).isEqualTo("Smith"); } @Test - void loadWhenBeanFactoryConversionServiceAndConverterBean() { + void loadWhenBeanFactoryConversionServiceAndConverterBeanCanUseBeanFactoryConverter() { DefaultConversionService conversionService = new DefaultConversionService(); conversionService.addConverter(new AlienConverter()); this.context.getBeanFactory().setConversionService(conversionService); - load(new Class[] { ConverterConfiguration.class, PersonAndAlienProperties.class }, "test.person=John Smith", - "test.alien=Alf Tanner"); + load(new Class[] { PersonConverterConfiguration.class, PersonAndAlienProperties.class }, + "test.person=John Smith", "test.alien=Alf Tanner"); PersonAndAlienProperties properties = this.context.getBean(PersonAndAlienProperties.class); assertThat(properties.getPerson().firstName).isEqualTo("John"); assertThat(properties.getPerson().lastName).isEqualTo("Smith"); - assertThat(properties.getAlien().firstName).isEqualTo("Alf"); - assertThat(properties.getAlien().lastName).isEqualTo("Tanner"); + assertThat(properties.getAlien().name).isEqualTo("rennaT flA"); + } + + @Test + void loadWhenBeanFactoryConversionServiceAndConverterBeanCanUseConverterBean() { + DefaultConversionService conversionService = new DefaultConversionService(); + conversionService.addConverter(new PersonConverter()); + this.context.getBeanFactory().setConversionService(conversionService); + load(new Class[] { AlienConverterConfiguration.class, PersonAndAlienProperties.class }, + "test.person=John Smith", "test.alien=Alf Tanner"); + PersonAndAlienProperties properties = this.context.getBean(PersonAndAlienProperties.class); + assertThat(properties.getPerson().firstName).isEqualTo("John"); + assertThat(properties.getPerson().lastName).isEqualTo("Smith"); + assertThat(properties.getAlien().name).isEqualTo("rennaT flA"); } @Test @@ -1440,7 +1452,7 @@ class ConfigurationPropertiesTests { } @Configuration(proxyBeanMethods = false) - static class ConverterConfiguration { + static class PersonConverterConfiguration { @Bean @ConfigurationPropertiesBinding @@ -1450,6 +1462,17 @@ class ConfigurationPropertiesTests { } + @Configuration(proxyBeanMethods = false) + static class AlienConverterConfiguration { + + @Bean + @ConfigurationPropertiesBinding + Converter alienConverter() { + return new AlienConverter(); + } + + } + @Configuration(proxyBeanMethods = false) static class NonQualifiedConverterConfiguration { @@ -2398,8 +2421,7 @@ class ConfigurationPropertiesTests { @Override public Alien convert(String source) { - String[] content = StringUtils.split(source, " "); - return new Alien(content[0], content[1]); + return new Alien(new StringBuilder(source).reverse().toString()); } } @@ -2467,21 +2489,14 @@ class ConfigurationPropertiesTests { static class Alien { - private final String firstName; + private final String name; - private final String lastName; - - Alien(String firstName, String lastName) { - this.firstName = firstName; - this.lastName = lastName; + Alien(String name) { + this.name = name; } - String getFirstName() { - return this.firstName; - } - - String getLastName() { - return this.lastName; + String getName() { + return this.name; } } diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/ConversionServiceDeducerTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/ConversionServiceDeducerTests.java index 30286fe950b..06b27871c9e 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/ConversionServiceDeducerTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/ConversionServiceDeducerTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2021 the original author or authors. + * Copyright 2012-2023 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -30,6 +30,7 @@ import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.core.convert.ConversionService; import org.springframework.core.convert.converter.Converter; +import org.springframework.format.support.FormattingConversionService; import static org.assertj.core.api.Assertions.assertThat; @@ -69,14 +70,15 @@ class ConversionServiceDeducerTests { } @Test - void getConversionServiceWhenHasQualifiedConverterBeansContainsCustomizedApplicationService() { + void getConversionServiceWhenHasQualifiedConverterBeansContainsCustomizedFormattingService() { ApplicationContext applicationContext = new AnnotationConfigApplicationContext( CustomConverterConfiguration.class); ConversionServiceDeducer deducer = new ConversionServiceDeducer(applicationContext); List conversionServices = deducer.getConversionServices(); - assertThat(conversionServices).hasSize(1); - assertThat(conversionServices.get(0)).isNotSameAs(ApplicationConversionService.getSharedInstance()); + assertThat(conversionServices).hasSize(2); + assertThat(conversionServices.get(0)).isExactlyInstanceOf(FormattingConversionService.class); assertThat(conversionServices.get(0).canConvert(InputStream.class, OutputStream.class)).isTrue(); + assertThat(conversionServices.get(1)).isSameAs(ApplicationConversionService.getSharedInstance()); } @Configuration(proxyBeanMethods = false)