8380542: ZipOutputStream.setComment should specify that it throws IllegalArgumentException for unmappable characters by eirbjo · Pull Request #30338 · openjdk/jdk

eirbjo · 2026-03-20T13:20:59Z

Please review this PR which updates the specification of ZipOutputStream.setComment(String) to match the long-standing behavior of throwing IllegalArgumentException when a comment contains characters which are unmappable using the Charset passed in the constructor.

A new tests is added to reproduce calling setComment with unmappable characters and verify that it throws IAE.

A CSR has been drafted. Its specification section will be updated after an initial round of review of the specification text in this PR.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Change requires CSR request JDK-8380546 to be approved
Commit message must refer to an issue

Issues

JDK-8380542: ZipOutputStream.setComment should specify that it throws IllegalArgumentException for unmappable characters (Enhancement - P4)
JDK-8380546: ZipOutputStream.setComment should specify that it throws IllegalArgumentException for unmappable characters (CSR)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/30338/head:pull/30338
$ git checkout pull/30338

Update a local copy of the PR:
$ git checkout pull/30338
$ git pull https://git.openjdk.org/jdk.git pull/30338/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 30338

View PR using the GUI difftool:
$ git pr show -t 30338

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/30338.diff

Using Webrev

Link to Webrev Comment

…gumentException for unmappable characters in comment

eirbjo · 2026-03-20T13:21:15Z

/csr needed

bridgekeeper · 2026-03-20T13:23:11Z

👋 Welcome back eirbjo! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2026-03-20T13:24:16Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2026-03-20T13:25:15Z

@eirbjo an approved CSR request is already required for this pull request.

openjdk · 2026-03-20T13:26:03Z

@eirbjo The following label will be automatically applied to this pull request:

core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2026-03-20T13:38:29Z

Webrevs

jaikiran · 2026-03-20T13:52:06Z

src/java.base/share/classes/java/util/zip/ZipOutputStream.java

-     *            ZIP file comment is greater than 0xFFFF bytes
+     *            ZIP file comment is greater than 0xFFFF bytes or if the
+     *            comment contains characters not mappable using the Charset
+     *            passed to the constructor of this ZipOutputStream instance.
     */
    public void setComment(String comment) {


Given what this method currently does and the class level specification which states that "unless otherwise noted" a NullPointerException gets thrown if null if passed for method params, I think we might have to update this documentation like:

/** * Sets the ZIP file comment. * * @implSpec Passing {@code null} for {@code comment} will result in * the ZIP file having no comment. * * @param comment the comment string, can be {@code null} * @throws IllegalArgumentException if the length of the specified * ZIP file comment is greater than 0xFFFF bytes or if the * {@code comment} contains characters that cannot be mapped * by the {@code Charset} of this {@code ZipOutputStream} */

I am unsure if accepting null should be a @implSpec or just a formal API specification.

I am unsure if accepting null should be a @implSpec or just a formal API specification.

Me too. It's not like a subclass can affect the comment though other means than this method. One could imagine a subclass that filters or rejects comments. The distinction between these different spec levels is not super clear to me.

Perhaps other reviewers have opinions.

I'll update the PR to your suggestion in any case - that way we have something to base our discussion on.

I think this can be just part of the specification.

I replaced the @implSpec with a note on the comment param. Here I also mention that passing the empty string and null have equivalent effect:

* @param comment the comment string. Passing {@code null} or the empty string * will result in the output having no ZIP file comment

Have a look and let me know how you think this works.

I think this can be just part of the specification.

Yes, it needs to normative.

…to null or the empty string to produce no ZIP file comment

eirbjo · 2026-03-20T17:22:54Z

test/jdk/java/util/zip/ZipOutputStream/UnmappableZipFileComment.java

+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+/* @test
+ * @bug 8380542


Seeing this is a spec update, not a bug, should I remove the @bug tag here?

Marcono1234 · 2026-03-20T21:30:47Z

test/jdk/java/util/zip/ZipOutputStream/UnmappableZipFileComment.java

+import java.util.zip.ZipOutputStream;
+
+import static java.io.OutputStream.nullOutputStream;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;


Import for assertDoesNotThrow is unused?

Thanks, fixed in 7845c5e!

Update ZipOutputStream.setComment to declare that it throws IllegalAr…

830fc4d

…gumentException for unmappable characters in comment

openjdk bot added the csr Pull request needs approved CSR before integration label Mar 20, 2026

openjdk bot added the core-libs core-libs-dev@openjdk.org label Mar 20, 2026

eirbjo marked this pull request as ready for review March 20, 2026 13:33

openjdk bot added the rfr Pull request is ready for review label Mar 20, 2026

eirbjo mentioned this pull request Mar 20, 2026

8380452: ZipOutputStream::putNextEntry(ZipEntry) throws unspecified IllegalArgumentException for unmappable entry name #30319

Open

3 tasks

jaikiran reviewed Mar 20, 2026

View reviewed changes

eirbjo added 2 commits March 20, 2026 15:06

Update spec according to review suggestion

c5df60b

Add note to @param comment specification that the comment may be set …

78d491f

…to null or the empty string to produce no ZIP file comment

eirbjo commented Mar 20, 2026

View reviewed changes

Marcono1234 reviewed Mar 20, 2026

View reviewed changes

Remove unused import

7845c5e

Conversation

eirbjo commented Mar 20, 2026 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewing

Uh oh!

eirbjo commented Mar 20, 2026

Uh oh!

bridgekeeper bot commented Mar 20, 2026

Uh oh!

openjdk bot commented Mar 20, 2026

Uh oh!

openjdk bot commented Mar 20, 2026

Uh oh!

openjdk bot commented Mar 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented Mar 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

jaikiran Mar 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaikiran Mar 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eirbjo Mar 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LanceAndersen Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

eirbjo Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

AlanBateman Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

eirbjo Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

Marcono1234 Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

eirbjo Mar 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

eirbjo commented Mar 20, 2026 •

edited by openjdk bot

Loading

openjdk bot commented Mar 20, 2026 •

edited

Loading

mlbridge bot commented Mar 20, 2026 •

edited

Loading

jaikiran Mar 20, 2026 •

edited

Loading

jaikiran Mar 20, 2026 •

edited

Loading

eirbjo Mar 20, 2026 •

edited

Loading

eirbjo Mar 20, 2026 •

edited

Loading