On Fri, 20 Mar 2026 13:19:49 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.
>
> Eirik Bjørsnøs has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Make ZIP Path field static final
> - Typo in "with"
test/jdk/java/util/zip/ZipOutputStream/UnmappableZipEntryNameOrComment.java
line 59:
> 57: try (var out = new ZipOutputStream(nullOutputStream(), CHARSET)) {
> 58: assertThrows(ZipException.class, () -> out.putNextEntry(e));
> 59: }
One thing to think about which is minor
try (var out = new ZipOutputStream(nullOutputStream(), US_ASCII)) {
// assertThrows(ZipException.class, () -> {
// ZipEntry e = new ZipEntry("\u00f8"); // 'ø' unmappable in
US_ASCII
// out.putNextEntry(e);
// });
ZipEntry e = new ZipEntry("\u00f8"); // 'ø' unmappable in US_ASCII
out.putNextEntry(e);
} catch (Exception e) {
e.printStackTrace();
}
The IAE is thrown from writeLOC vs writeCEN (due to closing the Zip)
Question is do we want to remove the use of try/with/resources or leave as is?
With your change it might not matter though but wanted to throw it out there
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/30319#discussion_r2967601054
