From aed1f68e494c65ad50b98a3e0a7a2b6a794b2965 Mon Sep 17 00:00:00 2001
From: Ryan Ernst <ryan@iernst.net>
Date: Mon, 6 Jul 2015 18:22:06 -0700
Subject: [PATCH] Mappings: Enforce field names do not contain dot

Field names containing dots can cause problems. For example, @jpountz
made this recreation which cause no error, but can result in a
serialization exception if the type already exists:
https://gist.github.com/jpountz/8c66817e00a322b81f85

But this is not just a potential conflict. It also has larger problems,
since only the leaf mapper is created. The intermediate "foo" object
field would not exist if only "foo.bar" was in the mappings.

This change forbids the use of dots in field names. It also
fixes an issue with passing through the update_all_types setting,
which was always set to true whenever a type already existed (!).

I do not think we should worry about backwards compatibility here. This
should be a hard break (and added to the migration plugin).
---
 .../index/mapper/DocumentMapper.java            |  6 +++---
 .../index/mapper/object/ObjectMapper.java       | 17 ++++++++++-------
 .../index/mapper/simple/SimpleMapperTests.java  | 15 +++++++++++++++
 3 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/core/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java b/core/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java
index de201941ed7f7..1a847997e8429 100644
--- a/core/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java
+++ b/core/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java
@@ -399,10 +399,10 @@ public boolean isParent(String type) {
         return mapperService.getParentTypes().contains(type);
     }
 
-    private void addMappers(Collection<ObjectMapper> objectMappers, Collection<FieldMapper> fieldMappers) {
+    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, true);
+        mapperService.checkNewMappersCompatibility(objectMappers, fieldMappers, updateAllTypes);
 
         // update mappers for this document type
         MapBuilder<String, ObjectMapper> builder = MapBuilder.newMapBuilder(this.objectMappers);
@@ -424,7 +424,7 @@ public MergeResult merge(Mapping mapping, boolean simulate, boolean updateAllTyp
             final MergeResult mergeResult = new MergeResult(simulate, updateAllTypes);
             this.mapping.merge(mapping, mergeResult);
             if (simulate == false) {
-                addMappers(mergeResult.getNewObjectMappers(), mergeResult.getNewFieldMappers());
+                addMappers(mergeResult.getNewObjectMappers(), mergeResult.getNewFieldMappers(), updateAllTypes);
                 refreshSource();
             }
             return mergeResult;
diff --git a/core/src/main/java/org/elasticsearch/index/mapper/object/ObjectMapper.java b/core/src/main/java/org/elasticsearch/index/mapper/object/ObjectMapper.java
index 12f0319f32a57..30592f4e9fe23 100644
--- a/core/src/main/java/org/elasticsearch/index/mapper/object/ObjectMapper.java
+++ b/core/src/main/java/org/elasticsearch/index/mapper/object/ObjectMapper.java
@@ -278,7 +278,10 @@ protected static void parseProperties(ObjectMapper.Builder objBuilder, Map<Strin
             Iterator<Map.Entry<String, Object>> iterator = propsNode.entrySet().iterator();
             while (iterator.hasNext()) {
                 Map.Entry<String, Object> entry = iterator.next();
-                String propName = entry.getKey();
+                String fieldName = entry.getKey();
+                if (fieldName.contains(".")) {
+                    throw new MapperParsingException("Field name [" + fieldName + "] cannot contain '.'");
+                }
                 // Should accept empty arrays, as a work around for when the
                 // user can't provide an empty Map. (PHP for example)
                 boolean isEmptyList = entry.getValue() instanceof List && ((List<?>) entry.getValue()).isEmpty();
@@ -301,23 +304,23 @@ protected static void parseProperties(ObjectMapper.Builder objBuilder, Map<Strin
                             // any type, including core values, which
                             type = ObjectMapper.CONTENT_TYPE;
                         } else {
-                            throw new MapperParsingException("No type specified for property [" + propName + "]");
+                            throw new MapperParsingException("No type specified for field [" + fieldName + "]");
                         }
                     }
 
                     Mapper.TypeParser typeParser = parserContext.typeParser(type);
                     if (typeParser == null) {
-                        throw new MapperParsingException("No handler for type [" + type + "] declared on field [" + propName + "]");
+                        throw new MapperParsingException("No handler for type [" + type + "] declared on field [" + fieldName + "]");
                     }
-                    objBuilder.add(typeParser.parse(propName, propNode, parserContext));
+                    objBuilder.add(typeParser.parse(fieldName, propNode, parserContext));
                     propNode.remove("type");
-                    DocumentMapperParser.checkNoRemainingFields(propName, propNode, parserContext.indexVersionCreated());
+                    DocumentMapperParser.checkNoRemainingFields(fieldName, propNode, parserContext.indexVersionCreated());
                     iterator.remove();
                 } else if (isEmptyList) {
                     iterator.remove();
                 } else {
-                    throw new MapperParsingException("Expected map for property [fields] on field [" + propName + "] but got a "
-                            + propName.getClass());
+                    throw new MapperParsingException("Expected map for property [fields] on field [" + fieldName + "] but got a "
+                            + fieldName.getClass());
                 }
             }
 
diff --git a/core/src/test/java/org/elasticsearch/index/mapper/simple/SimpleMapperTests.java b/core/src/test/java/org/elasticsearch/index/mapper/simple/SimpleMapperTests.java
index 50815f55f3815..cf50a1d819e67 100644
--- a/core/src/test/java/org/elasticsearch/index/mapper/simple/SimpleMapperTests.java
+++ b/core/src/test/java/org/elasticsearch/index/mapper/simple/SimpleMapperTests.java
@@ -23,6 +23,7 @@
 import org.elasticsearch.common.bytes.BytesArray;
 import org.elasticsearch.common.bytes.BytesReference;
 import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.common.xcontent.XContentFactory;
 import org.elasticsearch.index.mapper.*;
 import org.elasticsearch.index.mapper.ParseContext.Document;
 import org.elasticsearch.index.IndexService;
@@ -135,4 +136,18 @@ public void testNoDocumentSent() throws Exception {
             assertThat(e.getMessage(), equalTo("failed to parse, document is empty"));
         }
     }
+
+    public void testHazardousFieldNames() throws Exception {
+        IndexService indexService = createIndex("test");
+        DocumentMapperParser mapperParser = indexService.mapperService().documentMapperParser();
+        String mapping = XContentFactory.jsonBuilder().startObject().startObject("type").startObject("properties")
+            .startObject("foo.bar").field("type", "string").endObject()
+            .endObject().endObject().string();
+        try {
+            mapperParser.parse(mapping);
+            fail("Mapping parse should have failed");
+        } catch (MapperParsingException e) {
+            assertTrue(e.getMessage(), e.getMessage().contains("cannot contain '.'"));
+        }
+    }
 }