From c66781a3d31f3654008d355108cdab4af65f5130 Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Sun, 18 Mar 2018 12:46:13 -0700 Subject: [PATCH] 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 --- .../properties/bind/CollectionBinder.java | 21 ++++-- .../context/properties/bind/MapBinder.java | 14 +++- .../bind/CollectionBinderTests.java | 51 +++++++++++---- .../properties/bind/MapBinderTests.java | 65 ++++++++++++++++++- 4 files changed, 131 insertions(+), 20 deletions(-) diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/CollectionBinder.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/CollectionBinder.java index ac953607dd..16f8e71379 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/CollectionBinder.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/CollectionBinder.java @@ -65,18 +65,27 @@ class CollectionBinder extends IndexedElementsBinder> { try { existingCollection.clear(); existingCollection.addAll(additional); - return existingCollection; + return copyIfPossible(existingCollection); } catch (UnsupportedOperationException ex) { return createNewCollection(additional); } } - private Collection createNewCollection(Collection additional) { - Collection merged = CollectionFactory - .createCollection(additional.getClass(), additional.size()); - merged.addAll(additional); - return merged; + private Collection copyIfPossible(Collection collection) { + try { + return createNewCollection(collection); + } + catch (Exception ex) { + return collection; + } + } + + private Collection createNewCollection(Collection collection) { + Collection result = CollectionFactory + .createCollection(collection.getClass(), collection.size()); + result.addAll(collection); + return result; } } diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/MapBinder.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/MapBinder.java index 1d02a1020f..5ad55455b4 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/MapBinder.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/bind/MapBinder.java @@ -91,7 +91,19 @@ class MapBinder extends AggregateBinder> { return additional; } existingMap.putAll(additional); - return existingMap; + return copyIfPossible(existingMap); + } + + private Map copyIfPossible(Map map) { + try { + Map result = CollectionFactory.createMap(map.getClass(), + map.size()); + result.putAll(map); + return result; + } + catch (Exception ex) { + return map; + } } private class EntryBinder { diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/bind/CollectionBinderTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/bind/CollectionBinderTests.java index 06fe47f936..3ceeff8dd7 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/bind/CollectionBinderTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/bind/CollectionBinderTests.java @@ -172,7 +172,6 @@ public class CollectionBinderTests { List 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 class CollectionBinderTests { 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 class CollectionBinderTests { } } - 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 { + public static class MyCustomNoDefaultConstructorList extends ArrayList { - private List items; - - public MyCustomList(List items) { - this.items = items; + public MyCustomNoDefaultConstructorList(List items) { + addAll(items); } - public List 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 { + } public static class BeanWithNestedCollection { diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/bind/MapBinderTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/bind/MapBinderTests.java index f56fa71667..ea4553e9c3 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/bind/MapBinderTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/bind/MapBinderTests.java @@ -261,7 +261,6 @@ public class MapBinderTests { .withExistingValue(existing); Map 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 class MapBinderTests { 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 Bindable> getMapBindable(Class keyGeneric, ResolvableType valueType) { ResolvableType keyType = ResolvableType.forClass(keyGeneric); @@ -657,4 +677,47 @@ public class MapBinderTests { } + 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 { + + public MyCustomNoDefaultConstructorList(Map 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 { + + } + }