Small simplifications to mapping validation. (#39777)

These simplifications to `MapperMergeValidator` are possible now that there is
always a single mapping definition.

* Remove the type argument in `validateMapperStructure`.
* Remove unnecessary checks against existing mappers.

Author: jtibshirani

modules/parent-join/src/test/java/org/elasticsearch/join/mapper/ParentJoinFieldMapperTests.java

@@ -400,7 +400,7 @@ public void testMultipleJoinFields() throws Exception {
                 .endObject());
             IllegalArgumentException exc = expectThrows(IllegalArgumentException.class, () -> indexService.mapperService().merge("type",
                 new CompressedXContent(mapping), MapperService.MergeReason.MAPPING_UPDATE));
-            assertThat(exc.getMessage(), containsString("Field [_parent_join] is defined twice in [type]"));
+            assertThat(exc.getMessage(), containsString("Field [_parent_join] is defined twice."));
         }
 
         {
@@ -426,7 +426,7 @@ public void testMultipleJoinFields() throws Exception {
                 .endObject());
             IllegalArgumentException exc = expectThrows(IllegalArgumentException.class, () -> indexService.mapperService().merge("type",
                 new CompressedXContent(updateMapping), MapperService.MergeReason.MAPPING_UPDATE));
-            assertThat(exc.getMessage(), containsString("Field [_parent_join] is defined twice in [type]"));
+            assertThat(exc.getMessage(), containsString("Field [_parent_join] is defined twice."));
         }
     }
 

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

