On Sat, 24 Feb 2024 16:57:50 GMT, Eirik Bjørsnøs <[email protected]> wrote:
>> Lance Andersen has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Updates based on 1st round of feedback
>
> src/java.base/share/classes/java/util/zip/ZipFile.java line 331:
>
>> 329: try {
>> 330: return res.zsrc.zc.toString(res.zsrc.comment);
>> 331: } catch(IllegalArgumentException iae) {
>
> Suggestion:
>
> } catch (IllegalArgumentException iae) {
fixed
> src/java.base/share/classes/java/util/zip/ZipInputStream.java line 526:
>
>> 524: throw (ZipException) new ZipException(
>> 525: "invalid LOC header (bad entry
>> name)").initCause(ex);
>> 526: }
>
> There is a lot going on here:
>
> * The creation of a ZipEntry
> * The interpretation of the general purpose bit flag
> * The split to call either a static or non-static toString depending on the
> flag interpretation
> * The exception handling, including the somewhat unusual initCause and cast
>
> The comment at 517 seems even more detached from the logic it tries to
> describe after this change.
>
> Perhaps we could benefit from introducing a `zipCoderFromFlag(int flag)`
> method, similar to that in ZipFile?
>
> This would allow the section to be rewritten to something that hide the flag
> parsing details, leaves us with a single (polymorphic) toString invocation
> and extracts `createZipEntry` from the try/catch scope.
>
> Something like this: (I also changed the exception handling here, that's a
> bit orthogonal to the main points above)
>
> Suggestion:
>
> String name;
> try {
> name = zipCoderForFlag(flag).toString(b, len);
> } catch (IllegalArgumentException ex) {
> ZipException e = new ZipException("invalid LOC header (bad entry
> name)");
> e.initCause(ex);
> throw e;
> }
> ZipEntry e = createZipEntry(name);
I believe you meant `ZipCoderFromPos(int flag)`. I don't think it is needed in
ZipInputStream but I did move the call to createZipEntry out of the try block
per your suggestion
> test/jdk/java/util/zip/ZipFile/InvalidBytesInEntryNameOrComment.java line 49:
>
>> 47: * @summary Validate that a ZipException is thrown when opening a ZIP
>> file via
>> 48: * ZipFile, traversing a Zip File via ZipInputStream, with invalid UTF-8
>> 49: * byte sequences in the name or comment fields fails with ZipException.
>
> The leading summary sentence needs a cleanup. Here's one suggestion:
>
> Suggestion:
>
> * @summary Validate that a ZipException is thrown when a ZIP file with
> * invalid UTF-8 byte sequences in the name or comment fields is opened via
> * ZipFile or traversed via ZipInputStream.
Updated
> test/jdk/java/util/zip/ZipFile/InvalidBytesInEntryNameOrComment.java line 60:
>
>> 58: private static final byte[] INVALID_UTF8_BYTE_SEQUENCE = {(byte)
>> 0xF0, (byte) 0xA4, (byte) 0xAD};
>> 59: // Expected error message when an invalid entry name or entry
>> comment is
>> 60: // encountered when accessing a CEN Header
>
> This two-line comment could benefit from an earlier line break.
The break makes sense as is in IntelliJ, so please clarify
> test/jdk/java/util/zip/ZipFile/InvalidBytesInEntryNameOrComment.java line 64:
>
>> 62:
>> 63: // Expected error message when an invalid entry name is encountered
>> when
>> 64: // accessing a LOC Header
>
> This two-line comment could benefit from an earlier line break.
Same comment as above
> test/jdk/java/util/zip/ZipFile/InvalidBytesInEntryNameOrComment.java line 65:
>
>> 63: // Expected error message when an invalid entry name is encountered
>> when
>> 64: // accessing a LOC Header
>> 65: private static final String LOC_BAD_ENTRY_NAME_OR_COMMENT = "invalid
>> LOC header (bad entry name)";
>
> Should this be renamed `LOC_BAD_ENTRY_NAME`?
Yep, I thought I made that change but must have just thought about it and never
went back to it
> test/jdk/java/util/zip/ZipFile/InvalidBytesInEntryNameOrComment.java line 128:
>
>> 126: * 0x93: Comment : [ZipFileComment]
>> 127: */
>> 128: public static byte[] VALID_ZIP = {
>
> I see this byte array encoding of ZIP files is a pattern used across tests.
> My preference would be to encode this in Base64 or hex, since that would make
> the consumption by external command line tools easier and the encoded
> representation somewhat prettier. But no big deal, this might just come down
> to personal preference. (This isn't human readable anyhow for the normal
> definition of "human" :)
Yes, this is a style preference and plan to leave as is.
> test/jdk/java/util/zip/ZipFile/InvalidBytesInEntryNameOrComment.java line 182:
>
>> 180: /**
>> 181: * Validate that the original Zip file can be opened via ZipFile.
>> 182: * @throws IOException if an entry occurs
>
> Suggestion:
>
> * @throws IOException if an error occurs
Not sure how I missed that. fixed
> test/jdk/java/util/zip/ZipFile/InvalidBytesInEntryNameOrComment.java line 196:
>
>> 194: * Validate that the original Zip file can be opened and traversed
>> via
>> 195: * ZipinputStream::getNextEntry.
>> 196: * @throws IOException if an entry occurs
>
> Suggestion:
>
> * @throws IOException if an error occurs
same comment above fixed
> test/jdk/java/util/zip/ZipFile/InvalidBytesInEntryNameOrComment.java line 201:
>
>> 199: public void traverseZipWithZipInputStreamTest() throws IOException {
>> 200: Files.write(ZIP_FILE, zipArray);
>> 201: try( ZipInputStream zis = new ZipInputStream(new
>> FileInputStream(ZIP_FILE.toFile()))) {
>
> Suggestion:
>
> try (ZipInputStream zis = new ZipInputStream(new
> FileInputStream(ZIP_FILE.toFile()))) {
fixed
> test/jdk/java/util/zip/ZipFile/InvalidBytesInEntryNameOrComment.java line 203:
>
>> 201: try( ZipInputStream zis = new ZipInputStream(new
>> FileInputStream(ZIP_FILE.toFile()))) {
>> 202: ZipEntry ze;
>> 203: while((ze = zis.getNextEntry()) != null ) {
>
> Suggestion:
>
> while ((ze = zis.getNextEntry()) != null) {
fixed
> test/jdk/java/util/zip/ZipFile/InvalidBytesInEntryNameOrComment.java line 243:
>
>> 241:
>> 242: /**
>> 243: * Validate that a ZipException is thrown when an entry name is
>> encountered
>
> Is the "is" here out of place or are we missing a "which"?
>
> Suggestion:
>
> * Validate that a ZipException is thrown when an entry name encountered
Modified the comment
> test/jdk/java/util/zip/ZipFile/InvalidBytesInEntryNameOrComment.java line 267:
>
>> 265: * @throws IOException if an error occurs
>> 266: */
>> 267: private void createInvalidUTFEntryInZipFile(int offset) throws
>> IOException {
>
> Is the "In" in this method name helpful? (To me it suggests a file argument,
> not a result)
> Suggestion:
>
> private void createInvalidUTFEntryInZipFile(int offset) throws
> IOException {
Well, your change is the same but to your comment we are modifying an entry in
the ZipFile. So I don't mind changing the method name but not sure its is
really needed
> test/jdk/java/util/zip/ZipFile/InvalidBytesInEntryNameOrComment.java line 304:
>
>> 302: * System.out.println(result);
>> 303: * }
>> 304: * </pre>
>
> Can we use `@snippet` in tests? Would allow syntax highlighting in IDEs
Sure changed to use `@snippet`
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17995#discussion_r1501653167
PR Review Comment: https://git.openjdk.org/jdk/pull/17995#discussion_r1501677114
PR Review Comment: https://git.openjdk.org/jdk/pull/17995#discussion_r1501670123
PR Review Comment: https://git.openjdk.org/jdk/pull/17995#discussion_r1501655822
PR Review Comment: https://git.openjdk.org/jdk/pull/17995#discussion_r1501662492
PR Review Comment: https://git.openjdk.org/jdk/pull/17995#discussion_r1501656632
PR Review Comment: https://git.openjdk.org/jdk/pull/17995#discussion_r1501657036
PR Review Comment: https://git.openjdk.org/jdk/pull/17995#discussion_r1501657953
PR Review Comment: https://git.openjdk.org/jdk/pull/17995#discussion_r1501658140
PR Review Comment: https://git.openjdk.org/jdk/pull/17995#discussion_r1501658526
PR Review Comment: https://git.openjdk.org/jdk/pull/17995#discussion_r1501658974
PR Review Comment: https://git.openjdk.org/jdk/pull/17995#discussion_r1501663806
PR Review Comment: https://git.openjdk.org/jdk/pull/17995#discussion_r1501660807
PR Review Comment: https://git.openjdk.org/jdk/pull/17995#discussion_r1501666735
