Fix missing intermediate object error when applying dynamic template by javanna · Pull Request #87622 · elastic/elasticsearch

javanna · 2022-06-13T21:08:25Z

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 #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 #87513

elasticmachine · 2022-06-14T10:26:13Z

Pinging @elastic/es-search (Team:Search)

elasticsearchmachine · 2022-06-14T10:26:35Z

Hi @javanna, I've created a changelog YAML for you.

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

javanna · 2022-06-15T13:15:13Z

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

    }

-    private static void parseObject(final DocumentParserContext context, ObjectMapper mapper, String currentFieldName) throws IOException {
+    private static void parseObject(final DocumentParserContext context, ObjectMapper parentObjectMapper, String currentFieldName)


all the changes in this file from now on are just renaming mapper -> parentObjectMapper

javanna · 2022-06-15T13:16:33Z

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

+                return parent.newBuilder(context.indexSettings().getIndexVersionCreated());
            }
-            throw new IllegalArgumentException("Missing intermediate object " + fullChildName);
+            throw new IllegalStateException("Missing intermediate object " + fullName);


this is the only relevant change in this file: IllegalArgumentException -> IllegalStateException . All the rest is renaming child to parent.

romseygeek

Thanks @javanna! I left a couple of comments.

romseygeek · 2022-06-16T08:45:56Z

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

+                String fullParentName = prefix == null ? parentName : prefix + "." + parentName;
+                ObjectMapper.Builder parentBuilder = findParentBuilder(fullParentName, context);
+                parentBuilder.addDynamic(name.substring(firstDotIndex + 1), fullParentName, mapper, context);
+                add(parentBuilder);


I appreciate that child maybe isn't the best name here, but I don't think that parent is very good either. Maybe immediateChild? It's a child of the object mapper that addDynamic is being called on, so I think calling it parent will just confuse things.

++ I see what you mean.

romseygeek · 2022-06-16T08:48:11Z

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

    }
+
+    public void testDynamicSubobject() throws IOException {
+        MapperService mapperService = createMapperService(topMapping(b -> {


Is it worth building these mappings as a text block as well? I think it reads much more easily than xcontentbuilders

yes, the tricky bit is that we define only part of the mappings, and part is defined in topMapping. I would leave this for another time to be honest :)

topMapping is just {"_doc":{ ... } } though. Up to you :)

I'd prefer to spend time on fixing dynamic object mappers lookup in the context etc. over rewriting tests that work fine :)

romseygeek · 2022-06-17T09:50:43Z

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

-            if (child != null) {
-                return child.newBuilder(context.indexSettings().getIndexVersionCreated());
+        private static ObjectMapper.Builder findObjectBuilder(String fullName, DocumentParserContext context) {
+            // does the parent mapper already exist? if so, use that


s/parent/object

romseygeek · 2022-06-17T09:51:01Z

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

-            child = context.getDynamicObjectMapper(fullChildName);
-            if (child != null) {
-                return child.newBuilder(context.indexSettings().getIndexVersionCreated());
+            // has the parent mapper been added as a dynamic update already?


s/parent/object/

romseygeek · 2022-06-17T09:51:56Z

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

    }
+
+    public void testDynamicSubobject() throws IOException {
+        MapperService mapperService = createMapperService(topMapping(b -> {


topMapping is just {"_doc":{ ... } } though. Up to you :)

romseygeek

LGTM, thanks @javanna

…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

…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 #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 #87513

elasticsearchmachine added the v8.4.0 label Jun 13, 2022

javanna force-pushed the fix/dynamic_template_objects branch 2 times, most recently from a3a8981 to f73fa0d Compare June 14, 2022 10:24

javanna changed the title ~~Clean up dynamic mappings~~ Fix missing intermediate object error whil applying dynamic template Jun 14, 2022

javanna changed the title ~~Fix missing intermediate object error whil applying dynamic template~~ Fix missing intermediate object error when applying dynamic template Jun 14, 2022

javanna force-pushed the fix/dynamic_template_objects branch from f73fa0d to eb82193 Compare June 14, 2022 10:25

javanna added v8.3.1 :Search Foundations/Mapping Index mappings, including merging and defining field types >bug labels Jun 14, 2022

javanna marked this pull request as ready for review June 14, 2022 10:26

elasticmachine added the Team:Search Meta label for search team label Jun 14, 2022

javanna force-pushed the fix/dynamic_template_objects branch from 3a55470 to 76f0af5 Compare June 14, 2022 10:28

javanna added 4 commits June 14, 2022 13:27

spotless

29d84c0

Merge branch 'main' into fix/dynamic_template_objects

0a83a88

Merge branch 'main' into fix/dynamic_template_objects

0d823ab

iter

278e903

javanna commented Jun 15, 2022

View reviewed changes

javanna requested a review from romseygeek June 15, 2022 13:39

romseygeek reviewed Jun 16, 2022

View reviewed changes

javanna added 2 commits June 17, 2022 11:36

iter

089f157

iter

c578ade

javanna requested a review from romseygeek June 17, 2022 09:45

romseygeek reviewed Jun 17, 2022

View reviewed changes

iter

e1afb3c

javanna requested a review from romseygeek June 17, 2022 10:37

romseygeek approved these changes Jun 17, 2022

View reviewed changes

javanna merged commit 28085cf into elastic:master Jun 17, 2022

javanna mentioned this pull request Jun 17, 2022

[8.3] Fix missing intermediate object error when applying dynamic template #87801

Merged

Conversation

javanna commented Jun 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Jun 14, 2022

Uh oh!

elasticsearchmachine commented Jun 14, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romseygeek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romseygeek left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

javanna commented Jun 13, 2022 •

edited

Loading