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

Allow Advanced PathSpec optional behavior in ServletContextHandler addServlet and addFilter #5810

Closed
joakime opened this issue Dec 15, 2020 · 6 comments
Closed

Allow Advanced PathSpec optional behavior in ServletContextHandler addServlet and addFilter #5810

joakime opened this issue Dec 15, 2020 · 6 comments
Labels
Enhancement Sponsored This issue affects a user with a commercial support agreement

Comments

@joakime
Copy link
Contributor

joakime commented Dec 15, 2020

Jetty version
Jetty 9.4.x

Description
Currently the ServletHandler uses the PathMappings feature of Jetty internals.

And it (correctly) only adds ServletPathSpec in response to the web descriptor or embedded usage
of ServletContextHandler.addServlet() or ServletContextHandler.addFilter() usage.

This is a request to add the ability to add servlets or filters based on one of the more advanced PathSpec features
Such as RegexPathSpec or UriTemplatePathSpec.

Ideally supporting :

  • Set this from a ServletContextHandler or WebAppContext via initialized PathSpec object.
  • Set from web descriptor (or annotations) via string with special keywords to break out of the Servlet behaviors.
    • Example: @WebServlet("regex:/a/[^/]+/b/[^/]+") or <url-pattern>uri:/a/{foo}/b/{zed}</url-pattern>
@joakime joakime added Enhancement Sponsored This issue affects a user with a commercial support agreement labels Dec 15, 2020
@joakime joakime added this to To do in Jetty 9.4.36 via automation Dec 15, 2020
@gregw
Copy link
Contributor

gregw commented Dec 16, 2020

Two approaches:

  1. allows arbitrary types of path spec to be passed in as Strings. This would need less plumbing changes and would allow such patterns to come from more sources. But we'd need a robust way of converting from such an arbitrary string to a specific pathSpec type. Also would need optional validation so that we could enforce normal pathspecs only from webapps.
  2. add API that allows PathSpecs to be passed in as an instance rather than as a string. Thus the caller will set the type. Will need more changes to the plumbing and will only be available from the new API.

I think 2) is the best approach if it matches the requirements.

joakime added a commit that referenced this issue Jan 5, 2021
@joakime
Issue #5810 - Support bespoke PathSpec for servlets in embedded-jetty
725b858 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@janbartel
Copy link
Contributor

Late to the party here, but I just want to note that I am not in favour of putting any extended path syntax into web.xml or annotations.

@joakime
Copy link
Contributor Author

joakime commented Jan 12, 2021

Late to the party here, but I just want to note that I am not in favour of putting any extended path syntax into web.xml or annotations.

I agree with this.
IMO, Leaving this feature for only embedded-jetty users and WEB-INF/jetty-web.xml users are the only acceptable solutions.

gregw added a commit that referenced this issue Jan 13, 2021
@gregw
Refactor of fix for #5810
77fe316 
 + Created a common `Mapping` base for `FilterMapping` and `ServletMapping`
 + `Mapping` contains an array of PathSpec.  The `String` based methods assume `ServletPathSpec`.
 + Convert most handling of string pathSpecs to `PathSpec` instances
 + Modified `ConstraintMapping` to also use a PathSpec
 + Modified JSP and annotation handling to cope with any existing non standard path specs
 + Made ServletHandler update mappings respect the order of the mappings

 TODO:
 + Should `ConstraintMapping` extend `Mapping` ?
 + Convert `ConstraintSecurityHandler` to use `PathMappings` so it can handle non standard PathSpecs
 + Review remaining calls to `Mapping.getPathSpecs` and `ConstrainMapping.getPathSpec`
 + Currently Quick Start operates on just the string versions of PathSpec, as it must be able to write a web.xml which can only contain standard path specs.  Should it fail when it sees a non standard pathspec, or assume that it will again be programmatically added during quick start?
 + Review how frequently we are creating new arrays of path spec and/or converting path specs to/from strings.  Best resolution for such things are ultility methods on Mapping (eg `#containsPathSpec`)
 + More tests
gregw added a commit that referenced this issue Jan 13, 2021
@gregw
Refactor of fix for #5810
f4e4a10 
 + Converted `ConstraintSecurityHandler` to use `PathMappings` so it can handle non standard PathSpecs
gregw added a commit that referenced this issue Jan 13, 2021
@gregw
Refactor of fix for #5810
30906f1 
 + Fixed test by reverting ServletMapping normalize
gregw added a commit that referenced this issue Jan 13, 2021
@gregw
Refactor of fix for #5810
eb66945 
 + More tests
@gregw
Copy link
Contributor

gregw commented Jan 13, 2021

So I have changed the PR so that rather than an extra non-standard PathSpec plus a String[] of stnadard path specs, all mappings are now stored as PathSpec[], including constraints, filters and servlets.
The string based methods remain, but only work on ServletPathSpec. Using the PathSpec API is the only way to add arbitrary path spec types.

I've added some testing, but more is probably needed. Specially for the complex way security constraints are merged.

@gregw gregw removed this from To do in Jetty 9.4.36 Jan 13, 2021
@gregw
Copy link
Contributor

gregw commented Jul 10, 2021

@joakime is there still demand for this?

@gregw gregw mentioned this issue Jul 13, 2021
Closed
@joakime
Copy link
Contributor Author

joakime commented Apr 7, 2022

Closed in favor of the approach in #7748

@joakime joakime closed this as completed Apr 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Enhancement Sponsored This issue affects a user with a commercial support agreement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@gregw @joakime @janbartel