On Sat, 24 Feb 2024 16:57:50 GMT, Eirik Bjørsnøs <eir...@openjdk.org> 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