[CSV-224] Some Multi Iterator Parsing Peek Sequences Incorrectly Consume Elements.
Project: http://git-wip-us.apache.org/repos/asf/commons-csv/repo Commit: http://git-wip-us.apache.org/repos/asf/commons-csv/commit/f368f64f Tree: http://git-wip-us.apache.org/repos/asf/commons-csv/tree/f368f64f Diff: http://git-wip-us.apache.org/repos/asf/commons-csv/diff/f368f64f Branch: refs/heads/release Commit: f368f64fa7f9acdcc01084f676e8b9c2b86f946e Parents: 33f662b Author: David Warshaw <david.wars...@gmail.com> Authored: Fri May 18 14:24:50 2018 -0600 Committer: Gary Gregory <garydgreg...@gmail.com> Committed: Fri May 18 14:24:50 2018 -0600 ---------------------------------------------------------------------- src/changes/changes.xml | 1 + .../java/org/apache/commons/csv/CSVParser.java | 93 +++++++++++--------- .../org/apache/commons/csv/CSVParserTest.java | 56 ++++++++++++ 3 files changed, 106 insertions(+), 44 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/commons-csv/blob/f368f64f/src/changes/changes.xml ---------------------------------------------------------------------- diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 8142c21..d94ccdd 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -45,6 +45,7 @@ <action issue="CSV-220" type="add" dev="ggregory" due-to="Gary Gregory">Add API org.apache.commons.csv.CSVFormat.withSystemRecordSeparator().</action> <action issue="CSV-223" type="fix" dev="ggregory" due-to="Samuel Martin">Inconsistency between Javadoc of CSVFormat DEFAULT EXCEL.</action> <action issue="CSV-209" type="fix" dev="ggregory" due-to="Gary Gregory">Create CSVFormat.ORACLE preset.</action> + <action issue="CSV-224" type="fix" dev="ggregory" due-to="David Warshaw">Some Multi Iterator Parsing Peek Sequences Incorrectly Consume Elements.</action> </release> <release version="1.5" date="2017-09-03" description="Feature and bug fix release"> <action issue="CSV-203" type="fix" dev="ggregory" due-to="Richard Wheeldon, Kai Paroth">withNullString value is printed without quotes when QuoteMode.ALL is specified; add QuoteMode.ALL_NON_NULL. PR #17.</action> http://git-wip-us.apache.org/repos/asf/commons-csv/blob/f368f64f/src/main/java/org/apache/commons/csv/CSVParser.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/commons/csv/CSVParser.java b/src/main/java/org/apache/commons/csv/CSVParser.java index 2a902cd..7e9d7d4 100644 --- a/src/main/java/org/apache/commons/csv/CSVParser.java +++ b/src/main/java/org/apache/commons/csv/CSVParser.java @@ -286,6 +286,8 @@ public final class CSVParser implements Iterable<CSVRecord>, Closeable { private final Lexer lexer; + private final CSVRecordIterator csvRecordIterator; + /** A record buffer for getRecord(). Grows as necessary and is reused. */ private final List<String> recordList = new ArrayList<>(); @@ -353,6 +355,7 @@ public final class CSVParser implements Iterable<CSVRecord>, Closeable { this.format = format; this.lexer = new Lexer(format, new ExtendedBufferedReader(reader)); + this.csvRecordIterator = new CSVRecordIterator(); this.headerMap = this.initializeHeader(); this.characterOffset = characterOffset; this.recordNumber = recordNumber - 1; @@ -519,55 +522,57 @@ public final class CSVParser implements Iterable<CSVRecord>, Closeable { */ @Override public Iterator<CSVRecord> iterator() { - return new Iterator<CSVRecord>() { - private CSVRecord current; - - private CSVRecord getNextRecord() { - try { - return CSVParser.this.nextRecord(); - } catch (final IOException e) { - throw new IllegalStateException( - e.getClass().getSimpleName() + " reading next record: " + e.toString(), e); - } + return csvRecordIterator; + } + + class CSVRecordIterator implements Iterator<CSVRecord> { + private CSVRecord current; + + private CSVRecord getNextRecord() { + try { + return CSVParser.this.nextRecord(); + } catch (final IOException e) { + throw new IllegalStateException( + e.getClass().getSimpleName() + " reading next record: " + e.toString(), e); } - - @Override - public boolean hasNext() { - if (CSVParser.this.isClosed()) { - return false; - } - if (this.current == null) { - this.current = this.getNextRecord(); - } - - return this.current != null; + } + + @Override + public boolean hasNext() { + if (CSVParser.this.isClosed()) { + return false; } - - @Override - public CSVRecord next() { - if (CSVParser.this.isClosed()) { - throw new NoSuchElementException("CSVParser has been closed"); - } - CSVRecord next = this.current; - this.current = null; - + if (this.current == null) { + this.current = this.getNextRecord(); + } + + return this.current != null; + } + + @Override + public CSVRecord next() { + if (CSVParser.this.isClosed()) { + throw new NoSuchElementException("CSVParser has been closed"); + } + CSVRecord next = this.current; + this.current = null; + + if (next == null) { + // hasNext() wasn't called before + next = this.getNextRecord(); if (next == null) { - // hasNext() wasn't called before - next = this.getNextRecord(); - if (next == null) { - throw new NoSuchElementException("No more CSV records available"); - } + throw new NoSuchElementException("No more CSV records available"); } - - return next; } - - @Override - public void remove() { - throw new UnsupportedOperationException(); - } - }; - } + + return next; + } + + @Override + public void remove() { + throw new UnsupportedOperationException(); + } + }; /** * Parses the next record from the current point in the stream. http://git-wip-us.apache.org/repos/asf/commons-csv/blob/f368f64f/src/test/java/org/apache/commons/csv/CSVParserTest.java ---------------------------------------------------------------------- diff --git a/src/test/java/org/apache/commons/csv/CSVParserTest.java b/src/test/java/org/apache/commons/csv/CSVParserTest.java index 07e06bc..1e1d7a6 100644 --- a/src/test/java/org/apache/commons/csv/CSVParserTest.java +++ b/src/test/java/org/apache/commons/csv/CSVParserTest.java @@ -989,6 +989,62 @@ public class CSVParserTest { Assert.assertEquals(3, record.size()); } + @Test + public void testIteratorSequenceBreaking() throws IOException { + final String fiveRows = "1\n2\n3\n4\n5\n"; + + // Iterator hasNext() shouldn't break sequence + CSVParser parser = CSVFormat.DEFAULT.parse(new StringReader(fiveRows)); + int recordNumber = 0; + Iterator<CSVRecord> iter = parser.iterator(); + recordNumber = 0; + while (iter.hasNext()) { + CSVRecord record = iter.next(); + recordNumber++; + assertEquals(String.valueOf(recordNumber), record.get(0)); + if (recordNumber >= 2) { + break; + } + } + iter.hasNext(); + while (iter.hasNext()) { + CSVRecord record = iter.next(); + recordNumber++; + assertEquals(String.valueOf(recordNumber), record.get(0)); + } + + // Consecutive enhanced for loops shouldn't break sequence + parser = CSVFormat.DEFAULT.parse(new StringReader(fiveRows)); + recordNumber = 0; + for (CSVRecord record : parser) { + recordNumber++; + assertEquals(String.valueOf(recordNumber), record.get(0)); + if (recordNumber >= 2) { + break; + } + } + for (CSVRecord record : parser) { + recordNumber++; + assertEquals(String.valueOf(recordNumber), record.get(0)); + } + + // Consecutive enhanced for loops with hasNext() peeking shouldn't break sequence + parser = CSVFormat.DEFAULT.parse(new StringReader(fiveRows)); + recordNumber = 0; + for (CSVRecord record : parser) { + recordNumber++; + assertEquals(String.valueOf(recordNumber), record.get(0)); + if (recordNumber >= 2) { + break; + } + } + parser.iterator().hasNext(); + for (CSVRecord record : parser) { + recordNumber++; + assertEquals(String.valueOf(recordNumber), record.get(0)); + } + } + private void validateLineNumbers(final String lineSeparator) throws IOException { try (final CSVParser parser = CSVParser.parse("a" + lineSeparator + "b" + lineSeparator + "c", CSVFormat.DEFAULT.withRecordSeparator(lineSeparator))) {
