From 367f8c87a6e39bf1ca443aae24f3c82d0a2830d6 Mon Sep 17 00:00:00 2001 From: Joerg Budischewski Date: Mon, 28 Jul 2025 23:34:13 +0200 Subject: [PATCH 1/2] [collections-722] basically 'renovated' stale PR commons-collections/pull/308 from collections-770, ensures that rechaining chainedIterators multiple times now result in a single IteratorChain instance with all picked up underlying iterators, thus avoiding deeply nested imperformant IteratorChain instances. Additionally the UnmodifiableIterator gets a special treatment, so that SetUtils.SetView.iterator() also benefits from this solution. Raised test coverage. --- .../collections4/iterators/IteratorChain.java | 26 +++++- .../iterators/UnmodifiableIterator.java | 11 +++ .../iterators/IteratorChainTest.java | 84 +++++++++++++++++++ .../iterators/UnmodifiableIteratorTest.java | 8 ++ 4 files changed, 128 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/apache/commons/collections4/iterators/IteratorChain.java b/src/main/java/org/apache/commons/collections4/iterators/IteratorChain.java index 7c69f3f749..59717d75a4 100644 --- a/src/main/java/org/apache/commons/collections4/iterators/IteratorChain.java +++ b/src/main/java/org/apache/commons/collections4/iterators/IteratorChain.java @@ -163,7 +163,31 @@ public IteratorChain(final Iterator first, final Iterator iterator) { checkLocked(); - iteratorQueue.add(Objects.requireNonNull(iterator, "iterator")); + Objects.requireNonNull(iterator, "iterator"); + if (iterator instanceof UnmodifiableIterator) { + @SuppressWarnings("unchecked") // safe to upcast + final IteratorChain possibleUnderlyingIteratorChain = + ((UnmodifiableIterator) iterator).getPossibleUnderlyingIteratorChain(); + if (possibleUnderlyingIteratorChain == null) { + // we don't know anything about the underlying iterator, simply add it here + iteratorQueue.add(iterator); + } else { + // in case it is an IteratorChain, wrap every underlying iterators as unmodifiable + // multiple rechainings would otherwise lead to exponential growing number of function calls + // when the iteratorChain gets used. + for (Iterator nestedIterator : possibleUnderlyingIteratorChain.iteratorQueue) { + iteratorQueue.add(UnmodifiableIterator.unmodifiableIterator(nestedIterator)); + } + } + } else if (iterator instanceof IteratorChain) { + // add the wrapped iterators directly instead of reusing the given instance + // multiple rechainings would otherwise lead to exponential growing number of function calls + // when the iteratorChain gets used. + iteratorQueue.addAll(((IteratorChain) iterator).iteratorQueue); + } else { + // arbitrary other iterator + iteratorQueue.add(iterator); + } } /** diff --git a/src/main/java/org/apache/commons/collections4/iterators/UnmodifiableIterator.java b/src/main/java/org/apache/commons/collections4/iterators/UnmodifiableIterator.java index d81b99a6ed..168c60dab1 100644 --- a/src/main/java/org/apache/commons/collections4/iterators/UnmodifiableIterator.java +++ b/src/main/java/org/apache/commons/collections4/iterators/UnmodifiableIterator.java @@ -79,4 +79,15 @@ public void remove() { throw new UnsupportedOperationException("remove() is not supported"); } + /** + * Allows package scoped access to the wrapped iterator for a very specific IteratorChain usecase. + * @return the wrapped IteratorChain instance or null when wrapped iterator is not a IteratorChain + */ + IteratorChain getPossibleUnderlyingIteratorChain() { + if (iterator instanceof IteratorChain) { + return (IteratorChain) iterator; + } + return null; + } + } diff --git a/src/test/java/org/apache/commons/collections4/iterators/IteratorChainTest.java b/src/test/java/org/apache/commons/collections4/iterators/IteratorChainTest.java index a452118533..424e896e7a 100644 --- a/src/test/java/org/apache/commons/collections4/iterators/IteratorChainTest.java +++ b/src/test/java/org/apache/commons/collections4/iterators/IteratorChainTest.java @@ -20,11 +20,18 @@ import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.assertTimeoutPreemptively; +import java.time.Duration; import java.util.ArrayList; +import java.util.Arrays; +import java.util.HashMap; import java.util.Iterator; import java.util.List; +import java.util.Map; +import java.util.Map.Entry; import java.util.NoSuchElementException; +import java.util.Set; import org.apache.commons.collections4.IteratorUtils; import org.apache.commons.collections4.Predicate; @@ -39,10 +46,14 @@ public class IteratorChainTest extends AbstractIteratorTest { protected String[] testArray = { "One", "Two", "Three", "Four", "Five", "Six" }; + protected String[] testArray1234 = { + "One", "Two", "Three", "Four", "Five", "Six", "Seven", "Eight" + }; protected List list1; protected List list2; protected List list3; + protected List list4; public List getList1() { return list1; @@ -86,6 +97,9 @@ public void setUp() { list3 = new ArrayList<>(); list3.add("Five"); list3.add("Six"); + list4 = new ArrayList<>(); + list4.add("Seven"); + list4.add("Eight"); } @Test @@ -98,9 +112,14 @@ void testConstructList() { expected.addAll(list2); expected.addAll(list3); final IteratorChain iter = new IteratorChain<>(list); + assertEquals(iter.size(), list.size()); + assertFalse(iter.isLocked()); final List actual = new ArrayList<>(); iter.forEachRemaining(actual::add); assertEquals(actual, expected); + assertTrue(iter.isLocked()); + assertThrows(UnsupportedOperationException.class, () -> iter.addIterator(list1.iterator()), + "adding iterators after iteratorChain has been traversed must fail"); } @Test @@ -183,4 +202,69 @@ void testRemoveFromFilteredIterator() { assertEquals(1, list2.size()); } + @Test + public void testChainOfChains() { + final Iterator iteratorChain1 = new IteratorChain<>(list1.iterator(), list2.iterator()); + final Iterator iteratorChain2 = new IteratorChain<>(list3.iterator(), list4.iterator()); + final Iterator iteratorChainOfChains = new IteratorChain<>(iteratorChain1, iteratorChain2); + + for (final String testValue : testArray1234) { + final String iterValue = (String) iteratorChainOfChains.next(); + assertEquals(testValue, iterValue, "Iteration value is correct"); + } + + assertFalse(iteratorChainOfChains.hasNext(), "Iterator should now be empty"); + assertThrows(NoSuchElementException.class, iteratorChainOfChains::next, "NoSuchElementException must be thrown"); + } + + @Test + public void testChainOfUnmodifiableChains() { + final Iterator iteratorChain1 = new IteratorChain<>(list1.iterator(), list2.iterator()); + final Iterator unmodifiableChain1 = IteratorUtils.unmodifiableIterator(iteratorChain1); + final Iterator iteratorChain2 = new IteratorChain<>(list3.iterator(), list4.iterator()); + final Iterator unmodifiableChain2 = IteratorUtils.unmodifiableIterator(iteratorChain2); + final Iterator iteratorChainOfChains = new IteratorChain<>(unmodifiableChain1, unmodifiableChain2); + + for (final String testValue : testArray1234) { + final String iterValue = (String) iteratorChainOfChains.next(); + assertEquals(testValue, iterValue, "Iteration value is correct"); + } + + assertFalse(iteratorChainOfChains.hasNext(), "Iterator should now be empty"); + assertThrows(NoSuchElementException.class, iteratorChainOfChains::next, "NoSuchElementException must be thrown"); + } + + @Test + public void testChainOfUnmodifiableChainsRetainsUnmodifiableBehaviourOfNestedIterator() { + final Iterator iteratorChain1 = new IteratorChain<>(list1.iterator(), list2.iterator()); + final Iterator unmodifiableChain1 = IteratorUtils.unmodifiableIterator(iteratorChain1); + final Iterator iteratorChain2 = new IteratorChain<>(list3.iterator(), list4.iterator()); + final Iterator unmodifiableChain2 = IteratorUtils.unmodifiableIterator(iteratorChain2); + final Iterator iteratorChainOfChains = new IteratorChain<>(unmodifiableChain1, unmodifiableChain2); + + iteratorChainOfChains.next(); + assertThrows(UnsupportedOperationException.class, iteratorChainOfChains::remove, + "Calling remove must fail when nested iterator is unmodifiable"); + } + + @Test + public void testMultipleChainedIteratorPerformWellCollections722() { + final Map> source = new HashMap<>(); + for (int i = 0; i < 50; i++) { + source.put(i, Arrays.asList(1, 2, 3)); + } + + Iterator iterator = IteratorUtils.emptyIterator(); + final Set>> entries = source.entrySet(); + for (final Entry> entry : entries) { + final Iterator next = entry.getValue().iterator(); + iterator = IteratorUtils.chainedIterator(iterator, next); + } + final Iterator lastIterator = iterator; + assertTimeoutPreemptively(Duration.ofSeconds(2), () -> { + while (lastIterator.hasNext()) { + lastIterator.next().toString(); + } + }); + } } diff --git a/src/test/java/org/apache/commons/collections4/iterators/UnmodifiableIteratorTest.java b/src/test/java/org/apache/commons/collections4/iterators/UnmodifiableIteratorTest.java index 35433ad73b..58f9328a62 100644 --- a/src/test/java/org/apache/commons/collections4/iterators/UnmodifiableIteratorTest.java +++ b/src/test/java/org/apache/commons/collections4/iterators/UnmodifiableIteratorTest.java @@ -20,6 +20,8 @@ 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 static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertEquals; import java.util.ArrayList; import java.util.Arrays; @@ -78,4 +80,10 @@ void testIterator() { assertTrue(makeEmptyIterator() instanceof Unmodifiable); } + @Test + void testIteratorChainRetrieval() { + assertNull(((UnmodifiableIterator) makeObject()).getPossibleUnderlyingIteratorChain()); + final IteratorChain iteratorChain = new IteratorChain(testList.iterator()); + assertEquals(((UnmodifiableIterator) UnmodifiableIterator.unmodifiableIterator(iteratorChain)).getPossibleUnderlyingIteratorChain(), iteratorChain); + } } From 2dff6526835d2b07939dd1be66796bc452bb76d7 Mon Sep 17 00:00:00 2001 From: Joerg Budischewski Date: Mon, 18 Aug 2025 23:18:41 +0200 Subject: [PATCH 2/2] [collections-722] added 722 to changes.xml --- src/changes/changes.xml | 1 + 1 file changed, 1 insertion(+) diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 70ef6bb2e8..42830e30ac 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -33,6 +33,7 @@ Fix exception message in org.apache.commons.collections4.functors.FunctorUtils.validate(Consumer...) Fix exception message in org.apache.commons.collections4.iterators.UnmodifiableIterator.remove() to match java.util.Iterator.remove(). Calling SetUtils.union on multiple instances of SetView causes JVM to hang + Improve IteratorUtils.chainedIterator() performance. Add generics to UnmodifiableIterator for the wrapped type.