Set using collection copies when possible

Update `Map` and `Collection` binders to create a copy of the existing
collection whenever possible. Prior to this commit the binder would
always mutate the existing value and then call the setter with the
same instance. This could cause issues if the setter expected a
different instance.

Fixes gh-12322

Author: philwebb

spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/CollectionBinder.java

@@ -65,18 +65,27 @@ protected Collection<Object> merge(Supplier<?> existing,
 		try {
 			existingCollection.clear();
 			existingCollection.addAll(additional);
-			return existingCollection;
+			return copyIfPossible(existingCollection);
 		}
 		catch (UnsupportedOperationException ex) {
 			return createNewCollection(additional);
 		}
 	}
 
-	private Collection<Object> createNewCollection(Collection<Object> additional) {
-		Collection<Object> merged = CollectionFactory
-				.createCollection(additional.getClass(), additional.size());
-		merged.addAll(additional);
-		return merged;
+	private Collection<Object> copyIfPossible(Collection<Object> collection) {
+		try {
+			return createNewCollection(collection);
+		}
+		catch (Exception ex) {
+			return collection;
+		}
+	}
+
+	private Collection<Object> createNewCollection(Collection<Object> collection) {
+		Collection<Object> result = CollectionFactory
+				.createCollection(collection.getClass(), collection.size());
+		result.addAll(collection);
+		return result;
 	}
 
 }

spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/MapBinder.java

@@ -91,7 +91,19 @@ protected Map<Object, Object> merge(Supplier<?> existing,
 			return additional;
 		}
 		existingMap.putAll(additional);
-		return existingMap;
+		return copyIfPossible(existingMap);
+	}
+
+	private Map<Object, Object> copyIfPossible(Map<Object, Object> map) {
+		try {
+			Map<Object, Object> result = CollectionFactory.createMap(map.getClass(),
+					map.size());
+			result.putAll(map);
+			return result;
+		}
+		catch (Exception ex) {
+			return map;
+		}
 	}
 
 	private class EntryBinder {

spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/bind/CollectionBinderTests.java

@@ -172,7 +172,6 @@ public void bindToCollectionWhenHasExistingCollectionShouldReplaceAllContents()
 		List<Integer> result = this.binder
 				.bind("foo", INTEGER_LIST.withExistingValue(existing)).get();
 		assertThat(result).isExactlyInstanceOf(LinkedList.class);
-		assertThat(result).isSameAs(existing);
 		assertThat(result).containsExactly(1);
 	}
 
@@ -309,7 +308,20 @@ public void bindToCollectionWithNoDefaultConstructor() {
 		MockConfigurationPropertySource source = new MockConfigurationPropertySource();
 		source.put("foo.items", "a,b,c,c");
 		this.sources.add(source);
-		ExampleCustomBean result = this.binder.bind("foo", ExampleCustomBean.class).get();
+		ExampleCustomNoDefaultConstructorBean result = this.binder
+				.bind("foo", ExampleCustomNoDefaultConstructorBean.class).get();
+		assertThat(result.getItems()).hasSize(4);
+		assertThat(result.getItems()).containsExactly("a", "b", "c", "c");
+	}
+
+	@Test
+	public void bindToCollectionWithDefaultConstructor() {
+		// gh-12322
+		MockConfigurationPropertySource source = new MockConfigurationPropertySource();
+		source.put("foo.items", "a,b,c,c");
+		this.sources.add(source);
+		ExampleCustomWithDefaultConstructorBean result = this.binder
+				.bind("foo", ExampleCustomWithDefaultConstructorBean.class).get();
 		assertThat(result.getItems()).hasSize(4);
 		assertThat(result.getItems()).containsExactly("a", "b", "c", "c");
 	}
@@ -415,31 +427,46 @@ public void setItemsSet(Set<String> itemsSet) {
 		}
 	}
 
