This is an automated email from the ASF dual-hosted git repository.
kturner pushed a commit to branch 2.1
in repository https://gitbox.apache.org/repos/asf/accumulo.git
The following commit(s) were added to refs/heads/2.1 by this push:
new df8c5cb51f fixes bugs in peeking iterator (#5435)
df8c5cb51f is described below
commit df8c5cb51fe76419cfdc54f4da2f27657f53c23d
Author: Keith Turner <[email protected]>
AuthorDate: Fri Mar 28 19:16:04 2025 -0400
fixes bugs in peeking iterator (#5435)
PeekingIterator was not handling null properly and was not following the
iterator contract and throwing NoSuchElementException when it should.
The null bug was if the source iterator returned null for next() this
would cause peeking iterator to falsely claim there was no next. When
the iterator did not have a next it would return null for next() instead
of throwing an exception.
Fixed both bugs and added unit test. Looked at the test coverage of the
class and added a few other test that cover more then these bugs to get
100% coverage.
---
.../apache/accumulo/core/util/PeekingIterator.java | 20 +++---
.../accumulo/core/util/PeekingIteratorTest.java | 78 +++++++++++++++++++++-
2 files changed, 88 insertions(+), 10 deletions(-)
diff --git
a/core/src/main/java/org/apache/accumulo/core/util/PeekingIterator.java
b/core/src/main/java/org/apache/accumulo/core/util/PeekingIterator.java
index c7b85ec159..37d7e55277 100644
--- a/core/src/main/java/org/apache/accumulo/core/util/PeekingIterator.java
+++ b/core/src/main/java/org/apache/accumulo/core/util/PeekingIterator.java
@@ -19,6 +19,7 @@
package org.apache.accumulo.core.util;
import java.util.Iterator;
+import java.util.NoSuchElementException;
import java.util.function.Predicate;
import com.google.common.base.Preconditions;
@@ -28,15 +29,10 @@ public class PeekingIterator<E> implements Iterator<E> {
boolean isInitialized;
Iterator<E> source;
E top;
+ boolean hasNext;
public PeekingIterator(Iterator<E> source) {
- this.source = source;
- if (source.hasNext()) {
- top = source.next();
- } else {
- top = null;
- }
- isInitialized = true;
+ initialize(source);
}
/**
@@ -54,8 +50,10 @@ public class PeekingIterator<E> implements Iterator<E> {
this.source = source;
if (source.hasNext()) {
top = source.next();
+ hasNext = true;
} else {
top = null;
+ hasNext = false;
}
isInitialized = true;
return this;
@@ -73,11 +71,17 @@ public class PeekingIterator<E> implements Iterator<E> {
if (!isInitialized) {
throw new IllegalStateException("Iterator has not yet been initialized");
}
+ if (!hasNext) {
+ throw new NoSuchElementException();
+ }
+
E lastPeeked = top;
if (source.hasNext()) {
top = source.next();
+ hasNext = true;
} else {
top = null;
+ hasNext = false;
}
return lastPeeked;
}
@@ -92,7 +96,7 @@ public class PeekingIterator<E> implements Iterator<E> {
if (!isInitialized) {
throw new IllegalStateException("Iterator has not yet been initialized");
}
- return top != null;
+ return hasNext;
}
/**
diff --git
a/core/src/test/java/org/apache/accumulo/core/util/PeekingIteratorTest.java
b/core/src/test/java/org/apache/accumulo/core/util/PeekingIteratorTest.java
index 27bf76a725..ce76c91b0d 100644
--- a/core/src/test/java/org/apache/accumulo/core/util/PeekingIteratorTest.java
+++ b/core/src/test/java/org/apache/accumulo/core/util/PeekingIteratorTest.java
@@ -24,13 +24,79 @@ import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
+import java.util.ArrayList;
+import java.util.Collections;
import java.util.Iterator;
+import java.util.List;
+import java.util.NoSuchElementException;
+import java.util.Objects;
import java.util.stream.IntStream;
import org.junit.jupiter.api.Test;
public class PeekingIteratorTest {
+ @Test
+ public void testEmpty() {
+ var iter = new PeekingIterator<>(Collections.emptyIterator());
+ assertFalse(iter.hasNext());
+ assertNull(iter.peek());
+ assertThrows(NoSuchElementException.class, iter::next);
+ }
+
+ @Test
+ public void testNotInitialized() {
+ var iter = new PeekingIterator<>();
+ assertThrows(IllegalStateException.class, iter::hasNext);
+ assertThrows(IllegalStateException.class, iter::next);
+ assertThrows(IllegalStateException.class, iter::peek);
+ }
+
+ @Test
+ public void testNull() {
+ List<String> list = new ArrayList<>();
+
+ list.add("a");
+ list.add(null);
+ list.add("b");
+
+ var iter = new PeekingIterator<>(list.iterator());
+
+ assertTrue(iter.hasNext());
+ assertEquals("a", iter.peek());
+ assertEquals("a", iter.next());
+ assertThrows(UnsupportedOperationException.class, iter::remove);
+ assertTrue(iter.hasNext());
+ assertNull(iter.peek());
+ assertNull(iter.next());
+ assertTrue(iter.hasNext());
+ assertEquals("b", iter.peek());
+ assertEquals("b", iter.next());
+ assertFalse(iter.hasNext());
+ assertThrows(NoSuchElementException.class, iter::next);
+
+ iter = new PeekingIterator<>(list.iterator());
+ iter.findWithin(Objects::isNull, 3);
+ assertTrue(iter.hasNext());
+ assertNull(iter.peek());
+ assertNull(iter.next());
+ assertTrue(iter.hasNext());
+ assertEquals("b", iter.peek());
+ assertEquals("b", iter.next());
+ assertFalse(iter.hasNext());
+ assertThrows(NoSuchElementException.class, iter::next);
+
+ iter = new PeekingIterator<>(list.iterator());
+ iter.findWithin(e -> Objects.equals(e, "b"), 3);
+ assertTrue(iter.hasNext());
+ assertEquals("b", iter.peek());
+ assertEquals("b", iter.next());
+ assertFalse(iter.hasNext());
+ assertThrows(NoSuchElementException.class, iter::next);
+
+ assertEquals(3, list.size());
+ }
+
@Test
public void testPeek() {
Iterator<Integer> ints = IntStream.range(1, 11).iterator();
@@ -49,7 +115,7 @@ public class PeekingIteratorTest {
}
assertFalse(peek.hasNext());
- assertNull(peek.next());
+ assertThrows(NoSuchElementException.class, peek::next);
}
@Test
@@ -81,7 +147,15 @@ public class PeekingIteratorTest {
// Try to advance past the end
assertFalse(peek.findWithin((x) -> x != null && x == 7, 3));
assertFalse(peek.hasNext());
- assertNull(peek.next());
+ assertNull(peek.peek());
+ assertThrows(NoSuchElementException.class, peek::next);
+
+ // test finding that exhaust iterator w/o finding anything
+ var peek2 = new PeekingIterator<>(ints);
+ peek2.findWithin(x -> Objects.equals(x, 100), 50);
+ assertFalse(peek2.hasNext());
+ assertNull(peek2.peek());
+ assertThrows(NoSuchElementException.class, peek2::next);
}
