Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
42 changes: 41 additions & 1 deletion src/main/java/org/apache/commons/csv/ExtendedBufferedReader.java
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,37 @@ public void mark(final int readAheadLimit) throws IOException {
super.mark(readAheadLimit);
}

/**
* Fills {@code array} with the characters that follow the current position without consuming them.
* <p>
* Overridden because the inherited implementation stops at the first short read, which leaves the tail of the array holding stale content when the source
* delivers data in chunks. Callers compare the whole array against a multi-character delimiter, so a partial fill makes them miss a delimiter that is
* really there.
* </p>
*
* @param array the buffer to fill.
* @return the number of characters peeked, or {@link IOUtils#EOF} at the end of the stream.
* @throws IOException If an I/O error occurs.
*/
@Override
public int peek(final char[] array) throws IOException {
final int length = array.length;
if (length == 0) {
return 0;
}
super.mark(length);
int len = 0;
while (len < length) {
final int more = super.read(array, len, length - len);
if (more == EOF) {
break;
}
len += more;
}
super.reset();
return len == 0 ? EOF : len;
}

@Override
public int read() throws IOException {
final int current = super.read();
Expand All @@ -244,7 +275,16 @@ public int read(final char[] buf, final int offset, final int length) throws IOE
if (length == 0) {
return 0;
}
final int len = super.read(buf, offset, length);
int len = super.read(buf, offset, length);
// The underlying buffered reader stops early once the source reports it is not ready, so a stream that delivers data in chunks (a socket or a pipe)
// yields a short read. Callers match multi-character sequences against this buffer, so keep reading until it is full or the source is exhausted.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Isn't there still a risk that even filling to the end of the buffer breaks up multi-character sequences?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No, because the callers size the array to the exact sequence they're matching. Lexer allocates delimiterBuf as delimiter.length - 1 and escapeDelimiterBuf as 2 * delimiter.length - 1, so once the array is full it holds the whole tail of the delimiter (or escaped delimiter), never a piece of it. Same for CSVFormat.printWithEscapes, which peeks delimiter.length - 1.

The only way the loop exits short now is EOF, and then the remaining characters genuinely don't exist, so no complete sequence can be there to miss. Before the change the loop could exit short just because the source wasn't ready yet, which is the case that broke.

while (len > 0 && len < length) {
final int more = super.read(buf, offset + len, length - len);
if (more == EOF) {
break;
}
len += more;
}
if (encoder != null && len > 0) {
this.bytesRead += getEncodedCharLength(buf, offset, len);
}
Expand Down
41 changes: 41 additions & 0 deletions src/test/java/org/apache/commons/csv/CSVParserTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -1709,6 +1709,47 @@ void testParseWithDelimiterStringWithEscape() throws IOException {
}
}

@Test
void testParseWithDelimiterStringFromChunkedReader() throws IOException {
// A reader that hands out one character at a time and never reports itself ready, like a socket or pipe.
final Reader chunked = new Reader() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If this is the same as the behavior of the ChunkedReader in the other test, then refactor for reuse.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Right, they were the same thing. Pulled it out into a package-private ChunkedReader in the test tree so both this test and ExtendedBufferedReaderTest use it.


private final String content = "a[|]b\r\nc![!|!]d[|]e";
private int index;

@Override
public void close() {
// nothing to close
}

@Override
public boolean ready() {
return false;
}

@Override
public int read(final char[] buf, final int offset, final int length) {
if (index >= content.length()) {
return -1;
}
if (length <= 0) {
return 0;
}
buf[offset] = content.charAt(index++);
return 1;
}
};
final CSVFormat csvFormat = CSVFormat.DEFAULT.builder().setDelimiter("[|]").setEscape('!').get();
try (CSVParser csvParser = csvFormat.parse(chunked)) {
CSVRecord csvRecord = csvParser.nextRecord();
assertEquals("a", csvRecord.get(0));
assertEquals("b", csvRecord.get(1));
csvRecord = csvParser.nextRecord();
assertEquals("c[|]d", csvRecord.get(0));
assertEquals("e", csvRecord.get(1));
}
}

@Test
void testParseWithDelimiterStringWithQuote() throws IOException {
final String source = "'a[|]b[|]c'[|]xyz\r\nabc[abc][|]xyz";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -240,4 +240,72 @@ void testReadLookahead2() throws Exception {
assertEquals('d', br.getLastChar());
}
}

@Test
void testReadAndPeekArrayFromChunkedReader() throws Exception {
try (ExtendedBufferedReader br = new ExtendedBufferedReader(new ChunkedReader("abcdef"))) {
final char[] peeked = new char[3];
assertEquals(3, br.peek(peeked));
assertArrayEquals(new char[] { 'a', 'b', 'c' }, peeked);
final char[] read = new char[3];
assertEquals(3, br.read(read, 0, 3));
assertArrayEquals(new char[] { 'a', 'b', 'c' }, read);
}
}

@Test
void testReadArrayPastEndOfChunkedReader() throws Exception {
try (ExtendedBufferedReader br = new ExtendedBufferedReader(new ChunkedReader("ab"))) {
final char[] read = new char[4];
assertEquals(2, br.read(read, 0, 4));
assertEquals('a', read[0]);
assertEquals('b', read[1]);
assertEquals(EOF, br.read(read, 0, 4));
}
}

@Test
void testPeekArrayPastEndOfChunkedReader() throws Exception {
try (ExtendedBufferedReader br = new ExtendedBufferedReader(new ChunkedReader("ab"))) {
final char[] peeked = new char[4];
assertEquals(2, br.peek(peeked));
assertEquals('a', peeked[0]);
assertEquals('b', peeked[1]);
}
}

/**
* A reader that returns one character per call and never reports itself ready, like a socket or pipe that delivers data in chunks.
*/
private static final class ChunkedReader extends java.io.Reader {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would it be simpler to extend StringReader and override those methods?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. It's now a small ChunkedReader extends StringReader that overrides read(char[], int, int) to hand back one char and ready() to return false, which is a lot less code than the raw Reader.


private final String content;
private int index;

ChunkedReader(final String content) {
this.content = content;
}

@Override
public void close() {
// nothing to close
}

@Override
public boolean ready() {
return false;
}

@Override
public int read(final char[] buf, final int offset, final int length) {
if (index >= content.length()) {
return EOF;
}
if (length <= 0) {
return 0;
}
buf[offset] = content.charAt(index++);
return 1;
}
}
}