* Moved validation of dynamic templates to where dynamic templates are parsed.

martijnvg · martijnvg · commit a8c10793c9f4 · 2020-02-14T11:54:06.000+01:00
* Added tests and updated the documentation
diff --git a/docs/reference/mapping/dynamic/templates.asciidoc b/docs/reference/mapping/dynamic/templates.asciidoc
@@ -37,6 +37,10 @@ Dynamic templates are specified as an array of named objects:
 <2> The match conditions can include any of : `match_mapping_type`, `match`, `match_pattern`, `unmatch`, `path_match`, `path_unmatch`.
 <3> The mapping that the matched field should use.
 
+If an provided mapping contains an invalid mapping snippet then that results in
+a validation error when updating a mapping. This is to ensure that if an unmapped
+field is mapped via dynamic templates then this will result in a valid mapping update
+and not fail the index or update request with a document that contains an invalid field.
 
 Templates are processed in order -- the first matching template wins. When
 putting new dynamic templates through the <<indices-put-mapping, put mapping>> API,
diff --git a/docs/reference/release-notes/8.0.0-alpha1.asciidoc b/docs/reference/release-notes/8.0.0-alpha1.asciidoc
@@ -6,3 +6,10 @@ The changes listed below have been released for the first time in {es}
 
 coming[8.0.0]
 
