From c024313141ce4be61151e220e7acbbf469c1d283 Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Fri, 22 Dec 2017 12:15:12 -0800 Subject: [PATCH] Drop environment variable "__" list support Remove support for the `__` environment variable list shortcut in order to reduce complexity. The shortcut was developed before the new `Binder` code was fully formed and isn't really necessary now since comma-lists can be converted automatically. Fixes gh-11410 --- .../source/DefaultPropertyMapper.java | 6 +- .../properties/source/PropertyMapper.java | 8 +-- .../properties/source/PropertyMapping.java | 26 ------- .../SpringConfigurationPropertySource.java | 14 ++-- ...ngIterableConfigurationPropertySource.java | 2 +- .../SystemEnvironmentPropertyMapper.java | 69 +------------------ .../source/AbstractPropertyMapperTests.java | 14 +--- ...pringConfigurationPropertySourceTests.java | 14 ---- ...rableConfigurationPropertySourceTests.java | 15 ---- .../SystemEnvironmentPropertyMapperTests.java | 51 +------------- .../properties/source/TestPropertyMapper.java | 6 +- 11 files changed, 20 insertions(+), 205 deletions(-) diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/DefaultPropertyMapper.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/DefaultPropertyMapper.java index 30c3ad04ae..47857a321f 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/DefaultPropertyMapper.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/DefaultPropertyMapper.java @@ -19,7 +19,6 @@ package org.springframework.boot.context.properties.source; import java.util.Collections; import java.util.List; -import org.springframework.core.env.PropertySource; import org.springframework.util.ObjectUtils; /** @@ -44,7 +43,7 @@ final class DefaultPropertyMapper implements PropertyMapper { } @Override - public List map(PropertySource propertySource, + public List map( ConfigurationPropertyName configurationPropertyName) { // Use a local copy in case another thread changes things LastMapping last = this.lastMappedConfigurationPropertyName; @@ -60,8 +59,7 @@ final class DefaultPropertyMapper implements PropertyMapper { } @Override - public List map(PropertySource propertySource, - String propertySourceName) { + public List map(String propertySourceName) { // Use a local copy in case another thread changes things LastMapping last = this.lastMappedPropertyName; if (last != null && last.isFrom(propertySourceName)) { diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/PropertyMapper.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/PropertyMapper.java index b0646ea196..5bd37c757b 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/PropertyMapper.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/PropertyMapper.java @@ -43,20 +43,16 @@ interface PropertyMapper { /** * Provide mappings from a {@link ConfigurationPropertySource} * {@link ConfigurationPropertyName}. - * @param propertySource the property source * @param configurationPropertyName the name to map * @return a stream of mappings or {@code Stream#empty()} */ - List map(PropertySource propertySource, - ConfigurationPropertyName configurationPropertyName); + List map(ConfigurationPropertyName configurationPropertyName); /** * Provide mappings from a {@link PropertySource} property name. - * @param propertySource the property source * @param propertySourceName the name to map * @return a stream of mappings or {@code Stream#empty()} */ - List map(PropertySource propertySource, - String propertySourceName); + List map(String propertySourceName); } diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/PropertyMapping.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/PropertyMapping.java index d6badbd6a8..ffc2acaddc 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/PropertyMapping.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/PropertyMapping.java @@ -16,8 +16,6 @@ package org.springframework.boot.context.properties.source; -import java.util.function.Function; - import org.springframework.core.env.PropertySource; /** @@ -34,8 +32,6 @@ class PropertyMapping { private final ConfigurationPropertyName configurationPropertyName; - private final Function valueExtractor; - /** * Create a new {@link PropertyMapper} instance. * @param propertySourceName the {@link PropertySource} name @@ -44,22 +40,8 @@ class PropertyMapping { */ PropertyMapping(String propertySourceName, ConfigurationPropertyName configurationPropertyName) { - this(propertySourceName, configurationPropertyName, Function.identity()); - } - - /** - * Create a new {@link PropertyMapper} instance. - * @param propertySourceName the {@link PropertySource} name - * @param configurationPropertyName the {@link ConfigurationPropertySource} - * {@link ConfigurationPropertyName} - * @param valueExtractor the extractor used to obtain the value - */ - PropertyMapping(String propertySourceName, - ConfigurationPropertyName configurationPropertyName, - Function valueExtractor) { this.propertySourceName = propertySourceName; this.configurationPropertyName = configurationPropertyName; - this.valueExtractor = valueExtractor; } /** @@ -81,14 +63,6 @@ class PropertyMapping { } - /** - * Return a function that can be used to extract the {@link PropertySource} value. - * @return the value extractor (never {@code null}) - */ - public Function getValueExtractor() { - return this.valueExtractor; - } - /** * Return if this mapping is applicable for the given * {@link ConfigurationPropertyName}. 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 3aab820e9a..b75bc3080b 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 @@ -40,7 +40,7 @@ import org.springframework.util.Assert; *

