From b9a90eca20b2406ee7af8033ff6a2a445603520d Mon Sep 17 00:00:00 2001 From: Martijn van Groningen Date: Wed, 21 Dec 2016 17:24:32 +0100 Subject: [PATCH] inner hits: Don't inline inner hits if the query the inner hits is inlined into can't resolve mappings and ignore_unmapped has been set to true Closes #21620 --- .../index/query/HasChildQueryBuilder.java | 12 +- .../index/query/HasParentQueryBuilder.java | 8 +- .../index/query/InnerHitBuilder.java | 53 ++++++-- .../index/query/NestedQueryBuilder.java | 6 +- .../index/query/QueryShardContext.java | 9 ++ .../query/HasChildQueryBuilderTests.java | 7 +- .../query/HasParentQueryBuilderTests.java | 4 +- .../index/query/InnerHitBuilderTests.java | 83 +++++++++++-- .../index/query/NestedQueryBuilderTests.java | 4 +- .../search/child/ChildQuerySearchIT.java | 4 +- .../search/fetch/subphase/InnerHitsIT.java | 116 ++++++++++++------ 11 files changed, 228 insertions(+), 78 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/index/query/HasChildQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/HasChildQueryBuilder.java index 85b4fd18da5..eb2ae57f6df 100644 --- a/core/src/main/java/org/elasticsearch/index/query/HasChildQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/HasChildQueryBuilder.java @@ -145,8 +145,8 @@ public class HasChildQueryBuilder extends AbstractQueryBuilder innerHitBuilder.ignoreUnmapped = value, IGNORE_UNMAPPED); PARSER.declareInt(InnerHitBuilder::setFrom, SearchSourceBuilder.FROM_FIELD); PARSER.declareInt(InnerHitBuilder::setSize, SearchSourceBuilder.SIZE_FIELD); PARSER.declareBoolean(InnerHitBuilder::setExplain, SearchSourceBuilder.EXPLAIN_FIELD); @@ -130,6 +132,7 @@ public final class InnerHitBuilder extends ToXContentToBytes implements Writeabl private String name; private String nestedPath; private String parentChildType; + private boolean ignoreUnmapped; private int from; private int size = 3; @@ -151,6 +154,7 @@ public final class InnerHitBuilder extends ToXContentToBytes implements Writeabl private InnerHitBuilder(InnerHitBuilder other) { name = other.name; + this.ignoreUnmapped = other.ignoreUnmapped; from = other.from; size = other.size; explain = other.explain; @@ -180,19 +184,21 @@ public final class InnerHitBuilder extends ToXContentToBytes implements Writeabl } - InnerHitBuilder(InnerHitBuilder other, String nestedPath, QueryBuilder query) { + InnerHitBuilder(InnerHitBuilder other, String nestedPath, QueryBuilder query, boolean ignoreUnmapped) { this(other); this.query = query; this.nestedPath = nestedPath; + this.ignoreUnmapped = ignoreUnmapped; if (name == null) { this.name = nestedPath; } } - InnerHitBuilder(InnerHitBuilder other, QueryBuilder query, String parentChildType) { + InnerHitBuilder(InnerHitBuilder other, QueryBuilder query, String parentChildType, boolean ignoreUnmapped) { this(other); this.query = query; this.parentChildType = parentChildType; + this.ignoreUnmapped = ignoreUnmapped; if (name == null) { this.name = parentChildType; } @@ -205,6 +211,9 @@ public final class InnerHitBuilder extends ToXContentToBytes implements Writeabl name = in.readOptionalString(); nestedPath = in.readOptionalString(); parentChildType = in.readOptionalString(); + if (in.getVersion().onOrAfter(Version.V_5_2_0_UNRELEASED)) { + ignoreUnmapped = in.readBoolean(); + } from = in.readVInt(); size = in.readVInt(); explain = in.readBoolean(); @@ -243,6 +252,9 @@ public final class InnerHitBuilder extends ToXContentToBytes implements Writeabl out.writeOptionalString(name); out.writeOptionalString(nestedPath); out.writeOptionalString(parentChildType); + if (out.getVersion().onOrAfter(Version.V_5_2_0_UNRELEASED)) { + out.writeBoolean(ignoreUnmapped); + } out.writeVInt(from); out.writeVInt(size); out.writeBoolean(explain); @@ -289,6 +301,13 @@ public final class InnerHitBuilder extends ToXContentToBytes implements Writeabl return this; } + /** + * Whether to include inner hits in the search response hits if required mappings is missing + */ + public boolean isIgnoreUnmapped() { + return ignoreUnmapped; + } + public int getFrom() { return from; } @@ -523,6 +542,14 @@ public final class InnerHitBuilder extends ToXContentToBytes implements Writeabl QueryShardContext queryShardContext = parentSearchContext.getQueryShardContext(); if (nestedPath != null) { ObjectMapper nestedObjectMapper = queryShardContext.getObjectMapper(nestedPath); + if (nestedObjectMapper == null) { + if (ignoreUnmapped == false) { + throw new IllegalStateException("[" + query.getName() + "] no mapping found for type [" + nestedPath + "]"); + } else { + return null; + } + } + ObjectMapper parentObjectMapper = queryShardContext.nestedScope().nextLevel(nestedObjectMapper); InnerHitsContext.NestedInnerHits nestedInnerHits = new InnerHitsContext.NestedInnerHits( name, parentSearchContext, parentObjectMapper, nestedObjectMapper @@ -535,7 +562,15 @@ public final class InnerHitBuilder extends ToXContentToBytes implements Writeabl innerHitsContext.addInnerHitDefinition(nestedInnerHits); return nestedInnerHits; } else if (parentChildType != null) { - DocumentMapper documentMapper = queryShardContext.getMapperService().documentMapper(parentChildType); + DocumentMapper documentMapper = queryShardContext.documentMapper(parentChildType); + if (documentMapper == null) { + if (ignoreUnmapped == false) { + throw new IllegalStateException("[" + query.getName() + "] no mapping found for type [" + parentChildType + "]"); + } else { + return null; + } + } + InnerHitsContext.ParentChildInnerHits parentChildInnerHits = new InnerHitsContext.ParentChildInnerHits( name, parentSearchContext, queryShardContext.getMapperService(), documentMapper ); @@ -556,7 +591,9 @@ public final class InnerHitBuilder extends ToXContentToBytes implements Writeabl InnerHitsContext.BaseInnerHits childInnerHit = entry.getValue().build( parentSearchContext, new InnerHitsContext() ); - childInnerHits.put(entry.getKey(), childInnerHit); + if (childInnerHit != null) { + childInnerHits.put(entry.getKey(), childInnerHit); + } } innerHits.setChildInnerHits(childInnerHits); } @@ -617,6 +654,7 @@ public final class InnerHitBuilder extends ToXContentToBytes implements Writeabl if (name != null) { builder.field(NAME_FIELD.getPreferredName(), name); } + builder.field(IGNORE_UNMAPPED.getPreferredName(), ignoreUnmapped); builder.field(SearchSourceBuilder.FROM_FIELD.getPreferredName(), from); builder.field(SearchSourceBuilder.SIZE_FIELD.getPreferredName(), size); builder.field(SearchSourceBuilder.VERSION_FIELD.getPreferredName(), version); @@ -672,6 +710,7 @@ public final class InnerHitBuilder extends ToXContentToBytes implements Writeabl return Objects.equals(name, that.name) && Objects.equals(nestedPath, that.nestedPath) && Objects.equals(parentChildType, that.parentChildType) && + Objects.equals(ignoreUnmapped, that.ignoreUnmapped) && Objects.equals(from, that.from) && Objects.equals(size, that.size) && Objects.equals(explain, that.explain) && @@ -689,8 +728,8 @@ public final class InnerHitBuilder extends ToXContentToBytes implements Writeabl @Override public int hashCode() { - return Objects.hash(name, nestedPath, parentChildType, from, size, explain, version, trackScores, storedFieldsContext, - docValueFields, scriptFields, fetchSourceContext, sorts, highlightBuilder, query, childInnerHits); + return Objects.hash(name, nestedPath, parentChildType, ignoreUnmapped, from, size, explain, version, trackScores, + storedFieldsContext, docValueFields, scriptFields, fetchSourceContext, sorts, highlightBuilder, query, childInnerHits); } public static InnerHitBuilder fromXContent(QueryParseContext context) throws IOException { diff --git a/core/src/main/java/org/elasticsearch/index/query/NestedQueryBuilder.java b/core/src/main/java/org/elasticsearch/index/query/NestedQueryBuilder.java index 6c1a8b27f6b..519abe41107 100644 --- a/core/src/main/java/org/elasticsearch/index/query/NestedQueryBuilder.java +++ b/core/src/main/java/org/elasticsearch/index/query/NestedQueryBuilder.java @@ -103,8 +103,8 @@ public class NestedQueryBuilder extends AbstractQueryBuilder return innerHitBuilder; } - public NestedQueryBuilder innerHit(InnerHitBuilder innerHit) { - this.innerHitBuilder = new InnerHitBuilder(innerHit, path, query); + public NestedQueryBuilder innerHit(InnerHitBuilder innerHit, boolean ignoreUnmapped) { + this.innerHitBuilder = new InnerHitBuilder(innerHit, path, query, ignoreUnmapped); return this; } @@ -194,7 +194,7 @@ public class NestedQueryBuilder extends AbstractQueryBuilder .queryName(queryName) .boost(boost); if (innerHitBuilder != null) { - queryBuilder.innerHit(innerHitBuilder); + queryBuilder.innerHit(innerHitBuilder, ignoreUnmapped); } return queryBuilder; } diff --git a/core/src/main/java/org/elasticsearch/index/query/QueryShardContext.java b/core/src/main/java/org/elasticsearch/index/query/QueryShardContext.java index 31668922e6e..e075368a2b4 100644 --- a/core/src/main/java/org/elasticsearch/index/query/QueryShardContext.java +++ b/core/src/main/java/org/elasticsearch/index/query/QueryShardContext.java @@ -41,6 +41,7 @@ import org.elasticsearch.index.cache.bitset.BitsetFilterCache; import org.elasticsearch.index.fielddata.IndexFieldData; import org.elasticsearch.index.fielddata.IndexFieldDataService; import org.elasticsearch.index.mapper.ContentPath; +import org.elasticsearch.index.mapper.DocumentMapper; import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.index.mapper.Mapper; import org.elasticsearch.index.mapper.MapperService; @@ -207,6 +208,14 @@ public class QueryShardContext extends QueryRewriteContext { return mapperService.getObjectMapper(name); } + /** + * Returns s {@link DocumentMapper} instance for the given type. + * Delegates to {@link MapperService#documentMapper(String)} + */ + public DocumentMapper documentMapper(String type) { + return mapperService.documentMapper(type); + } + /** * Gets the search analyzer for the given field, or the default if there is none present for the field * TODO: remove this by moving defaults into mappers themselves diff --git a/core/src/test/java/org/elasticsearch/index/query/HasChildQueryBuilderTests.java b/core/src/test/java/org/elasticsearch/index/query/HasChildQueryBuilderTests.java index 0caf112bd26..e2c8e8b292c 100644 --- a/core/src/test/java/org/elasticsearch/index/query/HasChildQueryBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/index/query/HasChildQueryBuilderTests.java @@ -104,13 +104,13 @@ public class HasChildQueryBuilderTests extends AbstractQueryTestCase innerHitBuilders = new HashMap<>(); nestedQueryBuilder.extractInnerHitBuilders(innerHitBuilders); assertThat(innerHitBuilders.get(leafInnerHits.getName()), notNullValue()); @@ -129,7 +133,7 @@ public class InnerHitBuilderTests extends ESTestCase { public void testInlineLeafInnerHitsHasChildQuery() throws Exception { InnerHitBuilder leafInnerHits = randomInnerHits(); HasChildQueryBuilder hasChildQueryBuilder = new HasChildQueryBuilder("type", new MatchAllQueryBuilder(), ScoreMode.None) - .innerHit(leafInnerHits); + .innerHit(leafInnerHits, false); Map innerHitBuilders = new HashMap<>(); hasChildQueryBuilder.extractInnerHitBuilders(innerHitBuilders); assertThat(innerHitBuilders.get(leafInnerHits.getName()), notNullValue()); @@ -138,7 +142,7 @@ public class InnerHitBuilderTests extends ESTestCase { public void testInlineLeafInnerHitsHasParentQuery() throws Exception { InnerHitBuilder leafInnerHits = randomInnerHits(); HasParentQueryBuilder hasParentQueryBuilder = new HasParentQueryBuilder("type", new MatchAllQueryBuilder(), false) - .innerHit(leafInnerHits); + .innerHit(leafInnerHits, false); Map innerHitBuilders = new HashMap<>(); hasParentQueryBuilder.extractInnerHitBuilders(innerHitBuilders); assertThat(innerHitBuilders.get(leafInnerHits.getName()), notNullValue()); @@ -147,7 +151,7 @@ public class InnerHitBuilderTests extends ESTestCase { public void testInlineLeafInnerHitsNestedQueryViaBoolQuery() { InnerHitBuilder leafInnerHits = randomInnerHits(); NestedQueryBuilder nestedQueryBuilder = new NestedQueryBuilder("path", new MatchAllQueryBuilder(), ScoreMode.None) - .innerHit(leafInnerHits); + .innerHit(leafInnerHits, false); BoolQueryBuilder boolQueryBuilder = new BoolQueryBuilder().should(nestedQueryBuilder); Map innerHitBuilders = new HashMap<>(); boolQueryBuilder.extractInnerHitBuilders(innerHitBuilders); @@ -157,7 +161,7 @@ public class InnerHitBuilderTests extends ESTestCase { public void testInlineLeafInnerHitsNestedQueryViaConstantScoreQuery() { InnerHitBuilder leafInnerHits = randomInnerHits(); NestedQueryBuilder nestedQueryBuilder = new NestedQueryBuilder("path", new MatchAllQueryBuilder(), ScoreMode.None) - .innerHit(leafInnerHits); + .innerHit(leafInnerHits, false); ConstantScoreQueryBuilder constantScoreQueryBuilder = new ConstantScoreQueryBuilder(nestedQueryBuilder); Map innerHitBuilders = new HashMap<>(); constantScoreQueryBuilder.extractInnerHitBuilders(innerHitBuilders); @@ -167,10 +171,10 @@ public class InnerHitBuilderTests extends ESTestCase { public void testInlineLeafInnerHitsNestedQueryViaBoostingQuery() { InnerHitBuilder leafInnerHits1 = randomInnerHits(); NestedQueryBuilder nestedQueryBuilder1 = new NestedQueryBuilder("path", new MatchAllQueryBuilder(), ScoreMode.None) - .innerHit(leafInnerHits1); + .innerHit(leafInnerHits1, false); InnerHitBuilder leafInnerHits2 = randomInnerHits(); NestedQueryBuilder nestedQueryBuilder2 = new NestedQueryBuilder("path", new MatchAllQueryBuilder(), ScoreMode.None) - .innerHit(leafInnerHits2); + .innerHit(leafInnerHits2, false); BoostingQueryBuilder constantScoreQueryBuilder = new BoostingQueryBuilder(nestedQueryBuilder1, nestedQueryBuilder2); Map innerHitBuilders = new HashMap<>(); constantScoreQueryBuilder.extractInnerHitBuilders(innerHitBuilders); @@ -181,13 +185,68 @@ public class InnerHitBuilderTests extends ESTestCase { public void testInlineLeafInnerHitsNestedQueryViaFunctionScoreQuery() { InnerHitBuilder leafInnerHits = randomInnerHits(); NestedQueryBuilder nestedQueryBuilder = new NestedQueryBuilder("path", new MatchAllQueryBuilder(), ScoreMode.None) - .innerHit(leafInnerHits); + .innerHit(leafInnerHits, false); FunctionScoreQueryBuilder functionScoreQueryBuilder = new FunctionScoreQueryBuilder(nestedQueryBuilder); Map innerHitBuilders = new HashMap<>(); ((AbstractQueryBuilder) functionScoreQueryBuilder).extractInnerHitBuilders(innerHitBuilders); assertThat(innerHitBuilders.get(leafInnerHits.getName()), notNullValue()); } + public void testBuild_ingoreUnmappedNestQuery() throws Exception { + QueryShardContext queryShardContext = mock(QueryShardContext.class); + when(queryShardContext.getObjectMapper("path")).thenReturn(null); + SearchContext searchContext = mock(SearchContext.class); + when(searchContext.getQueryShardContext()).thenReturn(queryShardContext); + + InnerHitBuilder leafInnerHits = randomInnerHits(); + NestedQueryBuilder query1 = new NestedQueryBuilder("path", new MatchAllQueryBuilder(), ScoreMode.None); + query1.innerHit(leafInnerHits, false); + expectThrows(IllegalStateException.class, () -> query1.innerHit().build(searchContext, new InnerHitsContext())); + + NestedQueryBuilder query2 = new NestedQueryBuilder("path", new MatchAllQueryBuilder(), ScoreMode.None); + query2.innerHit(leafInnerHits, true); + InnerHitsContext innerHitsContext = new InnerHitsContext(); + query2.innerHit().build(searchContext, innerHitsContext); + assertThat(innerHitsContext.getInnerHits().size(), equalTo(0)); + } + + public void testBuild_ignoreUnmappedHasChildQuery() throws Exception { + QueryShardContext queryShardContext = mock(QueryShardContext.class); + when(queryShardContext.documentMapper("type")).thenReturn(null); + SearchContext searchContext = mock(SearchContext.class); + when(searchContext.getQueryShardContext()).thenReturn(queryShardContext); + + InnerHitBuilder leafInnerHits = randomInnerHits(); + HasChildQueryBuilder query1 = new HasChildQueryBuilder("type", new MatchAllQueryBuilder(), ScoreMode.None) + .innerHit(leafInnerHits, false); + expectThrows(IllegalStateException.class, () -> query1.innerHit().build(searchContext, new InnerHitsContext())); + + HasChildQueryBuilder query2 = new HasChildQueryBuilder("type", new MatchAllQueryBuilder(), ScoreMode.None) + .innerHit(leafInnerHits, true); + InnerHitsContext innerHitsContext = new InnerHitsContext(); + query2.innerHit().build(searchContext, innerHitsContext); + assertThat(innerHitsContext.getInnerHits().size(), equalTo(0)); + } + + public void testBuild_ingoreUnmappedHasParentQuery() throws Exception { + QueryShardContext queryShardContext = mock(QueryShardContext.class); + when(queryShardContext.documentMapper("type")).thenReturn(null); + SearchContext searchContext = mock(SearchContext.class); + when(searchContext.getQueryShardContext()).thenReturn(queryShardContext); + + InnerHitBuilder leafInnerHits = randomInnerHits(); + HasParentQueryBuilder query1 = new HasParentQueryBuilder("type", new MatchAllQueryBuilder(), false) + .innerHit(leafInnerHits, false); + expectThrows(IllegalStateException.class, () -> query1.innerHit().build(searchContext, new InnerHitsContext())); + + HasParentQueryBuilder query2 = new HasParentQueryBuilder("type", new MatchAllQueryBuilder(), false) + .innerHit(leafInnerHits, true); + InnerHitsContext innerHitsContext = new InnerHitsContext(); + query2.innerHit().build(searchContext, innerHitsContext); + assertThat(innerHitsContext.getInnerHits().size(), equalTo(0)); + } + + public static InnerHitBuilder randomInnerHits() { return randomInnerHits(true, true); } @@ -236,9 +295,9 @@ public class InnerHitBuilderTests extends ESTestCase { if (includeQueryTypeOrPath) { QueryBuilder query = new MatchQueryBuilder(randomAsciiOfLengthBetween(1, 16), randomAsciiOfLengthBetween(1, 16)); if (randomBoolean()) { - return new InnerHitBuilder(innerHits, randomAsciiOfLength(8), query); + return new InnerHitBuilder(innerHits, randomAsciiOfLength(8), query, randomBoolean()); } else { - return new InnerHitBuilder(innerHits, query, randomAsciiOfLength(8)); + return new InnerHitBuilder(innerHits, query, randomAsciiOfLength(8), randomBoolean()); } } else { return innerHits; @@ -248,8 +307,8 @@ public class InnerHitBuilderTests extends ESTestCase { public void testCopyConstructor() throws Exception { InnerHitBuilder original = randomInnerHits(); InnerHitBuilder copy = original.getNestedPath() != null ? - new InnerHitBuilder(original, original.getNestedPath(), original.getQuery()) : - new InnerHitBuilder(original, original.getQuery(), original.getParentChildType()); + new InnerHitBuilder(original, original.getNestedPath(), original.getQuery(), original.isIgnoreUnmapped()) : + new InnerHitBuilder(original, original.getQuery(), original.getParentChildType(), original.isIgnoreUnmapped()); assertThat(copy, equalTo(original)); copy = mutate(copy); assertThat(copy, not(equalTo(original))); diff --git a/core/src/test/java/org/elasticsearch/index/query/NestedQueryBuilderTests.java b/core/src/test/java/org/elasticsearch/index/query/NestedQueryBuilderTests.java index 988f6f5c4ba..c322e0a7190 100644 --- a/core/src/test/java/org/elasticsearch/index/query/NestedQueryBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/index/query/NestedQueryBuilderTests.java @@ -73,13 +73,13 @@ public class NestedQueryBuilderTests extends AbstractQueryTestCase