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.

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 @@ public final class DocumentParser {
         Map<String, Consumer<LeafReaderContext>> 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 @@ public final class DocumentParser {
                     // 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 @@ public final class DocumentParser {
             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<Mapper> dynamicMappers,
                                        List<RuntimeField> dynamicRuntimeFields) {
@@ -319,7 +323,7 @@ public final class DocumentParser {
         }
         popMappers(parentMappers, 1, true);
         assert parentMappers.size() == 1;
-        return (RootObjectMapper)parentMappers.get(0);
+        return (RootObjectMapper) parentMappers.get(0);
     }
 
     private static void popMappers(List<ObjectMapper> parentMappers, int keepBefore, boolean merge) {
@@ -375,7 +379,9 @@ public final class DocumentParser {
         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<ObjectMapper> parentMappers, String[] nameParts, int i,
                                                            MappingLookup mappingLookup, Mapper newMapper) {
         String updateParentName = nameParts[i];
@@ -389,7 +395,9 @@ public final class DocumentParser {
         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<ObjectMapper> parentMappers = new ArrayList<>();
         ObjectMapper previousIntermediate = parent;
@@ -397,8 +405,8 @@ public final class DocumentParser {
             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 @@ public final class DocumentParser {
             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 @@ public final class DocumentParser {
         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<String> copyToFields) throws IOException {
         context = context.createCopyToContext();
         for (String field : copyToFields) {
@@ -729,7 +739,9 @@ public final class DocumentParser {
         }
     }
 
-    /** 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 @@ public final class DocumentParser {
             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<Integer, ObjectMapper> parentMapperTuple = getDynamicParentMapper(context, paths, null);
             ObjectMapper objectMapper = parentMapperTuple.v2();
             parseDynamicValue(context, objectMapper, fieldName, context.parser().currentToken());
@@ -761,7 +773,7 @@ public final class DocumentParser {
         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 @@ public final class DocumentParser {
                     // 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 @@ public final class DocumentParser {
             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 @@ public final class DocumentParser {
         // 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 final class DocumentParser {
         protected String contentType() {
             throw new UnsupportedOperationException();
         }
-    }
+    };
 
     private static class NoOpObjectMapper extends ObjectMapper {
         NoOpObjectMapper(String name, String fullPath) {

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

@@ -92,6 +92,7 @@ public abstract class DocumentParserContext {
     private final Set<String> newFieldsSeen;
     private final Map<String, ObjectMapper> dynamicObjectMappers;
     private final List<RuntimeField> dynamicRuntimeFields;
+    private final Set<String> shadowedFields;
     private Field version;
     private SeqNoFieldMapper.SequenceIDFields seqID;
 
@@ -107,6 +108,7 @@ public abstract class DocumentParserContext {
         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 @@ public abstract class DocumentParserContext {
         this.newFieldsSeen = new HashSet<>();
         this.dynamicObjectMappers = new HashMap<>();
         this.dynamicRuntimeFields = new ArrayList<>();
+        this.shadowedFields = buildShadowedFields(mappingLookup);
+    }
+
+    private static Set<String> buildShadowedFields(MappingLookup lookup) {
+        Set<String> 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 abstract class DocumentParserContext {
         return dynamicMappers;
     }
 
+    public final boolean isShadowed(String field) {
+        return shadowedFields.contains(field);
+    }
+
     public final ObjectMapper getObjectMapper(String name) {
         return dynamicObjectMappers.get(name);
     }

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

@@ -308,7 +308,7 @@ public class BooleanScriptFieldTypeTests extends AbstractNonTextScriptFieldTypeT
             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;

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

@@ -1928,6 +1928,35 @@ public class DocumentParserTests extends MapperServiceTestCase {
         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 @@ public class DocumentParserTests extends MapperServiceTestCase {
         public Map<String, MetadataFieldMapper.TypeParser> getMetadataMappers() {
             return Collections.singletonMap(MockMetadataMapper.CONTENT_TYPE, MockMetadataMapper.PARSER);
         }
+
+        @Override
+        public Map<String, RuntimeField.Parser> 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")
+                        ));
+                    }
+                })
+            );
+        }
     }
 }

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

@@ -17,6 +17,9 @@ import java.util.Collection;
 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<MappedFieldType> subfields;
 
@@ -41,7 +44,10 @@ public final class TestRuntimeField implements RuntimeField {
 
     @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 {

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);
     }
 
     /**