Check mapping compatibility up-front.

Today we only check mapping compatibility when adding mappers to the
lookup structure. However, at this stage, the mapping has already been merged
partially, so we can leave mappings in a bad state. This commit removes the
compatibility check from Mapper.merge entirely and performs it _before_ we
call Mapper.merge.

One minor regression is that the exception messages don't group together errors
that come from MappedFieldType.checkCompatibility and Mapper.merge. Since we
run the former before the latter, Mapper.merge won't even have a chance to let
the user know about conflicts if conflicts were discovered by
MappedFieldType.checkCompatibility.

Close #15049

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

@@ -336,8 +336,6 @@ public class DocumentMapper implements ToXContent {
 
     private void addMappers(Collection<ObjectMapper> objectMappers, Collection<FieldMapper> fieldMappers, boolean updateAllTypes) {
         assert mappingLock.isWriteLockedByCurrentThread();
-        // first ensure we don't have any incompatible new fields
-        mapperService.checkNewMappersCompatibility(objectMappers, fieldMappers, updateAllTypes);
 
         // update mappers for this document type
         Map<String, ObjectMapper> builder = new HashMap<>(this.objectMappers);
@@ -356,6 +354,7 @@ public class DocumentMapper implements ToXContent {
 
     public MergeResult merge(Mapping mapping, boolean simulate, boolean updateAllTypes) {
         try (ReleasableLock lock = mappingWriteLock.acquire()) {
+            mapperService.checkMappersCompatibility(type, mapping, updateAllTypes);
             final MergeResult mergeResult = new MergeResult(simulate, updateAllTypes);
             this.mapping.merge(mapping, mergeResult);
             if (simulate == false) {

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

@@ -307,7 +307,6 @@ public abstract class FieldMapper extends Mapper {
         if (ref.get().equals(fieldType()) == false) {
             throw new IllegalStateException("Cannot overwrite field type reference to unequal reference");
         }
-        ref.incrementAssociatedMappers();
         this.fieldTypeRef = ref;
     }
 
@@ -380,11 +379,6 @@ public abstract class FieldMapper extends Mapper {
             return;
         }
 
-        boolean strict = this.fieldTypeRef.getNumAssociatedMappers() > 1 && mergeResult.updateAllTypes() == false;
-        fieldType().checkCompatibility(fieldMergeWith.fieldType(), subConflicts, strict);
-        for (String conflict : subConflicts) {
-            mergeResult.addConflict(conflict);
-        }
         multiFields.merge(mergeWith, mergeResult);
 
         if (mergeResult.simulate() == false && mergeResult.hasConflicts() == false) {

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

@@ -24,6 +24,7 @@ import org.elasticsearch.common.regex.Regex;
 
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
@@ -38,18 +39,49 @@ class FieldTypeLookup implements Iterable<MappedFieldType> {
     /** Full field name to field type */
     private final CopyOnWriteHashMap<String, MappedFieldTypeReference> fullNameToFieldType;
 
+    /** Full field name to types containing a mapping for this full name. */
+    private final CopyOnWriteHashMap<String, Set<String>> fullNameToTypes;
+
     /** Index field name to field type */
     private final CopyOnWriteHashMap<String, MappedFieldTypeReference> indexNameToFieldType;
 
+    /** Index field name to types containing a mapping for this index name. */
+    private final CopyOnWriteHashMap<String, Set<String>> indexNameToTypes;
+
     /** Create a new empty instance. */
     public FieldTypeLookup() {
         fullNameToFieldType = new CopyOnWriteHashMap<>();
+        fullNameToTypes = new CopyOnWriteHashMap<>();
         indexNameToFieldType = new CopyOnWriteHashMap<>();
+        indexNameToTypes = new CopyOnWriteHashMap<>();
     }
 
-    private FieldTypeLookup(CopyOnWriteHashMap<String, MappedFieldTypeReference> fullName, CopyOnWriteHashMap<String, MappedFieldTypeReference> indexName) {
+    private FieldTypeLookup(
-        fullNameToFieldType = fullName;
+            CopyOnWriteHashMap<String, MappedFieldTypeReference> fullName,
-        indexNameToFieldType = indexName;
+            CopyOnWriteHashMap<String, Set<String>> fullNameToTypes,
+            CopyOnWriteHashMap<String, MappedFieldTypeReference> indexName,
+            CopyOnWriteHashMap<String, Set<String>> indexNameToTypes) {
+        this.fullNameToFieldType = fullName;
+        this.fullNameToTypes = fullNameToTypes;
+        this.indexNameToFieldType = indexName;
+        this.indexNameToTypes = indexNameToTypes;
+    }
+
+    private static CopyOnWriteHashMap<String, Set<String>> addType(CopyOnWriteHashMap<String, Set<String>> map, String key, String type) {
+        Set<String> types = map.get(key);
+        if (types == null) {
+            return map.copyAndPut(key, Collections.singleton(type));
+        } else if (types.contains(type)) {
+            // noting to do
+            return map;
+        } else {
+            Set<String> newTypes = new HashSet<>(types.size() + 1);
+            newTypes.addAll(types);
+            newTypes.add(type);
+            assert newTypes.size() == types.size() + 1;
+            newTypes = Collections.unmodifiableSet(newTypes);
+            return map.copyAndPut(key, newTypes);
+        }
     }
 
     /**
@@ -63,7 +95,9 @@ class FieldTypeLookup implements Iterable<MappedFieldType> {
             throw new IllegalArgumentException("Default mappings should not be added to the lookup");
         }
         CopyOnWriteHashMap<String, MappedFieldTypeReference> fullName = this.fullNameToFieldType;
+        CopyOnWriteHashMap<String, Set<String>> fullNameToTypes = this.fullNameToTypes;
         CopyOnWriteHashMap<String, MappedFieldTypeReference> indexName = this.indexNameToFieldType;
+        CopyOnWriteHashMap<String, Set<String>> indexNameToTypes = this.indexNameToTypes;
 
         for (FieldMapper fieldMapper : newFieldMappers) {
             MappedFieldType fieldType = fieldMapper.fieldType();
@@ -91,8 +125,23 @@ class FieldTypeLookup implements Iterable<MappedFieldType> {
                 // this new field bridges between two existing field names (a full and index name), which we cannot support
                 throw new IllegalStateException("insane mappings found. field " + fieldType.names().fullName() + " maps across types to field " + fieldType.names().indexName());
             }
+
+            fullNameToTypes = addType(fullNameToTypes, fieldType.names().fullName(), type);
+            indexNameToTypes = addType(indexNameToTypes, fieldType.names().indexName(), type);
+        }
+        return new FieldTypeLookup(fullName, fullNameToTypes, indexName, indexNameToTypes);
+    }
+
+    private static boolean beStrict(String type, Set<String> types, boolean updateAllTypes) {
+        assert types.size() >= 1;
+        if (updateAllTypes) {
+            return false;
+        } else if (types.size() == 1 && types.contains(type)) {
+            // we are implicitly updating all types
+            return false;
+        } else {
+            return true;
         }
-        return new FieldTypeLookup(fullName, indexName);
     }
 
     /**
@@ -100,14 +149,15 @@ class FieldTypeLookup implements Iterable<MappedFieldType> {
      * If any are not compatible, an IllegalArgumentException is thrown.
      * If updateAllTypes is true, only basic compatibility is checked.
      */
-    public void checkCompatibility(Collection<FieldMapper> newFieldMappers, boolean updateAllTypes) {
+    public void checkCompatibility(String type, Collection<FieldMapper> fieldMappers, boolean updateAllTypes) {
-        for (FieldMapper fieldMapper : newFieldMappers) {
+        for (FieldMapper fieldMapper : fieldMappers) {
             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
-                    boolean strict = updateAllTypes == false;
+                    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) {
@@ -121,7 +171,8 @@ class FieldTypeLookup implements Iterable<MappedFieldType> {
                 List<String> conflicts = new ArrayList<>();
                 indexNameRef.get().checkTypeName(fieldMapper.fieldType(), conflicts);
                 if (conflicts.isEmpty()) { // only check compat if they are the same type
-                    boolean strict = updateAllTypes == false;
+                    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) {
@@ -138,6 +189,15 @@ class FieldTypeLookup implements Iterable<MappedFieldType> {
         return ref.get();
     }
 
+    /** Get the set of types that have a mapping for the given field. */
+    public Set<String> getTypes(String field) {
+        Set<String> types = fullNameToTypes.get(field);
+        if (types == null) {
+            types = Collections.emptySet();
+        }
+        return types;
+    }
+
     /** Returns the field type for the given index name */
     public MappedFieldType getByIndexName(String field) {
         MappedFieldTypeReference ref = indexNameToFieldType.get(field);
@@ -145,6 +205,15 @@ class FieldTypeLookup implements Iterable<MappedFieldType> {
         return ref.get();
     }
 
+    /** Get the set of types that have a mapping for the given field. */
+    public Set<String> getTypesByIndexName(String field) {
+        Set<String> types = indexNameToTypes.get(field);
+        if (types == null) {
+            types = Collections.emptySet();
+        }
+        return types;
+    }
+
     /**
      * Returns a list of the index names of a simple match regex like pattern against full name and index name.
      */

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

@@ -23,12 +23,10 @@ package org.elasticsearch.index.mapper;
  */
 public class MappedFieldTypeReference {
     private MappedFieldType fieldType; // the current field type this reference points to
-    private int numAssociatedMappers;
 
     public MappedFieldTypeReference(MappedFieldType fieldType) {
         fieldType.freeze(); // ensure frozen
         this.fieldType = fieldType;
-        this.numAssociatedMappers = 1;
     }
 
     public MappedFieldType get() {
@@ -40,11 +38,4 @@ public class MappedFieldTypeReference {
         this.fieldType = fieldType;
     }
 
-    public int getNumAssociatedMappers() {
-        return numAssociatedMappers;
-    }
-
-    public void incrementAssociatedMappers() {
-        ++numAssociatedMappers;
-    }
 }

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

@@ -33,6 +33,7 @@ import org.elasticsearch.ElasticsearchGenerationException;
 import org.elasticsearch.Version;
 import org.elasticsearch.common.Nullable;
 import org.elasticsearch.common.collect.ImmutableOpenMap;
+import org.elasticsearch.common.collect.Tuple;
 import org.elasticsearch.common.compress.CompressedXContent;
 import org.elasticsearch.common.lucene.search.Queries;
 import org.elasticsearch.common.regex.Regex;
@@ -260,13 +261,10 @@ public class MapperService extends AbstractIndexComponent implements Closeable {
                 assert result.hasConflicts() == false; // we already simulated
                 return oldMapper;
             } else {
-                List<ObjectMapper> newObjectMappers = new ArrayList<>();
+                Tuple<Collection<ObjectMapper>, Collection<FieldMapper>> newMappers = checkMappersCompatibility(
-                List<FieldMapper> newFieldMappers = new ArrayList<>();
+                        mapper.type(), mapper.mapping(), updateAllTypes);
-                for (MetadataFieldMapper metadataMapper : mapper.mapping().metadataMappers) {
+                Collection<ObjectMapper> newObjectMappers = newMappers.v1();
-                    newFieldMappers.add(metadataMapper);
+                Collection<FieldMapper> newFieldMappers = newMappers.v2();
-                }
-                MapperUtils.collect(mapper.mapping().root, newObjectMappers, newFieldMappers);
-                checkNewMappersCompatibility(newObjectMappers, newFieldMappers, updateAllTypes);
                 addMappers(mapper.type(), newObjectMappers, newFieldMappers);
 
                 for (DocumentTypeListener typeListener : typeListeners) {
@@ -302,9 +300,9 @@ public class MapperService extends AbstractIndexComponent implements Closeable {
         return true;
     }
 
-    protected void checkNewMappersCompatibility(Collection<ObjectMapper> newObjectMappers, Collection<FieldMapper> newFieldMappers, boolean updateAllTypes) {
+    protected void checkMappersCompatibility(String type, Collection<ObjectMapper> objectMappers, Collection<FieldMapper> fieldMappers, boolean updateAllTypes) {
         assert mappingLock.isWriteLockedByCurrentThread();
-        for (ObjectMapper newObjectMapper : newObjectMappers) {
+        for (ObjectMapper newObjectMapper : objectMappers) {
             ObjectMapper existingObjectMapper = fullPathObjectMappers.get(newObjectMapper.fullPath());
             if (existingObjectMapper != null) {
                 MergeResult result = new MergeResult(true, updateAllTypes);
@@ -315,7 +313,19 @@ public class MapperService extends AbstractIndexComponent implements Closeable {
                 }
             }
         }
-        fieldTypes.checkCompatibility(newFieldMappers, updateAllTypes);
+        fieldTypes.checkCompatibility(type, fieldMappers, updateAllTypes);
+    }
+
+    protected Tuple<Collection<ObjectMapper>, Collection<FieldMapper>> checkMappersCompatibility(
+            String type, Mapping mapping, boolean updateAllTypes) {
+        List<ObjectMapper> objectMappers = new ArrayList<>();
+        List<FieldMapper> fieldMappers = new ArrayList<>();
+        for (MetadataFieldMapper metadataMapper : mapping.metadataMappers) {
+            fieldMappers.add(metadataMapper);
+        }
+        MapperUtils.collect(mapping.root, objectMappers, fieldMappers);
+        checkMappersCompatibility(type, objectMappers, fieldMappers, updateAllTypes);
+        return new Tuple<>(objectMappers, fieldMappers);
     }
 
     protected void addMappers(String type, Collection<ObjectMapper> objectMappers, Collection<FieldMapper> fieldMappers) {

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

@@ -135,6 +135,15 @@ public abstract class NumberFieldMapper extends FieldMapper implements AllFieldM
             super(ref);
         }
 
+        @Override
+        public void checkCompatibility(MappedFieldType other,
+                List<String> conflicts, boolean strict) {
+            super.checkCompatibility(other, conflicts, strict);
+            if (numericPrecisionStep() != other.numericPrecisionStep()) {
+                conflicts.add("mapper [" + names().fullName() + "] has different [precision_step] values");
+            }
+        }
+
         public abstract NumberFieldType clone();
 
         @Override
@@ -251,11 +260,6 @@ public abstract class NumberFieldMapper extends FieldMapper implements AllFieldM
             return;
         }
         NumberFieldMapper nfmMergeWith = (NumberFieldMapper) mergeWith;
-        if (this.fieldTypeRef.getNumAssociatedMappers() > 1 && mergeResult.updateAllTypes() == false) {
-            if (fieldType().numericPrecisionStep() != nfmMergeWith.fieldType().numericPrecisionStep()) {
-                mergeResult.addConflict("mapper [" + fieldType().names().fullName() + "] is used by multiple types. Set update_all_types to true to update precision_step across all types.");
-            }
-        }
 
         if (mergeResult.simulate() == false && mergeResult.hasConflicts() == false) {
             this.includeInAll = nfmMergeWith.includeInAll;

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

@@ -37,6 +37,8 @@ public class FieldTypeLookupTests extends ESTestCase {
         FieldTypeLookup lookup = new FieldTypeLookup();
         assertNull(lookup.get("foo"));
         assertNull(lookup.getByIndexName("foo"));
+        assertEquals(Collections.emptySet(), lookup.getTypes("foo"));
+        assertEquals(Collections.emptySet(), lookup.getTypesByIndexName("foo"));
         Collection<String> names = lookup.simpleMatchToFullName("foo");
         assertNotNull(names);
         assertTrue(names.isEmpty());
@@ -70,6 +72,14 @@ public class FieldTypeLookupTests extends ESTestCase {
         assertNull(lookup.get("bar"));
         assertEquals(f.fieldType(), lookup2.getByIndexName("bar"));
         assertNull(lookup.getByIndexName("foo"));
+        assertEquals(Collections.emptySet(), lookup.getTypes("foo"));
+        assertEquals(Collections.emptySet(), lookup.getTypesByIndexName("foo"));
+        assertEquals(Collections.emptySet(), lookup.getTypes("bar"));
+        assertEquals(Collections.emptySet(), lookup.getTypesByIndexName("bar"));
+        assertEquals(Collections.singleton("type"), lookup2.getTypes("foo"));
+        assertEquals(Collections.emptySet(), lookup2.getTypesByIndexName("foo"));
+        assertEquals(Collections.emptySet(), lookup2.getTypes("bar"));
+        assertEquals(Collections.singleton("type"), lookup2.getTypesByIndexName("bar"));
         assertEquals(1, size(lookup2.iterator()));
     }
 
@@ -144,7 +154,7 @@ public class FieldTypeLookupTests extends ESTestCase {
     public void testCheckCompatibilityNewField() {
         FakeFieldMapper f1 = new FakeFieldMapper("foo", "bar");
         FieldTypeLookup lookup = new FieldTypeLookup();
-        lookup.checkCompatibility(newList(f1), false);
+        lookup.checkCompatibility("type", newList(f1), false);
     }
 
     public void testCheckCompatibilityMismatchedTypes() {
@@ -155,14 +165,14 @@ public class FieldTypeLookupTests extends ESTestCase {
         MappedFieldType ft2 = FakeFieldMapper.makeOtherFieldType("foo", "foo");
         FieldMapper f2 = new FakeFieldMapper("foo", ft2);
         try {
-            lookup.checkCompatibility(newList(f2), false);
+            lookup.checkCompatibility("type2", 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);
+            lookup.checkCompatibility("type2", newList(f2), true);
             fail("expected type mismatch");
         } catch (IllegalArgumentException e) {
             assertTrue(e.getMessage().contains("cannot be changed from type [faketype] to [otherfaketype]"));
@@ -178,25 +188,27 @@ public class FieldTypeLookupTests extends ESTestCase {
         ft2.setBoost(2.0f);
         FieldMapper f2 = new FakeFieldMapper("foo", ft2);
         try {
-            lookup.checkCompatibility(newList(f2), false);
+            // different type
+            lookup.checkCompatibility("type2", 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
+        lookup.checkCompatibility("type", newList(f2), false); // boost is updateable, so ok since we are implicitly updating all types
+        lookup.checkCompatibility("type2", 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);
+            lookup.checkCompatibility("type2", 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);
+            lookup.checkCompatibility("type2", newList(f3), true);
             fail("expected conflict");
         } catch (IllegalArgumentException e) {
             assertTrue(e.getMessage().contains("has different [store] values"));

core/src/test/java/org/elasticsearch/index/mapper/geo/GeoPointFieldMapperTests.java

@@ -25,12 +25,14 @@ import org.elasticsearch.action.admin.indices.create.CreateIndexRequestBuilder;
 import org.elasticsearch.action.search.SearchResponse;
 import org.elasticsearch.cluster.metadata.IndexMetaData;
 import org.elasticsearch.common.Priority;
+import org.elasticsearch.common.compress.CompressedXContent;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentFactory;
 import org.elasticsearch.index.mapper.DocumentMapper;
 import org.elasticsearch.index.mapper.DocumentMapperParser;
 import org.elasticsearch.index.mapper.MapperParsingException;
+import org.elasticsearch.index.mapper.MapperService;
 import org.elasticsearch.index.mapper.MergeResult;
 import org.elasticsearch.index.mapper.ParsedDocument;
 import org.elasticsearch.search.SearchHitField;
@@ -715,28 +717,25 @@ public class GeoPointFieldMapperTests extends ESSingleNodeTestCase {
         String stage1Mapping = XContentFactory.jsonBuilder().startObject().startObject("type")
                 .startObject("properties").startObject("point").field("type", "geo_point").field("lat_lon", true)
                 .field("geohash", true).endObject().endObject().endObject().endObject().string();
-        DocumentMapperParser parser = createIndex("test", settings).mapperService().documentMapperParser();
+        MapperService mapperService = createIndex("test", settings).mapperService();
-        DocumentMapper stage1 = parser.parse(stage1Mapping);
+        DocumentMapper stage1 = mapperService.merge("type", new CompressedXContent(stage1Mapping), true, false);
         String stage2Mapping = XContentFactory.jsonBuilder().startObject().startObject("type")
                 .startObject("properties").startObject("point").field("type", "geo_point").field("lat_lon", false)
                 .field("geohash", false).endObject().endObject().endObject().endObject().string();
-        DocumentMapper stage2 = parser.parse(stage2Mapping);
+        try {
-
+            mapperService.merge("type", new CompressedXContent(stage2Mapping), false, false);
-        MergeResult mergeResult = stage1.merge(stage2.mapping(), false, false);
+            fail();
-        assertThat(mergeResult.hasConflicts(), equalTo(true));
+        } catch (IllegalArgumentException e) {
-        assertThat(mergeResult.buildConflicts().length, equalTo(3));
+            assertThat(e.getMessage(), containsString("mapper [point] has different [lat_lon]"));
-        // todo better way of checking conflict?
+            assertThat(e.getMessage(), containsString("mapper [point] has different [geohash]"));
-        assertThat("mapper [point] has different [lat_lon]", isIn(new ArrayList<>(Arrays.asList(mergeResult.buildConflicts()))));
+            assertThat(e.getMessage(), containsString("mapper [point] has different [geohash_precision]"));
-        assertThat("mapper [point] has different [geohash]", isIn(new ArrayList<>(Arrays.asList(mergeResult.buildConflicts()))));
+        }
-        assertThat("mapper [point] has different [geohash_precision]", isIn(new ArrayList<>(Arrays.asList(mergeResult.buildConflicts()))));
 
         // correct mapping and ensure no failures
         stage2Mapping = XContentFactory.jsonBuilder().startObject().startObject("type")
                 .startObject("properties").startObject("point").field("type", "geo_point").field("lat_lon", true)
                 .field("geohash", true).endObject().endObject().endObject().endObject().string();
-        stage2 = parser.parse(stage2Mapping);
+        mapperService.merge("type", new CompressedXContent(stage2Mapping), false, false);
-        mergeResult = stage1.merge(stage2.mapping(), false, false);
-        assertThat(Arrays.toString(mergeResult.buildConflicts()), mergeResult.hasConflicts(), equalTo(false));
     }
 
     public void testGeoHashSearch() throws Exception {

core/src/test/java/org/elasticsearch/index/mapper/geo/GeoShapeFieldMapperTests.java

@@ -22,12 +22,14 @@ import org.apache.lucene.spatial.prefix.PrefixTreeStrategy;
 import org.apache.lucene.spatial.prefix.RecursivePrefixTreeStrategy;
 import org.apache.lucene.spatial.prefix.tree.GeohashPrefixTree;
 import org.apache.lucene.spatial.prefix.tree.QuadPrefixTree;
+import org.elasticsearch.common.compress.CompressedXContent;
 import org.elasticsearch.common.geo.GeoUtils;
 import org.elasticsearch.common.geo.builders.ShapeBuilder;
 import org.elasticsearch.common.xcontent.XContentFactory;
 import org.elasticsearch.index.mapper.DocumentMapper;
 import org.elasticsearch.index.mapper.DocumentMapperParser;
 import org.elasticsearch.index.mapper.FieldMapper;
+import org.elasticsearch.index.mapper.MapperService;
 import org.elasticsearch.index.mapper.MergeResult;
 import org.elasticsearch.test.ESSingleNodeTestCase;
 
@@ -35,6 +37,7 @@ import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Arrays;
 
+import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.instanceOf;
 import static org.hamcrest.Matchers.isIn;
@@ -376,23 +379,21 @@ public class GeoShapeFieldMapperTests extends ESSingleNodeTestCase {
                 .startObject("shape").field("type", "geo_shape").field("tree", "geohash").field("strategy", "recursive")
                 .field("precision", "1m").field("tree_levels", 8).field("distance_error_pct", 0.01).field("orientation", "ccw")
                 .endObject().endObject().endObject().endObject().string();
-        DocumentMapperParser parser = createIndex("test").mapperService().documentMapperParser();
+        MapperService mapperService = createIndex("test").mapperService();
-        DocumentMapper stage1 = parser.parse(stage1Mapping);
+        DocumentMapper stage1 = mapperService.merge("type", new CompressedXContent(stage1Mapping), true, false);
         String stage2Mapping = XContentFactory.jsonBuilder().startObject().startObject("type")
                 .startObject("properties").startObject("shape").field("type", "geo_shape").field("tree", "quadtree")
                 .field("strategy", "term").field("precision", "1km").field("tree_levels", 26).field("distance_error_pct", 26)
                 .field("orientation", "cw").endObject().endObject().endObject().endObject().string();
-        DocumentMapper stage2 = parser.parse(stage2Mapping);
+        try {
-
+            mapperService.merge("type", new CompressedXContent(stage2Mapping), false, false);
-        MergeResult mergeResult = stage1.merge(stage2.mapping(), false, false);
+            fail();
-        // check correct conflicts
+        } catch (IllegalArgumentException e) {
-        assertThat(mergeResult.hasConflicts(), equalTo(true));
+            assertThat(e.getMessage(), containsString("mapper [shape] has different [strategy]"));
-        assertThat(mergeResult.buildConflicts().length, equalTo(4));
+            assertThat(e.getMessage(), containsString("mapper [shape] has different [tree]"));
-        ArrayList<String> conflicts = new ArrayList<>(Arrays.asList(mergeResult.buildConflicts()));
+            assertThat(e.getMessage(), containsString("mapper [shape] has different [tree_levels]"));
-        assertThat("mapper [shape] has different [strategy]", isIn(conflicts));
+            assertThat(e.getMessage(), containsString("mapper [shape] has different [precision]"));
-        assertThat("mapper [shape] has different [tree]", isIn(conflicts));
+        }
-        assertThat("mapper [shape] has different [tree_levels]", isIn(conflicts));
-        assertThat("mapper [shape] has different [precision]", isIn(conflicts));
 
         // verify nothing changed
         FieldMapper fieldMapper = stage1.mappers().getMapper("shape");
@@ -411,11 +412,7 @@ public class GeoShapeFieldMapperTests extends ESSingleNodeTestCase {
         stage2Mapping = XContentFactory.jsonBuilder().startObject().startObject("type")
                 .startObject("properties").startObject("shape").field("type", "geo_shape").field("precision", "1m")
                 .field("tree_levels", 8).field("distance_error_pct", 0.001).field("orientation", "cw").endObject().endObject().endObject().endObject().string();
-        stage2 = parser.parse(stage2Mapping);
+        mapperService.merge("type", new CompressedXContent(stage2Mapping), false, false);
-        mergeResult = stage1.merge(stage2.mapping(), false, false);
-
-        // verify mapping changes, and ensure no failures
-        assertThat(Arrays.toString(mergeResult.buildConflicts()), mergeResult.hasConflicts(), equalTo(false));
 
         fieldMapper = stage1.mappers().getMapper("shape");
         assertThat(fieldMapper, instanceOf(GeoShapeFieldMapper.class));

core/src/test/java/org/elasticsearch/index/mapper/multifield/merge/JavaMultiFieldMergeTests.java

@@ -22,9 +22,11 @@ package org.elasticsearch.index.mapper.multifield.merge;
 import org.apache.lucene.index.IndexOptions;
 import org.apache.lucene.index.IndexableField;
 import org.elasticsearch.common.bytes.BytesReference;
+import org.elasticsearch.common.compress.CompressedXContent;
 import org.elasticsearch.common.xcontent.XContentFactory;
 import org.elasticsearch.index.mapper.DocumentMapper;
 import org.elasticsearch.index.mapper.DocumentMapperParser;
+import org.elasticsearch.index.mapper.MapperService;
 import org.elasticsearch.index.mapper.MergeResult;
 import org.elasticsearch.index.mapper.ParseContext.Document;
 import org.elasticsearch.test.ESSingleNodeTestCase;
@@ -32,6 +34,7 @@ import org.elasticsearch.test.ESSingleNodeTestCase;
 import java.util.Arrays;
 
 import static org.elasticsearch.test.StreamsUtils.copyToStringFromClasspath;
+import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.notNullValue;
 import static org.hamcrest.Matchers.nullValue;
@@ -113,9 +116,9 @@ public class JavaMultiFieldMergeTests extends ESSingleNodeTestCase {
 
     public void testUpgradeFromMultiFieldTypeToMultiFields() throws Exception {
         String mapping = copyToStringFromClasspath("/org/elasticsearch/index/mapper/multifield/merge/test-mapping1.json");
-        DocumentMapperParser parser = createIndex("test").mapperService().documentMapperParser();
+        MapperService mapperService = createIndex("test").mapperService();
 
-        DocumentMapper docMapper = parser.parse(mapping);
+        DocumentMapper docMapper = mapperService.merge("person", new CompressedXContent(mapping), true, false);
 
         assertNotSame(IndexOptions.NONE, docMapper.mappers().getMapper("name").fieldType().indexOptions());
         assertThat(docMapper.mappers().getMapper("name.indexed"), nullValue());
@@ -129,12 +132,7 @@ public class JavaMultiFieldMergeTests extends ESSingleNodeTestCase {
 
 
         mapping = copyToStringFromClasspath("/org/elasticsearch/index/mapper/multifield/merge/upgrade1.json");
-        DocumentMapper docMapper2 = parser.parse(mapping);
+        mapperService.merge("person", new CompressedXContent(mapping), false, false);
-
-        MergeResult mergeResult = docMapper.merge(docMapper2.mapping(), true, false);
-        assertThat(Arrays.toString(mergeResult.buildConflicts()), mergeResult.hasConflicts(), equalTo(false));
-
-        docMapper.merge(docMapper2.mapping(), false, false);
 
         assertNotSame(IndexOptions.NONE, docMapper.mappers().getMapper("name").fieldType().indexOptions());
 
@@ -151,12 +149,7 @@ public class JavaMultiFieldMergeTests extends ESSingleNodeTestCase {
         assertThat(f, notNullValue());
 
         mapping = copyToStringFromClasspath("/org/elasticsearch/index/mapper/multifield/merge/upgrade2.json");
-        DocumentMapper docMapper3 = parser.parse(mapping);
+        mapperService.merge("person", new CompressedXContent(mapping), false, false);
-
-        mergeResult = docMapper.merge(docMapper3.mapping(), true, false);
-        assertThat(Arrays.toString(mergeResult.buildConflicts()), mergeResult.hasConflicts(), equalTo(false));
-
-        docMapper.merge(docMapper3.mapping(), false, false);
 
         assertNotSame(IndexOptions.NONE, docMapper.mappers().getMapper("name").fieldType().indexOptions());
 
@@ -168,24 +161,19 @@ public class JavaMultiFieldMergeTests extends ESSingleNodeTestCase {
 
 
         mapping = copyToStringFromClasspath("/org/elasticsearch/index/mapper/multifield/merge/upgrade3.json");
-        DocumentMapper docMapper4 = parser.parse(mapping);
+        try {
-        mergeResult = docMapper.merge(docMapper4.mapping(), true, false);
+            mapperService.merge("person", new CompressedXContent(mapping), false, false);
-        assertThat(Arrays.toString(mergeResult.buildConflicts()), mergeResult.hasConflicts(), equalTo(true));
+            fail();
-        assertThat(mergeResult.buildConflicts()[0], equalTo("mapper [name] has different [index] values"));
+        } catch (IllegalArgumentException e) {
-        assertThat(mergeResult.buildConflicts()[1], equalTo("mapper [name] has different [store] values"));
+            assertThat(e.getMessage(), containsString("mapper [name] has different [index] values"));
+            assertThat(e.getMessage(), containsString("mapper [name] has different [store] values"));
+        }
 
-        mergeResult = docMapper.merge(docMapper4.mapping(), false, false);
+        // There are conflicts, so the `name.not_indexed3` has not been added
-        assertThat(Arrays.toString(mergeResult.buildConflicts()), mergeResult.hasConflicts(), equalTo(true));
-
-        assertNotSame(IndexOptions.NONE, docMapper.mappers().getMapper("name").fieldType().indexOptions());
-        assertThat(mergeResult.buildConflicts()[0], equalTo("mapper [name] has different [index] values"));
-        assertThat(mergeResult.buildConflicts()[1], equalTo("mapper [name] has different [store] values"));
-
-        // There are conflicts, but the `name.not_indexed3` has been added, b/c that field has no conflicts
         assertNotSame(IndexOptions.NONE, docMapper.mappers().getMapper("name").fieldType().indexOptions());
         assertThat(docMapper.mappers().getMapper("name.indexed"), notNullValue());
         assertThat(docMapper.mappers().getMapper("name.not_indexed"), notNullValue());
         assertThat(docMapper.mappers().getMapper("name.not_indexed2"), notNullValue());
-        assertThat(docMapper.mappers().getMapper("name.not_indexed3"), notNullValue());
+        assertThat(docMapper.mappers().getMapper("name.not_indexed3"), nullValue());
     }
 }

core/src/test/java/org/elasticsearch/index/mapper/string/SimpleStringMappingTests.java

@@ -25,6 +25,7 @@ import org.apache.lucene.index.IndexableField;
 import org.apache.lucene.index.IndexableFieldType;
 import org.elasticsearch.Version;
 import org.elasticsearch.cluster.metadata.IndexMetaData;
+import org.elasticsearch.common.compress.CompressedXContent;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.xcontent.ToXContent;
 import org.elasticsearch.common.xcontent.XContentBuilder;
@@ -478,7 +479,7 @@ public class SimpleStringMappingTests extends ESSingleNodeTestCase {
                 .startObject("properties").startObject("field").field("type", "string").endObject().endObject()
                 .endObject().endObject().string();
 
-        DocumentMapper defaultMapper = parser.parse(mapping);
+        DocumentMapper defaultMapper = indexService.mapperService().merge("type", new CompressedXContent(mapping), true, false);
 
         ParsedDocument doc = defaultMapper.parse("test", "type", "1", XContentFactory.jsonBuilder()
                 .startObject()
@@ -507,10 +508,12 @@ public class SimpleStringMappingTests extends ESSingleNodeTestCase {
         updatedMapping = XContentFactory.jsonBuilder().startObject().startObject("type")
                 .startObject("properties").startObject("field").field("type", "string").startObject("norms").field("enabled", true).endObject()
                 .endObject().endObject().endObject().endObject().string();
-        mergeResult = defaultMapper.merge(parser.parse(updatedMapping).mapping(), true, false);
+        try {
-        assertTrue(mergeResult.hasConflicts());
+            defaultMapper.merge(parser.parse(updatedMapping).mapping(), true, false);
-        assertEquals(1, mergeResult.buildConflicts().length);
+            fail();
-        assertTrue(mergeResult.buildConflicts()[0].contains("different [omit_norms]"));
+        } catch (IllegalArgumentException e) {
+            assertThat(e.getMessage(), containsString("different [omit_norms]"));
+        }
     }
 
     /**

core/src/test/java/org/elasticsearch/index/mapper/timestamp/TimestampMappingTests.java

@@ -41,6 +41,7 @@ import org.elasticsearch.index.mapper.DocumentMapper;
 import org.elasticsearch.index.mapper.DocumentMapperParser;
 import org.elasticsearch.index.mapper.MappedFieldType;
 import org.elasticsearch.index.mapper.MapperParsingException;
+import org.elasticsearch.index.mapper.MapperService;
 import org.elasticsearch.index.mapper.MergeResult;
 import org.elasticsearch.index.mapper.ParsedDocument;
 import org.elasticsearch.index.mapper.SourceToParse;
@@ -557,7 +558,6 @@ public class TimestampMappingTests extends ESSingleNodeTestCase {
     public void testMergingConflicts() throws Exception {
         String mapping = XContentFactory.jsonBuilder().startObject().startObject("type")
                 .startObject("_timestamp").field("enabled", true)
-                .startObject("fielddata").field("format", "doc_values").endObject()
                 .field("store", "yes")
                 .field("index", "analyzed")
                 .field("path", "foo")
@@ -565,9 +565,9 @@ public class TimestampMappingTests extends ESSingleNodeTestCase {
                 .endObject()
                 .endObject().endObject().string();
         Settings indexSettings = Settings.builder().put(IndexMetaData.SETTING_VERSION_CREATED, Version.V_1_4_2.id).build();
-        DocumentMapperParser parser = createIndex("test", indexSettings).mapperService().documentMapperParser();
+        MapperService mapperService = createIndex("test", indexSettings).mapperService();
 
-        DocumentMapper docMapper = parser.parse(mapping);
+        DocumentMapper docMapper = mapperService.merge("type", new CompressedXContent(mapping), true, false);
         assertThat(docMapper.timestampFieldMapper().fieldType().fieldDataType().getLoading(), equalTo(MappedFieldType.Loading.LAZY));
         mapping = XContentFactory.jsonBuilder().startObject().startObject("type")
                 .startObject("_timestamp").field("enabled", false)
@@ -579,20 +579,32 @@ public class TimestampMappingTests extends ESSingleNodeTestCase {
                 .endObject()
                 .endObject().endObject().string();
 
-        MergeResult mergeResult = docMapper.merge(parser.parse(mapping).mapping(), true, false);
+        try {
-        List<String> expectedConflicts = new ArrayList<>(Arrays.asList(
+            mapperService.merge("type", new CompressedXContent(mapping), false, false);
-            "mapper [_timestamp] has different [index] values",
+            fail();
-            "mapper [_timestamp] has different [store] values",
+        } catch (IllegalArgumentException e) {
-            "Cannot update default in _timestamp value. Value is 1970-01-01 now encountering 1970-01-02",
+            assertThat(e.getMessage(), containsString("mapper [_timestamp] has different [index] values"));
-            "Cannot update path in _timestamp value. Value is foo path in merged mapping is bar"));
+            assertThat(e.getMessage(), containsString("mapper [_timestamp] has different [store] values"));
-
-        for (String conflict : mergeResult.buildConflicts()) {
-            assertTrue("found unexpected conflict [" + conflict + "]", expectedConflicts.remove(conflict));
         }
-        assertTrue("missing conflicts: " + Arrays.toString(expectedConflicts.toArray()), expectedConflicts.isEmpty());
+
         assertThat(docMapper.timestampFieldMapper().fieldType().fieldDataType().getLoading(), equalTo(MappedFieldType.Loading.LAZY));
         assertTrue(docMapper.timestampFieldMapper().enabled());
-        assertThat(docMapper.timestampFieldMapper().fieldType().fieldDataType().getFormat(indexSettings), equalTo("doc_values"));
+
+        mapping = XContentFactory.jsonBuilder().startObject().startObject("type")
+                .startObject("_timestamp").field("enabled", true)
+                .field("store", "yes")
+                .field("index", "analyzed")
+                .field("path", "bar")
+                .field("default", "1970-01-02")
+                .endObject()
+                .endObject().endObject().string();
+        try {
+            mapperService.merge("type", new CompressedXContent(mapping), false, false);
+            fail();
+        } catch (IllegalArgumentException e) {
+            assertThat(e.getMessage(), containsString("Cannot update default in _timestamp value. Value is 1970-01-01 now encountering 1970-01-02"));
+            assertThat(e.getMessage(), containsString("Cannot update path in _timestamp value. Value is foo path in merged mapping is bar"));
+        }
     }
 
     public void testBackcompatMergingConflictsForIndexValues() throws Exception {

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

@@ -48,7 +48,7 @@ public class UpdateMappingOnClusterIT extends ESIntegTestCase {
     public void testAllConflicts() throws Exception {
         String mapping = copyToStringFromClasspath("/org/elasticsearch/index/mapper/update/all_mapping_create_index.json");
         String mappingUpdate = copyToStringFromClasspath("/org/elasticsearch/index/mapper/update/all_mapping_update_with_conflicts.json");
-        String[] errorMessage = {"[_all] enabled is true now encountering false",
+        String[] errorMessage = {
                 "[_all] has different [omit_norms] values",
                 "[_all] has different [store] values",
                 "[_all] has different [store_term_vector] values",
@@ -61,6 +61,13 @@ public class UpdateMappingOnClusterIT extends ESIntegTestCase {
         testConflict(mapping, mappingUpdate, errorMessage);
     }
 
+    public void testAllDisabled() throws Exception {
+        XContentBuilder mapping = jsonBuilder().startObject().startObject("mappings").startObject(TYPE).startObject("_all").field("enabled", true).endObject().endObject().endObject().endObject();
+        XContentBuilder mappingUpdate = jsonBuilder().startObject().startObject("_all").field("enabled", false).endObject().startObject("properties").startObject("text").field("type", "string").endObject().endObject().endObject();
+        String errorMessage = "[_all] enabled is true now encountering false";
+        testConflict(mapping.string(), mappingUpdate.string(), errorMessage);
+    }
+
     public void testAllWithDefault() throws Exception {
         String defaultMapping = jsonBuilder().startObject().startObject("_default_")
                 .startObject("_all")

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

@@ -123,14 +123,14 @@ public class UpdateMappingTests extends ESSingleNodeTestCase {
             mapperService.merge("type", new CompressedXContent(update.string()), false, false);
             fail();
         } catch (IllegalArgumentException e) {
-            assertThat(e.getMessage(), containsString("Merge failed"));
+            assertThat(e.getMessage(), containsString("mapper [foo] cannot be changed from type [long] to [double]"));
         }
 
         try {
             mapperService.merge("type", new CompressedXContent(update.string()), false, false);
             fail();
         } catch (IllegalArgumentException e) {
-            assertThat(e.getMessage(), containsString("Merge failed"));
+            assertThat(e.getMessage(), containsString("mapper [foo] cannot be changed from type [long] to [double]"));
         }
 
         assertTrue(mapperService.documentMapper("type").mapping().root().getMapper("foo") instanceof LongFieldMapper);
@@ -167,7 +167,6 @@ public class UpdateMappingTests extends ESSingleNodeTestCase {
     }
 
     // same as the testConflictNewType except that the mapping update is on an existing type
-    @AwaitsFix(bugUrl="https://github.com/elastic/elasticsearch/issues/15049")
     public void testConflictNewTypeUpdate() throws Exception {
         XContentBuilder mapping1 = XContentFactory.jsonBuilder().startObject().startObject("type1")
                 .startObject("properties").startObject("foo").field("type", "long").endObject()

core/src/test/java/org/elasticsearch/indices/mapping/UpdateMappingIntegrationIT.java

@@ -140,7 +140,7 @@ public class UpdateMappingIntegrationIT extends ESIntegTestCase {
                     .setSource("{\"type\":{\"properties\":{\"body\":{\"type\":\"integer\"}}}}").execute().actionGet();
             fail("Expected MergeMappingException");
         } catch (IllegalArgumentException e) {
-            assertThat(e.getMessage(), containsString("mapper [body] of different type"));
+            assertThat(e.getMessage(), containsString("mapper [body] cannot be changed from type [string] to [int]"));
         }
     }