Fix missing intermediate object error when applying dynamic template (e…

…lastic#87622)

When we apply dynamic mappings, we iterate over all the dynamic mappers retrieved from the DocumentParserContext, which are registered while parsing the document, and for each field look up their parent (going back multiple levels if necessary), and add it to the dynamic mapping update with the added/modified sub-field.

Dynamic mappers that are iterated through consist of a flat set of mappers, which can be both object mappers or field mappers. Most times, the object mappers from such set have no sub-fields as they are mapped as a result of parsing a document where the object appears for the first time, which also has sub-fields that are going to be added to the set of dynamic mappers themselves once they are parsed.

There are scenarios though where a dynamic template matches an object, and defines its structure including its subobjects as well as sub-fields. In that case the dynamically mapped object may hold sub-fields as well as define non-default values for dynamic, enabled or subobjects. The described situation was not well covered in tests so far, and is currently affected by a bug introduced with elastic#81449. When an object mapper is dynamically mapped, it is added to the map of dynamic object mappers, which makes it discoverable for sub-fields that will need to be added to it later, as well as to the set of dynamic mappers so that it becomes part of the mappings in case the document being parsed defines no sub-fields for it. What is missing is to recursively add its sub-fields to the dynamic object mappers. As a result of missing this step, intermediate objects that were dynamically mapped are not made discoverable which causes a cryptic "Missing intermediate object" error.

This commit fixed the problem by recursively registering inner sub-objects to the dynamic mappers whenever an object mapper is added to the dynamic mappers. It also changes the "missing intermediate object" error to make it more evident that it's an internal error and not a user error: it is now an IllegalStateException instead of an IllegalArgumentException.

Closes elastic#87513

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

@@ -403,8 +403,8 @@ private static DocumentParserContext nestedContext(DocumentParserContext context
     }
 
     static void parseObjectOrField(DocumentParserContext context, Mapper mapper) throws IOException {
-        if (mapper instanceof ObjectMapper) {
-            parseObjectOrNested(context, (ObjectMapper) mapper);
+        if (mapper instanceof ObjectMapper objectMapper) {
+            parseObjectOrNested(context, objectMapper);
         } else if (mapper instanceof FieldMapper fieldMapper) {
             fieldMapper.parse(context);
             if (context.isWithinCopyTo() == false) {
@@ -447,9 +447,10 @@ private static void throwOnCopyToOnObject(Mapper mapper, List<String> copyToFiel
         );
     }
 
-    private static void parseObject(final DocumentParserContext context, ObjectMapper mapper, String currentFieldName) throws IOException {
+    private static void parseObject(final DocumentParserContext context, ObjectMapper parentObjectMapper, String currentFieldName)
+        throws IOException {
         assert currentFieldName != null;
-        Mapper objectMapper = getMapper(context, mapper, currentFieldName);
+        Mapper objectMapper = getMapper(context, parentObjectMapper, currentFieldName);
         if (objectMapper != null) {
             context.path().add(currentFieldName);
             if (objectMapper instanceof ObjectMapper objMapper) {
@@ -461,16 +462,17 @@ private static void parseObject(final DocumentParserContext context, ObjectMappe
             context.path().setWithinLeafObject(false);
             context.path().remove();
         } else {
-            parseObjectDynamic(context, mapper, currentFieldName);
+            parseObjectDynamic(context, parentObjectMapper, currentFieldName);
         }
     }
 
-    private static void parseObjectDynamic(DocumentParserContext context, ObjectMapper mapper, String currentFieldName) throws IOException {
-        ObjectMapper.Dynamic dynamic = dynamicOrDefault(mapper, context);
+    private static void parseObjectDynamic(DocumentParserContext context, ObjectMapper parentObjectMapper, String currentFieldName)
+        throws IOException {
+        ObjectMapper.Dynamic dynamic = dynamicOrDefault(parentObjectMapper, context);
         if (dynamic == ObjectMapper.Dynamic.STRICT) {
-            throw new StrictDynamicMappingException(mapper.fullPath(), currentFieldName);
+            throw new StrictDynamicMappingException(parentObjectMapper.fullPath(), currentFieldName);
         } else if (dynamic == ObjectMapper.Dynamic.FALSE) {
-            failIfMatchesRoutingPath(context, mapper, currentFieldName);
+            failIfMatchesRoutingPath(context, parentObjectMapper, currentFieldName);
             // not dynamic, read everything up to end object
             context.parser().skipChildren();
         } else {

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

@@ -234,20 +234,44 @@ public final void addDynamicMapper(Mapper mapper) {
             && newFieldsSeen.add(mapper.name())) {
             mappingLookup.checkFieldLimit(indexSettings().getMappingTotalFieldsLimit(), newFieldsSeen.size());
         }
-        if (mapper instanceof ObjectMapper) {
-            dynamicObjectMappers.put(mapper.name(), (ObjectMapper) mapper);
+        if (mapper instanceof ObjectMapper objectMapper) {
+            dynamicObjectMappers.put(objectMapper.name(), objectMapper);
+            // dynamic object mappers may have been obtained from applying a dynamic template, in which case their definition may contain
+            // sub-fields as well as sub-objects that need to be added to the mappings
+            for (Mapper submapper : objectMapper.mappers.values()) {
+                // we could potentially skip the step of adding these to the dynamic mappers, because their parent is already added to
+                // that list, and what is important is that all of the intermediate objects are added to the dynamic object mappers so that
+                // they can be looked up once sub-fields need to be added to them. For simplicity, we treat these like any other object
+                addDynamicMapper(submapper);
+            }
         }
+        // TODO we may want to stop adding object mappers to the dynamic mappers list: most times they will be mapped when parsing their
+        // sub-fields (see ObjectMapper.Builder#addDynamic), which causes extra work as the two variants of the same object field
+        // will be merged together when creating the final dynamic update. The only cases where object fields need extra treatment are
+        // dynamically mapped objects when the incoming document defines no sub-fields in them:
+        // 1) by default, they would be empty containers in the mappings, is it then important to map them?
+        // 2) they can be the result of applying a dynamic template which may define sub-fields or set dynamic, enabled or subobjects.
         dynamicMappers.add(mapper);
     }
 
     /**
-     * Get dynamic mappers created while parsing.
+     * Get dynamic mappers created as a result of parsing an incoming document. Responsible for exposing all the newly created
+     * fields that need to be merged into the existing mappings. Used to create the required mapping update at the end of document parsing.
+     * Consists of a flat set of {@link Mapper}s that will need to be added to their respective parent {@link ObjectMapper}s in order
+     * to become part of the resulting dynamic mapping update.
      */
     public final List<Mapper> getDynamicMappers() {
         return dynamicMappers;
     }
 
-    public final ObjectMapper getDynamicObjectMapper(String name) {
+    /**
+     * Get a dynamic object mapper by name. Allows consumers to lookup objects that have been dynamically added as a result
+     * of parsing an incoming document. Used to find the parent object for new fields that are being dynamically mapped whose parent is
+     * also not mapped yet. Such new fields will need to be dynamically added to their parent according to its dynamic behaviour.
+     * Holds a flat set of object mappers, meaning that an object field named <code>foo.bar</code> can be looked up directly with its
+     * dotted name.
+     */
+    final ObjectMapper getDynamicObjectMapper(String name) {
         return dynamicObjectMappers.get(name);
     }
 
@@ -259,7 +283,9 @@ public final void addDynamicRuntimeField(RuntimeField runtimeField) {
     }
 
     /**
-     * Get dynamic runtime fields created while parsing.
+     * Get dynamic runtime fields created while parsing. Holds a flat set of {@link RuntimeField}s.
+     * Runtime fields get dynamically mapped when {@link org.elasticsearch.index.mapper.ObjectMapper.Dynamic#RUNTIME} is used,
+     * or when dynamic templates specify a <code>runtime</code> section.
      */
     public final List<RuntimeField> getDynamicRuntimeFields() {
         return Collections.unmodifiableList(dynamicRuntimeFields);

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

@@ -102,14 +102,14 @@ Builder addMappers(Map<String, Mapper> mappers) {
         }
 
         /**
-         * Adds a dynamically created Mapper to this builder.
+         * Adds a dynamically created {@link Mapper} to this builder.
          *
          * @param name      the name of the Mapper, including object prefixes
          * @param prefix    the object prefix of this mapper
          * @param mapper    the mapper to add
          * @param context   the DocumentParserContext in which the mapper has been built
          */
-        public void addDynamic(String name, String prefix, Mapper mapper, DocumentParserContext context) {
+        public final void addDynamic(String name, String prefix, Mapper mapper, DocumentParserContext context) {
             // If the mapper to add has no dots, or the current object mapper has subobjects set to false,
             // we just add it as it is for sure a leaf mapper
             if (name.contains(".") == false || subobjects.value() == false) {
@@ -118,29 +118,29 @@ public void addDynamic(String name, String prefix, Mapper mapper, DocumentParser
             // otherwise we strip off the first object path of the mapper name, load or create
             // the relevant object mapper, and then recurse down into it, passing the remainder
             // of the mapper name. So for a mapper 'foo.bar.baz', we locate 'foo' and then
-            // call addDynamic on it with the name 'bar.baz'.
+            // call addDynamic on it with the name 'bar.baz', and next call addDynamic on 'bar' with the name 'baz'.
             else {
                 int firstDotIndex = name.indexOf(".");
-                String childName = name.substring(0, firstDotIndex);
-                String fullChildName = prefix == null ? childName : prefix + "." + childName;
-                ObjectMapper.Builder childBuilder = findChild(fullChildName, context);
-                childBuilder.addDynamic(name.substring(firstDotIndex + 1), fullChildName, mapper, context);
-                add(childBuilder);
+                String immediateChild = name.substring(0, firstDotIndex);
+                String immediateChildFullName = prefix == null ? immediateChild : prefix + "." + immediateChild;
+                ObjectMapper.Builder parentBuilder = findObjectBuilder(immediateChildFullName, context);
+                parentBuilder.addDynamic(name.substring(firstDotIndex + 1), immediateChildFullName, mapper, context);
+                add(parentBuilder);
             }
         }
 
-        private static ObjectMapper.Builder findChild(String fullChildName, DocumentParserContext context) {
-            // does the child mapper already exist? if so, use that
-            ObjectMapper child = context.mappingLookup().objectMappers().get(fullChildName);
-            if (child != null) {
-                return child.newBuilder(context.indexSettings().getIndexVersionCreated());
+        private static ObjectMapper.Builder findObjectBuilder(String fullName, DocumentParserContext context) {
+            // does the object mapper already exist? if so, use that
+            ObjectMapper objectMapper = context.mappingLookup().objectMappers().get(fullName);
+            if (objectMapper != null) {
+                return objectMapper.newBuilder(context.indexSettings().getIndexVersionCreated());
             }
-            // has the child mapper been added as a dynamic update already?
-            child = context.getDynamicObjectMapper(fullChildName);
-            if (child != null) {
-                return child.newBuilder(context.indexSettings().getIndexVersionCreated());
+            // has the object mapper been added as a dynamic update already?
+            objectMapper = context.getDynamicObjectMapper(fullName);
+            if (objectMapper != null) {
+                return objectMapper.newBuilder(context.indexSettings().getIndexVersionCreated());
             }
-            throw new IllegalArgumentException("Missing intermediate object " + fullChildName);
+            throw new IllegalStateException("Missing intermediate object " + fullName);
         }
 
         protected final Map<String, Mapper> buildMappers(boolean root, MapperBuilderContext context) {

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

@@ -734,6 +734,20 @@ public void testObjectMappingUpdate() throws Exception {
         assertNotNull(((ObjectMapper) barMapper).getMapper("baz"));
     }
 
+    public void testEmptyObjectGetsMapped() throws Exception {
+        MapperService mapperService = createMapperService();
+        DocumentMapper docMapper = mapperService.documentMapper();
+        ParsedDocument doc = docMapper.parse(source(b -> {
+            b.startObject("foo");
+            b.endObject();
+        }));
+        Mapping mapping = doc.dynamicMappingsUpdate();
+        assertNotNull(mapping);
+        Mapper foo = mapping.getRoot().getMapper("foo");
+        assertThat(foo, instanceOf(ObjectMapper.class));
+        assertEquals(0, ((ObjectMapper) foo).mappers.size());
+    }
+
     public void testDynamicGeoPointArrayWithTemplate() throws Exception {
         DocumentMapper mapper = createDocumentMapper(topMapping(b -> {
             b.startArray("dynamic_templates");