Remove DocumentMapper#merge method (elastic#69005)

DocumentMapper exposes a merge method that simply calls merge on the inner Mapping object. We have started to migrate away from building the entire document mapper only for merging, as the inner Mapping object is enough to do so. This will allow us to further simplify and isolate our merging code so that it may not even require a MapperService instance.

With this commit, we remove the DocumentMapper#merge method in favour of calling Mapping#merge instead.

Author: javanna

server/src/main/java/org/elasticsearch/cluster/metadata/MetadataMappingService.java

@@ -30,6 +30,7 @@
 import org.elasticsearch.index.mapper.DocumentMapper;
 import org.elasticsearch.index.mapper.MapperService;
 import org.elasticsearch.index.mapper.MapperService.MergeReason;
+import org.elasticsearch.index.mapper.Mapping;
 import org.elasticsearch.indices.IndicesService;
 
 import java.io.IOException;
@@ -103,12 +104,9 @@ private ClusterState applyRequest(ClusterState currentState, PutMappingClusterSt
                 // we used for the validation, it makes this mechanism little less scary (a little)
                 updateList.add(indexMetadata);
                 // try and parse it (no need to add it here) so we can bail early in case of parsing exception
-                DocumentMapper existingMapper = mapperService.documentMapper();
-                DocumentMapper newMapper = mapperService.parse(MapperService.SINGLE_MAPPING_NAME, mappingUpdateSource);
-                if (existingMapper != null) {
-                    // first, simulate: just call merge and ignore the result
-                    existingMapper.merge(newMapper.mapping(), MergeReason.MAPPING_UPDATE);
-                }
+                // first, simulate: just call merge and ignore the result
+                Mapping mapping = mapperService.parseMapping(MapperService.SINGLE_MAPPING_NAME, mappingUpdateSource);
+                MapperService.mergeMappings(mapperService.documentMapper(), mapping, MergeReason.MAPPING_UPDATE);
             }
             Metadata.Builder builder = Metadata.builder(metadata);
             boolean updated = false;

server/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java

@@ -17,7 +17,6 @@
 import org.elasticsearch.common.xcontent.XContentType;
 import org.elasticsearch.index.IndexSettings;
 import org.elasticsearch.index.analysis.IndexAnalyzers;
-import org.elasticsearch.index.mapper.MapperService.MergeReason;
 
 import java.util.Arrays;
 import java.util.Collection;
@@ -132,11 +131,6 @@ public ParsedDocument createNoopTombstoneDoc(String index, String reason) throws
         return parsedDoc;
     }
 
