-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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
Composable templates merge dynamic templates in reverse order #76702
Comments
Pinging @elastic/es-search (Team:Search) |
Pinging @elastic/es-core-features (Team:Core/Features) |
First, summarizing the problem since it's a bit tricky: dynamic templates are defined in a list, and when deciding which one to apply for a field, we always select the first matching dynamic template. When merging composable template mappings to produce the final mapping, we append the higher-order template's dynamic templates at the end of the list. This means that its dynamic templates actually have lower precedence than earlier templates, which goes against the idea that mappings in higher-order templates should always take precedence. With legacy templates, the merge strategy is less sophisticated, but it happens to add dynamic templates for the the higher-order template at the start of the list. To me this seems like a bug in composable template merging -- if higher-order mappings are supposed to take higher precedence, then their dynamic templates should be given higher precedence too. The fix would be to add the dynamic templates at the start of the list (instead of end). Unfortunately this fix breaks existing behavior, so we'd need to think through how to introduce it. Maybe it'd be fine to just change the behavior in 8.0, avoiding any changes in 7.x. As part of the fix we could double-check the behavior for any other mapping components that are arrays. |
Perhaps it would be good to update the docs as a minimum? At the moment they are don't reflect the behaviour (and it certainly cost me some development time to work that out!). |
@jacksmith15 this is a good point, but I'm struggling to see where it's appropriate to document this... it would be as if we were documenting buggy behavior. In any case, I'll try to make some progress on this issue. |
Pinging @elastic/es-data-management (Team:Data Management) |
Elasticsearch version 7.13.4 (testing using official docker image)
Plugins installed: []
JVM version (
java -version
): 16+36OS version (
uname -a
if on a Unix-like system):Linux d1da479e9601 5.8.0-63-generic #71-Ubuntu SMP Tue Jul 13 15:59:12 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
Description of the problem including expected versus actual behavior:
According to the documentation:
The following is also stated in the API documentation:
However this is not true for dynamic templates, as dynamic templates from higher precedence component templates are added to the end of the list, which is actually lower precedence, as described in the dynamic templates documentation:
This seems to occur because a generic dictionary merge function is used to resolve templates, rather than one which is context-aware to template semantics.
Steps to reproduce:
For simplicity, I will show a single template with index mapping overrides, but the same is true for the relationship between multiple component templates, or a component template and an index template (happy to provide additional examples if required):
The result is that the field uses the mapping from the lower precedence template:
Examination of the index mappings show that this is because the dynamic templates have been merged with higher-precedence last:
For context the legacy template API produces the opposite result:
Legacy template example
This results in the field using the mapping from the higher precedence template:
Additional notes
The text was updated successfully, but these errors were encountered: