You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "gbidsilva (via GitHub)" <gi...@apache.org> on 2023/08/30 08:12:29 UTC
[GitHub] [commons-csv] gbidsilva opened a new pull request, #347: [CSV-147] Error message optimization during faulty CSV record read
gbidsilva opened a new pull request, #347:
URL: https://github.com/apache/commons-csv/pull/347
This fix is related to : https://issues.apache.org/jira/browse/CSV-147.
If we have some faulty data in the CSV then the current error message which we are getting is something similar to below.
`java.io.IOException: (line 2) invalid char between encapsulated token and delimiter`
With this fix, what we will be getting is something similar to below,
`java.io.IOException: An error occurred while tying to parse the CSV content. Error in line: 2, position: 94, last parsed content: ...rec4,rec5,rec6,rec7,rec8`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] gbidsilva commented on a diff in pull request #347: [CSV-147] Error message optimization during faulty CSV record read
Posted by "gbidsilva (via GitHub)" <gi...@apache.org>.
gbidsilva commented on code in PR #347:
URL: https://github.com/apache/commons-csv/pull/347#discussion_r1314921125
##########
src/main/java/org/apache/commons/csv/CSVParser.java:
##########
@@ -765,7 +765,13 @@ CSVRecord nextRecord() throws IOException {
final long startCharPosition = lexer.getCharacterPosition() + this.characterOffset;
do {
this.reusableToken.reset();
- this.lexer.nextToken(this.reusableToken);
+ try {
+ this.lexer.nextToken(this.reusableToken);
+ } catch (IOException ioe) {
+ String errorMessage = "Exception during parsing at line: "
+ + this.lexer.getCurrentLineNumber() + ", position: " + this.lexer.getCharacterPosition();
Review Comment:
@garydgregory
I had a look at what you are suggesting. Below are my thoughts...
If we look at the internals of method `Token nextToken(final Token token) throws IOException{}` in class `Lexer`, there are around 10 places where `IOException` can be thrown.
Among them method calls to `parseEncapsulatedToken()`, `parseSimpleToken()` and `reader.read()` have high possibility of throwing parsing related `IOException`.
In that case we have following options.
1. handle each of method calls for customized IOException throwings with our enhanced exception message.
2. wrap `nextToken()` in class `Lexer` entirely and throw `IOException` with enhanced exception message.
3. keep the `this.lexer.nextToken(this.reusableToken)` wrapped with try catch in class `CSVParser` and rethrow IOException with enhanced message - this is what we have at the moment in PR.
IMO, keeping the try catch block in the current place we have at the moment seems more simple rather than handling exception in multiple places inside `Lexer` class.
Appreciate your opinion on this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] elharo commented on a diff in pull request #347: [CSV-147] Error message optimization during faulty CSV record read
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #347:
URL: https://github.com/apache/commons-csv/pull/347#discussion_r1315124751
##########
src/main/java/org/apache/commons/csv/CSVParser.java:
##########
@@ -765,7 +765,13 @@ CSVRecord nextRecord() throws IOException {
final long startCharPosition = lexer.getCharacterPosition() + this.characterOffset;
do {
this.reusableToken.reset();
- this.lexer.nextToken(this.reusableToken);
+ try {
+ this.lexer.nextToken(this.reusableToken);
+ } catch (IOException ioe) {
+ String errorMessage = "Exception during parsing at line: "
+ + this.lexer.getCurrentLineNumber() + ", position: " + this.lexer.getCharacterPosition();
Review Comment:
I vote for number 1. try to keep exception logic in one place.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] gbidsilva commented on pull request #347: [CSV-147] Error message optimization during faulty CSV record read
Posted by "gbidsilva (via GitHub)" <gi...@apache.org>.
gbidsilva commented on PR #347:
URL: https://github.com/apache/commons-csv/pull/347#issuecomment-1701028200
@elharo: Thank you for the feedback. Changes are updated in the PR now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] garydgregory commented on a diff in pull request #347: [CSV-147] Error message optimization during faulty CSV record read
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #347:
URL: https://github.com/apache/commons-csv/pull/347#discussion_r1312125582
##########
src/main/java/org/apache/commons/csv/CSVParser.java:
##########
@@ -438,6 +440,44 @@ public CSVParser(final Reader reader, final CSVFormat format, final long charact
this.recordNumber = recordNumber - 1;
}
+ /**
+ * Return the parsed CSV content of currently parsed line up until this method is called.
+ * <p>
+ * Maximum parsed token length set by the 'maxParsedTokenCount' is considered during the construction of return string.
+ * </p>
+ * <p>
+ * Example:
+ * </p>
+ * </p>
+ * If currently reading CSV record row contains following data and 'maxParsedTokenCount' is set to 5 and current reading position is col7
+ * </p>
+ * <pre>
+ * col1, col2, col3, col4, col5, col6, col7
+ * </pre>
+ * <p>
+ * then method returns following
+ * </p>
+ * <pre>
+ * col3, col4, col5, col6, col7
+ * </pre>
+ * @return parsed CSV content of current reading line
+ */
+ public String getLastParsedContent() {
Review Comment:
I'll take another look later but, the fact that someone created a ticket does not make it a requirement for the component. As I've previously stated, adding position information is fine but not data IMO.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] gbidsilva commented on a diff in pull request #347: [CSV-147] Error message optimization during faulty CSV record read
Posted by "gbidsilva (via GitHub)" <gi...@apache.org>.
gbidsilva commented on code in PR #347:
URL: https://github.com/apache/commons-csv/pull/347#discussion_r1311701467
##########
src/main/java/org/apache/commons/csv/CSVParser.java:
##########
@@ -765,7 +805,14 @@ CSVRecord nextRecord() throws IOException {
final long startCharPosition = lexer.getCharacterPosition() + this.characterOffset;
do {
this.reusableToken.reset();
- this.lexer.nextToken(this.reusableToken);
+ // https://issues.apache.org/jira/browse/CSV-147
+ try {
+ this.lexer.nextToken(this.reusableToken);
+ } catch (IOException ioe) {
+ String errorMessage = "An exception occurred while parsing the CSV content. Issue in line: "
+ + this.lexer.getCurrentLineNumber() + ", position: " + this.lexer.getCharacterPosition();
Review Comment:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] gbidsilva commented on a diff in pull request #347: [CSV-147] Error message optimization during faulty CSV record read
Posted by "gbidsilva (via GitHub)" <gi...@apache.org>.
gbidsilva commented on code in PR #347:
URL: https://github.com/apache/commons-csv/pull/347#discussion_r1311699099
##########
src/main/java/org/apache/commons/csv/CSVParser.java:
##########
@@ -765,7 +805,14 @@ CSVRecord nextRecord() throws IOException {
final long startCharPosition = lexer.getCharacterPosition() + this.characterOffset;
do {
this.reusableToken.reset();
- this.lexer.nextToken(this.reusableToken);
+ // https://issues.apache.org/jira/browse/CSV-147
Review Comment:
done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] gbidsilva commented on pull request #347: [CSV-147] Error message optimization during faulty CSV record read
Posted by "gbidsilva (via GitHub)" <gi...@apache.org>.
gbidsilva commented on PR #347:
URL: https://github.com/apache/commons-csv/pull/347#issuecomment-1715623466
@garydgregory : Anything pending from development side for this to be merged ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] codecov-commenter commented on pull request #347: [CSV-147] Error message optimization during faulty CSV record read
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #347:
URL: https://github.com/apache/commons-csv/pull/347#issuecomment-1717515848
## [Codecov](https://app.codecov.io/gh/apache/commons-csv/pull/347?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#347](https://app.codecov.io/gh/apache/commons-csv/pull/347?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (c0759cd) into [master](https://app.codecov.io/gh/apache/commons-csv/commit/710459843cbe3cf4f1ca3aaac5a6eb22d049e643?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (7104598) will **increase** coverage by `0.00%`.
> Report is 13 commits behind head on master.
> The diff coverage is `100.00%`.
```diff
@@ Coverage Diff @@
## master #347 +/- ##
=========================================
Coverage 97.87% 97.87%
Complexity 549 549
=========================================
Files 11 11
Lines 1178 1179 +1
Branches 204 204
=========================================
+ Hits 1153 1154 +1
Misses 13 13
Partials 12 12
```
| [Files Changed](https://app.codecov.io/gh/apache/commons-csv/pull/347?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [src/main/java/org/apache/commons/csv/Lexer.java](https://app.codecov.io/gh/apache/commons-csv/pull/347?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvY3N2L0xleGVyLmphdmE=) | `98.81% <100.00%> (+<0.01%)` | :arrow_up: |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] elharo commented on a diff in pull request #347: [CSV-147] Error message optimization during faulty CSV record read
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #347:
URL: https://github.com/apache/commons-csv/pull/347#discussion_r1316570099
##########
src/main/java/org/apache/commons/csv/Lexer.java:
##########
@@ -367,8 +367,7 @@ private Token parseEncapsulatedToken(final Token token) throws IOException {
}
if (!Character.isWhitespace((char)c)) {
// error invalid char between token and next delimiter
- throw new IOException("(line " + getCurrentLineNumber() +
- ") invalid char between encapsulated token and delimiter");
+ throw new IOException("Invalid char between encapsulated token and delimiter at line: " + getCurrentLineNumber() + ", position: " + getCharacterPosition());
Review Comment:
This probably shouldn't be an IOException but that issue is not new with this PR
##########
src/test/java/org/apache/commons/csv/CSVParserTest.java:
##########
@@ -1642,4 +1642,26 @@ private void validateRecordPosition(final String lineSeparator) throws IOExcepti
parser.close();
}
+ @Test
+ public void testFaultyCSVShouldThrowExceptionWithLineAndPosition() throws IOException {
+ String csvContent = "col1,col2,col3,col4,col5,col6,col7,col8,col9,col10\n" +
+ "rec1,rec2,rec3,rec4,rec5,rec6,rec7,rec8,\"\"rec9\"\",rec10";
+
+ StringReader stringReader = new StringReader(csvContent);
+ CSVFormat csvFormat = CSVFormat.DEFAULT.builder()
+ .setHeader()
+ .setSkipHeaderRecord(true)
+ .build();
+
+ CSVParser csvParser = csvFormat.parse(stringReader);
+ Exception exception = assertThrows(UncheckedIOException.class, () -> {
Review Comment:
UncheckedIOException is not right either, but again not new in this PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] garydgregory commented on a diff in pull request #347: [CSV-147] Error message optimization during faulty CSV record read
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #347:
URL: https://github.com/apache/commons-csv/pull/347#discussion_r1313274252
##########
src/main/java/org/apache/commons/csv/CSVParser.java:
##########
@@ -765,7 +765,13 @@ CSVRecord nextRecord() throws IOException {
final long startCharPosition = lexer.getCharacterPosition() + this.characterOffset;
do {
this.reusableToken.reset();
- this.lexer.nextToken(this.reusableToken);
+ try {
+ this.lexer.nextToken(this.reusableToken);
+ } catch (IOException ioe) {
+ String errorMessage = "Exception during parsing at line: "
+ + this.lexer.getCurrentLineNumber() + ", position: " + this.lexer.getCharacterPosition();
Review Comment:
Wait a sec, why is this code in the parser instead of in the lexer? All this catch does it extract information out of the lexer, so why not have exception thrown by the lexer provide this information in the first place in a better message?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] gbidsilva commented on pull request #347: [CSV-147] Error message optimization during faulty CSV record read
Posted by "gbidsilva (via GitHub)" <gi...@apache.org>.
gbidsilva commented on PR #347:
URL: https://github.com/apache/commons-csv/pull/347#issuecomment-1699425939
@garydgregory :
That is a good point. But IMO, whenever someone log the exception, at that point they should consider the security risks.
Isn't usually exception provide such details? as I remember I have came across such scenarios.
What would be the best practice in this case ? - good to know :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] gbidsilva commented on a diff in pull request #347: [CSV-147] Error message optimization during faulty CSV record read
Posted by "gbidsilva (via GitHub)" <gi...@apache.org>.
gbidsilva commented on code in PR #347:
URL: https://github.com/apache/commons-csv/pull/347#discussion_r1311673503
##########
src/test/java/org/apache/commons/csv/CSVParserTest.java:
##########
@@ -1642,4 +1643,70 @@ private void validateRecordPosition(final String lineSeparator) throws IOExcepti
parser.close();
}
+ @Test
+ public void testFaultyCSVShouldThrowErrorAndDetailedMessageShouldBeAvailable_1() {
+ String csvContent = "col1,col2,col3,col4,col5,col6,col7,col8,col9,col10\n" +
+ "rec1,rec2,rec3,rec4,rec5,rec6,rec7,rec8,\"\"rec9\"\",rec10";
+
+ StringReader stringReader = new StringReader(csvContent);
+ CSVFormat csvFormat = CSVFormat.DEFAULT.builder()
+ .setHeader()
+ .setSkipHeaderRecord(true)
+ .build();
+
+ CSVParser csvParser = null;
+ try {
+ csvParser = csvFormat.parse(stringReader);
+ } catch (IOException e) {
+ fail("Failed to parse the CSV content");
Review Comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] gbidsilva commented on pull request #347: [CSV-147] Error message optimization during faulty CSV record read
Posted by "gbidsilva (via GitHub)" <gi...@apache.org>.
gbidsilva commented on PR #347:
URL: https://github.com/apache/commons-csv/pull/347#issuecomment-1706553247
@garydgregory @elharo
Exception handling moved to Lexer class.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] gbidsilva commented on a diff in pull request #347: [CSV-147] Error message optimization during faulty CSV record read
Posted by "gbidsilva (via GitHub)" <gi...@apache.org>.
gbidsilva commented on code in PR #347:
URL: https://github.com/apache/commons-csv/pull/347#discussion_r1311708454
##########
src/test/java/org/apache/commons/csv/CSVParserTest.java:
##########
@@ -1642,4 +1643,70 @@ private void validateRecordPosition(final String lineSeparator) throws IOExcepti
parser.close();
}
+ @Test
+ public void testFaultyCSVShouldThrowErrorAndDetailedMessageShouldBeAvailable_1() {
+ String csvContent = "col1,col2,col3,col4,col5,col6,col7,col8,col9,col10\n" +
+ "rec1,rec2,rec3,rec4,rec5,rec6,rec7,rec8,\"\"rec9\"\",rec10";
+
+ StringReader stringReader = new StringReader(csvContent);
+ CSVFormat csvFormat = CSVFormat.DEFAULT.builder()
+ .setHeader()
+ .setSkipHeaderRecord(true)
+ .build();
+
+ CSVParser csvParser = null;
+ try {
+ csvParser = csvFormat.parse(stringReader);
+ } catch (IOException e) {
+ fail("Failed to parse the CSV content");
+ }
+ final Iterable<CSVRecord> finalRecords = csvParser;
+ Exception exception = assertThrows(UncheckedIOException.class, () -> {
+ for (CSVRecord record : finalRecords) {
+ System.out.println(record.get(0) + " " + record.get(1) + " " + record.get(2) + " " + record.get(3)
+ + " " + record.get(4) + " " + record.get(5) + " " + record.get(6) + " " + record.get(7)
+ + " " + record.get(8) + " " + record.get(9));
+ }
+ });
+ String expectedErrorMessage = "Exception reading next record: java.io.IOException: An exception occurred " +
+ "while parsing the CSV content. Issue in line: 2, position: 94";
+ String actualMessage = exception.getMessage();
+ assertTrue(actualMessage.contains(expectedErrorMessage));
+ assertNotNull(csvParser);
Review Comment:
Got it!
I was referring to something else.
Removed.
##########
src/test/java/org/apache/commons/csv/CSVParserTest.java:
##########
@@ -1642,4 +1643,70 @@ private void validateRecordPosition(final String lineSeparator) throws IOExcepti
parser.close();
}
+ @Test
+ public void testFaultyCSVShouldThrowErrorAndDetailedMessageShouldBeAvailable_1() {
+ String csvContent = "col1,col2,col3,col4,col5,col6,col7,col8,col9,col10\n" +
+ "rec1,rec2,rec3,rec4,rec5,rec6,rec7,rec8,\"\"rec9\"\",rec10";
+
+ StringReader stringReader = new StringReader(csvContent);
+ CSVFormat csvFormat = CSVFormat.DEFAULT.builder()
+ .setHeader()
+ .setSkipHeaderRecord(true)
+ .build();
+
+ CSVParser csvParser = null;
+ try {
+ csvParser = csvFormat.parse(stringReader);
+ } catch (IOException e) {
+ fail("Failed to parse the CSV content");
+ }
+ final Iterable<CSVRecord> finalRecords = csvParser;
+ Exception exception = assertThrows(UncheckedIOException.class, () -> {
+ for (CSVRecord record : finalRecords) {
+ System.out.println(record.get(0) + " " + record.get(1) + " " + record.get(2) + " " + record.get(3)
+ + " " + record.get(4) + " " + record.get(5) + " " + record.get(6) + " " + record.get(7)
+ + " " + record.get(8) + " " + record.get(9));
+ }
+ });
+ String expectedErrorMessage = "Exception reading next record: java.io.IOException: An exception occurred " +
+ "while parsing the CSV content. Issue in line: 2, position: 94";
+ String actualMessage = exception.getMessage();
+ assertTrue(actualMessage.contains(expectedErrorMessage));
+ assertNotNull(csvParser);
+ String expectedLastParsedContent = "...rec4,rec5,rec6,rec7,rec8";
+ assertEquals(expectedLastParsedContent, csvParser.getLastParsedContent());
+ }
+
+ @Test
+ public void testFaultyCSVShouldThrowErrorAndDetailedMessageShouldBeAvailable_2() {
+ String csvContent = "col1,col2,col3,col4,col5,col6,col7,col8\n" +
+ "rec1,rec2,rec3,rec4,\"\"rec5\"\",rec6,rec7,rec8";
+
+ StringReader stringReader = new StringReader(csvContent);
+ CSVFormat csvFormat = CSVFormat.DEFAULT.builder()
+ .setHeader()
+ .setSkipHeaderRecord(true)
+ .build();
+
+ CSVParser csvParser = null;
Review Comment:
removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] gbidsilva commented on pull request #347: [CSV-147] Error message optimization during faulty CSV record read
Posted by "gbidsilva (via GitHub)" <gi...@apache.org>.
gbidsilva commented on PR #347:
URL: https://github.com/apache/commons-csv/pull/347#issuecomment-1701144851
@garydgregory @elharo :
Requested changes are done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] gbidsilva commented on pull request #347: [CSV-147] Error message optimization during faulty CSV record read
Posted by "gbidsilva (via GitHub)" <gi...@apache.org>.
gbidsilva commented on PR #347:
URL: https://github.com/apache/commons-csv/pull/347#issuecomment-1702144619
@garydgregory :
I understand your point. In this case, we will treat `getLastParsedContent` method as a new feature and decide whether it is actually needed or not in the product.
To make this PR work, I have removed the `getLastParsedContent` method content and only kept the positioning related changes.
Appreciate your feedback :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] gbidsilva commented on a diff in pull request #347: [CSV-147] Error message optimization during faulty CSV record read
Posted by "gbidsilva (via GitHub)" <gi...@apache.org>.
gbidsilva commented on code in PR #347:
URL: https://github.com/apache/commons-csv/pull/347#discussion_r1311699693
##########
src/test/java/org/apache/commons/csv/CSVParserTest.java:
##########
@@ -1642,4 +1643,70 @@ private void validateRecordPosition(final String lineSeparator) throws IOExcepti
parser.close();
}
+ @Test
+ public void testFaultyCSVShouldThrowErrorAndDetailedMessageShouldBeAvailable_1() {
+ String csvContent = "col1,col2,col3,col4,col5,col6,col7,col8,col9,col10\n" +
+ "rec1,rec2,rec3,rec4,rec5,rec6,rec7,rec8,\"\"rec9\"\",rec10";
+
+ StringReader stringReader = new StringReader(csvContent);
+ CSVFormat csvFormat = CSVFormat.DEFAULT.builder()
+ .setHeader()
+ .setSkipHeaderRecord(true)
+ .build();
+
+ CSVParser csvParser = null;
+ try {
+ csvParser = csvFormat.parse(stringReader);
+ } catch (IOException e) {
+ fail("Failed to parse the CSV content");
Review Comment:
please check now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] gbidsilva commented on a diff in pull request #347: [CSV-147] Error message optimization during faulty CSV record read
Posted by "gbidsilva (via GitHub)" <gi...@apache.org>.
gbidsilva commented on code in PR #347:
URL: https://github.com/apache/commons-csv/pull/347#discussion_r1311973714
##########
src/main/java/org/apache/commons/csv/CSVParser.java:
##########
@@ -438,6 +440,44 @@ public CSVParser(final Reader reader, final CSVFormat format, final long charact
this.recordNumber = recordNumber - 1;
}
+ /**
+ * Return the parsed CSV content of currently parsed line up until this method is called.
+ * <p>
+ * Maximum parsed token length set by the 'maxParsedTokenCount' is considered during the construction of return string.
+ * </p>
+ * <p>
+ * Example:
+ * </p>
+ * </p>
+ * If currently reading CSV record row contains following data and 'maxParsedTokenCount' is set to 5 and current reading position is col7
+ * </p>
+ * <pre>
+ * col1, col2, col3, col4, col5, col6, col7
+ * </pre>
+ * <p>
+ * then method returns following
+ * </p>
+ * <pre>
+ * col3, col4, col5, col6, col7
+ * </pre>
+ * @return parsed CSV content of current reading line
+ */
+ public String getLastParsedContent() {
Review Comment:
@garydgregory :
I think the whole point of this change is to provide more details on the error line in CSV to the user if there is any requirement.
In case if they want to know what has been parsed already and how it is parsed then they can use this method.
In fact, I think that is the original requirement of : https://issues.apache.org/jira/browse/CSV-147
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] elharo commented on a diff in pull request #347: [CSV-147] Error message optimization during faulty CSV record read
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #347:
URL: https://github.com/apache/commons-csv/pull/347#discussion_r1311531227
##########
src/main/java/org/apache/commons/csv/CSVParser.java:
##########
@@ -438,6 +440,44 @@ public CSVParser(final Reader reader, final CSVFormat format, final long charact
this.recordNumber = recordNumber - 1;
}
+ /**
+ * Return the parsed CSV content of current reading line up until this method is called.
Review Comment:
"reading line" is awkward. Consider rephrasing
##########
src/main/java/org/apache/commons/csv/CSVParser.java:
##########
@@ -438,6 +440,44 @@ public CSVParser(final Reader reader, final CSVFormat format, final long charact
this.recordNumber = recordNumber - 1;
}
+ /**
+ * Return the parsed CSV content of current reading line up until this method is called.
+ * <p>
+ * Maximum parsed token length set by the 'maxParsedTokenCount' is considered during the construction of return string.
+ * </p>
+ * <p>
+ * Example:
+ * </p>
+ * </p>
+ * If currently reading CSV record row contains following data and 'maxParsedTokenCount' is set to 5 and current reading position is col7
+ * </p>
+ * <pre>
+ * col1, col2, col3, col4, col5, col6, col7
+ * </pre>
+ * <p>
+ * then this would return following
Review Comment:
would return --> returns
##########
src/main/java/org/apache/commons/csv/CSVParser.java:
##########
@@ -765,7 +805,15 @@ CSVRecord nextRecord() throws IOException {
final long startCharPosition = lexer.getCharacterPosition() + this.characterOffset;
do {
this.reusableToken.reset();
- this.lexer.nextToken(this.reusableToken);
+ // https://issues.apache.org/jira/browse/CSV-147
+ try {
+ this.lexer.nextToken(this.reusableToken);
+ } catch (IOException ioe) {
+ String errorMessage = "An exception occurred while tying to parse the CSV content. Issue in line: "
Review Comment:
tying to parse --> parsing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] gbidsilva commented on a diff in pull request #347: [CSV-147] Error message optimization during faulty CSV record read
Posted by "gbidsilva (via GitHub)" <gi...@apache.org>.
gbidsilva commented on code in PR #347:
URL: https://github.com/apache/commons-csv/pull/347#discussion_r1314826686
##########
src/main/java/org/apache/commons/csv/CSVParser.java:
##########
@@ -765,7 +765,13 @@ CSVRecord nextRecord() throws IOException {
final long startCharPosition = lexer.getCharacterPosition() + this.characterOffset;
do {
this.reusableToken.reset();
- this.lexer.nextToken(this.reusableToken);
+ try {
+ this.lexer.nextToken(this.reusableToken);
+ } catch (IOException ioe) {
+ String errorMessage = "Exception during parsing at line: "
+ + this.lexer.getCurrentLineNumber() + ", position: " + this.lexer.getCharacterPosition();
Review Comment:
I will have a look on this...
Thanks for the review @garydgregory
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] garydgregory merged pull request #347: [CSV-147] Error message optimization during faulty CSV record read
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory merged PR #347:
URL: https://github.com/apache/commons-csv/pull/347
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] gbidsilva commented on pull request #347: [CSV-147] Error message optimization during faulty CSV record read
Posted by "gbidsilva (via GitHub)" <gi...@apache.org>.
gbidsilva commented on PR #347:
URL: https://github.com/apache/commons-csv/pull/347#issuecomment-1713838677
@garydgregory : Checkstyle issue has been fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] garydgregory commented on pull request #347: [CSV-147] Error message optimization during faulty CSV record read
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #347:
URL: https://github.com/apache/commons-csv/pull/347#issuecomment-1699381640
I'm OK with adding the position but I am guessing someone will create a security issue for data exfiltration.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] garydgregory commented on a diff in pull request #347: [CSV-147] Error message optimization during faulty CSV record read
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #347:
URL: https://github.com/apache/commons-csv/pull/347#discussion_r1311663575
##########
src/main/java/org/apache/commons/csv/CSVParser.java:
##########
@@ -765,7 +805,14 @@ CSVRecord nextRecord() throws IOException {
final long startCharPosition = lexer.getCharacterPosition() + this.characterOffset;
do {
this.reusableToken.reset();
- this.lexer.nextToken(this.reusableToken);
+ // https://issues.apache.org/jira/browse/CSV-147
+ try {
+ this.lexer.nextToken(this.reusableToken);
+ } catch (IOException ioe) {
+ String errorMessage = "An exception occurred while parsing the CSV content. Issue in line: "
+ + this.lexer.getCurrentLineNumber() + ", position: " + this.lexer.getCharacterPosition();
Review Comment:
Simply message: "Exception parsing at line N, column C."
##########
src/main/java/org/apache/commons/csv/CSVParser.java:
##########
@@ -765,7 +805,14 @@ CSVRecord nextRecord() throws IOException {
final long startCharPosition = lexer.getCharacterPosition() + this.characterOffset;
do {
this.reusableToken.reset();
- this.lexer.nextToken(this.reusableToken);
+ // https://issues.apache.org/jira/browse/CSV-147
Review Comment:
No need for this comment IMO.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] gbidsilva commented on a diff in pull request #347: [CSV-147] Error message optimization during faulty CSV record read
Posted by "gbidsilva (via GitHub)" <gi...@apache.org>.
gbidsilva commented on code in PR #347:
URL: https://github.com/apache/commons-csv/pull/347#discussion_r1311698594
##########
src/main/java/org/apache/commons/csv/CSVParser.java:
##########
@@ -765,7 +805,14 @@ CSVRecord nextRecord() throws IOException {
final long startCharPosition = lexer.getCharacterPosition() + this.characterOffset;
do {
this.reusableToken.reset();
- this.lexer.nextToken(this.reusableToken);
+ // https://issues.apache.org/jira/browse/CSV-147
+ try {
+ this.lexer.nextToken(this.reusableToken);
+ } catch (IOException ioe) {
+ String errorMessage = "An exception occurred while parsing the CSV content. Issue in line: "
+ + this.lexer.getCurrentLineNumber() + ", position: " + this.lexer.getCharacterPosition();
Review Comment:
Done.
However, column and position are two different things. Therefore kept the position as it is.
lexer is not having any method to access the column as I see.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] gbidsilva commented on pull request #347: [CSV-147] Error message optimization during faulty CSV record read
Posted by "gbidsilva (via GitHub)" <gi...@apache.org>.
gbidsilva commented on PR #347:
URL: https://github.com/apache/commons-csv/pull/347#issuecomment-1716887748
> @gbidsilva Thank you for your updates. Please see my comments.
Completed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] gbidsilva commented on a diff in pull request #347: [CSV-147] Error message optimization during faulty CSV record read
Posted by "gbidsilva (via GitHub)" <gi...@apache.org>.
gbidsilva commented on code in PR #347:
URL: https://github.com/apache/commons-csv/pull/347#discussion_r1311674342
##########
src/test/java/org/apache/commons/csv/CSVParserTest.java:
##########
@@ -1642,4 +1643,70 @@ private void validateRecordPosition(final String lineSeparator) throws IOExcepti
parser.close();
}
+ @Test
+ public void testFaultyCSVShouldThrowErrorAndDetailedMessageShouldBeAvailable_1() {
+ String csvContent = "col1,col2,col3,col4,col5,col6,col7,col8,col9,col10\n" +
+ "rec1,rec2,rec3,rec4,rec5,rec6,rec7,rec8,\"\"rec9\"\",rec10";
+
+ StringReader stringReader = new StringReader(csvContent);
+ CSVFormat csvFormat = CSVFormat.DEFAULT.builder()
+ .setHeader()
+ .setSkipHeaderRecord(true)
+ .build();
+
+ CSVParser csvParser = null;
+ try {
+ csvParser = csvFormat.parse(stringReader);
+ } catch (IOException e) {
+ fail("Failed to parse the CSV content");
+ }
+ final Iterable<CSVRecord> finalRecords = csvParser;
Review Comment:
Once above try catch is removed, we can simplify this.
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] elharo commented on a diff in pull request #347: [CSV-147] Error message optimization during faulty CSV record read
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #347:
URL: https://github.com/apache/commons-csv/pull/347#discussion_r1311695526
##########
src/main/java/org/apache/commons/csv/CSVParser.java:
##########
@@ -438,6 +440,44 @@ public CSVParser(final Reader reader, final CSVFormat format, final long charact
this.recordNumber = recordNumber - 1;
}
+ /**
+ * Return the parsed CSV content of currently parsed line up until this method is called.
Review Comment:
If it's just a string I'm not sure it's "parsed". Maybe just "Return the text of the current line up until this method is called."
##########
src/test/java/org/apache/commons/csv/CSVParserTest.java:
##########
@@ -1642,4 +1643,70 @@ private void validateRecordPosition(final String lineSeparator) throws IOExcepti
parser.close();
}
+ @Test
+ public void testFaultyCSVShouldThrowErrorAndDetailedMessageShouldBeAvailable_1() {
+ String csvContent = "col1,col2,col3,col4,col5,col6,col7,col8,col9,col10\n" +
+ "rec1,rec2,rec3,rec4,rec5,rec6,rec7,rec8,\"\"rec9\"\",rec10";
+
+ StringReader stringReader = new StringReader(csvContent);
+ CSVFormat csvFormat = CSVFormat.DEFAULT.builder()
+ .setHeader()
+ .setSkipHeaderRecord(true)
+ .build();
+
+ CSVParser csvParser = null;
+ try {
+ csvParser = csvFormat.parse(stringReader);
+ } catch (IOException e) {
+ fail("Failed to parse the CSV content");
Review Comment:
I still see it. Do you need to push?
##########
src/test/java/org/apache/commons/csv/CSVParserTest.java:
##########
@@ -1642,4 +1643,70 @@ private void validateRecordPosition(final String lineSeparator) throws IOExcepti
parser.close();
}
+ @Test
+ public void testFaultyCSVShouldThrowErrorAndDetailedMessageShouldBeAvailable_1() {
+ String csvContent = "col1,col2,col3,col4,col5,col6,col7,col8,col9,col10\n" +
+ "rec1,rec2,rec3,rec4,rec5,rec6,rec7,rec8,\"\"rec9\"\",rec10";
+
+ StringReader stringReader = new StringReader(csvContent);
+ CSVFormat csvFormat = CSVFormat.DEFAULT.builder()
+ .setHeader()
+ .setSkipHeaderRecord(true)
+ .build();
+
+ CSVParser csvParser = null;
+ try {
+ csvParser = csvFormat.parse(stringReader);
+ } catch (IOException e) {
+ fail("Failed to parse the CSV content");
+ }
+ final Iterable<CSVRecord> finalRecords = csvParser;
+ Exception exception = assertThrows(UncheckedIOException.class, () -> {
+ for (CSVRecord record : finalRecords) {
+ System.out.println(record.get(0) + " " + record.get(1) + " " + record.get(2) + " " + record.get(3)
+ + " " + record.get(4) + " " + record.get(5) + " " + record.get(6) + " " + record.get(7)
+ + " " + record.get(8) + " " + record.get(9));
+ }
+ });
+ String expectedErrorMessage = "Exception reading next record: java.io.IOException: An exception occurred " +
+ "while parsing the CSV content. Issue in line: 2, position: 94";
+ String actualMessage = exception.getMessage();
+ assertTrue(actualMessage.contains(expectedErrorMessage));
+ assertNotNull(csvParser);
Review Comment:
csvParser is guaranteed non-null by the time this line is reached
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] garydgregory commented on a diff in pull request #347: [CSV-147] Error message optimization during faulty CSV record read
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #347:
URL: https://github.com/apache/commons-csv/pull/347#discussion_r1313274252
##########
src/main/java/org/apache/commons/csv/CSVParser.java:
##########
@@ -765,7 +765,13 @@ CSVRecord nextRecord() throws IOException {
final long startCharPosition = lexer.getCharacterPosition() + this.characterOffset;
do {
this.reusableToken.reset();
- this.lexer.nextToken(this.reusableToken);
+ try {
+ this.lexer.nextToken(this.reusableToken);
+ } catch (IOException ioe) {
+ String errorMessage = "Exception during parsing at line: "
+ + this.lexer.getCurrentLineNumber() + ", position: " + this.lexer.getCharacterPosition();
Review Comment:
Wait a sec, why is code in the parser instead of in the lexer? All this catch does it extract information out of the lexer, so why not have exception thrown by the lexer provide this information in the first place in a better message?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] gbidsilva commented on pull request #347: [CSV-147] Error message optimization during faulty CSV record read
Posted by "gbidsilva (via GitHub)" <gi...@apache.org>.
gbidsilva commented on PR #347:
URL: https://github.com/apache/commons-csv/pull/347#issuecomment-1701036226
@garydgregory:
I have looked around what would be the best practice in adding data records in Exceptions. It seems that arguments are there for both the sides. However, as you suggested, we should avoid adding such details in Exception and keep it consistent with how commons were creating exceptions so far.
To rectify this, I have removed the content part from the exception message and made 'getLastParsedContent()' method public. Therefore, if there is any need then anyone can call that method and can get the last parsed content.
Test cases are added to the PR :-)
Appreciate your feedback on this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] elharo commented on a diff in pull request #347: [CSV-147] Error message optimization during faulty CSV record read
Posted by "elharo (via GitHub)" <gi...@apache.org>.
elharo commented on code in PR #347:
URL: https://github.com/apache/commons-csv/pull/347#discussion_r1311642239
##########
src/test/java/org/apache/commons/csv/CSVParserTest.java:
##########
@@ -1642,4 +1643,70 @@ private void validateRecordPosition(final String lineSeparator) throws IOExcepti
parser.close();
}
+ @Test
+ public void testFaultyCSVShouldThrowErrorAndDetailedMessageShouldBeAvailable_1() {
+ String csvContent = "col1,col2,col3,col4,col5,col6,col7,col8,col9,col10\n" +
+ "rec1,rec2,rec3,rec4,rec5,rec6,rec7,rec8,\"\"rec9\"\",rec10";
+
+ StringReader stringReader = new StringReader(csvContent);
+ CSVFormat csvFormat = CSVFormat.DEFAULT.builder()
+ .setHeader()
+ .setSkipHeaderRecord(true)
+ .build();
+
+ CSVParser csvParser = null;
+ try {
+ csvParser = csvFormat.parse(stringReader);
+ } catch (IOException e) {
+ fail("Failed to parse the CSV content");
+ }
+ final Iterable<CSVRecord> finalRecords = csvParser;
+ Exception exception = assertThrows(UncheckedIOException.class, () -> {
+ for (CSVRecord record : finalRecords) {
+ System.out.println(record.get(0) + " " + record.get(1) + " " + record.get(2) + " " + record.get(3)
Review Comment:
don't print anything except test failure from tests so failures are easier to find
##########
src/test/java/org/apache/commons/csv/CSVParserTest.java:
##########
@@ -1642,4 +1643,70 @@ private void validateRecordPosition(final String lineSeparator) throws IOExcepti
parser.close();
}
+ @Test
+ public void testFaultyCSVShouldThrowErrorAndDetailedMessageShouldBeAvailable_1() {
+ String csvContent = "col1,col2,col3,col4,col5,col6,col7,col8,col9,col10\n" +
+ "rec1,rec2,rec3,rec4,rec5,rec6,rec7,rec8,\"\"rec9\"\",rec10";
+
+ StringReader stringReader = new StringReader(csvContent);
+ CSVFormat csvFormat = CSVFormat.DEFAULT.builder()
+ .setHeader()
+ .setSkipHeaderRecord(true)
+ .build();
+
+ CSVParser csvParser = null;
+ try {
+ csvParser = csvFormat.parse(stringReader);
+ } catch (IOException e) {
+ fail("Failed to parse the CSV content");
+ }
+ final Iterable<CSVRecord> finalRecords = csvParser;
Review Comment:
I don't see why there's a new variable for the same thing. Can you just csvParser?
##########
src/test/java/org/apache/commons/csv/CSVParserTest.java:
##########
@@ -1642,4 +1643,70 @@ private void validateRecordPosition(final String lineSeparator) throws IOExcepti
parser.close();
}
+ @Test
+ public void testFaultyCSVShouldThrowErrorAndDetailedMessageShouldBeAvailable_1() {
+ String csvContent = "col1,col2,col3,col4,col5,col6,col7,col8,col9,col10\n" +
+ "rec1,rec2,rec3,rec4,rec5,rec6,rec7,rec8,\"\"rec9\"\",rec10";
+
+ StringReader stringReader = new StringReader(csvContent);
+ CSVFormat csvFormat = CSVFormat.DEFAULT.builder()
+ .setHeader()
+ .setSkipHeaderRecord(true)
+ .build();
+
+ CSVParser csvParser = null;
+ try {
+ csvParser = csvFormat.parse(stringReader);
+ } catch (IOException e) {
+ fail("Failed to parse the CSV content");
+ }
+ final Iterable<CSVRecord> finalRecords = csvParser;
+ Exception exception = assertThrows(UncheckedIOException.class, () -> {
+ for (CSVRecord record : finalRecords) {
+ System.out.println(record.get(0) + " " + record.get(1) + " " + record.get(2) + " " + record.get(3)
+ + " " + record.get(4) + " " + record.get(5) + " " + record.get(6) + " " + record.get(7)
+ + " " + record.get(8) + " " + record.get(9));
+ }
+ });
+ String expectedErrorMessage = "Exception reading next record: java.io.IOException: An exception occurred " +
+ "while parsing the CSV content. Issue in line: 2, position: 94";
+ String actualMessage = exception.getMessage();
+ assertTrue(actualMessage.contains(expectedErrorMessage));
+ assertNotNull(csvParser);
+ String expectedLastParsedContent = "...rec4,rec5,rec6,rec7,rec8";
+ assertEquals(expectedLastParsedContent, csvParser.getLastParsedContent());
+ }
+
+ @Test
+ public void testFaultyCSVShouldThrowErrorAndDetailedMessageShouldBeAvailable_2() {
+ String csvContent = "col1,col2,col3,col4,col5,col6,col7,col8\n" +
+ "rec1,rec2,rec3,rec4,\"\"rec5\"\",rec6,rec7,rec8";
+
+ StringReader stringReader = new StringReader(csvContent);
+ CSVFormat csvFormat = CSVFormat.DEFAULT.builder()
+ .setHeader()
+ .setSkipHeaderRecord(true)
+ .build();
+
+ CSVParser csvParser = null;
Review Comment:
same comments as above
##########
src/test/java/org/apache/commons/csv/CSVParserTest.java:
##########
@@ -1642,4 +1643,70 @@ private void validateRecordPosition(final String lineSeparator) throws IOExcepti
parser.close();
}
+ @Test
+ public void testFaultyCSVShouldThrowErrorAndDetailedMessageShouldBeAvailable_1() {
+ String csvContent = "col1,col2,col3,col4,col5,col6,col7,col8,col9,col10\n" +
+ "rec1,rec2,rec3,rec4,rec5,rec6,rec7,rec8,\"\"rec9\"\",rec10";
+
+ StringReader stringReader = new StringReader(csvContent);
+ CSVFormat csvFormat = CSVFormat.DEFAULT.builder()
+ .setHeader()
+ .setSkipHeaderRecord(true)
+ .build();
+
+ CSVParser csvParser = null;
+ try {
+ csvParser = csvFormat.parse(stringReader);
+ } catch (IOException e) {
+ fail("Failed to parse the CSV content");
+ }
+ final Iterable<CSVRecord> finalRecords = csvParser;
+ Exception exception = assertThrows(UncheckedIOException.class, () -> {
+ for (CSVRecord record : finalRecords) {
+ System.out.println(record.get(0) + " " + record.get(1) + " " + record.get(2) + " " + record.get(3)
+ + " " + record.get(4) + " " + record.get(5) + " " + record.get(6) + " " + record.get(7)
+ + " " + record.get(8) + " " + record.get(9));
+ }
+ });
+ String expectedErrorMessage = "Exception reading next record: java.io.IOException: An exception occurred " +
+ "while parsing the CSV content. Issue in line: 2, position: 94";
+ String actualMessage = exception.getMessage();
+ assertTrue(actualMessage.contains(expectedErrorMessage));
+ assertNotNull(csvParser);
Review Comment:
this assert doesn't seem helpful
##########
src/test/java/org/apache/commons/csv/CSVParserTest.java:
##########
@@ -1642,4 +1643,70 @@ private void validateRecordPosition(final String lineSeparator) throws IOExcepti
parser.close();
}
+ @Test
+ public void testFaultyCSVShouldThrowErrorAndDetailedMessageShouldBeAvailable_1() {
+ String csvContent = "col1,col2,col3,col4,col5,col6,col7,col8,col9,col10\n" +
+ "rec1,rec2,rec3,rec4,rec5,rec6,rec7,rec8,\"\"rec9\"\",rec10";
+
+ StringReader stringReader = new StringReader(csvContent);
+ CSVFormat csvFormat = CSVFormat.DEFAULT.builder()
+ .setHeader()
+ .setSkipHeaderRecord(true)
+ .build();
+
+ CSVParser csvParser = null;
+ try {
+ csvParser = csvFormat.parse(stringReader);
+ } catch (IOException e) {
+ fail("Failed to parse the CSV content");
Review Comment:
not needed, an exception will fail the test. Remove the try-catch
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] gbidsilva commented on a diff in pull request #347: [CSV-147] Error message optimization during faulty CSV record read
Posted by "gbidsilva (via GitHub)" <gi...@apache.org>.
gbidsilva commented on code in PR #347:
URL: https://github.com/apache/commons-csv/pull/347#discussion_r1311680340
##########
src/test/java/org/apache/commons/csv/CSVParserTest.java:
##########
@@ -1642,4 +1643,70 @@ private void validateRecordPosition(final String lineSeparator) throws IOExcepti
parser.close();
}
+ @Test
+ public void testFaultyCSVShouldThrowErrorAndDetailedMessageShouldBeAvailable_1() {
+ String csvContent = "col1,col2,col3,col4,col5,col6,col7,col8,col9,col10\n" +
+ "rec1,rec2,rec3,rec4,rec5,rec6,rec7,rec8,\"\"rec9\"\",rec10";
+
+ StringReader stringReader = new StringReader(csvContent);
+ CSVFormat csvFormat = CSVFormat.DEFAULT.builder()
+ .setHeader()
+ .setSkipHeaderRecord(true)
+ .build();
+
+ CSVParser csvParser = null;
+ try {
+ csvParser = csvFormat.parse(stringReader);
+ } catch (IOException e) {
+ fail("Failed to parse the CSV content");
+ }
+ final Iterable<CSVRecord> finalRecords = csvParser;
+ Exception exception = assertThrows(UncheckedIOException.class, () -> {
+ for (CSVRecord record : finalRecords) {
+ System.out.println(record.get(0) + " " + record.get(1) + " " + record.get(2) + " " + record.get(3)
+ + " " + record.get(4) + " " + record.get(5) + " " + record.get(6) + " " + record.get(7)
+ + " " + record.get(8) + " " + record.get(9));
+ }
+ });
+ String expectedErrorMessage = "Exception reading next record: java.io.IOException: An exception occurred " +
+ "while parsing the CSV content. Issue in line: 2, position: 94";
+ String actualMessage = exception.getMessage();
+ assertTrue(actualMessage.contains(expectedErrorMessage));
+ assertNotNull(csvParser);
Review Comment:
this is just to understand the usage and how the error message looks like for anyone looking at the test cases to figure the behavior.
##########
src/test/java/org/apache/commons/csv/CSVParserTest.java:
##########
@@ -1642,4 +1643,70 @@ private void validateRecordPosition(final String lineSeparator) throws IOExcepti
parser.close();
}
+ @Test
+ public void testFaultyCSVShouldThrowErrorAndDetailedMessageShouldBeAvailable_1() {
+ String csvContent = "col1,col2,col3,col4,col5,col6,col7,col8,col9,col10\n" +
+ "rec1,rec2,rec3,rec4,rec5,rec6,rec7,rec8,\"\"rec9\"\",rec10";
+
+ StringReader stringReader = new StringReader(csvContent);
+ CSVFormat csvFormat = CSVFormat.DEFAULT.builder()
+ .setHeader()
+ .setSkipHeaderRecord(true)
+ .build();
+
+ CSVParser csvParser = null;
+ try {
+ csvParser = csvFormat.parse(stringReader);
+ } catch (IOException e) {
+ fail("Failed to parse the CSV content");
+ }
+ final Iterable<CSVRecord> finalRecords = csvParser;
+ Exception exception = assertThrows(UncheckedIOException.class, () -> {
+ for (CSVRecord record : finalRecords) {
+ System.out.println(record.get(0) + " " + record.get(1) + " " + record.get(2) + " " + record.get(3)
+ + " " + record.get(4) + " " + record.get(5) + " " + record.get(6) + " " + record.get(7)
+ + " " + record.get(8) + " " + record.get(9));
+ }
+ });
+ String expectedErrorMessage = "Exception reading next record: java.io.IOException: An exception occurred " +
+ "while parsing the CSV content. Issue in line: 2, position: 94";
+ String actualMessage = exception.getMessage();
+ assertTrue(actualMessage.contains(expectedErrorMessage));
+ assertNotNull(csvParser);
+ String expectedLastParsedContent = "...rec4,rec5,rec6,rec7,rec8";
+ assertEquals(expectedLastParsedContent, csvParser.getLastParsedContent());
+ }
+
+ @Test
+ public void testFaultyCSVShouldThrowErrorAndDetailedMessageShouldBeAvailable_2() {
+ String csvContent = "col1,col2,col3,col4,col5,col6,col7,col8\n" +
+ "rec1,rec2,rec3,rec4,\"\"rec5\"\",rec6,rec7,rec8";
+
+ StringReader stringReader = new StringReader(csvContent);
+ CSVFormat csvFormat = CSVFormat.DEFAULT.builder()
+ .setHeader()
+ .setSkipHeaderRecord(true)
+ .build();
+
+ CSVParser csvParser = null;
Review Comment:
this is just to understand the usage and how the error message looks like for anyone looking at the test cases to figure the behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] gbidsilva commented on a diff in pull request #347: [CSV-147] Error message optimization during faulty CSV record read
Posted by "gbidsilva (via GitHub)" <gi...@apache.org>.
gbidsilva commented on code in PR #347:
URL: https://github.com/apache/commons-csv/pull/347#discussion_r1311678295
##########
src/test/java/org/apache/commons/csv/CSVParserTest.java:
##########
@@ -1642,4 +1643,70 @@ private void validateRecordPosition(final String lineSeparator) throws IOExcepti
parser.close();
}
+ @Test
+ public void testFaultyCSVShouldThrowErrorAndDetailedMessageShouldBeAvailable_1() {
+ String csvContent = "col1,col2,col3,col4,col5,col6,col7,col8,col9,col10\n" +
+ "rec1,rec2,rec3,rec4,rec5,rec6,rec7,rec8,\"\"rec9\"\",rec10";
+
+ StringReader stringReader = new StringReader(csvContent);
+ CSVFormat csvFormat = CSVFormat.DEFAULT.builder()
+ .setHeader()
+ .setSkipHeaderRecord(true)
+ .build();
+
+ CSVParser csvParser = null;
+ try {
+ csvParser = csvFormat.parse(stringReader);
+ } catch (IOException e) {
+ fail("Failed to parse the CSV content");
+ }
+ final Iterable<CSVRecord> finalRecords = csvParser;
+ Exception exception = assertThrows(UncheckedIOException.class, () -> {
+ for (CSVRecord record : finalRecords) {
+ System.out.println(record.get(0) + " " + record.get(1) + " " + record.get(2) + " " + record.get(3)
Review Comment:
done and asserted instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] gbidsilva commented on a diff in pull request #347: [CSV-147] Error message optimization during faulty CSV record read
Posted by "gbidsilva (via GitHub)" <gi...@apache.org>.
gbidsilva commented on code in PR #347:
URL: https://github.com/apache/commons-csv/pull/347#discussion_r1311703780
##########
src/main/java/org/apache/commons/csv/CSVParser.java:
##########
@@ -438,6 +440,44 @@ public CSVParser(final Reader reader, final CSVFormat format, final long charact
this.recordNumber = recordNumber - 1;
}
+ /**
+ * Return the parsed CSV content of currently parsed line up until this method is called.
Review Comment:
What we have in the buffer up until the error point is parsed as tokens. So I feel this is kind of OK.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] garydgregory commented on a diff in pull request #347: [CSV-147] Error message optimization during faulty CSV record read
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #347:
URL: https://github.com/apache/commons-csv/pull/347#discussion_r1311959935
##########
src/main/java/org/apache/commons/csv/CSVParser.java:
##########
@@ -765,7 +805,14 @@ CSVRecord nextRecord() throws IOException {
final long startCharPosition = lexer.getCharacterPosition() + this.characterOffset;
do {
this.reusableToken.reset();
- this.lexer.nextToken(this.reusableToken);
+ // https://issues.apache.org/jira/browse/CSV-147
+ try {
+ this.lexer.nextToken(this.reusableToken);
+ } catch (IOException ioe) {
+ String errorMessage = "An exception occurred while parsing the CSV content. Issue in line: "
+ + this.lexer.getCurrentLineNumber() + ", position: " + this.lexer.getCharacterPosition();
Review Comment:
> Done. However, column and position are two different things. Therefore kept the position as it is. lexer is not having any method to access the column as I see.
I meant position, sorry.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] garydgregory commented on a diff in pull request #347: [CSV-147] Error message optimization during faulty CSV record read
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #347:
URL: https://github.com/apache/commons-csv/pull/347#discussion_r1311964030
##########
src/main/java/org/apache/commons/csv/CSVParser.java:
##########
@@ -438,6 +440,44 @@ public CSVParser(final Reader reader, final CSVFormat format, final long charact
this.recordNumber = recordNumber - 1;
}
+ /**
+ * Return the parsed CSV content of currently parsed line up until this method is called.
+ * <p>
+ * Maximum parsed token length set by the 'maxParsedTokenCount' is considered during the construction of return string.
+ * </p>
+ * <p>
+ * Example:
+ * </p>
+ * </p>
+ * If currently reading CSV record row contains following data and 'maxParsedTokenCount' is set to 5 and current reading position is col7
+ * </p>
+ * <pre>
+ * col1, col2, col3, col4, col5, col6, col7
+ * </pre>
+ * <p>
+ * then method returns following
+ * </p>
+ * <pre>
+ * col3, col4, col5, col6, col7
+ * </pre>
+ * @return parsed CSV content of current reading line
+ */
+ public String getLastParsedContent() {
Review Comment:
I don't think this is needed anymore.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-csv] gbidsilva commented on pull request #347: [CSV-147] Error message optimization during faulty CSV record read
Posted by "gbidsilva (via GitHub)" <gi...@apache.org>.
gbidsilva commented on PR #347:
URL: https://github.com/apache/commons-csv/pull/347#issuecomment-1702728548
@garydgregory : let us know if there is anymore change to be done in this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org