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

Minor fix in dynamic template validation #66166

Merged

Conversation

javanna
Copy link
Member

@javanna javanna commented Dec 10, 2020

As part of #66156 the validation of dynamic templates has been adapted to support runtime fields. As part of that change, a break was forgotten, which causes needless additional validation rounds after a template has been already successfully validated against one of the field types.

As part of elastic#66156 the validation of dynamic templates has been adapted to support runtime fields. As part of that change, a `break` was forgotten, which causes needless additional validation rounds after a template has been already successfully validated against one of the field types.
@javanna javanna added :Search Foundations/Mapping Index mappings, including merging and defining field types v8.0.0 labels Dec 10, 2020
@javanna javanna requested a review from romseygeek December 10, 2020 13:28
@elasticmachine elasticmachine added the Team:Search Meta label for search team label Dec 10, 2020
@elasticmachine
Copy link
Collaborator

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

@javanna
Copy link
Member Author

javanna commented Dec 10, 2020

I am fixing this same issue for 7.x as part of #66156 , the backport of the PR that introduced the problem, which does cause problems in 7.x due to deprecation warnings.

Copy link
Contributor

@romseygeek romseygeek left a comment

Choose a reason for hiding this comment

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

LGTM, although trying to understand that outer loop is giving me a headache. Given that a DynamicTemplate can only have one fieldtype, maybe we should rework this in a followup so that templates themselves are responsible for validation?

@javanna
Copy link
Member Author

javanna commented Dec 10, 2020

@romseygeek I am not sure there is a way around that loop. When a template is not mapped against a specific type (because match_mapping_type is * or not specified), we try to validate it against all possible dynamic field types, and if at least one does not throw error, we accept the template, although it may actually cause problems at index time depending on the field it gets applied to in practice.

@javanna
Copy link
Member Author

javanna commented Dec 10, 2020

run elasticsearch-ci/eql-correctness

@javanna javanna merged commit 5fe7b7f into elastic:master Dec 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>non-issue :Search Foundations/Mapping Index mappings, including merging and defining field types Team:Search Meta label for search team v7.11.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants