Move merge compatibility logic from MappedFieldType to FieldMapper (#56915)

Merging logic is currently split between FieldMapper, with its merge() method, and
MappedFieldType, which checks for merging compatibility. The compatibility checks
are called from a third class, MappingMergeValidator. This makes it difficult to reason
about what is or is not compatible in updates, and even what is in fact updateable - we
have a number of tests that check compatibility on changes in mapping configuration
that are not in fact possible.

This commit refactors the compatibility logic so that it all sits on FieldMapper, and
makes it called at merge time. It adds a new FieldMapperTestCase base class that
FieldMapper tests can extend, and moves the compatibility testing machinery from
FieldTypeTestCase to here.

Relates to #56814

Author: romseygeek

modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/RankFeatureFieldMapper.java

@@ -134,14 +134,6 @@ public int hashCode() {
             return h;
         }
 
-        @Override
-        public void checkCompatibility(MappedFieldType other, List<String> conflicts) {
-            super.checkCompatibility(other, conflicts);
-            if (positiveScoreImpact != ((RankFeatureFieldType) other).positiveScoreImpact()) {
-                conflicts.add("mapper [" + name() + "] has different [positive_score_impact] values");
-            }
-        }
-
         @Override
         public String typeName() {
             return CONTENT_TYPE;
@@ -230,4 +222,12 @@ protected void doXContentBody(XContentBuilder builder, boolean includeDefaults,
             builder.field("positive_score_impact", fieldType().positiveScoreImpact());
         }
     }
+
+    @Override
+    protected void mergeOptions(FieldMapper other, List<String> conflicts) {
+        RankFeatureFieldType ft = (RankFeatureFieldType) other.fieldType();
+        if (fieldType().positiveScoreImpact != ft.positiveScoreImpact()) {
+            conflicts.add("mapper [" + name() + "] has different [positive_score_impact] values");
+        }
+    }
 }

modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/RankFeaturesFieldMapper.java

@@ -29,6 +29,7 @@
 import org.elasticsearch.index.query.QueryShardContext;
 
 import java.io.IOException;
