Issue #10164 - improve `MetaInfConfiguration` use of `Resource` objects #10171

joakime · 2023-07-28T18:40:23Z

Replace the use of ResourceFactory.newResource(URI) with a simple Resource.resolve(String) where appropriate.

Remove unused methods.
Align ee10 and ee9 versions of MetaInfConfiguration behaviors with regards to Resource APIs.

lorban · 2023-07-31T08:35:30Z

...ee10/jetty-ee10-webapp/src/main/java/org/eclipse/jetty/ee10/webapp/MetaInfConfiguration.java

+            FileID.isJavaArchive(resource.getURI()) &&
+            !"jar".equals(resource.getURI().getScheme()))
+        {
+            return context.getResourceFactory().newJarFileResource(resource.getURI());


You're only using the context to get the ResourceFactory, so you should pass the ResourceFactory as a parameter instead of the context.

Why do we need to do this? Why can't we just trust that the Resource will be the right type of resource, and that it will open the jar when and if it needs to? Why does the user of the Resource suddenly have to be aware of the internal implementation of Resource and JarFileResource?

lorban · 2023-07-31T08:36:10Z

...ee10/jetty-ee10-webapp/src/main/java/org/eclipse/jetty/ee10/webapp/MetaInfConfiguration.java

@@ -767,6 +714,17 @@ protected List<Resource> findExtraClasspathDirs(WebAppContext context)
            .collect(Collectors.toList());
    }

+    private Resource ensureJarsAreOpened(WebAppContext context, Resource resource)


Not sure about this method's name. At the very least, it should have a singular name as it only deals with one resource.

lorban · 2023-07-31T08:36:49Z

.../jetty-ee10-webapp/src/test/java/org/eclipse/jetty/ee10/webapp/MetaInfConfigurationTest.java

