Repository: assertj/assertj (Java)

Issue:
Regression in `AbstractMapAssert#containsOnlyKeys` with Spring's `MultiValueMapAdapter`
#### Summary
We saw this when updating from AssertJ 3.19.0 to 3.20.0. It appears that `AbstractMapAssert#containsOnlyKeys` is mutating the map that we're asserting on, which leads to test failures in our case. This is happening on an instance of `org.springframework.util.MultiValueMapAdapter`

#### Example

```java
var underlyingMap = new HashMap<String, List<String>>();
underlyingMap.put("Hello", List.of("World"));

var multiValueMap = CollectionUtils.toMultiValueMap(underlyingMap);
// This assertion passes
assertThat(multiValueMap).containsOnlyKeys("Hello");
// This assertion fails, as `multiValueMap` and `underlyingMap` are now empty
assertThat(multiValueMap).containsOnlyKeys("Hello");
```

The issue seems to have been introduced in #2167, and is caused by [this](https://github.com/assertj/assertj-core/pull/2167/files#diff-abf21eb7e9d518345dcc39f1097da30169084ee0a538e0b174fba36807cf7386R397) use of `Map#remove` on a "clone" of the `Map` being asserted on. In our case that `Map` is a Spring `MultiValueMapAdapter`, which delegates operations to the underlying `Map` that it was constructed from. The `remove` call on the clone delegates to `multiValueMap#remove` which in turn delegates to `underlyingMap#remove`.



Hints:
With a quick look, the only option seems to be performing a check of the fully-qualified class name: if it's `org.springframework.util.MultiValueMapAdapter`, we don't try to clone the map but copy it directly.

Patch:
diff --git a/src/main/java/org/assertj/core/internal/Maps.java b/src/main/java/org/assertj/core/internal/Maps.java
--- a/src/main/java/org/assertj/core/internal/Maps.java
+++ b/src/main/java/org/assertj/core/internal/Maps.java
@@ -419,6 +419,8 @@ private static <K> Set<K> getNotExpectedKeys(Map<K, ?> actual, K[] expectedKeys)
 
   @SuppressWarnings("unchecked")
   private static <K, V> Map<K, V> clone(Map<K, V> map) throws NoSuchMethodException {
+    if (isMultiValueMapAdapterInstance(map)) throw new IllegalArgumentException("Cannot clone MultiValueMapAdapter");
+
     try {
       if (map instanceof Cloneable) {
         return (Map<K, V>) map.getClass().getMethod("clone").invoke(map);
@@ -438,6 +440,19 @@ private static <K, V> Map<K, V> clone(Map<K, V> map) throws NoSuchMethodExceptio
     }
   }
 
+  private static boolean isMultiValueMapAdapterInstance(Map<?, ?> map) {
+    return isInstanceOf(map, "org.springframework.util.MultiValueMapAdapter");
+  }
+
+  private static boolean isInstanceOf(Object object, String className) {
+    try {
+      Class<?> type = Class.forName(className);
+      return type.isInstance(object);
+    } catch (ClassNotFoundException e) {
+      return false;
+    }
+  }
+
   public <K, V> void assertContainsValue(AssertionInfo info, Map<K, V> actual, V value) {
     assertNotNull(info, actual);
     if (!containsValue(actual, value)) throw failures.failure(info, shouldContainValue(actual, value));


Test patch:
diff --git a/src/test/java/org/assertj/core/internal/maps/Maps_assertContainsOnlyKeys_Test.java b/src/test/java/org/assertj/core/internal/maps/Maps_assertContainsOnlyKeys_Test.java
--- a/src/test/java/org/assertj/core/internal/maps/Maps_assertContainsOnlyKeys_Test.java
+++ b/src/test/java/org/assertj/core/internal/maps/Maps_assertContainsOnlyKeys_Test.java
@@ -29,6 +29,7 @@
 import static org.assertj.core.util.Arrays.array;
 import static org.assertj.core.util.AssertionsUtil.expectAssertionError;
 import static org.assertj.core.util.FailureMessages.actualIsNull;
+import static org.assertj.core.util.Lists.list;
 import static org.assertj.core.util.Sets.set;
 import static org.junit.jupiter.params.provider.Arguments.arguments;
 
@@ -45,6 +46,7 @@
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.Arguments;
 import org.junit.jupiter.params.provider.MethodSource;
+import org.springframework.util.MultiValueMapAdapter;
 
 import com.google.common.collect.ImmutableMap;
 
@@ -90,9 +92,13 @@ void should_fail_if_given_keys_array_is_empty() {
       "caseInsensitiveMapsSuccessfulTestCases",
   })
   void should_pass(Map<String, String> actual, String[] expected) {
+    // GIVEN
+    int initialSize = actual.size();
     // WHEN/THEN
     assertThatNoException().as(actual.getClass().getName())
                            .isThrownBy(() -> maps.assertContainsOnlyKeys(info, actual, expected));
+
+    then(actual).hasSize(initialSize);
   }
 
   private static Stream<Arguments> unmodifiableMapsSuccessfulTestCases() {
@@ -127,6 +133,18 @@ private static Stream<Arguments> caseInsensitiveMapsSuccessfulTestCases() {
                                                           array("Job", "Name"))));
   }
 
