From e624b7acae8e4ee9f8c8b1633e32ba3bca447431 Mon Sep 17 00:00:00 2001 From: Naveed Khan Date: Wed, 1 Jul 2026 13:19:28 +0530 Subject: [PATCH 1/2] do not mutate input map in switch closure/transformer factories --- src/changes/changes.xml | 1 + .../commons/collections4/ClosureUtils.java | 9 ++++++--- .../commons/collections4/TransformerUtils.java | 9 ++++++--- .../collections4/functors/SwitchClosure.java | 10 ++++++---- .../commons/collections4/ClosureUtilsTest.java | 17 +++++++++++++++++ .../collections4/TransformerUtilsTest.java | 10 ++++++++++ 6 files changed, 46 insertions(+), 10 deletions(-) diff --git a/src/changes/changes.xml b/src/changes/changes.xml index eb1d1315dc..ad12f961a0 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -64,6 +64,7 @@ Fix integer overflow in SetOperations.cosineSimilarity (#693). Validate deserialized size in CircularFifoQueue.readObject (#678). Do not mutate the input map in SwitchTransformer.switchTransformer (#696). + Do not mutate the input map in SwitchClosure.switchClosure, ClosureUtils.switchMapClosure and TransformerUtils.switchMapTransformer. Add generics to UnmodifiableIterator for the wrapped type. Add a Maven benchmark profile for JMH. diff --git a/src/main/java/org/apache/commons/collections4/ClosureUtils.java b/src/main/java/org/apache/commons/collections4/ClosureUtils.java index 4ad525c4dd..b90584a577 100644 --- a/src/main/java/org/apache/commons/collections4/ClosureUtils.java +++ b/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 Closure switchClosure(final Predicate[] predicat @SuppressWarnings("unchecked") public static Closure switchMapClosure(final Map> objectsAndClosures) { Objects.requireNonNull(objectsAndClosures, "objectsAndClosures"); - final Closure def = objectsAndClosures.remove(null); - final int size = objectsAndClosures.size(); + // copy so the caller's map is not mutated + final Map> objects = new LinkedHashMap<>(objectsAndClosures); + final Closure def = objects.remove(null); + final int size = objects.size(); final Closure[] trs = new Closure[size]; final Predicate[] preds = new Predicate[size]; int i = 0; - for (final Map.Entry> entry : objectsAndClosures.entrySet()) { + for (final Map.Entry> entry : objects.entrySet()) { preds[i] = EqualPredicate.equalPredicate(entry.getKey()); trs[i] = entry.getValue(); i++; diff --git a/src/main/java/org/apache/commons/collections4/TransformerUtils.java b/src/main/java/org/apache/commons/collections4/TransformerUtils.java index 3b1bdceabe..8376b78c2c 100644 --- a/src/main/java/org/apache/commons/collections4/TransformerUtils.java +++ b/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 Transformer switchMapTransformer( final Map> objectsAndTransformers) { Objects.requireNonNull(objectsAndTransformers, "objectsAndTransformers"); - final Transformer def = objectsAndTransformers.remove(null); - final int size = objectsAndTransformers.size(); + // copy so the caller's map is not mutated + final Map> objects = new LinkedHashMap<>(objectsAndTransformers); + final Transformer def = objects.remove(null); + final int size = objects.size(); final Transformer[] trs = new Transformer[size]; final Predicate[] preds = new Predicate[size]; int i = 0; - for (final Map.Entry> entry : objectsAndTransformers.entrySet()) { + for (final Map.Entry> entry : objects.entrySet()) { preds[i] = EqualPredicate.equalPredicate(entry.getKey()); trs[i++] = entry.getValue(); } diff --git a/src/main/java/org/apache/commons/collections4/functors/SwitchClosure.java b/src/main/java/org/apache/commons/collections4/functors/SwitchClosure.java index d3aff19856..12329473bd 100644 --- a/src/main/java/org/apache/commons/collections4/functors/SwitchClosure.java +++ b/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 implements Closure, Serializable { @SuppressWarnings("unchecked") public static Closure switchClosure(final Map, Closure> predicatesAndClosures) { Objects.requireNonNull(predicatesAndClosures, "predicatesAndClosures"); - // convert to array like this to guarantee iterator() ordering - final Closure defaultClosure = predicatesAndClosures.remove(null); - final int size = predicatesAndClosures.size(); + // copy so the caller's map is not mutated; LinkedHashMap preserves iterator() ordering + final Map, Closure> entries = new LinkedHashMap<>(predicatesAndClosures); + final Closure defaultClosure = entries.remove(null); + final int size = entries.size(); if (size == 0) { return (Closure) (defaultClosure == null ? NOPClosure.nopClosure() : defaultClosure); } final Closure[] closures = new Closure[size]; final Predicate[] preds = new Predicate[size]; int i = 0; - for (final Map.Entry, Closure> entry : predicatesAndClosures.entrySet()) { + for (final Map.Entry, Closure> entry : entries.entrySet()) { preds[i] = entry.getKey(); closures[i] = entry.getValue(); i++; diff --git a/src/test/java/org/apache/commons/collections4/ClosureUtilsTest.java b/src/test/java/org/apache/commons/collections4/ClosureUtilsTest.java index 546b2e0dd8..d30d4a3f1b 100644 --- a/src/test/java/org/apache/commons/collections4/ClosureUtilsTest.java +++ b/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,22 @@ void testSwitchMapClosure() { assertThrows(NullPointerException.class, () -> ClosureUtils.switchMapClosure(null)); } + @Test + void testSwitchClosureDoesNotMutateInputMap() { + final Closure def = new MockClosure<>(); + final Map, Closure> predicateMap = new HashMap<>(); + predicateMap.put(null, def); + predicateMap.put(EqualPredicate.equalPredicate("HELLO"), new MockClosure<>()); + ClosureUtils.switchClosure(predicateMap); + assertTrue(predicateMap.containsKey(null)); + + final Map> objectMap = new HashMap<>(); + objectMap.put(null, def); + objectMap.put("HELLO", new MockClosure<>()); + ClosureUtils.switchMapClosure(objectMap); + assertTrue(objectMap.containsKey(null)); + } + @Test void testTransformerClosure() { final MockTransformer mock = new MockTransformer<>(); diff --git a/src/test/java/org/apache/commons/collections4/TransformerUtilsTest.java b/src/test/java/org/apache/commons/collections4/TransformerUtilsTest.java index 5029f2e60f..99f8ac5078 100644 --- a/src/test/java/org/apache/commons/collections4/TransformerUtilsTest.java +++ b/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,15 @@ void testSwitchMapTransformer() { assertThrows(NullPointerException.class, () -> TransformerUtils.switchMapTransformer(null)); } + @Test + void testSwitchMapTransformerDoesNotMutateInputMap() { + final Map> map = new HashMap<>(); + map.put(null, TransformerUtils.constantTransformer("default")); + map.put("HELLO", TransformerUtils.constantTransformer("A")); + TransformerUtils.switchMapTransformer(map); + assertTrue(map.containsKey(null)); + } + @Test @SuppressWarnings("unchecked") void testSwitchTransformer() { From e16b8e8257fb9cd401c45d9d8358904d2a043e9f Mon Sep 17 00:00:00 2001 From: Naveed Khan Date: Thu, 2 Jul 2026 11:50:21 +0530 Subject: [PATCH 2/2] assert built switch result is valid in map-copy tests Signed-off-by: Naveed Khan --- .../collections4/ClosureUtilsTest.java | 23 ++++++++++++++----- .../collections4/TransformerUtilsTest.java | 4 +++- .../SwitchTransformerMapMutatesTest.java | 5 +++- 3 files changed, 24 insertions(+), 8 deletions(-) diff --git a/src/test/java/org/apache/commons/collections4/ClosureUtilsTest.java b/src/test/java/org/apache/commons/collections4/ClosureUtilsTest.java index d30d4a3f1b..4a7ea5f193 100644 --- a/src/test/java/org/apache/commons/collections4/ClosureUtilsTest.java +++ b/src/test/java/org/apache/commons/collections4/ClosureUtilsTest.java @@ -312,18 +312,29 @@ void testSwitchMapClosure() { @Test void testSwitchClosureDoesNotMutateInputMap() { - final Closure def = new MockClosure<>(); + final MockClosure def = new MockClosure<>(); + final MockClosure match = new MockClosure<>(); final Map, Closure> predicateMap = new HashMap<>(); predicateMap.put(null, def); - predicateMap.put(EqualPredicate.equalPredicate("HELLO"), new MockClosure<>()); - ClosureUtils.switchClosure(predicateMap); + predicateMap.put(EqualPredicate.equalPredicate("HELLO"), match); + final Closure closure = ClosureUtils.switchClosure(predicateMap); assertTrue(predicateMap.containsKey(null)); + closure.execute("HELLO"); + closure.execute("WORLD"); + assertEquals(1, match.count); + assertEquals(1, def.count); + final MockClosure mapDef = new MockClosure<>(); + final MockClosure mapMatch = new MockClosure<>(); final Map> objectMap = new HashMap<>(); - objectMap.put(null, def); - objectMap.put("HELLO", new MockClosure<>()); - ClosureUtils.switchMapClosure(objectMap); + objectMap.put(null, mapDef); + objectMap.put("HELLO", mapMatch); + final Closure mapClosure = ClosureUtils.switchMapClosure(objectMap); assertTrue(objectMap.containsKey(null)); + mapClosure.execute("HELLO"); + mapClosure.execute("WORLD"); + assertEquals(1, mapMatch.count); + assertEquals(1, mapDef.count); } @Test diff --git a/src/test/java/org/apache/commons/collections4/TransformerUtilsTest.java b/src/test/java/org/apache/commons/collections4/TransformerUtilsTest.java index 99f8ac5078..9be5e3c784 100644 --- a/src/test/java/org/apache/commons/collections4/TransformerUtilsTest.java +++ b/src/test/java/org/apache/commons/collections4/TransformerUtilsTest.java @@ -290,8 +290,10 @@ void testSwitchMapTransformerDoesNotMutateInputMap() { final Map> map = new HashMap<>(); map.put(null, TransformerUtils.constantTransformer("default")); map.put("HELLO", TransformerUtils.constantTransformer("A")); - TransformerUtils.switchMapTransformer(map); + final Transformer transformer = TransformerUtils.switchMapTransformer(map); assertTrue(map.containsKey(null)); + assertEquals("A", transformer.transform("HELLO")); + assertEquals("default", transformer.transform("WORLD")); } @Test diff --git a/src/test/java/org/apache/commons/collections4/functors/SwitchTransformerMapMutatesTest.java b/src/test/java/org/apache/commons/collections4/functors/SwitchTransformerMapMutatesTest.java index 3cde120689..1bad1dbe46 100644 --- a/src/test/java/org/apache/commons/collections4/functors/SwitchTransformerMapMutatesTest.java +++ b/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 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")); } }