From 715c90bf7d6fd03d1c1e1bc72a6099d2a71fe1f3 Mon Sep 17 00:00:00 2001 From: Zachary Tong Date: Thu, 23 Apr 2020 14:26:38 -0400 Subject: [PATCH] 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) --- .../bucket/range-aggregation.asciidoc | 27 ------------------- .../test/search.aggregation/20_terms.yml | 12 +++++++++ .../ValuesSourceAggregationBuilder.java | 4 +++ .../AggregatorFactoriesBuilderTests.java | 8 +++--- .../aggregations/bucket/GeoHashGridTests.java | 1 + .../aggregations/bucket/GeoTileGridTests.java | 1 + .../search/RandomSearchRequestGenerator.java | 2 +- .../StringStatsAggregationBuilderTests.java | 5 +++- 8 files changed, 27 insertions(+), 33 deletions(-) diff --git a/docs/reference/aggregations/bucket/range-aggregation.asciidoc b/docs/reference/aggregations/bucket/range-aggregation.asciidoc index b8de777e37a..df8d72070fe 100644 --- a/docs/reference/aggregations/bucket/range-aggregation.asciidoc +++ b/docs/reference/aggregations/bucket/range-aggregation.asciidoc @@ -379,30 +379,3 @@ Response: } -------------------------------------------------- // 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 diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/20_terms.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/20_terms.yml index 3d9f8a9c8af..e992bba7935 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/20_terms.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/20_terms.yml @@ -765,6 +765,18 @@ setup: - 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 } } } } + diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceAggregationBuilder.java index 3f8aa17c2e5..526238c6331 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceAggregationBuilder.java @@ -66,6 +66,10 @@ public abstract class ValuesSourceAggregationBuilder Script.parse(parser), 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) { diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/AggregatorFactoriesBuilderTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/AggregatorFactoriesBuilderTests.java index 4cbc78acad1..e92380543af 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/AggregatorFactoriesBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/AggregatorFactoriesBuilderTests.java @@ -139,13 +139,13 @@ public class AggregatorFactoriesBuilderTests extends AbstractSerializingTestCase final int randomAggregatorPoolSize = 4; switch (randomIntBetween(1, randomAggregatorPoolSize)) { case 1: - return AggregationBuilders.avg(randomAlphaOfLengthBetween(3, 10)); + return AggregationBuilders.avg(randomAlphaOfLengthBetween(3, 10)).field("foo"); case 2: - return AggregationBuilders.min(randomAlphaOfLengthBetween(3, 10)); + return AggregationBuilders.min(randomAlphaOfLengthBetween(3, 10)).field("foo"); case 3: - return AggregationBuilders.max(randomAlphaOfLengthBetween(3, 10)); + return AggregationBuilders.max(randomAlphaOfLengthBetween(3, 10)).field("foo"); case 4: - return AggregationBuilders.sum(randomAlphaOfLengthBetween(3, 10)); + return AggregationBuilders.sum(randomAlphaOfLengthBetween(3, 10)).field("foo"); } // never reached diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoHashGridTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoHashGridTests.java index 01ef307cefb..7fe5013e8db 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoHashGridTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoHashGridTests.java @@ -42,6 +42,7 @@ public class GeoHashGridTests extends BaseAggregationTestCase