On Thu, 19 Mar 2026 20:52:36 GMT, Eirik Bjørsnøs <[email protected]> wrote:
>> Please review this PR which improves validation of unmappable characters in
>> names in the `ZipFileSystem` and `ZipFileOutputStream` APIs.
>>
>> Currently, `ZipFileSystem::getPath` and `ZipFileOutputStream:putNextEntry`
>> both throw `IllegalArgumentException` when rejecting a path or entry name
>> which cannot be encoded with the given charset.
>>
>> This PR fixes `ZipFileSystem::getPath` to instead throw
>> `InvalidPathException` as specified. Similarly,
>> `ZipOutputStream::putNextEntry` is updated to throw `ZipException` a
>> specified.
>>
>> Related, `ZipOutputStream::putNextEntry` is updated to reject unmappable
>> ZipEntry comments in a similar fashion.
>>
>> This change effectively means that `ZipOutputStream` now encodes names and
>> comments twice, once in `putNextEntry` and second time in `writeCEN` when
>> the stream is closed. An alternative would be to capture the encoded byte
>> arrays in the `XEntry`, however this would increase retained heap memory for
>> large number of entries.
>>
>> New tests are added in the ZipFS and ZipFileOutputStream area to verify that
>> these APIs throw exceptions according to their specifications when faced
>> with unmappable characters.
>>
>> Curiously, `ZipFileOutputStream::setComment` is not specified to throw for
>> an unmappable comment. Long standing unspecified behavior is to throw
>> IllegalArgumentException. To prevent regressions, a test is added to verify
>> this. An alternative path here would be to update the specification with a
>> CSR.
>
> Eirik Bjørsnøs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> initCause original exception
test/jdk/java/util/zip/ZipOutputStream/UnmappableZipEntryNameOrComment.java
line 58:
> 56: void rejectUnmappableZipEntryName() throws IOException {
> 57: ZipEntry e = new ZipEntry(UNMAPPABLE);
> 58: assertDoesNotThrow(() -> {
If any of that code throws something unexpected then it will get propagated as
an exception that results in a test failure, like any other test. So I think
the `assertDoesNotThrow()` is not needed here, isn't it?
test/jdk/java/util/zip/ZipOutputStream/UnmappableZipEntryNameOrComment.java
line 75:
> 73: ZipEntry e = new ZipEntry("file.txt");
> 74: e.setComment(UNMAPPABLE);
> 75: assertDoesNotThrow(() -> {
Same comment here and the other places this is used. Given how many times it's
used, I'm now starting to wonder if there's something I am missing about this
usage.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/30319#discussion_r2965516331
PR Review Comment: https://git.openjdk.org/jdk/pull/30319#discussion_r2965519836
