Mappings: Fix field type compatiblity check to work when only one previous type exists.

This was a straight up bug found in #12753. If only one type existed,
the compatibility check for a new type was not strict, so changes to
an updateable setting like search_analyzer got through (but only
partially). This change fixes the check and adds tests (which were
previously a TODO).

This also fixes a bug in dynamic field creation which woudln't copy
fielddata settings when duplicating a pre-existing field with the
same name.

closes #12753

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

@@ -629,6 +629,7 @@ class DocumentParser implements Closeable {
                 // best-effort to not introduce a conflict
                 if (builder instanceof StringFieldMapper.Builder) {
                     StringFieldMapper.Builder stringBuilder = (StringFieldMapper.Builder) builder;
+                    stringBuilder.fieldDataSettings(existingFieldType.fieldDataType().getSettings());
                     stringBuilder.store(existingFieldType.stored());
                     stringBuilder.indexOptions(existingFieldType.indexOptions());
                     stringBuilder.tokenized(existingFieldType.tokenized());
@@ -638,6 +639,7 @@ class DocumentParser implements Closeable {
                     stringBuilder.searchAnalyzer(existingFieldType.searchAnalyzer());
                 } else if (builder instanceof NumberFieldMapper.Builder) {
                     NumberFieldMapper.Builder<?,?> numberBuilder = (NumberFieldMapper.Builder<?, ?>) builder;
+                    numberBuilder.fieldDataSettings(existingFieldType.fieldDataType().getSettings());
                     numberBuilder.store(existingFieldType.stored());
                     numberBuilder.indexOptions(existingFieldType.indexOptions());
                     numberBuilder.tokenized(existingFieldType.tokenized());

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

@@ -110,21 +110,21 @@ class FieldTypeLookup implements Iterable<MappedFieldType> {
                 List<String> conflicts = new ArrayList<>();
                 ref.get().checkTypeName(fieldMapper.fieldType(), conflicts);
                 if (conflicts.isEmpty()) { // only check compat if they are the same type
-                    boolean strict = ref.getNumAssociatedMappers() > 1 && updateAllTypes == false;
+                    boolean strict = updateAllTypes == false;
                     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" + conflicts.toString());
+                    throw new IllegalArgumentException("Mapper for [" + fieldMapper.fieldType().names().fullName() + "] conflicts with existing mapping in other types:\n" + conflicts.toString());
                 }
             }
 
             // field type for the index name must be compatible too
-            MappedFieldTypeReference indexNameRef = fullNameToFieldType.get(fieldMapper.fieldType().names().indexName());
+            MappedFieldTypeReference indexNameRef = indexNameToFieldType.get(fieldMapper.fieldType().names().indexName());
             if (indexNameRef != null) {
                 List<String> conflicts = new ArrayList<>();
-                ref.get().checkTypeName(fieldMapper.fieldType(), conflicts);
+                indexNameRef.get().checkTypeName(fieldMapper.fieldType(), conflicts);
                 if (conflicts.isEmpty()) { // only check compat if they are the same type
-                    boolean strict = indexNameRef.getNumAssociatedMappers() > 1 && updateAllTypes == false;
+                    boolean strict = updateAllTypes == false;
                     indexNameRef.get().checkCompatibility(fieldMapper.fieldType(), conflicts, strict);
                 }
                 if (conflicts.isEmpty() == false) {

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

@@ -274,7 +274,7 @@ public abstract class MappedFieldType extends FieldType {
             conflicts.add("mapper [" + names().fullName() + "] has different analyzer");
         }
 
-        if (!names().equals(other.names())) {
+        if (!names().indexName().equals(other.names().indexName())) {
             conflicts.add("mapper [" + names().fullName() + "] has different index_name");
         }
         if (Objects.equals(similarity(), other.similarity()) == false) {

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

@@ -24,6 +24,7 @@ import com.google.common.collect.Lists;
 import org.elasticsearch.Version;
 import org.elasticsearch.cluster.metadata.IndexMetaData;
 import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.index.mapper.core.StringFieldMapper;
 import org.elasticsearch.test.ESTestCase;
 
 import java.io.IOException;
@@ -131,7 +132,67 @@ public class FieldTypeLookupTests extends ESTestCase {
         }
     }
 
-    // TODO: add tests for validation
+    public void testCheckCompatibilityNewField() {
+        FakeFieldMapper f1 = new FakeFieldMapper("foo", "bar");
+        FieldTypeLookup lookup = new FieldTypeLookup();
+        lookup.checkCompatibility(newList(f1), false);
+    }
+
+    public void testCheckCompatibilityMismatchedTypes() {
+        FieldMapper f1 = new FakeFieldMapper("foo", "bar");
+        FieldTypeLookup lookup = new FieldTypeLookup();
+        lookup = lookup.copyAndAddAll(newList(f1));
+
+        MappedFieldType ft2 = FakeFieldMapper.makeOtherFieldType("foo", "foo");
+        FieldMapper f2 = new FakeFieldMapper("foo", ft2);
+        try {
+            lookup.checkCompatibility(newList(f2), false);
+            fail("expected type mismatch");
+        } catch (IllegalArgumentException e) {
+            assertTrue(e.getMessage().contains("cannot be changed from type [faketype] to [otherfaketype]"));
+        }
+        // fails even if updateAllTypes == true
+        try {
+            lookup.checkCompatibility(newList(f2), true);
+            fail("expected type mismatch");
+        } catch (IllegalArgumentException e) {
+            assertTrue(e.getMessage().contains("cannot be changed from type [faketype] to [otherfaketype]"));
+        }
+    }
+
+    public void testCheckCompatibilityConflict() {
+        FieldMapper f1 = new FakeFieldMapper("foo", "bar");
+        FieldTypeLookup lookup = new FieldTypeLookup();
+        lookup = lookup.copyAndAddAll(newList(f1));
+
+        MappedFieldType ft2 = FakeFieldMapper.makeFieldType("foo", "bar");
+        ft2.setBoost(2.0f);
+        FieldMapper f2 = new FakeFieldMapper("foo", ft2);
+        try {
+            lookup.checkCompatibility(newList(f2), false);
+            fail("expected conflict");
+        } catch (IllegalArgumentException e) {
+            assertTrue(e.getMessage().contains("to update [boost] across all types"));
+        }
+        lookup.checkCompatibility(newList(f2), true); // boost is updateable, so ok if forcing
+        // now with a non changeable setting
+        MappedFieldType ft3 = FakeFieldMapper.makeFieldType("foo", "bar");
+        ft3.setStored(true);
+        FieldMapper f3 = new FakeFieldMapper("foo", ft3);
+        try {
+            lookup.checkCompatibility(newList(f3), false);
+            fail("expected conflict");
+        } catch (IllegalArgumentException e) {
+            assertTrue(e.getMessage().contains("has different store values"));
+        }
+        // even with updateAllTypes == true, incompatible
+        try {
+            lookup.checkCompatibility(newList(f3), true);
+            fail("expected conflict");
+        } catch (IllegalArgumentException e) {
+            assertTrue(e.getMessage().contains("has different store values"));
+        }
+    }
 
     public void testSimpleMatchIndexNames() {
         FakeFieldMapper f1 = new FakeFieldMapper("foo", "baz");
@@ -179,11 +240,19 @@ public class FieldTypeLookupTests extends ESTestCase {
         public FakeFieldMapper(String fullName, String indexName) {
             super(fullName, makeFieldType(fullName, indexName), makeFieldType(fullName, indexName), dummySettings, null, null);
         }
+        public FakeFieldMapper(String fullName, MappedFieldType fieldType) {
+            super(fullName, fieldType, fieldType, dummySettings, null, null);
+        }
         static MappedFieldType makeFieldType(String fullName, String indexName) {
             FakeFieldType fieldType = new FakeFieldType();
             fieldType.setNames(new MappedFieldType.Names(indexName, indexName, fullName));
             return fieldType;
         }
+        static MappedFieldType makeOtherFieldType(String fullName, String indexName) {
+            OtherFakeFieldType fieldType = new OtherFakeFieldType();
+            fieldType.setNames(new MappedFieldType.Names(indexName, indexName, fullName));
+            return fieldType;
+        }
         static class FakeFieldType extends MappedFieldType {
             public FakeFieldType() {}
             protected FakeFieldType(FakeFieldType ref) {
@@ -198,6 +267,20 @@ public class FieldTypeLookupTests extends ESTestCase {
                 return "faketype";
             }
         }
+        static class OtherFakeFieldType extends MappedFieldType {
+            public OtherFakeFieldType() {}
+            protected OtherFakeFieldType(OtherFakeFieldType ref) {
+                super(ref);
+            }
+            @Override
+            public MappedFieldType clone() {
+                return new OtherFakeFieldType(this);
+            }
+            @Override
+            public String typeName() {
+                return "otherfaketype";
+            }
+        }
         @Override
         protected String contentType() { return null; }
         @Override

core/src/test/java/org/elasticsearch/search/child/ParentFieldLoadingIT.java

@@ -57,7 +57,8 @@ public class ParentFieldLoadingIT extends ESIntegTestCase {
         assertAcked(prepareCreate("test")
                 .setSettings(indexSettings)
                 .addMapping("parent")
-                .addMapping("child", childMapping(MappedFieldType.Loading.LAZY)));
+                .addMapping("child", childMapping(MappedFieldType.Loading.LAZY))
+                .setUpdateAllTypes(true));
         ensureGreen();
 
         client().prepareIndex("test", "parent", "1").setSource("{}").get();
@@ -72,7 +73,8 @@ public class ParentFieldLoadingIT extends ESIntegTestCase {
         assertAcked(prepareCreate("test")
                 .setSettings(indexSettings)
                 .addMapping("parent")
-                .addMapping("child", "_parent", "type=parent"));
+                .addMapping("child", "_parent", "type=parent")
+                .setUpdateAllTypes(true));
         ensureGreen();
 
         client().prepareIndex("test", "parent", "1").setSource("{}").get();
@@ -87,7 +89,8 @@ public class ParentFieldLoadingIT extends ESIntegTestCase {
         assertAcked(prepareCreate("test")
                 .setSettings(indexSettings)
                 .addMapping("parent")
-                .addMapping("child", childMapping(MappedFieldType.Loading.EAGER)));
+                .addMapping("child", childMapping(MappedFieldType.Loading.EAGER))
+                .setUpdateAllTypes(true));
         ensureGreen();
 
         client().prepareIndex("test", "parent", "1").setSource("{}").get();
@@ -102,7 +105,8 @@ public class ParentFieldLoadingIT extends ESIntegTestCase {
         assertAcked(prepareCreate("test")
                 .setSettings(indexSettings)
                 .addMapping("parent")
-                .addMapping("child", childMapping(MappedFieldType.Loading.EAGER_GLOBAL_ORDINALS)));
+                .addMapping("child", childMapping(MappedFieldType.Loading.EAGER_GLOBAL_ORDINALS))
+                .setUpdateAllTypes(true));
         ensureGreen();
 
         // Need to do 2 separate refreshes, otherwise we have 1 segment and then we can't measure if global ordinals