Skip to content

Commit 9eefc03

Browse files
javannajavanna
authored
Assign the right path to objects merged when parsing mappings (#89389)
When parsing mappings, we may find a field with same name specified twice, either because JSON duplicate keys are allowed, or because a mix of object notation and dot notation is used when providing mappings. The same can happen when applying dynamic mappings as part of parsing an incoming document, as well as when merging separate index templates that may contain the definition for the same field using a mix of object notation and dot notation. While we propagate the MapperBuilderContext across merge calls thanks to #86946, we do not propagate the right context when we call merge on objects as part of parsing/building mappings. This causes a situation in which the leaf fields that result from the merge have the wrong path, which misses the first portion e.g. sub.field instead of obj.sub.field. This commit applies the correct mapper builder context when building the object mapper builder and two objects with same name are found. Relates to #86946 Closes #88573
1 parent e8f66e5 commit 9eefc03

File tree

9 files changed

+318
-21
lines changed

9 files changed

+318
-21
lines changed

‎docs/changelog/89389.yaml‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
pr: 89389
2+
summary: Assign the right path to objects merged when parsing mappings
3+
area: Mapping
4+
type: bug
5+
issues:
6+
- 88573

‎server/src/internalClusterTest/java/org/elasticsearch/indices/template/SimpleIndexTemplateIT.java‎

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@
1414
import org.elasticsearch.action.admin.indices.template.get.GetIndexTemplatesResponse;
1515
import org.elasticsearch.action.admin.indices.template.put.PutIndexTemplateRequestBuilder;
1616
import org.elasticsearch.action.bulk.BulkResponse;
17+
import org.elasticsearch.action.fieldcaps.FieldCapabilities;
18+
import org.elasticsearch.action.fieldcaps.FieldCapabilitiesResponse;
1719
import org.elasticsearch.action.index.IndexRequest;
1820
import org.elasticsearch.action.search.SearchResponse;
1921
import org.elasticsearch.cluster.ClusterState;
@@ -39,6 +41,7 @@
3941
import java.util.Collections;
4042
import java.util.HashSet;
4143
import java.util.List;
44+
import java.util.Map;
4245
import java.util.Set;
4346

4447
import static org.elasticsearch.action.support.WriteRequest.RefreshPolicy.IMMEDIATE;
@@ -1009,4 +1012,88 @@ public void testPartitionedTemplate() throws Exception {
10091012
GetSettingsResponse getSettingsResponse = client().admin().indices().prepareGetSettings("test_good").get();
10101013
assertEquals("6", getSettingsResponse.getIndexToSettings().get("test_good").get("index.routing_partition_size"));
10111014
}
1015+
1016+
public void testIndexTemplatesWithSameSubfield() {
1017+
client().admin()
1018+
.indices()
1019+
.preparePutTemplate("template_1")
1020+
.setPatterns(Collections.singletonList("te*"))
1021+
.setSettings(indexSettings())
1022+
.setOrder(100)
1023+
.setMapping("""
1024+
{
1025+
"_doc": {
1026+
"properties": {
1027+
"kwm": {
1028+
"properties": {
1029+
"source": {
1030+
"properties": {
1031+
"geo": {
1032+
"properties": {
1033+
"location": {
1034+
"type": "geo_point"
1035+
}
1036+
}
1037+
}
1038+
}
1039+
}
1040+
}
1041+
},
1042+
"source": {
1043+
"properties": {
1044+
"geo": {
1045+
"properties": {
1046+
"location": {
1047+
"type": "geo_point"
1048+
}
1049+
}
1050+
}
1051+
}
1052+
}
1053+
}
1054+
}
1055+
}
1056+
""", XContentType.JSON)
1057+
.get();
1058+
1059+
client().admin()
1060+
.indices()
1061+
.preparePutTemplate("template_2")
1062+
.setPatterns(Collections.singletonList("test*"))
1063+
.setSettings(indexSettings())
1064+
.setOrder(1)
1065+
.setMapping("""
1066+
{
1067+
"_doc": {
1068+
"properties": {
1069+
"kwm.source.geo": {
1070+
"properties": {
1071+
"location": {
1072+
"type": "geo_point"
1073+
}
1074+
}
1075+
}
1076+
}
1077+
}
1078+
}
1079+
""", XContentType.JSON)
1080+
.get();
1081+
1082+
client().prepareIndex("test").setSource().get();
1083+
FieldCapabilitiesResponse fieldCapabilitiesResponse = client().prepareFieldCaps("test").setFields("*location").get();
1084+
{
1085+
Map<String, FieldCapabilities> field = fieldCapabilitiesResponse.getField("kwm.source.geo.location");
1086+
assertNotNull(field);
1087+
FieldCapabilities fieldCapabilities = field.get("geo_point");
1088+
assertTrue(fieldCapabilities.isSearchable());
1089+
assertTrue(fieldCapabilities.isAggregatable());
1090+
}
1091+
{
1092+
Map<String, FieldCapabilities> field = fieldCapabilitiesResponse.getField("source.geo.location");
1093+
assertNotNull(field);
1094+
FieldCapabilities fieldCapabilities = field.get("geo_point");
1095+
assertTrue(fieldCapabilities.isSearchable());
1096+
assertTrue(fieldCapabilities.isAggregatable());
1097+
}
1098+
}
10121099
}

