multi dynamic_templates incorrect

[cyinll]

Elasticsearch version (bin/elasticsearch --version): 5.3.0

Plugins installed: []

JVM version (java -version): 1.8

OS version (uname -a if on a Unix-like system):

Description of the problem including expected versus actual behavior:

Steps to reproduce:

1. create template with dynamic_templates in mappings

PUT /_template/dynamic
{
  "template" : "*",
  "mappings": {
    "default": {
      "dynamic_templates": [
        {
          "string_as_text": {
            "match": "*_text",
            "match_mapping_type": "string",
            "mapping": {
              "type": "text",
              "analyzer": "ik_max_word"
            }
          }
        }
      ]
    }
  }
}

2. create index with dynamic_templates in mappings

PUT /test_dynamic
{
  "mappings": {
    "default": {
      "dynamic_templates": [
        {
          "string_as_text": {
            "match_mapping_type": "string",
            "mapping": {
              "type": "text",
              "analyzer": "ik_max_word"
            }
          }
        }
      ]
    }
  }
}

3. get index mappings, i hope it use the second dynamic_templates without 'match' parameter, but it will use the first

GET /test_dynamic/_mappings

{
  "test_dynamic" : {
    "mappings" : {
      "default" : {
        "dynamic_templates" : [
          {
            "string_as_text" : {
              "match" : "*_text",
              "match_mapping_type" : "string",
              "mapping" : {
                "analyzer" : "ik_max_word",
                "type" : "text"
              }
            }
          }
        ]
      }
    }
  }
}

[colings86]

So whats happening here is that dynamic templates with the same name are being merged together to produce the final template. You can see this more clearly if you add an extra setting to the index creeation command as below:

PUT /test_dynamic
{
  "mappings": {
    "default": {
      "dynamic_templates": [
        {
          "string_as_text": {
            "match_mapping_type": "string",
            "mapping": {
              "type": "text",
              "analyzer": "ik_max_word",
              "norms": false
            }
          }
        }
      ]
    }
  }
}

The resulting mapping is now:

{
  "test_dynamic": {
    "mappings": {
      "default": {
        "dynamic_templates": [
          {
            "string_as_text": {
              "match": "*_text",
              "match_mapping_type": "string",
              "mapping": {
                "analyzer": "ik_max_word",
                "norms": false,
                "type": "text"
              }
            }
          }
        ]
      }
    }
  }
}

Note that the mapping now includes both the match line and thee norms line.

It's tricky to know what to do here as its certainly useful to be able to augment rules made in a base template by adding or overriding settings in the rules in the index creation command. Maybe we should only merge the mapping part of the dynamic template and have the match and match_mapping_type only take from the last template in the precedence order, however I'm not sure how complex this would be in the code and they could well be other users that rely on being able to merge the match and match_mapping_type in their templates. (@clintongormley @jpountz what do you think?)

A workaround for this would be to add "match": "*", to the dynamic template in the create index request which will mean the template will match any name of field as you require.

/cc @elastic/es-search-aggs

[colings86]

We discussed this in FixItFriday and it was agreed that it is unfortunate that dynamic templates with the same name in different index templates/create index request are merged rather than replaced and we would like to change this.

Another concern was raised that when merging the index template and create index request which even have different dynamic templates its not clear what the order of the dynamic templates would be in the final mappings and we should fix this too.

Since this would be a breaking fix we would probably need to have a temporary setting to specify whether to merge or replace dynamic templates in mappings when merging the mappings and then switch the default of this setting in a major version and remove the setting in the subsequent major version

[jtibshirani]

I've started to look into fixing this. @colings86 there are a few points that would be helpful to clarify:

1. When combining mappings across index templates and create index requests, we do a ‘deep merge’, which attempts to recursively combine all map keys. This clearly differs from the normal way mappings are merged during a ‘put mappings’ request. From the documentation on index templates:

   Note, for mappings, the merging is "deep", meaning that specific object/property based mappings can easily be added/overridden on higher order templates, with lower order templates providing the basis.

   It sounds like we want to keep this deep merge strategy for all other parts of the mapping, but change it only for dynamic templates? This seems a bit inconsistent to me, as other top-level parts of the mapping (like _meta) will still still undergo a deep merge. I don’t have context from the discussion though, and am likely missing some important reasoning here.

2. After merging the index templates and create index request, it seems like the dynamic templates should be listed in the following order: the entries from the ‘create index’ request, then the entries from each matching index template, starting from the index template with the highest ‘order’ parameter and progressing to the lowest. It looks like this is in fact what’s happening, but I can make sure to add docs + a test for this behavior.

   An alternative would be to introduce an ‘order’ parameter for dynamic field templates, as we have for index templates. This option may be overkill though, depending on the typical use case for these templates.

Lastly @cyinll — just a quick note that I modified the first step of your reproduction above to include the _template endpoint.

[colings86]

@jtibshirani thanks for looking into this

It sounds like we want to keep this deep merge strategy for all other parts of the mapping, but change it only for dynamic templates? This seems a bit inconsistent to me, as other top-level parts of the mapping (like _meta) will still still undergo a deep merge. I don’t have context from the discussion though, and am likely missing some important reasoning here.

Yes we would like the deep merge to effectively treat the rules in the dynamic mappings as immutable blocks so rules with the same name will overwrite the existing rules rather than merge into the existing rules. You are right that this is a departure from the rest of the mapping when merging but we felt when discussing this that having this rules overwrite rather than merge makes it easier for the user to reason about how their template will be integrated with other ones. It also allows the use to specify rules that don't apply certain settings like the one in the example above.

If you think this warrants another discussion I'm happy to discuss again as there might be things we did not consider before.

It looks like this is in fact what’s happening, but I can make sure to add docs + a test for this behaviour.

Great, I think adding to the docs and creating tests for this behaviour would be a good idea.

[simitt]

APM creates multiple keys with the same name for default dynamic_templates, with the purpose of having different mappings on different match_mapping_type definitions. E.g

{
    "dynamic_templates": [
        {
            "labels": {
                "path_match": "labels.*",
                "match_mapping_type": "string",
                "mapping": {
                    "type": "keyword"
                }
            }
        },
        {
            "labels": {
                "path_match": "labels.*",
                "match_mapping_type": "boolean",
                "mapping": {
                    "type": "boolean"
                }
            }
        },
        {
            "labels": {
                "path_match": "labels.*",
                "mapping": {
                    "scaling_factor": 1000000,
                    "type": "scaled_float"
                }
            }
        }
    ]
}

This works fine as long as users don't add custom templates defining dynamic_templates, as this leads to merging the dynamic templates, removing all but one entry with the same name in the final mapping.

The issue was identified by a user, for more details see elastic/apm-server#2693 (comment)

As pointed out to me by @jtibshirani the behavior seems related to the discussions on this issue.

[cbuescher]

I opened #52062 with the current plan to deprecate and remove the ability to use the same name for dynamic templates and will close this issue because of this.

[elasticsearchmachine]

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

[javanna]

Closing in favour of #53326 which covers the exact same problem.