fix: replace IAE with IOException on byte array overflow#785
fix: replace IAE with IOException on byte array overflow#785garydgregory merged 1 commit intomasterfrom
Conversation
The method `IOUtils#toByteArray(InputStream)` previously threw an `IllegalArgumentException` when the number of bytes read exceeded what could be stored in a `byte[]`. This change updates the method to throw an `IOException` instead, which is more semantically appropriate for stream-reading failures. A dedicated test has been added to verify this behavior.
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the exception type thrown by IOUtils.toByteArray(InputStream) when the input stream size exceeds the maximum byte array length, changing from IllegalArgumentException to IOException for better semantic alignment with I/O operations.
- Replace
IllegalArgumentExceptionwithIOExceptionwhen byte array overflow occurs - Update method documentation to reflect the exception type change
- Add comprehensive test coverage for the overflow scenario using mocking
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/main/java/org/apache/commons/io/IOUtils.java | Changes exception type from IllegalArgumentException to IOException and updates javadoc |
| src/test/java/org/apache/commons/io/IOUtilsTest.java | Adds test case with mocking to verify IOException is thrown on overflow |
| src/changes/changes.xml | Documents the bug fix in the changelog |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
garydgregory
left a comment
There was a problem hiding this comment.
Hi @ppkarwasz
I'm did a review for consistency. In git master, we have, eleven (11) IOUtils methods that throw IllegalArgumentException. Since the test in this method doesn't test the actual input arguments, it seems semantically OK to make this change. I just wanted to mention it since all other APIs in IOUtils throw IllegalArgumentException by explicitly testing their input arguments.
| final UnsynchronizedByteArrayOutputStream output = copyToOutputStream(inputStream, SOFT_MAX_ARRAY_LENGTH + 1, DEFAULT_BUFFER_SIZE); | ||
| if (output.size() > SOFT_MAX_ARRAY_LENGTH) { | ||
| throw new IllegalArgumentException(String.format("Cannot read more than %,d into a byte array", SOFT_MAX_ARRAY_LENGTH)); | ||
| throw new IOException(String.format("Cannot read more than %,d into a byte array", SOFT_MAX_ARRAY_LENGTH)); |
There was a problem hiding this comment.
Is it worth also testing
if (inputStream instanceof FileInputStream && ((FileInputStream) inputStream).getChannel().size() > SOFT_MAX_ARRAY_LENGTH`) {
throw new IllegalArgumentException(...);
}before we call copyToOutputStream() ?
The above would be justified in an IllegalArgumentException. WDYT? Is that too specific?
There was a problem hiding this comment.
In my opinion IllegalArgumentException should be thrown for conditions that the caller is supposed to verify himself. Asking the user to verify this condition seems like a big requirement. @ecki, what do you think?
However, I think that:
- We can add the condition as a fail-fast option and throw an
IOException. The condition should besize() - position() > SOFT_MAX_ARRAY_LENGTH. - Depending on the remaining size of the
FileInputStream, we can also useFileChannel.map()to map the remaining bytes into direct memory and then copy them into Java heap memory to a newly createdbyte[]. - We could actually use this technique for all
toByteArraymethods. In this case, we should reword thetoByteArray(InputStream, int, int)Javadoc to say thatchunkSizeis the maximum size of temporary byte arrays used to load the data.
The method
IOUtils#toByteArray(InputStream)previously threw anIllegalArgumentExceptionwhen the number of bytes read exceeded what could be stored in abyte[].This change updates the method to throw an
IOExceptioninstead, which is more semantically appropriate for stream-reading failures. A dedicated test has been added to verify this behavior.Before you push a pull request, review this list:
mvn; that'smvnon the command line by itself.