One single (global) way to register custom query parsers #11481
Conversation
javanna
added
>enhancement
>breaking
v2.0.0-beta1
review
:Core/Infra/Plugins
| queryParsersClasses.add(queryParser); | ||
| return this; | ||
| } | ||
|
|
||
| public synchronized IndicesQueriesModule addQuery(QueryParser queryParser) { |
There was a problem hiding this comment.
I am conflicted on whether we should remove this or not. I think the above addQuery(Class<? extends QueryParser>) is the way to go, while this one would force binding to a specific instance. Can restore it if there is some usecase for it, it's just that I don't like having too many ways to do the same thing.
There was a problem hiding this comment.
I typically defaulted to this method because it avoids the generics gymnastics I mentioned above and I'd use the Class version when I needed guice to build the query. Since you've fixed the signature above I don't see a good reason for this. If you need the global state then you can register the extra classes with guice elsewhere and have them injected.
There was a problem hiding this comment.
I mean - global state in a query parser is probably a bad idea so I don't feel bad about you making it harder to do.
I have some vague worries that plugins require both an understanding of guice and Elasticsearch's onModule style initializers which makes plugin development daunting. But I don't have a solution to that. This pull request helps though, so yay!
|
|
||
| public synchronized IndicesQueriesModule addQuery(Class<QueryParser> queryParser) { | ||
| public synchronized IndicesQueriesModule addQuery(Class<? extends QueryParser> queryParser) { |
There was a problem hiding this comment.
+1 on the <? extends QueryParser> bit. I dislike the generics gymnastics you have to do to register the query without it.
|
This looks great to me. Much simpler! |
|
thanks @rmuir for looking, I will push this soon if there are no objections (will just let the move to maven submodules settle a bit first) |
clintongormley
changed the title
javanna
force-pushed
the
enhancement/custom_query_one_way
branch
from
4275b19
to
a5c3a71
Compare
|
LGTM |
Also fix the generics in `IndicesQueriesModule#addQuery(Class)` so that it can be used as the single way to register custom query parsers. Relates to elastic#11481 Closes elastic#11532
Also fix the generics in `IndicesQueriesModule#addQuery(Class)` so that it can be used as the single way to register custom query parsers. Relates to elastic#11481 Closes elastic#11532
javanna
changed the title
There are different ways to register custom query parsers through plugins, a couple of them work per index via index settings, which is probably even too flexible. There also three different ways to add a global custom query parser through either IndicesQueriesModule or IndicesQueriesRegistry. This commit consolidates the registration of custom query parsers via IndicesQueriesModule#addQuery(Class<? extends QueryParser>). The complexity of supporting parsers per index is not needed hence it got removed. Also the other ways of registering global custom parsers are dropped in favour of the one mentioned above. Closes elastic#11481
javanna
force-pushed
the
enhancement/custom_query_one_way
branch
from
1697a85
to
2ef0fcf
Compare
clintongormley
changed the title
There are different ways to register custom query parsers through plugins, a couple of them work per index via index settings, which doesn't seem to be needed. There also three different ways to add a global custom query parser through either IndicesQueriesModule or IndicesQueriesRegistry. This commit consolidates the registration of custom query parsers via IndicesQueriesModule#addQuery(Class<? extends QueryParser>). The complexity of supporting parsers per index is not needed hence it got removed. Also the other ways of registering global custom parsers are dropped in favour of the one mentioned above by providing the class of the parser.
This PR breaks plugins that plug in their own custom queries. If the change looks good we can deprecate in 1.x the methods that we are removing here.