8352858: Make java.net.JarURLConnection fields final

[eirbjo]

Please help review this cleanup PR which makes the java.net.JarURLConnection fields jarFileURL and entryName final.

The current parseSpec method is somewhat crufty, with some code quality issues which this PR aims to improve:

• The method-level comment seems stale and misplaced
• The local variable separator is confusingly incremented during parsing (using pre AND post increment operators)
• Unused local variables introduced for the sole purpose of attaching @SuppressWarnings annotations
• The jarFileURL and entryName fields are both assigned more than once
• Block comments are used where line comments would suffice

The PR addresses the above issues by inlining parseSpec into the constructor, then extracting static helper methods for parsing the file URL and the entry name. This allows the fields to be made final.

Since this is purely a refactoring PR, no tests are updated and the noreg-cleanup label is added in JBS.

Reviewing individual commits in this PR may aid verification of separate refactorings.

---

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-8352858: Make java.net.JarURLConnection fields final (Enhancement - P4)

Reviewers

• Jaikiran Pai (@jaikiran - Reviewer)
• Daniel Fuchs (@dfuch - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24218

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back eirbjo! 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]

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

8352858: Make java.net.JarURLConnection fields final

Reviewed-by: jpai, dfuchs

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

• f5a0db4: 8315447: Invalid Type Annotation attached to a method instead of a lambda
• 60544a1: 8164714: Constructor.newInstance creates instance of inner class with null outer class
• c856b34: 8352587: C2 SuperWord: we must avoid Multiversioning for PeelMainPost loops
• 993eae4: 8346948: Update CLDR to Version 47.0
• e98838f: 8352065: [PPC64] C2: Implement PopCountVL, CountLeadingZerosV and CountTrailingZerosV nodes
• 03105fc: 8351601: [JMH] test UnixSocketChannelReadWrite failed for 2 threads config
• fe03e2e: 8351897: Extra closing curly brace typos in Javadoc
• fa0b18b: 8352509: Update jdk.test.lib.SecurityTools jar method to accept List parameter
• 3ac9678: 8351224: Deprecate com.sun.tools.attach.AttachPermission for removal
• 997aa17: 8351310: Deprecate com.sun.jdi.JDIPermission for removal
• ... and 804 more: https://git.openjdk.org/jdk/compare/6cc1c0abdbf8cd3d01722951cf34ebcb667f1380...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]

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

• net

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]

Webrevs

• 01: Full - Incremental (ddcb18d9)
• 00: Full (1f9fc452)

[jaikiran]

This looks reasonable to me. Please run tier1 and tier2 to make sure nothing fails unexpectedly.

[jaikiran]

Thank you for the update. Looks fine to me.

[eirbjo]

@jaikiran GHA is green and tier2 completed with a single test failing:

java.lang.RuntimeException: 'signer certificate expired on .*. However, the JAR will be valid' missing from stdout/stderr
	at jdk.test.lib.process.OutputAnalyzer.shouldMatch(OutputAnalyzer.java:372)
	at TimestampCheck.main(TimestampCheck.java:242)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
	at java.base/java.lang.reflect.Method.invoke(Method.java:565)

This seems unrelated and already fixed in JDK-8352302

I'll let the earth spin just in case other reviewers have opinions, then integrate.

[dfuch]

Hi Eirik, looks reasonable - thanks for holding off integrating: I've sent the patch to our CI and will report back when I get the results.

[dfuch]

Proposed changes look reasonable. More final is good!
CI results showed up green.

[eirbjo]

Just noticed that this comment seems misplaced and is confusing.

The check just asserts that the URL contains the separator. I don't see how this is related to nesting at all. If the URL was nested, we would have more than one separator, right?

If we agree this comment is misplaced, perhaps we should just remove it?

Or perhaps the REMIND here is a type of TODO?

[dfuch]

I believe this is just a reminder that there should just be one single "!/" because nested "!/" in URL are not supported.

[eirbjo]

Alright, however mysterious it seems to carry some value. Let's keep it around.

Thanks for your reviews!

[eirbjo]

/integrate

[openjdk]

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

• a81250c: 8352673: RISC-V: Vector can't be turned on with -XX:+UseRVV
• 0935ba9: 8352481: Enforce the use of lld with clang
• dbc620f: 8352299: GenShen: Young cycles that interrupt old cycles cannot be cancelled
• f5a0db4: 8315447: Invalid Type Annotation attached to a method instead of a lambda
• 60544a1: 8164714: Constructor.newInstance creates instance of inner class with null outer class
• c856b34: 8352587: C2 SuperWord: we must avoid Multiversioning for PeelMainPost loops
• 993eae4: 8346948: Update CLDR to Version 47.0
• e98838f: 8352065: [PPC64] C2: Implement PopCountVL, CountLeadingZerosV and CountTrailingZerosV nodes
• 03105fc: 8351601: [JMH] test UnixSocketChannelReadWrite failed for 2 threads config
• fe03e2e: 8351897: Extra closing curly brace typos in Javadoc
• ... and 807 more: https://git.openjdk.org/jdk/compare/6cc1c0abdbf8cd3d01722951cf34ebcb667f1380...master

Your commit was automatically rebased without conflicts.

[openjdk]

@eirbjo Pushed as commit 1a8c8e0.

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