Skip to content

Commit 7df0567

Browse files
authored
Require the name field for inner_hits for collapse (#104666)
`name` is de facto required for `collapse.inner_hits`. It always has been, but we have never validated up front. Instead we accidentally try to serialize `null`, which leads to exciting and confusing errors. closes: #104647
1 parent a534751 commit 7df0567

File tree

4 files changed

+29
-2
lines changed

4 files changed

+29
-2
lines changed

docs/changelog/104666.yaml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
pr: 104666
2+
summary: Require the name field for `inner_hits` for collapse
3+
area: Search
4+
type: bug
5+
issues: []

rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search/110_field_collapsing.yml

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -515,3 +515,15 @@ setup:
515515
- match: { hits.hits.2.inner_hits.sub_hits.hits.hits.1._id: "4" }
516516
- gte: { hits.hits.2.inner_hits.sub_hits.hits.hits.1._seq_no: 0 }
517517
- gte: { hits.hits.2.inner_hits.sub_hits.hits.hits.1._primary_term: 1 }
518+
---
519+
"Test collapse with inner_hits and missing name fails":
520+
- skip:
521+
version: " - 8.12.99"
522+
reason: fixed in 8.13
523+
- do:
524+
catch: bad_request
525+
search:
526+
index: test
527+
body:
528+
collapse: { field: numeric_group, inner_hits: { size: 1 } }
529+
sort: [{ sort: desc }]

server/src/main/java/org/elasticsearch/search/collapse/CollapseBuilder.java

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -102,11 +102,21 @@ private CollapseBuilder setField(String field) {
102102
}
103103

104104
public CollapseBuilder setInnerHits(InnerHitBuilder innerHit) {
105+
if (innerHit.getName() == null) {
106+
throw new IllegalArgumentException("inner_hits must have a [name]; set the [name] field in the inner_hits definition");
107+
}
105108
this.innerHits = Collections.singletonList(innerHit);
106109
return this;
107110
}
108111

109112
public CollapseBuilder setInnerHits(List<InnerHitBuilder> innerHits) {
113+
if (innerHits != null) {
114+
for (InnerHitBuilder innerHit : innerHits) {
115+
if (innerHit.getName() == null) {
116+
throw new IllegalArgumentException("inner_hits must have a [name]; set the [name] field in the inner_hits definition");
117+
}
118+
}
119+
}
110120
this.innerHits = innerHits;
111121
return this;
112122
}

server/src/test/java/org/elasticsearch/search/collapse/CollapseBuilderTests.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -174,7 +174,7 @@ public void testBuild() throws IOException {
174174
null
175175
);
176176
when(searchExecutionContext.getFieldType("field")).thenReturn(numberFieldType);
177-
builder.setInnerHits(new InnerHitBuilder());
177+
builder.setInnerHits(new InnerHitBuilder().setName("field"));
178178
exc = expectThrows(IllegalArgumentException.class, () -> builder.build(searchExecutionContext));
179179
assertEquals(
180180
exc.getMessage(),
@@ -194,7 +194,7 @@ public void testBuild() throws IOException {
194194

195195
keywordFieldType = new KeywordFieldMapper.KeywordFieldType("field", false, true, Collections.emptyMap());
196196
when(searchExecutionContext.getFieldType("field")).thenReturn(keywordFieldType);
197-
kbuilder.setInnerHits(new InnerHitBuilder());
197+
kbuilder.setInnerHits(new InnerHitBuilder().setName("field"));
198198
exc = expectThrows(IllegalArgumentException.class, () -> builder.build(searchExecutionContext));
199199
assertEquals(
200200
exc.getMessage(),

0 commit comments

Comments
 (0)