‎server/src/main/java/org/elasticsearch/index/mapper/NestedObjectMapper.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -170,7 +170,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
170170
}
171171

172172
@Override
173-
public ObjectMapper merge(Mapper mergeWith, MapperService.MergeReason reason, MapperBuilderContext mapperBuilderContext) {
173+
public ObjectMapper merge(Mapper mergeWith, MapperService.MergeReason reason, MapperBuilderContext parentBuilderContext) {
174174
if ((mergeWith instanceof NestedObjectMapper) == false) {
175175
throw new IllegalArgumentException("can't merge a non nested mapping [" + mergeWith.name() + "] with a nested mapping");
176176
}
@@ -191,7 +191,7 @@ public ObjectMapper merge(Mapper mergeWith, MapperService.MergeReason reason, Ma
191191
throw new MapperException("the [include_in_root] parameter can't be updated on a nested object mapping");
192192
}
193193
}
194-
toMerge.doMerge(mergeWithObject, reason, mapperBuilderContext);
194+
toMerge.doMerge(mergeWithObject, reason, parentBuilderContext);
195195
return toMerge;
196196
}
197197

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

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,11 @@ protected final Map<String, Mapper> buildMappers(boolean root, MapperBuilderCont
150150
assert mapper instanceof ObjectMapper == false || subobjects.value() : "unexpected object while subobjects are disabled";
151151
Mapper existing = mappers.get(mapper.simpleName());
152152
if (existing != null) {
153+
// The same mappings or document may hold the same field twice, either because duplicated JSON keys are allowed or
154+
// the same field is provided using the object notation as well as the dot notation at the same time.
155+
// This can also happen due to multiple index templates being merged into a single mappings definition using
156+
// XContentHelper#mergeDefaults, again in case some index templates contained mappings for the same field using a
157+
// mix of object notation and dot notation.
153158
mapper = existing.merge(mapper, mapperBuilderContext);
154159
}
155160
mappers.put(mapper.simpleName(), mapper);
@@ -426,7 +431,11 @@ public void validate(MappingLookup mappers) {
426431
}
427432
}
428433