+    @BeforeEach
+    public void beforeEach()
+    {
+        assertThat(FileSystemPool.INSTANCE.mounts(), empty());


👍
more of these == better

Is this static call thread safe?
As tests from other classes are running in parallel I remember having random failures with this when I implemented parallel tests.

@olamy Tests including this assertion cannot be parallelized. Is there a way to hint that with an annotation or something?

Class level annotation @Isolated from import org.junit.jupiter.api.parallel.Isolated

lorban · 2023-07-31T08:45:12Z

jetty-ee9/jetty-ee9-webapp/src/main/java/org/eclipse/jetty/ee9/webapp/MetaInfConfiguration.java

@@ -387,10 +361,11 @@ public void scanJars(final WebAppContext context, Collection<Resource> jars, boo
    public void scanForResources(WebAppContext context, Resource target, ConcurrentHashMap<Resource, Resource> cache)
    {
        // Resource target does not exist
-        if (target == null)
+        if (Resources.missing(target))


You should also check that target is a directory now that you ensure jars are opened, shouldn't you? And javadoc it?

lorban · 2023-07-31T08:46:24Z

jetty-ee9/jetty-ee9-webapp/src/main/java/org/eclipse/jetty/ee9/webapp/MetaInfConfiguration.java

@@ -450,7 +416,7 @@ else if (LOG.isDebugEnabled())

    private static boolean isEmptyResource(Resource resourcesDir)


This method should probably be inlined since it has an equivalent helper.

plus the name doesn't at all match what the method actually tests. It returns true if the resource is not a directory or is not readable... so how is that empty?

lorban · 2023-07-31T08:52:16Z

jetty-ee9/jetty-ee9-webapp/src/main/java/org/eclipse/jetty/ee9/webapp/MetaInfConfiguration.java

     * @return the list of tlds found
     * @throws IOException if unable to scan the directory
     */
-    public Collection<URL> getTlds(Path dir) throws IOException
+    private Collection<URL> getTlds(Resource res) throws IOException


The logic of this method changed: it doesn't walk the entire jar anymore but only lists the contents of META-INF. We should check the spec to make sure this is correct, and document this behavior in the javadoc.

lorban · 2023-07-31T08:54:23Z

jetty-ee9/jetty-ee9-webapp/src/main/java/org/eclipse/jetty/ee9/webapp/MetaInfConfiguration.java

-        if (Resources.isReadableDirectory(webInf))
-        {
-            Resource webInfLib = webInf.resolve("lib");
+        if (webInf == null || !webInf.exists() || !webInf.isDirectory())


I think you should keep the isReadableDirectory() check instead of exploding/slightly changing it.

lorban · 2023-07-31T08:55:09Z

jetty-ee9/jetty-ee9-webapp/src/main/java/org/eclipse/jetty/ee9/webapp/MetaInfConfiguration.java

@@ -777,6 +714,17 @@ protected List<Resource> findExtraClasspathDirs(WebAppContext context)
            .collect(Collectors.toList());
    }

+    private Resource ensureJarsAreOpened(WebAppContext context, Resource resource)


Ditto EE10: I don't like this name.

lorban · 2023-07-31T08:56:05Z

...ee10/jetty-ee10-webapp/src/main/java/org/eclipse/jetty/ee10/webapp/MetaInfConfiguration.java

     * @return the list of tlds found
     * @throws IOException if unable to scan the directory
     */
-    private Collection<URL> getTlds(Path dir) throws IOException
+    private Collection<URL> getTlds(Resource res) throws IOException


Ditto EE9: you changed the logic, so we should check if it's valid, and if so, change the javadoc accordingly.

lorban · 2023-07-31T08:57:56Z

...ee10/jetty-ee10-webapp/src/main/java/org/eclipse/jetty/ee10/webapp/MetaInfConfiguration.java

-                resourcesDir = resourceFactory.newResource(URIUtil.uriJarPrefix(uri, "!/META-INF/resources"));
-            }
+
+            resourcesDir = target.resolve("META-INF/resources");


This requires target to be a directory, this should be asserted and javadoc'ed.

You still need the if (target.isDirectory()) don't you?

janbartel

This PR seems to be now requiring more typing of Resource rather than less. Also, I thought the task here was to somehow obtain these META-INF resources/tlds/fragments from the classpath, rather than requiring us to explicitly open jars.

janbartel · 2023-07-31T14:15:46Z

...ee10/jetty-ee10-webapp/src/main/java/org/eclipse/jetty/ee10/webapp/MetaInfConfiguration.java

-                resourcesDir = resourceFactory.newResource(URIUtil.uriJarPrefix(uri, "!/META-INF/resources"));
-            }
+
+            resourcesDir = target.resolve("META-INF/resources");


You still need the if (target.isDirectory()) don't you?

janbartel · 2023-07-31T14:16:59Z

...ee10/jetty-ee10-webapp/src/main/java/org/eclipse/jetty/ee10/webapp/MetaInfConfiguration.java

-                webFrag = resourceFactory.newResource(URIUtil.uriJarPrefix(uri, "!/META-INF/web-fragment.xml"));
-            }
+
+            webFrag = jar.resolve("META-INF/web-fragment.xml");


As above, don't you need to test first if it is a dir or a jar?

janbartel · 2023-07-31T14:24:52Z

...ee10/jetty-ee10-webapp/src/main/java/org/eclipse/jetty/ee10/webapp/MetaInfConfiguration.java

+            FileID.isJavaArchive(resource.getURI()) &&
+            !"jar".equals(resource.getURI().getScheme()))
+        {
+            return context.getResourceFactory().newJarFileResource(resource.getURI());


Why do we need to do this? Why can't we just trust that the Resource will be the right type of resource, and that it will open the jar when and if it needs to? Why does the user of the Resource suddenly have to be aware of the internal implementation of Resource and JarFileResource?

janbartel · 2023-07-31T14:27:55Z

...-ee10-osgi-boot/src/main/java/org/eclipse/jetty/ee10/osgi/boot/OSGiMetaInfConfiguration.java

@@ -285,28 +285,33 @@ private List<Resource> getBundleAsResource(ResourceFactory resourceFactory, Bund
        File file = BundleFileLocatorHelperFactory.getFactory().getHelper().getBundleInstallLocation(bundle);
        if (file.isDirectory())
        {
+            // Add directory bundle as a directory


We shouldn't add the directory in which the bundles are stored as a Resource, we just want the individual bundles.

janbartel · 2023-07-31T14:28:54Z

...-ee10-osgi-boot/src/main/java/org/eclipse/jetty/ee10/osgi/boot/OSGiMetaInfConfiguration.java

                        }
                    }
                }
            }
