Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Apr 22, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 79 additions & 8 deletions server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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");
Comment on lines +569 to +570
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

innerTemplateMapping is only used to initialize innerTemplateNonProperties

Would it make sense for these statements to be executed in reversed order to avoid over allocating innerTemplateNonProperties just to immediately remove "properties" ?

ie.

Map<String, Object> maybeProperties = (Map<String, Object>) innerTemplateMapping.remove("properties");
Map<String, Object> innerTemplateNonProperties = new HashMap<>(innerTemplateMapping);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, ignore this actually, as we'd be modifying a map we didn't create. It's fine as it is


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) {
Expand Down
2 changes: 0 additions & 2 deletions server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -767,8 +767,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);
}

Expand Down
76 changes: 70 additions & 6 deletions server/src/test/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexServiceTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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"));
Expand Down Expand Up @@ -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)),
Expand Down Expand Up @@ -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));
}

Expand All @@ -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));
}

Expand All @@ -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));
}

Expand Down Expand Up @@ -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);
Expand Down
10 changes: 5 additions & 5 deletions ...r/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateServiceTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -677,16 +677,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 {
Expand Down