Browse Source

Add bwc for parsing mappings from dynamic templates (#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 #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 #66765
Christoph Büscher 4 years ago
parent
commit
54c8d45fa5

+ 78 - 0
server/src/internalClusterTest/java/org/elasticsearch/indices/mapping/MalformedDynamicTemplateIT.java

@@ -0,0 +1,78 @@
+/*
+ * 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.index.mapper.MapperParsingException;
+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;
+import static org.hamcrest.Matchers.containsString;
+
+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.randomPreviousCompatibleVersion(random(), Version.V_8_0_0))
+        ).setMapping(mapping).get());
+        client().prepareIndex(indexName).setSource("{\"foo\" : \"bar\"}", XContentType.JSON).get();
+        assertNoFailures((client().admin().indices().prepareRefresh(indexName)).get());
+        assertHitCount(client().prepareSearch(indexName).get(), 1);
+
+        MapperParsingException ex = expectThrows(
+            MapperParsingException.class,
+            () -> prepareCreate("malformed_dynamic_template_8.0").setSettings(
+                Settings.builder().put(indexSettings()).put("number_of_shards", 1).put("index.version.created", Version.CURRENT)
+            ).setMapping(mapping).get()
+        );
+        assertThat(ex.getMessage(), containsString("dynamic template [my_template] has invalid content"));
+    }
+
+}

+ 4 - 3
server/src/main/java/org/elasticsearch/index/mapper/DynamicFieldsBuilder.java

@@ -212,7 +212,7 @@ final class DynamicFieldsBuilder {
             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 @@ final class DynamicFieldsBuilder {
         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 + "]");

+ 32 - 19
server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java

@@ -19,23 +19,6 @@
 
 package org.elasticsearch.index.mapper;
 
-import java.io.IOException;
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.Collections;
-import java.util.Comparator;
-import java.util.HashMap;
-import java.util.Iterator;
-import java.util.LinkedHashSet;
-import java.util.List;
-import java.util.Map;
-import java.util.Objects;
-import java.util.Set;
-import java.util.function.BiFunction;
-import java.util.function.Consumer;
-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;
@@ -53,6 +36,23 @@ 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;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.HashMap;
+import java.util.Iterator;
+import java.util.LinkedHashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.function.BiFunction;
+import java.util.function.Consumer;
+import java.util.function.Function;
+import java.util.function.Supplier;
+
 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);
@@ -990,8 +990,21 @@ public abstract class FieldMapper extends Mapper implements Cloneable {
                         iterator.remove();
                         continue;
                     }
-                    throw new MapperParsingException("unknown parameter [" + propName
-                        + "] on mapper [" + name + "] of type [" + type + "]");
+                    if (parserContext.isFromDynamicTemplate() && parserContext.indexVersionCreated().before(Version.V_8_0_0)) {
+                        // 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(DeprecationCategory.API, 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 (parameter.deprecated) {
                     deprecationLogger.deprecate(DeprecationCategory.API, propName,

+ 22 - 2
server/src/main/java/org/elasticsearch/index/mapper/Mapper.java

@@ -144,6 +144,11 @@ public abstract class Mapper implements ToXContentFragment, Iterable<Mapper> {
 
             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 abstract class Mapper implements ToXContentFragment, Iterable<Mapper> {
                 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 @@ public abstract class Mapper implements ToXContentFragment, Iterable<Mapper> {
                 @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;

+ 29 - 1
server/src/test/java/org/elasticsearch/index/mapper/ParametrizedMapperTests.java

@@ -34,6 +34,7 @@ import org.elasticsearch.index.analysis.NamedAnalyzer;
 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.Collection;
@@ -196,7 +197,7 @@ public class ParametrizedMapperTests extends MapperServiceTestCase {
         }
     }
 
-    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(
             Map.of("_standard", Lucene.STANDARD_ANALYZER,
@@ -216,11 +217,18 @@ public class ParametrizedMapperTests extends MapperServiceTestCase {
             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);
     }
@@ -380,6 +388,26 @@ public class ParametrizedMapperTests extends MapperServiceTestCase {
         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 warning before 8.0.0
+     */
+    public void testBWCunknownParametersfromDynamicTemplates() {
+        String mapping = "{\"type\":\"test_mapper\",\"some_unknown_parameter\":true,\"required\":\"value\"}";
+        TestMapper mapper = fromMapping(mapping, VersionUtils.randomPreviousCompatibleVersion(random(), Version.V_8_0_0), 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));
+
+        MapperParsingException ex = expectThrows(
+            MapperParsingException.class,
+            () -> fromMapping(mapping, Version.V_8_0_0, true)
+        );
+        assertEquals("unknown parameter [some_unknown_parameter] on mapper [field] of type [test_mapper]", ex.getMessage());
+    }
+
     public void testAnalyzers() {
         String mapping = "{\"type\":\"test_mapper\",\"analyzer\":\"_standard\",\"required\":\"value\"}";
         TestMapper mapper = fromMapping(mapping);