8273935: (zipfs) Files.getFileAttributeView() throws UOE instead of returning null when view not supported #5579

LanceAndersen · 2021-09-19T15:34:44Z

Hi all,

Please review this patch which addresses the issue where Zip FS will throw a UOE instead of returning null when Files.getFileAttributeView() is invoked and the view not supported.

Mach5 tiers1 - tier3 are clean.

Best
Lance

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8273935: (zipfs) Files.getFileAttributeView() throws UOE instead of returning null when view not supported

Reviewers

Alan Bateman (@AlanBateman - Reviewer)
Brian Burkhalter (@bplb - Reviewer)
Severin Gehwolf (@jerboaa - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5579/head:pull/5579
$ git checkout pull/5579

Update a local copy of the PR:
$ git checkout pull/5579
$ git pull https://git.openjdk.java.net/jdk pull/5579/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5579

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5579.diff

…n view not supported

bridgekeeper · 2021-09-19T15:34:59Z

👋 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.

LanceAndersen · 2021-09-20T10:18:18Z

/cc core-libs nio-dev

openjdk · 2021-09-20T11:17:17Z

@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 · 2021-09-20T11:23:18Z

Webrevs

AlanBateman · 2021-09-20T11:28:10Z

src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipPath.java

@@ -713,7 +713,7 @@ public void remove() {
            if (type == FileOwnerAttributeView.class)
                return (V)new ZipPosixFileAttributeView(this,true);
        }
-        throw new UnsupportedOperationException("view <" + type + "> is not supported");
+        return (V) null;


I assume (V) isn't needed here.

No, it is not and can also probably be removed from open/src/java.base/unix/classes/sun/nio/fs/UnixFileSystemProvider.java at some point

AlanBateman · 2021-09-20T11:28:47Z

test/jdk/jdk/nio/zipfs/testng/test/PosixAttributeViewTest.java

+    @BeforeTest
+    public void setup() throws IOException {
+        Files.deleteIfExists(ZIP_FILE);
+        Entry e0 = Entry.of(ZIP_ENTRY, ZipEntry.DEFLATED,


Is there a reason why this is named "e0"?

Not really. Historically that was the variable name format used in some of the Zip FS tests. I changed the name to entry

AlanBateman · 2021-09-20T11:29:15Z

test/jdk/jdk/nio/zipfs/testng/test/PosixAttributeViewTest.java

+    protected Object[][] zipfsMap() {
+        return new Object[][]{
+                {Map.of()},
+                { Map.of("enablePosixFileAttributes", "true")}


Minor nit, inconsistent formatting with L81 vs. L82.

Fixed, thanks for catching that.

AlanBateman · 2021-09-20T11:29:27Z

test/jdk/jdk/nio/zipfs/testng/test/PosixAttributeViewTest.java

+     */
+    @Test(dataProvider = "zipfsMap")
+    public void testPosixAttributeView(Map<String, String> env) throws Exception {
+


Spurious blank line.

test/jdk/jdk/nio/zipfs/testng/test/PosixAttributeViewTest.java

openjdk · 2021-09-20T13:39:49Z

@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:

8273935: (zipfs) Files.getFileAttributeView() throws UOE instead of returning null when view not supported

Reviewed-by: alanb, bpb, sgehwolf

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 354 new commits pushed to the master branch:

65ed0a7: 8273655: content-types.properties files are missing some common types
c60bcd0: 8273928: Use named run ids when problem listing tests
6642d2e: 8273783: Simplify Metaspace arena guard handling
f242cb5: 8273797: Stop impersonating "server" VM in all VM variants
240fa6e: 8273927: Enable hsdis for riscv64
9c91ff5: 8274031: Typo in StringBuilder.readObject
d16bf04: 8273613: JFR: RemoteRecordingStream::start() blocks close()
7ce60c6: 8273651: JFR: onMetadata(), setStartTime(), setEndTime() lacks test coverage
1bd11a7: 8273887: [macos] java/awt/color/ICC_ColorSpace/MTTransformReplacedProfile.java timed out
ee3576a: 8256735: JFR: 'jfr' tool displays incorrect timestamps
... and 344 more: https://git.openjdk.java.net/jdk/compare/4d6593ce0243457e7431a5990957a8f880e0a3fb...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.

openjdk · 2021-09-20T13:43:14Z

@LanceAndersen The core-libs label was already applied.
The nio label was already applied.

jerboaa · 2021-09-20T17:05:33Z

test/jdk/jdk/nio/zipfs/testng/test/PosixAttributeViewTest.java

+            Path entry = fs.getPath(ZIP_ENTRY);
+            PosixFileAttributeView view = Files.getFileAttributeView(entry,
+                    PosixFileAttributeView.class);
+            System.out.printf("View returned: %s%n", view);


It might be useful to also print whether the environment is empty:
Suggestion: System.out.printf("View returned: %s, (empty_env=%s)%n", view, env.isEmpty());

Well, you can see that whether the Map is/is not empty in the jtr file for the test run (based on the DataProvider Properties for a given run).

However, per your suggestion, I tweaked the test to dump the Map values.

LanceAndersen · 2021-09-21T19:20:59Z

/integrate

openjdk · 2021-09-21T19:24:16Z

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

0fc47e9: 8266666: Implementation for snippets
6d91a3e: 8269039: Disable SHA-1 Signed JARs
42d5d2a: 8274056: JavaAccessibilityUtilities leaks JNI objects
57df0db: 8270873: JFR: Catch DirectoryIteratorException when scanning for .jfr files
111d5e1: 8273915: Create 'nosafepoint' rank
7acec3f: 8236505: Mark jdk/editpad/EditPadTest.java as @headful
afd218d: 8274053: [BACKOUT] JDK-8270842: G1: Only young regions need to redirty outside references in remset.
a5108a6: 8273646: Add openssl from path variable also in to Default System Openssl Path in OpensslArtifactFetcher
65ed0a7: 8273655: content-types.properties files are missing some common types
c60bcd0: 8273928: Use named run ids when problem listing tests
... and 352 more: https://git.openjdk.java.net/jdk/compare/4d6593ce0243457e7431a5990957a8f880e0a3fb...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-09-21T19:24:32Z

@LanceAndersen Pushed as commit 161fdb4.

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

LanceAndersen added 3 commits September 19, 2021 11:16

Files.getFileAttributeView() throws UOE instead of returning null whe…

3fc60d8

…n view not supported

adjust copyright

b20d9e2

Add Cleanup Method

e814a9f

openjdk bot added the rfr Pull request is ready for review label Sep 19, 2021

openjdk bot added nio nio-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Sep 20, 2021

AlanBateman approved these changes Sep 20, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 20, 2021

jerboaa reviewed Sep 20, 2021

View reviewed changes

bplb approved these changes Sep 20, 2021

View reviewed changes

LanceAndersen added 2 commits September 20, 2021 14:09

Updates based on feedback

6ff4919

fix minor typo

65184a5

AlanBateman approved these changes Sep 20, 2021

View reviewed changes

bplb approved these changes Sep 20, 2021

View reviewed changes

jerboaa approved these changes Sep 21, 2021

View reviewed changes

openjdk bot closed this Sep 21, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8273935: (zipfs) Files.getFileAttributeView() throws UOE instead of returning null when view not supported #5579

8273935: (zipfs) Files.getFileAttributeView() throws UOE instead of returning null when view not supported #5579

LanceAndersen commented Sep 19, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 19, 2021

LanceAndersen commented Sep 20, 2021

openjdk bot commented Sep 20, 2021

mlbridge bot commented Sep 20, 2021 •

edited

Loading

AlanBateman Sep 20, 2021

LanceAndersen Sep 20, 2021

AlanBateman Sep 20, 2021

LanceAndersen Sep 20, 2021

AlanBateman Sep 20, 2021

LanceAndersen Sep 20, 2021

AlanBateman Sep 20, 2021

LanceAndersen Sep 20, 2021

openjdk bot commented Sep 20, 2021 •

edited

Loading

openjdk bot commented Sep 20, 2021

jerboaa Sep 20, 2021

LanceAndersen Sep 20, 2021

LanceAndersen commented Sep 21, 2021

openjdk bot commented Sep 21, 2021

openjdk bot commented Sep 21, 2021

8273935: (zipfs) Files.getFileAttributeView() throws UOE instead of returning null when view not supported #5579

8273935: (zipfs) Files.getFileAttributeView() throws UOE instead of returning null when view not supported #5579

Conversation

LanceAndersen commented Sep 19, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Sep 19, 2021

LanceAndersen commented Sep 20, 2021

openjdk bot commented Sep 20, 2021

mlbridge bot commented Sep 20, 2021 • edited Loading

Webrevs

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

openjdk bot commented Sep 20, 2021 • edited Loading

openjdk bot commented Sep 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LanceAndersen commented Sep 21, 2021

openjdk bot commented Sep 21, 2021

openjdk bot commented Sep 21, 2021

LanceAndersen commented Sep 19, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 20, 2021 •

edited

Loading

openjdk bot commented Sep 20, 2021 •

edited

Loading