Fixes #5521 ResourceCollection NPE #5527

gregw · 2020-10-28T14:43:50Z

Fix constructor and addPath so that all resources in a RC must exist when created for #5521

Fix constructor and addPath so that all resources in a RC must exist when created.

gregw · 2020-10-28T14:58:24Z

@joakime So the approach I've taken is to make all methods check that all resources exist - rather than just some.
However, since a resource can always be deleted after the collection is created, then perhaps the other approach we can take is to always allow non existent resources in the collection.

I just don't think we should have some that allow and some that don't.

Thoughts?

joakime

Just a question.

joakime · 2020-10-28T15:15:20Z

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

+        if (resources == null)
+        {
+            if (addedResource != null)
+                return addedResource; // This will not exist


Is it possible to get here if there are no resources in the RC?
I thought we prevented that in the constructors.

perhaps not... but I think the EmptyResource is a good safety net in case anything changes.

Although I'm now thinking more fondly of allowing a RC to have directories that don't exist... as we can't prevent them from being deleted anyway.

The addPath method IMHO is problematic:

the javadoc is wrong, it does NOT only return the first contained resource, it can return a ResourceCollection of directories

what is returned is inconsistent: you either get back a single directory that does not exist, or a ResourceCollection of directories that do

you get different results depending on the implementation of addPath by the various Resource subclasses: for example, PathResource will throw MalformedURLException if you give it a path that tries to escape above the root, but URLResource will just return you the original path

I agree the javadoc could be improved, but I don't see the problem with the variable return, as that is just what all the different scenarioes that do exist. Specifically if the result is

a file that exists in at least one of the collection, then the first one found is returned.

a subdirectory that exists in at exactly one of the collection, then that directory is returned

a subdirectory that exists in at more than one of the collection, then a resource collection is returned

a resource that doesn't exist in any of the collection, then the non existent resource of the first collection is returned.

if any of the collection is asked to form a resource that it cannot handle, then MalformedURLException is thrown

If I updated the javadoc to say that, then would that be OK?

a file that exists in at least one of the collection, then the first one found is returned.

Makes sense

a subdirectory that exists in at exactly one of the collection, then that directory is returned

Yup, agreed.

a subdirectory that exists in at more than one of the collection, then a resource collection is returned

This makes sense to me, a sub-collection.
But what if the asked for path has hits for both for files and directories (of the same addPath) across the RC?
What does that mean?

Example:

The file /tmp/foo/zed exists
The directory /tmp/bar/zed/ exists
RC of [/tmp/foo, /tmp/bar] is created.
RC.addPath("zed") means what?

Is this case 1?

a resource that doesn't exist in any of the collection, then the non existent resource of the first collection is returned.

This is a tough one.
I could argue that the return on this should be another ResourceCollection (of the same size) with each entry being the original RC with the non-existent path tacked on.

But what is the value on either approach?

In both approaches, a user of the Resource would test for exists() on the result and then move on.
But in the second approach, we allow for "potential" resources.

But that only works for directories IMO.
Having potential files across multiple hits would just be extra confusing.

Yeah, I'm not sure there is a perfect solution here.

if any of the collection is asked to form a resource that it cannot handle, then MalformedURLException is thrown

Yup, this one makes sense too.

@joakime and @janbartel we can only merge directories, we cannot merge files, nor can we merge files with directories nor directories with errors or files with errors.

ResourceCollection is implementing defacto standard directory overlays as done by things like docker and fancy file systems. If you mount layer A on top of layer B, then files in A replace files in B; if you list files then you see the union of files in A & B; if you create a new file it is created in A.

I believe the semantics we have implemented does that.

... and also we are just fixing NPE here, not re-inventing this class :)

The questions still stand.
Even if they are not relevant for this PR, they could lead to filing a new issue and PR.

If you are going to fix the javadoc here, then the javadoc needs to be clear.
So far, your first proposal for the javadoc changes has vague parts (hence the questions).
IMO, If there are known warts, then the javadoc should point out those warts.

a file that exists in at least one of the collection, then the first one found is returned.

OK.

