Do not mutate the input map in switch closure/transformer factories

src/changes/changes.xml

@@ -64,6 +64,7 @@
     <action type="fix" dev="ggregory" due-to="Vasiliy Mikhailov, Gary Gregory">Fix integer overflow in SetOperations.cosineSimilarity (#693).</action>
     <action type="fix" dev="ggregory" due-to="Vasiliy Mikhailov, Gary Gregory">Validate deserialized size in CircularFifoQueue.readObject (#678).</action>
     <action type="fix" dev="ggregory" due-to="Vasiliy Mikhailov, Gary Gregory">Do not mutate the input map in SwitchTransformer.switchTransformer (#696).</action>
+    <action type="fix" dev="ggregory" due-to="Naveed Khan, Gary Gregory">Do not mutate the input map in SwitchClosure.switchClosure, ClosureUtils.switchMapClosure and TransformerUtils.switchMapTransformer.</action>
     <!-- ADD -->
     <action type="add" dev="ggregory" due-to="Gary Gregory">Add generics to UnmodifiableIterator for the wrapped type.</action>
     <action type="add" dev="ggregory" due-to="Gary Gregory">Add a Maven benchmark profile for JMH.</action>

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

@@ -17,6 +17,7 @@
 package org.apache.commons.collections4;
 
 import java.util.Collection;
+import java.util.LinkedHashMap;
 import java.util.Map;
 import java.util.Objects;
 
@@ -324,12 +325,14 @@ public static <E> Closure<E> switchClosure(final Predicate<? super E>[] predicat
     @SuppressWarnings("unchecked")
     public static <E> Closure<E> switchMapClosure(final Map<? extends E, Closure<E>> objectsAndClosures) {
         Objects.requireNonNull(objectsAndClosures, "objectsAndClosures");
-        final Closure<? super E> def = objectsAndClosures.remove(null);
-        final int size = objectsAndClosures.size();
+        // copy so the caller's map is not mutated
+        final Map<? extends E, Closure<E>> objects = new LinkedHashMap<>(objectsAndClosures);
+        final Closure<? super E> def = objects.remove(null);
+        final int size = objects.size();
         final Closure<? super E>[] trs = new Closure[size];
         final Predicate<E>[] preds = new Predicate[size];
         int i = 0;
-        for (final Map.Entry<? extends E, Closure<E>> entry : objectsAndClosures.entrySet()) {
+        for (final Map.Entry<? extends E, Closure<E>> entry : objects.entrySet()) {
             preds[i] = EqualPredicate.<E>equalPredicate(entry.getKey());
             trs[i] = entry.getValue();
             i++;

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

@@ -17,6 +17,7 @@
 package org.apache.commons.collections4;
 
 import java.util.Collection;
+import java.util.LinkedHashMap;
 import java.util.Map;
 import java.util.Objects;
 
@@ -372,12 +373,14 @@ public static <I, O> Transformer<I, O> switchMapTransformer(
             final Map<I, Transformer<I, O>> objectsAndTransformers) {
 
         Objects.requireNonNull(objectsAndTransformers, "objectsAndTransformers");
-        final Transformer<? super I, ? extends O> def = objectsAndTransformers.remove(null);
-        final int size = objectsAndTransformers.size();
+        // copy so the caller's map is not mutated
+        final Map<I, Transformer<I, O>> objects = new LinkedHashMap<>(objectsAndTransformers);
+        final Transformer<? super I, ? extends O> def = objects.remove(null);
+        final int size = objects.size();
         final Transformer<? super I, ? extends O>[] trs = new Transformer[size];
         final Predicate<I>[] preds = new Predicate[size];
         int i = 0;
-        for (final Map.Entry<I, Transformer<I, O>> entry : objectsAndTransformers.entrySet()) {
+        for (final Map.Entry<I, Transformer<I, O>> entry : objects.entrySet()) {
             preds[i] = EqualPredicate.<I>equalPredicate(entry.getKey());
             trs[i++] = entry.getValue();
         }

src/main/java/org/apache/commons/collections4/functors/SwitchClosure.java

@@ -17,6 +17,7 @@
 package org.apache.commons.collections4.functors;
 
 import java.io.Serializable;
+import java.util.LinkedHashMap;
 import java.util.Map;
 import java.util.Objects;
 
@@ -57,16 +58,17 @@ public class SwitchClosure<T> implements Closure<T>, Serializable {
     @SuppressWarnings("unchecked")
     public static <E> Closure<E> switchClosure(final Map<Predicate<E>, Closure<E>> predicatesAndClosures) {
         Objects.requireNonNull(predicatesAndClosures, "predicatesAndClosures");
-        // convert to array like this to guarantee iterator() ordering
-        final Closure<? super E> defaultClosure = predicatesAndClosures.remove(null);
-        final int size = predicatesAndClosures.size();
+        // copy so the caller's map is not mutated; LinkedHashMap preserves iterator() ordering
+        final Map<Predicate<E>, Closure<E>> entries = new LinkedHashMap<>(predicatesAndClosures);
+        final Closure<? super E> defaultClosure = entries.remove(null);
+        final int size = entries.size();
         if (size == 0) {
             return (Closure<E>) (defaultClosure == null ? NOPClosure.<E>nopClosure() : defaultClosure);
         }
         final Closure<E>[] closures = new Closure[size];
         final Predicate<E>[] preds = new Predicate[size];
         int i = 0;
-        for (final Map.Entry<Predicate<E>, Closure<E>> entry : predicatesAndClosures.entrySet()) {
+        for (final Map.Entry<Predicate<E>, Closure<E>> entry : entries.entrySet()) {
             preds[i] = entry.getKey();
             closures[i] = entry.getValue();
             i++;

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

@@ -20,6 +20,7 @@
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertSame;
 import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 
 import java.io.IOException;
 import java.util.ArrayList;
@@ -309,6 +310,33 @@ void testSwitchMapClosure() {
         assertThrows(NullPointerException.class, () -> ClosureUtils.switchMapClosure(null));
     }
 
+    @Test
+    void testSwitchClosureDoesNotMutateInputMap() {
+        final MockClosure<String> def = new MockClosure<>();
+        final MockClosure<String> match = new MockClosure<>();
+        final Map<Predicate<String>, Closure<String>> predicateMap = new HashMap<>();
+        predicateMap.put(null, def);
+        predicateMap.put(EqualPredicate.equalPredicate("HELLO"), match);
+        final Closure<String> closure = ClosureUtils.switchClosure(predicateMap);
+        assertTrue(predicateMap.containsKey(null));
+        closure.execute("HELLO");
+        closure.execute("WORLD");
+        assertEquals(1, match.count);
+        assertEquals(1, def.count);
+
+        final MockClosure<String> mapDef = new MockClosure<>();
+        final MockClosure<String> mapMatch = new MockClosure<>();
+        final Map<String, Closure<String>> objectMap = new HashMap<>();
+        objectMap.put(null, mapDef);
+        objectMap.put("HELLO", mapMatch);
+        final Closure<String> mapClosure = ClosureUtils.switchMapClosure(objectMap);
+        assertTrue(objectMap.containsKey(null));
+        mapClosure.execute("HELLO");
+        mapClosure.execute("WORLD");
+        assertEquals(1, mapMatch.count);
+        assertEquals(1, mapDef.count);
+    }
+
     @Test
     void testTransformerClosure() {
         final MockTransformer<Object> mock = new MockTransformer<>();

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

@@ -21,6 +21,7 @@
 import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertSame;
 import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 
 import java.io.IOException;
 import java.util.ArrayList;
@@ -284,6 +285,17 @@ void testSwitchMapTransformer() {
         assertThrows(NullPointerException.class, () -> TransformerUtils.switchMapTransformer(null));
     }
 
+    @Test
+    void testSwitchMapTransformerDoesNotMutateInputMap() {
+        final Map<String, Transformer<String, String>> map = new HashMap<>();
+        map.put(null, TransformerUtils.constantTransformer("default"));
+        map.put("HELLO", TransformerUtils.constantTransformer("A"));
+        final Transformer<String, String> transformer = TransformerUtils.switchMapTransformer(map);
+        assertTrue(map.containsKey(null));
+        assertEquals("A", transformer.transform("HELLO"));
+        assertEquals("default", transformer.transform("WORLD"));
+    }
+
     @Test
     @SuppressWarnings("unchecked")
     void testSwitchTransformer() {

src/test/java/org/apache/commons/collections4/functors/SwitchTransformerMapMutatesTest.java

@@ -44,9 +44,12 @@ void testSwitchTransformerMapDoesNotMutateInput() {
         final int sizeBefore = map.size();
         assertEquals(2, sizeBefore);
         // Call the factory method
-        SwitchTransformer.switchTransformer(map);
+        final Transformer<String, String> result = SwitchTransformer.switchTransformer(map);
         // The map should NOT have been mutated - null key must still be present
         final int sizeAfter = map.size();
         assertEquals(sizeBefore, sizeAfter, "switchTransformer must not mutate the input map; expected size");
+        // The returned transformer must still route via the copied map
+        assertEquals("value", result.transform(null));
+        assertEquals("default", result.transform("other"));
     }
 }