From f5a71230440381427d7d652e94a44a8658357025 Mon Sep 17 00:00:00 2001 From: Yanming Zhou Date: Fri, 2 Oct 2026 22:26:53 +0800 Subject: [PATCH] Honor `Collection.remove()` contract in `CompositeCollection` Before this commit, `second.remove(o)` was called no matter whether `first.remove(o)` returned `true` or `false`. If the element was present in both collections, it was removed from both, which violates the `Collection.remove(Object)` contract of removing a single instance. Closes gh-37332 Signed-off-by: Yanming Zhou --- .../springframework/util/CompositeCollection.java | 4 +--- .../util/CompositeCollectionTests.java | 14 +++++++++++++- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/spring-core/src/main/java/org/springframework/util/CompositeCollection.java b/spring-core/src/main/java/org/springframework/util/CompositeCollection.java index 42d3bcf8655..4e7d441c38a 100644 --- a/spring-core/src/main/java/org/springframework/util/CompositeCollection.java +++ b/spring-core/src/main/java/org/springframework/util/CompositeCollection.java @@ -114,9 +114,7 @@ class CompositeCollection implements Collection { @Override public boolean remove(Object o) { - boolean firstResult = this.first.remove(o); - boolean secondResult = this.second.remove(o); - return firstResult || secondResult; + return (this.first.remove(o) || this.second.remove(o)); } @Override diff --git a/spring-core/src/test/java/org/springframework/util/CompositeCollectionTests.java b/spring-core/src/test/java/org/springframework/util/CompositeCollectionTests.java index cd158eaeab9..51253469698 100644 --- a/spring-core/src/test/java/org/springframework/util/CompositeCollectionTests.java +++ b/spring-core/src/test/java/org/springframework/util/CompositeCollectionTests.java @@ -124,8 +124,19 @@ class CompositeCollectionTests { assertThat(composite.remove("foo")).isTrue(); assertThat(composite.contains("foo")).isFalse(); assertThat(first).containsExactly("bar"); - assertThat(composite.remove("quux")).isFalse(); + + // test remove element existing in both first and second + first = new ArrayList<>(List.of("foo", "bar")); + second = new ArrayList<>(List.of("foo", "qux")); + composite = new CompositeCollection<>(first, second); + + assertThat(composite.remove("foo")).isTrue(); + assertThat(first).containsExactly("bar"); + assertThat(composite.contains("foo")).isTrue(); + assertThat(composite.remove("foo")).isTrue(); + assertThat(second).containsExactly("qux"); + assertThat(composite.contains("foo")).isFalse(); } @Test @@ -207,4 +218,5 @@ class CompositeCollectionTests { assertThat(composite).containsExactly("foo", null, "bar", null); } + }