Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use java.nio and FileSystems to resolve files in PathMatchingResourcePatternResolver #29163

Closed
dsyer opened this issue Sep 16, 2022 · 6 comments
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Milestone

Comments

@dsyer
Copy link
Member

dsyer commented Sep 16, 2022

The JDK has decent support for walking a file system (via FileSystem in java.nio), but Spring still uses hand-rolled pre-Java 7 code. Also, in principle, FileSystem can be an abstraction used to express any hierarchy of resources through their URIs. This is a good fit with the Spring Resource abstraction, and for instance would mean that native images could potentially do classpath scanning through a custom URI and FileSystem already provided by GraalVM (oracle/graal#1108).

Here's a prototype that shows how it could work: https://github.com/scratches/pattern-resolver-poc. It replaces one method (and several subsidiary protected methods) in PathMatchingResourcePatternResolver. The unit test is a copy from spring-core verifying that it works as normal with files. It also works with classpath resources in native images, as demonstrated by the main() method provided.

Slightly ugly is the hack that works around the fact that GraalVM's FileSystem for resource: URIs doesn't accept leading or trailing slashes in folder names (the opposite of the file: implementation provided by the JDK, but similar to the way ClassLoader.getResource() works).

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Sep 16, 2022
@jhoeller jhoeller self-assigned this Sep 16, 2022
@jhoeller jhoeller added in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Sep 16, 2022
@jhoeller jhoeller added this to the 6.0.0-RC1 milestone Sep 16, 2022
@dsyer
Copy link
Member Author

dsyer commented Sep 16, 2022

Update: you can run the tests in the project above in a native image (./mvnw test -PnativeTest), but 1 of them had to be modified (there is no File associated with a classpath resource) and another disabled (you can't scan from the root of the classpath)

@bclozel
Copy link
Member

bclozel commented Sep 20, 2022

I've raised oracle/graal#5020 to address the lack of root resource resolution. I think this is a must have if we want to offer this feature in Framework. We don't need to wait for a fix in GraalVM though, as this issue would cover quite a lot of cases already.

@sbrannen sbrannen assigned sbrannen and unassigned jhoeller Sep 24, 2022
sbrannen added a commit that referenced this issue Sep 25, 2022
In preparation for gh-29163, this commit revamps
PathMatchingResourcePatternResolverTests as follows.

- organizes tests into @nested test classes

- reintroduces the @disabled classpathStarWithPatternOnFileSystem() test

- stops asserting the protocol of Resource URLs, since the protocol is
  dependent on the environment -- for example, file: and jar: URLs are
  actually resource: URLs within a GraalVM native image

- simplifies implementation of test methods and assertFilenames()
sbrannen pushed a commit to sbrannen/spring-framework that referenced this issue Sep 28, 2022
sbrannen added a commit to sbrannen/spring-framework that referenced this issue Sep 28, 2022
sbrannen added a commit to sbrannen/spring-framework that referenced this issue Sep 28, 2022
sbrannen added a commit to sbrannen/spring-framework that referenced this issue Sep 28, 2022
sbrannen added a commit to sbrannen/spring-framework that referenced this issue Sep 28, 2022
sbrannen added a commit to sbrannen/spring-framework that referenced this issue Sep 28, 2022
sbrannen added a commit to sbrannen/spring-framework that referenced this issue Sep 28, 2022
sbrannen pushed a commit to sbrannen/spring-framework that referenced this issue Sep 28, 2022
…PatternResolver

The JDK has decent support for walking a file system (via the
FileSystem API in java.nio.file), but prior to this commit Spring still
used hand-rolled pre-Java 7 code. Also, in principle, FileSystem can be
an abstraction used to express any hierarchy of resources through their
URIs. This is a good fit with the Spring Resource abstraction, and for
instance would mean that GraalVM native images could potentially do
classpath scanning through a custom URI and FileSystem already provided
by GraalVM.

In light of the above, this commit overhauls the implementation of
PathMatchingResourcePatternResolver to use a java.nio.file.FileSystem
to find matching resources within a file system.

As a side effect, several obsolete `protected` methods have been
removed from PathMatchingResourcePatternResolver.

See spring-projectsgh-29163
sbrannen added a commit to sbrannen/spring-framework that referenced this issue Sep 28, 2022
sbrannen added a commit to sbrannen/spring-framework that referenced this issue Sep 28, 2022
sbrannen added a commit to sbrannen/spring-framework that referenced this issue Sep 28, 2022
sbrannen added a commit to sbrannen/spring-framework that referenced this issue Sep 28, 2022
sbrannen added a commit to sbrannen/spring-framework that referenced this issue Sep 28, 2022
sbrannen added a commit to sbrannen/spring-framework that referenced this issue Sep 28, 2022
@sbrannen
Copy link
Member

Your work on this issue has been incorporated into the main branch in 0eb6678.

Thanks, @dsyer! 👏

@sbrannen
Copy link
Member

Update: you can run the tests in the project above in a native image (./mvnw test -PnativeTest),

To get PathMatchingResourcePatternResolverTests (current version from main) working, I ran the tests using the tracing agent (./mvnw clean -PnativeTest -Dagent=true test) and also had to create the following resource-config.json file.

{
	"resources": {
		"includes": [
			{
				"pattern": "^.+\\.dtd$"
			},
			{
				"pattern": "\\Qorg/springframework/core/io/\\E.*?\\/resource.+\\.txt$"
			},
			{
				"pattern": "\\Qorg/springframework/core/io/support/\\E.+\\.class$"
			},
			{
				"pattern": "\\Qreactor/util/annotation/NonNull.class\\E"
			},
			{
				"pattern": "\\Qreactor/util/annotation/NonNullApi.class\\E"
			},
			{
				"pattern": "\\Qreactor/util/annotation/Nullable.class\\E"
			}
		]
	}
}

but 1 of them had to be modified (there is no File associated with a classpath resource)

I addressed that in 29442d4.

and another disabled (you can't scan from the root of the classpath)

Indeed, rootPatternRetrievalInJarFiles() is the only remaining test failing within a native image, but that will be addressed in oracle/graal#5020.

@sbrannen
Copy link
Member

This has introduced a regression in that a matching folder is now returned in the results.

I am therefore reopening this issue to address that.

@sbrannen sbrannen reopened this Sep 30, 2022
sbrannen added a commit to sbrannen/spring-framework that referenced this issue Oct 7, 2022
Prior to this commit, if a FileSystemResource was created from a
java.nio.file.Path or java.nio.file.FileSystem, the URI returned from
getURI() was based on the semantics of Path#toUri() which always
includes the "authority component" (i.e., "//") after the scheme (e.g.,
"file://..."). In contrast, if a FileSystemResource is created from a
java.io.File or String, the URI returned from getURI() never includes
the "authority component" due to the semantics of File#toURI().

Consequently, invoking myFileSystemResource.getURI().toString() could
result in "file:/my/path" or "file:///my/path", depending on how the
FileSystemResource was created.

This behavior is not new; however, recent changes in
PathMatchingResourcePatternResolver -- which result in a
FileSystemResource being created from a java.nio.file.Path instead of
from a java.io.File -- make this difference more noticeable to users
and may in fact surface as a breaking change.

To address that issue, this commit revises the implementation of
FileSystemResource.getURI() by normalizing any `file:` URIs that it
returns. Effectively, URIs like "file:///my/path" will now take the
form "file:/my/path".

See spring-projectsgh-29163
Closes spring-projectsgh-29275
sbrannen added a commit to sbrannen/spring-framework that referenced this issue Aug 13, 2023
The recent switch to java.nio.file.FileSystem-based classpath scanning
(see spring-projectsgh-29163) resulted in a regression when scanning the classpath in
the Windows filesystem.

This commit addresses this regression by introducing several
workarounds for dealing with `file:` resources on Windows.

See the in-line comments in this commit for details on those
workarounds.

This fix has been tested in the following environments.

- macOS 12.6 / Java 17 Oracle (build 17.0.4.1+1-LTS-2)

- macOS 12.6 / GraalVM CE 22.2.0 (build 17.0.4+8-jvmci-22.2-b06)

- MS Windows 11 / Java 17 Microsoft (build 17.0.4.1+1-LTS)

Closes spring-projectsgh-29226
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

5 participants