[CSV-147] Better error message during faulty CSV record read #347

gbidsilva · 2023-08-30T08:12:27Z

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

Update
It has been decided to only to add the record position into the exception message and treat getLastParsedContent method as a new feature. Therefore this PR only contains the position related changes.

…into CSV-147

garydgregory · 2023-08-30T15:19:24Z

I'm OK with adding the position but I am guessing someone will create a security issue for data exfiltration.

gbidsilva · 2023-08-30T15:45:11Z

@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 :-)

src/main/java/org/apache/commons/csv/CSVParser.java

…espective method public

gbidsilva · 2023-08-31T13:20:36Z

@elharo: Thank you for the feedback. Changes are updated in the PR now.

gbidsilva · 2023-08-31T13:25:38Z

@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.

src/test/java/org/apache/commons/csv/CSVParserTest.java

garydgregory

@gbidsilva
Thank you for your updates. Please see my comments.

src/main/java/org/apache/commons/csv/CSVParser.java

src/test/java/org/apache/commons/csv/CSVParserTest.java

gbidsilva · 2023-08-31T14:19:06Z

@garydgregory @elharo :
Requested changes are done.

src/main/java/org/apache/commons/csv/CSVParser.java

gbidsilva · 2023-09-01T04:36:04Z

@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 :-)

gbidsilva · 2023-09-01T13:12:00Z

@garydgregory : let us know if there is anymore change to be done in this PR.

src/main/java/org/apache/commons/csv/CSVParser.java

gbidsilva · 2023-09-05T12:45:03Z

@garydgregory @elharo
Exception handling moved to Lexer class.

elharo · 2023-09-06T01:00:56Z

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());


This probably shouldn't be an IOException but that issue is not new with this PR

elharo · 2023-09-06T01:02:00Z

src/test/java/org/apache/commons/csv/CSVParserTest.java