-            resources.add(resourceFactory.newResource(file.toPath())); //TODO really???


Yeah, I doubt we need the directory that is the bundle installation directory as a Resource.

janbartel · 2023-07-31T14:29:22Z

...-ee10-osgi-boot/src/main/java/org/eclipse/jetty/ee10/osgi/boot/OSGiMetaInfConfiguration.java

        }
        else
        {
-            resources.add(resourceFactory.newResource(file.toPath()));
+            // Treat bundle as jar file that needs to be opened (so that resources within it can be found)
+            resources.add(resourceFactory.newJarFileResource(file.toPath().toUri()));


Why are we explicitly having to use JarFileResource?

janbartel · 2023-07-31T14:30:29Z

...y-ee10-webapp/src/test/java/org/eclipse/jetty/ee10/webapp/WebAppDefaultServletCacheTest.java

@@ -34,7 +34,7 @@ public class WebAppDefaultServletCacheTest
    private Path resourcePath;

    @BeforeEach
-    protected void setUp() throws Exception


Is this change directly relevant to this PR?

I've moved this to PR #10180, once that's merged, this one will go away here.

janbartel · 2023-07-31T14:30:39Z

...y-ee10-webapp/src/test/java/org/eclipse/jetty/ee10/webapp/WebAppDefaultServletCacheTest.java

@@ -48,7 +48,7 @@ protected void setUp() throws Exception
    }

    @AfterEach
-    protected void tearDown() throws Exception
+    public void tearDown() throws Exception


See previous comment.

I've moved this to PR #10180 (same as above), once that's merged, this one will go away here.

jetty-ee10/jetty-ee10-webapp/src/test/java/org/eclipse/jetty/ee10/webapp/WebAppContextTest.java

jetty-ee9/jetty-ee9-webapp/src/test/java/org/eclipse/jetty/ee9/webapp/OrderingTest.java

…ss-metainf-refcounts

gregw

I find the structure of much of the existing code difficult to understand... and whilst this PR is probably going in the right direction, I think it makes it even more difficult to understand.

findAndFilterContainerPaths is a good example (at being bad):

The addContainerResource variable does nothing for clarity, as it puts action before the iteration. Just inlining this variable would help with clarity, but as we are streaming anyway, then I think we'd do better to split up that consumer into individual maps and filters and embrace the stream to clearly show the steps taken to process the list

The code should be something like:

  getAllContainerJars(context).stream()
      .filter(new UriPatternPredicate(pattern, false))
      .map(resourceFactory::newResource)
      .filter(Objects::nonNull)
      .filter(Resource::exists)
      .map(MetaInfConfiguration::toDirectory) // if jar open, otherwise leave as is
      .filter(Resource::isDirectory) // probably not needed?
      .forEach(context.getMetaData()::addContentResource);

Edit: the toDirectory method is rename of ensureJarsAreOpened

…ss-metainf-refcounts

joakime · 2023-07-31T21:53:22Z

To address some of the questions from @janbartel ...

A Jar file is different than an open Jar.

A Resource pointing to the file file://path/to/foo.jar is different than one where we are walking the contents of it via jar:file://path/to/foo.jar!/
This distinction has been present since the beginning.
While the majority of cases in code we need the open Jar, we do occasionally need the Jar file (eg: when we copy the WEB-INF/lib/foo.jar to ${webapp.tempdir}/foo.jar to support META-INF/resources)

