Jetty 12 - Issue #8474 - return null on resolve() if resource doesn't exist #8597

joakime · 2022-09-16T16:55:53Z

makes Resource.resolve(String) return null if the resulting Resource would not exist.
makes ResourceFactory.newResource() (and similar methods) return null if the resulting Resource would not exist.

joakime · 2022-09-16T16:56:33Z

@sbordet here's the minimal change to have PathResource.resolve() behave the same as ResourceCollection.resolve().

gregw

This is a bit of an omnibus PR. There are too many changes in it that are unrelated to the premise (of a null return).

There is goodness in this PR, but it is too hard to review when there are 70 files changed with renames, reformatting, various fixes and cleanups.

Can you break this out into multiple PRs, so that ultimately there is a single PR with the null return change.

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

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

joakime · 2022-09-22T11:11:23Z

Thank you for the preliminary review.

For now, hold off on any further reviews until I can move some of the oddball changes out of this PR into separate PRs (as once those are merged, it will cause a rebase in this PR to clean out those changes)

joakime · 2022-09-22T13:04:38Z

The removal of exist() has not been done on this PR.

gregw · 2022-09-23T06:52:45Z

Thank you for the preliminary review.

For now, hold off on any further reviews until I can move some of the oddball changes out of this PR into separate PRs (as once those are merged, it will cause a rebase in this PR to clean out those changes)

For anything like renaming of variables / fields etc. I think you can probably do them without a PR if they are not part of the public API. Just check with another developer and if they are OK, push directly to the branch.

…experiment-resource-resolve-null-on-notexist

…jar:file: based URI)

…experiment-resource-resolve-null-on-notexist

…-existent Resource

joakime · 2022-09-28T20:46:34Z

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

@@ -252,30 +259,25 @@ public static Stream<Arguments> scenarios() throws Exception

    public WorkDir workDir;

+    @Disabled


Need to revisit this

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

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

joakime · 2022-09-29T00:21:15Z

From latest run https://jenkins.webtide.net/blue/organizations/jenkins/jetty.project/detail/PR-8597/10/tests

org.eclipse.jetty.tests.distribution.DemoModulesTests.testJspDump([1] ee8) Test Failure

java.lang.IllegalArgumentException: KeyStore Path does not exist: /home/jenkins/agent/workspace/jetty.project_PR-8597
  /tests/test-distribution/test-distribution-common/target/tests/test-bases/jetty_base_5828043306936187437/etc/test-keystore.p12

I wonder if this should be throwing, or not.
I'm leaning towards throwing. (and fixing the testcase to actually have a keystore)

@sbordet and @lorban what do you think?

gregw · 2022-09-29T23:11:29Z

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

