-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Jetty-12 Rewrite RuleProcessor #8934
Conversation
This decouples the rewrite module from the `WrapperProcessor` class, which is being considered for significant refactoring or removal. Having a module specific version of that class allows better code readability and a more appropriate API that avoids duplication request instances.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the process(Response, Callback)
method.
All the rest I am not sure.
Not sure whether having 2 abstractions that represent the same thing (a request wrapper that is also a request processor) makes sense.
Definitely not sold about reusability that I would remove for now from this PR until we settle on a decision.
jetty-core/jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/Rule.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/Rule.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/Rule.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/Rule.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/CookiePatternRule.java
Show resolved
Hide resolved
jetty-core/jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/Rule.java
Show resolved
Hide resolved
jetty-core/jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/HeaderPatternRule.java
Show resolved
Hide resolved
jetty-core/jetty-rewrite/src/main/java/org/eclipse/jetty/rewrite/handler/Rule.java
Outdated
Show resolved
Hide resolved
@sbordet I removed all the residual re-use code (that was unintentional included by refactor). I don't mind having two abstractions for Request + Processor, as I believe they will diverge as we clean up the issues in the Handler tree. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just clean up Rule.Processor
as per comments.
This extracts some goodness from the closed #8793.
This PR decouples the rewrite module from the
WrapperProcessor
class, which is being considered for significant refactoring or removal. Having a module specific version of that class allows better code readability and a more appropriate API that avoids duplication request instances.