+  @Test
+  void should_pass_with_MultiValueMapAdapter() {
+    // GIVEN
+    MultiValueMapAdapter<String, String> actual = new MultiValueMapAdapter<>(mapOf(entry("name", list("Yoda"))));
+    String[] expected = array("name");
+    int initialSize = actual.size();
+    // WHEN
+    maps.assertContainsOnlyKeys(info, actual, expected);
+    // THEN
+    then(actual).hasSize(initialSize);
+  }
+
   @ParameterizedTest
   @MethodSource({
       "unmodifiableMapsFailureTestCases",
@@ -135,12 +153,16 @@ private static Stream<Arguments> caseInsensitiveMapsSuccessfulTestCases() {
       "commonsCollectionsCaseInsensitiveMapFailureTestCases",
   })
   void should_fail(Map<String, String> actual, String[] expected, Set<String> notFound, Set<String> notExpected) {
+    // GIVEN
+    int initialSize = actual.size();
     // WHEN
     assertThatExceptionOfType(AssertionError.class).as(actual.getClass().getName())
                                                    .isThrownBy(() -> maps.assertContainsOnlyKeys(info, actual, expected))
                                                    // THEN
                                                    .withMessage(shouldContainOnlyKeys(actual, expected,
                                                                                       notFound, notExpected).create());
+
+    then(actual).hasSize(initialSize);
   }
 
   private static Stream<Arguments> unmodifiableMapsFailureTestCases() {
@@ -213,4 +235,20 @@ private static Stream<Arguments> commonsCollectionsCaseInsensitiveMapFailureTest
                                set("job"))); // internal keys are always lowercase
   }
 
+  @Test
+  void should_fail_with_MultiValueMapAdapter() {
+    // GIVEN
+    MultiValueMapAdapter<String, String> actual = new MultiValueMapAdapter<>(mapOf(entry("name", list("Yoda")),
+                                                                                   entry("job", list("Jedi"))));
+    String[] expected = array("name", "color");
+    Set<String> notFound = set("color");
+    Set<String> notExpected = set("job");
+    int initialSize = actual.size();
+    // WHEN
+    AssertionError error = expectAssertionError(() -> maps.assertContainsOnlyKeys(info, actual, expected));
+    // THEN
+    then(error).hasMessage(shouldContainOnlyKeys(actual, expected, notFound, notExpected).create());
+    then(actual).hasSize(initialSize);
+  }
+
 }
diff --git a/src/test/java/org/assertj/core/internal/maps/Maps_assertContainsOnly_Test.java b/src/test/java/org/assertj/core/internal/maps/Maps_assertContainsOnly_Test.java
--- a/src/test/java/org/assertj/core/internal/maps/Maps_assertContainsOnly_Test.java
+++ b/src/test/java/org/assertj/core/internal/maps/Maps_assertContainsOnly_Test.java
@@ -29,10 +29,12 @@
 import static org.assertj.core.util.Arrays.array;
 import static org.assertj.core.util.AssertionsUtil.expectAssertionError;
 import static org.assertj.core.util.FailureMessages.actualIsNull;
+import static org.assertj.core.util.Lists.list;
 import static org.assertj.core.util.Sets.set;
 import static org.junit.jupiter.params.provider.Arguments.arguments;
 
 import java.util.LinkedHashMap;
+import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
 import java.util.Set;
@@ -41,11 +43,13 @@
 import org.apache.commons.collections4.map.CaseInsensitiveMap;
 import org.apache.commons.collections4.map.SingletonMap;
 import org.apache.commons.lang3.ArrayUtils;
+import org.assertj.core.data.MapEntry;
 import org.assertj.core.internal.MapsBaseTest;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.Arguments;
 import org.junit.jupiter.params.provider.MethodSource;
+import org.springframework.util.MultiValueMapAdapter;
 
 import com.google.common.collect.ImmutableMap;
 
@@ -84,6 +88,15 @@ void should_fail_if_given_entries_array_is_empty() {
     then(error).hasMessage(shouldBeEmpty(actual).create());
   }
 
