jetty / jetty.project

Eclipse Jetty® - Web Container & Clients - supports HTTP/2, HTTP/1.1, HTTP/1.0, websocket, servlets, and more
https://eclipse.dev/jetty
Other
3.84k stars 1.91k forks source link

Make the core ResourceService a bit more reusable for subclasses #12281

Open garydgregory opened 1 week ago

garydgregory commented 1 week ago

Make the core ResourceService a bit more reusable for subclasses

garydgregory commented 1 week ago

CC: @lorban

lorban commented 1 week ago

Can you share an example of subclass you'd like to create? Feel free to modify ResourceService as you wish, we'll consider the best way to achieve your goal.

Thanks!

garydgregory commented 1 week ago

Here is what I am after and I can update this PR is the idea is acceptable.

The code I currently have is hard-coded to Jetty EE9 classes, but there is nothing really EE9 about it. I would prefer to configure Jetty using Core classes. I've recently refactored it to make it work with Jetty Core and EE9 without too much duplication, but I can't quite get there cleanly with the way Jetty Core classes are written today.

I have a class FilteredResourceService that extends org.eclipse.jetty.ee9.nested.ResourceService that you configure with a regular expression and a flag (the arguments to Pattern.compile(String, int)) such that when a directory is listed, only resources that match the regex are returned. This lets us "safely" serve Swagger files (like .*\.openapi|.*\.swagger), for example, without worrying that users can download whatever else might be laying around.

I do not want/cannot use a Jetty resource Factory because of the use case I'll outline later.

I believe the above could be folder into the Core ResourceService if the Core ResourceHandler adds a setter and/or constructor that takes a Core ResourceService. Right now I subclass ResourceService and override newResourceService() to provide a different ResourceService. This would let me configure a ResourceService with a ResourceService which itself defines a regex.

I am not suggesting the following is appropriate for Jetty but this is what we need: A subclass of EE9 FilteredResourceService that looks at each request and request content to perform string substitution on the contents of the files. This is done using Apache Commons Text's StringSubstitutor. The string replacement needs data from the request (for example the host name, and so on). The only to do what we need is to override org.eclipse.jetty.ee9.nested.ResourceService.sendData(HttpServletRequest, HttpServletResponse, boolean, HttpContent, Enumeration<String>) and do our business there. We basically build a new HttpContent and pass it to super.

The above all works as Jetty EE9 code. I think this would be better as Jetty Core code.

The scope of the PR would be to edit Jetty Core and Jetty EE9 such that each ResourceService can use a regex for directory listings.

joakime commented 1 week ago

I do not want/cannot use a Jetty resource Factory because of the use case I'll outline later.

You can wrap Resource, and that can provide the regex based filter in Resource.list() as well. No need to be messing with ResourceFactory.

joakime commented 1 week ago

The scope of the PR would be to edit Jetty Core and Jetty EE9 such that each ResourceService can use a regex for directory listings.

Having ResourceService support regex on directory listings seems like a natural evolution of the API.

But I think having it as ResourceService.setDirectoryListingPredicate(Predicate<Resource> listingPredicate) would be a more versatile approach.

You can do it via regex, someone else could do it via filenames, someone else could do it via java.nio.file.PathMatcher, etc ...

lorban commented 1 week ago

Opening up the internals of ResourceService as an API looks sketchy to me, so I agree with @joakime that adding a method to add a Predicate<Resource> filter that would be passed on to ResourceListing.getAsXHTML() would be more natural and versatile.

garydgregory commented 1 week ago

Thank you all for the suggestions. I'll update this PR over the next couple of days.