Extended ActionFilter to also enable filtering the response side #7465
Conversation
| if (i < this.action.filters.length) { | ||
| this.action.filters[i].process(actionName, request, listener, this); | ||
| } else if (i == this.action.filters.length) { | ||
| this.action.doExecute((Request) request, new FilteredActionListener<Response>(actionName, listener, new ResponseFilterChain(this.action.filters, logger))); |
There was a problem hiding this comment.
given that we also filter responses by creating a new response filter chain and filtered action listener, this inner class is not just a request filter chain... can we maybe merge the two at this point? Seems like in the end we either filters nothing or both (request and response) anyway...
There was a problem hiding this comment.
merging it means there's no proper handling for implementation that accidentally/intentionally call continueProcessing multiple times. Have two different chain instances keeps it clean and safe in that regard. The other option is to merge the code, but still create separate instances, but that's ugly and creates a mess (having single instance that behaves differently in different scenarios)... hence the two separate internal implementations - it's clean.
There was a problem hiding this comment.
I have to agree with luca I think this entire recursive continueProcessing is very confusing. I wonder if we can solve this simply interatively by returning a boolean from process that way we can implement this as a simple loop? Maybe I am missing the big picture?
There was a problem hiding this comment.
the recursive nature of the filters is actually not related to this specific issue here... but while on the subject, the goal here was to create a async filtering pipeline... if I understand you correctly (might be that I'm not), a loop will block.
As for this specific issue, the question was whether to use the same chain for both sides (request & response)... and we could... but that means we'll loose the handling of the "too many calls for continueProcessing" errors.
There was a problem hiding this comment.
Correct me if I am wrong but these filters have to be executed in a serial fashion one after another, right? So you can make this async if you need to on top of the blocking loop? I would like to see an example where this is used to understand the rational please :)
|
Left a few comments |
| * Enables filtering the execution of an action on the response side, either by sending a response through the | ||
| * {@link ActionListener} or by continuing the execution through the given {@link ActionFilterChain chain} | ||
| */ | ||
| void process(String action, ActionResponse response, ActionListener listener, ActionFilterChain chain); |
There was a problem hiding this comment.
in general I think this method should be called apply rather than process?
There was a problem hiding this comment.
agree! I'd also add the ActionFilterChain#continueProcessing should be renamed to ActionFilterChain#proceed
Enables filtering the actions on both sides - request and response. Also added a base class for filter implementations (cleans up filters that only need to filter one side) Also refactored the filter & filter chain methods to more intuitive names
|
@s1monw as we discussed, added a simple base class that relieves the implementation from dealing with filter chain. The base class is an abstract component as the way action filters are registered is via the modules (and the module binds them). Also renamed the methods to have more intuitive names. |
|
LGTM except of the |
|
merged in 333a39c |
clintongormley
changed the title
clintongormley
changed the title
Enables filtering the actions on both sides - request and response. Also added a base class for filter implementations (cleans up filters that only need to filter one side)