Merge pull request #15245 from jpountz/fix/check_type_name

Make MappedFieldType.checkTypeName part of MappedFieldType.checkCompatibility.

core/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java

@@ -370,15 +370,6 @@ public abstract class FieldMapper extends Mapper {
             return;
         }
         FieldMapper fieldMergeWith = (FieldMapper) mergeWith;
-        List<String> subConflicts = new ArrayList<>(); // TODO: just expose list from MergeResult?
-        fieldType().checkTypeName(fieldMergeWith.fieldType(), subConflicts);
-        if (subConflicts.isEmpty() == false) {
-            // return early if field types don't match
-            assert subConflicts.size() == 1;
-            mergeResult.addConflict(subConflicts.get(0));
-            return;
-        }
-
         multiFields.merge(mergeWith, mergeResult);
 
         if (mergeResult.simulate() == false && mergeResult.hasConflicts() == false) {

core/src/main/java/org/elasticsearch/index/mapper/FieldTypeLookup.java

@@ -154,12 +154,9 @@ class FieldTypeLookup implements Iterable<MappedFieldType> {
             MappedFieldTypeReference ref = fullNameToFieldType.get(fieldMapper.fieldType().names().fullName());
             if (ref != null) {
                 List<String> conflicts = new ArrayList<>();
-                ref.get().checkTypeName(fieldMapper.fieldType(), conflicts);
-                if (conflicts.isEmpty()) { // only check compat if they are the same type
-                    final Set<String> types = fullNameToTypes.get(fieldMapper.fieldType().names().fullName());
-                    boolean strict = beStrict(type, types, updateAllTypes);
-                    ref.get().checkCompatibility(fieldMapper.fieldType(), conflicts, strict);
-                }
+                final Set<String> types = fullNameToTypes.get(fieldMapper.fieldType().names().fullName());
+                boolean strict = beStrict(type, types, updateAllTypes);
+                ref.get().checkCompatibility(fieldMapper.fieldType(), conflicts, strict);
                 if (conflicts.isEmpty() == false) {
                     throw new IllegalArgumentException("Mapper for [" + fieldMapper.fieldType().names().fullName() + "] conflicts with existing mapping in other types:\n" + conflicts.toString());
                 }
@@ -169,12 +166,9 @@ class FieldTypeLookup implements Iterable<MappedFieldType> {
             MappedFieldTypeReference indexNameRef = indexNameToFieldType.get(fieldMapper.fieldType().names().indexName());
             if (indexNameRef != null) {
                 List<String> conflicts = new ArrayList<>();
-                indexNameRef.get().checkTypeName(fieldMapper.fieldType(), conflicts);
-                if (conflicts.isEmpty()) { // only check compat if they are the same type
-                    final Set<String> types = indexNameToTypes.get(fieldMapper.fieldType().names().indexName());
-                    boolean strict = beStrict(type, types, updateAllTypes);
-                    indexNameRef.get().checkCompatibility(fieldMapper.fieldType(), conflicts, strict);
-                }
+                final Set<String> types = indexNameToTypes.get(fieldMapper.fieldType().names().indexName());
+                boolean strict = beStrict(type, types, updateAllTypes);
+                indexNameRef.get().checkCompatibility(fieldMapper.fieldType(), conflicts, strict);
                 if (conflicts.isEmpty() == false) {
                     throw new IllegalArgumentException("Mapper for [" + fieldMapper.fieldType().names().fullName() + "] conflicts with mapping with the same index name in other types" + conflicts.toString());
                 }

core/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java

@@ -229,9 +229,9 @@ public abstract class MappedFieldType extends FieldType {
     public abstract String typeName();
 
     /** Checks this type is the same type as other. Adds a conflict if they are different. */
-    public final void checkTypeName(MappedFieldType other, List<String> conflicts) {
+    private final void checkTypeName(MappedFieldType other) {
         if (typeName().equals(other.typeName()) == false) {
-            conflicts.add("mapper [" + names().fullName() + "] cannot be changed from type [" + typeName() + "] to [" + other.typeName() + "]");
+            throw new IllegalArgumentException("mapper [" + names().fullName() + "] cannot be changed from type [" + typeName() + "] to [" + other.typeName() + "]");
         } else if (getClass() != other.getClass()) {
             throw new IllegalStateException("Type names equal for class " + getClass().getSimpleName() + " and " + other.getClass().getSimpleName());
         }
@@ -243,6 +243,8 @@ public abstract class MappedFieldType extends FieldType {
      * Otherwise, only properties which must never change in an index are checked.
      */
     public void checkCompatibility(MappedFieldType other, List<String> conflicts, boolean strict) {
+        checkTypeName(other);
+
         boolean indexed =  indexOptions() != IndexOptions.NONE;
         boolean mergeWithIndexed = other.indexOptions() != IndexOptions.NONE;
         // TODO: should be validating if index options go "up" (but "down" is ok)

core/src/test/java/org/elasticsearch/index/mapper/FieldTypeTestCase.java

@@ -281,7 +281,7 @@ public abstract class FieldTypeTestCase extends ESTestCase {
     public void testCheckTypeName() {
         final MappedFieldType fieldType = createNamedDefaultFieldType();
         List<String> conflicts = new ArrayList<>();
-        fieldType.checkTypeName(fieldType, conflicts);
+        fieldType.checkCompatibility(fieldType, conflicts, random().nextBoolean()); // no exception
         assertTrue(conflicts.toString(), conflicts.isEmpty());
 
         MappedFieldType bogus = new MappedFieldType() {
@@ -291,7 +291,7 @@ public abstract class FieldTypeTestCase extends ESTestCase {
             public String typeName() { return fieldType.typeName();}
         };
         try {
-            fieldType.checkTypeName(bogus, conflicts);
+            fieldType.checkCompatibility(bogus, conflicts, random().nextBoolean());
             fail("expected bad types exception");
         } catch (IllegalStateException e) {
             assertTrue(e.getMessage().contains("Type names equal"));
@@ -304,10 +304,13 @@ public abstract class FieldTypeTestCase extends ESTestCase {
             @Override
             public String typeName() { return "othertype";}
         };
-        fieldType.checkTypeName(other, conflicts);
-        assertFalse(conflicts.isEmpty());
-        assertTrue(conflicts.get(0).contains("cannot be changed from type"));
-        assertEquals(1, conflicts.size());
+        try {
+            fieldType.checkCompatibility(other, conflicts, random().nextBoolean());
+            fail();
+        } catch (IllegalArgumentException e) {
+            assertTrue(e.getMessage(), e.getMessage().contains("cannot be changed from type"));
+        }
+        assertTrue(conflicts.toString(), conflicts.isEmpty());
     }
 
     public void testCheckCompatibility() {

core/src/test/java/org/elasticsearch/index/mapper/update/UpdateMappingTests.java

@@ -151,7 +151,7 @@ public class UpdateMappingTests extends ESSingleNodeTestCase {
             fail();
         } catch (IllegalArgumentException e) {
             // expected
-            assertTrue(e.getMessage().contains("conflicts with existing mapping in other types"));
+            assertTrue(e.getMessage(), e.getMessage().contains("mapper [foo] cannot be changed from type [long] to [double]"));
         }
 
         try {
@@ -159,7 +159,7 @@ public class UpdateMappingTests extends ESSingleNodeTestCase {
             fail();
         } catch (IllegalArgumentException e) {
             // expected
-            assertTrue(e.getMessage().contains("conflicts with existing mapping in other types"));
+            assertTrue(e.getMessage(), e.getMessage().contains("mapper [foo] cannot be changed from type [long] to [double]"));
         }
 
         assertTrue(mapperService.documentMapper("type1").mapping().root().getMapper("foo") instanceof LongFieldMapper);
@@ -186,7 +186,7 @@ public class UpdateMappingTests extends ESSingleNodeTestCase {
             fail();
         } catch (IllegalArgumentException e) {
             // expected
-            assertTrue(e.getMessage().contains("conflicts with existing mapping in other types"));
+            assertTrue(e.getMessage(), e.getMessage().contains("mapper [foo] cannot be changed from type [long] to [double]"));
         }
 
         try {
@@ -194,7 +194,7 @@ public class UpdateMappingTests extends ESSingleNodeTestCase {
             fail();
         } catch (IllegalArgumentException e) {
             // expected
-            assertTrue(e.getMessage().contains("conflicts with existing mapping in other types"));
+            assertTrue(e.getMessage(), e.getMessage().contains("mapper [foo] cannot be changed from type [long] to [double]"));
         }
 
         assertTrue(mapperService.documentMapper("type1").mapping().root().getMapper("foo") instanceof LongFieldMapper);