From f033016364691e50877651116e22b352d275b937 Mon Sep 17 00:00:00 2001 From: Andy Wilkinson Date: Mon, 9 Sep 2019 15:43:02 +0100 Subject: [PATCH] Allow @Component on mutable @ConfigurationProperties Update configuration properties support to allow the `@Component` annotation to be used on `@ConfigurationProperties` beans as long as they are mutable. This restores the behavior of Spring Boot 2.1 for mutable beans whilst still allowing us to enforce the stricter rules for immutable value object configuration properties. Closes gh-18138 --- .../main/asciidoc/spring-boot-features.adoc | 13 +- .../ConfigurationPropertiesScanRegistrar.java | 15 +-- ...validConfigurationPropertiesException.java | 50 -------- ...onfigurationPropertiesFailureAnalyzer.java | 80 ------------ ...igurationPropertiesScanRegistrarTests.java | 32 ++--- .../c/CombinedConfiguration.java} | 4 +- .../d/OtherCombinedConfiguration.java} | 4 +- ...urationPropertiesFailureAnalyzerTests.java | 118 ------------------ 8 files changed, 28 insertions(+), 288 deletions(-) delete mode 100644 spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/InvalidConfigurationPropertiesException.java delete mode 100644 spring-boot-project/spring-boot/src/main/java/org/springframework/boot/diagnostics/analyzer/InvalidConfigurationPropertiesFailureAnalyzer.java rename spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/scan/{invalid/c/InvalidConfiguration.java => combined/c/CombinedConfiguration.java} (89%) rename spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/scan/{invalid/d/OtherInvalidConfiguration.java => combined/d/OtherCombinedConfiguration.java} (89%) delete mode 100644 spring-boot-project/spring-boot/src/test/java/org/springframework/boot/diagnostics/analyzer/InvalidConfigurationPropertiesFailureAnalyzerTests.java diff --git a/spring-boot-project/spring-boot-docs/src/main/asciidoc/spring-boot-features.adoc b/spring-boot-project/spring-boot-docs/src/main/asciidoc/spring-boot-features.adoc index 862f36810b..db5ad2d4d3 100644 --- a/spring-boot-project/spring-boot-docs/src/main/asciidoc/spring-boot-features.adoc +++ b/spring-boot-project/spring-boot-docs/src/main/asciidoc/spring-boot-features.adoc @@ -903,6 +903,8 @@ In this setup one, and only one constructor must be defined with the list of pro Default values can be specified using `@DefaultValue` and the same conversion service will be applied to coerce the `String` value to the target type of a missing property. +NOTE: To use constructor binding the class must not be annotated with `@Component` and must be enabled using `@EnableConfigurationProperties` or configuration property scanning instead. + [[boot-features-external-config-enabling]] @@ -933,20 +935,15 @@ In these cases, you can specify the list of types to process on any `@Configurat [NOTE] ==== -When the `@ConfigurationProperties` bean is registered using scanning or via `@EnableConfigurationProperties`, the bean has a conventional name: `-`, where `` is the environment key prefix specified in the `@ConfigurationProperties` annotation and `` is the fully qualified name of the bean. +When the `@ConfigurationProperties` bean is registered using configuration property scanning or via `@EnableConfigurationProperties`, the bean has a conventional name: `-`, where `` is the environment key prefix specified in the `@ConfigurationProperties` annotation and `` is the fully qualified name of the bean. If the annotation does not provide any prefix, only the fully qualified name of the bean is used. The bean name in the example above is `acme-com.example.AcmeProperties`. ==== We recommend that `@ConfigurationProperties` only deal with the environment and, in particular, does not inject other beans from the context. -In particular, it is not possible to inject other beans using the constructor as this would trigger the constructor binder that only deals with the environment. - -For corner cases, setter injection can be used or any of the `*Aware` interfaces provided -by the framework (such as `EnvironmentAware` if you need access to the `Environment`). - -NOTE: Annotating a `@ConfigurationProperties` type with `@Component` will result in two beans of the same type if the type is also scanned as part of classpath scanning. -If you want to register the bean yourself using `@Component`, consider disabling scanning of `@ConfigurationProperties`. +For corner cases, setter injection can be used or any of the `*Aware` interfaces provided by the framework (such as `EnvironmentAware` if you need access to the `Environment`). +If you still want to inject other beans using the constructor, the configuration properties bean must be annotated with `@Component` and use JavaBean-based property binding. diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/ConfigurationPropertiesScanRegistrar.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/ConfigurationPropertiesScanRegistrar.java index d52cd0f920..cf3674ec2d 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/ConfigurationPropertiesScanRegistrar.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/ConfigurationPropertiesScanRegistrar.java @@ -28,8 +28,8 @@ import org.springframework.context.ResourceLoaderAware; import org.springframework.context.annotation.ClassPathScanningCandidateComponentProvider; import org.springframework.context.annotation.ImportBeanDefinitionRegistrar; import org.springframework.core.annotation.AnnotationAttributes; -import org.springframework.core.annotation.MergedAnnotation; import org.springframework.core.annotation.MergedAnnotations; +import org.springframework.core.annotation.MergedAnnotations.SearchStrategy; import org.springframework.core.env.Environment; import org.springframework.core.io.ResourceLoader; import org.springframework.core.type.AnnotationMetadata; @@ -99,8 +99,9 @@ class ConfigurationPropertiesScanRegistrar String beanClassName = candidate.getBeanClassName(); try { Class type = ClassUtils.forName(beanClassName, null); - validateScanConfiguration(type); - ConfigurationPropertiesBeanDefinitionRegistrar.register(registry, beanFactory, type); + if (!isComponent(type)) { + ConfigurationPropertiesBeanDefinitionRegistrar.register(registry, beanFactory, type); + } } catch (ClassNotFoundException ex) { // Ignore @@ -108,12 +109,8 @@ class ConfigurationPropertiesScanRegistrar } } - private void validateScanConfiguration(Class type) { - MergedAnnotation component = MergedAnnotations - .from(type, MergedAnnotations.SearchStrategy.TYPE_HIERARCHY).get(Component.class); - if (component.isPresent()) { - throw new InvalidConfigurationPropertiesException(type, component.getRoot().getType()); - } + private boolean isComponent(Class type) { + return MergedAnnotations.from(type, SearchStrategy.TYPE_HIERARCHY).isPresent(Component.class); } @Override diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/InvalidConfigurationPropertiesException.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/InvalidConfigurationPropertiesException.java deleted file mode 100644 index 74cba661a2..0000000000 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/InvalidConfigurationPropertiesException.java +++ /dev/null @@ -1,50 +0,0 @@ -/* - * Copyright 2012-2019 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. - * You may obtain a copy of the License at - * - * https://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package org.springframework.boot.context.properties; - -import org.springframework.util.Assert; - -/** - * Exception thrown when a {@link ConfigurationProperties @ConfigurationProperties} has - * been misconfigured. - * - * @author Madhura Bhave - * @since 2.2.0 - */ -public class InvalidConfigurationPropertiesException extends RuntimeException { - - private final Class configurationProperties; - - private final Class component; - - public InvalidConfigurationPropertiesException(Class configurationProperties, Class component) { - super("Found @" + component.getSimpleName() + " and @ConfigurationProperties on " - + configurationProperties.getName() + "."); - Assert.notNull(configurationProperties, "Class must not be null"); - this.configurationProperties = configurationProperties; - this.component = component; - } - - public Class getConfigurationProperties() { - return this.configurationProperties; - } - - public Class getComponent() { - return this.component; - } - -} diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/diagnostics/analyzer/InvalidConfigurationPropertiesFailureAnalyzer.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/diagnostics/analyzer/InvalidConfigurationPropertiesFailureAnalyzer.java deleted file mode 100644 index b2b5a0db39..0000000000 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/diagnostics/analyzer/InvalidConfigurationPropertiesFailureAnalyzer.java +++ /dev/null @@ -1,80 +0,0 @@ -/* - * Copyright 2012-2019 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. - * You may obtain a copy of the License at - * - * https://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package org.springframework.boot.diagnostics.analyzer; - -import java.lang.reflect.Constructor; - -import org.springframework.boot.context.properties.InvalidConfigurationPropertiesException; -import org.springframework.boot.diagnostics.AbstractFailureAnalyzer; -import org.springframework.boot.diagnostics.FailureAnalysis; - -/** - * An {@link AbstractFailureAnalyzer} that performs analysis of failures caused by - * {@link InvalidConfigurationPropertiesException}. - * - * @author Madhura Bhave - * @author Stephane Nicoll - * @since 2.2.0 - */ -public class InvalidConfigurationPropertiesFailureAnalyzer - extends AbstractFailureAnalyzer { - - @Override - protected FailureAnalysis analyze(Throwable rootFailure, InvalidConfigurationPropertiesException cause) { - Class target = cause.getConfigurationProperties(); - Constructor autowiringConstructor = getAutowiringConstructor(target); - String componentName = cause.getComponent().getSimpleName(); - return new FailureAnalysis(getDescription(target, autowiringConstructor, componentName), - getAction(target, autowiringConstructor, componentName), cause); - } - - private String getDescription(Class target, Constructor autowiringConstructor, String componentName) { - String targetName = target.getSimpleName(); - StringBuilder sb = new StringBuilder(targetName); - sb.append(" is annotated with @ConfigurationProperties and @").append(componentName) - .append(". This may cause the @ConfigurationProperties bean to be registered twice."); - if (autowiringConstructor != null) { - sb.append(" Also, autowiring by constructor is enabled for ").append(targetName) - .append(" which conflicts with properties constructor binding."); - } - return sb.toString(); - } - - private String getAction(Class target, Constructor autowiringConstructor, String componentName) { - StringBuilder sb = new StringBuilder(); - if (autowiringConstructor != null) { - sb.append("Consider refactoring ").append(target.getSimpleName()).append( - " so that it does not rely on other beans. Alternatively, a default constructor should be added and @Autowired should be defined on ") - .append(autowiringConstructor.toGenericString()).append(String.format(".%n%n")); - } - sb.append("Remove @").append(componentName).append(" from ").append(target.getName()) - .append(" or consider disabling automatic @ConfigurationProperties scanning."); - return sb.toString(); - } - - private Constructor getAutowiringConstructor(Class target) { - Constructor[] candidates = target.getDeclaredConstructors(); - if (candidates.length == 1) { - Constructor candidate = candidates[0]; - if (candidate.getParameterCount() > 0) { - return candidate; - } - } - return null; - } - -} diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/ConfigurationPropertiesScanRegistrarTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/ConfigurationPropertiesScanRegistrarTests.java index 099ba88b78..926377af21 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/ConfigurationPropertiesScanRegistrarTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/ConfigurationPropertiesScanRegistrarTests.java @@ -23,15 +23,14 @@ import org.junit.jupiter.api.Test; import org.springframework.beans.factory.config.BeanDefinition; import org.springframework.beans.factory.support.DefaultListableBeanFactory; import org.springframework.beans.factory.support.GenericBeanDefinition; -import org.springframework.boot.context.properties.scan.invalid.c.InvalidConfiguration; -import org.springframework.boot.context.properties.scan.invalid.d.OtherInvalidConfiguration; +import org.springframework.boot.context.properties.scan.combined.c.CombinedConfiguration; +import org.springframework.boot.context.properties.scan.combined.d.OtherCombinedConfiguration; import org.springframework.boot.context.properties.scan.valid.ConfigurationPropertiesScanConfiguration; import org.springframework.core.type.AnnotationMetadata; import org.springframework.core.type.classreading.SimpleMetadataReaderFactory; import org.springframework.mock.env.MockEnvironment; import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.assertThatExceptionOfType; /** * Tests for {@link ConfigurationPropertiesScanRegistrar}. @@ -99,38 +98,33 @@ class ConfigurationPropertiesScanRegistrarTests { } @Test - void scanWhenComponentAnnotationPresentShouldThrowException() { + void scanWhenComponentAnnotationPresentShouldSkipType() throws IOException { DefaultListableBeanFactory beanFactory = new DefaultListableBeanFactory(); beanFactory.setAllowBeanDefinitionOverriding(false); - assertThatExceptionOfType(InvalidConfigurationPropertiesException.class) - .isThrownBy(() -> this.registrar - .registerBeanDefinitions(getAnnotationMetadata(InvalidScanConfiguration.class), beanFactory)) - .withMessageContaining( - "Found @Component and @ConfigurationProperties on org.springframework.boot.context.properties.scan.invalid.c.InvalidConfiguration$MyProperties."); + this.registrar.registerBeanDefinitions(getAnnotationMetadata(CombinedScanConfiguration.class), beanFactory); + assertThat(beanFactory.getBeanDefinitionCount()).isEqualTo(0); } @Test - void scanWhenOtherComponentAnnotationPresentShouldThrowException() { + void scanWhenOtherComponentAnnotationPresentShouldSkipType() throws IOException { DefaultListableBeanFactory beanFactory = new DefaultListableBeanFactory(); beanFactory.setAllowBeanDefinitionOverriding(false); - assertThatExceptionOfType(InvalidConfigurationPropertiesException.class) - .isThrownBy(() -> this.registrar.registerBeanDefinitions( - getAnnotationMetadata(OtherInvalidScanConfiguration.class), beanFactory)) - .withMessageContaining( - "Found @RestController and @ConfigurationProperties on org.springframework.boot.context.properties.scan.invalid.d.OtherInvalidConfiguration$MyControllerProperties."); + this.registrar.registerBeanDefinitions(getAnnotationMetadata(OtherCombinedScanConfiguration.class), + beanFactory); + assertThat(beanFactory.getBeanDefinitionCount()).isEqualTo(0); } private AnnotationMetadata getAnnotationMetadata(Class source) throws IOException { return new SimpleMetadataReaderFactory().getMetadataReader(source.getName()).getAnnotationMetadata(); } - @ConfigurationPropertiesScan(basePackageClasses = InvalidConfiguration.class) - static class InvalidScanConfiguration { + @ConfigurationPropertiesScan(basePackageClasses = CombinedConfiguration.class) + static class CombinedScanConfiguration { } - @ConfigurationPropertiesScan(basePackageClasses = OtherInvalidConfiguration.class) - static class OtherInvalidScanConfiguration { + @ConfigurationPropertiesScan(basePackageClasses = OtherCombinedConfiguration.class) + static class OtherCombinedScanConfiguration { } diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/scan/invalid/c/InvalidConfiguration.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/scan/combined/c/CombinedConfiguration.java similarity index 89% rename from spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/scan/invalid/c/InvalidConfiguration.java rename to spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/scan/combined/c/CombinedConfiguration.java index b04f9cba91..70519f4672 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/scan/invalid/c/InvalidConfiguration.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/scan/combined/c/CombinedConfiguration.java @@ -13,7 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package org.springframework.boot.context.properties.scan.invalid.c; +package org.springframework.boot.context.properties.scan.combined.c; import org.springframework.boot.context.properties.ConfigurationProperties; import org.springframework.stereotype.Component; @@ -21,7 +21,7 @@ import org.springframework.stereotype.Component; /** * @author Madhura Bhave */ -public class InvalidConfiguration { +public class CombinedConfiguration { @Component @ConfigurationProperties(prefix = "b") diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/scan/invalid/d/OtherInvalidConfiguration.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/scan/combined/d/OtherCombinedConfiguration.java similarity index 89% rename from spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/scan/invalid/d/OtherInvalidConfiguration.java rename to spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/scan/combined/d/OtherCombinedConfiguration.java index af63828703..f39a4546bb 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/scan/invalid/d/OtherInvalidConfiguration.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/scan/combined/d/OtherCombinedConfiguration.java @@ -14,7 +14,7 @@ * limitations under the License. */ -package org.springframework.boot.context.properties.scan.invalid.d; +package org.springframework.boot.context.properties.scan.combined.d; import org.springframework.boot.context.properties.ConfigurationProperties; import org.springframework.web.bind.annotation.RestController; @@ -22,7 +22,7 @@ import org.springframework.web.bind.annotation.RestController; /** * @author Madhura Bhave */ -public class OtherInvalidConfiguration { +public class OtherCombinedConfiguration { @RestController @ConfigurationProperties(prefix = "c") diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/diagnostics/analyzer/InvalidConfigurationPropertiesFailureAnalyzerTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/diagnostics/analyzer/InvalidConfigurationPropertiesFailureAnalyzerTests.java deleted file mode 100644 index cb66e0487d..0000000000 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/diagnostics/analyzer/InvalidConfigurationPropertiesFailureAnalyzerTests.java +++ /dev/null @@ -1,118 +0,0 @@ -/* - * Copyright 2012-2019 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. - * You may obtain a copy of the License at - * - * https://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package org.springframework.boot.diagnostics.analyzer; - -import java.util.function.Function; - -import org.junit.jupiter.api.Test; - -import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.boot.context.properties.ConfigurationProperties; -import org.springframework.boot.context.properties.InvalidConfigurationPropertiesException; -import org.springframework.boot.diagnostics.FailureAnalysis; -import org.springframework.stereotype.Component; - -import static org.assertj.core.api.Assertions.assertThat; - -/** - * Tests for {@link InvalidConfigurationPropertiesFailureAnalyzer} - * - * @author Madhura Bhave - * @author Stephane Nicoll - */ -class InvalidConfigurationPropertiesFailureAnalyzerTests { - - private final InvalidConfigurationPropertiesFailureAnalyzer analyzer = new InvalidConfigurationPropertiesFailureAnalyzer(); - - @Test - void analysisForInvalidConfigurationOfConfigurationProperties() { - FailureAnalysis analysis = performAnalysis(TestProperties.class); - assertThat(analysis.getDescription()).isEqualTo(getBasicDescription(TestProperties.class)); - assertThat(analysis.getAction()).isEqualTo(getBasicAction(TestProperties.class)); - } - - @Test - void analysisForInvalidConfigurationOfConfigurationPropertiesWithSingleConstructor() { - FailureAnalysis analysis = performAnalysis(TestPropertiesWithSingleConstructor.class); - assertThat(analysis.getDescription()).containsSequence( - getBasicDescription(TestPropertiesWithSingleConstructor.class), - " Also, autowiring by constructor is enabled for " - + TestPropertiesWithSingleConstructor.class.getSimpleName() - + " which conflicts with properties constructor binding."); - assertThat(analysis.getAction()).containsSubsequence("Consider refactoring TestPropertiesWithSingleConstructor " - + "so that it does not rely on other beans. Alternatively, a default constructor should be added and " - + "@Autowired should be defined on " - + "org.springframework.boot.diagnostics.analyzer.InvalidConfigurationPropertiesFailureAnalyzerTests$TestPropertiesWithSingleConstructor(java.lang.Object,java.util.function.Function).", - getBasicAction(TestPropertiesWithSingleConstructor.class)); - } - - @Test - void analysisForInvalidConfigurationOfConfigurationPropertiesWithSeveralConstructors() { - FailureAnalysis analysis = performAnalysis(TestPropertiesWithSeveralConstructors.class); - assertThat(analysis.getDescription()) - .isEqualTo(getBasicDescription(TestPropertiesWithSeveralConstructors.class)); - assertThat(analysis.getAction()).isEqualTo(getBasicAction(TestPropertiesWithSeveralConstructors.class)); - } - - private String getBasicDescription(Class target) { - return target.getSimpleName() + " is annotated with @ConfigurationProperties and @Component" - + ". This may cause the @ConfigurationProperties bean to be registered twice."; - } - - private String getBasicAction(Class target) { - return "Remove @Component from " + target.getName() - + " or consider disabling automatic @ConfigurationProperties scanning."; - } - - private FailureAnalysis performAnalysis(Class target) { - FailureAnalysis analysis = this.analyzer - .analyze(new InvalidConfigurationPropertiesException(target, Component.class)); - assertThat(analysis).isNotNull(); - return analysis; - } - - @ConfigurationProperties - @Component - static class TestProperties { - - } - - @ConfigurationProperties - @Component - static class TestPropertiesWithSingleConstructor { - - TestPropertiesWithSingleConstructor(Object firstService, Function factory) { - - } - - } - - @ConfigurationProperties - @Component - static class TestPropertiesWithSeveralConstructors { - - TestPropertiesWithSeveralConstructors() { - } - - @Autowired - TestPropertiesWithSeveralConstructors(Object someService) { - - } - - } - -}