From 8a65d1c4bf7fc1e2b31400d72f980c331dd1f5b7 Mon Sep 17 00:00:00 2001 From: Christos Soulios Date: Wed, 26 Aug 2020 18:00:33 +0300 Subject: [PATCH 01/12] Configurable metadata field mappers in the _source --- .../elasticsearch/index/mapper/DocumentParser.java | 2 +- .../elasticsearch/index/mapper/MapperService.java | 12 ++++++++++++ .../index/mapper/MetadataFieldMapper.java | 8 ++++++++ .../elasticsearch/indices/mapper/MapperRegistry.java | 12 ++++++++++++ 4 files changed, 33 insertions(+), 1 deletion(-) 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 0d198e854aca6..9e2f20889053c 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java @@ -391,7 +391,7 @@ private static void innerParseObject(ParseContext context, ObjectMapper mapper, if (token == XContentParser.Token.FIELD_NAME) { currentFieldName = parser.currentName(); paths = splitAndValidatePath(currentFieldName); - if (context.mapperService().isMetadataField(context.path().pathAsText(currentFieldName))) { + if (context.mapperService().isFieldAllowedInSource(context.path().pathAsText(currentFieldName)) == false) { throw new MapperParsingException("Field [" + currentFieldName + "] is a metadata field and cannot be added inside" + " a document. Use the index API request parameters."); } else if (containsDisabledObjectMapper(mapper, paths)) { diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index 66caf9d41bdeb..ed8087464f281 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -495,6 +495,18 @@ public boolean isMetadataField(String field) { return mapperRegistry.isMetadataField(indexVersionCreated, field); } + /** + * @return Whether a field should be included in the document _source. + */ + public boolean isFieldAllowedInSource(String field) { + if(isMetadataField(field)) { + // Metadata fields may not be allowed in the document _source. + return mapperRegistry.isAllowedInSource(indexVersionCreated, field); + } else { + return true; + } + } + /** An analyzer wrapper that can lookup fields within the index mappings */ final class MapperAnalyzerWrapper extends DelegatingAnalyzerWrapper { diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MetadataFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/MetadataFieldMapper.java index 1737a9ec82831..1fe6647f0594d 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MetadataFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MetadataFieldMapper.java @@ -45,6 +45,14 @@ MetadataFieldMapper.Builder parse(String name, Map node, * @param parserContext context that may be useful to build the field like analyzers */ MetadataFieldMapper getDefault(ParserContext parserContext); + + /** + * @return Whether a metadata field can be included in the document _source. + */ + default boolean isAllowedInSource() { + // By default return false for metadata fields. + return false; + } } /** diff --git a/server/src/main/java/org/elasticsearch/indices/mapper/MapperRegistry.java b/server/src/main/java/org/elasticsearch/indices/mapper/MapperRegistry.java index e5c1132267624..81f3f9e5ba40a 100644 --- a/server/src/main/java/org/elasticsearch/indices/mapper/MapperRegistry.java +++ b/server/src/main/java/org/elasticsearch/indices/mapper/MapperRegistry.java @@ -78,6 +78,18 @@ public boolean isMetadataField(Version indexCreatedVersion, String field) { return getMetadataMapperParsers(indexCreatedVersion).containsKey(field); } + /** + * Returns true if the provided field can be included in the document _source. + */ + public boolean isAllowedInSource(Version indexCreatedVersion, String field) { + MetadataFieldMapper.TypeParser parser = getMetadataMapperParsers(indexCreatedVersion).get(field); + if (parser != null) { + return parser.isAllowedInSource(); + } + // Non metadata fields should alway be allowed in document _source. + return true; + } + /** * Returns a function that given an index name, returns a predicate that fields must match in order to be returned by get mappings, * get index, get field mappings and field capabilities API. Useful to filter the fields that such API return. From d83b67723679cb7900f468a738c7635ef5789620 Mon Sep 17 00:00:00 2001 From: Christos Soulios Date: Wed, 2 Sep 2020 14:57:51 +0300 Subject: [PATCH 02/12] Changes to support metadata fields in _source Added test testDocumentContainsAllowedMetadataField() --- .../index/mapper/DocumentParser.java | 33 +++++- .../elasticsearch/index/mapper/Mapping.java | 4 + .../indices/mapper/MapperRegistry.java | 2 +- .../index/mapper/DocumentParserTests.java | 30 ++++- .../mapper/MockMetadataMapperPlugin.java | 111 ++++++++++++++++++ 5 files changed, 173 insertions(+), 7 deletions(-) create mode 100644 server/src/test/java/org/elasticsearch/index/mapper/MockMetadataMapperPlugin.java 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 9e2f20889053c..ce90732a8b3aa 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java @@ -391,9 +391,21 @@ private static void innerParseObject(ParseContext context, ObjectMapper mapper, if (token == XContentParser.Token.FIELD_NAME) { currentFieldName = parser.currentName(); paths = splitAndValidatePath(currentFieldName); - if (context.mapperService().isFieldAllowedInSource(context.path().pathAsText(currentFieldName)) == false) { - throw new MapperParsingException("Field [" + currentFieldName + "] is a metadata field and cannot be added inside" - + " a document. Use the index API request parameters."); + + if (context.mapperService().isMetadataField(currentFieldName)) { + if (context.mapperService().isFieldAllowedInSource(currentFieldName)) { + // If token is a metadata field and is allowed in source, parse its value + token = parser.nextToken(); + if (token != null && token.isValue()) { + parseValue(context, mapper, currentFieldName, token, paths); + } else { + throw new MapperParsingException("Field [" + currentFieldName + + "] is a metadata field and must have a concrete value."); + } + } else { + throw new MapperParsingException("Field [" + currentFieldName + "] is a metadata field and cannot be added inside" + + " a document. Use the index API request parameters."); + } } else if (containsDisabledObjectMapper(mapper, paths)) { parser.nextToken(); parser.skipChildren(); @@ -596,7 +608,20 @@ private static void parseValue(final ParseContext context, ObjectMapper parentMa throw new MapperParsingException("object mapping [" + parentMapper.name() + "] trying to serialize a value with" + " no field associated with it, current value [" + context.parser().textOrNull() + "]"); } - Mapper mapper = getMapper(parentMapper, currentFieldName, paths); + + Mapper mapper = null; + if (context.mapperService().isMetadataField(currentFieldName) + && context.mapperService().isFieldAllowedInSource(currentFieldName)) { + + for (MetadataFieldMapper metadataFieldMapper : context.docMapper().mapping().getMetadataMappers()) { + if (currentFieldName.equals(metadataFieldMapper.name())) { + mapper = metadataFieldMapper; + break; + } + } + } else { + mapper = getMapper(parentMapper, currentFieldName, paths); + } if (mapper != null) { parseObjectOrField(context, mapper); } else { 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 cbce56d66fa22..69db34e328923 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java @@ -136,6 +136,10 @@ public Mapping merge(Mapping mergeWith, MergeReason reason) { return new Mapping(indexCreated, mergedRoot, mergedMetadataMappers.values().toArray(new MetadataFieldMapper[0]), mergedMeta); } + public MetadataFieldMapper[] getMetadataMappers() { + return metadataMappers; + } + @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { root.toXContent(builder, params, new ToXContent() { diff --git a/server/src/main/java/org/elasticsearch/indices/mapper/MapperRegistry.java b/server/src/main/java/org/elasticsearch/indices/mapper/MapperRegistry.java index 81f3f9e5ba40a..9101acc3d79f4 100644 --- a/server/src/main/java/org/elasticsearch/indices/mapper/MapperRegistry.java +++ b/server/src/main/java/org/elasticsearch/indices/mapper/MapperRegistry.java @@ -86,7 +86,7 @@ public boolean isAllowedInSource(Version indexCreatedVersion, String field) { if (parser != null) { return parser.isAllowedInSource(); } - // Non metadata fields should alway be allowed in document _source. + // Non-metadata fields should always be allowed in document _source. return true; } 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 b467032c88d63..3c02152a32b53 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java @@ -36,8 +36,8 @@ import org.elasticsearch.index.mapper.ParseContext.Document; import org.elasticsearch.plugins.Plugin; import org.elasticsearch.test.ESSingleNodeTestCase; -import org.elasticsearch.test.TestGeoShapeFieldMapperPlugin; import org.elasticsearch.test.InternalSettingsPlugin; +import org.elasticsearch.test.TestGeoShapeFieldMapperPlugin; import java.io.IOException; import java.math.BigDecimal; @@ -62,7 +62,7 @@ public class DocumentParserTests extends ESSingleNodeTestCase { @Override protected Collection> getPlugins() { - return pluginList(InternalSettingsPlugin.class, TestGeoShapeFieldMapperPlugin.class); + return pluginList(InternalSettingsPlugin.class, TestGeoShapeFieldMapperPlugin.class, MockMetadataMapperPlugin.class); } public void testFieldDisabled() throws Exception { @@ -1158,6 +1158,32 @@ public void testDocumentContainsMetadataField() throws Exception { mapper.parse(new SourceToParse("test", "1", bytes2, XContentType.JSON)); // parses without error } + public void testDocumentContainsAllowedMetadataField() throws Exception { + DocumentMapperParser mapperParser = createIndex("test").mapperService().documentMapperParser(); + String mapping = Strings.toString(XContentFactory.jsonBuilder().startObject().startObject("_doc").endObject().endObject()); + DocumentMapper mapper = mapperParser.parse("_doc", new CompressedXContent(mapping)); + + // A metadata field that is allowed in source must always be a value. Objects/arrays are not allowed + BytesReference metadataAsObject = BytesReference.bytes(XContentFactory.jsonBuilder() + .startObject() + .field(MockMetadataMapperPlugin.MockMetadataMapper.CONTENT_TYPE) + .startObject().field("sub-field", "true").endObject() + .endObject()); + + MapperParsingException e = expectThrows(MapperParsingException.class, () -> + mapper.parse(new SourceToParse("test", "1", metadataAsObject, XContentType.JSON))); + assertTrue(e.getMessage(), + e.getMessage().contains("Field [_mock_metadata] is a metadata field and must have a concrete value.")); + + BytesReference bytes = BytesReference.bytes(XContentFactory.jsonBuilder() + .startObject() + .field(MockMetadataMapperPlugin.MockMetadataMapper.CONTENT_TYPE, "mock-metadata-field-value") + .endObject()); + ParsedDocument doc = mapper.parse(new SourceToParse("test", "1", bytes, XContentType.JSON)); // parses without error + IndexableField field = doc.rootDoc().getField(MockMetadataMapperPlugin.MockMetadataMapper.CONTENT_TYPE); + assertEquals("mock-metadata-field-value", field.stringValue()); + } + public void testSimpleMapper() throws Exception { IndexService indexService = createIndex("test"); DocumentMapper docMapper = new DocumentMapper.Builder( diff --git a/server/src/test/java/org/elasticsearch/index/mapper/MockMetadataMapperPlugin.java b/server/src/test/java/org/elasticsearch/index/mapper/MockMetadataMapperPlugin.java new file mode 100644 index 0000000000000..7a92f9f330a2e --- /dev/null +++ b/server/src/test/java/org/elasticsearch/index/mapper/MockMetadataMapperPlugin.java @@ -0,0 +1,111 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.index.mapper; + +import org.apache.lucene.document.Field; +import org.apache.lucene.document.StringField; +import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.plugins.MapperPlugin; +import org.elasticsearch.plugins.Plugin; + +import java.io.IOException; +import java.util.Collections; +import java.util.Iterator; +import java.util.List; +import java.util.Map; + +/** + * Mapper plugin providing a mock metadata field mapper implementation that supports setting its value + * through the document source. + */ +public class MockMetadataMapperPlugin extends Plugin implements MapperPlugin { + + /** + * A mock metadata field mapper that supports being set from the document source. + */ + public static class MockMetadataMapper extends MetadataFieldMapper { + + static final String CONTENT_TYPE = "_mock_metadata"; + static final String FIELD_NAME = "_mock_metadata"; + + protected MockMetadataMapper() { + super(new KeywordFieldMapper.KeywordFieldType(FIELD_NAME)); + } + + @Override + protected void parseCreateField(ParseContext context) throws IOException { + if (context.parser().currentToken() == XContentParser.Token.VALUE_STRING) { + context.doc().add(new StringField(FIELD_NAME, context.parser().text(), Field.Store.YES)); + } else { + return; + } + } + + @Override + public Iterator iterator() { + return Collections.emptyIterator(); + } + + @Override + protected String contentType() { + return CONTENT_TYPE; + } + + @Override + public void preParse(ParseContext context) throws IOException { + } + + @Override + public void postParse(ParseContext context) throws IOException { + } + + public static class Builder extends MetadataFieldMapper.Builder { + + protected Builder() { + super(FIELD_NAME); + } + + @Override + protected List> getParameters() { + return Collections.emptyList(); + } + + @Override + public MockMetadataMapper build(BuilderContext context) { + return new MockMetadataMapper(); + } + } + + public static final TypeParser PARSER = new ConfigurableTypeParser( + c -> new MockMetadataMapper(), + c -> new MockMetadataMapper.Builder()) { + + @Override + public boolean isAllowedInSource() { + return true; + } + }; + } + + @Override + public Map getMetadataMappers() { + return Collections.singletonMap(MockMetadataMapper.CONTENT_TYPE, MockMetadataMapper.PARSER); + } +} From 4934129da49025b3d3d314efe8d65d922053c9d1 Mon Sep 17 00:00:00 2001 From: Christos Soulios Date: Wed, 9 Sep 2020 14:49:59 +0300 Subject: [PATCH 03/12] Merged DocumentParserTests from master Fixed broken tests --- .../index/mapper/DocumentParser.java | 4 +-- .../index/mapper/DocumentParserTests.java | 32 +++++++++---------- 2 files changed, 17 insertions(+), 19 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 ce90732a8b3aa..89f608f375af5 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java @@ -392,8 +392,8 @@ private static void innerParseObject(ParseContext context, ObjectMapper mapper, currentFieldName = parser.currentName(); paths = splitAndValidatePath(currentFieldName); - if (context.mapperService().isMetadataField(currentFieldName)) { - if (context.mapperService().isFieldAllowedInSource(currentFieldName)) { + if (context.mapperService().isMetadataField(context.path().pathAsText(currentFieldName))) { + if (context.mapperService().isFieldAllowedInSource(context.path().pathAsText(currentFieldName))) { // If token is a metadata field and is allowed in source, parse its value token = parser.nextToken(); if (token != null && token.isValue()) { 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 a5ffd0d130b57..3db01d90948e6 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java @@ -31,15 +31,18 @@ import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.index.IndexSettings; import org.elasticsearch.index.mapper.ParseContext.Document; +import org.elasticsearch.plugins.Plugin; 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; +import static java.util.Collections.singletonList; import static org.elasticsearch.test.StreamsUtils.copyToBytesFromClasspath; import static org.elasticsearch.test.StreamsUtils.copyToStringFromClasspath; import static org.hamcrest.Matchers.containsString; @@ -50,6 +53,11 @@ public class DocumentParserTests extends MapperServiceTestCase { + @Override + protected Collection getPlugins() { + return singletonList(new MockMetadataMapperPlugin()); + } + public void testFieldDisabled() throws Exception { DocumentMapper mapper = createDocumentMapper(mapping(b -> { b.startObject("foo").field("enabled", false).endObject(); @@ -938,27 +946,17 @@ public void testDocumentContainsMetadataField() throws Exception { } public void testDocumentContainsAllowedMetadataField() throws Exception { - DocumentMapperParser mapperParser = createIndex("test").mapperService().documentMapperParser(); - String mapping = Strings.toString(XContentFactory.jsonBuilder().startObject().startObject("_doc").endObject().endObject()); - DocumentMapper mapper = mapperParser.parse("_doc", new CompressedXContent(mapping)); + DocumentMapper mapper = createDocumentMapper(mapping(b -> {})); // A metadata field that is allowed in source must always be a value. Objects/arrays are not allowed - BytesReference metadataAsObject = BytesReference.bytes(XContentFactory.jsonBuilder() - .startObject() - .field(MockMetadataMapperPlugin.MockMetadataMapper.CONTENT_TYPE) - .startObject().field("sub-field", "true").endObject() - .endObject()); - MapperParsingException e = expectThrows(MapperParsingException.class, () -> - mapper.parse(new SourceToParse("test", "1", metadataAsObject, XContentType.JSON))); - assertTrue(e.getMessage(), - e.getMessage().contains("Field [_mock_metadata] is a metadata field and must have a concrete value.")); + mapper.parse(source(b -> b.field(MockMetadataMapperPlugin.MockMetadataMapper.CONTENT_TYPE) + .startObject().field("sub-field", "true").endObject()))); + assertTrue(e.getMessage(), e.getMessage().contains("Field [_mock_metadata] is a metadata field and must have a concrete value.")); - BytesReference bytes = BytesReference.bytes(XContentFactory.jsonBuilder() - .startObject() - .field(MockMetadataMapperPlugin.MockMetadataMapper.CONTENT_TYPE, "mock-metadata-field-value") - .endObject()); - ParsedDocument doc = mapper.parse(new SourceToParse("test", "1", bytes, XContentType.JSON)); // parses without error + ParsedDocument doc = mapper.parse(source(b -> + b.field(MockMetadataMapperPlugin.MockMetadataMapper.CONTENT_TYPE, "mock-metadata-field-value") + )); IndexableField field = doc.rootDoc().getField(MockMetadataMapperPlugin.MockMetadataMapper.CONTENT_TYPE); assertEquals("mock-metadata-field-value", field.stringValue()); } From 2effa2f46e16d7bd1a8f63113d71c39a1b023c2f Mon Sep 17 00:00:00 2001 From: Christos Soulios Date: Thu, 10 Sep 2020 19:11:03 +0300 Subject: [PATCH 04/12] Handle non string values --- .../elasticsearch/index/mapper/MockMetadataMapperPlugin.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/test/java/org/elasticsearch/index/mapper/MockMetadataMapperPlugin.java b/server/src/test/java/org/elasticsearch/index/mapper/MockMetadataMapperPlugin.java index 7a92f9f330a2e..8180a004ca2a3 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/MockMetadataMapperPlugin.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/MockMetadataMapperPlugin.java @@ -54,7 +54,7 @@ protected void parseCreateField(ParseContext context) throws IOException { if (context.parser().currentToken() == XContentParser.Token.VALUE_STRING) { context.doc().add(new StringField(FIELD_NAME, context.parser().text(), Field.Store.YES)); } else { - return; + throw new IllegalArgumentException("Field [" + fieldType().name() + "] must be a string."); } } From 1f5aa63dc67bb78a7c34b79df34b6764b954f04d Mon Sep 17 00:00:00 2001 From: Christos Soulios Date: Wed, 16 Sep 2020 15:00:57 +0300 Subject: [PATCH 05/12] Allow metadata fields to parse values/objects/arrays/null --- .../index/mapper/DocumentParser.java | 41 +++++++------------ .../index/mapper/DocumentParserTests.java | 9 +++- 2 files changed, 21 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 89f608f375af5..4941cd8ca9601 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java @@ -393,16 +393,7 @@ private static void innerParseObject(ParseContext context, ObjectMapper mapper, paths = splitAndValidatePath(currentFieldName); if (context.mapperService().isMetadataField(context.path().pathAsText(currentFieldName))) { - if (context.mapperService().isFieldAllowedInSource(context.path().pathAsText(currentFieldName))) { - // If token is a metadata field and is allowed in source, parse its value - token = parser.nextToken(); - if (token != null && token.isValue()) { - parseValue(context, mapper, currentFieldName, token, paths); - } else { - throw new MapperParsingException("Field [" + currentFieldName - + "] is a metadata field and must have a concrete value."); - } - } else { + if (context.mapperService().isFieldAllowedInSource(context.path().pathAsText(currentFieldName)) == false) { throw new MapperParsingException("Field [" + currentFieldName + "] is a metadata field and cannot be added inside" + " a document. Use the index API request parameters."); } @@ -494,7 +485,7 @@ private static void parseObject(final ParseContext context, ObjectMapper mapper, String[] paths) throws IOException { assert currentFieldName != null; - Mapper objectMapper = getMapper(mapper, currentFieldName, paths); + Mapper objectMapper = getMapper(context, mapper, currentFieldName, paths); if (objectMapper != null) { context.path().add(currentFieldName); parseObjectOrField(context, objectMapper); @@ -531,7 +522,7 @@ private static void parseArray(ParseContext context, ObjectMapper parentMapper, String[] paths) throws IOException { String arrayFieldName = lastFieldName; - Mapper mapper = getMapper(parentMapper, lastFieldName, paths); + Mapper mapper = getMapper(context, parentMapper, lastFieldName, paths); if (mapper != null) { // There is a concrete mapper for this field already. Need to check if the mapper // expects an array, if so we pass the context straight to the mapper and if not @@ -609,19 +600,7 @@ private static void parseValue(final ParseContext context, ObjectMapper parentMa + " no field associated with it, current value [" + context.parser().textOrNull() + "]"); } - Mapper mapper = null; - if (context.mapperService().isMetadataField(currentFieldName) - && context.mapperService().isFieldAllowedInSource(currentFieldName)) { - - for (MetadataFieldMapper metadataFieldMapper : context.docMapper().mapping().getMetadataMappers()) { - if (currentFieldName.equals(metadataFieldMapper.name())) { - mapper = metadataFieldMapper; - break; - } - } - } else { - mapper = getMapper(parentMapper, currentFieldName, paths); - } + Mapper mapper = getMapper(context, parentMapper, currentFieldName, paths); if (mapper != null) { parseObjectOrField(context, mapper); } else { @@ -638,7 +617,7 @@ private static void parseValue(final ParseContext context, ObjectMapper parentMa private static void parseNullValue(ParseContext context, ObjectMapper parentMapper, String lastFieldName, String[] paths) throws IOException { // we can only handle null values if we have mappings for them - Mapper mapper = getMapper(parentMapper, lastFieldName, paths); + Mapper mapper = getMapper(context, parentMapper, lastFieldName, paths); if (mapper != null) { // TODO: passing null to an object seems bogus? parseObjectOrField(context, mapper); @@ -906,7 +885,15 @@ private static ObjectMapper.Dynamic dynamicOrDefault(ObjectMapper parentMapper, } // looks up a child mapper, but takes into account field names that expand to objects - private static Mapper getMapper(ObjectMapper objectMapper, String fieldName, String[] subfields) { + private static Mapper getMapper(final ParseContext context, ObjectMapper objectMapper, String fieldName, String[] subfields) { + if (context.mapperService().isMetadataField(fieldName)) { + for (MetadataFieldMapper metadataFieldMapper : context.docMapper().mapping().getMetadataMappers()) { + if (fieldName.equals(metadataFieldMapper.name())) { + return metadataFieldMapper; + } + } + } + for (int i = 0; i < subfields.length - 1; ++i) { Mapper mapper = objectMapper.getMapper(subfields[i]); if (mapper == null || (mapper instanceof ObjectMapper) == false) { 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 3db01d90948e6..af43b7a7a0ac6 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java @@ -948,11 +948,16 @@ public void testDocumentContainsMetadataField() throws Exception { public void testDocumentContainsAllowedMetadataField() throws Exception { DocumentMapper mapper = createDocumentMapper(mapping(b -> {})); - // A metadata field that is allowed in source must always be a value. Objects/arrays are not allowed + // A metadata field that parses a value fails to parse a null value + MapperParsingException e2 = expectThrows(MapperParsingException.class, () -> + mapper.parse(source(b -> b.nullField(MockMetadataMapperPlugin.MockMetadataMapper.CONTENT_TYPE)))); + assertTrue(e2.getMessage(), e2.getMessage().contains("failed to parse field [_mock_metadata]")); + + // A metadata field that parses a value fails to parse an object MapperParsingException e = expectThrows(MapperParsingException.class, () -> mapper.parse(source(b -> b.field(MockMetadataMapperPlugin.MockMetadataMapper.CONTENT_TYPE) .startObject().field("sub-field", "true").endObject()))); - assertTrue(e.getMessage(), e.getMessage().contains("Field [_mock_metadata] is a metadata field and must have a concrete value.")); + assertTrue(e.getMessage(), e.getMessage().contains("failed to parse field [_mock_metadata]")); ParsedDocument doc = mapper.parse(source(b -> b.field(MockMetadataMapperPlugin.MockMetadataMapper.CONTENT_TYPE, "mock-metadata-field-value") From 78faca38b4b4e8eb0a40af4d6fc5d60343da36e3 Mon Sep 17 00:00:00 2001 From: Christos Soulios Date: Wed, 16 Sep 2020 19:55:17 +0300 Subject: [PATCH 06/12] Removed MetadataFieldMapper.isAllowedInSource() method Delegated this functionality to MetadataFieldMapper.parse() --- .../mapper/RankFeatureMetaFieldMapper.java | 6 --- .../index/mapper/size/SizeFieldMapper.java | 11 +----- .../index/mapper/DocumentParser.java | 8 +--- .../index/mapper/FieldNamesFieldMapper.java | 14 ------- .../index/mapper/IdFieldMapper.java | 2 +- .../index/mapper/IgnoredFieldMapper.java | 11 +----- .../index/mapper/IndexFieldMapper.java | 7 ---- .../index/mapper/MapperService.java | 12 ------ .../index/mapper/MetadataFieldMapper.java | 36 +++++++++++++----- .../index/mapper/NestedPathFieldMapper.java | 12 +----- .../index/mapper/RoutingFieldMapper.java | 9 +---- .../index/mapper/SeqNoFieldMapper.java | 7 +--- .../index/mapper/SourceFieldMapper.java | 7 +--- .../index/mapper/TypeFieldMapper.java | 12 +----- .../index/mapper/VersionFieldMapper.java | 7 +--- .../indices/mapper/MapperRegistry.java | 12 ------ .../MetadataIndexTemplateServiceTests.java | 11 ------ .../index/mapper/DocumentParserTests.java | 37 ++++++++++--------- .../index/mapper/ExternalMetadataMapper.java | 9 ----- .../mapper/MockMetadataMapperPlugin.java | 18 +++------ .../DataStreamTimestampFieldMapper.java | 3 -- 21 files changed, 61 insertions(+), 190 deletions(-) diff --git a/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/RankFeatureMetaFieldMapper.java b/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/RankFeatureMetaFieldMapper.java index 347d018cb72c9..13658570de362 100644 --- a/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/RankFeatureMetaFieldMapper.java +++ b/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/RankFeatureMetaFieldMapper.java @@ -65,17 +65,11 @@ private RankFeatureMetaFieldMapper() { super(RankFeatureMetaFieldType.INSTANCE); } - @Override - public void preParse(ParseContext context) {} - @Override protected void parseCreateField(ParseContext context) { throw new AssertionError("Should never be called"); } - @Override - public void postParse(ParseContext context) {} - @Override protected String contentType() { return CONTENT_TYPE; diff --git a/plugins/mapper-size/src/main/java/org/elasticsearch/index/mapper/size/SizeFieldMapper.java b/plugins/mapper-size/src/main/java/org/elasticsearch/index/mapper/size/SizeFieldMapper.java index 9a5bd5591c757..39661d826ffb0 100644 --- a/plugins/mapper-size/src/main/java/org/elasticsearch/index/mapper/size/SizeFieldMapper.java +++ b/plugins/mapper-size/src/main/java/org/elasticsearch/index/mapper/size/SizeFieldMapper.java @@ -79,19 +79,10 @@ public boolean enabled() { return this.enabled.value(); } - @Override - public void preParse(ParseContext context) { - } - @Override public void postParse(ParseContext context) throws IOException { // we post parse it so we get the size stored, possibly compressed (source will be preParse) - super.parse(context); - } - - @Override - public void parse(ParseContext context) { - // nothing to do here, we call the parent in postParse + doParse(context); } @Override 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 4941cd8ca9601..47cb9ae70f90f 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java @@ -391,13 +391,7 @@ private static void innerParseObject(ParseContext context, ObjectMapper mapper, if (token == XContentParser.Token.FIELD_NAME) { currentFieldName = parser.currentName(); paths = splitAndValidatePath(currentFieldName); - - if (context.mapperService().isMetadataField(context.path().pathAsText(currentFieldName))) { - if (context.mapperService().isFieldAllowedInSource(context.path().pathAsText(currentFieldName)) == false) { - throw new MapperParsingException("Field [" + currentFieldName + "] is a metadata field and cannot be added inside" - + " a document. Use the index API request parameters."); - } - } else if (containsDisabledObjectMapper(mapper, paths)) { + if (containsDisabledObjectMapper(mapper, paths)) { parser.nextToken(); parser.skipChildren(); } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/FieldNamesFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/FieldNamesFieldMapper.java index 92380951b3fc8..0e99b1602da58 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/FieldNamesFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/FieldNamesFieldMapper.java @@ -27,7 +27,6 @@ import org.elasticsearch.common.logging.DeprecationLogger; import org.elasticsearch.index.query.QueryShardContext; -import java.io.IOException; import java.util.Collections; import java.util.Iterator; import java.util.List; @@ -160,19 +159,6 @@ public FieldNamesFieldType fieldType() { return (FieldNamesFieldType) super.fieldType(); } - @Override - public void preParse(ParseContext context) { - } - - @Override - public void postParse(ParseContext context) { - } - - @Override - protected void parseCreateField(ParseContext context) throws IOException { - // Adding values to the _field_names field is handled by the mappers for each field type - } - static Iterable extractFieldNames(final String fullPath) { return new Iterable() { @Override diff --git a/server/src/main/java/org/elasticsearch/index/mapper/IdFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/IdFieldMapper.java index 3c08d3cdb4ead..bf40c77437847 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/IdFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/IdFieldMapper.java @@ -257,7 +257,7 @@ private IdFieldMapper() { @Override public void preParse(ParseContext context) throws IOException { - super.parse(context); + doParse(context); } @Override diff --git a/server/src/main/java/org/elasticsearch/index/mapper/IgnoredFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/IgnoredFieldMapper.java index 766d80fecbada..6034cb2fbef02 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/IgnoredFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/IgnoredFieldMapper.java @@ -82,18 +82,9 @@ private IgnoredFieldMapper() { super(IgnoredFieldType.INSTANCE); } - @Override - public void preParse(ParseContext context) throws IOException { - } - @Override public void postParse(ParseContext context) throws IOException { - super.parse(context); - } - - @Override - public void parse(ParseContext context) throws IOException { - // done in post-parse + doParse(context); } @Override diff --git a/server/src/main/java/org/elasticsearch/index/mapper/IndexFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/IndexFieldMapper.java index 20cb3c8153eab..dcfa7e7e1c27c 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/IndexFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/IndexFieldMapper.java @@ -27,7 +27,6 @@ import org.elasticsearch.search.aggregations.support.CoreValuesSourceType; import org.elasticsearch.search.lookup.SearchLookup; -import java.io.IOException; import java.util.Collections; import java.util.function.Supplier; @@ -73,12 +72,6 @@ public IndexFieldMapper() { super(IndexFieldType.INSTANCE); } - @Override - public void preParse(ParseContext context) throws IOException {} - - @Override - protected void parseCreateField(ParseContext context) throws IOException {} - @Override protected String contentType() { return CONTENT_TYPE; diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index 5968a93d6753e..750b217059027 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -497,18 +497,6 @@ public boolean isMetadataField(String field) { return mapperRegistry.isMetadataField(indexVersionCreated, field); } - /** - * @return Whether a field should be included in the document _source. - */ - public boolean isFieldAllowedInSource(String field) { - if(isMetadataField(field)) { - // Metadata fields may not be allowed in the document _source. - return mapperRegistry.isAllowedInSource(indexVersionCreated, field); - } else { - return true; - } - } - /** An analyzer wrapper that can lookup fields within the index mappings */ final class MapperAnalyzerWrapper extends DelegatingAnalyzerWrapper { diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MetadataFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/MetadataFieldMapper.java index 1fe6647f0594d..3c1aa1399e5c2 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MetadataFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MetadataFieldMapper.java @@ -45,14 +45,6 @@ MetadataFieldMapper.Builder parse(String name, Map node, * @param parserContext context that may be useful to build the field like analyzers */ MetadataFieldMapper getDefault(ParserContext parserContext); - - /** - * @return Whether a metadata field can be included in the document _source. - */ - default boolean isAllowedInSource() { - // By default return false for metadata fields. - return false; - } } /** @@ -160,10 +152,35 @@ public final XContentBuilder toXContent(XContentBuilder builder, Params params) return builder.endObject(); } + /* + * By default metadata fields cannot be set through the document source and parse() method + * throws an exception. To enable a metadata field to parse the document source, this + * method must be overridden and doParse() should be called. + */ + @Override + public void parse(ParseContext context) throws IOException { + throw new MapperParsingException("Field [" + name() + "] is a metadata field and cannot be added inside" + + " a document. Use the index API request parameters."); + } + + /** + * Do the actual parse of the field by calling {@link FieldMapper#parse(ParseContext)} + */ + protected void doParse(ParseContext context) throws IOException { + super.parse(context); + } + + @Override + protected void parseCreateField(ParseContext context) throws IOException { + // do nothing + } + /** * Called before {@link FieldMapper#parse(ParseContext)} on the {@link RootObjectMapper}. */ - public abstract void preParse(ParseContext context) throws IOException; + public void preParse(ParseContext context) throws IOException { + // do nothing + } /** * Called after {@link FieldMapper#parse(ParseContext)} on the {@link RootObjectMapper}. @@ -176,5 +193,4 @@ public void postParse(ParseContext context) throws IOException { public ValueFetcher valueFetcher(MapperService mapperService, String format) { throw new UnsupportedOperationException("Cannot fetch values for internal field [" + name() + "]."); } - } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/NestedPathFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/NestedPathFieldMapper.java index ee79aae39d8f5..35fae16c6b847 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/NestedPathFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/NestedPathFieldMapper.java @@ -95,16 +95,7 @@ private NestedPathFieldMapper(Settings settings) { @Override public void preParse(ParseContext context) throws IOException { - super.parse(context); - } - - @Override - public void parse(ParseContext context) throws IOException { - // we parse in pre parse - } - - @Override - protected void parseCreateField(ParseContext context) throws IOException { + doParse(context); } @Override @@ -112,5 +103,4 @@ protected String contentType() { return NAME; } - } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/RoutingFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/RoutingFieldMapper.java index 82f7ee1fff57f..b44a33c46b0cc 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RoutingFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RoutingFieldMapper.java @@ -117,14 +117,7 @@ public boolean required() { @Override public void preParse(ParseContext context) throws IOException { - super.parse(context); - } - - @Override - public void parse(ParseContext context) throws IOException { - // no need ot parse here, we either get the routing in the sourceToParse - // or we don't have routing, if we get it in sourceToParse, we process it in preParse - // which will always be called + doParse(context); } @Override diff --git a/server/src/main/java/org/elasticsearch/index/mapper/SeqNoFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/SeqNoFieldMapper.java index a1137879c78c6..489c4945ec76c 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/SeqNoFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/SeqNoFieldMapper.java @@ -182,7 +182,7 @@ public SeqNoFieldMapper() { @Override public void preParse(ParseContext context) throws IOException { - super.parse(context); + doParse(context); } @Override @@ -196,11 +196,6 @@ protected void parseCreateField(ParseContext context) throws IOException { context.doc().add(seqID.primaryTerm); } - @Override - public void parse(ParseContext context) throws IOException { - // fields are added in parseCreateField - } - @Override public void postParse(ParseContext context) throws IOException { // In the case of nested docs, let's fill nested docs with the original diff --git a/server/src/main/java/org/elasticsearch/index/mapper/SourceFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/SourceFieldMapper.java index 8e6e20c6cfad0..89cec0aa8e873 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/SourceFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/SourceFieldMapper.java @@ -156,12 +156,7 @@ public boolean isComplete() { @Override public void preParse(ParseContext context) throws IOException { - super.parse(context); - } - - @Override - public void parse(ParseContext context) throws IOException { - // nothing to do here, we will call it in pre parse + doParse(context); } @Override diff --git a/server/src/main/java/org/elasticsearch/index/mapper/TypeFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/TypeFieldMapper.java index 6ee984fa5c2da..2c92d70dd7079 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/TypeFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/TypeFieldMapper.java @@ -170,17 +170,7 @@ private TypeFieldMapper() { @Override public void preParse(ParseContext context) throws IOException { - super.parse(context); - } - - @Override - public void parse(ParseContext context) throws IOException { - // we parse in pre parse - } - - @Override - protected void parseCreateField(ParseContext context) throws IOException { - + doParse(context); } @Override diff --git a/server/src/main/java/org/elasticsearch/index/mapper/VersionFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/VersionFieldMapper.java index 43242bc2b7a1c..4181f720540a6 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/VersionFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/VersionFieldMapper.java @@ -68,7 +68,7 @@ private VersionFieldMapper() { @Override public void preParse(ParseContext context) throws IOException { - super.parse(context); + doParse(context); } @Override @@ -79,11 +79,6 @@ protected void parseCreateField(ParseContext context) throws IOException { context.doc().add(version); } - @Override - public void parse(ParseContext context) throws IOException { - // _version added in preparse - } - @Override public void postParse(ParseContext context) throws IOException { // In the case of nested docs, let's fill nested docs with version=1 so that Lucene doesn't write a Bitset for documents diff --git a/server/src/main/java/org/elasticsearch/indices/mapper/MapperRegistry.java b/server/src/main/java/org/elasticsearch/indices/mapper/MapperRegistry.java index 9101acc3d79f4..e5c1132267624 100644 --- a/server/src/main/java/org/elasticsearch/indices/mapper/MapperRegistry.java +++ b/server/src/main/java/org/elasticsearch/indices/mapper/MapperRegistry.java @@ -78,18 +78,6 @@ public boolean isMetadataField(Version indexCreatedVersion, String field) { return getMetadataMapperParsers(indexCreatedVersion).containsKey(field); } - /** - * Returns true if the provided field can be included in the document _source. - */ - public boolean isAllowedInSource(Version indexCreatedVersion, String field) { - MetadataFieldMapper.TypeParser parser = getMetadataMapperParsers(indexCreatedVersion).get(field); - if (parser != null) { - return parser.isAllowedInSource(); - } - // Non-metadata fields should always be allowed in document _source. - return true; - } - /** * Returns a function that given an index name, returns a predicate that fields must match in order to be returned by get mappings, * get index, get field mappings and field capabilities API. Useful to filter the fields that such API return. diff --git a/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateServiceTests.java b/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateServiceTests.java index eeec74bc3a583..dc8c995194924 100644 --- a/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateServiceTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateServiceTests.java @@ -47,7 +47,6 @@ import org.elasticsearch.index.mapper.MapperService; import org.elasticsearch.index.mapper.MetadataFieldMapper; import org.elasticsearch.index.mapper.ParametrizedFieldMapper; -import org.elasticsearch.index.mapper.ParseContext; import org.elasticsearch.index.mapper.TextSearchInfo; import org.elasticsearch.index.query.QueryShardContext; import org.elasticsearch.indices.IndexTemplateMissingException; @@ -1554,16 +1553,6 @@ public ParametrizedFieldMapper.Builder getMergeBuilder() { return new MetadataTimestampFieldBuilder().init(this); } - @Override - public void preParse(ParseContext context) { - - } - - @Override - protected void parseCreateField(ParseContext context) { - - } - @Override protected String contentType() { return "_data_stream_timestamp"; 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 af43b7a7a0ac6..1379cec9e0cb8 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java @@ -947,23 +947,26 @@ public void testDocumentContainsMetadataField() throws Exception { public void testDocumentContainsAllowedMetadataField() throws Exception { DocumentMapper mapper = createDocumentMapper(mapping(b -> {})); - - // A metadata field that parses a value fails to parse a null value - MapperParsingException e2 = expectThrows(MapperParsingException.class, () -> - mapper.parse(source(b -> b.nullField(MockMetadataMapperPlugin.MockMetadataMapper.CONTENT_TYPE)))); - assertTrue(e2.getMessage(), e2.getMessage().contains("failed to parse field [_mock_metadata]")); - - // A metadata field that parses a value fails to parse an object - MapperParsingException e = expectThrows(MapperParsingException.class, () -> - mapper.parse(source(b -> b.field(MockMetadataMapperPlugin.MockMetadataMapper.CONTENT_TYPE) - .startObject().field("sub-field", "true").endObject()))); - assertTrue(e.getMessage(), e.getMessage().contains("failed to parse field [_mock_metadata]")); - - ParsedDocument doc = mapper.parse(source(b -> - b.field(MockMetadataMapperPlugin.MockMetadataMapper.CONTENT_TYPE, "mock-metadata-field-value") - )); - IndexableField field = doc.rootDoc().getField(MockMetadataMapperPlugin.MockMetadataMapper.CONTENT_TYPE); - assertEquals("mock-metadata-field-value", field.stringValue()); + { + // A metadata field that parses a value fails to parse a null value + MapperParsingException e = expectThrows(MapperParsingException.class, () -> + mapper.parse(source(b -> b.nullField(MockMetadataMapperPlugin.MockMetadataMapper.CONTENT_TYPE)))); + assertTrue(e.getMessage(), e.getMessage().contains("failed to parse field [_mock_metadata]")); + } + { + // A metadata field that parses a value fails to parse an object + MapperParsingException e = expectThrows(MapperParsingException.class, () -> + mapper.parse(source(b -> b.field(MockMetadataMapperPlugin.MockMetadataMapper.CONTENT_TYPE) + .startObject().field("sub-field", "true").endObject()))); + assertTrue(e.getMessage(), e.getMessage().contains("failed to parse field [_mock_metadata]")); + } + { + ParsedDocument doc = mapper.parse(source(b -> + b.field(MockMetadataMapperPlugin.MockMetadataMapper.CONTENT_TYPE, "mock-metadata-field-value") + )); + IndexableField field = doc.rootDoc().getField(MockMetadataMapperPlugin.MockMetadataMapper.CONTENT_TYPE); + assertEquals("mock-metadata-field-value", field.stringValue()); + } } public void testSimpleMapper() throws Exception { diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ExternalMetadataMapper.java b/server/src/test/java/org/elasticsearch/index/mapper/ExternalMetadataMapper.java index 62507f9b38c0e..32919b16179a6 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ExternalMetadataMapper.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ExternalMetadataMapper.java @@ -37,11 +37,6 @@ protected ExternalMetadataMapper() { super(new BooleanFieldMapper.BooleanFieldType(FIELD_NAME)); } - @Override - protected void parseCreateField(ParseContext context) throws IOException { - // handled in post parse - } - @Override public Iterator iterator() { return Collections.emptyIterator(); @@ -52,10 +47,6 @@ protected String contentType() { return CONTENT_TYPE; } - @Override - public void preParse(ParseContext context) throws IOException { - } - @Override public void postParse(ParseContext context) throws IOException { context.doc().add(new StringField(FIELD_NAME, FIELD_VALUE, Store.YES)); diff --git a/server/src/test/java/org/elasticsearch/index/mapper/MockMetadataMapperPlugin.java b/server/src/test/java/org/elasticsearch/index/mapper/MockMetadataMapperPlugin.java index 8180a004ca2a3..e83e794cf2fe3 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/MockMetadataMapperPlugin.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/MockMetadataMapperPlugin.java @@ -49,6 +49,11 @@ protected MockMetadataMapper() { super(new KeywordFieldMapper.KeywordFieldType(FIELD_NAME)); } + @Override + public void parse(ParseContext context) throws IOException { + doParse(context); + } + @Override protected void parseCreateField(ParseContext context) throws IOException { if (context.parser().currentToken() == XContentParser.Token.VALUE_STRING) { @@ -68,14 +73,6 @@ protected String contentType() { return CONTENT_TYPE; } - @Override - public void preParse(ParseContext context) throws IOException { - } - - @Override - public void postParse(ParseContext context) throws IOException { - } - public static class Builder extends MetadataFieldMapper.Builder { protected Builder() { @@ -96,11 +93,6 @@ public MockMetadataMapper build(BuilderContext context) { public static final TypeParser PARSER = new ConfigurableTypeParser( c -> new MockMetadataMapper(), c -> new MockMetadataMapper.Builder()) { - - @Override - public boolean isAllowedInSource() { - return true; - } }; } diff --git a/x-pack/plugin/data-streams/src/main/java/org/elasticsearch/xpack/datastreams/mapper/DataStreamTimestampFieldMapper.java b/x-pack/plugin/data-streams/src/main/java/org/elasticsearch/xpack/datastreams/mapper/DataStreamTimestampFieldMapper.java index 1dd2bc1f97c3d..298fb4a8dc8cf 100644 --- a/x-pack/plugin/data-streams/src/main/java/org/elasticsearch/xpack/datastreams/mapper/DataStreamTimestampFieldMapper.java +++ b/x-pack/plugin/data-streams/src/main/java/org/elasticsearch/xpack/datastreams/mapper/DataStreamTimestampFieldMapper.java @@ -171,9 +171,6 @@ public void doValidate(MappingLookup lookup) { } } - @Override - public void preParse(ParseContext context) throws IOException {} - @Override protected void parseCreateField(ParseContext context) throws IOException { // Meta field doesn't create any fields, so this shouldn't happen. From 39d19fe38d438bf5799dde3c5b1ff931e2fc1a1b Mon Sep 17 00:00:00 2001 From: Christos Soulios Date: Wed, 16 Sep 2020 22:57:42 +0300 Subject: [PATCH 07/12] Fixed bug that caused tests to break --- .../java/org/elasticsearch/index/mapper/DocumentParser.java | 5 +++-- 1 file changed, 3 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 47cb9ae70f90f..850ec1f648d7d 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java @@ -880,9 +880,10 @@ private static ObjectMapper.Dynamic dynamicOrDefault(ObjectMapper parentMapper, // looks up a child mapper, but takes into account field names that expand to objects private static Mapper getMapper(final ParseContext context, ObjectMapper objectMapper, String fieldName, String[] subfields) { - if (context.mapperService().isMetadataField(fieldName)) { + String fieldPath = context.path().pathAsText(fieldName); + if (context.mapperService().isMetadataField(fieldPath)) { for (MetadataFieldMapper metadataFieldMapper : context.docMapper().mapping().getMetadataMappers()) { - if (fieldName.equals(metadataFieldMapper.name())) { + if (fieldPath.equals(metadataFieldMapper.name())) { return metadataFieldMapper; } } From f0bb9578d911b0ad4c80a755e9983904082eaa74 Mon Sep 17 00:00:00 2001 From: Christos Soulios Date: Thu, 17 Sep 2020 18:41:05 +0300 Subject: [PATCH 08/12] Cleanup parsing for existing metadata fields --- .../elasticsearch/index/mapper/size/SizeFieldMapper.java | 5 ----- .../java/org/elasticsearch/index/mapper/IdFieldMapper.java | 5 ----- .../org/elasticsearch/index/mapper/IgnoredFieldMapper.java | 5 ----- .../org/elasticsearch/index/mapper/MetadataFieldMapper.java | 2 +- .../elasticsearch/index/mapper/NestedPathFieldMapper.java | 6 ------ .../org/elasticsearch/index/mapper/RoutingFieldMapper.java | 5 ----- .../org/elasticsearch/index/mapper/SeqNoFieldMapper.java | 5 ----- .../org/elasticsearch/index/mapper/TypeFieldMapper.java | 5 ----- .../org/elasticsearch/index/mapper/VersionFieldMapper.java | 5 ----- 9 files changed, 1 insertion(+), 42 deletions(-) diff --git a/plugins/mapper-size/src/main/java/org/elasticsearch/index/mapper/size/SizeFieldMapper.java b/plugins/mapper-size/src/main/java/org/elasticsearch/index/mapper/size/SizeFieldMapper.java index 39661d826ffb0..86a1123287294 100644 --- a/plugins/mapper-size/src/main/java/org/elasticsearch/index/mapper/size/SizeFieldMapper.java +++ b/plugins/mapper-size/src/main/java/org/elasticsearch/index/mapper/size/SizeFieldMapper.java @@ -82,11 +82,6 @@ public boolean enabled() { @Override public void postParse(ParseContext context) throws IOException { // we post parse it so we get the size stored, possibly compressed (source will be preParse) - doParse(context); - } - - @Override - protected void parseCreateField(ParseContext context) { if (enabled.value() == false) { return; } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/IdFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/IdFieldMapper.java index bf40c77437847..300688ebc2f3d 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/IdFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/IdFieldMapper.java @@ -257,11 +257,6 @@ private IdFieldMapper() { @Override public void preParse(ParseContext context) throws IOException { - doParse(context); - } - - @Override - protected void parseCreateField(ParseContext context) throws IOException { BytesRef id = Uid.encodeId(context.sourceToParse().id()); context.doc().add(new Field(NAME, id, Defaults.FIELD_TYPE)); } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/IgnoredFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/IgnoredFieldMapper.java index 6034cb2fbef02..c1e0568a4db03 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/IgnoredFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/IgnoredFieldMapper.java @@ -84,11 +84,6 @@ private IgnoredFieldMapper() { @Override public void postParse(ParseContext context) throws IOException { - doParse(context); - } - - @Override - protected void parseCreateField(ParseContext context) throws IOException { for (String field : context.getIgnoredFields()) { context.doc().add(new Field(NAME, field, Defaults.FIELD_TYPE)); } diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MetadataFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/MetadataFieldMapper.java index 3cb7b3f0170ab..9ea6fc59a6383 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MetadataFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MetadataFieldMapper.java @@ -165,7 +165,7 @@ public void parse(ParseContext context) throws IOException { } /** - * Do the actual parse of the field by calling {@link FieldMapper#parse(ParseContext)} + * Do the actual parsing of the field by calling {@link FieldMapper#parse(ParseContext)} */ protected void doParse(ParseContext context) throws IOException { super.parse(context); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/NestedPathFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/NestedPathFieldMapper.java index 35fae16c6b847..d9f300a1f7b1f 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/NestedPathFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/NestedPathFieldMapper.java @@ -31,7 +31,6 @@ import org.elasticsearch.index.IndexSettings; import org.elasticsearch.index.query.QueryShardContext; -import java.io.IOException; import java.util.Collections; public class NestedPathFieldMapper extends MetadataFieldMapper { @@ -93,11 +92,6 @@ private NestedPathFieldMapper(Settings settings) { super(new NestedPathFieldType(settings)); } - @Override - public void preParse(ParseContext context) throws IOException { - doParse(context); - } - @Override protected String contentType() { return NAME; diff --git a/server/src/main/java/org/elasticsearch/index/mapper/RoutingFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/RoutingFieldMapper.java index b44a33c46b0cc..4aeac3ea337b3 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/RoutingFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/RoutingFieldMapper.java @@ -117,11 +117,6 @@ public boolean required() { @Override public void preParse(ParseContext context) throws IOException { - doParse(context); - } - - @Override - protected void parseCreateField(ParseContext context) throws IOException { String routing = context.sourceToParse().routing(); if (routing != null) { context.doc().add(new Field(fieldType().name(), routing, Defaults.FIELD_TYPE)); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/SeqNoFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/SeqNoFieldMapper.java index 489c4945ec76c..59cdee09a576a 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/SeqNoFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/SeqNoFieldMapper.java @@ -182,11 +182,6 @@ public SeqNoFieldMapper() { @Override public void preParse(ParseContext context) throws IOException { - doParse(context); - } - - @Override - protected void parseCreateField(ParseContext context) throws IOException { // see InternalEngine.innerIndex to see where the real version value is set // also see ParsedDocument.updateSeqID (called by innerIndex) SequenceIDFields seqID = SequenceIDFields.emptySeqID(); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/TypeFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/TypeFieldMapper.java index 2c92d70dd7079..49638b26a9685 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/TypeFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/TypeFieldMapper.java @@ -168,11 +168,6 @@ private TypeFieldMapper() { super(new TypeFieldType()); } - @Override - public void preParse(ParseContext context) throws IOException { - doParse(context); - } - @Override protected String contentType() { return CONTENT_TYPE; diff --git a/server/src/main/java/org/elasticsearch/index/mapper/VersionFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/VersionFieldMapper.java index 4181f720540a6..bb4a711736f27 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/VersionFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/VersionFieldMapper.java @@ -68,11 +68,6 @@ private VersionFieldMapper() { @Override public void preParse(ParseContext context) throws IOException { - doParse(context); - } - - @Override - protected void parseCreateField(ParseContext context) throws IOException { // see InternalEngine.updateVersion to see where the real version value is set final Field version = new NumericDocValuesField(NAME, -1L); context.version(version); From a734ea888118bab84e2aa8af25ffd96eb4022a78 Mon Sep 17 00:00:00 2001 From: Christos Soulios Date: Thu, 17 Sep 2020 21:20:54 +0300 Subject: [PATCH 09/12] Cleanup parsing for existing metadata fields --- .../java/org/elasticsearch/index/mapper/DocumentParser.java | 1 - .../org/elasticsearch/index/mapper/SourceFieldMapper.java | 5 ----- 2 files changed, 6 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 850ec1f648d7d..fb747696c47c8 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java @@ -593,7 +593,6 @@ private static void parseValue(final ParseContext context, ObjectMapper parentMa throw new MapperParsingException("object mapping [" + parentMapper.name() + "] trying to serialize a value with" + " no field associated with it, current value [" + context.parser().textOrNull() + "]"); } - Mapper mapper = getMapper(context, parentMapper, currentFieldName, paths); if (mapper != null) { parseObjectOrField(context, mapper); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/SourceFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/SourceFieldMapper.java index 89cec0aa8e873..b06bf9f0c48a1 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/SourceFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/SourceFieldMapper.java @@ -156,11 +156,6 @@ public boolean isComplete() { @Override public void preParse(ParseContext context) throws IOException { - doParse(context); - } - - @Override - protected void parseCreateField(ParseContext context) throws IOException { BytesReference originalSource = context.sourceToParse().source(); XContentType contentType = context.sourceToParse().getXContentType(); final BytesReference adaptedSource = applyFilters(originalSource, contentType); From b8b3a9d4329ccadfa7332922047d6f205c0d7ccb Mon Sep 17 00:00:00 2001 From: Christos Soulios Date: Thu, 17 Sep 2020 22:07:54 +0300 Subject: [PATCH 10/12] Remove doParse() method --- .../mapper/RankFeatureMetaFieldMapper.java | 5 ----- .../index/mapper/MetadataFieldMapper.java | 19 +------------------ .../index/mapper/DocumentParserTests.java | 4 ++-- .../index/mapper/IdFieldMapperTests.java | 3 ++- .../mapper/MockMetadataMapperPlugin.java | 5 ----- .../index/mapper/RoutingFieldMapperTests.java | 2 +- .../DataStreamTimestampFieldMapper.java | 6 ------ 7 files changed, 6 insertions(+), 38 deletions(-) diff --git a/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/RankFeatureMetaFieldMapper.java b/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/RankFeatureMetaFieldMapper.java index 13658570de362..54977637e26ab 100644 --- a/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/RankFeatureMetaFieldMapper.java +++ b/modules/mapper-extras/src/main/java/org/elasticsearch/index/mapper/RankFeatureMetaFieldMapper.java @@ -65,11 +65,6 @@ private RankFeatureMetaFieldMapper() { super(RankFeatureMetaFieldType.INSTANCE); } - @Override - protected void parseCreateField(ParseContext context) { - throw new AssertionError("Should never be called"); - } - @Override protected String contentType() { return CONTENT_TYPE; diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MetadataFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/MetadataFieldMapper.java index 9ea6fc59a6383..1a56b92d7e417 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MetadataFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MetadataFieldMapper.java @@ -153,29 +153,12 @@ public final XContentBuilder toXContent(XContentBuilder builder, Params params) return builder.endObject(); } - /* - * By default metadata fields cannot be set through the document source and parse() method - * throws an exception. To enable a metadata field to parse the document source, this - * method must be overridden and doParse() should be called. - */ @Override - public void parse(ParseContext context) throws IOException { + protected void parseCreateField(ParseContext context) throws IOException { throw new MapperParsingException("Field [" + name() + "] is a metadata field and cannot be added inside" + " a document. Use the index API request parameters."); } - /** - * Do the actual parsing of the field by calling {@link FieldMapper#parse(ParseContext)} - */ - protected void doParse(ParseContext context) throws IOException { - super.parse(context); - } - - @Override - protected void parseCreateField(ParseContext context) throws IOException { - // do nothing - } - /** * Called before {@link FieldMapper#parse(ParseContext)} on the {@link RootObjectMapper}. */ 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 1379cec9e0cb8..c37537fbfecd2 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java @@ -939,8 +939,8 @@ public void testDocumentContainsMetadataField() throws Exception { DocumentMapper mapper = createDocumentMapper(mapping(b -> {})); MapperParsingException e = expectThrows(MapperParsingException.class, () -> mapper.parse(source(b -> b.field("_field_names", 0)))); - assertTrue(e.getMessage(), - e.getMessage().contains("Field [_field_names] is a metadata field and cannot be added inside a document.")); + assertTrue(e.getCause().getMessage(), + e.getCause().getMessage().contains("Field [_field_names] is a metadata field and cannot be added inside a document.")); mapper.parse(source(b -> b.field("foo._field_names", 0))); // parses without error } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/IdFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/IdFieldMapperTests.java index 4b425c378c1a6..fbf593df5fc5b 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/IdFieldMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/IdFieldMapperTests.java @@ -59,7 +59,8 @@ public void testIncludeInObjectNotAllowed() throws Exception { .startObject().field("_id", "1").endObject()), XContentType.JSON)); fail("Expected failure to parse metadata field"); } catch (MapperParsingException e) { - assertTrue(e.getMessage(), e.getMessage().contains("Field [_id] is a metadata field and cannot be added inside a document")); + assertTrue(e.getCause().getMessage(), + e.getCause().getMessage().contains("Field [_id] is a metadata field and cannot be added inside a document")); } } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/MockMetadataMapperPlugin.java b/server/src/test/java/org/elasticsearch/index/mapper/MockMetadataMapperPlugin.java index e83e794cf2fe3..db2ddf8efb5d2 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/MockMetadataMapperPlugin.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/MockMetadataMapperPlugin.java @@ -49,11 +49,6 @@ protected MockMetadataMapper() { super(new KeywordFieldMapper.KeywordFieldType(FIELD_NAME)); } - @Override - public void parse(ParseContext context) throws IOException { - doParse(context); - } - @Override protected void parseCreateField(ParseContext context) throws IOException { if (context.parser().currentToken() == XContentParser.Token.VALUE_STRING) { diff --git a/server/src/test/java/org/elasticsearch/index/mapper/RoutingFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/RoutingFieldMapperTests.java index d4e6f685aeeb2..cf1404875382c 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/RoutingFieldMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/RoutingFieldMapperTests.java @@ -58,7 +58,7 @@ public void testIncludeInObjectNotAllowed() throws Exception { .startObject().field("_routing", "foo").endObject()),XContentType.JSON)); fail("Expected failure to parse metadata field"); } catch (MapperParsingException e) { - assertThat(e.getMessage(), e.getMessage(), + assertThat(e.getCause().getMessage(), e.getCause().getMessage(), containsString("Field [_routing] is a metadata field and cannot be added inside a document")); } } diff --git a/x-pack/plugin/data-streams/src/main/java/org/elasticsearch/xpack/datastreams/mapper/DataStreamTimestampFieldMapper.java b/x-pack/plugin/data-streams/src/main/java/org/elasticsearch/xpack/datastreams/mapper/DataStreamTimestampFieldMapper.java index 298fb4a8dc8cf..042c84adbd3bd 100644 --- a/x-pack/plugin/data-streams/src/main/java/org/elasticsearch/xpack/datastreams/mapper/DataStreamTimestampFieldMapper.java +++ b/x-pack/plugin/data-streams/src/main/java/org/elasticsearch/xpack/datastreams/mapper/DataStreamTimestampFieldMapper.java @@ -171,12 +171,6 @@ public void doValidate(MappingLookup lookup) { } } - @Override - protected void parseCreateField(ParseContext context) throws IOException { - // Meta field doesn't create any fields, so this shouldn't happen. - throw new IllegalStateException(NAME + " field mapper cannot create fields"); - } - @Override public void postParse(ParseContext context) throws IOException { if (enabled == false) { From 1560e732c1222c71ad0c2b64b5ae0dbeeb82bf84 Mon Sep 17 00:00:00 2001 From: Christos Soulios Date: Thu, 17 Sep 2020 22:41:06 +0300 Subject: [PATCH 11/12] Fix broken test --- .../index/mapper/RankFeatureMetaFieldMapperTests.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/modules/mapper-extras/src/test/java/org/elasticsearch/index/mapper/RankFeatureMetaFieldMapperTests.java b/modules/mapper-extras/src/test/java/org/elasticsearch/index/mapper/RankFeatureMetaFieldMapperTests.java index e11a08597dc24..36e55012da849 100644 --- a/modules/mapper-extras/src/test/java/org/elasticsearch/index/mapper/RankFeatureMetaFieldMapperTests.java +++ b/modules/mapper-extras/src/test/java/org/elasticsearch/index/mapper/RankFeatureMetaFieldMapperTests.java @@ -69,6 +69,7 @@ public void testDocumentParsingFailsOnMetaField() throws Exception { BytesReference bytes = BytesReference.bytes(XContentFactory.jsonBuilder().startObject().field(rfMetaField, 0).endObject()); MapperParsingException e = expectThrows(MapperParsingException.class, () -> mapper.parse(new SourceToParse("test", "1", bytes, XContentType.JSON))); - assertTrue(e.getMessage().contains("Field ["+ rfMetaField + "] is a metadata field and cannot be added inside a document.")); + assertTrue( + e.getCause().getMessage().contains("Field ["+ rfMetaField + "] is a metadata field and cannot be added inside a document.")); } } From 0c962c48fb24a46d42210c1bf98218d0cb278f1d Mon Sep 17 00:00:00 2001 From: Christos Soulios Date: Thu, 17 Sep 2020 23:02:52 +0300 Subject: [PATCH 12/12] Lookup metadata mapper by name Instead of linear scan --- .../elasticsearch/index/mapper/DocumentParser.java | 12 +++++------- .../java/org/elasticsearch/index/mapper/Mapping.java | 8 ++++++-- 2 files changed, 11 insertions(+), 9 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 fb747696c47c8..1fa8268eccb4b 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java @@ -880,16 +880,14 @@ private static ObjectMapper.Dynamic dynamicOrDefault(ObjectMapper parentMapper, // looks up a child mapper, but takes into account field names that expand to objects private static Mapper getMapper(final ParseContext context, ObjectMapper objectMapper, String fieldName, String[] subfields) { String fieldPath = context.path().pathAsText(fieldName); - if (context.mapperService().isMetadataField(fieldPath)) { - for (MetadataFieldMapper metadataFieldMapper : context.docMapper().mapping().getMetadataMappers()) { - if (fieldPath.equals(metadataFieldMapper.name())) { - return metadataFieldMapper; - } - } + // Check if mapper is a metadata mapper first + Mapper mapper = context.docMapper().mapping().getMetadataMapper(fieldPath); + if (mapper != null) { + return mapper; } for (int i = 0; i < subfields.length - 1; ++i) { - Mapper mapper = objectMapper.getMapper(subfields[i]); + mapper = objectMapper.getMapper(subfields[i]); if (mapper == null || (mapper instanceof ObjectMapper) == false) { return null; } 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 69db34e328923..b36d240be0b73 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/Mapping.java @@ -48,6 +48,7 @@ public final class Mapping implements ToXContentFragment { final RootObjectMapper root; final MetadataFieldMapper[] metadataMappers; final Map, MetadataFieldMapper> metadataMappersMap; + final Map metadataMappersByName; final Map meta; public Mapping(Version indexCreated, RootObjectMapper rootObjectMapper, @@ -55,8 +56,10 @@ public Mapping(Version indexCreated, RootObjectMapper rootObjectMapper, this.indexCreated = indexCreated; this.metadataMappers = metadataMappers; Map, MetadataFieldMapper> metadataMappersMap = new HashMap<>(); + Map metadataMappersByName = new HashMap<>(); for (MetadataFieldMapper metadataMapper : metadataMappers) { metadataMappersMap.put(metadataMapper.getClass(), metadataMapper); + metadataMappersByName.put(metadataMapper.name(), metadataMapper); } this.root = rootObjectMapper; // keep root mappers sorted for consistent serialization @@ -67,6 +70,7 @@ public int compare(Mapper o1, Mapper o2) { } }); this.metadataMappersMap = unmodifiableMap(metadataMappersMap); + this.metadataMappersByName = unmodifiableMap(metadataMappersByName); this.meta = meta; } @@ -136,8 +140,8 @@ public Mapping merge(Mapping mergeWith, MergeReason reason) { return new Mapping(indexCreated, mergedRoot, mergedMetadataMappers.values().toArray(new MetadataFieldMapper[0]), mergedMeta); } - public MetadataFieldMapper[] getMetadataMappers() { - return metadataMappers; + public MetadataFieldMapper getMetadataMapper(String mapperName) { + return metadataMappersByName.get(mapperName); } @Override