On Wed, 18 Jun 2025 16:48:45 GMT, David Beaumont <[email protected]> wrote:
>> Simplifying JavaRuntimeURLConnection to avoid accidentally returning
>> non-resource data to users.
>>
>> This change has the following distinct parts:
>> 1. Refactor code to use Node instead of directly accessing low level
>> ImageLocation type.
>> 2. Remove unnecessary use of "Resource" interface and related URL generation
>> code (completely unreachable).
>> 3. Adding comments explaining why there's a non-obvious distinction in how
>> module and resource names are treated with respect to URL percent encoding.
>> 4. Small constructor logic simplification (module name cannot be null
>> anymore)
>> 5. Small simplification around 'READER' use, since it is impossible for that
>> to ever be null (other users of ImageReaderFactory already assume it could
>> never be null, and code path analysis agrees).
>> 6. Adding tests for the non-resource cases.
>> 7. Adding extra test data to check the behaviour with respect to things like
>> percent escaping (previously untested).
>> 8. Adding TODO comments for things I could do in this PR or later (reviewer
>> opinions welcome).
>
> David Beaumont has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Reworking comment and adjusting TODOs.
The changes looks mostly okay, just minor comments on comments, TODOs and
naming.
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)".
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.
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.
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.
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.
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.
-------------
PR Review: https://git.openjdk.org/jdk/pull/25871#pullrequestreview-2943467201
PR Review Comment: https://git.openjdk.org/jdk/pull/25871#discussion_r2157287702
PR Review Comment: https://git.openjdk.org/jdk/pull/25871#discussion_r2157291827
PR Review Comment: https://git.openjdk.org/jdk/pull/25871#discussion_r2157284956
PR Review Comment: https://git.openjdk.org/jdk/pull/25871#discussion_r2157300971
PR Review Comment: https://git.openjdk.org/jdk/pull/25871#discussion_r2157289343
PR Review Comment: https://git.openjdk.org/jdk/pull/25871#discussion_r2157296409
