More pipeline aggregation cleanup (#54298)

This replaces the last bit of validation that pipeline aggregations
performed on the data nodes with explicit checks in a few
`PipelineAggregationBuilders`. We were *already* catching these
validation errors for pipeline aggregations that require that their
parent be squentially ordered. This just adds validation for pipelines
that require *any* parent like `bucket_selector` and `bucket_sort`.

Author: nik9000

rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/300_pipeline.yml

@@ -98,3 +98,26 @@ setup:
             the_bad_max:
               max_bucket:
                 buckets_path: "the_terms>the_percentiles"
+
+---
+"Top level bucket_sort":
+  - skip:
+      version: " - 7.99.99"
+      reason:  This validation was changed in 8.0.0 to be backported to 7.8.0
+
+  - do:
+      catch: /bucket_sort aggregation \[the_bad_bucket_sort\] must be declared inside of another aggregation/
+      search:
+        body:
+          aggs:
+            the_terms:
+              terms:
+                field: "int_field"
+              aggs:
+                the_max:
+                  max:
+                    field: "int_field"
+            the_bad_bucket_sort:
+              bucket_sort:
+                sort:
+                  - the_terms>the_max

server/src/main/java/org/elasticsearch/search/aggregations/AggregationPhase.java

@@ -24,8 +24,6 @@
 import org.elasticsearch.common.lucene.search.Queries;
 import org.elasticsearch.search.SearchPhase;
 import org.elasticsearch.search.aggregations.bucket.global.GlobalAggregator;
-import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator;
-import org.elasticsearch.search.aggregations.pipeline.SiblingPipelineAggregator;
 import org.elasticsearch.search.internal.SearchContext;
 import org.elasticsearch.search.profile.query.CollectorResult;
 import org.elasticsearch.search.profile.query.InternalProfileCollector;
@@ -132,15 +130,6 @@ public void execute(SearchContext context) {
                 throw new AggregationExecutionException("Failed to build aggregation [" + aggregator.name() + "]", e);
             }
         }
-        List<PipelineAggregator> pipelineAggregators = context.aggregations().factories().createPipelineAggregators();
-        for (PipelineAggregator pipelineAggregator : pipelineAggregators) {
-            if (false == pipelineAggregator instanceof SiblingPipelineAggregator) {
-                // TODO move this to request validation after #53669
-                throw new AggregationExecutionException("Invalid pipeline aggregation named [" + pipelineAggregator.name()
-                    + "] of type [" + pipelineAggregator.getWriteableName() + "]. Only sibling pipeline aggregations are "
-                    + "allowed at the top level");
-            }
-        }
         context.queryResult().aggregations(new InternalAggregations(aggregations,
                 context.request().source().aggregations()::buildPipelineTree));
 

server/src/main/java/org/elasticsearch/search/aggregations/PipelineAggregationBuilder.java

@@ -120,6 +120,11 @@ public Collection<PipelineAggregationBuilder> getSiblingPipelineAggregations() {
                 return siblingPipelineAggregations;
             }
 