* Each {@link ConfigurationPropertySource#getConfigurationProperty * getConfigurationProperty} call attempts to - * {@link PropertyMapper#map(PropertySource, ConfigurationPropertyName) map} the + * {@link PropertyMapper#map(ConfigurationPropertyName) map} the * {@link ConfigurationPropertyName} to one or more {@code String} based names. This * allows fast property resolution for well formed property sources. *

@@ -85,7 +85,7 @@ class SpringConfigurationPropertySource implements ConfigurationPropertySource { @Override public ConfigurationProperty getConfigurationProperty( ConfigurationPropertyName name) { - List mappings = getMapper().map(getPropertySource(), name); + List mappings = getMapper().map(name); return find(mappings, name); } @@ -112,7 +112,6 @@ class SpringConfigurationPropertySource implements ConfigurationPropertySource { if (value == null) { return null; } - value = mapping.getValueExtractor().apply(value); ConfigurationPropertyName configurationPropertyName = mapping .getConfigurationPropertyName(); Origin origin = PropertySourceOrigin.get(this.propertySource, propertySourceName); @@ -215,10 +214,10 @@ class SpringConfigurationPropertySource implements ConfigurationPropertySource { } @Override - public List map(PropertySource propertySource, + public List map( ConfigurationPropertyName configurationPropertyName) { try { - return this.mapper.map(propertySource, configurationPropertyName); + return this.mapper.map(configurationPropertyName); } catch (Exception ex) { return Collections.emptyList(); @@ -226,10 +225,9 @@ class SpringConfigurationPropertySource implements ConfigurationPropertySource { } @Override - public List map(PropertySource propertySource, - String propertySourceName) { + public List map(String propertySourceName) { try { - return this.mapper.map(propertySource, propertySourceName); + return this.mapper.map(propertySourceName); } catch (Exception ex) { return Collections.emptyList(); diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySource.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySource.java index 23c1282410..21cfad0848 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySource.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySource.java @@ -119,7 +119,7 @@ class SpringIterableConfigurationPropertySource extends SpringConfigurationPrope String[] names = getPropertySource().getPropertyNames(); mappings = new ArrayList<>(names.length); for (String name : names) { - mappings.addAll(getMapper().map(getPropertySource(), name)); + mappings.addAll(getMapper().map(name)); } mappings = Collections.unmodifiableList(mappings); if (cache != null) { diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/SystemEnvironmentPropertyMapper.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/SystemEnvironmentPropertyMapper.java index 5da50654ef..d2069fbe31 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/SystemEnvironmentPropertyMapper.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/source/SystemEnvironmentPropertyMapper.java @@ -21,12 +21,9 @@ import java.util.Collections; import java.util.LinkedHashSet; import java.util.List; import java.util.Set; -import java.util.function.Function; import java.util.stream.IntStream; import org.springframework.boot.context.properties.source.ConfigurationPropertyName.Form; -import org.springframework.core.env.PropertySource; -import org.springframework.util.StringUtils; /** * {@link PropertyMapper} for system environment variables. Names are mapped by removing @@ -52,7 +49,7 @@ final class SystemEnvironmentPropertyMapper implements PropertyMapper { } @Override - public List map(PropertySource propertySource, + public List map( ConfigurationPropertyName configurationPropertyName) { Set names = new LinkedHashSet<>(); names.add(convertName(configurationPropertyName)); @@ -60,39 +57,15 @@ final class SystemEnvironmentPropertyMapper implements PropertyMapper { List result = new ArrayList<>(); names.forEach((name) -> result .add(new PropertyMapping(name, configurationPropertyName))); - if (isListShortcutPossible(configurationPropertyName)) { - result.addAll(mapListShortcut(propertySource, configurationPropertyName)); - } return result; } - private boolean isListShortcutPossible(ConfigurationPropertyName name) { - return (name.isLastElementIndexed() && isNumber(name.getLastElement(Form.UNIFORM)) - && name.getNumberOfElements() >= 1); - } - - private List mapListShortcut(PropertySource propertySource, - ConfigurationPropertyName name) { - String result = convertName(name, name.getNumberOfElements() - 1) + "__"; - if (propertySource.containsProperty(result)) { - int index = Integer.parseInt(name.getLastElement(Form.UNIFORM)); - return Collections.singletonList( - new PropertyMapping(result, name, new ElementExtractor(index))); - } - return Collections.emptyList(); - } - @Override - public List map(PropertySource propertySource, - String propertySourceName) { + public List map(String propertySourceName) { ConfigurationPropertyName name = convertName(propertySourceName); if (name == null || name.isEmpty()) { return Collections.emptyList(); } - if (propertySourceName.endsWith("__")) { - return expandListShortcut(propertySourceName, name, - propertySource.getProperty(propertySourceName)); - } return Collections.singletonList(new PropertyMapping(propertySourceName, name)); } @@ -106,22 +79,6 @@ final class SystemEnvironmentPropertyMapper implements PropertyMapper { } } - private List expandListShortcut(String propertySourceName, - ConfigurationPropertyName rootName, Object value) { - if (value == null) { - return Collections.emptyList(); - } - List mappings = new ArrayList<>(); - String[] elements = StringUtils - .commaDelimitedListToStringArray(String.valueOf(value)); - for (int i = 0; i < elements.length; i++) { - ConfigurationPropertyName name = rootName.append("[" + i + "]"); - mappings.add(new PropertyMapping(propertySourceName, name, - new ElementExtractor(i))); - } - return mappings; - } - private String convertName(ConfigurationPropertyName name) { return convertName(name, name.getNumberOfElements()); } @@ -159,26 +116,4 @@ final class SystemEnvironmentPropertyMapper implements PropertyMapper { return !hasNonDigit; } - /** - * Function used to extract an element from a comma list. - */ - private static class ElementExtractor implements Function { - - private final int index; - - ElementExtractor(int index) { - this.index = index; - } - - @Override - public Object apply(Object value) { - if (value == null) { - return null; - } - return StringUtils - .commaDelimitedListToStringArray(value.toString())[this.index]; - } - - } - } diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/AbstractPropertyMapperTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/AbstractPropertyMapperTests.java index cdcf5a079a..aab3ec1e4d 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/AbstractPropertyMapperTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/AbstractPropertyMapperTests.java @@ -16,12 +16,8 @@ package org.springframework.boot.context.properties.source; -import java.util.Collections; import java.util.Iterator; -import org.springframework.core.env.MapPropertySource; -import org.springframework.core.env.PropertySource; - /** * Abstract base class for {@link PropertyMapper} tests. * @@ -38,9 +34,7 @@ public abstract class AbstractPropertyMapperTests { } protected final Iterator namesFromString(String name, Object value) { - PropertySource propertySource = new MapPropertySource("test", - Collections.singletonMap(name, value)); - return getMapper().map(propertySource, name).stream() + return getMapper().map(name).stream() .map((mapping) -> mapping.getConfigurationPropertyName().toString()) .iterator(); } @@ -50,10 +44,8 @@ public abstract class AbstractPropertyMapperTests { } protected final Iterator namesFromConfiguration(String name, String value) { - PropertySource propertySource = new MapPropertySource("test", - Collections.singletonMap(name, value)); - return getMapper().map(propertySource, ConfigurationPropertyName.of(name)) - .stream().map(PropertyMapping::getPropertySourceName).iterator(); + return getMapper().map(ConfigurationPropertyName.of(name)).stream() + .map(PropertyMapping::getPropertySourceName).iterator(); } } diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/SpringConfigurationPropertySourceTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/SpringConfigurationPropertySourceTests.java index a3877cf56b..bdf483446f 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/SpringConfigurationPropertySourceTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/SpringConfigurationPropertySourceTests.java @@ -71,20 +71,6 @@ public class SpringConfigurationPropertySourceTests { assertThat(adapter.getConfigurationProperty(name).getValue()).isEqualTo("value2"); } - @Test - public void getValueShouldUseExtractor() { - Map source = new LinkedHashMap<>(); - source.put("key", "value"); - PropertySource propertySource = new MapPropertySource("test", source); - TestPropertyMapper mapper = new TestPropertyMapper(); - ConfigurationPropertyName name = ConfigurationPropertyName.of("my.key"); - mapper.addFromConfigurationProperty(name, "key", - (value) -> value.toString().replace("ue", "let")); - SpringConfigurationPropertySource adapter = new SpringConfigurationPropertySource( - propertySource, mapper, null); - assertThat(adapter.getConfigurationProperty(name).getValue()).isEqualTo("vallet"); - } - @Test public void getValueOrigin() { Map source = new LinkedHashMap<>(); diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySourceTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySourceTests.java index b05a0b2797..4873cb4db1 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySourceTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/SpringIterableConfigurationPropertySourceTests.java @@ -110,21 +110,6 @@ public class SpringIterableConfigurationPropertySourceTests { assertThat(adapter.getConfigurationProperty(name).getValue()).isEqualTo("value2"); } - @Test - public void getValueShouldUseExtractor() { - Map source = new LinkedHashMap<>(); - source.put("key", "value"); - EnumerablePropertySource propertySource = new MapPropertySource("test", - source); - TestPropertyMapper mapper = new TestPropertyMapper(); - ConfigurationPropertyName name = ConfigurationPropertyName.of("my.key"); - mapper.addFromConfigurationProperty(name, "key", - (value) -> value.toString().replace("ue", "let")); - SpringIterableConfigurationPropertySource adapter = new SpringIterableConfigurationPropertySource( - propertySource, mapper); - assertThat(adapter.getConfigurationProperty(name).getValue()).isEqualTo("vallet"); - } - @Test public void getValueOrigin() { Map source = new LinkedHashMap<>(); diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/SystemEnvironmentPropertyMapperTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/SystemEnvironmentPropertyMapperTests.java index a99ddab42e..5e52530473 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/SystemEnvironmentPropertyMapperTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/SystemEnvironmentPropertyMapperTests.java @@ -16,16 +16,10 @@ package org.springframework.boot.context.properties.source; -import java.util.ArrayList; -import java.util.LinkedHashMap; import java.util.List; -import java.util.Map; import org.junit.Test; -import org.springframework.core.env.MapPropertySource; -import org.springframework.core.env.PropertySource; - import static org.assertj.core.api.Assertions.assertThat; /** @@ -50,10 +44,6 @@ public class SystemEnvironmentPropertyMapperTests extends AbstractPropertyMapper assertThat(namesFromString("HOST_0_NAME")).containsExactly("host[0].name"); assertThat(namesFromString("HOST_F00_NAME")).containsExactly("host.f00.name"); assertThat(namesFromString("S-ERVER")).containsExactly("s-erver"); - assertThat(namesFromString("SERVERS__", "1,2,3")).containsExactly("servers[0]", - "servers[1]", "servers[2]"); - assertThat(namesFromString("SERVERS_0__", "1,2,3")) - .containsExactly("servers[0][0]", "servers[0][1]", "servers[0][2]"); } @Test @@ -69,44 +59,9 @@ public class SystemEnvironmentPropertyMapperTests extends AbstractPropertyMapper "FOO_THE_BAR"); } - @Test - public void mapFromStringWhenListShortcutShouldExtractValues() { - Map source = new LinkedHashMap<>(); - source.put("SERVER__", "foo,bar,baz"); - PropertySource propertySource = new MapPropertySource("test", source); - List mappings = getMapper().map(propertySource, "SERVER__"); - List result = new ArrayList<>(); - for (PropertyMapping mapping : mappings) { - Object value = propertySource.getProperty(mapping.getPropertySourceName()); - value = mapping.getValueExtractor().apply(value); - result.add(value); - } - assertThat(result).containsExactly("foo", "bar", "baz"); - } - - @Test - public void mapFromConfigurationShouldIncludeShortcutAndExtractValues() { - Map source = new LinkedHashMap<>(); - source.put("SERVER__", "foo,bar,baz"); - PropertySource propertySource = new MapPropertySource("test", source); - List mappings = getMapper().map(propertySource, - ConfigurationPropertyName.of("server[1]")); - List result = new ArrayList<>(); - for (PropertyMapping mapping : mappings) { - Object value = propertySource.getProperty(mapping.getPropertySourceName()); - value = mapping.getValueExtractor().apply(value); - if (value != null) { - result.add(value); - } - } - assertThat(result).containsExactly("bar"); - } - @Test public void underscoreShouldNotMapToEmptyString() { - Map source = new LinkedHashMap<>(); - PropertySource propertySource = new MapPropertySource("test", source); - List mappings = getMapper().map(propertySource, "_"); + List mappings = getMapper().map("_"); boolean applicable = false; for (PropertyMapping mapping : mappings) { applicable = mapping.isApplicable(ConfigurationPropertyName.of("")); @@ -116,9 +71,7 @@ public class SystemEnvironmentPropertyMapperTests extends AbstractPropertyMapper @Test public void underscoreWithWhitespaceShouldNotMapToEmptyString() { - Map source = new LinkedHashMap<>(); - PropertySource propertySource = new MapPropertySource("test", source); - List mappings = getMapper().map(propertySource, " _"); + List mappings = getMapper().map(" _"); boolean applicable = false; for (PropertyMapping mapping : mappings) { applicable = mapping.isApplicable(ConfigurationPropertyName.of("")); diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/TestPropertyMapper.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/TestPropertyMapper.java index fbae31e4d1..9033347bcf 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/TestPropertyMapper.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/source/TestPropertyMapper.java @@ -20,7 +20,6 @@ import java.util.Collections; import java.util.List; import java.util.function.Function; -import org.springframework.core.env.PropertySource; import org.springframework.util.LinkedMultiValueMap; import org.springframework.util.MultiValueMap; @@ -53,13 +52,12 @@ class TestPropertyMapper implements PropertyMapper { } @Override - public List map(PropertySource propertySource, - String propertySourceName) { + public List map(String propertySourceName) { return this.fromSource.getOrDefault(propertySourceName, Collections.emptyList()); } @Override - public List map(PropertySource propertySource, + public List map( ConfigurationPropertyName configurationPropertyName) { return this.fromConfig.getOrDefault(configurationPropertyName, Collections.emptyList());