Make dynamic template parsing less lenient. #17249

Today unknown parameters are ignored yet carried through serialization.

core/src/main/java/org/elasticsearch/index/mapper/object/DynamicTemplate.java

@@ -19,11 +19,15 @@
 
 package org.elasticsearch.index.mapper.object;
 
+import org.elasticsearch.Version;
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.regex.Regex;
+import org.elasticsearch.common.xcontent.ToXContent;
+import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.index.mapper.ContentPath;
 import org.elasticsearch.index.mapper.MapperParsingException;
 
+import java.io.IOException;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.List;
@@ -33,30 +37,41 @@ import java.util.TreeMap;
 /**
  *
  */
-public class DynamicTemplate {
+public class DynamicTemplate implements ToXContent {
 
     public static enum MatchType {
-        SIMPLE,
-        REGEX;
+        SIMPLE {
+            @Override
+            public String toString() {
+                return "simple";
+            }
+        },
+        REGEX {
+            @Override
+            public String toString() {
+                return "regex";
+            }
+        };
 
         public static MatchType fromString(String value) {
-            if ("simple".equals(value)) {
-                return SIMPLE;
-            } else if ("regex".equals(value)) {
-                return REGEX;
+            for (MatchType v : values()) {
+                if (v.toString().equals(value)) {
+                    return v;
+                }
             }
             throw new IllegalArgumentException("No matching pattern matched on [" + value + "]");
         }
     }
 
-    public static DynamicTemplate parse(String name, Map<String, Object> conf) throws MapperParsingException {
+    public static DynamicTemplate parse(String name, Map<String, Object> conf,
+            Version indexVersionCreated) throws MapperParsingException {
         String match = null;
         String pathMatch = null;
         String unmatch = null;
         String pathUnmatch = null;
         Map<String, Object> mapping = null;
         String matchMappingType = null;
-        String matchPattern = "simple";
+        String matchPattern = MatchType.SIMPLE.toString();
 
         for (Map.Entry<String, Object> entry : conf.entrySet()) {
             String propName = Strings.toUnderscoreCase(entry.getKey());
@@ -74,22 +89,18 @@ public class DynamicTemplate {
                 matchPattern = entry.getValue().toString();
             } else if ("mapping".equals(propName)) {
                 mapping = (Map<String, Object>) entry.getValue();
+            } else if (indexVersionCreated.onOrAfter(Version.V_5_0_0)) {
+                // unknown parameters were ignored before but still carried through serialization
+                // so we need to ignore them at parsing time for old indices
+                throw new IllegalArgumentException("Illegal dynamic template parameter: [" + propName + "]");
             }
         }
 
-        if (match == null && pathMatch == null && matchMappingType == null) {
-            throw new MapperParsingException("template must have match, path_match or match_mapping_type set");
-        }
-        if (mapping == null) {
-            throw new MapperParsingException("template must have mapping set");
-        }
-        return new DynamicTemplate(name, conf, pathMatch, pathUnmatch, match, unmatch, matchMappingType, MatchType.fromString(matchPattern), mapping);
+        return new DynamicTemplate(name, pathMatch, pathUnmatch, match, unmatch, matchMappingType, MatchType.fromString(matchPattern), mapping);
     }
 
     private final String name;
 
-    private final Map<String, Object> conf;
-
     private final String pathMatch;
 
     private final String pathUnmatch;
@@ -104,9 +115,14 @@ public class DynamicTemplate {
 
     private final Map<String, Object> mapping;
 
-    public DynamicTemplate(String name, Map<String, Object> conf, String pathMatch, String pathUnmatch, String match, String unmatch, String matchMappingType, MatchType matchType, Map<String, Object> mapping) {
+    public DynamicTemplate(String name, String pathMatch, String pathUnmatch, String match, String unmatch, String matchMappingType, MatchType matchType, Map<String, Object> mapping) {
+        if (match == null && pathMatch == null && matchMappingType == null) {
+            throw new MapperParsingException("template must have match, path_match or match_mapping_type set");
+        }
+        if (mapping == null) {
+            throw new MapperParsingException("template must have mapping set");
+        }
         this.name = name;
-        this.conf = new TreeMap<>(conf);
         this.pathMatch = pathMatch;
         this.pathUnmatch = pathUnmatch;
         this.match = match;
@@ -120,10 +136,6 @@ public class DynamicTemplate {
         return this.name;
     }
 
-    public Map<String, Object> conf() {
-        return this.conf;
-    }
-
     public boolean match(ContentPath path, String name, String dynamicType) {
         if (pathMatch != null && !patternMatch(pathMatch, path.pathAsText(name))) {
             return false;
@@ -148,10 +160,6 @@ public class DynamicTemplate {
         return true;
     }
 
-    public boolean hasType() {
-        return mapping.containsKey("type");
-    }
-
     public String mappingType(String dynamicType) {
         return mapping.containsKey("type") ? mapping.get("type").toString().replace("{dynamic_type}", dynamicType).replace("{dynamicType}", dynamicType) : dynamicType;
     }
@@ -200,40 +208,29 @@ public class DynamicTemplate {
     }
 
     @Override
-    public boolean equals(Object o) {
-        if (this == o) {
-            return true;
+    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
+        builder.startObject();
+        if (match != null) {
+            builder.field("match", match);
         }
-        if (o == null || getClass() != o.getClass()) {
-            return false;
+        if (pathMatch != null) {
+            builder.field("path_match", pathMatch);
         }
-
-        DynamicTemplate that = (DynamicTemplate) o;
-
-        // check if same matching, if so, replace the mapping
-        if (match != null ? !match.equals(that.match) : that.match != null) {
-            return false;
+        if (unmatch != null) {
+            builder.field("unmatch", unmatch);
         }
-        if (matchMappingType != null ? !matchMappingType.equals(that.matchMappingType) : that.matchMappingType != null) {
-            return false;
+        if (pathUnmatch != null) {
+            builder.field("path_unmatch", pathUnmatch);
         }
-        if (matchType != that.matchType) {
-            return false;
+        if (matchMappingType != null) {
+            builder.field("match_mapping_type", matchMappingType);
         }
-        if (unmatch != null ? !unmatch.equals(that.unmatch) : that.unmatch != null) {
-            return false;
+        if (matchType != MatchType.SIMPLE) {
+            builder.field("match_pattern", matchType);
         }
-
-        return true;
-    }
-
-    @Override
-    public int hashCode() {
-        // check if same matching, if so, replace the mapping
-        int result = match != null ? match.hashCode() : 0;
-        result = 31 * result + (unmatch != null ? unmatch.hashCode() : 0);
-        result = 31 * result + (matchType != null ? matchType.hashCode() : 0);
-        result = 31 * result + (matchMappingType != null ? matchMappingType.hashCode() : 0);
-        return result;
+        // use a sorted map for consistent serialization
+        builder.field("mapping", new TreeMap<>(mapping));
+        builder.endObject();
+        return builder;
     }
 }

core/src/main/java/org/elasticsearch/index/mapper/object/RootObjectMapper.java

@@ -19,6 +19,7 @@
 
 package org.elasticsearch.index.mapper.object;
 
+import org.elasticsearch.Version;
 import org.elasticsearch.common.Nullable;
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.joda.FormatDateTimeFormatter;
@@ -140,14 +141,15 @@ public class RootObjectMapper extends ObjectMapper {
                 String fieldName = Strings.toUnderscoreCase(entry.getKey());
                 Object fieldNode = entry.getValue();
                 if (parseObjectOrDocumentTypeProperties(fieldName, fieldNode, parserContext, builder)
-                        || processField(builder, fieldName, fieldNode)) {
+                        || processField(builder, fieldName, fieldNode, parserContext.indexVersionCreated())) {
                     iterator.remove();
                 }
             }
             return builder;
         }
 
-        protected boolean processField(ObjectMapper.Builder builder, String fieldName, Object fieldNode) {
+        protected boolean processField(ObjectMapper.Builder builder, String fieldName, Object fieldNode,
+                Version indexVersionCreated) {
             if (fieldName.equals("date_formats") || fieldName.equals("dynamic_date_formats")) {
                 List<FormatDateTimeFormatter> dateTimeFormatters = new ArrayList<>();
                 if (fieldNode instanceof List) {
@@ -185,7 +187,10 @@ public class RootObjectMapper extends ObjectMapper {
                         throw new MapperParsingException("A dynamic template must be defined with a name");
                     }
                     Map.Entry<String, Object> entry = tmpl.entrySet().iterator().next();
-                    ((Builder) builder).add(DynamicTemplate.parse(entry.getKey(), (Map<String, Object>) entry.getValue()));
+                    String templateName = entry.getKey();
+                    Map<String, Object> templateParams = (Map<String, Object>) entry.getValue();
+                    DynamicTemplate template = DynamicTemplate.parse(templateName, templateParams, indexVersionCreated);
+                    ((Builder) builder).add(template);
                 }
                 return true;
             } else if (fieldName.equals("date_detection")) {
@@ -329,8 +334,7 @@ public class RootObjectMapper extends ObjectMapper {
             builder.startArray("dynamic_templates");
             for (DynamicTemplate dynamicTemplate : dynamicTemplates) {
                 builder.startObject();
-                builder.field(dynamicTemplate.name());
-                builder.map(dynamicTemplate.conf());
+                builder.field(dynamicTemplate.name(), dynamicTemplate);
                 builder.endObject();
             }
             builder.endArray();

core/src/test/java/org/elasticsearch/index/mapper/DynamicTemplateTests.java

@@ -0,0 +1,93 @@
+/*
+ * 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.index.mapper;
+
+import org.elasticsearch.Version;
+import org.elasticsearch.common.xcontent.ToXContent;
+import org.elasticsearch.common.xcontent.XContentBuilder;
+import org.elasticsearch.common.xcontent.json.JsonXContent;
+import org.elasticsearch.index.mapper.object.DynamicTemplate;
+import org.elasticsearch.test.ESTestCase;
+
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+
+public class DynamicTemplateTests extends ESTestCase {
+
+    public void testParseUnknownParam() throws Exception {
+        Map<String, Object> templateDef = new HashMap<>();
+        templateDef.put("match_mapping_type", "string");
+        templateDef.put("mapping", Collections.singletonMap("store", true));
+        templateDef.put("random_param", "random_value");
+
+        IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
+                () -> DynamicTemplate.parse("my_template", templateDef, Version.V_5_0_0));
+        assertEquals("Illegal dynamic template parameter: [random_param]", e.getMessage());
+
+        // but no issues on 2.x for bw compat
+        DynamicTemplate template = DynamicTemplate.parse("my_template", templateDef, Version.V_2_3_0);
+        XContentBuilder builder = JsonXContent.contentBuilder();
+        template.toXContent(builder, ToXContent.EMPTY_PARAMS);
+        assertEquals("{\"match_mapping_type\":\"string\",\"mapping\":{\"store\":true}}", builder.string());
+    }
+
+    public void testSerialization() throws Exception {
+        // type-based template
+        Map<String, Object> templateDef = new HashMap<>();
+        templateDef.put("match_mapping_type", "string");
+        templateDef.put("mapping", Collections.singletonMap("store", true));
+        DynamicTemplate template = DynamicTemplate.parse("my_template", templateDef, Version.V_5_0_0);
+        XContentBuilder builder = JsonXContent.contentBuilder();
+        template.toXContent(builder, ToXContent.EMPTY_PARAMS);
+        assertEquals("{\"match_mapping_type\":\"string\",\"mapping\":{\"store\":true}}", builder.string());
+
+        // name-based template
+        templateDef = new HashMap<>();
+        templateDef.put("match", "*name");
+        templateDef.put("unmatch", "first_name");
+        templateDef.put("mapping", Collections.singletonMap("store", true));
+        template = DynamicTemplate.parse("my_template", templateDef, Version.V_5_0_0);
+        builder = JsonXContent.contentBuilder();
+        template.toXContent(builder, ToXContent.EMPTY_PARAMS);
+        assertEquals("{\"match\":\"*name\",\"unmatch\":\"first_name\",\"mapping\":{\"store\":true}}", builder.string());
+
+        // path-based template
+        templateDef = new HashMap<>();
+        templateDef.put("path_match", "*name");
+        templateDef.put("path_unmatch", "first_name");
+        templateDef.put("mapping", Collections.singletonMap("store", true));
+        template = DynamicTemplate.parse("my_template", templateDef, Version.V_5_0_0);
+        builder = JsonXContent.contentBuilder();
+        template.toXContent(builder, ToXContent.EMPTY_PARAMS);
+        assertEquals("{\"path_match\":\"*name\",\"path_unmatch\":\"first_name\",\"mapping\":{\"store\":true}}",
+                builder.string());
+
+        // regex matching
+        templateDef = new HashMap<>();
+        templateDef.put("match", "^a$");
+        templateDef.put("match_pattern", "regex");
+        templateDef.put("mapping", Collections.singletonMap("store", true));
+        template = DynamicTemplate.parse("my_template", templateDef, Version.V_5_0_0);
+        builder = JsonXContent.contentBuilder();
+        template.toXContent(builder, ToXContent.EMPTY_PARAMS);
+        assertEquals("{\"match\":\"^a$\",\"match_pattern\":\"regex\",\"mapping\":{\"store\":true}}", builder.string());
+    }
+}