+                .build();
+
+        CSVParser csvParser = csvFormat.parse(stringReader);
+        Exception exception = assertThrows(UncheckedIOException.class, () -> {


UncheckedIOException is not right either, but again not new in this PR

gbidsilva · 2023-09-11T13:03:48Z

@garydgregory : Checkstyle issue has been fixed.

gbidsilva · 2023-09-12T12:23:40Z

@garydgregory : Anything pending from development side for this to be merged ?

gbidsilva · 2023-09-13T03:31:49Z

@gbidsilva Thank you for your updates. Please see my comments.

Completed.

codecov-commenter · 2023-09-13T12:17:30Z

Codecov Report

Merging #347 (c0759cd) into master (7104598) will increase coverage by 0.00%.
Report is 13 commits behind head on master.
The diff coverage is 100.00%.

@@            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	Coverage Δ
src/main/java/org/apache/commons/csv/Lexer.java	`98.81% <100.00%> (+<0.01%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

garydgregory

LGTM

Clean up new test

gbidsilva added 9 commits August 25, 2023 16:54

adding test file

e1f8da1

adding more details into the error message during the CSV parser issues

72ed67b

Merge branch 'apache:master' into CSV-147

1e8014b

removing test file

991763f

Merge branch 'CSV-147' of https://github.com/gbidsilva/commons-csv-osc …

b4abb01

…into CSV-147

formatting changes

f0391ea

changing the message

e60b8ac

Merge branch 'apache:master' into CSV-147

156f1f5

Aligning error messages to be align with master branch

9c50c7b

elharo suggested changes Aug 31, 2023

View reviewed changes

src/main/java/org/apache/commons/csv/CSVParser.java Outdated Show resolved Hide resolved

src/main/java/org/apache/commons/csv/CSVParser.java Outdated Show resolved Hide resolved

src/main/java/org/apache/commons/csv/CSVParser.java Outdated Show resolved Hide resolved

gbidsilva added 2 commits August 31, 2023 18:33

removing last parsed content from exception message, instead making r…

ae8c0cd

…espective method public

adding code review typo fixes

2448f4f

elharo suggested changes Aug 31, 2023

View reviewed changes

garydgregory requested changes Aug 31, 2023

View reviewed changes

src/main/java/org/apache/commons/csv/CSVParser.java Outdated Show resolved Hide resolved

src/main/java/org/apache/commons/csv/CSVParser.java Outdated Show resolved Hide resolved

fixing code review changes

76ee73d

elharo reviewed Aug 31, 2023

View reviewed changes

src/main/java/org/apache/commons/csv/CSVParser.java Outdated Show resolved Hide resolved

src/test/java/org/apache/commons/csv/CSVParserTest.java Outdated Show resolved Hide resolved

src/test/java/org/apache/commons/csv/CSVParserTest.java Outdated Show resolved Hide resolved

removing unwanted assert

2f44a6b

gbidsilva requested a review from garydgregory August 31, 2023 14:20

garydgregory reviewed Aug 31, 2023

View reviewed changes

src/main/java/org/apache/commons/csv/CSVParser.java Outdated Show resolved Hide resolved

gbidsilva added 2 commits September 1, 2023 09:55

breaking the PR into to two parts by removing the new method

4b1d09a

fixing imports

76d5a65

gbidsilva requested a review from garydgregory September 1, 2023 05:47

removing unused variable

7273a04

garydgregory reviewed Sep 1, 2023

View reviewed changes

src/main/java/org/apache/commons/csv/CSVParser.java Outdated Show resolved Hide resolved

moving exception handling to the Lexer

0532276

elharo approved these changes Sep 6, 2023

View reviewed changes

gbidsilva requested a review from garydgregory September 6, 2023 04:50

fixing checkstyle issue

c0759cd

elharo approved these changes Sep 13, 2023

View reviewed changes

garydgregory approved these changes Sep 13, 2023

View reviewed changes

garydgregory merged commit a33b24c into apache:master Sep 13, 2023
6 checks passed

garydgregory changed the title ~~[CSV-147] Error message optimization during faulty CSV record read~~ [CSV-147] Better error message during faulty CSV record read Sep 13, 2023

asfgit pushed a commit that referenced this pull request Sep 13, 2023

[CSV-147] Better error message during faulty CSV record read #347

dbdd56c

Clean up new test

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CSV-147] Better error message during faulty CSV record read #347

[CSV-147] Better error message during faulty CSV record read #347

gbidsilva commented Aug 30, 2023 •

edited

garydgregory commented Aug 30, 2023

gbidsilva commented Aug 30, 2023

gbidsilva commented Aug 31, 2023

gbidsilva commented Aug 31, 2023

garydgregory left a comment

gbidsilva commented Aug 31, 2023

gbidsilva commented Sep 1, 2023

gbidsilva commented Sep 1, 2023

gbidsilva commented Sep 5, 2023

elharo Sep 6, 2023

elharo Sep 6, 2023

gbidsilva commented Sep 11, 2023

gbidsilva commented Sep 12, 2023

gbidsilva commented Sep 13, 2023

codecov-commenter commented Sep 13, 2023

garydgregory left a comment

[CSV-147] Better error message during faulty CSV record read #347

[CSV-147] Better error message during faulty CSV record read #347

Conversation

gbidsilva commented Aug 30, 2023 • edited

garydgregory commented Aug 30, 2023

gbidsilva commented Aug 30, 2023

gbidsilva commented Aug 31, 2023

gbidsilva commented Aug 31, 2023

garydgregory left a comment

Choose a reason for hiding this comment

gbidsilva commented Aug 31, 2023

gbidsilva commented Sep 1, 2023

gbidsilva commented Sep 1, 2023

gbidsilva commented Sep 5, 2023

elharo Sep 6, 2023

Choose a reason for hiding this comment

elharo Sep 6, 2023

Choose a reason for hiding this comment

gbidsilva commented Sep 11, 2023

gbidsilva commented Sep 12, 2023

gbidsilva commented Sep 13, 2023

codecov-commenter commented Sep 13, 2023

Codecov Report

garydgregory left a comment

Choose a reason for hiding this comment

gbidsilva commented Aug 30, 2023 •

edited