Convert ConstantKeywordFieldMapper to parametrized form (#62688)

As part of the conversion, adds the ability to customize merge validation - in this case, we
allow an update to the constant value if it is currently set to null, but refuse further
updates once it has been set once.

This commit also converts ParametrizedMapperTests to use MapperServiceTestCase.

server/src/main/java/org/elasticsearch/index/mapper/ParametrizedFieldMapper.java

@@ -43,6 +43,7 @@ import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
 import java.util.function.BiFunction;
+import java.util.function.BiPredicate;
 import java.util.function.BooleanSupplier;
 import java.util.function.Consumer;
 import java.util.function.Function;
@@ -148,7 +149,8 @@ public abstract class ParametrizedFieldMapper extends FieldMapper {
         private Consumer<T> validator = null;
         private Serializer<T> serializer = XContentBuilder::field;
         private BooleanSupplier serializerPredicate = () -> true;
-        private Function<T, String> conflictSerializer = Object::toString;
+        private Function<T, String> conflictSerializer = Objects::toString;
+        private BiPredicate<T, T> mergeValidator;
         private T value;
         private boolean isSet;
 
@@ -168,6 +170,7 @@ public abstract class ParametrizedFieldMapper extends FieldMapper {
             this.parser = parser;
             this.initializer = initializer;
             this.updateable = updateable;
+            this.mergeValidator = (previous, toMerge) -> updateable || Objects.equals(previous, toMerge);
         }
 
         /**
@@ -241,6 +244,15 @@ public abstract class ParametrizedFieldMapper extends FieldMapper {
             return this;
         }
 
+        /**
+         * Sets a custom merge validator.  By default, merges are accepted if the
+         * parameter is updateable, or if the previous and new values are equal
+         */
+        public Parameter<T> setMergeValidator(BiPredicate<T, T> mergeValidator) {
+            this.mergeValidator = mergeValidator;
+            return this;
+        }
+
         private void validate() {
             if (validator != null) {
                 validator.accept(getValue());
@@ -258,10 +270,10 @@ public abstract class ParametrizedFieldMapper extends FieldMapper {
         private void merge(FieldMapper toMerge, Conflicts conflicts) {
             T value = initializer.apply(toMerge);
             T current = getValue();
-            if (updateable == false && Objects.equals(current, value) == false) {
+            if (mergeValidator.test(current, value)) {
-                conflicts.addConflict(name, conflictSerializer.apply(current), conflictSerializer.apply(value));
-            } else {
                 setValue(value);
+            } else {
+                conflicts.addConflict(name, conflictSerializer.apply(current), conflictSerializer.apply(value));
             }
         }
 

server/src/test/java/org/elasticsearch/index/mapper/ParametrizedMapperTests.java

@@ -29,7 +29,6 @@ import org.elasticsearch.common.xcontent.ToXContent;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentHelper;
 import org.elasticsearch.common.xcontent.json.JsonXContent;
-import org.elasticsearch.index.IndexService;
 import org.elasticsearch.index.analysis.AnalyzerScope;
 import org.elasticsearch.index.analysis.IndexAnalyzers;
 import org.elasticsearch.index.analysis.NamedAnalyzer;
@@ -37,7 +36,6 @@ import org.elasticsearch.index.mapper.ParametrizedFieldMapper.Parameter;
 import org.elasticsearch.plugins.MapperPlugin;
 import org.elasticsearch.plugins.Plugin;
 import org.elasticsearch.search.lookup.SearchLookup;
-import org.elasticsearch.test.ESSingleNodeTestCase;
 
 import java.io.IOException;
 import java.util.Arrays;
@@ -47,11 +45,12 @@ import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 
+import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.instanceOf;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
-public class ParametrizedMapperTests extends ESSingleNodeTestCase {
+public class ParametrizedMapperTests extends MapperServiceTestCase {
 
     public static class TestPlugin extends Plugin implements MapperPlugin {
         @Override
@@ -61,8 +60,8 @@ public class ParametrizedMapperTests extends ESSingleNodeTestCase {
     }
 
     @Override
-    protected Collection<Class<? extends Plugin>> getPlugins() {
+    protected Collection<Plugin> getPlugins() {
-        return Collections.singletonList(TestPlugin.class);
+        return Collections.singletonList(new TestPlugin());
     }
 
     private static class StringWrapper {
@@ -111,7 +110,8 @@ public class ParametrizedMapperTests extends ESSingleNodeTestCase {
                 if (n > 50) {
                     throw new IllegalArgumentException("Value of [n] cannot be greater than 50");
                 }
-            });
+            })
+            .setMergeValidator((o, n) -> n >= o);
         final Parameter<NamedAnalyzer> analyzer
             = Parameter.analyzerParam("analyzer", false, m -> toType(m).analyzer, () -> Lucene.KEYWORD_ANALYZER);
         final Parameter<NamedAnalyzer> searchAnalyzer
@@ -334,8 +334,6 @@ public class ParametrizedMapperTests extends ESSingleNodeTestCase {
 
     public void testObjectSerialization() throws IOException {
 
-        IndexService indexService = createIndex("test");
-
         String mapping = "{\"_doc\":{" +
             "\"properties\":{" +
             "\"actual\":{\"type\":\"double\"}," +
@@ -344,11 +342,12 @@ public class ParametrizedMapperTests extends ESSingleNodeTestCase {
             "\"anomaly_score\":{\"type\":\"double\"}," +
             "\"bucket_span\":{\"type\":\"long\"}," +
             "\"is_interim\":{\"type\":\"boolean\"}}}}}}";
-        indexService.mapperService().merge("_doc", new CompressedXContent(mapping), MapperService.MergeReason.MAPPING_UPDATE);
-        assertEquals(mapping, Strings.toString(indexService.mapperService().documentMapper()));
 
-        indexService.mapperService().merge("_doc", new CompressedXContent(mapping), MapperService.MergeReason.MAPPING_UPDATE);
+        MapperService mapperService = createMapperService("_doc", mapping);
-        assertEquals(mapping, Strings.toString(indexService.mapperService().documentMapper()));
+        assertEquals(mapping, Strings.toString(mapperService.documentMapper()));
+
+        mapperService.merge("_doc", new CompressedXContent(mapping), MapperService.MergeReason.MAPPING_UPDATE);
+        assertEquals(mapping, Strings.toString(mapperService.documentMapper()));
     }
 
     // test custom serializer
@@ -485,4 +484,19 @@ public class ParametrizedMapperTests extends ESSingleNodeTestCase {
         }
     }
 
+    public void testCustomMergeValidation() throws IOException {
+        MapperService mapperService = createMapperService(fieldMapping(b -> {
+            b.field("type", "test_mapper");
+            b.field("required", "a");
+            b.field("int_value", 10);
+        }));
+
+        IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> merge(mapperService, fieldMapping(b -> {
+            b.field("type", "test_mapper");
+            b.field("required", "a");
+            b.field("int_value", 5);    // custom merge validator says that int_value can only increase
+        })));
+        assertThat(e.getMessage(), containsString("int_value"));
+    }
+
 }

x-pack/plugin/mapper-constant-keyword/src/internalClusterTest/java/org/elasticsearch/xpack/constantkeyword/mapper/ConstantKeywordFieldMapperTests.java

@@ -7,80 +7,48 @@
 package org.elasticsearch.xpack.constantkeyword.mapper;
 
 import org.elasticsearch.common.Strings;
-import org.elasticsearch.common.bytes.BytesReference;
-import org.elasticsearch.common.compress.CompressedXContent;
 import org.elasticsearch.common.collect.List;
+import org.elasticsearch.common.compress.CompressedXContent;
 import org.elasticsearch.common.xcontent.XContentBuilder;
-import org.elasticsearch.common.xcontent.XContentFactory;
-import org.elasticsearch.common.xcontent.XContentType;
 import org.elasticsearch.index.mapper.DocumentMapper;
 import org.elasticsearch.index.mapper.FieldMapper;
-import org.elasticsearch.index.mapper.FieldMapperTestCase2;
 import org.elasticsearch.index.mapper.MapperParsingException;
 import org.elasticsearch.index.mapper.MapperService;
 import org.elasticsearch.index.mapper.MapperService.MergeReason;
+import org.elasticsearch.index.mapper.MapperTestCase;
 import org.elasticsearch.index.mapper.ParsedDocument;
-import org.elasticsearch.index.mapper.SourceToParse;
 import org.elasticsearch.index.mapper.ValueFetcher;
 import org.elasticsearch.plugins.Plugin;
 import org.elasticsearch.search.lookup.SourceLookup;
 import org.elasticsearch.xpack.constantkeyword.ConstantKeywordMapperPlugin;
-import org.junit.Before;
 
 import java.io.IOException;
 import java.util.Collection;
 import java.util.Collections;
-import java.util.Set;
 
 import static java.util.Collections.singleton;
 
-public class ConstantKeywordFieldMapperTests extends FieldMapperTestCase2<ConstantKeywordFieldMapper.Builder> {
+public class ConstantKeywordFieldMapperTests extends MapperTestCase {
 
     @Override
     protected Collection<Plugin> getPlugins() {
         return singleton(new ConstantKeywordMapperPlugin());
     }
 
-    @Override
-    protected Set<String> unsupportedProperties() {
-        return org.elasticsearch.common.collect.Set.of("analyzer", "similarity", "store", "doc_values", "index");
-    }
-
-    @Override
-    protected ConstantKeywordFieldMapper.Builder newBuilder() {
-        return new ConstantKeywordFieldMapper.Builder("constant");
-    }
-
-    @Before
-    public void addModifiers() {
-        addModifier("value", false, (a, b) -> {
-            a.setValue("foo");
-            b.setValue("bar");
-        });
-        addModifier("unset", false, (a, b) -> {
-            a.setValue("foo");
-            ;
-        });
-        addModifier("value-from-null", true, (a, b) -> { b.setValue("bar"); });
-    }
-
     public void testDefaults() throws Exception {
         XContentBuilder mapping = fieldMapping(b -> b.field("type", "constant_keyword").field("value", "foo"));
         DocumentMapper mapper = createDocumentMapper(mapping);
         assertEquals(Strings.toString(mapping), mapper.mappingSource().toString());
 
-        BytesReference source = BytesReference.bytes(XContentFactory.jsonBuilder().startObject().endObject());
+        ParsedDocument doc = mapper.parse(source(b -> {}));
-        ParsedDocument doc = mapper.parse(new SourceToParse("test", "_doc", "1", source, XContentType.JSON));
         assertNull(doc.rootDoc().getField("field"));
 
-        source = BytesReference.bytes(XContentFactory.jsonBuilder().startObject().field("field", "foo").endObject());
+        doc = mapper.parse(source(b -> b.field("field", "foo")));
-        doc = mapper.parse(new SourceToParse("test", "_doc", "1", source, XContentType.JSON));
         assertNull(doc.rootDoc().getField("field"));
 
-        BytesReference illegalSource = BytesReference.bytes(XContentFactory.jsonBuilder().startObject().field("field", "bar").endObject());
         MapperParsingException e = expectThrows(
             MapperParsingException.class,
-            () -> mapper.parse(new SourceToParse("test", "_doc", "1", illegalSource, XContentType.JSON))
+            () -> mapper.parse(source(b -> b.field("field", "bar")))
         );
         assertEquals(
             "[constant_keyword] field [field] only accepts values that are equal to the value defined in the mappings [foo], "
@@ -92,8 +60,7 @@ public class ConstantKeywordFieldMapperTests extends FieldMapperTestCase2<Consta
     public void testDynamicValue() throws Exception {
         MapperService mapperService = createMapperService(fieldMapping(b -> b.field("type", "constant_keyword")));
 
-        BytesReference source = BytesReference.bytes(XContentFactory.jsonBuilder().startObject().field("field", "foo").endObject());
+        ParsedDocument doc = mapperService.documentMapper().parse(source(b -> b.field("field", "foo")));
-        ParsedDocument doc = mapperService.documentMapper().parse(new SourceToParse("test", "_doc", "1", source, XContentType.JSON));
         assertNull(doc.rootDoc().getField("field"));
         assertNotNull(doc.dynamicMappingsUpdate());
 
@@ -102,11 +69,55 @@ public class ConstantKeywordFieldMapperTests extends FieldMapperTestCase2<Consta
         String expectedMapping = Strings.toString(fieldMapping(b -> b.field("type", "constant_keyword").field("value", "foo")));
         assertEquals(expectedMapping, updatedMapper.mappingSource().toString());
 
-        doc = updatedMapper.parse(new SourceToParse("test", "_doc", "1", source, XContentType.JSON));
+        doc = updatedMapper.parse(source(b -> b.field("field", "foo")));
         assertNull(doc.rootDoc().getField("field"));
         assertNull(doc.dynamicMappingsUpdate());
     }
 
+    public void testBadValues() {
+        {
+            MapperParsingException e = expectThrows(MapperParsingException.class, () -> createMapperService(fieldMapping(b -> {
+                b.field("type", "constant_keyword");
+                b.nullField("value");
+            })));
+            assertEquals(e.getMessage(),
+                "Failed to parse mapping [_doc]: [value] on mapper [field] of type [constant_keyword] must not have a [null] value");
+        }
+        {
+            MapperParsingException e = expectThrows(MapperParsingException.class, () -> createMapperService(fieldMapping(b -> {
+                b.field("type", "constant_keyword");
+                b.startObject("value").field("foo", "bar").endObject();
+            })));
+            assertEquals(e.getMessage(),
+                "Failed to parse mapping [_doc]: Property [value] on field [field] must be a number or a string, but got [{foo=bar}]");
+        }
+    }
+
+    public void testNumericValue() throws IOException {
+        MapperService mapperService = createMapperService(fieldMapping(b -> {
+            b.field("type", "constant_keyword");
+            b.field("value", 74);
+        }));
+        ConstantKeywordFieldMapper.ConstantKeywordFieldType ft
+            = (ConstantKeywordFieldMapper.ConstantKeywordFieldType) mapperService.fieldType("field");
+        assertEquals("74", ft.value());
+    }
+
+    public void testUpdate() throws IOException {
+        MapperService mapperService = createMapperService(fieldMapping(b -> {
+            b.field("type", "constant_keyword");
+            b.field("value", "foo");
+        }));
+
+        IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> merge(mapperService, fieldMapping(b -> {
+            b.field("type", "constant_keyword");
+            b.field("value", "bar");
+        })));
+        assertEquals(e.getMessage(),
+            "Mapper for [field] conflicts with existing mapper:\n" +
+            "\tCannot update parameter [value] from [foo] to [bar]");
+    }
+
     @Override
     protected void minimalMapping(XContentBuilder b) throws IOException {
         b.field("type", "constant_keyword");

x-pack/plugin/mapper-constant-keyword/src/main/java/org/elasticsearch/xpack/constantkeyword/ConstantKeywordMapperPlugin.java

@@ -19,7 +19,7 @@ import static java.util.Collections.singletonMap;
 public class ConstantKeywordMapperPlugin extends Plugin implements MapperPlugin, ActionPlugin {
     @Override
     public Map<String, Mapper.TypeParser> getMappers() {
-        return singletonMap(ConstantKeywordFieldMapper.CONTENT_TYPE, new ConstantKeywordFieldMapper.TypeParser());
+        return singletonMap(ConstantKeywordFieldMapper.CONTENT_TYPE, ConstantKeywordFieldMapper.PARSER);
     }
 
 }

x-pack/plugin/mapper-constant-keyword/src/main/java/org/elasticsearch/xpack/constantkeyword/mapper/ConstantKeywordFieldMapper.java

@@ -7,8 +7,6 @@
 
 package org.elasticsearch.xpack.constantkeyword.mapper;
 
-import org.apache.lucene.document.FieldType;
-import org.apache.lucene.index.IndexOptions;
 import org.apache.lucene.search.MatchAllDocsQuery;
 import org.apache.lucene.search.MatchNoDocsQuery;
 import org.apache.lucene.search.MultiTermQuery;
@@ -24,7 +22,6 @@ import org.elasticsearch.common.lucene.BytesRefs;
 import org.elasticsearch.common.regex.Regex;
 import org.elasticsearch.common.time.DateMathParser;
 import org.elasticsearch.common.unit.Fuzziness;
-import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.index.fielddata.IndexFieldData;
 import org.elasticsearch.index.fielddata.plain.ConstantIndexFieldData;
@@ -35,8 +32,8 @@ import org.elasticsearch.index.mapper.MappedFieldType;
 import org.elasticsearch.index.mapper.Mapper;
 import org.elasticsearch.index.mapper.MapperParsingException;
 import org.elasticsearch.index.mapper.MapperService;
+import org.elasticsearch.index.mapper.ParametrizedFieldMapper;
 import org.elasticsearch.index.mapper.ParseContext;
-import org.elasticsearch.index.mapper.TypeParsers;
 import org.elasticsearch.index.mapper.ValueFetcher;
 import org.elasticsearch.index.query.QueryShardContext;
 import org.elasticsearch.search.aggregations.support.CoreValuesSourceType;
@@ -44,6 +41,7 @@ import org.elasticsearch.search.lookup.SearchLookup;
 
 import java.io.IOException;
 import java.time.ZoneId;
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
@@ -53,65 +51,52 @@ import java.util.function.Supplier;
 /**
  * A {@link FieldMapper} that assigns every document the same value.
  */
-public class ConstantKeywordFieldMapper extends FieldMapper {
+public class ConstantKeywordFieldMapper extends ParametrizedFieldMapper {
 
     public static final String CONTENT_TYPE = "constant_keyword";
 
-    public static class Defaults {
+    private static ConstantKeywordFieldMapper toType(FieldMapper in) {
-        public static final FieldType FIELD_TYPE = new FieldType();
+        return (ConstantKeywordFieldMapper) in;
-        static {
-            FIELD_TYPE.setIndexOptions(IndexOptions.NONE);
-            FIELD_TYPE.freeze();
-        }
     }
 
-    public static class Builder extends FieldMapper.Builder<Builder> {
+    @Override
+    public ParametrizedFieldMapper.Builder getMergeBuilder() {
+        return new Builder(simpleName()).init(this);
+    }
 
-        String value;
+    public static class Builder extends ParametrizedFieldMapper.Builder {
+
+        // This is defined as updateable because it can be updated once, from [null] to any value,
+        // by a dynamic mapping update.  Once it has been set, however, the value cannot be changed.
+        private final Parameter<String> value = new Parameter<>("value", true, () -> null,
+            (n, c, o) -> {
+                if (o instanceof Number == false && o instanceof CharSequence == false) {
+                    throw new MapperParsingException("Property [value] on field [" + n +
+                        "] must be a number or a string, but got [" + o + "]");
+                }
+                return o.toString();
+            }, m -> toType(m).fieldType().value);
+        private final Parameter<Map<String, String>> meta = Parameter.metaParam();
 
         public Builder(String name) {
-            super(name, Defaults.FIELD_TYPE);
+            super(name);
-            builder = this;
+            value.setShouldSerialize(() -> value.getValue() != null);
+            value.setMergeValidator((previous, current) -> previous == null || Objects.equals(previous, current));
         }
 
-        // TODO we should ban setting 'index' on constant keyword
+        @Override
-
+        protected List<Parameter<?>> getParameters() {
-        public Builder setValue(String value) {
+            return Arrays.asList(value, meta);
-            this.value = value;
-            return this;
         }
 
         @Override
         public ConstantKeywordFieldMapper build(BuilderContext context) {
             return new ConstantKeywordFieldMapper(
-                    name, fieldType, new ConstantKeywordFieldType(buildFullName(context), value, meta));
+                    name, new ConstantKeywordFieldType(buildFullName(context), value.getValue(), meta.getValue()));
         }
     }
 
-    public static class TypeParser implements Mapper.TypeParser {
+    public static final TypeParser PARSER = new TypeParser((n, c) -> new Builder(n));
-        @Override
-        public Mapper.Builder<?> parse(String name, Map<String, Object> node, ParserContext parserContext) throws MapperParsingException {
-            Object value = null;
-            if (node.containsKey("value")) {
-                value = node.remove("value");
-                if (value == null) {
-                    throw new MapperParsingException("Property [value] of field [" + name + "] can't be [null].");
-                }
-                if (value instanceof Number == false && value instanceof CharSequence == false) {
-                    throw new MapperParsingException("Property [value] of field [" + name +
-                            "] must be a number or a string, but got [" + value + "]");
-                }
-            }
-            ConstantKeywordFieldMapper.Builder builder = new ConstantKeywordFieldMapper.Builder(name);
-            if (value != null) {
-                builder.setValue(value.toString());
-            }
-            if (node.containsKey("meta")) {
-                builder.meta(TypeParsers.parseMeta(name, node.remove("meta")));
-            }
-            return builder;
-        }
-    }
 
     public static final class ConstantKeywordFieldType extends ConstantFieldType {
 
@@ -227,8 +212,8 @@ public class ConstantKeywordFieldMapper extends FieldMapper {
 
     }
 
-    ConstantKeywordFieldMapper(String simpleName, FieldType fieldType, MappedFieldType mappedFieldType) {
+    ConstantKeywordFieldMapper(String simpleName, MappedFieldType mappedFieldType) {
-        super(simpleName, fieldType, mappedFieldType, MultiFields.empty(), CopyTo.empty());
+        super(simpleName, mappedFieldType, MultiFields.empty(), CopyTo.empty());
     }
 
     @Override
@@ -257,7 +242,7 @@ public class ConstantKeywordFieldMapper extends FieldMapper {
 
         if (fieldType().value == null) {
             ConstantKeywordFieldType newFieldType = new ConstantKeywordFieldType(fieldType().name(), value, fieldType().meta());
-            Mapper update = new ConstantKeywordFieldMapper(simpleName(), fieldType, newFieldType);
+            Mapper update = new ConstantKeywordFieldMapper(simpleName(), newFieldType);
             context.addDynamicMapper(update);
         } else if (Objects.equals(fieldType().value, value) == false) {
             throw new IllegalArgumentException("[constant_keyword] field [" + name() +
@@ -277,29 +262,9 @@ public class ConstantKeywordFieldMapper extends FieldMapper {
             : lookup -> org.elasticsearch.common.collect.List.of(fieldType().value);
     }
 
-    @Override
-    protected void mergeOptions(FieldMapper other, List<String> conflicts) {
-        ConstantKeywordFieldType newConstantKeywordFT = (ConstantKeywordFieldType) other.fieldType();
-        if (this.fieldType().value != null) {
-            if (newConstantKeywordFT.value == null) {
-                conflicts.add("mapper [" + name() + "] cannot unset [value]");
-            } else if (Objects.equals(fieldType().value, newConstantKeywordFT.value) == false) {
-                conflicts.add("mapper [" + name() + "] has different [value] from the value that is configured in mappings: ["
-                    + fieldType().value + "] vs. [" + newConstantKeywordFT.value + "]");
-            }
-        }
-    }
-
     @Override
     protected String contentType() {
         return CONTENT_TYPE;
     }
 
-    @Override
-    protected void doXContentBody(XContentBuilder builder, boolean includeDefaults, Params params) throws IOException {
-        super.doXContentBody(builder, includeDefaults, params);
-        if (fieldType().value() != null) {
-            builder.field("value", fieldType().value());
-        }
-    }
 }