RFR: 4890732: GZIPOutputStream doesn't support optional GZIP fields [v12]

[liach]

On Mon, 6 Sep 2021 07:20:11 GMT, Lin Zang <lzang at openjdk.org> wrote:

>> 4890732: GZIPOutputStream doesn't support optional GZIP fields
>
> Lin Zang has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 17 commits:
> 
>  - Merge branch 'master' into gzip-field
>  - delete trailing spaces
>  - refine code
>  - Merge branch 'master' into gzip-field
>  - change since version to 18
>  - Merge branch 'master' into gzip-field
>  - Merge branch 'master' into gzip-field
>  - Add api in GZIPInputStream to get header data
>  - Merge remote-tracking branch 'upstream/master' into gzip-field
>  - remove trailing spaces
>  - ... and 7 more: https://git.openjdk.java.net/jdk/compare/c640fe42...196caedb

src/java.base/share/classes/java/util/zip/GZIPHeaderBuilder.java line 65:

> 63:      * Add extra field in GZIP file header.
> 64:      * This method verifies the extra fileds layout per RFC-1952.
> 65:      * See comments of {@code verifyExtraFieldLayout}

You should specify the layout here than refer to a private method, as private methods don't appear in generated online javadocs

src/java.base/share/classes/java/util/zip/GZIPHeaderBuilder.java line 186:

> 184:      * @since 18
> 185:      */
> 186:     public byte[] generateBytes(byte cm,

Is there any reason to leave this public? If this is just an implementation detail, this should be kept private as public apis are maintenance burdens that are risky to change (hence csrs)

src/java.base/share/classes/java/util/zip/GZIPHeaderBuilder.java line 247:

> 245:              *    +=========================================+
> 246:              */
> 247:             byte[] filenameBytes = filename.getBytes("ISO-8859-1");

Should use [`StandardCharsets.ISO_8859_1`](https://github.com/openjdk/jdk/blob/8876eae42993d4425ba9886dde94b08f6101a257/src/java.base/share/classes/java/nio/charset/StandardCharsets.java#L55) than string names. Using the charset object is significantly faster than looking up charsets by string names, see https://bugs.openjdk.java.net/browse/JDK-8272120

src/java.base/share/classes/java/util/zip/GZIPHeaderBuilder.java line 328:

> 326:                                   String fileComment,
> 327:                                   int headerCRC16,
> 328:                                   byte[] headerBytes) {

Need to override the getter for byte array fields to return copies than original arrays to prevent modifications. For the extraFieldBytes one, the call to copy needs a null check too.

src/java.base/share/classes/java/util/zip/GZIPHeaderBuilder.java line 359:

> 357:         private static final int FEXTRA     = 4;    // Extra field
> 358:         private static final int FNAME      = 8;    // File name
> 359:         private static final int FCOMMENT   = 16;   // File comment

Since the `flags` record component is public, These probably should be left public as well to benefit users, especially given checking the flags can replace null checks for `extraFieldBytes`, `filename`, and `fileContent`.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3072