Skip to content

Call fixRedundantIncludes on dynamic mapping updates #74903

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 5, 2021
Merged

Call fixRedundantIncludes on dynamic mapping updates #74903

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,7 @@ static Mapping createDynamicUpdate(MappingLookup mappingLookup,
RootObjectMapper root;
if (dynamicMappers.isEmpty() == false) {
root = createDynamicUpdate(mappingLookup, dynamicMappers);
root.fixRedundantIncludes();
Copy link
Member

@javanna javanna Jul 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a reason that we don't always do this when we create a root object mapper?

Copy link
Contributor Author

@romseygeek romseygeek Jul 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to do this on RootObjectMapper.Builder.build(), but unfortunately RootObjectMapper (and indeed ObjectMapper in general) is not immutable, so we need to have this extra method.

Copy link
Member

@javanna javanna Jul 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

arg, I was initially thinking that calling this in the DocumentMapper construction would fix it but it does not. But why can't build call this method before returning the root?

Copy link
Contributor Author

@romseygeek romseygeek Jul 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But why can't build call this method before returning the root?

It could, but because the resulting object is not immutable it wouldn't help us here. Mappers can still be added to the RootObjectMapper after build() has been called, and in fact that's precisely what happens in createDynamicUpdate().

Copy link
Member

@javanna javanna Jul 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that makes sense now :)

Copy link
Member

@javanna javanna Jul 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I heard somebody say we should use builders all the way, not sure who it was :)

Copy link
Contributor Author

@romseygeek romseygeek Jul 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Long term we should change dynamic mappers to use builders and not mapper objects, but that's a different discussion :)

} else {
root = mappingLookup.getMapping().getRoot().copyAndReset();
}
Expand Down
68 changes: 68 additions & 0 deletions server/src/test/java/org/elasticsearch/index/mapper/NestedObjectMapperTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,44 @@ public void testMultipleLevelsIncludeRoot1() throws Exception {
assertThat(fields.size(), equalTo(new HashSet<>(fields).size()));
}

public void testRecursiveIncludeInParent() throws IOException {

// if we have a nested hierarchy, and all nested mappers have 'include_in_parent'
// set to 'true', then values from the grandchild nodes should be copied all the
// way up the hierarchy and into the root document, even if 'include_in_root' has
// explicitly been set to 'false'.

MapperService mapperService = createMapperService(mapping(b -> {
b.startObject("nested1");
b.field("type", "nested");
b.field("include_in_parent", true);
b.field("include_in_root", false);
b.startObject("properties");
b.startObject("nested1_id").field("type", "keyword").endObject();
b.startObject("nested2");
b.field("type", "nested");
b.field("include_in_parent", true);
b.field("include_in_root", false);
b.startObject("properties");
b.startObject("nested2_id").field("type", "keyword").endObject();
b.endObject();
b.endObject();
b.endObject();
b.endObject();
}));

ParsedDocument doc = mapperService.documentMapper().parse(source(b -> {
b.startObject("nested1");
b.field("nested1_id", "1");
b.startObject("nested2");
b.field("nested2_id", "2");
b.endObject();
b.endObject();
}));

assertNotNull(doc.rootDoc().getField("nested1.nested2.nested2_id"));
}

/**
* Same as {@link NestedObjectMapperTests#testMultipleLevelsIncludeRoot1()} but tests for the
* case where the transitive {@code include_in_parent} and redundant {@code include_in_root}
Expand Down Expand Up @@ -801,4 +839,34 @@ public void testMergeNestedMappings() throws IOException {
})));
assertEquals("the [include_in_root] parameter can't be updated on a nested object mapping", e2.getMessage());
}

public void testMergeNestedMappingsFromDynamicUpdate() throws IOException {

// Check that dynamic mappings have redundant includes removed

MapperService mapperService = createMapperService(topMapping(b -> {
b.startArray("dynamic_templates");
b.startObject();
b.startObject("object_fields");
b.field("match_mapping_type", "object");
b.startObject("mapping");
b.field("type", "nested");
b.field("include_in_parent", true);
b.field("include_in_root", true);
b.endObject();
b.field("match", "*");
b.endObject();
b.endObject();
b.endArray();
}));

ParsedDocument doc = mapperService.documentMapper().parse(source(b -> b.startObject("object").endObject()));

merge(mapperService, Strings.toString(doc.dynamicMappingsUpdate()));
merge(mapperService, Strings.toString(doc.dynamicMappingsUpdate()));

assertThat(
Strings.toString(mapperService.documentMapper().mapping()),
containsString("\"properties\":{\"object\":{\"type\":\"nested\",\"include_in_parent\":true}}"));
}
}