@@ -37,37 +37,18 @@ class MapperMergeValidator {
      * duplicate fields are present, and if the provided fields have already been
      * defined with a different data type.
      *
-     * @param type The mapping type, for use in error messages.
      * @param objectMappers The newly added object mappers.
      * @param fieldMappers The newly added field mappers.
      * @param fieldAliasMappers The newly added field alias mappers.
-     * @param fullPathObjectMappers All object mappers, indexed by their full path.
-     * @param fieldTypes All field and field alias mappers, collected into a lookup structure.
      */
-    public static void validateMapperStructure(String type,
-                                               Collection<ObjectMapper> objectMappers,
+    public static void validateMapperStructure(Collection<ObjectMapper> objectMappers,
                                                Collection<FieldMapper> fieldMappers,
-                                               Collection<FieldAliasMapper> fieldAliasMappers,
-                                               Map<String, ObjectMapper> fullPathObjectMappers,
-                                               FieldTypeLookup fieldTypes) {
-        checkFieldUniqueness(type, objectMappers, fieldMappers,
-            fieldAliasMappers, fullPathObjectMappers, fieldTypes);
-        checkObjectsCompatibility(objectMappers, fullPathObjectMappers);
-    }
-
-    private static void checkFieldUniqueness(String type,
-                                             Collection<ObjectMapper> objectMappers,
-                                             Collection<FieldMapper> fieldMappers,
-                                             Collection<FieldAliasMapper> fieldAliasMappers,
-                                             Map<String, ObjectMapper> fullPathObjectMappers,
-                                             FieldTypeLookup fieldTypes) {
-
-        // first check within mapping
+                                               Collection<FieldAliasMapper> fieldAliasMappers) {
         Set<String> objectFullNames = new HashSet<>();
         for (ObjectMapper objectMapper : objectMappers) {
             String fullPath = objectMapper.fullPath();
             if (objectFullNames.add(fullPath) == false) {
-                throw new IllegalArgumentException("Object mapper [" + fullPath + "] is defined twice in mapping for type [" + type + "]");
+                throw new IllegalArgumentException("Object mapper [" + fullPath + "] is defined twice.");
             }
         }
 
@@ -76,38 +57,11 @@ private static void checkFieldUniqueness(String type,
             .forEach(mapper -> {
                 String name = mapper.name();
                 if (objectFullNames.contains(name)) {
-                    throw new IllegalArgumentException("Field [" + name + "] is defined both as an object and a field in [" + type + "]");
+                    throw new IllegalArgumentException("Field [" + name + "] is defined both as an object and a field.");
                 } else if (fieldNames.add(name) == false) {
-                    throw new IllegalArgumentException("Field [" + name + "] is defined twice in [" + type + "]");
+                    throw new IllegalArgumentException("Field [" + name + "] is defined twice.");
                 }
             });
-
-        // then check other types
-        for (String fieldName : fieldNames) {
-            if (fullPathObjectMappers.containsKey(fieldName)) {
-                throw new IllegalArgumentException("[" + fieldName + "] is defined as a field in mapping [" + type
-                    + "] but this name is already used for an object in other types");
-            }
-        }
-
-        for (String objectPath : objectFullNames) {
-            if (fieldTypes.get(objectPath) != null) {
-                throw new IllegalArgumentException("[" + objectPath + "] is defined as an object in mapping [" + type
-                    + "] but this name is already used for a field in other types");
-            }
-        }
-    }
-
-    private static void checkObjectsCompatibility(Collection<ObjectMapper> objectMappers,
-                                                  Map<String, ObjectMapper> fullPathObjectMappers) {
-        for (ObjectMapper newObjectMapper : objectMappers) {
-            ObjectMapper existingObjectMapper = fullPathObjectMappers.get(newObjectMapper.fullPath());
-            if (existingObjectMapper != null) {
-                // simulate a merge and ignore the result, we are just interested
-                // in exceptions here
-                existingObjectMapper.merge(newObjectMapper);
-            }
-        }
     }
 
     /**

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

@@ -471,8 +471,7 @@ private synchronized Map<String, DocumentMapper> internalMerge(@Nullable Documen
             Collections.addAll(fieldMappers, metadataMappers);
             MapperUtils.collect(newMapper.mapping().root(), objectMappers, fieldMappers, fieldAliasMappers);
 
-            MapperMergeValidator.validateMapperStructure(newMapper.type(), objectMappers, fieldMappers,
-                fieldAliasMappers, fullPathObjectMappers, fieldTypes);
+            MapperMergeValidator.validateMapperStructure(objectMappers, fieldMappers, fieldAliasMappers);
             checkPartitionedIndexConstraints(newMapper);
 
             // update lookup data-structures

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

@@ -36,13 +36,11 @@ public void testDuplicateFieldAliasAndObject() {
         FieldAliasMapper aliasMapper = new FieldAliasMapper("path", "some.path", "field");
 
         IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () ->
-            MapperMergeValidator.validateMapperStructure("type",
+            MapperMergeValidator.validateMapperStructure(
                 singletonList(objectMapper),
                 emptyList(),
-                singletonList(aliasMapper),
-                emptyMap(),
-                new FieldTypeLookup()));
-        assertEquals("Field [some.path] is defined both as an object and a field in [type]", e.getMessage());
+                singletonList(aliasMapper)));
+        assertEquals("Field [some.path] is defined both as an object and a field.", e.getMessage());
     }
 
     public void testFieldAliasWithNestedScope() {

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

@@ -880,7 +880,7 @@ public void testIndexPrefixMapping() throws IOException {
 
             IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () ->
                 indexService.mapperService().merge("type", new CompressedXContent(illegalMapping), MergeReason.MAPPING_UPDATE));
-            assertThat(e.getMessage(), containsString("Field [field._index_prefix] is defined twice in [type]"));
+            assertThat(e.getMessage(), containsString("Field [field._index_prefix] is defined twice."));
 
         }
 

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

@@ -151,14 +151,14 @@ public void testReuseMetaField() throws IOException {
             mapperService.merge("type", new CompressedXContent(Strings.toString(mapping)), MapperService.MergeReason.MAPPING_UPDATE);
             fail();
         } catch (IllegalArgumentException e) {
-            assertTrue(e.getMessage().contains("Field [_id] is defined twice in [type]"));
+            assertTrue(e.getMessage().contains("Field [_id] is defined twice."));
         }
 
         try {
             mapperService.merge("type", new CompressedXContent(Strings.toString(mapping)), MapperService.MergeReason.MAPPING_UPDATE);
             fail();
         } catch (IllegalArgumentException e) {
-            assertTrue(e.getMessage().contains("Field [_id] is defined twice in [type]"));
+            assertTrue(e.getMessage().contains("Field [_id] is defined twice."));
         }
     }