-	public static class ExampleCustomBean {
+	public static class ExampleCustomNoDefaultConstructorBean {
 
-		private MyCustomList items = new MyCustomList(Collections.singletonList("foo"));
+		private MyCustomNoDefaultConstructorList items = new MyCustomNoDefaultConstructorList(
+				Collections.singletonList("foo"));
 
-		public MyCustomList getItems() {
+		public MyCustomNoDefaultConstructorList getItems() {
 			return this.items;
 		}
 
-		public void setItems(MyCustomList items) {
+		public void setItems(MyCustomNoDefaultConstructorList items) {
 			this.items = items;
 		}
-	}
 
-	public static class MyCustomList extends ArrayList<String> {
+	}
 
-		private List<String> items;
+	public static class MyCustomNoDefaultConstructorList extends ArrayList<String> {
 
-		public MyCustomList(List<String> items) {
-			this.items = items;
+		public MyCustomNoDefaultConstructorList(List<String> items) {
+			addAll(items);
 		}
 
-		public List<String> getItems() {
+	}
+
+	public static class ExampleCustomWithDefaultConstructorBean {
+
+		private MyCustomWithDefaultConstructorList items = new MyCustomWithDefaultConstructorList();
+
+		public MyCustomWithDefaultConstructorList getItems() {
 			return this.items;
 		}
 
+		public void setItems(MyCustomWithDefaultConstructorList items) {
+			this.items.clear();
+			this.items.addAll(items);
+		}
+
+	}
+
+	public static class MyCustomWithDefaultConstructorList extends ArrayList<String> {
+
 	}
 
 	public static class BeanWithNestedCollection {

spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/bind/MapBinderTests.java

@@ -261,7 +261,6 @@ public void bindToMapWhenHasExistingMapShouldReplaceOnlyNewContents() {
 				.withExistingValue(existing);
 		Map<String, Integer> result = this.binder.bind("foo", target).get();
 		assertThat(result).isExactlyInstanceOf(HashMap.class);
-		assertThat(result).isSameAs(existing);
 		assertThat(result).hasSize(2);
 		assertThat(result).containsEntry("bar", 1);
 		assertThat(result).containsEntry("baz", 1001);
@@ -595,6 +594,27 @@ public void bindToMapWithPropertyEditorForValue() {
 		assertThat(map).containsExactly(entry("bar", RuntimeException.class));
 	}
 
+	@Test
+	public void bindToMapWithNoDefaultConstructor() {
+		MockConfigurationPropertySource source = new MockConfigurationPropertySource();
+		source.put("foo.items.a", "b");
+		this.sources.add(source);
+		ExampleCustomNoDefaultConstructorBean result = this.binder
+				.bind("foo", ExampleCustomNoDefaultConstructorBean.class).get();
+		assertThat(result.getItems()).containsOnly(entry("foo", "bar"), entry("a", "b"));
+	}
+
+	@Test
+	public void bindToMapWithDefaultConstructor() {
+		// gh-12322
+		MockConfigurationPropertySource source = new MockConfigurationPropertySource();
+		source.put("foo.items.a", "b");
+		this.sources.add(source);
+		ExampleCustomWithDefaultConstructorBean result = this.binder
+				.bind("foo", ExampleCustomWithDefaultConstructorBean.class).get();
+		assertThat(result.getItems()).containsExactly(entry("a", "b"));
+	}
+
 	private <K, V> Bindable<Map<K, V>> getMapBindable(Class<K> keyGeneric,
 			ResolvableType valueType) {
 		ResolvableType keyType = ResolvableType.forClass(keyGeneric);
@@ -657,4 +677,47 @@ public Map<String, String> convert(String s) {
 
 	}
 
+	public static class ExampleCustomNoDefaultConstructorBean {
+
+		private MyCustomNoDefaultConstructorList items = new MyCustomNoDefaultConstructorList(
+				Collections.singletonMap("foo", "bar"));
+
+		public MyCustomNoDefaultConstructorList getItems() {
+			return this.items;
+		}
+
+		public void setItems(MyCustomNoDefaultConstructorList items) {
+			this.items = items;
+		}
+
+	}
+
+	public static class MyCustomNoDefaultConstructorList extends HashMap<String, String> {
+
+		public MyCustomNoDefaultConstructorList(Map<String, String> items) {
+			putAll(items);
+		}
+
+	}
+
+	public static class ExampleCustomWithDefaultConstructorBean {
+
+		private MyCustomWithDefaultConstructorList items = new MyCustomWithDefaultConstructorList();
+
+		public MyCustomWithDefaultConstructorList getItems() {
+			return this.items;
+		}
+
+		public void setItems(MyCustomWithDefaultConstructorList items) {
+			this.items.clear();
+			this.items.putAll(items);
+		}
+
+	}
+
+	public static class MyCustomWithDefaultConstructorList
+			extends HashMap<String, String> {
+
+	}
+
 }