Then there's the nuance of where we can be deep in a WAR (jar:file://opt/mybase/webapps/foo.war!/WEB-INF/lib/foo.jar) which is a Jar file reference inside of a war.

The purpose of this PR is to reduce the constant ResourceFactory.newResource() usages in favor of a simple Resource.resolve(String) where appropriate.
That means, in code, we work with open Jar files.
We don't have code that supported auto-opening of JAR files, but that can be a solution too. (I'll experiment, probably in a different branch).

This PR is intended to be a first-pass to cleanup our ResourceFactory usages in a sane way (so we have reference counting, and the dump is more sane too).
A future PR will cleanup our open Resource's too, if the WebApp Configuration is done and the Resource does not need to be open for the running of the webapp, then close it before starting the webapp.

janbartel · 2023-08-01T07:07:41Z

I think If you can rename the 'ensureOpenJars' method to something that indicates what it is really doing, namely creating a directory resource, then things will be a lot clearer.

gregw · 2023-08-01T13:38:33Z

I think If you can rename the 'ensureOpenJars' method to something that indicates what it is really doing, namely creating a directory resource, then things will be a lot clearer.

I think toDirectory works if it is used with a stream map, as you map to directory (if already a directory then return it, if it can be mounted as a directory mount it, otherwise return null). In fact could probably simplify the suggestion above to:

  getAllContainerJars(context).stream()
      .filter(new UriPatternPredicate(pattern, false))
      .map(resourceFactory::newResource)
      .map(MetaInfConfiguration::toDirectory)
      .filter(Objects::nonNull)
      .forEach(context.getMetaData()::addContentResource);

gregw · 2023-08-01T13:42:29Z

The purpose of this PR is to reduce the constant ResourceFactory.newResource() usages in favor of a simple Resource.resolve(String) where appropriate.

That is a good goal and it is what this PR does.... but it took Jan and me half the width of Italy on a very fast train to work it out. I think the problem is, start with a confusing spec, implement in java 1.5, let go stale, then partially update to streams, then change resource abstraction a few times, then add a poorly named method in this PR, and it is all very confusing.

So I think we both like what this PR is doing, but perhaps not where it is doing it. Hence my suggestion to clean up the whole class and fully embrace the stream approach so that each phase of the stream well documents the transformations needed.

…ss-metainf-refcounts

joakime · 2023-08-03T19:45:36Z

I think If you can rename the 'ensureOpenJars' method to something that indicates what it is really doing, namely creating a directory resource, then things will be a lot clearer.

I think toDirectory works if it is used with a stream map, as you map to directory (if already a directory then return it, if it can be mounted as a directory mount it, otherwise return null). In fact could probably simplify the suggestion above to:
  getAllContainerJars(context).stream()
      .filter(new UriPatternPredicate(pattern, false))
      .map(resourceFactory::newResource)
      .map(MetaInfConfiguration::toDirectory)
      .filter(Objects::nonNull)
      .forEach(context.getMetaData()::addContentResource);

I like it, and I'm working towards this approach (in a different branch ATM)

Issue #10164 - improve MetaInfConfiguration use of Resource

a3e1384

joakime added the Enhancement label Jul 28, 2023

joakime requested review from janbartel and lorban July 28, 2023 18:40

joakime self-assigned this Jul 28, 2023

joakime mentioned this pull request Jul 28, 2023

Needless META-INF/resources and web-fragment.xml mounts #10164

Closed

joakime linked an issue Jul 28, 2023 that may be closed by this pull request

Needless META-INF/resources and web-fragment.xml mounts #10164

Closed

Remove unused method

cc67dc5

joakime marked this pull request as draft July 28, 2023 21:21

Simplify getTlds() - fixes JstlTest

2acb6a7

joakime marked this pull request as ready for review July 28, 2023 21:56

joakime added 4 commits July 29, 2023 12:46

Make WorkDir public (junit requirement)

d958a1b

Make test case public (junit requirement)

3d36534

Make @beforeeach / @AfterEach methods public (junit requirement)

