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

Author: ywelsch

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

@@ -399,6 +399,16 @@ private synchronized Map<String, DocumentMapper> internalMerge(@Nullable Documen
             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());
@@ -496,15 +506,6 @@ private synchronized Map<String, DocumentMapper> internalMerge(@Nullable Documen
             }
         }
 
-        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

@@ -21,6 +21,7 @@
 
 import com.carrotsearch.hppc.cursors.ObjectCursor;
 import org.elasticsearch.ElasticsearchException;
+import org.elasticsearch.Version;
 import org.elasticsearch.action.ActionListener;
 import org.elasticsearch.action.UnavailableShardsException;
 import org.elasticsearch.action.admin.cluster.state.ClusterStateResponse;
@@ -38,11 +39,16 @@
 import org.elasticsearch.env.NodeEnvironment;
 import org.elasticsearch.index.IndexNotFoundException;
 import org.elasticsearch.index.query.RangeQueryBuilder;
+import org.elasticsearch.plugins.Plugin;
 import org.elasticsearch.test.ESIntegTestCase;
 import org.elasticsearch.test.ESIntegTestCase.ClusterScope;
 import org.elasticsearch.test.ESIntegTestCase.Scope;
+import org.elasticsearch.test.InternalSettingsPlugin;
 import org.elasticsearch.test.InternalTestCluster;
+import org.elasticsearch.test.VersionUtils;
 
+import java.util.Arrays;
+import java.util.Collection;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Set;
@@ -66,6 +72,11 @@
 @ClusterScope(scope = Scope.TEST)
 public class CreateIndexIT extends ESIntegTestCase {
 
+    @Override
+    protected Collection<Class<? extends Plugin>> nodePlugins() {
+        return Arrays.asList(InternalSettingsPlugin.class); // uses index.version.created
+    }
+
     public void testCreationDateGivenFails() {
         try {
             prepareCreate("test").setSettings(Settings.builder().put(IndexMetaData.SETTING_CREATION_DATE, 4L)).get();
@@ -268,7 +279,9 @@ public void onFailure(Exception e) {
      * Asserts that the root cause of mapping conflicts is readable.
      */
     public void testMappingConflictRootCause() throws Exception {
-        CreateIndexRequestBuilder b = prepareCreate("test");
+        CreateIndexRequestBuilder b = prepareCreate("test")
+            .setSettings(Settings.builder().put(IndexMetaData.SETTING_VERSION_CREATED,
+                VersionUtils.randomVersionBetween(random(), Version.V_5_0_0, Version.V_5_6_9)));
         b.addMapping("type1", jsonBuilder().startObject().startObject("properties")
                 .startObject("text")
                     .field("type", "text")

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

@@ -182,7 +182,7 @@ public void testMappingDepthExceedsLimit() throws Throwable {
         indexService2.mapperService().merge("type", objectMapping, MergeReason.MAPPING_UPDATE, false);
 
         IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
-                () -> indexService1.mapperService().merge("type2", objectMapping, MergeReason.MAPPING_UPDATE, false));
+                () -> indexService1.mapperService().merge("type", objectMapping, MergeReason.MAPPING_UPDATE, false));
         assertThat(e.getMessage(), containsString("Limit of mapping depth [1] in index [test1] has been exceeded"));
     }
 
@@ -290,7 +290,6 @@ public void testPartitionedConstraints() {
         // 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()
@@ -342,6 +341,23 @@ public void testForbidMultipleTypes() throws IOException {
         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, false);
+
+        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, false));
+        assertThat(e.getMessage(), Matchers.startsWith("Rejecting mapping update to [test] as the final mapping would have more than 1 type: "));
+    }
+
     public void testDefaultMappingIsDeprecated() 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

@@ -120,7 +120,8 @@ public void testConflictNewType() throws Exception {
         XContentBuilder mapping = XContentFactory.jsonBuilder().startObject().startObject("type1")
                 .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().put("index.version.created",
+            Version.V_5_6_0).build(), "type1", mapping).mapperService();
 
         XContentBuilder update = XContentFactory.jsonBuilder().startObject().startObject("type2")
                 .startObject("properties").startObject("foo").field("type", "double").endObject()
@@ -134,17 +135,8 @@ public void testConflictNewType() throws Exception {
             assertTrue(e.getMessage(), e.getMessage().contains("mapper [foo] cannot be changed from type [long] to [double]"));
         }
 
-        try {
-            mapperService.merge("type2", new CompressedXContent(Strings.toString(update)), MapperService.MergeReason.MAPPING_UPDATE, false);
-            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
@@ -224,18 +216,53 @@ public void testRejectFieldDefinedTwice() throws IOException {
                     .endObject()
                 .endObject().endObject());
 
-        MapperService mapperService1 = createIndex("test1").mapperService();
+        MapperService mapperService1 = createIndex("test1", Settings.builder().put("index.version.created",
+            Version.V_5_6_0).build()).mapperService();
+
         mapperService1.merge("type1", new CompressedXContent(mapping1), MergeReason.MAPPING_UPDATE, false);
         IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
-                () -> mapperService1.merge("type2", new CompressedXContent(mapping2), MergeReason.MAPPING_UPDATE, false));
+            () -> mapperService1.merge("type2", new CompressedXContent(mapping2), MergeReason.MAPPING_UPDATE, false));
         assertThat(e.getMessage(), equalTo("[foo] is defined as a field in mapping [type2"
-                + "] but this name is already used for an object in other types"));
+            + "] but this name is already used for an object in other types"));
 
-        MapperService mapperService2 = createIndex("test2").mapperService();
+        MapperService mapperService2 = createIndex("test2", Settings.builder().put("index.version.created",
+            Version.V_5_6_0).build()).mapperService();
         mapperService2.merge("type2", new CompressedXContent(mapping2), MergeReason.MAPPING_UPDATE, false);
         e = expectThrows(IllegalArgumentException.class,
-                () -> mapperService2.merge("type1", new CompressedXContent(mapping1), MergeReason.MAPPING_UPDATE, false));
+            () -> mapperService2.merge("type1", new CompressedXContent(mapping1), MergeReason.MAPPING_UPDATE, false));
         assertThat(e.getMessage(), equalTo("[foo] is defined as an object in mapping [type1"
-                + "] but this name is already used for a field in other types"));
+            + "] but this name is already used for a field in other types"));
+    }
+
+    public void testRejectFieldDefinedTwiceInSameType() throws IOException {
+        String mapping1 = Strings.toString(XContentFactory.jsonBuilder().startObject()
+            .startObject("type")
+            .startObject("properties")
+            .startObject("foo")
+            .field("type", "object")
+            .endObject()
+            .endObject()
+            .endObject().endObject());
+        String mapping2 = Strings.toString(XContentFactory.jsonBuilder().startObject()
+            .startObject("type")
+            .startObject("properties")
+            .startObject("foo")
+            .field("type", "long")
+            .endObject()
+            .endObject()
+            .endObject().endObject());
+
+        MapperService mapperService1 = createIndex("test1").mapperService();
+
+        mapperService1.merge("type", new CompressedXContent(mapping1), MergeReason.MAPPING_UPDATE, false);
+        IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
+            () -> mapperService1.merge("type", new CompressedXContent(mapping2), MergeReason.MAPPING_UPDATE, false));
+        assertThat(e.getMessage(), equalTo("Can't merge a non object mapping [foo] with an object mapping [foo]"));
+
+        MapperService mapperService2 = createIndex("test2").mapperService();
+        mapperService2.merge("type", new CompressedXContent(mapping2), MergeReason.MAPPING_UPDATE, false);
+        e = expectThrows(IllegalArgumentException.class,
+            () -> mapperService2.merge("type", new CompressedXContent(mapping1), MergeReason.MAPPING_UPDATE, false));
+        assertThat(e.getMessage(), equalTo("mapper [foo] of different type, current_type [long], merged_type [ObjectMapper]"));
     }
 }