diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/ConfigurationPropertiesBeanDefinitionValidator.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/ConfigurationPropertiesBeanDefinitionValidator.java deleted file mode 100644 index 3b34b4da59..0000000000 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/ConfigurationPropertiesBeanDefinitionValidator.java +++ /dev/null @@ -1,91 +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.beans.BeansException; -import org.springframework.beans.factory.BeanCreationException; -import org.springframework.beans.factory.CannotLoadBeanClassException; -import org.springframework.beans.factory.config.BeanDefinition; -import org.springframework.beans.factory.config.BeanFactoryPostProcessor; -import org.springframework.beans.factory.config.ConfigurableListableBeanFactory; -import org.springframework.beans.factory.support.BeanDefinitionRegistry; -import org.springframework.beans.factory.support.GenericBeanDefinition; -import org.springframework.boot.context.properties.ConfigurationPropertiesBean.BindMethod; -import org.springframework.core.Ordered; -import org.springframework.util.Assert; - -/** - * {@link BeanFactoryPostProcessor} to validate that regular bean definitions aren't - * creating {@link ConstructorBinding} beans. - * - * @author Phillip Webb - */ -class ConfigurationPropertiesBeanDefinitionValidator implements BeanFactoryPostProcessor, Ordered { - - private static final String BEAN_NAME = ConfigurationPropertiesBeanDefinitionValidator.class.getName(); - - @Override - public void postProcessBeanFactory(ConfigurableListableBeanFactory beanFactory) throws BeansException { - for (String beanName : beanFactory.getBeanDefinitionNames()) { - if (!(beanFactory.containsSingleton(beanName) || isValueObjectBeanDefinition(beanFactory, beanName))) { - validate(beanFactory, beanName); - } - } - } - - private boolean isValueObjectBeanDefinition(ConfigurableListableBeanFactory beanFactory, String beanName) { - BeanDefinition definition = beanFactory.getBeanDefinition(beanName); - return (definition instanceof ConfigurationPropertiesValueObjectBeanDefinition); - } - - @Override - public int getOrder() { - return Ordered.LOWEST_PRECEDENCE; - } - - private void validate(ConfigurableListableBeanFactory beanFactory, String beanName) { - try { - Class beanClass = beanFactory.getType(beanName, false); - if (beanClass != null && BindMethod.forType(beanClass) == BindMethod.VALUE_OBJECT) { - throw new BeanCreationException(beanName, - "@EnableConfigurationProperties or @ConfigurationPropertiesScan must be used to add " - + "@ConstructorBinding type " + beanClass.getName()); - } - } - catch (CannotLoadBeanClassException ex) { - // Ignore - } - - } - - /** - * Register a {@link ConfigurationPropertiesBeanDefinitionValidator} bean if one is - * not already registered. - * @param registry the bean definition registry - */ - static void register(BeanDefinitionRegistry registry) { - Assert.notNull(registry, "Registry must not be null"); - if (!registry.containsBeanDefinition(BEAN_NAME)) { - GenericBeanDefinition definition = new GenericBeanDefinition(); - definition.setBeanClass(ConfigurationPropertiesBeanDefinitionValidator.class); - definition.setRole(BeanDefinition.ROLE_INFRASTRUCTURE); - registry.registerBeanDefinition(BEAN_NAME, definition); - } - ConfigurationPropertiesBinder.register(registry); - } - -} diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/EnableConfigurationPropertiesRegistrar.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/EnableConfigurationPropertiesRegistrar.java index f5f14cea14..20d3b9e7f9 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/EnableConfigurationPropertiesRegistrar.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/EnableConfigurationPropertiesRegistrar.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2019 the original author or authors. + * Copyright 2012-2020 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. @@ -49,7 +49,6 @@ class EnableConfigurationPropertiesRegistrar implements ImportBeanDefinitionRegi @SuppressWarnings("deprecation") static void registerInfrastructureBeans(BeanDefinitionRegistry registry) { ConfigurationPropertiesBindingPostProcessor.register(registry); - ConfigurationPropertiesBeanDefinitionValidator.register(registry); ConfigurationBeanFactoryMetadata.register(registry); } diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/NotConstructorBoundInjectionFailureAnalyzer.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/NotConstructorBoundInjectionFailureAnalyzer.java new file mode 100644 index 0000000000..7f5944999f --- /dev/null +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/NotConstructorBoundInjectionFailureAnalyzer.java @@ -0,0 +1,83 @@ +/* + * Copyright 2012-2020 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 java.lang.reflect.Constructor; + +import org.springframework.beans.factory.InjectionPoint; +import org.springframework.beans.factory.NoSuchBeanDefinitionException; +import org.springframework.beans.factory.UnsatisfiedDependencyException; +import org.springframework.boot.context.properties.ConfigurationPropertiesBean.BindMethod; +import org.springframework.boot.diagnostics.FailureAnalysis; +import org.springframework.boot.diagnostics.analyzer.AbstractInjectionFailureAnalyzer; +import org.springframework.core.Ordered; +import org.springframework.core.annotation.MergedAnnotation; +import org.springframework.core.annotation.MergedAnnotations; + +/** + * An {@AbstractInjectionFailureAnalyzer} for + * {@link ConfigurationProperties @ConfigurationProperties} that are intended to use + * {@link ConstructorBinding constructor binding} but did not. + * + * @author Andy Wilkinson + */ +class NotConstructorBoundInjectionFailureAnalyzer + extends AbstractInjectionFailureAnalyzer implements Ordered { + + @Override + public int getOrder() { + return 0; + } + + @Override + protected FailureAnalysis analyze(Throwable rootFailure, NoSuchBeanDefinitionException cause, String description) { + InjectionPoint injectionPoint = findInjectionPoint(rootFailure); + if (isConstructorBindingConfigurationProperties(injectionPoint)) { + String simpleName = injectionPoint.getMember().getDeclaringClass().getSimpleName(); + String action = String.format("Update your configuration so that " + simpleName + " is defined via @" + + ConfigurationPropertiesScan.class.getSimpleName() + " or @" + + EnableConfigurationProperties.class.getSimpleName() + ".", simpleName); + return new FailureAnalysis( + simpleName + " is annotated with @" + ConstructorBinding.class.getSimpleName() + + " but it is defined as a regular bean which caused dependency injection to fail.", + action, cause); + } + return null; + } + + private boolean isConstructorBindingConfigurationProperties(InjectionPoint injectionPoint) { + if (injectionPoint != null && injectionPoint.getMember() instanceof Constructor) { + Constructor constructor = (Constructor) injectionPoint.getMember(); + Class declaringClass = constructor.getDeclaringClass(); + MergedAnnotation configurationProperties = MergedAnnotations.from(declaringClass) + .get(ConfigurationProperties.class); + return configurationProperties.isPresent() + && BindMethod.forType(constructor.getDeclaringClass()) == BindMethod.VALUE_OBJECT; + } + return false; + } + + private InjectionPoint findInjectionPoint(Throwable failure) { + UnsatisfiedDependencyException unsatisfiedDependencyException = findCause(failure, + UnsatisfiedDependencyException.class); + if (unsatisfiedDependencyException == null) { + return null; + } + return unsatisfiedDependencyException.getInjectionPoint(); + } + +} diff --git a/spring-boot-project/spring-boot/src/main/resources/META-INF/spring.factories b/spring-boot-project/spring-boot/src/main/resources/META-INF/spring.factories index e4d3047264..3228a4a7b4 100644 --- a/spring-boot-project/spring-boot/src/main/resources/META-INF/spring.factories +++ b/spring-boot-project/spring-boot/src/main/resources/META-INF/spring.factories @@ -41,6 +41,7 @@ org.springframework.boot.reactor.DebugAgentEnvironmentPostProcessor # Failure Analyzers org.springframework.boot.diagnostics.FailureAnalyzer=\ +org.springframework.boot.context.properties.NotConstructorBoundInjectionFailureAnalyzer,\ org.springframework.boot.diagnostics.analyzer.BeanCurrentlyInCreationFailureAnalyzer,\ org.springframework.boot.diagnostics.analyzer.BeanDefinitionOverrideFailureAnalyzer,\ org.springframework.boot.diagnostics.analyzer.BeanNotOfRequiredTypeFailureAnalyzer,\ 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 fdb675d57c..3a455d4ec3 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 @@ -1,5 +1,5 @@ /* - * Copyright 2012-2019 the original author or authors. + * Copyright 2012-2020 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. @@ -58,7 +58,6 @@ import org.springframework.boot.testsupport.system.OutputCaptureExtension; import org.springframework.context.annotation.AnnotationConfigApplicationContext; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; -import org.springframework.context.annotation.Import; import org.springframework.context.annotation.ImportResource; import org.springframework.context.annotation.Scope; import org.springframework.context.support.PropertySourcesPlaceholderConfigurer; @@ -762,13 +761,6 @@ class ConfigurationPropertiesTests { assertThat(bean.getBar()).isEqualTo(5); } - @Test // gh-17831 - void loadWhenBindingConstructorParametersViaImportShouldThrowException() { - assertThatExceptionOfType(BeanCreationException.class) - .isThrownBy(() -> load(ImportConstructorParameterPropertiesConfiguration.class)) - .withMessageContaining("@EnableConfigurationProperties or @ConfigurationPropertiesScan must be used"); - } - @Test void loadWhenBindingToConstructorParametersWithDefaultValuesShouldBind() { load(ConstructorParameterConfiguration.class); @@ -1925,13 +1917,6 @@ class ConfigurationPropertiesTests { } - @Configuration(proxyBeanMethods = false) - @EnableConfigurationProperties - @Import(ConstructorParameterProperties.class) - static class ImportConstructorParameterPropertiesConfiguration { - - } - @ConstructorBinding @ConfigurationProperties(prefix = "test") @Validated diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/NotConstructorBoundInjectionFailureAnalyzerTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/NotConstructorBoundInjectionFailureAnalyzerTests.java new file mode 100644 index 0000000000..d06d547150 --- /dev/null +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/NotConstructorBoundInjectionFailureAnalyzerTests.java @@ -0,0 +1,125 @@ +/* + * Copyright 2012-2020 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.junit.jupiter.api.Test; + +import org.springframework.beans.FatalBeanException; +import org.springframework.boot.diagnostics.FailureAnalysis; +import org.springframework.boot.diagnostics.LoggingFailureAnalysisReporter; +import org.springframework.context.annotation.AnnotationConfigApplicationContext; +import org.springframework.context.annotation.Configuration; +import org.springframework.context.annotation.Import; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Tests for {@link NotConstructorBoundInjectionFailureAnalyzer}. + * + * @author Andy Wilkinson + */ +class NotConstructorBoundInjectionFailureAnalyzerTests { + + private final NotConstructorBoundInjectionFailureAnalyzer analyzer = new NotConstructorBoundInjectionFailureAnalyzer(); + + @Test + void failureAnalysisForConfigurationPropertiesThatShouldHaveBeenConstructorBound() { + FailureAnalysis analysis = analyzeFailure( + createFailure(ShouldHaveUsedConstructorBindingPropertiesConfiguration.class)); + assertThat(analysis.getDescription()).isEqualTo(ConstructorBoundProperties.class.getSimpleName() + + " is annotated with @" + ConstructorBinding.class.getSimpleName() + + " but it is defined as a regular bean which caused dependency injection to fail."); + assertThat(analysis.getAction()) + .isEqualTo("Update your configuration so that " + ConstructorBoundProperties.class.getSimpleName() + + " is defined via @" + ConfigurationPropertiesScan.class.getSimpleName() + " or @" + + EnableConfigurationProperties.class.getSimpleName() + "."); + } + + @Test + void failureAnaylsisForNonConstructorBoundProperties() { + FailureAnalysis analysis = analyzeFailure(createFailure(JavaBeanBoundPropertiesConfiguration.class)); + assertThat(analysis).isNull(); + } + + private FatalBeanException createFailure(Class config) { + try (AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext()) { + context.register(config); + context.refresh(); + return null; + } + catch (FatalBeanException ex) { + return ex; + } + } + + private FailureAnalysis analyzeFailure(Exception failure) { + assertThat(failure).isNotNull(); + FailureAnalysis analysis = this.analyzer.analyze(failure); + if (analysis != null) { + new LoggingFailureAnalysisReporter().report(analysis); + } + return analysis; + } + + @ConstructorBinding + @ConfigurationProperties("test") + static class ConstructorBoundProperties { + + private final String name; + + ConstructorBoundProperties(String name) { + this.name = name; + } + + String getName() { + return this.name; + } + + } + + @Configuration(proxyBeanMethods = false) + @Import(ConstructorBoundProperties.class) + static class ShouldHaveUsedConstructorBindingPropertiesConfiguration { + + } + + @ConfigurationProperties("test") + static class JavaBeanBoundProperties { + + private String name; + + JavaBeanBoundProperties(String dependency) { + + } + + String getName() { + return this.name; + } + + void setName(String name) { + this.name = name; + } + + } + + @Configuration(proxyBeanMethods = false) + @Import(JavaBeanBoundProperties.class) + static class JavaBeanBoundPropertiesConfiguration { + + } + +}