Fix `PathMatchingResourcePatternResolver` manifest classpath discovery #33705

philwebb · 2024-10-15T05:31:23Z

Update PathMatchingResourcePatternResolver so that in addition to searching the java.class.path system property for classpath enties, it also searches the MANIFEST.MF files from within those jars.

Prior to this commit, the addClassPathManifestEntries() method expected that the JVM had added Class-Path manifest entries to the java.class.path system property, however, this did not always happen.

The updated code now performs a deep search by loading MANIFEST.MF files from jars discovered from the system property. To deal with potential performance issue, loaded results are also now cached.

The updated code has been tested with Spring Boot 3.3 jars extracted using java -Djarmode=tools.

Update `PathMatchingResourcePatternResolver` so that in addition to searching the `java.class.path` system property for classpath enties, it also searches the `MANIFEST.MF` files from within those jars. Prior to this commit, the `addClassPathManifestEntries()` method expected that the JVM had added `Class-Path` manifest entries to the `java.class.path` system property, however, this did not always happen. The updated code now performs a deep search by loading `MANIFEST.MF` files from jars discovered from the system property. To deal with potential performance issue, loaded results are also now cached. The updated code has been tested with Spring Boot 3.3 jars extracted using `java -Djarmode=tools`. See spring-projectsgh-33705

philwebb · 2024-10-15T05:31:58Z

See Classpath resources not found using layer-tools spring-boot#42591 for background

jhoeller · 2024-10-21T17:22:32Z

Digesting this PR and reviewing occurrences of the problem elsewhere, the topic seems rather involved. For example, classgraph/classgraph#20 (comment) claims that such manifest entries are actually meant to be URLs, not file paths. The approach taken there seems similar to ours in terms of manifest parsing, so I suppose we can proceed with this PR but we should review our assumptions in terms of file path parsing, accepting URLs as well?

Overall, @philwebb - is there a particular driver for submitting this PR for the 6.1.x branch? Such a signifcant refactoring in such a subtle core area seems better off in 6.2 for me, fixing the problem for Boot 3.4 rather than 3.3.x. If that's acceptable, could you please review the definition of manifest entries (URLs vs file paths) from your perspective and revisit the PR accordingly - and while in the process of this, rebase it onto main? I can take over from there then, bringing it into 6.2 GA for November.

philwebb · 2024-10-21T19:44:49Z

is there a particular driver for submitting this PR for the 6.1.x branch?

Nothing other than I considered it a bug so I thought an earlier branch might be better. I'll rebase it onto main.

manifest entries are actually meant to be URLs, not file paths

I think this code is the current source of truth and it does look a little more involved than my PR. I will try to align our logic with theirs.

Update `PathMatchingResourcePatternResolver` so that in addition to searching the `java.class.path` system property for classpath enties, it also searches the `MANIFEST.MF` files from within those jars. Prior to this commit, the `addClassPathManifestEntries()` method expected that the JVM had added `Class-Path` manifest entries to the `java.class.path` system property, however, this did not always happen. The updated code now performs a deep search by loading `MANIFEST.MF` files from jars discovered from the system property. To deal with potential performance issue, loaded results are also now cached. The updated code has been tested with Spring Boot 3.3 jars extracted using `java -Djarmode=tools`. See spring-projectsgh-33705

philwebb · 2024-10-21T22:02:00Z

@jhoeller Rebased to main and forced pushed. I've aligned with the JDK code as best I can, but it's unfortunately pretty hard to test this stuff.

juliojgd · 2024-10-22T14:33:28Z

...e/src/main/java/org/springframework/core/io/support/PathMatchingResourcePatternResolver.java

