From b4cdf86a36c3c5f2f9c2b46dffe0c6b22fdfe526 Mon Sep 17 00:00:00 2001 From: Lee Hinman Date: Wed, 22 Apr 2020 12:51:44 -0600 Subject: [PATCH 1/2] Merge V2 index/component template mappings in specific manner (#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 #53101 --- .../metadata/MetadataCreateIndexService.java | 95 ++++++++++++++++--- .../MetadataIndexTemplateService.java | 2 - .../MetadataCreateIndexServiceTests.java | 75 +++++++++++++-- .../MetadataIndexTemplateServiceTests.java | 10 +- 4 files changed, 157 insertions(+), 25 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java index 19666439dd4c9..92907623d3920 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java +++ b/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> mappings = Collections.unmodifiableMap(parseMappings(request.mappings(), + final Map> 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> 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> 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> resolveV2Mappings(final String requestMappings, + final ClusterState currentState, + final String templateName, + final NamedXContentRegistry xContentRegistry) throws Exception { + final Map> 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> parseV2Mappings(String mappingsJson, List templateMappings, + NamedXContentRegistry xContentRegistry) throws Exception { + Map requestMappings = MapperService.parseMapping(xContentRegistry, mappingsJson); + // apply templates, merging the mappings into the request mapping if exists + Map properties = new HashMap<>(); + Map nonProperties = new HashMap<>(); + for (CompressedXContent mapping : templateMappings) { + if (mapping != null) { + Map 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 innerTemplateMapping = (Map) templateMapping.get(MapperService.SINGLE_MAPPING_NAME); + Map innerTemplateNonProperties = new HashMap<>(innerTemplateMapping); + Map maybeProperties = (Map) innerTemplateNonProperties.remove("properties"); + + XContentHelper.mergeDefaults(innerTemplateNonProperties, nonProperties); + nonProperties = innerTemplateNonProperties; + + if (maybeProperties != null) { + properties.putAll(maybeProperties); + } + } + } + + if (requestMappings.get(MapperService.SINGLE_MAPPING_NAME) != null) { + Map innerRequestMappings = (Map) requestMappings.get(MapperService.SINGLE_MAPPING_NAME); + Map innerRequestNonProperties = new HashMap<>(innerRequestMappings); + Map maybeRequestProperties = (Map) innerRequestNonProperties.remove("properties"); + + XContentHelper.mergeDefaults(innerRequestNonProperties, nonProperties); + nonProperties = innerRequestNonProperties; + + if (maybeRequestProperties != null) { + properties.putAll(maybeRequestProperties); + } + } + + Map 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> parseMappings(Map requestMappings, - List> templateMappings, - NamedXContentRegistry xContentRegistry) throws Exception { + static Map> parseV1Mappings(Map requestMappings, + List> templateMappings, + NamedXContentRegistry xContentRegistry) throws Exception { Map> mappings = new HashMap<>(); for (Map.Entry entry : requestMappings.entrySet()) { Map mapping = MapperService.parseMapping(xContentRegistry, entry.getValue()); diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java index c004671a8ba00..d9186344a39fa 100644 --- 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 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 index 3229241187d4f..d720cdf848026 100644 --- a/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexServiceTests.java +++ b/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> parsedMappings = MetadataCreateIndexService.parseMappings(request.mappings(), + Map> 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> parsedMappings = MetadataCreateIndexService.parseMappings(request.mappings(), + Map> parsedMappings = MetadataCreateIndexService.parseV1Mappings(request.mappings(), Collections.singletonList(convertMappings(templateMetadata.mappings())), xContentRegistry()); List resolvedAliases = resolveAndValidateAliases(request.index(), request.aliases(), MetadataIndexTemplateService.resolveAliases(Collections.singletonList(templateMetadata)), @@ -878,7 +878,7 @@ public void testParseMappingsWithTypedTemplateAndTypelessIndexMapping() throws E } }); - Map> mappings = parseMappings(singletonMap(MapperService.SINGLE_MAPPING_NAME, "{\"_doc\":{}}"), + Map> 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> mappings = parseMappings(emptyMap(), + Map> 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> mappings = parseMappings(emptyMap(), + Map> 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 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> 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 innerResolved = (Map) resolved.get(MapperService.SINGLE_MAPPING_NAME); + assertThat("was: " + innerResolved, innerResolved.size(), equalTo(3)); + + Map nonProperties = new HashMap<>(innerResolved); + nonProperties.remove("properties"); + Map expectedNonProperties = new HashMap<>(); + expectedNonProperties.put("_source", Collections.singletonMap("enabled", false)); + Map 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 innerInnerResolved = (Map) innerResolved.get("properties"); + assertThat(innerInnerResolved.size(), equalTo(2)); + assertThat(innerInnerResolved.get("bar"), equalTo(Collections.singletonMap("type", "text"))); + Map 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 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 index 7ccf9f98fd406..f073d79616ef4 100644 --- a/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateServiceTests.java +++ b/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 { From ee5b2a829afd7929d4a700a3df0150d466079456 Mon Sep 17 00:00:00 2001 From: Lee Hinman Date: Wed, 22 Apr 2020 13:37:32 -0600 Subject: [PATCH 2/2] Fix assertion when mappings are empty --- .../cluster/metadata/MetadataCreateIndexService.java | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java index 92907623d3920..9f04012ff5a2f 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java @@ -494,8 +494,13 @@ private ClusterState applyCreateIndexRequestWithV2Template(final ClusterState cu throws Exception { logger.info("applying create index request using v2 template [{}]", templateName); - 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 String sourceMappings; + if (request.mappings().size() > 0) { + assert request.mappings().size() == 1 : "expected request metadata mappings to have 1 type but it had: " + request.mappings(); + sourceMappings = request.mappings().values().iterator().next(); + } else { + sourceMappings = "{}"; + } final Map> mappings = resolveV2Mappings(sourceMappings, currentState, templateName, xContentRegistry);