+            @Override
+            public void validateHasParent(String type, String name) {
+                addValidationError(type + " aggregation [" + name + "] must be declared inside of another aggregation");
+            }
+
             @Override
             public void validateParentAggSequentiallyOrdered(String type, String name) {
                 addValidationError(type + " aggregation [" + name
@@ -145,6 +150,11 @@ public Collection<PipelineAggregationBuilder> getSiblingPipelineAggregations() {
                 return parent.getPipelineAggregations();
             }
 
+            @Override
+            public void validateHasParent(String type, String name) {
+                // There is a parent inside the tree.
+            }
+
             @Override
             public void validateParentAggSequentiallyOrdered(String type, String name) {
                 if (parent instanceof HistogramAggregationBuilder) {
@@ -195,6 +205,11 @@ public void addBucketPathValidationError(String error) {
             addValidationError(PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() + ' ' + error);
         }
 
+        /**
+         * Validates that there <strong>is</strong> a parent aggregation.
+         */
+        public abstract void validateHasParent(String type, String name);
+
         /**
          * Validates that the parent is sequentially ordered.
          */

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

@@ -200,7 +200,7 @@ protected XContentBuilder internalXContent(XContentBuilder builder, Params param
 
     @Override
     protected void validate(ValidationContext context) {
-        // Nothing to check
+        context.validateHasParent(NAME, name);
     }
 
     @Override

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

@@ -195,7 +195,7 @@ public static BucketSelectorPipelineAggregationBuilder parse(String reducerName,
 
     @Override
     protected void validate(ValidationContext context) {
-        // Nothing to check
+        context.validateHasParent(NAME, name);
     }
 
     @Override

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

@@ -141,6 +141,7 @@ protected PipelineAggregator createInternal(Map<String, Object> metadata) {
 
     @Override
     protected void validate(ValidationContext context) {
+        context.validateHasParent(NAME, name);
         if (sorts.isEmpty() && size == null && from == 0) {
             context.addValidationError("[" + name + "] is configured to perform nothing. Please set either of "
                     + Arrays.asList(SearchSourceBuilder.SORT_FIELD.getPreferredName(), SIZE.getPreferredName(), FROM.getPreferredName())

server/src/test/java/org/elasticsearch/search/aggregations/pipeline/BucketScriptPipelineAggregationBuilderTests.java

@@ -0,0 +1,48 @@
+/*
+ * 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.script.Script;
+import org.elasticsearch.search.aggregations.BasePipelineAggregationTestCase;
+
+import java.util.HashMap;
+import java.util.Map;
+
+import static java.util.Collections.emptyList;
+import static java.util.Collections.emptyMap;
+import static org.hamcrest.CoreMatchers.equalTo;
+
+public class BucketScriptPipelineAggregationBuilderTests extends BasePipelineAggregationTestCase<BucketScriptPipelineAggregationBuilder> {
+    @Override
+    protected BucketScriptPipelineAggregationBuilder createTestAggregatorFactory() {
+        Map<String, String> bucketsPathsMap = new HashMap<>();
+        int targetBucketsPathsMapSize = randomInt(5);
+        while (bucketsPathsMap.size() < targetBucketsPathsMapSize) {
+            bucketsPathsMap.put(randomAlphaOfLength(5), randomAlphaOfLength(5));
+        }
+        Script script = new Script(randomAlphaOfLength(4));
+        return new BucketScriptPipelineAggregationBuilder(randomAlphaOfLength(3), bucketsPathsMap, script);
+    }
+
+    public void testNoParent() {
+        assertThat(validate(emptyList(), new BucketScriptPipelineAggregationBuilder("foo", emptyMap(), new Script("foo"))), 
+            equalTo("Validation Failed: 1: bucket_script aggregation [foo] must be declared inside of another aggregation;"));
+    }
+}

server/src/test/java/org/elasticsearch/search/aggregations/pipeline/BucketSelectorTests.java

@@ -27,6 +27,10 @@
 import java.util.HashMap;
 import java.util.Map;
 
+import static java.util.Collections.emptyList;
+import static java.util.Collections.emptyMap;
+import static org.hamcrest.CoreMatchers.equalTo;
+
 public class BucketSelectorTests extends BasePipelineAggregationTestCase<BucketSelectorPipelineAggregationBuilder> {
 
     @Override
@@ -56,4 +60,8 @@ protected BucketSelectorPipelineAggregationBuilder createTestAggregatorFactory()
         return factory;
     }
 
+    public void testNoParent() {
+        assertThat(validate(emptyList(), new BucketSelectorPipelineAggregationBuilder("foo", emptyMap(), new Script("foo"))), 
+            equalTo("Validation Failed: 1: bucket_selector aggregation [foo] must be declared inside of another aggregation;"));
+    }
 }

server/src/test/java/org/elasticsearch/search/aggregations/pipeline/BucketSortTests.java

@@ -19,15 +19,15 @@
 package org.elasticsearch.search.aggregations.pipeline;
 
 import org.elasticsearch.search.aggregations.BasePipelineAggregationTestCase;
-import org.elasticsearch.search.aggregations.pipeline.BucketHelpers;
-import org.elasticsearch.search.aggregations.pipeline.BucketSortPipelineAggregationBuilder;
 import org.elasticsearch.search.sort.FieldSortBuilder;
 import org.elasticsearch.search.sort.SortOrder;
 
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
 
+import static java.util.Collections.emptyList;
+import static java.util.Collections.singletonList;
 import static org.hamcrest.Matchers.equalTo;
 
 public class BucketSortTests extends BasePipelineAggregationTestCase<BucketSortPipelineAggregationBuilder> {
@@ -85,4 +85,10 @@ public void testNullGapPolicy() {
                 () -> new BucketSortPipelineAggregationBuilder("foo", Collections.emptyList()).gapPolicy(null));
         assertThat(e.getMessage(), equalTo("[gap_policy] must not be null: [foo]"));
     }
+
+    public void testNoParent() {
+        List<FieldSortBuilder> sorts = singletonList(new FieldSortBuilder("bar"));
+        assertThat(validate(emptyList(), new BucketSortPipelineAggregationBuilder("foo", sorts)), 
+            equalTo("Validation Failed: 1: bucket_sort aggregation [foo] must be declared inside of another aggregation;"));
+    }
 }