429-
public ObjectMapper merge(Mapper mergeWith, MergeReason reason, MapperBuilderContext mapperBuilderContext) {
434+
protected MapperBuilderContext createChildContext(MapperBuilderContext mapperBuilderContext, String name) {
435+
return mapperBuilderContext.createChildContext(name);
436+
}
437+
438+
public ObjectMapper merge(Mapper mergeWith, MergeReason reason, MapperBuilderContext parentBuilderContext) {
430439
if ((mergeWith instanceof ObjectMapper) == false) {
431440
throw new IllegalArgumentException("can't merge a non object mapping [" + mergeWith.name() + "] with an object mapping");
432441
}
@@ -436,12 +445,11 @@ public ObjectMapper merge(Mapper mergeWith, MergeReason reason, MapperBuilderCon
436445
}
437446
ObjectMapper mergeWithObject = (ObjectMapper) mergeWith;
438447
ObjectMapper merged = clone();
439-
merged.doMerge(mergeWithObject, reason, mapperBuilderContext);
448+
merged.doMerge(mergeWithObject, reason, parentBuilderContext);
440449
return merged;
441450
}
442451

443-
protected void doMerge(final ObjectMapper mergeWith, MergeReason reason, MapperBuilderContext mapperBuilderContext) {
444-
452+
protected void doMerge(final ObjectMapper mergeWith, MergeReason reason, MapperBuilderContext parentBuilderContext) {
445453
if (mergeWith.dynamic != null) {
446454
this.dynamic = mergeWith.dynamic;
447455
}
@@ -462,6 +470,7 @@ protected void doMerge(final ObjectMapper mergeWith, MergeReason reason, MapperB
462470
}
463471
}
464472

473+
MapperBuilderContext objectBuilderContext = createChildContext(parentBuilderContext, simpleName());
465474
Map<String, Mapper> mergedMappers = null;
466475
for (Mapper mergeWithMapper : mergeWith) {
467476
Mapper mergeIntoMapper = (mergedMappers == null ? mappers : mergedMappers).get(mergeWithMapper.simpleName());
@@ -470,8 +479,7 @@ protected void doMerge(final ObjectMapper mergeWith, MergeReason reason, MapperB
470479
if (mergeIntoMapper == null) {
471480
merged = mergeWithMapper;
472481
} else if (mergeIntoMapper instanceof ObjectMapper objectMapper) {
473-
MapperBuilderContext childContext = mapperBuilderContext.createChildContext(objectMapper.simpleName());
474-
merged = objectMapper.merge(mergeWithMapper, reason, childContext);
482+
merged = objectMapper.merge(mergeWithMapper, reason, objectBuilderContext);
475483
} else {
476484
assert mergeIntoMapper instanceof FieldMapper || mergeIntoMapper instanceof FieldAliasMapper;
477485
if (mergeWithMapper instanceof ObjectMapper) {
@@ -485,7 +493,7 @@ protected void doMerge(final ObjectMapper mergeWith, MergeReason reason, MapperB
485493
if (reason == MergeReason.INDEX_TEMPLATE) {
486494
merged = mergeWithMapper;
487495
} else {
488-
merged = mergeIntoMapper.merge(mergeWithMapper, mapperBuilderContext);
496+
merged = mergeIntoMapper.merge(mergeWithMapper, objectBuilderContext);
489497
}
490498
}
491499
if (mergedMappers == null) {

‎server/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -322,13 +322,19 @@ RuntimeField getRuntimeField(String name) {
322322
}
323323

324324
@Override
325-
public RootObjectMapper merge(Mapper mergeWith, MergeReason reason, MapperBuilderContext mapperBuilderContext) {
326-
return (RootObjectMapper) super.merge(mergeWith, reason, mapperBuilderContext);
325+
protected MapperBuilderContext createChildContext(MapperBuilderContext mapperBuilderContext, String name) {
326+
assert mapperBuilderContext == MapperBuilderContext.ROOT;
327+
return mapperBuilderContext;
327328
}
328329

329330
@Override
330-
protected void doMerge(ObjectMapper mergeWith, MergeReason reason, MapperBuilderContext mapperBuilderContext) {
331-
super.doMerge(mergeWith, reason, mapperBuilderContext);
331+
public RootObjectMapper merge(Mapper mergeWith, MergeReason reason, MapperBuilderContext parentBuilderContext) {
332+
return (RootObjectMapper) super.merge(mergeWith, reason, parentBuilderContext);
333+
}
334+
335+
@Override
336+
protected void doMerge(ObjectMapper mergeWith, MergeReason reason, MapperBuilderContext parentBuilderContext) {
337+
super.doMerge(mergeWith, reason, parentBuilderContext);
332338
RootObjectMapper mergeWithObject = (RootObjectMapper) mergeWith;
333339
if (mergeWithObject.numericDetection.explicit()) {
334340
this.numericDetection = mergeWithObject.numericDetection;

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

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
import java.math.BigDecimal;
3434
import java.math.BigInteger;
3535
import java.nio.charset.StandardCharsets;
36+
import java.util.Arrays;
3637
import java.util.Collection;
3738
import java.util.Collections;
3839
import java.util.List;
@@ -2351,6 +2352,61 @@ public void testDocumentDescriptionInTsdb() throws IOException {
23512352
}
23522353
}
23532354

2355+
public void testMergeSubfieldWhileBuildingMappers() throws Exception {
2356+
MapperService mapperService = createMapperService();
2357+
/*
2358+
We had a bug (https://github.com/elastic/elasticsearch/issues/88573) building an object mapper (ObjectMapper.Builder#buildMappers).
2359+
A sub-field that already exists is merged with the existing one. As a result, the leaf field would get the wrong field path
2360+
(missing the first portion of its path). The only way to trigger this scenario for dynamic mappings is to either allow duplicate
2361+
JSON keys or ingest the same field with dots collapsed as well as expanded within the same document. Note that the two fields with
2362+
same name need to be part of the same mappings (hence the same document). If they are in two distinct mappings they are properly
2363+
merged as part of RootObjectMapper#merge.
2364+
*/
2365+
ParsedDocument doc = mapperService.documentMapper().parse(source("""
2366+
{
2367+
"foo" : {
2368+
"bar" : {
2369+
"baz" : 1
2370+
}
2371+
},
2372+
"foo.bar.baz" : 2
2373+
}
2374+
"""));
2375+
Mapping mapping = doc.dynamicMappingsUpdate();
2376+
assertNotNull(mapping);
2377+
Mapper fooMapper = mapping.getRoot().getMapper("foo");
2378+
assertNotNull(fooMapper);
2379+
assertTrue(fooMapper instanceof ObjectMapper);
2380+
Mapper barMapper = ((ObjectMapper) fooMapper).getMapper("bar");
2381+
assertTrue(barMapper instanceof ObjectMapper);
2382+
Mapper baz = ((ObjectMapper) barMapper).getMapper("baz");
2383+
assertNotNull(baz);
2384+
assertEquals("foo.bar.baz", baz.name());
2385+
assertEquals("baz", baz.simpleName());
2386+
IndexableField[] fields = doc.rootDoc().getFields("foo.bar.baz");
2387+
assertEquals(4, fields.length);
2388+
long[] longs = Arrays.stream(fields).mapToLong(value -> value.numericValue().longValue()).toArray();
2389+
assertArrayEquals(new long[] { 1, 1, 2, 2 }, longs);
2390+
2391+
// merge without going through toXContent and reparsing, otherwise the potential leaf path issue gets fixed on its own
2392+
Mapping newMapping = MapperService.mergeMappings(mapperService.documentMapper(), mapping, MapperService.MergeReason.MAPPING_UPDATE);
2393+
DocumentMapper newDocMapper = new DocumentMapper(mapperService.documentParser(), newMapping, newMapping.toCompressedXContent());
2394+
ParsedDocument doc2 = newDocMapper.parse(source("""
2395+
{
2396+
"foo" : {
2397+
"bar" : {
2398+
"baz" : 10
2399+
}
2400+
}
2401+
}
2402+
"""));
2403+
assertNull(doc2.dynamicMappingsUpdate());
2404+
IndexableField[] fields2 = doc2.rootDoc().getFields("foo.bar.baz");
2405+
assertEquals(2, fields2.length);
2406+
long[] longs2 = Arrays.stream(fields2).mapToLong(value -> value.numericValue().longValue()).toArray();
2407+
assertArrayEquals(new long[] { 10, 10 }, longs2);
2408+
}
2409+
23542410
/**
23552411
* Mapper plugin providing a mock metadata field mapper implementation that supports setting its value
23562412
*/

‎server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,10 @@
88

99
package org.elasticsearch.index.mapper;
1010

11+
import org.apache.lucene.index.IndexableField;
1112
import org.elasticsearch.Version;
1213
import org.elasticsearch.cluster.metadata.IndexMetadata;
14+
import org.elasticsearch.common.Strings;
1315
import org.elasticsearch.common.bytes.BytesReference;
1416
import org.elasticsearch.common.compress.CompressedXContent;
1517
import org.elasticsearch.common.settings.Settings;
@@ -353,4 +355,68 @@ public void testMultiFieldChecks() throws IOException {
353355
assertFalse(mapperService.isMultiField("object.subfield1"));
354356
}
355357

358+
public void testMergeObjectSubfieldWhileParsing() throws IOException {
359+
/*
360+
If we are parsing mappings that hold the definition of the same field twice, the two are merged together. This can happen when
361+
mappings have the same field specified using the object notation as well as the dot notation, as well as when applying index
362+
templates, in which case the two definitions may come from separate index templates that end up in the same map (through
363+
XContentHelper#mergeDefaults, see MetadataCreateIndexService#parseV1Mappings).
364+
We had a bug (https://github.com/elastic/elasticsearch/issues/88573) triggered by this scenario that caused the merged leaf fields
365+
to get the wrong path (missing the first portion).
366+
*/
367+
MapperService mapperService = createMapperService("""
368+
{
369+
"_doc": {
370+
"properties": {
371+
"obj": {
372+
"properties": {
373+
"sub": {
374+
"properties": {
375+
"string": {
376+
"type": "keyword"
377+
}
378+
}
379+
}
380+
}
381+
},
382+
"obj.sub.string" : {
383+
"type" : "keyword"
384+
}
385+
}
386+
}
387+
}
388+
""");
389+
390+
assertNotNull(mapperService.mappingLookup().getMapper("obj.sub.string"));
391+
MappedFieldType fieldType = mapperService.mappingLookup().getFieldType("obj.sub.string");
392+
assertNotNull(fieldType);
393+
assertEquals("""
394+
{
395+
"_doc" : {
396+
"properties" : {
397+
"obj" : {
398+
"properties" : {
399+
"sub" : {
400+
"properties" : {
401+
"string" : {
402+
"type" : "keyword"
403+
}
404+
}
405+
}
406+
}
407+
}
408+
}
409+
}
410+
}""", Strings.toString(mapperService.documentMapper().mapping(), true, true));
411+
412+
// check that with the resulting mappings a new document has the previously merged field indexed properly
413+
ParsedDocument parsedDocument = mapperService.documentMapper().parse(source("""
414+
{
415+
"obj.sub.string" : "value"
416+
}"""));
417+
418+
assertNull(parsedDocument.dynamicMappingsUpdate());
419+
IndexableField[] fields = parsedDocument.rootDoc().getFields("obj.sub.string");
420+
assertEquals(2, fields.length);
421+
}
356422
}

0 commit comments

Comments
 (0)