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

Handles the case where InputStream.skip provides fewer bytes than requested before reaching EOF. #689

Merged
merged 1 commit into from
Jan 9, 2024

Conversation

tgregg
Copy link
Contributor

@tgregg tgregg commented Jan 4, 2024

Description of changes:

InputStream implementations are allowed to skip fewer bytes than requested, and doing so does not necessarily indicate that the end of the stream has been reached. S3's S3ObjectInputStream has this behavior in certain cases.

Before this fix, the binary reader would throw an IonException for unexpected EOF if this happened when trying to skip of a scalar value that was not entirely buffered. After the fix, the reader will continue requesting to skip bytes from the stream until EOF is definitely reached or all requested bytes have been skipped.

The testshouldNotFailWhenAnInputStreamProvidesFewerBytesThanRequestedWithoutReachingEofWithAnUnboundedBufferAndTheReaderSkipsTheValue is the one that reproduced the problem, and now succeeds. The fix is similar to that of #623, which handled this for InputStream.read. At the time I thought I had the InputStream.skip case covered as well, but it had only been covered for the oversized value case; the new test covers the case where the value to be skipped is not oversized but extends beyond the current end of the buffer. I added parameterization to other related tests to lessen the likelihood that similar issues are still lurking.

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 4, 2024

Codecov Report

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

Comparison is base (342ccd7) 67.27% compared to head (6d631ed) 67.26%.

Files Patch % Lines
...main/java/com/amazon/ion/impl/IonCursorBinary.java 80.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master     #689      +/-   ##
============================================
- Coverage     67.27%   67.26%   -0.01%     
- Complexity     5489     5491       +2     
============================================
  Files           159      159              
  Lines         23016    23019       +3     
  Branches       4125     4126       +1     
============================================
+ Hits          15483    15484       +1     
- Misses         6254     6257       +3     
+ Partials       1279     1278       -1     

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

@tgregg tgregg merged commit 140761a into master Jan 9, 2024
10 of 35 checks passed
@tgregg tgregg deleted the incomplete-seek branch January 9, 2024 20:43
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