Skip to content

Commit 6c34452

Browse files
martijnvgmartijnvg
committed
inner hits: Query and top level inner hit definitions shouldn't overwrite each other.
Closes #16218
1 parent b039360 commit 6c34452

File tree

15 files changed

+58
-110
lines changed

15 files changed

+58
-110
lines changed

‎core/src/main/java/org/elasticsearch/index/query/QueryShardContext.java

+1-7
Original file line numberDiff line numberDiff line change
@@ -236,13 +236,7 @@ public void addInnerHits(String name, InnerHitsContext.BaseInnerHits context) {
236236
throw new QueryShardException(this, "inner_hits unsupported");
237237
}
238238

239-
InnerHitsContext innerHitsContext;
240-
if (sc.innerHits() == null) {
241-
innerHitsContext = new InnerHitsContext(new HashMap<>());
242-
sc.innerHits(innerHitsContext);
243-
} else {
244-
innerHitsContext = sc.innerHits();
245-
}
239+
InnerHitsContext innerHitsContext = sc.innerHits();
246240
innerHitsContext.addInnerHitDefinition(name, context);
247241
}
248242

‎core/src/main/java/org/elasticsearch/percolator/PercolateContext.java

-5
Original file line numberDiff line numberDiff line change
@@ -674,11 +674,6 @@ public Counter timeEstimateCounter() {
674674
throw new UnsupportedOperationException();
675675
}
676676

