8305945: (zipfs) Opening a directory to get input stream produces incorrect exception message #13482

LanceAndersen · 2023-04-14T20:24:00Z

Please review this trivial change when ZipFS returns the wrong java.nio.file.FileSystemException message due the the parameters being reversed.

I also included a simple junit test as part of the fix.

Mach5 tiers1-3 are clean

Best
Lance

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8305945: (zipfs) Opening a directory to get input stream produces incorrect exception message

Reviewers

Naoto Sato (@naotoj - Reviewer) ⚠️ Review applies to e99a91bf
Christian Stein (@sormuras - Committer) ⚠️ Review applies to e99a91bf
@eirbjo (no known openjdk.org user name / role) ⚠️ Review applies to e99a91bf

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13482

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

Using diff file

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

Webrev

Link to Webrev Comment

…ception message

bridgekeeper · 2023-04-14T20:24:51Z

👋 Welcome back lancea! 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 · 2023-04-14T20:26:52Z

@LanceAndersen The following labels will be automatically applied to this pull request:

core-libs
nio

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

mlbridge · 2023-04-14T20:30:33Z

Webrevs

naotoj

Looks good, Lance. Nit: copyright year -> 2023

sormuras

Looks good to me.

openjdk · 2023-04-14T21:08:54Z

@LanceAndersen This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8305945: (zipfs) Opening a directory to get input stream produces incorrect exception message

Reviewed-by: naoto, cstein

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 209 new commits pushed to the master branch:

caa841d: 8306029: ProblemList runtime/ErrorHandling/TestDwarf.java on linux
314bad3: 8305935: Resolve multiple definition of 'jmm_<interface|version>' when statically linking with JDK native libraries
793da60: 8305403: Shenandoah evacuation workers may deadlock
2cc4bf1: 8305085: Suppress removal warning for finalize() from test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefineFinalizer.java
5a78865: 8304930: Enable Link Time Optimization as an option for Visual C++
30a140b: 8304912: Use OperatingSystem enum in java.desktop module
1fd4006: 8305405: Compile_lock not needed in Universe::genesis()
ebeee6d: 8305404: Compile_lock not needed for InstanceKlass::implementor()
d2ce04b: 8301496: Replace NULL with nullptr in cpu/riscv
54bf370: 8170945: Collectors$Partition should override more Map methods
... and 199 more: https://git.openjdk.org/jdk/compare/7239150f8aff0e3dc07c5b27f6b7fb07237bfc55...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

LanceAndersen · 2023-04-14T21:32:24Z

Looks good, Lance. Nit: copyright year -> 2023

Geez, working in too many workspaces. Thank you, just pushed the update

LanceAndersen · 2023-04-14T21:33:05Z

Looks good to me.

Thank you Christian

eirbjo

LGTM. Leaving some minor comments on the test.

eirbjo · 2023-04-14T21:29:12Z

test/jdk/jdk/nio/zipfs/ZipFSDirectoryExceptionMessageTest.java

+    /**
+     * Zip file to create
+     */
+    public static final String ZIP_FILE = "directoryExceptionTest.zip";


Maybe ZIP_FILE could be a Path, to avoid Path.of(ZIP_FILE) wrapping later in the test?

I often do that but chose not to when I created the test but have updated per your suggestion

eirbjo · 2023-04-14T21:30:40Z

test/jdk/jdk/nio/zipfs/ZipFSDirectoryExceptionMessageTest.java

