-
Notifications
You must be signed in to change notification settings - Fork 703
Backport Objects.checkFromIndexSize to IOUtils
#790
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
Conversation
Java 9 introduced `Objects.checkFromIndexSize` for validating the offset/length ranges used in `read` and `write` operations. This PR adds an equivalent method, `IOUtils.checkFromIndexSize`, to provide the same validation in Commons IO. It simplifies range checks in stream implementations and makes the utility available to projects that still target Java 8.
|
Hi @ppkarwasz Shouldn't his be called from our own code base to show it's what we really need in IO? Otherwise it belongs in Lang since it hangs on Object in Java 9. |
garydgregory
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ppkarwasz
There are a lot of builds failures here.
Error: Failures:
Error: ClosedInputStreamTest.testReadArray:89 expected: <-1> but was: <0>
Error: ClosedInputStreamTest.testReadArrayIndex:98 expected: <-1> but was: <0>
Error: ClosedReaderTest.testReadArray:49 expected: <-1> but was: <0>
Error: ClosedReaderTest.testReadArrayIndex:58 expected: <-1> but was: <0>
Error: ClosedReaderTest.testReadCharBuffer:67 expected: <-1> but was: <0>
Error: UnsynchronizedBufferedReaderTest.testReadArray:362 Unexpected exception type thrown, expected: <java.io.IOException> but was: <java.lang.NullPointerException>
Error: UnsynchronizedBufferedReaderTest.testReadArrayException:459 Unexpected exception type thrown, expected: <java.lang.IndexOutOfBoundsException> but was: <java.lang.NullPointerException>
Error: ByteArrayOutputStreamTest.testInvalidWriteOffsetUnder:180 Unexpected exception type thrown, expected: <java.lang.IndexOutOfBoundsException> but was: <java.lang.NullPointerException>
Error: ByteArrayOutputStreamTest.testInvalidWriteOffsetUnder:180 Unexpected exception type thrown, expected: <java.lang.IndexOutOfBoundsException> but was: <java.lang.NullPointerException>
Error: Errors:
Error: MarkShieldInputStreamTest.testReadByteArrayIntIntAfterClose:155 » IndexOutOfBounds Range [0, 0 + 1) out of bounds for length 0
Error: MarkShieldInputStreamTest.testReadByteArrayIntIntAfterClose:155 » IndexOutOfBounds Range [0, 0 + 1) out of bounds for length 0
Error: MarkShieldInputStreamTest.testReadByteArrayIntIntAfterClose:155 » IndexOutOfBounds Range [0, 0 + 1) out of bounds for length 0
Error: MarkShieldInputStreamTest.testReadByteArrayIntIntAfterClose:155 » IndexOutOfBounds Range [0, 0 + 1) out of bounds for length 0
Error: MarkShieldInputStreamTest.testReadByteArrayIntIntAfterClose:155 » IndexOutOfBounds Range [0, 0 + 1) out of bounds for length 0
Error: MarkShieldInputStreamTest.testReadByteArrayIntIntAfterClose:155 » IndexOutOfBounds Range [0, 0 + 1) out of bounds for length 0
Error: MarkShieldInputStreamTest.testReadByteArrayIntIntAfterClose:155 » IndexOutOfBounds Range [0, 0 + 1) out of bounds for length 0
Error: MarkShieldInputStreamTest.testReadByteArrayIntIntAfterClose:155 » IndexOutOfBounds Range [0, 0 + 1) out of bounds for length 0
Error: MarkShieldInputStreamTest.testReadByteArrayIntIntAfterClose:155 » IndexOutOfBounds Range [0, 0 + 1) out of bounds for length 0
Error: NullInputStreamTest.testReadByteArrayIntIntAfterClose:252 » IndexOutOfBounds Range [0, 0 + 1) out of bounds for length 0
Error: ProxyReaderTest.testNullCharArray:58 » NullPointer char array
Error: NullAppendableTest.testNull:34 » NullPointer char sequence
Error: ProxyWriterTest.testNullString:224 » NullPointer str
[INFO]
Error: Tests run: 6077, Failures: 9, Errors: 13, Skipped: 26
|
Hi @garydgregory,
Yes, I noticed: they all come from subtle changes in how those classes validate their arguments. I’ll take a closer look tomorrow. For |
There is no way to insert `@Override` in a Javadoc snippet without triggering a warning in either Java 8 or 21.
|
Hi @garydgregory, I’ve fixed the failing tests in commit 051a86a. The failures stemmed from a few subtly incorrect assumptions in the original tests or my code. Below is a breakdown of what I changed and why:
|
garydgregory
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ppkarwasz
Thank you for the PR. See my one comment, which applies to multiple call sites.
garydgregory
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ppkarwasz
It seems to me that our "Null", "Broken", and "Closed" classes are different beasts from other stream/reader/writer implementations.
Please see my comments scattered throughout. Some comments apply to more than one call site.
TY
src/main/java/org/apache/commons/io/output/FilterCollectionWriter.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/commons/io/output/FilterCollectionWriter.java
Outdated
Show resolved
Hide resolved
The tests are improved to ensure that the given exception is thrown **before** parameter validation.
Revert `BrokenWriter` and add test cases to ensure the exception is thrown **before** parameter validation.
|
Hi @garydgregory, Thanks for your review 🙏. For several of your remarks I applied the exact solution you suggested and marked those conversations as resolved. For others, I took a slightly different approach, so I left those threads open for your feedback. Could you please take another look and close any conversations you now consider resolved? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds checkFromIndexSize and checkFromToIndex methods to IOUtils to provide Java 8-compatible backports of the argument validation methods introduced in Java 9's Objects class. These methods standardize array/string bounds checking across Commons IO stream implementations.
Key changes:
- Implements comprehensive argument validation methods with consistent error messages
- Updates all stream classes to use the new validation methods
- Adds extensive test coverage for edge cases and parameter validation
Reviewed Changes
Copilot reviewed 49 out of 49 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
IOUtils.java |
Adds core validation methods checkFromIndexSize and checkFromToIndex |
| Multiple stream classes | Replaces custom validation logic with calls to new utility methods |
| Test files | Updates existing tests to expect proper validation and adds comprehensive test coverage |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/main/java/org/apache/commons/io/output/StringBuilderWriter.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/commons/io/input/buffer/CircularBufferInputStream.java
Show resolved
Hide resolved
garydgregory
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ppkarwasz
I added comments.
A lot of them are: Javadoc @throws IndexOutOfBoundsException ... is missing.
src/main/java/org/apache/commons/io/output/StringBuilderWriter.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/commons/io/output/StringBuilderWriter.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/commons/io/output/ThresholdingOutputStream.java
Show resolved
Hide resolved
In #790 I introduced `IOUtils#checkIndexFromLength` calls to validate arguments across the codebase. Ironically, the `IOUtils` class itself was left out. This PR addresses that omission by adding argument validation to `IOUtils#read` and `IOUtils#readFully`. Key points: * Ensures consistency with the rest of Commons IO by validating `offset` and `length`. * Fixes inconsistent exception behavior: * Previously, `length < 0` resulted in an `IllegalArgumentException`. * `offset < 0` did not trigger validation and failed later with an `IndexOutOfBoundsException`. * With this change, both invalid cases are handled consistently and upfront.
* Add missing `read` argument validation in `IOUtils` In #790 I introduced `IOUtils#checkIndexFromLength` calls to validate arguments across the codebase. Ironically, the `IOUtils` class itself was left out. This PR addresses that omission by adding argument validation to `IOUtils#read` and `IOUtils#readFully`. Key points: * Ensures consistency with the rest of Commons IO by validating `offset` and `length`. * Fixes inconsistent exception behavior: * Previously, `length < 0` resulted in an `IllegalArgumentException`. * `offset < 0` did not trigger validation and failed later with an `IndexOutOfBoundsException`. * With this change, both invalid cases are handled consistently and upfront. * fix: failing tests
Java 9 introduced
Objects.checkFromIndexSizefor validating the offset/length ranges used inreadandwriteoperations.This PR adds an equivalent method,
IOUtils.checkFromIndexSize, to provide the same validation in Commons IO. It simplifies range checks in stream implementations and makes the utility available to projects that still target Java 8.