On Wed, 18 Jun 2025 12:53:56 GMT, David Beaumont <d...@openjdk.org> 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). Preloading a few explanatory comments. src/java.base/share/classes/sun/net/www/protocol/jrt/JavaRuntimeURLConnection.java line 49: > 47: > 48: // ImageReader to access resources in jimage > 49: private static final ImageReader reader = > ImageReaderFactory.getImageReader(); Examination of code and other users shows that this could never be null. Other users also name it "READER". src/java.base/share/classes/sun/net/www/protocol/jrt/JavaRuntimeURLConnection.java line 56: > 54: > 55: // the Resource when connected > 56: private volatile Resource resource; The Resource API was never needed here and adds uncalled methods such as `getURL()` which are then implemented with code that could never be invoked. src/java.base/share/classes/sun/net/www/protocol/jrt/JavaRuntimeURLConnection.java line 64: > 62: throw new MalformedURLException(url + " missing path or /"); > 63: if (path.length() == 1) { > 64: this.module = null; There's no reason for the module==null case, since the only thing caring that this might be null just converts null to the empty string anyway. src/java.base/share/classes/sun/net/www/protocol/jrt/JavaRuntimeURLConnection.java line 101: > 99: } > 100: this.resource = node; > 101: super.connected = true; I know I don't *need* to use super here, but it documents the fact that this is not a field of this subclass, without readers having to go check. Mutable protected fields are weird and error prone, so I felt calling it out a bit was worth it. Happy to replace with a comment if people feel that's better though. src/java.base/share/classes/sun/net/www/protocol/jrt/JavaRuntimeURLConnection.java line 119: > 117: public long getContentLengthLong() { > 118: try { > 119: return getResourceNode().size(); Having getResourceNode() return the (lazily fetched) node avoids the reader needing to know/reason about how "connect()" has the side-effect of making the "resource" field non-null. ------------- PR Review: https://git.openjdk.org/jdk/pull/25871#pullrequestreview-2939094081 PR Review Comment: https://git.openjdk.org/jdk/pull/25871#discussion_r2154527602 PR Review Comment: https://git.openjdk.org/jdk/pull/25871#discussion_r2154529805 PR Review Comment: https://git.openjdk.org/jdk/pull/25871#discussion_r2154532882 PR Review Comment: https://git.openjdk.org/jdk/pull/25871#discussion_r2154538127 PR Review Comment: https://git.openjdk.org/jdk/pull/25871#discussion_r2154543346