Issue #13588 - Improve ResourceFactory.split() and URIUtil.toURI to work properly on Windows. #13652

joakime · 2025-10-02T16:22:38Z

New testcases in ResourceFactoryTest.testSplit*
- including a test for doing a split of the java.class.path in an expected way.
Restore URIUtil.toURI(String) for the purpose of converting a raw string reference to a URI without causing a Resource mount to occur.

The existing implementation would Mount, then unwrap, then leave the mount mounted.
This new implementation doesn't even create the mount if unwrap is true.
The responsibility is to convert String references to absolute URIs.
Then it can be used to create a Resource.
Having this all be done within Resource.newResource() results in extra mounts and reference counts for no good reason.

…ork properly on Windows. + New testcases in ResourceFactoryTest.testResourceSplit* + Restore URIUtil.toURI(String) for the purpose of converting a raw string reference to a URI without causing a Resource mount to occur.

janbartel · 2025-10-02T23:23:07Z

jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/URIUtil.java

-                if (uri.getScheme().length() == 1)
+                // At this point we have a string detected as a URI.
+                // But that could also include Windows paths like "C:\path\to\foo.jar" or "C:/path/to/foo.jar"
+                String scheme = uri.getScheme();


If on windows I pass in a reference of file:///c:/some/thing.txt then is the scheme reported after conversion to URI file or c?

The scheme would be file

See the testcases in ResourceFactoryTest.testSplitIsAbsolute for some windows examples.

