Check presence of multi-types before validating new mapping (#29316)

Before doing any kind of validation on a new mapping, we should first do the multi-type validation in
order to provide better error messages. For #29313, this means that the exception message will be
Rejecting mapping update to [range_index_new] as the final mapping would have more than 1 type:
[_doc, mytype]
instead of
[expected_attendees] is defined as an object in mapping [mytype] but this name is already used for
a field in other types

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

@@ -385,6 +385,16 @@ public class MapperService extends AbstractIndexComponent implements Closeable {
             results.put(DEFAULT_MAPPING, defaultMapper);
         }
 
+        if (indexSettings.isSingleType()) {
+            Set<String> actualTypes = new HashSet<>(mappers.keySet());
+            documentMappers.forEach(mapper -> actualTypes.add(mapper.type()));
+            actualTypes.remove(DEFAULT_MAPPING);
+            if (actualTypes.size() > 1) {
+                throw new IllegalArgumentException(
+                    "Rejecting mapping update to [" + index().getName() + "] as the final mapping would have more than 1 type: " + actualTypes);
+            }
+        }
+
         for (DocumentMapper mapper : documentMappers) {
             // check naming
             validateTypeName(mapper.type());
@@ -478,15 +488,6 @@ public class MapperService extends AbstractIndexComponent implements Closeable {
             }
         }
 
-        if (indexSettings.isSingleType()) {
-            Set<String> actualTypes = new HashSet<>(mappers.keySet());
-            actualTypes.remove(DEFAULT_MAPPING);
-            if (actualTypes.size() > 1) {
-                throw new IllegalArgumentException(
-                        "Rejecting mapping update to [" + index().getName() + "] as the final mapping would have more than 1 type: " + actualTypes);
-            }
-        }
-
         // make structures immutable
         mappers = Collections.unmodifiableMap(mappers);
         results = Collections.unmodifiableMap(results);

server/src/test/java/org/elasticsearch/action/admin/indices/create/CreateIndexIT.java

@@ -264,25 +264,6 @@ public class CreateIndexIT extends ESIntegTestCase {
         logger.info("total: {}", expected.getHits().getTotalHits());
     }
 
-    /**
-     * Asserts that the root cause of mapping conflicts is readable.
-     */
-    public void testMappingConflictRootCause() throws Exception {
-        CreateIndexRequestBuilder b = prepareCreate("test");
-        b.addMapping("type1", jsonBuilder().startObject().startObject("properties")
-                .startObject("text")
-                    .field("type", "text")
-                    .field("analyzer", "standard")
-                    .field("search_analyzer", "whitespace")
-                .endObject().endObject().endObject());
-        b.addMapping("type2", jsonBuilder().humanReadable(true).startObject().startObject("properties")
-                .startObject("text")
-                    .field("type", "text")
-                .endObject().endObject().endObject());
-        IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> b.get());
-        assertThat(e.getMessage(), containsString("Mapper for [text] conflicts with existing mapping:"));
-    }
-
     public void testRestartIndexCreationAfterFullClusterRestart() throws Exception {
         client().admin().cluster().prepareUpdateSettings().setTransientSettings(Settings.builder().put("cluster.routing.allocation.enable",
             "none")).get();

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

@@ -164,7 +164,7 @@ public class MapperServiceTests extends ESSingleNodeTestCase {
         indexService2.mapperService().merge("type", objectMapping, MergeReason.MAPPING_UPDATE);
 
         IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
-                () -> indexService1.mapperService().merge("type2", objectMapping, MergeReason.MAPPING_UPDATE));
+                () -> indexService1.mapperService().merge("type", objectMapping, MergeReason.MAPPING_UPDATE));
         assertThat(e.getMessage(), containsString("Limit of mapping depth [1] in index [test1] has been exceeded"));
     }
 