+  @Test
+  void should_pass_if_value_type_is_array() {
+    // GIVEN
+    Map<String, byte[]> actual = mapOf(entry("key1", new byte[] { 1, 2 }), entry("key2", new byte[] { 3, 4, 5 }));
+    Entry<String, byte[]>[] expected = array(entry("key2", new byte[] { 3, 4, 5 }), entry("key1", new byte[] { 1, 2 }));
+    // WHEN/THEN
+    assertThatNoException().isThrownBy(() -> maps.assertContainsOnly(info, actual, expected));
+  }
+
   @ParameterizedTest
   @MethodSource({
       "unmodifiableMapsSuccessfulTestCases",
@@ -91,9 +104,13 @@ void should_fail_if_given_entries_array_is_empty() {
       "caseInsensitiveMapsSuccessfulTestCases",
   })
   void should_pass(Map<String, String> actual, Entry<String, String>[] expected) {
+    // GIVEN
+    int initialSize = actual.size();
     // WHEN/THEN
     assertThatNoException().as(actual.getClass().getName())
                            .isThrownBy(() -> maps.assertContainsOnly(info, actual, expected));
+
+    then(actual).hasSize(initialSize);
   }
 
   private static Stream<Arguments> unmodifiableMapsSuccessfulTestCases() {
@@ -132,6 +149,18 @@ private static Stream<Arguments> caseInsensitiveMapsSuccessfulTestCases() {
                                                           array(entry("Job", "Jedi"), entry("Name", "Yoda")))));
   }
 
+  @Test
+  void should_pass_with_MultiValueMapAdapter() {
+    // GIVEN
+    MultiValueMapAdapter<String, String> actual = new MultiValueMapAdapter<>(mapOf(entry("name", list("Yoda"))));
+    Entry<String, List<String>>[] expected = array(entry("name", list("Yoda")));
+    int initialSize = actual.size();
+    // WHEN
+    maps.assertContainsOnly(info, actual, expected);
+    // THEN
+    then(actual).hasSize(initialSize);
+  }
+
   @ParameterizedTest
   @MethodSource({
       "unmodifiableMapsFailureTestCases",
@@ -142,12 +171,16 @@ private static Stream<Arguments> caseInsensitiveMapsSuccessfulTestCases() {
   })
   void should_fail(Map<String, String> actual, Entry<String, String>[] expected,
                    Set<Entry<String, String>> notFound, Set<Entry<String, String>> notExpected) {
+    // GIVEN
+    int initialSize = actual.size();
     // WHEN
     assertThatExceptionOfType(AssertionError.class).as(actual.getClass().getName())
                                                    .isThrownBy(() -> maps.assertContainsOnly(info, actual, expected))
                                                    // THEN
                                                    .withMessage(shouldContainOnly(actual, expected,
                                                                                   notFound, notExpected).create());
+
+    then(actual).hasSize(initialSize);
   }
 
   private static Stream<Arguments> unmodifiableMapsFailureTestCases() {
@@ -219,14 +252,20 @@ private static Stream<Arguments> orderDependentFailureTestCases() {
                                set(entry("name", "Yoda"), entry("job", "Jedi"))));
   }
 
-  @SuppressWarnings("unchecked")
   @Test
-  void should_pass_if_value_type_is_array() {
+  void should_fail_with_MultiValueMapAdapter() {
     // GIVEN
-    Map<String, byte[]> actual = mapOf(entry("key1", new byte[] { 1, 2 }), entry("key2", new byte[] { 3, 4, 5 }));
-    Entry<String, byte[]>[] expected = new Entry[] { entry("key2", new byte[] { 3, 4, 5 }), entry("key1", new byte[] { 1, 2 }) };
-    // WHEN/THEN
-    assertThatNoException().isThrownBy(() -> maps.assertContainsOnly(info, actual, expected));
+    MultiValueMapAdapter<String, String> actual = new MultiValueMapAdapter<>(mapOf(entry("name", list("Yoda")),
+                                                                                   entry("job", list("Jedi"))));
+    MapEntry<String, List<String>>[] expected = array(entry("name", list("Yoda")), entry("color", list("Green")));
+    Set<MapEntry<String, List<String>>> notFound = set(entry("color", list("Green")));
+    Set<MapEntry<String, List<String>>> notExpected = set(entry("job", list("Jedi")));
+    int initialSize = actual.size();
+    // WHEN
+    AssertionError error = expectAssertionError(() -> maps.assertContainsOnly(info, actual, expected));
+    // THEN
+    then(error).hasMessage(shouldContainOnly(actual, expected, notFound, notExpected).create());
+    then(actual).hasSize(initialSize);
   }
 
 }