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.

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

Cleanup ServletHandler, specifically with respect to making filter chains more extensible #5022

Closed
joakime opened this issue Jul 3, 2020 · 2 comments
Assignees
Labels
Bug For general bugs on Jetty side Sponsored This issue affects a user with a commercial support agreement

Comments

@joakime
Copy link
Contributor

joakime commented Jul 3, 2020

Jetty version
9.4.30

Description
The ServletHandler has a method for overriding a CachedChain via the newCachedChain() method.

https://github.com/eclipse/jetty.project/blob/ae43b70a9f8907b8a2444f051bebab60f4a9162c/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHandler.java#L826-L836

But this cannot be used, as the CachedChain inner class is protected.

This method is only called from ServletHandler.getFilterChain().

https://github.com/eclipse/jetty.project/blob/ae43b70a9f8907b8a2444f051bebab60f4a9162c/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHandler.java#L636-L637

There is also a missing related method for non-cached that should be created in ServletHandler.

public Chain newChain(Request baseRequest, List<FilterHolder> filters, ServletHolder servletHolder)

Which is used in getFilterChain()

https://github.com/eclipse/jetty.project/blob/ae43b70a9f8907b8a2444f051bebab60f4a9162c/jetty-servlet/src/main/java/org/eclipse/jetty/servlet/ServletHandler.java#L660-L661

@joakime joakime added Bug For general bugs on Jetty side Sponsored This issue affects a user with a commercial support agreement labels Jul 3, 2020
@gregw
Copy link
Contributor

gregw commented Sep 1, 2020

@joakime does the newCachedChain method need to return a CachedChain? Can it just return a FilterChain ?

@gregw gregw self-assigned this Sep 15, 2020
@gregw
Copy link
Contributor

gregw commented Sep 15, 2020

If we are going to expose more of this API.... we need to clean it up... in fact this whole class needs a bit of a refresh.... changing title!

@gregw gregw changed the title Missing ServletHandler.newChain(Request baseRequest, List<FilterHolder> filters, ServletHolder servletHolder) overridable Cleanup ServletHandler, specifically with respect to making filter chains more extensible Sep 15, 2020
gregw added a commit that referenced this issue Sep 15, 2020
Issue #5022 Filter Cache cleanup:
 + Fixed many compiler warnings
 + removed old LazyList leftovers
 + Don't create holder string for source unless required
 + Only have a single type of chain, so it can be wrapped regardless of cache
 + Reverse mappings lists to make filter chain creation easier
 + build chain directly rather than build a list then a chain

Signed-off-by: Greg Wilkins <gregw@webtide.com>
gregw added a commit that referenced this issue Oct 5, 2020
* Issue #5022 Filter Cache cleanup

Issue #5022 Filter Cache cleanup:
 + Fixed many compiler warnings
 + removed old LazyList leftovers
 + Don't create holder string for source unless required
 + Only have a single type of chain, so it can be wrapped regardless of cache
 + Reverse mappings lists to make filter chain creation easier
 + build chain directly rather than build a list then a chain

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* added comment to explain ordering

Signed-off-by: gregw <gregw@webtide.com>

* More cleanups

* fixed toString format
turn off debug in OSGI test
@gregw gregw closed this as completed Oct 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side Sponsored This issue affects a user with a commercial support agreement
Projects
None yet
Development

No branches or pull requests

2 participants