8f9a9e2

Make assertions valid for always opened JARs

e2e7f6e

joakime marked this pull request as draft July 29, 2023 17:49

joakime added 2 commits July 29, 2023 13:18

Properly close ResourceFactory in test case

7da62f5

Update OSGiMetaInfConfiguration too also open bundles for later scanning

5e7d74d

lorban requested changes Jul 31, 2023

View reviewed changes

janbartel requested changes Jul 31, 2023

View reviewed changes

Merge remote-tracking branch 'origin/jetty-12.0.x' into fix/12.0.x/le…

0904fe9

…ss-metainf-refcounts

gregw requested changes Jul 31, 2023

View reviewed changes

joakime added 2 commits July 31, 2023 15:29

Resource.equals(Resource) replacement

c3f4fc6

Merge remote-tracking branch 'origin/jetty-12.0.x' into fix/12.0.x/le…

6351f5a

…ss-metainf-refcounts

joakime added 2 commits August 1, 2023 15:37

Merge remote-tracking branch 'origin/jetty-12.0.x' into fix/12.0.x/le…

520c282

…ss-metainf-refcounts

Ensure JPMS and TLDs can coexist happily

e4cc4b4

joakime added 2 commits August 3, 2023 09:37

Merge remote-tracking branch 'origin/jetty-12.0.x' into fix/12.0.x/le…

c9d139b

…ss-metainf-refcounts

Better method naming per reviews

5b0e9b6

joakime closed this Oct 11, 2023

joakime deleted the fix/12.0.x/less-metainf-refcounts branch October 25, 2023 18:02

joakime mentioned this pull request Oct 31, 2023

Issue #10164 - MetaInfConfiguration Overhaul #10816

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #10164 - improve `MetaInfConfiguration` use of `Resource` objects #10171

Issue #10164 - improve `MetaInfConfiguration` use of `Resource` objects #10171

joakime commented Jul 28, 2023

lorban Jul 31, 2023

janbartel Jul 31, 2023

lorban Jul 31, 2023

lorban Jul 31, 2023

olamy Jul 31, 2023

lorban Jul 31, 2023

olamy Jul 31, 2023

lorban Jul 31, 2023

lorban Jul 31, 2023

gregw Jul 31, 2023

lorban Jul 31, 2023

lorban Jul 31, 2023

lorban Jul 31, 2023

lorban Jul 31, 2023

lorban Jul 31, 2023

janbartel Jul 31, 2023

janbartel left a comment

janbartel Jul 31, 2023

janbartel Jul 31, 2023

janbartel Jul 31, 2023

janbartel Jul 31, 2023

janbartel Jul 31, 2023

janbartel Jul 31, 2023

janbartel Jul 31, 2023

joakime Jul 31, 2023

janbartel Jul 31, 2023

joakime Jul 31, 2023

gregw left a comment •

edited

Loading

joakime commented Jul 31, 2023 •

edited

Loading

janbartel commented Aug 1, 2023

gregw commented Aug 1, 2023

gregw commented Aug 1, 2023

joakime commented Aug 3, 2023

		@@ -450,7 +416,7 @@ else if (LOG.isDebugEnabled())

		private static boolean isEmptyResource(Resource resourcesDir)

Issue #10164 - improve MetaInfConfiguration use of Resource objects #10171

Issue #10164 - improve MetaInfConfiguration use of Resource objects #10171

Conversation

joakime commented Jul 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janbartel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregw left a comment • edited Loading

Choose a reason for hiding this comment

joakime commented Jul 31, 2023 • edited Loading

janbartel commented Aug 1, 2023

gregw commented Aug 1, 2023

gregw commented Aug 1, 2023

joakime commented Aug 3, 2023

Issue #10164 - improve `MetaInfConfiguration` use of `Resource` objects #10171

Issue #10164 - improve `MetaInfConfiguration` use of `Resource` objects #10171

gregw left a comment •

edited

Loading

joakime commented Jul 31, 2023 •

edited

Loading