Mapper: Upgrading a simple mapping to multi_field mapping fails on merge conflicts, closes #369.

modules/elasticsearch/src/main/java/org/elasticsearch/index/mapper/xcontent/XContentMultiFieldMapper.java

@@ -202,43 +202,53 @@ public class XContentMultiFieldMapper implements XContentMapper, XContentInclude
     }
 
     @Override public void merge(XContentMapper mergeWith, MergeContext mergeContext) throws MergeMappingException {
-        if (!(mergeWith instanceof XContentMultiFieldMapper)) {
+        if (!(mergeWith instanceof XContentMultiFieldMapper) && !(mergeWith instanceof XContentFieldMapper)) {
-            mergeContext.addConflict("Can't merge a non multi_field mapping [" + mergeWith.name() + "] with a multi_field mapping [" + name() + "]");
+            mergeContext.addConflict("Can't merge a non multi_field / non simple mapping [" + mergeWith.name() + "] with a multi_field mapping [" + name() + "]");
             return;
         }
-        XContentMultiFieldMapper mergeWithMultiField = (XContentMultiFieldMapper) mergeWith;
         synchronized (mutex) {
-            // merge the default mapper
+            if (mergeWith instanceof XContentFieldMapper) {
-            if (defaultMapper == null) {
+                // its a single field mapper, upgraded into a multi field mapper, just update the default mapper
-                if (mergeWithMultiField.defaultMapper != null) {
+                if (defaultMapper == null) {
                     if (!mergeContext.mergeFlags().simulate()) {
-                        defaultMapper = mergeWithMultiField.defaultMapper;
+                        defaultMapper = mergeWith;
                         mergeContext.docMapper().addFieldMapper((FieldMapper) defaultMapper);
                     }
                 }
             } else {
-                if (mergeWithMultiField.defaultMapper != null) {
+                XContentMultiFieldMapper mergeWithMultiField = (XContentMultiFieldMapper) mergeWith;
-                    defaultMapper.merge(mergeWithMultiField.defaultMapper, mergeContext);
+                // merge the default mapper
-                }
+                if (defaultMapper == null) {
-            }
+                    if (mergeWithMultiField.defaultMapper != null) {
-
+                        if (!mergeContext.mergeFlags().simulate()) {
-            // merge all the other mappers
+                            defaultMapper = mergeWithMultiField.defaultMapper;
-            for (XContentMapper mergeWithMapper : mergeWithMultiField.mappers.values()) {
+                            mergeContext.docMapper().addFieldMapper((FieldMapper) defaultMapper);
-                XContentMapper mergeIntoMapper = mappers.get(mergeWithMapper.name());
-                if (mergeIntoMapper == null) {
-                    // no mapping, simply add it if not simulating
-                    if (!mergeContext.mergeFlags().simulate()) {
-                        // disable the mapper from being in all, only the default mapper is in all
-                        if (mergeWithMapper instanceof XContentIncludeInAllMapper) {
-                            ((XContentIncludeInAllMapper) mergeWithMapper).includeInAll(false);
-                        }
-                        mappers = newMapBuilder(mappers).put(mergeWithMapper.name(), mergeWithMapper).immutableMap();
-                        if (mergeWithMapper instanceof XContentFieldMapper) {
-                            mergeContext.docMapper().addFieldMapper((FieldMapper) mergeWithMapper);
                         }
                     }
                 } else {
-                    mergeIntoMapper.merge(mergeWithMapper, mergeContext);
+                    if (mergeWithMultiField.defaultMapper != null) {
+                        defaultMapper.merge(mergeWithMultiField.defaultMapper, mergeContext);
+                    }
+                }
+
+                // merge all the other mappers
+                for (XContentMapper mergeWithMapper : mergeWithMultiField.mappers.values()) {
+                    XContentMapper mergeIntoMapper = mappers.get(mergeWithMapper.name());
+                    if (mergeIntoMapper == null) {
+                        // no mapping, simply add it if not simulating
+                        if (!mergeContext.mergeFlags().simulate()) {
+                            // disable the mapper from being in all, only the default mapper is in all
+                            if (mergeWithMapper instanceof XContentIncludeInAllMapper) {
+                                ((XContentIncludeInAllMapper) mergeWithMapper).includeInAll(false);
+                            }
+                            mappers = newMapBuilder(mappers).put(mergeWithMapper.name(), mergeWithMapper).immutableMap();
+                            if (mergeWithMapper instanceof XContentFieldMapper) {
+                                mergeContext.docMapper().addFieldMapper((FieldMapper) mergeWithMapper);
+                            }
+                        }
+                    } else {
+                        mergeIntoMapper.merge(mergeWithMapper, mergeContext);
+                    }
                 }
             }
         }

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

@@ -21,10 +21,14 @@ package org.elasticsearch.index.mapper.xcontent.multifield.merge;
 
 import org.apache.lucene.document.Document;
 import org.apache.lucene.document.Field;
+import org.elasticsearch.index.mapper.DocumentMapper;
 import org.elasticsearch.index.mapper.xcontent.XContentDocumentMapper;
+import org.elasticsearch.index.mapper.xcontent.XContentDocumentMapperParser;
 import org.elasticsearch.index.mapper.xcontent.XContentMapperTests;
 import org.testng.annotations.Test;
 
+import java.util.Arrays;
+
 import static org.elasticsearch.common.io.Streams.*;
 import static org.elasticsearch.index.mapper.DocumentMapper.MergeFlags.*;
 import static org.hamcrest.MatcherAssert.*;
@@ -38,7 +42,9 @@ public class JavaMultiFieldMergeTests {
 
     @Test public void testMergeMultiField() throws Exception {
         String mapping = copyToStringFromClasspath("/org/elasticsearch/index/mapper/xcontent/multifield/merge/test-mapping1.json");
-        XContentDocumentMapper docMapper = XContentMapperTests.newParser().parse(mapping);
+        XContentDocumentMapperParser parser = XContentMapperTests.newParser();
+
+        XContentDocumentMapper docMapper = parser.parse(mapping);
 
         assertThat(docMapper.mappers().fullName("name").mapper().indexed(), equalTo(true));
         assertThat(docMapper.mappers().fullName("name.indexed"), nullValue());
@@ -52,9 +58,10 @@ public class JavaMultiFieldMergeTests {
 
 
         mapping = copyToStringFromClasspath("/org/elasticsearch/index/mapper/xcontent/multifield/merge/test-mapping2.json");
-        XContentDocumentMapper docMapper2 = XContentMapperTests.newParser().parse(mapping);
+        XContentDocumentMapper docMapper2 = parser.parse(mapping);
 
-        docMapper.merge(docMapper2, mergeFlags().simulate(true));
+        DocumentMapper.MergeResult mergeResult = docMapper.merge(docMapper2, mergeFlags().simulate(true));
+        assertThat(Arrays.toString(mergeResult.conflicts()), mergeResult.hasConflicts(), equalTo(false));
 
         docMapper.merge(docMapper2, mergeFlags().simulate(false));
 
@@ -62,6 +69,9 @@ public class JavaMultiFieldMergeTests {
 
         assertThat(docMapper.mappers().fullName("name").mapper().indexed(), equalTo(true));
         assertThat(docMapper.mappers().fullName("name.indexed").mapper(), notNullValue());
+        assertThat(docMapper.mappers().fullName("name.not_indexed").mapper(), notNullValue());
+        assertThat(docMapper.mappers().fullName("name.not_indexed2"), nullValue());
+        assertThat(docMapper.mappers().fullName("name.not_indexed3"), nullValue());
 
         json = copyToBytesFromClasspath("/org/elasticsearch/index/mapper/xcontent/multifield/merge/test-data.json");
         doc = docMapper.parse(json).doc();
@@ -69,5 +79,38 @@ public class JavaMultiFieldMergeTests {
         assertThat(f, notNullValue());
         f = doc.getField("name.indexed");
         assertThat(f, notNullValue());
+
+        mapping = copyToStringFromClasspath("/org/elasticsearch/index/mapper/xcontent/multifield/merge/test-mapping3.json");
+        XContentDocumentMapper docMapper3 = parser.parse(mapping);
+
+        mergeResult = docMapper.merge(docMapper3, mergeFlags().simulate(true));
+        assertThat(Arrays.toString(mergeResult.conflicts()), mergeResult.hasConflicts(), equalTo(false));
+
+        docMapper.merge(docMapper3, mergeFlags().simulate(false));
+
+        assertThat(docMapper.mappers().name("name").mapper().indexed(), equalTo(true));
+
+        assertThat(docMapper.mappers().fullName("name").mapper().indexed(), equalTo(true));
+        assertThat(docMapper.mappers().fullName("name.indexed").mapper(), notNullValue());
+        assertThat(docMapper.mappers().fullName("name.not_indexed").mapper(), notNullValue());
+        assertThat(docMapper.mappers().fullName("name.not_indexed2").mapper(), notNullValue());
+        assertThat(docMapper.mappers().fullName("name.not_indexed3"), nullValue());
+
+
+        mapping = copyToStringFromClasspath("/org/elasticsearch/index/mapper/xcontent/multifield/merge/test-mapping4.json");
+        XContentDocumentMapper docMapper4 = parser.parse(mapping);
+
+        mergeResult = docMapper.merge(docMapper4, mergeFlags().simulate(true));
+        assertThat(Arrays.toString(mergeResult.conflicts()), mergeResult.hasConflicts(), equalTo(false));
+
+        docMapper.merge(docMapper4, mergeFlags().simulate(false));
+
+        assertThat(docMapper.mappers().name("name").mapper().indexed(), equalTo(true));
+
+        assertThat(docMapper.mappers().fullName("name").mapper().indexed(), equalTo(true));
+        assertThat(docMapper.mappers().fullName("name.indexed").mapper(), notNullValue());
+        assertThat(docMapper.mappers().fullName("name.not_indexed").mapper(), notNullValue());
+        assertThat(docMapper.mappers().fullName("name.not_indexed2").mapper(), notNullValue());
+        assertThat(docMapper.mappers().fullName("name.not_indexed3").mapper(), notNullValue());
     }
 }

modules/elasticsearch/src/test/java/org/elasticsearch/index/mapper/xcontent/multifield/merge/test-mapping3.json

@@ -0,0 +1,15 @@
+{
+    person : {
+        properties : {
+            "name" : {
+                type : "multi_field",
+                "fields" : {
+                    "name" : {type: "string", index : "analyzed", store : "yes"},
+                    "indexed" : {type: "string", index : "analyzed"},
+                    "not_indexed" : {type: "string", index : "no", store : "yes"},
+                    "not_indexed2" : {type: "string", index : "no", store : "yes"}
+                }
+            }
+        }
+    }
+}

modules/elasticsearch/src/test/java/org/elasticsearch/index/mapper/xcontent/multifield/merge/test-mapping4.json

@@ -0,0 +1,12 @@
+{
+    person : {
+        properties : {
+            "name" : {
+                type : "multi_field",
+                "fields" : {
+                    "not_indexed3" : {type: "string", index : "no", store : "yes"}
+                }
+            }
+        }
+    }
+}