Merge V2 index/component template mappings in specific manner (#55607)

dakrone · web-flow · commit 4574802dc54a · 2020-04-22T12:51:44.000-06:00
This commit changes the way that V2 index, component, and request mappings are merged. Specifically: - Fields are merged in a "replacement" manner, meaning that the entire definition is replaced rather than merging the interior configuration - Mapping metadata (all fields outside of `properties`) are merged recursively. The merging for V1 templates does not change. Relates to #53101
diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java
@@ -456,7 +456,7 @@ private ClusterState applyCreateIndexRequestWithV1Templates(final ClusterState c
         logger.info("applying create index request using v1 templates {}",
             templates.stream().map(IndexTemplateMetadata::name).collect(Collectors.toList()));
 
-        final Map<String, Object> mappings = Collections.unmodifiableMap(parseMappings(request.mappings(),
+        final Map<String, Object> mappings = Collections.unmodifiableMap(parseV1Mappings(request.mappings(),
             templates.stream().map(IndexTemplateMetadata::getMappings).collect(toList()), xContentRegistry));
 
         final Settings aggregatedIndexSettings =
@@ -483,8 +483,7 @@ private ClusterState applyCreateIndexRequestWithV2Template(final ClusterState cu
                                                                                     throws Exception {
         logger.info("applying create index request using v2 template [{}]", templateName);
 
-        final Map<String, Object> mappings = Collections.unmodifiableMap(parseMappings(request.mappings(),
-            MetadataIndexTemplateService.resolveMappings(currentState, templateName), xContentRegistry));
+        final Map<String, Object> mappings = resolveV2Mappings(request.mappings(), currentState, templateName, xContentRegistry);
 
         final Settings aggregatedIndexSettings =
             aggregateIndexSettings(currentState, request,
@@ -503,6 +502,15 @@ private ClusterState applyCreateIndexRequestWithV2Template(final ClusterState cu
             Collections.singletonList(templateName), metadataTransformer);
     }
 
+    static Map<String, Object> resolveV2Mappings(final String requestMappings,
+                                                 final ClusterState currentState,
+                                                 final String templateName,
+                                                 final NamedXContentRegistry xContentRegistry) throws Exception {
+        final Map<String, Object> mappings = Collections.unmodifiableMap(parseV2Mappings(requestMappings,
+            MetadataIndexTemplateService.resolveMappings(currentState, templateName), xContentRegistry));
+        return mappings;
+    }
+
     private ClusterState applyCreateIndexRequestWithExistingMetadata(final ClusterState currentState,
                                                                      final CreateIndexClusterStateUpdateRequest request,
                                                                      final boolean silent,
@@ -529,13 +537,76 @@ private ClusterState applyCreateIndexRequestWithExistingMetadata(final ClusterSt
     }
 
     /**
-     * Parses the provided mappings json and the inheritable mappings from the templates (if any) into a map.
+     * Parses the provided mappings json and the inheritable mappings from the templates (if any)
+     * into a map.
      *
-     * The template mappings are applied in the order they are encountered in the list (clients should make sure the lower index, closer
-     * to the head of the list, templates have the highest {@link IndexTemplateMetadata#order()})
+     * The template mappings are applied in the order they are encountered in the list, with the
+     * caveat that mapping fields are only merged at the top-level, meaning that field settings are
+     * not merged, instead they replace any previous field definition.
+     */
+    @SuppressWarnings("unchecked")
+    static Map<String, Object> parseV2Mappings(String mappingsJson, List<CompressedXContent> templateMappings,
+                                               NamedXContentRegistry xContentRegistry) throws Exception {
+        Map<String, Object> requestMappings = MapperService.parseMapping(xContentRegistry, mappingsJson);
+        // apply templates, merging the mappings into the request mapping if exists
+        Map<String, Object> properties = new HashMap<>();
+        Map<String, Object> nonProperties = new HashMap<>();
+        for (CompressedXContent mapping : templateMappings) {
+            if (mapping != null) {
+                Map<String, Object> templateMapping = MapperService.parseMapping(xContentRegistry, mapping.string());
+                if (templateMapping.isEmpty()) {
+                    // Someone provided an empty '{}' for mappings, which is okay, but to avoid
+                    // tripping the below assertion, we can safely ignore it
+                    continue;
+                }
+                assert templateMapping.size() == 1 : "expected exactly one mapping value, got: " + templateMapping;
+                if (templateMapping.get(MapperService.SINGLE_MAPPING_NAME) instanceof Map == false) {
+                    throw new IllegalStateException("invalid mapping definition, expected a single map underneath [" +
+                        MapperService.SINGLE_MAPPING_NAME + "] but it was: [" + templateMapping + "]");
+                }
+
+                Map<String, Object> innerTemplateMapping = (Map<String, Object>) templateMapping.get(MapperService.SINGLE_MAPPING_NAME);
+                Map<String, Object> innerTemplateNonProperties = new HashMap<>(innerTemplateMapping);
+                Map<String, Object> maybeProperties = (Map<String, Object>) innerTemplateNonProperties.remove("properties");
+
+                XContentHelper.mergeDefaults(innerTemplateNonProperties, nonProperties);
+                nonProperties = innerTemplateNonProperties;
+
+                if (maybeProperties != null) {
+                    properties.putAll(maybeProperties);
+                }
+            }
+        }
+
+        if (requestMappings.get(MapperService.SINGLE_MAPPING_NAME) != null) {
+            Map<String, Object> innerRequestMappings = (Map<String, Object>) requestMappings.get(MapperService.SINGLE_MAPPING_NAME);
+            Map<String, Object> innerRequestNonProperties = new HashMap<>(innerRequestMappings);
+            Map<String, Object> maybeRequestProperties = (Map<String, Object>) innerRequestNonProperties.remove("properties");
+
+            XContentHelper.mergeDefaults(innerRequestNonProperties, nonProperties);
+            nonProperties = innerRequestNonProperties;
+
+            if (maybeRequestProperties != null) {
+                properties.putAll(maybeRequestProperties);
+            }
+        }
+
+        Map<String, Object> finalMappings = new HashMap<>(nonProperties);
+        finalMappings.put("properties", properties);
+        return Collections.singletonMap(MapperService.SINGLE_MAPPING_NAME, finalMappings);
+    }
+
+    /**
+     * Parses the provided mappings json and the inheritable mappings from the templates (if any)
+     * into a map.
+     *
+     * The template mappings are applied in the order they are encountered in the list (clients
+     * should make sure the lower index, closer to the head of the list, templates have the highest
+     * {@link IndexTemplateMetadata#order()}). This merging makes no distinction between field
+     * definitions, as may result in an invalid field definition
      */
-    static Map<String, Object> parseMappings(String mappingsJson, List<CompressedXContent> templateMappings,
-                                             NamedXContentRegistry xContentRegistry) throws Exception {
+    static Map<String, Object> parseV1Mappings(String mappingsJson, List<CompressedXContent> templateMappings,
+                                               NamedXContentRegistry xContentRegistry) throws Exception {
         Map<String, Object> mappings = MapperService.parseMapping(xContentRegistry, mappingsJson);
         // apply templates, merging the mappings into the request mapping if exists
         for (CompressedXContent mapping : templateMappings) {
diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java
@@ -768,8 +768,6 @@ public static List<CompressedXContent> resolveMappings(final ClusterState state,
         Optional.ofNullable(template.template())
             .map(Template::mappings)
             .ifPresent(mappings::add);
-        // When actually merging mappings, the highest precedence ones should go first, so reverse the list
-        Collections.reverse(mappings);
         return Collections.unmodifiableList(mappings);
     }
 
diff --git a/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexServiceTests.java b/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexServiceTests.java
@@ -73,6 +73,7 @@
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Comparator;
+import java.util.HashMap;
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
@@ -98,7 +99,7 @@
 import static org.elasticsearch.cluster.metadata.MetadataCreateIndexService.buildIndexMetadata;
 import static org.elasticsearch.cluster.metadata.MetadataCreateIndexService.clusterStateCreateIndex;
 import static org.elasticsearch.cluster.metadata.MetadataCreateIndexService.getIndexNumberOfRoutingShards;
-import static org.elasticsearch.cluster.metadata.MetadataCreateIndexService.parseMappings;
+import static org.elasticsearch.cluster.metadata.MetadataCreateIndexService.parseV1Mappings;
 import static org.elasticsearch.cluster.metadata.MetadataCreateIndexService.resolveAndValidateAliases;
 import static org.elasticsearch.cluster.shards.ClusterShardLimitIT.ShardCounts.forDataNodeCount;
 import static org.elasticsearch.index.IndexSettings.INDEX_SOFT_DELETES_SETTING;
@@ -622,7 +623,7 @@ public void testParseMappingsAppliesDataFromTemplateAndRequest() throws Exceptio
         });
         request.mappings(createMapping("mapping_from_request", "text").string());
 
-        Map<String, Object> parsedMappings = MetadataCreateIndexService.parseMappings(request.mappings(),
+        Map<String, Object> parsedMappings = MetadataCreateIndexService.parseV1Mappings(request.mappings(),
             List.of(templateMetadata.getMappings()), NamedXContentRegistry.EMPTY);
 
         assertThat(parsedMappings, hasKey("_doc"));
@@ -678,7 +679,7 @@ public void testRequestDataHavePriorityOverTemplateData() throws Exception {
         request.aliases(Set.of(new Alias("alias").searchRouting("fromRequest")));
         request.settings(Settings.builder().put("key1", "requestValue").build());
 
-        Map<String, Object> parsedMappings = MetadataCreateIndexService.parseMappings(request.mappings(),
+        Map<String, Object> parsedMappings = MetadataCreateIndexService.parseV1Mappings(request.mappings(),
             List.of(templateMetadata.mappings()), xContentRegistry());
         List<AliasMetadata> resolvedAliases = resolveAndValidateAliases(request.index(), request.aliases(),
             MetadataIndexTemplateService.resolveAliases(List.of(templateMetadata)),
@@ -848,7 +849,7 @@ public void testParseMappingsWithTypedTemplateAndTypelessIndexMapping() throws E
             }
         });
 
-        Map<String, Object> mappings = parseMappings("{\"_doc\":{}}", List.of(templateMetadata.mappings()), xContentRegistry());
+        Map<String, Object> mappings = parseV1Mappings("{\"_doc\":{}}", List.of(templateMetadata.mappings()), xContentRegistry());
         assertThat(mappings, Matchers.hasKey(MapperService.SINGLE_MAPPING_NAME));
     }
 
@@ -861,7 +862,7 @@ public void testParseMappingsWithTypedTemplate() throws Exception {
                 ExceptionsHelper.reThrowIfNotNull(e);
             }
         });
-        Map<String, Object> mappings = parseMappings("", List.of(templateMetadata.mappings()), xContentRegistry());
+        Map<String, Object> mappings = parseV1Mappings("", List.of(templateMetadata.mappings()), xContentRegistry());
         assertThat(mappings, Matchers.hasKey(MapperService.SINGLE_MAPPING_NAME));
     }
 
@@ -873,7 +874,7 @@ public void testParseMappingsWithTypelessTemplate() throws Exception {
                 ExceptionsHelper.reThrowIfNotNull(e);
             }
         });
-        Map<String, Object> mappings = parseMappings("", List.of(templateMetadata.mappings()), xContentRegistry());
+        Map<String, Object> mappings = parseV1Mappings("", List.of(templateMetadata.mappings()), xContentRegistry());
         assertThat(mappings, Matchers.hasKey(MapperService.SINGLE_MAPPING_NAME));
     }
 
@@ -983,6 +984,69 @@ public void testDeprecateTranslogRetentionSettings() {
             + "and [index.translog.retention.size] are deprecated and effectively ignored. They will be removed in a future version.");
     }
 
+    @SuppressWarnings("unchecked")
+    public void testMappingsMergingIsSmart() throws Exception {
+        Template ctt1 = new Template(null,
+            new CompressedXContent("{\"_doc\":{\"_source\":{\"enabled\": false},\"_meta\":{\"ct1\":{\"ver\": \"text\"}}," +
+                "\"properties\":{\"foo\":{\"type\":\"text\",\"ignore_above\":7,\"analyzer\":\"english\"}}}}"), null);
+        Template ctt2 = new Template(null,
+            new CompressedXContent("{\"_doc\":{\"_meta\":{\"ct1\":{\"ver\": \"keyword\"},\"ct2\":\"potato\"}," +
+                "\"properties\":{\"foo\":{\"type\":\"keyword\",\"ignore_above\":13}}}}"), null);
+
+        ComponentTemplate ct1 = new ComponentTemplate(ctt1, null, null);
+        ComponentTemplate ct2 = new ComponentTemplate(ctt2, null, null);
+
+        boolean shouldBeText = randomBoolean();
+        List<String> composedOf = shouldBeText ? Arrays.asList("ct2", "ct1") : Arrays.asList("ct1", "ct2");
+        logger.info("--> the {} analyzer should win ({})", shouldBeText ? "text" : "keyword", composedOf);
+        IndexTemplateV2 template = new IndexTemplateV2(Collections.singletonList("index"), null, composedOf, null, null, null);
+
+        ClusterState state = ClusterState.builder(ClusterState.EMPTY_STATE)
+            .metadata(Metadata.builder(Metadata.EMPTY_METADATA)
+                .put("ct1", ct1)
+                .put("ct2", ct2)
+                .put("index-template", template)
+                .build())
+            .build();
+
+        Map<String, Object> resolved =
+            MetadataCreateIndexService.resolveV2Mappings("{\"_doc\":{\"_meta\":{\"ct2\":\"eggplant\"}," +
+                    "\"properties\":{\"bar\":{\"type\":\"text\"}}}}", state,
+                "index-template", new NamedXContentRegistry(Collections.emptyList()));
+
+        assertThat("expected exactly one type but was: " + resolved, resolved.size(), equalTo(1));
+        Map<String, Object> innerResolved = (Map<String, Object>) resolved.get(MapperService.SINGLE_MAPPING_NAME);
+        assertThat("was: " + innerResolved, innerResolved.size(), equalTo(3));
+
+        Map<String, Object> nonProperties = new HashMap<>(innerResolved);
+        nonProperties.remove("properties");
+        Map<String, Object> expectedNonProperties = new HashMap<>();
+        expectedNonProperties.put("_source", Collections.singletonMap("enabled", false));
+        Map<String, Object> meta = new HashMap<>();
+        meta.put("ct2", "eggplant");
+        if (shouldBeText) {
+            meta.put("ct1", Collections.singletonMap("ver", "text"));
+        } else {
+            meta.put("ct1", Collections.singletonMap("ver", "keyword"));
+        }
+        expectedNonProperties.put("_meta", meta);
+        assertThat(nonProperties, equalTo(expectedNonProperties));
+
+        Map<String, Object> innerInnerResolved = (Map<String, Object>) innerResolved.get("properties");
+        assertThat(innerInnerResolved.size(), equalTo(2));
+        assertThat(innerInnerResolved.get("bar"), equalTo(Collections.singletonMap("type", "text")));
+        Map<String, Object> fooMappings = new HashMap<>();
+        if (shouldBeText) {
+            fooMappings.put("type", "text");
+            fooMappings.put("ignore_above", 7);
+            fooMappings.put("analyzer", "english");
+        } else {
+            fooMappings.put("type", "keyword");
+            fooMappings.put("ignore_above", 13);
+        }
+        assertThat(innerInnerResolved.get("foo"), equalTo(fooMappings));
+    }
+
     private IndexTemplateMetadata addMatchingTemplate(Consumer<IndexTemplateMetadata.Builder> configurator) {
         IndexTemplateMetadata.Builder builder = templateMetadataBuilder("template1", "te*");
         configurator.accept(builder);
diff --git a/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateServiceTests.java b/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateServiceTests.java
@@ -696,16 +696,16 @@ public void testResolveMappings() throws Exception {
             .collect(Collectors.toList());
 
         // The order of mappings should be:
-        // - index template
-        // - ct_high
         // - ct_low
-        // Because the first elements when merging mappings have the highest precedence
+        // - ct_high
+        // - index template
+        // Because the first elements when merging mappings have the lowest precedence
         assertThat(parsedMappings.get(0),
-            equalTo(Map.of("_doc", Map.of("properties", Map.of("field", Map.of("type", "keyword"))))));
+            equalTo(Map.of("_doc", Map.of("properties", Map.of("field2", Map.of("type", "text"))))));
         assertThat(parsedMappings.get(1),
             equalTo(Map.of("_doc", Map.of("properties", Map.of("field2", Map.of("type", "keyword"))))));
         assertThat(parsedMappings.get(2),
-            equalTo(Map.of("_doc", Map.of("properties", Map.of("field2", Map.of("type", "text"))))));
+            equalTo(Map.of("_doc", Map.of("properties", Map.of("field", Map.of("type", "keyword"))))));
     }
 
     public void testResolveSettings() throws Exception {

Original file line number	Diff line number	Diff line change
`@@ -768,8 +768,6 @@ public static List<CompressedXContent> resolveMappings(final ClusterState state,`
`768`	`768`	`Optional.ofNullable(template.template())`
`769`	`769`	`.map(Template::mappings)`
`770`	`770`	`.ifPresent(mappings::add);`
`771`		`- // When actually merging mappings, the highest precedence ones should go first, so reverse the list`
`772`		`- Collections.reverse(mappings);`
`773`	`771`	`return Collections.unmodifiableList(mappings);`
`774`	`772`	`}`
`775`	`773`