From eb4a557d7079d1678eed7f14b7cdc5797dfbc8ad Mon Sep 17 00:00:00 2001 From: Andrei Dan Date: Tue, 12 May 2020 14:24:05 +0100 Subject: [PATCH] ITV2: disallow duplicate dynamic templates (#56291) Dynamic templates can contain multiple templates with the same name. This commit changes the way V2 index templates mappings are resolved to add deduplication of dynamic templates when resolving the mappings, by having the last dynamic templates override the ones defined with the same name earlier in the `dynamic_templates` array. This also filters duplicate dynamic templates when merging different component templates that specify the same dynamic template and when merging the mappings specified in the request with the ones in the index template. --- .../metadata/MetadataCreateIndexService.java | 95 +++++++++- .../MetadataCreateIndexServiceTests.java | 173 ++++++++++++++++++ 2 files changed, 267 insertions(+), 1 deletion(-) 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 ea45338be2908..5724b76af17b4 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java @@ -82,6 +82,7 @@ import java.util.Collection; import java.util.Collections; import java.util.HashMap; +import java.util.LinkedHashMap; import java.util.List; import java.util.Locale; import java.util.Map; @@ -561,6 +562,7 @@ static Map parseV2Mappings(String mappingsJson, List innerTemplateNonProperties = new HashMap<>(innerTemplateMapping); Map maybeProperties = (Map) innerTemplateNonProperties.remove("properties"); + nonProperties = removeDuplicatedDynamicTemplates(nonProperties, innerTemplateNonProperties); XContentHelper.mergeDefaults(innerTemplateNonProperties, nonProperties); nonProperties = innerTemplateNonProperties; @@ -575,6 +577,7 @@ static Map parseV2Mappings(String mappingsJson, List innerRequestNonProperties = new HashMap<>(innerRequestMappings); Map maybeRequestProperties = (Map) innerRequestNonProperties.remove("properties"); + nonProperties = removeDuplicatedDynamicTemplates(nonProperties, innerRequestMappings); XContentHelper.mergeDefaults(innerRequestNonProperties, nonProperties); nonProperties = innerRequestNonProperties; @@ -583,11 +586,101 @@ static Map parseV2Mappings(String mappingsJson, List finalMappings = new HashMap<>(nonProperties); + Map finalMappings = dedupDynamicTemplates(nonProperties); finalMappings.put("properties", properties); return Collections.singletonMap(MapperService.SINGLE_MAPPING_NAME, finalMappings); } + /** + * Removes the already seen/processed dynamic templates from the previouslySeenMapping if they are defined (we're + * identifying the dynamic templates based on the name only, *not* on the full definition) in the newMapping we are about to + * process (and merge) + */ + @SuppressWarnings("unchecked") + private static Map removeDuplicatedDynamicTemplates(Map previouslySeenMapping, + Map newMapping) { + Map result = new HashMap<>(previouslySeenMapping); + List> newDynamicTemplates = (List>) newMapping.get("dynamic_templates"); + List> previouslySeenDynamicTemplates = + (List>) previouslySeenMapping.get("dynamic_templates"); + + List> filteredDynamicTemplates = removeOverlapping(previouslySeenDynamicTemplates, newDynamicTemplates); + + // if we removed any mappings from the previously seen ones, we'll re-add them on merge time, see + // {@link XContentHelper#mergeDefaults}, so update the result to contain the filtered ones + if (filteredDynamicTemplates != previouslySeenDynamicTemplates) { + result.put("dynamic_templates", filteredDynamicTemplates); + } + return result; + } + + /** + * Removes all the items from the first list that are already present in the second list + * + * Similar to {@link List#removeAll(Collection)} but the list parameters are not modified. + * + * This expects both list values to be Maps of size one and the "contains" operation that will determine if a value + * from the second list is present in the first list (and be removed from the first list) is based on key name. + * + * eg. + * removeAll([ {"key1" : {}}, {"key2" : {}} ], [ {"key1" : {}}, {"key3" : {}} ]) + * Returns: + * [ {"key2" : {}} ] + */ + private static List> removeOverlapping(List> first, List> second) { + if (first == null) { + return first; + } else { + validateValuesAreMapsOfSizeOne(first); + } + + if (second == null) { + return first; + } else { + validateValuesAreMapsOfSizeOne(second); + } + + Set keys = second.stream() + .map(value -> value.keySet().iterator().next()) + .collect(Collectors.toSet()); + + return first.stream().filter(value -> keys.contains(value.keySet().iterator().next()) == false).collect(toList()); + } + + private static void validateValuesAreMapsOfSizeOne(List> second) { + for (Map map : second) { + // all are in the form of [ {"key1" : {}}, {"key2" : {}} ] + if (map.size() != 1) { + throw new IllegalArgumentException("unexpected argument, expected maps with one key, but got " + map); + } + } + } + + /** + * Parses the `dynamic_templates` from the provided mappings, if any are configured, and returns a mappings map containing dynamic + * templates with unique names. + * + * The later templates in the provided mapping's `dynamic_templates` array will override the templates with the same name defined + * earlier in the `dynamic_templates` array. + */ + @SuppressWarnings("unchecked") + private static Map dedupDynamicTemplates(Map mappings) { + Objects.requireNonNull(mappings, "deduping the dynamic templates a non-null mapping"); + Map results = new HashMap<>(mappings); + List> dynamicTemplates = (List>) mappings.get("dynamic_templates"); + if (dynamicTemplates == null) { + return results; + } + + LinkedHashMap> dedupedDynamicTemplates = new LinkedHashMap<>(dynamicTemplates.size(), 1f); + for (Map dynamicTemplate : dynamicTemplates) { + dedupedDynamicTemplates.put(dynamicTemplate.keySet().iterator().next(), dynamicTemplate); + } + + results.put("dynamic_templates", new ArrayList<>(dedupedDynamicTemplates.values())); + return results; + } + /** * Add the objects in the second map to the first, where the keys in the {@code second} map have * higher predecence and overwrite the keys in the {@code first} map. In the event of a key with 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 ecf23fffaeed9..4b5965c24c6fe 100644 --- a/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexServiceTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexServiceTests.java @@ -1111,6 +1111,179 @@ public void testMergeIgnoringDots() throws Exception { assertThat(results, equalTo(expected)); } + @SuppressWarnings("unchecked") + public void testDedupTemplateDynamicTemplates() throws Exception { + Template template = new Template(null, + new CompressedXContent("{\"_doc\":{\"_source\":{\"enabled\": false}, \"dynamic_templates\": [" + + "{\n" + + " \"docker.container.labels\": {\n" + + " \"mapping\": {\n" + + " \"type\": \"keyword\"\n" + + " },\n" + + " \"match_mapping_type\": \"string\",\n" + + " \"path_match\": \"labels.*\"\n" + + " }\n" + + " },\n" + + " {\n" + + " \"docker.container.labels\": {\n" + + " \"mapping\": {\n" + + " \"type\": \"keyword\"\n" + + " },\n" + + " \"match_mapping_type\": \"string\",\n" + + " \"path_match\": \"docker.container.labels.*\"\n" + + " }\n" + + "}]}}"), null); + + IndexTemplateV2 indexTemplate = new IndexTemplateV2(Collections.singletonList("index"), template, null, null, null, null); + + ClusterState state = ClusterState.builder(ClusterState.EMPTY_STATE) + .metadata(Metadata.builder(Metadata.EMPTY_METADATA) + .put("index-template", indexTemplate) + .build()) + .build(); + + Map resolved = + MetadataCreateIndexService.resolveV2Mappings("{}", state, + "index-template", new NamedXContentRegistry(Collections.emptyList())); + + Map doc = (Map) resolved.get(MapperService.SINGLE_MAPPING_NAME); + List> dynamicTemplates = (List>) doc.get("dynamic_templates"); + assertThat(dynamicTemplates.size(), is(1)); + Map dynamicMapping = (Map) dynamicTemplates.get(0).get("docker.container.labels"); + assertThat(dynamicMapping, is(notNullValue())); + assertThat("last mapping with the same name must override previously defined mappings with the same name", + dynamicMapping.get("path_match"), is("docker.container.labels.*")); + } + + public void testDedupRequestDynamicTemplates() throws Exception { + String requestMappingJson = "{\"_doc\":{\"_source\":{\"enabled\": false}, \"dynamic_templates\": [" + + "{\n" + + " \"docker.container.labels\": {\n" + + " \"mapping\": {\n" + + " \"type\": \"keyword\"\n" + + " },\n" + + " \"match_mapping_type\": \"string\",\n" + + " \"path_match\": \"labels.*\"\n" + + " }\n" + + " },\n" + + " {\n" + + " \"docker.container.labels\": {\n" + + " \"mapping\": {\n" + + " \"type\": \"keyword\"\n" + + " },\n" + + " \"match_mapping_type\": \"string\",\n" + + " \"path_match\": \"source.request.*\"\n" + + " }\n" + + "}]}}"; + + String templateMappingJson = "{\"_doc\":{\"_source\":{\"enabled\": false}, \"dynamic_templates\": [" + + "{\n" + + " \"docker.container.labels\": {\n" + + " \"mapping\": {\n" + + " \"type\": \"text\",\n" + + " \"copy_to\": \"text_labels\"\n" + + " },\n" + + " \"match_mapping_type\": \"string\",\n" + + " \"path_match\": \"source.template.*\"\n" + + " }\n" + + " }\n" + + "]}}"; + Template template = new Template(null, new CompressedXContent(templateMappingJson), null); + + IndexTemplateV2 indexTemplate = new IndexTemplateV2(Collections.singletonList("index"), template, null, null, null, null); + + ClusterState state = ClusterState.builder(ClusterState.EMPTY_STATE) + .metadata(Metadata.builder(Metadata.EMPTY_METADATA) + .put("index-template", indexTemplate) + .build()) + .build(); + + Map resolved = + MetadataCreateIndexService.resolveV2Mappings(requestMappingJson, state, + "index-template", new NamedXContentRegistry(Collections.emptyList())); + + Map doc = (Map) resolved.get(MapperService.SINGLE_MAPPING_NAME); + List> dynamicTemplates = (List>) doc.get("dynamic_templates"); + assertThat(dynamicTemplates.size(), is(1)); + Map dynamicMapping = (Map) dynamicTemplates.get(0).get("docker.container.labels"); + assertThat(dynamicMapping, is(notNullValue())); + assertThat("last mapping with the same name must override previously defined mappings with the same name", + dynamicMapping.get("path_match"), is("source.request.*")); + Map mapping = (Map) dynamicMapping.get("mapping"); + assertThat("the dynamic template defined in the request must not be merged with the dynamic template with the " + + "same name defined in the index template", mapping.size(), is(1)); + assertThat(mapping.get("type"), is("keyword")); + } + + public void testMultipleComponentTemplatesDefineSameDynamicTemplate() throws Exception { + String ct1Mapping = "{\"_doc\":{\"_source\":{\"enabled\": false}, \"dynamic_templates\": [" + + "{\n" + + " \"docker.container.labels\": {\n" + + " \"mapping\": {\n" + + " \"type\": \"text\",\n" + + " \"copy_to\": \"text_labels\"\n" + + " },\n" + + " \"match_mapping_type\": \"string\",\n" + + " \"path_match\": \"source.first.ct.*\"\n" + + " }\n" + + " },\n" + + "{\n" + + " \"other.labels\": {\n" + + " \"mapping\": {\n" + + " \"type\": \"keyword\"\n" + + " },\n" + + " \"match_mapping_type\": \"string\",\n" + + " \"path_match\": \"source.first.ct.other.labels*\"\n" + + " }\n" + + " }\n" + + "]}}"; + String ct2Mapping = "{\"_doc\":{\"_source\":{\"enabled\": false}, \"dynamic_templates\": [" + + "{\n" + + " \"docker.container.labels\": {\n" + + " \"mapping\": {\n" + + " \"type\": \"keyword\"\n" + + " },\n" + + " \"match_mapping_type\": \"string\",\n" + + " \"path_match\": \"source.second.ct.*\"\n" + + " }\n" + + " }\n" + + "]}}"; + + Template ctt1 = new Template(null, new CompressedXContent(ct1Mapping), null); + Template ctt2 = new Template(null, new CompressedXContent(ct2Mapping), null); + ComponentTemplate ct1 = new ComponentTemplate(ctt1, null, null); + ComponentTemplate ct2 = new ComponentTemplate(ctt2, null, null); + + IndexTemplateV2 template = new IndexTemplateV2(Collections.singletonList("index"), null, Arrays.asList("ct1", "ct2"), + 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("{}", state, + "index-template", new NamedXContentRegistry(Collections.emptyList())); + + Map doc = (Map) resolved.get(MapperService.SINGLE_MAPPING_NAME); + List> dynamicTemplates = (List>) doc.get("dynamic_templates"); + assertThat(dynamicTemplates.size(), is(2)); + Map dockerLabelsDynamicTemplate = dynamicTemplates.get(0).get("docker.container.labels") != null ? + dynamicTemplates.get(0) : dynamicTemplates.get(1); + Map dynamicMapping = (Map) dockerLabelsDynamicTemplate.get("docker.container.labels"); + assertThat(dynamicMapping, is(notNullValue())); + assertThat("dynamic template defined in the last defined component template must override the previously defined dynamic templates", + dynamicMapping.get("path_match"), is("source.second.ct.*")); + Map mapping = (Map) dynamicMapping.get("mapping"); + assertThat("the dynamic template defined in the second component template must not be merged with the dynamic template with the " + + "same name defined in the first component template", mapping.size(), is(1)); + assertThat(mapping.get("type"), is("keyword")); + } + private IndexTemplateMetadata addMatchingTemplate(Consumer configurator) { IndexTemplateMetadata.Builder builder = templateMetadataBuilder("template1", "te*"); configurator.accept(builder);