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

Check field scripts return serializable object #94877

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

rjernst
Copy link
Member

@rjernst rjernst commented Mar 29, 2023

When a script field is used, the returned value must be serializable, sometimes with transport (if there is more than one node) and always with x-content. Currently returning a non-serializable value can result in errors later. This commit more aggressively checks the value returned from field scripts to ensure a more confusing error is not encountered.

When a script field is used, the returned value must be serializable,
sometimes with transport (if there is more than one node) and always
with x-content. Currently returning a non-serializable value can result
in errors later. This commit more aggressively checks the value returned
from field scripts to ensure a more confusing error is not encountered.
@rjernst rjernst added >bug :Search/Search Search-related issues that do not fall into other categories v8.8.0 labels Mar 29, 2023
@elasticsearchmachine
Copy link
Collaborator

Hi @rjernst, I've created a changelog YAML for you.

@cla-checker-service
Copy link

cla-checker-service bot commented Apr 6, 2023

💚 CLA has been signed

@rjernst rjernst force-pushed the script_field_serialization_validation2 branch from d9b6bbd to 5809075 Compare April 6, 2023 21:48
@gmarouli gmarouli added v8.9.0 and removed v8.8.0 labels Apr 26, 2023
@rjernst rjernst marked this pull request as ready for review May 9, 2023 02:28
@elasticsearchmachine elasticsearchmachine added the Team:Search Meta label for search team label May 9, 2023
@elasticsearchmachine
Copy link
Collaborator

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

@rjernst
Copy link
Member Author

rjernst commented May 9, 2023

@romseygeek I opened this PR a while ago to generally stop objects returned by doc scripts from causing serialization errors. The PR has sat stalled because other failures (ie other classes that do not have either xcontent or transport available are returned by some test scripts). I just found your recent PR #95673 which changed the behavior to have the script fallback to <unserializable>. I guess that is ok, but I think this more general purpose check is better, right next to where the script is run. Additionally, this more narrowly checks serializabilty, while other errors will continue to be propagated.

My only difficult is finding a way to test this...ScriptFieldPhase does not have unit tests, and I don't see a way to make LeafReaderContext mockable, but maybe you have ideas on how to test it?

@quux00 quux00 added v8.11.0 and removed v8.10.0 labels Aug 16, 2023
@mattc58 mattc58 added v8.12.0 and removed v8.11.0 labels Oct 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants