From 76ab2631a6db2024ff22a53b33b8145dbc9d5334 Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Mon, 3 Jul 2023 12:19:01 +0300 Subject: [PATCH 01/38] Fix composable templates with subobjects: false --- .../15_composition.yml | 34 ++++++++ .../MetadataIndexTemplateService.java | 4 +- .../index/mapper/MapperService.java | 79 ++++++++++++++++--- .../index/mapper/MappingParser.java | 44 +++++++---- .../index/mapper/ObjectMapper.java | 24 +++++- .../index/mapper/RootObjectMapper.java | 12 ++- .../MetadataIndexTemplateServiceTests.java | 55 +++++++++++++ 7 files changed, 221 insertions(+), 31 deletions(-) diff --git a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.put_index_template/15_composition.yml b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.put_index_template/15_composition.yml index 2aaf492f0ff0d..d048ac1deb49f 100644 --- a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.put_index_template/15_composition.yml +++ b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.put_index_template/15_composition.yml @@ -349,3 +349,37 @@ index_patterns: ["purple-index"] composed_of: ["red", "blue"] ignore_missing_component_templates: ["foo"] + +--- +"Composable index templates that include subobjects: false": + - skip: + version: ' - 8.9.1' + reason: 'https://github.com/elastic/elasticsearch/issues/96768 fixed after 8.9.0' + + - do: + cluster.put_component_template: + name: test-subobjects + body: + template: + mappings: + subobjects: false + + - do: + cluster.put_component_template: + name: test-field + body: + template: + mappings: + properties: + parent.subfield: + type: keyword + + - do: + indices.put_index_template: + name: logs-composable-template + body: + index_patterns: + - test-* + composed_of: + - test-subobjects + - test-field 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 c3884618ffe1f..cd2a994cccb68 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java @@ -1592,9 +1592,7 @@ private static void validateCompositeTemplate( List mappings = collectMappings(stateWithIndex, templateName, indexName); try { MapperService mapperService = tempIndexService.mapperService(); - for (CompressedXContent mapping : mappings) { - mapperService.merge(MapperService.SINGLE_MAPPING_NAME, mapping, MapperService.MergeReason.INDEX_TEMPLATE); - } + mapperService.merge(MapperService.SINGLE_MAPPING_NAME, mappings, MapperService.MergeReason.INDEX_TEMPLATE); if (template.getDataStreamTemplate() != null) { validateTimestampFieldMapping(mapperService.mappingLookup()); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index 3556843a301e2..3f1d33945af01 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -12,11 +12,13 @@ import org.elasticsearch.cluster.metadata.IndexMetadata; import org.elasticsearch.cluster.metadata.MappingMetadata; import org.elasticsearch.cluster.service.ClusterService; +import org.elasticsearch.common.Explicit; import org.elasticsearch.common.compress.CompressedXContent; import org.elasticsearch.common.compress.CompressorFactory; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Setting.Property; import org.elasticsearch.common.xcontent.LoggingDeprecationHandler; +import org.elasticsearch.core.Nullable; import org.elasticsearch.index.AbstractIndexComponent; import org.elasticsearch.index.IndexSettings; import org.elasticsearch.index.IndexVersion; @@ -38,6 +40,7 @@ import java.io.InputStream; import java.util.Collections; import java.util.LinkedHashMap; +import java.util.LinkedList; import java.util.List; import java.util.Map; import java.util.Objects; @@ -352,24 +355,62 @@ public void merge(IndexMetadata indexMetadata, MergeReason reason) { } } + /** + * Merging the provided mappings. If the provided mapping list is ordered, merging will maintain the original ordering. + */ + public DocumentMapper merge(String type, List mappingSources, MergeReason reason) { + final DocumentMapper currentMapper = this.mapper; + if (currentMapper != null && mappingSources.size() == 1 && currentMapper.mappingSource().equals(mappingSources.get(0))) { + return currentMapper; + } + + List> convertedMappings = new LinkedList<>(); + Explicit explicitSubobjects = null; + for (CompressedXContent mappingSource : mappingSources) { + Map mapping = MappingParser.convertToMap(mappingSource); + convertedMappings.add(mapping); + Explicit tmpSubobjects = ObjectMapper.checkSubobjectsInMappingRoot(mapping); + if (tmpSubobjects != null) { + if (explicitSubobjects != null && tmpSubobjects.value() != explicitSubobjects.value()) { + throw new MapperParsingException("Failed to parse mappings: contradicting subobjects settings provided"); + } + explicitSubobjects = tmpSubobjects; + } + } + + DocumentMapper ret = null; + for (Map mapping : convertedMappings) { + ret = doMerge(type, reason, mapping, explicitSubobjects); + } + return ret; + } + public DocumentMapper merge(String type, CompressedXContent mappingSource, MergeReason reason) { final DocumentMapper currentMapper = this.mapper; if (currentMapper != null && currentMapper.mappingSource().equals(mappingSource)) { return currentMapper; } - synchronized (this) { - Mapping incomingMapping = parseMapping(type, mappingSource); - Mapping mapping = mergeMappings(this.mapper, incomingMapping, reason); - // TODO: In many cases the source here is equal to mappingSource so we need not serialize again. - // We should identify these cases reliably and save expensive serialization here - DocumentMapper newMapper = newDocumentMapper(mapping, reason, mapping.toCompressedXContent()); - if (reason == MergeReason.MAPPING_UPDATE_PREFLIGHT) { - return newMapper; - } - this.mapper = newMapper; - assert assertSerialization(newMapper); + Map mappingSourceAsMap = MappingParser.convertToMap(mappingSource); + return doMerge(type, reason, mappingSourceAsMap, null); + } + + private synchronized DocumentMapper doMerge( + String type, + MergeReason reason, + Map mappingSourceAsMap, + @Nullable Explicit explicitSubobjects + ) { + Mapping incomingMapping = parseMapping(type, mappingSourceAsMap, explicitSubobjects); + Mapping mapping = mergeMappings(this.mapper, incomingMapping, reason); + // TODO: In many cases the source here is equal to mappingSource so we need not serialize again. + // We should identify these cases reliably and save expensive serialization here + DocumentMapper newMapper = newDocumentMapper(mapping, reason, mapping.toCompressedXContent()); + if (reason == MergeReason.MAPPING_UPDATE_PREFLIGHT) { return newMapper; } + this.mapper = newMapper; + assert assertSerialization(newMapper); + return newMapper; } private DocumentMapper newDocumentMapper(Mapping mapping, MergeReason reason, CompressedXContent mappingSource) { @@ -386,6 +427,22 @@ public Mapping parseMapping(String mappingType, CompressedXContent mappingSource } } + /** + * A method to parse mapping from a source in a map form, that allows to specify explicit/implicit {@code subobjects} configuration. + * Since parsing is affected by the {@code subobjects} setting, the resulted mapping may change according to this setting. + * @param mappingType the mapping type + * @param mappingSource mapping source already converted to a map form, but not yet processed otherwise + * @param explicitSubobjects subobjects configuration to use for this parsing operation + * @return a parsed mapping + */ + public Mapping parseMapping(String mappingType, Map mappingSource, @Nullable Explicit explicitSubobjects) { + try { + return mappingParser.parse(mappingType, mappingSource, explicitSubobjects); + } catch (Exception e) { + throw new MapperParsingException("Failed to parse mapping: {}", e, e.getMessage()); + } + } + public static Mapping mergeMappings(DocumentMapper currentMapper, Mapping incomingMapping, MergeReason reason) { Mapping newMapping; if (currentMapper == null) { diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MappingParser.java b/server/src/main/java/org/elasticsearch/index/mapper/MappingParser.java index 9cfe6ea1c5410..4de2bd6db6ee2 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MappingParser.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MappingParser.java @@ -8,6 +8,7 @@ package org.elasticsearch.index.mapper; +import org.elasticsearch.common.Explicit; import org.elasticsearch.common.compress.CompressedXContent; import org.elasticsearch.common.xcontent.XContentHelper; import org.elasticsearch.core.Nullable; @@ -72,19 +73,37 @@ private static String getRemainingFields(Map map) { return remainingFields.toString(); } - @SuppressWarnings("unchecked") - Mapping parse(@Nullable String type, CompressedXContent source) throws MapperParsingException { + static Map convertToMap(CompressedXContent source) { Objects.requireNonNull(source, "source cannot be null"); - Map mapping = XContentHelper.convertToMap(source.compressedReference(), true, XContentType.JSON).v2(); - if (mapping.isEmpty()) { + return XContentHelper.convertToMap(source.compressedReference(), true, XContentType.JSON).v2(); + } + + Mapping parse(@Nullable String type, CompressedXContent source) throws MapperParsingException { + Map mapping = convertToMap(source); + return parse(type, mapping, null); + } + + /** + * A method to parse mapping from a source in a map form, that allows to specify explicit/implicit {@code subobjects} configuration. + * Since parsing is affected by the {@code subobjects} setting, the resulted mapping may change according to this setting. + * @param type the mapping type + * @param mappingSource mapping source already converted to a map form, but not yet processed otherwise + * @param explicitSubobjects subobjects configuration to use for this parsing operation + * @return a parsed mapping + * @throws MapperParsingException in case of parsing error + */ + @SuppressWarnings("unchecked") + Mapping parse(@Nullable String type, Map mappingSource, @Nullable Explicit explicitSubobjects) + throws MapperParsingException { + if (mappingSource.isEmpty()) { if (type == null) { - throw new MapperParsingException("malformed mapping, no type name found"); + throw new MapperParsingException("malformed mappingSource, no type name found"); } } else { - String rootName = mapping.keySet().iterator().next(); + String rootName = mappingSource.keySet().iterator().next(); if (type == null || type.equals(rootName) || documentTypeResolver.apply(type).equals(rootName)) { type = rootName; - mapping = (Map) mapping.get(rootName); + mappingSource = (Map) mappingSource.get(rootName); } } if (type == null) { @@ -93,19 +112,16 @@ Mapping parse(@Nullable String type, CompressedXContent source) throws MapperPar if (type.isEmpty()) { throw new MapperParsingException("type cannot be an empty string"); } - return parse(type, mapping); - } - private Mapping parse(String type, Map mapping) throws MapperParsingException { final MappingParserContext mappingParserContext = mappingParserContextSupplier.get(); - RootObjectMapper.Builder rootObjectMapper = RootObjectMapper.parse(type, mapping, mappingParserContext); + RootObjectMapper.Builder rootObjectMapper = RootObjectMapper.parse(type, mappingSource, mappingParserContext, explicitSubobjects); Map, MetadataFieldMapper> metadataMappers = metadataMappersSupplier.get(); Map meta = null; boolean isSourceSynthetic = false; - Iterator> iterator = mapping.entrySet().iterator(); + Iterator> iterator = mappingSource.entrySet().iterator(); while (iterator.hasNext()) { Map.Entry entry = iterator.next(); String fieldName = entry.getKey(); @@ -130,7 +146,7 @@ private Mapping parse(String type, Map mapping) throws MapperPar } @SuppressWarnings("unchecked") - Map removed = (Map) mapping.remove("_meta"); + Map removed = (Map) mappingSource.remove("_meta"); if (removed != null) { /* * It may not be required to copy meta here to maintain immutability but the cost is pretty low here. @@ -150,7 +166,7 @@ private Mapping parse(String type, Map mapping) throws MapperPar } if (mappingParserContext.indexVersionCreated().isLegacyIndexVersion() == false) { // legacy indices are allowed to have extra definitions that we ignore (we will drop them on import) - checkNoRemainingFields(mapping, "Root mapping definition has unsupported parameters: "); + checkNoRemainingFields(mappingSource, "Root mapping definition has unsupported parameters: "); } return new Mapping( diff --git a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java index c59e25e54ae70..1e03ef9f56aab 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java @@ -14,6 +14,7 @@ import org.elasticsearch.common.logging.DeprecationCategory; import org.elasticsearch.common.logging.DeprecationLogger; import org.elasticsearch.common.xcontent.support.XContentMapValues; +import org.elasticsearch.core.Nullable; import org.elasticsearch.index.IndexVersion; import org.elasticsearch.index.mapper.MapperService.MergeReason; import org.elasticsearch.xcontent.ToXContent; @@ -251,7 +252,7 @@ protected static Explicit parseSubobjects(Map node) { if (subobjectsNode != null) { return Explicit.explicitBoolean(XContentMapValues.nodeBooleanValue(subobjectsNode, "subobjects.subobjects")); } - return Explicit.IMPLICIT_TRUE; + return Defaults.SUBOBJECTS; } protected static void parseProperties( @@ -589,6 +590,27 @@ protected void doXContent(XContentBuilder builder, Params params) throws IOExcep } + /** + * Checks the {@code subobjects} config in provided mapping's root. + * @param mapping a raw mapping in a map form + * @return an explicit {@code subobjects} value if such is configured directly under the root, or {@code null} otherwise + */ + @Nullable + static Explicit checkSubobjectsInMappingRoot(Map mapping) { + if (mapping.size() == 1) { + String rootName = mapping.keySet().iterator().next(); + Object childNode = mapping.get(rootName); + if (childNode instanceof Map) { + @SuppressWarnings("unchecked") + Object subobjectsNode = ((Map) mapping.get(rootName)).get("subobjects"); + if (subobjectsNode != null) { + return Explicit.explicitBoolean(XContentMapValues.nodeBooleanValue(subobjectsNode, "subobjects.subobjects")); + } + } + } + return null; + } + public SourceLoader.SyntheticFieldLoader syntheticFieldLoader(Stream extra) { return new SyntheticSourceFieldLoader( Stream.concat(extra, mappers.values().stream()) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java index fa8aef1317cb1..53a62920621bc 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java @@ -14,6 +14,7 @@ import org.elasticsearch.common.logging.DeprecationCategory; import org.elasticsearch.common.logging.DeprecationLogger; import org.elasticsearch.common.time.DateFormatter; +import org.elasticsearch.core.Nullable; import org.elasticsearch.index.IndexVersion; import org.elasticsearch.index.mapper.DynamicTemplate.XContentFieldType; import org.elasticsearch.index.mapper.MapperService.MergeReason; @@ -399,9 +400,16 @@ protected void startSyntheticField(XContentBuilder b) throws IOException { b.startObject(); } - public static RootObjectMapper.Builder parse(String name, Map node, MappingParserContext parserContext) - throws MapperParsingException { + public static RootObjectMapper.Builder parse( + String name, + Map node, + MappingParserContext parserContext, + @Nullable Explicit explicitSubobjects + ) throws MapperParsingException { Explicit subobjects = parseSubobjects(node); + if (subobjects.explicit() == false && explicitSubobjects != null) { + subobjects = explicitSubobjects; + } RootObjectMapper.Builder builder = new Builder(name, subobjects); Iterator> iterator = node.entrySet().iterator(); while (iterator.hasNext()) { 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 84a6e03ce8cc5..af2f16fa767ab 100644 --- a/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateServiceTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateServiceTests.java @@ -2492,6 +2492,61 @@ public void testAddInvalidTemplateIgnoreService() throws Exception { assertThat(e.getMessage(), containsString("missing component templates [fail] that does not exist")); } + public void testComposableTemplateWithSubobjectsFalse() throws Exception { + final MetadataIndexTemplateService service = getMetadataIndexTemplateService(); + ClusterState state = ClusterState.EMPTY_STATE; + + ComponentTemplate subobjects = new ComponentTemplate(new Template(null, new CompressedXContent(""" + { + "subobjects": false + } + """), null), null, null); + + ComponentTemplate fieldMapping = new ComponentTemplate(new Template(null, new CompressedXContent(""" + { + "properties": { + "parent.subfield": { + "type": "keyword" + } + } + } + """), null), null, null); + + state = service.addComponentTemplate(state, true, "subobjects", subobjects); + state = service.addComponentTemplate(state, true, "field_mapping", fieldMapping); + ComposableIndexTemplate it = new ComposableIndexTemplate( + List.of("test-*"), + new Template(null, null, null), + List.of("subobjects", "field_mapping"), + 0L, + 1L, + null, + null, + null + ); + state = service.addIndexTemplateV2(state, true, "composable-template", it); + + List mappings = MetadataIndexTemplateService.collectMappings(state, "composable-template", "test-index"); + + assertNotNull(mappings); + assertThat(mappings.size(), equalTo(2)); + List> parsedMappings = mappings.stream().map(m -> { + try { + return MapperService.parseMapping(NamedXContentRegistry.EMPTY, m); + } catch (Exception e) { + logger.error(e); + fail("failed to parse mappings: " + m.string()); + return null; + } + }).toList(); + + assertThat(parsedMappings.get(0), equalTo(Map.of("_doc", Map.of("subobjects", false)))); + assertThat( + parsedMappings.get(1), + equalTo(Map.of("_doc", Map.of("properties", Map.of("parent.subfield", Map.of("type", "keyword"))))) + ); + } + private static List putTemplate(NamedXContentRegistry xContentRegistry, PutRequest request) { ThreadPool testThreadPool = mock(ThreadPool.class); ClusterService clusterService = ClusterServiceUtils.createClusterService(testThreadPool); From d3b292371d349bd444424491f3c0818b66f0f4cb Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Mon, 3 Jul 2023 12:27:32 +0300 Subject: [PATCH 02/38] Update docs/changelog/97317.yaml --- docs/changelog/97317.yaml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 docs/changelog/97317.yaml diff --git a/docs/changelog/97317.yaml b/docs/changelog/97317.yaml new file mode 100644 index 0000000000000..7c45ecacd00b7 --- /dev/null +++ b/docs/changelog/97317.yaml @@ -0,0 +1,6 @@ +pr: 97317 +summary: "Fix composable templates with `subobjects: false`" +area: Mapping +type: bug +issues: + - 96768 From faf83a5724887af32f92d93f7b576763bbae2d56 Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Mon, 31 Jul 2023 06:45:06 +0300 Subject: [PATCH 03/38] Add unit tests --- .../index/mapper/MapperServiceTests.java | 90 +++++++++++++++++++ 1 file changed, 90 insertions(+) diff --git a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java index 93c85533f9e4c..9c631b7044717 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java @@ -417,4 +417,94 @@ to get the wrong path (missing the first portion). List fields = parsedDocument.rootDoc().getFields("obj.sub.string"); assertEquals(1, fields.size()); } + + public void testBulkMerge() throws IOException { + final MapperService mapperService = createMapperService(mapping(b -> {})); + CompressedXContent mapping1 = createTestMapping1(); + CompressedXContent mapping2 = createTestMapping2(); + mapperService.merge("_doc", mapping1, MergeReason.INDEX_TEMPLATE); + DocumentMapper sequentiallyMergedMapper = mapperService.merge("_doc", mapping2, MergeReason.INDEX_TEMPLATE); + DocumentMapper bulkMergedMapper = mapperService.merge("_doc", List.of(mapping1, mapping2), MergeReason.INDEX_TEMPLATE); + assertEquals(sequentiallyMergedMapper.mappingSource(), bulkMergedMapper.mappingSource()); + } + + public void testMergeSubobjectsFalseOrder() throws IOException { + final MapperService mapperService = createMapperService(mapping(b -> {})); + CompressedXContent mapping1 = createTestMapping1(); + CompressedXContent mapping2 = createTestMapping2(); + DocumentMapper subobjectsFirst = mapperService.merge("_doc", List.of(mapping1, mapping2), MergeReason.INDEX_TEMPLATE); + DocumentMapper subobjectsLast = mapperService.merge("_doc", List.of(mapping2, mapping1), MergeReason.INDEX_TEMPLATE); + assertEquals(subobjectsFirst.mappingSource(), subobjectsLast.mappingSource()); + } + + public void testMergeContradictingSubobjects() throws IOException { + final MapperService mapperService = createMapperService(mapping(b -> {})); + CompressedXContent mapping1 = createTestMapping1(); + CompressedXContent mapping2 = createTestMapping2(); + CompressedXContent subobjectsTrueExplicitly; + try (XContentBuilder xContentBuilder = XContentFactory.jsonBuilder()) { + subobjectsTrueExplicitly = new CompressedXContent( + BytesReference.bytes( + xContentBuilder.startObject() + .startObject("_doc") + .field("subobjects", true) + .startObject("properties") + .startObject("parent.other_subfield") + .field("type", "text") + .endObject() + .endObject() + .endObject() + .endObject() + ) + ); + } + + MapperParsingException e = expectThrows( + MapperParsingException.class, + () -> mapperService.merge("_doc", List.of(mapping2, mapping1, subobjectsTrueExplicitly), MergeReason.INDEX_TEMPLATE) + ); + assertEquals("Failed to parse mappings: contradicting subobjects settings provided", e.getMessage()); + } + + private static CompressedXContent createTestMapping1() throws IOException { + CompressedXContent mapping1; + try (XContentBuilder xContentBuilder = XContentFactory.jsonBuilder()) { + mapping1 = new CompressedXContent( + BytesReference.bytes( + xContentBuilder.startObject() + .startObject("_doc") + .field("subobjects", false) + .startObject("properties") + .startObject("parent") + .field("type", "text") + .endObject() + .endObject() + .endObject() + .endObject() + ) + ); + } + return mapping1; + } + + private static CompressedXContent createTestMapping2() throws IOException { + CompressedXContent mapping2; + try (XContentBuilder xContentBuilder = XContentFactory.jsonBuilder()) { + mapping2 = new CompressedXContent( + BytesReference.bytes( + xContentBuilder.startObject() + .startObject("_doc") + .field("subobjects", false) + .startObject("properties") + .startObject("parent.subfield") + .field("type", "text") + .endObject() + .endObject() + .endObject() + .endObject() + ) + ); + } + return mapping2; + } } From 0ca0233890043f94b367898e79e58d42ff6a125a Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Mon, 31 Jul 2023 07:25:00 +0300 Subject: [PATCH 04/38] Change changelog summary --- docs/changelog/97317.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/changelog/97317.yaml b/docs/changelog/97317.yaml index 7c45ecacd00b7..64fcd55e67e28 100644 --- a/docs/changelog/97317.yaml +++ b/docs/changelog/97317.yaml @@ -1,5 +1,5 @@ pr: 97317 -summary: "Fix composable templates with `subobjects: false`" +summary: "Fix merges of mappings with `subobjects: false` for composable index templates" area: Mapping type: bug issues: From b45863ee564d0e87788d05992d4245e0b23f97db Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Mon, 31 Jul 2023 11:29:11 +0300 Subject: [PATCH 05/38] Small refactor: switch from sequential to bulk component templates merge --- .../DataStreamIndexSettingsProvider.java | 5 +---- .../post/TransportSimulateIndexTemplateAction.java | 4 +--- .../metadata/MetadataCreateIndexService.java | 9 +++++---- .../index/mapper/DynamicTemplatesTests.java | 14 +++++++------- .../index/mapper/ObjectMapperTests.java | 11 +++++++---- 5 files changed, 21 insertions(+), 22 deletions(-) diff --git a/modules/data-streams/src/main/java/org/elasticsearch/datastreams/DataStreamIndexSettingsProvider.java b/modules/data-streams/src/main/java/org/elasticsearch/datastreams/DataStreamIndexSettingsProvider.java index f4e660c2b18f4..cb220eacac375 100644 --- a/modules/data-streams/src/main/java/org/elasticsearch/datastreams/DataStreamIndexSettingsProvider.java +++ b/modules/data-streams/src/main/java/org/elasticsearch/datastreams/DataStreamIndexSettingsProvider.java @@ -158,10 +158,7 @@ private List findRoutingPaths(String indexName, Settings allSettings, Li tmpIndexMetadata.settings(finalResolvedSettings); // Create MapperService just to extract keyword dimension fields: try (var mapperService = mapperServiceFactory.apply(tmpIndexMetadata.build())) { - for (var mapping : combinedTemplateMappings) { - mapperService.merge(MapperService.SINGLE_MAPPING_NAME, mapping, MapperService.MergeReason.INDEX_TEMPLATE); - } - + mapperService.merge(MapperService.SINGLE_MAPPING_NAME, combinedTemplateMappings, MapperService.MergeReason.INDEX_TEMPLATE); List routingPaths = new ArrayList<>(); for (var fieldMapper : mapperService.documentMapper().mappers().fieldMappers()) { extractPath(routingPaths, fieldMapper); diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/template/post/TransportSimulateIndexTemplateAction.java b/server/src/main/java/org/elasticsearch/action/admin/indices/template/post/TransportSimulateIndexTemplateAction.java index b1c15094e517b..7a0769db46972 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/template/post/TransportSimulateIndexTemplateAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/template/post/TransportSimulateIndexTemplateAction.java @@ -294,9 +294,7 @@ public static Template resolveTemplate( indexMetadata, tempIndexService -> { MapperService mapperService = tempIndexService.mapperService(); - for (CompressedXContent mapping : mappings) { - mapperService.merge(MapperService.SINGLE_MAPPING_NAME, mapping, MapperService.MergeReason.INDEX_TEMPLATE); - } + mapperService.merge(MapperService.SINGLE_MAPPING_NAME, mappings, MapperService.MergeReason.INDEX_TEMPLATE); DocumentMapper documentMapper = mapperService.documentMapper(); return documentMapper != null ? documentMapper.mappingSource() : null; 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 763e3d3574752..3302549a1b860 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java @@ -1331,13 +1331,14 @@ private static void updateIndexMappingsAndBuildSortOrder( ) throws IOException { MapperService mapperService = indexService.mapperService(); IndexMode indexMode = indexService.getIndexSettings() != null ? indexService.getIndexSettings().getMode() : IndexMode.STANDARD; + List allMappings = new ArrayList<>(); final CompressedXContent defaultMapping = indexMode.getDefaultMapping(); if (defaultMapping != null) { - mapperService.merge(MapperService.SINGLE_MAPPING_NAME, defaultMapping, MergeReason.INDEX_TEMPLATE); - } - for (CompressedXContent mapping : mappings) { - mapperService.merge(MapperService.SINGLE_MAPPING_NAME, mapping, MergeReason.INDEX_TEMPLATE); + allMappings.add(defaultMapping); } + allMappings.addAll(mappings); + mapperService.merge(MapperService.SINGLE_MAPPING_NAME, allMappings, MergeReason.INDEX_TEMPLATE); + indexMode.validateTimestampFieldMapping(request.dataStreamName() != null, mapperService.mappingLookup()); if (sourceMetadata == null) { diff --git a/server/src/test/java/org/elasticsearch/index/mapper/DynamicTemplatesTests.java b/server/src/test/java/org/elasticsearch/index/mapper/DynamicTemplatesTests.java index 07aa25c643cba..0951ef0e10ed1 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/DynamicTemplatesTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/DynamicTemplatesTests.java @@ -310,7 +310,7 @@ public void testDynamicTemplates() throws Exception { } public void testDynamicTemplatesForIndexTemplate() throws IOException { - String mapping = Strings.toString( + String mapping1 = Strings.toString( XContentFactory.jsonBuilder() .startObject() .startArray("dynamic_templates") @@ -333,11 +333,9 @@ public void testDynamicTemplatesForIndexTemplate() throws IOException { .endArray() .endObject() ); - MapperService mapperService = createMapperService(IndexVersion.current(), Settings.EMPTY, () -> true); - mapperService.merge(MapperService.SINGLE_MAPPING_NAME, new CompressedXContent(mapping), MapperService.MergeReason.INDEX_TEMPLATE); // There should be no update if templates are not set. - mapping = Strings.toString( + String mapping2 = Strings.toString( XContentFactory.jsonBuilder() .startObject() .startObject("properties") @@ -347,9 +345,11 @@ public void testDynamicTemplatesForIndexTemplate() throws IOException { .endObject() .endObject() ); + + MapperService mapperService = createMapperService(IndexVersion.current(), Settings.EMPTY, () -> true); DocumentMapper mapper = mapperService.merge( MapperService.SINGLE_MAPPING_NAME, - new CompressedXContent(mapping), + List.of(new CompressedXContent(mapping1), new CompressedXContent(mapping2)), MapperService.MergeReason.INDEX_TEMPLATE ); @@ -361,7 +361,7 @@ public void testDynamicTemplatesForIndexTemplate() throws IOException { assertEquals("second", templates[1].pathMatch().get(0)); // Dynamic templates should be appended and deduplicated. - mapping = Strings.toString( + String mapping3 = Strings.toString( XContentFactory.jsonBuilder() .startObject() .startArray("dynamic_templates") @@ -386,7 +386,7 @@ public void testDynamicTemplatesForIndexTemplate() throws IOException { ); mapper = mapperService.merge( MapperService.SINGLE_MAPPING_NAME, - new CompressedXContent(mapping), + new CompressedXContent(mapping3), MapperService.MergeReason.INDEX_TEMPLATE ); diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java index d84805d570b6d..a1d8df983cc25 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java @@ -20,6 +20,7 @@ import org.elasticsearch.xcontent.XContentType; import java.io.IOException; +import java.util.List; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.instanceOf; @@ -199,7 +200,6 @@ public void testFieldReplacementForIndexTemplates() throws IOException { .endObject() .endObject() ); - mapperService.merge(MapperService.SINGLE_MAPPING_NAME, new CompressedXContent(mapping), MergeReason.INDEX_TEMPLATE); String update = Strings.toString( XContentFactory.jsonBuilder() @@ -220,7 +220,7 @@ public void testFieldReplacementForIndexTemplates() throws IOException { ); DocumentMapper mapper = mapperService.merge( MapperService.SINGLE_MAPPING_NAME, - new CompressedXContent(update), + List.of(new CompressedXContent(mapping), new CompressedXContent(update)), MergeReason.INDEX_TEMPLATE ); @@ -269,7 +269,6 @@ public void testDisallowFieldReplacementForIndexTemplates() throws IOException { .endObject() .endObject() ); - mapperService.merge(MapperService.SINGLE_MAPPING_NAME, new CompressedXContent(mapping), MergeReason.INDEX_TEMPLATE); String firstUpdate = Strings.toString( XContentFactory.jsonBuilder() @@ -287,7 +286,11 @@ public void testDisallowFieldReplacementForIndexTemplates() throws IOException { ); IllegalArgumentException e = expectThrows( IllegalArgumentException.class, - () -> mapperService.merge(MapperService.SINGLE_MAPPING_NAME, new CompressedXContent(firstUpdate), MergeReason.INDEX_TEMPLATE) + () -> mapperService.merge( + MapperService.SINGLE_MAPPING_NAME, + List.of(new CompressedXContent(mapping), new CompressedXContent(firstUpdate)), + MergeReason.INDEX_TEMPLATE + ) ); assertThat(e.getMessage(), containsString("can't merge a non object mapping [object.field2] with an object mapping")); From a8cfed5f629a9dcd21b3332ee7f9bb5032334a83 Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Sun, 6 Aug 2023 18:02:56 +0300 Subject: [PATCH 06/38] Merging mappings in raw map form --- .../15_composition.yml | 4 + .../common/xcontent/XContentHelper.java | 91 ++++++++++++++----- .../index/mapper/MapperService.java | 71 ++++++++------- .../index/mapper/MappingParser.java | 11 +-- .../index/mapper/RootObjectMapper.java | 12 +-- .../xcontent/support/XContentHelperTests.java | 33 +++++++ .../index/mapper/MapperServiceTests.java | 47 +++++++++- 7 files changed, 199 insertions(+), 70 deletions(-) diff --git a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.put_index_template/15_composition.yml b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.put_index_template/15_composition.yml index d048ac1deb49f..578079eb06536 100644 --- a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.put_index_template/15_composition.yml +++ b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.put_index_template/15_composition.yml @@ -355,6 +355,7 @@ - skip: version: ' - 8.9.1' reason: 'https://github.com/elastic/elasticsearch/issues/96768 fixed after 8.9.0' + features: allowed_warnings - do: cluster.put_component_template: @@ -375,6 +376,8 @@ type: keyword - do: + allowed_warnings: + - "index template [logs-composable-template] has index patterns [test-*] matching patterns from existing older templates [global] with patterns (global => [*]); this template [logs-composable-template] will take precedence during new index creation" indices.put_index_template: name: logs-composable-template body: @@ -383,3 +386,4 @@ composed_of: - test-subobjects - test-field + - is_true: acknowledged diff --git a/server/src/main/java/org/elasticsearch/common/xcontent/XContentHelper.java b/server/src/main/java/org/elasticsearch/common/xcontent/XContentHelper.java index 98d27dbc7cf80..81e55ad3f4881 100644 --- a/server/src/main/java/org/elasticsearch/common/xcontent/XContentHelper.java +++ b/server/src/main/java/org/elasticsearch/common/xcontent/XContentHelper.java @@ -377,54 +377,84 @@ public static boolean update(Map source, Map cha } /** - * Merges the defaults provided as the second parameter into the content of the first. Only does recursive merge - * for inner maps. + * Merges the defaults provided as the second parameter into the content of the first. Only does recursive merge for inner maps. */ public static void mergeDefaults(Map content, Map defaults) { - for (Map.Entry defaultEntry : defaults.entrySet()) { - if (content.containsKey(defaultEntry.getKey()) == false) { + merge(content, defaults, null); + } + + /** + * Merges the map provided as the second parameter into the content of the first. Only does recursive merge for inner maps. + * If a non-null {@link CustomMerge} is provided, it is applied whenever a merge is required, meaning - whenever both the first and + * the second map has values for the same key. Otherwise, values from the first map will always have precedence, meaning - if the + * first map contains a key, its value will not be overriden. + * @param first the map which serves as the merge base + * @param second the map of which contents are merged into the base map + * @param customMerge a custom merge rule to apply whenever a key has concrete values (i.e. not a map or a collection) in both maps + */ + public static void merge(Map first, Map second, @Nullable CustomMerge customMerge) { + merge(first, second, customMerge, null); + } + + private static void merge( + Map first, + Map second, + @Nullable CustomMerge customMerge, + @Nullable String parent + ) { + for (Map.Entry toMergeEntry : second.entrySet()) { + if (first.containsKey(toMergeEntry.getKey()) == false) { // copy it over, it does not exists in the content - content.put(defaultEntry.getKey(), defaultEntry.getValue()); + first.put(toMergeEntry.getKey(), toMergeEntry.getValue()); } else { - // in the content and in the default, only merge compound ones (maps) - if (content.get(defaultEntry.getKey()) instanceof Map && defaultEntry.getValue() instanceof Map) { - mergeDefaults((Map) content.get(defaultEntry.getKey()), (Map) defaultEntry.getValue()); - } else if (content.get(defaultEntry.getKey()) instanceof List && defaultEntry.getValue() instanceof List) { - List defaultList = (List) defaultEntry.getValue(); - List contentList = (List) content.get(defaultEntry.getKey()); - - if (allListValuesAreMapsOfOne(defaultList) && allListValuesAreMapsOfOne(contentList)) { + // has values in both maps, merge compound ones (maps) + Object baseValue = first.get(toMergeEntry.getKey()); + if (baseValue instanceof Map && toMergeEntry.getValue() instanceof Map) { + merge( + (Map) baseValue, + (Map) toMergeEntry.getValue(), + customMerge, + toMergeEntry.getKey() + ); + } else if (baseValue instanceof List && toMergeEntry.getValue() instanceof List) { + List listToMerge = (List) toMergeEntry.getValue(); + List baseList = (List) baseValue; + + if (allListValuesAreMapsOfOne(listToMerge) && allListValuesAreMapsOfOne(baseList)) { // all are in the form of [ {"key1" : {}}, {"key2" : {}} ], merge based on keys Map> processed = new LinkedHashMap<>(); - for (Object o : contentList) { + for (Object o : baseList) { Map map = (Map) o; Map.Entry entry = map.entrySet().iterator().next(); processed.put(entry.getKey(), map); } - for (Object o : defaultList) { + for (Object o : listToMerge) { Map map = (Map) o; Map.Entry entry = map.entrySet().iterator().next(); if (processed.containsKey(entry.getKey())) { - mergeDefaults(processed.get(entry.getKey()), map); + merge(processed.get(entry.getKey()), map, customMerge, toMergeEntry.getKey()); } else { - // put the default entries after the content ones. + // append the second list's entries after the first list's entries. processed.put(entry.getKey(), map); } } - content.put(defaultEntry.getKey(), new ArrayList<>(processed.values())); + first.put(toMergeEntry.getKey(), new ArrayList<>(processed.values())); } else { - // if both are lists, simply combine them, first the defaults, then the content + // if both are lists, simply combine them, first the second list's values, then the first's // just make sure not to add the same value twice - List mergedList = new ArrayList<>(defaultList); + // custom merge is not applicable here + List mergedList = new ArrayList<>(listToMerge); - for (Object o : contentList) { + for (Object o : baseList) { if (mergedList.contains(o) == false) { mergedList.add(o); } } - content.put(defaultEntry.getKey(), mergedList); + first.put(toMergeEntry.getKey(), mergedList); } + } else if (customMerge != null) { + first.put(toMergeEntry.getKey(), customMerge.merge(parent, toMergeEntry.getKey(), baseValue, toMergeEntry.getValue())); } } } @@ -442,6 +472,23 @@ private static boolean allListValuesAreMapsOfOne(List list) { return true; } + /** + * A {@code FunctionalInterface} that can be used in order to customize map merges. + */ + @FunctionalInterface + public interface CustomMerge { + /** + * Based on the provided arguments, decide which value to use for the given key. + * This method doesn't throw a checked exception, but it is expected that illegal merges will result in a {@link RuntimeException}. + * @param parent merged field's parent + * @param key merged field's name + * @param oldValue original value of the provided key + * @param newValue the new value of the provided key which is to be merged with the original + * @return the merged value to use for the given key + */ + Object merge(String parent, String key, Object oldValue, Object newValue); + } + /** * Writes a "raw" (bytes) field, handling cases where the bytes are compressed, and tries to optimize writing using * {@link XContentBuilder#rawField(String, InputStream)}. diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index bfacf9a8a6dfb..4bdf7be2f8a53 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -12,12 +12,12 @@ import org.elasticsearch.cluster.metadata.IndexMetadata; import org.elasticsearch.cluster.metadata.MappingMetadata; import org.elasticsearch.cluster.service.ClusterService; -import org.elasticsearch.common.Explicit; import org.elasticsearch.common.compress.CompressedXContent; import org.elasticsearch.common.compress.CompressorFactory; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Setting.Property; import org.elasticsearch.common.xcontent.LoggingDeprecationHandler; +import org.elasticsearch.common.xcontent.XContentHelper; import org.elasticsearch.core.Nullable; import org.elasticsearch.index.AbstractIndexComponent; import org.elasticsearch.index.IndexSettings; @@ -40,10 +40,10 @@ import java.io.InputStream; import java.util.Collections; import java.util.LinkedHashMap; -import java.util.LinkedList; import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Set; import java.util.function.Function; import java.util.function.Supplier; @@ -75,6 +75,7 @@ public enum MergeReason { public static final String SINGLE_MAPPING_NAME = "_doc"; public static final String TYPE_FIELD_NAME = "_type"; + public static final Set OBJECT_FIELD_TYPES = Set.of(ObjectMapper.CONTENT_TYPE, NestedObjectMapper.CONTENT_TYPE); public static final Setting INDEX_MAPPING_NESTED_FIELDS_LIMIT_SETTING = Setting.longSetting( "index.mapping.nested_fields.limit", 50L, @@ -356,7 +357,8 @@ public void merge(IndexMetadata indexMetadata, MergeReason reason) { } /** - * Merging the provided mappings. If the provided mapping list is ordered, merging will maintain the original ordering. + * Merging the provided mappings. Actual merging is done in the raw, non-parsed, form of the mappings. This allows to do a proper bulk + * merge, where parsing is done only when all raw mapping settings are already merged. */ public DocumentMapper merge(String type, List mappingSources, MergeReason reason) { final DocumentMapper currentMapper = this.mapper; @@ -364,25 +366,37 @@ public DocumentMapper merge(String type, List mappingSources return currentMapper; } - List> convertedMappings = new LinkedList<>(); - Explicit explicitSubobjects = null; + Map mergedRawMapping = null; for (CompressedXContent mappingSource : mappingSources) { - Map mapping = MappingParser.convertToMap(mappingSource); - convertedMappings.add(mapping); - Explicit tmpSubobjects = ObjectMapper.checkSubobjectsInMappingRoot(mapping); - if (tmpSubobjects != null) { - if (explicitSubobjects != null && tmpSubobjects.value() != explicitSubobjects.value()) { - throw new MapperParsingException("Failed to parse mappings: contradicting subobjects settings provided"); - } - explicitSubobjects = tmpSubobjects; + Map rawMapping = MappingParser.convertToMap(mappingSource); + if (mergedRawMapping == null) { + mergedRawMapping = rawMapping; + } else { + XContentHelper.merge(mergedRawMapping, rawMapping, ((parent, key, oldValue, newValue) -> { + switch (key) { + case "type" -> { + // todo: verify this check is valid + if (oldValue.equals(newValue) == false + && (OBJECT_FIELD_TYPES.contains(String.valueOf(oldValue)) + || OBJECT_FIELD_TYPES.contains(String.valueOf(newValue)))) { + throw new MapperParsingException( + "can't merge a non object mapping [" + parent + "] with an object mapping" + ); + } + } + case "subobjects" -> { + if (oldValue.equals(newValue) == false) { + throw new MapperParsingException( + "Failed to parse mappings: contradicting subobjects settings provided for field: " + parent + ); + } + } + } + return newValue; + })); } } - - DocumentMapper ret = null; - for (Map mapping : convertedMappings) { - ret = doMerge(type, reason, mapping, explicitSubobjects); - } - return ret; + return (mergedRawMapping != null) ? doMerge(type, reason, mergedRawMapping) : null; } public DocumentMapper merge(String type, CompressedXContent mappingSource, MergeReason reason) { @@ -391,16 +405,11 @@ public DocumentMapper merge(String type, CompressedXContent mappingSource, Merge return currentMapper; } Map mappingSourceAsMap = MappingParser.convertToMap(mappingSource); - return doMerge(type, reason, mappingSourceAsMap, null); + return doMerge(type, reason, mappingSourceAsMap); } - private synchronized DocumentMapper doMerge( - String type, - MergeReason reason, - Map mappingSourceAsMap, - @Nullable Explicit explicitSubobjects - ) { - Mapping incomingMapping = parseMapping(type, mappingSourceAsMap, explicitSubobjects); + private synchronized DocumentMapper doMerge(String type, MergeReason reason, Map mappingSourceAsMap) { + Mapping incomingMapping = parseMapping(type, mappingSourceAsMap); Mapping mapping = mergeMappings(this.mapper, incomingMapping, reason); // TODO: In many cases the source here is equal to mappingSource so we need not serialize again. // We should identify these cases reliably and save expensive serialization here @@ -430,14 +439,14 @@ public Mapping parseMapping(String mappingType, CompressedXContent mappingSource /** * A method to parse mapping from a source in a map form, that allows to specify explicit/implicit {@code subobjects} configuration. * Since parsing is affected by the {@code subobjects} setting, the resulted mapping may change according to this setting. - * @param mappingType the mapping type + * + * @param mappingType the mapping type * @param mappingSource mapping source already converted to a map form, but not yet processed otherwise - * @param explicitSubobjects subobjects configuration to use for this parsing operation * @return a parsed mapping */ - public Mapping parseMapping(String mappingType, Map mappingSource, @Nullable Explicit explicitSubobjects) { + public Mapping parseMapping(String mappingType, Map mappingSource) { try { - return mappingParser.parse(mappingType, mappingSource, explicitSubobjects); + return mappingParser.parse(mappingType, mappingSource); } catch (Exception e) { throw new MapperParsingException("Failed to parse mapping: {}", e, e.getMessage()); } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MappingParser.java b/server/src/main/java/org/elasticsearch/index/mapper/MappingParser.java index 865cc82fbdef0..1b5463ba965ca 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MappingParser.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MappingParser.java @@ -8,7 +8,6 @@ package org.elasticsearch.index.mapper; -import org.elasticsearch.common.Explicit; import org.elasticsearch.common.compress.CompressedXContent; import org.elasticsearch.common.xcontent.XContentHelper; import org.elasticsearch.core.Nullable; @@ -81,20 +80,20 @@ static Map convertToMap(CompressedXContent source) { Mapping parse(@Nullable String type, CompressedXContent source) throws MapperParsingException { Map mapping = convertToMap(source); - return parse(type, mapping, null); + return parse(type, mapping); } /** * A method to parse mapping from a source in a map form, that allows to specify explicit/implicit {@code subobjects} configuration. * Since parsing is affected by the {@code subobjects} setting, the resulted mapping may change according to this setting. - * @param type the mapping type + * + * @param type the mapping type * @param mappingSource mapping source already converted to a map form, but not yet processed otherwise - * @param explicitSubobjects subobjects configuration to use for this parsing operation * @return a parsed mapping * @throws MapperParsingException in case of parsing error */ @SuppressWarnings("unchecked") - Mapping parse(@Nullable String type, Map mappingSource, @Nullable Explicit explicitSubobjects) + Mapping parse(@Nullable String type, Map mappingSource) throws MapperParsingException { if (mappingSource.isEmpty()) { if (type == null) { @@ -116,7 +115,7 @@ Mapping parse(@Nullable String type, Map mappingSource, @Nullabl final MappingParserContext mappingParserContext = mappingParserContextSupplier.get(); - RootObjectMapper.Builder rootObjectMapper = RootObjectMapper.parse(type, mappingSource, mappingParserContext, explicitSubobjects); + RootObjectMapper.Builder rootObjectMapper = RootObjectMapper.parse(type, mappingSource, mappingParserContext); Map, MetadataFieldMapper> metadataMappers = metadataMappersSupplier.get(); Map meta = null; diff --git a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java index 53a62920621bc..fa8aef1317cb1 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java @@ -14,7 +14,6 @@ import org.elasticsearch.common.logging.DeprecationCategory; import org.elasticsearch.common.logging.DeprecationLogger; import org.elasticsearch.common.time.DateFormatter; -import org.elasticsearch.core.Nullable; import org.elasticsearch.index.IndexVersion; import org.elasticsearch.index.mapper.DynamicTemplate.XContentFieldType; import org.elasticsearch.index.mapper.MapperService.MergeReason; @@ -400,16 +399,9 @@ protected void startSyntheticField(XContentBuilder b) throws IOException { b.startObject(); } - public static RootObjectMapper.Builder parse( - String name, - Map node, - MappingParserContext parserContext, - @Nullable Explicit explicitSubobjects - ) throws MapperParsingException { + public static RootObjectMapper.Builder parse(String name, Map node, MappingParserContext parserContext) + throws MapperParsingException { Explicit subobjects = parseSubobjects(node); - if (subobjects.explicit() == false && explicitSubobjects != null) { - subobjects = explicitSubobjects; - } RootObjectMapper.Builder builder = new Builder(name, subobjects); Iterator> iterator = node.entrySet().iterator(); while (iterator.hasNext()) { diff --git a/server/src/test/java/org/elasticsearch/common/xcontent/support/XContentHelperTests.java b/server/src/test/java/org/elasticsearch/common/xcontent/support/XContentHelperTests.java index 7dccae9ec6086..8ea3f9514a287 100644 --- a/server/src/test/java/org/elasticsearch/common/xcontent/support/XContentHelperTests.java +++ b/server/src/test/java/org/elasticsearch/common/xcontent/support/XContentHelperTests.java @@ -29,6 +29,7 @@ import java.util.Map; import java.util.Set; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; public class XContentHelperTests extends ESTestCase { @@ -66,6 +67,38 @@ public void testMergingListValuesAreMapsOfOne() { assertThat(content, equalTo(expected)); } + public void testMergingDefaults() { + Map content = getMap("key1", "content", "key3", "content"); + Map defaults = getMap("key2", "default", "key3", "default"); + Map expected = getMap("key1", "content", "key2", "default", "key3", "content"); + XContentHelper.mergeDefaults(content, defaults); + assertThat(content, equalTo(expected)); + } + + public void testMergingWithCustomMerge() { + Map content = getMap("key1", "old", "key3", "old", "key4", "old"); + Map defaults = getMap("key2", "new", "key3", "new", "key4", "new"); + Map expected = getMap("key1", "old", "key2", "new", "key3", "new", "key4", "old"); + XContentHelper.merge(content, defaults, (parent, key, oldValue, newValue) -> "key3".equals(key) ? newValue : oldValue); + assertThat(content, equalTo(expected)); + } + + public void testMergingWithCustomMergeWithException() { + final Map content = getMap("key1", "old", "key3", "old", "key4", "old"); + final Map defaults = getMap("key2", "new", "key3", "new", "key4", "new"); + final XContentHelper.CustomMerge customMerge = (parent, key, oldValue, newValue) -> { + if ("key3".equals(key)) { + throw new IllegalArgumentException(key + " is not allowed"); + } + return oldValue; + }; + IllegalArgumentException e = expectThrows( + IllegalArgumentException.class, + () -> XContentHelper.merge(content, defaults, customMerge) + ); + assertThat(e.getMessage(), containsString("key3 is not allowed")); + } + public void testToXContent() throws IOException { final XContentType xContentType = randomFrom(XContentType.values()); final ToXContent toXContent; diff --git a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java index 9c631b7044717..35c7e95a315eb 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java @@ -463,7 +463,7 @@ public void testMergeContradictingSubobjects() throws IOException { MapperParsingException.class, () -> mapperService.merge("_doc", List.of(mapping2, mapping1, subobjectsTrueExplicitly), MergeReason.INDEX_TEMPLATE) ); - assertEquals("Failed to parse mappings: contradicting subobjects settings provided", e.getMessage()); + assertEquals("Failed to parse mappings: contradicting subobjects settings provided for field: _doc", e.getMessage()); } private static CompressedXContent createTestMapping1() throws IOException { @@ -507,4 +507,49 @@ private static CompressedXContent createTestMapping2() throws IOException { } return mapping2; } + + public void testSubobjectsDisabledNotAtRoot() throws IOException { + final MapperService mapperService = createMapperService(mapping(b -> {})); + CompressedXContent mapping1; + try (XContentBuilder xContentBuilder = XContentFactory.jsonBuilder()) { + mapping1 = new CompressedXContent( + BytesReference.bytes( + xContentBuilder.startObject() + .startObject("_doc") + .startObject("properties") + .startObject("parent") + .field("subobjects", false) + .field("type", "object") + .endObject() + .endObject() + .endObject() + .endObject() + ) + ); + } + CompressedXContent mapping2; + try (XContentBuilder xContentBuilder = XContentFactory.jsonBuilder()) { + mapping2 = new CompressedXContent( + BytesReference.bytes( + xContentBuilder.startObject() + .startObject("_doc") + .startObject("properties") + .startObject("parent") + .startObject("properties") + .startObject("child.grandchild") + .field("type", "text") + .endObject() + .endObject() + .endObject() + .endObject() + .endObject() + .endObject() + ) + ); + } + + DocumentMapper subobjectsFirst = mapperService.merge("_doc", List.of(mapping1, mapping2), MergeReason.INDEX_TEMPLATE); + DocumentMapper subobjectsLast = mapperService.merge("_doc", List.of(mapping2, mapping1), MergeReason.INDEX_TEMPLATE); + assertEquals(subobjectsFirst.mappingSource(), subobjectsLast.mappingSource()); + } } From 699943ee925451f24f3ff16ed960947a2d06c0d9 Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Mon, 7 Aug 2023 07:22:31 +0300 Subject: [PATCH 07/38] Spoteless apply --- .../java/org/elasticsearch/index/mapper/MappingParser.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MappingParser.java b/server/src/main/java/org/elasticsearch/index/mapper/MappingParser.java index 1b5463ba965ca..2a07ea6281129 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MappingParser.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MappingParser.java @@ -93,8 +93,7 @@ Mapping parse(@Nullable String type, CompressedXContent source) throws MapperPar * @throws MapperParsingException in case of parsing error */ @SuppressWarnings("unchecked") - Mapping parse(@Nullable String type, Map mappingSource) - throws MapperParsingException { + Mapping parse(@Nullable String type, Map mappingSource) throws MapperParsingException { if (mappingSource.isEmpty()) { if (type == null) { throw new MapperParsingException("malformed mappingSource, no type name found"); From 90e9634710599e4b05325b9356edeab28be2dabd Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Mon, 7 Aug 2023 09:46:07 +0300 Subject: [PATCH 08/38] Use type that supports ignore_malformed in test --- .../java/org/elasticsearch/datastreams/DataStreamIT.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/data-streams/src/internalClusterTest/java/org/elasticsearch/datastreams/DataStreamIT.java b/modules/data-streams/src/internalClusterTest/java/org/elasticsearch/datastreams/DataStreamIT.java index 803c63d1836d7..69e8443b8238c 100644 --- a/modules/data-streams/src/internalClusterTest/java/org/elasticsearch/datastreams/DataStreamIT.java +++ b/modules/data-streams/src/internalClusterTest/java/org/elasticsearch/datastreams/DataStreamIT.java @@ -514,7 +514,7 @@ public void testTimeStampValidationInvalidFieldMapping() throws Exception { { "properties": { "@timestamp": { - "type": "keyword" + "type": "long" } } }"""; @@ -538,7 +538,7 @@ public void testTimeStampValidationInvalidFieldMapping() throws Exception { ); assertThat( e.getCause().getCause().getMessage(), - equalTo("data stream timestamp field [@timestamp] is of type [keyword], but [date,date_nanos] is expected") + equalTo("data stream timestamp field [@timestamp] is of type [long], but [date,date_nanos] is expected") ); } From b28ab161dea33113a612520ee811ce0481af8478 Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Mon, 7 Aug 2023 10:36:36 +0300 Subject: [PATCH 09/38] Temporarily disable a test --- .../java/org/elasticsearch/datastreams/DataStreamIT.java | 1 + 1 file changed, 1 insertion(+) diff --git a/modules/data-streams/src/internalClusterTest/java/org/elasticsearch/datastreams/DataStreamIT.java b/modules/data-streams/src/internalClusterTest/java/org/elasticsearch/datastreams/DataStreamIT.java index 69e8443b8238c..ae01c26fa00d6 100644 --- a/modules/data-streams/src/internalClusterTest/java/org/elasticsearch/datastreams/DataStreamIT.java +++ b/modules/data-streams/src/internalClusterTest/java/org/elasticsearch/datastreams/DataStreamIT.java @@ -1939,6 +1939,7 @@ public static void putComposableIndexTemplate(String id, List patterns) putComposableIndexTemplate(id, null, patterns, null, null); } + @AwaitsFix(bugUrl = "open an issue or remove") public void testPartitionedTemplate() throws IOException { /** * partition size with no routing required From 1eaf3d79db5abeeb7c6a12ddb384be8311f3a82c Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Mon, 7 Aug 2023 17:11:50 +0300 Subject: [PATCH 10/38] Handle routing settings --- .../datastreams/DataStreamIT.java | 40 +++++++++++++++---- .../MetadataIndexTemplateService.java | 9 +++-- .../index/mapper/MapperService.java | 9 +++-- .../index/mapper/MapperServiceTests.java | 2 +- .../elasticsearch/xpack/ccr/AutoFollowIT.java | 2 +- 5 files changed, 46 insertions(+), 16 deletions(-) diff --git a/modules/data-streams/src/internalClusterTest/java/org/elasticsearch/datastreams/DataStreamIT.java b/modules/data-streams/src/internalClusterTest/java/org/elasticsearch/datastreams/DataStreamIT.java index ae01c26fa00d6..384970bdc7ab9 100644 --- a/modules/data-streams/src/internalClusterTest/java/org/elasticsearch/datastreams/DataStreamIT.java +++ b/modules/data-streams/src/internalClusterTest/java/org/elasticsearch/datastreams/DataStreamIT.java @@ -1939,7 +1939,6 @@ public static void putComposableIndexTemplate(String id, List patterns) putComposableIndexTemplate(id, null, patterns, null, null); } - @AwaitsFix(bugUrl = "open an issue or remove") public void testPartitionedTemplate() throws IOException { /** * partition size with no routing required @@ -1989,18 +1988,13 @@ public void testPartitionedTemplate() throws IOException { ).actionGet(); /** - * routing enable with allow custom routing false + * routing settings with allow custom routing false */ template = new ComposableIndexTemplate( List.of("logs"), new Template( Settings.builder().put("index.number_of_shards", "3").put("index.routing_partition_size", "2").build(), - new CompressedXContent(""" - { - "_routing": { - "required": true - } - }"""), + null, null ), null, @@ -2025,6 +2019,36 @@ public void testPartitionedTemplate() throws IOException { ); } + public void testRoutingEnabledInMappingDisabledInDataStreamTemplate() throws IOException { + ComposableIndexTemplate template = new ComposableIndexTemplate( + List.of("logs"), + new Template( + Settings.builder().put("index.number_of_shards", "3").put("index.routing_partition_size", "2").build(), + new CompressedXContent(""" + { + "_routing": { + "required": true + } + }"""), + null + ), + null, + null, + null, + null, + new ComposableIndexTemplate.DataStreamTemplate(false, false) + ); + Exception e = expectThrows( + IllegalArgumentException.class, + () -> client().execute( + PutComposableIndexTemplateAction.INSTANCE, + new PutComposableIndexTemplateAction.Request("my-it").indexTemplate(template) + ).actionGet() + ); + Exception actualException = (Exception) e.getCause(); + assertTrue(Throwables.getRootCause(actualException).getMessage().contains("contradicting `_routing.required` settings")); + } + public void testSearchWithRouting() throws IOException, ExecutionException, InterruptedException { /** * partition size with routing required 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 28be54e64e2dd..b881fa73a6513 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java @@ -84,7 +84,7 @@ public class MetadataIndexTemplateService { public static final String DEFAULT_TIMESTAMP_FIELD = "@timestamp"; - public static final CompressedXContent DEFAULT_TIMESTAMP_MAPPING; + public static final CompressedXContent DEFAULT_TIMESTAMP_MAPPING_WITHOUT_ROUTING; private static final CompressedXContent DEFAULT_TIMESTAMP_MAPPING_WITH_ROUTING; @@ -96,8 +96,11 @@ public class MetadataIndexTemplateService { Map.of("type", DateFieldMapper.CONTENT_TYPE, "ignore_malformed", "false") ); try { - DEFAULT_TIMESTAMP_MAPPING = new CompressedXContent( + DEFAULT_TIMESTAMP_MAPPING_WITHOUT_ROUTING = new CompressedXContent( (builder, params) -> builder.startObject(MapperService.SINGLE_MAPPING_NAME) + .startObject(RoutingFieldMapper.NAME) + .field("required", false) + .endObject() .field("properties", defaultTimestampField) .endObject() ); @@ -1303,7 +1306,7 @@ public static List collectMappings( if (template.getDataStreamTemplate().isAllowCustomRouting()) { mappings.add(0, DEFAULT_TIMESTAMP_MAPPING_WITH_ROUTING); } else { - mappings.add(0, DEFAULT_TIMESTAMP_MAPPING); + mappings.add(0, DEFAULT_TIMESTAMP_MAPPING_WITHOUT_ROUTING); } } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index 5d4de2b05bc4c..025d9383d5de6 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -398,9 +398,12 @@ public DocumentMapper merge(String type, List mappingSources } case "subobjects" -> { if (oldValue.equals(newValue) == false) { - throw new MapperParsingException( - "Failed to parse mappings: contradicting subobjects settings provided for field: " + parent - ); + throw new MapperParsingException("contradicting subobjects settings provided for field: " + parent); + } + } + case "required" -> { + if ("_routing".equals(parent) && oldValue != newValue) { + throw new MapperParsingException("contradicting `_routing.required` settings"); } } } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java index 35c7e95a315eb..0ed7b4b1ccf97 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java @@ -463,7 +463,7 @@ public void testMergeContradictingSubobjects() throws IOException { MapperParsingException.class, () -> mapperService.merge("_doc", List.of(mapping2, mapping1, subobjectsTrueExplicitly), MergeReason.INDEX_TEMPLATE) ); - assertEquals("Failed to parse mappings: contradicting subobjects settings provided for field: _doc", e.getMessage()); + assertEquals("contradicting subobjects settings provided for field: _doc", e.getMessage()); } private static CompressedXContent createTestMapping1() throws IOException { diff --git a/x-pack/plugin/ccr/src/internalClusterTest/java/org/elasticsearch/xpack/ccr/AutoFollowIT.java b/x-pack/plugin/ccr/src/internalClusterTest/java/org/elasticsearch/xpack/ccr/AutoFollowIT.java index ee461ba9a562a..5896ecacb9ca8 100644 --- a/x-pack/plugin/ccr/src/internalClusterTest/java/org/elasticsearch/xpack/ccr/AutoFollowIT.java +++ b/x-pack/plugin/ccr/src/internalClusterTest/java/org/elasticsearch/xpack/ccr/AutoFollowIT.java @@ -693,7 +693,7 @@ public void testAutoFollowDatastreamWithClosingFollowerIndex() throws Exception leaderClient().admin() .indices() .prepareCreate(indexInDatastream) - .setMapping(MetadataIndexTemplateService.DEFAULT_TIMESTAMP_MAPPING.toString()) + .setMapping(MetadataIndexTemplateService.DEFAULT_TIMESTAMP_MAPPING_WITHOUT_ROUTING.toString()) .get() ); leaderClient().prepareIndex(indexInDatastream) From 20737315f26add389e26ea2eb6ee93637c4f58bb Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Mon, 7 Aug 2023 17:45:51 +0300 Subject: [PATCH 11/38] Adjust tests to use explicit routing disabling --- .../MetadataIndexTemplateServiceTests.java | 21 ++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) 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 678be75490938..2a965d003f40b 100644 --- a/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateServiceTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateServiceTests.java @@ -1252,7 +1252,12 @@ public void testDefinedTimestampMappingIsAddedForDataStreamTemplates() throws Ex equalTo( Map.of( "_doc", - Map.of("properties", Map.of(DEFAULT_TIMESTAMP_FIELD, Map.of("type", "date", "ignore_malformed", "false"))) + Map.of( + "properties", + Map.of(DEFAULT_TIMESTAMP_FIELD, Map.of("type", "date", "ignore_malformed", "false")), + "_routing", + Map.of("required", false) + ) ) ) ); @@ -1364,7 +1369,12 @@ public void testUserDefinedMappingTakesPrecedenceOverDefault() throws Exception equalTo( Map.of( "_doc", - Map.of("properties", Map.of(DEFAULT_TIMESTAMP_FIELD, Map.of("type", "date", "ignore_malformed", "false"))) + Map.of( + "properties", + Map.of(DEFAULT_TIMESTAMP_FIELD, Map.of("type", "date", "ignore_malformed", "false")), + "_routing", + Map.of("required", false) + ) ) ) ); @@ -1418,7 +1428,12 @@ public void testUserDefinedMappingTakesPrecedenceOverDefault() throws Exception equalTo( Map.of( "_doc", - Map.of("properties", Map.of(DEFAULT_TIMESTAMP_FIELD, Map.of("type", "date", "ignore_malformed", "false"))) + Map.of( + "properties", + Map.of(DEFAULT_TIMESTAMP_FIELD, Map.of("type", "date", "ignore_malformed", "false")), + "_routing", + Map.of("required", false) + ) ) ) ); From f5f457cee04f9c968e3451eaa0814415a336c46c Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Tue, 8 Aug 2023 13:15:08 +0300 Subject: [PATCH 12/38] Applying review comments --- .../index/mapper/MapperService.java | 3 +-- .../index/mapper/MappingParser.java | 3 +-- .../index/mapper/ObjectMapper.java | 22 ------------------- 3 files changed, 2 insertions(+), 26 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index 025d9383d5de6..ccd4c28f0822a 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -452,8 +452,7 @@ public Mapping parseMapping(String mappingType, CompressedXContent mappingSource } /** - * A method to parse mapping from a source in a map form, that allows to specify explicit/implicit {@code subobjects} configuration. - * Since parsing is affected by the {@code subobjects} setting, the resulted mapping may change according to this setting. + * A method to parse mapping from a source in a map form. * * @param mappingType the mapping type * @param mappingSource mapping source already converted to a map form, but not yet processed otherwise diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MappingParser.java b/server/src/main/java/org/elasticsearch/index/mapper/MappingParser.java index 2a07ea6281129..19710ecbfddba 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MappingParser.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MappingParser.java @@ -84,8 +84,7 @@ Mapping parse(@Nullable String type, CompressedXContent source) throws MapperPar } /** - * A method to parse mapping from a source in a map form, that allows to specify explicit/implicit {@code subobjects} configuration. - * Since parsing is affected by the {@code subobjects} setting, the resulted mapping may change according to this setting. + * A method to parse mapping from a source in a map form. * * @param type the mapping type * @param mappingSource mapping source already converted to a map form, but not yet processed otherwise diff --git a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java index 1e03ef9f56aab..ee78fc6a27464 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java @@ -14,7 +14,6 @@ import org.elasticsearch.common.logging.DeprecationCategory; import org.elasticsearch.common.logging.DeprecationLogger; import org.elasticsearch.common.xcontent.support.XContentMapValues; -import org.elasticsearch.core.Nullable; import org.elasticsearch.index.IndexVersion; import org.elasticsearch.index.mapper.MapperService.MergeReason; import org.elasticsearch.xcontent.ToXContent; @@ -590,27 +589,6 @@ protected void doXContent(XContentBuilder builder, Params params) throws IOExcep } - /** - * Checks the {@code subobjects} config in provided mapping's root. - * @param mapping a raw mapping in a map form - * @return an explicit {@code subobjects} value if such is configured directly under the root, or {@code null} otherwise - */ - @Nullable - static Explicit checkSubobjectsInMappingRoot(Map mapping) { - if (mapping.size() == 1) { - String rootName = mapping.keySet().iterator().next(); - Object childNode = mapping.get(rootName); - if (childNode instanceof Map) { - @SuppressWarnings("unchecked") - Object subobjectsNode = ((Map) mapping.get(rootName)).get("subobjects"); - if (subobjectsNode != null) { - return Explicit.explicitBoolean(XContentMapValues.nodeBooleanValue(subobjectsNode, "subobjects.subobjects")); - } - } - } - return null; - } - public SourceLoader.SyntheticFieldLoader syntheticFieldLoader(Stream extra) { return new SyntheticSourceFieldLoader( Stream.concat(extra, mappers.values().stream()) From 0e347b76130dd0d1e11935719c44447e166fdeb4 Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Tue, 8 Aug 2023 16:43:15 +0300 Subject: [PATCH 13/38] Adding test case and fix for CreateIndex as well --- .../15_composition.yml | 67 ++++++++++++++++++- .../metadata/MetadataCreateIndexService.java | 9 +-- 2 files changed, 69 insertions(+), 7 deletions(-) diff --git a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.put_index_template/15_composition.yml b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.put_index_template/15_composition.yml index 578079eb06536..2b90935b2358a 100644 --- a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.put_index_template/15_composition.yml +++ b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.put_index_template/15_composition.yml @@ -351,7 +351,7 @@ ignore_missing_component_templates: ["foo"] --- -"Composable index templates that include subobjects: false": +"Composable index templates that include subobjects: false at root": - skip: version: ' - 8.9.1' reason: 'https://github.com/elastic/elasticsearch/issues/96768 fixed after 8.9.0' @@ -377,9 +377,9 @@ - do: allowed_warnings: - - "index template [logs-composable-template] has index patterns [test-*] matching patterns from existing older templates [global] with patterns (global => [*]); this template [logs-composable-template] will take precedence during new index creation" + - "index template [test-composable-template] has index patterns [test-*] matching patterns from existing older templates [global] with patterns (global => [*]); this template [test-composable-template] will take precedence during new index creation" indices.put_index_template: - name: logs-composable-template + name: test-composable-template body: index_patterns: - test-* @@ -387,3 +387,64 @@ - test-subobjects - test-field - is_true: acknowledged + + - do: + indices.create: + index: test-generic + + - do: + indices.get_mapping: + index: test-generic + - match: { test-generic.mappings.properties.parent\.subfield.type: "keyword" } + +--- +"Composable index templates that include subobjects: false on arbitrary field": + - skip: + version: ' - 8.9.1' + reason: 'https://github.com/elastic/elasticsearch/issues/96768 fixed after 8.9.0' + features: allowed_warnings + + - do: + cluster.put_component_template: + name: test-subobjects + body: + template: + mappings: + properties: + parent: + type: object + subobjects: false + + - do: + cluster.put_component_template: + name: test-subfield + body: + template: + mappings: + properties: + parent: + properties: + child.grandchild: + type: keyword + + - do: + allowed_warnings: + - "index template [test-composable-template] has index patterns [test-*] matching patterns from existing older templates [global] with patterns (global => [*]); this template [test-composable-template] will take precedence during new index creation" + indices.put_index_template: + name: test-composable-template + body: + index_patterns: + - test-* + composed_of: + - test-subobjects + - test-subfield + - is_true: acknowledged + + - do: + indices.create: + index: test-generic + + - do: + indices.get_mapping: + index: test-generic + - match: { test-generic.mappings.properties.parent.properties.child\.grandchild.type: "keyword" } 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 763e3d3574752..5d649b0a0cfbb 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java @@ -1331,13 +1331,14 @@ private static void updateIndexMappingsAndBuildSortOrder( ) throws IOException { MapperService mapperService = indexService.mapperService(); IndexMode indexMode = indexService.getIndexSettings() != null ? indexService.getIndexSettings().getMode() : IndexMode.STANDARD; + List allMappings = new ArrayList<>(); final CompressedXContent defaultMapping = indexMode.getDefaultMapping(); if (defaultMapping != null) { - mapperService.merge(MapperService.SINGLE_MAPPING_NAME, defaultMapping, MergeReason.INDEX_TEMPLATE); - } - for (CompressedXContent mapping : mappings) { - mapperService.merge(MapperService.SINGLE_MAPPING_NAME, mapping, MergeReason.INDEX_TEMPLATE); + allMappings.add(defaultMapping); } + allMappings.addAll(mappings); + mapperService.merge(MapperService.SINGLE_MAPPING_NAME, allMappings, MergeReason.INDEX_TEMPLATE); + indexMode.validateTimestampFieldMapping(request.dataStreamName() != null, mapperService.mappingLookup()); if (sourceMetadata == null) { From e23b2f245264d6adaa863bc872794d6370616d41 Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Tue, 8 Aug 2023 17:00:57 +0300 Subject: [PATCH 14/38] spotless --- .../cluster/metadata/MetadataCreateIndexService.java | 2 +- 1 file changed, 1 insertion(+), 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 5d649b0a0cfbb..3302549a1b860 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java @@ -1338,7 +1338,7 @@ private static void updateIndexMappingsAndBuildSortOrder( } allMappings.addAll(mappings); mapperService.merge(MapperService.SINGLE_MAPPING_NAME, allMappings, MergeReason.INDEX_TEMPLATE); - + indexMode.validateTimestampFieldMapping(request.dataStreamName() != null, mapperService.mappingLookup()); if (sourceMetadata == null) { From 6db4fa0d756608aa03a7735ff67c8366c46f13cf Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Tue, 8 Aug 2023 17:08:42 +0300 Subject: [PATCH 15/38] Fix test- use type that supports ignore_malformed --- .../rest-api-spec/test/tsdb/15_timestamp_mapping.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/tsdb/15_timestamp_mapping.yml b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/tsdb/15_timestamp_mapping.yml index dbc8076c0a1a8..21b5399b0eaad 100644 --- a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/tsdb/15_timestamp_mapping.yml +++ b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/tsdb/15_timestamp_mapping.yml @@ -148,7 +148,7 @@ reject @timestamp with wrong type: reason: introduced in 8.1.0 - do: - catch: /data stream timestamp field \[@timestamp\] is of type \[keyword\], but \[date,date_nanos\] is expected/ + catch: /data stream timestamp field \[@timestamp\] is of type \[long\], but \[date,date_nanos\] is expected/ indices.create: index: test body: @@ -163,7 +163,7 @@ reject @timestamp with wrong type: mappings: properties: "@timestamp": - type: keyword + type: long --- reject timestamp meta field with wrong type: From ad1d2832ff9875e63567400c35184c8f1b298d24 Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Wed, 9 Aug 2023 13:00:24 +0300 Subject: [PATCH 16/38] Extract mapping under root before merging --- .../common/xcontent/XContentHelper.java | 25 +++++++++++++------ .../index/mapper/MapperService.java | 13 +++++++++- .../index/mapper/MappingParser.java | 6 ++++- 3 files changed, 35 insertions(+), 9 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/xcontent/XContentHelper.java b/server/src/main/java/org/elasticsearch/common/xcontent/XContentHelper.java index e18654c9b786b..30e3d6f212218 100644 --- a/server/src/main/java/org/elasticsearch/common/xcontent/XContentHelper.java +++ b/server/src/main/java/org/elasticsearch/common/xcontent/XContentHelper.java @@ -424,14 +424,25 @@ public static void mergeDefaults(Map content, Map first, Map second, @Nullable CustomMerge customMerge) { - merge(first, second, customMerge, null); + merge(null, first, second, customMerge); } - private static void merge( + /** + * Merges the map provided as the second parameter into the content of the first. Only does recursive merge for inner maps. + * If a non-null {@link CustomMerge} is provided, it is applied whenever a merge is required, meaning - whenever both the first and + * the second map has values for the same key. Otherwise, values from the first map will always have precedence, meaning - if the + * first map contains a key, its value will not be overriden. + * + * @param parent used for recursion to maintain knowledge about the common parent of the currently merged sub-maps + * @param first the map which serves as the merge base + * @param second the map of which contents are merged into the base map + * @param customMerge a custom merge rule to apply whenever a key has concrete values (i.e. not a map or a collection) in both maps + */ + public static void merge( + @Nullable String parent, Map first, Map second, - @Nullable CustomMerge customMerge, - @Nullable String parent + @Nullable CustomMerge customMerge ) { for (Map.Entry toMergeEntry : second.entrySet()) { if (first.containsKey(toMergeEntry.getKey()) == false) { @@ -442,10 +453,10 @@ private static void merge( Object baseValue = first.get(toMergeEntry.getKey()); if (baseValue instanceof Map && toMergeEntry.getValue() instanceof Map) { merge( + toMergeEntry.getKey(), (Map) baseValue, (Map) toMergeEntry.getValue(), - customMerge, - toMergeEntry.getKey() + customMerge ); } else if (baseValue instanceof List && toMergeEntry.getValue() instanceof List) { List listToMerge = (List) toMergeEntry.getValue(); @@ -463,7 +474,7 @@ private static void merge( Map map = (Map) o; Map.Entry entry = map.entrySet().iterator().next(); if (processed.containsKey(entry.getKey())) { - merge(processed.get(entry.getKey()), map, customMerge, toMergeEntry.getKey()); + merge(toMergeEntry.getKey(), processed.get(entry.getKey()), map, customMerge); } else { // append the second list's entries after the first list's entries. processed.put(entry.getKey(), map); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index ccd4c28f0822a..f3338e19a79b8 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -40,6 +40,7 @@ import java.io.IOException; import java.io.InputStream; import java.util.Collections; +import java.util.Iterator; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -372,6 +373,7 @@ public void merge(IndexMetadata indexMetadata, MergeReason reason) { * Merging the provided mappings. Actual merging is done in the raw, non-parsed, form of the mappings. This allows to do a proper bulk * merge, where parsing is done only when all raw mapping settings are already merged. */ + @SuppressWarnings("unchecked") public DocumentMapper merge(String type, List mappingSources, MergeReason reason) { final DocumentMapper currentMapper = this.mapper; if (currentMapper != null && mappingSources.size() == 1 && currentMapper.mappingSource().equals(mappingSources.get(0))) { @@ -381,10 +383,17 @@ public DocumentMapper merge(String type, List mappingSources Map mergedRawMapping = null; for (CompressedXContent mappingSource : mappingSources) { Map rawMapping = MappingParser.convertToMap(mappingSource); + Iterator iterator = rawMapping.keySet().iterator(); + if (iterator.hasNext()) { + String rootName = iterator.next(); + if (mappingParser.isRootMappingType(rootName, type)) { + rawMapping = (Map) rawMapping.get(rootName); + } + } if (mergedRawMapping == null) { mergedRawMapping = rawMapping; } else { - XContentHelper.merge(mergedRawMapping, rawMapping, ((parent, key, oldValue, newValue) -> { + XContentHelper.merge(type, mergedRawMapping, rawMapping, ((parent, key, oldValue, newValue) -> { switch (key) { case "type" -> { // todo: verify this check is valid @@ -411,6 +420,8 @@ public DocumentMapper merge(String type, List mappingSources })); } } + // todo: doMerge will apply parsing, which expects a single root in the provided map. Should we fail here if there is more than + // one root? return (mergedRawMapping != null) ? doMerge(type, reason, mergedRawMapping) : null; } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MappingParser.java b/server/src/main/java/org/elasticsearch/index/mapper/MappingParser.java index 19710ecbfddba..529e5d80d98f9 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MappingParser.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MappingParser.java @@ -99,7 +99,7 @@ Mapping parse(@Nullable String type, Map mappingSource) throws M } } else { String rootName = mappingSource.keySet().iterator().next(); - if (type == null || type.equals(rootName) || documentTypeResolver.apply(type).equals(rootName)) { + if (type == null || isRootMappingType(rootName, type)) { type = rootName; mappingSource = (Map) mappingSource.get(rootName); } @@ -177,4 +177,8 @@ Mapping parse(@Nullable String type, Map mappingSource) throws M meta ); } + + boolean isRootMappingType(String rootName, String type) { + return type.equals(rootName) || documentTypeResolver.apply(type).equals(rootName); + } } From 83347bd983486f8f04d1dac293ffd978a0fa87ea Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Wed, 9 Aug 2023 16:29:34 +0300 Subject: [PATCH 17/38] Better normalization of multiple sources --- .../common/xcontent/XContentHelper.java | 2 +- .../index/mapper/MapperService.java | 23 ++-- .../index/mapper/MappingParser.java | 6 +- .../index/mapper/MapperServiceTests.java | 123 ++++++++++++++++++ 4 files changed, 139 insertions(+), 15 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/xcontent/XContentHelper.java b/server/src/main/java/org/elasticsearch/common/xcontent/XContentHelper.java index 30e3d6f212218..3a8e4fd06656d 100644 --- a/server/src/main/java/org/elasticsearch/common/xcontent/XContentHelper.java +++ b/server/src/main/java/org/elasticsearch/common/xcontent/XContentHelper.java @@ -433,7 +433,7 @@ public static void merge(Map first, Map second, * the second map has values for the same key. Otherwise, values from the first map will always have precedence, meaning - if the * first map contains a key, its value will not be overriden. * - * @param parent used for recursion to maintain knowledge about the common parent of the currently merged sub-maps + * @param parent used for recursion to maintain knowledge about the common parent of the currently merged sub-maps, if such exists * @param first the map which serves as the merge base * @param second the map of which contents are merged into the base map * @param customMerge a custom merge rule to apply whenever a key has concrete values (i.e. not a map or a collection) in both maps diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index f3338e19a79b8..a69a71a1ea0d7 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -40,7 +40,6 @@ import java.io.IOException; import java.io.InputStream; import java.util.Collections; -import java.util.Iterator; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -373,7 +372,6 @@ public void merge(IndexMetadata indexMetadata, MergeReason reason) { * Merging the provided mappings. Actual merging is done in the raw, non-parsed, form of the mappings. This allows to do a proper bulk * merge, where parsing is done only when all raw mapping settings are already merged. */ - @SuppressWarnings("unchecked") public DocumentMapper merge(String type, List mappingSources, MergeReason reason) { final DocumentMapper currentMapper = this.mapper; if (currentMapper != null && mappingSources.size() == 1 && currentMapper.mappingSource().equals(mappingSources.get(0))) { @@ -383,13 +381,19 @@ public DocumentMapper merge(String type, List mappingSources Map mergedRawMapping = null; for (CompressedXContent mappingSource : mappingSources) { Map rawMapping = MappingParser.convertToMap(mappingSource); - Iterator iterator = rawMapping.keySet().iterator(); - if (iterator.hasNext()) { - String rootName = iterator.next(); - if (mappingParser.isRootMappingType(rootName, type)) { - rawMapping = (Map) rawMapping.get(rootName); + if (rawMapping.isEmpty()) { + continue; + } + + // normalize mappings, making sure that all have the provided type as a single root + if (rawMapping.containsKey(type)) { + if (rawMapping.size() > 1) { + throw new MapperParsingException("cannot merge a map with multiple roots, one of which is [" + type + "]"); } + } else { + rawMapping = Map.of(type, rawMapping); } + if (mergedRawMapping == null) { mergedRawMapping = rawMapping; } else { @@ -420,8 +424,9 @@ public DocumentMapper merge(String type, List mappingSources })); } } - // todo: doMerge will apply parsing, which expects a single root in the provided map. Should we fail here if there is more than - // one root? + if (mergedRawMapping != null && mergedRawMapping.size() > 1) { + throw new MapperParsingException("cannot merge mapping sources with different roots"); + } return (mergedRawMapping != null) ? doMerge(type, reason, mergedRawMapping) : null; } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MappingParser.java b/server/src/main/java/org/elasticsearch/index/mapper/MappingParser.java index 529e5d80d98f9..19710ecbfddba 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MappingParser.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MappingParser.java @@ -99,7 +99,7 @@ Mapping parse(@Nullable String type, Map mappingSource) throws M } } else { String rootName = mappingSource.keySet().iterator().next(); - if (type == null || isRootMappingType(rootName, type)) { + if (type == null || type.equals(rootName) || documentTypeResolver.apply(type).equals(rootName)) { type = rootName; mappingSource = (Map) mappingSource.get(rootName); } @@ -177,8 +177,4 @@ Mapping parse(@Nullable String type, Map mappingSource) throws M meta ); } - - boolean isRootMappingType(String rootName, String type) { - return type.equals(rootName) || documentTypeResolver.apply(type).equals(rootName); - } } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java index 0ed7b4b1ccf97..f285539903bcd 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java @@ -552,4 +552,127 @@ public void testSubobjectsDisabledNotAtRoot() throws IOException { DocumentMapper subobjectsLast = mapperService.merge("_doc", List.of(mapping2, mapping1), MergeReason.INDEX_TEMPLATE); assertEquals(subobjectsFirst.mappingSource(), subobjectsLast.mappingSource()); } + + public void testMergeMultipleRoots() throws IOException { + final MapperService mapperService = createMapperService(mapping(b -> {})); + + CompressedXContent mapping1 = new CompressedXContent(""" + { + "properties" : { + "field" : { + "subobjects" : false, + "type" : "object" + } + } + } + """); + + CompressedXContent mapping2 = new CompressedXContent(""" + { + "_doc" : { + "_meta" : { + "meta-field" : "some-info" + }, + "properties" : { + "field" : { + "properties" : { + "subfield" : { + "type" : "keyword" + } + } + } + } + } + }"""); + + mapperService.merge("_doc", List.of(mapping1, mapping2), MergeReason.INDEX_TEMPLATE); + assertEquals(""" + { + "_doc" : { + "_meta" : { + "meta-field" : "some-info" + }, + "properties" : { + "field" : { + "subobjects" : false, + "properties" : { + "subfield" : { + "type" : "keyword" + } + } + } + } + } + }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); + } + + public void testMergeMultipleRootsWithRootType() throws IOException { + final MapperService mapperService = createMapperService(mapping(b -> {})); + + CompressedXContent mapping1 = new CompressedXContent(""" + { + "properties" : { + "field" : { + "type" : "keyword" + } + } + } + """); + + CompressedXContent mapping2 = new CompressedXContent(""" + { + "_doc" : { + "_meta" : { + "meta-field" : "some-info" + } + }, + "properties" : { + "field" : { + "subobjects" : false + } + } + }"""); + + MapperParsingException e = expectThrows( + MapperParsingException.class, + () -> mapperService.merge("_doc", List.of(mapping1, mapping2), MergeReason.INDEX_TEMPLATE) + ); + assertThat(e.getMessage(), containsString("cannot merge a map with multiple roots, one of which is [_doc]")); + } + + public void testMergeMultipleRootsWithoutRootType() throws IOException { + final MapperService mapperService = createMapperService(mapping(b -> {})); + + CompressedXContent mapping1 = new CompressedXContent(""" + { + "properties" : { + "field" : { + "type" : "keyword" + } + } + } + """); + + CompressedXContent mapping2 = new CompressedXContent(""" + { + "_meta" : { + "meta-field" : "some-info" + } + }"""); + + mapperService.merge("_doc", List.of(mapping1, mapping2), MergeReason.INDEX_TEMPLATE); + assertEquals(""" + { + "_doc" : { + "_meta" : { + "meta-field" : "some-info" + }, + "properties" : { + "field" : { + "type" : "keyword" + } + } + } + }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); + } } From a865e25cb1eaa32f7a2d470954d46994dd541626 Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Sun, 13 Aug 2023 15:22:16 +0300 Subject: [PATCH 18/38] Never return null map from merge --- .../index/mapper/MapperService.java | 53 +++++++++---------- 1 file changed, 25 insertions(+), 28 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index a69a71a1ea0d7..3e369719a3f20 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -40,6 +40,7 @@ import java.io.IOException; import java.io.InputStream; import java.util.Collections; +import java.util.HashMap; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -378,7 +379,7 @@ public DocumentMapper merge(String type, List mappingSources return currentMapper; } - Map mergedRawMapping = null; + Map mergedRawMapping = new HashMap<>(); for (CompressedXContent mappingSource : mappingSources) { Map rawMapping = MappingParser.convertToMap(mappingSource); if (rawMapping.isEmpty()) { @@ -394,40 +395,36 @@ public DocumentMapper merge(String type, List mappingSources rawMapping = Map.of(type, rawMapping); } - if (mergedRawMapping == null) { - mergedRawMapping = rawMapping; - } else { - XContentHelper.merge(type, mergedRawMapping, rawMapping, ((parent, key, oldValue, newValue) -> { - switch (key) { - case "type" -> { - // todo: verify this check is valid - if (oldValue.equals(newValue) == false - && (OBJECT_FIELD_TYPES.contains(String.valueOf(oldValue)) - || OBJECT_FIELD_TYPES.contains(String.valueOf(newValue)))) { - throw new MapperParsingException( - "can't merge a non object mapping [" + parent + "] with an object mapping" - ); - } + XContentHelper.merge(type, mergedRawMapping, rawMapping, ((parent, key, oldValue, newValue) -> { + switch (key) { + case "type" -> { + // todo: verify this check is valid + if (oldValue.equals(newValue) == false + && (OBJECT_FIELD_TYPES.contains(String.valueOf(oldValue)) + || OBJECT_FIELD_TYPES.contains(String.valueOf(newValue)))) { + throw new MapperParsingException( + "can't merge a non object mapping [" + parent + "] with an object mapping" + ); } - case "subobjects" -> { - if (oldValue.equals(newValue) == false) { - throw new MapperParsingException("contradicting subobjects settings provided for field: " + parent); - } + } + case "subobjects" -> { + if (oldValue.equals(newValue) == false) { + throw new MapperParsingException("contradicting subobjects settings provided for field: " + parent); } - case "required" -> { - if ("_routing".equals(parent) && oldValue != newValue) { - throw new MapperParsingException("contradicting `_routing.required` settings"); - } + } + case "required" -> { + if ("_routing".equals(parent) && oldValue != newValue) { + throw new MapperParsingException("contradicting `_routing.required` settings"); } } - return newValue; - })); - } + } + return newValue; + })); } - if (mergedRawMapping != null && mergedRawMapping.size() > 1) { + if (mergedRawMapping.size() > 1) { throw new MapperParsingException("cannot merge mapping sources with different roots"); } - return (mergedRawMapping != null) ? doMerge(type, reason, mergedRawMapping) : null; + return doMerge(type, reason, mergedRawMapping); } public DocumentMapper merge(String type, CompressedXContent mappingSource, MergeReason reason) { From 67bb863305b744cc32d3a316cec8d363a92161af Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Sun, 13 Aug 2023 15:37:38 +0300 Subject: [PATCH 19/38] Spotless again :-( --- .../java/org/elasticsearch/index/mapper/MapperService.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index 3e369719a3f20..215a35e14196c 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -401,10 +401,8 @@ public DocumentMapper merge(String type, List mappingSources // todo: verify this check is valid if (oldValue.equals(newValue) == false && (OBJECT_FIELD_TYPES.contains(String.valueOf(oldValue)) - || OBJECT_FIELD_TYPES.contains(String.valueOf(newValue)))) { - throw new MapperParsingException( - "can't merge a non object mapping [" + parent + "] with an object mapping" - ); + || OBJECT_FIELD_TYPES.contains(String.valueOf(newValue)))) { + throw new MapperParsingException("can't merge a non object mapping [" + parent + "] with an object mapping"); } } case "subobjects" -> { From 4b8d20a66c3c3b3b35b72031a25c6670697ed19d Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Mon, 14 Aug 2023 08:35:04 +0300 Subject: [PATCH 20/38] Restore null return, avoid empty map creation --- .../index/mapper/MapperService.java | 52 ++++++++++--------- 1 file changed, 27 insertions(+), 25 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index 215a35e14196c..773a6d4fc8239 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -40,7 +40,6 @@ import java.io.IOException; import java.io.InputStream; import java.util.Collections; -import java.util.HashMap; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -379,12 +378,9 @@ public DocumentMapper merge(String type, List mappingSources return currentMapper; } - Map mergedRawMapping = new HashMap<>(); + Map mergedRawMapping = null; for (CompressedXContent mappingSource : mappingSources) { Map rawMapping = MappingParser.convertToMap(mappingSource); - if (rawMapping.isEmpty()) { - continue; - } // normalize mappings, making sure that all have the provided type as a single root if (rawMapping.containsKey(type)) { @@ -395,34 +391,40 @@ public DocumentMapper merge(String type, List mappingSources rawMapping = Map.of(type, rawMapping); } - XContentHelper.merge(type, mergedRawMapping, rawMapping, ((parent, key, oldValue, newValue) -> { - switch (key) { - case "type" -> { - // todo: verify this check is valid - if (oldValue.equals(newValue) == false - && (OBJECT_FIELD_TYPES.contains(String.valueOf(oldValue)) + if (mergedRawMapping == null) { + mergedRawMapping = rawMapping; + } else { + XContentHelper.merge(type, mergedRawMapping, rawMapping, ((parent, key, oldValue, newValue) -> { + switch (key) { + case "type" -> { + // todo: verify this check is valid + if (oldValue.equals(newValue) == false + && (OBJECT_FIELD_TYPES.contains(String.valueOf(oldValue)) || OBJECT_FIELD_TYPES.contains(String.valueOf(newValue)))) { - throw new MapperParsingException("can't merge a non object mapping [" + parent + "] with an object mapping"); + throw new MapperParsingException( + "can't merge a non object mapping [" + parent + "] with an object mapping" + ); + } } - } - case "subobjects" -> { - if (oldValue.equals(newValue) == false) { - throw new MapperParsingException("contradicting subobjects settings provided for field: " + parent); + case "subobjects" -> { + if (oldValue.equals(newValue) == false) { + throw new MapperParsingException("contradicting subobjects settings provided for field: " + parent); + } } - } - case "required" -> { - if ("_routing".equals(parent) && oldValue != newValue) { - throw new MapperParsingException("contradicting `_routing.required` settings"); + case "required" -> { + if ("_routing".equals(parent) && oldValue != newValue) { + throw new MapperParsingException("contradicting `_routing.required` settings"); + } } } - } - return newValue; - })); + return newValue; + })); + } } - if (mergedRawMapping.size() > 1) { + if (mergedRawMapping != null && mergedRawMapping.size() > 1) { throw new MapperParsingException("cannot merge mapping sources with different roots"); } - return doMerge(type, reason, mergedRawMapping); + return (mergedRawMapping != null) ? doMerge(type, reason, mergedRawMapping) : null; } public DocumentMapper merge(String type, CompressedXContent mappingSource, MergeReason reason) { From 11942fc9697f3ba7021312a56f65d80e5132a4e8 Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Mon, 14 Aug 2023 09:04:43 +0300 Subject: [PATCH 21/38] My god this spotless is annoying --- .../main/java/org/elasticsearch/index/mapper/MapperService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index 773a6d4fc8239..65ecf20525688 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -400,7 +400,7 @@ public DocumentMapper merge(String type, List mappingSources // todo: verify this check is valid if (oldValue.equals(newValue) == false && (OBJECT_FIELD_TYPES.contains(String.valueOf(oldValue)) - || OBJECT_FIELD_TYPES.contains(String.valueOf(newValue)))) { + || OBJECT_FIELD_TYPES.contains(String.valueOf(newValue)))) { throw new MapperParsingException( "can't merge a non object mapping [" + parent + "] with an object mapping" ); From d6c8fa64ed0622bfb22c5c7589d28c2e4c98520e Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Mon, 14 Aug 2023 10:50:29 +0300 Subject: [PATCH 22/38] Standardize mapping type conflicts error messages --- .../index/mapper/MapperErrors.java | 19 +++++++++++++++++++ .../index/mapper/MapperService.java | 15 +++++++-------- .../index/mapper/NestedObjectMapper.java | 2 +- .../index/mapper/ObjectMapper.java | 12 ++++++------ .../index/mapper/DocumentMapperTests.java | 2 +- .../index/mapper/ObjectMapperTests.java | 10 +++++++--- 6 files changed, 41 insertions(+), 19 deletions(-) create mode 100644 server/src/main/java/org/elasticsearch/index/mapper/MapperErrors.java diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperErrors.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperErrors.java new file mode 100644 index 0000000000000..6097144d6a8ac --- /dev/null +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperErrors.java @@ -0,0 +1,19 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.index.mapper; + +public class MapperErrors { + static void throwObjectMappingConflictError(String fieldName) throws IllegalArgumentException { + throw new IllegalArgumentException("can't merge a non object mapping [" + fieldName + "] with an object mapping"); + } + + static void throwNestedMappingConflictError(String fieldName) throws IllegalArgumentException { + throw new IllegalArgumentException("can't merge a non nested mapping [" + fieldName + "] with a nested mapping"); + } +} diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index 65ecf20525688..1e0588ee3ff36 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -44,7 +44,6 @@ import java.util.List; import java.util.Map; import java.util.Objects; -import java.util.Set; import java.util.function.Function; import java.util.function.Supplier; @@ -76,7 +75,6 @@ public enum MergeReason { public static final String SINGLE_MAPPING_NAME = "_doc"; public static final String TYPE_FIELD_NAME = "_type"; - public static final Set OBJECT_FIELD_TYPES = Set.of(ObjectMapper.CONTENT_TYPE, NestedObjectMapper.CONTENT_TYPE); public static final Setting INDEX_MAPPING_NESTED_FIELDS_LIMIT_SETTING = Setting.longSetting( "index.mapping.nested_fields.limit", 50L, @@ -398,12 +396,13 @@ public DocumentMapper merge(String type, List mappingSources switch (key) { case "type" -> { // todo: verify this check is valid - if (oldValue.equals(newValue) == false - && (OBJECT_FIELD_TYPES.contains(String.valueOf(oldValue)) - || OBJECT_FIELD_TYPES.contains(String.valueOf(newValue)))) { - throw new MapperParsingException( - "can't merge a non object mapping [" + parent + "] with an object mapping" - ); + if (oldValue.equals(newValue) == false) { + if (oldValue.equals(ObjectMapper.CONTENT_TYPE) || newValue.equals(ObjectMapper.CONTENT_TYPE)) { + MapperErrors.throwObjectMappingConflictError(parent); + } else if (oldValue.equals(NestedObjectMapper.CONTENT_TYPE) + || newValue.equals(NestedObjectMapper.CONTENT_TYPE)) { + MapperErrors.throwNestedMappingConflictError(parent); + } } } case "subobjects" -> { diff --git a/server/src/main/java/org/elasticsearch/index/mapper/NestedObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/NestedObjectMapper.java index 68f07c1a2cad1..d772df91a3b40 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/NestedObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/NestedObjectMapper.java @@ -201,7 +201,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws @Override public ObjectMapper merge(Mapper mergeWith, MapperService.MergeReason reason, MapperBuilderContext parentBuilderContext) { if ((mergeWith instanceof NestedObjectMapper) == false) { - throw new IllegalArgumentException("can't merge a non nested mapping [" + mergeWith.name() + "] with a nested mapping"); + MapperErrors.throwNestedMappingConflictError(mergeWith.name()); } NestedObjectMapper mergeWithObject = (NestedObjectMapper) mergeWith; NestedObjectMapper toMerge = (NestedObjectMapper) clone(); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java index ee78fc6a27464..c62f010b35af2 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java @@ -467,11 +467,11 @@ protected MapperBuilderContext createChildContext(MapperBuilderContext mapperBui public ObjectMapper merge(Mapper mergeWith, MergeReason reason, MapperBuilderContext parentBuilderContext) { if ((mergeWith instanceof ObjectMapper) == false) { - throw new IllegalArgumentException("can't merge a non object mapping [" + mergeWith.name() + "] with an object mapping"); + MapperErrors.throwObjectMappingConflictError(mergeWith.name()); } if (mergeWith instanceof NestedObjectMapper) { // TODO stop NestedObjectMapper extending ObjectMapper? - throw new IllegalArgumentException("can't merge a nested mapping [" + mergeWith.name() + "] with a non-nested mapping"); + MapperErrors.throwNestedMappingConflictError(mergeWith.name()); } ObjectMapper mergeWithObject = (ObjectMapper) mergeWith; ObjectMapper merged = clone(); @@ -512,10 +512,10 @@ protected void doMerge(final ObjectMapper mergeWith, MergeReason reason, MapperB merged = objectMapper.merge(mergeWithMapper, reason, objectBuilderContext); } else { assert mergeIntoMapper instanceof FieldMapper || mergeIntoMapper instanceof FieldAliasMapper; - if (mergeWithMapper instanceof ObjectMapper) { - throw new IllegalArgumentException( - "can't merge a non object mapping [" + mergeWithMapper.name() + "] with an object mapping" - ); + if (mergeWithMapper instanceof NestedObjectMapper) { + MapperErrors.throwNestedMappingConflictError(mergeWithMapper.name()); + } else if (mergeWithMapper instanceof ObjectMapper) { + MapperErrors.throwObjectMappingConflictError(mergeWithMapper.name()); } // If we're merging template mappings when creating an index, then a field definition always diff --git a/server/src/test/java/org/elasticsearch/index/mapper/DocumentMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/DocumentMapperTests.java index 19bc4bc3947f1..f8b21e0ca1236 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/DocumentMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/DocumentMapperTests.java @@ -95,7 +95,7 @@ public void testMergeObjectAndNested() throws Exception { IllegalArgumentException.class, () -> MapperService.mergeMappings(objectMapper, nestedMapper.mapping(), reason) ); - assertThat(e.getMessage(), containsString("can't merge a nested mapping [obj] with a non-nested mapping")); + assertThat(e.getMessage(), containsString("can't merge a non nested mapping [obj] with a nested mapping")); } { IllegalArgumentException e = expectThrows( diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java index a1d8df983cc25..fd3cbc9c98e7c 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java @@ -292,7 +292,7 @@ public void testDisallowFieldReplacementForIndexTemplates() throws IOException { MergeReason.INDEX_TEMPLATE ) ); - assertThat(e.getMessage(), containsString("can't merge a non object mapping [object.field2] with an object mapping")); + assertThat(e.getMessage(), containsString("can't merge a non nested mapping [field2] with a nested mapping")); String secondUpdate = Strings.toString( XContentFactory.jsonBuilder() @@ -310,9 +310,13 @@ public void testDisallowFieldReplacementForIndexTemplates() throws IOException { ); e = expectThrows( IllegalArgumentException.class, - () -> mapperService.merge(MapperService.SINGLE_MAPPING_NAME, new CompressedXContent(secondUpdate), MergeReason.INDEX_TEMPLATE) + () -> mapperService.merge( + MapperService.SINGLE_MAPPING_NAME, + List.of(new CompressedXContent(mapping), new CompressedXContent(secondUpdate)), + MergeReason.INDEX_TEMPLATE + ) ); - assertThat(e.getMessage(), containsString("can't merge a non object mapping [object.field1] with an object mapping")); + assertThat(e.getMessage(), containsString("can't merge a non object mapping [field1] with an object mapping")); } public void testUnknownLegacyFields() throws Exception { From cda02364a6d56741388b608133f90ba41cfdedd5 Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Mon, 14 Aug 2023 12:38:08 +0300 Subject: [PATCH 23/38] Adding comment --- .../cluster/metadata/MetadataIndexTemplateService.java | 3 +++ 1 file changed, 3 insertions(+) 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 51f2311ddbaea..5415454a10486 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java @@ -98,6 +98,9 @@ public class MetadataIndexTemplateService { try { DEFAULT_TIMESTAMP_MAPPING_WITHOUT_ROUTING = new CompressedXContent( (builder, params) -> builder.startObject(MapperService.SINGLE_MAPPING_NAME) + // adding explicit "_routing": {"required": false}, even though this is the default, because this snippet is used + // later for resolving a RoutingFieldMapper, where we need this information to validate that does not conflict with + // any mapping. .startObject(RoutingFieldMapper.NAME) .field("required", false) .endObject() From e3d7e76aa9da98ea965c3c65cb02b14dd9e9b6ea Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Tue, 22 Aug 2023 15:48:29 +0300 Subject: [PATCH 24/38] Add tests --- .../index/mapper/MapperServiceTests.java | 332 ++++++++++++++++++ 1 file changed, 332 insertions(+) diff --git a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java index 8aec00beb0e47..bc5d51b335cc8 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java @@ -674,4 +674,336 @@ public void testMergeMultipleRootsWithoutRootType() throws IOException { } }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); } + + public void testValidMappingSubtreeSubstitution() throws IOException { + final MapperService mapperService = createMapperService(mapping(b -> { + })); + + CompressedXContent mapping1 = new CompressedXContent(""" + { + "properties": { + "field": { + "type": "object", + "properties": { + "subfield": { + "type": "keyword" + } + } + } + } + }"""); + + mapperService.merge("_doc", mapping1, MergeReason.INDEX_TEMPLATE); + + CompressedXContent mapping2 = new CompressedXContent(""" + { + "properties": { + "field": { + "type": "long", + "coerce": true + } + } + }"""); + + mapperService.merge("_doc", mapping2, MergeReason.INDEX_TEMPLATE); + assertEquals(""" + { + "_doc" : { + "properties" : { + "field" : { + "type" : "long", + "coerce" : true + } + } + } + }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); + } + + public void testObjectAndNestedTypeSubstitution() throws IOException { + final MapperService mapperService = createMapperService(mapping(b -> { + })); + + CompressedXContent mapping1 = new CompressedXContent(""" + { + "properties" : { + "field": { + "type": "nested", + "include_in_parent": true, + "properties": { + "subfield1": { + "type": "keyword" + } + } + } + } + }"""); + + mapperService.merge("_doc", mapping1, MergeReason.INDEX_TEMPLATE); + + CompressedXContent mapping2 = new CompressedXContent(""" + { + "properties": { + "field": { + "type": "object", + "properties": { + "subfield2": { + "type": "keyword" + } + } + } + } + }"""); + + mapperService.merge("_doc", mapping2, MergeReason.INDEX_TEMPLATE); + assertEquals(""" + { + "_doc" : { + "properties" : { + "field" : { + "type" : "object", + "properties" : { + "subfield1" : { + "type" : "keyword" + }, + "subfield2" : { + "type" : "keyword" + } + } + } + } + } + }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); + } + + public void testNestedContradictingProperties() throws IOException { + final MapperService mapperService = createMapperService(mapping(b -> { + })); + + CompressedXContent mapping1 = new CompressedXContent(""" + { + "properties": { + "field": { + "type": "nested", + "include_in_parent": false, + "properties": { + "subfield1": { + "type": "keyword" + } + } + } + } + }"""); + + mapperService.merge("_doc", mapping1, MergeReason.INDEX_TEMPLATE); + + CompressedXContent mapping2 = new CompressedXContent(""" + { + "properties": { + "field": { + "type": "nested", + "include_in_parent": true, + "properties": { + "subfield2": { + "type": "keyword" + } + } + } + } + }"""); + + mapperService.merge("_doc", mapping2, MergeReason.INDEX_TEMPLATE); + assertEquals(""" + { + "_doc" : { + "properties" : { + "field" : { + "type" : "nested", + "include_in_parent" : true, + "properties" : { + "subfield1" : { + "type" : "keyword" + }, + "subfield2" : { + "type" : "keyword" + } + } + } + } + } + }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); + } + + public void testImplicitObjectHierarchy() throws IOException { + final MapperService mapperService = createMapperService(mapping(b -> { + })); + + CompressedXContent mapping1 = new CompressedXContent(""" + { + "properties": { + "parent": { + "properties": { + "child.grandchild": { + "type": "keyword" + } + } + } + } + }"""); + + mapperService.merge("_doc", mapping1, MergeReason.INDEX_TEMPLATE); + assertEquals(""" + { + "_doc" : { + "properties" : { + "parent" : { + "properties" : { + "child" : { + "properties" : { + "grandchild" : { + "type" : "keyword" + } + } + } + } + } + } + } + }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); + } + + public void testSubobjectsMerge() throws IOException { + final MapperService mapperService = createMapperService(mapping(b -> { + })); + + CompressedXContent mapping1 = new CompressedXContent(""" + { + "properties": { + "parent": { + "type": "object", + "subobjects": false + } + } + }"""); + + mapperService.merge("_doc", mapping1, MergeReason.INDEX_TEMPLATE); + + CompressedXContent mapping2 = new CompressedXContent(""" + { + "properties": { + "parent": { + "properties": { + "child.grandchild": { + "type": "keyword" + } + } + } + } + }"""); + + mapperService.merge("_doc", mapping2, MergeReason.INDEX_TEMPLATE); + assertEquals(""" + { + "_doc" : { + "properties" : { + "parent" : { + "type" : "object", + "subobjects": false, + "properties" : { + "child.grandchild" : { + "type" : "keyword" + } + } + } + } + } + }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); + } + + public void testSubobjectsImplicitObjectsMerge() throws IOException { + final MapperService mapperService = createMapperService(mapping(b -> { + })); + + CompressedXContent mapping1 = new CompressedXContent(""" + { + "properties": { + "parent": { + "type": "object", + "subobjects": false + } + } + }"""); + + mapperService.merge("_doc", mapping1, MergeReason.INDEX_TEMPLATE); + + CompressedXContent mapping2 = new CompressedXContent(""" + { + "properties": { + "parent.child.grandchild": { + "type": "keyword" + } + } + }"""); + + MapperParsingException e = expectThrows( + MapperParsingException.class, + () -> mapperService.merge("_doc", mapping2, MergeReason.INDEX_TEMPLATE) + ); + assertThat(e.getMessage(), containsString("Tried to add subobject [child] to object [parent] which does not support subobjects")); + } + + public void testSameTypeMerge() throws IOException { + final MapperService mapperService = createMapperService(mapping(b -> { + })); + + CompressedXContent mapping1 = new CompressedXContent(""" + { + "properties": { + "field": { + "type": "keyword", + "ignore_above": 256, + "doc_values": false, + "fields": { + "text": { + "type": "text" + } + } + } + } + }"""); + + mapperService.merge("_doc", mapping1, MergeReason.INDEX_TEMPLATE); + + CompressedXContent mapping2 = new CompressedXContent(""" + { + "properties": { + "field": { + "type": "keyword", + "ignore_above": 1024, + "fields": { + "other_text": { + "type": "text" + } + } + } + } + }"""); + + mapperService.merge("_doc", mapping2, MergeReason.INDEX_TEMPLATE); + assertEquals(""" + { + "_doc" : { + "properties" : { + "field" : { + "type" : "keyword", + "ignore_above" : 1024, + "fields" : { + "other_text" : { + "type" : "text" + } + } + } + } + } + }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); + } } From 5dbcf02c95de5966f57e75d6e5f2846b409555d8 Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Wed, 23 Aug 2023 14:15:55 +0300 Subject: [PATCH 25/38] Apply algorithm changes --- .../15_composition.yml | 44 ++ .../common/xcontent/XContentHelper.java | 31 +- .../index/mapper/MapperService.java | 104 ++++- .../MetadataIndexTemplateServiceTests.java | 15 +- .../index/mapper/MapperServiceTests.java | 439 +++++++++++------- .../index/mapper/ObjectMapperTests.java | 21 +- 6 files changed, 434 insertions(+), 220 deletions(-) diff --git a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.put_index_template/15_composition.yml b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.put_index_template/15_composition.yml index 2b90935b2358a..e60aa61986a19 100644 --- a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.put_index_template/15_composition.yml +++ b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.put_index_template/15_composition.yml @@ -448,3 +448,47 @@ indices.get_mapping: index: test-generic - match: { test-generic.mappings.properties.parent.properties.child\.grandchild.type: "keyword" } + +--- +"Composition of component templates with different legal field mappings": + - skip: + features: allowed_warnings + + - do: + cluster.put_component_template: + name: mapping + body: + template: + mappings: + properties: + field: + type: long + coerce: true + + - do: + allowed_warnings: + - "index template [test-composable-template] has index patterns [test-*] matching patterns from existing older templates [global] with patterns (global => [*]); this template [test-composable-template] will take precedence during new index creation" + indices.put_index_template: + name: test-composable-template + body: + index_patterns: + - test-* + composed_of: + - mapping + template: + mappings: + properties: + field: + type: keyword + ignore_above: 1024 + - is_true: acknowledged + + - do: + indices.create: + index: test-generic + + - do: + indices.get_mapping: + index: test-generic + - match: { test-generic.mappings.properties.field.type: "keyword" } + - match: { test-generic.mappings.properties.field.ignore_above: 1024 } diff --git a/server/src/main/java/org/elasticsearch/common/xcontent/XContentHelper.java b/server/src/main/java/org/elasticsearch/common/xcontent/XContentHelper.java index 3a8e4fd06656d..e813919bb0e03 100644 --- a/server/src/main/java/org/elasticsearch/common/xcontent/XContentHelper.java +++ b/server/src/main/java/org/elasticsearch/common/xcontent/XContentHelper.java @@ -446,18 +446,31 @@ public static void merge( ) { for (Map.Entry toMergeEntry : second.entrySet()) { if (first.containsKey(toMergeEntry.getKey()) == false) { - // copy it over, it does not exists in the content + // copy it over, it does not exist in the content first.put(toMergeEntry.getKey(), toMergeEntry.getValue()); } else { // has values in both maps, merge compound ones (maps) Object baseValue = first.get(toMergeEntry.getKey()); if (baseValue instanceof Map && toMergeEntry.getValue() instanceof Map) { - merge( - toMergeEntry.getKey(), - (Map) baseValue, - (Map) toMergeEntry.getValue(), - customMerge - ); + Map mergedValue = null; + if (customMerge != null) { + Object tmp = customMerge.merge(parent, toMergeEntry.getKey(), baseValue, toMergeEntry.getValue()); + if (tmp != null && tmp instanceof Map == false) { + throw new IllegalStateException("merging of values for [" + toMergeEntry.getKey() + "] must yield a map"); + } + mergedValue = (Map) tmp; + } + if (mergedValue != null) { + first.put(toMergeEntry.getKey(), mergedValue); + } else { + // if custom merge does not yield a value to be used, continue recursive merge + merge( + toMergeEntry.getKey(), + (Map) baseValue, + (Map) toMergeEntry.getValue(), + customMerge + ); + } } else if (baseValue instanceof List && toMergeEntry.getValue() instanceof List) { List listToMerge = (List) toMergeEntry.getValue(); List baseList = (List) baseValue; @@ -520,13 +533,13 @@ private static boolean allListValuesAreMapsOfOne(List list) { @FunctionalInterface public interface CustomMerge { /** - * Based on the provided arguments, decide which value to use for the given key. + * Based on the provided arguments, compute a value to use for the given key as a merge result. * This method doesn't throw a checked exception, but it is expected that illegal merges will result in a {@link RuntimeException}. * @param parent merged field's parent * @param key merged field's name * @param oldValue original value of the provided key * @param newValue the new value of the provided key which is to be merged with the original - * @return the merged value to use for the given key + * @return the merged value to use for the given key, or {@code null} if there is no custom merge required for it */ Object merge(String parent, String key, Object oldValue, Object newValue); } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index 1e0588ee3ff36..3fdc509609133 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -40,10 +40,12 @@ import java.io.IOException; import java.io.InputStream; import java.util.Collections; +import java.util.HashMap; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Set; import java.util.function.Function; import java.util.function.Supplier; @@ -392,38 +394,90 @@ public DocumentMapper merge(String type, List mappingSources if (mergedRawMapping == null) { mergedRawMapping = rawMapping; } else { - XContentHelper.merge(type, mergedRawMapping, rawMapping, ((parent, key, oldValue, newValue) -> { - switch (key) { - case "type" -> { - // todo: verify this check is valid - if (oldValue.equals(newValue) == false) { - if (oldValue.equals(ObjectMapper.CONTENT_TYPE) || newValue.equals(ObjectMapper.CONTENT_TYPE)) { - MapperErrors.throwObjectMappingConflictError(parent); - } else if (oldValue.equals(NestedObjectMapper.CONTENT_TYPE) - || newValue.equals(NestedObjectMapper.CONTENT_TYPE)) { - MapperErrors.throwNestedMappingConflictError(parent); - } - } + XContentHelper.merge(type, mergedRawMapping, rawMapping, RawFieldMappingMerge.INSTANCE); + } + } + if (mergedRawMapping != null && mergedRawMapping.size() > 1) { + throw new MapperParsingException("cannot merge mapping sources with different roots"); + } + return (mergedRawMapping != null) ? doMerge(type, reason, mergedRawMapping) : null; + } + + private static class RawFieldMappingMerge implements XContentHelper.CustomMerge { + private static final XContentHelper.CustomMerge INSTANCE = new RawFieldMappingMerge(); + + private static final Set MERGEABLE_OBJECT_TYPES = Set.of(ObjectMapper.CONTENT_TYPE, NestedObjectMapper.CONTENT_TYPE); + + private RawFieldMappingMerge() {} + + @SuppressWarnings("unchecked") + @Override + public Object merge(String parent, String key, Object oldValue, Object newValue) { + if (oldValue instanceof Map && newValue instanceof Map) { + if ("properties".equals(parent)) { + // merging two mappings of the same field, where "key" is the field name + Map baseMap = (Map) oldValue; + Map mapToMerge = (Map) newValue; + if (shouldMergeFieldMappings(baseMap, mapToMerge)) { + // if two field mappings are to be merged, we only want to keep some specific entries from the base mapping and + // let all others be overridden by the second mapping + Map mergedMappings = new HashMap<>(); + if (baseMap.containsKey("properties")) { + mergedMappings.put("properties", new HashMap<>((Map) baseMap.get("properties"))); + } + if (baseMap.containsKey("subobjects")) { + mergedMappings.put("subobjects", baseMap.get("subobjects")); } - case "subobjects" -> { - if (oldValue.equals(newValue) == false) { - throw new MapperParsingException("contradicting subobjects settings provided for field: " + parent); - } + // recursively merge these two field mappings + XContentHelper.merge(key, mergedMappings, mapToMerge, INSTANCE); + return mergedMappings; + } else { + // non-mergeable types - replace the entire mapping subtree for this field + return mapToMerge; + } + } + // anything else (e.g. "_doc", "_meta", "properties") - no custom merge, rely on caller merge logic + // field mapping entries of Map type (like "fields" and "meta") are handled above and should never reach here + return null; + } else { + switch (key) { + case "subobjects" -> { + if (oldValue.equals(newValue) == false) { + throw new MapperParsingException("contradicting subobjects settings provided for field: " + parent); } - case "required" -> { - if ("_routing".equals(parent) && oldValue != newValue) { - throw new MapperParsingException("contradicting `_routing.required` settings"); - } + } + case "required" -> { + if ("_routing".equals(parent) && oldValue != newValue) { + throw new MapperParsingException("contradicting `_routing.required` settings"); } } - return newValue; - })); + } + return newValue; } } - if (mergedRawMapping != null && mergedRawMapping.size() > 1) { - throw new MapperParsingException("cannot merge mapping sources with different roots"); + + /** + * Normally, we don't want to merge raw field mappings, however there are cases where we do, for example - two + * "object" (or "nested") mappings. + * + * @param mappings1 first mapping of a field + * @param mappings2 second mapping of a field + * @return true if mapping + */ + private boolean shouldMergeFieldMappings(Map mappings1, Map mappings2) { + String type1 = (String) mappings1.get("type"); + if (type1 == null && mappings1.get("properties") != null) { + type1 = ObjectMapper.CONTENT_TYPE; + } + String type2 = (String) mappings2.get("type"); + if (type2 == null && mappings2.get("properties") != null) { + type2 = ObjectMapper.CONTENT_TYPE; + } + if (type1 == null || type2 == null) { + return false; + } + return MERGEABLE_OBJECT_TYPES.contains(type1) && MERGEABLE_OBJECT_TYPES.contains(type2); } - return (mergedRawMapping != null) ? doMerge(type, reason, mergedRawMapping) : null; } public DocumentMapper merge(String type, CompressedXContent mappingSource, MergeReason reason) { 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 9b735e1b1e1c4..6d503ba8740df 100644 --- a/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateServiceTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateServiceTests.java @@ -1835,6 +1835,7 @@ public void testIndexTemplateFailsToOverrideComponentTemplateMappingField() thro "properties": { "field2": { "type": "object", + "subobjects": false, "properties": { "foo": { "type": "integer" @@ -1857,7 +1858,8 @@ public void testIndexTemplateFailsToOverrideComponentTemplateMappingField() thro { "properties": { "field2": { - "type": "text" + "type": "object", + "subobjects": true } } }"""), null), randomBoolean() ? Arrays.asList("c1", "c2") : Arrays.asList("c2", "c1"), 0L, 1L, null, null, null); @@ -1877,10 +1879,7 @@ public void testIndexTemplateFailsToOverrideComponentTemplateMappingField() thro assertThat(e.getCause().getMessage(), containsString("invalid composite mappings for [my-template]")); assertNotNull(e.getCause().getCause()); - assertThat( - e.getCause().getCause().getMessage(), - containsString("can't merge a non object mapping [field2] with an object mapping") - ); + assertThat(e.getCause().getCause().getMessage(), containsString("contradicting subobjects settings provided for field: field2")); } /** @@ -1926,6 +1925,7 @@ public void testUpdateComponentTemplateFailsIfResolvedIndexTemplatesWouldBeInval "properties": { "field2": { "type": "object", + "subobjects": false, "properties": { "foo": { "type": "integer" @@ -1964,7 +1964,8 @@ public void testUpdateComponentTemplateFailsIfResolvedIndexTemplatesWouldBeInval { "properties": { "field2": { - "type": "text" + "type": "object", + "subobjects": true } } } @@ -1990,7 +1991,7 @@ public void testUpdateComponentTemplateFailsIfResolvedIndexTemplatesWouldBeInval assertNotNull(e.getCause().getCause().getCause()); assertThat( e.getCause().getCause().getCause().getMessage(), - containsString("can't merge a non object mapping [field2] with an object mapping") + containsString("contradicting subobjects settings provided for field: field2") ); } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java index bc5d51b335cc8..86ab0193b96c4 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java @@ -553,8 +553,6 @@ public void testSubobjectsDisabledNotAtRoot() throws IOException { } public void testMergeMultipleRoots() throws IOException { - final MapperService mapperService = createMapperService(mapping(b -> {})); - CompressedXContent mapping1 = new CompressedXContent(""" { "properties" : { @@ -584,7 +582,11 @@ public void testMergeMultipleRoots() throws IOException { } }"""); + final MapperService mapperService = createMapperService(mapping(b -> { + })); mapperService.merge("_doc", List.of(mapping1, mapping2), MergeReason.INDEX_TEMPLATE); + + assertEquals(""" { "_doc" : { @@ -606,8 +608,6 @@ public void testMergeMultipleRoots() throws IOException { } public void testMergeMultipleRootsWithRootType() throws IOException { - final MapperService mapperService = createMapperService(mapping(b -> {})); - CompressedXContent mapping1 = new CompressedXContent(""" { "properties" : { @@ -632,6 +632,8 @@ public void testMergeMultipleRootsWithRootType() throws IOException { } }"""); + final MapperService mapperService = createMapperService(mapping(b -> { + })); MapperParsingException e = expectThrows( MapperParsingException.class, () -> mapperService.merge("_doc", List.of(mapping1, mapping2), MergeReason.INDEX_TEMPLATE) @@ -640,8 +642,6 @@ public void testMergeMultipleRootsWithRootType() throws IOException { } public void testMergeMultipleRootsWithoutRootType() throws IOException { - final MapperService mapperService = createMapperService(mapping(b -> {})); - CompressedXContent mapping1 = new CompressedXContent(""" { "properties" : { @@ -659,7 +659,10 @@ public void testMergeMultipleRootsWithoutRootType() throws IOException { } }"""); + final MapperService mapperService = createMapperService(mapping(b -> { + })); mapperService.merge("_doc", List.of(mapping1, mapping2), MergeReason.INDEX_TEMPLATE); + assertEquals(""" { "_doc" : { @@ -675,15 +678,51 @@ public void testMergeMultipleRootsWithoutRootType() throws IOException { }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); } - public void testValidMappingSubtreeSubstitution() throws IOException { + public void testValidMappingSubstitution() throws IOException { + CompressedXContent mapping1 = new CompressedXContent(""" + { + "properties": { + "field": { + "type": "keyword", + "ignore_above": 1024 + } + } + }"""); + + CompressedXContent mapping2 = new CompressedXContent(""" + { + "properties": { + "field": { + "type": "long", + "coerce": true + } + } + }"""); + final MapperService mapperService = createMapperService(mapping(b -> { })); + mapperService.merge("_doc", List.of(mapping1, mapping2), MergeReason.INDEX_TEMPLATE); + assertEquals(""" + { + "_doc" : { + "properties" : { + "field" : { + "type" : "long", + "coerce" : true + } + } + } + }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); + } + + public void testValidMappingSubtreeSubstitution() throws IOException { CompressedXContent mapping1 = new CompressedXContent(""" { "properties": { "field": { "type": "object", + "subobjects": false, "properties": { "subfield": { "type": "keyword" @@ -693,8 +732,6 @@ public void testValidMappingSubtreeSubstitution() throws IOException { } }"""); - mapperService.merge("_doc", mapping1, MergeReason.INDEX_TEMPLATE); - CompressedXContent mapping2 = new CompressedXContent(""" { "properties": { @@ -705,7 +742,10 @@ public void testValidMappingSubtreeSubstitution() throws IOException { } }"""); - mapperService.merge("_doc", mapping2, MergeReason.INDEX_TEMPLATE); + final MapperService mapperService = createMapperService(mapping(b -> { + })); + mapperService.merge("_doc", List.of(mapping1, mapping2), MergeReason.INDEX_TEMPLATE); + assertEquals(""" { "_doc" : { @@ -719,10 +759,61 @@ public void testValidMappingSubtreeSubstitution() throws IOException { }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); } - public void testObjectAndNestedTypeSubstitution() throws IOException { + public void testSameTypeMerge() throws IOException { + CompressedXContent mapping1 = new CompressedXContent(""" + { + "properties": { + "field": { + "type": "keyword", + "ignore_above": 256, + "doc_values": false, + "fields": { + "text": { + "type": "text" + } + } + } + } + }"""); + + CompressedXContent mapping2 = new CompressedXContent(""" + { + "properties": { + "field": { + "type": "keyword", + "ignore_above": 1024, + "fields": { + "other_text": { + "type": "text" + } + } + } + } + }"""); + final MapperService mapperService = createMapperService(mapping(b -> { })); + mapperService.merge("_doc", List.of(mapping1, mapping2), MergeReason.INDEX_TEMPLATE); + + assertEquals(""" + { + "_doc" : { + "properties" : { + "field" : { + "type" : "keyword", + "ignore_above" : 1024, + "fields" : { + "other_text" : { + "type" : "text" + } + } + } + } + } + }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); + } + public void testObjectAndNestedTypeSubstitution() throws IOException { CompressedXContent mapping1 = new CompressedXContent(""" { "properties" : { @@ -738,8 +829,6 @@ public void testObjectAndNestedTypeSubstitution() throws IOException { } }"""); - mapperService.merge("_doc", mapping1, MergeReason.INDEX_TEMPLATE); - CompressedXContent mapping2 = new CompressedXContent(""" { "properties": { @@ -754,13 +843,15 @@ public void testObjectAndNestedTypeSubstitution() throws IOException { } }"""); - mapperService.merge("_doc", mapping2, MergeReason.INDEX_TEMPLATE); + final MapperService mapperService = createMapperService(mapping(b -> { + })); + mapperService.merge("_doc", List.of(mapping1, mapping2), MergeReason.INDEX_TEMPLATE); + assertEquals(""" { "_doc" : { "properties" : { "field" : { - "type" : "object", "properties" : { "subfield1" : { "type" : "keyword" @@ -776,9 +867,6 @@ public void testObjectAndNestedTypeSubstitution() throws IOException { } public void testNestedContradictingProperties() throws IOException { - final MapperService mapperService = createMapperService(mapping(b -> { - })); - CompressedXContent mapping1 = new CompressedXContent(""" { "properties": { @@ -794,8 +882,6 @@ public void testNestedContradictingProperties() throws IOException { } }"""); - mapperService.merge("_doc", mapping1, MergeReason.INDEX_TEMPLATE); - CompressedXContent mapping2 = new CompressedXContent(""" { "properties": { @@ -811,7 +897,10 @@ public void testNestedContradictingProperties() throws IOException { } }"""); - mapperService.merge("_doc", mapping2, MergeReason.INDEX_TEMPLATE); + final MapperService mapperService = createMapperService(mapping(b -> { + })); + mapperService.merge("_doc", List.of(mapping1, mapping2), MergeReason.INDEX_TEMPLATE); + assertEquals(""" { "_doc" : { @@ -833,96 +922,93 @@ public void testNestedContradictingProperties() throws IOException { }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); } - public void testImplicitObjectHierarchy() throws IOException { - final MapperService mapperService = createMapperService(mapping(b -> { - })); - - CompressedXContent mapping1 = new CompressedXContent(""" - { - "properties": { - "parent": { - "properties": { - "child.grandchild": { - "type": "keyword" - } - } - } - } - }"""); - - mapperService.merge("_doc", mapping1, MergeReason.INDEX_TEMPLATE); - assertEquals(""" - { - "_doc" : { - "properties" : { - "parent" : { - "properties" : { - "child" : { - "properties" : { - "grandchild" : { - "type" : "keyword" - } - } - } - } - } - } - } - }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); - } - - public void testSubobjectsMerge() throws IOException { - final MapperService mapperService = createMapperService(mapping(b -> { - })); - - CompressedXContent mapping1 = new CompressedXContent(""" - { - "properties": { - "parent": { - "type": "object", - "subobjects": false - } - } - }"""); - - mapperService.merge("_doc", mapping1, MergeReason.INDEX_TEMPLATE); - - CompressedXContent mapping2 = new CompressedXContent(""" - { - "properties": { - "parent": { - "properties": { - "child.grandchild": { - "type": "keyword" - } - } - } - } - }"""); - - mapperService.merge("_doc", mapping2, MergeReason.INDEX_TEMPLATE); - assertEquals(""" - { - "_doc" : { - "properties" : { - "parent" : { - "type" : "object", - "subobjects": false, - "properties" : { - "child.grandchild" : { - "type" : "keyword" - } - } - } - } - } - }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); - } +// public void testImplicitObjectHierarchy() throws IOException { +// CompressedXContent mapping1 = new CompressedXContent(""" +// { +// "properties": { +// "parent": { +// "properties": { +// "child.grandchild": { +// "type": "keyword" +// } +// } +// } +// } +// }"""); +// +// final MapperService mapperService = createMapperService(mapping(b -> { +// })); +// DocumentMapper bulkMerge = mapperService.merge("_doc", List.of(mapping1), MergeReason.INDEX_TEMPLATE); +// +// assertEquals(""" +// { +// "_doc" : { +// "properties" : { +// "parent" : { +// "properties" : { +// "child" : { +// "properties" : { +// "grandchild" : { +// "type" : "keyword" +// } +// } +// } +// } +// } +// } +// } +// }""", Strings.toString(bulkMerge.mapping(), true, true)); +// +// DocumentMapper sequentialMerge = mapperService.merge("_doc", mapping1, MergeReason.INDEX_TEMPLATE); +// assertEquals(bulkMerge.mappingSource(), sequentialMerge.mappingSource()); +// } + +// public void testSubobjectsMerge() throws IOException { +// CompressedXContent mapping1 = new CompressedXContent(""" +// { +// "properties": { +// "parent": { +// "type": "object", +// "subobjects": false +// } +// } +// }"""); +// +// CompressedXContent mapping2 = new CompressedXContent(""" +// { +// "properties": { +// "parent": { +// "properties": { +// "child.grandchild": { +// "type": "keyword" +// } +// } +// } +// } +// }"""); +// +// final MapperService mapperService = createMapperService(mapping(b -> { +// })); +// mapperService.merge("_doc", List.of(mapping1, mapping2), MergeReason.INDEX_TEMPLATE); +// +// assertEquals(""" +// { +// "_doc" : { +// "properties" : { +// "parent" : { +// "subobjects" : false, +// "properties" : { +// "child.grandchild" : { +// "type" : "keyword" +// } +// } +// } +// } +// } +// }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); +// } public void testSubobjectsImplicitObjectsMerge() throws IOException { - final MapperService mapperService = createMapperService(mapping(b -> { - })); - CompressedXContent mapping1 = new CompressedXContent(""" { "properties": { @@ -933,8 +1019,6 @@ public void testSubobjectsImplicitObjectsMerge() throws IOException { } }"""); - mapperService.merge("_doc", mapping1, MergeReason.INDEX_TEMPLATE); - CompressedXContent mapping2 = new CompressedXContent(""" { "properties": { @@ -944,66 +1028,87 @@ public void testSubobjectsImplicitObjectsMerge() throws IOException { } }"""); + final MapperService mapperService = createMapperService(mapping(b -> { + })); MapperParsingException e = expectThrows( MapperParsingException.class, - () -> mapperService.merge("_doc", mapping2, MergeReason.INDEX_TEMPLATE) + () -> mapperService.merge("_doc", List.of(mapping1, mapping2), MergeReason.INDEX_TEMPLATE) ); assertThat(e.getMessage(), containsString("Tried to add subobject [child] to object [parent] which does not support subobjects")); } - public void testSameTypeMerge() throws IOException { - final MapperService mapperService = createMapperService(mapping(b -> { - })); - - CompressedXContent mapping1 = new CompressedXContent(""" - { - "properties": { - "field": { - "type": "keyword", - "ignore_above": 256, - "doc_values": false, - "fields": { - "text": { - "type": "text" - } - } - } - } - }"""); - - mapperService.merge("_doc", mapping1, MergeReason.INDEX_TEMPLATE); - - CompressedXContent mapping2 = new CompressedXContent(""" - { - "properties": { - "field": { - "type": "keyword", - "ignore_above": 1024, - "fields": { - "other_text": { - "type": "text" - } - } - } - } - }"""); - - mapperService.merge("_doc", mapping2, MergeReason.INDEX_TEMPLATE); - assertEquals(""" - { - "_doc" : { - "properties" : { - "field" : { - "type" : "keyword", - "ignore_above" : 1024, - "fields" : { - "other_text" : { - "type" : "text" - } - } - } - } - } - }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); - } +// public void testMultipleTypeMerges() throws IOException { +// CompressedXContent mapping1 = new CompressedXContent(""" +// { +// "properties" : { +// "parent": { +// "properties": { +// "child": { +// "type": "object", +// "properties": { +// "grandchild1": { +// "type": "keyword" +// }, +// "grandchild2": { +// "type": "date" +// } +// } +// } +// } +// } +// } +// }"""); +// +// CompressedXContent mapping2 = new CompressedXContent(""" +// { +// "properties" : { +// "parent": { +// "type": "object", +// "properties": { +// "child": { +// "type": "nested", +// "properties": { +// "grandchild1": { +// "type": "text" +// }, +// "grandchild3": { +// "type": "text" +// } +// } +// } +// } +// } +// } +// }"""); +// +// final MapperService mapperService = createMapperService(mapping(b -> { +// })); +// mapperService.merge("_doc", List.of(mapping1, mapping2), MergeReason.INDEX_TEMPLATE); +// +// assertEquals(""" +// { +// "_doc" : { +// "properties" : { +// "parent" : { +// "properties" : { +// "child" : { +// "type" : "nested", +// "properties" : { +// "grandchild1" : { +// "type" : "text" +// }, +// "grandchild2" : { +// "type" : "date" +// }, +// "grandchild3" : { +// "type" : "text" +// } +// } +// } +// } +// } +// } +// } +// }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); +// } } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java index fd3cbc9c98e7c..943a7bb58ba22 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java @@ -284,15 +284,16 @@ public void testDisallowFieldReplacementForIndexTemplates() throws IOException { .endObject() .endObject() ); + + // We can only check such assertion in sequential merges. Bulk merges allow such type substitution as it replaces entire field + // mapping subtrees + mapperService.merge(MapperService.SINGLE_MAPPING_NAME, new CompressedXContent(mapping), MergeReason.INDEX_TEMPLATE); + IllegalArgumentException e = expectThrows( IllegalArgumentException.class, - () -> mapperService.merge( - MapperService.SINGLE_MAPPING_NAME, - List.of(new CompressedXContent(mapping), new CompressedXContent(firstUpdate)), - MergeReason.INDEX_TEMPLATE - ) + () -> mapperService.merge(MapperService.SINGLE_MAPPING_NAME, new CompressedXContent(firstUpdate), MergeReason.INDEX_TEMPLATE) ); - assertThat(e.getMessage(), containsString("can't merge a non nested mapping [field2] with a nested mapping")); + assertThat(e.getMessage(), containsString("can't merge a non nested mapping [object.field2] with a nested mapping")); String secondUpdate = Strings.toString( XContentFactory.jsonBuilder() @@ -310,13 +311,9 @@ public void testDisallowFieldReplacementForIndexTemplates() throws IOException { ); e = expectThrows( IllegalArgumentException.class, - () -> mapperService.merge( - MapperService.SINGLE_MAPPING_NAME, - List.of(new CompressedXContent(mapping), new CompressedXContent(secondUpdate)), - MergeReason.INDEX_TEMPLATE - ) + () -> mapperService.merge(MapperService.SINGLE_MAPPING_NAME, new CompressedXContent(secondUpdate), MergeReason.INDEX_TEMPLATE) ); - assertThat(e.getMessage(), containsString("can't merge a non object mapping [field1] with an object mapping")); + assertThat(e.getMessage(), containsString("can't merge a non object mapping [object.field1] with an object mapping")); } public void testUnknownLegacyFields() throws Exception { From 59be7edba99ab826b4523eb687395206d424de3d Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Wed, 23 Aug 2023 14:18:59 +0300 Subject: [PATCH 26/38] Restore test --- .../index/mapper/MapperServiceTests.java | 80 +++++++++---------- 1 file changed, 40 insertions(+), 40 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java index 86ab0193b96c4..3ace5086578d2 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java @@ -922,46 +922,46 @@ public void testNestedContradictingProperties() throws IOException { }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); } -// public void testImplicitObjectHierarchy() throws IOException { -// CompressedXContent mapping1 = new CompressedXContent(""" -// { -// "properties": { -// "parent": { -// "properties": { -// "child.grandchild": { -// "type": "keyword" -// } -// } -// } -// } -// }"""); -// -// final MapperService mapperService = createMapperService(mapping(b -> { -// })); -// DocumentMapper bulkMerge = mapperService.merge("_doc", List.of(mapping1), MergeReason.INDEX_TEMPLATE); -// -// assertEquals(""" -// { -// "_doc" : { -// "properties" : { -// "parent" : { -// "properties" : { -// "child" : { -// "properties" : { -// "grandchild" : { -// "type" : "keyword" -// } -// } -// } -// } -// } -// } -// } -// }""", Strings.toString(bulkMerge.mapping(), true, true)); -// -// DocumentMapper sequentialMerge = mapperService.merge("_doc", mapping1, MergeReason.INDEX_TEMPLATE); -// assertEquals(bulkMerge.mappingSource(), sequentialMerge.mappingSource()); -// } + public void testImplicitObjectHierarchy() throws IOException { + CompressedXContent mapping1 = new CompressedXContent(""" + { + "properties": { + "parent": { + "properties": { + "child.grandchild": { + "type": "keyword" + } + } + } + } + }"""); + + final MapperService mapperService = createMapperService(mapping(b -> { + })); + DocumentMapper bulkMerge = mapperService.merge("_doc", List.of(mapping1), MergeReason.INDEX_TEMPLATE); + + assertEquals(""" + { + "_doc" : { + "properties" : { + "parent" : { + "properties" : { + "child" : { + "properties" : { + "grandchild" : { + "type" : "keyword" + } + } + } + } + } + } + } + }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); + + DocumentMapper sequentialMerge = mapperService.merge("_doc", mapping1, MergeReason.INDEX_TEMPLATE); + assertEquals(bulkMerge.mappingSource(), sequentialMerge.mappingSource()); + } // public void testSubobjectsMerge() throws IOException { // CompressedXContent mapping1 = new CompressedXContent(""" From ef7c9ec04c628c6e2fafa416a7a59293f126cdfd Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Wed, 23 Aug 2023 14:21:50 +0300 Subject: [PATCH 27/38] Restore test --- .../index/mapper/MapperServiceTests.java | 88 +++++++++---------- 1 file changed, 44 insertions(+), 44 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java index 3ace5086578d2..8936947452a5f 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java @@ -963,50 +963,50 @@ public void testImplicitObjectHierarchy() throws IOException { assertEquals(bulkMerge.mappingSource(), sequentialMerge.mappingSource()); } -// public void testSubobjectsMerge() throws IOException { -// CompressedXContent mapping1 = new CompressedXContent(""" -// { -// "properties": { -// "parent": { -// "type": "object", -// "subobjects": false -// } -// } -// }"""); -// -// CompressedXContent mapping2 = new CompressedXContent(""" -// { -// "properties": { -// "parent": { -// "properties": { -// "child.grandchild": { -// "type": "keyword" -// } -// } -// } -// } -// }"""); -// -// final MapperService mapperService = createMapperService(mapping(b -> { -// })); -// mapperService.merge("_doc", List.of(mapping1, mapping2), MergeReason.INDEX_TEMPLATE); -// -// assertEquals(""" -// { -// "_doc" : { -// "properties" : { -// "parent" : { -// "subobjects" : false, -// "properties" : { -// "child.grandchild" : { -// "type" : "keyword" -// } -// } -// } -// } -// } -// }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); -// } + public void testSubobjectsMerge() throws IOException { + CompressedXContent mapping1 = new CompressedXContent(""" + { + "properties": { + "parent": { + "type": "object", + "subobjects": false + } + } + }"""); + + CompressedXContent mapping2 = new CompressedXContent(""" + { + "properties": { + "parent": { + "properties": { + "child.grandchild": { + "type": "keyword" + } + } + } + } + }"""); + + final MapperService mapperService = createMapperService(mapping(b -> { + })); + mapperService.merge("_doc", List.of(mapping1, mapping2), MergeReason.INDEX_TEMPLATE); + + assertEquals(""" + { + "_doc" : { + "properties" : { + "parent" : { + "subobjects" : false, + "properties" : { + "child.grandchild" : { + "type" : "keyword" + } + } + } + } + } + }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); + } public void testSubobjectsImplicitObjectsMerge() throws IOException { CompressedXContent mapping1 = new CompressedXContent(""" From ac60c6d4531f498488ac3ce79376e1fca67a5158 Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Wed, 23 Aug 2023 14:31:22 +0300 Subject: [PATCH 28/38] Restore test --- .../index/mapper/MapperServiceTests.java | 149 +++++++++--------- 1 file changed, 75 insertions(+), 74 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java index 8936947452a5f..2364d868c594f 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java @@ -1037,78 +1037,79 @@ public void testSubobjectsImplicitObjectsMerge() throws IOException { assertThat(e.getMessage(), containsString("Tried to add subobject [child] to object [parent] which does not support subobjects")); } -// public void testMultipleTypeMerges() throws IOException { -// CompressedXContent mapping1 = new CompressedXContent(""" -// { -// "properties" : { -// "parent": { -// "properties": { -// "child": { -// "type": "object", -// "properties": { -// "grandchild1": { -// "type": "keyword" -// }, -// "grandchild2": { -// "type": "date" -// } -// } -// } -// } -// } -// } -// }"""); -// -// CompressedXContent mapping2 = new CompressedXContent(""" -// { -// "properties" : { -// "parent": { -// "type": "object", -// "properties": { -// "child": { -// "type": "nested", -// "properties": { -// "grandchild1": { -// "type": "text" -// }, -// "grandchild3": { -// "type": "text" -// } -// } -// } -// } -// } -// } -// }"""); -// -// final MapperService mapperService = createMapperService(mapping(b -> { -// })); -// mapperService.merge("_doc", List.of(mapping1, mapping2), MergeReason.INDEX_TEMPLATE); -// -// assertEquals(""" -// { -// "_doc" : { -// "properties" : { -// "parent" : { -// "properties" : { -// "child" : { -// "type" : "nested", -// "properties" : { -// "grandchild1" : { -// "type" : "text" -// }, -// "grandchild2" : { -// "type" : "date" -// }, -// "grandchild3" : { -// "type" : "text" -// } -// } -// } -// } -// } -// } -// } -// }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); -// } + public void testMultipleTypeMerges() throws IOException { + CompressedXContent mapping1 = new CompressedXContent(""" + { + "properties" : { + "parent": { + "type": "object", + "properties": { + "child": { + "type": "object", + "properties": { + "grandchild1": { + "type": "keyword" + }, + "grandchild2": { + "type": "date" + } + } + } + } + } + } + }"""); + + CompressedXContent mapping2 = new CompressedXContent(""" + { + "properties" : { + "parent": { + "type": "object", + "properties": { + "child": { + "type": "nested", + "properties": { + "grandchild1": { + "type": "text" + }, + "grandchild3": { + "type": "text" + } + } + } + } + } + } + }"""); + + final MapperService mapperService = createMapperService(mapping(b -> { + })); + mapperService.merge("_doc", List.of(mapping1, mapping2), MergeReason.INDEX_TEMPLATE); + + assertEquals(""" + { + "_doc" : { + "properties" : { + "parent" : { + "properties" : { + "child" : { + "type" : "nested", + "properties" : { + "grandchild1" : { + "type" : "text" + }, + "grandchild2" : { + "type" : "date" + }, + "grandchild3" : { + "type" : "text" + } + } + } + } + } + } + } + }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); + } } From 5e2b52f4b5f1ac391cf23f612b80e4153055c97c Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Wed, 23 Aug 2023 14:35:24 +0300 Subject: [PATCH 29/38] Add test case --- .../index/mapper/MapperServiceTests.java | 43 ++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java index 2364d868c594f..bbb923a50803a 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java @@ -436,7 +436,7 @@ public void testMergeSubobjectsFalseOrder() throws IOException { assertEquals(subobjectsFirst.mappingSource(), subobjectsLast.mappingSource()); } - public void testMergeContradictingSubobjects() throws IOException { + public void testMergeContradictingSubobjectsInRoot() throws IOException { final MapperService mapperService = createMapperService(mapping(b -> {})); CompressedXContent mapping1 = createTestMapping1(); CompressedXContent mapping2 = createTestMapping2(); @@ -1008,6 +1008,47 @@ public void testSubobjectsMerge() throws IOException { }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); } + public void testSubobjectsMergeNotInRoot() throws IOException { + CompressedXContent mapping1 = new CompressedXContent(""" + { + "properties": { + "field": { + "type": "object", + "subobjects": false, + "properties": { + "subfield1": { + "type": "text" + } + } + } + } + }"""); + + CompressedXContent mapping2 = new CompressedXContent(""" + { + "properties": { + "field": { + "type": "object", + "subobjects": true, + "properties": { + "subfield2": { + "type": "text" + } + } + } + } + }"""); + + final MapperService mapperService = createMapperService(mapping(b -> { + })); + + MapperParsingException e = expectThrows( + MapperParsingException.class, + () -> mapperService.merge("_doc", List.of(mapping1, mapping2), MergeReason.INDEX_TEMPLATE) + ); + assertEquals("contradicting subobjects settings provided for field: field", e.getMessage()); + } + public void testSubobjectsImplicitObjectsMerge() throws IOException { CompressedXContent mapping1 = new CompressedXContent(""" { From cd1d116a97488757201cd0e5be5778101c68b31d Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Wed, 23 Aug 2023 14:42:52 +0300 Subject: [PATCH 30/38] spotless --- .../MetadataIndexTemplateServiceTests.java | 2 +- .../index/mapper/MapperServiceTests.java | 386 +++++++++--------- 2 files changed, 187 insertions(+), 201 deletions(-) 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 6d503ba8740df..b30211deff031 100644 --- a/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateServiceTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateServiceTests.java @@ -1991,7 +1991,7 @@ public void testUpdateComponentTemplateFailsIfResolvedIndexTemplatesWouldBeInval assertNotNull(e.getCause().getCause().getCause()); assertThat( e.getCause().getCause().getCause().getMessage(), - containsString("contradicting subobjects settings provided for field: field2") + containsString("contradicting subobjects settings provided for field: field2") ); } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java index bbb923a50803a..5e904e858b350 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java @@ -582,11 +582,9 @@ public void testMergeMultipleRoots() throws IOException { } }"""); - final MapperService mapperService = createMapperService(mapping(b -> { - })); + final MapperService mapperService = createMapperService(mapping(b -> {})); mapperService.merge("_doc", List.of(mapping1, mapping2), MergeReason.INDEX_TEMPLATE); - assertEquals(""" { "_doc" : { @@ -632,8 +630,7 @@ public void testMergeMultipleRootsWithRootType() throws IOException { } }"""); - final MapperService mapperService = createMapperService(mapping(b -> { - })); + final MapperService mapperService = createMapperService(mapping(b -> {})); MapperParsingException e = expectThrows( MapperParsingException.class, () -> mapperService.merge("_doc", List.of(mapping1, mapping2), MergeReason.INDEX_TEMPLATE) @@ -659,8 +656,7 @@ public void testMergeMultipleRootsWithoutRootType() throws IOException { } }"""); - final MapperService mapperService = createMapperService(mapping(b -> { - })); + final MapperService mapperService = createMapperService(mapping(b -> {})); mapperService.merge("_doc", List.of(mapping1, mapping2), MergeReason.INDEX_TEMPLATE); assertEquals(""" @@ -680,40 +676,39 @@ public void testMergeMultipleRootsWithoutRootType() throws IOException { public void testValidMappingSubstitution() throws IOException { CompressedXContent mapping1 = new CompressedXContent(""" - { - "properties": { - "field": { - "type": "keyword", - "ignore_above": 1024 - } - } - }"""); + { + "properties": { + "field": { + "type": "keyword", + "ignore_above": 1024 + } + } + }"""); CompressedXContent mapping2 = new CompressedXContent(""" - { - "properties": { - "field": { - "type": "long", - "coerce": true - } - } - }"""); + { + "properties": { + "field": { + "type": "long", + "coerce": true + } + } + }"""); - final MapperService mapperService = createMapperService(mapping(b -> { - })); + final MapperService mapperService = createMapperService(mapping(b -> {})); mapperService.merge("_doc", List.of(mapping1, mapping2), MergeReason.INDEX_TEMPLATE); assertEquals(""" - { - "_doc" : { - "properties" : { - "field" : { - "type" : "long", - "coerce" : true - } - } + { + "_doc" : { + "properties" : { + "field" : { + "type" : "long", + "coerce" : true } - }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); + } + } + }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); } public void testValidMappingSubtreeSubstitution() throws IOException { @@ -742,8 +737,7 @@ public void testValidMappingSubtreeSubstitution() throws IOException { } }"""); - final MapperService mapperService = createMapperService(mapping(b -> { - })); + final MapperService mapperService = createMapperService(mapping(b -> {})); mapperService.merge("_doc", List.of(mapping1, mapping2), MergeReason.INDEX_TEMPLATE); assertEquals(""" @@ -761,56 +755,55 @@ public void testValidMappingSubtreeSubstitution() throws IOException { public void testSameTypeMerge() throws IOException { CompressedXContent mapping1 = new CompressedXContent(""" - { - "properties": { - "field": { - "type": "keyword", - "ignore_above": 256, - "doc_values": false, - "fields": { - "text": { - "type": "text" - } - } + { + "properties": { + "field": { + "type": "keyword", + "ignore_above": 256, + "doc_values": false, + "fields": { + "text": { + "type": "text" } } - }"""); + } + } + }"""); CompressedXContent mapping2 = new CompressedXContent(""" - { - "properties": { - "field": { - "type": "keyword", - "ignore_above": 1024, - "fields": { - "other_text": { - "type": "text" - } - } + { + "properties": { + "field": { + "type": "keyword", + "ignore_above": 1024, + "fields": { + "other_text": { + "type": "text" } } - }"""); + } + } + }"""); - final MapperService mapperService = createMapperService(mapping(b -> { - })); + final MapperService mapperService = createMapperService(mapping(b -> {})); mapperService.merge("_doc", List.of(mapping1, mapping2), MergeReason.INDEX_TEMPLATE); assertEquals(""" - { - "_doc" : { - "properties" : { - "field" : { - "type" : "keyword", - "ignore_above" : 1024, - "fields" : { - "other_text" : { - "type" : "text" - } - } + { + "_doc" : { + "properties" : { + "field" : { + "type" : "keyword", + "ignore_above" : 1024, + "fields" : { + "other_text" : { + "type" : "text" } } } - }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); + } + } + }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); } public void testObjectAndNestedTypeSubstitution() throws IOException { @@ -843,8 +836,7 @@ public void testObjectAndNestedTypeSubstitution() throws IOException { } }"""); - final MapperService mapperService = createMapperService(mapping(b -> { - })); + final MapperService mapperService = createMapperService(mapping(b -> {})); mapperService.merge("_doc", List.of(mapping1, mapping2), MergeReason.INDEX_TEMPLATE); assertEquals(""" @@ -897,8 +889,7 @@ public void testNestedContradictingProperties() throws IOException { } }"""); - final MapperService mapperService = createMapperService(mapping(b -> { - })); + final MapperService mapperService = createMapperService(mapping(b -> {})); mapperService.merge("_doc", List.of(mapping1, mapping2), MergeReason.INDEX_TEMPLATE); assertEquals(""" @@ -924,40 +915,39 @@ public void testNestedContradictingProperties() throws IOException { public void testImplicitObjectHierarchy() throws IOException { CompressedXContent mapping1 = new CompressedXContent(""" - { + { + "properties": { + "parent": { "properties": { - "parent": { - "properties": { - "child.grandchild": { - "type": "keyword" - } - } + "child.grandchild": { + "type": "keyword" } } - }"""); + } + } + }"""); - final MapperService mapperService = createMapperService(mapping(b -> { - })); + final MapperService mapperService = createMapperService(mapping(b -> {})); DocumentMapper bulkMerge = mapperService.merge("_doc", List.of(mapping1), MergeReason.INDEX_TEMPLATE); assertEquals(""" - { - "_doc" : { + { + "_doc" : { + "properties" : { + "parent" : { "properties" : { - "parent" : { + "child" : { "properties" : { - "child" : { - "properties" : { - "grandchild" : { - "type" : "keyword" - } - } + "grandchild" : { + "type" : "keyword" } } } } } - }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); + } + } + }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); DocumentMapper sequentialMerge = mapperService.merge("_doc", mapping1, MergeReason.INDEX_TEMPLATE); assertEquals(bulkMerge.mappingSource(), sequentialMerge.mappingSource()); @@ -965,86 +955,84 @@ public void testImplicitObjectHierarchy() throws IOException { public void testSubobjectsMerge() throws IOException { CompressedXContent mapping1 = new CompressedXContent(""" - { - "properties": { - "parent": { - "type": "object", - "subobjects": false - } - } - }"""); + { + "properties": { + "parent": { + "type": "object", + "subobjects": false + } + } + }"""); CompressedXContent mapping2 = new CompressedXContent(""" - { + { + "properties": { + "parent": { "properties": { - "parent": { - "properties": { - "child.grandchild": { - "type": "keyword" - } - } + "child.grandchild": { + "type": "keyword" } } - }"""); + } + } + }"""); - final MapperService mapperService = createMapperService(mapping(b -> { - })); + final MapperService mapperService = createMapperService(mapping(b -> {})); mapperService.merge("_doc", List.of(mapping1, mapping2), MergeReason.INDEX_TEMPLATE); assertEquals(""" - { - "_doc" : { + { + "_doc" : { + "properties" : { + "parent" : { + "subobjects" : false, "properties" : { - "parent" : { - "subobjects" : false, - "properties" : { - "child.grandchild" : { - "type" : "keyword" - } - } + "child.grandchild" : { + "type" : "keyword" } } } - }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); + } + } + }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); } public void testSubobjectsMergeNotInRoot() throws IOException { CompressedXContent mapping1 = new CompressedXContent(""" - { + { + "properties": { + "field": { + "type": "object", + "subobjects": false, "properties": { - "field": { - "type": "object", - "subobjects": false, - "properties": { - "subfield1": { - "type": "text" - } - } + "subfield1": { + "type": "text" } } - }"""); + } + } + }"""); CompressedXContent mapping2 = new CompressedXContent(""" - { + { + "properties": { + "field": { + "type": "object", + "subobjects": true, "properties": { - "field": { - "type": "object", - "subobjects": true, - "properties": { - "subfield2": { - "type": "text" - } - } + "subfield2": { + "type": "text" } } - }"""); + } + } + }"""); - final MapperService mapperService = createMapperService(mapping(b -> { - })); + final MapperService mapperService = createMapperService(mapping(b -> {})); MapperParsingException e = expectThrows( - MapperParsingException.class, - () -> mapperService.merge("_doc", List.of(mapping1, mapping2), MergeReason.INDEX_TEMPLATE) + MapperParsingException.class, + () -> mapperService.merge("_doc", List.of(mapping1, mapping2), MergeReason.INDEX_TEMPLATE) ); assertEquals("contradicting subobjects settings provided for field: field", e.getMessage()); } @@ -1069,88 +1057,86 @@ public void testSubobjectsImplicitObjectsMerge() throws IOException { } }"""); - final MapperService mapperService = createMapperService(mapping(b -> { - })); + final MapperService mapperService = createMapperService(mapping(b -> {})); MapperParsingException e = expectThrows( MapperParsingException.class, - () -> mapperService.merge("_doc", List.of(mapping1, mapping2), MergeReason.INDEX_TEMPLATE) + () -> mapperService.merge("_doc", List.of(mapping1, mapping2), MergeReason.INDEX_TEMPLATE) ); assertThat(e.getMessage(), containsString("Tried to add subobject [child] to object [parent] which does not support subobjects")); } public void testMultipleTypeMerges() throws IOException { CompressedXContent mapping1 = new CompressedXContent(""" - { - "properties" : { - "parent": { + { + "properties" : { + "parent": { + "type": "object", + "properties": { + "child": { "type": "object", "properties": { - "child": { - "type": "object", - "properties": { - "grandchild1": { - "type": "keyword" - }, - "grandchild2": { - "type": "date" - } - } + "grandchild1": { + "type": "keyword" + }, + "grandchild2": { + "type": "date" } } } } - }"""); + } + } + }"""); CompressedXContent mapping2 = new CompressedXContent(""" - { - "properties" : { - "parent": { - "type": "object", + { + "properties" : { + "parent": { + "type": "object", + "properties": { + "child": { + "type": "nested", "properties": { - "child": { - "type": "nested", - "properties": { - "grandchild1": { - "type": "text" - }, - "grandchild3": { - "type": "text" - } - } + "grandchild1": { + "type": "text" + }, + "grandchild3": { + "type": "text" } } } } - }"""); + } + } + }"""); - final MapperService mapperService = createMapperService(mapping(b -> { - })); + final MapperService mapperService = createMapperService(mapping(b -> {})); mapperService.merge("_doc", List.of(mapping1, mapping2), MergeReason.INDEX_TEMPLATE); assertEquals(""" - { - "_doc" : { + { + "_doc" : { + "properties" : { + "parent" : { "properties" : { - "parent" : { + "child" : { + "type" : "nested", "properties" : { - "child" : { - "type" : "nested", - "properties" : { - "grandchild1" : { - "type" : "text" - }, - "grandchild2" : { - "type" : "date" - }, - "grandchild3" : { - "type" : "text" - } - } + "grandchild1" : { + "type" : "text" + }, + "grandchild2" : { + "type" : "date" + }, + "grandchild3" : { + "type" : "text" } } } } } - }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); + } + } + }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); } } From 32ef7f24936acf4297eb1b1942db9f688fcc1e2e Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Thu, 24 Aug 2023 12:06:43 +0300 Subject: [PATCH 31/38] Update skip version --- .../test/indices.put_index_template/15_composition.yml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.put_index_template/15_composition.yml b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.put_index_template/15_composition.yml index e60aa61986a19..4046e04d93df0 100644 --- a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.put_index_template/15_composition.yml +++ b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.put_index_template/15_composition.yml @@ -353,8 +353,8 @@ --- "Composable index templates that include subobjects: false at root": - skip: - version: ' - 8.9.1' - reason: 'https://github.com/elastic/elasticsearch/issues/96768 fixed after 8.9.0' + version: ' - 8.9.2' + reason: 'https://github.com/elastic/elasticsearch/issues/96768 fixed after 8.9.1' features: allowed_warnings - do: @@ -400,8 +400,8 @@ --- "Composable index templates that include subobjects: false on arbitrary field": - skip: - version: ' - 8.9.1' - reason: 'https://github.com/elastic/elasticsearch/issues/96768 fixed after 8.9.0' + version: ' - 8.9.2' + reason: 'https://github.com/elastic/elasticsearch/issues/96768 fixed after 8.9.1' features: allowed_warnings - do: From 5ff854b7d992bd896c44626a909f38acfadef49b Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Thu, 24 Aug 2023 13:03:07 +0300 Subject: [PATCH 32/38] Update skip version --- .../test/indices.put_index_template/15_composition.yml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.put_index_template/15_composition.yml b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.put_index_template/15_composition.yml index 4046e04d93df0..98e9650cd239c 100644 --- a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.put_index_template/15_composition.yml +++ b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.put_index_template/15_composition.yml @@ -353,8 +353,8 @@ --- "Composable index templates that include subobjects: false at root": - skip: - version: ' - 8.9.2' - reason: 'https://github.com/elastic/elasticsearch/issues/96768 fixed after 8.9.1' + version: ' - 8.10.1' + reason: 'https://github.com/elastic/elasticsearch/issues/96768 fixed after 8.10.0' features: allowed_warnings - do: @@ -400,8 +400,8 @@ --- "Composable index templates that include subobjects: false on arbitrary field": - skip: - version: ' - 8.9.2' - reason: 'https://github.com/elastic/elasticsearch/issues/96768 fixed after 8.9.1' + version: ' - 8.10.1' + reason: 'https://github.com/elastic/elasticsearch/issues/96768 fixed after 8.10.0' features: allowed_warnings - do: From 39c7d0407790af87c967d6f1691d7ff9e17e33a5 Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Thu, 24 Aug 2023 19:09:58 +0300 Subject: [PATCH 33/38] Add raw mapping merge unit tests --- .../common/xcontent/XContentHelper.java | 17 +- .../xcontent/support/XContentHelperTests.java | 147 ++++++++++++++++-- 2 files changed, 151 insertions(+), 13 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/xcontent/XContentHelper.java b/server/src/main/java/org/elasticsearch/common/xcontent/XContentHelper.java index e813919bb0e03..e11edd1dbf89d 100644 --- a/server/src/main/java/org/elasticsearch/common/xcontent/XContentHelper.java +++ b/server/src/main/java/org/elasticsearch/common/xcontent/XContentHelper.java @@ -509,7 +509,10 @@ public static void merge( first.put(toMergeEntry.getKey(), mergedList); } } else if (customMerge != null) { - first.put(toMergeEntry.getKey(), customMerge.merge(parent, toMergeEntry.getKey(), baseValue, toMergeEntry.getValue())); + Object mergedValue = customMerge.merge(parent, toMergeEntry.getKey(), baseValue, toMergeEntry.getValue()); + if (mergedValue != null) { + first.put(toMergeEntry.getKey(), mergedValue); + } } } } @@ -534,13 +537,23 @@ private static boolean allListValuesAreMapsOfOne(List list) { public interface CustomMerge { /** * Based on the provided arguments, compute a value to use for the given key as a merge result. + * If this method returns a non-{@code null} value, then the merge result will replace the original value of the provided key in + * the base map. + * If this method returns {@code null}, then: + *
    + *
  • if the values are of map types, the old and new values will be merged recursively + *
  • otherwise, the original value will be maintained + *
* This method doesn't throw a checked exception, but it is expected that illegal merges will result in a {@link RuntimeException}. * @param parent merged field's parent * @param key merged field's name * @param oldValue original value of the provided key * @param newValue the new value of the provided key which is to be merged with the original - * @return the merged value to use for the given key, or {@code null} if there is no custom merge required for it + * @return the merged value to use for the given key, or {@code null} if there is no custom merge result for it. If {@code null} + * is returned, the algorithm will live the original value as is, unless it is a map, in which case the new map will be merged + * into the old map recursively. */ + @Nullable Object merge(String parent, String key, Object oldValue, Object newValue); } diff --git a/server/src/test/java/org/elasticsearch/common/xcontent/support/XContentHelperTests.java b/server/src/test/java/org/elasticsearch/common/xcontent/support/XContentHelperTests.java index 8ea3f9514a287..d1972a041bf34 100644 --- a/server/src/test/java/org/elasticsearch/common/xcontent/support/XContentHelperTests.java +++ b/server/src/test/java/org/elasticsearch/common/xcontent/support/XContentHelperTests.java @@ -70,32 +70,157 @@ public void testMergingListValuesAreMapsOfOne() { public void testMergingDefaults() { Map content = getMap("key1", "content", "key3", "content"); Map defaults = getMap("key2", "default", "key3", "default"); - Map expected = getMap("key1", "content", "key2", "default", "key3", "content"); XContentHelper.mergeDefaults(content, defaults); + Map expected = getMap("key1", "content", "key2", "default", "key3", "content"); assertThat(content, equalTo(expected)); } public void testMergingWithCustomMerge() { - Map content = getMap("key1", "old", "key3", "old", "key4", "old"); - Map defaults = getMap("key2", "new", "key3", "new", "key4", "new"); + Map base = getMap("key1", "old", "key3", "old", "key4", "old"); + Map toMerge = getMap("key2", "new", "key3", "new", "key4", "new"); + XContentHelper.merge(base, toMerge, (parent, key, oldValue, newValue) -> "key3".equals(key) ? newValue : oldValue); Map expected = getMap("key1", "old", "key2", "new", "key3", "new", "key4", "old"); - XContentHelper.merge(content, defaults, (parent, key, oldValue, newValue) -> "key3".equals(key) ? newValue : oldValue); - assertThat(content, equalTo(expected)); + assertThat(base, equalTo(expected)); + } + + public void testMergingWithCustomMapReplacement() { + Map base = getMap( + "key1", + "old", + "key3", + "old", + "key4", + "old", + "map", + Map.of("key1", "old", "key3", "old", "key4", "old") + ); + Map toMerge = getMap( + "key2", + "new", + "key3", + "new", + "key4", + "new", + "map", + Map.of("key2", "new", "key3", "new", "key4", "new") + ); + XContentHelper.merge( + base, + toMerge, + (parent, key, oldValue, newValue) -> "key3".equals(key) || "map".equals(key) ? newValue : oldValue + ); + Map expected = getMap( + "key1", + "old", + "key2", + "new", + "key3", + "new", + "key4", + "old", + "map", + Map.of("key2", "new", "key3", "new", "key4", "new") + ); + assertThat(base, equalTo(expected)); + } + + public void testMergingWithCustomMapMerge() { + Map base = getMap( + "key1", + "old", + "key3", + "old", + "key4", + "old", + "map", + new HashMap<>(Map.of("key1", "old", "key3", "old", "key4", "old")) + ); + Map toMerge = getMap( + "key2", + "new", + "key3", + "new", + "key4", + "new", + "map", + Map.of("key2", "new", "key3", "new", "key4", "new") + ); + XContentHelper.merge(base, toMerge, (parent, key, oldValue, newValue) -> "key3".equals(key) ? oldValue : null); + Map expected = getMap( + "key1", + "old", + "key2", + "new", + "key3", + "old", + "key4", + "old", + "map", + Map.of("key1", "old", "key2", "new", "key3", "old", "key4", "old") + ); + assertThat(base, equalTo(expected)); + } + + public void testMergingListValueWithCustomMapReplacement() { + Map base = getMap( + "key", + List.of("value1", "value3", "value4"), + "list", + List.of(new HashMap<>(Map.of("map", new HashMap<>(Map.of("key1", "old", "key3", "old", "key4", "old"))))) + ); + Map toMerge = getMap( + "key", + List.of("value1", "value2", "value4"), + "list", + List.of(Map.of("map", Map.of("key2", "new", "key3", "new", "key4", "new"))) + ); + XContentHelper.merge( + base, + toMerge, + (parent, key, oldValue, newValue) -> "key3".equals(key) || "map".equals(key) ? newValue : oldValue + ); + Map expected = getMap( + "key", + List.of("value1", "value2", "value4", "value3"), + "list", + List.of(Map.of("map", Map.of("key2", "new", "key3", "new", "key4", "new"))) + ); + assertThat(base, equalTo(expected)); + } + + public void testMergingListValueWithCustomMapMerge() { + Map base = getMap( + "key", + List.of("value1", "value3", "value4"), + "list", + List.of(new HashMap<>(Map.of("map", new HashMap<>(Map.of("key1", "old", "key3", "old", "key4", "old"))))) + ); + Map toMerge = getMap( + "key", + List.of("value1", "value2", "value4"), + "list", + List.of(Map.of("map", Map.of("key2", "new", "key3", "new", "key4", "new"))) + ); + XContentHelper.merge(base, toMerge, (parent, key, oldValue, newValue) -> "key3".equals(key) ? newValue : null); + Map expected = getMap( + "key", + List.of("value1", "value2", "value4", "value3"), + "list", + List.of(Map.of("map", Map.of("key1", "old", "key2", "new", "key3", "new", "key4", "old"))) + ); + assertThat(base, equalTo(expected)); } public void testMergingWithCustomMergeWithException() { - final Map content = getMap("key1", "old", "key3", "old", "key4", "old"); - final Map defaults = getMap("key2", "new", "key3", "new", "key4", "new"); + final Map base = getMap("key1", "old", "key3", "old", "key4", "old"); + final Map toMerge = getMap("key2", "new", "key3", "new", "key4", "new"); final XContentHelper.CustomMerge customMerge = (parent, key, oldValue, newValue) -> { if ("key3".equals(key)) { throw new IllegalArgumentException(key + " is not allowed"); } return oldValue; }; - IllegalArgumentException e = expectThrows( - IllegalArgumentException.class, - () -> XContentHelper.merge(content, defaults, customMerge) - ); + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> XContentHelper.merge(base, toMerge, customMerge)); assertThat(e.getMessage(), containsString("key3 is not allowed")); } From d54a8eb1a30b6a9bcbc8990c68ad96d87c0c1f06 Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Tue, 29 Aug 2023 12:35:35 +0300 Subject: [PATCH 34/38] Apply review proposals and add a test case --- .../15_composition.yml | 8 +- .../common/xcontent/XContentHelper.java | 4 +- .../index/mapper/MapperErrors.java | 2 +- .../index/mapper/MapperService.java | 27 ++++++ .../index/mapper/DocumentMapperTests.java | 4 +- .../index/mapper/MapperServiceTests.java | 94 +++++++++++++++++++ .../index/mapper/ObjectMapperTests.java | 2 +- 7 files changed, 131 insertions(+), 10 deletions(-) diff --git a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.put_index_template/15_composition.yml b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.put_index_template/15_composition.yml index 98e9650cd239c..51c12892c4859 100644 --- a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.put_index_template/15_composition.yml +++ b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.put_index_template/15_composition.yml @@ -353,8 +353,8 @@ --- "Composable index templates that include subobjects: false at root": - skip: - version: ' - 8.10.1' - reason: 'https://github.com/elastic/elasticsearch/issues/96768 fixed after 8.10.0' + version: ' - 8.10.99' + reason: 'https://github.com/elastic/elasticsearch/issues/96768 fixed at 8.11.0' features: allowed_warnings - do: @@ -400,8 +400,8 @@ --- "Composable index templates that include subobjects: false on arbitrary field": - skip: - version: ' - 8.10.1' - reason: 'https://github.com/elastic/elasticsearch/issues/96768 fixed after 8.10.0' + version: ' - 8.10.99' + reason: 'https://github.com/elastic/elasticsearch/issues/96768 fixed at 8.11.0' features: allowed_warnings - do: diff --git a/server/src/main/java/org/elasticsearch/common/xcontent/XContentHelper.java b/server/src/main/java/org/elasticsearch/common/xcontent/XContentHelper.java index e11edd1dbf89d..70269b9e321c8 100644 --- a/server/src/main/java/org/elasticsearch/common/xcontent/XContentHelper.java +++ b/server/src/main/java/org/elasticsearch/common/xcontent/XContentHelper.java @@ -431,7 +431,7 @@ public static void merge(Map first, Map second, * Merges the map provided as the second parameter into the content of the first. Only does recursive merge for inner maps. * If a non-null {@link CustomMerge} is provided, it is applied whenever a merge is required, meaning - whenever both the first and * the second map has values for the same key. Otherwise, values from the first map will always have precedence, meaning - if the - * first map contains a key, its value will not be overriden. + * first map contains a key, its value will not be overridden. * * @param parent used for recursion to maintain knowledge about the common parent of the currently merged sub-maps, if such exists * @param first the map which serves as the merge base @@ -541,7 +541,7 @@ public interface CustomMerge { * the base map. * If this method returns {@code null}, then: *
    - *
  • if the values are of map types, the old and new values will be merged recursively + *
  • if the values are of map type, the old and new values will be merged recursively *
  • otherwise, the original value will be maintained *
* This method doesn't throw a checked exception, but it is expected that illegal merges will result in a {@link RuntimeException}. diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperErrors.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperErrors.java index 6097144d6a8ac..29b5b18d12b1e 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperErrors.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperErrors.java @@ -14,6 +14,6 @@ static void throwObjectMappingConflictError(String fieldName) throws IllegalArgu } static void throwNestedMappingConflictError(String fieldName) throws IllegalArgumentException { - throw new IllegalArgumentException("can't merge a non nested mapping [" + fieldName + "] with a nested mapping"); + throw new IllegalArgumentException("can't merge a non-nested mapping [" + fieldName + "] with a nested mapping"); } } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index a6994ed276581..358cc0099e8c9 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -405,6 +405,31 @@ public DocumentMapper merge(String type, List mappingSources return (mergedRawMapping != null) ? doMerge(type, reason, mergedRawMapping) : null; } + /** + * A {@link org.elasticsearch.common.xcontent.XContentHelper.CustomMerge} for raw map merges that are suitable for index/field mappings. + * The default raw map merge algorithm doesn't override values - if there are multiple values for a key, then: + *
    + *
  • if the values are of map type, the old and new values will be merged recursively + *
  • otherwise, the original value will be maintained + *
+ * When merging field mappings, we want something else. Specifically: + *
    + *
  • within field mappings node (which is nested within a {@code properties} node): + *
      + *
    • if both the base mapping and the mapping to merge into it are of mergeable types (e.g {@code object -> object}, + * {@code object -> nested}), then we only want to merge specific mapping entries: + *
        + *
      • {@code properties} node - merging fields from both mappings + *
      • {@code subobjects} entry - since this setting affects an entire subtree, we need to keep it when merging + *
      + *
    • otherwise, for any couple of non-mergeable types ((e.g {@code object -> long}, {@code long -> long}) - we just want + * to replace the entire mappings subtree, let the last one win + *
    + *
  • any other map values that are not encountered within a {@code properties} node (e.g. "_doc", "_meta" or "properties" + * itself - apply recursive merge as the default algorithm would apply + *
  • any non-map values - override the value of the base map with the value of the merged map + *
+ */ private static class RawFieldMappingMerge implements XContentHelper.CustomMerge { private static final XContentHelper.CustomMerge INSTANCE = new RawFieldMappingMerge(); @@ -424,9 +449,11 @@ public Object merge(String parent, String key, Object oldValue, Object newValue) // if two field mappings are to be merged, we only want to keep some specific entries from the base mapping and // let all others be overridden by the second mapping Map mergedMappings = new HashMap<>(); + // we must keep the "properties" node, otherwise our merge has no point if (baseMap.containsKey("properties")) { mergedMappings.put("properties", new HashMap<>((Map) baseMap.get("properties"))); } + // the "subobjects" setting affects an entire subtree and not only locally where it is configured if (baseMap.containsKey("subobjects")) { mergedMappings.put("subobjects", baseMap.get("subobjects")); } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/DocumentMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/DocumentMapperTests.java index 3d3d85c23bd5b..f0458add93c78 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/DocumentMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/DocumentMapperTests.java @@ -95,14 +95,14 @@ public void testMergeObjectAndNested() throws Exception { IllegalArgumentException.class, () -> MapperService.mergeMappings(objectMapper, nestedMapper.mapping(), reason) ); - assertThat(e.getMessage(), containsString("can't merge a non nested mapping [obj] with a nested mapping")); + assertThat(e.getMessage(), containsString("can't merge a non-nested mapping [obj] with a nested mapping")); } { IllegalArgumentException e = expectThrows( IllegalArgumentException.class, () -> MapperService.mergeMappings(nestedMapper, objectMapper.mapping(), reason) ); - assertThat(e.getMessage(), containsString("can't merge a non nested mapping [obj] with a nested mapping")); + assertThat(e.getMessage(), containsString("can't merge a non-nested mapping [obj] with a nested mapping")); } } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java index 5e904e858b350..d1be559b28a60 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java @@ -27,6 +27,7 @@ import static org.hamcrest.CoreMatchers.containsString; import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.notNullValue; import static org.hamcrest.Matchers.nullValue; @@ -1139,4 +1140,97 @@ public void testMultipleTypeMerges() throws IOException { } }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); } + + public void testPropertiesField() throws IOException { + CompressedXContent mapping1 = new CompressedXContent(""" + { + "properties": { + "properties": { + "type": "object", + "properties": { + "child": { + "type": "object", + "dynamic": true, + "properties": { + "grandchild": { + "type": "keyword" + } + } + } + } + } + } + }"""); + + CompressedXContent mapping2 = new CompressedXContent(""" + { + "properties": { + "properties": { + "properties": { + "child": { + "type": "long", + "coerce": true + } + } + } + } + }"""); + + MapperService mapperService = createMapperService(mapping(b -> { + })); + mapperService.merge("_doc", List.of(mapping1, mapping2), MergeReason.INDEX_TEMPLATE); + assertEquals(""" + { + "_doc" : { + "properties" : { + "properties" : { + "properties" : { + "child" : { + "type" : "long", + "coerce" : true + } + } + } + } + } + }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); + + Mapper propertiesMapper = mapperService.documentMapper().mapping().getRoot().getMapper("properties"); + assertThat(propertiesMapper, instanceOf(ObjectMapper.class)); + Mapper childMapper = ((ObjectMapper) propertiesMapper).getMapper("child"); + assertThat(childMapper, instanceOf(FieldMapper.class)); + assertEquals("long", childMapper.typeName()); + + // Now checking the opposite merge + mapperService = createMapperService(mapping(b -> { + })); + mapperService.merge("_doc", List.of(mapping2, mapping1), MergeReason.INDEX_TEMPLATE); + assertEquals(""" + { + "_doc" : { + "properties" : { + "properties" : { + "properties" : { + "child" : { + "dynamic" : "true", + "properties" : { + "grandchild" : { + "type" : "keyword" + } + } + } + } + } + } + } + }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); + + propertiesMapper = mapperService.documentMapper().mapping().getRoot().getMapper("properties"); + assertThat(propertiesMapper, instanceOf(ObjectMapper.class)); + childMapper = ((ObjectMapper) propertiesMapper).getMapper("child"); + assertThat(childMapper, instanceOf(ObjectMapper.class)); + Mapper grandchildMapper = ((ObjectMapper) childMapper).getMapper("grandchild"); + assertThat(grandchildMapper, instanceOf(FieldMapper.class)); + assertEquals("keyword", grandchildMapper.typeName()); + } } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java index 943a7bb58ba22..3c77bf20b37d2 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java @@ -293,7 +293,7 @@ public void testDisallowFieldReplacementForIndexTemplates() throws IOException { IllegalArgumentException.class, () -> mapperService.merge(MapperService.SINGLE_MAPPING_NAME, new CompressedXContent(firstUpdate), MergeReason.INDEX_TEMPLATE) ); - assertThat(e.getMessage(), containsString("can't merge a non nested mapping [object.field2] with a nested mapping")); + assertThat(e.getMessage(), containsString("can't merge a non-nested mapping [object.field2] with a nested mapping")); String secondUpdate = Strings.toString( XContentFactory.jsonBuilder() From 551685c42266615d758da2eb3694d9b33669c3ac Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Tue, 29 Aug 2023 12:53:03 +0300 Subject: [PATCH 35/38] goddamned you spotless --- .../org/elasticsearch/index/mapper/MapperServiceTests.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java index d1be559b28a60..11220d4f26451 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java @@ -1176,8 +1176,7 @@ public void testPropertiesField() throws IOException { } }"""); - MapperService mapperService = createMapperService(mapping(b -> { - })); + MapperService mapperService = createMapperService(mapping(b -> {})); mapperService.merge("_doc", List.of(mapping1, mapping2), MergeReason.INDEX_TEMPLATE); assertEquals(""" { @@ -1202,8 +1201,7 @@ public void testPropertiesField() throws IOException { assertEquals("long", childMapper.typeName()); // Now checking the opposite merge - mapperService = createMapperService(mapping(b -> { - })); + mapperService = createMapperService(mapping(b -> {})); mapperService.merge("_doc", List.of(mapping2, mapping1), MergeReason.INDEX_TEMPLATE); assertEquals(""" { From e243d5f6dc6f8730d8ba75498cce17559da7b7fc Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Tue, 5 Sep 2023 16:59:31 +0300 Subject: [PATCH 36/38] Remove subobects contradiction invalidation --- .../index/mapper/MapperService.java | 13 +-- .../MetadataIndexTemplateServiceTests.java | 21 +++-- .../index/mapper/MapperServiceTests.java | 87 ++++++++++--------- 3 files changed, 65 insertions(+), 56 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index 358cc0099e8c9..3d98804bae88e 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -469,16 +469,9 @@ public Object merge(String parent, String key, Object oldValue, Object newValue) // field mapping entries of Map type (like "fields" and "meta") are handled above and should never reach here return null; } else { - switch (key) { - case "subobjects" -> { - if (oldValue.equals(newValue) == false) { - throw new MapperParsingException("contradicting subobjects settings provided for field: " + parent); - } - } - case "required" -> { - if ("_routing".equals(parent) && oldValue != newValue) { - throw new MapperParsingException("contradicting `_routing.required` settings"); - } + if (key.equals("required")) { + if ("_routing".equals(parent) && oldValue != newValue) { + throw new MapperParsingException("contradicting `_routing.required` settings"); } } return newValue; 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 b30211deff031..993146362acad 100644 --- a/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateServiceTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateServiceTests.java @@ -1858,8 +1858,12 @@ public void testIndexTemplateFailsToOverrideComponentTemplateMappingField() thro { "properties": { "field2": { - "type": "object", - "subobjects": true + "type": "object", + "properties": { + "bar": { + "type": "object" + } + } } } }"""), null), randomBoolean() ? Arrays.asList("c1", "c2") : Arrays.asList("c2", "c1"), 0L, 1L, null, null, null); @@ -1879,7 +1883,10 @@ public void testIndexTemplateFailsToOverrideComponentTemplateMappingField() thro assertThat(e.getCause().getMessage(), containsString("invalid composite mappings for [my-template]")); assertNotNull(e.getCause().getCause()); - assertThat(e.getCause().getCause().getMessage(), containsString("contradicting subobjects settings provided for field: field2")); + assertThat( + e.getCause().getCause().getMessage(), + containsString("Tried to add subobject [bar] to object [field2] which does not support subobjects") + ); } /** @@ -1965,7 +1972,11 @@ public void testUpdateComponentTemplateFailsIfResolvedIndexTemplatesWouldBeInval "properties": { "field2": { "type": "object", - "subobjects": true + "properties": { + "bar": { + "type": "object" + } + } } } } @@ -1991,7 +2002,7 @@ public void testUpdateComponentTemplateFailsIfResolvedIndexTemplatesWouldBeInval assertNotNull(e.getCause().getCause().getCause()); assertThat( e.getCause().getCause().getCause().getMessage(), - containsString("contradicting subobjects settings provided for field: field2") + containsString("Tried to add subobject [bar] to object [field2] which does not support subobjects") ); } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java index 11220d4f26451..69b44d383193a 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java @@ -437,35 +437,6 @@ public void testMergeSubobjectsFalseOrder() throws IOException { assertEquals(subobjectsFirst.mappingSource(), subobjectsLast.mappingSource()); } - public void testMergeContradictingSubobjectsInRoot() throws IOException { - final MapperService mapperService = createMapperService(mapping(b -> {})); - CompressedXContent mapping1 = createTestMapping1(); - CompressedXContent mapping2 = createTestMapping2(); - CompressedXContent subobjectsTrueExplicitly; - try (XContentBuilder xContentBuilder = XContentFactory.jsonBuilder()) { - subobjectsTrueExplicitly = new CompressedXContent( - BytesReference.bytes( - xContentBuilder.startObject() - .startObject("_doc") - .field("subobjects", true) - .startObject("properties") - .startObject("parent.other_subfield") - .field("type", "text") - .endObject() - .endObject() - .endObject() - .endObject() - ) - ); - } - - MapperParsingException e = expectThrows( - MapperParsingException.class, - () -> mapperService.merge("_doc", List.of(mapping2, mapping1, subobjectsTrueExplicitly), MergeReason.INDEX_TEMPLATE) - ); - assertEquals("contradicting subobjects settings provided for field: _doc", e.getMessage()); - } - private static CompressedXContent createTestMapping1() throws IOException { CompressedXContent mapping1; try (XContentBuilder xContentBuilder = XContentFactory.jsonBuilder()) { @@ -998,15 +969,15 @@ public void testSubobjectsMerge() throws IOException { }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); } - public void testSubobjectsMergeNotInRoot() throws IOException { + public void testContradictingSubobjects() throws IOException { CompressedXContent mapping1 = new CompressedXContent(""" { "properties": { - "field": { + "parent": { "type": "object", "subobjects": false, "properties": { - "subfield1": { + "child.grandchild": { "type": "text" } } @@ -1017,25 +988,59 @@ public void testSubobjectsMergeNotInRoot() throws IOException { CompressedXContent mapping2 = new CompressedXContent(""" { "properties": { - "field": { + "parent": { "type": "object", "subobjects": true, "properties": { - "subfield2": { - "type": "text" + "child.grandchild": { + "type": "long" } } } } }"""); - final MapperService mapperService = createMapperService(mapping(b -> {})); + MapperService mapperService = createMapperService(mapping(b -> {})); + mapperService.merge("_doc", List.of(mapping1, mapping2), MergeReason.INDEX_TEMPLATE); - MapperParsingException e = expectThrows( - MapperParsingException.class, - () -> mapperService.merge("_doc", List.of(mapping1, mapping2), MergeReason.INDEX_TEMPLATE) - ); - assertEquals("contradicting subobjects settings provided for field: field", e.getMessage()); + assertEquals(""" + { + "_doc" : { + "properties" : { + "parent" : { + "subobjects" : true, + "properties" : { + "child" : { + "properties" : { + "grandchild" : { + "type" : "long" + } + } + } + } + } + } + } + }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); + + mapperService = createMapperService(mapping(b -> {})); + mapperService.merge("_doc", List.of(mapping2, mapping1), MergeReason.INDEX_TEMPLATE); + + assertEquals(""" + { + "_doc" : { + "properties" : { + "parent" : { + "subobjects" : false, + "properties" : { + "child.grandchild" : { + "type" : "text" + } + } + } + } + } + }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); } public void testSubobjectsImplicitObjectsMerge() throws IOException { From 817a346a32d597ab380fedff75b4c76c5f4a0fd0 Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Wed, 13 Sep 2023 11:32:37 +0300 Subject: [PATCH 37/38] Apply review suggestions --- .../index/mapper/MapperService.java | 7 ++-- .../index/mapper/MappingParser.java | 2 +- .../xcontent/support/XContentHelperTests.java | 33 ++++++++++++++++--- 3 files changed, 34 insertions(+), 8 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index 3d98804bae88e..9f9e45f53b837 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -426,7 +426,7 @@ public DocumentMapper merge(String type, List mappingSources * to replace the entire mappings subtree, let the last one win * *
  • any other map values that are not encountered within a {@code properties} node (e.g. "_doc", "_meta" or "properties" - * itself - apply recursive merge as the default algorithm would apply + * itself) - apply recursive merge as the default algorithm would apply *
  • any non-map values - override the value of the base map with the value of the merged map * */ @@ -470,6 +470,9 @@ public Object merge(String parent, String key, Object oldValue, Object newValue) return null; } else { if (key.equals("required")) { + // we look for explicit `_routing.required` settings because we use them to detect contradictions of this setting + // that comes from mappings with such that comes from the optional `data_stream` configuration of composable index + // templates if ("_routing".equals(parent) && oldValue != newValue) { throw new MapperParsingException("contradicting `_routing.required` settings"); } @@ -484,7 +487,7 @@ public Object merge(String parent, String key, Object oldValue, Object newValue) * * @param mappings1 first mapping of a field * @param mappings2 second mapping of a field - * @return true if mapping + * @return {@code true} if the second mapping should be merged into the first mapping */ private boolean shouldMergeFieldMappings(Map mappings1, Map mappings2) { String type1 = (String) mappings1.get("type"); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MappingParser.java b/server/src/main/java/org/elasticsearch/index/mapper/MappingParser.java index 1edd7e588ca54..4cc0a48e939b1 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MappingParser.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MappingParser.java @@ -95,7 +95,7 @@ Mapping parse(@Nullable String type, CompressedXContent source) throws MapperPar Mapping parse(@Nullable String type, Map mappingSource) throws MapperParsingException { if (mappingSource.isEmpty()) { if (type == null) { - throw new MapperParsingException("malformed mappingSource, no type name found"); + throw new MapperParsingException("malformed mapping, no type name found"); } } else { String rootName = mappingSource.keySet().iterator().next(); diff --git a/server/src/test/java/org/elasticsearch/common/xcontent/support/XContentHelperTests.java b/server/src/test/java/org/elasticsearch/common/xcontent/support/XContentHelperTests.java index d1972a041bf34..aab04d3a26a07 100644 --- a/server/src/test/java/org/elasticsearch/common/xcontent/support/XContentHelperTests.java +++ b/server/src/test/java/org/elasticsearch/common/xcontent/support/XContentHelperTests.java @@ -68,11 +68,34 @@ public void testMergingListValuesAreMapsOfOne() { } public void testMergingDefaults() { - Map content = getMap("key1", "content", "key3", "content"); - Map defaults = getMap("key2", "default", "key3", "default"); - XContentHelper.mergeDefaults(content, defaults); - Map expected = getMap("key1", "content", "key2", "default", "key3", "content"); - assertThat(content, equalTo(expected)); + Map base = getMap( + "key1", + "old", + "key3", + "old", + "map", + getMap("key1", "old", "key3", "old") + ); + Map toMerge = getMap( + "key2", + "new", + "key3", + "new", + "map", + getMap("key2", "new", "key3", "new") + ); + XContentHelper.mergeDefaults(base, toMerge); + Map expected = getMap( + "key1", + "old", + "key2", + "new", + "key3", + "old", + "map", + Map.of("key1", "old", "key2", "new", "key3", "old") + ); + assertThat(base, equalTo(expected)); } public void testMergingWithCustomMerge() { From 5f11c39dcac885ac62f962881a451d14cafe7468 Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Wed, 13 Sep 2023 11:47:38 +0200 Subject: [PATCH 38/38] Apply spotless suggestions --- .../xcontent/support/XContentHelperTests.java | 18 ++---------------- 1 file changed, 2 insertions(+), 16 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/common/xcontent/support/XContentHelperTests.java b/server/src/test/java/org/elasticsearch/common/xcontent/support/XContentHelperTests.java index aab04d3a26a07..5b50eb63e1489 100644 --- a/server/src/test/java/org/elasticsearch/common/xcontent/support/XContentHelperTests.java +++ b/server/src/test/java/org/elasticsearch/common/xcontent/support/XContentHelperTests.java @@ -68,22 +68,8 @@ public void testMergingListValuesAreMapsOfOne() { } public void testMergingDefaults() { - Map base = getMap( - "key1", - "old", - "key3", - "old", - "map", - getMap("key1", "old", "key3", "old") - ); - Map toMerge = getMap( - "key2", - "new", - "key3", - "new", - "map", - getMap("key2", "new", "key3", "new") - ); + Map base = getMap("key1", "old", "key3", "old", "map", getMap("key1", "old", "key3", "old")); + Map toMerge = getMap("key2", "new", "key3", "new", "map", getMap("key2", "new", "key3", "new")); XContentHelper.mergeDefaults(base, toMerge); Map expected = getMap( "key1",