+/**
+ * @test
+ * @bug 8305945
+ * @summary Validate that Zip FS provides the correct exception message


This summary could perhaps be a bit more specific about which condition / message it is testing.

eirbjo · 2023-04-14T22:09:03Z

test/jdk/jdk/nio/zipfs/ZipFSDirectoryExceptionMessageTest.java

+        try (FileSystem zipfs = FileSystems.newFileSystem(ZIP_FILE)) {
+            var file = zipfs.getPath(DIRECTORY_NAME);
+            var x = assertThrows(FileSystemException.class, () -> Files.newInputStream(file));
+            assertEquals(x.getMessage(), DIR_EXCEPTION_MESSAGE);


JUnit asserts have the expected parameter first followed by the actual.

AlanBateman · 2023-04-15T05:43:09Z

test/jdk/jdk/nio/zipfs/ZipFSDirectoryExceptionMessageTest.java

+        try (FileSystem zipfs = FileSystems.newFileSystem(ZIP_FILE)) {
+            var file = zipfs.getPath(DIRECTORY_NAME);
+            var x = assertThrows(FileSystemException.class, () -> Files.newInputStream(file));
+            assertEquals(DIR_EXCEPTION_MESSAGE, x.getMessage());


FYI, test doesn't need to depend the exception message. Checking FileSystemException::getOtherFile returns null would be a more robust way of checking that it didn't provide a second file by mistake, e.g.

try { Files.newInputStream(DIRECTORY_NAME); fail(); } catch (FileSystemException e) { assertNull(e.getOtherFile()); } catch (IOException ioe) { // allowed }

added validation for other file being null but also kept the existing validation of the message

Okay, although I assume this test will fail if it throws a more general IOException (it's allowed to do that) or there is any adjustment to the error message.

Yes, in the unlikely event that either FileSystemException or the code below from zipfs is changed, then yes the error message could evolve

if (e.isDir()) throw new FileSystemException(getString(path), null, "is a directory");

I do not have a strong preference once way or the other and am happy to remove the message validation if that is what you prefer. I kept the validation as the original issue was surrounding the actual message being generated which aligns with the text included by zipfs when it threw the exception when obtaining the inputstream

I think it is worth mentioning that we have other tests that also validate exception messages(and we recently updated a couple zip tests because the message changed). So if we are going to move in the direction of not validating exception messages in tests, we should probably consider adding verbiage to the developers guide to provide that guidance and be consistent within the test suite

Let me know your preference and thank you for your thoughts as it is a useful discussion

What you have is fine, I'm just making the point that the exception message is somewhat secondary, the important test is that if FileSystemException is thrown then its getOtherFile return should null as there is only one file in this operation.

What you have is fine, I'm just making the point that the exception message is somewhat secondary

I guess this is also a question of testing the interface vs. testing the implementation.

To get good coverage of validation scenarios, I often find it quite useful to test on the actual message. This makes the test overspecific to the interface, but it helps make sure that each error condition is covered independently. Without asserting on the actual message, it can be hard to know if you are being shadowed by another higher level error condition. I've seen this a lot when adding test coverage for various ZIP processing code.

LanceAndersen · 2023-04-20T14:07:34Z

/integrate

openjdk · 2023-04-20T14:09:38Z

Going to push as commit c6a288d.
Since your change was applied there have been 276 commits pushed to the master branch:

73018b3: 8306284: G1: Remove assertion in G1ScanHRForRegionClosure::do_claimed_block
33a7978: 8306538: Zero variant build failure after JDK-8257967
9c2e5b3: 8306459: s390x: Replace NULL to nullptr
6a7dff3: 8305880: Loom: Avoid putting stale object pointers in oops
310aa93: 8304291: [AIX] Broken build after JDK-8301998
64ed816: 8305943: Open source few AWT Focus related tests
b8f0a66: 8041676: remove the java.compiler system property
781d6d7: 8306372: Open source AWT CardLayout and Checkbox tests
d03128d: 8306280: Open source several choice AWT tests
e764e9b: 8306452: Fix Amazon copyright in JDK-8305425 test
... and 266 more: https://git.openjdk.org/jdk/compare/7239150f8aff0e3dc07c5b27f6b7fb07237bfc55...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-04-20T14:09:59Z

@LanceAndersen Pushed as commit c6a288d.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

(zipfs) Opening a directory to get input stream produces incorrect ex…

e99a91b

…ception message

openjdk bot added the rfr Pull request is ready for review label Apr 14, 2023

openjdk bot added nio nio-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Apr 14, 2023

naotoj approved these changes Apr 14, 2023

View reviewed changes

sormuras approved these changes Apr 14, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 14, 2023

Update the copyright year

b1dfab8

Minor wordsmithing of test comment

8fbf69a

eirbjo approved these changes Apr 14, 2023

View reviewed changes

Address additional feedback

f91f8f6

eirbjo reviewed Apr 14, 2023

View reviewed changes

re-arrage assertEquals params

3067fd1

AlanBateman reviewed Apr 15, 2023

View reviewed changes

added valiation that other file is null

74534d5

openjdk bot added the integrated Pull request has been integrated label Apr 20, 2023

openjdk bot closed this Apr 20, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8305945: (zipfs) Opening a directory to get input stream produces incorrect exception message #13482

8305945: (zipfs) Opening a directory to get input stream produces incorrect exception message #13482

LanceAndersen commented Apr 14, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Apr 14, 2023

openjdk bot commented Apr 14, 2023

mlbridge bot commented Apr 14, 2023 •

edited

Loading

naotoj left a comment

sormuras left a comment

openjdk bot commented Apr 14, 2023 •

edited

Loading

LanceAndersen commented Apr 14, 2023

LanceAndersen commented Apr 14, 2023

eirbjo left a comment

eirbjo Apr 14, 2023

LanceAndersen Apr 14, 2023

eirbjo Apr 14, 2023

LanceAndersen Apr 14, 2023

eirbjo Apr 14, 2023

AlanBateman Apr 15, 2023

LanceAndersen Apr 15, 2023

AlanBateman Apr 16, 2023

LanceAndersen Apr 16, 2023

AlanBateman Apr 17, 2023

eirbjo Apr 17, 2023

LanceAndersen commented Apr 20, 2023

openjdk bot commented Apr 20, 2023

openjdk bot commented Apr 20, 2023

8305945: (zipfs) Opening a directory to get input stream produces incorrect exception message #13482

8305945: (zipfs) Opening a directory to get input stream produces incorrect exception message #13482

Conversation

LanceAndersen commented Apr 14, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Apr 14, 2023

openjdk bot commented Apr 14, 2023

mlbridge bot commented Apr 14, 2023 • edited Loading

Webrevs

naotoj left a comment

Choose a reason for hiding this comment

sormuras left a comment

Choose a reason for hiding this comment

openjdk bot commented Apr 14, 2023 • edited Loading

LanceAndersen commented Apr 14, 2023

LanceAndersen commented Apr 14, 2023

eirbjo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LanceAndersen commented Apr 20, 2023

openjdk bot commented Apr 20, 2023

openjdk bot commented Apr 20, 2023

LanceAndersen commented Apr 14, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Apr 14, 2023 •

edited

Loading

openjdk bot commented Apr 14, 2023 •

edited

Loading