jetty.project
jetty.project copied to clipboard
jetty
Jetty 12 : eliminate `Resource.list()` and `Resource.getAllResources()` methods
Now that Resource is based on Path, we can eliminate the Resource.list() and Resource.getAllResources() methods.
This now puts the users of those methods in control of what they get/want from the Resource they are working with.
(eg: how deep of a walk. filter by type/id/name. ignore hidden. etc)
This also puts the user of the Resource to decide how they want to deal with the resulting list/stream of Paths.
Use them directly as Path, or use a ResourceFactory that they control to create new Resources from results of their usage.
The biggest user of these APIs prior to this PR was the AnnotationParser, which as of PR #8404 does not use these APIs anymore.
Note also that we are required to support the ServletContext#getResourcePaths API, which requires list semantics that will combine file system resources and potentially resources from within fragment jars.
This is the second time we have inadvertently removed support for this in jetty-12. I think we need and explicit test for this functionality so that we stop deleting it!
Something has gone wrong here! A directory Resource might not be based on Path. It may be a ResourceCollection or some maven aggregate structure or an future in memory mechanism.
As of today, we are required to list all of the combined resources within a resource collection, so I think we must have lost that functionality in the previous PRs as it cannot be assumed that a directory request resolves to a single Path
I took a look through the usages of Resource.list(), and here's what I found with the non-test usages.
org.eclipse.jetty.ee10.runner.Runner
There could be a ResourceCollection from the command line arguments
--lib=<entry> and --jar=<entry>, but that wasn't supported in any release of Runner.
The changes committed to this branch now supports that, and even has a bonus side effect of supporting globs.
ServletContextHandler.getResourcePaths(String) and ContextHandler.getResourcePaths(String)
This collected a Set<String> of entries consisting of <input-path>/<classified-filename>.
This would actually fail if one entry in the ResourceCollection didn't exist (the entire method would be skipped)
or if one entry wasn't a directory (this could actually crash the ResourceCollection.list() implementation)
The changes committed to this branch now supports a ResourceCollection that has empty/non-existent/non-directory entries, without creating throwaway Resource objects to just get a filename.
MetaInfConfiguration.findWebInfLibJars(WebAppContext context)
This should not ever support ResourceCollection as this would mean WebAppContext.getWebInf() is returning content that doesn't belong to the main artifact.
Such as WEB-INF/lib/foo.jar!/WEB-INF or META-INF/versions/#/WEB-INF and similar things.
I've added more TODOs to address this bug in WebAppContext.getWebInf()
That's it, that's all that's left of Resource.list() usage before this PR.
That's it, that's all that's left of
Resource.list()usage before this PR.
Exactly! that's my point. A usage of list from the ResourceService has been removed and not detected. We need to restore that mechanism and add a test in to be sure it is not accidentally deleted again.
That's it, that's all that's left of
Resource.list()usage before this PR.Exactly! that's my point. A usage of list from the ResourceService has been removed and not detected. We need to restore that mechanism and add a test in to be sure it is not accidentally deleted again.
I was originally confused as to where you got that ResourceService had Resource.list() usage ...
https://github.com/eclipse/jetty.project/blob/jetty-10.0.x/jetty-server/src/main/java/org/eclipse/jetty/server/ResourceService.java
There's no Resource.list() usage in Jetty 10.
The only thing remotely list() like in Jetty 10's ResourceService is the resource.getListHTML call, which would have used Resource.list() internally back in Jetty 10.
In Jetty 12, that's handled by the ResourceListing component in jetty-core/jetty-server.
Interestingly, the existing ResourceCollection tests that result in a Directory Index/Listing are passing.
I'll beef up the testing of ResourceListing directly, and add some ResourceCollection behaviors.
Commit e0a9c21 added tests showing that Resource.list() was still necessary. It reverted some of the changes to list as HTML to pass the tests. I'll soon do the same for getting resources from the context that has a resource collection.
So I'm going to close this PR for now, as its basic premise is not correct. I think there is some goodness/cleanups that are incidental in the branch, but I'll let @joakime harvest those into a new branch/PR.
Didn't have to close this, I can do the requested changes in this branch still.
Didn't have to close this, I can do the requested changes in this branch still.
What request changes?
This is looking like an omnibus PR with lots of unrelated changes with little motivation.
Can this be broken up into smaller motivated PRs
Lets start with PR #8471