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

Author: philwebb

spring-boot/src/main/java/org/springframework/boot/bind/PropertySourcesPropertyValues.java

@@ -21,6 +21,8 @@
 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 @@
  * used with the latter.
  *
  * @author Dave Syer
+ * @author Phillip Webb
  */
 public class PropertySourcesPropertyValues implements PropertyValues {
 
-	private final Map<String, PropertyValue> propertyValues = new LinkedHashMap<String, PropertyValue>();
-
-	private final PropertySources propertySources;
-
 	private static final Collection<String> 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<String, PropertyValue> propertyValues = new LinkedHashMap<String, PropertyValue>();
+
+	private final ConcurrentHashMap<String, PropertySource<?>> collectionOwners = new ConcurrentHashMap<String, PropertySource<?>>();
+
 	/**
 	 * Create a new PropertyValues from the given PropertySources
 	 * @param propertySources a PropertySources instance
@@ -122,10 +129,7 @@ private void processEnumerablePropertySource(EnumerablePropertySource<?> source,
 					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 @@ private void processDefaultPropertySource(PropertySource<?> source,
 			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 PropertyValue getPropertyValue(String propertyName) {
 		}
 		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;
 			}

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.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 Object getProperty(String name) {
 		assertEquals(null, target.getName());
 	}
 
+	@Test
+	public void testCollectionProperty() throws Exception {
+		ListBean target = new ListBean();
+		DataBinder binder = new DataBinder(target);
+		Map<String, Object> map = new LinkedHashMap<String, Object>();
+		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<String, Object> first = new LinkedHashMap<String, Object>();
+		first.put("list[0]", "f0");
+		Map<String, Object> second = new LinkedHashMap<String, Object>();
+		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 void setName(String name) {
 	}
 
 	public static class FooBean {
+
 		private String foo;
 
 		public String getFoo() {
@@ -214,6 +248,20 @@ public String getFoo() {
 		public void setFoo(String foo) {
 			this.foo = foo;
 		}
+
+	}
+
+	public static class ListBean {
+
+		private List<String> list = new ArrayList<String>();
+
+		public List<String> getList() {
+			return this.list;
+		}
+
+		public void setList(List<String> list) {
+			this.list = list;
+		}
 	}
 
 }