diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/300_pipeline.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/300_pipeline.yml new file mode 100644 index 00000000000..0016c9f9894 --- /dev/null +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/300_pipeline.yml @@ -0,0 +1,100 @@ +setup: + - skip: + version: " - 7.99.99" #TODO change this after backport + reason: These new error messages were added in 7.1 + + - do: + indices.create: + index: test_1 + body: + settings: + number_of_replicas: 0 + mappings: + properties: + int_field: + type : integer + + - do: + bulk: + refresh: true + body: + - index: + _index: test_1 + _id: 1 + - int_field: 1 + - index: + _index: test_1 + _id: 2 + - int_field: 2 + - index: + _index: test_1 + _id: 3 + - int_field: 3 + - index: + _index: test_1 + _id: 4 + - int_field: 4 + +--- +"Max pipeline through terms agg": + + - do: + catch: /\[Object\[\]\] at aggregation \[the_terms_2\]/ + search: + rest_total_hits_as_int: true + body: + aggs: + the_terms: + terms: + field: "int_field" + aggs: + the_terms_2: + terms: + field: "int_field" + aggs: + the_max: + max: + field: "int_field" + the_bad_max: + max_bucket: + buckets_path: "the_terms>the_terms_2>the_max" + +--- +"Max pipeline on terms agg": + + - do: + catch: /\[LongTerms\] at aggregation \[the_terms_2\]/ + search: + rest_total_hits_as_int: true + body: + aggs: + the_terms: + terms: + field: "int_field" + aggs: + the_terms_2: + terms: + field: "int_field" + the_bad_max: + max_bucket: + buckets_path: "the_terms>the_terms_2" + +--- +"Max pipeline on percentiles agg without specifying percent": + + - do: + catch: /buckets_path must reference either a number value or a single value numeric metric aggregation, but \[the_percentiles\] contains multiple values. Please specify which to use\./ + search: + rest_total_hits_as_int: true + body: + aggs: + the_terms: + terms: + field: "int_field" + aggs: + the_percentiles: + percentiles: + field: "int_field" + the_bad_max: + max_bucket: + buckets_path: "the_terms>the_percentiles" diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/BucketHelpers.java b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/BucketHelpers.java index d2c973ebec2..1a863a20982 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/BucketHelpers.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/BucketHelpers.java @@ -156,6 +156,7 @@ public class BucketHelpers { InternalMultiBucketAggregation.InternalBucket bucket, List aggPathAsList, GapPolicy gapPolicy) { try { Object propertyValue = bucket.getProperty(agg.getName(), aggPathAsList); + if (propertyValue == null) { throw new AggregationExecutionException(AbstractPipelineAggregationBuilder.BUCKETS_PATH_FIELD.getPreferredName() + " must reference either a number value or a single value numeric metric aggregation"); @@ -166,9 +167,7 @@ public class BucketHelpers { } else if (propertyValue instanceof InternalNumericMetricsAggregation.SingleValue) { value = ((InternalNumericMetricsAggregation.SingleValue) propertyValue).value(); } else { - throw new AggregationExecutionException(AbstractPipelineAggregationBuilder.BUCKETS_PATH_FIELD.getPreferredName() - + " must reference either a number value or a single value numeric metric aggregation, got: " - + propertyValue.getClass().getCanonicalName()); + throw formatResolutionError(agg, aggPathAsList, propertyValue); } // doc count never has missing values so gap policy doesn't apply here boolean isDocCountProperty = aggPathAsList.size() == 1 && "_count".equals(aggPathAsList.get(0)); @@ -188,4 +187,29 @@ public class BucketHelpers { return null; } } + + /** + * Inspects where we are in the agg tree and tries to format a helpful error + */ + private static AggregationExecutionException formatResolutionError(MultiBucketsAggregation agg, + List aggPathAsList, Object propertyValue) { + String currentAggName; + Object currentAgg; + if (aggPathAsList.isEmpty()) { + currentAggName = agg.getName(); + currentAgg = agg; + } else { + currentAggName = aggPathAsList.get(0); + currentAgg = propertyValue; + } + if (currentAgg instanceof InternalNumericMetricsAggregation.MultiValue) { + return new AggregationExecutionException(AbstractPipelineAggregationBuilder.BUCKETS_PATH_FIELD.getPreferredName() + + " must reference either a number value or a single value numeric metric aggregation, but [" + currentAggName + + "] contains multiple values. Please specify which to use."); + } else { + return new AggregationExecutionException(AbstractPipelineAggregationBuilder.BUCKETS_PATH_FIELD.getPreferredName() + + " must reference either a number value or a single value numeric metric aggregation, got: [" + + propertyValue.getClass().getSimpleName() + "] at aggregation [" + currentAggName + "]"); + } + } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/BucketHelpersTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/BucketHelpersTests.java new file mode 100644 index 00000000000..fbf8ad9d65a --- /dev/null +++ b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/BucketHelpersTests.java @@ -0,0 +1,185 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.search.aggregations.pipeline; + +import org.elasticsearch.common.io.stream.StreamOutput; +import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.search.aggregations.AggregationExecutionException; +import org.elasticsearch.search.aggregations.Aggregations; +import org.elasticsearch.search.aggregations.InternalMultiBucketAggregation; +import org.elasticsearch.search.aggregations.bucket.MultiBucketsAggregation; +import org.elasticsearch.search.aggregations.metrics.InternalTDigestPercentiles; +import org.elasticsearch.test.ESTestCase; + +import java.io.IOException; +import java.util.List; +import java.util.Map; + +import static org.hamcrest.Matchers.equalTo; +import static org.mockito.Mockito.mock; + +public class BucketHelpersTests extends ESTestCase { + + public void testReturnsObjectArray() { + + MultiBucketsAggregation agg = new MultiBucketsAggregation() { + @Override + public List getBuckets() { + return null; + } + + @Override + public String getName() { + return "foo"; + } + + @Override + public String getType() { + return null; + } + + @Override + public Map getMetaData() { + return null; + } + + @Override + public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { + return null; + } + }; + + InternalMultiBucketAggregation.InternalBucket bucket = new InternalMultiBucketAggregation.InternalBucket() { + @Override + public void writeTo(StreamOutput out) throws IOException { + + } + + @Override + public Object getKey() { + return null; + } + + @Override + public String getKeyAsString() { + return null; + } + + @Override + public long getDocCount() { + return 0; + } + + @Override + public Aggregations getAggregations() { + return null; + } + + @Override + public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { + return null; + } + + @Override + public Object getProperty(String containingAggName, List path) { + return new Object[0]; + } + }; + + AggregationExecutionException e = expectThrows(AggregationExecutionException.class, + () -> BucketHelpers.resolveBucketValue(agg, bucket, "foo>bar", BucketHelpers.GapPolicy.SKIP)); + + assertThat(e.getMessage(), equalTo("buckets_path must reference either a number value or a single value numeric " + + "metric aggregation, got: [Object[]] at aggregation [foo]")); + } + + public void testReturnMultiValueObject() { + + MultiBucketsAggregation agg = new MultiBucketsAggregation() { + @Override + public List getBuckets() { + return null; + } + + @Override + public String getName() { + return "foo"; + } + + @Override + public String getType() { + return null; + } + + @Override + public Map getMetaData() { + return null; + } + + @Override + public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { + return null; + } + }; + + InternalMultiBucketAggregation.InternalBucket bucket = new InternalMultiBucketAggregation.InternalBucket() { + @Override + public void writeTo(StreamOutput out) throws IOException { + + } + + @Override + public Object getKey() { + return null; + } + + @Override + public String getKeyAsString() { + return null; + } + + @Override + public long getDocCount() { + return 0; + } + + @Override + public Aggregations getAggregations() { + return null; + } + + @Override + public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { + return null; + } + + @Override + public Object getProperty(String containingAggName, List path) { + return mock(InternalTDigestPercentiles.class); + } + }; + + AggregationExecutionException e = expectThrows(AggregationExecutionException.class, + () -> BucketHelpers.resolveBucketValue(agg, bucket, "foo>bar", BucketHelpers.GapPolicy.SKIP)); + + assertThat(e.getMessage(), equalTo("buckets_path must reference either a number value or a single value numeric " + + "metric aggregation, but [foo] contains multiple values. Please specify which to use.")); + } +}