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

Don't modify source map when parsing composite runtime field #89114

Merged
merged 6 commits into from
Aug 17, 2022

Conversation

romseygeek
Copy link
Contributor

When calling RuntimeField.parseRuntimeFields() for fields defined in the
search request, we need to wrap the Map containing field definitions in another
Map that supports value removal, so that we don't inadvertently remove the
definitions from the root request. CompositeRuntimeField was not doing this
extra wrapping, which meant that requests that went to multiple shards and
that therefore parsed the definitions multiple times would throw an error
complaining that the fields parameter was missing, because the root request
had been modified.

@romseygeek romseygeek added >bug :Search Foundations/Mapping Index mappings, including merging and defining field types v8.4.1 v8.5.0 labels Aug 4, 2022
@romseygeek romseygeek requested a review from javanna August 4, 2022 11:40
@romseygeek romseygeek self-assigned this Aug 4, 2022
@elasticsearchmachine elasticsearchmachine added the Team:Search Meta label for search team label Aug 4, 2022
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine
Copy link
Collaborator

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

@romseygeek
Copy link
Contributor Author

OK this doesn't quite work because we need to distinguish between parsing at mapping time or at search time within the composite field as well. I'm digging further...

@romseygeek
Copy link
Contributor Author

Actually it looks as though it was a bug in serialization that wasn't caught before because the fields map was being emptied at parse time and so the unnecessary code to output it never actually got triggered. This is ready for review now @javanna

@romseygeek
Copy link
Contributor Author

@elasticmachine update branch

@romseygeek romseygeek requested a review from javanna August 16, 2022 12:28
Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

LGTM

@romseygeek romseygeek merged commit 189f279 into elastic:main Aug 17, 2022
javanna pushed a commit to javanna/elasticsearch that referenced this pull request Aug 17, 2022
…#89114)

When calling RuntimeField.parseRuntimeFields() for fields defined in the
search request, we need to wrap the Map containing field definitions in another
Map that supports value removal, so that we don't inadvertently remove the
definitions from the root request. CompositeRuntimeField was not doing this
extra wrapping, which meant that requests that went to multiple shards and
that therefore parsed the definitions multiple times would throw an error
complaining that the fields parameter was missing, because the root request
had been modified.
@mark-vieira mark-vieira added v8.4.0 and removed v8.4.1 labels Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Search Foundations/Mapping Index mappings, including merging and defining field types Team:Search Meta label for search team v8.4.0 v8.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants