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

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 elastic#53101

Author: dakrone

server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java

@@ -457,7 +457,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, Map<String, Object>> mappings = Collections.unmodifiableMap(parseMappings(request.mappings(),
+        final Map<String, Map<String, Object>> mappings = Collections.unmodifiableMap(parseV1Mappings(request.mappings(),
             templates.stream().map(IndexTemplateMetadata::getMappings)
                 // Converts the ImmutableOpenMap into a non-terrible HashMap
                 .map(iom -> {
@@ -494,11 +494,10 @@ private ClusterState applyCreateIndexRequestWithV2Template(final ClusterState cu
                                                                                     throws Exception {
         logger.info("applying create index request using v2 template [{}]", templateName);
 
-        final Map<String, Map<String, Object>> mappings = Collections.unmodifiableMap(parseMappings(request.mappings(),
-            MetadataIndexTemplateService.resolveMappings(currentState, templateName).stream()
-                .map(compressedMapping -> Collections.singletonMap(MapperService.SINGLE_MAPPING_NAME, compressedMapping))
-                .collect(toList()),
-            xContentRegistry));
+        assert request.mappings().size() == 1 : "expected request metadata mappings to have 1 type but it had: " + request.mappings();
+        String sourceMappings = request.mappings().values().iterator().next();
+        final Map<String, Map<String, Object>> mappings = resolveV2Mappings(sourceMappings,
+            currentState, templateName, xContentRegistry);
 
         final Settings aggregatedIndexSettings =
             aggregateIndexSettings(currentState, request,
@@ -517,6 +516,15 @@ private ClusterState applyCreateIndexRequestWithV2Template(final ClusterState cu
             Collections.singletonList(templateName), metadataTransformer);
     }
 
+    static Map<String, Map<String, Object>> resolveV2Mappings(final String requestMappings,
+                                                              final ClusterState currentState,
+                                                              final String templateName,
+                                                              final NamedXContentRegistry xContentRegistry) throws Exception {
+        final Map<String, 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,
@@ -551,14 +559,77 @@ 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, 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, Map<String, Object>> parseMappings(Map<String, String> requestMappings,
-                                                          List<Map<String, CompressedXContent>> templateMappings,
-                                                          NamedXContentRegistry xContentRegistry) throws Exception {
+    static Map<String, Map<String, Object>> parseV1Mappings(Map<String, String> requestMappings,
+                                                            List<Map<String, CompressedXContent>> templateMappings,
+                                                            NamedXContentRegistry xContentRegistry) throws Exception {
         Map<String, Map<String, Object>> mappings = new HashMap<>();
         for (Map.Entry<String, String> entry : requestMappings.entrySet()) {
             Map<String, Object> mapping = MapperService.parseMapping(xContentRegistry, entry.getValue());

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);
     }
 

server/src/test/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexServiceTests.java

@@ -104,7 +104,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;
@@ -647,7 +647,7 @@ public void testParseMappingsAppliesDataFromTemplateAndRequest() throws Exceptio
         });
         request.mappings(singletonMap("type", createMapping("mapping_from_request", "text").string()));
 
-        Map<String, Map<String, Object>> parsedMappings = MetadataCreateIndexService.parseMappings(request.mappings(),
+        Map<String, Map<String, Object>> parsedMappings = MetadataCreateIndexService.parseV1Mappings(request.mappings(),
             Collections.singletonList(convertMappings(templateMetadata.getMappings())), NamedXContentRegistry.EMPTY);
 
         assertThat(parsedMappings, hasKey("type"));
@@ -705,7 +705,7 @@ public void testRequestDataHavePriorityOverTemplateData() throws Exception {
         request.aliases(singleton(new Alias("alias").searchRouting("fromRequest")));
         request.settings(Settings.builder().put("key1", "requestValue").build());
 
-        Map<String, Map<String, Object>> parsedMappings = MetadataCreateIndexService.parseMappings(request.mappings(),
+        Map<String, Map<String, Object>> parsedMappings = MetadataCreateIndexService.parseV1Mappings(request.mappings(),
             Collections.singletonList(convertMappings(templateMetadata.mappings())), xContentRegistry());
         List<AliasMetadata> resolvedAliases = resolveAndValidateAliases(request.index(), request.aliases(),
             MetadataIndexTemplateService.resolveAliases(Collections.singletonList(templateMetadata)),
@@ -878,7 +878,7 @@ public void testParseMappingsWithTypedTemplateAndTypelessIndexMapping() throws E
             }
         });
 
-        Map<String, Map<String, Object>> mappings = parseMappings(singletonMap(MapperService.SINGLE_MAPPING_NAME, "{\"_doc\":{}}"),
+        Map<String, Map<String, Object>> mappings = parseV1Mappings(singletonMap(MapperService.SINGLE_MAPPING_NAME, "{\"_doc\":{}}"),
             Collections.singletonList(convertMappings(templateMetadata.mappings())), xContentRegistry());
         assertThat(mappings, Matchers.hasKey(MapperService.SINGLE_MAPPING_NAME));
     }
@@ -892,7 +892,7 @@ public void testParseMappingsWithTypedTemplate() throws Exception {
                 ExceptionsHelper.reThrowIfNotNull(e);
             }
         });
-        Map<String, Map<String, Object>> mappings = parseMappings(emptyMap(),
+        Map<String, Map<String, Object>> mappings = parseV1Mappings(emptyMap(),
             Collections.singletonList(convertMappings(templateMetadata.mappings())), xContentRegistry());
         assertThat(mappings, Matchers.hasKey("type"));
     }
@@ -905,7 +905,7 @@ public void testParseMappingsWithTypelessTemplate() throws Exception {
                 ExceptionsHelper.reThrowIfNotNull(e);
             }
         });
-        Map<String, Map<String, Object>> mappings = parseMappings(emptyMap(),
+        Map<String, Map<String, Object>> mappings = parseV1Mappings(emptyMap(),
             Collections.singletonList(convertMappings(templateMetadata.mappings())), xContentRegistry());
         assertThat(mappings, Matchers.hasKey(MapperService.SINGLE_MAPPING_NAME));
     }
@@ -1001,6 +1001,69 @@ public void testValidateTranslogRetentionSettings() {
             + "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, 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);

server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateServiceTests.java

@@ -699,19 +699,19 @@ 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(Collections.singletonMap("_doc", Collections.singletonMap("properties",
-                Collections.singletonMap("field", Collections.singletonMap("type", "keyword"))))));
+                Collections.singletonMap("field2", Collections.singletonMap("type", "text"))))));
         assertThat(parsedMappings.get(1),
             equalTo(Collections.singletonMap("_doc", Collections.singletonMap("properties",
                 Collections.singletonMap("field2", Collections.singletonMap("type", "keyword"))))));
         assertThat(parsedMappings.get(2),
             equalTo(Collections.singletonMap("_doc", Collections.singletonMap("properties",
-                Collections.singletonMap("field2", Collections.singletonMap("type", "text"))))));
+                Collections.singletonMap("field", Collections.singletonMap("type", "keyword"))))));
     }
 
     public void testResolveSettings() throws Exception {