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

Consolidate script parsing from object #59507

Merged

Conversation

javanna
Copy link
Member

@javanna javanna commented Jul 14, 2020

The update by query action parses a script from an object (map or string). We will need to do the same for runtime fields as they are parsed as part of mappings (#59391).

This commit moves the existing parsing of a script from an object from RestUpdateByQueryAction to the Script class. It also adds tests and adjusts some error messages that are incorrect. Also, options were not parsed before and they are now. And unsupported fields are no longer silently ignored.

I plan on backporting this change to 7.x without the handling for unsupported fields, as that is a breaking change.

The update by query action parses a script from an object (map or string). We will need to do the same for runtime fields as they are parsed as part of mappings (elastic#59391).

This commit moves the existing parsing of a script from an object from RestUpdateByQueryAction to the Script class. It also adds tests and adjusts some error messages that are incorrect. Also, options were not parsed before and they are now. And unsupported fields are no longer silently ignored.
@javanna javanna added >breaking :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache v8.0.0 labels Jul 14, 2020
@javanna javanna requested a review from jdconrad July 14, 2020 08:58
@elasticmachine
Copy link
Collaborator

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

@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Jul 14, 2020
@javanna javanna requested a review from nik9000 July 14, 2020 09:00
Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

Awesome!

@javanna javanna merged commit cc31035 into elastic:master Jul 14, 2020
javanna added a commit to javanna/elasticsearch that referenced this pull request Jul 14, 2020
@jdconrad
Copy link
Contributor

I think in a follow up PR we should make the builder in Script used for XContent generic enough to either XContent or a Map or even a String. This way the code follows the same path for both parsing and error checking instead of having separate parsers here.

@javanna
Copy link
Member Author

javanna commented Jul 15, 2020

@jdconrad I agree that it's odd to have two separate code paths. On the other hand, it's two different sources that are parsed differently. I don't see how we can share the parsing code without converting the source format e.g. writing the map as xcontent and parsing it back. Can you elaborate?

@jdconrad
Copy link
Contributor

@javanna I see what you mean, I do think there's an opportunity here to use the same validation, though for both. I will give this some thought.

javanna added a commit that referenced this pull request Jul 15, 2020
jrodewig added a commit that referenced this pull request Sep 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>breaking :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache 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.

5 participants