COMPRESS-721 -- avoid reflection when possible in unpack200#764
COMPRESS-721 -- avoid reflection when possible in unpack200#764tballison wants to merge 1 commit into
Conversation
|
I added unit tests that fail when building without the |
garydgregory
left a comment
There was a problem hiding this comment.
Hello @tballison
-1: These tests don't test anything new. They always pass, whether or not the changes to main are applied, inviting a regression to whatever you think you're fixing.
|
@garydgregory LOL...y, I added those when I was manually testing by removing the java >=17 profile in the pom.xml. However, as you point out, the build already fails spectacularly with existing tests when that is removed because of exactly this issue. |
|
As a side note, the more aggressive fix is to get rid of reflection entirely. I chose to modify the existing code as little as possible on this PR, but happy to go bigger if desired. |
|
Sorry for the noise, |
|
Hello @tballison There might be a way to do this without reflection, if so please provide it 😁 Otherwise, these untested changes are Just asking for a regression in the future... |
|
fix incoming, again apologies for the noise. |
|
@garydgregory this is the go bigger option with fully removing reflection. Pack200ModuleAccessTest fails when the fix is reverted. |
|
K, I moved this to draft to do a bit more research. I think this is ready for review now. My belief is that the band-length OOM guard added in a3474d1 (unpack200 hardening) makes the BoundedInputStream's Codec.check() tripwire redundant...checkIntArray runs whether or not the stream is bounded. This means that we could remove the BoundedInputStream entirely, but that would touch more of the code and would be a bigger PR. Let me know what you think. |
Thanks for your contribution to Apache Commons! Your help is appreciated!
Before you push a pull request, review this list:
Claude Opus 4.6 significantly
mvn; that'smvnon the command line by itself.