From 9dda2f32b990cf2b35885e058d6c03e14a35bd60 Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Tue, 23 Jun 2020 10:12:08 +0100 Subject: [PATCH 01/11] First go: need to fix merging with BuilderContext --- .../index/mapper/BinaryFieldMapper.java | 67 +++-- .../index/mapper/BooleanFieldMapper.java | 83 +++--- .../index/mapper/FieldMapper.java | 2 +- .../index/mapper/ParametrizedFieldMapper.java | 256 ++++++++++++++++++ .../index/mapper/TypeParsers.java | 14 +- 5 files changed, 333 insertions(+), 89 deletions(-) create mode 100644 server/src/main/java/org/elasticsearch/index/mapper/ParametrizedFieldMapper.java diff --git a/server/src/main/java/org/elasticsearch/index/mapper/BinaryFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/BinaryFieldMapper.java index 57bb89f0422d9..4a79abb71ea92 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/BinaryFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/BinaryFieldMapper.java @@ -20,8 +20,8 @@ package org.elasticsearch.index.mapper; import com.carrotsearch.hppc.ObjectArrayList; -import org.apache.lucene.document.Field; import org.apache.lucene.document.FieldType; +import org.apache.lucene.document.StoredField; import org.apache.lucene.index.IndexOptions; import org.apache.lucene.index.Term; import org.apache.lucene.search.DocValuesFieldExistsQuery; @@ -48,9 +48,7 @@ import java.util.List; import java.util.Map; -import static org.elasticsearch.index.mapper.TypeParsers.parseField; - -public class BinaryFieldMapper extends FieldMapper { +public class BinaryFieldMapper extends ParametrizedFieldMapper { public static final String CONTENT_TYPE = "binary"; @@ -63,26 +61,28 @@ public static class Defaults { } } - public static class Builder extends FieldMapper.Builder { + private static BinaryFieldMapper toType(FieldMapper in) { + return (BinaryFieldMapper) in; + } + + public static class Builder extends ParametrizedFieldMapper.Builder { + + final Parameter stored = Parameter.boolParam("stored", false, m -> toType(m).stored, false); + final Parameter hasDocValues = Parameter.boolParam("doc_values", false, m -> toType(m).hasDocValues, false); public Builder(String name) { - super(name, Defaults.FIELD_TYPE); - hasDocValues = false; - builder = this; + super(name); } @Override - public BinaryFieldMapper build(BuilderContext context) { - return new BinaryFieldMapper(name, fieldType, new BinaryFieldType(buildFullName(context), hasDocValues, meta), - multiFieldsBuilder.build(this, context), copyTo); + public List> getParameters() { + return List.of(meta, stored, hasDocValues); } @Override - public Builder index(boolean index) { - if (index) { - throw new MapperParsingException("Binary field [" + name() + "] cannot be indexed"); - } - return builder; + public BinaryFieldMapper build(BuilderContext context) { + return new BinaryFieldMapper(name, new BinaryFieldType(buildFullName(context), hasDocValues.getValue(), meta.getValue()), + multiFieldsBuilder.build(this, context), copyTo, stored.getValue(), hasDocValues.getValue()); } } @@ -91,7 +91,7 @@ public static class TypeParser implements Mapper.TypeParser { public BinaryFieldMapper.Builder parse(String name, Map node, ParserContext parserContext) throws MapperParsingException { BinaryFieldMapper.Builder builder = new BinaryFieldMapper.Builder(name); - parseField(builder, name, node, parserContext); + builder.parse(name, node); return builder; } } @@ -166,14 +166,19 @@ public Query termQuery(Object value, QueryShardContext context) { } } - protected BinaryFieldMapper(String simpleName, FieldType fieldType, MappedFieldType mappedFieldType, - MultiFields multiFields, CopyTo copyTo) { - super(simpleName, fieldType, mappedFieldType, multiFields, copyTo); + private final boolean stored; + private final boolean hasDocValues; + + protected BinaryFieldMapper(String simpleName, MappedFieldType mappedFieldType, + MultiFields multiFields, CopyTo copyTo, boolean stored, boolean hasDocValues) { + super(simpleName, mappedFieldType, multiFields, copyTo); + this.stored = stored; + this.hasDocValues = hasDocValues; } @Override protected void parseCreateField(ParseContext context) throws IOException { - if (!fieldType.stored() && !fieldType().hasDocValues()) { + if (stored == false && hasDocValues == false) { return; } byte[] value = context.parseExternalValue(byte[].class); @@ -187,11 +192,11 @@ protected void parseCreateField(ParseContext context) throws IOException { if (value == null) { return; } - if (fieldType.stored()) { - context.doc().add(new Field(fieldType().name(), value, fieldType)); + if (stored) { + context.doc().add(new StoredField(fieldType().name(), value)); } - if (fieldType().hasDocValues()) { + if (hasDocValues) { CustomBinaryDocValuesField field = (CustomBinaryDocValuesField) context.doc().getByKey(fieldType().name()); if (field == null) { field = new CustomBinaryDocValuesField(fieldType().name(), value); @@ -209,18 +214,8 @@ protected void parseCreateField(ParseContext context) throws IOException { } @Override - protected boolean indexedByDefault() { - return false; - } - - @Override - protected boolean docValuesByDefault() { - return false; - } - - @Override - protected void mergeOptions(FieldMapper other, List conflicts) { - + public ParametrizedFieldMapper.Builder getMergeBuilder() { + return new BinaryFieldMapper.Builder(name()).init(this); } @Override diff --git a/server/src/main/java/org/elasticsearch/index/mapper/BooleanFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/BooleanFieldMapper.java index 8d359f3baf549..5ce72ed325244 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/BooleanFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/BooleanFieldMapper.java @@ -22,6 +22,7 @@ import org.apache.lucene.document.Field; import org.apache.lucene.document.FieldType; import org.apache.lucene.document.SortedNumericDocValuesField; +import org.apache.lucene.document.StoredField; import org.apache.lucene.index.IndexOptions; import org.apache.lucene.index.Term; import org.apache.lucene.search.DocValuesFieldExistsQuery; @@ -30,7 +31,6 @@ import org.apache.lucene.search.TermRangeQuery; import org.apache.lucene.util.BytesRef; import org.elasticsearch.common.Nullable; -import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.support.XContentMapValues; import org.elasticsearch.index.fielddata.IndexFieldData; @@ -42,16 +42,13 @@ import java.io.IOException; import java.time.ZoneId; import java.util.Collections; -import java.util.Iterator; import java.util.List; import java.util.Map; -import static org.elasticsearch.index.mapper.TypeParsers.parseField; - /** * A field mapper for boolean fields. */ -public class BooleanFieldMapper extends FieldMapper { +public class BooleanFieldMapper extends ParametrizedFieldMapper { public static final String CONTENT_TYPE = "boolean"; @@ -71,25 +68,39 @@ public static class Values { public static final BytesRef FALSE = new BytesRef("F"); } - public static class Builder extends FieldMapper.Builder { + private static BooleanFieldMapper toType(FieldMapper in) { + return (BooleanFieldMapper) in; + } - private Boolean nullValue; + public static class Builder extends ParametrizedFieldMapper.Builder { + + private final Parameter boost = Parameter.floatParam("boost", true, m -> m.fieldType().boost(), 1.0f); + private final Parameter docValues + = Parameter.boolParam("doc_values", false, m -> m.fieldType().hasDocValues(), true); + private final Parameter indexed + = Parameter.boolParam("index", false, m -> m.fieldType().isSearchable(), true); + private final Parameter nullValue = new Parameter<>("null_value", false, null, (n, o) -> { + if (o == null) { + return null; + } + return XContentMapValues.nodeBooleanValue(o); + }, m -> toType(m).nullValue).acceptsNull(); + private final Parameter stored = Parameter.boolParam("store", false, m -> toType(m).stored, false); public Builder(String name) { - super(name, Defaults.FIELD_TYPE); - this.builder = this; + super(name); } - public Builder nullValue(Boolean nullValue) { - this.nullValue = nullValue; - return builder; + @Override + protected List> getParameters() { + return List.of(meta, boost, docValues, indexed, nullValue, stored); } @Override public BooleanFieldMapper build(BuilderContext context) { - return new BooleanFieldMapper(name, fieldType, - new BooleanFieldType(buildFullName(context), indexed, hasDocValues, meta), - multiFieldsBuilder.build(this, context), copyTo, nullValue); + return new BooleanFieldMapper(name, + new BooleanFieldType(buildFullName(context), indexed.getValue(), docValues.getValue(), meta.getValue()), + multiFieldsBuilder.build(this, context), copyTo, nullValue.getValue(), stored.getValue()); } } @@ -98,19 +109,7 @@ public static class TypeParser implements Mapper.TypeParser { public BooleanFieldMapper.Builder parse(String name, Map node, ParserContext parserContext) throws MapperParsingException { BooleanFieldMapper.Builder builder = new BooleanFieldMapper.Builder(name); - parseField(builder, name, node, parserContext); - for (Iterator> iterator = node.entrySet().iterator(); iterator.hasNext();) { - Map.Entry entry = iterator.next(); - String propName = entry.getKey(); - Object propNode = entry.getValue(); - if (propName.equals("null_value")) { - if (propNode == null) { - throw new MapperParsingException("Property [null_value] cannot be null."); - } - builder.nullValue(XContentMapValues.nodeBooleanValue(propNode, name + ".null_value")); - iterator.remove(); - } - } + builder.parse(name, node); return builder; } } @@ -217,11 +216,13 @@ public Query rangeQuery(Object lowerTerm, Object upperTerm, boolean includeLower } private final Boolean nullValue; + private final boolean stored; - protected BooleanFieldMapper(String simpleName, FieldType fieldType, MappedFieldType mappedFieldType, - MultiFields multiFields, CopyTo copyTo, Boolean nullValue) { - super(simpleName, fieldType, mappedFieldType, multiFields, copyTo); + protected BooleanFieldMapper(String simpleName, MappedFieldType mappedFieldType, + MultiFields multiFields, CopyTo copyTo, Boolean nullValue, boolean stored) { + super(simpleName, mappedFieldType, multiFields, copyTo); this.nullValue = nullValue; + this.stored = stored; } @Override @@ -231,7 +232,7 @@ public BooleanFieldType fieldType() { @Override protected void parseCreateField(ParseContext context) throws IOException { - if (fieldType().isSearchable() == false && !fieldType.stored() && !fieldType().hasDocValues()) { + if (fieldType().isSearchable() == false && stored == false && !fieldType().hasDocValues()) { return; } @@ -250,8 +251,11 @@ protected void parseCreateField(ParseContext context) throws IOException { if (value == null) { return; } - if (fieldType().isSearchable() || fieldType.stored()) { - context.doc().add(new Field(fieldType().name(), value ? "T" : "F", fieldType)); + if (fieldType().isSearchable()) { + context.doc().add(new Field(fieldType().name(), value ? "T" : "F", Defaults.FIELD_TYPE)); + } + if (stored) { + context.doc().add(new StoredField(fieldType().name(), value ? "T" : "F")); } if (fieldType().hasDocValues()) { context.doc().add(new SortedNumericDocValuesField(fieldType().name(), value ? 1 : 0)); @@ -261,8 +265,8 @@ protected void parseCreateField(ParseContext context) throws IOException { } @Override - protected void mergeOptions(FieldMapper other, List conflicts) { - // TODO ban updating null values + public ParametrizedFieldMapper.Builder getMergeBuilder() { + return new Builder(name()).init(this); } @Override @@ -270,11 +274,4 @@ protected String contentType() { return CONTENT_TYPE; } - @Override - protected void doXContentBody(XContentBuilder builder, boolean includeDefaults, Params params) throws IOException { - super.doXContentBody(builder, includeDefaults, params); - if (includeDefaults || nullValue != null) { - builder.field("null_value", nullValue); - } - } } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java index 58d7e3d34a79d..5a8a61011f487 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java @@ -300,7 +300,7 @@ protected FieldMapper clone() { } @Override - public final FieldMapper merge(Mapper mergeWith) { + public FieldMapper merge(Mapper mergeWith) { FieldMapper merged = clone(); List conflicts = new ArrayList<>(); if (mergeWith instanceof FieldMapper == false) { diff --git a/server/src/main/java/org/elasticsearch/index/mapper/ParametrizedFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/ParametrizedFieldMapper.java new file mode 100644 index 0000000000000..1f458f2409dd1 --- /dev/null +++ b/server/src/main/java/org/elasticsearch/index/mapper/ParametrizedFieldMapper.java @@ -0,0 +1,256 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.index.mapper; + +import joptsimple.internal.Strings; +import org.apache.lucene.document.FieldType; +import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.common.xcontent.support.XContentMapValues; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashMap; +import java.util.Iterator; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.function.BiFunction; +import java.util.function.Function; + +public abstract class ParametrizedFieldMapper extends FieldMapper { + + protected ParametrizedFieldMapper(String simpleName, MappedFieldType mappedFieldType, MultiFields multiFields, CopyTo copyTo) { + super(simpleName, new FieldType(), mappedFieldType, multiFields, copyTo); + } + + public abstract ParametrizedFieldMapper.Builder getMergeBuilder(); + + public ParametrizedFieldMapper(String simpleName, FieldType fieldType, MappedFieldType mappedFieldType, MultiFields multiFields, CopyTo copyTo) { + super(simpleName, fieldType, mappedFieldType, multiFields, copyTo); + } + + @Override + public final ParametrizedFieldMapper merge(Mapper mergeWith) { + + if (mergeWith instanceof ParametrizedFieldMapper == false) { + throw new IllegalArgumentException("mapper [" + mappedFieldType.name() + "] cannot be changed from type [" + + contentType() + "] to [" + mergeWith.getClass().getSimpleName() + "]"); + } + if (Objects.equals(this.getClass(), mergeWith.getClass()) == false) { + throw new IllegalArgumentException("mapper [" + mappedFieldType.name() + "] cannot be changed from type [" + + contentType() + "] to [" + ((FieldMapper) mergeWith).contentType() + "]"); + } + + ParametrizedFieldMapper.Builder builder = getMergeBuilder(); + Conflicts conflicts = new Conflicts(simpleName()); + builder.merge((FieldMapper) mergeWith, conflicts); + conflicts.check(); + return builder.build(new BuilderContext(null, null)); + } + + @Override + protected final void mergeOptions(FieldMapper other, List conflicts) { + // TODO remove when everything is parametrized + } + + @Override + public final XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { + builder.startObject(simpleName()); + builder.field("type", contentType()); + boolean includeDefaults = params.paramAsBoolean("include_defaults", false); + getMergeBuilder().toXContent(builder, includeDefaults); + return builder.endObject(); + } + + public static final class Parameter { + + private final T defaultValue; + private final BiFunction parser; + private final Function merger; + private final String name; + private final boolean updateable; + private boolean acceptsNull = false; + private T value; + private boolean frozen = false; + + public Parameter(String name, boolean updateable, T defaultValue, BiFunction parser, Function merger) { + this.name = name; + this.defaultValue = defaultValue; + this.value = defaultValue; + this.parser = parser; + this.merger = merger; + this.updateable = updateable; + } + + public T getValue() { + return value; + } + + public Parameter acceptsNull() { + this.acceptsNull = true; + return this; + } + + public Parameter init(FieldMapper toInit) { + assert frozen == false; + this.value = merger.apply(toInit); + return this; + } + + public Parameter update(T value) { + this.value = value; + return this; + } + + public Parameter parse(String field, Object in) { + assert frozen == false; + this.value = parser.apply(field, in); + return this; + } + + public Parameter merge(FieldMapper toMerge, Conflicts conflicts) { + T value = merger.apply(toMerge); + if (frozen && Objects.equals(this.value, value) == false) { + conflicts.addConflict(name, this.value.toString(), value.toString()); + } else { + this.value = value; + } + return this; + } + + public Parameter freeze() { + this.frozen = true; + return this; + } + + public XContentBuilder toXContent(XContentBuilder builder, boolean includeDefaults) throws IOException { + if (includeDefaults || (Objects.equals(defaultValue, value) == false)) { + builder.field(name, value); + } + return builder; + } + + public static Parameter boolParam(String name, boolean updateable, Function merger, boolean defaultValue) { + return new Parameter<>(name, updateable, defaultValue, (n, o) -> XContentMapValues.nodeBooleanValue(o), merger); + } + + public static Parameter floatParam(String name, boolean updateable, Function merger, float defaultValue) { + return new Parameter<>(name, updateable, defaultValue, (n, o) -> XContentMapValues.nodeFloatValue(o), merger); + } + + public static Parameter stringParam(String name, boolean updateable, Function merger, String defaultValue) { + return new Parameter<>(name, updateable, defaultValue, (n, o) -> XContentMapValues.nodeStringValue(o, defaultValue), merger); + } + } + + public static final class Conflicts { + + private final String mapperName; + private final List conflicts = new ArrayList<>(); + + public Conflicts(String mapperName) { + this.mapperName = mapperName; + } + + public void addConflict(String parameter, String existing, String toMerge) { + conflicts.add("Cannot update parameter [" + parameter + "] from [" + existing + "] to [" + toMerge + "]"); + } + + public void check() { + if (conflicts.isEmpty()) { + return; + } + String message = "Mapper for [" + mapperName + "] conflicts with existing mapper:\n\t" + + Strings.join(conflicts, "\n\t"); + throw new IllegalArgumentException(message); + } + + } + + public abstract static class Builder extends FieldMapper.Builder { + + protected final MultiFields.Builder multiFieldsBuilder = new MultiFields.Builder(); + protected CopyTo copyTo = CopyTo.empty(); + + protected final Parameter> meta + = new Parameter<>("meta", true, Collections.emptyMap(), TypeParsers::parseMeta, m -> m.fieldType().meta()); + + protected Builder(String name) { + super(name, new FieldType()); + } + + protected Builder init(FieldMapper base) { + for (Parameter param : getParameters()) { + param.init(base); + } + return this; + } + + public final void merge(FieldMapper in, Conflicts conflicts) { + for (Parameter param : getParameters()) { + if (param.updateable == false) { + param.freeze(); + } + param.merge(in, conflicts); + } + } + + protected abstract List> getParameters(); + + @Override + public abstract ParametrizedFieldMapper build(BuilderContext context); + + protected String buildFullName(BuilderContext context) { + return context.path().pathAsText(name); + } + + public final XContentBuilder toXContent(XContentBuilder builder, boolean includeDefaults) throws IOException { + for (Parameter parameter : getParameters()) { + parameter.toXContent(builder, includeDefaults); + } + return builder; + } + + public final void parse(String name, Map fieldNode) { + Map> paramsMap = new HashMap<>(); + for (Parameter param : getParameters()) { + paramsMap.put(param.name, param); + } + String type = (String) fieldNode.remove("type"); + for (Iterator> iterator = fieldNode.entrySet().iterator(); iterator.hasNext();) { + Map.Entry entry = iterator.next(); + final String propName = entry.getKey(); + final Object propNode = entry.getValue(); + Parameter parameter = paramsMap.get(propName); + if (parameter == null) { + throw new MapperParsingException("unknown parameter [" + propName + + "] on mapper [" + name + "] of type [" + type + "]"); + } + if (propNode == null && parameter.acceptsNull == false) { + throw new MapperParsingException("[" + propName + "] on mapper [" + name + + "] of type [" + type + "] must not have a [null] value"); + } + parameter.parse(name, propNode); + iterator.remove(); + } + } + } +} diff --git a/server/src/main/java/org/elasticsearch/index/mapper/TypeParsers.java b/server/src/main/java/org/elasticsearch/index/mapper/TypeParsers.java index b2f67ee03d5ac..899afe1629994 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/TypeParsers.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/TypeParsers.java @@ -172,12 +172,7 @@ public static void parseTextField(FieldMapper.Builder builder, String name, M /** * Parse the {@code meta} key of the mapping. */ - public static void parseMeta(FieldMapper.Builder builder, String name, Map fieldNode) { - Object metaObject = fieldNode.remove("meta"); - if (metaObject == null) { - // no meta - return; - } + public static Map parseMeta(String name, Object metaObject) { if (metaObject instanceof Map == false) { throw new MapperParsingException("[meta] must be an object, got " + metaObject.getClass().getSimpleName() + "[" + metaObject + "] for field [" + name +"]"); @@ -210,17 +205,18 @@ public static void parseMeta(FieldMapper.Builder builder, String name, Map, Object> entryValueFunction = Map.Entry::getValue; final Function stringCast = String.class::cast; - Map checkedMeta = meta.entrySet().stream() + return meta.entrySet().stream() .collect(Collectors.toUnmodifiableMap(Map.Entry::getKey, entryValueFunction.andThen(stringCast))); - builder.meta(checkedMeta); } + + /** * Parse common field attributes such as {@code doc_values} or {@code store}. */ public static void parseField(FieldMapper.Builder builder, String name, Map fieldNode, Mapper.TypeParser.ParserContext parserContext) { - parseMeta(builder, name, fieldNode); + builder.meta(parseMeta(name, fieldNode)); for (Iterator> iterator = fieldNode.entrySet().iterator(); iterator.hasNext();) { Map.Entry entry = iterator.next(); final String propName = entry.getKey(); From 56ad229abe0592f771626c5ea076894841680085 Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Mon, 29 Jun 2020 12:16:44 +0100 Subject: [PATCH 02/11] dedicated tests for parametrized mapper --- .../index/mapper/BinaryFieldMapper.java | 18 +- .../index/mapper/BooleanFieldMapper.java | 10 +- .../index/mapper/CompletionFieldMapper.java | 2 +- .../index/mapper/ContentPath.java | 7 + .../index/mapper/DateFieldMapper.java | 2 +- .../index/mapper/FieldMapper.java | 32 ++- .../index/mapper/IpFieldMapper.java | 2 +- .../index/mapper/ParametrizedFieldMapper.java | 54 +++-- .../index/mapper/RangeFieldMapper.java | 2 +- .../index/mapper/TypeParsers.java | 28 ++- .../index/mapper/BinaryFieldMapperTests.java | 14 +- .../index/mapper/BooleanFieldMapperTests.java | 15 +- .../index/mapper/ParametrizedMapperTests.java | 200 ++++++++++++++++++ 13 files changed, 319 insertions(+), 67 deletions(-) create mode 100644 server/src/test/java/org/elasticsearch/index/mapper/ParametrizedMapperTests.java diff --git a/server/src/main/java/org/elasticsearch/index/mapper/BinaryFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/BinaryFieldMapper.java index 91993799548c5..eb99848d44cc4 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/BinaryFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/BinaryFieldMapper.java @@ -67,13 +67,19 @@ private static BinaryFieldMapper toType(FieldMapper in) { public static class Builder extends ParametrizedFieldMapper.Builder { - final Parameter stored = Parameter.boolParam("stored", false, m -> toType(m).stored, false); + final Parameter stored = Parameter.boolParam("store", false, m -> toType(m).stored, false); final Parameter hasDocValues = Parameter.boolParam("doc_values", false, m -> toType(m).hasDocValues, false); public Builder(String name) { super(name); } + // For testing + public Builder docValues(boolean hasDocValues) { + this.hasDocValues.update(hasDocValues); + return this; + } + @Override public List> getParameters() { return List.of(meta, stored, hasDocValues); @@ -82,7 +88,7 @@ public List> getParameters() { @Override public BinaryFieldMapper build(BuilderContext context) { return new BinaryFieldMapper(name, new BinaryFieldType(buildFullName(context), hasDocValues.getValue(), meta.getValue()), - multiFieldsBuilder.build(this, context), copyTo, stored.getValue(), hasDocValues.getValue()); + multiFieldsBuilder.build(this, context), copyTo.build(), this); } } @@ -91,7 +97,7 @@ public static class TypeParser implements Mapper.TypeParser { public BinaryFieldMapper.Builder parse(String name, Map node, ParserContext parserContext) throws MapperParsingException { BinaryFieldMapper.Builder builder = new BinaryFieldMapper.Builder(name); - builder.parse(name, node); + builder.parse(name, parserContext, node); return builder; } } @@ -170,10 +176,10 @@ public Query termQuery(Object value, QueryShardContext context) { private final boolean hasDocValues; protected BinaryFieldMapper(String simpleName, MappedFieldType mappedFieldType, - MultiFields multiFields, CopyTo copyTo, boolean stored, boolean hasDocValues) { + MultiFields multiFields, CopyTo copyTo, Builder builder) { super(simpleName, mappedFieldType, multiFields, copyTo); - this.stored = stored; - this.hasDocValues = hasDocValues; + this.stored = builder.stored.getValue(); + this.hasDocValues = builder.hasDocValues.getValue(); } @Override diff --git a/server/src/main/java/org/elasticsearch/index/mapper/BooleanFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/BooleanFieldMapper.java index 367d80e78bca0..1181dbdb32a17 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/BooleanFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/BooleanFieldMapper.java @@ -100,7 +100,7 @@ protected List> getParameters() { public BooleanFieldMapper build(BuilderContext context) { return new BooleanFieldMapper(name, new BooleanFieldType(buildFullName(context), indexed.getValue(), docValues.getValue(), meta.getValue()), - multiFieldsBuilder.build(this, context), copyTo, nullValue.getValue(), stored.getValue()); + multiFieldsBuilder.build(this, context), copyTo.build(), this); } } @@ -109,7 +109,7 @@ public static class TypeParser implements Mapper.TypeParser { public BooleanFieldMapper.Builder parse(String name, Map node, ParserContext parserContext) throws MapperParsingException { BooleanFieldMapper.Builder builder = new BooleanFieldMapper.Builder(name); - builder.parse(name, node); + builder.parse(name, parserContext, node); return builder; } } @@ -219,10 +219,10 @@ public Query rangeQuery(Object lowerTerm, Object upperTerm, boolean includeLower private final boolean stored; protected BooleanFieldMapper(String simpleName, MappedFieldType mappedFieldType, - MultiFields multiFields, CopyTo copyTo, Boolean nullValue, boolean stored) { + MultiFields multiFields, CopyTo copyTo, Builder builder) { super(simpleName, mappedFieldType, multiFields, copyTo); - this.nullValue = nullValue; - this.stored = stored; + this.nullValue = builder.nullValue.getValue(); + this.stored = builder.stored.getValue(); } @Override diff --git a/server/src/main/java/org/elasticsearch/index/mapper/CompletionFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/CompletionFieldMapper.java index 55f2d745d0694..7c7d0b391c4bf 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/CompletionFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/CompletionFieldMapper.java @@ -158,7 +158,7 @@ public Mapper.Builder parse(String name, Map node, ParserCont } else if (Fields.CONTEXTS.match(fieldName, LoggingDeprecationHandler.INSTANCE)) { builder.contextMappings(ContextMappings.load(fieldNode, parserContext.indexVersionCreated())); iterator.remove(); - } else if (parseMultiField(builder, name, parserContext, fieldName, fieldNode)) { + } else if (parseMultiField(builder::addMultiField, name, parserContext, fieldName, fieldNode)) { iterator.remove(); } } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/ContentPath.java b/server/src/main/java/org/elasticsearch/index/mapper/ContentPath.java index 7f3e26312ad87..015293864e55b 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ContentPath.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ContentPath.java @@ -45,6 +45,13 @@ public ContentPath(int offset) { this.index = 0; } + public ContentPath(String path) { + this.sb = new StringBuilder(); + this.offset = 0; + this.index = 0; + add(path); + } + public void add(String name) { path[index++] = name; if (index == path.length) { // expand if needed diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java index 98f222e822477..59cb203faa1d9 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java @@ -287,7 +287,7 @@ public Mapper.Builder parse(String name, Map node, ParserCont } else if (propName.equals("format")) { builder.format(propNode.toString()); iterator.remove(); - } else if (TypeParsers.parseMultiField(builder, name, parserContext, propName, propNode)) { + } else if (TypeParsers.parseMultiField(builder::addMultiField, name, parserContext, propName, propNode)) { iterator.remove(); } } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java index a068271dd4836..de2984e08a65d 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java @@ -27,6 +27,7 @@ import org.elasticsearch.common.collect.ImmutableOpenMap; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Setting.Property; +import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.support.AbstractXContentParser; @@ -483,7 +484,7 @@ public static String termVectorOptionsToString(FieldType fieldType) { protected abstract String contentType(); - public static class MultiFields { + public static class MultiFields implements Iterable { public static MultiFields empty() { return new MultiFields(ImmutableOpenMap.of()); @@ -498,8 +499,29 @@ public Builder add(Mapper.Builder builder) { return this; } + public Builder add(Mapper mapper) { + mapperBuilders.put(mapper.simpleName(), new Mapper.Builder(mapper.simpleName()) { + @Override + public Mapper build(BuilderContext context) { + return mapper; + } + }); + return this; + } + + public Builder update(Mapper toMerge, ContentPath contentPath) { + if (mapperBuilders.containsKey(toMerge.simpleName()) == false) { + add(toMerge); + } else { + Mapper.Builder builder = mapperBuilders.get(toMerge.simpleName()); + Mapper existing = builder.build(new BuilderContext(Settings.EMPTY, contentPath)); + add(existing.merge(toMerge)); + } + return this; + } + @SuppressWarnings("unchecked") - public MultiFields build(FieldMapper.Builder mainFieldBuilder, BuilderContext context) { + public MultiFields build(Mapper.Builder mainFieldBuilder, BuilderContext context) { if (mapperBuilders.isEmpty()) { return empty(); } else { @@ -564,6 +586,7 @@ public MultiFields merge(MultiFields mergeWith) { return new MultiFields(mappers); } + @Override public Iterator iterator() { return StreamSupport.stream(mappers.values().spliterator(), false).map((p) -> (Mapper)p.value).iterator(); } @@ -630,6 +653,11 @@ public CopyTo build() { } return new CopyTo(Collections.unmodifiableList(copyToBuilders)); } + + public void reset(CopyTo copyTo) { + copyToBuilders.clear(); + copyToBuilders.addAll(copyTo.copyToFields); + } } public List copyToFields() { diff --git a/server/src/main/java/org/elasticsearch/index/mapper/IpFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/IpFieldMapper.java index d836bc0284eb3..42dd60cff421c 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/IpFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/IpFieldMapper.java @@ -130,7 +130,7 @@ public Mapper.Builder parse(String name, Map node, ParserCont } else if (propName.equals("ignore_malformed")) { builder.ignoreMalformed(XContentMapValues.nodeBooleanValue(propNode, name + ".ignore_malformed")); iterator.remove(); - } else if (TypeParsers.parseMultiField(builder, name, parserContext, propName, propNode)) { + } else if (TypeParsers.parseMultiField(builder::addMultiField, name, parserContext, propName, propNode)) { iterator.remove(); } } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/ParametrizedFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/ParametrizedFieldMapper.java index 1f458f2409dd1..3a0bb267fb8bb 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ParametrizedFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ParametrizedFieldMapper.java @@ -21,6 +21,7 @@ import joptsimple.internal.Strings; import org.apache.lucene.document.FieldType; +import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.support.XContentMapValues; @@ -43,27 +44,31 @@ protected ParametrizedFieldMapper(String simpleName, MappedFieldType mappedField public abstract ParametrizedFieldMapper.Builder getMergeBuilder(); - public ParametrizedFieldMapper(String simpleName, FieldType fieldType, MappedFieldType mappedFieldType, MultiFields multiFields, CopyTo copyTo) { - super(simpleName, fieldType, mappedFieldType, multiFields, copyTo); - } - @Override public final ParametrizedFieldMapper merge(Mapper mergeWith) { - if (mergeWith instanceof ParametrizedFieldMapper == false) { - throw new IllegalArgumentException("mapper [" + mappedFieldType.name() + "] cannot be changed from type [" + if (mergeWith instanceof FieldMapper == false) { + throw new IllegalArgumentException("mapper [" + name() + "] cannot be changed from type [" + contentType() + "] to [" + mergeWith.getClass().getSimpleName() + "]"); } if (Objects.equals(this.getClass(), mergeWith.getClass()) == false) { - throw new IllegalArgumentException("mapper [" + mappedFieldType.name() + "] cannot be changed from type [" + throw new IllegalArgumentException("mapper [" + name() + "] cannot be changed from type [" + contentType() + "] to [" + ((FieldMapper) mergeWith).contentType() + "]"); } ParametrizedFieldMapper.Builder builder = getMergeBuilder(); - Conflicts conflicts = new Conflicts(simpleName()); + Conflicts conflicts = new Conflicts(name()); builder.merge((FieldMapper) mergeWith, conflicts); conflicts.check(); - return builder.build(new BuilderContext(null, null)); + return builder.build(new BuilderContext(Settings.EMPTY, parentPath(name()))); + } + + private static ContentPath parentPath(String name) { + int endPos = name.lastIndexOf("."); + if (endPos == -1) { + return new ContentPath(0); + } + return new ContentPath(name.substring(endPos)); } @Override @@ -77,6 +82,8 @@ public final XContentBuilder toXContent(XContentBuilder builder, Params params) builder.field("type", contentType()); boolean includeDefaults = params.paramAsBoolean("include_defaults", false); getMergeBuilder().toXContent(builder, includeDefaults); + multiFields.toXContent(builder, params); + copyTo.toXContent(builder, params); return builder.endObject(); } @@ -85,8 +92,8 @@ public static final class Parameter { private final T defaultValue; private final BiFunction parser; private final Function merger; - private final String name; - private final boolean updateable; + public final String name; + public final boolean updateable; private boolean acceptsNull = false; private T value; private boolean frozen = false; @@ -185,22 +192,25 @@ public void check() { } - public abstract static class Builder extends FieldMapper.Builder { + public abstract static class Builder extends Mapper.Builder { protected final MultiFields.Builder multiFieldsBuilder = new MultiFields.Builder(); - protected CopyTo copyTo = CopyTo.empty(); + protected CopyTo.Builder copyTo = new CopyTo.Builder(); protected final Parameter> meta = new Parameter<>("meta", true, Collections.emptyMap(), TypeParsers::parseMeta, m -> m.fieldType().meta()); protected Builder(String name) { - super(name, new FieldType()); + super(name); } protected Builder init(FieldMapper base) { for (Parameter param : getParameters()) { param.init(base); } + for (Mapper subField : base.multiFields) { + multiFieldsBuilder.add(subField); + } return this; } @@ -211,6 +221,10 @@ public final void merge(FieldMapper in, Conflicts conflicts) { } param.merge(in, conflicts); } + for (Mapper newSubField : in.multiFields) { + multiFieldsBuilder.update(newSubField, parentPath(newSubField.name())); + } + this.copyTo.reset(in.copyTo); } protected abstract List> getParameters(); @@ -229,7 +243,7 @@ public final XContentBuilder toXContent(XContentBuilder builder, boolean include return builder; } - public final void parse(String name, Map fieldNode) { + public final void parse(String name, TypeParser.ParserContext parserContext, Map fieldNode) { Map> paramsMap = new HashMap<>(); for (Parameter param : getParameters()) { paramsMap.put(param.name, param); @@ -239,6 +253,16 @@ public final void parse(String name, Map fieldNode) { Map.Entry entry = iterator.next(); final String propName = entry.getKey(); final Object propNode = entry.getValue(); + if (Objects.equals("fields", propName)) { + TypeParsers.parseMultiField(multiFieldsBuilder::add, name, parserContext, propName, propNode); + iterator.remove(); + continue; + } + if (Objects.equals("copy_to", propName)) { + TypeParsers.parseCopyFields(propNode).forEach(copyTo::add); + iterator.remove(); + continue; + } Parameter parameter = paramsMap.get(propName); if (parameter == null) { throw new MapperParsingException("unknown parameter [" + propName diff --git a/server/src/main/java/org/elasticsearch/index/mapper/RangeFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/RangeFieldMapper.java index b6336887f69db..e643e44692ba3 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RangeFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RangeFieldMapper.java @@ -172,7 +172,7 @@ public Mapper.Builder parse(String name, Map node, } else if (propName.equals("format")) { builder.format(propNode.toString()); iterator.remove(); - } else if (TypeParsers.parseMultiField(builder, name, parserContext, propName, propNode)) { + } else if (TypeParsers.parseMultiField(builder::addMultiField, name, parserContext, propName, propNode)) { iterator.remove(); } } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/TypeParsers.java b/server/src/main/java/org/elasticsearch/index/mapper/TypeParsers.java index ac6d134b85315..4a1e107689074 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/TypeParsers.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/TypeParsers.java @@ -30,10 +30,12 @@ import org.elasticsearch.index.analysis.NamedAnalyzer; import org.elasticsearch.index.similarity.SimilarityProvider; +import java.util.ArrayList; import java.util.Collections; import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.function.Consumer; import java.util.function.Function; import java.util.stream.Collectors; @@ -226,7 +228,6 @@ public static Map parseMeta(String name, Object metaObject) { */ public static void parseField(FieldMapper.Builder builder, String name, Map fieldNode, Mapper.TypeParser.ParserContext parserContext) { - builder.meta(parseMeta(name, fieldNode)); for (Iterator> iterator = fieldNode.entrySet().iterator(); iterator.hasNext();) { Map.Entry entry = iterator.next(); final String propName = entry.getKey(); @@ -235,6 +236,9 @@ public static void parseField(FieldMapper.Builder builder, String name, Map builder, String name, Map copyFields = parseCopyFields(propNode); + FieldMapper.CopyTo.Builder cpBuilder = new FieldMapper.CopyTo.Builder(); + copyFields.forEach(cpBuilder::add); + builder.copyTo(cpBuilder.build()); } iterator.remove(); } @@ -266,7 +273,7 @@ public static void parseField(FieldMapper.Builder builder, String name, Map multiFieldsBuilder, String name, Mapper.TypeParser.ParserContext parserContext, String propName, Object propNode) { if (propName.equals("fields")) { if (parserContext.isWithinMultiField()) { @@ -324,7 +331,7 @@ public static boolean parseMultiField(FieldMapper.Builder builder, String name, if (typeParser == null) { throw new MapperParsingException("no handler for type [" + type + "] declared on field [" + multiFieldName + "]"); } - builder.addMultiField(typeParser.parse(multiFieldName, multiFieldNodes, parserContext)); + multiFieldsBuilder.accept(typeParser.parse(multiFieldName, multiFieldNodes, parserContext)); multiFieldNodes.remove("type"); DocumentMapperParser.checkNoRemainingFields(propName, multiFieldNodes, parserContext.indexVersionCreated()); } @@ -380,17 +387,16 @@ public static void parseTermVector(String fieldName, String termVector, FieldMap } } - @SuppressWarnings({"unchecked", "rawtypes"}) - public static void parseCopyFields(Object propNode, FieldMapper.Builder builder) { - FieldMapper.CopyTo.Builder copyToBuilder = new FieldMapper.CopyTo.Builder(); + public static List parseCopyFields(Object propNode) { + List copyFields = new ArrayList<>(); if (isArray(propNode)) { for (Object node : (List) propNode) { - copyToBuilder.add(nodeStringValue(node, null)); + copyFields.add(nodeStringValue(node, null)); } } else { - copyToBuilder.add(nodeStringValue(propNode, null)); + copyFields.add(nodeStringValue(propNode, null)); } - builder.copyTo(copyToBuilder.build()); + return copyFields; } public static SimilarityProvider resolveSimilarity(Mapper.TypeParser.ParserContext parserContext, String name, String value) { diff --git a/server/src/test/java/org/elasticsearch/index/mapper/BinaryFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/BinaryFieldMapperTests.java index dad6ac927a667..2de5ab8f75d79 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/BinaryFieldMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/BinaryFieldMapperTests.java @@ -32,28 +32,18 @@ import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.plugins.Plugin; +import org.elasticsearch.test.ESSingleNodeTestCase; import org.elasticsearch.test.InternalSettingsPlugin; import java.io.IOException; import java.util.Arrays; import java.util.Collection; -import java.util.Set; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.instanceOf; -public class BinaryFieldMapperTests extends FieldMapperTestCase { - - @Override - protected Set unsupportedProperties() { - return Set.of("analyzer", "eager_global_ordinals", "norms", "similarity", "index"); - } - - @Override - protected BinaryFieldMapper.Builder newBuilder() { - return new BinaryFieldMapper.Builder("binary"); - } +public class BinaryFieldMapperTests extends ESSingleNodeTestCase { @Override protected Collection> getPlugins() { diff --git a/server/src/test/java/org/elasticsearch/index/mapper/BooleanFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/BooleanFieldMapperTests.java index 564be1efdca85..5dff9b8044904 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/BooleanFieldMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/BooleanFieldMapperTests.java @@ -41,25 +41,21 @@ import org.elasticsearch.index.mapper.MapperService.MergeReason; import org.elasticsearch.index.mapper.ParseContext.Document; import org.elasticsearch.plugins.Plugin; +import org.elasticsearch.test.ESSingleNodeTestCase; import org.elasticsearch.test.InternalSettingsPlugin; import org.junit.Before; import java.io.IOException; import java.util.Collection; import java.util.Collections; -import java.util.Set; import static org.hamcrest.Matchers.containsString; -public class BooleanFieldMapperTests extends FieldMapperTestCase { +public class BooleanFieldMapperTests extends ESSingleNodeTestCase { + private IndexService indexService; private DocumentMapperParser parser; - @Override - protected Set unsupportedProperties() { - return Set.of("analyzer", "similarity"); - } - @Before public void setup() { indexService = createIndex("test"); @@ -285,9 +281,4 @@ public void testMeta() throws Exception { assertEquals(mapping3, mapper.mappingSource().toString()); } - @Override - protected BooleanFieldMapper.Builder newBuilder() { - return new BooleanFieldMapper.Builder("boolean"); - } - } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ParametrizedMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/ParametrizedMapperTests.java new file mode 100644 index 0000000000000..efb14f1228586 --- /dev/null +++ b/server/src/test/java/org/elasticsearch/index/mapper/ParametrizedMapperTests.java @@ -0,0 +1,200 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.index.mapper; + +import org.elasticsearch.Version; +import org.elasticsearch.common.Strings; +import org.elasticsearch.common.settings.Settings; +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.test.ESTestCase; + +import java.io.IOException; +import java.util.List; +import java.util.Map; +import java.util.Objects; + +public class ParametrizedMapperTests extends ESTestCase { + + private static TestMapper toType(Mapper in) { + return (TestMapper) in; + } + + public static class Builder extends ParametrizedFieldMapper.Builder { + + final ParametrizedFieldMapper.Parameter fixed + = ParametrizedFieldMapper.Parameter.boolParam("fixed", false, m -> toType(m).fixed, true); + final ParametrizedFieldMapper.Parameter fixed2 + = ParametrizedFieldMapper.Parameter.boolParam("fixed2", false, m -> toType(m).fixed2, false); + final ParametrizedFieldMapper.Parameter variable + = ParametrizedFieldMapper.Parameter.stringParam("variable", true, m -> toType(m).variable, "default"); + + protected Builder(String name) { + super(name); + } + + @Override + protected List> getParameters() { + return List.of(fixed, fixed2, variable); + } + + @Override + public ParametrizedFieldMapper build(Mapper.BuilderContext context) { + return new TestMapper(buildFullName(context), fixed.getValue(), fixed2.getValue(), variable.getValue(), + multiFieldsBuilder.build(this, context), copyTo.build()); + } + } + + public static class TypeParser implements Mapper.TypeParser { + + @Override + public Mapper.Builder parse(String name, Map node, ParserContext parserContext) throws MapperParsingException { + Builder builder = new Builder(name); + builder.parse(name, parserContext, node); + return builder; + } + } + + public static class TestMapper extends ParametrizedFieldMapper { + + private final boolean fixed; + private final boolean fixed2; + private final String variable; + + protected TestMapper(String simpleName, boolean fixed, boolean fixed2, String variable, MultiFields multiFields, CopyTo copyTo) { + super(simpleName, new KeywordFieldMapper.KeywordFieldType("test"), multiFields, copyTo); + this.fixed = fixed; + this.fixed2 = fixed2; + this.variable = variable; + } + + @Override + public Builder getMergeBuilder() { + return new ParametrizedMapperTests.Builder(name()).init(this); + } + + @Override + protected void parseCreateField(ParseContext context) throws IOException { + + } + + @Override + protected String contentType() { + return "test_mapper"; + } + } + + private static TestMapper fromMapping(String mapping) { + Mapper.TypeParser.ParserContext pc = new Mapper.TypeParser.ParserContext(s -> null, null, s -> { + if (Objects.equals("keyword", s)) { + return new KeywordFieldMapper.TypeParser(); + } + if (Objects.equals("binary", s)) { + return new BinaryFieldMapper.TypeParser(); + } + return null; + }, Version.CURRENT, () -> null); + return (TestMapper) new TypeParser() + .parse("test", XContentHelper.convertToMap(JsonXContent.jsonXContent, mapping, true), pc) + .build(new Mapper.BuilderContext(Settings.EMPTY, new ContentPath(0))); + } + + // defaults - create empty builder config, and serialize with and without defaults + public void testDefaults() throws IOException { + String mapping = "{\"type\":\"test_mapper\"}"; + TestMapper mapper = fromMapping(mapping); + + assertTrue(mapper.fixed); + assertEquals("default", mapper.variable); + + assertEquals("{\"test\":" + mapping + "}", Strings.toString(mapper)); + + XContentBuilder builder = JsonXContent.contentBuilder(); + ToXContent.Params params = new ToXContent.MapParams(Map.of("include_defaults", "true")); + builder.startObject(); + mapper.toXContent(builder, params); + builder.endObject(); + assertEquals("{\"test\":{\"type\":\"test_mapper\",\"fixed\":true,\"variable\":\"default\"}}", + Strings.toString(builder)); + } + + // merging - try updating 'fixed' and 'fixed2' should get an error, try updating 'variable' and verify update + public void testMerging() { + String mapping = "{\"type\":\"test_mapper\",\"fixed\":false}"; + TestMapper mapper = fromMapping(mapping); + assertEquals("{\"test\":" + mapping + "}", Strings.toString(mapper)); + + TestMapper badMerge = fromMapping("{\"type\":\"test_mapper\",\"fixed\":true,\"fixed2\":true}"); + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> mapper.merge(badMerge)); + String expectedError = "Mapper for [test] conflicts with existing mapper:\n" + + "\tCannot update parameter [fixed] from [false] to [true]\n" + + "\tCannot update parameter [fixed2] from [false] to [true]"; + assertEquals(expectedError, e.getMessage()); + + assertEquals("{\"test\":" + mapping + "}", Strings.toString(mapper)); // original mapping is unaffected + + // TODO: should we have to include 'fixed' here? Or should updates take as 'defaults' the existing values? + TestMapper goodMerge = fromMapping("{\"type\":\"test_mapper\",\"fixed\":false,\"variable\":\"updated\"}"); + TestMapper merged = (TestMapper) mapper.merge(goodMerge); + + assertEquals("{\"test\":" + mapping + "}", Strings.toString(mapper)); // original mapping is unaffected + assertEquals("{\"test\":{\"type\":\"test_mapper\",\"fixed\":false,\"variable\":\"updated\"}}", Strings.toString(merged)); + + } + + // add multifield, verify, add second multifield, verify, overwrite second multifield + public void testMultifields() { + String mapping = "{\"type\":\"test_mapper\",\"variable\":\"foo\",\"fields\":{\"sub\":{\"type\":\"keyword\"}}}"; + TestMapper mapper = fromMapping(mapping); + assertEquals("{\"test\":" + mapping + "}", Strings.toString(mapper)); + + String addSubField = "{\"type\":\"test_mapper\",\"variable\":\"foo\",\"fields\":{\"sub2\":{\"type\":\"keyword\"}}}"; + TestMapper toMerge = fromMapping(addSubField); + TestMapper merged = (TestMapper) mapper.merge(toMerge); + assertEquals("{\"test\":{\"type\":\"test_mapper\",\"variable\":\"foo\"," + + "\"fields\":{\"sub\":{\"type\":\"keyword\"},\"sub2\":{\"type\":\"keyword\"}}}}", Strings.toString(merged)); + + String badSubField = "{\"type\":\"test_mapper\",\"variable\":\"foo\",\"fields\":{\"sub2\":{\"type\":\"binary\"}}}"; + TestMapper badToMerge = fromMapping(badSubField); + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> merged.merge(badToMerge)); + assertEquals("mapper [test.sub2] cannot be changed from type [keyword] to [binary]", e.getMessage()); + } + + // add copy_to, verify + public void testCopyTo() { + String mapping = "{\"type\":\"test_mapper\",\"variable\":\"foo\",\"copy_to\":[\"other\"]}"; + TestMapper mapper = fromMapping(mapping); + assertEquals("{\"test\":" + mapping + "}", Strings.toString(mapper)); + + // On update, copy_to is completely replaced + + TestMapper toMerge = fromMapping("{\"type\":\"test_mapper\",\"variable\":\"updated\",\"copy_to\":[\"foo\",\"bar\"]}"); + TestMapper merged = (TestMapper) mapper.merge(toMerge); + assertEquals("{\"test\":{\"type\":\"test_mapper\",\"variable\":\"updated\",\"copy_to\":[\"foo\",\"bar\"]}}", + Strings.toString(merged)); + + TestMapper removeCopyTo = fromMapping("{\"type\":\"test_mapper\",\"variable\":\"updated\"}"); + TestMapper noCopyTo = (TestMapper) merged.merge(removeCopyTo); + assertEquals("{\"test\":{\"type\":\"test_mapper\",\"variable\":\"updated\"}}", Strings.toString(noCopyTo)); + } + +} From da0aca5066de8525dd97a03a76265e351566c021 Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Mon, 29 Jun 2020 12:45:21 +0100 Subject: [PATCH 03/11] tests --- .../index/mapper/BooleanFieldMapper.java | 8 ++------ .../index/mapper/NullValueTests.java | 20 ++++++++++--------- .../index/mapper/ParametrizedMapperTests.java | 2 +- .../index/mapper/RootObjectMapperTests.java | 4 ++-- 4 files changed, 16 insertions(+), 18 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/BooleanFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/BooleanFieldMapper.java index 1181dbdb32a17..cb2ddbcc604db 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/BooleanFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/BooleanFieldMapper.java @@ -79,12 +79,8 @@ public static class Builder extends ParametrizedFieldMapper.Builder { = Parameter.boolParam("doc_values", false, m -> m.fieldType().hasDocValues(), true); private final Parameter indexed = Parameter.boolParam("index", false, m -> m.fieldType().isSearchable(), true); - private final Parameter nullValue = new Parameter<>("null_value", false, null, (n, o) -> { - if (o == null) { - return null; - } - return XContentMapValues.nodeBooleanValue(o); - }, m -> toType(m).nullValue).acceptsNull(); + private final Parameter nullValue + = new Parameter<>("null_value", false, null, (n, o) -> XContentMapValues.nodeBooleanValue(o), m -> toType(m).nullValue); private final Parameter stored = Parameter.boolParam("store", false, m -> toType(m).stored, false); public Builder(String name) { diff --git a/server/src/test/java/org/elasticsearch/index/mapper/NullValueTests.java b/server/src/test/java/org/elasticsearch/index/mapper/NullValueTests.java index f38f83b6e418f..fce744553456a 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/NullValueTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/NullValueTests.java @@ -2,6 +2,14 @@ import org.elasticsearch.common.Strings; import org.elasticsearch.common.compress.CompressedXContent; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.xcontent.XContentFactory; +import org.elasticsearch.index.IndexService; +import org.elasticsearch.test.ESSingleNodeTestCase; + +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.either; +import static org.hamcrest.Matchers.equalTo; /* * Licensed to Elasticsearch under one or more contributor @@ -22,14 +30,6 @@ * under the License. */ - -import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.common.xcontent.XContentFactory; -import org.elasticsearch.index.IndexService; -import org.elasticsearch.test.ESSingleNodeTestCase; - -import static org.hamcrest.Matchers.equalTo; - public class NullValueTests extends ESSingleNodeTestCase { public void testNullNullValue() throws Exception { IndexService indexService = createIndex("test", Settings.builder().build()); @@ -52,7 +52,9 @@ public void testNullNullValue() throws Exception { indexService.mapperService().documentMapperParser().parse("type", new CompressedXContent(mapping)); fail("Test should have failed because [null_value] was null."); } catch (MapperParsingException e) { - assertThat(e.getMessage(), equalTo("Property [null_value] cannot be null.")); + assertThat(e.getMessage(), + either(equalTo("Property [null_value] cannot be null.")) + .or(containsString("must not have a [null] value"))); } } } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ParametrizedMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/ParametrizedMapperTests.java index efb14f1228586..c07d3d5376883 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ParametrizedMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ParametrizedMapperTests.java @@ -133,7 +133,7 @@ public void testDefaults() throws IOException { builder.startObject(); mapper.toXContent(builder, params); builder.endObject(); - assertEquals("{\"test\":{\"type\":\"test_mapper\",\"fixed\":true,\"variable\":\"default\"}}", + assertEquals("{\"test\":{\"type\":\"test_mapper\",\"fixed\":true,\"fixed2\":false,\"variable\":\"default\"}}", Strings.toString(builder)); } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java index d4e8864926eb4..f32d0d3858bbe 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java @@ -353,8 +353,8 @@ public void testIllegalDynamicTemplateNoMappingType() throws Exception { mapping.endObject(); MapperParsingException e = expectThrows(MapperParsingException.class, () -> mapperService.merge("type", new CompressedXContent(Strings.toString(mapping)), MergeReason.MAPPING_UPDATE)); - assertThat(e.getRootCause(), instanceOf(IllegalArgumentException.class)); - assertThat(e.getRootCause().getMessage(), equalTo("Unused mapping attributes [{foo=bar}]")); + assertThat(e.getRootCause(), instanceOf(MapperParsingException.class)); + assertThat(e.getRootCause().getMessage(), equalTo("unknown parameter [foo] on mapper [__dummy__] of type [null]")); } } From 6e99626bb19412788cb6f08a61291c7e573f1579 Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Mon, 29 Jun 2020 12:48:29 +0100 Subject: [PATCH 04/11] precommit --- .../percolator/PercolatorFieldMapper.java | 2 -- .../index/mapper/ParametrizedFieldMapper.java | 12 ++++++++---- .../org/elasticsearch/index/mapper/TypeParsers.java | 4 ++-- .../xpack/analytics/mapper/HistogramFieldMapper.java | 2 +- .../mapper/ConstantKeywordFieldMapper.java | 6 +++--- 5 files changed, 14 insertions(+), 12 deletions(-) diff --git a/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorFieldMapper.java b/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorFieldMapper.java index 253f86e149d12..60be035389ac6 100644 --- a/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorFieldMapper.java +++ b/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorFieldMapper.java @@ -157,8 +157,6 @@ static KeywordFieldMapper createExtractQueryFieldBuilder(String name, BuilderCon static BinaryFieldMapper createQueryBuilderFieldBuilder(BuilderContext context) { BinaryFieldMapper.Builder builder = new BinaryFieldMapper.Builder(QUERY_BUILDER_FIELD_NAME); builder.docValues(true); - builder.indexOptions(IndexOptions.NONE); - builder.store(false); return builder.build(context); } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/ParametrizedFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/ParametrizedFieldMapper.java index 3a0bb267fb8bb..c1d422fa81774 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ParametrizedFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ParametrizedFieldMapper.java @@ -98,7 +98,8 @@ public static final class Parameter { private T value; private boolean frozen = false; - public Parameter(String name, boolean updateable, T defaultValue, BiFunction parser, Function merger) { + public Parameter(String name, boolean updateable, T defaultValue, + BiFunction parser, Function merger) { this.name = name; this.defaultValue = defaultValue; this.value = defaultValue; @@ -155,15 +156,18 @@ public XContentBuilder toXContent(XContentBuilder builder, boolean includeDefaul return builder; } - public static Parameter boolParam(String name, boolean updateable, Function merger, boolean defaultValue) { + public static Parameter boolParam(String name, boolean updateable, + Function merger, boolean defaultValue) { return new Parameter<>(name, updateable, defaultValue, (n, o) -> XContentMapValues.nodeBooleanValue(o), merger); } - public static Parameter floatParam(String name, boolean updateable, Function merger, float defaultValue) { + public static Parameter floatParam(String name, boolean updateable, + Function merger, float defaultValue) { return new Parameter<>(name, updateable, defaultValue, (n, o) -> XContentMapValues.nodeFloatValue(o), merger); } - public static Parameter stringParam(String name, boolean updateable, Function merger, String defaultValue) { + public static Parameter stringParam(String name, boolean updateable, + Function merger, String defaultValue) { return new Parameter<>(name, updateable, defaultValue, (n, o) -> XContentMapValues.nodeStringValue(o, defaultValue), merger); } } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/TypeParsers.java b/server/src/main/java/org/elasticsearch/index/mapper/TypeParsers.java index 4a1e107689074..25e624342d936 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/TypeParsers.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/TypeParsers.java @@ -273,8 +273,8 @@ public static void parseField(FieldMapper.Builder builder, String name, Map multiFieldsBuilder, String name, Mapper.TypeParser.ParserContext parserContext, - String propName, Object propNode) { + public static boolean parseMultiField(Consumer multiFieldsBuilder, String name, + Mapper.TypeParser.ParserContext parserContext, String propName, Object propNode) { if (propName.equals("fields")) { if (parserContext.isWithinMultiField()) { // For indices created prior to 8.0, we only emit a deprecation warning and do not fail type parsing. This is to diff --git a/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/mapper/HistogramFieldMapper.java b/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/mapper/HistogramFieldMapper.java index 6d07c70974e21..5cfdc0c2ee58b 100644 --- a/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/mapper/HistogramFieldMapper.java +++ b/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/mapper/HistogramFieldMapper.java @@ -121,7 +121,7 @@ public static class TypeParser implements Mapper.TypeParser { public Mapper.Builder parse(String name, Map node, ParserContext parserContext) throws MapperParsingException { Builder builder = new HistogramFieldMapper.Builder(name); - TypeParsers.parseMeta(builder, name, node); + builder.meta(TypeParsers.parseMeta(name, node)); for (Iterator> iterator = node.entrySet().iterator(); iterator.hasNext();) { Map.Entry entry = iterator.next(); String propName = entry.getKey(); diff --git a/x-pack/plugin/mapper-constant-keyword/src/main/java/org/elasticsearch/xpack/constantkeyword/mapper/ConstantKeywordFieldMapper.java b/x-pack/plugin/mapper-constant-keyword/src/main/java/org/elasticsearch/xpack/constantkeyword/mapper/ConstantKeywordFieldMapper.java index 24067a70e5c50..c2c856666db80 100644 --- a/x-pack/plugin/mapper-constant-keyword/src/main/java/org/elasticsearch/xpack/constantkeyword/mapper/ConstantKeywordFieldMapper.java +++ b/x-pack/plugin/mapper-constant-keyword/src/main/java/org/elasticsearch/xpack/constantkeyword/mapper/ConstantKeywordFieldMapper.java @@ -102,7 +102,7 @@ public Mapper.Builder parse(String name, Map node, ParserCont if (value != null) { builder.setValue(value.toString()); } - TypeParsers.parseMeta(builder, name, node); + builder.meta(TypeParsers.parseMeta(name, node)); return builder; } } @@ -152,11 +152,11 @@ public String value() { public String typeName() { return CONTENT_TYPE; } - + @Override public String familyTypeName() { return KeywordFieldMapper.CONTENT_TYPE; - } + } @Override public IndexFieldData.Builder fielddataBuilder(String fullyQualifiedIndexName) { From 7326c49ec1bee5377382e200f3489b86909b36d9 Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Mon, 29 Jun 2020 13:58:42 +0100 Subject: [PATCH 05/11] meta parsing --- .../xpack/analytics/mapper/HistogramFieldMapper.java | 5 ++++- .../constantkeyword/mapper/ConstantKeywordFieldMapper.java | 4 +++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/mapper/HistogramFieldMapper.java b/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/mapper/HistogramFieldMapper.java index 5cfdc0c2ee58b..67832936cfa3e 100644 --- a/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/mapper/HistogramFieldMapper.java +++ b/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/mapper/HistogramFieldMapper.java @@ -121,7 +121,6 @@ public static class TypeParser implements Mapper.TypeParser { public Mapper.Builder parse(String name, Map node, ParserContext parserContext) throws MapperParsingException { Builder builder = new HistogramFieldMapper.Builder(name); - builder.meta(TypeParsers.parseMeta(name, node)); for (Iterator> iterator = node.entrySet().iterator(); iterator.hasNext();) { Map.Entry entry = iterator.next(); String propName = entry.getKey(); @@ -130,6 +129,10 @@ public Mapper.Builder parse(String name, Map node, Pars builder.ignoreMalformed(XContentMapValues.nodeBooleanValue(propNode, name + "." + Names.IGNORE_MALFORMED)); iterator.remove(); } + if (propName.equals("meta")) { + builder.meta(TypeParsers.parseMeta(propName, propNode)); + iterator.remove(); + } } return builder; } diff --git a/x-pack/plugin/mapper-constant-keyword/src/main/java/org/elasticsearch/xpack/constantkeyword/mapper/ConstantKeywordFieldMapper.java b/x-pack/plugin/mapper-constant-keyword/src/main/java/org/elasticsearch/xpack/constantkeyword/mapper/ConstantKeywordFieldMapper.java index c2c856666db80..f8dc530e26a5f 100644 --- a/x-pack/plugin/mapper-constant-keyword/src/main/java/org/elasticsearch/xpack/constantkeyword/mapper/ConstantKeywordFieldMapper.java +++ b/x-pack/plugin/mapper-constant-keyword/src/main/java/org/elasticsearch/xpack/constantkeyword/mapper/ConstantKeywordFieldMapper.java @@ -102,7 +102,9 @@ public Mapper.Builder parse(String name, Map node, ParserCont if (value != null) { builder.setValue(value.toString()); } - builder.meta(TypeParsers.parseMeta(name, node)); + if (node.containsKey("meta")) { + builder.meta(TypeParsers.parseMeta(name, node.remove("meta"))); + } return builder; } } From 5bb39b2de4645b21b88b06959e24a1ccdbb0aadb Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Tue, 30 Jun 2020 10:03:24 +0100 Subject: [PATCH 06/11] Use simple name in merge builder --- .../index/mapper/BinaryFieldMapper.java | 2 +- .../index/mapper/BooleanFieldMapper.java | 2 +- .../index/mapper/ParametrizedMapperTests.java | 50 +++++++++++++++---- 3 files changed, 43 insertions(+), 11 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/BinaryFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/BinaryFieldMapper.java index eb99848d44cc4..7664e9dc62353 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/BinaryFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/BinaryFieldMapper.java @@ -221,7 +221,7 @@ protected void parseCreateField(ParseContext context) throws IOException { @Override public ParametrizedFieldMapper.Builder getMergeBuilder() { - return new BinaryFieldMapper.Builder(name()).init(this); + return new BinaryFieldMapper.Builder(simpleName()).init(this); } @Override diff --git a/server/src/main/java/org/elasticsearch/index/mapper/BooleanFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/BooleanFieldMapper.java index cb2ddbcc604db..68e37f49a0fb6 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/BooleanFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/BooleanFieldMapper.java @@ -262,7 +262,7 @@ protected void parseCreateField(ParseContext context) throws IOException { @Override public ParametrizedFieldMapper.Builder getMergeBuilder() { - return new Builder(name()).init(this); + return new Builder(simpleName()).init(this); } @Override diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ParametrizedMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/ParametrizedMapperTests.java index c07d3d5376883..8f89e014ad9d9 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ParametrizedMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ParametrizedMapperTests.java @@ -21,19 +21,37 @@ import org.elasticsearch.Version; import org.elasticsearch.common.Strings; +import org.elasticsearch.common.compress.CompressedXContent; import org.elasticsearch.common.settings.Settings; 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.test.ESTestCase; +import org.elasticsearch.index.query.QueryShardContext; +import org.elasticsearch.plugins.MapperPlugin; +import org.elasticsearch.plugins.Plugin; +import org.elasticsearch.test.AbstractBuilderTestCase; +import org.elasticsearch.test.TestGeoShapeFieldMapperPlugin; import java.io.IOException; +import java.util.Collection; import java.util.List; import java.util.Map; import java.util.Objects; -public class ParametrizedMapperTests extends ESTestCase { +public class ParametrizedMapperTests extends AbstractBuilderTestCase { + + public static class TestPlugin extends Plugin implements MapperPlugin { + @Override + public Map getMappers() { + return Map.of("test_mapper", new TypeParser()); + } + } + + @Override + protected Collection> getPlugins() { + return List.of(TestPlugin.class, TestGeoShapeFieldMapperPlugin.class); //WTF + } private static TestMapper toType(Mapper in) { return (TestMapper) in; @@ -59,8 +77,8 @@ protected List> getParameters() { @Override public ParametrizedFieldMapper build(Mapper.BuilderContext context) { - return new TestMapper(buildFullName(context), fixed.getValue(), fixed2.getValue(), variable.getValue(), - multiFieldsBuilder.build(this, context), copyTo.build()); + return new TestMapper(name(), buildFullName(context), + multiFieldsBuilder.build(this, context), copyTo.build(), this); } } @@ -80,11 +98,12 @@ public static class TestMapper extends ParametrizedFieldMapper { private final boolean fixed2; private final String variable; - protected TestMapper(String simpleName, boolean fixed, boolean fixed2, String variable, MultiFields multiFields, CopyTo copyTo) { - super(simpleName, new KeywordFieldMapper.KeywordFieldType("test"), multiFields, copyTo); - this.fixed = fixed; - this.fixed2 = fixed2; - this.variable = variable; + protected TestMapper(String simpleName, String fullName, MultiFields multiFields, CopyTo copyTo, + ParametrizedMapperTests.Builder builder) { + super(simpleName, new KeywordFieldMapper.KeywordFieldType(fullName), multiFields, copyTo); + this.fixed = builder.fixed.getValue(); + this.fixed2 = builder.fixed2.getValue(); + this.variable = builder.variable.getValue(); } @Override @@ -197,4 +216,17 @@ public void testCopyTo() { assertEquals("{\"test\":{\"type\":\"test_mapper\",\"variable\":\"updated\"}}", Strings.toString(noCopyTo)); } + public void testObjectSerialization() throws IOException { + + QueryShardContext c = createShardContext(); + + String mapping = "{\"properties\":{\"object\":{\"properties\":{\"nestedobject\":{\"type\":\"test_mapper\"}}}}}"; + DocumentMapperParser parser = new DocumentMapperParser(c.getIndexSettings(), c.getMapperService(), + c.getXContentRegistry(), c.getSimilarityService(), c.getMapperService().mapperRegistry, () -> c); + + DocumentMapper mapper = parser.parse("_doc", new CompressedXContent(mapping)); + assertEquals("{\"_doc\":" + mapping + "}", Strings.toString(mapper)); + + } + } From 928a15f9cb2ab75d65c1448d7d32c490abf32b93 Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Tue, 30 Jun 2020 14:53:54 +0100 Subject: [PATCH 07/11] feedback --- .../percolator/PercolatorFieldMapper.java | 3 +- .../index/mapper/BinaryFieldMapper.java | 9 +- .../index/mapper/ParametrizedFieldMapper.java | 110 +++++++++++------- .../fielddata/AbstractFieldDataTestCase.java | 2 +- .../index/mapper/ParametrizedMapperTests.java | 36 +++--- 5 files changed, 94 insertions(+), 66 deletions(-) diff --git a/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorFieldMapper.java b/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorFieldMapper.java index 60be035389ac6..eaec4b1d73778 100644 --- a/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorFieldMapper.java +++ b/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolatorFieldMapper.java @@ -155,8 +155,7 @@ static KeywordFieldMapper createExtractQueryFieldBuilder(String name, BuilderCon } static BinaryFieldMapper createQueryBuilderFieldBuilder(BuilderContext context) { - BinaryFieldMapper.Builder builder = new BinaryFieldMapper.Builder(QUERY_BUILDER_FIELD_NAME); - builder.docValues(true); + BinaryFieldMapper.Builder builder = new BinaryFieldMapper.Builder(QUERY_BUILDER_FIELD_NAME, true); return builder.build(context); } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/BinaryFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/BinaryFieldMapper.java index 7664e9dc62353..5ce3e91a06181 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/BinaryFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/BinaryFieldMapper.java @@ -71,13 +71,12 @@ public static class Builder extends ParametrizedFieldMapper.Builder { final Parameter hasDocValues = Parameter.boolParam("doc_values", false, m -> toType(m).hasDocValues, false); public Builder(String name) { - super(name); + this(name, false); } - // For testing - public Builder docValues(boolean hasDocValues) { - this.hasDocValues.update(hasDocValues); - return this; + public Builder(String name, boolean hasDocValues) { + super(name); + this.hasDocValues.setValue(hasDocValues); } @Override diff --git a/server/src/main/java/org/elasticsearch/index/mapper/ParametrizedFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/ParametrizedFieldMapper.java index c1d422fa81774..c27a062d763b9 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ParametrizedFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ParametrizedFieldMapper.java @@ -87,88 +87,117 @@ public final XContentBuilder toXContent(XContentBuilder builder, Params params) return builder.endObject(); } + /** + * A configurable parameter for a field mapper + * @param the type of the value the parameter holds + */ public static final class Parameter { + public final String name; private final T defaultValue; private final BiFunction parser; - private final Function merger; - public final String name; - public final boolean updateable; + private final Function initializer; + private final boolean updateable; private boolean acceptsNull = false; private T value; - private boolean frozen = false; + /** + * Creates a new Parameter + * @param name the parameter name, used in parsing and serialization + * @param updateable whether the parameter can be updated with a new value during a mapping update + * @param defaultValue the default value for the parameter, used if unspecified in mappings + * @param parser a function that converts an object to a parameter value + * @param initializer a function that reads a parameter value from an existing mapper + */ public Parameter(String name, boolean updateable, T defaultValue, - BiFunction parser, Function merger) { + BiFunction parser, Function initializer) { this.name = name; this.defaultValue = defaultValue; this.value = defaultValue; this.parser = parser; - this.merger = merger; + this.initializer = initializer; this.updateable = updateable; } + /** + * Returns the current value of the parameter + */ public T getValue() { return value; } + /** + * Sets the current value of the parameter + */ + public void setValue(T value) { + this.value = value; + } + + /** + * Allows the parameter to accept a {@code null} value + */ public Parameter acceptsNull() { this.acceptsNull = true; return this; } - public Parameter init(FieldMapper toInit) { - assert frozen == false; - this.value = merger.apply(toInit); - return this; + private void init(FieldMapper toInit) { + this.value = initializer.apply(toInit); } - public Parameter update(T value) { - this.value = value; - return this; - } - - public Parameter parse(String field, Object in) { - assert frozen == false; + private void parse(String field, Object in) { this.value = parser.apply(field, in); - return this; } - public Parameter merge(FieldMapper toMerge, Conflicts conflicts) { - T value = merger.apply(toMerge); - if (frozen && Objects.equals(this.value, value) == false) { + private void merge(FieldMapper toMerge, Conflicts conflicts) { + T value = initializer.apply(toMerge); + if (updateable == false && Objects.equals(this.value, value) == false) { conflicts.addConflict(name, this.value.toString(), value.toString()); } else { this.value = value; } - return this; - } - - public Parameter freeze() { - this.frozen = true; - return this; } - public XContentBuilder toXContent(XContentBuilder builder, boolean includeDefaults) throws IOException { + private void toXContent(XContentBuilder builder, boolean includeDefaults) throws IOException { if (includeDefaults || (Objects.equals(defaultValue, value) == false)) { builder.field(name, value); } - return builder; } + /** + * Defines a parameter that takes the values {@code true} or {@code false} + * @param name the parameter name + * @param updateable whether the parameter can be changed by a mapping update + * @param initializer a function that reads the parameter value from an existing mapper + * @param defaultValue the default value, to be used if the parameter is undefined in a mapping + */ public static Parameter boolParam(String name, boolean updateable, - Function merger, boolean defaultValue) { - return new Parameter<>(name, updateable, defaultValue, (n, o) -> XContentMapValues.nodeBooleanValue(o), merger); + Function initializer, boolean defaultValue) { + return new Parameter<>(name, updateable, defaultValue, (n, o) -> XContentMapValues.nodeBooleanValue(o), initializer); } + /** + * Defines a parameter that takes a float value + * @param name the parameter name + * @param updateable whether the parameter can be changed by a mapping update + * @param initializer a function that reads the parameter value from an existing mapper + * @param defaultValue the default value, to be used if the parameter is undefined in a mapping + */ public static Parameter floatParam(String name, boolean updateable, - Function merger, float defaultValue) { - return new Parameter<>(name, updateable, defaultValue, (n, o) -> XContentMapValues.nodeFloatValue(o), merger); + Function initializer, float defaultValue) { + return new Parameter<>(name, updateable, defaultValue, (n, o) -> XContentMapValues.nodeFloatValue(o), initializer); } + /** + * Defines a parameter that takes a string value + * @param name the parameter name + * @param updateable whether the parameter can be changed by a mapping update + * @param initializer a function that reads the parameter value from an existing mapper + * @param defaultValue the default value, to be used if the parameter is undefined in a mapping + */ public static Parameter stringParam(String name, boolean updateable, - Function merger, String defaultValue) { - return new Parameter<>(name, updateable, defaultValue, (n, o) -> XContentMapValues.nodeStringValue(o, defaultValue), merger); + Function initializer, String defaultValue) { + return new Parameter<>(name, updateable, defaultValue, (n, o) -> XContentMapValues.nodeStringValue(o, defaultValue), initializer); } } @@ -208,21 +237,18 @@ protected Builder(String name) { super(name); } - protected Builder init(FieldMapper base) { + protected Builder init(FieldMapper initializer) { for (Parameter param : getParameters()) { - param.init(base); + param.init(initializer); } - for (Mapper subField : base.multiFields) { + for (Mapper subField : initializer.multiFields) { multiFieldsBuilder.add(subField); } return this; } - public final void merge(FieldMapper in, Conflicts conflicts) { + private void merge(FieldMapper in, Conflicts conflicts) { for (Parameter param : getParameters()) { - if (param.updateable == false) { - param.freeze(); - } param.merge(in, conflicts); } for (Mapper newSubField : in.multiFields) { diff --git a/server/src/test/java/org/elasticsearch/index/fielddata/AbstractFieldDataTestCase.java b/server/src/test/java/org/elasticsearch/index/fielddata/AbstractFieldDataTestCase.java index c7883c8cd7603..a6bdf6cfacef9 100644 --- a/server/src/test/java/org/elasticsearch/index/fielddata/AbstractFieldDataTestCase.java +++ b/server/src/test/java/org/elasticsearch/index/fielddata/AbstractFieldDataTestCase.java @@ -122,7 +122,7 @@ public > IFD getForField(String type, String field } else if (type.equals("geo_point")) { fieldType = new GeoPointFieldMapper.Builder(fieldName).docValues(docValues).build(context).fieldType(); } else if (type.equals("binary")) { - fieldType = new BinaryFieldMapper.Builder(fieldName).docValues(docValues).build(context).fieldType(); + fieldType = new BinaryFieldMapper.Builder(fieldName, docValues).build(context).fieldType(); } else { throw new UnsupportedOperationException(type); } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ParametrizedMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/ParametrizedMapperTests.java index 8f89e014ad9d9..8e6d0566f1e02 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ParametrizedMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ParametrizedMapperTests.java @@ -27,11 +27,11 @@ import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentHelper; import org.elasticsearch.common.xcontent.json.JsonXContent; -import org.elasticsearch.index.query.QueryShardContext; +import org.elasticsearch.index.IndexService; +import org.elasticsearch.index.mapper.ParametrizedFieldMapper.Parameter; import org.elasticsearch.plugins.MapperPlugin; import org.elasticsearch.plugins.Plugin; -import org.elasticsearch.test.AbstractBuilderTestCase; -import org.elasticsearch.test.TestGeoShapeFieldMapperPlugin; +import org.elasticsearch.test.ESSingleNodeTestCase; import java.io.IOException; import java.util.Collection; @@ -39,7 +39,7 @@ import java.util.Map; import java.util.Objects; -public class ParametrizedMapperTests extends AbstractBuilderTestCase { +public class ParametrizedMapperTests extends ESSingleNodeTestCase { public static class TestPlugin extends Plugin implements MapperPlugin { @Override @@ -50,7 +50,7 @@ public Map getMappers() { @Override protected Collection> getPlugins() { - return List.of(TestPlugin.class, TestGeoShapeFieldMapperPlugin.class); //WTF + return List.of(TestPlugin.class); } private static TestMapper toType(Mapper in) { @@ -59,19 +59,19 @@ private static TestMapper toType(Mapper in) { public static class Builder extends ParametrizedFieldMapper.Builder { - final ParametrizedFieldMapper.Parameter fixed - = ParametrizedFieldMapper.Parameter.boolParam("fixed", false, m -> toType(m).fixed, true); - final ParametrizedFieldMapper.Parameter fixed2 - = ParametrizedFieldMapper.Parameter.boolParam("fixed2", false, m -> toType(m).fixed2, false); - final ParametrizedFieldMapper.Parameter variable - = ParametrizedFieldMapper.Parameter.stringParam("variable", true, m -> toType(m).variable, "default"); + final Parameter fixed + = Parameter.boolParam("fixed", false, m -> toType(m).fixed, true); + final Parameter fixed2 + = Parameter.boolParam("fixed2", false, m -> toType(m).fixed2, false); + final Parameter variable + = Parameter.stringParam("variable", true, m -> toType(m).variable, "default"); protected Builder(String name) { super(name); } @Override - protected List> getParameters() { + protected List> getParameters() { return List.of(fixed, fixed2, variable); } @@ -108,7 +108,7 @@ protected TestMapper(String simpleName, String fullName, MultiFields multiFields @Override public Builder getMergeBuilder() { - return new ParametrizedMapperTests.Builder(name()).init(this); + return new ParametrizedMapperTests.Builder(simpleName()).init(this); } @Override @@ -218,11 +218,15 @@ public void testCopyTo() { public void testObjectSerialization() throws IOException { - QueryShardContext c = createShardContext(); + IndexService indexService = createIndex("test"); String mapping = "{\"properties\":{\"object\":{\"properties\":{\"nestedobject\":{\"type\":\"test_mapper\"}}}}}"; - DocumentMapperParser parser = new DocumentMapperParser(c.getIndexSettings(), c.getMapperService(), - c.getXContentRegistry(), c.getSimilarityService(), c.getMapperService().mapperRegistry, () -> c); + DocumentMapperParser parser = new DocumentMapperParser( + indexService.getIndexSettings(), + indexService.mapperService(), + indexService.xContentRegistry(), + indexService.similarityService(), + indexService.mapperService().mapperRegistry, () -> null); DocumentMapper mapper = parser.parse("_doc", new CompressedXContent(mapping)); assertEquals("{\"_doc\":" + mapping + "}", Strings.toString(mapper)); From 47c17e4f522674030e4262425849411e60dc37ab Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Tue, 30 Jun 2020 15:04:00 +0100 Subject: [PATCH 08/11] checkstyle --- .../elasticsearch/index/mapper/ParametrizedFieldMapper.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/ParametrizedFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/ParametrizedFieldMapper.java index c27a062d763b9..d6bd94f8d2c18 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ParametrizedFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ParametrizedFieldMapper.java @@ -197,7 +197,8 @@ public static Parameter floatParam(String name, boolean updateable, */ public static Parameter stringParam(String name, boolean updateable, Function initializer, String defaultValue) { - return new Parameter<>(name, updateable, defaultValue, (n, o) -> XContentMapValues.nodeStringValue(o, defaultValue), initializer); + return new Parameter<>(name, updateable, defaultValue, + (n, o) -> XContentMapValues.nodeStringValue(o, defaultValue), initializer); } } From 4bf93273e7d22d56fd37e8fa15d8e2a75c713711 Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Wed, 1 Jul 2020 13:26:36 +0100 Subject: [PATCH 09/11] wut --- .../elasticsearch/index/mapper/ParametrizedFieldMapper.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/ParametrizedFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/ParametrizedFieldMapper.java index d6bd94f8d2c18..d9782f2c2c2bb 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ParametrizedFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ParametrizedFieldMapper.java @@ -19,7 +19,6 @@ package org.elasticsearch.index.mapper; -import joptsimple.internal.Strings; import org.apache.lucene.document.FieldType; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.xcontent.XContentBuilder; @@ -220,7 +219,7 @@ public void check() { return; } String message = "Mapper for [" + mapperName + "] conflicts with existing mapper:\n\t" - + Strings.join(conflicts, "\n\t"); + + String.join("\n\t", conflicts); throw new IllegalArgumentException(message); } From bb35edaf9b835bb2ecfa561983b0f2e81d5f008f Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Thu, 2 Jul 2020 17:33:26 +0100 Subject: [PATCH 10/11] fix object serialization; javadocs --- .../xcontent/support/XContentMapValues.java | 7 +++ .../index/mapper/BinaryFieldMapper.java | 10 +-- .../index/mapper/BooleanFieldMapper.java | 2 + .../index/mapper/ParametrizedFieldMapper.java | 63 ++++++++++++++----- .../index/mapper/ParametrizedMapperTests.java | 62 +++++++++++------- 5 files changed, 101 insertions(+), 43 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/xcontent/support/XContentMapValues.java b/server/src/main/java/org/elasticsearch/common/xcontent/support/XContentMapValues.java index 05f8cb3a2fc45..a51ebb4a88c8c 100644 --- a/server/src/main/java/org/elasticsearch/common/xcontent/support/XContentMapValues.java +++ b/server/src/main/java/org/elasticsearch/common/xcontent/support/XContentMapValues.java @@ -332,6 +332,13 @@ public static String nodeStringValue(Object node, String defaultValue) { return node.toString(); } + public static String nodeStringValue(Object node) { + if (node == null) { + return null; + } + return node.toString(); + } + public static float nodeFloatValue(Object node, float defaultValue) { if (node == null) { return defaultValue; diff --git a/server/src/main/java/org/elasticsearch/index/mapper/BinaryFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/BinaryFieldMapper.java index a94fbfa88e672..de9b80031d266 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/BinaryFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/BinaryFieldMapper.java @@ -20,9 +20,7 @@ package org.elasticsearch.index.mapper; import com.carrotsearch.hppc.ObjectArrayList; -import org.apache.lucene.document.FieldType; import org.apache.lucene.document.StoredField; -import org.apache.lucene.index.IndexOptions; import org.apache.lucene.index.Term; import org.apache.lucene.search.DocValuesFieldExistsQuery; import org.apache.lucene.search.Query; @@ -51,15 +49,17 @@ public class BinaryFieldMapper extends ParametrizedFieldMapper { public static final String CONTENT_TYPE = "binary"; - + private static BinaryFieldMapper toType(FieldMapper in) { return (BinaryFieldMapper) in; } public static class Builder extends ParametrizedFieldMapper.Builder { - final Parameter stored = Parameter.boolParam("store", false, m -> toType(m).stored, false); - final Parameter hasDocValues = Parameter.boolParam("doc_values", false, m -> toType(m).hasDocValues, false); + private final Parameter stored = Parameter.boolParam("store", false, m -> toType(m).stored, false); + private final Parameter hasDocValues = Parameter.boolParam("doc_values", false, m -> toType(m).hasDocValues, false); + private final Parameter> meta + = new Parameter<>("meta", true, Collections.emptyMap(), TypeParsers::parseMeta, m -> m.fieldType().meta()); public Builder(String name) { this(name, false); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/BooleanFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/BooleanFieldMapper.java index 68e37f49a0fb6..004b281b6fed1 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/BooleanFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/BooleanFieldMapper.java @@ -82,6 +82,8 @@ public static class Builder extends ParametrizedFieldMapper.Builder { private final Parameter nullValue = new Parameter<>("null_value", false, null, (n, o) -> XContentMapValues.nodeBooleanValue(o), m -> toType(m).nullValue); private final Parameter stored = Parameter.boolParam("store", false, m -> toType(m).stored, false); + private final Parameter> meta + = new Parameter<>("meta", true, Collections.emptyMap(), TypeParsers::parseMeta, m -> m.fieldType().meta()); public Builder(String name) { super(name); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/ParametrizedFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/ParametrizedFieldMapper.java index d9782f2c2c2bb..f0fe1906d05e1 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ParametrizedFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ParametrizedFieldMapper.java @@ -26,7 +26,6 @@ import java.io.IOException; import java.util.ArrayList; -import java.util.Collections; import java.util.HashMap; import java.util.Iterator; import java.util.List; @@ -35,12 +34,31 @@ import java.util.function.BiFunction; import java.util.function.Function; +/** + * Defines how a particular field should be indexed and searched + * + * Configuration {@link Parameter}s for the mapper are defined on a {@link Builder} subclass, + * and returned by its {@link Builder#getParameters()} method. Merging, serialization + * and parsing of the mapper are all mediated through this set of parameters. + * + * Subclasses should implement a {@link Builder} that is returned from the + * {@link #getMergeBuilder()} method, initialised with the existing builder. + */ public abstract class ParametrizedFieldMapper extends FieldMapper { + /** + * Creates a new ParametrizedFieldMapper + */ protected ParametrizedFieldMapper(String simpleName, MappedFieldType mappedFieldType, MultiFields multiFields, CopyTo copyTo) { super(simpleName, new FieldType(), mappedFieldType, multiFields, copyTo); } + /** + * Returns a {@link Builder} to be used for merging and serialization + * + * Implement as follows: + * {@code return new MyBuilder(simpleName()).init(this); } + */ public abstract ParametrizedFieldMapper.Builder getMergeBuilder(); @Override @@ -67,7 +85,7 @@ private static ContentPath parentPath(String name) { if (endPos == -1) { return new ContentPath(0); } - return new ContentPath(name.substring(endPos)); + return new ContentPath(name.substring(0, endPos)); } @Override @@ -197,24 +215,24 @@ public static Parameter floatParam(String name, boolean updateable, public static Parameter stringParam(String name, boolean updateable, Function initializer, String defaultValue) { return new Parameter<>(name, updateable, defaultValue, - (n, o) -> XContentMapValues.nodeStringValue(o, defaultValue), initializer); + (n, o) -> XContentMapValues.nodeStringValue(o), initializer); } } - public static final class Conflicts { + private static final class Conflicts { private final String mapperName; private final List conflicts = new ArrayList<>(); - public Conflicts(String mapperName) { + Conflicts(String mapperName) { this.mapperName = mapperName; } - public void addConflict(String parameter, String existing, String toMerge) { + void addConflict(String parameter, String existing, String toMerge) { conflicts.add("Cannot update parameter [" + parameter + "] from [" + existing + "] to [" + toMerge + "]"); } - public void check() { + void check() { if (conflicts.isEmpty()) { return; } @@ -225,19 +243,25 @@ public void check() { } + /** + * A Builder for a ParametrizedFieldMapper + */ public abstract static class Builder extends Mapper.Builder { protected final MultiFields.Builder multiFieldsBuilder = new MultiFields.Builder(); - protected CopyTo.Builder copyTo = new CopyTo.Builder(); - - protected final Parameter> meta - = new Parameter<>("meta", true, Collections.emptyMap(), TypeParsers::parseMeta, m -> m.fieldType().meta()); + protected final CopyTo.Builder copyTo = new CopyTo.Builder(); + /** + * Creates a new Builder with a field name + */ protected Builder(String name) { super(name); } - protected Builder init(FieldMapper initializer) { + /** + * Initialises all parameters from an existing mapper + */ + public Builder init(FieldMapper initializer) { for (Parameter param : getParameters()) { param.init(initializer); } @@ -257,22 +281,33 @@ private void merge(FieldMapper in, Conflicts conflicts) { this.copyTo.reset(in.copyTo); } + /** + * @return the list of parameters defined for this mapper + */ protected abstract List> getParameters(); @Override public abstract ParametrizedFieldMapper build(BuilderContext context); + /** + * Builds the full name of the field, taking into account parent objects + */ protected String buildFullName(BuilderContext context) { return context.path().pathAsText(name); } - public final XContentBuilder toXContent(XContentBuilder builder, boolean includeDefaults) throws IOException { + private void toXContent(XContentBuilder builder, boolean includeDefaults) throws IOException { for (Parameter parameter : getParameters()) { parameter.toXContent(builder, includeDefaults); } - return builder; } + /** + * Parse mapping parameters from a map of mappings + * @param name the field mapper name + * @param parserContext the parser context + * @param fieldNode the root node of the map of mappings for this field + */ public final void parse(String name, TypeParser.ParserContext parserContext, Map fieldNode) { Map> paramsMap = new HashMap<>(); for (Parameter param : getParameters()) { diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ParametrizedMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/ParametrizedMapperTests.java index 8e6d0566f1e02..ea236871cd380 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ParametrizedMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ParametrizedMapperTests.java @@ -64,7 +64,7 @@ public static class Builder extends ParametrizedFieldMapper.Builder { final Parameter fixed2 = Parameter.boolParam("fixed2", false, m -> toType(m).fixed2, false); final Parameter variable - = Parameter.stringParam("variable", true, m -> toType(m).variable, "default"); + = Parameter.stringParam("variable", true, m -> toType(m).variable, "default").acceptsNull(); protected Builder(String name) { super(name); @@ -133,7 +133,7 @@ private static TestMapper fromMapping(String mapping) { return null; }, Version.CURRENT, () -> null); return (TestMapper) new TypeParser() - .parse("test", XContentHelper.convertToMap(JsonXContent.jsonXContent, mapping, true), pc) + .parse("field", XContentHelper.convertToMap(JsonXContent.jsonXContent, mapping, true), pc) .build(new Mapper.BuilderContext(Settings.EMPTY, new ContentPath(0))); } @@ -145,14 +145,14 @@ public void testDefaults() throws IOException { assertTrue(mapper.fixed); assertEquals("default", mapper.variable); - assertEquals("{\"test\":" + mapping + "}", Strings.toString(mapper)); + assertEquals("{\"field\":" + mapping + "}", Strings.toString(mapper)); XContentBuilder builder = JsonXContent.contentBuilder(); ToXContent.Params params = new ToXContent.MapParams(Map.of("include_defaults", "true")); builder.startObject(); mapper.toXContent(builder, params); builder.endObject(); - assertEquals("{\"test\":{\"type\":\"test_mapper\",\"fixed\":true,\"fixed2\":false,\"variable\":\"default\"}}", + assertEquals("{\"field\":{\"type\":\"test_mapper\",\"fixed\":true,\"fixed2\":false,\"variable\":\"default\"}}", Strings.toString(builder)); } @@ -160,23 +160,23 @@ public void testDefaults() throws IOException { public void testMerging() { String mapping = "{\"type\":\"test_mapper\",\"fixed\":false}"; TestMapper mapper = fromMapping(mapping); - assertEquals("{\"test\":" + mapping + "}", Strings.toString(mapper)); + assertEquals("{\"field\":" + mapping + "}", Strings.toString(mapper)); TestMapper badMerge = fromMapping("{\"type\":\"test_mapper\",\"fixed\":true,\"fixed2\":true}"); IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> mapper.merge(badMerge)); - String expectedError = "Mapper for [test] conflicts with existing mapper:\n" + + String expectedError = "Mapper for [field] conflicts with existing mapper:\n" + "\tCannot update parameter [fixed] from [false] to [true]\n" + "\tCannot update parameter [fixed2] from [false] to [true]"; assertEquals(expectedError, e.getMessage()); - assertEquals("{\"test\":" + mapping + "}", Strings.toString(mapper)); // original mapping is unaffected + assertEquals("{\"field\":" + mapping + "}", Strings.toString(mapper)); // original mapping is unaffected // TODO: should we have to include 'fixed' here? Or should updates take as 'defaults' the existing values? TestMapper goodMerge = fromMapping("{\"type\":\"test_mapper\",\"fixed\":false,\"variable\":\"updated\"}"); TestMapper merged = (TestMapper) mapper.merge(goodMerge); - assertEquals("{\"test\":" + mapping + "}", Strings.toString(mapper)); // original mapping is unaffected - assertEquals("{\"test\":{\"type\":\"test_mapper\",\"fixed\":false,\"variable\":\"updated\"}}", Strings.toString(merged)); + assertEquals("{\"field\":" + mapping + "}", Strings.toString(mapper)); // original mapping is unaffected + assertEquals("{\"field\":{\"type\":\"test_mapper\",\"fixed\":false,\"variable\":\"updated\"}}", Strings.toString(merged)); } @@ -184,52 +184,66 @@ public void testMerging() { public void testMultifields() { String mapping = "{\"type\":\"test_mapper\",\"variable\":\"foo\",\"fields\":{\"sub\":{\"type\":\"keyword\"}}}"; TestMapper mapper = fromMapping(mapping); - assertEquals("{\"test\":" + mapping + "}", Strings.toString(mapper)); + assertEquals("{\"field\":" + mapping + "}", Strings.toString(mapper)); String addSubField = "{\"type\":\"test_mapper\",\"variable\":\"foo\",\"fields\":{\"sub2\":{\"type\":\"keyword\"}}}"; TestMapper toMerge = fromMapping(addSubField); TestMapper merged = (TestMapper) mapper.merge(toMerge); - assertEquals("{\"test\":{\"type\":\"test_mapper\",\"variable\":\"foo\"," + + assertEquals("{\"field\":{\"type\":\"test_mapper\",\"variable\":\"foo\"," + "\"fields\":{\"sub\":{\"type\":\"keyword\"},\"sub2\":{\"type\":\"keyword\"}}}}", Strings.toString(merged)); String badSubField = "{\"type\":\"test_mapper\",\"variable\":\"foo\",\"fields\":{\"sub2\":{\"type\":\"binary\"}}}"; TestMapper badToMerge = fromMapping(badSubField); IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> merged.merge(badToMerge)); - assertEquals("mapper [test.sub2] cannot be changed from type [keyword] to [binary]", e.getMessage()); + assertEquals("mapper [field.sub2] cannot be changed from type [keyword] to [binary]", e.getMessage()); } // add copy_to, verify public void testCopyTo() { String mapping = "{\"type\":\"test_mapper\",\"variable\":\"foo\",\"copy_to\":[\"other\"]}"; TestMapper mapper = fromMapping(mapping); - assertEquals("{\"test\":" + mapping + "}", Strings.toString(mapper)); + assertEquals("{\"field\":" + mapping + "}", Strings.toString(mapper)); // On update, copy_to is completely replaced TestMapper toMerge = fromMapping("{\"type\":\"test_mapper\",\"variable\":\"updated\",\"copy_to\":[\"foo\",\"bar\"]}"); TestMapper merged = (TestMapper) mapper.merge(toMerge); - assertEquals("{\"test\":{\"type\":\"test_mapper\",\"variable\":\"updated\",\"copy_to\":[\"foo\",\"bar\"]}}", + assertEquals("{\"field\":{\"type\":\"test_mapper\",\"variable\":\"updated\",\"copy_to\":[\"foo\",\"bar\"]}}", Strings.toString(merged)); TestMapper removeCopyTo = fromMapping("{\"type\":\"test_mapper\",\"variable\":\"updated\"}"); TestMapper noCopyTo = (TestMapper) merged.merge(removeCopyTo); - assertEquals("{\"test\":{\"type\":\"test_mapper\",\"variable\":\"updated\"}}", Strings.toString(noCopyTo)); + assertEquals("{\"field\":{\"type\":\"test_mapper\",\"variable\":\"updated\"}}", Strings.toString(noCopyTo)); + } + + public void testNullables() { + String mapping = "{\"type\":\"test_mapper\",\"fixed\":null}"; + MapperParsingException e = expectThrows(MapperParsingException.class, () -> fromMapping(mapping)); + assertEquals("[fixed] on mapper [field] of type [test_mapper] must not have a [null] value", e.getMessage()); + + String fine = "{\"type\":\"test_mapper\",\"variable\":null}"; + TestMapper mapper = fromMapping(fine); + assertEquals("{\"field\":" + fine + "}", Strings.toString(mapper)); } public void testObjectSerialization() throws IOException { IndexService indexService = createIndex("test"); - String mapping = "{\"properties\":{\"object\":{\"properties\":{\"nestedobject\":{\"type\":\"test_mapper\"}}}}}"; - DocumentMapperParser parser = new DocumentMapperParser( - indexService.getIndexSettings(), - indexService.mapperService(), - indexService.xContentRegistry(), - indexService.similarityService(), - indexService.mapperService().mapperRegistry, () -> null); + String mapping = "{\"_doc\":{" + + "\"properties\":{" + + "\"actual\":{\"type\":\"double\"}," + + "\"bucket_count\":{\"type\":\"long\"}," + + "\"bucket_influencers\":{\"type\":\"nested\",\"properties\":{" + + "\"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); + assertEquals(mapping, Strings.toString(indexService.mapperService().documentMapper())); - DocumentMapper mapper = parser.parse("_doc", new CompressedXContent(mapping)); - assertEquals("{\"_doc\":" + mapping + "}", Strings.toString(mapper)); } From 37ea8d669e95c7d3311218779d1f525ccaa224f6 Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Wed, 8 Jul 2020 09:54:33 +0100 Subject: [PATCH 11/11] feedback --- .../xcontent/support/XContentMapValues.java | 3 ++ .../index/mapper/BooleanFieldMapper.java | 28 +++++++++++-------- .../index/mapper/BooleanFieldMapperTests.java | 12 ++++++++ 3 files changed, 31 insertions(+), 12 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/xcontent/support/XContentMapValues.java b/server/src/main/java/org/elasticsearch/common/xcontent/support/XContentMapValues.java index a51ebb4a88c8c..dfbb507365fa9 100644 --- a/server/src/main/java/org/elasticsearch/common/xcontent/support/XContentMapValues.java +++ b/server/src/main/java/org/elasticsearch/common/xcontent/support/XContentMapValues.java @@ -332,6 +332,9 @@ public static String nodeStringValue(Object node, String defaultValue) { return node.toString(); } + /** + * Returns the {@link Object#toString} value of its input, or {@code null} if the input is null + */ public static String nodeStringValue(Object node) { if (node == null) { return null; diff --git a/server/src/main/java/org/elasticsearch/index/mapper/BooleanFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/BooleanFieldMapper.java index 004b281b6fed1..5912e5107774b 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/BooleanFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/BooleanFieldMapper.java @@ -74,14 +74,14 @@ private static BooleanFieldMapper toType(FieldMapper in) { public static class Builder extends ParametrizedFieldMapper.Builder { - private final Parameter boost = Parameter.floatParam("boost", true, m -> m.fieldType().boost(), 1.0f); - private final Parameter docValues - = Parameter.boolParam("doc_values", false, m -> m.fieldType().hasDocValues(), true); - private final Parameter indexed - = Parameter.boolParam("index", false, m -> m.fieldType().isSearchable(), true); + private final Parameter docValues = Parameter.boolParam("doc_values", false, m -> toType(m).hasDocValues, true); + private final Parameter indexed = Parameter.boolParam("index", false, m -> toType(m).indexed, true); + private final Parameter stored = Parameter.boolParam("store", false, m -> toType(m).stored, false); + private final Parameter nullValue = new Parameter<>("null_value", false, null, (n, o) -> XContentMapValues.nodeBooleanValue(o), m -> toType(m).nullValue); - private final Parameter stored = Parameter.boolParam("store", false, m -> toType(m).stored, false); + + private final Parameter boost = Parameter.floatParam("boost", true, m -> m.fieldType().boost(), 1.0f); private final Parameter> meta = new Parameter<>("meta", true, Collections.emptyMap(), TypeParsers::parseMeta, m -> m.fieldType().meta()); @@ -96,9 +96,9 @@ protected List> getParameters() { @Override public BooleanFieldMapper build(BuilderContext context) { - return new BooleanFieldMapper(name, - new BooleanFieldType(buildFullName(context), indexed.getValue(), docValues.getValue(), meta.getValue()), - multiFieldsBuilder.build(this, context), copyTo.build(), this); + MappedFieldType ft = new BooleanFieldType(buildFullName(context), indexed.getValue(), docValues.getValue(), meta.getValue()); + ft.setBoost(boost.getValue()); + return new BooleanFieldMapper(name, ft, multiFieldsBuilder.build(this, context), copyTo.build(), this); } } @@ -214,6 +214,8 @@ public Query rangeQuery(Object lowerTerm, Object upperTerm, boolean includeLower } private final Boolean nullValue; + private final boolean indexed; + private final boolean hasDocValues; private final boolean stored; protected BooleanFieldMapper(String simpleName, MappedFieldType mappedFieldType, @@ -221,6 +223,8 @@ protected BooleanFieldMapper(String simpleName, MappedFieldType mappedFieldType, super(simpleName, mappedFieldType, multiFields, copyTo); this.nullValue = builder.nullValue.getValue(); this.stored = builder.stored.getValue(); + this.indexed = builder.indexed.getValue(); + this.hasDocValues = builder.docValues.getValue(); } @Override @@ -230,7 +234,7 @@ public BooleanFieldType fieldType() { @Override protected void parseCreateField(ParseContext context) throws IOException { - if (fieldType().isSearchable() == false && stored == false && !fieldType().hasDocValues()) { + if (indexed == false && stored == false && hasDocValues == false) { return; } @@ -249,13 +253,13 @@ protected void parseCreateField(ParseContext context) throws IOException { if (value == null) { return; } - if (fieldType().isSearchable()) { + if (indexed) { context.doc().add(new Field(fieldType().name(), value ? "T" : "F", Defaults.FIELD_TYPE)); } if (stored) { context.doc().add(new StoredField(fieldType().name(), value ? "T" : "F")); } - if (fieldType().hasDocValues()) { + if (hasDocValues) { context.doc().add(new SortedNumericDocValuesField(fieldType().name(), value ? 1 : 0)); } else { createFieldNamesField(context); diff --git a/server/src/test/java/org/elasticsearch/index/mapper/BooleanFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/BooleanFieldMapperTests.java index 5dff9b8044904..0d5ed6d24803a 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/BooleanFieldMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/BooleanFieldMapperTests.java @@ -27,6 +27,9 @@ import org.apache.lucene.index.IndexableField; import org.apache.lucene.index.LeafReader; import org.apache.lucene.index.SortedNumericDocValues; +import org.apache.lucene.index.Term; +import org.apache.lucene.search.BoostQuery; +import org.apache.lucene.search.TermQuery; import org.apache.lucene.store.ByteBuffersDirectory; import org.apache.lucene.store.Directory; import org.apache.lucene.util.BytesRef; @@ -281,4 +284,13 @@ public void testMeta() throws Exception { assertEquals(mapping3, mapper.mappingSource().toString()); } + public void testBoosts() throws Exception { + String mapping = "{\"_doc\":{\"properties\":{\"field\":{\"type\":\"boolean\",\"boost\":2.0}}}}"; + DocumentMapper mapper = indexService.mapperService().merge("_doc", new CompressedXContent(mapping), MergeReason.MAPPING_UPDATE); + assertEquals(mapping, mapper.mappingSource().toString()); + + MappedFieldType ft = indexService.mapperService().fieldType("field"); + assertEquals(new BoostQuery(new TermQuery(new Term("field", "T")), 2.0f), ft.termQuery("true", null)); + } + }