Skip to content

Commit

Permalink
ITV2: disallow duplicate dynamic templates (elastic#56291)
Browse files Browse the repository at this point in the history 
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.

(cherry picked from commit eb4a557)
Signed-off-by: Andrei Dan <[email protected]>
  • Loading branch information
@andreidan
andreidan committed May 12, 2020
1 parent 2306420 commit 1f51f56
Show file tree
Hide file tree
Showing 2 changed files with 267 additions and 1 deletion.
95 changes: 94 additions & 1 deletion server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,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;
Expand Down Expand Up @@ -584,6 +585,7 @@ static Map<String, Map<String, Object>> parseV2Mappings(String mappingsJson, Lis
Map<String, Object> innerTemplateNonProperties = new HashMap<>(innerTemplateMapping);
Map<String, Object> maybeProperties = (Map<String, Object>) innerTemplateNonProperties.remove("properties");

nonProperties = removeDuplicatedDynamicTemplates(nonProperties, innerTemplateNonProperties);
XContentHelper.mergeDefaults(innerTemplateNonProperties, nonProperties);
nonProperties = innerTemplateNonProperties;

Expand All @@ -598,6 +600,7 @@ static Map<String, Map<String, Object>> parseV2Mappings(String mappingsJson, Lis
Map<String, Object> innerRequestNonProperties = new HashMap<>(innerRequestMappings);
Map<String, Object> maybeRequestProperties = (Map<String, Object>) innerRequestNonProperties.remove("properties");

nonProperties = removeDuplicatedDynamicTemplates(nonProperties, innerRequestMappings);
XContentHelper.mergeDefaults(innerRequestNonProperties, nonProperties);
nonProperties = innerRequestNonProperties;

Expand All @@ -606,11 +609,101 @@ static Map<String, Map<String, Object>> parseV2Mappings(String mappingsJson, Lis
}
}

Map<String, Object> finalMappings = new HashMap<>(nonProperties);
Map<String, Object> 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<String, Object> removeDuplicatedDynamicTemplates(Map<String, Object> previouslySeenMapping,
Map<String, Object> newMapping) {
Map<String, Object> result = new HashMap<>(previouslySeenMapping);
List<Map<String, Object>> newDynamicTemplates = (List<Map<String, Object>>) newMapping.get("dynamic_templates");
List<Map<String, Object>> previouslySeenDynamicTemplates =
(List<Map<String, Object>>) previouslySeenMapping.get("dynamic_templates");

List<Map<String, Object>> 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<Map<String, Object>> removeOverlapping(List<Map<String, Object>> first, List<Map<String, Object>> second) {
if (first == null) {
return first;
} else {
validateValuesAreMapsOfSizeOne(first);
}

if (second == null) {
return first;
} else {
validateValuesAreMapsOfSizeOne(second);
}

Set<String> 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<Map<String, Object>> second) {
for (Map<String, Object> 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<String, Object> dedupDynamicTemplates(Map<String, Object> mappings) {
Objects.requireNonNull(mappings, "deduping the dynamic templates a non-null mapping");
Map<String, Object> results = new HashMap<>(mappings);
List<Map<String, Object>> dynamicTemplates = (List<Map<String, Object>>) mappings.get("dynamic_templates");
if (dynamicTemplates == null) {
return results;
}

LinkedHashMap<String, Map<String, Object>> dedupedDynamicTemplates = new LinkedHashMap<>(dynamicTemplates.size(), 1f);
for (Map<String, Object> 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
Expand Down
173 changes: 173 additions & 0 deletions server/src/test/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexServiceTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1128,6 +1128,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<String, Object> resolved =
MetadataCreateIndexService.resolveV2Mappings("{}", state,
"index-template", new NamedXContentRegistry(Collections.emptyList()));

Map<String, Object> doc = (Map<String, Object>) resolved.get(MapperService.SINGLE_MAPPING_NAME);
List<Map<String, Object>> dynamicTemplates = (List<Map<String, Object>>) doc.get("dynamic_templates");
assertThat(dynamicTemplates.size(), is(1));
Map<String, Object> dynamicMapping = (Map<String, Object>) 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<String, Object> resolved =
MetadataCreateIndexService.resolveV2Mappings(requestMappingJson, state,
"index-template", new NamedXContentRegistry(Collections.emptyList()));

Map<String, Object> doc = (Map<String, Object>) resolved.get(MapperService.SINGLE_MAPPING_NAME);
List<Map<String, Object>> dynamicTemplates = (List<Map<String, Object>>) doc.get("dynamic_templates");
assertThat(dynamicTemplates.size(), is(1));
Map<String, Object> dynamicMapping = (Map<String, Object>) 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<String, Object> mapping = (Map<String, Object>) 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<String, Object> resolved =
MetadataCreateIndexService.resolveV2Mappings("{}", state,
"index-template", new NamedXContentRegistry(Collections.emptyList()));

Map<String, Object> doc = (Map<String, Object>) resolved.get(MapperService.SINGLE_MAPPING_NAME);
List<Map<String, Object>> dynamicTemplates = (List<Map<String, Object>>) doc.get("dynamic_templates");
assertThat(dynamicTemplates.size(), is(2));
Map<String, Object> dockerLabelsDynamicTemplate = dynamicTemplates.get(0).get("docker.container.labels") != null ?
dynamicTemplates.get(0) : dynamicTemplates.get(1);
Map<String, Object> dynamicMapping = (Map<String, Object>) 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<String, Object> mapping = (Map<String, Object>) 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<IndexTemplateMetadata.Builder> configurator) {
IndexTemplateMetadata.Builder builder = templateMetadataBuilder("template1", "te*");
configurator.accept(builder);
Expand Down

0 comments on commit 1f51f56

Please sign in to comment.