@@ -126,8 +126,8 @@ public Resource resolve(String subUriPath)
        for (Resource res : _resources)
        {
            addedResource = res.resolve(subUriPath);
-            if (!addedResource.exists())
-                continue;
+            if (addedResource == null)


I think more changes below are needed for code clarity.

specifically if resources == null, can we just explicitly return null?

lorban · 2022-09-30T12:00:08Z

...-core/jetty-deploy/src/main/java/org/eclipse/jetty/deploy/providers/ScanningAppProvider.java

@@ -226,7 +226,7 @@ protected void doStart() throws Exception
        List<Path> files = new ArrayList<>();
        for (Resource resource : _monitored)
        {
-            if (resource.exists() && Files.isReadable(resource.getPath()))
+            if (resource != null && Files.isReadable(resource.getPath()))


We are not adding nulls into _monitored for non-existing resources, so this null check can go away.

lorban · 2022-09-30T12:01:32Z

...-core/jetty-deploy/src/main/java/org/eclipse/jetty/deploy/providers/ScanningAppProvider.java

+        for (Resource resource: resources)
+        {
+            if (resource != null)
+                _monitored.addAll(resources);


Why adding the resources collection for each resource?

lorban · 2022-09-30T12:17:09Z

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ContextHandler.java

+            return;
+        }
+
+        Resource resource = ResourceFactory.root().newResource(path);


The Path may require mounting, right? So this may lead to a mount leak if I'm not mistaken.

lorban · 2022-09-30T12:18:41Z

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

@@ -26,6 +28,14 @@
 */
 public class MountedPathResource extends PathResource
 {
+    public static MountedPathResource of(URI uri) throws IOException


Is public needed?

lorban · 2022-09-30T12:18:47Z

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

@@ -47,6 +47,14 @@ public class PathResource extends Resource
        .with("jrt")
        .build();

+    public static PathResource of(URI uri) throws IOException


Is public needed?

lorban · 2022-09-30T12:19:08Z

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

@@ -34,6 +44,18 @@ public class MountedPathResource extends PathResource
        containerUri = URIUtil.unwrapContainer(getURI());
    }

+    MountedPathResource(Path path, URI uri)


Is the other constructor still needed?

…experiment-resource-resolve-null-on-notexist

joakime · 2022-10-10T17:36:13Z

Closing this large (in commit count) PR in favor of #8702

joakime requested a review from sbordet September 16, 2022 16:56

gregw requested changes Sep 22, 2022

View reviewed changes

joakime added 5 commits September 22, 2022 06:01

Issue #8474 - return null on resolve() if resource doesn't exist

20f4b5c

In process change to resolve is null

bd29121

Fixing jetty-util expectations of resolve() returning null

8482ce3

Initial fixing of resolve(non-existent) returning null

74acb62

Resource.resolve is now abstract

7f86ec1

joakime force-pushed the fix/jetty-12-experiment-resource-resolve-null-on-notexist branch from b277dac to 7f86ec1 Compare September 22, 2022 11:02

joakime mentioned this pull request Sep 22, 2022

Jetty 12 : QuickStart generation based on Path, usage based on Resource #8614

Merged

joakime added the jetty 12 label Sep 23, 2022

joakime self-assigned this Sep 23, 2022

joakime added 11 commits September 23, 2022 12:54

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

d2e57e4

…experiment-resource-resolve-null-on-notexist

Rolling back changes that are now in other PRs

1812737

Fixing compilation

9ad61ec

More resolve() == null checks

0c05840

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

1a49f7e

…experiment-resource-resolve-null-on-notexist

Minor tweak to show full URI of resource in logging (helps if it's a …

cb795ab

…jar:file: based URI)

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

5b811f5

…experiment-resource-resolve-null-on-notexist

newResource(URI) can now return null if the URI would result in a non…

459ea92

…-existent Resource

More protections on blank/null inputs

72abb2a

Better error handling for non-existent keystore/truststore

7edb795

Don't add non-existent web fragments to cache

1f2b73d

Continued work on resolve() returns null on non-existent

1e43680

joakime commented Sep 28, 2022

View reviewed changes

Remove no longer used class

2335007

github-advanced-security bot found potential problems Sep 28, 2022

View reviewed changes

jetty-ee10/jetty-ee10-webapp/src/main/java/org/eclipse/jetty/ee10/webapp/ClassMatcher.java Fixed Show fixed Hide fixed

jetty-ee9/jetty-ee9-webapp/src/main/java/org/eclipse/jetty/ee9/webapp/ClassMatcher.java Fixed Show fixed Hide fixed

test-keystore should occur before ssl

fdba6ed

gregw reviewed Sep 29, 2022

View reviewed changes

lorban requested changes Sep 30, 2022

View reviewed changes

joakime added 6 commits October 5, 2022 08:12

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

c549a97

…experiment-resource-resolve-null-on-notexist

Fix CachedContentFactory

dedf53d

Fixing ServerListener

7369e11

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

ebe0ce4

…experiment-resource-resolve-null-on-notexist

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

4438232

…experiment-resource-resolve-null-on-notexist

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

ade5345

…experiment-resource-resolve-null-on-notexist

joakime marked this pull request as ready for review October 7, 2022 16:42

joakime changed the title ~~Jetty 12 - EXPERIMENT - Issue #8474 - return null on resolve() if resource doesn't exist~~ Jetty 12 - Issue #8474 - return null on resolve() if resource doesn't exist Oct 7, 2022

joakime requested review from lorban and gregw October 7, 2022 16:46

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

6fa6199

…experiment-resource-resolve-null-on-notexist

joakime mentioned this pull request Oct 10, 2022

Jetty 12 - Resource resolve() and newResource() return null on resources that do not exist #8702

Merged

joakime closed this Oct 10, 2022

joakime deleted the fix/jetty-12-experiment-resource-resolve-null-on-notexist branch October 27, 2022 18:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jetty 12 - Issue #8474 - return null on resolve() if resource doesn't exist #8597

Jetty 12 - Issue #8474 - return null on resolve() if resource doesn't exist #8597

joakime commented Sep 16, 2022 •

edited

Loading

joakime commented Sep 16, 2022

gregw left a comment

joakime commented Sep 22, 2022

joakime commented Sep 22, 2022

gregw commented Sep 23, 2022

joakime Sep 28, 2022

joakime commented Sep 29, 2022 •

edited

Loading

gregw Sep 29, 2022

lorban Sep 30, 2022

lorban Sep 30, 2022

lorban Sep 30, 2022

lorban Sep 30, 2022

lorban Sep 30, 2022

lorban Sep 30, 2022

joakime commented Oct 10, 2022

		@@ -252,30 +259,25 @@ public static Stream<Arguments> scenarios() throws Exception

		public WorkDir workDir;

		@Disabled

Jetty 12 - Issue #8474 - return null on resolve() if resource doesn't exist #8597

Jetty 12 - Issue #8474 - return null on resolve() if resource doesn't exist #8597

Conversation

joakime commented Sep 16, 2022 • edited Loading

joakime commented Sep 16, 2022

gregw left a comment

Choose a reason for hiding this comment

joakime commented Sep 22, 2022

joakime commented Sep 22, 2022

gregw commented Sep 23, 2022

Choose a reason for hiding this comment

joakime commented Sep 29, 2022 • edited Loading

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

joakime commented Oct 10, 2022

joakime commented Sep 16, 2022 •

edited

Loading

joakime commented Sep 29, 2022 •

edited

Loading