Skip to content

Commit 89fb4d6

Browse files
romseygeekromseygeek
authored
Move nested object redundant include fix into NestedObjectMapper (#92876)
In order to stop deeply nested mappers indexing their content multiple times, we have some logic in RootObjectMapper that finds 'redundant' includes - that is, if a nested object mapper has both 'includeInRoot' and 'includeInParent' set, and its parent is already included in root, then we remove its 'includeInRoot' setting. This is currently done with an explicit fixRedundantIncludes() call directly on the root object mapper after it has been constructed. In the past, bugs have been caused by this call being missed, and it would be neater if this logic was handled internally by NestedObjectMapper itself. This commit adds a new MapperBuilderContext, private to NestedObjectMapper, that holds information about the include status of a nested parent. Fixing of includes is now done entirely within NestedObjectMapper itself, either in the builder or in its merge logic.
1 parent 8f37934 commit 89fb4d6

File tree

7 files changed

+125
-57
lines changed

7 files changed

+125
-57
lines changed

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -232,7 +232,6 @@ static Mapping createDynamicUpdate(DocumentParserContext context) {
232232
rootBuilder.addRuntimeField(runtimeField);
233233
}
234234
RootObjectMapper root = rootBuilder.build(MapperBuilderContext.root(context.mappingLookup().isSourceSynthetic()));
235-
root.fixRedundantIncludes();
236235
return context.mappingLookup().getMapping().mappingUpdate(root);
237236
}
238237

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@
1616
/**
1717
* Holds context for building Mapper objects from their Builders
1818
*/
19-
public final class MapperBuilderContext {
19+
public class MapperBuilderContext {
2020

2121
/**
2222
* The root context, to be used when building a tree of mappers

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -352,7 +352,6 @@ public DocumentMapper merge(String type, CompressedXContent mappingSource, Merge
352352

353353
private DocumentMapper newDocumentMapper(Mapping mapping, MergeReason reason, CompressedXContent mappingSource) {
354354
DocumentMapper newMapper = new DocumentMapper(documentParser, mapping, mappingSource);
355-
newMapper.mapping().getRoot().fixRedundantIncludes();
356355
newMapper.validate(indexSettings, reason != MergeReason.MAPPING_RECOVERY);
357356
return newMapper;
358357
}

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

Lines changed: 49 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,21 @@ Builder includeInParent(boolean includeInParent) {
4949

5050
@Override
5151
public NestedObjectMapper build(MapperBuilderContext context) {
52-
return new NestedObjectMapper(name, context.buildFullName(name), buildMappers(context.createChildContext(name)), this);
52+
boolean parentIncludedInRoot = this.includeInRoot.value();
53+
if (context instanceof NestedMapperBuilderContext nc) {
54+
// we're already inside a nested mapper, so adjust our includes
55+
if (nc.parentIncludedInRoot && this.includeInParent.value()) {
56+
this.includeInRoot = Explicit.IMPLICIT_FALSE;
57+
}
58+
} else {
59+
// this is a top-level nested mapper, so include_in_parent = include_in_root
60+
parentIncludedInRoot |= this.includeInParent.value();
61+
if (this.includeInParent.value()) {
62+
this.includeInRoot = Explicit.IMPLICIT_FALSE;
63+
}
64+
}
65+
NestedMapperBuilderContext nestedContext = new NestedMapperBuilderContext(context.buildFullName(name), parentIncludedInRoot);
66+
return new NestedObjectMapper(name, context.buildFullName(name), buildMappers(nestedContext), this);
5367
}
5468
}
5569

@@ -89,6 +103,21 @@ protected static void parseNested(String name, Map<String, Object> node, NestedO
89103
}
90104
}
91105

106+
private static class NestedMapperBuilderContext extends MapperBuilderContext {
107+
108+
final boolean parentIncludedInRoot;
109+
110+
NestedMapperBuilderContext(String path, boolean parentIncludedInRoot) {
111+
super(path, false);
112+
this.parentIncludedInRoot = parentIncludedInRoot;
113+
}
114+
115+
@Override
116+
public MapperBuilderContext createChildContext(String name) {
117+
return new NestedMapperBuilderContext(buildFullName(name), parentIncludedInRoot);
118+
}
119+
}
120+
92121
private Explicit<Boolean> includeInRoot;
93122
private Explicit<Boolean> includeInParent;
94123
private final String nestedTypePath;
@@ -153,7 +182,7 @@ public ObjectMapper.Builder newBuilder(Version indexVersionCreated) {
153182
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
154183
builder.startObject(simpleName());
155184
builder.field("type", CONTENT_TYPE);
156-
if (includeInParent.value()) {
185+
if (includeInParent.explicit() && includeInParent.value()) {
157186
builder.field("include_in_parent", includeInParent.value());
158187
}
159188
if (includeInRoot.value()) {
@@ -191,10 +220,28 @@ public ObjectMapper merge(Mapper mergeWith, MapperService.MergeReason reason, Ma
191220
throw new MapperException("the [include_in_root] parameter can't be updated on a nested object mapping");
192221
}
193222
}
223+
if (parentBuilderContext instanceof NestedMapperBuilderContext nc) {
224+
if (nc.parentIncludedInRoot && toMerge.includeInParent.value()) {
225+
toMerge.includeInRoot = Explicit.IMPLICIT_FALSE;
226+
}
227+
} else {
228+
if (toMerge.includeInParent.value()) {
229+
toMerge.includeInRoot = Explicit.IMPLICIT_FALSE;
230+
}
231+
}
194232
toMerge.doMerge(mergeWithObject, reason, parentBuilderContext);
195233
return toMerge;
196234
}
197235

236+
@Override
237+
protected MapperBuilderContext createChildContext(MapperBuilderContext mapperBuilderContext, String name) {
238+
boolean parentIncludedInRoot = this.includeInRoot.value();
239+
if (mapperBuilderContext instanceof NestedMapperBuilderContext == false) {
240+
parentIncludedInRoot |= this.includeInParent.value();
241+
}
242+
return new NestedMapperBuilderContext(mapperBuilderContext.buildFullName(name), parentIncludedInRoot);
243+
}
244+
198245
@Override
199246
public SourceLoader.SyntheticFieldLoader syntheticFieldLoader() {
200247
throw new IllegalArgumentException("field [" + name() + "] of type [" + typeName() + "] doesn't support synthetic source");

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

Lines changed: 0 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -120,36 +120,6 @@ public RootObjectMapper build(MapperBuilderContext context) {
120120
}
121121
}
122122

123-
/**
124-
* Removes redundant root includes in {@link NestedObjectMapper} trees to avoid duplicate
125-
* fields on the root mapper when {@code isIncludeInRoot} is {@code true} for a node that is
126-
* itself included into a parent node, for which either {@code isIncludeInRoot} is
127-
* {@code true} or which is transitively included in root by a chain of nodes with
128-
* {@code isIncludeInParent} returning {@code true}.
129-
*/
130-
// TODO it would be really nice to make this an implementation detail of NestedObjectMapper
131-
// and run it as part of the builder, but this does not yet work because of the way that
132-
// index templates are merged together. If merge() was run on Builder objects rather than
133-
// on Mappers then we could move this.
134-
public void fixRedundantIncludes() {
135-
fixRedundantIncludes(this, true);
136-
}
137-
138-
private static void fixRedundantIncludes(ObjectMapper objectMapper, boolean parentIncluded) {
139-
for (Mapper mapper : objectMapper) {
140-
if (mapper instanceof NestedObjectMapper child) {
141-
boolean isNested = child.isNested();
142-
boolean includeInRootViaParent = parentIncluded && isNested && child.isIncludeInParent();
143-
boolean includedInRoot = isNested && child.isIncludeInRoot();
144-
if (includeInRootViaParent && includedInRoot) {
145-
child.setIncludeInParent(true);
146-
child.setIncludeInRoot(false);
147-
}
148-
fixRedundantIncludes(child, includeInRootViaParent || includedInRoot);
149-
}
150-
}
151-
}
152-
153123
private Explicit<DateFormatter[]> dynamicDateTimeFormatters;
154124
private Explicit<Boolean> dateDetection;
155125
private Explicit<Boolean> numericDetection;

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

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1439,4 +1439,79 @@ public void testNestedDoesNotSupportSubobjectsParameter() {
14391439
);
14401440
assertEquals("Failed to parse mapping: Nested type [nested1] does not support [subobjects] parameter", exception.getMessage());
14411441
}
1442+
1443+
public void testIndexTemplatesMergeIncludes() throws IOException {
1444+
{
1445+
MapperService mapperService = createMapperService("""
1446+
{ "_doc" : { "properties" : {
1447+
"field" : {
1448+
"type" : "nested",
1449+
"include_in_root" : true,
1450+
"properties" : {
1451+
"text" : { "type" : "text" }
1452+
}
1453+
}
1454+
}}}
1455+
""");
1456+
merge(mapperService, MergeReason.INDEX_TEMPLATE, """
1457+
{ "_doc" : { "properties" : {
1458+
"field" : {
1459+
"type" : "nested",
1460+
"include_in_parent" : true,
1461+
"properties" : {
1462+
"text" : { "type" : "text" }
1463+
}
1464+
}
1465+
}}}
1466+
""");
1467+
assertThat(Strings.toString(mapperService.documentMapper().mapping()), containsString("""
1468+
{"type":"nested","include_in_parent":true,"properties":{"""));
1469+
}
1470+
{
1471+
MapperService mapperService = createMapperService("""
1472+
{ "_doc" : { "properties" : {
1473+
"field" : {
1474+
"type" : "nested",
1475+
"include_in_parent" : true,
1476+
"properties" : {
1477+
"text" : { "type" : "text" }
1478+
}
1479+
}
1480+
}}}
1481+
""");
1482+
merge(mapperService, MergeReason.INDEX_TEMPLATE, """
1483+
{ "_doc" : { "properties" : {
1484+
"field" : {
1485+
"type" : "nested",
1486+
"include_in_root" : true,
1487+
"properties" : {
1488+
"text" : { "type" : "text" }
1489+
}
1490+
}
1491+
}}}
1492+
""");
1493+
assertThat(Strings.toString(mapperService.documentMapper().mapping()), containsString("""
1494+
{"type":"nested","include_in_parent":true,"properties":{"""));
1495+
}
1496+
}
1497+
1498+
public void testMergeNested() {
1499+
NestedObjectMapper firstMapper = new NestedObjectMapper.Builder("nested1", Version.CURRENT).includeInParent(true)
1500+
.includeInRoot(true)
1501+
.build(MapperBuilderContext.root(false));
1502+
NestedObjectMapper secondMapper = new NestedObjectMapper.Builder("nested1", Version.CURRENT).includeInParent(false)
1503+
.includeInRoot(true)
1504+
.build(MapperBuilderContext.root(false));
1505+
1506+
MapperException e = expectThrows(MapperException.class, () -> firstMapper.merge(secondMapper, MapperBuilderContext.root(false)));
1507+
assertThat(e.getMessage(), containsString("[include_in_parent] parameter can't be updated on a nested object mapping"));
1508+
1509+
NestedObjectMapper result = (NestedObjectMapper) firstMapper.merge(
1510+
secondMapper,
1511+
MapperService.MergeReason.INDEX_TEMPLATE,
1512+
MapperBuilderContext.root(false)
1513+
);
1514+
assertFalse(result.isIncludeInParent());
1515+
assertTrue(result.isIncludeInRoot());
1516+
}
14421517
}

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

