Better error messages when pipelines reference incompatible aggs (#40068)

Pipelines require single-valued agg or a numeric to be returned.
If they don't get that, they throw an exception.  Unfortunately, this
exception text is very confusing to users because it usually arises
from pathing "through" multiple terms aggs.  The final target is a numeric,
but it's the intermediary aggs that cause the problem.

This commit adds the current agg name to the exception message
so the user knows which "level" is the issue.

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"

server/src/main/java/org/elasticsearch/search/aggregations/pipeline/BucketHelpers.java

@@ -156,6 +156,7 @@ public class BucketHelpers {
             InternalMultiBucketAggregation.InternalBucket bucket, List<String> 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<String> 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 + "]");
+        }
+    }
 }

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<? extends Bucket> getBuckets() {
+                return null;
+            }
+
+            @Override
+            public String getName() {
+                return "foo";
+            }
+
+            @Override
+            public String getType() {
+                return null;
+            }
+
+            @Override
+            public Map<String, Object> 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<String> 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<? extends Bucket> getBuckets() {
+                return null;
+            }
+
+            @Override
+            public String getName() {
+                return "foo";
+            }
+
+            @Override
+            public String getType() {
+                return null;
+            }
+
+            @Override
+            public Map<String, Object> 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<String> 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."));
+    }
+}