From 915092dc28c9541d045791ad3d97c038fae769fe Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Tue, 7 Apr 2020 10:40:34 -0400 Subject: [PATCH] More pipeline aggregation cleanup (backport of #54298) (#54890) 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`. --- .../test/search.aggregation/300_pipeline.yml | 23 +++++++++ .../search/aggregations/AggregationPhase.java | 11 ----- .../PipelineAggregationBuilder.java | 15 ++++++ ...ucketScriptPipelineAggregationBuilder.java | 2 +- ...ketSelectorPipelineAggregationBuilder.java | 2 +- .../BucketSortPipelineAggregationBuilder.java | 1 + ...ScriptPipelineAggregationBuilderTests.java | 48 +++++++++++++++++++ .../pipeline/BucketSelectorTests.java | 8 ++++ .../pipeline/BucketSortTests.java | 10 +++- 9 files changed, 105 insertions(+), 15 deletions(-) create mode 100644 server/src/test/java/org/elasticsearch/search/aggregations/pipeline/BucketScriptPipelineAggregationBuilderTests.java 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 index 2e3d796383a..562b8c2c6e1 100644 --- 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 @@ -98,3 +98,26 @@ setup: the_bad_max: max_bucket: buckets_path: "the_terms>the_percentiles" + +--- +"Top level bucket_sort": + - skip: + version: " - 7.7.99" + reason: This validation was changed in 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 diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/AggregationPhase.java b/server/src/main/java/org/elasticsearch/search/aggregations/AggregationPhase.java index c0460106f0a..64c2785c54b 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/AggregationPhase.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/AggregationPhase.java @@ -24,8 +24,6 @@ import org.elasticsearch.common.inject.Inject; 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 class AggregationPhase implements SearchPhase { throw new AggregationExecutionException("Failed to build aggregation [" + aggregator.name() + "]", e); } } - List 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)); diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/PipelineAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/PipelineAggregationBuilder.java index c00652715bf..0f977b72e97 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/PipelineAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/PipelineAggregationBuilder.java @@ -120,6 +120,11 @@ public abstract class PipelineAggregationBuilder implements NamedWriteable, Base 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 abstract class PipelineAggregationBuilder implements NamedWriteable, Base 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 abstract class PipelineAggregationBuilder implements NamedWriteable, Base addValidationError(PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() + ' ' + error); } + /** + * Validates that there is a parent aggregation. + */ + public abstract void validateHasParent(String type, String name); + /** * Validates that the parent is sequentially ordered. */ diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/BucketScriptPipelineAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/BucketScriptPipelineAggregationBuilder.java index b0f5c468280..60930f76a3a 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/BucketScriptPipelineAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/BucketScriptPipelineAggregationBuilder.java @@ -200,7 +200,7 @@ public class BucketScriptPipelineAggregationBuilder extends AbstractPipelineAggr @Override protected void validate(ValidationContext context) { - // Nothing to check + context.validateHasParent(NAME, name); } @Override diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/BucketSelectorPipelineAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/BucketSelectorPipelineAggregationBuilder.java index 6fa39962146..0f21a6b0c8f 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/BucketSelectorPipelineAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/BucketSelectorPipelineAggregationBuilder.java @@ -195,7 +195,7 @@ public class BucketSelectorPipelineAggregationBuilder extends AbstractPipelineAg @Override protected void validate(ValidationContext context) { - // Nothing to check + context.validateHasParent(NAME, name); } @Override diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/BucketSortPipelineAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/BucketSortPipelineAggregationBuilder.java index 992664c985c..665fa773975 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/BucketSortPipelineAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/BucketSortPipelineAggregationBuilder.java @@ -141,6 +141,7 @@ public class BucketSortPipelineAggregationBuilder extends AbstractPipelineAggreg @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()) diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/BucketScriptPipelineAggregationBuilderTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/BucketScriptPipelineAggregationBuilderTests.java new file mode 100644 index 00000000000..2a2b7aeae55 --- /dev/null +++ b/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 { + @Override + protected BucketScriptPipelineAggregationBuilder createTestAggregatorFactory() { + Map 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;")); + } +} diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/BucketSelectorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/BucketSelectorTests.java index fb882786083..fba750e5f50 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/BucketSelectorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/BucketSelectorTests.java @@ -27,6 +27,10 @@ import org.elasticsearch.search.aggregations.pipeline.BucketHelpers.GapPolicy; 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 { @Override @@ -56,4 +60,8 @@ public class BucketSelectorTests extends BasePipelineAggregationTestCase { @@ -85,4 +85,10 @@ public class BucketSortTests extends BasePipelineAggregationTestCase new BucketSortPipelineAggregationBuilder("foo", Collections.emptyList()).gapPolicy(null)); assertThat(e.getMessage(), equalTo("[gap_policy] must not be null: [foo]")); } + + public void testNoParent() { + List 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;")); + } }