RFR: 8359808: JavaRuntimeURLConnection should only connect to non-directory resources [v2]

[David Beaumont]

On Thu, 19 Jun 2025 15:36:13 GMT, Alan Bateman <alanb at openjdk.org> wrote:

>> David Beaumont has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Reworking comment and adjusting TODOs.
>
> src/java.base/share/classes/sun/net/www/protocol/jrt/JavaRuntimeURLConnection.java line 48:
> 
>> 46: 
>> 47:     // ImageReader to access resources in jimage (never null).
>> 48:     private static final ImageReader READER = ImageReaderFactory.getImageReader();
> 
> I think it simpler if you drop "(never null)".

I was just stressing that (unlike the old code) there's actually no need for a null check.

> src/java.base/share/classes/sun/net/www/protocol/jrt/JavaRuntimeURLConnection.java line 50:
> 
>> 48:     private static final ImageReader READER = ImageReaderFactory.getImageReader();
>> 49: 
>> 50:     // The module and resource name in the URL ("jrt:/<module-name>/<resource-name>").
> 
> In JEP 220 we use jrt:/[$MODULE[/$PATH]] to make it clear that the resource is optionally.

Well the resource is optional only it the same way that you can given any invalid URL. Connection only works when both a module and a resource name are present.

> src/java.base/share/classes/sun/net/www/protocol/jrt/JavaRuntimeURLConnection.java line 56:
> 
>> 54:     // about percent encoding, and it is likely that any differences between how
>> 55:     // module names and resource names are treated is unintentional. The rules
>> 56:     // about percent encoding may well be tightened up in the future.
> 
> I think it would be better to drop this paragraph from the comment, it just begs too many questions.

Okay. This will become moot when the URL encoding is tidied up.

> src/java.base/share/classes/sun/net/www/protocol/jrt/JavaRuntimeURLConnection.java line 64:
> 
>> 62: 
>> 63:     // The resource node (when connected).
>> 64:     private volatile Node resource;
> 
> Maybe better to rename to node, resourceNode, or imageNode here.

Done (resourceNode).

> src/java.base/share/classes/sun/net/www/protocol/jrt/JavaRuntimeURLConnection.java line 69:
> 
>> 67:         super(url);
>> 68:         // TODO: Allow percent encoding in module names.
>> 69:         // TODO: Consider rejecting URLs with fragments, queries or authority.
> 
> I'd prefer not include all the TODO messages.

These were here explicitly to be discussed in review. If you think it's now covered by JDK-8359949, I can remove them.

> src/java.base/share/classes/sun/net/www/protocol/jrt/JavaRuntimeURLConnection.java line 78:
> 
>> 76:             // No trailing resource path. This can never "connect" or return a
>> 77:             // resource, but might be useful as a representation to pass around.
>> 78:             // The module name *can* be empty here (e.g. "jrt:/") but not null.
> 
> If the URL scheme from JEP 220 goes into the first comment then it will be clear that the resource is optionally. This will allow the comment here to be reduced.

Can you elaborate? I'm not quite clear what changes you're suggesting here.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/25871#discussion_r2158674474
PR Review Comment: https://git.openjdk.org/jdk/pull/25871#discussion_r2158681461
PR Review Comment: https://git.openjdk.org/jdk/pull/25871#discussion_r2158672447
PR Review Comment: https://git.openjdk.org/jdk/pull/25871#discussion_r2158683837
PR Review Comment: https://git.openjdk.org/jdk/pull/25871#discussion_r2158677926
PR Review Comment: https://git.openjdk.org/jdk/pull/25871#discussion_r2158683599