diff --git a/src/main/java/org/apache/commons/collections4/MultiMapUtils.java b/src/main/java/org/apache/commons/collections4/MultiMapUtils.java index 82bebab7c8..e1d838092e 100644 --- a/src/main/java/org/apache/commons/collections4/MultiMapUtils.java +++ b/src/main/java/org/apache/commons/collections4/MultiMapUtils.java @@ -102,14 +102,11 @@ public static Collection getCollection(final MultiValuedMap map, * @param 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 Bag getValuesAsBag(final MultiValuedMap map, final K key) { if (map != null) { final Collection col = map.get(key); - if (col instanceof Bag) { - return (Bag) col; - } return new HashBag<>(col); } return null; @@ -122,22 +119,16 @@ public static Bag getValuesAsBag(final MultiValuedMap map, final * @param 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 List getValuesAsList(final MultiValuedMap map, final K key) { if (map != null) { final Collection col = map.get(key); - if (col instanceof List) { - return (List) 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. * @@ -145,14 +136,11 @@ public static List getValuesAsList(final MultiValuedMap map, fin * @param 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 Set getValuesAsSet(final MultiValuedMap map, final K key) { if (map != null) { final Collection col = map.get(key); - if (col instanceof Set) { - return (Set) col; - } return new HashSet<>(col); } return null; diff --git a/src/test/java/org/apache/commons/collections4/MultiMapUtilsTest.java b/src/test/java/org/apache/commons/collections4/MultiMapUtilsTest.java index 7ff98ba078..4bb3528d0b 100644 --- a/src/test/java/org/apache/commons/collections4/MultiMapUtilsTest.java +++ b/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 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 mockMap = createMock(MultiValuedMap.class); + final Bag bagToReturn = new HashBag<>(); + bagToReturn.addAll(Arrays.asList(values)); + expect(mockMap.get("key1")).andReturn(bagToReturn); + replay(mockMap); + + final Bag 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 map = new ArrayListValuedHashMap<>(); + for (final String val : values) { + map.put("key1", val); + } + + final List list = MultiMapUtils.getValuesAsList(map, "key1"); + list.add("v4"); + assertFalse(map.containsMapping("key1", "v4")); + } + + @Test + void testGetValuesAsSetIsSafeCopy() { + final String[] values = { "v1", "v2", "v3" }; + final MultiValuedMap map = new HashSetValuedHashMap<>(); + for (final String val : values) { + map.put("key1", val); + } + + final Set set = MultiMapUtils.getValuesAsSet(map, "key1"); + set.add("v4"); + assertFalse(map.containsMapping("key1", "v4")); + } + @Test void testInvert() { final HashSetValuedHashMap usages = new HashSetValuedHashMap<>();