Adding ApiFilteringActionFilter #97985
Conversation
|
Hi @masseyke, I've created a changelog YAML for you. |
|
|
||
| public abstract Class<Res> getResponseClass(); | ||
|
|
||
| public abstract CheckedFunction<Res, Res, Exception> getFilteringFunction(); |
There was a problem hiding this comment.
Is there a reason we need the extra level of indirection? Couldn't this be protected abstract Res filterResponse(Res response) throws Exception?
There was a problem hiding this comment.
Yeah I just did it this way because that's what I poorly refactored some code I'd written before.ActionListener::map (which is consuming this) requires. But no reason to leak that detail here
|
|
||
| private final ThreadContext threadContext; | ||
|
|
||
| public ApiFilteringActionFilter(ThreadContext threadContext) { |
| ); | ||
| } | ||
|
|
||
| public abstract String getActionName(); |
There was a problem hiding this comment.
This could be passed in as a ctor arg instead of having a dynamic method?
|
|
||
| public abstract String getActionName(); | ||
|
|
||
| public abstract Class<Res> getResponseClass(); |
There was a problem hiding this comment.
Same as the above, might be less boiler plate to pass this as a ctor arg
…nstructor, simplifying the filter method signature
…/elasticsearch into adding-ApiFilteringActionFilter
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
| @SuppressWarnings("unchecked") | ||
| private <Response extends ActionResponse> Response filter(Response response) throws Exception { | ||
| if (response.getClass().equals(responseClass)) { | ||
| return (Response) filterResponse((Res) response); |
There was a problem hiding this comment.
Are the casts necessary? Seems like the types match.
There was a problem hiding this comment.
I think that they are necessary. The problem is that ActionFilter doesn't have class-level generic types I could take advantage of -- they're just on the apply method. I introduced generic types at the class level of ApiFilteringActionFilter so that I could use them throughout the class. They're the same as the ones for the apply method, but the compiler doesn't know that. So I have to do the cast. I think the fix would be to add types at the class level for ActionFilter, but I wasn't sure what all that might break (since it can be used in 3rd-party plugins). Let me know if you're seeing something I'm not seeing though.
There was a problem hiding this comment.
Ah sorry, I see it is necessary for the argument to cast, but since Res is already a Response, no cast should be necessary?
There was a problem hiding this comment.
The compiler disagrees with you :)
> Task :x-pack:plugin:core:compileJava FAILED
/Users/kmassey/workspace/elasticsearch/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/api/filtering/ApiFilteringActionFilter.java:59: error: incompatible types: Res cannot be converted to Response
return filterResponse((Res) response);
^
where Res,Response are type-variables:
Res extends ActionResponse declared in class ApiFilteringActionFilter
Response extends ActionResponse declared in method <Response>filter(Response)
1 error
There was a problem hiding this comment.
Ok, I see, the names are quite confusing. :) Both Res and Response implement ActionFilter, but there is no association between the two. We can improve this in the future.
There was a problem hiding this comment.
I'm going to go ahead and merge this so that I can get some other PRs going, but let me know if I'm missing something and I'll fix this in a followup PR.
This adds an abstract ActionFilter meant to be extended in serverless modules that need to redact part of a response. When an implementing class is registered with a plugin, filterResponse() is applied if the transport action being called is the actionName passed to the constructor and the response's class is the responseClass passed to the constructor.
This PR adds an abstract ActionFilter meant to be extended in serverless modules that need to redact part of a response. When an implementing class is registered with a plugin, filterResponse() is applied if the transport action being called is the actionName passed to the constructor and the response's class is the responseClass passed to the constructor.