-    public DocumentMapper merge(Mapping mapping, MergeReason reason) {
-        Mapping merged = this.mapping().merge(mapping, reason);
-        return new DocumentMapper(mappingLookup.getIndexSettings(), mappingLookup.getIndexAnalyzers(), documentParser, merged);
-    }
-
     public void validate(IndexSettings settings, boolean checkLimits) {
         this.mapping().validate(this.mappingLookup);
         if (settings.getIndexMetadata().isRoutingPartitionedIndex()) {

server/src/main/java/org/elasticsearch/index/mapper/MapperService.java

@@ -182,7 +182,7 @@ public void updateMapping(final IndexMetadata currentIndexMetadata, final IndexM
         if (newMappingMetadata != null) {
             String type = newMappingMetadata.type();
             CompressedXContent incomingMappingSource = newMappingMetadata.source();
-            Mapping incomingMapping = parseMappings(type, incomingMappingSource);
+            Mapping incomingMapping = parseMapping(type, incomingMappingSource);
             DocumentMapper previousMapper;
             synchronized (this) {
                 previousMapper = this.mapper;
@@ -277,7 +277,7 @@ public DocumentMapper merge(String type, CompressedXContent mappingSource, Merge
     }
 
     private synchronized DocumentMapper mergeAndApplyMappings(String mappingType, CompressedXContent mappingSource, MergeReason reason) {
-        Mapping incomingMapping = parseMappings(mappingType, mappingSource);
+        Mapping incomingMapping = parseMapping(mappingType, mappingSource);
         Mapping mapping = mergeMappings(this.mapper, incomingMapping, reason);
         DocumentMapper newMapper = newDocumentMapper(mapping, reason);
         if (reason == MergeReason.MAPPING_UPDATE_PREFLIGHT) {
@@ -295,15 +295,15 @@ private DocumentMapper newDocumentMapper(Mapping mapping, MergeReason reason) {
         return newMapper;
     }
 
-    private Mapping parseMappings(String mappingType, CompressedXContent mappingSource) {
+    public Mapping parseMapping(String mappingType, CompressedXContent mappingSource) {
         try {
             return mappingParser.parse(mappingType, mappingSource);
         } catch (Exception e) {
             throw new MapperParsingException("Failed to parse mapping: {}", e, e.getMessage());
         }
     }
 
-    private static Mapping mergeMappings(DocumentMapper currentMapper, Mapping incomingMapping, MergeReason reason) {
+    public static Mapping mergeMappings(DocumentMapper currentMapper, Mapping incomingMapping, MergeReason reason) {
         Mapping newMapping;
         if (currentMapper == null) {
             newMapping = incomingMapping;

server/src/test/java/org/elasticsearch/index/mapper/DocumentMapperTests.java

@@ -12,7 +12,9 @@
 import org.apache.lucene.analysis.core.KeywordAnalyzer;
 import org.apache.lucene.analysis.core.WhitespaceAnalyzer;
 import org.apache.lucene.analysis.standard.StandardAnalyzer;
+import org.elasticsearch.Version;
 import org.elasticsearch.common.compress.CompressedXContent;
+import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.index.IndexSettings;
 import org.elasticsearch.index.analysis.AnalyzerScope;
 import org.elasticsearch.index.analysis.IndexAnalyzers;
@@ -49,15 +51,17 @@ public void testAddFields() throws Exception {
         }));
 
         MergeReason reason = randomFrom(MergeReason.MAPPING_UPDATE, MergeReason.INDEX_TEMPLATE);
-        DocumentMapper merged = stage1.merge(stage2.mapping(), reason);
-
+        Mapping merged = MapperService.mergeMappings(stage1, stage2.mapping(), reason);
         // stage1 mapping should not have been modified
         assertThat(stage1.mappers().getMapper("age"), nullValue());
         assertThat(stage1.mappers().getMapper("obj1.prop1"), nullValue());
         // but merged should
-        assertThat(merged.mappers().getMapper("age"), notNullValue());
-        assertThat(merged.mappers().getMapper("obj1.prop1"), notNullValue());
-}
+        IndexSettings indexSettings = createIndexSettings(Version.CURRENT, Settings.EMPTY);
+        IndexAnalyzers indexAnalyzers = createIndexAnalyzers(indexSettings);
+        DocumentMapper mergedMapper = new DocumentMapper(indexSettings, indexAnalyzers, null, merged);
+        assertThat(mergedMapper.mappers().getMapper("age"), notNullValue());
+        assertThat(mergedMapper.mappers().getMapper("obj1.prop1"), notNullValue());
+    }
 
     public void testMergeObjectDynamic() throws Exception {
         DocumentMapper mapper = createDocumentMapper(mapping(b -> { }));
@@ -66,7 +70,7 @@ public void testMergeObjectDynamic() throws Exception {
         DocumentMapper withDynamicMapper = createDocumentMapper(topMapping(b -> b.field("dynamic", "false")));
         assertThat(withDynamicMapper.root().dynamic(), equalTo(ObjectMapper.Dynamic.FALSE));
 
-        DocumentMapper merged = mapper.merge(withDynamicMapper.mapping(), MergeReason.MAPPING_UPDATE);
+        Mapping merged = MapperService.mergeMappings(mapper, withDynamicMapper.mapping(), MergeReason.MAPPING_UPDATE);
         assertThat(merged.root().dynamic(), equalTo(ObjectMapper.Dynamic.FALSE));
     }
 
@@ -77,12 +81,12 @@ public void testMergeObjectAndNested() throws Exception {
 
         {
             IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
-                () -> objectMapper.merge(nestedMapper.mapping(), reason));
+                () -> MapperService.mergeMappings(objectMapper, nestedMapper.mapping(), reason));
             assertThat(e.getMessage(), containsString("cannot change object mapping from non-nested to nested"));
         }
         {
             IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
-                () -> nestedMapper.merge(objectMapper.mapping(), reason));
+                () -> MapperService.mergeMappings(nestedMapper, objectMapper.mapping(), reason));
             assertThat(e.getMessage(), containsString("cannot change object mapping from nested to non-nested"));
         }
     }
@@ -220,13 +224,13 @@ public void testMergeMeta() throws IOException {
 
         DocumentMapper updatedMapper = createDocumentMapper(fieldMapping(b -> b.field("type", "text")));
 
-        DocumentMapper mergedMapper = initMapper.merge(updatedMapper.mapping(), MergeReason.MAPPING_UPDATE);
-        assertThat(mergedMapper.meta().get("foo"), equalTo("bar"));
+        Mapping merged = MapperService.mergeMappings(initMapper, updatedMapper.mapping(), MergeReason.MAPPING_UPDATE);
+        assertThat(merged.meta.get("foo"), equalTo("bar"));
 
         updatedMapper
             = createDocumentMapper(topMapping(b -> b.startObject("_meta").field("foo", "new_bar").endObject()));
-        mergedMapper = initMapper.merge(updatedMapper.mapping(), MergeReason.MAPPING_UPDATE);
-        assertThat(mergedMapper.meta().get("foo"), equalTo("new_bar"));
+        merged = MapperService.mergeMappings(initMapper, updatedMapper.mapping(), MergeReason.MAPPING_UPDATE);
+        assertThat(merged.meta.get("foo"), equalTo("new_bar"));
     }
 
     public void testMergeMetaForIndexTemplate() throws IOException {
@@ -249,8 +253,8 @@ public void testMergeMetaForIndexTemplate() throws IOException {
         assertThat(initMapper.meta(), equalTo(expected));
 
         DocumentMapper updatedMapper = createDocumentMapper(fieldMapping(b -> b.field("type", "text")));
-        DocumentMapper mergedMapper = initMapper.merge(updatedMapper.mapping(), MergeReason.INDEX_TEMPLATE);
-        assertThat(mergedMapper.meta(), equalTo(expected));
+        Mapping merged = MapperService.mergeMappings(initMapper, updatedMapper.mapping(), MergeReason.INDEX_TEMPLATE);
+        assertThat(merged.meta, equalTo(expected));
 
         updatedMapper = createDocumentMapper(topMapping(b -> {
             b.startObject("_meta");
@@ -265,10 +269,10 @@ public void testMergeMetaForIndexTemplate() throws IOException {
             }
             b.endObject();
         }));
-        mergedMapper = mergedMapper.merge(updatedMapper.mapping(), MergeReason.INDEX_TEMPLATE);
+        merged = merged.merge(updatedMapper.mapping(), MergeReason.INDEX_TEMPLATE);
 
         expected = Map.of("field", "value",
             "object", Map.of("field1", "value1", "field2", "new_value", "field3", "value3"));
-        assertThat(mergedMapper.meta(), equalTo(expected));
+        assertThat(merged.meta, equalTo(expected));
     }
 }