From 16aff4cd2c403148c4da29283901ade1be50b759 Mon Sep 17 00:00:00 2001 From: Madhura Bhave Date: Mon, 30 Jul 2018 16:32:10 -0700 Subject: [PATCH] Refactor PropertySourcesDeducer Prior to the changes that fixed #12451, `FilteredPropertySources` and `CompositePropertySources` were required by the `PropertySourcesDeducer` to ensure that configuration properties binding could see changes to the environment even when there was a PropertySourcesPlaceholderConfigurer in the context. #12451 changed the way property sources are adapted by `SpringConfigurationPropertySources`, removing the need for `FilteredPropertySources` and `CompositePropertySources`. Fixes gh-13738 --- .../properties/CompositePropertySources.java | 70 --------- .../properties/FilteredPropertySources.java | 74 --------- .../properties/PropertySourcesDeducer.java | 29 +--- .../CompositePropertySourcesTests.java | 144 ------------------ .../ConfigurationPropertiesTests.java | 25 +++ .../FilteredPropertySourcesTests.java | 85 ----------- 6 files changed, 33 insertions(+), 394 deletions(-) delete mode 100644 spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/CompositePropertySources.java delete mode 100644 spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/FilteredPropertySources.java delete mode 100644 spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/CompositePropertySourcesTests.java delete mode 100644 spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/FilteredPropertySourcesTests.java diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/CompositePropertySources.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/CompositePropertySources.java deleted file mode 100644 index 47fe40d600..0000000000 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/CompositePropertySources.java +++ /dev/null @@ -1,70 +0,0 @@ -/* - * Copyright 2012-2018 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 - * - * http://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.util.Arrays; -import java.util.Iterator; -import java.util.List; -import java.util.stream.StreamSupport; - -import org.springframework.core.env.PropertySource; -import org.springframework.core.env.PropertySources; - -/** - * A composite {@link PropertySources} backed by one or more {@code PropertySources}. - * Changes to the backing {@code PropertySources} are automatically reflected in the - * composite. - * - * @author Andy Wilkinson - */ -final class CompositePropertySources implements PropertySources { - - private final List propertySources; - - CompositePropertySources(PropertySources... propertySources) { - this.propertySources = Arrays.asList(propertySources); - } - - @Override - public Iterator> iterator() { - return this.propertySources.stream() - .flatMap((sources) -> StreamSupport.stream(sources.spliterator(), false)) - .iterator(); - } - - @Override - public boolean contains(String name) { - for (PropertySources sources : this.propertySources) { - if (sources.contains(name)) { - return true; - } - } - return false; - } - - @Override - public PropertySource get(String name) { - for (PropertySources sources : this.propertySources) { - PropertySource source = sources.get(name); - if (source != null) { - return source; - } - } - return null; - } - -} diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/FilteredPropertySources.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/FilteredPropertySources.java deleted file mode 100644 index b213f5cf21..0000000000 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/FilteredPropertySources.java +++ /dev/null @@ -1,74 +0,0 @@ -/* - * Copyright 2012-2017 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 - * - * http://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.util.Arrays; -import java.util.HashSet; -import java.util.Iterator; -import java.util.Set; -import java.util.stream.StreamSupport; - -import org.springframework.core.env.PropertySource; -import org.springframework.core.env.PropertySources; - -/** - * A {@link PropertySources} decorator that filters property sources by name. - * - * @author Andy Wilkinson - */ -final class FilteredPropertySources implements PropertySources { - - private final PropertySources delegate; - - private final Set filtered; - - FilteredPropertySources(PropertySources delegate, String... filtered) { - this.delegate = delegate; - this.filtered = new HashSet<>(Arrays.asList(filtered)); - } - - @Override - public boolean contains(String name) { - if (included(name)) { - return this.delegate.contains(name); - } - return false; - } - - @Override - public PropertySource get(String name) { - if (included(name)) { - return this.delegate.get(name); - } - return null; - } - - @Override - public Iterator> iterator() { - return StreamSupport.stream(this.delegate.spliterator(), false) - .filter(this::included).iterator(); - } - - private boolean included(PropertySource propertySource) { - return included(propertySource.getName()); - } - - private boolean included(String name) { - return (!this.filtered.contains(name)); - } - -} diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/PropertySourcesDeducer.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/PropertySourcesDeducer.java index aea197253e..7253472c95 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/PropertySourcesDeducer.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/PropertySourcesDeducer.java @@ -27,7 +27,6 @@ import org.springframework.core.env.ConfigurableEnvironment; import org.springframework.core.env.Environment; import org.springframework.core.env.MutablePropertySources; import org.springframework.core.env.PropertySources; -import org.springframework.util.Assert; /** * Utility to deduce the {@link PropertySources} to use for configuration binding. @@ -45,20 +44,16 @@ class PropertySourcesDeducer { } public PropertySources getPropertySources() { - MutablePropertySources environmentPropertySources = extractEnvironmentPropertySources(); - PropertySourcesPlaceholderConfigurer placeholderConfigurer = getSinglePropertySourcesPlaceholderConfigurer(); - if (placeholderConfigurer == null) { - Assert.state(environmentPropertySources != null, - "Unable to obtain PropertySources from " - + "PropertySourcesPlaceholderConfigurer or Environment"); - return environmentPropertySources; + PropertySourcesPlaceholderConfigurer configurer = getSinglePropertySourcesPlaceholderConfigurer(); + if (configurer != null) { + return configurer.getAppliedPropertySources(); } - PropertySources appliedPropertySources = placeholderConfigurer - .getAppliedPropertySources(); - if (environmentPropertySources == null) { - return appliedPropertySources; + MutablePropertySources sources = extractEnvironmentPropertySources(); + if (sources != null) { + return sources; } - return merge(environmentPropertySources, appliedPropertySources); + throw new IllegalStateException("Unable to obtain PropertySources from " + + "PropertySourcesPlaceholderConfigurer or Environment"); } private MutablePropertySources extractEnvironmentPropertySources() { @@ -84,12 +79,4 @@ class PropertySourcesDeducer { return null; } - private PropertySources merge(PropertySources environmentPropertySources, - PropertySources appliedPropertySources) { - FilteredPropertySources filtered = new FilteredPropertySources( - appliedPropertySources, - PropertySourcesPlaceholderConfigurer.ENVIRONMENT_PROPERTIES_PROPERTY_SOURCE_NAME); - return new CompositePropertySources(filtered, environmentPropertySources); - } - } diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/CompositePropertySourcesTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/CompositePropertySourcesTests.java deleted file mode 100644 index 79de13ea27..0000000000 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/CompositePropertySourcesTests.java +++ /dev/null @@ -1,144 +0,0 @@ -/* - * Copyright 2012-2017 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 - * - * http://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.util.Collections; - -import org.junit.Test; - -import org.springframework.core.env.MapPropertySource; -import org.springframework.core.env.MutablePropertySources; - -import static org.assertj.core.api.Assertions.assertThat; - -/** - * Tests for {@link CompositePropertySources}. - * - * @author Andy Wilkinson - */ -public class CompositePropertySourcesTests { - - @Test - public void containsReturnsFalseWithNoBackingSources() { - assertThat(new CompositePropertySources().contains("foo")).isFalse(); - } - - @Test - public void getReturnsNullWithNoBackingSources() { - assertThat(new CompositePropertySources().get("foo")).isNull(); - } - - @Test - public void iteratorIsEmptyWithNoBackingSources() { - assertThat(new CompositePropertySources().iterator()).hasSize(0); - } - - @Test - public void containsReturnsTrueForPropertySourceFoundInBackingSources() { - MutablePropertySources sources = new MutablePropertySources(); - sources.addFirst(new MapPropertySource("foo", Collections.emptyMap())); - assertThat(new CompositePropertySources(sources).contains("foo")).isTrue(); - } - - @Test - public void containsReturnsFalseForPropertySourceNotFoundInBackingSources() { - MutablePropertySources sources = new MutablePropertySources(); - sources.addFirst(new MapPropertySource("bar", Collections.emptyMap())); - assertThat(new CompositePropertySources(sources).contains("foo")).isFalse(); - } - - @Test - public void getReturnsPropertySourceFoundInBackingSources() { - MutablePropertySources sources = new MutablePropertySources(); - MapPropertySource fooSource = new MapPropertySource("foo", - Collections.emptyMap()); - sources.addFirst(fooSource); - assertThat(new CompositePropertySources(sources).get("foo")).isEqualTo(fooSource); - } - - @Test - public void getReturnsNullWhenPropertySourceNotFoundInBackingSources() { - MutablePropertySources sources = new MutablePropertySources(); - sources.addFirst(new MapPropertySource("foo", Collections.emptyMap())); - assertThat(new CompositePropertySources(sources).get("bar")).isNull(); - } - - @Test - public void iteratorContainsSingleEntryWithSingleBackingSource() { - MutablePropertySources sources = new MutablePropertySources(); - MapPropertySource fooSource = new MapPropertySource("foo", - Collections.emptyMap()); - sources.addFirst(fooSource); - assertThat(new CompositePropertySources(sources).iterator()) - .containsExactly(fooSource); - } - - @Test - public void iteratorReflectsOrderingOfSourcesAcrossMultipleBackingSources() { - MutablePropertySources sourcesOne = new MutablePropertySources(); - MapPropertySource fooSource = new MapPropertySource("foo", - Collections.emptyMap()); - sourcesOne.addFirst(fooSource); - MapPropertySource barSource = new MapPropertySource("bar", - Collections.emptyMap()); - sourcesOne.addFirst(barSource); - MutablePropertySources sourcesTwo = new MutablePropertySources(); - MapPropertySource bazSource = new MapPropertySource("baz", - Collections.emptyMap()); - sourcesTwo.addFirst(bazSource); - assertThat(new CompositePropertySources(sourcesOne, sourcesTwo).iterator()) - .containsExactly(barSource, fooSource, bazSource); - } - - @Test - public void containsReflectsChangesInTheBackingSources() { - MutablePropertySources sources = new MutablePropertySources(); - sources.addFirst(new MapPropertySource("foo", Collections.emptyMap())); - assertThat(new CompositePropertySources(sources).contains("bar")).isFalse(); - MapPropertySource barSource = new MapPropertySource("bar", - Collections.emptyMap()); - sources.addFirst(barSource); - assertThat(new CompositePropertySources(sources).contains("bar")).isTrue(); - } - - @Test - public void getReflectsChangesInTheBackingSources() { - MutablePropertySources sources = new MutablePropertySources(); - sources.addFirst(new MapPropertySource("foo", Collections.emptyMap())); - assertThat(new CompositePropertySources(sources).get("bar")).isNull(); - MapPropertySource barSource = new MapPropertySource("bar", - Collections.emptyMap()); - sources.addFirst(barSource); - assertThat(new CompositePropertySources(sources).get("bar")).isEqualTo(barSource); - } - - @Test - public void iteratorReflectsChangesInTheBackingSources() { - MutablePropertySources sources = new MutablePropertySources(); - MapPropertySource fooSource = new MapPropertySource("foo", - Collections.emptyMap()); - sources.addFirst(fooSource); - assertThat(new CompositePropertySources(sources).iterator()) - .containsExactly(fooSource); - MapPropertySource barSource = new MapPropertySource("bar", - Collections.emptyMap()); - sources.addFirst(barSource); - assertThat(new CompositePropertySources(sources).iterator()) - .containsExactly(barSource, fooSource); - } - -} 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 3d607b4872..dab87db0f9 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 @@ -25,6 +25,7 @@ import java.util.HashMap; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.Properties; import java.util.Set; import javax.annotation.PostConstruct; @@ -352,6 +353,15 @@ public class ConfigurationPropertiesTests { assertThat(bean.getValue()).isEqualTo("foo"); } + @Test + public void loadWithPropertyPlaceholderShouldNotAlterPropertySourceOrder() { + load(WithPropertyPlaceholderWithLocalPropertiesValueConfiguration.class, + "com.example.bar=a"); + SimplePrefixedProperties bean = this.context + .getBean(SimplePrefixedProperties.class); + assertThat(bean.getBar()).isEqualTo("a"); + } + @Test public void loadWhenHasPostConstructShouldTriggerPostConstructWithBoundBean() { MockEnvironment environment = new MockEnvironment(); @@ -959,6 +969,21 @@ public class ConfigurationPropertiesTests { } + @Configuration + @EnableConfigurationProperties(SimplePrefixedProperties.class) + static class WithPropertyPlaceholderWithLocalPropertiesValueConfiguration { + + @Bean + public static PropertySourcesPlaceholderConfigurer configurer() { + PropertySourcesPlaceholderConfigurer placeholderConfigurer = new PropertySourcesPlaceholderConfigurer(); + Properties properties = new Properties(); + properties.put("com.example.bar", "b"); + placeholderConfigurer.setProperties(properties); + return placeholderConfigurer; + } + + } + @Configuration @EnableConfigurationProperties static class WithFactoryBeanConfiguration { diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/FilteredPropertySourcesTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/FilteredPropertySourcesTests.java deleted file mode 100644 index 8a96330e30..0000000000 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/FilteredPropertySourcesTests.java +++ /dev/null @@ -1,85 +0,0 @@ -/* - * Copyright 2012-2017 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 - * - * http://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.util.Collections; - -import org.junit.Test; - -import org.springframework.core.env.MapPropertySource; -import org.springframework.core.env.MutablePropertySources; - -import static org.assertj.core.api.Assertions.assertThat; - -/** - * Tests for {@link FilteredPropertySources}. - * - * @author Andy Wilkinson - */ -public class FilteredPropertySourcesTests { - - @Test - public void getReturnsNullForFilteredSource() { - MutablePropertySources delegate = new MutablePropertySources(); - delegate.addFirst(new MapPropertySource("foo", Collections.emptyMap())); - assertThat(new FilteredPropertySources(delegate, "foo").get("foo")).isNull(); - } - - @Test - public void getReturnsSourceThatIsNotFiltered() { - MutablePropertySources delegate = new MutablePropertySources(); - delegate.addFirst(new MapPropertySource("foo", Collections.emptyMap())); - MapPropertySource barSource = new MapPropertySource("bar", - Collections.emptyMap()); - delegate.addFirst(barSource); - assertThat(new FilteredPropertySources(delegate, "foo").get("bar")) - .isEqualTo(barSource); - } - - @Test - public void containsReturnsFalseForFilteredSource() { - MutablePropertySources delegate = new MutablePropertySources(); - delegate.addFirst(new MapPropertySource("foo", Collections.emptyMap())); - assertThat(new FilteredPropertySources(delegate, "foo").contains("foo")) - .isFalse(); - } - - @Test - public void containsReturnsTrueForSourceThatIsNotFiltered() { - MutablePropertySources delegate = new MutablePropertySources(); - delegate.addFirst(new MapPropertySource("foo", Collections.emptyMap())); - MapPropertySource barSource = new MapPropertySource("bar", - Collections.emptyMap()); - delegate.addFirst(barSource); - assertThat(new FilteredPropertySources(delegate, "foo").contains("bar")).isTrue(); - } - - @Test - public void iteratorOmitsSourceThatIsFiltered() { - MutablePropertySources delegate = new MutablePropertySources(); - MapPropertySource barSource = new MapPropertySource("bar", - Collections.emptyMap()); - delegate.addFirst(barSource); - delegate.addFirst(new MapPropertySource("foo", Collections.emptyMap())); - MapPropertySource bazSource = new MapPropertySource("baz", - Collections.emptyMap()); - delegate.addFirst(bazSource); - assertThat(new FilteredPropertySources(delegate, "foo").iterator()) - .containsExactly(bazSource, barSource); - } - -}