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

Extended ActionFilter to also enable filtering the response side #7465

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
4 participants
@uboness
Copy link
Contributor

uboness commented Aug 26, 2014

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)

@javanna

View changes

src/main/java/org/elasticsearch/action/support/TransportAction.java Outdated
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)));

This comment has been minimized.

Copy link
@javanna

javanna Aug 27, 2014

Member

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...

This comment has been minimized.

Copy link
@uboness

uboness Aug 27, 2014

Author Contributor

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.

This comment has been minimized.

Copy link
@s1monw

s1monw Aug 28, 2014

Contributor

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?

This comment has been minimized.

Copy link
@uboness

uboness Aug 28, 2014

Author Contributor

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.

This comment has been minimized.

Copy link
@s1monw

s1monw Aug 29, 2014

Contributor

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 :)

@javanna

View changes

src/main/java/org/elasticsearch/action/support/TransportAction.java Outdated
}

@Override
public void continueProcessing(String action, ActionResponse response, ActionListener listener) {

This comment has been minimized.

Copy link
@javanna

javanna Aug 27, 2014

Member

I find these two empty continueProcessing methods confusing, if we manage to merge the two filter chains impl as said above, we would get rid of them I think

This comment has been minimized.

Copy link
@uboness

uboness Aug 27, 2014

Author Contributor

see comment above

@javanna

View changes

src/main/java/org/elasticsearch/action/support/ActionFilter.java Outdated
* A base class for action filters with default implementation for the {@code process} methods on both the request
* and response sides (where by default they just continue execution)
*/
public static abstract class Base implements ActionFilter {

This comment has been minimized.

Copy link
@javanna

javanna Aug 27, 2014

Member

Not sure whether this base class should be here, we use only for testing, would it be useful for something else?

This comment has been minimized.

Copy link
@uboness

uboness Aug 27, 2014

Author Contributor

now that the filter has two filtering method I think it makes sense (a lot of times you only want only one of them)

This comment has been minimized.

Copy link
@s1monw

s1monw Aug 28, 2014

Contributor

I wonder if we should make this an abstract class to begin with then we can pull all this up into the base class?

This comment has been minimized.

Copy link
@uboness

uboness Aug 28, 2014

Author Contributor

the rationale here is that by having it as an interface we can have an abstract component implement an action filter... having a base class we could potentially have a composite filter as an abstract component, that delegates to internal non-component filters

@javanna

This comment has been minimized.

Copy link
Member

javanna commented Aug 27, 2014

Left a few comments

@javanna javanna removed the review label Aug 27, 2014

@s1monw

View changes

src/main/java/org/elasticsearch/action/support/ActionFilter.java Outdated
* 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);

This comment has been minimized.

Copy link
@s1monw

s1monw Aug 28, 2014

Contributor

in general I think this method should be called apply rather than process?

This comment has been minimized.

Copy link
@uboness

uboness Aug 28, 2014

Author Contributor

agree! I'd also add the ActionFilterChain#continueProcessing should be renamed to ActionFilterChain#proceed

*/
void process(final String action, final ActionRequest actionRequest, final ActionListener actionListener, final ActionFilterChain actionFilterChain);
int order();

This comment has been minimized.

Copy link
@s1monw

s1monw Aug 28, 2014

Contributor

maybe call it ord or position?

This comment has been minimized.

Copy link
@uboness

uboness Aug 28, 2014

Author Contributor

the name right now is aligned with other "ordered" constructs in es (e.g. index templates, rest filters, etc...)

@Test
public void testActionFilters() throws ExecutionException, InterruptedException {
public void testActionFilters_Request() throws ExecutionException, InterruptedException {

This comment has been minimized.

Copy link
@s1monw

s1monw Aug 28, 2014

Contributor

can we please not add _ into method names?

This comment has been minimized.

Copy link
@uboness

uboness Aug 28, 2014

Author Contributor

I disagree here actually... I believe that the test method describes best the scenario that you're testing, and having too long method names become unreadable. I like puting _ in test method names cause it makes them more readable and descriptive.

This comment has been minimized.

Copy link
@s1monw

s1monw Sep 5, 2014

Contributor

I don't think this is worth the inconsistency. Please remove that and make it camel cased

Extended ActionFilter to also enable filtering the response side
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

@uboness uboness force-pushed the uboness:enhance/action_filter branch to 775a657 Sep 3, 2014

@uboness

This comment has been minimized.

Copy link
Contributor Author

uboness commented Sep 3, 2014

@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.

@s1monw

This comment has been minimized.

this looks backwards actually now that I look at it. IMO the hierarchy should be ActionFilter, AsyncActionFilter extends ActionFilter and AsyncActionFilter should maybe just get the chain as a ctor arg.... not sure here

This comment has been minimized.

Copy link
Owner Author

uboness replied Sep 5, 2014

funny... I think we see things opposite (for me blocking cleanly extends async and not the other way around :))... in any case, we can't pass anything in the ctor as the filters are meant to be injected. To do that, we would need to introduce an ActionFilterFactory notion and that will just complicate things

This comment has been minimized.

Copy link

s1monw replied Sep 5, 2014

alright fair enough...

@s1monw

This comment has been minimized.

Copy link
Contributor

s1monw commented Sep 5, 2014

LGTM except of the _ in the method name please fix that one

@uboness

This comment has been minimized.

Copy link
Contributor Author

uboness commented Sep 6, 2014

merged in 333a39c

@uboness uboness closed this Sep 6, 2014

@clintongormley clintongormley changed the title Extended ActionFilter to also enable filtering the response side Internal: Extended ActionFilter to also enable filtering the response side Sep 8, 2014

@clintongormley clintongormley changed the title Internal: Extended ActionFilter to also enable filtering the response side Extended ActionFilter to also enable filtering the response side Jun 7, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.