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. 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 97373b13cd..433f60d46b 100644 --- a/src/main/java/org/apache/commons/collections4/iterators/IteratorChain.java +++ b/src/main/java/org/apache/commons/collections4/iterators/IteratorChain.java @@ -168,7 +168,29 @@ public IteratorChain(final Iterator first, final Iterator iterator) { checkLocked(); - iteratorQueue.add(Objects.requireNonNull(iterator, "iterator")); + Objects.requireNonNull(iterator, "iterator"); + if (iterator instanceof UnmodifiableIterator) { + final Iterator underlyingIterator = ((UnmodifiableIterator) iterator).unwrap(); + if (underlyingIterator instanceof IteratorChain) { + // 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 : ((IteratorChain) underlyingIterator).iteratorQueue) { + iteratorQueue.add(UnmodifiableIterator.unmodifiableIterator(nestedIterator)); + } + } else { + // we don't know anything about the underlying iterator, simply add it here + iteratorQueue.add(iterator); + } + } 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 42b11b41ad..1fb56b5ccc 100644 --- a/src/main/java/org/apache/commons/collections4/iterators/UnmodifiableIterator.java +++ b/src/main/java/org/apache/commons/collections4/iterators/UnmodifiableIterator.java @@ -91,5 +91,4 @@ public void remove() { T unwrap() { return iterator; } - } 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 be17dac084..8d642e774b 100644 --- a/src/test/java/org/apache/commons/collections4/iterators/IteratorChainTest.java +++ b/src/test/java/org/apache/commons/collections4/iterators/IteratorChainTest.java @@ -25,9 +25,13 @@ 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; @@ -42,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; @@ -89,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 @@ -149,9 +160,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 @@ -263,4 +279,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(); + } + }); + } }