Skip to content

Accept any FieldValue for terms_set #2906

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

Merged
merged 2 commits into from
Sep 24, 2024

Conversation

mspielberg
Copy link
Contributor

Change terms_set definition to accept any FieldValue, as the terms query already does, and as the Elasticsearch server implementation already does.

This is not a breaking change at the Typescript level, as FieldValue includes string as one of its alternatives, but the materialization of the change may result in a breaking API change in specific language (e.g. Java) compiled clients.

Copy link

cla-checker-service bot commented Sep 18, 2024

💚 CLA has been signed

@l-trotta
Copy link
Contributor

Looks correct, we probably got this from the documentation, which says that terms is a string array. Going from String to FieldValue breaks the java constructor for this, could we not backport this and leave it for 9? @flobernd @pquentin

@flobernd
Copy link
Member

Fine with not backporting this for now. For C# it's not a breaking change as we have an implicit conversion operator from string -> FieldValue.

@pquentin
Copy link
Member

Also happy to not backport.

@mspielberg
Copy link
Contributor Author

As a side note, I already submitted elastic/elasticsearch#113043 to correct the documentation as well.

Copy link
Contributor

@l-trotta l-trotta 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 for the contribution @mspielberg

@l-trotta
Copy link
Contributor

correctly validated here, merging

@l-trotta l-trotta merged commit 6c5b01d into elastic:main Sep 24, 2024
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants