From 7b759fad42c54ae31f0d83cb9a284680aa9e8510 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Mon, 3 Feb 2020 15:42:58 +0100 Subject: [PATCH 1/2] Disallow duplicate `dynamic_templates` names Currently the same `dynamic_templates` name can be used more than once in the same mappings file. This most certainly is unintentional and can create issues like those reported in #28988. Furthermore, when templates are matched later in `RootObjectMapper#findTemplate`, it looks like we simply pick the first matching one we see in the input array, so using the same template name twice sounds like an error prone idea anyway. This change checks for duplicate names on parsing and throws an error if we encounter the same name twice. Closes #28988 --- .../index/mapper/RootObjectMapper.java | 8 +++++ .../index/mapper/RootObjectMapperTests.java | 32 +++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java index cd3242e98a2d7..ee83876edb345 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java @@ -32,9 +32,11 @@ import java.util.ArrayList; import java.util.Collection; import java.util.Collections; +import java.util.HashSet; import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.Set; import static org.elasticsearch.common.xcontent.support.XContentMapValues.nodeBooleanValue; import static org.elasticsearch.index.mapper.TypeParsers.parseDateTimeFormatter; @@ -170,6 +172,7 @@ protected boolean processField(RootObjectMapper.Builder builder, String fieldNam */ List tmplNodes = (List) fieldNode; List templates = new ArrayList<>(); + Set templatesAlreadyDefinded = new HashSet<>(); for (Object tmplNode : tmplNodes) { Map tmpl = (Map) tmplNode; if (tmpl.size() != 1) { @@ -180,7 +183,12 @@ protected boolean processField(RootObjectMapper.Builder builder, String fieldNam Map templateParams = (Map) entry.getValue(); DynamicTemplate template = DynamicTemplate.parse(templateName, templateParams); if (template != null) { + if (templatesAlreadyDefinded.contains(templateName)) { + throw new MapperParsingException("A dynamic template cannot be defined twice, but a template with name [" + + templateName + "] has already been defined."); + } templates.add(template); + templatesAlreadyDefinded.add(template.name()); } } builder.dynamicTemplates(templates); diff --git a/server/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java index 0b805eb726646..6521184275e42 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java @@ -160,6 +160,38 @@ public void testDynamicTemplates() throws Exception { assertEquals(mapping3, mapper.mappingSource().toString()); } + public void testDynamicTemplatesDuplicationThrows() throws Exception { + String mapping = Strings.toString(XContentFactory.jsonBuilder() + .startObject() + .startObject("type") + .startArray("dynamic_templates") + .startObject() + .startObject("my_template") + .field("match_mapping_type", "string") + .startObject("mapping") + .field("type", "keyword") + .endObject() + .endObject() + .endObject() + .startObject() + .startObject("my_template") + .field("match_mapping_type", "string") + .startObject("mapping") + .field("type", "keyword") + .endObject() + .endObject() + .endObject() + .endArray() + .endObject() + .endObject()); + MapperService mapperService = createIndex("test").mapperService(); + MapperParsingException ex = expectThrows(MapperParsingException.class, + () -> mapperService.merge("type", new CompressedXContent(mapping), MergeReason.MAPPING_UPDATE)); + assertEquals("Failed to parse mapping: A dynamic template cannot be defined twice, but a template with name [my_template]" + + " has already been defined.", ex.getMessage()); + + } + public void testIllegalFormatField() throws Exception { String dynamicMapping = Strings.toString(XContentFactory.jsonBuilder() .startObject() From d9769ad1319daeb3ea84df4c179c6c4c1a872960 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Mon, 3 Feb 2020 16:35:38 +0100 Subject: [PATCH 2/2] Fix test --- .../org/elasticsearch/index/mapper/DynamicMappingTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/test/java/org/elasticsearch/index/mapper/DynamicMappingTests.java b/server/src/test/java/org/elasticsearch/index/mapper/DynamicMappingTests.java index 9e0d9155d43bd..2ae8d0daf637c 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/DynamicMappingTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/DynamicMappingTests.java @@ -607,7 +607,7 @@ public void testDateDetectionInheritsFormat() throws Exception { .endObject() .endObject() .startObject() - .startObject("dates") + .startObject("dates_with_explicit_format") .field("match_mapping_type", "date") .field("match", "*3") .startObject("mapping")