Merge pull request #15243 from jpountz/fix/field_uniqueness

Validate that fields are defined only once.

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

@@ -32,7 +32,6 @@ import org.apache.lucene.util.BytesRef;
 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;
@@ -92,7 +91,7 @@ public class MapperService extends AbstractIndexComponent implements Closeable {
     private final ReleasableLock mappingWriteLock = new ReleasableLock(mappingLock.writeLock());
 
     private volatile FieldTypeLookup fieldTypes;
-    private volatile ImmutableOpenMap<String, ObjectMapper> fullPathObjectMappers = ImmutableOpenMap.of();
+    private volatile Map<String, ObjectMapper> fullPathObjectMappers = new HashMap<>();
     private boolean hasNested = false; // updated dynamically to true when a nested object is added
 
     private final DocumentMapperParser documentParser;
@@ -293,8 +292,41 @@ public class MapperService extends AbstractIndexComponent implements Closeable {
         return true;
     }
 
+    private void checkFieldUniqueness(String type, Collection<ObjectMapper> objectMappers, Collection<FieldMapper> fieldMappers) {
+        final Set<String> objectFullNames = new HashSet<>();
+        for (ObjectMapper objectMapper : objectMappers) {
+            final String fullPath = objectMapper.fullPath();
+            if (objectFullNames.add(fullPath) == false) {
+                throw new IllegalArgumentException("Object mapper [" + fullPath + "] is defined twice in mapping for type [" + type + "]");
+            }
+        }
+
+        if (indexSettings.getIndexVersionCreated().before(Version.V_3_0_0)) {
+            // Before 3.0 some metadata mappers are also registered under the root object mapper
+            // So we avoid false positives by deduplicating mappers
+            // given that we check exact equality, this would still catch the case that a mapper
+            // is defined under the root object 
+            Collection<FieldMapper> uniqueFieldMappers = Collections.newSetFromMap(new IdentityHashMap<>());
+            uniqueFieldMappers.addAll(fieldMappers);
+            fieldMappers = uniqueFieldMappers;
+        }
+
+        final Set<String> fieldNames = new HashSet<>();
+        for (FieldMapper fieldMapper : fieldMappers) {
+            final String name = fieldMapper.name();
+            if (objectFullNames.contains(name)) {
+                throw new IllegalArgumentException("Field [" + name + "] is defined both as an object and a field in [" + type + "]");
+            } else if (fieldNames.add(name) == false) {
+                throw new IllegalArgumentException("Field [" + name + "] is defined twice in [" + type + "]");
+            }
+        }
+    }
+
     protected void checkMappersCompatibility(String type, Collection<ObjectMapper> objectMappers, Collection<FieldMapper> fieldMappers, boolean updateAllTypes) {
         assert mappingLock.isWriteLockedByCurrentThread();
+
+        checkFieldUniqueness(type, objectMappers, fieldMappers);
+
         for (ObjectMapper newObjectMapper : objectMappers) {
             ObjectMapper existingObjectMapper = fullPathObjectMappers.get(newObjectMapper.fullPath());
             if (existingObjectMapper != null) {
@@ -303,6 +335,13 @@ public class MapperService extends AbstractIndexComponent implements Closeable {
                 existingObjectMapper.merge(newObjectMapper, updateAllTypes);
             }
         }
+
+        for (FieldMapper fieldMapper : fieldMappers) {
+            if (fullPathObjectMappers.containsKey(fieldMapper.name())) {
+                throw new IllegalArgumentException("Field [{}] is defined as a field in mapping [" + fieldMapper.name() + "] but this name is already used for an object in other types");
+            }
+        }
+
         fieldTypes.checkCompatibility(type, fieldMappers, updateAllTypes);
     }
 
@@ -318,14 +357,14 @@ public class MapperService extends AbstractIndexComponent implements Closeable {
 
     protected void addMappers(String type, Collection<ObjectMapper> objectMappers, Collection<FieldMapper> fieldMappers) {
         assert mappingLock.isWriteLockedByCurrentThread();
-        ImmutableOpenMap.Builder<String, ObjectMapper> fullPathObjectMappers = ImmutableOpenMap.builder(this.fullPathObjectMappers);
+        Map<String, ObjectMapper> fullPathObjectMappers = new HashMap<>(this.fullPathObjectMappers);
         for (ObjectMapper objectMapper : objectMappers) {
             fullPathObjectMappers.put(objectMapper.fullPath(), objectMapper);
             if (objectMapper.nested().isNested()) {
                 hasNested = true;
             }
         }
-        this.fullPathObjectMappers = fullPathObjectMappers.build();
+        this.fullPathObjectMappers = Collections.unmodifiableMap(fullPathObjectMappers);
         this.fieldTypes = this.fieldTypes.copyAndAddAll(type, fieldMappers);
     }
 

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

@@ -27,10 +27,12 @@ import org.elasticsearch.index.mapper.object.RootObjectMapper;
 
 import java.io.IOException;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.Comparator;
 import java.util.HashMap;
-import java.util.List;
+import java.util.HashSet;
 import java.util.Map;
+import java.util.Set;
 
 import static java.util.Collections.emptyMap;
 import static java.util.Collections.unmodifiableMap;
@@ -41,7 +43,9 @@ import static java.util.Collections.unmodifiableMap;
  */
 public final class Mapping implements ToXContent {
 
-    public static final List<String> LEGACY_INCLUDE_IN_OBJECT = Arrays.asList("_all", "_id", "_parent", "_routing", "_timestamp", "_ttl");
+    // Set of fields that were included into the root object mapper before 2.0
+    public static final Set<String> LEGACY_INCLUDE_IN_OBJECT = Collections.unmodifiableSet(new HashSet<>(
+            Arrays.asList("_all", "_id", "_parent", "_routing", "_timestamp", "_ttl")));
 
     final Version indexCreated;
     final RootObjectMapper root;

core/src/test/java/org/elasticsearch/index/mapper/externalvalues/ExternalValuesMapperIntegrationIT.java

@@ -87,7 +87,7 @@ public class ExternalValuesMapperIntegrationIT extends ESIntegTestCase {
                 .startObject("f")
                     .field("type", ExternalMapperPlugin.EXTERNAL_UPPER)
                     .startObject("fields")
-                        .startObject("f")
+                        .startObject("g")
                             .field("type", "string")
                             .field("store", "yes")
                             .startObject("fields")
@@ -107,7 +107,7 @@ public class ExternalValuesMapperIntegrationIT extends ESIntegTestCase {
         refresh();
 
         SearchResponse response = client().prepareSearch("test-idx")
-                .setQuery(QueryBuilders.termQuery("f.f.raw", "FOO BAR"))
+                .setQuery(QueryBuilders.termQuery("f.g.raw", "FOO BAR"))
                 .execute().actionGet();
 
         assertThat(response.getHits().totalHits(), equalTo((long) 1));

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

@@ -202,6 +202,51 @@ public class UpdateMappingTests extends ESSingleNodeTestCase {
         assertNull(mapperService.documentMapper("type2").mapping().root().getMapper("foo"));
     }
 
+    public void testReuseMetaField() throws IOException {
+        XContentBuilder mapping = XContentFactory.jsonBuilder().startObject().startObject("type")
+                .startObject("properties").startObject("_id").field("type", "string").endObject()
+                .endObject().endObject().endObject();
+        MapperService mapperService = createIndex("test", Settings.settingsBuilder().build()).mapperService();
+
+        try {
+            mapperService.merge("type", new CompressedXContent(mapping.string()), false, false);
+            fail();
+        } catch (IllegalArgumentException e) {
+            assertTrue(e.getMessage().contains("Field [_id] is defined twice in [type]"));
+        }
+
+        try {
+            mapperService.merge("type", new CompressedXContent(mapping.string()), false, false);
+            fail();
+        } catch (IllegalArgumentException e) {
+            assertTrue(e.getMessage().contains("Field [_id] is defined twice in [type]"));
+        }
+    }
+
+    public void testReuseMetaFieldBackCompat() throws IOException {
+        XContentBuilder mapping = XContentFactory.jsonBuilder().startObject().startObject("type")
+                .startObject("properties").startObject("_id").field("type", "string").endObject()
+                .endObject().endObject().endObject();
+        // the logic is different for 2.x indices since they record some meta mappers (including _id)
+        // in the root object
+        Settings settings = Settings.builder().put(IndexMetaData.SETTING_VERSION_CREATED, Version.V_2_1_0).build();
+        MapperService mapperService = createIndex("test", settings).mapperService();
+
+        try {
+            mapperService.merge("type", new CompressedXContent(mapping.string()), false, false);
+            fail();
+        } catch (IllegalArgumentException e) {
+            assertTrue(e.getMessage().contains("Field [_id] is defined twice in [type]"));
+        }
+
+        try {
+            mapperService.merge("type", new CompressedXContent(mapping.string()), false, false);
+            fail();
+        } catch (IllegalArgumentException e) {
+            assertTrue(e.getMessage().contains("Field [_id] is defined twice in [type]"));
+        }
+    }
+
     public void testIndexFieldParsingBackcompat() throws IOException {
         IndexService indexService = createIndex("test", Settings.settingsBuilder().put(IndexMetaData.SETTING_VERSION_CREATED, Version.V_1_4_2.id).build());
         XContentBuilder indexMapping = XContentFactory.jsonBuilder();

modules/lang-groovy/src/test/java/org/elasticsearch/messy/tests/SearchFieldsTests.java

@@ -392,8 +392,7 @@ public class SearchFieldsTests extends ESIntegTestCase {
         createIndex("test");
         client().admin().cluster().prepareHealth().setWaitForEvents(Priority.LANGUID).setWaitForYellowStatus().execute().actionGet();
 
-        String mapping = XContentFactory.jsonBuilder().startObject().startObject("type1").startObject("properties")
+        String mapping = XContentFactory.jsonBuilder().startObject().startObject("type1").startObject("_source").field("enabled", false).endObject().startObject("properties")
-                .startObject("_source").field("enabled", false).endObject()
                 .startObject("byte_field").field("type", "byte").field("store", "yes").endObject()
                 .startObject("short_field").field("type", "short").field("store", "yes").endObject()
                 .startObject("integer_field").field("type", "integer").field("store", "yes").endObject()
@@ -556,8 +555,7 @@ public class SearchFieldsTests extends ESIntegTestCase {
         createIndex("test");
         client().admin().cluster().prepareHealth().setWaitForEvents(Priority.LANGUID).setWaitForYellowStatus().execute().actionGet();
 
-        String mapping = XContentFactory.jsonBuilder().startObject().startObject("type1").startObject("properties")
+        String mapping = XContentFactory.jsonBuilder().startObject().startObject("type1").startObject("_source").field("enabled", false).endObject().startObject("properties")
-                .startObject("_source").field("enabled", false).endObject()
                 .startObject("string_field").field("type", "string").endObject()
                 .startObject("byte_field").field("type", "byte").endObject()
                 .startObject("short_field").field("type", "short").endObject()