[COLLECTIONS-881] MultiMapUtils.getXXX ensure safe copy

Author: clapppp

src/main/java/org/apache/commons/collections4/MultiMapUtils.java

@@ -102,14 +102,11 @@ public static <K, V> Collection<V> getCollection(final MultiValuedMap<K, V> map,
      * @param <V> the value type
      * @param map  the {@link MultiValuedMap} to use
      * @param key  the key to look up
-     * @return the Collection in the {@link MultiValuedMap} as Bag, or null if input map is null
+     * @return a new Bag containing the values from the {@link MultiValuedMap}, or null if input map is null
      */
     public static <K, V> Bag<V> getValuesAsBag(final MultiValuedMap<K, V> map, final K key) {
         if (map != null) {
             final Collection<V> col = map.get(key);
-            if (col instanceof Bag) {
-                return (Bag<V>) col;
-            }
             return new HashBag<>(col);
         }
         return null;
@@ -122,37 +119,28 @@ public static <K, V> Bag<V> getValuesAsBag(final MultiValuedMap<K, V> map, final
      * @param <V> the value type
      * @param map  the {@link MultiValuedMap} to use
      * @param key  the key to look up
-     * @return the Collection in the {@link MultiValuedMap} as List, or null if input map is null
+     * @return a new List containing the values from the {@link MultiValuedMap}, or null if input map is null
      */
     public static <K, V> List<V> getValuesAsList(final MultiValuedMap<K, V> map, final K key) {
         if (map != null) {
             final Collection<V> col = map.get(key);
-            if (col instanceof List) {
-                return (List<V>) col;
-            }
             return new ArrayList<>(col);
         }
         return null;
     }
 
-    // TODO: review the getValuesAsXXX methods - depending on the actual MultiValuedMap type, changes
-    // to the returned collection might update the backing map. This should be clarified and/or prevented.
-
     /**
      * Gets a Set from {@code MultiValuedMap} in a null-safe manner.
      *
      * @param <K> the key type
      * @param <V> the value type
      * @param map  the {@link MultiValuedMap} to use
      * @param key  the key to look up
-     * @return the Collection in the {@link MultiValuedMap} as Set, or null if input map is null
+     * @return a new Set containing the values from the {@link MultiValuedMap}, or null if input map is null
      */
     public static <K, V> Set<V> getValuesAsSet(final MultiValuedMap<K, V> map, final K key) {
         if (map != null) {
             final Collection<V> col = map.get(key);
-            if (col instanceof Set) {
-                return (Set<V>) col;
-            }
             return new HashSet<>(col);
         }
         return null;

src/test/java/org/apache/commons/collections4/MultiMapUtilsTest.java

@@ -16,6 +16,10 @@
  */
 package org.apache.commons.collections4;
 
+import static org.easymock.EasyMock.expect;
+import static org.easymock.EasyMock.replay;
+import static org.easymock.EasyMock.verify;
+import static org.easymock.EasyMock.createMock;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNull;
@@ -28,6 +32,7 @@
 import java.util.List;
 import java.util.Set;
 
+import org.apache.commons.collections4.bag.HashBag;
 import org.apache.commons.collections4.multimap.ArrayListValuedHashMap;
 import org.apache.commons.collections4.multimap.HashSetValuedHashMap;
 import org.apache.commons.collections4.multimap.LinkedHashSetValuedLinkedHashMap;
@@ -105,7 +110,7 @@ void testGetValuesAsList() {
 
     @Test
     void testGetValuesAsSet() {
-        assertNull(MultiMapUtils.getValuesAsList(null, "key1"));
+        assertNull(MultiMapUtils.getValuesAsSet(null, "key1"));
 
         final String[] values = { "v1", "v2", "v3" };
         final MultiValuedMap<String, String> map = new ArrayListValuedHashMap<>();
@@ -118,6 +123,47 @@ void testGetValuesAsSet() {
         assertEquals(new HashSet<>(Arrays.asList(values)), set);
     }
 
+    @Test
+    void testGetValuesAsBagIsSafeCopy() {
+        final String[] values = { "v1", "v2", "v3" };
+        final MultiValuedMap<String, String> mockMap = createMock(MultiValuedMap.class);
+        final Bag<String> bagToReturn = new HashBag<>();
+        bagToReturn.addAll(Arrays.asList(values));
+        expect(mockMap.get("key1")).andReturn(bagToReturn);
+        replay(mockMap);
+
+        final Bag<String> bag = MultiMapUtils.getValuesAsBag(mockMap, "key1");
+        bag.add("v4");
+        assertFalse(bagToReturn.contains("v4"));
+        verify(mockMap);
+    }
+
+    @Test
+    void testGetValuesAsListIsSafeCopy() {
+        final String[] values = { "v1", "v2", "v3" };
+        final MultiValuedMap<String, String> map = new ArrayListValuedHashMap<>();
+        for (final String val : values) {
+            map.put("key1", val);
+        }
+
+        final List<String> list = MultiMapUtils.getValuesAsList(map, "key1");
+        list.add("v4");
+        assertFalse(map.containsMapping("key1", "v4"));
+    }
+
+    @Test
+    void testGetValuesAsSetIsSafeCopy() {
+        final String[] values = { "v1", "v2", "v3" };
+        final MultiValuedMap<String, String> map = new HashSetValuedHashMap<>();
+        for (final String val : values) {
+            map.put("key1", val);
+        }
+
+        final Set<String> set = MultiMapUtils.getValuesAsSet(map, "key1");
+        set.add("v4");
+        assertFalse(map.containsMapping("key1", "v4"));
+    }
+
     @Test
     void testInvert() {
         final HashSetValuedHashMap<String, String> usages = new HashSetValuedHashMap<>();