Lines changed: 0 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,6 @@
1313

1414
import java.util.Collections;
1515

16-
import static org.hamcrest.Matchers.containsString;
17-
1816
public class ObjectMapperMergeTests extends ESTestCase {
1917

2018
private final RootObjectMapper rootObjectMapper = createMapping(false, true, true, false);
@@ -117,26 +115,6 @@ public void testMergeDisabledRootMapper() {
117115
assertEquals("test", merged.runtimeFields().iterator().next().name());
118116
}
119117

120-
public void testMergeNested() {
121-
NestedObjectMapper firstMapper = new NestedObjectMapper.Builder("nested1", Version.CURRENT).includeInParent(true)
122-
.includeInRoot(true)
123-
.build(MapperBuilderContext.root(false));
124-
NestedObjectMapper secondMapper = new NestedObjectMapper.Builder("nested1", Version.CURRENT).includeInParent(false)
125-
.includeInRoot(true)
126-
.build(MapperBuilderContext.root(false));
127-
128-
MapperException e = expectThrows(MapperException.class, () -> firstMapper.merge(secondMapper, MapperBuilderContext.root(false)));
129-
assertThat(e.getMessage(), containsString("[include_in_parent] parameter can't be updated on a nested object mapping"));
130-
131-
NestedObjectMapper result = (NestedObjectMapper) firstMapper.merge(
132-
secondMapper,
133-
MapperService.MergeReason.INDEX_TEMPLATE,
134-
MapperBuilderContext.root(false)
135-
);
136-
assertFalse(result.isIncludeInParent());
137-
assertTrue(result.isIncludeInRoot());
138-
}
139-
140118
public void testMergedFieldNamesFieldWithDotsSubobjectsFalseAtRoot() {
141119
RootObjectMapper mergeInto = createRootSubobjectFalseLeafWithDots();
142120
RootObjectMapper mergeWith = createRootSubobjectFalseLeafWithDots();

0 commit comments

Comments
 (0)