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

Emit deprecation warning when TermsLookup contains a type #53731

Merged
merged 16 commits into from
Mar 20, 2020

Conversation

romseygeek
Copy link
Contributor

TermsLookup in master no longer accepts a type parameter. We should emit
a deprecate warning in 7.x when a terms lookup requests includes type to prepare
users for its removal.

Relates to #41059

@romseygeek romseygeek added :Search/Search Search-related issues that do not fall into other categories >deprecation v7.7.0 labels Mar 18, 2020
@romseygeek romseygeek self-assigned this Mar 18, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (:Search/Search)

@romseygeek
Copy link
Contributor Author

This also refactors TermsLookup to use an ObjectParser. I'll open a separate PR against master to forward port this part.

romseygeek added a commit that referenced this pull request Mar 19, 2020
This commit refactors the fromXContent method in TermsLookup to use an
ObjectParser and adds an explicit parsing test.

Related to #53731
Copy link
Contributor

@mayya-sharipova mayya-sharipova left a comment

Choose a reason for hiding this comment

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

Thanks @romseygeek, this looks very good for me.
We also need to adjust documentation that id, path and routing are required fields in terms lookup. Or may the documentation meant that the whole terms lookup is optional, in this case we don't need to change anything there.

@romseygeek
Copy link
Contributor Author

@elasticsearchmachine test this please

@romseygeek romseygeek merged commit a3f21f2 into elastic:7.x Mar 20, 2020
@jpountz jpountz mentioned this pull request Mar 20, 2020
66 tasks
.boost(boost)
.queryName(queryName);

if (builder.isTypeless() == false) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@romseygeek sorry for the late review, I'm wondering why this check wasn't sufficient for emitting a deprecation warning? Was it not triggered on all the relevant code paths?

It would be nice to keep this strategy if possible, since it's consistent with all our other types deprecation warnings (deprecatedAndMaybeLog("api_name_with_types", TYPES_DEPRECATION_MESSAGE);). It also lets us avoid adding an allowed_warnings section to the yml tests, since in 7.x we already ignore all warnings in yml tests that start with [types removal] ... .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was a path that didn't trigger the warning, but I can't find it now, so it may have been refactored away elsewhere. I'll have a look, it would be good to remove some of these extra allowed_warnings messages.

@pgomulka pgomulka mentioned this pull request Mar 25, 2020
66 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>deprecation :Search/Search Search-related issues that do not fall into other categories v7.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants