From 7c7bb531b45edda5621dd1acb6a1e47491a2e881 Mon Sep 17 00:00:00 2001 From: Andy Wilkinson Date: Wed, 4 Dec 2024 13:21:29 +0000 Subject: [PATCH] Improve handling of environment variables in failure analysis Prior to this change, the failure analysis for an invalid configuration property value filtered out the configuration property sources property source. This property source contains a "duplicate" of all of the environment's other property sources but with configuration property support (such as relaxed/fuzzy matching of environment variables). This was done to prevent the reporting of duplicates when a property was found in both the configuration property sources property source and the "normal" property sources. An unwanted side-effect of this was that fuzzy matching of environment variables was lost so the origin of com.example.some-property would be found in the environment variable was COM_EXAMPLE_SOME_PROPERTY but would not be found if it was COM_EXAMPLE_SOMEPROPERTY. This commit addresses this side-effect by no longer filtering out the configuration property sources property source. To then prevent duplicates from being reported in the analysis, it instead deduplicates things based on the origin of each property that's found in the environment's property sources. Fixes gh-43380 --- ...igurationPropertyValueFailureAnalyzer.java | 21 +++-- ...tionPropertyValueFailureAnalyzerTests.java | 83 ++++++++++++++++--- 2 files changed, 87 insertions(+), 17 deletions(-) diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/diagnostics/analyzer/InvalidConfigurationPropertyValueFailureAnalyzer.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/diagnostics/analyzer/InvalidConfigurationPropertyValueFailureAnalyzer.java index 9e2bd7a235..048024a447 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/diagnostics/analyzer/InvalidConfigurationPropertyValueFailureAnalyzer.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/diagnostics/analyzer/InvalidConfigurationPropertyValueFailureAnalyzer.java @@ -16,16 +16,18 @@ package org.springframework.boot.diagnostics.analyzer; +import java.util.HashSet; import java.util.List; +import java.util.Set; import java.util.stream.Stream; -import org.springframework.boot.context.properties.source.ConfigurationPropertySources; import org.springframework.boot.context.properties.source.InvalidConfigurationPropertyValueException; import org.springframework.boot.diagnostics.AbstractFailureAnalyzer; import org.springframework.boot.diagnostics.FailureAnalysis; import org.springframework.boot.diagnostics.FailureAnalyzer; import org.springframework.boot.origin.Origin; import org.springframework.boot.origin.OriginLookup; +import org.springframework.boot.origin.PropertySourceOrigin; import org.springframework.core.env.ConfigurableEnvironment; import org.springframework.core.env.Environment; import org.springframework.core.env.PropertySource; @@ -61,18 +63,23 @@ class InvalidConfigurationPropertyValueFailureAnalyzer } private List getDescriptors(String propertyName) { + Set seen = new HashSet<>(); return getPropertySources().filter((source) -> source.containsProperty(propertyName)) .map((source) -> Descriptor.get(source, propertyName)) + .filter((descriptor) -> seen.add(getOrigin(descriptor))) .toList(); } - private Stream> getPropertySources() { - if (this.environment == null) { - return Stream.empty(); + private Origin getOrigin(Descriptor descriptor) { + Origin origin = descriptor.origin; + if (origin instanceof PropertySourceOrigin propertySourceOrigin) { + origin = propertySourceOrigin.getOrigin(); } - return this.environment.getPropertySources() - .stream() - .filter((source) -> !ConfigurationPropertySources.isAttachedConfigurationPropertySource(source)); + return origin; + } + + private Stream> getPropertySources() { + return (this.environment != null) ? this.environment.getPropertySources().stream() : Stream.empty(); } private void appendDetails(StringBuilder message, InvalidConfigurationPropertyValueException cause, diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/diagnostics/analyzer/InvalidConfigurationPropertyValueFailureAnalyzerTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/diagnostics/analyzer/InvalidConfigurationPropertyValueFailureAnalyzerTests.java index d50cd24c28..df2b18b02d 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/diagnostics/analyzer/InvalidConfigurationPropertyValueFailureAnalyzerTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/diagnostics/analyzer/InvalidConfigurationPropertyValueFailureAnalyzerTests.java @@ -20,13 +20,16 @@ import java.util.Collections; import org.junit.jupiter.api.Test; +import org.springframework.boot.context.properties.source.ConfigurationPropertySources; import org.springframework.boot.context.properties.source.InvalidConfigurationPropertyValueException; import org.springframework.boot.diagnostics.FailureAnalysis; import org.springframework.boot.origin.Origin; import org.springframework.boot.origin.OriginLookup; import org.springframework.core.env.EnumerablePropertySource; import org.springframework.core.env.MapPropertySource; +import org.springframework.core.env.SystemEnvironmentPropertySource; import org.springframework.mock.env.MockEnvironment; +import org.springframework.util.ObjectUtils; import static org.assertj.core.api.Assertions.assertThat; @@ -63,6 +66,26 @@ class InvalidConfigurationPropertyValueFailureAnalyzerTests { .doesNotContain("Additionally, this property is also set"); } + @Test + void analysisWithKnownPropertyFromSystemEnvironment() { + MapPropertySource source = new SystemEnvironmentPropertySource("systemEnvironment", + Collections.singletonMap("COM_EXAMPLE_TESTPROPERTY", "invalid")); + this.environment.getPropertySources().addFirst(source); + ConfigurationPropertySources.attach(this.environment); + assertThat(this.environment.getProperty("com.example.test-property")).isEqualTo("invalid"); + InvalidConfigurationPropertyValueException failure = new InvalidConfigurationPropertyValueException( + "com.example.test-property", "invalid", "This is not valid."); + FailureAnalysis analysis = performAnalysis(failure); + assertThat(analysis.getDescription()).contains("com.example.test-property") + .contains("invalid") + .contains("property source \"systemEnvironment\""); + assertThat(analysis.getCause()).isSameAs(failure); + assertThat(analysis.getAction()).contains("Review the value of the property with the provided reason."); + assertThat(analysis.getDescription()).contains("Validation failed for the following reason") + .contains("This is not valid.") + .doesNotContain("Additionally, this property is also set"); + } + @Test void analysisWithKnownPropertyAndNoReason() { MapPropertySource source = new MapPropertySource("test", Collections.singletonMap("test.property", "invalid")); @@ -84,6 +107,8 @@ class InvalidConfigurationPropertyValueFailureAnalyzerTests { this.environment.getPropertySources().addFirst(OriginCapablePropertySource.get(source)); this.environment.getPropertySources().addLast(additional); this.environment.getPropertySources().addLast(OriginCapablePropertySource.get(another)); + this.environment.getPropertySources().addLast(OriginCapablePropertySource.get("another-again", another)); + ConfigurationPropertySources.attach(this.environment); InvalidConfigurationPropertyValueException failure = new InvalidConfigurationPropertyValueException( "test.property", "invalid", "This is not valid."); FailureAnalysis analysis = performAnalysis(failure); @@ -92,7 +117,8 @@ class InvalidConfigurationPropertyValueFailureAnalyzerTests { assertThat(analysis.getDescription()) .contains("Additionally, this property is also set in the following property sources:") .contains("In 'additional' with the value 'valid'") - .contains("In 'another' with the value 'test' (originating from 'TestOrigin test.property')"); + .contains("In 'another' with the value 'test' (originating from 'TestOrigin test.property')") + .doesNotContain("another-again"); } @Test @@ -122,7 +148,11 @@ class InvalidConfigurationPropertyValueFailureAnalyzerTests { private final EnumerablePropertySource propertySource; OriginCapablePropertySource(EnumerablePropertySource propertySource) { - super(propertySource.getName(), propertySource.getSource()); + this(propertySource.getName(), propertySource); + } + + OriginCapablePropertySource(String name, EnumerablePropertySource propertySource) { + super(name, propertySource.getSource()); this.propertySource = propertySource; } @@ -138,20 +168,53 @@ class InvalidConfigurationPropertyValueFailureAnalyzerTests { @Override public Origin getOrigin(String name) { - return new Origin() { - - @Override - public String toString() { - return "TestOrigin " + name; - } - - }; + return new TestOrigin(name, this.propertySource.getName()); } static OriginCapablePropertySource get(EnumerablePropertySource propertySource) { return new OriginCapablePropertySource<>(propertySource); } + static OriginCapablePropertySource get(String name, EnumerablePropertySource propertySource) { + return new OriginCapablePropertySource<>(name, propertySource); + } + + static final class TestOrigin implements Origin { + + private final String name; + + private final String sourceName; + + private TestOrigin(String name, String sourceName) { + this.name = name; + this.sourceName = sourceName; + } + + @Override + public boolean equals(Object obj) { + if (this == obj) { + return true; + } + if (obj == null || getClass() != obj.getClass()) { + return false; + } + TestOrigin other = (TestOrigin) obj; + return ObjectUtils.nullSafeEquals(this.name, other.name) + && ObjectUtils.nullSafeEquals(this.sourceName, other.sourceName); + } + + @Override + public int hashCode() { + return ObjectUtils.nullSafeHashCode(this.name); + } + + @Override + public String toString() { + return "TestOrigin " + this.name; + } + + } + } }