Add bwc for parsing mappings from dynamic templates (elastic#67763)

During refactoring of the mapper parsing code, the part that checks for unknown mapper parameters and throws an error when one is found was moved to FieldMapper.Builder#parse which gets also executed when applying matching dynamic templates. However, dynamic templates introduced during the 7.x versions may still contain arbitrary parameters, although we have deprecation warnings for that in place on latest 7.x now. When using these templates, indexing a new document with a new field that triggers one of these mappings to be parsed can create an error as demonstrated in elastic#66765. Instead we need to be lenient in these cases and simply ignore the unknown parameter while issuing a deprecation warning here as well. Closes elastic#66765
cbuescher · Jan 26, 2021 · 5aee5f9 · 5aee5f9
1 parent 07af846
commit 5aee5f9
Show file tree

Hide file tree

Showing 5 changed files with 148 additions and 24 deletions.
diff --git a/...nternalClusterTest/java/org/elasticsearch/indices/mapping/MalformedDynamicTemplateIT.java b/...nternalClusterTest/java/org/elasticsearch/indices/mapping/MalformedDynamicTemplateIT.java
@@ -0,0 +1,68 @@
+/*
+ * Licensed to Elasticsearch under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.elasticsearch.indices.mapping;
+
+import org.elasticsearch.Version;
+import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.common.xcontent.XContentType;
+import org.elasticsearch.test.ESIntegTestCase;
+import org.elasticsearch.test.VersionUtils;
+
+import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
+import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount;
+import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertNoFailures;
+
+public class MalformedDynamicTemplateIT extends ESIntegTestCase {
+
+    @Override
+    protected boolean forbidPrivateIndexSettings() {
+        return false;
+    }
+
+    /**
+     * Check that we can index a document into an 7.x index with a matching dynamic template that
+     * contains unknown parameters. We were able to create those templates in 7.x still, so we need
+     * to be able to index new documents into them. Indexing should issue a deprecation warning though.
+     */
+    public void testBWCMalformedDynamicTemplate() {
+        String mapping = "{ \"dynamic_templates\": [\n"
+            + "      {\n"
+            + "        \"my_template\": {\n"
+            + "          \"mapping\": {\n"
+            + "            \"ignore_malformed\": true,\n" // this parameter is not supported by "keyword" field type
+            + "            \"type\": \"keyword\"\n"
+            + "          },\n"
+            + "          \"path_match\": \"*\"\n"
+            + "        }\n"
+            + "      }\n"
+            + "    ]\n"
+            + "  }\n"
+            + "}}";
+        String indexName = "malformed_dynamic_template";
+        assertAcked(prepareCreate(indexName).setSettings(Settings.builder().put(indexSettings())
+            .put("number_of_shards", 1)
+            .put("index.version.created", VersionUtils.randomCompatibleVersion(random(), Version.CURRENT))
+        ).addMapping("_doc", mapping, XContentType.JSON).get());
+        client().prepareIndex(indexName, "_doc").setSource("{\"foo\" : \"bar\"}", XContentType.JSON).get();
+        assertNoFailures((client().admin().indices().prepareRefresh(indexName)).get());
+        assertHitCount(client().prepareSearch(indexName).get(), 1);
+    }
+
+}
diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DynamicFieldsBuilder.java b/server/src/main/java/org/elasticsearch/index/mapper/DynamicFieldsBuilder.java
@@ -212,7 +212,7 @@ private static boolean applyMatchingTemplate(ParseContext context,
             RuntimeFieldType runtimeFieldType = parser.parse(fullName, mapping, parserContext);
             Runtime.createDynamicField(runtimeFieldType, context);
         } else {
-            Mapper.Builder builder = parseMapping(name, mappingType, mapping, dateFormatter, context);
+            Mapper.Builder builder = parseDynamicTemplateMapping(name, mappingType, mapping, dateFormatter, context);
             CONCRETE.createDynamicField(builder, context);
         }
         return true;
@@ -227,15 +227,16 @@ private static Mapper.Builder findTemplateBuilderForObject(ParseContext context,
         String dynamicType = matchType.defaultMappingType();
         String mappingType = dynamicTemplate.mappingType(dynamicType);
         Map<String, Object> mapping = dynamicTemplate.mappingForName(name, dynamicType);
-        return parseMapping(name, mappingType, mapping, null, context);
+        return parseDynamicTemplateMapping(name, mappingType, mapping, null, context);
     }
 
