From f54f87a0cfe179410086d477a1c5b156ff1da707 Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Thu, 7 Oct 2021 10:49:45 +0100 Subject: [PATCH 01/13] WIP --- .../index/mapper/DocumentParser.java | 22 ++++-- .../index/mapper/MappingParserContext.java | 42 ++++++++++-- .../index/mapper/ObjectMapper.java | 58 +++++++++++----- .../index/mapper/RootObjectMapper.java | 68 +++++++++++++++++-- .../index/mapper/DynamicMappingTests.java | 18 +++++ .../index/mapper/MappingParserTests.java | 36 +++++++++- .../index/mapper/RootObjectMapperTests.java | 10 +++ 7 files changed, 217 insertions(+), 37 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java index 155d6e1b33d72..1d79fe0c52f91 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java @@ -679,12 +679,16 @@ private static void parseValue(final DocumentParserContext context, ObjectMapper if (mapper != null) { parseObjectOrField(context, mapper); } else { - currentFieldName = paths[paths.length - 1]; - Tuple parentMapperTuple = getDynamicParentMapper(context, paths, parentMapper); - parentMapper = parentMapperTuple.v2(); - parseDynamicValue(context, parentMapper, currentFieldName, token); - for (int i = 0; i < parentMapperTuple.v1(); i++) { - context.path().remove(); + if (context.root().allowDotsInLeafFields()) { + parseDynamicValue(context, parentMapper, currentFieldName, token); + } else { + currentFieldName = paths[paths.length - 1]; + Tuple parentMapperTuple = getDynamicParentMapper(context, paths, parentMapper); + parentMapper = parentMapperTuple.v2(); + parseDynamicValue(context, parentMapper, currentFieldName, token); + for (int i = 0; i < parentMapperTuple.v1(); i++) { + context.path().remove(); + } } } } @@ -859,6 +863,12 @@ private static Mapper getMapper(final DocumentParserContext context, return mapper; } + // Is the full name of the mapper a direct child of this object? + mapper = objectMapper.getMapper(fieldPath); + if (mapper != null) { + return mapper; + } + for (int i = 0; i < subfields.length - 1; ++i) { mapper = objectMapper.getMapper(subfields[i]); if (mapper instanceof ObjectMapper == false) { diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MappingParserContext.java b/server/src/main/java/org/elasticsearch/index/mapper/MappingParserContext.java index 2a1a8e073e3d8..a880dcf5acc3a 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MappingParserContext.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MappingParserContext.java @@ -60,6 +60,21 @@ public MappingParserContext(Function similarityLooku this.idFieldDataEnabled = idFieldDataEnabled; } + protected MappingParserContext(MappingParserContext in) { + this( + in.similarityLookupService, + in.typeParsers, + in.runtimeFieldParsers, + in.indexVersionCreated.minimumIndexCompatibilityVersion(), + in.searchExecutionContextSupplier, + in.dateFormatter, + in.scriptCompiler, + in.indexAnalyzers, + in.indexSettings, + in.idFieldDataEnabled + ); + } + public IndexAnalyzers getIndexAnalyzers() { return indexAnalyzers; } @@ -116,6 +131,10 @@ public boolean isFromDynamicTemplate() { return false; } + public boolean allowDotsInFieldNames() { + return false; + } + protected Function similarityLookupService() { return similarityLookupService; } @@ -131,11 +150,13 @@ MappingParserContext createMultiFieldContext(MappingParserContext in) { return new MultiFieldParserContext(in); } + MappingParserContext createDotsInFieldNamesContext() { + return new DotsInFieldNamesParserContext(this); + } + private static class MultiFieldParserContext extends MappingParserContext { MultiFieldParserContext(MappingParserContext in) { - super(in.similarityLookupService, in.typeParsers, in.runtimeFieldParsers, in.indexVersionCreated, - in.searchExecutionContextSupplier, in.dateFormatter, in.scriptCompiler, in.indexAnalyzers, in.indexSettings, - in.idFieldDataEnabled); + super(in); } @Override @@ -146,9 +167,7 @@ public boolean isWithinMultiField() { static class DynamicTemplateParserContext extends MappingParserContext { DynamicTemplateParserContext(MappingParserContext in) { - super(in.similarityLookupService, in.typeParsers, in.runtimeFieldParsers, in.indexVersionCreated, - in.searchExecutionContextSupplier, in.dateFormatter, in.scriptCompiler, in.indexAnalyzers, in.indexSettings, - in.idFieldDataEnabled); + super(in); } @Override @@ -156,4 +175,15 @@ public boolean isFromDynamicTemplate() { return true; } } + + private static class DotsInFieldNamesParserContext extends MappingParserContext { + DotsInFieldNamesParserContext(MappingParserContext in) { + super(in); + } + + @Override + public boolean allowDotsInFieldNames() { + return true; + } + } } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java index 6875ea191a4ff..0a9d946f22f72 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java @@ -28,6 +28,7 @@ import java.util.List; import java.util.Locale; import java.util.Map; +import java.util.function.BiFunction; public class ObjectMapper extends Mapper implements Cloneable { private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(ObjectMapper.class); @@ -114,15 +115,42 @@ public Mapper.Builder parse(String name, Map node, MappingParserContext parserContext) throws MapperParsingException { - ObjectMapper.Builder builder = new Builder(name); - for (Iterator> iterator = node.entrySet().iterator(); iterator.hasNext();) { - Map.Entry entry = iterator.next(); - String fieldName = entry.getKey(); - Object fieldNode = entry.getValue(); - if (parseObjectOrDocumentTypeProperties(fieldName, fieldNode, parserContext, builder)) { - iterator.remove(); + + return parse(name, false, (n, c) -> { + ObjectMapper.Builder builder = new Builder(n); + for (Iterator> iterator = node.entrySet().iterator(); iterator.hasNext();) { + Map.Entry entry = iterator.next(); + String fieldName = entry.getKey(); + Object fieldNode = entry.getValue(); + if (parseObjectOrDocumentTypeProperties(fieldName, fieldNode, parserContext, builder)) { + iterator.remove(); + } } + return builder; + }, parserContext); + } + + + + private static Mapper.Builder parse( + String name, + boolean allowDotsInFieldNames, + BiFunction parser, + MappingParserContext parserContext + ) { + if (allowDotsInFieldNames) { + return parser.apply(name, parserContext); } + String[] fieldNameParts = name.split("\\."); + String realFieldName = fieldNameParts[fieldNameParts.length - 1]; + Mapper.Builder builder = parser.apply(realFieldName, parserContext); + for (int i = fieldNameParts.length - 2; i >= 0; --i) { + ObjectMapper.Builder intermediate + = new ObjectMapper.Builder(fieldNameParts[i]); + intermediate.add(builder); + builder = intermediate; + } + return builder; } @@ -199,16 +227,12 @@ protected static void parseProperties(ObjectMapper.Builder objBuilder, if (typeParser == null) { throw new MapperParsingException("No handler for type [" + type + "] declared on field [" + fieldName + "]"); } - String[] fieldNameParts = fieldName.split("\\."); - String realFieldName = fieldNameParts[fieldNameParts.length - 1]; - Mapper.Builder fieldBuilder = typeParser.parse(realFieldName, propNode, parserContext); - for (int i = fieldNameParts.length - 2; i >= 0; --i) { - ObjectMapper.Builder intermediate - = new ObjectMapper.Builder(fieldNameParts[i]); - intermediate.add(fieldBuilder); - fieldBuilder = intermediate; - } - objBuilder.add(fieldBuilder); + objBuilder.add(parse( + fieldName, + parserContext.allowDotsInFieldNames(), + (n, c) -> typeParser.parse(n, propNode, c), + parserContext + )); propNode.remove("type"); MappingParser.checkNoRemainingFields(fieldName, propNode); iterator.remove(); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java index 2477e3fed5547..3091300c17afe 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java @@ -17,6 +17,7 @@ import org.elasticsearch.common.time.DateFormatter; import org.elasticsearch.common.xcontent.ToXContent; import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.common.xcontent.support.XContentMapValues; import org.elasticsearch.index.mapper.DynamicTemplate.XContentFieldType; import org.elasticsearch.index.mapper.MapperService.MergeReason; @@ -51,6 +52,8 @@ public class RootObjectMapper extends ObjectMapper { */ static final String TOXCONTENT_SKIP_RUNTIME = "skip_runtime"; + public static final String ALLOW_DOTS_IN_LEAF_FIELDS = "allow_dots_in_leaf_fields"; + public static class Defaults { public static final DateFormatter[] DYNAMIC_DATE_TIME_FORMATTERS = new DateFormatter[]{ @@ -59,6 +62,7 @@ public static class Defaults { }; public static final boolean DATE_DETECTION = true; public static final boolean NUMERIC_DETECTION = false; + public static final boolean DOTS_IN_LEAF_FIELDS = false; } public static class Builder extends ObjectMapper.Builder { @@ -67,6 +71,7 @@ public static class Builder extends ObjectMapper.Builder { protected Explicit dynamicDateTimeFormatters = new Explicit<>(Defaults.DYNAMIC_DATE_TIME_FORMATTERS, false); protected Explicit dateDetection = new Explicit<>(Defaults.DATE_DETECTION, false); protected Explicit numericDetection = new Explicit<>(Defaults.NUMERIC_DETECTION, false); + protected Explicit dotsInLeafFields = new Explicit<>(Defaults.DOTS_IN_LEAF_FIELDS, false); protected Map runtimeFields; public Builder(String name) { @@ -96,11 +101,17 @@ public RootObjectMapper.Builder setRuntime(Map runtimeFiel @Override public RootObjectMapper build(MapperBuilderContext context) { - return new RootObjectMapper(name, enabled, dynamic, buildMappers(true, context), + return new RootObjectMapper( + name, + enabled, + dynamic, + buildMappers(true, context), runtimeFields == null ? Collections.emptyMap() : runtimeFields, dynamicDateTimeFormatters, dynamicTemplates, - dateDetection, numericDetection); + dateDetection, + numericDetection, + dotsInLeafFields); } } @@ -141,6 +152,9 @@ static final class TypeParser extends ObjectMapper.TypeParser { public RootObjectMapper.Builder parse(String name, Map node, MappingParserContext parserContext) throws MapperParsingException { RootObjectMapper.Builder builder = new Builder(name); + if (allowDotsInLeafFieldNames(node, builder)) { + parserContext = parserContext.createDotsInFieldNamesContext(); + } Iterator> iterator = node.entrySet().iterator(); while (iterator.hasNext()) { Map.Entry entry = iterator.next(); @@ -154,6 +168,16 @@ public RootObjectMapper.Builder parse(String name, Map node, Map return builder; } + private static boolean allowDotsInLeafFieldNames(Map node, RootObjectMapper.Builder builder) { + if (node.containsKey(ALLOW_DOTS_IN_LEAF_FIELDS) == false) { + return false; + } + boolean value = XContentMapValues.nodeBooleanValue(node.get(ALLOW_DOTS_IN_LEAF_FIELDS), ALLOW_DOTS_IN_LEAF_FIELDS); + node.remove(ALLOW_DOTS_IN_LEAF_FIELDS); + builder.dotsInLeafFields = new Explicit<>(value, true); + return value; + } + @SuppressWarnings("unchecked") private boolean processField(RootObjectMapper.Builder builder, String fieldName, @@ -233,18 +257,29 @@ private boolean processField(RootObjectMapper.Builder builder, private Explicit dateDetection; private Explicit numericDetection; private Explicit dynamicTemplates; + private Explicit dotsInLeafFields; private Map runtimeFields; - RootObjectMapper(String name, Explicit enabled, Dynamic dynamic, Map mappers, - Map runtimeFields, - Explicit dynamicDateTimeFormatters, Explicit dynamicTemplates, - Explicit dateDetection, Explicit numericDetection) { + RootObjectMapper( + String name, + Explicit enabled, + Dynamic dynamic, + Map mappers, + Map runtimeFields, + + Explicit dynamicDateTimeFormatters, + Explicit dynamicTemplates, + Explicit dateDetection, + Explicit numericDetection, + Explicit dotsInLeafFields + ) { super(name, name, enabled, dynamic, mappers); this.runtimeFields = runtimeFields; this.dynamicTemplates = dynamicTemplates; this.dynamicDateTimeFormatters = dynamicDateTimeFormatters; this.dateDetection = dateDetection; this.numericDetection = numericDetection; + this.dotsInLeafFields = dotsInLeafFields; } @Override @@ -264,6 +299,7 @@ RootObjectMapper copyAndReset() { copy.dynamicDateTimeFormatters = new Explicit<>(Defaults.DYNAMIC_DATE_TIME_FORMATTERS, false); copy.dateDetection = new Explicit<>(Defaults.DATE_DETECTION, false); copy.numericDetection = new Explicit<>(Defaults.NUMERIC_DETECTION, false); + copy.dotsInLeafFields = new Explicit<>(Defaults.DOTS_IN_LEAF_FIELDS, false); //also no need to carry the already defined runtime fields, only new ones need to be added copy.runtimeFields.clear(); return copy; @@ -297,6 +333,10 @@ public DynamicTemplate[] dynamicTemplates() { return dynamicTemplates.value(); } + public boolean allowDotsInLeafFields() { + return dotsInLeafFields.value(); + } + Collection runtimeFields() { return runtimeFields.values(); } @@ -326,6 +366,19 @@ protected void doMerge(ObjectMapper mergeWith, MergeReason reason) { this.dynamicDateTimeFormatters = mergeWithObject.dynamicDateTimeFormatters; } + if (mergeWithObject.dotsInLeafFields.explicit()) { + if (reason == MergeReason.INDEX_TEMPLATE) { + this.dotsInLeafFields = mergeWithObject.dotsInLeafFields; + } else { + if (this.dotsInLeafFields.value().equals(mergeWithObject.dotsInLeafFields.value()) == false) { + throw new MapperParsingException( + "Can't change parameter [allow_dots_in_leaf_fields] from [" + + this.dotsInLeafFields.value() + "] to [" + mergeWithObject.dotsInLeafFields.value() + "]" + ); + } + } + } + if (mergeWithObject.dynamicTemplates.explicit()) { if (reason == MergeReason.INDEX_TEMPLATE) { Map templatesByKey = new LinkedHashMap<>(); @@ -386,6 +439,9 @@ protected void doXContent(XContentBuilder builder, ToXContent.Params params) thr if (numericDetection.explicit() || includeDefaults) { builder.field("numeric_detection", numericDetection.value()); } + if (dotsInLeafFields.explicit() || includeDefaults) { + builder.field("allow_dots_in_leaf_fields", dotsInLeafFields.value()); + } if (runtimeFields.size() > 0 && params.paramAsBoolean(TOXCONTENT_SKIP_RUNTIME, false) == false) { builder.startObject("runtime"); diff --git a/server/src/test/java/org/elasticsearch/index/mapper/DynamicMappingTests.java b/server/src/test/java/org/elasticsearch/index/mapper/DynamicMappingTests.java index 13949866e770c..e3b993d4d9303 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/DynamicMappingTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/DynamicMappingTests.java @@ -586,4 +586,22 @@ public void testDynamicRuntimeDotsInFieldNames() throws IOException { Strings.toString(doc.dynamicMappingsUpdate()) ); } + + public void testDynamicConcreteDotsInFieldNames() throws IOException { + MapperService mapperService = createMapperService(topMapping(b -> b.field("allow_dots_in_leaf_fields", true))); + ParsedDocument doc = mapperService.documentMapper().parse(source(b -> { + b.field("instrument.name", "fred"); + b.startObject("metrics"); + b.field("temp", 20); + b.field("temp.min", 15); + b.field("temp.max", 25); + b.endObject(); + })); + assertEquals("{\"_doc\":{\"properties\":{" + + "\"instrument.name\":{\"type\":\"text\",\"fields\":{\"instrument.name.keyword\":{\"type\":\"keyword\"}}}," + + "\"metrics\":{\"type\":\"object\",\"properties\":{" + + "\"temp\":{\"type\":\"int\"},\"temp.min\":{\"type\":\"int\"},\"temp.max\":{\"type\":\"int\"}" + + "}}}}}", + Strings.toString(doc.dynamicMappingsUpdate())); + } } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/MappingParserTests.java b/server/src/test/java/org/elasticsearch/index/mapper/MappingParserTests.java index b706834c7089f..6a4ad24828668 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/MappingParserTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/MappingParserTests.java @@ -8,6 +8,7 @@ package org.elasticsearch.index.mapper; +import org.apache.lucene.util.BytesRef; import org.elasticsearch.Version; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.compress.CompressedXContent; @@ -48,7 +49,7 @@ private static MappingParser createMappingParser(Settings settings) { () -> metadataMappers, type -> MapperService.SINGLE_MAPPING_NAME); } - public void testFieldNameWithDots() throws Exception { + public void testFieldNameWithDotsDisallowed() throws Exception { XContentBuilder builder = mapping(b -> { b.startObject("foo.bar").field("type", "text").endObject(); b.startObject("foo.baz").field("type", "keyword").endObject(); @@ -62,6 +63,24 @@ public void testFieldNameWithDots() throws Exception { assertNotNull(objectMapper.getMapper("baz")); } + public void testFieldNameWithDotsAllowed() throws Exception { + DocumentMapper docMapper = createDocumentMapper(topMapping(b -> { + b.field("allow_dots_in_leaf_fields", true); + b.startObject("properties"); + b.startObject("foo.bar").field("type", "text").endObject(); + b.startObject("foo.baz").field("type", "keyword").endObject(); + b.endObject(); + })); + ParsedDocument doc = docMapper.parse(source(b -> { + b.field("foo.bar", "first"); + b.field("foo.baz", "second"); + })); + + assertNull(doc.dynamicMappingsUpdate()); + assertNotNull(doc.rootDoc().getField("foo.bar")); + assertEquals(new BytesRef("second"), doc.rootDoc().getField("foo.baz").binaryValue()); + } + public void testFieldNameWithDeepDots() throws Exception { XContentBuilder builder = mapping(b -> { b.startObject("foo.bar").field("type", "text").endObject(); @@ -82,7 +101,7 @@ public void testFieldNameWithDeepDots() throws Exception { assertNotNull(mappingLookup.objectMappers().get("foo")); } - public void testFieldNameWithDotsConflict() throws IOException { + public void testFieldNameWithDotPrefixDisallowed() throws IOException { XContentBuilder builder = mapping(b -> { b.startObject("foo").field("type", "text").endObject(); b.startObject("foo.baz").field("type", "keyword").endObject(); @@ -92,6 +111,19 @@ public void testFieldNameWithDotsConflict() throws IOException { assertTrue(e.getMessage(), e.getMessage().contains("mapper [foo] cannot be changed from type [text] to [ObjectMapper]")); } + public void testFieldNameWithDotPrefixAllowed() throws IOException { + XContentBuilder builder = topMapping(b -> { + b.field("allow_dots_in_leaf_fields", true); + b.startObject("properties"); + b.startObject("foo").field("type", "text").endObject(); + b.startObject("foo.baz").field("type", "keyword").endObject(); + b.endObject(); + }); + MapperService mapperService = createMapperService(builder); + assertEquals("text", mapperService.fieldType("foo").typeName()); + assertEquals("keyword", mapperService.fieldType("foo.baz").typeName()); + } + public void testMultiFieldsWithFieldAlias() throws IOException { XContentBuilder builder = mapping(b -> { b.startObject("field"); 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 154dcff84a985..67d7a984bd67d 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java @@ -17,6 +17,7 @@ import java.util.Arrays; import java.util.Collections; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.instanceOf; public class RootObjectMapperTests extends MapperServiceTestCase { @@ -136,6 +137,15 @@ public void testIllegalFormatField() throws Exception { } } + public void testAllowDotsInLeafFields() throws Exception { + MapperService mapperService = createMapperService(topMapping(b -> b.field("allow_dots_in_leaf_fields", true))); + Exception e = expectThrows( + MapperParsingException.class, + () -> merge(mapperService, topMapping(b -> b.field("allow_dots_in_leaf_fields", false))) + ); + assertThat(e.getMessage(), containsString("Can't change parameter [allow_dots_in_leaf_fields] from [true] to [false]")); + } + public void testRuntimeSection() throws IOException { String mapping = Strings.toString(runtimeMapping(builder -> { builder.startObject("field1").field("type", "double").endObject(); From 28233e387ef36b04b614787ace431194257ec52d Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Tue, 12 Oct 2021 15:08:05 +0100 Subject: [PATCH 02/13] Add 'flatten' option to ObjectMapper --- .../index/mapper/DocumentMapper.java | 4 +- .../index/mapper/DocumentParser.java | 43 ++++++- .../index/mapper/DocumentParserContext.java | 4 + .../index/mapper/DynamicFieldsBuilder.java | 6 +- .../elasticsearch/index/mapper/Mapper.java | 5 + .../elasticsearch/index/mapper/Mapping.java | 3 +- .../index/mapper/MappingParserContext.java | 21 +--- .../index/mapper/NestedObjectMapper.java | 16 ++- .../index/mapper/ObjectMapper.java | 108 ++++++++++++++++-- .../index/mapper/RootObjectMapper.java | 76 +++++------- .../index/mapper/DynamicMappingTests.java | 20 ++-- .../index/mapper/DynamicTemplatesTests.java | 43 +++++++ .../FieldAliasMapperValidationTests.java | 1 + .../index/mapper/MappingLookupTests.java | 10 +- .../index/mapper/MappingParserTests.java | 4 +- .../index/mapper/ObjectMapperTests.java | 2 +- .../index/mapper/RootObjectMapperTests.java | 6 +- 17 files changed, 260 insertions(+), 112 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java index 0f52c02eeef27..545d544c86920 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java @@ -8,6 +8,7 @@ package org.elasticsearch.index.mapper; +import org.elasticsearch.common.Explicit; import org.elasticsearch.common.compress.CompressedXContent; import org.elasticsearch.index.IndexSettings; @@ -23,7 +24,8 @@ public class DocumentMapper { * @return the newly created document mapper */ public static DocumentMapper createEmpty(MapperService mapperService) { - RootObjectMapper root = new RootObjectMapper.Builder(MapperService.SINGLE_MAPPING_NAME).build(MapperBuilderContext.ROOT); + RootObjectMapper root = new RootObjectMapper.Builder(MapperService.SINGLE_MAPPING_NAME, new Explicit<>(false, false)) + .build(MapperBuilderContext.ROOT); MetadataFieldMapper[] metadata = mapperService.getMetadataMappers().values().toArray(new MetadataFieldMapper[0]); Mapping mapping = new Mapping(root, metadata, null); return new DocumentMapper(mapperService.documentParser(), mapping); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java index 2252822b61ab9..91492a1297e44 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java @@ -102,8 +102,7 @@ public ParsedDocument parseDocument(SourceToParse source, MappingLookup mappingL context.reorderParentAndGetDocs(), context.sourceToParse().source(), context.sourceToParse().getXContentType(), - createDynamicUpdate(mappingLookup, - context.getDynamicMappers(), context.getDynamicRuntimeFields()) + createDynamicUpdate(context) ); } @@ -250,6 +249,23 @@ private static String[] splitAndValidatePath(String fullFieldPath) { } } + static Mapping createDynamicUpdate(DocumentParserContext context) { + if (context.getDynamicMappers().isEmpty() && context.getDynamicRuntimeFields().isEmpty()) { + return null; + } + RootObjectMapper.Builder rootBuilder = context.updateRoot(); + for (Mapper mapper : context.getDynamicMappers()) { + splitAndValidatePath(mapper.name()); + rootBuilder.addDynamic(mapper.name(), null, mapper, context); + } + for (RuntimeField runtimeField : context.getDynamicRuntimeFields()) { + rootBuilder.addRuntimeField(runtimeField); + } + RootObjectMapper root = rootBuilder.build(MapperBuilderContext.ROOT); + root.fixRedundantIncludes(); + return context.mappingLookup().getMapping().mappingUpdate(root); + } + /** * Creates a Mapping containing any dynamically added fields, or returns null if there were no dynamic mappings. */ @@ -679,13 +695,18 @@ private static void parseValue(final DocumentParserContext context, ObjectMapper if (mapper != null) { parseObjectOrField(context, mapper); } else { - if (context.root().allowDotsInLeafFields()) { + if (parentMapper.flatten.value()) { parseDynamicValue(context, parentMapper, currentFieldName, token); } else { - currentFieldName = paths[paths.length - 1]; Tuple parentMapperTuple = getDynamicParentMapper(context, paths, parentMapper); parentMapper = parentMapperTuple.v2(); - parseDynamicValue(context, parentMapper, currentFieldName, token); + int pathLength = parentMapperTuple.v1(); + StringBuilder compositeFieldName = new StringBuilder(paths[pathLength]); + while (pathLength < paths.length - 1) { + pathLength++; + compositeFieldName.append(".").append(paths[pathLength]); + } + parseDynamicValue(context, parentMapper, compositeFieldName.toString(), token); for (int i = 0; i < parentMapperTuple.v1(); i++) { context.path().remove(); } @@ -817,6 +838,9 @@ private static Tuple getDynamicParentMapper(DocumentParse context.path().add(paths[i]); pathsAdded++; parent = mapper; + if (parent.flatten.value()) { + break; + } } return new Tuple<>(pathsAdded, mapper); } @@ -986,7 +1010,14 @@ protected String contentType() { private static class NoOpObjectMapper extends ObjectMapper { NoOpObjectMapper(String name, String fullPath) { - super(name, fullPath, new Explicit<>(true, false), Dynamic.RUNTIME, Collections.emptyMap()); + super( + name, + fullPath, + new Explicit<>(true, false), + new Explicit<>(false, false), + Dynamic.RUNTIME, + Collections.emptyMap() + ); } } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java index 9c4f5f5e82822..98d391b706884 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java @@ -257,6 +257,10 @@ public final List getDynamicRuntimeFields() { */ public abstract Iterable nonRootDocuments(); + public final RootObjectMapper.Builder updateRoot() { + return mappingLookup.getMapping().getRoot().mappingUpdate(); + } + /** * Return a new context that will be within a copy-to operation. */ diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DynamicFieldsBuilder.java b/server/src/main/java/org/elasticsearch/index/mapper/DynamicFieldsBuilder.java index 1820955a475d7..8aa06801d4d7e 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DynamicFieldsBuilder.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DynamicFieldsBuilder.java @@ -10,6 +10,7 @@ import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.common.CheckedBiConsumer; +import org.elasticsearch.common.Explicit; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.time.DateFormatter; import org.elasticsearch.xcontent.XContentParser; @@ -127,7 +128,10 @@ Mapper createDynamicObjectMapper(DocumentParserContext context, String name) { Mapper mapper = createObjectMapperFromTemplate(context, name); return mapper != null ? mapper - : new ObjectMapper.Builder(name).enabled(true).build(MapperBuilderContext.forPath(context.path())); + : new ObjectMapper.Builder( + name, + new Explicit<>(false, false)).enabled(true).build(MapperBuilderContext.forPath(context.path()) + ); } /** diff --git a/server/src/main/java/org/elasticsearch/index/mapper/Mapper.java b/server/src/main/java/org/elasticsearch/index/mapper/Mapper.java index ebb743dca2402..e110db2bcf1ee 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/Mapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/Mapper.java @@ -8,6 +8,7 @@ package org.elasticsearch.index.mapper; +import org.elasticsearch.common.Strings; import org.elasticsearch.xcontent.ToXContentFragment; import java.util.Map; @@ -66,4 +67,8 @@ public final String simpleName() { */ public abstract void validate(MappingLookup mappers); + @Override + public String toString() { + return name() + ":" + Strings.toString(this); + } } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java b/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java index ca6fe392236e3..88bef1251eb9d 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java @@ -9,6 +9,7 @@ package org.elasticsearch.index.mapper; import org.elasticsearch.ElasticsearchGenerationException; +import org.elasticsearch.common.Explicit; import org.elasticsearch.common.Strings; import org.elasticsearch.common.compress.CompressedXContent; import org.elasticsearch.xcontent.ToXContent; @@ -35,7 +36,7 @@ public final class Mapping implements ToXContentFragment { public static final Mapping EMPTY = new Mapping( - new RootObjectMapper.Builder("_doc").build(MapperBuilderContext.ROOT), + new RootObjectMapper.Builder("_doc", new Explicit<>(false, false)).build(MapperBuilderContext.ROOT), new MetadataFieldMapper[0], null); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MappingParserContext.java b/server/src/main/java/org/elasticsearch/index/mapper/MappingParserContext.java index a880dcf5acc3a..dc38b128721c8 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MappingParserContext.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MappingParserContext.java @@ -65,7 +65,7 @@ protected MappingParserContext(MappingParserContext in) { in.similarityLookupService, in.typeParsers, in.runtimeFieldParsers, - in.indexVersionCreated.minimumIndexCompatibilityVersion(), + in.indexVersionCreated, in.searchExecutionContextSupplier, in.dateFormatter, in.scriptCompiler, @@ -131,10 +131,6 @@ public boolean isFromDynamicTemplate() { return false; } - public boolean allowDotsInFieldNames() { - return false; - } - protected Function similarityLookupService() { return similarityLookupService; } @@ -150,10 +146,6 @@ MappingParserContext createMultiFieldContext(MappingParserContext in) { return new MultiFieldParserContext(in); } - MappingParserContext createDotsInFieldNamesContext() { - return new DotsInFieldNamesParserContext(this); - } - private static class MultiFieldParserContext extends MappingParserContext { MultiFieldParserContext(MappingParserContext in) { super(in); @@ -175,15 +167,4 @@ public boolean isFromDynamicTemplate() { return true; } } - - private static class DotsInFieldNamesParserContext extends MappingParserContext { - DotsInFieldNamesParserContext(MappingParserContext in) { - super(in); - } - - @Override - public boolean allowDotsInFieldNames() { - return true; - } - } } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/NestedObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/NestedObjectMapper.java index f14918e9da802..39ab4ae3549e4 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/NestedObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/NestedObjectMapper.java @@ -34,7 +34,7 @@ public static class Builder extends ObjectMapper.Builder { private final Version indexCreatedVersion; public Builder(String name, Version indexCreatedVersion) { - super(name); + super(name, new Explicit<>(false, false)); this.indexCreatedVersion = indexCreatedVersion; } @@ -94,6 +94,7 @@ protected static void parseNested(String name, Map node, NestedO private Explicit includeInParent; private final String nestedTypePath; private final Query nestedTypeFilter; + private final Version indexVersionCreated; NestedObjectMapper( String name, @@ -101,7 +102,7 @@ protected static void parseNested(String name, Map node, NestedO Map mappers, Builder builder ) { - super(name, fullPath, builder.enabled, builder.dynamic, mappers); + super(name, fullPath, builder.enabled, new Explicit<>(false, false), builder.dynamic, mappers); if (builder.indexCreatedVersion.before(Version.V_8_0_0)) { this.nestedTypePath = "__" + fullPath; } else { @@ -110,6 +111,7 @@ protected static void parseNested(String name, Map node, NestedO this.nestedTypeFilter = NestedPathFieldMapper.filter(builder.indexCreatedVersion, nestedTypePath); this.includeInParent = builder.includeInParent; this.includeInRoot = builder.includeInRoot; + this.indexVersionCreated = builder.indexCreatedVersion; } public Query nestedTypeFilter() { @@ -145,6 +147,16 @@ public Map getChildren() { return Collections.unmodifiableMap(this.mappers); } + @Override + public ObjectMapper.Builder mappingUpdate() { + NestedObjectMapper.Builder builder = new NestedObjectMapper.Builder(simpleName(), indexVersionCreated); + builder.enabled = enabled; + builder.dynamic = dynamic; + builder.includeInRoot = includeInRoot; + builder.includeInParent = includeInParent; + return builder; + } + @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(simpleName()); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java index abaf4ff8950e0..60d3ed8bf46ba 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java @@ -13,10 +13,10 @@ import org.elasticsearch.common.collect.CopyOnWriteHashMap; import org.elasticsearch.common.logging.DeprecationCategory; import org.elasticsearch.common.logging.DeprecationLogger; -import org.elasticsearch.xcontent.ToXContent; -import org.elasticsearch.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.support.XContentMapValues; import org.elasticsearch.index.mapper.MapperService.MergeReason; +import org.elasticsearch.xcontent.ToXContent; +import org.elasticsearch.xcontent.XContentBuilder; import java.io.IOException; import java.util.ArrayList; @@ -35,8 +35,11 @@ public class ObjectMapper extends Mapper implements Cloneable { public static final String CONTENT_TYPE = "object"; + public static final String FLATTEN = "flatten"; + public static class Defaults { public static final boolean ENABLED = true; + public static final boolean FLATTENED = false; } public enum Dynamic { @@ -63,13 +66,19 @@ DynamicFieldsBuilder getDynamicFieldsBuilder() { public static class Builder extends Mapper.Builder { protected Explicit enabled = new Explicit<>(true, false); + protected final Explicit flatten; protected Dynamic dynamic; protected final List mappersBuilders = new ArrayList<>(); - public Builder(String name) { + public Builder(String name, Explicit flatten) { super(name); + this.flatten = flatten; + } + + public Builder(String name) { + this(name, new Explicit<>(false, false)); } public Builder enabled(boolean enabled) { @@ -87,6 +96,40 @@ public Builder add(Mapper.Builder builder) { return this; } + public void addDynamic(String name, String prefix, Mapper mapper, DocumentParserContext context) { + if (flatten.value() || name.contains(".") == false) { + mappersBuilders.add(new Mapper.Builder(name) { + @Override + public Mapper build(MapperBuilderContext context) { + return mapper; + } + }); + } + else { + int firstDotIndex = name.indexOf("."); + String childName = name.substring(0, firstDotIndex); + String fullChildName = prefix == null ? childName : prefix + "." + childName; + ObjectMapper.Builder childBuilder = findChild(childName, fullChildName, context); + childBuilder.addDynamic(name.substring(firstDotIndex + 1), fullChildName, mapper, context); + mappersBuilders.add(childBuilder); + } + } + + private ObjectMapper.Builder findChild(String childName, String fullChildName, DocumentParserContext context) { + // does the child mapper already exist? if so, use that + ObjectMapper child = context.mappingLookup().objectMappers().get(fullChildName); + if (child != null) { + return child.mappingUpdate(); + } + // has the child mapper been added as a dynamic update already? + child = context.getObjectMapper(fullChildName); + if (child != null) { + return child.mappingUpdate(); + } + // create a new child mapper + return new ObjectMapper.Builder(childName); + } + protected final Map buildMappers(boolean root, MapperBuilderContext context) { if (root == false) { context = context.createChildContext(name); @@ -94,6 +137,10 @@ protected final Map buildMappers(boolean root, MapperBuilderCont Map mappers = new HashMap<>(); for (Mapper.Builder builder : mappersBuilders) { Mapper mapper = builder.build(context); + if (flatten.value() && mapper instanceof ObjectMapper) { + throw new MapperParsingException( + "Mapper [" + name + "] can only contain flat leaf fields, but [" + mapper.name() + "] is an object"); + } Mapper existing = mappers.get(mapper.simpleName()); if (existing != null) { mapper = existing.merge(mapper); @@ -105,7 +152,7 @@ protected final Map buildMappers(boolean root, MapperBuilderCont @Override public ObjectMapper build(MapperBuilderContext context) { - return new ObjectMapper(name, context.buildFullName(name), enabled, dynamic, buildMappers(false, context)); + return new ObjectMapper(name, context.buildFullName(name), enabled, flatten, dynamic, buildMappers(false, context)); } } @@ -117,7 +164,7 @@ public Mapper.Builder parse(String name, throws MapperParsingException { return parse(name, false, (n, c) -> { - ObjectMapper.Builder builder = new Builder(n); + ObjectMapper.Builder builder = new Builder(n, parseFlatten(node)); for (Iterator> iterator = node.entrySet().iterator(); iterator.hasNext();) { Map.Entry entry = iterator.next(); String fieldName = entry.getKey(); @@ -130,8 +177,6 @@ public Mapper.Builder parse(String name, }, parserContext); } - - private static Mapper.Builder parse( String name, boolean allowDotsInFieldNames, @@ -146,7 +191,7 @@ private static Mapper.Builder parse( Mapper.Builder builder = parser.apply(realFieldName, parserContext); for (int i = fieldNameParts.length - 2; i >= 0; --i) { ObjectMapper.Builder intermediate - = new ObjectMapper.Builder(fieldNameParts[i]); + = new ObjectMapper.Builder(fieldNameParts[i], new Explicit<>(false, false)); intermediate.add(builder); builder = intermediate; } @@ -229,7 +274,7 @@ protected static void parseProperties(ObjectMapper.Builder objBuilder, } objBuilder.add(parse( fieldName, - parserContext.allowDotsInFieldNames(), + objBuilder.flatten.value(), (n, c) -> typeParser.parse(n, propNode, c), parserContext )); @@ -246,23 +291,40 @@ protected static void parseProperties(ObjectMapper.Builder objBuilder, MappingParser.checkNoRemainingFields(propsNode, "DocType mapping definition has unsupported parameters: "); } + + protected static Explicit parseFlatten(Map node) { + if (node.containsKey(FLATTEN) == false) { + return new Explicit<>(false, false); + } + boolean value = XContentMapValues.nodeBooleanValue(node.get(FLATTEN), FLATTEN); + node.remove(FLATTEN); + return new Explicit<>(value, true); + } } private final String fullPath; protected Explicit enabled; - + protected Explicit flatten; protected volatile Dynamic dynamic; protected volatile CopyOnWriteHashMap mappers; - ObjectMapper(String name, String fullPath, Explicit enabled, Dynamic dynamic, Map mappers) { + ObjectMapper( + String name, + String fullPath, + Explicit enabled, + Explicit flatten, + Dynamic dynamic, + Map mappers + ) { super(name); if (name.isEmpty()) { throw new IllegalArgumentException("name cannot be empty string"); } this.fullPath = fullPath; this.enabled = enabled; + this.flatten = flatten; this.dynamic = dynamic; if (mappers == null) { this.mappers = new CopyOnWriteHashMap<>(); @@ -298,6 +360,13 @@ final ObjectMapper mappingUpdate(Mapper mapper) { return mappingUpdate; } + public ObjectMapper.Builder mappingUpdate() { + ObjectMapper.Builder builder = new ObjectMapper.Builder(simpleName(), flatten); + builder.enabled = this.enabled; + builder.dynamic = this.dynamic; + return builder; + } + @Override public String name() { return this.fullPath; @@ -377,6 +446,19 @@ protected void doMerge(final ObjectMapper mergeWith, MergeReason reason) { } } + if (mergeWith.flatten.explicit()) { + if (reason == MergeReason.INDEX_TEMPLATE) { + this.flatten = mergeWith.flatten; + } else { + if (this.flatten.value().equals(mergeWith.flatten.value()) == false) { + throw new MapperParsingException( + "Can't change parameter [flatten] from [" + + this.flatten.value() + "] to [" + mergeWith.flatten.value() + "]" + ); + } + } + } + for (Mapper mergeWithMapper : mergeWith) { Mapper mergeIntoMapper = mappers.get(mergeWithMapper.simpleName()); @@ -423,7 +505,9 @@ void toXContent(XContentBuilder builder, Params params, ToXContent custom) throw if (isEnabled() != Defaults.ENABLED) { builder.field("enabled", enabled.value()); } - + if (flatten.value() != Defaults.FLATTENED) { + builder.field("flatten", flatten.value()); + } if (custom != null) { custom.toXContent(builder, params); } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java index 16d6c9afb4b24..419f567e91378 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java @@ -15,7 +15,6 @@ import org.elasticsearch.common.logging.DeprecationCategory; import org.elasticsearch.common.logging.DeprecationLogger; import org.elasticsearch.common.time.DateFormatter; -import org.elasticsearch.common.xcontent.support.XContentMapValues; import org.elasticsearch.index.mapper.DynamicTemplate.XContentFieldType; import org.elasticsearch.index.mapper.MapperService.MergeReason; import org.elasticsearch.xcontent.ToXContent; @@ -52,8 +51,6 @@ public class RootObjectMapper extends ObjectMapper { */ static final String TOXCONTENT_SKIP_RUNTIME = "skip_runtime"; - public static final String ALLOW_DOTS_IN_LEAF_FIELDS = "allow_dots_in_leaf_fields"; - public static class Defaults { public static final DateFormatter[] DYNAMIC_DATE_TIME_FORMATTERS = new DateFormatter[]{ @@ -71,11 +68,14 @@ public static class Builder extends ObjectMapper.Builder { protected Explicit dynamicDateTimeFormatters = new Explicit<>(Defaults.DYNAMIC_DATE_TIME_FORMATTERS, false); protected Explicit dateDetection = new Explicit<>(Defaults.DATE_DETECTION, false); protected Explicit numericDetection = new Explicit<>(Defaults.NUMERIC_DETECTION, false); - protected Explicit dotsInLeafFields = new Explicit<>(Defaults.DOTS_IN_LEAF_FIELDS, false); - protected Map runtimeFields; + protected final Map runtimeFields = new HashMap<>(); + + public Builder(String name, Explicit flatten) { + super(name, flatten); + } public Builder(String name) { - super(name); + this(name, new Explicit<>(false, false)); } public Builder dynamicDateTimeFormatter(Collection dateTimeFormatters) { @@ -94,8 +94,13 @@ public RootObjectMapper.Builder add(Mapper.Builder builder) { return this; } + public RootObjectMapper.Builder addRuntimeField(RuntimeField runtimeField) { + this.runtimeFields.put(runtimeField.name(), runtimeField); + return this; + } + public RootObjectMapper.Builder setRuntime(Map runtimeFields) { - this.runtimeFields = runtimeFields; + this.runtimeFields.putAll(runtimeFields); return this; } @@ -104,14 +109,14 @@ public RootObjectMapper build(MapperBuilderContext context) { return new RootObjectMapper( name, enabled, + flatten, dynamic, buildMappers(true, context), runtimeFields == null ? Collections.emptyMap() : runtimeFields, dynamicDateTimeFormatters, dynamicTemplates, dateDetection, - numericDetection, - dotsInLeafFields); + numericDetection); } } @@ -151,10 +156,7 @@ static final class TypeParser extends ObjectMapper.TypeParser { @Override public RootObjectMapper.Builder parse(String name, Map node, MappingParserContext parserContext) throws MapperParsingException { - RootObjectMapper.Builder builder = new Builder(name); - if (allowDotsInLeafFieldNames(node, builder)) { - parserContext = parserContext.createDotsInFieldNamesContext(); - } + RootObjectMapper.Builder builder = new Builder(name, parseFlatten(node)); Iterator> iterator = node.entrySet().iterator(); while (iterator.hasNext()) { Map.Entry entry = iterator.next(); @@ -168,16 +170,6 @@ public RootObjectMapper.Builder parse(String name, Map node, Map return builder; } - private static boolean allowDotsInLeafFieldNames(Map node, RootObjectMapper.Builder builder) { - if (node.containsKey(ALLOW_DOTS_IN_LEAF_FIELDS) == false) { - return false; - } - boolean value = XContentMapValues.nodeBooleanValue(node.get(ALLOW_DOTS_IN_LEAF_FIELDS), ALLOW_DOTS_IN_LEAF_FIELDS); - node.remove(ALLOW_DOTS_IN_LEAF_FIELDS); - builder.dotsInLeafFields = new Explicit<>(value, true); - return value; - } - @SuppressWarnings("unchecked") private boolean processField(RootObjectMapper.Builder builder, String fieldName, @@ -257,29 +249,26 @@ private boolean processField(RootObjectMapper.Builder builder, private Explicit dateDetection; private Explicit numericDetection; private Explicit dynamicTemplates; - private Explicit dotsInLeafFields; private Map runtimeFields; RootObjectMapper( String name, Explicit enabled, + Explicit flatten, Dynamic dynamic, Map mappers, Map runtimeFields, - Explicit dynamicDateTimeFormatters, Explicit dynamicTemplates, Explicit dateDetection, - Explicit numericDetection, - Explicit dotsInLeafFields + Explicit numericDetection ) { - super(name, name, enabled, dynamic, mappers); + super(name, name, enabled, flatten, dynamic, mappers); this.runtimeFields = runtimeFields; this.dynamicTemplates = dynamicTemplates; this.dynamicDateTimeFormatters = dynamicDateTimeFormatters; this.dateDetection = dateDetection; this.numericDetection = numericDetection; - this.dotsInLeafFields = dotsInLeafFields; } @Override @@ -289,6 +278,14 @@ protected ObjectMapper clone() { return clone; } + @Override + public RootObjectMapper.Builder mappingUpdate() { + RootObjectMapper.Builder builder = new RootObjectMapper.Builder(name(), flatten); + builder.enabled = enabled; + builder.dynamic = dynamic; + return builder; + } + @Override RootObjectMapper copyAndReset() { RootObjectMapper copy = (RootObjectMapper) super.copyAndReset(); @@ -299,7 +296,6 @@ RootObjectMapper copyAndReset() { copy.dynamicDateTimeFormatters = new Explicit<>(Defaults.DYNAMIC_DATE_TIME_FORMATTERS, false); copy.dateDetection = new Explicit<>(Defaults.DATE_DETECTION, false); copy.numericDetection = new Explicit<>(Defaults.NUMERIC_DETECTION, false); - copy.dotsInLeafFields = new Explicit<>(Defaults.DOTS_IN_LEAF_FIELDS, false); //also no need to carry the already defined runtime fields, only new ones need to be added copy.runtimeFields.clear(); return copy; @@ -333,10 +329,6 @@ public DynamicTemplate[] dynamicTemplates() { return dynamicTemplates.value(); } - public boolean allowDotsInLeafFields() { - return dotsInLeafFields.value(); - } - Collection runtimeFields() { return runtimeFields.values(); } @@ -366,19 +358,6 @@ protected void doMerge(ObjectMapper mergeWith, MergeReason reason) { this.dynamicDateTimeFormatters = mergeWithObject.dynamicDateTimeFormatters; } - if (mergeWithObject.dotsInLeafFields.explicit()) { - if (reason == MergeReason.INDEX_TEMPLATE) { - this.dotsInLeafFields = mergeWithObject.dotsInLeafFields; - } else { - if (this.dotsInLeafFields.value().equals(mergeWithObject.dotsInLeafFields.value()) == false) { - throw new MapperParsingException( - "Can't change parameter [allow_dots_in_leaf_fields] from [" - + this.dotsInLeafFields.value() + "] to [" + mergeWithObject.dotsInLeafFields.value() + "]" - ); - } - } - } - if (mergeWithObject.dynamicTemplates.explicit()) { if (reason == MergeReason.INDEX_TEMPLATE) { Map templatesByKey = new LinkedHashMap<>(); @@ -439,9 +418,6 @@ protected void doXContent(XContentBuilder builder, ToXContent.Params params) thr if (numericDetection.explicit() || includeDefaults) { builder.field("numeric_detection", numericDetection.value()); } - if (dotsInLeafFields.explicit() || includeDefaults) { - builder.field("allow_dots_in_leaf_fields", dotsInLeafFields.value()); - } if (runtimeFields.size() > 0 && params.paramAsBoolean(TOXCONTENT_SKIP_RUNTIME, false) == false) { builder.startObject("runtime"); diff --git a/server/src/test/java/org/elasticsearch/index/mapper/DynamicMappingTests.java b/server/src/test/java/org/elasticsearch/index/mapper/DynamicMappingTests.java index edeaa793e0d2a..c6291e0c5cf42 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/DynamicMappingTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/DynamicMappingTests.java @@ -277,8 +277,8 @@ public void testIntroduceTwoFields() throws Exception { } public void testObject() throws Exception { - DocumentMapper mapper = createDocumentMapper(mapping(b -> {})); - ParsedDocument doc = mapper.parse(source(b -> { + MapperService mapperService = createMapperService(mapping(b -> {})); + ParsedDocument doc = mapperService.documentMapper().parse(source(b -> { b.startObject("foo"); { b.startObject("bar").field("baz", "foo").endObject(); @@ -287,7 +287,8 @@ public void testObject() throws Exception { })); assertNotNull(doc.dynamicMappingsUpdate()); - assertThat(Strings.toString(doc.dynamicMappingsUpdate()), + merge(mapperService, dynamicMapping(doc.dynamicMappingsUpdate())); + assertThat(Strings.toString(mapperService.documentMapper().mapping()), containsString("{\"foo\":{\"properties\":{\"bar\":{\"properties\":{\"baz\":{\"type\":\"text\"")); } @@ -588,20 +589,17 @@ public void testDynamicRuntimeDotsInFieldNames() throws IOException { } public void testDynamicConcreteDotsInFieldNames() throws IOException { - MapperService mapperService = createMapperService(topMapping(b -> b.field("allow_dots_in_leaf_fields", true))); + MapperService mapperService = createMapperService(topMapping(b -> b.field("flatten", true))); ParsedDocument doc = mapperService.documentMapper().parse(source(b -> { b.field("instrument.name", "fred"); - b.startObject("metrics"); b.field("temp", 20); b.field("temp.min", 15); b.field("temp.max", 25); - b.endObject(); })); - assertEquals("{\"_doc\":{\"properties\":{" + - "\"instrument.name\":{\"type\":\"text\",\"fields\":{\"instrument.name.keyword\":{\"type\":\"keyword\"}}}," + - "\"metrics\":{\"type\":\"object\",\"properties\":{" + - "\"temp\":{\"type\":\"int\"},\"temp.min\":{\"type\":\"int\"},\"temp.max\":{\"type\":\"int\"}" + - "}}}}}", + assertEquals("{\"_doc\":{\"flatten\":true,\"properties\":{" + + "\"instrument.name\":{\"type\":\"text\",\"fields\":{\"keyword\":{\"type\":\"keyword\",\"ignore_above\":256}}}," + + "\"temp\":{\"type\":\"long\"},\"temp.max\":{\"type\":\"long\"},\"temp.min\":{\"type\":\"long\"}" + + "}}}", Strings.toString(doc.dynamicMappingsUpdate())); } } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/DynamicTemplatesTests.java b/server/src/test/java/org/elasticsearch/index/mapper/DynamicTemplatesTests.java index f9e6924a26a4b..fc22eaa030061 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/DynamicTemplatesTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/DynamicTemplatesTests.java @@ -954,4 +954,47 @@ public void testDynamicRuntimeWithDynamicTemplate() throws IOException { Strings.toString(parsedDoc2.dynamicMappingsUpdate()) ); } + + public void testFlattenedObjects() throws IOException { + MapperService mapperService = createMapperService(topMapping(b -> { + b.startArray("dynamic_templates"); + { + b.startObject(); + { + b.startObject("test"); + { + b.field("match_mapping_type", "object"); + b.field("match", "metric"); + b.startObject("mapping").field("type", "object").field("flatten", true).endObject(); + } + b.endObject(); + } + b.endObject(); + } + b.endArray(); + })); + + ParsedDocument doc = mapperService.documentMapper().parse(source(b -> { + b.field("foo.bar", 10); + b.field("foo.metric.count", 10); + b.field("foo.metric.count.min", 4); + b.field("foo.metric.count.max", 15); + b.startObject("bar"); + b.startObject("baz").field("count", 4).endObject(); + b.startObject("bill"); + b.startObject("metric"); + b.field("count", 10); + b.field("count.min", 5); + b.field("count.max", 15); + b.endObject(); + b.endObject(); + b.endObject(); + })); + + merge(mapperService, dynamicMapping(doc.dynamicMappingsUpdate())); + + assertThat(mapperService.fieldType("foo.metric.count").typeName(), equalTo("long")); + assertThat(mapperService.fieldType("bar.bill.metric.count").typeName(), equalTo("long")); + assertNotNull(mapperService.mappingLookup().objectMappers().get("bar.bill.metric")); + } } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/FieldAliasMapperValidationTests.java b/server/src/test/java/org/elasticsearch/index/mapper/FieldAliasMapperValidationTests.java index e9e0f4ba1e52b..66c200ce2ed25 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/FieldAliasMapperValidationTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/FieldAliasMapperValidationTests.java @@ -185,6 +185,7 @@ private static FieldMapper createFieldMapper(String parent, String name) { private static ObjectMapper createObjectMapper(String name) { return new ObjectMapper(name, name, new Explicit<>(true, false), + new Explicit<>(false, false), ObjectMapper.Dynamic.FALSE, emptyMap()); } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/MappingLookupTests.java b/server/src/test/java/org/elasticsearch/index/mapper/MappingLookupTests.java index 7bab424a7e0a3..29fa83b841449 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/MappingLookupTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/MappingLookupTests.java @@ -67,8 +67,14 @@ public void testRuntimeFieldLeafOverride() { public void testSubfieldOverride() { MockFieldMapper fieldMapper = new MockFieldMapper("object.subfield"); - ObjectMapper objectMapper = new ObjectMapper("object", "object", new Explicit<>(true, true), - ObjectMapper.Dynamic.TRUE, Collections.singletonMap("object.subfield", fieldMapper)); + ObjectMapper objectMapper = new ObjectMapper( + "object", + "object", + new Explicit<>(true, true), + new Explicit<>(false, false), + ObjectMapper.Dynamic.TRUE, + Collections.singletonMap("object.subfield", fieldMapper) + ); MappingLookup mappingLookup = createMappingLookup( Collections.singletonList(fieldMapper), Collections.singletonList(objectMapper), diff --git a/server/src/test/java/org/elasticsearch/index/mapper/MappingParserTests.java b/server/src/test/java/org/elasticsearch/index/mapper/MappingParserTests.java index 0c4ae7731f839..94177267f8b6e 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/MappingParserTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/MappingParserTests.java @@ -65,7 +65,7 @@ public void testFieldNameWithDotsDisallowed() throws Exception { public void testFieldNameWithDotsAllowed() throws Exception { DocumentMapper docMapper = createDocumentMapper(topMapping(b -> { - b.field("allow_dots_in_leaf_fields", true); + b.field("flatten", true); b.startObject("properties"); b.startObject("foo.bar").field("type", "text").endObject(); b.startObject("foo.baz").field("type", "keyword").endObject(); @@ -113,7 +113,7 @@ public void testFieldNameWithDotPrefixDisallowed() throws IOException { public void testFieldNameWithDotPrefixAllowed() throws IOException { XContentBuilder builder = topMapping(b -> { - b.field("allow_dots_in_leaf_fields", true); + b.field("flatten", true); b.startObject("properties"); b.startObject("foo").field("type", "text").endObject(); b.startObject("foo.baz").field("type", "keyword").endObject(); diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java index 2d53b5566974e..7536b4471bd5a 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java @@ -40,7 +40,7 @@ public void testDifferentInnerObjectTokenFailure() throws Exception { " \"value\":\"value\"\n" + " }"), XContentType.JSON))); - assertTrue(e.getMessage(), e.getMessage().contains("cannot be changed from type")); + assertThat(e.getMessage(), containsString("can't merge a non object mapping [object.array.object] with an object mapping")); } public void testEmptyArrayProperties() throws Exception { 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 2fb283d1217b0..86ca6fbfb3413 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/RootObjectMapperTests.java @@ -138,12 +138,12 @@ public void testIllegalFormatField() throws Exception { } public void testAllowDotsInLeafFields() throws Exception { - MapperService mapperService = createMapperService(topMapping(b -> b.field("allow_dots_in_leaf_fields", true))); + MapperService mapperService = createMapperService(topMapping(b -> b.field("flatten", true))); Exception e = expectThrows( MapperParsingException.class, - () -> merge(mapperService, topMapping(b -> b.field("allow_dots_in_leaf_fields", false))) + () -> merge(mapperService, topMapping(b -> b.field("flatten", false))) ); - assertThat(e.getMessage(), containsString("Can't change parameter [allow_dots_in_leaf_fields] from [true] to [false]")); + assertThat(e.getMessage(), containsString("Can't change parameter [flatten] from [true] to [false]")); } public void testRuntimeSection() throws IOException { From 75ca57ca2e754f59a44695407b26e4d61dd5353c Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Tue, 12 Oct 2021 18:09:54 +0100 Subject: [PATCH 03/13] tidying up --- .../index/mapper/DocumentParser.java | 176 +----------------- .../index/mapper/DocumentParserContext.java | 2 +- .../index/mapper/NestedObjectMapper.java | 2 +- .../index/mapper/ObjectMapper.java | 21 +-- .../index/mapper/RootObjectMapper.java | 17 +- .../index/mapper/DocumentParserTests.java | 93 +++++---- 6 files changed, 68 insertions(+), 243 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java index 91492a1297e44..af25e9a0791e3 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java @@ -17,11 +17,7 @@ import org.elasticsearch.common.Strings; import org.elasticsearch.common.time.DateFormatter; import org.elasticsearch.common.xcontent.LoggingDeprecationHandler; -import org.elasticsearch.xcontent.NamedXContentRegistry; -import org.elasticsearch.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentHelper; -import org.elasticsearch.xcontent.XContentParser; -import org.elasticsearch.xcontent.XContentType; import org.elasticsearch.core.Tuple; import org.elasticsearch.index.IndexSettings; import org.elasticsearch.index.analysis.IndexAnalyzers; @@ -29,11 +25,14 @@ import org.elasticsearch.index.query.SearchExecutionContext; import org.elasticsearch.indices.breaker.NoneCircuitBreakerService; import org.elasticsearch.search.lookup.SearchLookup; +import org.elasticsearch.xcontent.NamedXContentRegistry; +import org.elasticsearch.xcontent.XContentBuilder; +import org.elasticsearch.xcontent.XContentParser; +import org.elasticsearch.xcontent.XContentType; import java.io.IOException; import java.util.ArrayList; import java.util.Collections; -import java.util.Comparator; import java.util.HashMap; import java.util.Iterator; import java.util.LinkedList; @@ -266,173 +265,6 @@ static Mapping createDynamicUpdate(DocumentParserContext context) { return context.mappingLookup().getMapping().mappingUpdate(root); } - /** - * Creates a Mapping containing any dynamically added fields, or returns null if there were no dynamic mappings. - */ - static Mapping createDynamicUpdate(MappingLookup mappingLookup, - List dynamicMappers, - List dynamicRuntimeFields) { - if (dynamicMappers.isEmpty() && dynamicRuntimeFields.isEmpty()) { - return null; - } - RootObjectMapper root; - if (dynamicMappers.isEmpty() == false) { - root = createDynamicUpdate(mappingLookup, dynamicMappers); - root.fixRedundantIncludes(); - } else { - root = mappingLookup.getMapping().getRoot().copyAndReset(); - } - root.addRuntimeFields(dynamicRuntimeFields); - return mappingLookup.getMapping().mappingUpdate(root); - } - - private static RootObjectMapper createDynamicUpdate(MappingLookup mappingLookup, - List dynamicMappers) { - - // We build a mapping by first sorting the mappers, so that all mappers containing a common prefix - // will be processed in a contiguous block. When the prefix is no longer seen, we pop the extra elements - // off the stack, merging them upwards into the existing mappers. - dynamicMappers.sort(Comparator.comparing(Mapper::name)); - Iterator dynamicMapperItr = dynamicMappers.iterator(); - List parentMappers = new ArrayList<>(); - Mapper firstUpdate = dynamicMapperItr.next(); - parentMappers.add(createUpdate(mappingLookup.getMapping().getRoot(), splitAndValidatePath(firstUpdate.name()), 0, firstUpdate)); - Mapper previousMapper = null; - while (dynamicMapperItr.hasNext()) { - Mapper newMapper = dynamicMapperItr.next(); - if (previousMapper != null && newMapper.name().equals(previousMapper.name())) { - // We can see the same mapper more than once, for example, if we had foo.bar and foo.baz, where - // foo did not yet exist. This will create 2 copies in dynamic mappings, which should be identical. - // Here we just skip over the duplicates, but we merge them to ensure there are no conflicts. - newMapper.merge(previousMapper); - continue; - } - previousMapper = newMapper; - String[] nameParts = splitAndValidatePath(newMapper.name()); - - // We first need the stack to only contain mappers in common with the previously processed mapper - // For example, if the first mapper processed was a.b.c, and we now have a.d, the stack will contain - // a.b, and we want to merge b back into the stack so it just contains a - int i = removeUncommonMappers(parentMappers, nameParts); - - // Then we need to add back mappers that may already exist within the stack, but are not on it. - // For example, if we processed a.b, followed by an object mapper a.c.d, and now are adding a.c.d.e - // then the stack will only have a on it because we will have already merged a.c.d into the stack. - // So we need to pull a.c, followed by a.c.d, onto the stack so e can be added to the end. - i = expandCommonMappers(parentMappers, nameParts, i); - - // If there are still parents of the new mapper which are not on the stack, we need to pull them - // from the existing mappings. In order to maintain the invariant that the stack only contains - // fields which are updated, we cannot simply add the existing mappers to the stack, since they - // may have other subfields which will not be updated. Instead, we pull the mapper from the existing - // mappings, and build an update with only the new mapper and its parents. This then becomes our - // "new mapper", and can be added to the stack. - if (i < nameParts.length - 1) { - newMapper = createExistingMapperUpdate(parentMappers, nameParts, i, mappingLookup, newMapper); - } - - if (newMapper instanceof ObjectMapper) { - parentMappers.add((ObjectMapper) newMapper); - } else { - addToLastMapper(parentMappers, newMapper, true); - } - } - popMappers(parentMappers, 1, true); - assert parentMappers.size() == 1; - return (RootObjectMapper) parentMappers.get(0); - } - - private static void popMappers(List parentMappers, int keepBefore, boolean merge) { - assert keepBefore >= 1; // never remove the root mapper - // pop off parent mappers not needed by the current mapper, - // merging them backwards since they are immutable - for (int i = parentMappers.size() - 1; i >= keepBefore; --i) { - addToLastMapper(parentMappers, parentMappers.remove(i), merge); - } - } - - /** - * Adds a mapper as an update into the last mapper. If merge is true, the new mapper - * will be merged in with other child mappers of the last parent, otherwise it will be a new update. - */ - private static void addToLastMapper(List parentMappers, Mapper mapper, boolean merge) { - assert parentMappers.size() >= 1; - int lastIndex = parentMappers.size() - 1; - ObjectMapper withNewMapper = parentMappers.get(lastIndex).mappingUpdate(mapper); - if (merge) { - withNewMapper = parentMappers.get(lastIndex).merge(withNewMapper); - } - parentMappers.set(lastIndex, withNewMapper); - } - - /** - * Removes mappers that exist on the stack, but are not part of the path of the current nameParts, - * Returns the next unprocessed index from nameParts. - */ - private static int removeUncommonMappers(List parentMappers, String[] nameParts) { - int keepBefore = 1; - while (keepBefore < parentMappers.size() && - parentMappers.get(keepBefore).simpleName().equals(nameParts[keepBefore - 1])) { - ++keepBefore; - } - popMappers(parentMappers, keepBefore, true); - return keepBefore - 1; - } - - /** - * Adds mappers from the end of the stack that exist as updates within those mappers. - * Returns the next unprocessed index from nameParts. - */ - private static int expandCommonMappers(List parentMappers, String[] nameParts, int i) { - ObjectMapper last = parentMappers.get(parentMappers.size() - 1); - while (i < nameParts.length - 1 && last.getMapper(nameParts[i]) != null) { - Mapper newLast = last.getMapper(nameParts[i]); - assert newLast instanceof ObjectMapper; - last = (ObjectMapper) newLast; - parentMappers.add(last); - ++i; - } - return i; - } - - /** - * Creates an update for intermediate object mappers that are not on the stack, but parents of newMapper. - */ - private static ObjectMapper createExistingMapperUpdate(List parentMappers, String[] nameParts, int i, - MappingLookup mappingLookup, Mapper newMapper) { - String updateParentName = nameParts[i]; - final ObjectMapper lastParent = parentMappers.get(parentMappers.size() - 1); - if (parentMappers.size() > 1) { - // only prefix with parent mapper if the parent mapper isn't the root (which has a fake name) - updateParentName = lastParent.name() + '.' + nameParts[i]; - } - ObjectMapper updateParent = mappingLookup.objectMappers().get(updateParentName); - assert updateParent != null : updateParentName + " doesn't exist"; - return createUpdate(updateParent, nameParts, i + 1, newMapper); - } - - /** - * Build an update for the parent which will contain the given mapper and any intermediate fields. - */ - private static ObjectMapper createUpdate(ObjectMapper parent, String[] nameParts, int i, Mapper mapper) { - List parentMappers = new ArrayList<>(); - ObjectMapper previousIntermediate = parent; - for (; i < nameParts.length - 1; ++i) { - Mapper intermediate = previousIntermediate.getMapper(nameParts[i]); - assert intermediate != null : "Field " + previousIntermediate.name() + " does not have a subfield " + nameParts[i]; - assert intermediate instanceof ObjectMapper; - parentMappers.add((ObjectMapper) intermediate); - previousIntermediate = (ObjectMapper) intermediate; - } - if (parentMappers.isEmpty() == false) { - // add the new mapper to the stack, and pop down to the original parent level - addToLastMapper(parentMappers, mapper, false); - popMappers(parentMappers, 1, false); - mapper = parentMappers.get(0); - } - return parent.mappingUpdate(mapper); - } - static void parseObjectOrNested(DocumentParserContext context, ObjectMapper mapper) throws IOException { if (mapper.isEnabled() == false) { context.parser().skipChildren(); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java index 98d391b706884..b406a78ba679f 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java @@ -258,7 +258,7 @@ public final List getDynamicRuntimeFields() { public abstract Iterable nonRootDocuments(); public final RootObjectMapper.Builder updateRoot() { - return mappingLookup.getMapping().getRoot().mappingUpdate(); + return mappingLookup.getMapping().getRoot().newBuilder(); } /** diff --git a/server/src/main/java/org/elasticsearch/index/mapper/NestedObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/NestedObjectMapper.java index 39ab4ae3549e4..2906f65445859 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/NestedObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/NestedObjectMapper.java @@ -148,7 +148,7 @@ public Map getChildren() { } @Override - public ObjectMapper.Builder mappingUpdate() { + public ObjectMapper.Builder newBuilder() { NestedObjectMapper.Builder builder = new NestedObjectMapper.Builder(simpleName(), indexVersionCreated); builder.enabled = enabled; builder.dynamic = dynamic; diff --git a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java index 60d3ed8bf46ba..93d84db8f357b 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java @@ -119,12 +119,12 @@ private ObjectMapper.Builder findChild(String childName, String fullChildName, D // does the child mapper already exist? if so, use that ObjectMapper child = context.mappingLookup().objectMappers().get(fullChildName); if (child != null) { - return child.mappingUpdate(); + return child.newBuilder(); } // has the child mapper been added as a dynamic update already? child = context.getObjectMapper(fullChildName); if (child != null) { - return child.mappingUpdate(); + return child.newBuilder(); } // create a new child mapper return new ObjectMapper.Builder(childName); @@ -344,23 +344,10 @@ protected ObjectMapper clone() { return clone; } - ObjectMapper copyAndReset() { - ObjectMapper copy = clone(); - // reset the sub mappers - copy.mappers = new CopyOnWriteHashMap<>(); - return copy; - } - /** - * Build a mapping update with the provided sub mapping update. + * @return a Builder that will produce an empty ObjectMapper with the same configuration as this one */ - final ObjectMapper mappingUpdate(Mapper mapper) { - ObjectMapper mappingUpdate = copyAndReset(); - mappingUpdate.putMapper(mapper); - return mappingUpdate; - } - - public ObjectMapper.Builder mappingUpdate() { + public ObjectMapper.Builder newBuilder() { ObjectMapper.Builder builder = new ObjectMapper.Builder(simpleName(), flatten); builder.enabled = this.enabled; builder.dynamic = this.dynamic; diff --git a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java index 419f567e91378..991b14cd7f9fb 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java @@ -279,28 +279,13 @@ protected ObjectMapper clone() { } @Override - public RootObjectMapper.Builder mappingUpdate() { + public RootObjectMapper.Builder newBuilder() { RootObjectMapper.Builder builder = new RootObjectMapper.Builder(name(), flatten); builder.enabled = enabled; builder.dynamic = dynamic; return builder; } - @Override - RootObjectMapper copyAndReset() { - RootObjectMapper copy = (RootObjectMapper) super.copyAndReset(); - // for dynamic updates, no need to carry root-specific options, we just - // set everything to their implicit default value so that they are not - // applied at merge time - copy.dynamicTemplates = new Explicit<>(new DynamicTemplate[0], false); - copy.dynamicDateTimeFormatters = new Explicit<>(Defaults.DYNAMIC_DATE_TIME_FORMATTERS, false); - copy.dateDetection = new Explicit<>(Defaults.DATE_DETECTION, false); - copy.numericDetection = new Explicit<>(Defaults.NUMERIC_DETECTION, false); - //also no need to carry the already defined runtime fields, only new ones need to be added - copy.runtimeFields.clear(); - return copy; - } - /** * Public API */ diff --git a/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java b/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java index 6089dfd478aa4..97eac3cb0d514 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java @@ -17,20 +17,19 @@ import org.elasticsearch.common.Strings; import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.bytes.BytesReference; -import org.elasticsearch.xcontent.XContentBuilder; -import org.elasticsearch.xcontent.XContentFactory; -import org.elasticsearch.xcontent.XContentParser; -import org.elasticsearch.xcontent.XContentType; import org.elasticsearch.plugins.MapperPlugin; import org.elasticsearch.plugins.Plugin; import org.elasticsearch.script.CompositeFieldScript; import org.elasticsearch.search.lookup.SearchLookup; +import org.elasticsearch.xcontent.XContentBuilder; +import org.elasticsearch.xcontent.XContentFactory; +import org.elasticsearch.xcontent.XContentParser; +import org.elasticsearch.xcontent.XContentType; import java.io.IOException; import java.math.BigDecimal; import java.math.BigInteger; import java.nio.charset.StandardCharsets; -import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.List; @@ -585,7 +584,22 @@ DocumentMapper createDummyMapping() throws Exception { } MapperService createMapperService() throws Exception { - return createMapperService(mapping(b -> { + return createMapperService(topMapping(b -> { + b.startArray("dynamic_templates"); + { + b.startObject(); + { + b.startObject("test"); + { + b.field("match", "runtime*"); + b.startObject("runtime").field("type", "keyword").endObject(); + } + b.endObject(); + } + b.endObject(); + } + b.endArray(); + b.startObject("properties"); b.startObject("y").field("type", "object").endObject(); b.startObject("x"); { @@ -605,6 +619,7 @@ MapperService createMapperService() throws Exception { b.endObject(); } b.endObject(); + b.endObject(); })); } @@ -620,30 +635,31 @@ private static ObjectMapper createObjectMapper(String name) { public void testEmptyMappingUpdate() throws Exception { DocumentMapper docMapper = createDummyMapping(); - assertNull(DocumentParser.createDynamicUpdate(docMapper.mappers(), Collections.emptyList(), Collections.emptyList())); + ParsedDocument doc = docMapper.parse(source(b -> {})); + assertNull(doc.dynamicMappingsUpdate()); } public void testSingleMappingUpdate() throws Exception { DocumentMapper docMapper = createDummyMapping(); - List updates = Collections.singletonList(new MockFieldMapper("foo")); - Mapping mapping = DocumentParser.createDynamicUpdate(docMapper.mappers(), updates, Collections.emptyList()); + ParsedDocument doc = docMapper.parse(source(b -> b.field("foo", 10))); + Mapping mapping = doc.dynamicMappingsUpdate(); assertNotNull(mapping); assertNotNull(mapping.getRoot().getMapper("foo")); } public void testSingleRuntimeFieldMappingUpdate() throws Exception { DocumentMapper docMapper = createDummyMapping(); - List updates = Collections.singletonList(new TestRuntimeField("foo", "any")); - Mapping mapping = DocumentParser.createDynamicUpdate(docMapper.mappers(), Collections.emptyList(), updates); + ParsedDocument doc = docMapper.parse(source(b -> b.field("runtime-field", "10"))); + Mapping mapping = doc.dynamicMappingsUpdate(); assertNotNull(mapping); - assertNull(mapping.getRoot().getMapper("foo")); - assertNotNull(mapping.getRoot().getRuntimeField("foo")); + assertNull(mapping.getRoot().getMapper("runtime-field")); + assertNotNull(mapping.getRoot().getRuntimeField("runtime-field")); } public void testSubfieldMappingUpdate() throws Exception { DocumentMapper docMapper = createDummyMapping(); - List updates = Collections.singletonList(new MockFieldMapper("x.foo")); - Mapping mapping = DocumentParser.createDynamicUpdate(docMapper.mappers(), updates, Collections.emptyList()); + ParsedDocument doc = docMapper.parse(source(b -> b.field("x.foo", 10))); + Mapping mapping = doc.dynamicMappingsUpdate(); assertNotNull(mapping); Mapper xMapper = mapping.getRoot().getMapper("x"); assertNotNull(xMapper); @@ -653,21 +669,22 @@ public void testSubfieldMappingUpdate() throws Exception { } public void testRuntimeSubfieldMappingUpdate() throws Exception { - DocumentMapper docMapper = createDummyMapping(); - List updates = Collections.singletonList(new TestRuntimeField("x.foo", "any")); - Mapping mapping = DocumentParser.createDynamicUpdate(docMapper.mappers(), Collections.emptyList(), updates); + DocumentMapper docMapper = createDocumentMapper(topMapping(b -> b.field("dynamic", "runtime"))); + ParsedDocument doc = docMapper.parse(source(b -> b.field("runtime.foo", 10))); + Mapping mapping = doc.dynamicMappingsUpdate(); assertNotNull(mapping); - Mapper xMapper = mapping.getRoot().getMapper("x"); + Mapper xMapper = mapping.getRoot().getMapper("runtime"); assertNull(xMapper); - assertNotNull(mapping.getRoot().getRuntimeField("x.foo")); + assertNotNull(mapping.getRoot().getRuntimeField("runtime.foo")); } public void testMultipleSubfieldMappingUpdate() throws Exception { DocumentMapper docMapper = createDummyMapping(); - List updates = new ArrayList<>(); - updates.add(new MockFieldMapper("x.foo")); - updates.add(new MockFieldMapper("x.bar")); - Mapping mapping = DocumentParser.createDynamicUpdate(docMapper.mappers(), updates, Collections.emptyList()); + ParsedDocument doc = docMapper.parse(source(b -> { + b.field("x.foo", 10); + b.field("x.bar", 20); + })); + Mapping mapping = doc.dynamicMappingsUpdate(); assertNotNull(mapping); Mapper xMapper = mapping.getRoot().getMapper("x"); assertNotNull(xMapper); @@ -679,8 +696,8 @@ public void testMultipleSubfieldMappingUpdate() throws Exception { public void testDeepSubfieldMappingUpdate() throws Exception { DocumentMapper docMapper = createDummyMapping(); - List updates = Collections.singletonList(new MockFieldMapper("x.subx.foo")); - Mapping mapping = DocumentParser.createDynamicUpdate(docMapper.mappers(), updates, Collections.emptyList()); + ParsedDocument doc = docMapper.parse(source(b -> b.field("x.subx.foo", 10))); + Mapping mapping = doc.dynamicMappingsUpdate(); assertNotNull(mapping); Mapper xMapper = mapping.getRoot().getMapper("x"); assertNotNull(xMapper); @@ -693,10 +710,11 @@ public void testDeepSubfieldMappingUpdate() throws Exception { public void testDeepSubfieldAfterSubfieldMappingUpdate() throws Exception { DocumentMapper docMapper = createDummyMapping(); - List updates = new ArrayList<>(); - updates.add(new MockFieldMapper("x.a")); - updates.add(new MockFieldMapper("x.subx.b")); - Mapping mapping = DocumentParser.createDynamicUpdate(docMapper.mappers(), updates, Collections.emptyList()); + ParsedDocument doc = docMapper.parse(source(b -> { + b.field("x.a", 10); + b.field("x.subx.b", 10); + })); + Mapping mapping = doc.dynamicMappingsUpdate(); assertNotNull(mapping); Mapper xMapper = mapping.getRoot().getMapper("x"); assertNotNull(xMapper); @@ -710,12 +728,15 @@ public void testDeepSubfieldAfterSubfieldMappingUpdate() throws Exception { public void testObjectMappingUpdate() throws Exception { MapperService mapperService = createMapperService(); DocumentMapper docMapper = mapperService.documentMapper(); - List updates = new ArrayList<>(); - updates.add(createObjectMapper("foo")); - updates.add(createObjectMapper("foo.bar")); - updates.add(new MockFieldMapper("foo.bar.baz")); - updates.add(new MockFieldMapper("foo.field")); - Mapping mapping = DocumentParser.createDynamicUpdate(docMapper.mappers(), updates, Collections.emptyList()); + ParsedDocument doc = docMapper.parse(source(b -> { + b.startObject("foo"); + b.startObject("bar"); + b.field("baz", 10); + b.endObject(); + b.field("field", 10); + b.endObject(); + })); + Mapping mapping = doc.dynamicMappingsUpdate(); assertNotNull(mapping); Mapper fooMapper = mapping.getRoot().getMapper("foo"); assertNotNull(fooMapper); From 4ee36b4f95206b6284e125c0df70dea1ea1248bd Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Wed, 13 Oct 2021 10:24:11 +0100 Subject: [PATCH 04/13] tidy up --- .../index/mapper/DocumentMapper.java | 3 +- .../index/mapper/DocumentParser.java | 9 ++- .../index/mapper/DocumentParserContext.java | 3 + .../index/mapper/DynamicFieldsBuilder.java | 7 +- .../elasticsearch/index/mapper/Mapping.java | 7 +- .../index/mapper/NestedObjectMapper.java | 4 +- .../index/mapper/ObjectMapper.java | 66 ++++++++++--------- .../index/mapper/RootObjectMapper.java | 9 +-- .../MetadataRolloverServiceTests.java | 2 +- .../index/MappingUpdatedActionTests.java | 2 +- .../index/mapper/DocumentParserTests.java | 10 --- .../FieldAliasMapperValidationTests.java | 4 +- .../index/mapper/MappingLookupTests.java | 4 +- .../index/mapper/ObjectMapperMergeTests.java | 10 +-- .../query/SearchExecutionContextTests.java | 2 +- 15 files changed, 67 insertions(+), 75 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java index 545d544c86920..4eb368ebc3b0c 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java @@ -8,7 +8,6 @@ package org.elasticsearch.index.mapper; -import org.elasticsearch.common.Explicit; import org.elasticsearch.common.compress.CompressedXContent; import org.elasticsearch.index.IndexSettings; @@ -24,7 +23,7 @@ public class DocumentMapper { * @return the newly created document mapper */ public static DocumentMapper createEmpty(MapperService mapperService) { - RootObjectMapper root = new RootObjectMapper.Builder(MapperService.SINGLE_MAPPING_NAME, new Explicit<>(false, false)) + RootObjectMapper root = new RootObjectMapper.Builder(MapperService.SINGLE_MAPPING_NAME, false) .build(MapperBuilderContext.ROOT); MetadataFieldMapper[] metadata = mapperService.getMetadataMappers().values().toArray(new MetadataFieldMapper[0]); Mapping mapping = new Mapping(root, metadata, null); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java index af25e9a0791e3..6be219863b71d 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java @@ -527,12 +527,15 @@ private static void parseValue(final DocumentParserContext context, ObjectMapper if (mapper != null) { parseObjectOrField(context, mapper); } else { - if (parentMapper.flatten.value()) { + if (parentMapper.flatten) { parseDynamicValue(context, parentMapper, currentFieldName, token); } else { Tuple parentMapperTuple = getDynamicParentMapper(context, paths, parentMapper); parentMapper = parentMapperTuple.v2(); int pathLength = parentMapperTuple.v1(); + // If our dynamic parent mapper is flattened, then we can't just assume that our name + // is the last path part. We instead need to construct it by concatenating all path + // parts from the parent mapper onwards. StringBuilder compositeFieldName = new StringBuilder(paths[pathLength]); while (pathLength < paths.length - 1) { pathLength++; @@ -670,7 +673,7 @@ private static Tuple getDynamicParentMapper(DocumentParse context.path().add(paths[i]); pathsAdded++; parent = mapper; - if (parent.flatten.value()) { + if (parent.flatten) { break; } } @@ -846,7 +849,7 @@ private static class NoOpObjectMapper extends ObjectMapper { name, fullPath, new Explicit<>(true, false), - new Explicit<>(false, false), + false, Dynamic.RUNTIME, Collections.emptyMap() ); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java index b406a78ba679f..fc157af32ac0f 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java @@ -257,6 +257,9 @@ public final List getDynamicRuntimeFields() { */ public abstract Iterable nonRootDocuments(); + /** + * @return a RootObjectMapper.Builder to be used to construct a dynamic mapping update + */ public final RootObjectMapper.Builder updateRoot() { return mappingLookup.getMapping().getRoot().newBuilder(); } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DynamicFieldsBuilder.java b/server/src/main/java/org/elasticsearch/index/mapper/DynamicFieldsBuilder.java index 8aa06801d4d7e..8bf3ea4c02a2b 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DynamicFieldsBuilder.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DynamicFieldsBuilder.java @@ -10,13 +10,12 @@ import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.common.CheckedBiConsumer; -import org.elasticsearch.common.Explicit; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.time.DateFormatter; -import org.elasticsearch.xcontent.XContentParser; import org.elasticsearch.core.CheckedRunnable; import org.elasticsearch.index.mapper.ObjectMapper.Dynamic; import org.elasticsearch.script.ScriptCompiler; +import org.elasticsearch.xcontent.XContentParser; import java.io.IOException; import java.time.format.DateTimeParseException; @@ -128,9 +127,7 @@ Mapper createDynamicObjectMapper(DocumentParserContext context, String name) { Mapper mapper = createObjectMapperFromTemplate(context, name); return mapper != null ? mapper - : new ObjectMapper.Builder( - name, - new Explicit<>(false, false)).enabled(true).build(MapperBuilderContext.forPath(context.path()) + : new ObjectMapper.Builder(name, false).enabled(true).build(MapperBuilderContext.forPath(context.path()) ); } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java b/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java index 88bef1251eb9d..1d94e0af096e6 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java @@ -9,16 +9,15 @@ package org.elasticsearch.index.mapper; import org.elasticsearch.ElasticsearchGenerationException; -import org.elasticsearch.common.Explicit; import org.elasticsearch.common.Strings; import org.elasticsearch.common.compress.CompressedXContent; +import org.elasticsearch.common.xcontent.XContentHelper; +import org.elasticsearch.index.mapper.MapperService.MergeReason; import org.elasticsearch.xcontent.ToXContent; import org.elasticsearch.xcontent.ToXContentFragment; import org.elasticsearch.xcontent.XContentBuilder; import org.elasticsearch.xcontent.XContentFactory; -import org.elasticsearch.common.xcontent.XContentHelper; import org.elasticsearch.xcontent.XContentType; -import org.elasticsearch.index.mapper.MapperService.MergeReason; import java.io.IOException; import java.io.UncheckedIOException; @@ -36,7 +35,7 @@ public final class Mapping implements ToXContentFragment { public static final Mapping EMPTY = new Mapping( - new RootObjectMapper.Builder("_doc", new Explicit<>(false, false)).build(MapperBuilderContext.ROOT), + new RootObjectMapper.Builder("_doc", false).build(MapperBuilderContext.ROOT), new MetadataFieldMapper[0], null); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/NestedObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/NestedObjectMapper.java index 2906f65445859..f79bb7ab66f53 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/NestedObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/NestedObjectMapper.java @@ -34,7 +34,7 @@ public static class Builder extends ObjectMapper.Builder { private final Version indexCreatedVersion; public Builder(String name, Version indexCreatedVersion) { - super(name, new Explicit<>(false, false)); + super(name, false); this.indexCreatedVersion = indexCreatedVersion; } @@ -102,7 +102,7 @@ protected static void parseNested(String name, Map node, NestedO Map mappers, Builder builder ) { - super(name, fullPath, builder.enabled, new Explicit<>(false, false), builder.dynamic, mappers); + super(name, fullPath, builder.enabled, false, builder.dynamic, mappers); if (builder.indexCreatedVersion.before(Version.V_8_0_0)) { this.nestedTypePath = "__" + fullPath; } else { diff --git a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java index 93d84db8f357b..0c53d94fd7295 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java @@ -39,7 +39,6 @@ public class ObjectMapper extends Mapper implements Cloneable { public static class Defaults { public static final boolean ENABLED = true; - public static final boolean FLATTENED = false; } public enum Dynamic { @@ -66,21 +65,17 @@ DynamicFieldsBuilder getDynamicFieldsBuilder() { public static class Builder extends Mapper.Builder { protected Explicit enabled = new Explicit<>(true, false); - protected final Explicit flatten; + protected final boolean flatten; protected Dynamic dynamic; protected final List mappersBuilders = new ArrayList<>(); - public Builder(String name, Explicit flatten) { + public Builder(String name, boolean flatten) { super(name); this.flatten = flatten; } - public Builder(String name) { - this(name, new Explicit<>(false, false)); - } - public Builder enabled(boolean enabled) { this.enabled = new Explicit<>(enabled, true); return this; @@ -96,8 +91,18 @@ public Builder add(Mapper.Builder builder) { return this; } + /** + * Adds a dynamically created Mapper to this builder. + * + * @param name the name of the Mapper, including object prefixes + * @param prefix the object prefix of this mapper + * @param mapper the mapper to add + * @param context the DocumentParserContext in which the mapper has been built + */ public void addDynamic(String name, String prefix, Mapper mapper, DocumentParserContext context) { - if (flatten.value() || name.contains(".") == false) { + // If we're a flattened mapper, or if the mapper to add has no dots and is therefore + // a leaf mapper, we just add it here + if (flatten || name.contains(".") == false) { mappersBuilders.add(new Mapper.Builder(name) { @Override public Mapper build(MapperBuilderContext context) { @@ -105,6 +110,10 @@ public Mapper build(MapperBuilderContext context) { } }); } + // otherwise we strip off the first object path of the mapper name, load or create + // the relevant object mapper, and then recurse down into it, passing the remainder + // of the mapper name. So for a mapper 'foo.bar.baz', we locate 'foo' and then + // call addDynamic on it with the name 'bar.baz'. else { int firstDotIndex = name.indexOf("."); String childName = name.substring(0, firstDotIndex); @@ -127,7 +136,7 @@ private ObjectMapper.Builder findChild(String childName, String fullChildName, D return child.newBuilder(); } // create a new child mapper - return new ObjectMapper.Builder(childName); + return new ObjectMapper.Builder(childName, false); } protected final Map buildMappers(boolean root, MapperBuilderContext context) { @@ -137,7 +146,7 @@ protected final Map buildMappers(boolean root, MapperBuilderCont Map mappers = new HashMap<>(); for (Mapper.Builder builder : mappersBuilders) { Mapper mapper = builder.build(context); - if (flatten.value() && mapper instanceof ObjectMapper) { + if (flatten && mapper instanceof ObjectMapper) { throw new MapperParsingException( "Mapper [" + name + "] can only contain flat leaf fields, but [" + mapper.name() + "] is an object"); } @@ -190,8 +199,7 @@ private static Mapper.Builder parse( String realFieldName = fieldNameParts[fieldNameParts.length - 1]; Mapper.Builder builder = parser.apply(realFieldName, parserContext); for (int i = fieldNameParts.length - 2; i >= 0; --i) { - ObjectMapper.Builder intermediate - = new ObjectMapper.Builder(fieldNameParts[i], new Explicit<>(false, false)); + ObjectMapper.Builder intermediate = new ObjectMapper.Builder(fieldNameParts[i], false); intermediate.add(builder); builder = intermediate; } @@ -274,7 +282,7 @@ protected static void parseProperties(ObjectMapper.Builder objBuilder, } objBuilder.add(parse( fieldName, - objBuilder.flatten.value(), + objBuilder.flatten, (n, c) -> typeParser.parse(n, propNode, c), parserContext )); @@ -292,20 +300,20 @@ protected static void parseProperties(ObjectMapper.Builder objBuilder, MappingParser.checkNoRemainingFields(propsNode, "DocType mapping definition has unsupported parameters: "); } - protected static Explicit parseFlatten(Map node) { + protected static boolean parseFlatten(Map node) { if (node.containsKey(FLATTEN) == false) { - return new Explicit<>(false, false); + return false; } boolean value = XContentMapValues.nodeBooleanValue(node.get(FLATTEN), FLATTEN); node.remove(FLATTEN); - return new Explicit<>(value, true); + return value; } } private final String fullPath; protected Explicit enabled; - protected Explicit flatten; + protected boolean flatten; protected volatile Dynamic dynamic; protected volatile CopyOnWriteHashMap mappers; @@ -314,7 +322,7 @@ protected static Explicit parseFlatten(Map node) { String name, String fullPath, Explicit enabled, - Explicit flatten, + boolean flatten, Dynamic dynamic, Map mappers ) { @@ -433,16 +441,14 @@ protected void doMerge(final ObjectMapper mergeWith, MergeReason reason) { } } - if (mergeWith.flatten.explicit()) { - if (reason == MergeReason.INDEX_TEMPLATE) { - this.flatten = mergeWith.flatten; - } else { - if (this.flatten.value().equals(mergeWith.flatten.value()) == false) { - throw new MapperParsingException( - "Can't change parameter [flatten] from [" - + this.flatten.value() + "] to [" + mergeWith.flatten.value() + "]" - ); - } + if (reason == MergeReason.INDEX_TEMPLATE) { + this.flatten = mergeWith.flatten; + } else { + if (this.flatten != mergeWith.flatten) { + throw new MapperParsingException( + "Can't change parameter [flatten] from [" + + this.flatten + "] to [" + mergeWith.flatten + "]" + ); } } @@ -492,8 +498,8 @@ void toXContent(XContentBuilder builder, Params params, ToXContent custom) throw if (isEnabled() != Defaults.ENABLED) { builder.field("enabled", enabled.value()); } - if (flatten.value() != Defaults.FLATTENED) { - builder.field("flatten", flatten.value()); + if (flatten) { + builder.field("flatten", flatten); } if (custom != null) { custom.toXContent(builder, params); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java index 991b14cd7f9fb..d2d98fe721ac3 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java @@ -59,7 +59,6 @@ public static class Defaults { }; public static final boolean DATE_DETECTION = true; public static final boolean NUMERIC_DETECTION = false; - public static final boolean DOTS_IN_LEAF_FIELDS = false; } public static class Builder extends ObjectMapper.Builder { @@ -70,14 +69,10 @@ public static class Builder extends ObjectMapper.Builder { protected Explicit numericDetection = new Explicit<>(Defaults.NUMERIC_DETECTION, false); protected final Map runtimeFields = new HashMap<>(); - public Builder(String name, Explicit flatten) { + public Builder(String name, boolean flatten) { super(name, flatten); } - public Builder(String name) { - this(name, new Explicit<>(false, false)); - } - public Builder dynamicDateTimeFormatter(Collection dateTimeFormatters) { this.dynamicDateTimeFormatters = new Explicit<>(dateTimeFormatters.toArray(new DateFormatter[0]), true); return this; @@ -254,7 +249,7 @@ private boolean processField(RootObjectMapper.Builder builder, RootObjectMapper( String name, Explicit enabled, - Explicit flatten, + boolean flatten, Dynamic dynamic, Map mappers, Map runtimeFields, diff --git a/server/src/test/java/org/elasticsearch/action/admin/indices/rollover/MetadataRolloverServiceTests.java b/server/src/test/java/org/elasticsearch/action/admin/indices/rollover/MetadataRolloverServiceTests.java index 9790fd700b7be..5a35c107013a3 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/indices/rollover/MetadataRolloverServiceTests.java +++ b/server/src/test/java/org/elasticsearch/action/admin/indices/rollover/MetadataRolloverServiceTests.java @@ -569,7 +569,7 @@ public void testRolloverClusterStateForDataStream() throws Exception { when(env.sharedDataFile()).thenReturn(null); AllocationService allocationService = mock(AllocationService.class); when(allocationService.reroute(any(ClusterState.class), any(String.class))).then(i -> i.getArguments()[0]); - RootObjectMapper.Builder root = new RootObjectMapper.Builder("_doc"); + RootObjectMapper.Builder root = new RootObjectMapper.Builder("_doc", false); root.add(new DateFieldMapper.Builder(dataStream.getTimeStampField().getName(), DateFieldMapper.Resolution.MILLISECONDS, DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER, ScriptCompiler.NONE, true, Version.CURRENT)); MetadataFieldMapper dtfm = getDataStreamTimestampFieldMapper(); diff --git a/server/src/test/java/org/elasticsearch/cluster/action/index/MappingUpdatedActionTests.java b/server/src/test/java/org/elasticsearch/cluster/action/index/MappingUpdatedActionTests.java index ae229c6df6444..4ac592b7b6697 100644 --- a/server/src/test/java/org/elasticsearch/cluster/action/index/MappingUpdatedActionTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/action/index/MappingUpdatedActionTests.java @@ -143,7 +143,7 @@ public void testSendUpdateMappingUsingAutoPutMappingAction() { new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS)); mua.setClient(client); - RootObjectMapper rootObjectMapper = new RootObjectMapper.Builder("name").build(MapperBuilderContext.ROOT); + RootObjectMapper rootObjectMapper = new RootObjectMapper.Builder("name", false).build(MapperBuilderContext.ROOT); Mapping update = new Mapping(rootObjectMapper, new MetadataFieldMapper[0], Map.of()); mua.sendUpdateMapping(new Index("name", "uuid"), update, ActionListener.wrap(() -> {})); diff --git a/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java b/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java index 97eac3cb0d514..f756793acd9c1 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java @@ -623,16 +623,6 @@ MapperService createMapperService() throws Exception { })); } - // creates an object mapper, which is about 100x harder than it should be.... - private static ObjectMapper createObjectMapper(String name) { - ContentPath path = new ContentPath(0); - String[] nameParts = name.split("\\."); - for (int i = 0; i < nameParts.length - 1; ++i) { - path.add(nameParts[i]); - } - return new ObjectMapper.Builder(nameParts[nameParts.length - 1]).enabled(true).build(MapperBuilderContext.forPath(path)); - } - public void testEmptyMappingUpdate() throws Exception { DocumentMapper docMapper = createDummyMapping(); ParsedDocument doc = docMapper.parse(source(b -> {})); diff --git a/server/src/test/java/org/elasticsearch/index/mapper/FieldAliasMapperValidationTests.java b/server/src/test/java/org/elasticsearch/index/mapper/FieldAliasMapperValidationTests.java index 66c200ce2ed25..e6a9d0e4db94f 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/FieldAliasMapperValidationTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/FieldAliasMapperValidationTests.java @@ -185,7 +185,7 @@ private static FieldMapper createFieldMapper(String parent, String name) { private static ObjectMapper createObjectMapper(String name) { return new ObjectMapper(name, name, new Explicit<>(true, false), - new Explicit<>(false, false), + false, ObjectMapper.Dynamic.FALSE, emptyMap()); } @@ -197,7 +197,7 @@ private static MappingLookup createMappingLookup(List fieldMappers, List objectMappers, List fieldAliasMappers, List runtimeFields) { - RootObjectMapper.Builder builder = new RootObjectMapper.Builder("_doc"); + RootObjectMapper.Builder builder = new RootObjectMapper.Builder("_doc", false); Map runtimeFieldTypes = runtimeFields.stream().collect(Collectors.toMap(RuntimeField::name, r -> r)); builder.setRuntime(runtimeFieldTypes); Mapping mapping = new Mapping( diff --git a/server/src/test/java/org/elasticsearch/index/mapper/MappingLookupTests.java b/server/src/test/java/org/elasticsearch/index/mapper/MappingLookupTests.java index 29fa83b841449..c767bbad4992b 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/MappingLookupTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/MappingLookupTests.java @@ -35,7 +35,7 @@ public class MappingLookupTests extends ESTestCase { private static MappingLookup createMappingLookup(List fieldMappers, List objectMappers, List runtimeFields) { - RootObjectMapper.Builder builder = new RootObjectMapper.Builder("_doc"); + RootObjectMapper.Builder builder = new RootObjectMapper.Builder("_doc", false); Map runtimeFieldTypes = runtimeFields.stream().collect(Collectors.toMap(RuntimeField::name, r -> r)); builder.setRuntime(runtimeFieldTypes); Mapping mapping = new Mapping( @@ -71,7 +71,7 @@ public void testSubfieldOverride() { "object", "object", new Explicit<>(true, true), - new Explicit<>(false, false), + false, ObjectMapper.Dynamic.TRUE, Collections.singletonMap("object.subfield", fieldMapper) ); diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java index e026b0cec1661..0062994b7a9b0 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java @@ -67,7 +67,7 @@ public void testMergeDisabledField() { // GIVEN a mapping with "foo" field disabled Map mappers = new HashMap<>(); //the field is disabled, and we are not trying to re-enable it, hence merge should work - mappers.put("disabled", new ObjectMapper.Builder("disabled").build(MapperBuilderContext.ROOT)); + mappers.put("disabled", new ObjectMapper.Builder("disabled", false).build(MapperBuilderContext.ROOT)); RootObjectMapper mergeWith = createRootObjectMapper("type1", true, Collections.unmodifiableMap(mappers)); RootObjectMapper merged = (RootObjectMapper)rootObjectMapper.merge(mergeWith); @@ -99,10 +99,10 @@ public void testMergeEnabledForRootMapper() { public void testMergeDisabledRootMapper() { String type = MapperService.SINGLE_MAPPING_NAME; final RootObjectMapper rootObjectMapper = - (RootObjectMapper) new RootObjectMapper.Builder(type).enabled(false).build(MapperBuilderContext.ROOT); + (RootObjectMapper) new RootObjectMapper.Builder(type, false).enabled(false).build(MapperBuilderContext.ROOT); //the root is disabled, and we are not trying to re-enable it, but we do want to be able to add runtime fields final RootObjectMapper mergeWith = - new RootObjectMapper.Builder(type) + new RootObjectMapper.Builder(type, false) .setRuntime(Collections.singletonMap("test", new TestRuntimeField("test", "long"))) .build(MapperBuilderContext.ROOT); @@ -133,7 +133,7 @@ public void testMergeNested() { private static RootObjectMapper createRootObjectMapper(String name, boolean enabled, Map mappers) { final RootObjectMapper rootObjectMapper - = (RootObjectMapper) new RootObjectMapper.Builder(name).enabled(enabled).build(MapperBuilderContext.ROOT); + = (RootObjectMapper) new RootObjectMapper.Builder(name, false).enabled(enabled).build(MapperBuilderContext.ROOT); mappers.values().forEach(rootObjectMapper::putMapper); @@ -141,7 +141,7 @@ private static RootObjectMapper createRootObjectMapper(String name, boolean enab } private static ObjectMapper createObjectMapper(String name, boolean enabled, Map mappers) { - final ObjectMapper mapper = new ObjectMapper.Builder(name).enabled(enabled).build(MapperBuilderContext.ROOT); + final ObjectMapper mapper = new ObjectMapper.Builder(name, false).enabled(enabled).build(MapperBuilderContext.ROOT); mappers.values().forEach(mapper::putMapper); diff --git a/server/src/test/java/org/elasticsearch/index/query/SearchExecutionContextTests.java b/server/src/test/java/org/elasticsearch/index/query/SearchExecutionContextTests.java index a3bfc11c74e17..ad500b5b44a32 100644 --- a/server/src/test/java/org/elasticsearch/index/query/SearchExecutionContextTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/SearchExecutionContextTests.java @@ -325,7 +325,7 @@ public void testFielddataLookupOneFieldManyReferences() throws IOException { private static MappingLookup createMappingLookup(List concreteFields, List runtimeFields) { List mappers = concreteFields.stream().map(MockFieldMapper::new).collect(Collectors.toList()); - RootObjectMapper.Builder builder = new RootObjectMapper.Builder("_doc"); + RootObjectMapper.Builder builder = new RootObjectMapper.Builder("_doc", false); Map runtimeFieldTypes = runtimeFields.stream().collect(Collectors.toMap(RuntimeField::name, r -> r)); builder.setRuntime(runtimeFieldTypes); Mapping mapping = new Mapping( From 0078b19ddb2590150fdae95313aa2ad11fd6c841 Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Tue, 7 Dec 2021 14:47:07 +0000 Subject: [PATCH 05/13] spotless --- .../index/mapper/DocumentMapper.java | 3 +- .../index/mapper/DocumentParser.java | 18 +++------ .../index/mapper/ObjectMapper.java | 10 +---- .../index/mapper/DynamicMappingTests.java | 6 ++- .../index/mapper/ObjectMapperTests.java | 39 ++++++++++++------- 5 files changed, 36 insertions(+), 40 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java index 525f244052a93..bf2d443312bda 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DocumentMapper.java @@ -26,8 +26,7 @@ public class DocumentMapper { * @return the newly created document mapper */ public static DocumentMapper createEmpty(MapperService mapperService) { - RootObjectMapper root = new RootObjectMapper.Builder(MapperService.SINGLE_MAPPING_NAME) - .build(MapperBuilderContext.ROOT); + RootObjectMapper root = new RootObjectMapper.Builder(MapperService.SINGLE_MAPPING_NAME).build(MapperBuilderContext.ROOT); MetadataFieldMapper[] metadata = mapperService.getMetadataMappers().values().toArray(new MetadataFieldMapper[0]); Mapping mapping = new Mapping(root, metadata, null); return new DocumentMapper(mapperService.documentParser(), mapping); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java index ba8e48e511a15..65999bf21ff74 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java @@ -214,7 +214,7 @@ private static MapperParsingException wrapInMapperParsingException(SourceToParse return new MapperParsingException("failed to parse", e); } - private static String[] splitAndValidatePath(String fullFieldPath) { + private static void validatePath(String fullFieldPath) { if (fullFieldPath.contains(".")) { String[] parts = fullFieldPath.split("\\."); if (parts.length == 0) { @@ -231,12 +231,10 @@ private static String[] splitAndValidatePath(String fullFieldPath) { ); } } - return parts; } else { if (Strings.isEmpty(fullFieldPath)) { throw new IllegalArgumentException("field name cannot be an empty string"); } - return new String[] { fullFieldPath }; } } @@ -246,7 +244,7 @@ static Mapping createDynamicUpdate(DocumentParserContext context) { } RootObjectMapper.Builder rootBuilder = context.updateRoot(); for (Mapper mapper : context.getDynamicMappers()) { - splitAndValidatePath(mapper.name()); + validatePath(mapper.name()); rootBuilder.addDynamic(mapper.name(), null, mapper, context); } for (RuntimeField runtimeField : context.getDynamicRuntimeFields()) { @@ -309,7 +307,7 @@ private static void innerParseObject(DocumentParserContext context, ObjectMapper while (token != XContentParser.Token.END_OBJECT) { if (token == XContentParser.Token.FIELD_NAME) { currentFieldName = context.parser().currentName(); - splitAndValidatePath(currentFieldName); + validatePath(currentFieldName); } else if (token == XContentParser.Token.START_OBJECT) { parseObject(context, mapper, currentFieldName); } else if (token == XContentParser.Token.START_ARRAY) { @@ -498,7 +496,7 @@ private static void parseNonDynamicArray( ) throws IOException { XContentParser parser = context.parser(); XContentParser.Token token; - splitAndValidatePath(lastFieldName); + validatePath(lastFieldName); while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) { if (token == XContentParser.Token.START_OBJECT) { parseObject(context, mapper, lastFieldName); @@ -744,13 +742,7 @@ protected String contentType() { private static class NoOpObjectMapper extends ObjectMapper { NoOpObjectMapper(String name, String fullPath) { - super( - name, - fullPath, - new Explicit<>(true, false), - Dynamic.RUNTIME, - Collections.emptyMap() - ); + super(name, fullPath, new Explicit<>(true, false), Dynamic.RUNTIME, Collections.emptyMap()); } } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java index f743ef33cc904..f43f93339738f 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java @@ -108,7 +108,7 @@ public Mapper build(MapperBuilderContext context) { } // otherwise we strip off the first object path of the mapper name, load or create // the relevant object mapper, and then recurse down into it, passing the remainder - // of the mapper name. So for a mapper 'foo.bar.baz', we locate 'foo' and then + // of the mapper name. So for a mapper 'foo.bar.baz', we locate 'foo' and then // call addDynamic on it with the name 'bar.baz'. else { int firstDotIndex = name.indexOf("."); @@ -289,13 +289,7 @@ protected static void parseProperties( protected volatile CopyOnWriteHashMap mappers; - ObjectMapper( - String name, - String fullPath, - Explicit enabled, - Dynamic dynamic, - Map mappers - ) { + ObjectMapper(String name, String fullPath, Explicit enabled, Dynamic dynamic, Map mappers) { super(name); if (name.isEmpty()) { throw new IllegalArgumentException("name cannot be empty string"); diff --git a/server/src/test/java/org/elasticsearch/index/mapper/DynamicMappingTests.java b/server/src/test/java/org/elasticsearch/index/mapper/DynamicMappingTests.java index 75a07bb8eca9a..8bbff7c28471b 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/DynamicMappingTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/DynamicMappingTests.java @@ -297,8 +297,10 @@ public void testObject() throws Exception { assertNotNull(doc.dynamicMappingsUpdate()); merge(mapperService, dynamicMapping(doc.dynamicMappingsUpdate())); - assertThat(Strings.toString(mapperService.documentMapper().mapping()), - containsString("{\"foo\":{\"properties\":{\"bar\":{\"properties\":{\"baz\":{\"type\":\"text\"")); + assertThat( + Strings.toString(mapperService.documentMapper().mapping()), + containsString("{\"foo\":{\"properties\":{\"bar\":{\"properties\":{\"baz\":{\"type\":\"text\"") + ); } public void testDynamicRuntimeFieldWithinObject() throws Exception { diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java index 90402fb61e0ce..52f8f0820f086 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperTests.java @@ -25,21 +25,30 @@ public class ObjectMapperTests extends MapperServiceTestCase { public void testDifferentInnerObjectTokenFailure() throws Exception { DocumentMapper defaultMapper = createDocumentMapper(mapping(b -> {})); - IllegalArgumentException e = expectThrows(IllegalArgumentException.class, - () -> defaultMapper.parse(new SourceToParse("1", new BytesArray(" {\n" + - " \"object\": {\n" + - " \"array\":[\n" + - " {\n" + - " \"object\": { \"value\": \"value\" }\n" + - " },\n" + - " {\n" + - " \"object\":\"value\"\n" + - " }\n" + - " ]\n" + - " },\n" + - " \"value\":\"value\"\n" + - " }"), - XContentType.JSON))); + IllegalArgumentException e = expectThrows( + IllegalArgumentException.class, + () -> defaultMapper.parse( + new SourceToParse( + "1", + new BytesArray( + " {\n" + + " \"object\": {\n" + + " \"array\":[\n" + + " {\n" + + " \"object\": { \"value\": \"value\" }\n" + + " },\n" + + " {\n" + + " \"object\":\"value\"\n" + + " }\n" + + " ]\n" + + " },\n" + + " \"value\":\"value\"\n" + + " }" + ), + XContentType.JSON + ) + ) + ); assertThat(e.getMessage(), containsString("can't merge a non object mapping [object.array.object] with an object mapping")); } From cc75426abc1ca3b1664925ca755c015f1422234c Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Tue, 7 Dec 2021 15:12:52 +0000 Subject: [PATCH 06/13] remove copyonwritehashmap from ObjectMapper --- .../index/mapper/ObjectMapper.java | 23 +++++++++++-------- .../index/mapper/ObjectMapperMergeTests.java | 14 ++++------- 2 files changed, 18 insertions(+), 19 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java index f43f93339738f..d9fbd258e49ef 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java @@ -10,7 +10,6 @@ import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.common.Explicit; -import org.elasticsearch.common.collect.CopyOnWriteHashMap; import org.elasticsearch.common.logging.DeprecationCategory; import org.elasticsearch.common.logging.DeprecationLogger; import org.elasticsearch.common.xcontent.support.XContentMapValues; @@ -87,6 +86,16 @@ public Builder add(Mapper.Builder builder) { return this; } + public Builder addMappers(Map mappers) { + mappers.forEach((name, mapper) -> mappersBuilders.add(new Mapper.Builder(name) { + @Override + public Mapper build(MapperBuilderContext context) { + return mapper; + } + })); + return this; + } + /** * Adds a dynamically created Mapper to this builder. * @@ -287,7 +296,7 @@ protected static void parseProperties( protected Explicit enabled; protected volatile Dynamic dynamic; - protected volatile CopyOnWriteHashMap mappers; + protected final Map mappers; ObjectMapper(String name, String fullPath, Explicit enabled, Dynamic dynamic, Map mappers) { super(name); @@ -298,9 +307,9 @@ protected static void parseProperties( this.enabled = enabled; this.dynamic = dynamic; if (mappers == null) { - this.mappers = new CopyOnWriteHashMap<>(); + this.mappers = new HashMap<>(); } else { - this.mappers = CopyOnWriteHashMap.copyOf(mappers); + this.mappers = new HashMap<>(mappers); } } @@ -347,10 +356,6 @@ public Mapper getMapper(String field) { return mappers.get(field); } - protected void putMapper(Mapper mapper) { - mappers = mappers.copyAndPut(mapper.simpleName(), mapper); - } - @Override public Iterator iterator() { return mappers.values().iterator(); @@ -429,7 +434,7 @@ protected void doMerge(final ObjectMapper mergeWith, MergeReason reason) { merged = mergeIntoMapper.merge(mergeWithMapper); } } - putMapper(merged); + mappers.put(merged.simpleName(), merged); } } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java index 0c6cb81015177..8e993740ec3e8 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java @@ -133,20 +133,14 @@ public void testMergeNested() { } private static RootObjectMapper createRootObjectMapper(String name, boolean enabled, Map mappers) { - final RootObjectMapper rootObjectMapper = (RootObjectMapper) new RootObjectMapper.Builder(name).enabled(enabled) + return (RootObjectMapper) new RootObjectMapper.Builder(name).enabled(enabled) + .addMappers(mappers) .build(MapperBuilderContext.ROOT); - - mappers.values().forEach(rootObjectMapper::putMapper); - - return rootObjectMapper; } private static ObjectMapper createObjectMapper(String name, boolean enabled, Map mappers) { - final ObjectMapper mapper = new ObjectMapper.Builder(name).enabled(enabled).build(MapperBuilderContext.ROOT); - - mappers.values().forEach(mapper::putMapper); - - return mapper; + return new ObjectMapper.Builder(name).enabled(enabled) + .addMappers(mappers).build(MapperBuilderContext.ROOT); } private TextFieldMapper createTextFieldMapper(String name) { From 39ade03681cebbcd6c7340f7c872845c3bb7a395 Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Wed, 8 Dec 2021 10:01:31 +0000 Subject: [PATCH 07/13] oops --- .../src/main/java/org/elasticsearch/index/mapper/Mapper.java | 2 +- .../main/java/org/elasticsearch/index/mapper/ObjectMapper.java | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/Mapper.java b/server/src/main/java/org/elasticsearch/index/mapper/Mapper.java index e110db2bcf1ee..c492cba3e6e4c 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/Mapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/Mapper.java @@ -69,6 +69,6 @@ public final String simpleName() { @Override public String toString() { - return name() + ":" + Strings.toString(this); + return Strings.toString(this); } } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java index d9fbd258e49ef..8ec3d6fe82ef7 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java @@ -296,7 +296,7 @@ protected static void parseProperties( protected Explicit enabled; protected volatile Dynamic dynamic; - protected final Map mappers; + protected Map mappers; ObjectMapper(String name, String fullPath, Explicit enabled, Dynamic dynamic, Map mappers) { super(name); @@ -321,6 +321,7 @@ protected ObjectMapper clone() { } catch (CloneNotSupportedException e) { throw new RuntimeException(e); } + clone.mappers = new HashMap<>(clone.mappers); return clone; } From 087886aec09ede1572c4bd0cc9cdbd8106a232e3 Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Wed, 8 Dec 2021 10:31:13 +0000 Subject: [PATCH 08/13] spotless --- .../elasticsearch/index/mapper/ObjectMapperMergeTests.java | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java index 8e993740ec3e8..d405641ff0b04 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java @@ -133,14 +133,11 @@ public void testMergeNested() { } private static RootObjectMapper createRootObjectMapper(String name, boolean enabled, Map mappers) { - return (RootObjectMapper) new RootObjectMapper.Builder(name).enabled(enabled) - .addMappers(mappers) - .build(MapperBuilderContext.ROOT); + return (RootObjectMapper) new RootObjectMapper.Builder(name).enabled(enabled).addMappers(mappers).build(MapperBuilderContext.ROOT); } private static ObjectMapper createObjectMapper(String name, boolean enabled, Map mappers) { - return new ObjectMapper.Builder(name).enabled(enabled) - .addMappers(mappers).build(MapperBuilderContext.ROOT); + return new ObjectMapper.Builder(name).enabled(enabled).addMappers(mappers).build(MapperBuilderContext.ROOT); } private TextFieldMapper createTextFieldMapper(String name) { From 9f4da05ab18e2d5331dadaeeaf69d67d095f0d57 Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Mon, 20 Dec 2021 10:19:39 +0000 Subject: [PATCH 09/13] deef --- .../org/elasticsearch/index/mapper/ObjectMapper.java | 2 +- .../elasticsearch/index/mapper/RootObjectMapper.java | 10 ++-------- .../index/mapper/FieldAliasMapperValidationTests.java | 2 +- .../elasticsearch/index/mapper/MappingLookupTests.java | 2 +- .../index/mapper/ObjectMapperMergeTests.java | 2 +- .../index/query/SearchExecutionContextTests.java | 2 +- 6 files changed, 7 insertions(+), 13 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java index 8ec3d6fe82ef7..6c38650868189 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java @@ -86,7 +86,7 @@ public Builder add(Mapper.Builder builder) { return this; } - public Builder addMappers(Map mappers) { + Builder addMappers(Map mappers) { mappers.forEach((name, mapper) -> mappersBuilders.add(new Mapper.Builder(name) { @Override public Mapper build(MapperBuilderContext context) { diff --git a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java index 9dcb229b8286c..f36c336ec8303 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java @@ -92,7 +92,7 @@ public RootObjectMapper.Builder addRuntimeField(RuntimeField runtimeField) { return this; } - public RootObjectMapper.Builder setRuntime(Map runtimeFields) { + public RootObjectMapper.Builder addRuntimeFields(Map runtimeFields) { this.runtimeFields.putAll(runtimeFields); return this; } @@ -230,7 +230,7 @@ private boolean processField( parserContext, true ); - builder.setRuntime(fields); + builder.addRuntimeFields(fields); return true; } else { throw new ElasticsearchParseException("runtime must be a map type"); @@ -363,12 +363,6 @@ protected void doMerge(ObjectMapper mergeWith, MergeReason reason) { } } - void addRuntimeFields(Collection runtimeFields) { - for (RuntimeField runtimeField : runtimeFields) { - this.runtimeFields.put(runtimeField.name(), runtimeField); - } - } - @Override protected void doXContent(XContentBuilder builder, ToXContent.Params params) throws IOException { final boolean includeDefaults = params.paramAsBoolean("include_defaults", false); diff --git a/server/src/test/java/org/elasticsearch/index/mapper/FieldAliasMapperValidationTests.java b/server/src/test/java/org/elasticsearch/index/mapper/FieldAliasMapperValidationTests.java index 28efe01b44349..ed06544c2b60a 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/FieldAliasMapperValidationTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/FieldAliasMapperValidationTests.java @@ -177,7 +177,7 @@ private static MappingLookup createMappingLookup( ) { RootObjectMapper.Builder builder = new RootObjectMapper.Builder("_doc"); Map runtimeFieldTypes = runtimeFields.stream().collect(Collectors.toMap(RuntimeField::name, r -> r)); - builder.setRuntime(runtimeFieldTypes); + builder.addRuntimeFields(runtimeFieldTypes); Mapping mapping = new Mapping(builder.build(MapperBuilderContext.ROOT), new MetadataFieldMapper[0], Collections.emptyMap()); return MappingLookup.fromMappers(mapping, fieldMappers, objectMappers, fieldAliasMappers); } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/MappingLookupTests.java b/server/src/test/java/org/elasticsearch/index/mapper/MappingLookupTests.java index 14129d3e12bf5..52c2c47067d88 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/MappingLookupTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/MappingLookupTests.java @@ -41,7 +41,7 @@ private static MappingLookup createMappingLookup( ) { RootObjectMapper.Builder builder = new RootObjectMapper.Builder("_doc"); Map runtimeFieldTypes = runtimeFields.stream().collect(Collectors.toMap(RuntimeField::name, r -> r)); - builder.setRuntime(runtimeFieldTypes); + builder.addRuntimeFields(runtimeFieldTypes); Mapping mapping = new Mapping(builder.build(MapperBuilderContext.ROOT), new MetadataFieldMapper[0], Collections.emptyMap()); return MappingLookup.fromMappers(mapping, fieldMappers, objectMappers, emptyList()); } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java index d405641ff0b04..b2d2e7bfdd392 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ObjectMapperMergeTests.java @@ -105,7 +105,7 @@ public void testMergeDisabledRootMapper() { final RootObjectMapper rootObjectMapper = (RootObjectMapper) new RootObjectMapper.Builder(type).enabled(false) .build(MapperBuilderContext.ROOT); // the root is disabled, and we are not trying to re-enable it, but we do want to be able to add runtime fields - final RootObjectMapper mergeWith = new RootObjectMapper.Builder(type).setRuntime( + final RootObjectMapper mergeWith = new RootObjectMapper.Builder(type).addRuntimeFields( Collections.singletonMap("test", new TestRuntimeField("test", "long")) ).build(MapperBuilderContext.ROOT); diff --git a/server/src/test/java/org/elasticsearch/index/query/SearchExecutionContextTests.java b/server/src/test/java/org/elasticsearch/index/query/SearchExecutionContextTests.java index f123deb7f8d8a..caa083370a0f1 100644 --- a/server/src/test/java/org/elasticsearch/index/query/SearchExecutionContextTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/SearchExecutionContextTests.java @@ -328,7 +328,7 @@ private static MappingLookup createMappingLookup(List concreteF List mappers = concreteFields.stream().map(MockFieldMapper::new).collect(Collectors.toList()); RootObjectMapper.Builder builder = new RootObjectMapper.Builder("_doc"); Map runtimeFieldTypes = runtimeFields.stream().collect(Collectors.toMap(RuntimeField::name, r -> r)); - builder.setRuntime(runtimeFieldTypes); + builder.addRuntimeFields(runtimeFieldTypes); Mapping mapping = new Mapping(builder.build(MapperBuilderContext.ROOT), new MetadataFieldMapper[0], Collections.emptyMap()); return MappingLookup.fromMappers(mapping, mappers, Collections.emptyList(), Collections.emptyList()); } From 6f1f42c4bb0122c7c57732e00bb118a1d8be8132 Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Thu, 6 Jan 2022 12:06:43 +0000 Subject: [PATCH 10/13] deef --- .../index/mapper/DocumentParser.java | 38 +++++++++---------- .../index/mapper/DocumentParserContext.java | 4 +- .../index/mapper/NestedObjectMapper.java | 4 +- .../index/mapper/ObjectMapper.java | 9 +++-- .../index/mapper/RootObjectMapper.java | 2 +- 5 files changed, 28 insertions(+), 29 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java index 4f7fc2df813b2..156daaaf39029 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java @@ -215,25 +215,25 @@ private static MapperParsingException wrapInMapperParsingException(SourceToParse } private static void validatePath(String fullFieldPath) { - if (fullFieldPath.contains(".")) { - String[] parts = fullFieldPath.split("\\."); - if (parts.length == 0) { - throw new IllegalArgumentException("field name cannot contain only dots"); - } - for (String part : parts) { - if (Strings.hasText(part) == false) { - // check if the field name contains only whitespace - if (Strings.isEmpty(part) == false) { - throw new IllegalArgumentException("object field cannot contain only whitespace: ['" + fullFieldPath + "']"); - } - throw new IllegalArgumentException( - "object field starting or ending with a [.] makes object resolution ambiguous: [" + fullFieldPath + "]" - ); + if (Strings.isEmpty(fullFieldPath)) { + throw new IllegalArgumentException("field name cannot be an empty string"); + } + if (fullFieldPath.contains(".") == false) { + return; + } + String[] parts = fullFieldPath.split("\\."); + if (parts.length == 0) { + throw new IllegalArgumentException("field name cannot contain only dots"); + } + for (String part : parts) { + if (Strings.hasText(part) == false) { + // check if the field name contains only whitespace + if (Strings.isEmpty(part) == false) { + throw new IllegalArgumentException("object field cannot contain only whitespace: ['" + fullFieldPath + "']"); } - } - } else { - if (Strings.isEmpty(fullFieldPath)) { - throw new IllegalArgumentException("field name cannot be an empty string"); + throw new IllegalArgumentException( + "object field starting or ending with a [.] makes object resolution ambiguous: [" + fullFieldPath + "]" + ); } } } @@ -615,7 +615,7 @@ private static ObjectMapper.Dynamic dynamicOrDefault(ObjectMapper parentMapper, parentMapper = context.mappingLookup().objectMappers().get(parentName); if (parentMapper == null) { // If parentMapper is null, it means the parent of the current mapper is being dynamically created right now - parentMapper = context.getObjectMapper(parentName); + parentMapper = context.getDynamicObjectMapper(parentName); if (parentMapper == null) { // it can still happen that the path is ambiguous and we are not able to locate the parent break; diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java index 612ce0266a463..67247733e9e4e 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java @@ -226,7 +226,7 @@ public final boolean isShadowed(String field) { return mappingLookup.isShadowed(field); } - public final ObjectMapper getObjectMapper(String name) { + public final ObjectMapper getDynamicObjectMapper(String name) { return dynamicObjectMappers.get(name); } @@ -254,7 +254,7 @@ public final List getDynamicRuntimeFields() { * @return a RootObjectMapper.Builder to be used to construct a dynamic mapping update */ public final RootObjectMapper.Builder updateRoot() { - return mappingLookup.getMapping().getRoot().newBuilder(); + return mappingLookup.getMapping().getRoot().newBuilder(indexSettings.getIndexVersionCreated()); } public boolean isWithinCopyTo() { diff --git a/server/src/main/java/org/elasticsearch/index/mapper/NestedObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/NestedObjectMapper.java index daa1330b60ef8..87370aa704f01 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/NestedObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/NestedObjectMapper.java @@ -91,7 +91,6 @@ protected static void parseNested(String name, Map node, NestedO private Explicit includeInParent; private final String nestedTypePath; private final Query nestedTypeFilter; - private final Version indexVersionCreated; NestedObjectMapper(String name, String fullPath, Map mappers, Builder builder) { super(name, fullPath, builder.enabled, builder.dynamic, mappers); @@ -103,7 +102,6 @@ protected static void parseNested(String name, Map node, NestedO this.nestedTypeFilter = NestedPathFieldMapper.filter(builder.indexCreatedVersion, nestedTypePath); this.includeInParent = builder.includeInParent; this.includeInRoot = builder.includeInRoot; - this.indexVersionCreated = builder.indexCreatedVersion; } public Query nestedTypeFilter() { @@ -140,7 +138,7 @@ public Map getChildren() { } @Override - public ObjectMapper.Builder newBuilder() { + public ObjectMapper.Builder newBuilder(Version indexVersionCreated) { NestedObjectMapper.Builder builder = new NestedObjectMapper.Builder(simpleName(), indexVersionCreated); builder.enabled = enabled; builder.dynamic = dynamic; diff --git a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java index 012cba8eede85..9ba8aec2e6af1 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java @@ -9,6 +9,7 @@ package org.elasticsearch.index.mapper; import org.elasticsearch.ElasticsearchParseException; +import org.elasticsearch.Version; import org.elasticsearch.common.Explicit; import org.elasticsearch.common.logging.DeprecationCategory; import org.elasticsearch.common.logging.DeprecationLogger; @@ -133,12 +134,12 @@ private ObjectMapper.Builder findChild(String childName, String fullChildName, D // does the child mapper already exist? if so, use that ObjectMapper child = context.mappingLookup().objectMappers().get(fullChildName); if (child != null) { - return child.newBuilder(); + return child.newBuilder(context.indexSettings().getIndexVersionCreated()); } // has the child mapper been added as a dynamic update already? - child = context.getObjectMapper(fullChildName); + child = context.getDynamicObjectMapper(fullChildName); if (child != null) { - return child.newBuilder(); + return child.newBuilder(context.indexSettings().getIndexVersionCreated()); } // create a new child mapper return new ObjectMapper.Builder(childName); @@ -328,7 +329,7 @@ protected ObjectMapper clone() { /** * @return a Builder that will produce an empty ObjectMapper with the same configuration as this one */ - public ObjectMapper.Builder newBuilder() { + public ObjectMapper.Builder newBuilder(Version indexVersionCreated) { ObjectMapper.Builder builder = new ObjectMapper.Builder(simpleName()); builder.enabled = this.enabled; builder.dynamic = this.dynamic; diff --git a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java index 2a651ad6068c3..dc1420bb0f9c7 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java @@ -276,7 +276,7 @@ protected ObjectMapper clone() { } @Override - public RootObjectMapper.Builder newBuilder() { + public RootObjectMapper.Builder newBuilder(Version indexVersionCreated) { RootObjectMapper.Builder builder = new RootObjectMapper.Builder(name()); builder.enabled = enabled; builder.dynamic = dynamic; From b8d4d1816cd3643e9cd083c5724ecd8234b66ec2 Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Thu, 6 Jan 2022 13:25:25 +0000 Subject: [PATCH 11/13] enforce that dynamic intermediate objects have been created already; failure in DocumentParserTests --- .../main/java/org/elasticsearch/index/mapper/ObjectMapper.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java index 9ba8aec2e6af1..171d163d70ab1 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java @@ -141,8 +141,7 @@ private ObjectMapper.Builder findChild(String childName, String fullChildName, D if (child != null) { return child.newBuilder(context.indexSettings().getIndexVersionCreated()); } - // create a new child mapper - return new ObjectMapper.Builder(childName); + throw new IllegalArgumentException("Missing intermediate object " + fullChildName); } public Optional getBuilder(String name) { From efc371f60a5210e90e1b0f7f62f06aa8ceb1254a Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Mon, 10 Jan 2022 17:11:48 +0000 Subject: [PATCH 12/13] spotless --- .../xcontent/DotExpandingXContentParserTests.java | 5 ++--- .../elasticsearch/index/mapper/DocumentParserTests.java | 8 ++------ 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/libs/x-content/src/test/java/org/elasticsearch/xcontent/DotExpandingXContentParserTests.java b/libs/x-content/src/test/java/org/elasticsearch/xcontent/DotExpandingXContentParserTests.java index 6969e935a23dd..e8e29660a4e2a 100644 --- a/libs/x-content/src/test/java/org/elasticsearch/xcontent/DotExpandingXContentParserTests.java +++ b/libs/x-content/src/test/java/org/elasticsearch/xcontent/DotExpandingXContentParserTests.java @@ -56,9 +56,8 @@ public void testEmbeddedValue() throws IOException { public void testTrailingDotsAreStripped() throws IOException { assertXContentMatches(""" - {"test":{"with":{"dots":"value"}},"nodots":"value"}""", - """ - {"test.":{"with.":{"dots":"value"}},"nodots":"value"}"""); + {"test":{"with":{"dots":"value"}},"nodots":"value"}""", """ + {"test.":{"with.":{"dots":"value"}},"nodots":"value"}"""); } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java b/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java index bea688da05c49..e146a4d4be8fa 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java @@ -1785,13 +1785,9 @@ public void testDynamicFieldsStartingAndEndingWithDot() throws Exception { MapperService mapperService = createMapperService(mapping(b -> {})); Exception e = expectThrows(MapperParsingException.class, () -> mapperService.documentMapper().parse(source(""" {"top..foo.":{"a":1}} - """ - ))); + """))); - assertThat( - e.getCause().getMessage(), - containsString("object field cannot contain only whitespace: ['top..foo.']") - ); + assertThat(e.getCause().getMessage(), containsString("object field cannot contain only whitespace: ['top..foo.']")); } public void testDynamicFieldsEmptyName() throws Exception { From 025be0645f502e87aeedb359a5bfd6ad3ab0836a Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Wed, 12 Jan 2022 13:42:52 +0000 Subject: [PATCH 13/13] Add back check for empty field name --- .../java/org/elasticsearch/index/mapper/DocumentParser.java | 5 +++++ .../org/elasticsearch/index/mapper/DocumentParserTests.java | 4 ++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java index 651b36ef62cfe..5d820526ef9bc 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java @@ -281,6 +281,11 @@ private static void innerParseObject(DocumentParserContext context, ObjectMapper while (token != XContentParser.Token.END_OBJECT) { if (token == XContentParser.Token.FIELD_NAME) { currentFieldName = context.parser().currentName(); + if (currentFieldName.isBlank()) { + throw new MapperParsingException( + "Field name cannot contain only whitespace: [" + context.path().pathAsText(currentFieldName) + "]" + ); + } } else if (token == XContentParser.Token.START_OBJECT) { parseObject(context, mapper, currentFieldName); } else if (token == XContentParser.Token.START_ARRAY) { diff --git a/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java b/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java index e146a4d4be8fa..c115f84da43e4 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java @@ -1793,7 +1793,7 @@ public void testDynamicFieldsStartingAndEndingWithDot() throws Exception { public void testDynamicFieldsEmptyName() throws Exception { DocumentMapper mapper = createDocumentMapper(mapping(b -> {})); - IllegalArgumentException emptyFieldNameException = expectThrows(IllegalArgumentException.class, () -> mapper.parse(source(b -> { + Exception emptyFieldNameException = expectThrows(MapperParsingException.class, () -> mapper.parse(source(b -> { b.startArray("top"); { b.startObject(); @@ -1805,7 +1805,7 @@ public void testDynamicFieldsEmptyName() throws Exception { b.endArray(); }))); - assertThat(emptyFieldNameException.getMessage(), containsString("object field cannot contain only whitespace: ['top.aoeu. ']")); + assertThat(emptyFieldNameException.getMessage(), containsString("Field name cannot contain only whitespace: [top.aoeu. ]")); } public void testBlankFieldNames() throws Exception {