From c840ea38122c8c5f5759617a86a4ac0cf7a18c59 Mon Sep 17 00:00:00 2001 From: Kostas Krikellas <131142368+kkrik-es@users.noreply.github.com> Date: Wed, 2 Oct 2024 18:21:54 +0300 Subject: [PATCH] Remove special handling for objects and arrays with `dynamic` overrides (#113924) * Remove special handling for objects and arrays with `dynamic` overrides * add test * add test --- .../indices.create/20_synthetic_source.yml | 22 ++-- .../index/mapper/DocumentParser.java | 22 ++-- .../mapper/IgnoredSourceFieldMapperTests.java | 107 +++++++++++++++++- 3 files changed, 127 insertions(+), 24 deletions(-) diff --git a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.create/20_synthetic_source.yml b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.create/20_synthetic_source.yml index b5a9146bc54a6..a999bb7816065 100644 --- a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.create/20_synthetic_source.yml +++ b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/indices.create/20_synthetic_source.yml @@ -535,6 +535,8 @@ object array in object with dynamic override: _source: mode: synthetic properties: + id: + type: integer path_no: dynamic: false properties: @@ -552,19 +554,25 @@ object array in object with dynamic override: refresh: true body: - '{ "create": { } }' - - '{ "path_no": [ { "some_int": 10 }, {"name": "foo"} ], "path_runtime": [ { "some_int": 20 }, {"name": "bar"} ], "name": "baz" }' + - '{ "id": 1, "path_no": [ { "some_int": 30 }, {"name": "baz"}, { "some_int": 20 }, {"name": "bar"} ], "name": "A" }' + - '{ "create": { } }' + - '{ "id": 2, "path_runtime": [ { "some_int": 30 }, {"name": "baz"}, { "some_int": 20 }, {"name": "bar"} ], "name": "B" }' - match: { errors: false } - do: search: index: test + sort: id - - match: { hits.total.value: 1 } - - match: { hits.hits.0._source.name: baz } - - match: { hits.hits.0._source.path_no.0.some_int: 10 } - - match: { hits.hits.0._source.path_no.1.name: foo } - - match: { hits.hits.0._source.path_runtime.0.some_int: 20 } - - match: { hits.hits.0._source.path_runtime.1.name: bar } + - match: { hits.hits.0._source.id: 1 } + - match: { hits.hits.0._source.name: A } + - match: { hits.hits.0._source.path_no.some_int: [ 30, 20 ] } + - match: { hits.hits.0._source.path_no.name: [ bar, baz ] } + + - match: { hits.hits.1._source.id: 2 } + - match: { hits.hits.1._source.name: B } + - match: { hits.hits.1._source.path_runtime.some_int: [ 30, 20 ] } + - match: { hits.hits.1._source.path_runtime.name: [ bar, baz ] } --- 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 ebe9f27f461cf..c82621baa717a 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java @@ -401,7 +401,7 @@ static void parseObjectOrNested(DocumentParserContext context) throws IOExceptio context.addIgnoredField( new IgnoredSourceFieldMapper.NameValue( context.parent().fullPath(), - context.parent().fullPath().lastIndexOf(currentFieldName), + context.parent().fullPath().lastIndexOf(context.parent().leafName()), XContentDataHelper.encodeToken(parser), context.doc() ) @@ -803,27 +803,25 @@ private static void parseNonDynamicArray( boolean objectRequiresStoringSource = mapper instanceof ObjectMapper objectMapper && (objectMapper.storeArraySource() || (context.sourceKeepModeFromIndexSettings() == Mapper.SourceKeepMode.ARRAYS - && objectMapper instanceof NestedObjectMapper == false) - || objectMapper.dynamic == ObjectMapper.Dynamic.RUNTIME); + && objectMapper instanceof NestedObjectMapper == false)); boolean fieldWithFallbackSyntheticSource = mapper instanceof FieldMapper fieldMapper && fieldMapper.syntheticSourceMode() == FieldMapper.SyntheticSourceMode.FALLBACK; boolean fieldWithStoredArraySource = mapper instanceof FieldMapper fieldMapper && getSourceKeepMode(context, fieldMapper.sourceKeepMode()) != Mapper.SourceKeepMode.NONE; - boolean dynamicRuntimeContext = context.dynamic() == ObjectMapper.Dynamic.RUNTIME; boolean copyToFieldHasValuesInDocument = context.isWithinCopyTo() == false && context.isCopyToDestinationField(fullPath); if (objectRequiresStoringSource || fieldWithFallbackSyntheticSource - || dynamicRuntimeContext || fieldWithStoredArraySource || copyToFieldHasValuesInDocument) { context = context.addIgnoredFieldFromContext(IgnoredSourceFieldMapper.NameValue.fromContext(context, fullPath, null)); - } else if (mapper instanceof ObjectMapper objectMapper - && (objectMapper.isEnabled() == false || objectMapper.dynamic == ObjectMapper.Dynamic.FALSE)) { - context.addIgnoredField( - IgnoredSourceFieldMapper.NameValue.fromContext(context, fullPath, XContentDataHelper.encodeToken(context.parser())) - ); - return; - } + } else if (mapper instanceof ObjectMapper objectMapper && (objectMapper.isEnabled() == false)) { + // No need to call #addIgnoredFieldFromContext as both singleton and array instances of this object + // get tracked through ignored source. + context.addIgnoredField( + IgnoredSourceFieldMapper.NameValue.fromContext(context, fullPath, XContentDataHelper.encodeToken(context.parser())) + ); + return; + } } // In synthetic source, if any array element requires storing its source as-is, it takes precedence over diff --git a/server/src/test/java/org/elasticsearch/index/mapper/IgnoredSourceFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/IgnoredSourceFieldMapperTests.java index eaa7bf6528203..4bc33558e104b 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/IgnoredSourceFieldMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/IgnoredSourceFieldMapperTests.java @@ -902,7 +902,6 @@ public void testObjectArrayAndValue() throws IOException { } b.endObject(); })).documentMapper(); - // { "path": [ { "stored":[ { "leaf": 10 } ] }, { "stored": { "leaf": 20 } } ] } var syntheticSource = syntheticSource(documentMapper, b -> { b.startArray("path"); { @@ -927,6 +926,91 @@ public void testObjectArrayAndValue() throws IOException { {"path":{"stored":[{"leaf":10},{"leaf":20}]}}""", syntheticSource); } + public void testObjectArrayAndValueDisabledObject() throws IOException { + DocumentMapper documentMapper = createMapperService(syntheticSourceMapping(b -> { + b.startObject("path").field("type", "object").startObject("properties"); + { + b.startObject("regular"); + { + b.startObject("properties").startObject("leaf").field("type", "integer").endObject().endObject(); + } + b.endObject(); + b.startObject("disabled").field("type", "object").field("enabled", false); + { + b.startObject("properties").startObject("leaf").field("type", "integer").endObject().endObject(); + } + b.endObject(); + } + b.endObject().endObject(); + })).documentMapper(); + var syntheticSource = syntheticSource(documentMapper, b -> { + b.startArray("path"); + { + b.startObject().startArray("disabled").startObject().field("leaf", 10).endObject().endArray().endObject(); + b.startObject().startObject("disabled").field("leaf", 20).endObject().endObject(); + b.startObject().startArray("regular").startObject().field("leaf", 10).endObject().endArray().endObject(); + b.startObject().startObject("regular").field("leaf", 20).endObject().endObject(); + } + b.endArray(); + }); + assertEquals(""" + {"path":{"disabled":[{"leaf":10},{"leaf":20}],"regular":{"leaf":[10,20]}}}""", syntheticSource); + } + + public void testObjectArrayAndValueNonDynamicObject() throws IOException { + DocumentMapper documentMapper = createMapperService(syntheticSourceMapping(b -> { + b.startObject("path").field("type", "object").startObject("properties"); + { + b.startObject("regular"); + { + b.startObject("properties").startObject("leaf").field("type", "integer").endObject().endObject(); + } + b.endObject(); + b.startObject("disabled").field("type", "object").field("dynamic", "false").endObject(); + } + b.endObject().endObject(); + })).documentMapper(); + var syntheticSource = syntheticSource(documentMapper, b -> { + b.startArray("path"); + { + b.startObject().startArray("disabled").startObject().field("leaf", 10).endObject().endArray().endObject(); + b.startObject().startObject("disabled").field("leaf", 20).endObject().endObject(); + b.startObject().startArray("regular").startObject().field("leaf", 10).endObject().endArray().endObject(); + b.startObject().startObject("regular").field("leaf", 20).endObject().endObject(); + } + b.endArray(); + }); + assertEquals(""" + {"path":{"disabled":{"leaf":[10,20]},"regular":{"leaf":[10,20]}}}""", syntheticSource); + } + + public void testObjectArrayAndValueDynamicRuntimeObject() throws IOException { + DocumentMapper documentMapper = createMapperService(syntheticSourceMapping(b -> { + b.startObject("path").field("type", "object").startObject("properties"); + { + b.startObject("regular"); + { + b.startObject("properties").startObject("leaf").field("type", "integer").endObject().endObject(); + } + b.endObject(); + b.startObject("runtime").field("type", "object").field("dynamic", "runtime").endObject(); + } + b.endObject().endObject(); + })).documentMapper(); + var syntheticSource = syntheticSource(documentMapper, b -> { + b.startArray("path"); + { + b.startObject().startArray("runtime").startObject().field("leaf", 10).endObject().endArray().endObject(); + b.startObject().startObject("runtime").field("leaf", 20).endObject().endObject(); + b.startObject().startArray("regular").startObject().field("leaf", 10).endObject().endArray().endObject(); + b.startObject().startObject("regular").field("leaf", 20).endObject().endObject(); + } + b.endArray(); + }); + assertEquals(""" + {"path":{"regular":{"leaf":[10,20]},"runtime":{"leaf":[10,20]}}}""", syntheticSource); + } + public void testDisabledObjectWithinHigherLevelArray() throws IOException { DocumentMapper documentMapper = createMapperService(syntheticSourceMapping(b -> { b.startObject("path"); @@ -1337,7 +1421,7 @@ public void testNoDynamicObjectSimpleArray() throws IOException { b.endArray(); }); assertEquals(""" - {"path":[{"name":"foo"},{"name":"bar"}]}""", syntheticSource); + {"path":{"name":["foo","bar"]}}""", syntheticSource); } public void testNoDynamicObjectSimpleValueArray() throws IOException { @@ -1365,7 +1449,20 @@ public void testNoDynamicObjectNestedArray() throws IOException { b.endArray(); }); assertEquals(""" - {"path":[{"to":{"foo":"A","bar":"B"}},{"to":{"foo":"C","bar":"D"}}]}""", syntheticSource); + {"path":{"to":[{"foo":"A","bar":"B"},{"foo":"C","bar":"D"}]}}""", syntheticSource); + } + + public void testNoDynamicRootObject() throws IOException { + DocumentMapper documentMapper = createMapperService(topMapping(b -> { + b.startObject("_source").field("mode", "synthetic").endObject().field("dynamic", "false"); + })).documentMapper(); + var syntheticSource = syntheticSource(documentMapper, b -> { + b.field("foo", "bar"); + b.startObject("path").field("X", "Y").endObject(); + b.array("name", "A", "D", "C", "B"); + }); + assertEquals(""" + {"foo":"bar","name":["A","D","C","B"],"path":{"X":"Y"}}""", syntheticSource); } public void testRuntimeDynamicObjectSingleField() throws IOException { @@ -1445,7 +1542,7 @@ public void testRuntimeDynamicObjectSimpleArray() throws IOException { b.endArray(); }); assertEquals(""" - {"path":[{"name":"foo"},{"name":"bar"}]}""", syntheticSource); + {"path":{"name":["foo","bar"]}}""", syntheticSource); } public void testRuntimeDynamicObjectSimpleValueArray() throws IOException { @@ -1473,7 +1570,7 @@ public void testRuntimeDynamicObjectNestedArray() throws IOException { b.endArray(); }); assertEquals(""" - {"path":[{"to":{"foo":"A","bar":"B"}},{"to":{"foo":"C","bar":"D"}}]}""", syntheticSource); + {"path":{"to":[{"foo":"A","bar":"B"},{"foo":"C","bar":"D"}]}}""", syntheticSource); } public void testDisabledSubObjectWithNameOverlappingParentName() throws IOException {