+[[breaking-8.0.0-alpha1]]
+[float]
+=== Breaking changes
+
+Mapping::
+* Dynamic mappings in indices created on 8.0 and later no longer accept invalid mapping snippets
+  (e.g. incorrect analyzer settings or unknown field types). {pull}51233[#51233]
diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetaDataCreateIndexService.java
@@ -688,7 +688,6 @@ private static void updateIndexMappingsAndBuildSortOrder(IndexService indexServi
         if (!mappings.isEmpty()) {
             assert mappings.size() == 1 : mappings;
             mapperService.merge(MapperService.SINGLE_MAPPING_NAME, mappings, MergeReason.MAPPING_UPDATE);
-            mapperService.validateDynamicTemplates();
         }
 
         if (sourceMetaData == null) {
diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java
@@ -717,8 +717,4 @@ public synchronized List<String> reloadSearchAnalyzers(AnalysisRegistry registry
         return reloadedAnalyzers;
     }
 
-    public void validateDynamicTemplates() {
-        Mapper.TypeParser.ParserContext parserContext = documentParser.parserContext();
-        documentMapper().root().validateDynamicTemplates(parserContext);
-    }
 }
diff --git a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java
@@ -138,7 +138,7 @@ public Mapper.Builder parse(String name, Map<String, Object> node, ParserContext
                 String fieldName = entry.getKey();
                 Object fieldNode = entry.getValue();
                 if (parseObjectOrDocumentTypeProperties(fieldName, fieldNode, parserContext, builder)
-                        || processField(builder, fieldName, fieldNode, parserContext.indexVersionCreated())) {
+                        || processField(builder, fieldName, fieldNode, parserContext)) {
                     iterator.remove();
                 }
             }
@@ -147,7 +147,7 @@ public Mapper.Builder parse(String name, Map<String, Object> node, ParserContext
 
         @SuppressWarnings("unchecked")
         protected boolean processField(RootObjectMapper.Builder builder, String fieldName, Object fieldNode,
-                                       Version indexVersionCreated) {
+                                       ParserContext parserContext) {
             if (fieldName.equals("date_formats") || fieldName.equals("dynamic_date_formats")) {
                 if (fieldNode instanceof List) {
                     List<DateFormatter> formatters = new ArrayList<>();
@@ -191,6 +191,7 @@ protected boolean processField(RootObjectMapper.Builder builder, String fieldNam
                     Map<String, Object> templateParams = (Map<String, Object>) entry.getValue();
                     DynamicTemplate template = DynamicTemplate.parse(templateName, templateParams);
                     if (template != null) {
+                        validateDynamicTemplate(parserContext, template);
                         templates.add(template);
                     }
                 }
@@ -342,12 +343,6 @@ protected void doXContent(XContentBuilder builder, ToXContent.Params params) thr
         }
     }
 
-    public void validateDynamicTemplates(Mapper.TypeParser.ParserContext parserContext) {
-        for (DynamicTemplate dynamicTemplate : dynamicTemplates.value()) {
-            validateDynamicTemplate(parserContext, dynamicTemplate);
-        }
-    }
-
     private static void validateDynamicTemplate(Mapper.TypeParser.ParserContext parserContext,
                                                 DynamicTemplate dynamicTemplate) {
 
@@ -370,18 +365,23 @@ private static void validateDynamicTemplate(Mapper.TypeParser.ParserContext pars
             String defaultDynamicType = contentFieldType.defaultMappingType();
             String mappingType = dynamicTemplate.mappingType(defaultDynamicType);
             Mapper.TypeParser typeParser = parserContext.typeParser(mappingType);
-            if (typeParser != null) {
-                Map<String, Object> fieldTypeConfig = dynamicTemplate.mappingForName("__dummy__", defaultDynamicType);
-                fieldTypeConfig.remove("type");
-                try {
-                    Mapper.Builder<?, ?> dummyBuilder = typeParser.parse("__dummy__", fieldTypeConfig, parserContext);
-                    if (fieldTypeConfig.isEmpty()) {
-                        dynamicTemplateInvalid = false;
-                        break;
-                    }
-                } catch (Exception e) {
-                    lastError = e;
+            if (typeParser == null) {
+                lastError = new IllegalArgumentException("No mapper found for type [" + mappingType + "]");
+                continue;
+            }
+
+            Map<String, Object> fieldTypeConfig = dynamicTemplate.mappingForName("__dummy__", defaultDynamicType);
+            fieldTypeConfig.remove("type");
+            try {
+                Mapper.Builder<?, ?> dummyBuilder = typeParser.parse("__dummy__", fieldTypeConfig, parserContext);
+                if (fieldTypeConfig.isEmpty()) {
+                    dynamicTemplateInvalid = false;
+                    break;
+                } else {
+                    lastError = new IllegalArgumentException("Unused mapping attributes [" + fieldTypeConfig + "]");
                 }
+            } catch (Exception e) {
+                lastError = e;
             }
         }
 
@@ -392,7 +392,13 @@ private static void validateDynamicTemplate(Mapper.TypeParser.ParserContext pars
             if (failInvalidDynamicTemplates) {
                 throw new IllegalArgumentException(message, lastError);
             } else {
-                DEPRECATION_LOGGER.deprecatedAndMaybeLog("invalid_dynamic_template", message);
+                final String deprecationMessage;
+                if (lastError != null) {
+                     deprecationMessage = String.format(Locale.ROOT, "%s, caused by [%s]", message, lastError.getMessage());
+                } else {
+                    deprecationMessage = message;
+                }
+                DEPRECATION_LOGGER.deprecatedAndMaybeLog("invalid_dynamic_template", deprecationMessage);
             }
         }
     }
diff --git a/server/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java
@@ -19,14 +19,23 @@
 
 package org.elasticsearch.index.mapper;
 
+import org.elasticsearch.Version;
+import org.elasticsearch.cluster.metadata.IndexMetaData;
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.compress.CompressedXContent;
+import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentFactory;
 import org.elasticsearch.index.mapper.MapperService.MergeReason;
 import org.elasticsearch.test.ESSingleNodeTestCase;
 
 import java.util.Arrays;
 
+import static org.elasticsearch.test.VersionUtils.randomVersionBetween;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.instanceOf;
+
 public class RootObjectMapperTests extends ESSingleNodeTestCase {
 
     public void testNumericDetection() throws Exception {
@@ -200,4 +209,188 @@ public void testIllegalDynamicTemplates() throws Exception {
                     () -> parser.parse("type", new CompressedXContent(mapping)));
             assertEquals("Dynamic template syntax error. An array of named objects is expected.", e.getMessage());
     }
+
+    public void testIllegalDynamicTemplateUnknownFieldType() throws Exception {
+        XContentBuilder mapping = XContentFactory.jsonBuilder();
+        mapping.startObject();
+        {
+            mapping.startObject("type");
+            mapping.startArray("dynamic_templates");
+            {
+                mapping.startObject();
+                mapping.startObject("my_template");
+                mapping.field("match_mapping_type", "string");
+                mapping.startObject("mapping");
+                mapping.field("type", "string");
+                mapping.endObject();
+                mapping.endObject();
+                mapping.endObject();
+            }
+            mapping.endArray();
+            mapping.endObject();
+        }
+        mapping.endObject();
+        MapperService mapperService = createIndex("test").mapperService();
+        MapperParsingException e = expectThrows(MapperParsingException.class,
+            () -> mapperService.merge("type", new CompressedXContent(Strings.toString(mapping)), MergeReason.MAPPING_UPDATE));
+        assertThat(e.getRootCause(), instanceOf(IllegalArgumentException.class));
+        assertThat(e.getRootCause().getMessage(), equalTo("No mapper found for type [string]"));
+    }
+
+    public void testIllegalDynamicTemplateUnknownAttribute() throws Exception {
+        XContentBuilder mapping = XContentFactory.jsonBuilder();
+        mapping.startObject();
+        {
+            mapping.startObject("type");
+            mapping.startArray("dynamic_templates");
+            {
+                mapping.startObject();
+                mapping.startObject("my_template");
+                mapping.field("match_mapping_type", "string");
+                mapping.startObject("mapping");
+                mapping.field("type", "keyword");
+                mapping.field("foo", "bar");
+                mapping.endObject();
+                mapping.endObject();
+                mapping.endObject();
+            }
+            mapping.endArray();
+            mapping.endObject();
+        }
+        mapping.endObject();
+        MapperService mapperService = createIndex("test").mapperService();
+        MapperParsingException e = expectThrows(MapperParsingException.class,
+            () -> mapperService.merge("type", new CompressedXContent(Strings.toString(mapping)), MergeReason.MAPPING_UPDATE));
+        assertThat(e.getRootCause(), instanceOf(IllegalArgumentException.class));
+        assertThat(e.getRootCause().getMessage(), equalTo("Unused mapping attributes [{foo=bar}]"));
+    }
+
+    public void testIllegalDynamicTemplateInvalidAttribute() throws Exception {
+        XContentBuilder mapping = XContentFactory.jsonBuilder();
+        mapping.startObject();
+        {
+            mapping.startObject("type");
+            mapping.startArray("dynamic_templates");
+            {
+                mapping.startObject();
+                mapping.startObject("my_template");
+                mapping.field("match_mapping_type", "string");
+                mapping.startObject("mapping");
+                mapping.field("type", "text");
+                mapping.field("analyzer", "foobar");
+                mapping.endObject();
+                mapping.endObject();
+                mapping.endObject();
+            }
+            mapping.endArray();
+            mapping.endObject();
+        }
+        mapping.endObject();
+        MapperService mapperService = createIndex("test").mapperService();
+        MapperParsingException e = expectThrows(MapperParsingException.class,
+            () -> mapperService.merge("type", new CompressedXContent(Strings.toString(mapping)), MergeReason.MAPPING_UPDATE));
+        assertThat(e.getRootCause(), instanceOf(MapperParsingException.class));
+        assertThat(e.getRootCause().getMessage(), equalTo("analyzer [foobar] not found for field [__dummy__]"));
+    }
+
+    public void testIllegalDynamicTemplateNoMappingType() throws Exception {
+        MapperService mapperService;
+
+        {
+            XContentBuilder mapping = XContentFactory.jsonBuilder();
+            mapping.startObject();
+            {
+                mapping.startObject("type");
+                mapping.startArray("dynamic_templates");
+                {
+                    mapping.startObject();
+                    mapping.startObject("my_template");
+                    if (randomBoolean()) {
+                        mapping.field("match_mapping_type", "*");
+                    } else {
+                        mapping.field("match", "string_*");
+                    }
+                    mapping.startObject("mapping");
+                    mapping.field("type", "{dynamic_type}");
+                    mapping.field("index_phrases", true);
+                    mapping.endObject();
+                    mapping.endObject();
+                    mapping.endObject();
+                }
+                mapping.endArray();
+                mapping.endObject();
+            }
+            mapping.endObject();
+            mapperService = createIndex("test").mapperService();
+            DocumentMapper mapper =
+                mapperService.merge("type", new CompressedXContent(Strings.toString(mapping)), MergeReason.MAPPING_UPDATE);
+            assertThat(mapper.mappingSource().toString(), containsString("\"index_phrases\":true"));
+        }
+        {
+            XContentBuilder mapping = XContentFactory.jsonBuilder();
+            mapping.startObject();
+            {
+                mapping.startObject("type");
+                mapping.startArray("dynamic_templates");
+                {
+                    mapping.startObject();
+                    mapping.startObject("my_template");
+                    if (randomBoolean()) {
+                        mapping.field("match_mapping_type", "*");
+                    } else {
+                        mapping.field("match", "string_*");
+                    }
+                    mapping.startObject("mapping");
+                    mapping.field("type", "{dynamic_type}");
+                    mapping.field("foo", "bar");
+                    mapping.endObject();
+                    mapping.endObject();
+                    mapping.endObject();
+                }
+                mapping.endArray();
+                mapping.endObject();
+            }
+            mapping.endObject();
+            MapperParsingException e = expectThrows(MapperParsingException.class,
+                () -> mapperService.merge("type", new CompressedXContent(Strings.toString(mapping)), MergeReason.MAPPING_UPDATE));
+            assertThat(e.getRootCause(), instanceOf(IllegalArgumentException.class));
+            assertThat(e.getRootCause().getMessage(), equalTo("Unused mapping attributes [{foo=bar}]"));
+        }
+    }
+
+    @Override
+    protected boolean forbidPrivateIndexSettings() {
+        return false;
+    }
+
+    public void testIllegalDynamicTemplate7DotXIndex() throws Exception {
+        XContentBuilder mapping = XContentFactory.jsonBuilder();
+        mapping.startObject();
+        {
+            mapping.startObject("type");
+            mapping.startArray("dynamic_templates");
+            {
+                mapping.startObject();
+                mapping.startObject("my_template");
+                mapping.field("match_mapping_type", "string");
+                mapping.startObject("mapping");
+                mapping.field("type", "string");
+                mapping.endObject();
+                mapping.endObject();
+                mapping.endObject();
+            }
+            mapping.endArray();
+            mapping.endObject();
+        }
+        mapping.endObject();
+        Version createdVersion = randomVersionBetween(random(), Version.V_7_0_0, Version.V_7_7_0);
+        Settings indexSettings = Settings.builder()
+            .put(IndexMetaData.SETTING_INDEX_VERSION_CREATED.getKey(), createdVersion)
+            .build();
+        MapperService mapperService = createIndex("test", indexSettings).mapperService();
+        DocumentMapper mapper = mapperService.merge("type", new CompressedXContent(Strings.toString(mapping)), MergeReason.MAPPING_UPDATE);
+        assertThat(mapper.mappingSource().toString(), containsString("\"type\":\"string\""));
+        assertWarnings("dynamic template [my_template] has invalid content [{\"match_mapping_type\":\"string\",\"mapping\":{\"type\":" +
+            "\"string\"}}], caused by [No mapper found for type [string]]");
+    }
 }

Original file line number	Diff line number	Diff line change
`@@ -688,7 +688,6 @@ private static void updateIndexMappingsAndBuildSortOrder(IndexService indexServi`
`688`	`688`	`if (!mappings.isEmpty()) {`
`689`	`689`	`assert mappings.size() == 1 : mappings;`
`690`	`690`	`mapperService.merge(MapperService.SINGLE_MAPPING_NAME, mappings, MergeReason.MAPPING_UPDATE);`
`691`		`- mapperService.validateDynamicTemplates();`
`692`	`691`	`}`
`693`	`692`
`694`	`693`	`if (sourceMetaData == null) {`
Original file line number	Diff line number	Diff line change
`@@ -717,8 +717,4 @@ public synchronized List<String> reloadSearchAnalyzers(AnalysisRegistry registry`
`717`	`717`	`return reloadedAnalyzers;`
`718`	`718`	`}`
`719`	`719`
`720`		`- public void validateDynamicTemplates() {`
`721`		`- Mapper.TypeParser.ParserContext parserContext = documentParser.parserContext();`
`722`		`- documentMapper().root().validateDynamicTemplates(parserContext);`
`723`		`- }`
`724`	`720`	`}`