From 558ea9782d81cf1b227508d2f52b9a3bb82145e9 Mon Sep 17 00:00:00 2001 From: Armin Date: Fri, 20 Oct 2017 22:24:44 +0200 Subject: [PATCH 1/3] #26990 prevent duplicate fields when mixing parent and root nested includes --- .../index/mapper/RootObjectMapper.java | 35 +++++++++++++++++++ .../index/mapper/NestedObjectMapperTests.java | 30 ++++++++++++++++ 2 files changed, 65 insertions(+) diff --git a/core/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java b/core/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java index 4b2f3265323d7..3feb752c02810 100644 --- a/core/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java +++ b/core/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java @@ -74,6 +74,41 @@ public Builder dynamicTemplates(Collection templates) { return this; } + @Override + public RootObjectMapper build(BuilderContext context) { + fixRedundantIncludes(this, true); + return super.build(context); + } + + /** + * Removes redundant root includes in {@link ObjectMapper.Nested} trees to avoid duplicate + * fields on the root mapper when {@code isIncludeInRoot} is {@code true} for a node that is + * itself included into a parent node, for which either {@code isIncludeInRoot} is + * {@code true} or which is transitively included in root by a chain of nodes with + * {@code isIncludeInParent} returning {@code true}. + * @param omb Builder whose children to check. + * @param parentIncluded True iff node is a child of root or a node that is included in + * root + */ + private static void fixRedundantIncludes(ObjectMapper.Builder omb, boolean parentIncluded) { + for (Object mapper : omb.mappersBuilders) { + if (mapper instanceof ObjectMapper.Builder) { + ObjectMapper.Builder child = (ObjectMapper.Builder) mapper; + Nested nested = child.nested; + boolean included; + if (parentIncluded) { + included = nested.isNested() && nested.isIncludeInParent(); + if (included) { + child.nested = Nested.newNested(true, false); + } + } else { + included = nested.isNested() && nested.isIncludeInRoot(); + } + fixRedundantIncludes(child, included); + } + } + } + @Override protected ObjectMapper createMapper(String name, String fullPath, boolean enabled, Nested nested, Dynamic dynamic, Map mappers, @Nullable Settings settings) { diff --git a/core/src/test/java/org/elasticsearch/index/mapper/NestedObjectMapperTests.java b/core/src/test/java/org/elasticsearch/index/mapper/NestedObjectMapperTests.java index a3b477a4b6f25..6e2f6eab414d2 100644 --- a/core/src/test/java/org/elasticsearch/index/mapper/NestedObjectMapperTests.java +++ b/core/src/test/java/org/elasticsearch/index/mapper/NestedObjectMapperTests.java @@ -19,6 +19,9 @@ package org.elasticsearch.index.mapper; +import java.util.HashMap; +import java.util.HashSet; +import org.apache.lucene.index.IndexableField; import org.elasticsearch.Version; import org.elasticsearch.common.compress.CompressedXContent; import org.elasticsearch.common.settings.Settings; @@ -333,6 +336,33 @@ public void testMultiRootAndNested1() throws Exception { assertThat(doc.docs().get(6).getFields("nested1.nested2.field2").length, equalTo(4)); } + /** + * Checks that multiple levels of nested includes where a node is both directly and transitively + * included in root by {@code include_in_root} and a chain of {@code include_in_parent} does not + * lead to duplicate fields on the root document. + */ + public void testMultipleLevelsIncludeRoot() throws Exception { + String mapping = XContentFactory.jsonBuilder() + .startObject().startObject("type").startObject("properties") + .startObject("nested1").field("type", "nested").field("include_in_root", true).field("include_in_parent", true).startObject("properties") + .startObject("nested2").field("type", "nested").field("include_in_root", true).field("include_in_parent", true) + .endObject().endObject().endObject() + .endObject().endObject().endObject().string(); + + DocumentMapper docMapper = createIndex("test").mapperService().documentMapperParser().parse("type", new CompressedXContent(mapping)); + + ParsedDocument doc = docMapper.parse(SourceToParse.source("test", "type", "1", XContentFactory.jsonBuilder() + .startObject().startArray("nested1") + .startObject().startArray("nested2").startObject().field("foo", "bar") + .endObject().endArray().endObject().endArray() + .endObject() + .bytes(), + XContentType.JSON)); + + final Collection fields = doc.rootDoc().getFields(); + assertThat(fields.size(), equalTo(new HashSet<>(fields).size())); + } + public void testNestedArrayStrict() throws Exception { String mapping = XContentFactory.jsonBuilder().startObject().startObject("type").startObject("properties") .startObject("nested1").field("type", "nested").field("dynamic", "strict").startObject("properties") From 335a23746f82b280c9bf0c89ab1b5fd0d7454ed9 Mon Sep 17 00:00:00 2001 From: Armin Date: Fri, 27 Oct 2017 10:38:26 +0200 Subject: [PATCH 2/3] #26990 prevent duplicate fields when mixing parent and root nested includes (follow up) --- .../index/mapper/RootObjectMapper.java | 15 ++++---- .../index/mapper/NestedObjectMapperTests.java | 36 ++++++++++++++++++- 2 files changed, 41 insertions(+), 10 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java b/core/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java index 3feb752c02810..6a9dbb282b5fd 100644 --- a/core/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java +++ b/core/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java @@ -95,16 +95,13 @@ private static void fixRedundantIncludes(ObjectMapper.Builder omb, boolean paren if (mapper instanceof ObjectMapper.Builder) { ObjectMapper.Builder child = (ObjectMapper.Builder) mapper; Nested nested = child.nested; - boolean included; - if (parentIncluded) { - included = nested.isNested() && nested.isIncludeInParent(); - if (included) { - child.nested = Nested.newNested(true, false); - } - } else { - included = nested.isNested() && nested.isIncludeInRoot(); + boolean isNested = nested.isNested(); + boolean includedInParent = parentIncluded && isNested && nested.isIncludeInParent(); + boolean includedInRoot = isNested && nested.isIncludeInRoot(); + if (includedInParent && includedInRoot) { + child.nested = Nested.newNested(true, false); } - fixRedundantIncludes(child, included); + fixRedundantIncludes(child, includedInParent || includedInRoot); } } } diff --git a/core/src/test/java/org/elasticsearch/index/mapper/NestedObjectMapperTests.java b/core/src/test/java/org/elasticsearch/index/mapper/NestedObjectMapperTests.java index 6e2f6eab414d2..39d4de2359e78 100644 --- a/core/src/test/java/org/elasticsearch/index/mapper/NestedObjectMapperTests.java +++ b/core/src/test/java/org/elasticsearch/index/mapper/NestedObjectMapperTests.java @@ -341,7 +341,7 @@ public void testMultiRootAndNested1() throws Exception { * included in root by {@code include_in_root} and a chain of {@code include_in_parent} does not * lead to duplicate fields on the root document. */ - public void testMultipleLevelsIncludeRoot() throws Exception { + public void testMultipleLevelsIncludeRoot1() throws Exception { String mapping = XContentFactory.jsonBuilder() .startObject().startObject("type").startObject("properties") .startObject("nested1").field("type", "nested").field("include_in_root", true).field("include_in_parent", true).startObject("properties") @@ -363,6 +363,40 @@ public void testMultipleLevelsIncludeRoot() throws Exception { assertThat(fields.size(), equalTo(new HashSet<>(fields).size())); } + /** + * Same as {@link NestedObjectMapperTests#testMultipleLevelsIncludeRoot1()} but tests for the + * case where the transitive {@code include_in_parent} and redundant {@code include_in_root} + * happen on a chain of nodes that starts from a parent node that is not directly connected to + * root by a chain of {@code include_in_parent}, i.e. that has {@code include_in_parent} set to + * {@code false} and {@code include_in_root} set to {@code true}. + */ + public void testMultipleLevelsIncludeRoot2() throws Exception { + String mapping = XContentFactory.jsonBuilder() + .startObject().startObject("type").startObject("properties") + .startObject("nested1").field("type", "nested") + .field("include_in_root", true).field("include_in_parent", true).startObject("properties") + .startObject("nested2").field("type", "nested") + .field("include_in_root", true).field("include_in_parent", false).startObject("properties") + .startObject("nested3").field("type", "nested") + .field("include_in_root", true).field("include_in_parent", true) + .endObject().endObject().endObject().endObject().endObject() + .endObject().endObject().endObject().string(); + + DocumentMapper docMapper = createIndex("test").mapperService().documentMapperParser().parse("type", new CompressedXContent(mapping)); + + ParsedDocument doc = docMapper.parse(SourceToParse.source("test", "type", "1", XContentFactory.jsonBuilder() + .startObject().startArray("nested1") + .startObject().startArray("nested2") + .startObject().startArray("nested3").startObject().field("foo", "bar") + .endObject().endArray().endObject().endArray().endObject().endArray() + .endObject() + .bytes(), + XContentType.JSON)); + + final Collection fields = doc.rootDoc().getFields(); + assertThat(fields.size(), equalTo(new HashSet<>(fields).size())); + } + public void testNestedArrayStrict() throws Exception { String mapping = XContentFactory.jsonBuilder().startObject().startObject("type").startObject("properties") .startObject("nested1").field("type", "nested").field("dynamic", "strict").startObject("properties") From a043337380a90f85116b98adc9e6c706dc7343f5 Mon Sep 17 00:00:00 2001 From: Armin Date: Fri, 27 Oct 2017 10:59:14 +0200 Subject: [PATCH 3/3] #26990 prevent duplicate fields when mixing parent and root nested includes (follow up) --- .../org/elasticsearch/index/mapper/RootObjectMapper.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java b/core/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java index 6a9dbb282b5fd..42341bfb96b2d 100644 --- a/core/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java +++ b/core/src/main/java/org/elasticsearch/index/mapper/RootObjectMapper.java @@ -96,12 +96,12 @@ private static void fixRedundantIncludes(ObjectMapper.Builder omb, boolean paren ObjectMapper.Builder child = (ObjectMapper.Builder) mapper; Nested nested = child.nested; boolean isNested = nested.isNested(); - boolean includedInParent = parentIncluded && isNested && nested.isIncludeInParent(); + boolean includeInRootViaParent = parentIncluded && isNested && nested.isIncludeInParent(); boolean includedInRoot = isNested && nested.isIncludeInRoot(); - if (includedInParent && includedInRoot) { + if (includeInRootViaParent && includedInRoot) { child.nested = Nested.newNested(true, false); } - fixRedundantIncludes(child, includedInParent || includedInRoot); + fixRedundantIncludes(child, includeInRootViaParent || includedInRoot); } } }