Fix merging of text field mapper (#40627)

On mapping updates the `text` field mapper does not update the field types for the underlying prefix and phrase fields. In practice this shouldn't be considered as a bug but we have an assert in the code that check that field types in the mapper service are identical to the ones present in field mappers.
elastic · Apr 1, 2019 · 7cc7912 · 7cc7912
1 parent 74bf4b8
commit 7cc7912
Show file tree

Hide file tree

Showing 2 changed files with 110 additions and 3 deletions.
diff --git a/server/src/main/java/org/elasticsearch/index/mapper/TextFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/TextFieldMapper.java
@@ -813,18 +813,33 @@ protected String contentType() {
         return CONTENT_TYPE;
     }
 
+    @Override
+    public FieldMapper updateFieldType(Map<String, MappedFieldType> fullNameToFieldType) {
+        TextFieldMapper mapper = (TextFieldMapper) super.updateFieldType(fullNameToFieldType);
+        if (mapper.prefixFieldMapper != null) {
+            mapper.prefixFieldMapper = (PrefixFieldMapper) mapper.prefixFieldMapper.updateFieldType(fullNameToFieldType);
+        }
+        if (mapper.phraseFieldMapper != null) {
+            mapper.phraseFieldMapper = (PhraseFieldMapper) mapper.phraseFieldMapper.updateFieldType(fullNameToFieldType);
+        }
+        return mapper;
+    }
+
     @Override
     protected void doMerge(Mapper mergeWith) {
         super.doMerge(mergeWith);
         TextFieldMapper mw = (TextFieldMapper) mergeWith;
+
         if (this.prefixFieldMapper != null && mw.prefixFieldMapper != null) {
             this.prefixFieldMapper = (PrefixFieldMapper) this.prefixFieldMapper.merge(mw.prefixFieldMapper);
-        }
-        else if (this.prefixFieldMapper != null || mw.prefixFieldMapper != null) {
+        } else if (this.prefixFieldMapper != null || mw.prefixFieldMapper != null) {
             throw new IllegalArgumentException("mapper [" + name() + "] has different index_prefix settings, current ["
                 + this.prefixFieldMapper + "], merged [" + mw.prefixFieldMapper + "]");
         }
-        else if (this.fieldType().indexPhrases != mw.fieldType().indexPhrases) {
+
+        if (this.phraseFieldMapper != null && mw.phraseFieldMapper != null) {
+            this.phraseFieldMapper = (PhraseFieldMapper) this.phraseFieldMapper.merge(mw.phraseFieldMapper);
+        } else if (this.fieldType().indexPhrases != mw.fieldType().indexPhrases) {
             throw new IllegalArgumentException("mapper [" + name() + "] has different index_phrases settings, current ["
                 + this.fieldType().indexPhrases + "], merged [" + mw.fieldType().indexPhrases + "]");
         }

diff --git a/server/src/test/java/org/elasticsearch/index/mapper/TextFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/TextFieldMapperTests.java
@@ -78,6 +78,7 @@
 
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.instanceOf;
 import static org.hamcrest.core.Is.is;
 
 public class TextFieldMapperTests extends ESSingleNodeTestCase {
@@ -1084,4 +1085,95 @@ public void testFastPhrasePrefixes() throws IOException {
             assertThat(q, equalTo(mpq));
         }
     }
+
+    public void testSimpleMerge() throws IOException {
+        MapperService mapperService = createIndex("test_mapping_merge").mapperService();
+        {
+            String mapping = Strings.toString(XContentFactory.jsonBuilder().startObject()
+                .startObject("_doc")
+                    .startObject("properties")
+                        .startObject("a_field")
+                            .field("type", "text")
+                            .startObject("index_prefixes").endObject()
+                            .field("index_phrases", true)
+                        .endObject()
+                    .endObject()
+                .endObject().endObject());
+            DocumentMapper mapper = mapperService.merge("_doc",
+                new CompressedXContent(mapping), MapperService.MergeReason.MAPPING_UPDATE);
+            assertThat(mapper.mappers().getMapper("a_field"), instanceOf(TextFieldMapper.class));
+        }
+
+        {
+            String mapping = Strings.toString(XContentFactory.jsonBuilder().startObject()
+                .startObject("_doc")
+                    .startObject("properties")
+                        .startObject("a_field")
+                            .field("type", "text")
+                            .startObject("index_prefixes").endObject()
+                            .field("index_phrases", true)
+                        .endObject()
+                    .endObject()
+                .endObject().endObject());
+            DocumentMapper mapper = mapperService.merge("_doc",
+                new CompressedXContent(mapping), MapperService.MergeReason.MAPPING_UPDATE);
+            assertThat(mapper.mappers().getMapper("a_field"), instanceOf(TextFieldMapper.class));
+        }
+
+        {
+            String mapping = Strings.toString(XContentFactory.jsonBuilder().startObject()
+                .startObject("_doc")
+                    .startObject("properties")
+                        .startObject("a_field")
+                            .field("type", "text")
+                                .startObject("index_prefixes")
+                                    .field("min_chars", "3")
+                                .endObject()
+                                .field("index_phrases", true)
+                            .endObject()
+                    .endObject()
+                .endObject().endObject());
+            IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
+                () -> mapperService.merge("_doc",
+                    new CompressedXContent(mapping), MapperService.MergeReason.MAPPING_UPDATE));
+            assertThat(e.getMessage(), containsString("different [index_prefixes]"));
+        }
+
+        {
+            String mapping = Strings.toString(XContentFactory.jsonBuilder().startObject()
+                .startObject("_doc")
+                    .startObject("properties")
+                        .startObject("a_field")
+                            .field("type", "text")
+                            .startObject("index_prefixes").endObject()
+                            .field("index_phrases", false)
+                        .endObject()
+                    .endObject()
+                .endObject().endObject());
+            IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
+                () -> mapperService.merge("_doc",
+                    new CompressedXContent(mapping), MapperService.MergeReason.MAPPING_UPDATE));
+            assertThat(e.getMessage(), containsString("different [index_phrases]"));
+        }
+
+        {
+            String mapping = Strings.toString(XContentFactory.jsonBuilder().startObject()
+                .startObject("_doc")
+                    .startObject("properties")
+                        .startObject("a_field")
+                            .field("type", "text")
+                            .startObject("index_prefixes").endObject()
+                            .field("index_phrases", true)
+                        .endObject()
+                        .startObject("b_field")
+                            .field("type", "keyword")
+                        .endObject()
+                    .endObject()
+                .endObject().endObject());
+            DocumentMapper mapper = mapperService.merge("_doc",
+                new CompressedXContent(mapping), MapperService.MergeReason.MAPPING_UPDATE);
+            assertThat(mapper.mappers().getMapper("a_field"), instanceOf(TextFieldMapper.class));
+            assertThat(mapper.mappers().getMapper("b_field"), instanceOf(KeywordFieldMapper.class));
+        }
+    }
 }