+import java.util.List;
 import java.util.Map;
 
 /**
@@ -127,6 +128,11 @@ protected RankFeaturesFieldMapper clone() {
         return (RankFeaturesFieldMapper) super.clone();
     }
 
+    @Override
+    protected void mergeOptions(FieldMapper other, List<String> conflicts) {
+
+    }
+
     @Override
     public RankFeaturesFieldType fieldType() {
         return (RankFeaturesFieldType) super.fieldType();

modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/ScaledFloatFieldMapper.java

@@ -213,14 +213,6 @@ public String typeName() {
             return CONTENT_TYPE;
         }
 
-        @Override
-        public void checkCompatibility(MappedFieldType other, List<String> conflicts) {
-            super.checkCompatibility(other, conflicts);
-            if (scalingFactor != ((ScaledFloatFieldType) other).getScalingFactor()) {
-                conflicts.add("mapper [" + name() + "] has different [scaling_factor] values");
-            }
-        }
-
         @Override
         public Query existsQuery(QueryShardContext context) {
             if (hasDocValues()) {
@@ -450,14 +442,17 @@ protected void parseCreateField(ParseContext context) throws IOException {
     }
 
     @Override
-    protected void doMerge(Mapper mergeWith) {
-        super.doMerge(mergeWith);
-        ScaledFloatFieldMapper other = (ScaledFloatFieldMapper) mergeWith;
-        if (other.ignoreMalformed.explicit()) {
-            this.ignoreMalformed = other.ignoreMalformed;
-        }
-        if (other.coerce.explicit()) {
-            this.coerce = other.coerce;
+    protected void mergeOptions(FieldMapper other, List<String> conflicts) {
+        ScaledFloatFieldMapper mergeWith = (ScaledFloatFieldMapper) other;
+        ScaledFloatFieldType ft = (ScaledFloatFieldType) other.fieldType();
+        if (fieldType().scalingFactor != ft.getScalingFactor()) {
+            conflicts.add("mapper [" + name() + "] has different [scaling_factor] values");
+        }
+        if (mergeWith.ignoreMalformed.explicit()) {
+            this.ignoreMalformed = mergeWith.ignoreMalformed;
+        }
+        if (mergeWith.coerce.explicit()) {
+            this.coerce = mergeWith.coerce;
         }
     }
 

modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/SearchAsYouTypeFieldMapper.java

@@ -343,21 +343,6 @@ public SpanQuery spanPrefixQuery(String value, SpanMultiTermQueryWrapper.SpanRew
             }
         }
 
-        @Override
-        public void checkCompatibility(MappedFieldType other, List<String> conflicts) {
-            super.checkCompatibility(other, conflicts);
-            final SearchAsYouTypeFieldType otherFieldType = (SearchAsYouTypeFieldType) other;
-            if (this.shingleFields.length != otherFieldType.shingleFields.length) {
-                conflicts.add("mapper [" + name() + "] has a different [max_shingle_size]");
-            } else if (Arrays.equals(this.shingleFields, otherFieldType.shingleFields) == false) {
-                conflicts.add("mapper [" + name() + "] has shingle subfields that are configured differently");
-            }
-
-            if (Objects.equals(this.prefixField, otherFieldType.prefixField) == false) {
-                conflicts.add("mapper [" + name() + "] has different [index_prefixes] settings");
-            }
-        }
-
         @Override
         public boolean equals(Object otherObject) {
             if (this == otherObject) {
@@ -488,6 +473,11 @@ protected void parseCreateField(ParseContext context) {
             throw new UnsupportedOperationException();
         }
 
+        @Override
+        protected void mergeOptions(FieldMapper other, List<String> conflicts) {
+
+        }
+
         @Override
         protected String contentType() {
             return "prefix";
@@ -515,6 +505,11 @@ protected void parseCreateField(ParseContext context) {
             throw new UnsupportedOperationException();
         }
 
+        @Override
+        protected void mergeOptions(FieldMapper other, List<String> conflicts) {
+
+        }
+
         @Override
         protected String contentType() {
             return "shingle";
@@ -613,18 +608,6 @@ public SpanQuery spanPrefixQuery(String value, SpanMultiTermQueryWrapper.SpanRew
             }
         }
 
-        @Override
-        public void checkCompatibility(MappedFieldType other, List<String> conflicts) {
-            super.checkCompatibility(other, conflicts);
-            ShingleFieldType ft = (ShingleFieldType) other;
-            if (ft.shingleSize != this.shingleSize) {
-                conflicts.add("mapper [" + name() + "] has different [shingle_size] values");
-            }
-            if (Objects.equals(this.prefixFieldType, ft.prefixFieldType) == false) {
-                conflicts.add("mapper [" + name() + "] has different [index_prefixes] settings");
-            }
-        }
-
         @Override
         public boolean equals(Object otherObject) {
             if (this == otherObject) {
@@ -698,18 +681,15 @@ protected String contentType() {
     }
 
     @Override
-    protected void doMerge(Mapper mergeWith) {
-        super.doMerge(mergeWith);
-        SearchAsYouTypeFieldMapper mw = (SearchAsYouTypeFieldMapper) mergeWith;
-        if (mw.maxShingleSize != maxShingleSize) {
-            throw new IllegalArgumentException("mapper [" + name() + "] has different [max_shingle_size] setting, current ["
-                + this.maxShingleSize + "], merged [" + mw.maxShingleSize + "]");
-        }
-        this.prefixField = (PrefixFieldMapper) this.prefixField.merge(mw.prefixField);
-
-        ShingleFieldMapper[] shingleFieldMappers = new ShingleFieldMapper[mw.shingleFields.length];
-        for (int i = 0; i < shingleFieldMappers.length; i++) {
-            this.shingleFields[i] = (ShingleFieldMapper) this.shingleFields[i].merge(mw.shingleFields[i]);
+    protected void mergeOptions(FieldMapper other, List<String> conflicts) {
+        final SearchAsYouTypeFieldMapper m = (SearchAsYouTypeFieldMapper) other;
+        if (this.shingleFields.length != m.shingleFields.length) {
+            conflicts.add("mapper [" + name() + "] has a different [max_shingle_size]");
+        } else {
+            this.prefixField = (PrefixFieldMapper) this.prefixField.merge(m.prefixField);
+            for (int i = 0; i < m.shingleFields.length; i++) {
+                this.shingleFields[i] = (ShingleFieldMapper) this.shingleFields[i].merge(m.shingleFields[i]);
+            }
         }
     }
 

modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/TokenCountFieldMapper.java

@@ -29,6 +29,7 @@
 
 import java.io.IOException;
 import java.util.Iterator;
+import java.util.List;
 import java.util.Map;
 
 import static org.elasticsearch.common.xcontent.support.XContentMapValues.nodeBooleanValue;
@@ -200,10 +201,9 @@ protected String contentType() {
     }
 
     @Override
-    protected void doMerge(Mapper mergeWith) {
-        super.doMerge(mergeWith);
-        this.analyzer = ((TokenCountFieldMapper) mergeWith).analyzer;
-        this.enablePositionIncrements = ((TokenCountFieldMapper) mergeWith).enablePositionIncrements;
+    protected void mergeOptions(FieldMapper other, List<String> conflicts) {
+        this.analyzer = ((TokenCountFieldMapper) other).analyzer;
+        this.enablePositionIncrements = ((TokenCountFieldMapper) other).enablePositionIncrements;
     }
 
     @Override

modules/mapper-extras/src/test/java/org/elasticsearch/index/mapper/RankFeatureFieldMapperTests.java

@@ -30,15 +30,14 @@
 import org.elasticsearch.common.xcontent.XContentType;
 import org.elasticsearch.index.IndexService;
 import org.elasticsearch.plugins.Plugin;
-import org.elasticsearch.test.ESSingleNodeTestCase;
 import org.hamcrest.Matchers;
 import org.junit.Before;
 
 import java.io.IOException;
 import java.util.Arrays;
 import java.util.Collection;
 
-public class RankFeatureFieldMapperTests extends ESSingleNodeTestCase {
+public class RankFeatureFieldMapperTests extends FieldMapperTestCase<RankFeatureFieldMapper.Builder> {
 
     IndexService indexService;
     DocumentMapperParser parser;
@@ -47,6 +46,10 @@ public class RankFeatureFieldMapperTests extends ESSingleNodeTestCase {
     public void setup() {
         indexService = createIndex("test");
         parser = indexService.mapperService().documentMapperParser();
+        addModifier("positive_score_impact", false, (a, b) -> {
+            a.fieldType().setPositiveScoreImpact(true);
+            b.fieldType().setPositiveScoreImpact(false);
+        });
     }
 
     @Override
@@ -63,6 +66,11 @@ static int getFrequency(TokenStream tk) throws IOException {
         return freq;
     }
 
+    @Override
+    protected RankFeatureFieldMapper.Builder newBuilder() {
+        return new RankFeatureFieldMapper.Builder("rank-feature");
+    }
+
     public void testDefaults() throws Exception {
         String mapping = Strings.toString(XContentFactory.jsonBuilder().startObject().startObject("type")
                 .startObject("properties").startObject("field").field("type", "rank_feature").endObject().endObject()
@@ -171,4 +179,5 @@ public void testRejectMultiValuedFields() throws MapperParsingException, IOExcep
         assertEquals("[rank_feature] fields do not support indexing multiple values for the same field [foo.field] in the same document",
                 e.getCause().getMessage());
     }
+
 }

modules/mapper-extras/src/test/java/org/elasticsearch/index/mapper/RankFeatureFieldTypeTests.java

@@ -19,31 +19,13 @@
 
 package org.elasticsearch.index.mapper;
 
-import org.junit.Before;
-
-public class RankFeatureFieldTypeTests extends FieldTypeTestCase {
+public class RankFeatureFieldTypeTests extends FieldTypeTestCase<MappedFieldType> {
 
     @Override
     protected MappedFieldType createDefaultFieldType() {
         return new RankFeatureFieldMapper.RankFeatureFieldType();
     }
 
-    @Before
-    public void setupProperties() {
-        addModifier(new Modifier("positive_score_impact", false) {
-            @Override
-            public void modify(MappedFieldType ft) {
-                RankFeatureFieldMapper.RankFeatureFieldType tft = (RankFeatureFieldMapper.RankFeatureFieldType)ft;
-                tft.setPositiveScoreImpact(tft.positiveScoreImpact() == false);
-            }
-            @Override
-            public void normalizeOther(MappedFieldType other) {
-                super.normalizeOther(other);
-                ((RankFeatureFieldMapper.RankFeatureFieldType) other).setPositiveScoreImpact(true);
-            }
-        });
-    }
-
     public void testIsAggregatable() {
         MappedFieldType fieldType = createDefaultFieldType();
         assertFalse(fieldType.isAggregatable());

modules/mapper-extras/src/test/java/org/elasticsearch/index/mapper/RankFeatureMetaFieldTypeTests.java

@@ -19,7 +19,7 @@
 
 package org.elasticsearch.index.mapper;
 
-public class RankFeatureMetaFieldTypeTests extends FieldTypeTestCase {
+public class RankFeatureMetaFieldTypeTests extends FieldTypeTestCase<MappedFieldType> {
 
     @Override
     protected MappedFieldType createDefaultFieldType() {

modules/mapper-extras/src/test/java/org/elasticsearch/index/mapper/RankFeaturesFieldTypeTests.java

@@ -19,7 +19,7 @@
 
 package org.elasticsearch.index.mapper;
 
-public class RankFeaturesFieldTypeTests extends FieldTypeTestCase {
+public class RankFeaturesFieldTypeTests extends FieldTypeTestCase<MappedFieldType> {
 
     @Override
     protected MappedFieldType createDefaultFieldType() {

modules/mapper-extras/src/test/java/org/elasticsearch/index/mapper/ScaledFloatFieldMapperTests.java

@@ -29,7 +29,6 @@
 import org.elasticsearch.index.IndexService;
 import org.elasticsearch.index.mapper.MapperService.MergeReason;
 import org.elasticsearch.plugins.Plugin;
-import org.elasticsearch.test.ESSingleNodeTestCase;
 import org.elasticsearch.test.InternalSettingsPlugin;
 import org.junit.Before;
 
@@ -41,7 +40,7 @@
 
 import static org.hamcrest.Matchers.containsString;
 
-public class ScaledFloatFieldMapperTests extends ESSingleNodeTestCase {
+public class ScaledFloatFieldMapperTests extends FieldMapperTestCase<ScaledFloatFieldMapper.Builder> {
 
     IndexService indexService;
     DocumentMapperParser parser;
@@ -50,13 +49,22 @@ public class ScaledFloatFieldMapperTests extends ESSingleNodeTestCase {
     public void setup() {
         indexService = createIndex("test");
         parser = indexService.mapperService().documentMapperParser();
+        addModifier("scaling_factor", false, (a, b) -> {
+            a.scalingFactor(10);
+            b.scalingFactor(100);
+        });
     }
 
     @Override
     protected Collection<Class<? extends Plugin>> getPlugins() {
         return pluginList(InternalSettingsPlugin.class, MapperExtrasPlugin.class);
     }
 
+    @Override
+    protected ScaledFloatFieldMapper.Builder newBuilder() {
+        return new ScaledFloatFieldMapper.Builder("scaled-float").scalingFactor(1);
+    }
+
     public void testDefaults() throws Exception {
         String mapping = Strings.toString(XContentFactory.jsonBuilder().startObject().startObject("type")
                 .startObject("properties").startObject("field").field("type", "scaled_float")

modules/mapper-extras/src/test/java/org/elasticsearch/index/mapper/ScaledFloatFieldTypeTests.java

@@ -29,20 +29,19 @@
 import org.apache.lucene.search.IndexSearcher;
 import org.apache.lucene.search.Query;
 import org.apache.lucene.store.Directory;
-import org.elasticsearch.core.internal.io.IOUtils;
 import org.elasticsearch.Version;
 import org.elasticsearch.cluster.metadata.IndexMetadata;
 import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.core.internal.io.IOUtils;
 import org.elasticsearch.index.IndexSettings;
-import org.elasticsearch.index.fielddata.LeafNumericFieldData;
 import org.elasticsearch.index.fielddata.IndexNumericFieldData;
+import org.elasticsearch.index.fielddata.LeafNumericFieldData;
 import org.elasticsearch.index.fielddata.SortedNumericDoubleValues;
-import org.junit.Before;
 
 import java.io.IOException;
 import java.util.Arrays;
 
-public class ScaledFloatFieldTypeTests extends FieldTypeTestCase {
+public class ScaledFloatFieldTypeTests extends FieldTypeTestCase<MappedFieldType> {
 
     @Override
     protected MappedFieldType createDefaultFieldType() {
@@ -51,22 +50,6 @@ protected MappedFieldType createDefaultFieldType() {
         return ft;
     }
 
-    @Before
-    public void setupProperties() {
-        addModifier(new Modifier("scaling_factor", false) {
-            @Override
-            public void modify(MappedFieldType ft) {
-                ScaledFloatFieldMapper.ScaledFloatFieldType tft = (ScaledFloatFieldMapper.ScaledFloatFieldType)ft;
-                tft.setScalingFactor(10);
-            }
-            @Override
-            public void normalizeOther(MappedFieldType other) {
-                super.normalizeOther(other);
-                ((ScaledFloatFieldMapper.ScaledFloatFieldType) other).setScalingFactor(100);
-            }
-        });
-    }
-
     public void testTermQuery() {
         ScaledFloatFieldMapper.ScaledFloatFieldType ft = new ScaledFloatFieldMapper.ScaledFloatFieldType();
         ft.setName("scaled_float");