-    private static Mapper.Builder parseMapping(String name,
+    private static Mapper.Builder parseDynamicTemplateMapping(String name,
                                                String mappingType,
                                                Map<String, Object> mapping,
                                                DateFormatter dateFormatter,
                                                ParseContext context) {
         Mapper.TypeParser.ParserContext parserContext = context.parserContext(dateFormatter);
+        parserContext = parserContext.createDynamicTemplateFieldContext(parserContext);
         Mapper.TypeParser typeParser = parserContext.typeParser(mappingType);
         if (typeParser == null) {
             throw new MapperParsingException("failed to find type parsed [" + mappingType + "] for [" + name + "]");

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java
@@ -19,6 +19,22 @@
 
 package org.elasticsearch.index.mapper;
 
+import org.apache.lucene.document.Field;
+import org.elasticsearch.Version;
+import org.elasticsearch.common.Explicit;
+import org.elasticsearch.common.TriFunction;
+import org.elasticsearch.common.logging.DeprecationLogger;
+import org.elasticsearch.common.settings.Setting;
+import org.elasticsearch.common.settings.Setting.Property;
+import org.elasticsearch.common.xcontent.ToXContent;
+import org.elasticsearch.common.xcontent.XContentBuilder;
+import org.elasticsearch.common.xcontent.XContentParser;
+import org.elasticsearch.common.xcontent.support.AbstractXContentParser;
+import org.elasticsearch.common.xcontent.support.XContentMapValues;
+import org.elasticsearch.index.analysis.NamedAnalyzer;
+import org.elasticsearch.index.mapper.FieldNamesFieldMapper.FieldNamesFieldType;
+import org.elasticsearch.index.mapper.Mapper.TypeParser.ParserContext;
+
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Arrays;
@@ -37,22 +53,6 @@
 import java.util.function.Function;
 import java.util.function.Supplier;
 
-import org.apache.lucene.document.Field;
-import org.elasticsearch.Version;
-import org.elasticsearch.common.Explicit;
-import org.elasticsearch.common.TriFunction;
-import org.elasticsearch.common.logging.DeprecationLogger;
-import org.elasticsearch.common.settings.Setting;
-import org.elasticsearch.common.settings.Setting.Property;
-import org.elasticsearch.common.xcontent.ToXContent;
-import org.elasticsearch.common.xcontent.XContentBuilder;
-import org.elasticsearch.common.xcontent.XContentParser;
-import org.elasticsearch.common.xcontent.support.AbstractXContentParser;
-import org.elasticsearch.common.xcontent.support.XContentMapValues;
-import org.elasticsearch.index.analysis.NamedAnalyzer;
-import org.elasticsearch.index.mapper.FieldNamesFieldMapper.FieldNamesFieldType;
-import org.elasticsearch.index.mapper.Mapper.TypeParser.ParserContext;
-
 public abstract class FieldMapper extends Mapper implements Cloneable {
     public static final Setting<Boolean> IGNORE_MALFORMED_SETTING =
         Setting.boolSetting("index.mapping.ignore_malformed", false, Property.IndexScope);
@@ -981,8 +981,21 @@ public final void parse(String name, ParserContext parserContext, Map<String, Ob
                         iterator.remove();
                         continue;
                     }
-                    throw new MapperParsingException("unknown parameter [" + propName
-                        + "] on mapper [" + name + "] of type [" + type + "]");
+                    if (parserContext.isFromDynamicTemplate()) {
+                        // The parameter is unknown, but this mapping is from a dynamic template.
+                        // Until 7.x it was possible to use unknown parameters there, so for bwc we need to ignore it
+                        deprecationLogger.deprecate(propName,
+                            "Parameter [{}] is used in a dynamic template mapping and has no effect on type [{}]. "
+                            + "Usage will result in an error in future major versions and should be removed.",
+                            propName,
+                            type
+                        );
+                        iterator.remove();
+                        continue;
+                    }
+                    throw new MapperParsingException(
+                        "unknown parameter [" + propName + "] on mapper [" + name + "] of type [" + type + "]"
+                    );
                 }
                 if (Objects.equals("boost", propName)) {
                     deprecationLogger.deprecate(

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/Mapper.java b/server/src/main/java/org/elasticsearch/index/mapper/Mapper.java
@@ -144,6 +144,11 @@ public DateFormatter getDateFormatter() {
 
             public boolean isWithinMultiField() { return false; }
 
+            /**
+             * true if this pars context is coming from parsing dynamic template mappings
+             */
+            public boolean isFromDynamicTemplate() { return false; }
+
             protected Function<String, SimilarityProvider> similarityLookupService() { return similarityLookupService; }
 
             /**
@@ -153,11 +158,15 @@ public ScriptService scriptService() {
                 return scriptService;
             }
 
-            public ParserContext createMultiFieldContext(ParserContext in) {
+            ParserContext createMultiFieldContext(ParserContext in) {
                 return new MultiFieldParserContext(in);
             }
 
-            static class MultiFieldParserContext extends ParserContext {
+            ParserContext createDynamicTemplateFieldContext(ParserContext in) {
+                return new DynamicTemplateParserContext(in);
+            }
+
+            private static class MultiFieldParserContext extends ParserContext {
                 MultiFieldParserContext(ParserContext in) {
                     super(in.similarityLookupService, in.typeParsers, in.runtimeTypeParsers, in.indexVersionCreated,
                         in.searchExecutionContextSupplier, in.dateFormatter, in.scriptService, in.indexAnalyzers, in.indexSettings,
@@ -167,6 +176,17 @@ static class MultiFieldParserContext extends ParserContext {
                 @Override
                 public boolean isWithinMultiField() { return true; }
             }
+
+            private static class DynamicTemplateParserContext extends ParserContext {
+                DynamicTemplateParserContext(ParserContext in) {
+                    super(in.similarityLookupService, in.typeParsers, in.runtimeTypeParsers, in.indexVersionCreated,
+                        in.searchExecutionContextSupplier, in.dateFormatter, in.scriptService, in.indexAnalyzers, in.indexSettings,
+                        in.idFieldDataEnabled, in.supportsDynamicRuntimeMappings);
+                }
+
+                @Override
+                public boolean isFromDynamicTemplate() { return true; }
+            }
         }
 
         Mapper.Builder parse(String name, Map<String, Object> node, ParserContext parserContext) throws MapperParsingException;

diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ParametrizedMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/ParametrizedMapperTests.java
@@ -34,6 +34,7 @@
 import org.elasticsearch.index.mapper.FieldMapper.Parameter;
 import org.elasticsearch.plugins.MapperPlugin;
 import org.elasticsearch.plugins.Plugin;
+import org.elasticsearch.test.VersionUtils;
 
 import java.io.IOException;
 import java.util.Arrays;
@@ -197,7 +198,7 @@ protected String contentType() {
         }
     }
 
-    private static TestMapper fromMapping(String mapping, Version version) {
+    private static TestMapper fromMapping(String mapping, Version version, boolean fromDynamicTemplate) {
         MapperService mapperService = mock(MapperService.class);
         IndexAnalyzers indexAnalyzers = new IndexAnalyzers(
             org.elasticsearch.common.collect.Map.of(
@@ -218,11 +219,18 @@ private static TestMapper fromMapping(String mapping, Version version) {
             mapperService.getIndexAnalyzers(), mapperService.getIndexSettings(), () -> {
             throw new UnsupportedOperationException();
         }, false);
+        if (fromDynamicTemplate) {
+            pc = pc.createDynamicTemplateFieldContext(pc);
+        }
         return (TestMapper) new TypeParser()
             .parse("field", XContentHelper.convertToMap(JsonXContent.jsonXContent, mapping, true), pc)
             .build(new ContentPath());
     }
 
+    private static TestMapper fromMapping(String mapping, Version version) {
+        return fromMapping(mapping, version, false);
+    }
+
     private static TestMapper fromMapping(String mapping) {
         return fromMapping(mapping, Version.CURRENT);
     }
@@ -381,6 +389,20 @@ public void testDeprecatedParameterName() {
         assertEquals("{\"field\":{\"type\":\"test_mapper\",\"fixed2\":true,\"required\":\"value\"}}", Strings.toString(mapper));
     }
 
+    /**
+     * test parsing mapping from dynamic templates, should ignore unknown parameters for bwc and log deprecation warning before 8.0.0
+     */
+    public void testBWCunknownParametersfromDynamicTemplates() {
+        String mapping = "{\"type\":\"test_mapper\",\"some_unknown_parameter\":true,\"required\":\"value\"}";
+        TestMapper mapper = fromMapping(mapping, VersionUtils.randomCompatibleVersion(random(), Version.CURRENT), true);
+        assertNotNull(mapper);
+        assertWarnings(
+            "Parameter [some_unknown_parameter] is used in a dynamic template mapping and has no effect on type [test_mapper]. "
+            + "Usage will result in an error in future major versions and should be removed."
+        );
+        assertEquals("{\"field\":{\"type\":\"test_mapper\",\"required\":\"value\"}}", Strings.toString(mapper));
+    }
+
     public void testAnalyzers() {
         String mapping = "{\"type\":\"test_mapper\",\"analyzer\":\"_standard\",\"required\":\"value\"}";
         TestMapper mapper = fromMapping(mapping);