+				StringTokenizer tokenizer = new StringTokenizer(classPath);
+				while (tokenizer.hasMoreTokens()) {
+					String path = tokenizer.nextToken();
+					System.out.println("Hello "+path);


Maybe this System.out was an early debug line? (I'm just pointing it out in case it should be removed before the merge).

@philwebb did you mean to have a debug log statement there? Or do you prefer to simply drop this?

Sorry! This stuff is hard to debug because breakpoints don't work. It was an accident. Will fix.

Feel free to replace it with an actual debug log or simply drop it, as you see fit.

I dropped it and force pushed

Update `PathMatchingResourcePatternResolver` so that in addition to searching the `java.class.path` system property for classpath enties, it also searches the `MANIFEST.MF` files from within those jars. Prior to this commit, the `addClassPathManifestEntries()` method expected that the JVM had added `Class-Path` manifest entries to the `java.class.path` system property, however, this did not always happen. The updated code now performs a deep search by loading `MANIFEST.MF` files from jars discovered from the system property. To deal with potential performance issue, loaded results are also now cached. The updated code has been tested with Spring Boot 3.3 jars extracted using `java -Djarmode=tools`. See spring-projectsgh-33705

Includes consistent clearCache() behavior for manifest entries. Closes gh-33771 See gh-33705

juliojgd · 2024-10-23T10:16:57Z

@jhoeller Will this be backported to 6.1.x line?

Thanks in advance

philwebb · 2024-10-23T18:31:21Z

@juliojgd I don't think there are plans to backport due to the risk. See #33705 (comment)

philwebb force-pushed the ClassPathManifest branch from 110fee4 to 5e5d11b Compare October 15, 2024 05:31

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Oct 15, 2024

philwebb mentioned this pull request Oct 15, 2024

Classpath resources not found using layer-tools spring-projects/spring-boot#42591

Closed

bclozel added the in: core Issues in core modules (aop, beans, core, context, expression) label Oct 15, 2024

jhoeller added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Oct 16, 2024

jhoeller self-assigned this Oct 16, 2024

jhoeller added this to the 6.1.15 milestone Oct 16, 2024

philwebb force-pushed the ClassPathManifest branch from 5e5d11b to c78a213 Compare October 21, 2024 22:00

philwebb changed the base branch from 6.1.x to main October 21, 2024 22:02

jhoeller modified the milestones: 6.1.15, 6.2.0 Oct 22, 2024

juliojgd reviewed Oct 22, 2024

View reviewed changes

philwebb force-pushed the ClassPathManifest branch from c78a213 to fe3c1b9 Compare October 22, 2024 16:31

jhoeller merged commit 1c69a3c into spring-projects:main Oct 22, 2024
2 checks passed

jhoeller added a commit that referenced this pull request Oct 22, 2024

Consistently sort jar entries in doFindPathMatchingJarResources

081d0b3

Includes consistent clearCache() behavior for manifest entries. Closes gh-33771 See gh-33705

jhoeller mentioned this pull request Oct 23, 2024

PathMatchingResourcePatternResolver sorting of JAR resources #33771

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `PathMatchingResourcePatternResolver` manifest classpath discovery #33705

Fix `PathMatchingResourcePatternResolver` manifest classpath discovery #33705

philwebb commented Oct 15, 2024

philwebb commented Oct 15, 2024 •

edited by sbrannen

Loading

jhoeller commented Oct 21, 2024 •

edited

Loading

philwebb commented Oct 21, 2024

philwebb commented Oct 21, 2024

juliojgd Oct 22, 2024 •

edited

Loading

jhoeller Oct 22, 2024

philwebb Oct 22, 2024

jhoeller Oct 22, 2024

philwebb Oct 22, 2024

juliojgd commented Oct 23, 2024

philwebb commented Oct 23, 2024

Fix PathMatchingResourcePatternResolver manifest classpath discovery #33705

Fix PathMatchingResourcePatternResolver manifest classpath discovery #33705

Conversation

philwebb commented Oct 15, 2024

philwebb commented Oct 15, 2024 • edited by sbrannen Loading

jhoeller commented Oct 21, 2024 • edited Loading

philwebb commented Oct 21, 2024

philwebb commented Oct 21, 2024

juliojgd Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

jhoeller Oct 22, 2024

Choose a reason for hiding this comment

philwebb Oct 22, 2024

Choose a reason for hiding this comment

jhoeller Oct 22, 2024

Choose a reason for hiding this comment

philwebb Oct 22, 2024

Choose a reason for hiding this comment

juliojgd commented Oct 23, 2024

philwebb commented Oct 23, 2024

Fix `PathMatchingResourcePatternResolver` manifest classpath discovery #33705

Fix `PathMatchingResourcePatternResolver` manifest classpath discovery #33705

philwebb commented Oct 15, 2024 •

edited by sbrannen

Loading

jhoeller commented Oct 21, 2024 •

edited

Loading

juliojgd Oct 22, 2024 •

edited

Loading