From 261518ec1b0d5e42b6465c2901f85f18dde3f589 Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Tue, 21 May 2024 17:47:38 +0300 Subject: [PATCH 1/8] Fixing mapping merge for fields named properties --- .../index/mapper/MapperService.java | 7 +- .../index/mapper/MapperServiceTests.java | 100 +++++++++++++++++- 2 files changed, 104 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index 19f8da0954c5d..efc820106c9bc 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -480,8 +480,11 @@ public Object merge(String parent, String key, Object oldValue, Object newValue) if (baseMap.containsKey("subobjects")) { mergedMappings.put("subobjects", baseMap.get("subobjects")); } - // recursively merge these two field mappings - XContentHelper.merge(key, mergedMappings, mapToMerge, INSTANCE); + // Recursively merge these two field mappings. + // Since "key" is an arbitrary field name, no need to pass it to the recursion as it shouldn't affect the merge + // logic. Specifically, passing "null" as parent here avoids merge failures of fields named "properties". See + // https://github.com/elastic/elasticsearch/issues/108866 + XContentHelper.merge(null, mergedMappings, mapToMerge, INSTANCE); return mergedMappings; } else { // non-mergeable types - replace the entire mapping subtree for this field diff --git a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java index 0a49907b25567..9f25dee28b67c 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java @@ -1147,7 +1147,7 @@ public void testMultipleTypeMerges() throws IOException { }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); } - public void testPropertiesField() throws IOException { + public void testPropertiesFieldSingleChildMerge() throws IOException { CompressedXContent mapping1 = new CompressedXContent(""" { "properties": { @@ -1238,6 +1238,104 @@ public void testPropertiesField() throws IOException { assertEquals("keyword", grandchildMapper.typeName()); } + public void testPropertiesFieldMultiChildMerge() throws IOException { + CompressedXContent mapping1 = new CompressedXContent(""" + { + "properties": { + "properties": { + "properties": { + "child1": { + "type": "text", + "fields": { + "keyword": { + "type": "keyword" + } + } + }, + "child2": { + "type": "text" + }, + "child3": { + "properties": { + "grandchild": { + "type": "text" + } + } + } + } + } + } + }"""); + + CompressedXContent mapping2 = new CompressedXContent(""" + { + "properties": { + "properties": { + "properties": { + "child2": { + "type": "integer" + }, + "child3": { + "properties": { + "grandchild": { + "type": "long" + } + } + } + } + } + } + }"""); + + MapperService mapperService = createMapperService(mapping(b -> { + })); + mapperService.merge("_doc", List.of(mapping1, mapping2), MergeReason.INDEX_TEMPLATE); + assertEquals(""" + { + "_doc" : { + "properties" : { + "properties" : { + "properties" : { + "child1" : { + "type" : "text", + "fields" : { + "keyword" : { + "type" : "keyword" + } + } + }, + "child2" : { + "type" : "integer" + }, + "child3" : { + "properties" : { + "grandchild" : { + "type" : "long" + } + } + } + } + } + } + } + }""", Strings.toString(mapperService.documentMapper().mapping(), true, true)); + + Mapper propertiesMapper = mapperService.documentMapper().mapping().getRoot().getMapper("properties"); + assertThat(propertiesMapper, instanceOf(ObjectMapper.class)); + Mapper childMapper = ((ObjectMapper) propertiesMapper).getMapper("child1"); + assertThat(childMapper, instanceOf(FieldMapper.class)); + assertEquals("text", childMapper.typeName()); + assertEquals(2, childMapper.getTotalFieldsCount()); + childMapper = ((ObjectMapper) propertiesMapper).getMapper("child2"); + assertThat(childMapper, instanceOf(FieldMapper.class)); + assertEquals("integer", childMapper.typeName()); + assertEquals(1, childMapper.getTotalFieldsCount()); + childMapper = ((ObjectMapper) propertiesMapper).getMapper("child3"); + assertThat(childMapper, instanceOf(ObjectMapper.class)); + Mapper grandchildMapper = ((ObjectMapper) childMapper).getMapper("grandchild"); + assertEquals("long", grandchildMapper.typeName()); + } + public void testMergeUntilLimit() throws IOException { CompressedXContent mapping1 = new CompressedXContent(""" { From b95f791331e245551195b36ad311aff7b6a804e2 Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Tue, 21 May 2024 17:52:28 +0300 Subject: [PATCH 2/8] refer in comment to the related bug --- .../main/java/org/elasticsearch/index/mapper/MapperService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index efc820106c9bc..654da8764fb0d 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -483,7 +483,7 @@ public Object merge(String parent, String key, Object oldValue, Object newValue) // Recursively merge these two field mappings. // Since "key" is an arbitrary field name, no need to pass it to the recursion as it shouldn't affect the merge // logic. Specifically, passing "null" as parent here avoids merge failures of fields named "properties". See - // https://github.com/elastic/elasticsearch/issues/108866 + // https://github.com/elastic/elasticsearch/issues/108866 XContentHelper.merge(null, mergedMappings, mapToMerge, INSTANCE); return mergedMappings; } else { From 682f2f83d98d23f83b3dddf4ede527fb965e11d6 Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Tue, 21 May 2024 17:57:38 +0300 Subject: [PATCH 3/8] Update docs/changelog/108867.yaml --- docs/changelog/108867.yaml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 docs/changelog/108867.yaml diff --git a/docs/changelog/108867.yaml b/docs/changelog/108867.yaml new file mode 100644 index 0000000000000..81d9c03162148 --- /dev/null +++ b/docs/changelog/108867.yaml @@ -0,0 +1,6 @@ +pr: 108867 +summary: Raw mapping merge fix for properties field +area: Mapping +type: bug +issues: + - 108866 From 1c742ad53efe53cbb6277da39320c1d2b0c681b7 Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Wed, 22 May 2024 08:40:42 +0300 Subject: [PATCH 4/8] Spotless --- .../org/elasticsearch/index/mapper/MapperServiceTests.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java index 9f25dee28b67c..bb5d50267642f 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java @@ -1287,8 +1287,7 @@ public void testPropertiesFieldMultiChildMerge() throws IOException { } }"""); - MapperService mapperService = createMapperService(mapping(b -> { - })); + MapperService mapperService = createMapperService(mapping(b -> {})); mapperService.merge("_doc", List.of(mapping1, mapping2), MergeReason.INDEX_TEMPLATE); assertEquals(""" { From 0f903f1476a2eadf00e8523c03d9b73fba51c4c8 Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Wed, 22 May 2024 08:43:49 +0300 Subject: [PATCH 5/8] Summary --- docs/changelog/108867.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/changelog/108867.yaml b/docs/changelog/108867.yaml index 81d9c03162148..545349dd84aeb 100644 --- a/docs/changelog/108867.yaml +++ b/docs/changelog/108867.yaml @@ -1,5 +1,5 @@ pr: 108867 -summary: Raw mapping merge fix for properties field +summary: Fix for raw mapping merge of fields named "properties" area: Mapping type: bug issues: From c06927376d05c0a9346a2c4170cdbb980137d4e1 Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Mon, 27 May 2024 09:54:30 +0300 Subject: [PATCH 6/8] Switching to use the accurate merge API --- .../org/elasticsearch/common/xcontent/XContentHelper.java | 2 +- .../org/elasticsearch/index/mapper/MapperService.java | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/xcontent/XContentHelper.java b/server/src/main/java/org/elasticsearch/common/xcontent/XContentHelper.java index b08e0d221cf77..9998cb55064e3 100644 --- a/server/src/main/java/org/elasticsearch/common/xcontent/XContentHelper.java +++ b/server/src/main/java/org/elasticsearch/common/xcontent/XContentHelper.java @@ -415,7 +415,7 @@ public static void mergeDefaults(Map content, Map Date: Mon, 27 May 2024 10:20:29 +0300 Subject: [PATCH 7/8] spotful --- .../main/java/org/elasticsearch/index/mapper/MapperService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index 7cb71e7cb0299..999873aa39300 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -483,7 +483,7 @@ public Object merge(String parent, String key, Object oldValue, Object newValue) // Recursively merge these two field mappings. // Since "key" is an arbitrary field name, for which we only need plain mapping subtrees merge, no need to pass it // to the recursion as it shouldn't affect the merge logic. Specifically, passing "null" as parent avoids merge - // failures of fields named "properties". See https://github.com/elastic/elasticsearch/issues/108866 + // failures of fields named "properties". See https://github.com/elastic/elasticsearch/issues/108866 XContentHelper.merge(mergedMappings, mapToMerge, INSTANCE); return mergedMappings; } else { From 4cbcb68de8debf1a536045cb02daf7821e13edd5 Mon Sep 17 00:00:00 2001 From: eyalkoren <41850454+eyalkoren@users.noreply.github.com> Date: Mon, 27 May 2024 10:23:11 +0300 Subject: [PATCH 8/8] Fixing comment --- .../main/java/org/elasticsearch/index/mapper/MapperService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java index 999873aa39300..d3665c3b978bd 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MapperService.java @@ -482,7 +482,7 @@ public Object merge(String parent, String key, Object oldValue, Object newValue) } // Recursively merge these two field mappings. // Since "key" is an arbitrary field name, for which we only need plain mapping subtrees merge, no need to pass it - // to the recursion as it shouldn't affect the merge logic. Specifically, passing "null" as parent avoids merge + // to the recursion as it shouldn't affect the merge logic. Specifically, passing a parent may cause merge // failures of fields named "properties". See https://github.com/elastic/elasticsearch/issues/108866 XContentHelper.merge(mergedMappings, mapToMerge, INSTANCE); return mergedMappings;