Make sure to validate the type before attempting to merge a new mapping. (#45157)

Currently, when adding a new mapping, we attempt to parse + merge it before
checking whether its top-level document type matches the existing type. So when
a user attempts to introduce a new mapping type, we may give a confusing error
message around merging instead of complaining that it's not possible to add
more than one type ("Rejecting mapping update to [my-index] as the final
mapping would have more than 1 type...").

This PR moves the type validation to the start of
`MetaDataMappingService#applyRequest` so that we make sure the type matches
before performing any mapper merging.

We already partially addressed this issue in #29316, but the tests there
focused on `MapperService` and did not catch this problem with end-to-end
mapping updates.

Addresses #43012.

Author: jtibshirani

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

@@ -265,6 +265,13 @@ private ClusterState applyRequest(ClusterState currentState, PutMappingClusterSt
                 // try and parse it (no need to add it here) so we can bail early in case of parsing exception
                 DocumentMapper newMapper;
                 DocumentMapper existingMapper = mapperService.documentMapper();
+
+                String typeForUpdate = mapperService.getTypeForUpdate(mappingType, mappingUpdateSource);
+                if (existingMapper != null && existingMapper.type().equals(typeForUpdate) == false) {
+                    throw new IllegalArgumentException("Rejecting mapping update to [" + mapperService.index().getName() +
+                        "] as the final mapping would have more than 1 type: " + Arrays.asList(existingMapper.type(), typeForUpdate));
+                }
+
                 if (MapperService.DEFAULT_MAPPING.equals(request.type())) {
                     // _default_ types do not go through merging, but we do test the new settings. Also don't apply the old default
                     newMapper = mapperService.parse(request.type(), mappingUpdateSource, false);
@@ -299,14 +306,7 @@ private ClusterState applyRequest(ClusterState currentState, PutMappingClusterSt
                 final Index index = indexMetaData.getIndex();
                 final MapperService mapperService = indexMapperServices.get(index);
 
-                // If the _type name is _doc and there is no _doc top-level key then this means that we
-                // are handling a typeless call. In such a case, we override _doc with the actual type
-                // name in the mappings. This allows to use typeless APIs on typed indices.
-                String typeForUpdate = mappingType; // the type to use to apply the mapping update
-                if (isMappingSourceTyped(request.type(), mappingUpdateSource) == false) {
-                    typeForUpdate = mapperService.resolveDocumentType(mappingType);
-                }
-
+                String typeForUpdate = mapperService.getTypeForUpdate(mappingType, mappingUpdateSource);
                 CompressedXContent existingSource = null;
                 DocumentMapper existingMapper = mapperService.documentMapper(typeForUpdate);
                 if (existingMapper != null) {

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

@@ -21,7 +21,6 @@
 
 import com.carrotsearch.hppc.ObjectHashSet;
 import com.carrotsearch.hppc.cursors.ObjectCursor;
-
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.message.ParameterizedMessage;
 import org.apache.lucene.analysis.Analyzer;
@@ -72,7 +71,6 @@
 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;
@@ -452,14 +450,6 @@ private synchronized Map<String, DocumentMapper> internalMerge(@Nullable Documen
             results.put(DEFAULT_MAPPING, defaultMapper);
         }
 
-        {
-            if (mapper != null && this.mapper != null && Objects.equals(this.mapper.type(), mapper.type()) == false) {
-                throw new IllegalArgumentException(
-                        "Rejecting mapping update to [" + index().getName() + "] as the final mapping would have more than 1 type: "
-                            + Arrays.asList(this.mapper.type(), mapper.type()));
-            }
-        }
-
         DocumentMapper newMapper = null;
         if (mapper != null) {
             // check naming
@@ -707,6 +697,15 @@ public static boolean isMappingSourceTyped(String type, CompressedXContent mappi
         return isMappingSourceTyped(type, root);
     }
 
+    /**
+     * If the _type name is _doc and there is no _doc top-level key then this means that we
+     * are handling a typeless call. In such a case, we override _doc with the actual type
+     * name in the mappings. This allows to use typeless APIs on typed indices.
+     */
+    public String getTypeForUpdate(String type, CompressedXContent mappingSource) {
+        return isMappingSourceTyped(type, mappingSource) == false ? resolveDocumentType(type) : type;
+    }
+
     /**
      * Resolves a type from a mapping-related request into the type that should be used when
      * merging and updating mappings.

server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataMappingServiceTests.java

@@ -19,11 +19,15 @@
 
 package org.elasticsearch.cluster.metadata;
 
+import org.elasticsearch.action.admin.indices.create.CreateIndexRequestBuilder;
 import org.elasticsearch.action.admin.indices.mapping.put.PutMappingClusterStateUpdateRequest;
 import org.elasticsearch.cluster.ClusterState;
 import org.elasticsearch.cluster.ClusterStateTaskExecutor;
 import org.elasticsearch.cluster.service.ClusterService;
+import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.compress.CompressedXContent;
+import org.elasticsearch.common.xcontent.XContentBuilder;
+import org.elasticsearch.common.xcontent.XContentFactory;
 import org.elasticsearch.index.Index;
 import org.elasticsearch.index.IndexService;
 import org.elasticsearch.index.mapper.MapperService;
@@ -34,6 +38,7 @@
 import java.util.Collection;
 import java.util.Collections;
 
+import static org.hamcrest.CoreMatchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.not;
 
@@ -134,4 +139,77 @@ public void testMappingUpdateAccepts_docAsType() throws Exception {
                 Collections.singletonMap("foo",
                         Collections.singletonMap("type", "keyword"))), mappingMetaData.sourceAsMap());
     }
+
+    public void testForbidMultipleTypes() throws Exception {
+        CreateIndexRequestBuilder createIndexRequest = client().admin().indices()
+            .prepareCreate("test")
+            .addMapping(MapperService.SINGLE_MAPPING_NAME);
+        IndexService indexService = createIndex("test", createIndexRequest);
+
+        MetaDataMappingService mappingService = getInstanceFromNode(MetaDataMappingService.class);
+        ClusterService clusterService = getInstanceFromNode(ClusterService.class);
+
+        PutMappingClusterStateUpdateRequest request = new PutMappingClusterStateUpdateRequest()
+            .type("other_type")
+            .indices(new Index[] {indexService.index()})
+            .source(Strings.toString(XContentFactory.jsonBuilder()
+                .startObject()
+                    .startObject("other_type").endObject()
+                .endObject()));
+        ClusterStateTaskExecutor.ClusterTasksResult<PutMappingClusterStateUpdateRequest> result =
+            mappingService.putMappingExecutor.execute(clusterService.state(), Collections.singletonList(request));
+        assertThat(result.executionResults.size(), equalTo(1));
+
+        ClusterStateTaskExecutor.TaskResult taskResult = result.executionResults.values().iterator().next();
+        assertFalse(taskResult.isSuccess());
+        assertThat(taskResult.getFailure().getMessage(), containsString(
+            "Rejecting mapping update to [test] as the final mapping would have more than 1 type: "));
+    }
+
+    /**
+     * This test checks that the multi-type validation is done before we do any other kind of validation
+     * on the mapping that's added, see https://github.com/elastic/elasticsearch/issues/29313
+     */
+    public void testForbidMultipleTypesWithConflictingMappings() throws Exception {
+        XContentBuilder mapping = XContentFactory.jsonBuilder().startObject()
+            .startObject(MapperService.SINGLE_MAPPING_NAME)
+                .startObject("properties")
+                    .startObject("field1")
+                        .field("type", "text")
+                    .endObject()
+                .endObject()
+            .endObject()
+        .endObject();
+
+        CreateIndexRequestBuilder createIndexRequest = client().admin().indices()
+            .prepareCreate("test")
+            .addMapping(MapperService.SINGLE_MAPPING_NAME, mapping);
+        IndexService indexService = createIndex("test", createIndexRequest);
+
+        MetaDataMappingService mappingService = getInstanceFromNode(MetaDataMappingService.class);
+        ClusterService clusterService = getInstanceFromNode(ClusterService.class);
+
+        String conflictingMapping = Strings.toString(XContentFactory.jsonBuilder().startObject()
+            .startObject("other_type")
+                .startObject("properties")
+                    .startObject("field1")
+                        .field("type", "keyword")
+                    .endObject()
+                .endObject()
+            .endObject()
+        .endObject());
+
+        PutMappingClusterStateUpdateRequest request = new PutMappingClusterStateUpdateRequest()
+            .type("other_type")
+            .indices(new Index[] {indexService.index()})
+            .source(conflictingMapping);
+        ClusterStateTaskExecutor.ClusterTasksResult<PutMappingClusterStateUpdateRequest> result =
+            mappingService.putMappingExecutor.execute(clusterService.state(), Collections.singletonList(request));
+        assertThat(result.executionResults.size(), equalTo(1));
+
+        ClusterStateTaskExecutor.TaskResult taskResult = result.executionResults.values().iterator().next();
+        assertFalse(taskResult.isSuccess());
+        assertThat(taskResult.getFailure().getMessage(), containsString(
+            "Rejecting mapping update to [test] as the final mapping would have more than 1 type: "));
+    }
 }

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

@@ -57,7 +57,6 @@
 
 import static org.hamcrest.CoreMatchers.containsString;
 import static org.hamcrest.Matchers.instanceOf;
-import static org.hamcrest.Matchers.startsWith;
 
 public class MapperServiceTests extends ESSingleNodeTestCase {
 
@@ -298,36 +297,6 @@ public void testTotalFieldsLimitWithFieldAlias() throws Throwable {
         assertEquals("Limit of total fields [" + numberOfNonAliasFields + "] in index [test2] has been exceeded", e.getMessage());
     }
 
-    public void testForbidMultipleTypes() throws IOException {
-        String mapping = Strings.toString(XContentFactory.jsonBuilder().startObject().startObject("type").endObject().endObject());
-        MapperService mapperService = createIndex("test").mapperService();
-        mapperService.merge("type", new CompressedXContent(mapping), MergeReason.MAPPING_UPDATE);
-
-        String mapping2 = Strings.toString(XContentFactory.jsonBuilder().startObject().startObject("type2").endObject().endObject());
-        IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
-                () -> mapperService.merge("type2", new CompressedXContent(mapping2), MergeReason.MAPPING_UPDATE));
-        assertThat(e.getMessage(), startsWith("Rejecting mapping update to [test] as the final mapping would have more than 1 type: "));
-    }
-
-    /**
-     * This test checks that the multi-type validation is done before we do any other kind of validation on the mapping that's added,
-     * see https://github.com/elastic/elasticsearch/issues/29313
-     */
-    public void testForbidMultipleTypesWithConflictingMappings() throws IOException {
-        String mapping = Strings.toString(XContentFactory.jsonBuilder().startObject().startObject("type")
-            .startObject("properties").startObject("field1").field("type", "integer_range")
-            .endObject().endObject().endObject().endObject());
-        MapperService mapperService = createIndex("test").mapperService();
-        mapperService.merge("type", new CompressedXContent(mapping), MergeReason.MAPPING_UPDATE);
-
-        String mapping2 = Strings.toString(XContentFactory.jsonBuilder().startObject().startObject("type2")
-            .startObject("properties").startObject("field1").field("type", "integer")
-            .endObject().endObject().endObject().endObject());
-        IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
-            () -> mapperService.merge("type2", new CompressedXContent(mapping2), MergeReason.MAPPING_UPDATE));
-        assertThat(e.getMessage(), startsWith("Rejecting mapping update to [test] as the final mapping would have more than 1 type: "));
-    }
-
     public void testDefaultMappingIsRejectedOn7() throws IOException {
         String mapping = Strings.toString(XContentFactory.jsonBuilder().startObject().startObject("_default_").endObject().endObject());
         MapperService mapperService = createIndex("test").mapperService();