Aggs must specify a `field` or `script` (or both) (#52226)

This adds a validation to VSParserHelper to ensure that a field or
script or both are specified by the user.  This is technically
required today already, but throws an exception much deeper
in the agg framework and has a very unintuitive error for the user
(as well as eating more resources instead of failing early)
This commit is contained in:
Zachary Tong 2020-04-23 14:26:38 -04:00 committed by Zachary Tong
parent c857adf603
commit 715c90bf7d
8 changed files with 27 additions and 33 deletions

View File

@ -379,30 +379,3 @@ Response:
} }
-------------------------------------------------- --------------------------------------------------
// TESTRESPONSE[s/\.\.\.//] // TESTRESPONSE[s/\.\.\.//]
If a sub aggregation is also based on the same value source as the range aggregation (like the `stats` aggregation in the example above) it is possible to leave out the value source definition for it. The following will return the same response as above:
[source,console]
--------------------------------------------------
GET /_search
{
"aggs" : {
"price_ranges" : {
"range" : {
"field" : "price",
"ranges" : [
{ "to" : 100 },
{ "from" : 100, "to" : 200 },
{ "from" : 200 }
]
},
"aggs" : {
"price_stats" : {
"stats" : {} <1>
}
}
}
}
}
--------------------------------------------------
<1> We don't need to specify the `price` as we "inherit" it by default from the parent `range` aggregation

View File

@ -765,6 +765,18 @@ setup:
- gt: { indices.test_1.total.fielddata.memory_size_in_bytes: 0} - gt: { indices.test_1.total.fielddata.memory_size_in_bytes: 0}
---
"No field or script":
- skip:
version: " - 7.7.99"
reason: "Exception was a deep, server exception before 7.8"
- do:
catch: /Required one of fields \[field, script\], but none were specified/
search:
rest_total_hits_as_int: true
index: test_1
body: { "size" : 0, "aggs" : { "no_field_terms" : { "terms" : { "size": 1 } } } }

View File

