From 7f065040068681bbf8b049458d5ea59a8f814fcc Mon Sep 17 00:00:00 2001 From: Stephane Nicoll Date: Mon, 19 Aug 2019 11:01:49 +0200 Subject: [PATCH] Detect ConfigurationProperties using autowiring constructor This commit improves InvalidConfigurationPropertiesException failure analysis by adding a dedicated description when a single autowiring constructor is found. When @Component is removed, such constructor is used for properties binding and will not apply the same semantics as before. Closes gh-17750 --- ...onfigurationPropertiesFailureAnalyzer.java | 49 ++++++++++--- ...urationPropertiesFailureAnalyzerTests.java | 71 ++++++++++++++++--- 2 files changed, 102 insertions(+), 18 deletions(-) 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 index b186d83cc7..b2b5a0db39 100644 --- 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 @@ -16,6 +16,8 @@ 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; @@ -25,6 +27,7 @@ import org.springframework.boot.diagnostics.FailureAnalysis; * {@link InvalidConfigurationPropertiesException}. * * @author Madhura Bhave + * @author Stephane Nicoll * @since 2.2.0 */ public class InvalidConfigurationPropertiesFailureAnalyzer @@ -32,20 +35,46 @@ public class InvalidConfigurationPropertiesFailureAnalyzer @Override protected FailureAnalysis analyze(Throwable rootFailure, InvalidConfigurationPropertiesException cause) { - String configurationProperties = cause.getConfigurationProperties().getName(); - String component = cause.getComponent().getSimpleName(); - return new FailureAnalysis(getDescription(configurationProperties, component), - getAction(configurationProperties, component), 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 getDescription(String configurationProperties, String component) { - return configurationProperties + " is annotated with @ConfigurationProperties and @" + component - + ". This may cause the @ConfigurationProperties bean to be registered twice."; + 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 String getAction(String configurationProperties, String component) { - return "Remove @" + component + " from " + configurationProperties - + " or consider disabling automatic @ConfigurationProperties scanning."; + 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/diagnostics/analyzer/InvalidConfigurationPropertiesFailureAnalyzerTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/diagnostics/analyzer/InvalidConfigurationPropertiesFailureAnalyzerTests.java index 8416687e7f..cb66e0487d 100644 --- 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 @@ -16,8 +16,11 @@ 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; @@ -29,6 +32,7 @@ import static org.assertj.core.api.Assertions.assertThat; * Tests for {@link InvalidConfigurationPropertiesFailureAnalyzer} * * @author Madhura Bhave + * @author Stephane Nicoll */ class InvalidConfigurationPropertiesFailureAnalyzerTests { @@ -36,20 +40,47 @@ class InvalidConfigurationPropertiesFailureAnalyzerTests { @Test void analysisForInvalidConfigurationOfConfigurationProperties() { - FailureAnalysis analysis = performAnalysis(); - assertThat(analysis.getDescription()).isEqualTo(getDescription()); - assertThat(analysis.getAction()).isEqualTo("Remove @Component from " + TestProperties.class.getName() - + " or consider disabling automatic @ConfigurationProperties scanning."); + 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 getDescription() { - return TestProperties.class.getName() + " is annotated with @ConfigurationProperties and @Component" + 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 FailureAnalysis performAnalysis() { + 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(TestProperties.class, Component.class)); + .analyze(new InvalidConfigurationPropertiesException(target, Component.class)); assertThat(analysis).isNotNull(); return analysis; } @@ -60,4 +91,28 @@ class InvalidConfigurationPropertiesFailureAnalyzerTests { } + @ConfigurationProperties + @Component + static class TestPropertiesWithSingleConstructor { + + TestPropertiesWithSingleConstructor(Object firstService, Function factory) { + + } + + } + + @ConfigurationProperties + @Component + static class TestPropertiesWithSeveralConstructors { + + TestPropertiesWithSeveralConstructors() { + } + + @Autowired + TestPropertiesWithSeveralConstructors(Object someService) { + + } + + } + }