Fix Len = 0 and Insufficient Buffer behaviours for positioned reads #203
Conversation
According to InputStream documentation Len = 0 should return 0 `If len is zero, then no bytes are read and 0 is returned;` Following the InputStream implementation, this commit is adding argument checks to Positioned Reads. This validation is implemented in the SeekableInputStream to ensure all subclasses behave the same way in the unhappy path.
fuatbasik
changed the title
| if (this.position >= getContentLength()) { | ||
| if (length == 0) { | ||
| return 0; | ||
| } else if (this.position >= getContentLength()) { |
There was a problem hiding this comment.
shall we move this validation as well to the method where we did the previous validations, so that all the validations are in same place?
There was a problem hiding this comment.
This is a good recommendation. I did not move this validation because full content length might not be available to other streams those will implement this class. In S3 case we know the size thanks to object metadata. Let me know what you think.
| Preconditions.checkArgument(position >= 0, "Position is negative"); | ||
|
|
||
| if (buffer == null) { | ||
| throw new NullPointerException("Null destination buffer"); |
There was a problem hiding this comment.
why not just have both of these in precondition checks?
There was a problem hiding this comment.
That makes perfect sense. Doing it now.
| * @throws NullPointerException if the buffer is null | ||
| * @throws IndexOutOfBoundsException if the offset or length are invalid for the given buffer | ||
| */ | ||
| protected void validatePositionedReadArgs(long position, byte[] buffer, int offset, int length) { |
There was a problem hiding this comment.
maybe add a unit test for this method? should be quick
There was a problem hiding this comment.
Well i didn't because it is a protected method and not sure if we want to expose it. Instead i added tests to S3SeekableInputStream class which is publicly exposed. Let me know if you think this is a good approach.
There was a problem hiding this comment.
Happy with this approach!
Just fyi I think there is a visibleForTesting Annotation that helps with this
|
Didn't look very deep but saw threads discussing stream spec/testing this. Where is the right place to test behaviour like this, unit tests or some higher level? Also, when we change higher level behaviour, do we need to update our reference model (this is an open question, but it feels like if we don't maintain the reference model then it will stop being useful over time)? Does anyone have a mental model yet? :) |
Description of change
According to InputStream documentation Len = 0 should return 0
If len is zero, then no bytes are read and 0 is returned;Following the InputStream implementation, this commit is adding argument checks to Positioned Reads. This validation is implemented in the SeekableInputStream to ensure all subclasses behave the same way in the unhappy path.
Checks are done in the same order with InputStream implementation.
Relevant issues
#201
Does this contribution introduce any breaking changes to the existing APIs or behaviors?
Yes. For a 0-length read to a 0-sized object read will now return 0 instead of -1.
Insufficient Buffer Capacity will now throw IndexOutOfBoundsException instead of IllegalArgumentException.
Does this contribution introduce any new public APIs or behaviors?
No, it changes existing public API behaviour.
How was the contribution tested?
Added new unit tests.
Does this contribution need a changelog entry?
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).