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

[Rest Api Compatibility] Typed query #75453

Merged
merged 23 commits into from
Aug 3, 2021

Conversation

pgomulka
Copy link
Contributor

@pgomulka pgomulka commented Jul 19, 2021

Type query support was removed in #47207. This query will throw an exception in v7 rest api compatibility indicating that the support was removed + deprecation warnings.
In v8 it will not be available and error about type query being not found will be returned.

relates main meta issue #51816
relates types removal meta #54160

pgomulka added 3 commits July 19, 2021 13:57
Type information is no longer available, thereofre type query in v7
compatibility is working as match_all query
previously removed in elastic#47207

relates main meta issue elastic#51816
relates types removal meta elastic#54160
@pgomulka pgomulka self-assigned this Jul 19, 2021
@pgomulka pgomulka added :Core/Infra/REST API REST infrastructure and utilities v8.0.0 labels Jul 19, 2021
@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Jul 19, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

Copy link
Contributor

@romseygeek romseygeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @pgomulka, I have a couple of comments.


@Override
protected Query doToQuery(SearchExecutionContext context) throws IOException {
return new MatchAllDocsQuery();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do actually still have type information available: context.mappingLookup().getMapping().getRoot().name(). So you could check against the top-level type and then return a MatchAll/MatchNone depending on whether or not you get a match.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so you suggest to check if the value used on a type query is _doc (this is the only type we can have, right?) then return matchAll otherwise matchNone?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what values can I expect from context.mappingLookup().getMapping().getRoot().name() ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can still specify non-_doc type names in 7x indexes. context .... name() will return the type name of the current index.

@@ -0,0 +1,89 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than adding this back into the core server package, could we put it into a separate module instead? I think that would avoid changes to Node as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea, will do this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the sake of simplicity I kept this query in the server module (same package as previously)
I was worried that if we keep all v7 queries in single module it will have far too many dependencies.

@pgomulka pgomulka marked this pull request as draft July 22, 2021 13:18
@pgomulka pgomulka marked this pull request as ready for review August 3, 2021 11:29
@pgomulka
Copy link
Contributor Author

pgomulka commented Aug 3, 2021

@romseygeek I have refactored the way we register queries in #75722 so that registering new compatible only queries looks almost the same as the 'current version' queries.
Would you be able to take a second look at this?

@pgomulka pgomulka requested a review from romseygeek August 3, 2021 11:31
Copy link
Contributor

@romseygeek romseygeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @pgomulka!

@pgomulka pgomulka merged commit f339282 into elastic:master Aug 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/REST API REST infrastructure and utilities Team:Core/Infra Meta label for core/infra team v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants