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.

Sign up for GitHub

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

Jump to bottom

Fixes #5521 ResourceCollection NPE #5527

Merged
merged 5 commits into from Nov 4, 2020
Merged

Fixes #5521 ResourceCollection NPE #5527

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a1e8375
Fixes #5521 ResourceCollection NPE
gregw Oct 28, 2020
6066574
Fixes #5521 ResourceCollection NPE
gregw Oct 28, 2020
288340a
Merge remote-tracking branch 'origin/jetty-10.0.x' into jetty-10.0.x-…
gregw Oct 28, 2020
ae4f9f6
Fixes #5521 ResourceCollection NPE
gregw Oct 28, 2020
1acef42
feedback from review
gregw Nov 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
47 changes: 24 additions & 23 deletions jetty-util/src/main/java/org/eclipse/jetty/util/resource/ResourceCollection.java
View file
Open in desktop
Expand Up @@ -62,8 +62,19 @@ public ResourceCollection()
* @param resources the resources to be added to collection
*/
public ResourceCollection(Resource... resources)
{
this(Arrays.asList(resources));
}

/**
* Instantiates a new resource collection.
*
* @param resources the resources to be added to collection
*/
public ResourceCollection(Collection<Resource> resources)
{
_resources = new ArrayList<>();

for (Resource r : resources)
{
if (r == null)
Expand All @@ -82,17 +93,6 @@ public ResourceCollection(Resource... resources)
}
}

/**
* Instantiates a new resource collection.
*
* @param resources the resources to be added to collection
*/
public ResourceCollection(Collection<Resource> resources)
{
_resources = new ArrayList<>();
_resources.addAll(resources);
}

/**
* Instantiates a new resource collection.
*
Expand Down Expand Up @@ -247,27 +247,28 @@ public Resource addPath(String path) throws IOException
ArrayList<Resource> resources = null;

// Attempt a simple (single) Resource lookup that exists
Resource addedResource = null;
for (Resource res : _resources)
{
Resource r = res.addPath(path);
if (!r.isDirectory() && r.exists())
{
// Return simple (non-directory) Resource
return r;
}

addedResource = res.addPath(path);
if (!addedResource.exists())
continue;
if (!addedResource.isDirectory())
return addedResource; // Return simple (non-directory) Resource
if (resources == null)
{
resources = new ArrayList<>();
}
resources.add(addedResource);
}

resources.add(r);
if (resources == null)
{
if (addedResource != null)
return addedResource; // This will not exist
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Makes sense

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

Yup, agreed.

  1. 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?

  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.

  1. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

return EmptyResource.INSTANCE;
}

if (resources.size() == 1)
{
return resources.get(0);
}

return new ResourceCollection(resources);
}
Expand Down
3 changes: 1 addition & 2 deletions jetty-util/src/test/java/org/eclipse/jetty/util/resource/ResourceCollectionTest.java
View file
Open in desktop
Expand Up @@ -35,7 +35,6 @@

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.contains;
import static org.hamcrest.Matchers.emptyArray;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.nullValue;
import static org.junit.jupiter.api.Assertions.assertEquals;
Expand Down Expand Up @@ -189,7 +188,7 @@ public void testList() throws Exception

assertThat(Arrays.asList(rc1.list()), contains("1.txt", "2.txt", "3.txt", "dir/"));
assertThat(Arrays.asList(rc1.addPath("dir").list()), contains("1.txt", "2.txt", "3.txt"));
assertThat(rc1.addPath("unknown").list(), emptyArray());
assertThat(rc1.addPath("unknown").list(), nullValue());
}

@Test
Expand Down