Plugins: Replace Rest filters with RestHandler wrapper #21905
Conversation
RestFilters are a complex way of allowing plugins to add extra code before rest actions are executed. This change removes rest filters, and replaces with a wrapper which a single plugin may provide.
rjernst
added
:Core/Infra/Plugins
| @@ -45,7 +53,7 @@ | |||
| * @param <Response> the type of the response | |||
| * @return a listener that listens for responses and invokes the consumer when received | |||
| */ | |||
| static <Response> ActionListener<Response> wrap(Consumer<Response> onResponse, Consumer<Exception> onFailure) { | |||
| static <Response> ActionListener<Response> wrap(CheckedConsumer<Response> onResponse, Consumer<Exception> onFailure) { | |||
| final RestHandler rawHandler = getHandler(request); | ||
| final RestHandler handler = handlerWrapper.apply(rawHandler); | ||
|
|
||
| if (handler != null) { |
There was a problem hiding this comment.
should we allow null here? I think it must not return null but the raw handler instead? maybe I am missing something
There was a problem hiding this comment.
I have null meaning "don't use a wrapper", in which case we convert the null here to an identity wrapper to simplify the code which uses the wrapper.
There was a problem hiding this comment.
sorry I dont' follow. if the wrapper returns null we say we didn't find a handler, are we ok with this?
There was a problem hiding this comment.
Ok I see now. You are right, this was a bug. I pushed 65501a8.
| UnaryOperator<RestHandler> newRestWrapper = plugin.getRestHandlerWrapper(threadPool.getThreadContext()); | ||
| if (newRestWrapper != null) { | ||
| logger.debug("Using REST wrapper from plugin " + plugin.getClass().getName()); | ||
| if (restWrapper != null) { |
There was a problem hiding this comment.
I am not sure if we can do this in 5.x like this. There might be users relying on the filters... The other option is to just have multple wrappers and for iterate over them? passing the last wrapped one to the next wrapper?
There was a problem hiding this comment.
The problem with multiple is then an order has to be defined. This complicates the api, and I'm not sure what benefit it really adds; a wrapper here is a special thing, and having multiple means ones with higher order can "hide" others by never calling them. As far as breaking in a minor, we've had the position of breaking plugin apis in minors is ok since we require exact version compatibility with plugins.
There was a problem hiding this comment.
alright, I think this is so low level we are good with 1 and only 1. I think it's saver anyway
There was a problem hiding this comment.
shall we check in the community which plugins may be using rest filters first? If people are using multiple plugins that make use of rest filters, only one of them can be used after this change at the same time. There may not be a real use-case, but I wouldn't want to rush such a change in. The notion of rest filters goes along with action filters that we also have, and in the java world it's a pretty familiar concept. I don't see how much this specific change simplifies things code-wise, but that is a different problem.
There was a problem hiding this comment.
The notion of rest filters goes along with action filters that we also have, and in the java world it's a pretty familiar concept. I don't see how much this specific change simplifies things code-wise, but that is a different problem.
I disagree with this but we can discuss
There was a problem hiding this comment.
I have no idea if there are plugins like this out there to be honest. I just see it as an odd change that you can have only one plugin installed that can do things before executing rest actions. And, my personal opinion, I don't see having a filter chain and multiple filters with ordering as over-engineering. But again, my own personal opinion and it doesn't mean that it should block the change, especially if I am the only one seeing it this way.
There was a problem hiding this comment.
we crossed streams, I replied before reading your comment Simon. No problem, go ahead.
There was a problem hiding this comment.
it's no secret that we use this for security, in such a case we really want to make sure nobody else has registered one. That is also true for anybody else that does stuff like this if you are the one protecting the handler with auth/autz you want to be sure nothing is executed before you. Now that said, I think as a compromise we could have 1 wrapper that is the primary wrapper and a set of wrappers that are unordered. Yet I still think we should try and find out if that is really needed and the only way is to wait until folks come to us with this. We can usually react pretty quickly on those things ie the compromise can come once it's a problem. I am not too concerned about this on that low level we are operation on.
There was a problem hiding this comment.
@javanna lemme know what you think
There was a problem hiding this comment.
I am fine with one single wrapper given your explanations, I am happy that we looked for implementors but there weren't any.
|
@elasticmachine test this please |
* Plugins: Replace Rest filters with RestHandler wrapper RestFilters are a complex way of allowing plugins to add extra code before rest actions are executed. This change removes rest filters, and replaces with a wrapper which a single plugin may provide.
RestFilters are a complex way of allowing plugins to add extra code
before rest actions are executed. This change removes rest filters, and
replaces with a wrapper which a single plugin may provide.