@ -66,6 +66,10 @@ public abstract class ValuesSourceAggregationBuilder<AB extends ValuesSourceAggr
objectParser.declareField(ValuesSourceAggregationBuilder::script, objectParser.declareField(ValuesSourceAggregationBuilder::script,
(parser, context) -> Script.parse(parser), (parser, context) -> Script.parse(parser),
Script.SCRIPT_PARSE_FIELD, ObjectParser.ValueType.OBJECT_OR_STRING); Script.SCRIPT_PARSE_FIELD, ObjectParser.ValueType.OBJECT_OR_STRING);
String[] fields = new String[]{ParseField.CommonFields.FIELD.getPreferredName(), Script.SCRIPT_PARSE_FIELD.getPreferredName()};
objectParser.declareRequiredFieldSet(fields);
} else {
objectParser.declareRequiredFieldSet(ParseField.CommonFields.FIELD.getPreferredName());
} }
if (timezoneAware) { if (timezoneAware) {

View File

@ -139,13 +139,13 @@ public class AggregatorFactoriesBuilderTests extends AbstractSerializingTestCase
final int randomAggregatorPoolSize = 4; final int randomAggregatorPoolSize = 4;
switch (randomIntBetween(1, randomAggregatorPoolSize)) { switch (randomIntBetween(1, randomAggregatorPoolSize)) {
case 1: case 1:
return AggregationBuilders.avg(randomAlphaOfLengthBetween(3, 10)); return AggregationBuilders.avg(randomAlphaOfLengthBetween(3, 10)).field("foo");
case 2: case 2:
return AggregationBuilders.min(randomAlphaOfLengthBetween(3, 10)); return AggregationBuilders.min(randomAlphaOfLengthBetween(3, 10)).field("foo");
case 3: case 3:
return AggregationBuilders.max(randomAlphaOfLengthBetween(3, 10)); return AggregationBuilders.max(randomAlphaOfLengthBetween(3, 10)).field("foo");
case 4: case 4:
return AggregationBuilders.sum(randomAlphaOfLengthBetween(3, 10)); return AggregationBuilders.sum(randomAlphaOfLengthBetween(3, 10)).field("foo");
} }
// never reached // never reached

View File

@ -42,6 +42,7 @@ public class GeoHashGridTests extends BaseAggregationTestCase<GeoGridAggregation
protected GeoHashGridAggregationBuilder createTestAggregatorBuilder() { protected GeoHashGridAggregationBuilder createTestAggregatorBuilder() {
String name = randomAlphaOfLengthBetween(3, 20); String name = randomAlphaOfLengthBetween(3, 20);
GeoHashGridAggregationBuilder factory = new GeoHashGridAggregationBuilder(name); GeoHashGridAggregationBuilder factory = new GeoHashGridAggregationBuilder(name);
factory.field("foo");
if (randomBoolean()) { if (randomBoolean()) {
int precision = randomIntBetween(1, 12); int precision = randomIntBetween(1, 12);
factory.precision(precision); factory.precision(precision);

View File

@ -43,6 +43,7 @@ public class GeoTileGridTests extends BaseAggregationTestCase<GeoGridAggregation
protected GeoTileGridAggregationBuilder createTestAggregatorBuilder() { protected GeoTileGridAggregationBuilder createTestAggregatorBuilder() {
String name = randomAlphaOfLengthBetween(3, 20); String name = randomAlphaOfLengthBetween(3, 20);
GeoTileGridAggregationBuilder factory = new GeoTileGridAggregationBuilder(name); GeoTileGridAggregationBuilder factory = new GeoTileGridAggregationBuilder(name);
factory.field("foo");
if (randomBoolean()) { if (randomBoolean()) {
factory.precision(randomIntBetween(0, GeoTileUtils.MAX_ZOOM)); factory.precision(randomIntBetween(0, GeoTileUtils.MAX_ZOOM));
} }

View File

@ -348,7 +348,7 @@ public class RandomSearchRequestGenerator {
} }
} }
if (randomBoolean()) { if (randomBoolean()) {
builder.aggregation(AggregationBuilders.avg(randomAlphaOfLengthBetween(5, 20))); builder.aggregation(AggregationBuilders.avg(randomAlphaOfLengthBetween(5, 20)).field("foo"));
} }
if (randomBoolean()) { if (randomBoolean()) {
builder.ext(randomExtBuilders.get()); builder.ext(randomExtBuilders.get());

View File

@ -54,6 +54,7 @@ public class StringStatsAggregationBuilderTests extends AbstractSerializingTestC
@Override @Override
protected StringStatsAggregationBuilder createTestInstance() { protected StringStatsAggregationBuilder createTestInstance() {
StringStatsAggregationBuilder builder = new StringStatsAggregationBuilder(randomAlphaOfLength(5)); StringStatsAggregationBuilder builder = new StringStatsAggregationBuilder(randomAlphaOfLength(5));
builder.field("foo");
builder.showDistribution(randomBoolean()); builder.showDistribution(randomBoolean());
return builder; return builder;
} }
@ -92,6 +93,8 @@ public class StringStatsAggregationBuilderTests extends AbstractSerializingTestC
StringStatsAggregationBuilder serverBuilder) { StringStatsAggregationBuilder serverBuilder) {
org.elasticsearch.client.analytics.StringStatsAggregationBuilder builder = org.elasticsearch.client.analytics.StringStatsAggregationBuilder builder =
new org.elasticsearch.client.analytics.StringStatsAggregationBuilder(serverBuilder.getName()); new org.elasticsearch.client.analytics.StringStatsAggregationBuilder(serverBuilder.getName());
return builder.showDistribution(serverBuilder.showDistribution()); return builder
.showDistribution(serverBuilder.showDistribution())
.field(serverBuilder.field());
} }
} }