Fail if an object is added after a field with the same name. #17568

Today we fail if you try to add a field and an object from another type already
has the same name. However, we do NOT fail if you insert the field first and the
object afterwards. This leads to bad bugs since mappings are not necessarily
parsed in the same order at recovery time, so a mapping update could succeed and
then you would fail to reopen the index.

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

@@ -361,6 +361,9 @@ public class MapperService extends AbstractIndexComponent implements Closeable {
     }
 
     private void checkFieldUniqueness(String type, Collection<ObjectMapper> objectMappers, Collection<FieldMapper> fieldMappers) {
+        assert Thread.holdsLock(this);
+
+        // first check within mapping
         final Set<String> objectFullNames = new HashSet<>();
         for (ObjectMapper objectMapper : objectMappers) {
             final String fullPath = objectMapper.fullPath();
@@ -378,13 +381,26 @@ public class MapperService extends AbstractIndexComponent implements Closeable {
                 throw new IllegalArgumentException("Field [" + name + "] is defined twice in [" + type + "]");
             }
         }
+
+        // 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 void checkObjectsCompatibility(String type, Collection<ObjectMapper> objectMappers, Collection<FieldMapper> fieldMappers, boolean updateAllTypes) {
         assert Thread.holdsLock(this);
 
-        checkFieldUniqueness(type, objectMappers, fieldMappers);
-
         for (ObjectMapper newObjectMapper : objectMappers) {
             ObjectMapper existingObjectMapper = fullPathObjectMappers.get(newObjectMapper.fullPath());
             if (existingObjectMapper != null) {
@@ -393,12 +409,6 @@ public class MapperService extends AbstractIndexComponent implements Closeable {
                 existingObjectMapper.merge(newObjectMapper, updateAllTypes);
             }
         }
-
-        for (FieldMapper fieldMapper : fieldMappers) {
-            if (fullPathObjectMappers.containsKey(fieldMapper.name())) {
-                throw new IllegalArgumentException("Field [" + fieldMapper.name() + "] is defined as a field in mapping [" + type + "] but this name is already used for an object in other types");
-            }
-        }
     }
 
     private void checkNestedFieldsLimit(Map<String, ObjectMapper> fullPathObjectMappers) {

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

@@ -29,6 +29,7 @@ import org.elasticsearch.common.xcontent.XContentFactory;
 import org.elasticsearch.index.IndexService;
 import org.elasticsearch.index.mapper.DocumentMapper;
 import org.elasticsearch.index.mapper.MapperService;
+import org.elasticsearch.index.mapper.MapperService.MergeReason;
 import org.elasticsearch.index.mapper.core.LongFieldMapper;
 import org.elasticsearch.plugins.Plugin;
 import org.elasticsearch.test.ESSingleNodeTestCase;
@@ -304,4 +305,37 @@ public class UpdateMappingTests extends ESSingleNodeTestCase {
         assertNotNull(response.getMappings().get("test2").get("type").getSourceAsMap().get("_timestamp"));
         assertTrue((Boolean)((LinkedHashMap)response.getMappings().get("test2").get("type").getSourceAsMap().get("_timestamp")).get("enabled"));
     }
+
+    public void testRejectFieldDefinedTwice() throws IOException {
+        String mapping1 = XContentFactory.jsonBuilder().startObject()
+                .startObject("type1")
+                    .startObject("properties")
+                        .startObject("foo")
+                            .field("type", "object")
+                        .endObject()
+                    .endObject()
+                .endObject().endObject().string();
+        String mapping2 = XContentFactory.jsonBuilder().startObject()
+                .startObject("type2")
+                    .startObject("properties")
+                        .startObject("foo")
+                            .field("type", "long")
+                        .endObject()
+                    .endObject()
+                .endObject().endObject().string();
+
+        MapperService mapperService1 = createIndex("test1").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));
+        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"));
+
+        MapperService mapperService2 = createIndex("test2").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));
+        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"));
+    }
 }

core/src/test/java/org/elasticsearch/search/functionscore/DecayFunctionScoreIT.java

@@ -663,7 +663,7 @@ public class DecayFunctionScoreIT extends ESIntegTestCase {
         ensureYellow();
         int numDocs = 2;
         client().index(
-                indexRequest("test").type("type1").source(
+                indexRequest("test").type("type").source(
                         jsonBuilder().startObject().field("test", "value").startObject("geo").field("lat", 1).field("lon", 2).endObject()
                                 .endObject())).actionGet();
         refresh();
@@ -674,7 +674,7 @@ public class DecayFunctionScoreIT extends ESIntegTestCase {
                 searchRequest().searchType(SearchType.QUERY_THEN_FETCH).source(
                         searchSource()
                                 .size(numDocs)
-                                .query(functionScoreQuery(termQuery("test", "value"), linearDecayFunction("type1.geo", lonlat, "1000km"))
+                                .query(functionScoreQuery(termQuery("test", "value"), linearDecayFunction("type.geo", lonlat, "1000km"))
                                         .scoreMode(FiltersFunctionScoreQuery.ScoreMode.MULTIPLY))));
         try {
             response.actionGet();