[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

                            // 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

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.87%. Comparing base (7104598) to head (c0759cd).
⚠️ Report is 844 commits behind head on master.

Additional details and impacted files

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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

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

[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 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

garydgregory commented Aug 30, 2023

Uh oh!

gbidsilva commented Aug 30, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gbidsilva commented Aug 31, 2023

Uh oh!

gbidsilva commented Aug 31, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

garydgregory left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gbidsilva commented Aug 31, 2023

Uh oh!

Uh oh!

gbidsilva commented Sep 1, 2023

Uh oh!

gbidsilva commented Sep 1, 2023

Uh oh!

Uh oh!

gbidsilva commented Sep 5, 2023

Uh oh!

elharo Sep 6, 2023

Choose a reason for hiding this comment

Uh oh!

elharo Sep 6, 2023

Choose a reason for hiding this comment

Uh oh!

gbidsilva commented Sep 11, 2023

Uh oh!

gbidsilva commented Sep 12, 2023

Uh oh!

gbidsilva commented Sep 13, 2023

Uh oh!

codecov-commenter commented Sep 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

garydgregory left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gbidsilva commented Aug 30, 2023 •

edited

Loading

codecov-commenter commented Sep 13, 2023 •

edited

Loading