@@ -255,7 +255,6 @@ public class MapperServiceTests extends ESSingleNodeTestCase {
         // partitioned index cannot have parent/child relationships
         IllegalArgumentException parentException = expectThrows(IllegalArgumentException.class, () -> {
             client().admin().indices().prepareCreate("test-index")
-                    .addMapping("parent", "{\"parent\":{\"_routing\":{\"required\":true}}}", XContentType.JSON)
                     .addMapping("child", "{\"child\": {\"_routing\":{\"required\":true}, \"_parent\": {\"type\": \"parent\"}}}",
                         XContentType.JSON)
                     .setSettings(Settings.builder()
@@ -307,6 +306,23 @@ public class MapperServiceTests extends ESSingleNodeTestCase {
         assertThat(e.getMessage(), Matchers.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(), Matchers.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();

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

@@ -117,34 +117,25 @@ public class UpdateMappingTests extends ESSingleNodeTestCase {
     }
 
     public void testConflictNewType() throws Exception {
-        XContentBuilder mapping = XContentFactory.jsonBuilder().startObject().startObject("type1")
+        XContentBuilder mapping = XContentFactory.jsonBuilder().startObject().startObject("type")
                 .startObject("properties").startObject("foo").field("type", "long").endObject()
                 .endObject().endObject().endObject();
-        MapperService mapperService = createIndex("test", Settings.builder().build(), "type1", mapping).mapperService();
+        MapperService mapperService = createIndex("test", Settings.builder().build(), "type", mapping).mapperService();
 
-        XContentBuilder update = XContentFactory.jsonBuilder().startObject().startObject("type2")
+        XContentBuilder update = XContentFactory.jsonBuilder().startObject().startObject("type")
                 .startObject("properties").startObject("foo").field("type", "double").endObject()
                 .endObject().endObject().endObject();
 
         try {
-            mapperService.merge("type2", new CompressedXContent(Strings.toString(update)), MapperService.MergeReason.MAPPING_UPDATE);
+            mapperService.merge("type", new CompressedXContent(Strings.toString(update)), MapperService.MergeReason.MAPPING_UPDATE);
             fail();
         } catch (IllegalArgumentException e) {
             // expected
             assertTrue(e.getMessage(), e.getMessage().contains("mapper [foo] cannot be changed from type [long] to [double]"));
         }
 
-        try {
+        assertThat(((FieldMapper) mapperService.documentMapper("type").mapping().root().getMapper("foo")).fieldType().typeName(),
-            mapperService.merge("type2", new CompressedXContent(Strings.toString(update)), MapperService.MergeReason.MAPPING_UPDATE);
-            fail();
-        } catch (IllegalArgumentException e) {
-            // expected
-            assertTrue(e.getMessage(), e.getMessage().contains("mapper [foo] cannot be changed from type [long] to [double]"));
-        }
-
-        assertThat(((FieldMapper) mapperService.documentMapper("type1").mapping().root().getMapper("foo")).fieldType().typeName(),
                 equalTo("long"));
-        assertNull(mapperService.documentMapper("type2"));
     }
 
     // same as the testConflictNewType except that the mapping update is on an existing type
@@ -208,7 +199,7 @@ public class UpdateMappingTests extends ESSingleNodeTestCase {
 
     public void testRejectFieldDefinedTwice() throws IOException {
         String mapping1 = Strings.toString(XContentFactory.jsonBuilder().startObject()
-                .startObject("type1")
+                .startObject("type")
                     .startObject("properties")
                         .startObject("foo")
                             .field("type", "object")
@@ -216,7 +207,7 @@ public class UpdateMappingTests extends ESSingleNodeTestCase {
                     .endObject()
                 .endObject().endObject());
         String mapping2 = Strings.toString(XContentFactory.jsonBuilder().startObject()
-                .startObject("type2")
+                .startObject("type")
                     .startObject("properties")
                         .startObject("foo")
                             .field("type", "long")
@@ -225,17 +216,15 @@ public class UpdateMappingTests extends ESSingleNodeTestCase {
                 .endObject().endObject());
 
         MapperService mapperService1 = createIndex("test1").mapperService();
-        mapperService1.merge("type1", new CompressedXContent(mapping1), MergeReason.MAPPING_UPDATE);
+        mapperService1.merge("type", new CompressedXContent(mapping1), MergeReason.MAPPING_UPDATE);
         IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
-                () -> mapperService1.merge("type2", new CompressedXContent(mapping2), MergeReason.MAPPING_UPDATE));
+                () -> mapperService1.merge("type", new CompressedXContent(mapping2), MergeReason.MAPPING_UPDATE));
-        assertThat(e.getMessage(), equalTo("[foo] is defined as a field in mapping [type2"
+        assertThat(e.getMessage(), equalTo("Can't merge a non object mapping [foo] with an object mapping [foo]"));
-                + "] but this name is already used for an object in other types"));
 
         MapperService mapperService2 = createIndex("test2").mapperService();
-        mapperService2.merge("type2", new CompressedXContent(mapping2), MergeReason.MAPPING_UPDATE);
+        mapperService2.merge("type", new CompressedXContent(mapping2), MergeReason.MAPPING_UPDATE);
         e = expectThrows(IllegalArgumentException.class,
-                () -> mapperService2.merge("type1", new CompressedXContent(mapping1), MergeReason.MAPPING_UPDATE));
+                () -> mapperService2.merge("type", new CompressedXContent(mapping1), MergeReason.MAPPING_UPDATE));
-        assertThat(e.getMessage(), equalTo("[foo] is defined as an object in mapping [type1"
+        assertThat(e.getMessage(), equalTo("mapper [foo] of different type, current_type [long], merged_type [ObjectMapper]"));
-                + "] but this name is already used for a field in other types"));
     }
 }