From ffcaffc29fa23f38756af8ebfabca9d16e560270 Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Thu, 22 Jul 2021 13:15:07 +0100 Subject: [PATCH] Handle runtime subfields when shadowing dynamic mappings (#75595) In #75454 we changed our dynamic shadowing logic to check that an unmapped field was truly shadowed by a runtime field before returning no-op mappers. However, this does not handle the case where the runtime field can have multiple subfields, as will be true for the upcoming composite field type. We instead need to check that the field in question would not be shadowed by any field type returned by any runtime field. This commit abstracts this logic into a new isShadowed() method on DocumentParserContext, which uses a set of runtime field type names built from the mapping lookup at construction time. It also simplifies the no-op mapper slightly by making it a singleton object, as we don't need to preserve field names here. --- .../index/mapper/DocumentParser.java | 97 ++++++++++--------- .../index/mapper/DocumentParserContext.java | 17 ++++ .../mapper/BooleanScriptFieldTypeTests.java | 2 +- .../index/mapper/DocumentParserTests.java | 45 +++++++++ .../index/mapper/TestRuntimeField.java | 8 +- .../mapper/TestDocumentParserContext.java | 2 +- 6 files changed, 124 insertions(+), 47 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 282df86be0a16..254de38e525ac 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java @@ -64,7 +64,8 @@ public final class DocumentParser { /** * Parse a document - * @param source the document to parse + * + * @param source the document to parse * @param mappingLookup the mappings information needed to parse the document * @return the parsed document * @throws MapperParsingException whenever there's a problem parsing the document @@ -160,6 +161,7 @@ private static void executeIndexTimeScripts(DocumentParserContext context) { Map> fieldScripts = new HashMap<>(); indexTimeScriptMappers.forEach(mapper -> fieldScripts.put(mapper.name(), new Consumer<>() { boolean executed = false; + @Override public void accept(LeafReaderContext leafReaderContext) { if (executed == false) { @@ -233,10 +235,10 @@ private static String[] splitAndValidatePath(String fullFieldPath) { // check if the field name contains only whitespace if (Strings.isEmpty(part) == false) { throw new IllegalArgumentException( - "object field cannot contain only whitespace: ['" + fullFieldPath + "']"); + "object field cannot contain only whitespace: ['" + fullFieldPath + "']"); } throw new IllegalArgumentException( - "object field starting or ending with a [.] makes object resolution ambiguous: [" + fullFieldPath + "]"); + "object field starting or ending with a [.] makes object resolution ambiguous: [" + fullFieldPath + "]"); } } return parts; @@ -244,11 +246,13 @@ private static String[] splitAndValidatePath(String fullFieldPath) { if (Strings.isEmpty(fullFieldPath)) { throw new IllegalArgumentException("field name cannot be an empty string"); } - return new String[] {fullFieldPath}; + return new String[]{fullFieldPath}; } } - /** Creates a Mapping containing any dynamically added fields, or returns null if there were no dynamic mappings. */ + /** + * 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) { @@ -319,7 +323,7 @@ private static RootObjectMapper createDynamicUpdate(MappingLookup mappingLookup, } popMappers(parentMappers, 1, true); assert parentMappers.size() == 1; - return (RootObjectMapper)parentMappers.get(0); + return (RootObjectMapper) parentMappers.get(0); } private static void popMappers(List parentMappers, int keepBefore, boolean merge) { @@ -375,7 +379,9 @@ private static int expandCommonMappers(List parentMappers, String[ return i; } - /** Creates an update for intermediate object mappers that are not on the stack, but parents of newMapper. */ + /** + * 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]; @@ -389,7 +395,9 @@ private static ObjectMapper createExistingMapperUpdate(List parent return createUpdate(updateParent, nameParts, i + 1, newMapper); } - /** Build an update for the parent which will contain the given mapper and any intermediate fields. */ + /** + * 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; @@ -397,8 +405,8 @@ private static ObjectMapper createUpdate(ObjectMapper parent, String[] nameParts 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; + 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 @@ -609,7 +617,7 @@ private static void parseArray(DocumentParserContext context, ObjectMapper paren ObjectMapper.Dynamic dynamic = dynamicOrDefault(parentMapper, context); if (dynamic == ObjectMapper.Dynamic.STRICT) { throw new StrictDynamicMappingException(parentMapper.fullPath(), arrayFieldName); - } else if (dynamic == ObjectMapper.Dynamic.FALSE) { + } else if (dynamic == ObjectMapper.Dynamic.FALSE) { // TODO: shouldn't this skip, not parse? parseNonDynamicArray(context, parentMapper, lastFieldName, arrayFieldName); } else { @@ -705,7 +713,9 @@ private static void parseDynamicValue(final DocumentParserContext context, Objec dynamic.getDynamicFieldsBuilder().createDynamicFieldFromValue(context, token, currentFieldName); } - /** Creates instances of the fields that the current field should be copied to */ + /** + * Creates instances of the fields that the current field should be copied to + */ private static void parseCopyFields(DocumentParserContext context, List copyToFields) throws IOException { context = context.createCopyToContext(); for (String field : copyToFields) { @@ -729,7 +739,9 @@ private static void parseCopyFields(DocumentParserContext context, List } } - /** Creates an copy of the current field with given field name and boost */ + /** + * Creates an copy of the current field with given field name and boost + */ private static void parseCopy(String field, DocumentParserContext context) throws IOException { Mapper mapper = context.mappingLookup().getMapper(field); if (mapper != null) { @@ -746,7 +758,7 @@ private static void parseCopy(String field, DocumentParserContext context) throw context = context.overridePath(new ContentPath(0)); final String[] paths = splitAndValidatePath(field); - final String fieldName = paths[paths.length-1]; + final String fieldName = paths[paths.length - 1]; Tuple parentMapperTuple = getDynamicParentMapper(context, paths, null); ObjectMapper objectMapper = parentMapperTuple.v2(); parseDynamicValue(context, objectMapper, fieldName, context.parser().currentToken()); @@ -761,7 +773,7 @@ private static Tuple getDynamicParentMapper(DocumentParse ObjectMapper mapper = currentParent == null ? context.root() : currentParent; int pathsAdded = 0; ObjectMapper parent = mapper; - for (int i = 0; i < paths.length-1; i++) { + for (int i = 0; i < paths.length - 1; i++) { String name = paths[i]; String currentPath = context.path().pathAsText(name); Mapper existingFieldMapper = context.mappingLookup().getMapper(currentPath); @@ -780,7 +792,7 @@ private static Tuple getDynamicParentMapper(DocumentParse // Should not dynamically create any more mappers so return the last mapper return new Tuple<>(pathsAdded, parent); } else if (dynamic == ObjectMapper.Dynamic.RUNTIME) { - mapper = new NoOpObjectMapper(name, currentPath); + mapper = new NoOpObjectMapper(name, currentPath); } else { final Mapper fieldMapper = dynamic.getDynamicFieldsBuilder().createDynamicObjectMapper(context, name); if (fieldMapper instanceof ObjectMapper == false) { @@ -852,11 +864,11 @@ private static Mapper getMapper(final DocumentParserContext context, if (mapper instanceof ObjectMapper == false) { return null; } - objectMapper = (ObjectMapper)mapper; + objectMapper = (ObjectMapper) mapper; if (objectMapper.isNested()) { throw new MapperParsingException("Cannot add a value for field [" - + fieldName + "] since one of the intermediate objects is mapped as a nested object: [" - + mapper.name() + "]"); + + fieldName + "] since one of the intermediate objects is mapped as a nested object: [" + + mapper.name() + "]"); } } String leafName = subfields[subfields.length - 1]; @@ -878,36 +890,33 @@ private static Mapper getLeafMapper(final DocumentParserContext context, // if a leaf field is not mapped, and is defined as a runtime field, then we // don't create a dynamic mapping for it and don't index it. String fieldPath = context.path().pathAsText(fieldName); - MappedFieldType fieldType = context.mappingLookup().getFieldType(fieldPath); - if (fieldType != null) { - RuntimeField runtimeField = context.root().getRuntimeField(fieldPath); - if (runtimeField != null) { - assert fieldType.hasDocValues() == false && fieldType.isAggregatable() && fieldType.isSearchable(); - return new NoOpFieldMapper(subfields[subfields.length - 1], fieldType.name()); - } + if (context.isShadowed(fieldPath)) { + return NO_OP_FIELDMAPPER; } return null; } - private static class NoOpFieldMapper extends FieldMapper { - NoOpFieldMapper(String simpleName, String fullName) { - super(simpleName, new MappedFieldType(fullName, false, false, false, TextSearchInfo.NONE, Collections.emptyMap()) { - @Override - public ValueFetcher valueFetcher(SearchExecutionContext context, String format) { - throw new UnsupportedOperationException(); - } + private static final FieldMapper NO_OP_FIELDMAPPER = new FieldMapper( + "no-op", + new MappedFieldType("no-op", false, false, false, TextSearchInfo.NONE, Collections.emptyMap()) { + @Override + public ValueFetcher valueFetcher(SearchExecutionContext context, String format) { + throw new UnsupportedOperationException(); + } - @Override - public String typeName() { - throw new UnsupportedOperationException(); - } + @Override + public String typeName() { + throw new UnsupportedOperationException(); + } - @Override - public Query termQuery(Object value, SearchExecutionContext context) { - throw new UnsupportedOperationException(); - } - }, MultiFields.empty(), CopyTo.empty()); - } + @Override + public Query termQuery(Object value, SearchExecutionContext context) { + throw new UnsupportedOperationException(); + } + }, + FieldMapper.MultiFields.empty(), + FieldMapper.CopyTo.empty() + ) { @Override protected void parseCreateField(DocumentParserContext context) throws IOException { @@ -963,7 +972,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws protected String contentType() { throw new UnsupportedOperationException(); } - } + }; private static class NoOpObjectMapper extends ObjectMapper { NoOpObjectMapper(String name, String fullPath) { 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 6a12a2e7194ff..912b8153f5c05 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java @@ -92,6 +92,7 @@ protected void addDoc(LuceneDocument doc) { private final Set newFieldsSeen; private final Map dynamicObjectMappers; private final List dynamicRuntimeFields; + private final Set shadowedFields; private Field version; private SeqNoFieldMapper.SequenceIDFields seqID; @@ -107,6 +108,7 @@ private DocumentParserContext(DocumentParserContext in) { this.newFieldsSeen = in.newFieldsSeen; this.dynamicObjectMappers = in.dynamicObjectMappers; this.dynamicRuntimeFields = in.dynamicRuntimeFields; + this.shadowedFields = in.shadowedFields; this.version = in.version; this.seqID = in.seqID; } @@ -127,6 +129,17 @@ protected DocumentParserContext(MappingLookup mappingLookup, this.newFieldsSeen = new HashSet<>(); this.dynamicObjectMappers = new HashMap<>(); this.dynamicRuntimeFields = new ArrayList<>(); + this.shadowedFields = buildShadowedFields(mappingLookup); + } + + private static Set buildShadowedFields(MappingLookup lookup) { + Set shadowedFields = new HashSet<>(); + for (RuntimeField runtimeField : lookup.getMapping().getRoot().runtimeFields()) { + for (MappedFieldType mft : runtimeField.asMappedFieldTypes()) { + shadowedFields.add(mft.name()); + } + } + return shadowedFields; } public final IndexSettings indexSettings() { @@ -229,6 +242,10 @@ public final List getDynamicMappers() { return dynamicMappers; } + public final boolean isShadowed(String field) { + return shadowedFields.contains(field); + } + public final ObjectMapper getObjectMapper(String name) { return dynamicObjectMappers.get(name); } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/BooleanScriptFieldTypeTests.java b/server/src/test/java/org/elasticsearch/index/mapper/BooleanScriptFieldTypeTests.java index 389e7dce3f3ac..227e367649228 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/BooleanScriptFieldTypeTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/BooleanScriptFieldTypeTests.java @@ -308,7 +308,7 @@ public void testDualingQueries() throws IOException { String source = "{\"foo\": " + values + "}"; XContentParser parser = createParser(JsonXContent.jsonXContent, source); SourceToParse sourceToParse = new SourceToParse("test", "test", new BytesArray(source), XContentType.JSON); - DocumentParserContext ctx = new TestDocumentParserContext(null, null, null, null, sourceToParse) { + DocumentParserContext ctx = new TestDocumentParserContext(MappingLookup.EMPTY, null, null, null, sourceToParse) { @Override public XContentParser parser() { return parser; 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 240a0862aaf01..efd862b95a77c 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java @@ -1928,6 +1928,35 @@ public void testParseWithFlattenedField() throws IOException { assertNull(doc.rootDoc().getField("field.second")); } + public void testDynamicShadowingOfRuntimeSubfields() throws IOException { + + // Create mappings with a runtime field called 'obj' that produces two subfields, + // 'obj.foo' and 'obj.bar' + + DocumentMapper mapper = createDocumentMapper(topMapping(b -> { + b.startObject("runtime"); + b.startObject("obj").field("type", "test-composite").endObject(); + b.endObject(); + })); + + // Incoming documents should not create mappings for 'obj.foo' fields, as they will + // be shadowed by the runtime fields; but other subfields are fine and should be + // indexed + + ParsedDocument doc = mapper.parse(source(b -> { + b.startObject("obj"); + b.field("foo", "ignored"); + b.field("baz", "indexed"); + b.field("bar", "ignored"); + b.endObject(); + })); + + assertNull(doc.rootDoc().getField("obj.foo")); + assertNotNull(doc.rootDoc().getField("obj.baz")); + assertNull(doc.rootDoc().getField("obj.bar")); + assertNotNull(doc.dynamicMappingsUpdate()); + } + /** * Mapper plugin providing a mock metadata field mapper implementation that supports setting its value * as well as a mock runtime field parser. @@ -1965,5 +1994,21 @@ protected String contentType() { public Map getMetadataMappers() { return Collections.singletonMap(MockMetadataMapper.CONTENT_TYPE, MockMetadataMapper.PARSER); } + + @Override + public Map getRuntimeFields() { + return Collections.singletonMap( + "test-composite", + new RuntimeField.Parser(n -> new RuntimeField.Builder(n) { + @Override + protected RuntimeField createRuntimeField(MappingParserContext parserContext) { + return new TestRuntimeField(n, List.of( + new KeywordFieldMapper.KeywordFieldType(n + ".foo"), + new KeywordFieldMapper.KeywordFieldType(n + ".bar") + )); + } + }) + ); + } } } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/TestRuntimeField.java b/server/src/test/java/org/elasticsearch/index/mapper/TestRuntimeField.java index 105d0577ba8d5..902d45f4cf53d 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/TestRuntimeField.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/TestRuntimeField.java @@ -17,6 +17,9 @@ import java.util.Collections; public final class TestRuntimeField implements RuntimeField { + + public static final String CONTENT_TYPE = "test-composite"; + private final String name; private final Collection subfields; @@ -41,7 +44,10 @@ public Collection asMappedFieldTypes() { @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { - throw new UnsupportedOperationException(); + builder.startObject(name); + builder.field("type", CONTENT_TYPE); + builder.endObject(); + return builder; } public static class TestRuntimeFieldType extends MappedFieldType { diff --git a/test/framework/src/main/java/org/elasticsearch/index/mapper/TestDocumentParserContext.java b/test/framework/src/main/java/org/elasticsearch/index/mapper/TestDocumentParserContext.java index d23cde1fba5ac..6f3dc62757ba8 100644 --- a/test/framework/src/main/java/org/elasticsearch/index/mapper/TestDocumentParserContext.java +++ b/test/framework/src/main/java/org/elasticsearch/index/mapper/TestDocumentParserContext.java @@ -31,7 +31,7 @@ public class TestDocumentParserContext extends DocumentParserContext { * Use with caution as it can cause {@link NullPointerException}s down the line. */ public TestDocumentParserContext() { - super(null, null, null, null, null); + super(MappingLookup.EMPTY, null, null, null, null); } /**