Refactor: Use InputStream.transferTo #2669
Conversation
Instead of longer code that accomplishes the same.
| if (null != dest) { | ||
| dest.flush(); | ||
| if (dest == null) { | ||
| dest = NullOutputStream.INSTANCE; |
There was a problem hiding this comment.
A commons-io utility
| @@ -261,6 +263,16 @@ public void write(OutputStream os) throws IOException { | |||
| } | |||
| } | |||
|
|
|||
| private static InputStream boundedInputStream(final InputStream is, final long maxLength) | |||
| throws IOException { | |||
| return new BoundedInputStream(is, maxLength) { | |||
There was a problem hiding this comment.
A commons-io utility
| @@ -22,6 +22,7 @@ | |||
| import java.io.UnsupportedEncodingException; | |||
| import java.util.Arrays; | |||
|
|
|||
| @Deprecated | |||
There was a problem hiding this comment.
could we add the since tag to help us remember when to remove this deprecated method? There are so many deprecated methods that just kind of hang around because we don't have good mechanisms for deciding/remembering to remove them!
There was a problem hiding this comment.
I deleted it; we don't need to wait for internal utilities
| try (BAOS bos = new BAOS()) { | ||
| long sz = 0; | ||
| int next = is.read(); |
There was a problem hiding this comment.
this code was very sad, reading one char at a time!
| ByteArrayOutputStream baos = new ByteArrayOutputStream(); | ||
| byte[] buf = new byte[1024]; | ||
| int r; | ||
| /** Reads input completely into a byte array. Closes the stream. */ |
There was a problem hiding this comment.
This feels super trappy to have this method close the stream and Utils.toByteArray leave it open, but not something we can fix here.
There was a problem hiding this comment.
Yes; it's unusual for a method to accept an InputStream and also close it. I at least documented it. And I reduced its scope from public. I was tempted to rename the method to something like streamAsBytesAndClose so that the closing is super apparent at the caller. Can still do.
There was a problem hiding this comment.
we have the andClose pattern in other places...
|
We're on commons-io 2.15.1? We should upgrade to 2.16.1 in this PR, since you're already looking at it and I remember that some stream closing behavior changed between these two versions. Otherwise somebody is going to be very confused without context when tests break. |
Already open PR for it, but blocked on forbidden-apis: #2398 and policeman-tools/forbidden-apis#249 |
That's a reason not to embed that change. If this PR breaks a test, I want to debug it on its own right without wondering if it's my fault or commons-io change. |
|
@madrob I made further changes based on your comments in the schema designer. LMK what you think. Otherwise I think it's mergeable (no JIRA). |
Instead of longer code that accomplishes the same.