Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

COLLECTIONS-770 When creating an iterator chain containing other iterator chains flatten the resulting internal object. #308

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,13 @@
*/
package org.apache.commons.collections4.iterators;

import org.apache.commons.collections4.IteratorUtils;

import java.util.Collection;
import java.util.Iterator;
import java.util.LinkedList;
import java.util.Objects;
import java.util.Optional;
import java.util.Queue;

/**
Expand Down Expand Up @@ -49,7 +52,7 @@
* @since 2.1
*/
public class IteratorChain<E> implements Iterator<E> {

public static long hasNextCalledCount = 0;
/** The chain of iterators */
private final Queue<Iterator<? extends E>> iteratorChain = new LinkedList<>();

Expand Down Expand Up @@ -151,7 +154,21 @@ public IteratorChain(final Collection<Iterator<? extends E>> iteratorChain) {
*/
public void addIterator(final Iterator<? extends E> iterator) {
checkLocked();
iteratorChain.add(Objects.requireNonNull(iterator, "iterator"));
Objects.requireNonNull(iterator, "iterator");
if (iterator instanceof UnmodifiableIterator) {
Optional<IteratorChain<? extends E>> nestedIteratorChain = ((UnmodifiableIterator) iterator).getIteratorChain();
if (nestedIteratorChain.isPresent()) {
for (Iterator<? extends E> nestedIterator : nestedIteratorChain.get().iteratorChain) {
iteratorChain.add(IteratorUtils.unmodifiableIterator(nestedIterator));
}
} else {
iteratorChain.add(iterator);
}
} else if (iterator instanceof IteratorChain) {
iteratorChain.addAll(((IteratorChain) iterator).iteratorChain);
} else {
iteratorChain.add(iterator);
}
}

/**
Expand Down Expand Up @@ -222,6 +239,7 @@ protected void updateCurrentIterator() {
*/
@Override
public boolean hasNext() {
hasNextCalledCount++;
lockChain();
updateCurrentIterator();
lastUsedIterator = currentIterator;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import java.util.Iterator;
import java.util.Objects;
import java.util.Optional;

import org.apache.commons.collections4.Unmodifiable;

Expand All @@ -35,6 +36,17 @@ public final class UnmodifiableIterator<E> implements Iterator<E>, Unmodifiable
/** The iterator being decorated */
private final Iterator<? extends E> iterator;

/**
* Obtain an Optional holding the nested IteratorChain, if it exists.
* @return Optional holding the iterator if it is an IteratorChain.
*/
Optional<IteratorChain<? extends E>> getIteratorChain() {
final IteratorChain<? extends E> iteratorChain = iterator instanceof IteratorChain
? (IteratorChain<? extends E>) iterator
: null;
return Optional.ofNullable(iteratorChain);
}

/**
* Decorates the specified iterator such that it cannot be modified.
* <p>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,14 @@ public class IteratorChainTest extends AbstractIteratorTest<String> {
protected String[] testArray = {
"One", "Two", "Three", "Four", "Five", "Six"
};
protected String[] testArray1234 = {
"One", "Two", "Three", "Four", "Five", "Six", "Seven", "Eight"
};

protected List<String> list1 = null;
protected List<String> list2 = null;
protected List<String> list3 = null;
protected List<String> list4 = null;

public IteratorChainTest() {
super(IteratorChainTest.class.getSimpleName());
Expand All @@ -57,6 +61,9 @@ public void setUp() {
list3 = new ArrayList<>();
list3.add("Five");
list3.add("Six");
list4 = new ArrayList<>();
list4.add("Seven");
list4.add("Eight");
}

@Override
Expand Down Expand Up @@ -170,4 +177,60 @@ public void testEmptyChain() {
);
}

@Test
public void testChainOfChains() {
final Iterator<String> iteratorChain1 = new IteratorChain<>(list1.iterator(), list2.iterator());
final Iterator<String> iteratorChain2 = new IteratorChain<>(list3.iterator(), list4.iterator());
final Iterator<String> iteratorChainOfChains = new IteratorChain<>(iteratorChain1, iteratorChain2);

for (final String testValue : testArray1234) {
final Object iterValue = iteratorChainOfChains.next();

assertEquals( "Iteration value is correct", testValue, iterValue );
}

assertFalse("Iterator should now be empty", iteratorChainOfChains.hasNext());

try {
iteratorChainOfChains.next();
} catch (final Exception e) {
assertEquals("NoSuchElementException must be thrown", e.getClass(), NoSuchElementException.class);
}
Comment on lines +194 to +198
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace this with assertThrows()

}

@Test
public void testChainOfUnmodifiableChains() {
final Iterator<String> iteratorChain1 = new IteratorChain<>(list1.iterator(), list2.iterator());
final Iterator<String> unmodifiableChain1 = IteratorUtils.unmodifiableIterator(iteratorChain1);
final Iterator<String> iteratorChain2 = new IteratorChain<>(list3.iterator(), list4.iterator());
final Iterator<String> unmodifiableChain2 = IteratorUtils.unmodifiableIterator(iteratorChain2);
final Iterator<String> iteratorChainOfChains = new IteratorChain<>(unmodifiableChain1, unmodifiableChain2);

for (final String testValue : testArray1234) {
final Object iterValue = iteratorChainOfChains.next();

assertEquals( "Iteration value is correct", testValue, iterValue );
}

assertFalse("Iterator should now be empty", iteratorChainOfChains.hasNext());

try {
iteratorChainOfChains.next();
} catch (final Exception e) {
assertEquals("NoSuchElementException must be thrown", e.getClass(), NoSuchElementException.class);
}
Comment on lines +217 to +221
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace this with assertThrows()


}

@Test
public void testChainOfUnmodifiableChainsRetainsUnmodifiableBehaviourOfNestedIterator() {
final Iterator<String> iteratorChain1 = new IteratorChain<>(list1.iterator(), list2.iterator());
final Iterator<String> unmodifiableChain1 = IteratorUtils.unmodifiableIterator(iteratorChain1);
final Iterator<String> iteratorChain2 = new IteratorChain<>(list3.iterator(), list4.iterator());
final Iterator<String> unmodifiableChain2 = IteratorUtils.unmodifiableIterator(iteratorChain2);
final Iterator<String> iteratorChainOfChains = new IteratorChain<>(unmodifiableChain1, unmodifiableChain2);

iteratorChainOfChains.next();
assertThrows(UnsupportedOperationException.class, iteratorChainOfChains::remove,
"Calling remove must fail when nested iterator is unmodifiable"); }
}