Also the testcases in URIUtilTest.testToURI (and it's parameters at URIUtilTest.toURICases)

jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/URIUtil.java

janbartel · 2025-10-02T23:27:47Z

jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/ResourceFactory.java

-                        if (!p.isAbsolute() && LOG.isDebugEnabled())
-                            LOG.warn("Non-absolute path: {}", reference);
-                        list.add(newResource(p.toAbsolutePath()));
+                        list.add(newResource(uri));


I'm a bit cautious that this is a behaviour change - previously relative references were ignored, but now they will be used.

Relative references are 100% necessary to support here.
This is a code path for many configuration from jetty-home/jetty-base and it's properties.
It's very common to use relative paths in the configuration, so that the same configuration can be supported across dev, qa, docker images, production, etc...

Prior to your change, relative references were not ignored.
Admittedly they were poorly tested, but that lack of testing is hopefully addressed in this PR.

Example:

[joakim@hyperion jetty-home-12.0.10]$ jshell --class-path lib/jetty-util-12.0.10.jar:lib/logging/jetty-slf4j-impl-12.0.10.jar:lib/logging/slf4j-api-2.0.12.jar | Welcome to JShell -- Version 17.0.15 | For an introduction type: /help intro jshell> import org.eclipse.jetty.util.resource.*; jshell> try (ResourceFactory.Closeable resourceFactory = ResourceFactory.closeable()) { ...> var result = resourceFactory.split("bar/a,bar/b,foo/z"); ...> System.out.println(result); ...> } [file:///home/joakim/code/jetty/distros/jetty-home-12.0.10/bar/a, file:///home/joakim/code/jetty/distros/jetty-home-12.0.10/bar/b, file:///home/joakim/code/jetty/distros/jetty-home-12.0.10/foo/z] jshell> /exit | Goodbye

janbartel · 2025-10-02T23:31:33Z

jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/resource/ResourceFactoryTest.java

-            resources = resourceFactory.split("jar:file:///foo/bar.jar!/", ",", false);
-            assertThat(resources.size(), is(0));
+            String rawConfig = String.join(",", rawInputs);
+            List<Resource> resources = resourceFactory.split(rawConfig, ",", true);


The previous code explicitly tests jar:file:///foo/bar.jar!/ with unwrap of false. Here it is tested with true.

We have the unwrap == false version in the other testcase.

See: testSplitExistingJarFileSystemNotUnwrappedIsMounted
and testSplitNonExistentFileReferenceNotUnwrapNotMounted

jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/resource/ResourceFactoryTest.java

janbartel · 2025-10-06T06:38:57Z

jetty-core/jetty-util/src/test/java/org/eclipse/jetty/util/resource/ResourceFactoryTest.java

+    /**
+     * If a jar filesystem {@code jar:file://} URI string reference is used against a real file that exists,
+     * with unwrap turned off, then this will result in a Resource that is mounted.
+     */


We don't ever want to result in any mounting from ResourceFactory.split. See my comment here: #13427 (comment)

The whole point of the unwrap boolean is to control that.

If you set unwrap == true then that's no mounting.

If you set unwrap == false then that's mounting.

Don't look at ResourceFactory.split as only having one use case.
There's many use cases for this method, and many of them want a mounted Resource.
It's only the java.class.path use case where we don't want a mounted resource from the get go.

No, unwrap doesn't determine whether or not it is mounted. It controls whether or not you want a jar:file: url stripped back to just the file: part. See the code comments here: https://github.com/jetty/jetty.project/blob/jetty-12.1.x/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/resource/ResourceFactory.java#L545

Also keep in mind that unwrap is only for jar:file:// based URIs in the ResourceFactory.split().

Other schemes, supported by Resource layer, can be present on the input string, and wont ever unwrap.

In graalvm a Jetty Resource can be created from the URI form resource:/foo/bar/root/.
That is a syntax often used for WebAppContext.setExtraClassPath(String) for example.
In graalvm, the root of that Path is already mounted by the time the JVM is fully started.
In Jetty Resource layer we use MountedPathResource to see it.

Don't look at ResourceFactory.split as only having one use case. There's many use cases for this method, and many of them want a mounted Resource. It's only the java.class.path use case where we don't want a mounted resource from the get go.

@joakime We've been down this road a few times... if utility methods that may be called during configuration mount, then we leak resources. Every time we've done this, we've been bitten and had to reverse out the changes. We cannot mount resources until we have an appropriate lifecycle managed resource factory that will handle the unmount.

Utility methods cannot mount because they cannot take responsibility for unmounting.

@joakime We've been down this road a few times... if utility methods that may be called during configuration mount, then we leak resources. Every time we've done this, we've been bitten and had to reverse out the changes. We cannot mount resources until we have an appropriate lifecycle managed resource factory that will handle the unmount.

Utility methods cannot mount because they cannot take responsibility for unmounting.

Then we should restore URIUtil.split() and work with ONLY the URIs.

As there are strings like resource:/foo/bar/ that would always use the Resource type MountedPathResource.
Mounting is a given if you are using Resource, not optional in some cases.

But working with URI is totally free of any ResourceFactory.newResource() side effects.

We can't work with URIs because we may be passed an filename rather than a URI. What is the issue with the current implementation of ResourceFactory.split that you are trying to solve?

We can't work with URIs because we may be passed an filename rather than a URI.

That's taken care of by URIUtil.split() now.
It will attempt to convert all inputs to absolute URIs.

What is the issue with the current implementation of ResourceFactory.split that you are trying to solve?

Jan has stated that ResourceFactory.split should NEVER mount.
I'm saying that's not 100% possible.
There are several scenarios (URIs on input that are not jar:file:// or file://) where it will mount, as there are inputs that have to use MountedPathResource for the Resource to even exist (see above mentioned graalvm behavior).

If Jan wants a utility method that never uses MountedPathResource, and wants to iterate through the results on her own, then do it via the URIs not the Resource.

Also, keep in mind that ResourceFactory.split() is not a static method. You must have a valid ResourceFactory instance to even call it, so you are in essence forced to use it with a valid ResourceFactory (such as one from a WebAppContext). So if you use it correctly, it will clean up after itself in the way you established the ResourceFactory.

These are the current usages of ResourceFactory.split() - (not an exhaustive list, just a quick search in the codebase)

MetaInfConfiguration.findAndFilterContainerPaths

will split on System.getProperty("java.class.path")

using File.pathSeparator and unwrap=true

Code will iterate through results.

This use case rarely has URIs as it uses java.class.path as input (I need to check if things like graalvm with scheme resource://, or jlink with resource jrt://, expose their own filesystem stuff in "java.class.path")

CoreAppContext.setExtraClassPath(String)
WebAppContext.setExtraClassPath(String)

will split input on ",;|" (this is a bug in some code bases)

Code will call setExtraClassPath(List<Resource> resources) with results of ResourceFactory.split()

This use case can have URIs on the input (and is a usecase seen in graalvm).

WebAppPropertyConverter.fromProperties

will split on File.pathSeparator, unwrap=true

Code will call WebAppContext.setExtraClassPath(String)

This is a bug, it should be calling WebAppContext.setExtraClassPath(String) and let a single implementation handle it.

The above mentioned bug also means that WebAppPropertyConverter cannot handle URIs in the input on Linux.

@joakime We've been down this road a few times... if utility methods that may be called during configuration mount, then we leak resources. Every time we've done this, we've been bitten and had to reverse out the changes. We cannot mount resources until we have an appropriate lifecycle managed resource factory that will handle the unmount.

Utility methods cannot mount because they cannot take responsibility for unmounting.

This is moot for ResourceFactory.split() as it is not a static utility method, but rather a method on an instance of ResourceFactory. A ResourceFactory which you should have created with the correct lifecycle before you called the split method.

Example: For WebAppContext.setExtraClassPath(String) use of ResourceFactory.split() it's the ResourceFactory belonging to the WebAppContext.

joakime · 2025-10-06T21:26:59Z

@janbartel should ResourceFactory.split() collapse duplicates?

Eg: if we have 3 entries ...

"/foo/bar.jar;/foo/zed.jar;/foo/bar.jar"

Should the second entry for "/foo/bar.jar" be ignored as it's already present?

What about equivalent entries?

"/foo/bar.jar;file:///foo/bar.jar"

Both of those point to the same URI.

janbartel · 2025-10-06T22:00:38Z

@janbartel should ResourceFactory.split() collapse duplicates?

Eg: if we have 3 entries ...
"/foo/bar.jar;/foo/zed.jar;/foo/bar.jar"
Should the second entry for "/foo/bar.jar" be ignored as it's already present?

What about equivalent entries?
"/foo/bar.jar;file:///foo/bar.jar"
Both of those point to the same URI.

No, I don't think its the job of ResourceFactory to second guess what the caller wants to do with the list that is passed in.

janbartel · 2025-10-07T05:39:12Z

@joakime I think we are slightly talking at cross purposes here. unwrap determines whether or not a jar:file: url will be reduced to file: before it is converted into a resource. If indeed the caller passed jar:file:/some/path!/ into split() but with unwrap=false, then yes, I guess the newResource() method will wind up mounting it. But that is for the caller to decide - as long as we have a way for the caller to control whether or not a mount occurs, then I'm fine. However, you seem to be arguing that split() should mount, which just cannot work.

If there are some schemes that we don't explicitly handle (BTW URIUtil.unwrapContainer() will ignore all non jar:schemes and just return the original uri so it can be called without first checking the uri starts with jar:) then they will be passed through to ResourceFactory.newResource() and I guess it depends on the implementation of that method as to whether or not a mount would occur. Looking at our impl for the resource: scheme, it looks like we do have some special handling for that scheme, and we would indeed do a mount and it would be leaked. That seems off, I think we should revisit our code for resource: handling.

joakime requested a review from janbartel October 2, 2025 16:22

joakime self-assigned this Oct 2, 2025

joakime added Bug For general bugs on Jetty side Microsoft Windows For issues unique to Microsoft Windows labels Oct 2, 2025

joakime added this to Jetty 12.1.3 Oct 2, 2025

joakime marked this pull request as draft October 2, 2025 16:43

joakime force-pushed the fix/12.1.x/13588/resourcefactory-split-absolute-paths branch from a8e5ad3 to f87847e Compare October 2, 2025 17:56

joakime moved this to 👀 In review in Jetty 12.1.3 Oct 2, 2025

joakime marked this pull request as ready for review October 2, 2025 19:51

Testing improvements

a404f65

joakime linked an issue Oct 2, 2025 that may be closed by this pull request

CONTAINER_JAR_PATTERN ignored on Windows #13588

Open

janbartel requested changes Oct 2, 2025

View reviewed changes

Changes from review

e3a4763

joakime requested a review from janbartel October 3, 2025 00:33

janbartel requested changes Oct 6, 2025

View reviewed changes

Adding requested test input

2025775

joakime requested a review from janbartel October 7, 2025 02:55

Issue #13588 - Improve ResourceFactory.split() and URIUtil.toURI to work properly on Windows. #13652

Are you sure you want to change the base?

Issue #13588 - Improve ResourceFactory.split() and URIUtil.toURI to work properly on Windows. #13652

Conversation

joakime commented Oct 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joakime Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joakime Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joakime commented Oct 6, 2025

Uh oh!

janbartel commented Oct 6, 2025

Uh oh!

janbartel commented Oct 7, 2025

Uh oh!

Uh oh!

joakime Oct 3, 2025 •

edited

Loading

joakime Oct 3, 2025 •

edited

Loading