677-
@Override
678-
public void innerHits(InnerHitsContext innerHitsContext) {
679-
throw new UnsupportedOperationException();
680-
}
681-
682677
@Override
683678
public InnerHitsContext innerHits() {
684679
throw new UnsupportedOperationException();

‎core/src/main/java/org/elasticsearch/search/fetch/innerhits/InnerHitsContext.java

+15
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,7 @@
5656
import org.elasticsearch.search.internal.SearchContext;
5757

5858
import java.io.IOException;
59+
import java.util.HashMap;
5960
import java.util.Map;
6061

6162
/**
@@ -64,6 +65,10 @@ public final class InnerHitsContext {
6465

6566
private final Map<String, BaseInnerHits> innerHits;
6667

68+
public InnerHitsContext() {
69+
this.innerHits = new HashMap<>();
70+
}
71+
6772
public InnerHitsContext(Map<String, BaseInnerHits> innerHits) {
6873
this.innerHits = innerHits;
6974
}
@@ -73,9 +78,19 @@ public Map<String, BaseInnerHits> getInnerHits() {
7378
}
7479

7580
public void addInnerHitDefinition(String name, BaseInnerHits innerHit) {
81+
if (innerHits.containsKey(name)) {
82+
throw new IllegalArgumentException("inner_hit definition with the name [" + name + "] already exists. Use a different inner_hit name");
83+
}
84+
7685
innerHits.put(name, innerHit);
7786
}
7887

88+
public void addInnerHitDefinitions(Map<String, BaseInnerHits> innerHits) {
89+
for (Map.Entry<String, BaseInnerHits> entry : innerHits.entrySet()) {
90+
addInnerHitDefinition(entry.getKey(), entry.getValue());
91+
}
92+
}
93+
7994
public static abstract class BaseInnerHits extends FilteredSearchContext {
8095

8196
protected final ParsedQuery query;

‎core/src/main/java/org/elasticsearch/search/fetch/innerhits/InnerHitsFetchSubPhase.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ public InnerHitsFetchSubPhase(SortParseElement sortParseElement, FetchSourcePars
6363

6464
@Override
6565
public boolean hitExecutionNeeded(SearchContext context) {
66-
return context.innerHits() != null;
66+
return context.innerHits() != null && context.innerHits().getInnerHits().size() > 0;
6767
}
6868

6969
@Override

‎core/src/main/java/org/elasticsearch/search/fetch/innerhits/InnerHitsParseElement.java

+4-3
Original file line numberDiff line numberDiff line change
@@ -61,9 +61,10 @@ public InnerHitsParseElement(SortParseElement sortParseElement, FetchSourceParse
6161
public void parse(XContentParser parser, SearchContext searchContext) throws Exception {
6262
QueryShardContext context = searchContext.indexShard().getQueryShardContext();
6363
context.reset(parser);
64-
Map<String, InnerHitsContext.BaseInnerHits> innerHitsMap = parseInnerHits(parser, context, searchContext);
65-
if (innerHitsMap != null) {
66-
searchContext.innerHits(new InnerHitsContext(innerHitsMap));
64+
Map<String, InnerHitsContext.BaseInnerHits> topLevelInnerHits = parseInnerHits(parser, context, searchContext);
65+
if (topLevelInnerHits != null) {
66+
InnerHitsContext innerHitsContext = searchContext.innerHits();
67+
innerHitsContext.addInnerHitDefinitions(topLevelInnerHits);
6768
}
6869
}
6970

‎core/src/main/java/org/elasticsearch/search/internal/DefaultSearchContext.java

-11
Original file line numberDiff line numberDiff line change
@@ -146,7 +146,6 @@ public class DefaultSearchContext extends SearchContext {
146146
private volatile long keepAlive;
147147
private final long originNanoTime = System.nanoTime();
148148
private volatile long lastAccessTime = -1;
149-
private InnerHitsContext innerHitsContext;
150149
private Profilers profilers;
151150

152151
private final Map<String, FetchSubPhaseContext> subPhaseContexts = new HashMap<>();
@@ -761,16 +760,6 @@ public Counter timeEstimateCounter() {
761760
return timeEstimateCounter;
762761
}
763762

764-
@Override
765-
public void innerHits(InnerHitsContext innerHitsContext) {
766-
this.innerHitsContext = innerHitsContext;
767-
}
768-
769-
@Override
770-
public InnerHitsContext innerHits() {
771-
return innerHitsContext;
772-
}
773-
774763
@Override
775764
public Map<Class<?>, Collector> queryCollectors() {
776765
return queryCollectors;

‎core/src/main/java/org/elasticsearch/search/internal/FilteredSearchContext.java

-5
Original file line numberDiff line numberDiff line change
@@ -177,11 +177,6 @@ public void highlight(SearchContextHighlight highlight) {
177177
in.highlight(highlight);
178178
}
179179

180-
@Override
181-
public void innerHits(InnerHitsContext innerHitsContext) {
182-
in.innerHits(innerHitsContext);
183-
}
184-
185180
@Override
186181
public InnerHitsContext innerHits() {
187182
return in.innerHits();

‎core/src/main/java/org/elasticsearch/search/internal/SearchContext.java

+7-3
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,7 @@ public static SearchContext current() {
8686

8787
private Map<Lifetime, List<Releasable>> clearables = null;
8888
private final AtomicBoolean closed = new AtomicBoolean(false);
89+
private InnerHitsContext innerHitsContext;
8990

9091
protected final ParseFieldMatcher parseFieldMatcher;
9192

@@ -168,9 +169,12 @@ public final boolean nowInMillisUsed() {
168169

169170
public abstract void highlight(SearchContextHighlight highlight);
170171

171-
public abstract void innerHits(InnerHitsContext innerHitsContext);
172-
173-
public abstract InnerHitsContext innerHits();
172+
public InnerHitsContext innerHits() {
173+
if (innerHitsContext == null) {
174+
innerHitsContext = new InnerHitsContext();
175+
}
176+
return innerHitsContext;
177+
}
174178

175179
public abstract SuggestionSearchContext suggest();
176180

‎core/src/main/java/org/elasticsearch/search/internal/SubSearchContext.java

-11
Original file line numberDiff line numberDiff line change
@@ -66,8 +66,6 @@ public class SubSearchContext extends FilteredSearchContext {
6666
private boolean trackScores;
6767
private boolean version;
6868

69-
private InnerHitsContext innerHitsContext;
70-
7169
public SubSearchContext(SearchContext context) {
7270
super(context);
7371
this.fetchSearchResult = new FetchSearchResult();
@@ -326,13 +324,4 @@ public Counter timeEstimateCounter() {
326324
throw new UnsupportedOperationException("Not supported");
327325
}
328326

329-
@Override
330-
public void innerHits(InnerHitsContext innerHitsContext) {
331-
this.innerHitsContext = innerHitsContext;
332-
}
333-
334-
@Override
335-
public InnerHitsContext innerHits() {
336-
return innerHitsContext;
337-
}
338327
}

‎core/src/test/java/org/elasticsearch/index/query/HasChildQueryBuilderTests.java

+1-13
Original file line numberDiff line numberDiff line change
@@ -85,18 +85,6 @@ protected void setSearchContext(String[] types) {
8585
final MapperService mapperService = queryShardContext().getMapperService();
8686
final IndexFieldDataService fieldData = indexFieldDataService();
8787
TestSearchContext testSearchContext = new TestSearchContext() {
88-
private InnerHitsContext context;
89-
90-
91-
@Override
92-
public void innerHits(InnerHitsContext innerHitsContext) {
93-
context = innerHitsContext;
94-
}
95-
96-
@Override
97-
public InnerHitsContext innerHits() {
98-
return context;
99-
}
10088

10189
@Override
10290
public MapperService mapperService() {
@@ -149,7 +137,7 @@ protected void doAssertLuceneQuery(HasChildQueryBuilder queryBuilder, Query quer
149137
assertEquals(innerHits.sort().getSort().length, 1);
150138
assertEquals(innerHits.sort().getSort()[0].getField(), STRING_FIELD_NAME);
151139
} else {
152-
assertNull(SearchContext.current().innerHits());
140+
assertThat(SearchContext.current().innerHits().getInnerHits().size(), equalTo(0));
153141
}
154142
}
155143
}

‎core/src/test/java/org/elasticsearch/index/query/HasParentQueryBuilderTests.java

+1-13
Original file line numberDiff line numberDiff line change
@@ -80,18 +80,6 @@ protected void setSearchContext(String[] types) {
8080
final MapperService mapperService = queryShardContext().getMapperService();
8181
final IndexFieldDataService fieldData = indexFieldDataService();
8282
TestSearchContext testSearchContext = new TestSearchContext() {
83-
private InnerHitsContext context;
84-
85-
86-
@Override
87-
public void innerHits(InnerHitsContext innerHitsContext) {
88-
context = innerHitsContext;
89-
}
90-
91-
@Override
92-
public InnerHitsContext innerHits() {
93-
return context;
94-
}
9583

9684
@Override
9785
public MapperService mapperService() {
@@ -139,7 +127,7 @@ protected void doAssertLuceneQuery(HasParentQueryBuilder queryBuilder, Query que
139127
assertEquals(innerHits.sort().getSort().length, 1);
140128
assertEquals(innerHits.sort().getSort()[0].getField(), STRING_FIELD_NAME);
141129
} else {
142-
assertNull(SearchContext.current().innerHits());
130+
assertThat(SearchContext.current().innerHits().getInnerHits().size(), equalTo(0));
143131
}
144132
}
145133
}

‎core/src/test/java/org/elasticsearch/index/query/NestedQueryBuilderTests.java

+5-16
Original file line numberDiff line numberDiff line change
@@ -27,16 +27,17 @@
2727
import org.elasticsearch.common.compress.CompressedXContent;
2828
import org.elasticsearch.index.fielddata.IndexFieldDataService;
2929
import org.elasticsearch.index.mapper.MapperService;
30-
import org.elasticsearch.index.query.support.QueryInnerHits;
31-
import org.elasticsearch.search.fetch.innerhits.InnerHitsBuilder;
32-
import org.elasticsearch.search.fetch.innerhits.InnerHitsContext;
3330
import org.elasticsearch.search.internal.SearchContext;
3431
import org.elasticsearch.search.sort.SortOrder;
3532
import org.elasticsearch.test.TestSearchContext;
33+
import org.elasticsearch.index.query.support.QueryInnerHits;
34+
import org.elasticsearch.search.fetch.innerhits.InnerHitsBuilder;
35+
import org.elasticsearch.search.fetch.innerhits.InnerHitsContext;
3636

3737
import java.io.IOException;
3838

3939
import static org.hamcrest.CoreMatchers.instanceOf;
40+
import static org.hamcrest.CoreMatchers.equalTo;
4041

4142
public class NestedQueryBuilderTests extends AbstractQueryTestCase<NestedQueryBuilder> {
4243

@@ -60,18 +61,6 @@ protected void setSearchContext(String[] types) {
6061
final MapperService mapperService = queryShardContext().getMapperService();
6162
final IndexFieldDataService fieldData = indexFieldDataService();
6263
TestSearchContext testSearchContext = new TestSearchContext() {
63-
private InnerHitsContext context;
64-
65-
66-
@Override
67-
public void innerHits(InnerHitsContext innerHitsContext) {
68-
context = innerHitsContext;
69-
}
70-
71-
@Override
72-
public InnerHitsContext innerHits() {
73-
return context;
74-
}
7564

7665
@Override
7766
public MapperService mapperService() {
@@ -119,7 +108,7 @@ protected void doAssertLuceneQuery(NestedQueryBuilder queryBuilder, Query query,
119108
assertEquals(innerHits.sort().getSort().length, 1);
120109
assertEquals(innerHits.sort().getSort()[0].getField(), STRING_FIELD_NAME);
121110
} else {
122-
assertNull(SearchContext.current().innerHits());
111+
assertThat(SearchContext.current().innerHits().getInnerHits().size(), equalTo(0));
123112
}
124113
}
125114
}

‎core/src/test/java/org/elasticsearch/index/query/ParentIdQueryBuilderTests.java

-12
Original file line numberDiff line numberDiff line change
@@ -65,18 +65,6 @@ protected void setSearchContext(String[] types) {
6565
final MapperService mapperService = queryShardContext().getMapperService();
6666
final IndexFieldDataService fieldData = indexFieldDataService();
6767
TestSearchContext testSearchContext = new TestSearchContext() {
68-
private InnerHitsContext context;
69-
70-
71-
@Override
72-
public void innerHits(InnerHitsContext innerHitsContext) {
73-
context = innerHitsContext;
74-
}
75-
76-
@Override
77-
public InnerHitsContext innerHits() {
78-
return context;
79-
}
8068

8169
@Override
8270
public MapperService mapperService() {

‎core/src/test/java/org/elasticsearch/search/innerhits/InnerHitsIT.java

+23
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,8 @@
2828
import org.elasticsearch.cluster.metadata.IndexMetaData;
2929
import org.elasticsearch.common.xcontent.XContentBuilder;
3030
import org.elasticsearch.index.query.BoolQueryBuilder;
31+
import org.elasticsearch.index.query.HasChildQueryBuilder;
32+
import org.elasticsearch.index.query.MatchAllQueryBuilder;
3133
import org.elasticsearch.index.query.support.QueryInnerHits;
3234
import org.elasticsearch.plugins.Plugin;
3335
import org.elasticsearch.script.MockScriptEngine;
@@ -1219,4 +1221,25 @@ public void testDontExplode() throws Exception {
12191221
assertHitCount(response, 1);
12201222
}
12211223

1224+
public void testTopLevelInnerHitsWithQueryInnerHits() throws Exception {
1225+
// top level inner hits shouldn't overwrite query inner hits definitions
1226+
1227+
assertAcked(prepareCreate("index1").addMapping("child", "_parent", "type=parent"));
1228+
List<IndexRequestBuilder> requests = new ArrayList<>();
1229+
requests.add(client().prepareIndex("index1", "parent", "1").setSource("{}"));
1230+
requests.add(client().prepareIndex("index1", "child", "2").setParent("1").setSource("{}"));
1231+
indexRandom(true, requests);
1232+
1233+
InnerHitsBuilder innerHitsBuilder = new InnerHitsBuilder();
1234+
innerHitsBuilder.addParentChildInnerHits("my-inner-hit", "child", new InnerHitsBuilder.InnerHit());
1235+
SearchResponse response = client().prepareSearch("index1")
1236+
.setQuery(hasChildQuery("child", new MatchAllQueryBuilder()).innerHit(new QueryInnerHits()))
1237+
.innerHits(innerHitsBuilder)
1238+
.get();
1239+
assertHitCount(response, 1);
1240+
assertThat(response.getHits().getAt(0).getInnerHits().size(), equalTo(2));
1241+
assertThat(response.getHits().getAt(0).getInnerHits().get("child").getAt(0).getId(), equalTo("2"));
1242+
assertThat(response.getHits().getAt(0).getInnerHits().get("my-inner-hit").getAt(0).getId(), equalTo("2"));
1243+
}
1244+
12221245
}

‎test/framework/src/main/java/org/elasticsearch/test/TestSearchContext.java

-10
Original file line numberDiff line numberDiff line change
@@ -582,16 +582,6 @@ public Counter timeEstimateCounter() {
582582
return timeEstimateCounter;
583583
}
584584

585-
@Override
586-
public void innerHits(InnerHitsContext innerHitsContext) {
587-
throw new UnsupportedOperationException();
588-
}
589-
590-
@Override
591-
public InnerHitsContext innerHits() {
592-
throw new UnsupportedOperationException();
593-
}
594-
595585
@Override
596586
public Profilers getProfilers() {
597587
return null; // no profiling

0 commit comments

Comments
 (0)