From 06b1b453a6561abfcdf50cf182c8bb6965362d6d Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Mon, 6 Nov 2017 13:12:25 -0800 Subject: [PATCH] Limit when SystemEnvironment mapping is used Update `SpringConfigurationPropertySource` so that the `SystemEnvironmentPropertyMapper` is only used for the "actual" system environment property source. This allows SystemEnvironmentProperySource class to be used for other purposes (for example, Spring Cloud uses it to as an override source providing decryption). Only property sources named `systemEnvironment` or ending with `-systemEnvironment` now have the `SystemEnvironmentPropertyMapper` applied. The `TestPropertyValues` has been retrofitted to name the source it adds appropriately. Fixes gh-10840 --- .../info/EnvironmentInfoContributorTests.java | 2 +- .../boot/test/util/TestPropertyValues.java | 19 ++++++++---- .../test/util/TestPropertyValuesTests.java | 6 ++-- .../SpringConfigurationPropertySource.java | 11 ++++++- .../ConfigurationPropertiesBinderTests.java | 4 ++- ...ackCompatibiltyBinderIntegrationTests.java | 27 +++++++++++++++-- ...ringConfigurationPropertySourcesTests.java | 30 ++++++++++++++++++- 7 files changed, 86 insertions(+), 13 deletions(-) diff --git a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/info/EnvironmentInfoContributorTests.java b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/info/EnvironmentInfoContributorTests.java index e92e5b05114..ab782fc1d46 100644 --- a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/info/EnvironmentInfoContributorTests.java +++ b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/info/EnvironmentInfoContributorTests.java @@ -57,7 +57,7 @@ public class EnvironmentInfoContributorTests { @SuppressWarnings("unchecked") public void propertiesFromEnvironmentShouldBindCorrectly() throws Exception { TestPropertyValues.of("INFO_ENVIRONMENT_FOO=green").applyTo(this.environment, - Type.SYSTEM); + Type.SYSTEM_ENVIRONMENT); Info actual = contributeFrom(this.environment); assertThat(actual.get("environment", Map.class)).containsEntry("foo", "green"); } diff --git a/spring-boot-project/spring-boot-test/src/main/java/org/springframework/boot/test/util/TestPropertyValues.java b/spring-boot-project/spring-boot-test/src/main/java/org/springframework/boot/test/util/TestPropertyValues.java index 86ecdb9be21..b9fe9bc9525 100644 --- a/spring-boot-project/spring-boot-test/src/main/java/org/springframework/boot/test/util/TestPropertyValues.java +++ b/spring-boot-project/spring-boot-test/src/main/java/org/springframework/boot/test/util/TestPropertyValues.java @@ -34,6 +34,7 @@ import org.springframework.core.env.Environment; import org.springframework.core.env.MapPropertySource; import org.springframework.core.env.MutablePropertySources; import org.springframework.core.env.PropertySource; +import org.springframework.core.env.StandardEnvironment; import org.springframework.core.env.SystemEnvironmentPropertySource; import org.springframework.util.Assert; import org.springframework.util.StringUtils; @@ -98,7 +99,7 @@ public final class TestPropertyValues { * @param type the type of {@link PropertySource} to be added. See {@link Type} */ public void applyTo(ConfigurableEnvironment environment, Type type) { - applyTo(environment, type, "test"); + applyTo(environment, type, type.applySuffix("test")); } /** @@ -212,23 +213,31 @@ public final class TestPropertyValues { /** * Used for {@link SystemEnvironmentPropertySource}. */ - SYSTEM(SystemEnvironmentPropertySource.class), + SYSTEM_ENVIRONMENT(SystemEnvironmentPropertySource.class, + StandardEnvironment.SYSTEM_ENVIRONMENT_PROPERTY_SOURCE_NAME), /** * Used for {@link MapPropertySource}. */ - MAP(MapPropertySource.class); + MAP(MapPropertySource.class, null); - private Class sourceClass; + private final Class sourceClass; - Type(Class sourceClass) { + private final String suffix; + + Type(Class sourceClass, String suffix) { this.sourceClass = sourceClass; + this.suffix = (suffix == null ? null : "-" + suffix); } public Class getSourceClass() { return this.sourceClass; } + protected String applySuffix(String name) { + return (this.suffix == null ? name : name + "-" + this.suffix); + } + } /** diff --git a/spring-boot-project/spring-boot-test/src/test/java/org/springframework/boot/test/util/TestPropertyValuesTests.java b/spring-boot-project/spring-boot-test/src/test/java/org/springframework/boot/test/util/TestPropertyValuesTests.java index fcdf54c02c1..d66ba17fb38 100644 --- a/spring-boot-project/spring-boot-test/src/test/java/org/springframework/boot/test/util/TestPropertyValuesTests.java +++ b/spring-boot-project/spring-boot-test/src/test/java/org/springframework/boot/test/util/TestPropertyValuesTests.java @@ -54,8 +54,10 @@ public class TestPropertyValuesTests { @Test public void applyToSystemPropertySource() throws Exception { - TestPropertyValues.of("FOO_BAR=BAZ").applyTo(this.environment, Type.SYSTEM); + TestPropertyValues.of("FOO_BAR=BAZ").applyTo(this.environment, Type.SYSTEM_ENVIRONMENT); assertThat(this.environment.getProperty("foo.bar")).isEqualTo("BAZ"); + assertThat(this.environment.getPropertySources().contains( + "test-" + StandardEnvironment.SYSTEM_ENVIRONMENT_PROPERTY_SOURCE_NAME)); } @Test @@ -70,7 +72,7 @@ public class TestPropertyValuesTests { throws Exception { TestPropertyValues.of("foo.bar=baz", "hello.world=hi").applyTo(this.environment, Type.MAP, "other"); - TestPropertyValues.of("FOO_BAR=BAZ").applyTo(this.environment, Type.SYSTEM, + TestPropertyValues.of("FOO_BAR=BAZ").applyTo(this.environment, Type.SYSTEM_ENVIRONMENT, "other"); assertThat(this.environment.getPropertySources().get("other")) .isInstanceOf(SystemEnvironmentPropertySource.class); diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/SpringConfigurationPropertySource.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/SpringConfigurationPropertySource.java index ae495ee7485..3aab820e9a7 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/SpringConfigurationPropertySource.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/SpringConfigurationPropertySource.java @@ -27,6 +27,7 @@ import org.springframework.boot.origin.Origin; import org.springframework.boot.origin.PropertySourceOrigin; import org.springframework.core.env.EnumerablePropertySource; import org.springframework.core.env.PropertySource; +import org.springframework.core.env.StandardEnvironment; import org.springframework.core.env.SystemEnvironmentPropertySource; import org.springframework.util.Assert; @@ -150,12 +151,20 @@ class SpringConfigurationPropertySource implements ConfigurationPropertySource { } private static PropertyMapper getPropertyMapper(PropertySource source) { - if (source instanceof SystemEnvironmentPropertySource) { + if (source instanceof SystemEnvironmentPropertySource + && hasSystemEnvironmentName(source)) { return SystemEnvironmentPropertyMapper.INSTANCE; } return DefaultPropertyMapper.INSTANCE; } + private static boolean hasSystemEnvironmentName(PropertySource source) { + String name = source.getName(); + return StandardEnvironment.SYSTEM_ENVIRONMENT_PROPERTY_SOURCE_NAME.equals(name) + || name.endsWith("-" + + StandardEnvironment.SYSTEM_ENVIRONMENT_PROPERTY_SOURCE_NAME); + } + private static boolean isFullEnumerable(PropertySource source) { PropertySource rootSource = getRootSource(source); if (rootSource.getSource() instanceof Map) { diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/ConfigurationPropertiesBinderTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/ConfigurationPropertiesBinderTests.java index cef94a5c1bd..e1910424c16 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/ConfigurationPropertiesBinderTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/ConfigurationPropertiesBinderTests.java @@ -27,6 +27,7 @@ import org.springframework.boot.context.properties.bind.validation.BindValidatio import org.springframework.boot.context.properties.bind.validation.ValidationErrors; import org.springframework.core.env.MapPropertySource; import org.springframework.core.env.MutablePropertySources; +import org.springframework.core.env.StandardEnvironment; import org.springframework.core.env.SystemEnvironmentPropertySource; import org.springframework.mock.env.MockEnvironment; import org.springframework.test.context.support.TestPropertySourceUtils; @@ -192,7 +193,8 @@ public class ConfigurationPropertiesBinderTests { @Test public void bindToMapWithSystemProperties() { MutablePropertySources propertySources = new MutablePropertySources(); - propertySources.addLast(new SystemEnvironmentPropertySource("system", + propertySources.addLast(new SystemEnvironmentPropertySource( + StandardEnvironment.SYSTEM_ENVIRONMENT_PROPERTY_SOURCE_NAME, Collections.singletonMap("TEST_MAP_FOO_BAR", "baz"))); ConfigurationPropertiesBinder binder = new ConfigurationPropertiesBinder( propertySources, null, null); diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/bind/BackCompatibiltyBinderIntegrationTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/bind/BackCompatibiltyBinderIntegrationTests.java index 5ee9a8a36e9..e64ad02e237 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/bind/BackCompatibiltyBinderIntegrationTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/bind/BackCompatibiltyBinderIntegrationTests.java @@ -34,11 +34,10 @@ import static org.assertj.core.api.Assertions.assertThat; */ public class BackCompatibiltyBinderIntegrationTests { - // gh-10873 - @Test public void bindWhenBindingCamelCaseToEnvironmentWithExtractUnderscore() throws Exception { + // gh-10873 MockEnvironment environment = new MockEnvironment(); SystemEnvironmentPropertySource propertySource = new SystemEnvironmentPropertySource( StandardEnvironment.SYSTEM_ENVIRONMENT_PROPERTY_SOURCE_NAME, @@ -49,6 +48,17 @@ public class BackCompatibiltyBinderIntegrationTests { assertThat(result.getZkNodes()).isEqualTo("foo"); } + @Test + public void bindWhenUsingSystemEnvronmentToOverride() { + MockEnvironment environment = new MockEnvironment(); + SystemEnvironmentPropertySource propertySource = new SystemEnvironmentPropertySource( + "override", Collections.singletonMap("foo.password", "test")); + environment.getPropertySources().addFirst(propertySource); + PasswordProperties result = Binder.get(environment) + .bind("foo", Bindable.of(PasswordProperties.class)).get(); + assertThat(result.getPassword()).isEqualTo("test"); + } + public static class ExampleCamelCaseBean { private String zkNodes; @@ -63,4 +73,17 @@ public class BackCompatibiltyBinderIntegrationTests { } + protected static class PasswordProperties { + + private String password; + + public String getPassword() { + return this.password; + } + + public void setPassword(String password) { + this.password = password; + } + + } } diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/SpringConfigurationPropertySourcesTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/SpringConfigurationPropertySourcesTests.java index ca0235abd40..67a16943990 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/SpringConfigurationPropertySourcesTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/SpringConfigurationPropertySourcesTests.java @@ -66,7 +66,22 @@ public class SpringConfigurationPropertySourcesTests { @Test public void shouldAdaptSystemEnvironmentPropertySource() throws Exception { MutablePropertySources sources = new MutablePropertySources(); - sources.addLast(new SystemEnvironmentPropertySource("system", + sources.addLast(new SystemEnvironmentPropertySource( + StandardEnvironment.SYSTEM_ENVIRONMENT_PROPERTY_SOURCE_NAME, + Collections.singletonMap("SERVER_PORT", "1234"))); + Iterator iterator = new SpringConfigurationPropertySources( + sources).iterator(); + ConfigurationPropertyName name = ConfigurationPropertyName.of("server.port"); + assertThat(iterator.next().getConfigurationProperty(name).getValue()) + .isEqualTo("1234"); + assertThat(iterator.hasNext()).isFalse(); + } + + @Test + public void shouldExtendedAdaptSystemEnvironmentPropertySource() throws Exception { + MutablePropertySources sources = new MutablePropertySources(); + sources.addLast(new SystemEnvironmentPropertySource( + "test-" + StandardEnvironment.SYSTEM_ENVIRONMENT_PROPERTY_SOURCE_NAME, Collections.singletonMap("SERVER_PORT", "1234"))); Iterator iterator = new SpringConfigurationPropertySources( sources).iterator(); @@ -76,6 +91,19 @@ public class SpringConfigurationPropertySourcesTests { assertThat(iterator.hasNext()).isFalse(); } + @Test + public void shouldNotAdaptSystemEnvironmentPropertyOverrideSource() throws Exception { + MutablePropertySources sources = new MutablePropertySources(); + sources.addLast(new SystemEnvironmentPropertySource("override", + Collections.singletonMap("server.port", "1234"))); + Iterator iterator = new SpringConfigurationPropertySources( + sources).iterator(); + ConfigurationPropertyName name = ConfigurationPropertyName.of("server.port"); + assertThat(iterator.next().getConfigurationProperty(name).getValue()) + .isEqualTo("1234"); + assertThat(iterator.hasNext()).isFalse(); + } + @Test public void shouldAdaptMultiplePropertySources() throws Exception { MutablePropertySources sources = new MutablePropertySources();