8320943: Files/probeContentType/Basic.java fails on latest Windows 11 - content type mismatch #16916

bplb · 2023-12-01T01:53:23Z

If Windows 11+ is detected, add an extra expected MIME type for certain file extensions.

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-8320943: Files/probeContentType/Basic.java fails on latest Windows 11 - content type mismatch (Bug - P4)

Reviewers

Christian Stein (@sormuras - Committer) ⚠️ Review applies to 44a7cab3
Roger Riggs (@RogerRiggs - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16916

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

Using diff file

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

Webrev

Link to Webrev Comment

… - content type mismatch

bridgekeeper · 2023-12-01T01:54:45Z

👋 Welcome back bpb! 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-12-01T01:55:21Z

@bplb The following label will be automatically applied to this pull request:

nio

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

mlbridge · 2023-12-01T01:58:34Z

Webrevs

sormuras

Works on my machine (w/o MS Office being installed).

RogerRiggs · 2023-12-01T16:12:02Z

test/jdk/java/nio/file/Files/probeContentType/Basic.java

+    private static final String EXTRA_BZ2 = IS_WIN11_PLUS ? "application/x-compressed" : "";
+    private static final String EXTRA_CSV = IS_WIN11_PLUS ? "application/vnd.ms-excel" : "";
+    private static final String EXTRA_RAR = IS_WIN11_PLUS ? "application/x-compressed" : "";
+    private static final String EXTRA_RTF = IS_WIN11_PLUS ? "application/msword" : "";
+    private static final String EXTRA_7Z  = IS_WIN11_PLUS ? "application/x-compressed" : "";


It could be misleading to put a empty string into the expected set.
Could the lists be created depending on the OS?

private static final List<String> BZ2_TYPES = IS_WIN11_PLUS ? List.of("application/bz2", "application/x-bzip2", "application/x-bzip", "application/x-compressed") ? List.of("application/bz2", "application/x-bzip2", "application/x-bzip");

Does WIndows 11 have a (user) way to change the mapping?
For example, the mapping of .csv to vnd.ms-excel might not be correct if the user selected OpenOffice to handle .csv. I don't think we have a mechanism to cope with that.

Does WIndows 11 have a (user) way to change the mapping?

The mapping comes from the Windows registry so it could be changed but I wouldn't expect that to be common. Part of me wonders if this test should be reduced to a small number of extensions, 30 seems a lot to have to keep up to date with changes like this.

Not related to the above comments, but it looks as if the Windows registry might only be able to handle one MIME type per extension. Also, I wonder how much other software was broken by this mapping change.

It could be misleading to put a empty string into the expected set. Could the lists be created depending on the OS?

I agree. I'll see what I can change.

Does WIndows 11 have a (user) way to change the mapping? For example, the mapping of .csv to vnd.ms-excel might not be correct if the user selected OpenOffice to handle .csv. I don't think we have a mechanism to cope with that.

No, we don't.

Part of me wonders if this test should be reduced to a small number of extensions, 30 seems a lot to have to keep up to date with changes like this.

I have kept them all for the time being. If a similar problem crops up in the future, however, I agree that clamping it to a smaller number of extensions might be better. The main thing is to test whether the framework functions correctly, not that every last extension yields the expected value.

…itionally on whether it is Windows 11+

RogerRiggs

Looks good; the use of List makes it easier to customize.

openjdk · 2023-12-01T19:48:17Z

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

8320943: Files/probeContentType/Basic.java fails on latest Windows 11 - content type mismatch

Reviewed-by: cstein, rriggs

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

30b5d42: 8321069: JvmtiThreadState::state_for_while_locked() returns nullptr for an attached JNI thread with a java.lang.Thread object after JDK-8319935
bd04f91: 8321131: Console read line with zero out should zero out underlying buffer in JLine
155abc5: 8311906: Improve robustness of String constructors with mutable array inputs
316b783: 8321276: runtime/cds/appcds/dynamicArchive/DynamicSharedSymbols.java failed with "'17 2: jdk/test/lib/apps ' missing from stdout/stderr"
65be5e0: 8305931: jdk/jfr/jcmd/TestJcmdDumpPathToGCRoots.java failed with "Expected chains but found none"
f6be922: 8316193: jdk/jfr/event/oldobject/TestListenerLeak.java java.lang.Exception: Could not find leak
a9de5c7: 8315128: jdk/jfr/event/runtime/TestResidentSetSizeEvent.java fails with "The size should be less than or equal to peak"
d2c529c: 8319072: JFR: Turn off events for JFR.view
d5f59cf: 8321220: JFR: RecordedClass reports incorrect modifiers
9769dfe: 8321214: Parallel: Remove unused SpaceInfo::_min_dense_prefix
... and 36 more: https://git.openjdk.org/jdk/compare/7ad700596fbc58c27fedfa46755e80ea57712fac...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.

RogerRiggs · 2023-12-01T19:49:28Z

The tests here are just of the code that is using system APIs to map from extension to type.
It doesn't need to verify every entry in the OS's database. Just a sampling and any special cases.

turbanoff · 2023-12-02T19:43:42Z

test/jdk/java/nio/file/Files/probeContentType/Basic.java

+        exTypes.add(new ExType("php", List.of("text/plain", "text/php", "application/x-php")));
+        exTypes.add(new ExType("png", List.of("image/png")));
+        exTypes.add(new ExType("ppt", List.of("application/vnd.ms-powerpoint")));
+        exTypes.add(new ExType("pptx",List.of("application/vnd.openxmlformats-officedocument.presentationml.presentation")));


Suggested change

exTypes.add(new ExType("pptx",List.of("application/vnd.openxmlformats-officedocument.presentationml.presentation")));

exTypes.add(new ExType("pptx", List.of("application/vnd.openxmlformats-officedocument.presentationml.presentation")));

bplb · 2023-12-04T16:59:53Z

test/jdk/java/nio/file/Files/probeContentType/Basic.java

+        if (OperatingSystem.isWindows() &&
+            (System.getProperty("os.name").endsWith("11") ||
+                new OSVersion(10, 0).compareTo(OSVersion.current()) > 0)) {
+            System.out.println("Windows 11+ detected: using different types");


I think it would be better if the Windows 11+ types were appended to the list of types here instead of replacing the types altogether.

RogerRiggs

LGTM

bplb · 2023-12-05T15:54:54Z

/integrate

openjdk · 2023-12-05T15:56:32Z

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

800f347: 8321216: SerialGC attempts to access the card table beyond the end of the heap during card table scan
a1fe16b: 8321300: Cleanup TestHFA
5b02188: 8321105: Enable UseCryptoPmullForCRC32 for Neoverse V2
4fbf22b: 8320652: ThreadInfo.isInNative needs to be updated to say what executing native code means
672f373: 8321163: [test] OutputAnalyzer.getExitValue() unnecessarily logs even when process has already completed
30817b7: 8317809: Insertion of free code blobs into code cache can be very slow during class unloading
a56286f: 8321269: Require platforms to define DEFAULT_CACHE_LINE_SIZE
1cf7ef5: 8321273: Parallel: Remove unused UpdateOnlyClosure::_space_id
517b178: 8306914: Implement JEP 458: Launch Multi-File Source-Code Programs
aec3865: 8320697: RISC-V: Small refactoring for runtime calls
... and 48 more: https://git.openjdk.org/jdk/compare/7ad700596fbc58c27fedfa46755e80ea57712fac...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-12-05T15:56:38Z

@bplb Pushed as commit 87516e2.

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

8320943: Files/probeContentType/Basic.java fails on latest Windows 11…

44a7cab

… - content type mismatch

openjdk bot added the rfr Pull request is ready for review label Dec 1, 2023

openjdk bot added the nio nio-dev@openjdk.org label Dec 1, 2023

sormuras approved these changes Dec 1, 2023

View reviewed changes

RogerRiggs reviewed Dec 1, 2023

View reviewed changes

8320943: Change exTypes to a list and add certain EXType records cond…

d8233c9

…itionally on whether it is Windows 11+

RogerRiggs approved these changes Dec 1, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 1, 2023

turbanoff reviewed Dec 2, 2023

View reviewed changes

bplb commented Dec 4, 2023

View reviewed changes

8320943: For Windows 11+ cases, append instead of replacing type lists

7a74f30

bplb requested a review from RogerRiggs December 4, 2023 21:50

RogerRiggs approved these changes Dec 4, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Dec 5, 2023

openjdk bot closed this Dec 5, 2023

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

bplb deleted the Files-probeContentType-8320943 branch December 6, 2023 04:25

	exTypes.add(new ExType("pptx",List.of("application/vnd.openxmlformats-officedocument.presentationml.presentation")));
	exTypes.add(new ExType("pptx", List.of("application/vnd.openxmlformats-officedocument.presentationml.presentation")));

8320943: Files/probeContentType/Basic.java fails on latest Windows 11 - content type mismatch #16916

8320943: Files/probeContentType/Basic.java fails on latest Windows 11 - content type mismatch #16916

Uh oh!

Conversation

bplb commented Dec 1, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Dec 1, 2023

Uh oh!

openjdk bot commented Dec 1, 2023

Uh oh!

mlbridge bot commented Dec 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

sormuras left a comment

Choose a reason for hiding this comment

Uh oh!

RogerRiggs Dec 1, 2023

Choose a reason for hiding this comment

Uh oh!

AlanBateman Dec 1, 2023

Choose a reason for hiding this comment

Uh oh!

bplb Dec 1, 2023

Choose a reason for hiding this comment

Uh oh!

bplb Dec 1, 2023

Choose a reason for hiding this comment

Uh oh!

bplb Dec 1, 2023

Choose a reason for hiding this comment

Uh oh!

RogerRiggs left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Dec 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RogerRiggs commented Dec 1, 2023

Uh oh!

turbanoff Dec 2, 2023

Choose a reason for hiding this comment

Uh oh!

bplb Dec 4, 2023

Choose a reason for hiding this comment

Uh oh!

RogerRiggs left a comment

Choose a reason for hiding this comment

Uh oh!

bplb commented Dec 5, 2023

Uh oh!

openjdk bot commented Dec 5, 2023

Uh oh!

openjdk bot commented Dec 5, 2023

Uh oh!

Uh oh!

bplb commented Dec 1, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Dec 1, 2023 •

edited

Loading

openjdk bot commented Dec 1, 2023 •

edited

Loading