a subdirectory that exists in at exactly one of the collection, then that directory is returned

OK.

a subdirectory that exists in at more than one of the collection, then a resource collection is returned

OK (so long as it is javadoc'ed).

a resource that doesn't exist in any of the collection, then the non existent resource of the first collection is returned.

? What do you mean "the non-existent resource of the first collection"??? I'm not in favour of returning something that is known not to exist. The caller should be able to expect consistency: either this method returns something that exists, or it throws an exception.

if any of the collection is asked to form a resource that it cannot handle, then MalformedURLException is thrown

OK.

@janbartel We have always been able to get resources that do not exist. In fact that is how we can create new resources eg when implementing PUT methods.

I'll update the javadoc.... but remember we have a real NPE here that we need to fix for a 10.0.0 before we go redesigning the entire resource contract.

Cleanup the vestiges of non existent directories detected by resource ending in /

…5521-ResourceCollection-NPE

Revert adding paths ending in / as jar:file resource needs them

janbartel

I think addPath needs work. Happy to discuss.

janbartel · 2020-11-03T10:46:26Z

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

+        if (resources == null)
+        {
+            if (addedResource != null)
+                return addedResource; // This will not exist


The addPath method IMHO is problematic:

the javadoc is wrong, it does NOT only return the first contained resource, it can return a ResourceCollection of directories

what is returned is inconsistent: you either get back a single directory that does not exist, or a ResourceCollection of directories that do

you get different results depending on the implementation of addPath by the various Resource subclasses: for example, PathResource will throw MalformedURLException if you give it a path that tries to escape above the root, but URLResource will just return you the original path

improved javadoc.

Fixes #5521 ResourceCollection NPE

a1e8375

Fix constructor and addPath so that all resources in a RC must exist when created.

gregw requested review from janbartel and joakime October 28, 2020 14:43

janbartel mentioned this pull request Oct 28, 2020

ClassCastException in Jetty #5525

Closed

joakime reviewed Oct 28, 2020

View reviewed changes

gregw added 3 commits October 28, 2020 17:12

Fixes #5521 ResourceCollection NPE

6066574

Cleanup the vestiges of non existent directories detected by resource ending in /

Merge remote-tracking branch 'origin/jetty-10.0.x' into jetty-10.0.x-…

288340a

…5521-ResourceCollection-NPE

Fixes #5521 ResourceCollection NPE

ae4f9f6

Revert adding paths ending in / as jar:file resource needs them

gregw linked an issue Oct 29, 2020 that may be closed by this pull request

ResourceCollection NPE in list() #5521

Closed

gregw requested a review from joakime October 29, 2020 08:34

janbartel requested changes Nov 3, 2020

View reviewed changes

gregw requested a review from janbartel November 3, 2020 14:07

feedback from review

1acef42

improved javadoc.

janbartel approved these changes Nov 3, 2020

View reviewed changes

joakime approved these changes Nov 3, 2020

View reviewed changes

joakime force-pushed the jetty-10.0.x branch from ef12d8a to 4fd75a0 Compare November 3, 2020 20:00

gregw merged commit 1904d32 into jetty-10.0.x Nov 4, 2020

gregw deleted the jetty-10.0.x-5521-ResourceCollection-NPE branch November 4, 2020 07:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #5521 ResourceCollection NPE #5527

Fixes #5521 ResourceCollection NPE #5527

gregw commented Oct 28, 2020 •

edited

Loading

gregw commented Oct 28, 2020

joakime left a comment

joakime Oct 28, 2020

gregw Oct 28, 2020

janbartel Nov 3, 2020

gregw Nov 3, 2020

joakime Nov 3, 2020

gregw Nov 3, 2020

joakime Nov 3, 2020

janbartel Nov 3, 2020

gregw Nov 3, 2020

janbartel left a comment

janbartel Nov 3, 2020

Fixes #5521 ResourceCollection NPE #5527

Fixes #5521 ResourceCollection NPE #5527

Conversation

gregw commented Oct 28, 2020 • edited Loading

gregw commented Oct 28, 2020

joakime 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

janbartel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregw commented Oct 28, 2020 •

edited

Loading