Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allows the text reader to fail cleanly when the user attempts to recover after catching IonException, but recovery is not possible. #709

Merged
merged 1 commit into from
Feb 2, 2024

Conversation

tgregg
Copy link
Contributor

@tgregg tgregg commented Jan 26, 2024

Issue #, if available:

FasterXML/jackson-dataformats-binary#471

Description of changes:

Sometimes errors conveyed via IonException are recoverable, and sometimes they are not. In cases where these exceptions are not recoverable but the user tries anyway (by calling another method on the reader), the reader should fail cleanly by throwing another IonException. Before this fix, the added test failed with AssertionError here because a save point had been started on a value that was later found to be invalid. The proposed fix clears the current save point if the current value is found to be malformed. This allows the reader to throw another IonException when the invalid character is encountered again during a subsequent request from the user. Note: the diff looks large, but it's just due to the added nesting level. The only change is the addition of the try/catch.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link

codecov bot commented Jan 26, 2024

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Comparison is base (3c1b6b1) 67.23% compared to head (c067930) 67.26%.
Report is 4 commits behind head on master.

❗ Current head c067930 differs from pull request most recent head 7e411b5. Consider uploading reports for the commit 7e411b5 to get more accurate results

Files Patch % Lines
...in/java/com/amazon/ion/impl/IonReaderTextRawX.java 71.05% 6 Missing and 5 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master     #709      +/-   ##
============================================
+ Coverage     67.23%   67.26%   +0.02%     
- Complexity     5484     5488       +4     
============================================
  Files           159      159              
  Lines         23025    23030       +5     
  Branches       4126     4126              
============================================
+ Hits          15481    15490       +9     
+ Misses         6262     6261       -1     
+ Partials       1282     1279       -3     

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

Copy link
Contributor

@jobarr-amzn jobarr-amzn left a comment

Choose a reason for hiding this comment

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

The diff is quite small if whitespace is hidden: https://github.com/amazon-ion/ion-java/pull/709/files?diff=unified&w=1 .

…ver after catching IonException, but recovery is not possible.
@tgregg tgregg force-pushed the attempted-recovery-fails-cleanly branch from c067930 to 7e411b5 Compare February 2, 2024 00:58
@tgregg tgregg merged commit 8dbd001 into master Feb 2, 2024
18 of 33 checks passed
@tgregg tgregg deleted the attempted-recovery-fails-cleanly branch February 2, 2024 01:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants