From 86d5c1925916043ca69da3e3443baf5b1ae4f8ad Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Thu, 10 Sep 2015 18:05:16 -0700 Subject: [PATCH] Don't mix collection values from different sources Update PropertySourcesPropertyValues so that collection values are only added from a single PropertySource. Prior to this commit, given the following: PropertySource-A list[0]=x PropertySource-B list[0]=y list[1]=z PropertySourcesPropertyValues would take `x` from A and `z` from B, resulting in a binding of `[x,z]`. The updated code now returns the more logical `[x]`. Fixes gh-2611 --- .../bind/PropertySourcesPropertyValues.java | 42 +++++++++++----- .../PropertySourcesPropertyValuesTests.java | 48 +++++++++++++++++++ 2 files changed, 77 insertions(+), 13 deletions(-) diff --git a/spring-boot/src/main/java/org/springframework/boot/bind/PropertySourcesPropertyValues.java b/spring-boot/src/main/java/org/springframework/boot/bind/PropertySourcesPropertyValues.java index 9d9c8721ac6..51f66a675e9 100644 --- a/spring-boot/src/main/java/org/springframework/boot/bind/PropertySourcesPropertyValues.java +++ b/spring-boot/src/main/java/org/springframework/boot/bind/PropertySourcesPropertyValues.java @@ -21,6 +21,8 @@ import java.util.Collection; import java.util.Collections; import java.util.LinkedHashMap; import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import java.util.regex.Pattern; import org.springframework.beans.MutablePropertyValues; import org.springframework.beans.PropertyValue; @@ -39,17 +41,22 @@ import org.springframework.validation.DataBinder; * used with the latter. * * @author Dave Syer + * @author Phillip Webb */ public class PropertySourcesPropertyValues implements PropertyValues { - private final Map propertyValues = new LinkedHashMap(); - - private final PropertySources propertySources; - private static final Collection PATTERN_MATCHED_PROPERTY_SOURCES = Arrays .asList(StandardEnvironment.SYSTEM_ENVIRONMENT_PROPERTY_SOURCE_NAME, StandardEnvironment.SYSTEM_PROPERTIES_PROPERTY_SOURCE_NAME); + private static final Pattern COLLECTION_PROPERTY = Pattern.compile("\\[(\\d+)\\]"); + + private final PropertySources propertySources; + + private final Map propertyValues = new LinkedHashMap(); + + private final ConcurrentHashMap> collectionOwners = new ConcurrentHashMap>(); + /** * Create a new PropertyValues from the given PropertySources * @param propertySources a PropertySources instance @@ -122,10 +129,7 @@ public class PropertySourcesPropertyValues implements PropertyValues { continue; } Object value = getEnumerableProperty(source, resolver, propertyName); - if (!this.propertyValues.containsKey(propertyName)) { - this.propertyValues.put(propertyName, new PropertyValue(propertyName, - value)); - } + putIfAbsent(propertyName, value, source); } } } @@ -166,9 +170,7 @@ public class PropertySourcesPropertyValues implements PropertyValues { if (value == null) { value = source.getProperty(propertyName.toUpperCase()); } - if (value != null && !this.propertyValues.containsKey(propertyName)) { - this.propertyValues.put(propertyName, new PropertyValue(propertyName, - value)); + if (putIfAbsent(propertyName, value, source) != null) { continue; } } @@ -188,8 +190,22 @@ public class PropertySourcesPropertyValues implements PropertyValues { } for (PropertySource source : this.propertySources) { Object value = source.getProperty(propertyName); - if (value != null) { - propertyValue = new PropertyValue(propertyName, value); + propertyValue = putIfAbsent(propertyName, value, source); + if (propertyValue != null) { + return propertyValue; + } + } + return null; + } + + private PropertyValue putIfAbsent(String propertyName, Object value, + PropertySource source) { + if (value != null && !this.propertyValues.containsKey(propertyName)) { + PropertySource collectionOwner = this.collectionOwners.putIfAbsent( + COLLECTION_PROPERTY.matcher(propertyName).replaceAll("[]"), source); + if (collectionOwner == null || collectionOwner == source) { + this.collectionOwners.get(this.collectionOwners); + PropertyValue propertyValue = new PropertyValue(propertyName, value); this.propertyValues.put(propertyName, propertyValue); return propertyValue; } diff --git a/spring-boot/src/test/java/org/springframework/boot/bind/PropertySourcesPropertyValuesTests.java b/spring-boot/src/test/java/org/springframework/boot/bind/PropertySourcesPropertyValuesTests.java index 2cf79477cd7..b480c97486b 100644 --- a/spring-boot/src/test/java/org/springframework/boot/bind/PropertySourcesPropertyValuesTests.java +++ b/spring-boot/src/test/java/org/springframework/boot/bind/PropertySourcesPropertyValuesTests.java @@ -17,9 +17,11 @@ package org.springframework.boot.bind; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.LinkedHashMap; +import java.util.List; import java.util.Map; import org.junit.Before; @@ -31,12 +33,15 @@ import org.springframework.core.env.MutablePropertySources; import org.springframework.core.env.PropertySource; import org.springframework.validation.DataBinder; +import static org.hamcrest.Matchers.equalTo; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThat; /** * Tests for {@link PropertySourcesPropertyValues}. * * @author Dave Syer + * @author Phillip Webb */ public class PropertySourcesPropertyValuesTests { @@ -192,7 +197,35 @@ public class PropertySourcesPropertyValuesTests { assertEquals(null, target.getName()); } + @Test + public void testCollectionProperty() throws Exception { + ListBean target = new ListBean(); + DataBinder binder = new DataBinder(target); + Map map = new LinkedHashMap(); + map.put("list[0]", "v0"); + map.put("list[1]", "v1"); + this.propertySources.addFirst(new MapPropertySource("values", map)); + binder.bind(new PropertySourcesPropertyValues(this.propertySources)); + assertThat(target.getList(), equalTo(Arrays.asList("v0", "v1"))); + } + + @Test + public void testFirstCollectionPropertyWins() throws Exception { + ListBean target = new ListBean(); + DataBinder binder = new DataBinder(target); + Map first = new LinkedHashMap(); + first.put("list[0]", "f0"); + Map second = new LinkedHashMap(); + second.put("list[0]", "s0"); + second.put("list[1]", "s1"); + this.propertySources.addFirst(new MapPropertySource("s", second)); + this.propertySources.addFirst(new MapPropertySource("f", first)); + binder.bind(new PropertySourcesPropertyValues(this.propertySources)); + assertThat(target.getList(), equalTo(Collections.singletonList("f0"))); + } + public static class TestBean { + private String name; public String getName() { @@ -205,6 +238,7 @@ public class PropertySourcesPropertyValuesTests { } public static class FooBean { + private String foo; public String getFoo() { @@ -214,6 +248,20 @@ public class PropertySourcesPropertyValuesTests { public void setFoo(String foo) { this.foo = foo; } + + } + + public static class ListBean { + + private List list = new ArrayList(); + + public List getList() { + return this.list; + } + + public void setList(List list) { + this.list = list; + } } }