From db7beb0535002062b21bd3ec7dc2d0b282530a28 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Mon, 23 Mar 2020 14:39:30 -0400 Subject: [PATCH] Move pipeline agg validation to coordinating node (backport of #53669) This moves the pipeline aggregation validation from the data node to the coordinating node so that we, eventually, can stop sending pipeline aggregations to the data nodes entirely. In fact, it moves it into the "request validation" stage so multiple errors can be accumulated and sent back to the requester for the entire request. We can't always take advantage of that, but it'll be nice for folks not to have to play whack-a-mole with validation. This is implemented by replacing `PipelineAggretionBuilder#validate` with: ``` protected abstract void validate(ValidationContext context); ``` The `ValidationContext` handles the accumulation of validation failures, provides access to the aggregation's siblings, and implements a few validation utility methods. --- .../action/search/SearchRequest.java | 5 + .../aggregations/AggregatorFactories.java | 51 +++++- .../PipelineAggregationBuilder.java | 148 +++++++++++++++++- .../AbstractPipelineAggregationBuilder.java | 42 ----- ...cketMetricsPipelineAggregationBuilder.java | 30 ++-- ...ucketScriptPipelineAggregationBuilder.java | 5 + ...ketSelectorPipelineAggregationBuilder.java | 5 + .../BucketSortPipelineAggregationBuilder.java | 9 +- ...mulativeSumPipelineAggregationBuilder.java | 13 +- .../DerivativePipelineAggregationBuilder.java | 13 +- ...StatsBucketPipelineAggregationBuilder.java | 15 +- .../MovAvgPipelineAggregationBuilder.java | 37 ++--- .../MovFnPipelineAggregationBuilder.java | 12 +- ...tilesBucketPipelineAggregationBuilder.java | 13 +- .../SerialDiffPipelineAggregationBuilder.java | 11 +- .../search/SearchModuleTests.java | 3 + .../AbstractBucketMetricsTestCase.java | 1 - .../aggregations/pipeline/AvgBucketTests.java | 25 ++- .../aggregations/pipeline/BucketSortIT.java | 10 +- .../CumulativeSumAggregatorTests.java | 33 ---- .../pipeline/CumulativeSumTests.java | 29 +++- .../pipeline/DerivativeTests.java | 28 ++-- .../pipeline/ExtendedStatsBucketTests.java | 24 ++- .../aggregations/pipeline/MaxBucketTests.java | 25 ++- .../aggregations/pipeline/MinBucketTests.java | 25 ++- .../aggregations/pipeline/MovAvgIT.java | 53 ++++--- .../aggregations/pipeline/MovAvgTests.java | 31 ++-- ...nitTests.java => MovFnAggrgatorTests.java} | 36 +---- ...eAggregationBuilderSerializationTests.java | 42 +++-- .../pipeline/PercentilesBucketTests.java | 24 ++- .../PipelineAggregationHelperTests.java | 47 ++---- .../pipeline/SerialDifferenceTests.java | 41 ++--- .../pipeline/StatsBucketTests.java | 25 ++- .../aggregations/pipeline/SumBucketTests.java | 25 ++- .../BasePipelineAggregationTestCase.java | 58 +++++-- ...CardinalityPipelineAggregationBuilder.java | 19 ++- .../analytics/StubAggregatorFactory.java | 56 ------- .../CumulativeCardinalityAggregatorTests.java | 66 -------- .../CumulativeCardinalityTests.java | 73 +++++++++ 39 files changed, 607 insertions(+), 601 deletions(-) rename server/src/test/java/org/elasticsearch/search/aggregations/pipeline/{MovFnUnitTests.java => MovFnAggrgatorTests.java} (79%) rename {server/src/test => test/framework/src/main}/java/org/elasticsearch/search/aggregations/BasePipelineAggregationTestCase.java (86%) delete mode 100644 x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/StubAggregatorFactory.java create mode 100644 x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/cumulativecardinality/CumulativeCardinalityTests.java diff --git a/server/src/main/java/org/elasticsearch/action/search/SearchRequest.java b/server/src/main/java/org/elasticsearch/action/search/SearchRequest.java index 90a2fbfdd2b44..a1fbbf8e930ef 100644 --- a/server/src/main/java/org/elasticsearch/action/search/SearchRequest.java +++ b/server/src/main/java/org/elasticsearch/action/search/SearchRequest.java @@ -277,6 +277,11 @@ public ActionRequestValidationException validate() { addValidationError("[request_cache] cannot be used in a scroll context", validationException); } } + if (source != null) { + if (source.aggregations() != null) { + validationException = source.aggregations().validate(validationException); + } + } return validationException; } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactories.java b/server/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactories.java index 1998a53ad06b0..397c6ea762cf1 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactories.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactories.java @@ -18,6 +18,7 @@ */ package org.elasticsearch.search.aggregations; +import org.elasticsearch.action.ActionRequestValidationException; import org.elasticsearch.common.ParsingException; import org.elasticsearch.common.Strings; import org.elasticsearch.common.io.stream.StreamInput; @@ -283,8 +284,6 @@ public boolean mustVisitAllDocs() { return false; } - - public Builder addAggregator(AggregationBuilder factory) { if (!names.add(factory.name)) { throw new IllegalArgumentException("Two sibling aggregations cannot have the same name: [" + factory.name + "]"); @@ -298,15 +297,51 @@ public Builder addPipelineAggregator(PipelineAggregationBuilder pipelineAggregat return this; } + /** + * Validate the root of the aggregation tree. + */ + public ActionRequestValidationException validate(ActionRequestValidationException e) { + PipelineAggregationBuilder.ValidationContext context = + PipelineAggregationBuilder.ValidationContext.forTreeRoot(aggregationBuilders, pipelineAggregatorBuilders, e); + validatePipelines(context); + return validateChildren(context.getValidationException()); + } + + /** + * Validate a the pipeline aggregations in this factory. + */ + private void validatePipelines(PipelineAggregationBuilder.ValidationContext context) { + List orderedPipelineAggregators; + try { + orderedPipelineAggregators = resolvePipelineAggregatorOrder(pipelineAggregatorBuilders, aggregationBuilders); + } catch (IllegalArgumentException iae) { + context.addValidationError(iae.getMessage()); + return; + } + for (PipelineAggregationBuilder builder : orderedPipelineAggregators) { + builder.validate(context); + } + } + + /** + * Validate a the children of this factory. + */ + private ActionRequestValidationException validateChildren(ActionRequestValidationException e) { + for (AggregationBuilder agg : aggregationBuilders) { + PipelineAggregationBuilder.ValidationContext context = + PipelineAggregationBuilder.ValidationContext.forInsideTree(agg, e); + agg.factoriesBuilder.validatePipelines(context); + e = agg.factoriesBuilder.validateChildren(context.getValidationException()); + } + return e; + } + public AggregatorFactories build(QueryShardContext queryShardContext, AggregatorFactory parent) throws IOException { if (aggregationBuilders.isEmpty() && pipelineAggregatorBuilders.isEmpty()) { return EMPTY; } - List orderedpipelineAggregators = null; - orderedpipelineAggregators = resolvePipelineAggregatorOrder(this.pipelineAggregatorBuilders, this.aggregationBuilders); - for (PipelineAggregationBuilder builder : orderedpipelineAggregators) { - builder.validate(parent, aggregationBuilders, pipelineAggregatorBuilders); - } + List orderedPipelineAggregators = + resolvePipelineAggregatorOrder(pipelineAggregatorBuilders, aggregationBuilders); AggregatorFactory[] aggFactories = new AggregatorFactory[aggregationBuilders.size()]; int i = 0; @@ -314,7 +349,7 @@ public AggregatorFactories build(QueryShardContext queryShardContext, Aggregator aggFactories[i] = agg.build(queryShardContext, parent); ++i; } - return new AggregatorFactories(aggFactories, orderedpipelineAggregators); + return new AggregatorFactories(aggFactories, orderedPipelineAggregators); } private List resolvePipelineAggregatorOrder( 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 85b9a7dcb0ca5..aaffcfc440bce 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/PipelineAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/PipelineAggregationBuilder.java @@ -18,14 +18,20 @@ */ package org.elasticsearch.search.aggregations; +import org.elasticsearch.action.ActionRequestValidationException; +import org.elasticsearch.action.ValidateActions; import org.elasticsearch.common.Strings; import org.elasticsearch.common.io.stream.NamedWriteable; import org.elasticsearch.common.xcontent.ToXContentFragment; import org.elasticsearch.search.aggregations.AggregatorFactories.Builder; +import org.elasticsearch.search.aggregations.bucket.histogram.AutoDateHistogramAggregationBuilder; +import org.elasticsearch.search.aggregations.bucket.histogram.DateHistogramAggregationBuilder; +import org.elasticsearch.search.aggregations.bucket.histogram.HistogramAggregationBuilder; import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; import java.util.Collection; import java.util.Map; +import java.util.Objects; /** * A factory that knows how to create an {@link PipelineAggregator} of a @@ -64,11 +70,145 @@ public final String[] getBucketsPaths() { } /** - * Internal: Validates the state of this factory (makes sure the factory is properly - * configured) + * Makes sure this builder is properly configured. */ - protected abstract void validate(AggregatorFactory parent, Collection aggregationBuilders, - Collection pipelineAggregatorBuilders); + protected abstract void validate(ValidationContext context); + public abstract static class ValidationContext { + /** + * Build the context for the root of the aggregation tree. + */ + public static ValidationContext forTreeRoot(Collection siblingAggregations, + Collection siblingPipelineAggregations, + ActionRequestValidationException validationFailuresSoFar) { + return new ForTreeRoot(siblingAggregations, siblingPipelineAggregations, validationFailuresSoFar); + } + + /** + * Build the context for a node inside the aggregation tree. + */ + public static ValidationContext forInsideTree(AggregationBuilder parent, + ActionRequestValidationException validationFailuresSoFar) { + return new ForInsideTree(parent, validationFailuresSoFar); + } + + + private ActionRequestValidationException e; + + private ValidationContext(ActionRequestValidationException validationFailuresSoFar) { + this.e = validationFailuresSoFar; + } + + private static class ForTreeRoot extends ValidationContext { + private final Collection siblingAggregations; + private final Collection siblingPipelineAggregations; + + ForTreeRoot(Collection siblingAggregations, + Collection siblingPipelineAggregations, + ActionRequestValidationException validationFailuresSoFar) { + super(validationFailuresSoFar); + this.siblingAggregations = Objects.requireNonNull(siblingAggregations); + this.siblingPipelineAggregations = Objects.requireNonNull(siblingPipelineAggregations); + } + + @Override + public Collection getSiblingAggregations() { + return siblingAggregations; + } + + @Override + public Collection getSiblingPipelineAggregations() { + return siblingPipelineAggregations; + } + + @Override + public void validateParentAggSequentiallyOrdered(String type, String name) { + addValidationError(type + " aggregation [" + name + + "] must have a histogram, date_histogram or auto_date_histogram as parent but doesn't have a parent"); + } + } + + private static class ForInsideTree extends ValidationContext { + private final AggregationBuilder parent; + + ForInsideTree(AggregationBuilder parent, ActionRequestValidationException validationFailuresSoFar) { + super(validationFailuresSoFar); + this.parent = Objects.requireNonNull(parent); + } + + @Override + public Collection getSiblingAggregations() { + return parent.getSubAggregations(); + } + + @Override + public Collection getSiblingPipelineAggregations() { + return parent.getPipelineAggregations(); + } + + @Override + public void validateParentAggSequentiallyOrdered(String type, String name) { + if (parent instanceof HistogramAggregationBuilder) { + HistogramAggregationBuilder histoParent = (HistogramAggregationBuilder) parent; + if (histoParent.minDocCount() != 0) { + addValidationError( + "parent histogram of " + type + " aggregation [" + name + "] must have min_doc_count of 0"); + } + } else if (parent instanceof DateHistogramAggregationBuilder) { + DateHistogramAggregationBuilder histoParent = (DateHistogramAggregationBuilder) parent; + if (histoParent.minDocCount() != 0) { + addValidationError( + "parent histogram of " + type + " aggregation [" + name + "] must have min_doc_count of 0"); + } + } else if (parent instanceof AutoDateHistogramAggregationBuilder) { + // Nothing to check + } else { + addValidationError( + type + " aggregation [" + name + "] must have a histogram, date_histogram or auto_date_histogram as parent"); + } + } + } + + /** + * Aggregations that are siblings to the aggregation being validated. + */ + public abstract Collection getSiblingAggregations(); + + /** + * Pipeline aggregations that are siblings to the aggregation being validated. + */ + public abstract Collection getSiblingPipelineAggregations(); + + /** + * Add a validation error to this context. All validation errors + * are accumulated in a list and, if there are any, the request + * is not executed and the entire list is returned as the error + * response. + */ + public void addValidationError(String error) { + e = ValidateActions.addValidationError(error, e); + } + + /** + * Add a validation error about the {@code buckets_path}. + */ + public void addBucketPathValidationError(String error) { + addValidationError(PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() + ' ' + error); + } + + /** + * Validates that the parent is sequentially ordered. + */ + public abstract void validateParentAggSequentiallyOrdered(String type, String name); + + /** + * The validation exception, if there is one. It'll be {@code null} + * if the context wasn't provided with any exception on creation + * and none were added. + */ + public ActionRequestValidationException getValidationException() { + return e; + } + } /** * Creates the pipeline aggregator diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/AbstractPipelineAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/AbstractPipelineAggregationBuilder.java index fa2f8c8090bcd..5a1eff4c605f9 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/AbstractPipelineAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/AbstractPipelineAggregationBuilder.java @@ -22,16 +22,10 @@ import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.xcontent.XContentBuilder; -import org.elasticsearch.search.aggregations.AggregationBuilder; -import org.elasticsearch.search.aggregations.AggregatorFactory; import org.elasticsearch.search.aggregations.PipelineAggregationBuilder; -import org.elasticsearch.search.aggregations.bucket.histogram.AutoDateHistogramAggregatorFactory; -import org.elasticsearch.search.aggregations.bucket.histogram.DateHistogramAggregatorFactory; -import org.elasticsearch.search.aggregations.bucket.histogram.HistogramAggregatorFactory; import java.io.IOException; import java.util.Arrays; -import java.util.Collection; import java.util.Map; import java.util.Objects; @@ -79,16 +73,6 @@ public String type() { return type; } - /** - * Validates the state of this factory (makes sure the factory is properly - * configured) - */ - @Override - public final void validate(AggregatorFactory parent, Collection factories, - Collection pipelineAggregatorFactories) { - doValidate(parent, factories, pipelineAggregatorFactories); - } - protected abstract PipelineAggregator createInternal(Map metaData); /** @@ -102,32 +86,6 @@ public final PipelineAggregator create() { return aggregator; } - public void doValidate(AggregatorFactory parent, Collection factories, - Collection pipelineAggregatorFactories) { - } - - /** - * Validates pipeline aggregations that need sequentially ordered data. - */ - public static void validateSequentiallyOrderedParentAggs(AggregatorFactory parent, String type, String name) { - if ((parent instanceof HistogramAggregatorFactory || parent instanceof DateHistogramAggregatorFactory - || parent instanceof AutoDateHistogramAggregatorFactory) == false) { - throw new IllegalStateException( - type + " aggregation [" + name + "] must have a histogram, date_histogram or auto_date_histogram as parent"); - } - if (parent instanceof HistogramAggregatorFactory) { - HistogramAggregatorFactory histoParent = (HistogramAggregatorFactory) parent; - if (histoParent.minDocCount() != 0) { - throw new IllegalStateException("parent histogram of " + type + " aggregation [" + name + "] must have min_doc_count of 0"); - } - } else if (parent instanceof DateHistogramAggregatorFactory) { - DateHistogramAggregatorFactory histoParent = (DateHistogramAggregatorFactory) parent; - if (histoParent.minDocCount() != 0) { - throw new IllegalStateException("parent histogram of " + type + " aggregation [" + name + "] must have min_doc_count of 0"); - } - } - } - @SuppressWarnings("unchecked") @Override public PAB setMetaData(Map metaData) { diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/BucketMetricsPipelineAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/BucketMetricsPipelineAggregationBuilder.java index 0c4aa6f1359bd..425f4990e392c 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/BucketMetricsPipelineAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/BucketMetricsPipelineAggregationBuilder.java @@ -24,13 +24,10 @@ import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.search.DocValueFormat; import org.elasticsearch.search.aggregations.AggregationBuilder; -import org.elasticsearch.search.aggregations.AggregatorFactory; -import org.elasticsearch.search.aggregations.PipelineAggregationBuilder; import org.elasticsearch.search.aggregations.bucket.MultiBucketAggregationBuilder; import org.elasticsearch.search.aggregations.pipeline.BucketHelpers.GapPolicy; import java.io.IOException; -import java.util.Collection; import java.util.Map; import java.util.Objects; import java.util.Optional; @@ -107,28 +104,27 @@ public GapPolicy gapPolicy() { protected abstract PipelineAggregator createInternal(Map metaData); @Override - public void doValidate(AggregatorFactory parent, Collection aggBuilders, - Collection pipelineAggregatorFactories) { + protected void validate(ValidationContext context) { if (bucketsPaths.length != 1) { - throw new IllegalStateException(PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() - + " must contain a single entry for aggregation [" + name + "]"); + context.addBucketPathValidationError("must contain a single entry for aggregation [" + name + "]"); + return; } // Need to find the first agg name in the buckets path to check its a // multi bucket agg: aggs are split with '>' and can optionally have a // metric name after them by using '.' so need to split on both to get // just the agg name final String firstAgg = bucketsPaths[0].split("[>\\.]")[0]; - Optional aggBuilder = aggBuilders.stream().filter((builder) -> builder.getName().equals(firstAgg)) + Optional aggBuilder = context.getSiblingAggregations().stream() + .filter(builder -> builder.getName().equals(firstAgg)) .findAny(); - if (aggBuilder.isPresent()) { - if ((aggBuilder.get() instanceof MultiBucketAggregationBuilder) == false) { - throw new IllegalArgumentException("The first aggregation in " + PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() - + " must be a multi-bucket aggregation for aggregation [" + name + "] found :" - + aggBuilder.get().getClass().getName() + " for buckets path: " + bucketsPaths[0]); - } - } else { - throw new IllegalArgumentException(PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() - + " aggregation does not exist for aggregation [" + name + "]: " + bucketsPaths[0]); + if (false == aggBuilder.isPresent()) { + context.addBucketPathValidationError("aggregation does not exist for aggregation [" + name + "]: " + bucketsPaths[0]); + return; + } + if ((aggBuilder.get() instanceof MultiBucketAggregationBuilder) == false) { + context.addValidationError("The first aggregation in " + PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() + + " must be a multi-bucket aggregation for aggregation [" + name + "] found :" + + aggBuilder.get().getClass().getName() + " for buckets path: " + bucketsPaths[0]); } } 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 2beac58b41f9b..981665e6c533a 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 @@ -198,6 +198,11 @@ protected XContentBuilder internalXContent(XContentBuilder builder, Params param return builder; } + @Override + protected void validate(ValidationContext context) { + // Nothing to check + } + @Override protected boolean overrideBucketsPath() { return true; 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 6bb83c60dea83..0981acbc9bafd 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 @@ -193,6 +193,11 @@ public static BucketSelectorPipelineAggregationBuilder parse(String reducerName, return factory; } + @Override + protected void validate(ValidationContext context) { + // Nothing to check + } + @Override protected boolean overrideBucketsPath() { return true; 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 1e80c6f78c3c5..7f6b491975868 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 @@ -25,9 +25,6 @@ import org.elasticsearch.common.xcontent.ObjectParser; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; -import org.elasticsearch.search.aggregations.AggregationBuilder; -import org.elasticsearch.search.aggregations.AggregatorFactory; -import org.elasticsearch.search.aggregations.PipelineAggregationBuilder; import org.elasticsearch.search.aggregations.pipeline.BucketHelpers.GapPolicy; import org.elasticsearch.search.builder.SearchSourceBuilder; import org.elasticsearch.search.sort.FieldSortBuilder; @@ -36,7 +33,6 @@ import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; -import java.util.Collection; import java.util.Collections; import java.util.List; import java.util.Locale; @@ -144,10 +140,9 @@ protected PipelineAggregator createInternal(Map metaData) { } @Override - public void doValidate(AggregatorFactory parent, Collection aggFactories, - Collection pipelineAggregatoractories) { + protected void validate(ValidationContext context) { if (sorts.isEmpty() && size == null && from == 0) { - throw new IllegalStateException("[" + name + "] is configured to perform nothing. Please set either of " + context.addValidationError("[" + name + "] is configured to perform nothing. Please set either of " + Arrays.asList(SearchSourceBuilder.SORT_FIELD.getPreferredName(), SIZE.getPreferredName(), FROM.getPreferredName()) + " to use " + NAME); } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/CumulativeSumPipelineAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/CumulativeSumPipelineAggregationBuilder.java index fc9b8caf72769..c1af597b4c275 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/CumulativeSumPipelineAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/CumulativeSumPipelineAggregationBuilder.java @@ -25,13 +25,9 @@ import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.search.DocValueFormat; -import org.elasticsearch.search.aggregations.AggregationBuilder; -import org.elasticsearch.search.aggregations.AggregatorFactory; -import org.elasticsearch.search.aggregations.PipelineAggregationBuilder; import java.io.IOException; import java.util.ArrayList; -import java.util.Collection; import java.util.List; import java.util.Map; import java.util.Objects; @@ -93,14 +89,11 @@ protected PipelineAggregator createInternal(Map metaData) { } @Override - public void doValidate(AggregatorFactory parent, Collection aggFactories, - Collection pipelineAggregatorFactories) { + protected void validate(ValidationContext context) { if (bucketsPaths.length != 1) { - throw new IllegalStateException(BUCKETS_PATH.getPreferredName() - + " must contain a single entry for aggregation [" + name + "]"); + context.addBucketPathValidationError("must contain a single entry for aggregation [" + name + "]"); } - - validateSequentiallyOrderedParentAggs(parent, NAME, name); + context.validateParentAggSequentiallyOrdered(NAME, name); } @Override diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/DerivativePipelineAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/DerivativePipelineAggregationBuilder.java index 4aa4155ac063f..106a8a97f5728 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/DerivativePipelineAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/DerivativePipelineAggregationBuilder.java @@ -28,16 +28,12 @@ import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.search.DocValueFormat; -import org.elasticsearch.search.aggregations.AggregationBuilder; -import org.elasticsearch.search.aggregations.AggregatorFactory; -import org.elasticsearch.search.aggregations.PipelineAggregationBuilder; import org.elasticsearch.search.aggregations.bucket.histogram.DateHistogramAggregationBuilder; import org.elasticsearch.search.aggregations.bucket.histogram.DateHistogramInterval; import org.elasticsearch.search.aggregations.pipeline.BucketHelpers.GapPolicy; import java.io.IOException; import java.util.ArrayList; -import java.util.Collection; import java.util.List; import java.util.Map; import java.util.Objects; @@ -152,14 +148,13 @@ protected PipelineAggregator createInternal(Map metaData) { } @Override - public void doValidate(AggregatorFactory parent, Collection aggFactories, - Collection pipelineAggregatoractories) { + protected void validate(ValidationContext context) { if (bucketsPaths.length != 1) { - throw new IllegalStateException(PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() + context.addValidationError(PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() + " must contain a single entry for aggregation [" + name + "]"); } - - validateSequentiallyOrderedParentAggs(parent, NAME, name); + + context.validateParentAggSequentiallyOrdered(NAME, name); } @Override diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/ExtendedStatsBucketPipelineAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/ExtendedStatsBucketPipelineAggregationBuilder.java index 89816a1fb22e8..97948d0e8638a 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/ExtendedStatsBucketPipelineAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/ExtendedStatsBucketPipelineAggregationBuilder.java @@ -22,12 +22,8 @@ import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.xcontent.XContentBuilder; -import org.elasticsearch.search.aggregations.AggregationBuilder; -import org.elasticsearch.search.aggregations.AggregatorFactory; -import org.elasticsearch.search.aggregations.PipelineAggregationBuilder; import java.io.IOException; -import java.util.Collection; import java.util.Map; import java.util.Objects; @@ -80,13 +76,10 @@ protected PipelineAggregator createInternal(Map metaData) { } @Override - public void doValidate(AggregatorFactory parent, Collection aggBuilders, - Collection pipelineAggregatorFactories) { - super.doValidate(parent, aggBuilders, pipelineAggregatorFactories); - - if (sigma < 0.0 ) { - throw new IllegalStateException(ExtendedStatsBucketParser.SIGMA.getPreferredName() - + " must be a non-negative double"); + protected void validate(ValidationContext context) { + super.validate(context); + if (sigma < 0.0) { + context.addValidationError(ExtendedStatsBucketParser.SIGMA.getPreferredName() + " must be a non-negative double"); } } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/MovAvgPipelineAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/MovAvgPipelineAggregationBuilder.java index f75cdda5bc40c..9651e7320fc86 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/MovAvgPipelineAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/MovAvgPipelineAggregationBuilder.java @@ -19,6 +19,17 @@ package org.elasticsearch.search.aggregations.pipeline; +import static org.elasticsearch.search.aggregations.pipeline.PipelineAggregator.Parser.BUCKETS_PATH; +import static org.elasticsearch.search.aggregations.pipeline.PipelineAggregator.Parser.FORMAT; +import static org.elasticsearch.search.aggregations.pipeline.PipelineAggregator.Parser.GAP_POLICY; + +import java.io.IOException; +import java.text.ParseException; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.Objects; + import org.apache.logging.log4j.LogManager; import org.elasticsearch.common.ParseField; import org.elasticsearch.common.ParsingException; @@ -29,23 +40,8 @@ import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.search.DocValueFormat; -import org.elasticsearch.search.aggregations.AggregationBuilder; -import org.elasticsearch.search.aggregations.AggregatorFactory; -import org.elasticsearch.search.aggregations.PipelineAggregationBuilder; import org.elasticsearch.search.aggregations.pipeline.BucketHelpers.GapPolicy; -import java.io.IOException; -import java.text.ParseException; -import java.util.ArrayList; -import java.util.Collection; -import java.util.List; -import java.util.Map; -import java.util.Objects; - -import static org.elasticsearch.search.aggregations.pipeline.PipelineAggregator.Parser.BUCKETS_PATH; -import static org.elasticsearch.search.aggregations.pipeline.PipelineAggregator.Parser.FORMAT; -import static org.elasticsearch.search.aggregations.pipeline.PipelineAggregator.Parser.GAP_POLICY; - public class MovAvgPipelineAggregationBuilder extends AbstractPipelineAggregationBuilder { public static final String NAME = "moving_avg"; @@ -258,20 +254,19 @@ protected PipelineAggregator createInternal(Map metaData) { } @Override - public void doValidate(AggregatorFactory parent, Collection aggFactories, - Collection pipelineAggregatoractories) { + protected void validate(ValidationContext context) { if (minimize != null && minimize && !model.canBeMinimized()) { // If the user asks to minimize, but this model doesn't support // it, throw exception - throw new IllegalStateException("The [" + model + "] model cannot be minimized for aggregation [" + name + "]"); + context.addValidationError("The [" + model + "] model cannot be minimized for aggregation [" + name + "]"); } if (bucketsPaths.length != 1) { - throw new IllegalStateException(PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() - + " must contain a single entry for aggregation [" + name + "]"); + context.addBucketPathValidationError("must contain a single entry for aggregation [" + name + "]"); + return; } // Validate any model-specific window requirements model.validate(window, name); - validateSequentiallyOrderedParentAggs(parent, NAME, name); + context.validateParentAggSequentiallyOrdered(NAME, name); } @Override diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/MovFnPipelineAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/MovFnPipelineAggregationBuilder.java index d24710631d8fd..e5ec2780b0c14 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/MovFnPipelineAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/MovFnPipelineAggregationBuilder.java @@ -30,13 +30,9 @@ import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.script.Script; import org.elasticsearch.search.DocValueFormat; -import org.elasticsearch.search.aggregations.AggregationBuilder; -import org.elasticsearch.search.aggregations.AggregatorFactory; -import org.elasticsearch.search.aggregations.PipelineAggregationBuilder; import org.elasticsearch.search.aggregations.pipeline.BucketHelpers.GapPolicy; import java.io.IOException; -import java.util.Collection; import java.util.Locale; import java.util.Map; import java.util.Objects; @@ -179,13 +175,11 @@ public void setShift(int shift) { } @Override - public void doValidate(AggregatorFactory parent, Collection aggFactories, - Collection pipelineAggregatorFactories) { + protected void validate(ValidationContext context) { if (window <= 0) { - throw new IllegalArgumentException("[" + WINDOW.getPreferredName() + "] must be a positive, non-zero integer."); + context.addValidationError("[" + WINDOW.getPreferredName() + "] must be a positive, non-zero integer."); } - - validateSequentiallyOrderedParentAggs(parent, NAME, name); + context.validateParentAggSequentiallyOrdered(NAME, name); } @Override diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/PercentilesBucketPipelineAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/PercentilesBucketPipelineAggregationBuilder.java index a301688daa9f1..0b3aa65241acc 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/PercentilesBucketPipelineAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/PercentilesBucketPipelineAggregationBuilder.java @@ -27,13 +27,9 @@ import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; -import org.elasticsearch.search.aggregations.AggregationBuilder; -import org.elasticsearch.search.aggregations.AggregatorFactory; -import org.elasticsearch.search.aggregations.PipelineAggregationBuilder; import java.io.IOException; import java.util.Arrays; -import java.util.Collection; import java.util.Map; import java.util.Objects; @@ -117,14 +113,13 @@ protected PipelineAggregator createInternal(Map metaData) { } @Override - public void doValidate(AggregatorFactory parent, Collection aggFactories, - Collection pipelineAggregatorFactories) { - super.doValidate(parent, aggFactories, pipelineAggregatorFactories); - + protected void validate(ValidationContext context) { + super.validate(context); for (Double p : percents) { if (p == null || p < 0.0 || p > 100.0) { - throw new IllegalStateException(PERCENTS_FIELD.getPreferredName() + context.addValidationError(PERCENTS_FIELD.getPreferredName() + " must only contain non-null doubles from 0.0-100.0 inclusive"); + return; } } } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/SerialDiffPipelineAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/SerialDiffPipelineAggregationBuilder.java index 3b02edf51579a..fe6bb921b9129 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/SerialDiffPipelineAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/SerialDiffPipelineAggregationBuilder.java @@ -26,14 +26,10 @@ import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.search.DocValueFormat; -import org.elasticsearch.search.aggregations.AggregationBuilder; -import org.elasticsearch.search.aggregations.AggregatorFactory; -import org.elasticsearch.search.aggregations.PipelineAggregationBuilder; import org.elasticsearch.search.aggregations.pipeline.BucketHelpers.GapPolicy; import java.io.IOException; import java.util.ArrayList; -import java.util.Collection; import java.util.List; import java.util.Map; import java.util.Objects; @@ -138,11 +134,10 @@ protected DocValueFormat formatter() { protected PipelineAggregator createInternal(Map metaData) { return new SerialDiffPipelineAggregator(name, bucketsPaths, formatter(), gapPolicy, lag, metaData); } - + @Override - public void doValidate(AggregatorFactory parent, Collection aggFactories, - Collection pipelineAggregatoractories) { - validateSequentiallyOrderedParentAggs(parent, NAME, name); + protected void validate(ValidationContext context) { + context.validateParentAggSequentiallyOrdered(NAME, name); } @Override diff --git a/server/src/test/java/org/elasticsearch/search/SearchModuleTests.java b/server/src/test/java/org/elasticsearch/search/SearchModuleTests.java index 8582fc64faccd..49fb1ac350477 100644 --- a/server/src/test/java/org/elasticsearch/search/SearchModuleTests.java +++ b/server/src/test/java/org/elasticsearch/search/SearchModuleTests.java @@ -447,6 +447,9 @@ protected XContentBuilder internalXContent(XContentBuilder builder, Params param private static TestPipelineAggregationBuilder fromXContent(String name, XContentParser p) { return null; } + + @Override + protected void validate(ValidationContext context) {} } /** diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/AbstractBucketMetricsTestCase.java b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/AbstractBucketMetricsTestCase.java index 8f268cecaedd2..d53a147261cb1 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/AbstractBucketMetricsTestCase.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/AbstractBucketMetricsTestCase.java @@ -40,5 +40,4 @@ protected final PAF createTestAggregatorFactory() { } protected abstract PAF doCreateTestAggregatorFactory(String name, String bucketsPath); - } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/AvgBucketTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/AvgBucketTests.java index 0dc10cb7a7a13..2c5f0f9f343c3 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/AvgBucketTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/AvgBucketTests.java @@ -24,10 +24,12 @@ import org.elasticsearch.search.aggregations.bucket.terms.TermsAggregationBuilder; import org.elasticsearch.search.aggregations.support.ValueType; -import java.util.Collections; import java.util.HashSet; import java.util.Set; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.nullValue; + public class AvgBucketTests extends AbstractBucketMetricsTestCase { @Override @@ -43,23 +45,18 @@ public void testValidate() { aggBuilders.add(multiBucketAgg); // First try to point to a non-existent agg - final AvgBucketPipelineAggregationBuilder builder = new AvgBucketPipelineAggregationBuilder("name", "invalid_agg>metric"); - IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, - () -> builder.validate(null, aggBuilders, Collections.emptySet())); - assertEquals(PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() - + " aggregation does not exist for aggregation [name]: invalid_agg>metric", ex.getMessage()); + assertThat(validate(aggBuilders, new AvgBucketPipelineAggregationBuilder("name", "invalid_agg>metric")), equalTo( + "Validation Failed: 1: " + PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() + + " aggregation does not exist for aggregation [name]: invalid_agg>metric;")); // Now try to point to a single bucket agg - AvgBucketPipelineAggregationBuilder builder2 = new AvgBucketPipelineAggregationBuilder("name", "global>metric"); - ex = expectThrows(IllegalArgumentException.class, () -> builder2.validate(null, aggBuilders, Collections.emptySet())); - assertEquals("The first aggregation in " + PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() + assertThat(validate(aggBuilders, new AvgBucketPipelineAggregationBuilder("name", "global>metric")), equalTo( + "Validation Failed: 1: The first aggregation in " + PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() + " must be a multi-bucket aggregation for aggregation [name] found :" + GlobalAggregationBuilder.class.getName() - + " for buckets path: global>metric", ex.getMessage()); - - // Now try to point to a valid multi-bucket agg (no exception should be thrown) - AvgBucketPipelineAggregationBuilder builder3 = new AvgBucketPipelineAggregationBuilder("name", "terms>metric"); - builder3.validate(null, aggBuilders, Collections.emptySet()); + + " for buckets path: global>metric;")); + // Now try to point to a valid multi-bucket agg which is valid + assertThat(validate(aggBuilders, new AvgBucketPipelineAggregationBuilder("name", "terms>metric")), nullValue()); } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/BucketSortIT.java b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/BucketSortIT.java index 22a4fdbdf67bf..8518b985d9194 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/BucketSortIT.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/BucketSortIT.java @@ -19,8 +19,8 @@ package org.elasticsearch.search.aggregations.pipeline; +import org.elasticsearch.action.ActionRequestValidationException; import org.elasticsearch.action.index.IndexRequestBuilder; -import org.elasticsearch.action.search.SearchPhaseExecutionException; import org.elasticsearch.action.search.SearchResponse; import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.common.xcontent.XContentBuilder; @@ -460,21 +460,21 @@ public void testEmptyBuckets() { } public void testInvalidPath() { - SearchPhaseExecutionException e = expectThrows(SearchPhaseExecutionException.class, + Exception e = expectThrows(ActionRequestValidationException.class, () -> client().prepareSearch(INDEX) .addAggregation(terms("foos").field(TERM_FIELD) .subAggregation(bucketSort("bucketSort", Arrays.asList(new FieldSortBuilder("invalid"))))) .get()); - assertThat(e.getCause().getMessage(), containsString("No aggregation found for path [invalid]")); + assertThat(e.getMessage(), containsString("No aggregation found for path [invalid]")); } public void testNeitherSortsNorSizeSpecifiedAndFromIsDefault_ShouldThrowValidation() { - SearchPhaseExecutionException e = expectThrows(SearchPhaseExecutionException.class, + Exception e = expectThrows(ActionRequestValidationException.class, () -> client().prepareSearch(INDEX) .addAggregation(terms("foos").field(TERM_FIELD) .subAggregation(bucketSort("bucketSort", Collections.emptyList()))) .get()); - assertThat(e.getCause().getMessage(), containsString("[bucketSort] is configured to perform nothing." + + assertThat(e.getMessage(), containsString("[bucketSort] is configured to perform nothing." + " Please set either of [sort, size, from] to use bucket_sort")); } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/CumulativeSumAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/CumulativeSumAggregatorTests.java index 9d27663d275f7..dd3e971f4400b 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/CumulativeSumAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/CumulativeSumAggregatorTests.java @@ -38,8 +38,6 @@ import org.elasticsearch.search.aggregations.AggregationBuilder; import org.elasticsearch.search.aggregations.AggregatorTestCase; import org.elasticsearch.search.aggregations.InternalAggregation; -import org.elasticsearch.search.aggregations.PipelineAggregationBuilder; -import org.elasticsearch.search.aggregations.TestAggregatorFactory; import org.elasticsearch.search.aggregations.bucket.histogram.DateHistogramAggregationBuilder; import org.elasticsearch.search.aggregations.bucket.histogram.DateHistogramInterval; import org.elasticsearch.search.aggregations.bucket.histogram.Histogram; @@ -52,10 +50,7 @@ import java.io.IOException; import java.util.Arrays; -import java.util.Collections; -import java.util.HashSet; import java.util.List; -import java.util.Set; import java.util.function.Consumer; import static org.hamcrest.Matchers.equalTo; @@ -291,34 +286,6 @@ public void testNoBuckets() throws IOException { } }); } - - /** - * The validation should verify the parent aggregation is allowed. - */ - public void testValidate() throws IOException { - final Set aggBuilders = new HashSet<>(); - aggBuilders.add(new CumulativeSumPipelineAggregationBuilder("cusum", "sum")); - - final CumulativeSumPipelineAggregationBuilder builder = new CumulativeSumPipelineAggregationBuilder("name", "valid"); - builder.validate(PipelineAggregationHelperTests.getRandomSequentiallyOrderedParentAgg(), Collections.emptySet(), aggBuilders); - } - - /** - * The validation should throw an IllegalArgumentException, since parent - * aggregation is not a type of HistogramAggregatorFactory, - * DateHistogramAggregatorFactory or AutoDateHistogramAggregatorFactory. - */ - public void testValidateException() throws IOException { - final Set aggBuilders = new HashSet<>(); - aggBuilders.add(new CumulativeSumPipelineAggregationBuilder("cusum", "sum")); - TestAggregatorFactory parentFactory = TestAggregatorFactory.createInstance(); - - final CumulativeSumPipelineAggregationBuilder builder = new CumulativeSumPipelineAggregationBuilder("name", "invalid_agg>metric"); - IllegalStateException ex = expectThrows(IllegalStateException.class, - () -> builder.validate(parentFactory, Collections.emptySet(), aggBuilders)); - assertEquals("cumulative_sum aggregation [name] must have a histogram, date_histogram or auto_date_histogram as parent", - ex.getMessage()); - } private void executeTestCase(Query query, AggregationBuilder aggBuilder, Consumer verify) throws IOException { executeTestCase(query, aggBuilder, verify, indexWriter -> { diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/CumulativeSumTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/CumulativeSumTests.java index edf879ce77f68..f51b8ec479d8b 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/CumulativeSumTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/CumulativeSumTests.java @@ -19,10 +19,18 @@ package org.elasticsearch.search.aggregations.pipeline; +import org.elasticsearch.search.aggregations.AggregationBuilder; import org.elasticsearch.search.aggregations.BasePipelineAggregationTestCase; -public class CumulativeSumTests extends BasePipelineAggregationTestCase { +import java.io.IOException; + +import static java.util.Collections.emptyList; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.nullValue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; +public class CumulativeSumTests extends BasePipelineAggregationTestCase { @Override protected CumulativeSumPipelineAggregationBuilder createTestAggregatorFactory() { String name = randomAlphaOfLengthBetween(3, 20); @@ -34,4 +42,23 @@ protected CumulativeSumPipelineAggregationBuilder createTestAggregatorFactory() return factory; } + public void testValidate() throws IOException { + assertThat(validate(PipelineAggregationHelperTests.getRandomSequentiallyOrderedParentAgg(), + new CumulativeSumPipelineAggregationBuilder("name", "valid")), nullValue()); + } + + public void testInvalidParent() throws IOException { + AggregationBuilder parent = mock(AggregationBuilder.class); + when(parent.getName()).thenReturn("name"); + + assertThat(validate(parent, new CumulativeSumPipelineAggregationBuilder("name", "invalid_agg>metric")), equalTo( + "Validation Failed: 1: cumulative_sum aggregation [name] must have a histogram, date_histogram " + + "or auto_date_histogram as parent;")); + } + + public void testNoParent() throws IOException { + assertThat(validate(emptyList(), new CumulativeSumPipelineAggregationBuilder("name", "invalid_agg>metric")), equalTo( + "Validation Failed: 1: cumulative_sum aggregation [name] must have a histogram, date_histogram " + + "or auto_date_histogram as parent but doesn't have a parent;")); + } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/DerivativeTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/DerivativeTests.java index 7fd6e1e86266b..662778315f7e4 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/DerivativeTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/DerivativeTests.java @@ -19,16 +19,20 @@ package org.elasticsearch.search.aggregations.pipeline; +import org.elasticsearch.search.aggregations.AggregationBuilder; import org.elasticsearch.search.aggregations.BasePipelineAggregationTestCase; import org.elasticsearch.search.aggregations.PipelineAggregationBuilder; -import org.elasticsearch.search.aggregations.TestAggregatorFactory; import org.elasticsearch.search.aggregations.pipeline.BucketHelpers.GapPolicy; import java.io.IOException; -import java.util.Collections; import java.util.HashSet; import java.util.Set; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.nullValue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + public class DerivativeTests extends BasePipelineAggregationTestCase { @Override @@ -51,16 +55,13 @@ protected DerivativePipelineAggregationBuilder createTestAggregatorFactory() { } return factory; } - + /** * The validation should verify the parent aggregation is allowed. */ public void testValidate() throws IOException { - final Set aggBuilders = new HashSet<>(); - aggBuilders.add(new DerivativePipelineAggregationBuilder("deriv", "der")); - - final DerivativePipelineAggregationBuilder builder = new DerivativePipelineAggregationBuilder("name", "valid"); - builder.validate(PipelineAggregationHelperTests.getRandomSequentiallyOrderedParentAgg(), Collections.emptySet(), aggBuilders); + assertThat(validate(PipelineAggregationHelperTests.getRandomSequentiallyOrderedParentAgg(), + new DerivativePipelineAggregationBuilder("name", "valid")), nullValue()); } /** @@ -71,12 +72,11 @@ public void testValidate() throws IOException { public void testValidateException() throws IOException { final Set aggBuilders = new HashSet<>(); aggBuilders.add(new DerivativePipelineAggregationBuilder("deriv", "der")); - TestAggregatorFactory parentFactory = TestAggregatorFactory.createInstance(); + AggregationBuilder parent = mock(AggregationBuilder.class); + when(parent.getName()).thenReturn("name"); - final DerivativePipelineAggregationBuilder builder = new DerivativePipelineAggregationBuilder("name", "invalid_agg>metric"); - IllegalStateException ex = expectThrows(IllegalStateException.class, - () -> builder.validate(parentFactory, Collections.emptySet(), aggBuilders)); - assertEquals("derivative aggregation [name] must have a histogram, date_histogram or auto_date_histogram as parent", - ex.getMessage()); + assertThat(validate(parent, new DerivativePipelineAggregationBuilder("name", "invalid_agg>metric")), equalTo( + "Validation Failed: 1: derivative aggregation [name] must have a histogram, " + + "date_histogram or auto_date_histogram as parent;")); } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/ExtendedStatsBucketTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/ExtendedStatsBucketTests.java index 9930541cb007e..90cc9be95e3be 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/ExtendedStatsBucketTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/ExtendedStatsBucketTests.java @@ -26,11 +26,11 @@ import org.elasticsearch.search.aggregations.bucket.terms.TermsAggregationBuilder; import org.elasticsearch.search.aggregations.support.ValueType; -import java.util.Collections; import java.util.HashSet; import java.util.Set; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.nullValue; public class ExtendedStatsBucketTests extends AbstractBucketMetricsTestCase { @@ -68,23 +68,17 @@ public void testValidate() { aggBuilders.add(multiBucketAgg); // First try to point to a non-existent agg - final ExtendedStatsBucketPipelineAggregationBuilder builder = new ExtendedStatsBucketPipelineAggregationBuilder("name", - "invalid_agg>metric"); - IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, - () -> builder.validate(null, aggBuilders, Collections.emptySet())); - assertEquals(PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() - + " aggregation does not exist for aggregation [name]: invalid_agg>metric", ex.getMessage()); + assertThat(validate(aggBuilders, new ExtendedStatsBucketPipelineAggregationBuilder("name", "invalid_agg>metric")), equalTo( + "Validation Failed: 1: " + PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() + + " aggregation does not exist for aggregation [name]: invalid_agg>metric;")); // Now try to point to a single bucket agg - ExtendedStatsBucketPipelineAggregationBuilder builder2 = new ExtendedStatsBucketPipelineAggregationBuilder("name", "global>metric"); - ex = expectThrows(IllegalArgumentException.class, () -> builder2.validate(null, aggBuilders, Collections.emptySet())); - assertEquals("The first aggregation in " + PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() + assertThat(validate(aggBuilders, new ExtendedStatsBucketPipelineAggregationBuilder("name", "global>metric")), equalTo( + "Validation Failed: 1: The first aggregation in " + PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() + " must be a multi-bucket aggregation for aggregation [name] found :" + GlobalAggregationBuilder.class.getName() - + " for buckets path: global>metric", ex.getMessage()); + + " for buckets path: global>metric;")); - // Now try to point to a valid multi-bucket agg (no exception should be - // thrown) - ExtendedStatsBucketPipelineAggregationBuilder builder3 = new ExtendedStatsBucketPipelineAggregationBuilder("name", "terms>metric"); - builder3.validate(null, aggBuilders, Collections.emptySet()); + // Now try to point to a valid multi-bucket agg + assertThat(validate(aggBuilders, new ExtendedStatsBucketPipelineAggregationBuilder("name", "terms>metric")), nullValue()); } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/MaxBucketTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/MaxBucketTests.java index c55152c68c3a6..edbc1cda3eae2 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/MaxBucketTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/MaxBucketTests.java @@ -24,10 +24,12 @@ import org.elasticsearch.search.aggregations.bucket.terms.TermsAggregationBuilder; import org.elasticsearch.search.aggregations.support.ValueType; -import java.util.Collections; import java.util.HashSet; import java.util.Set; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.nullValue; + public class MaxBucketTests extends AbstractBucketMetricsTestCase { @Override @@ -43,23 +45,18 @@ public void testValidate() { aggBuilders.add(multiBucketAgg); // First try to point to a non-existent agg - final MaxBucketPipelineAggregationBuilder builder = new MaxBucketPipelineAggregationBuilder("name", "invalid_agg>metric"); - IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, - () -> builder.validate(null, aggBuilders, Collections.emptySet())); - assertEquals(PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() - + " aggregation does not exist for aggregation [name]: invalid_agg>metric", ex.getMessage()); + assertThat(validate(aggBuilders, new MaxBucketPipelineAggregationBuilder("name", "invalid_agg>metric")), equalTo( + "Validation Failed: 1: " + PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() + + " aggregation does not exist for aggregation [name]: invalid_agg>metric;")); // Now try to point to a single bucket agg - MaxBucketPipelineAggregationBuilder builder2 = new MaxBucketPipelineAggregationBuilder("name", "global>metric"); - ex = expectThrows(IllegalArgumentException.class, () -> builder2.validate(null, aggBuilders, Collections.emptySet())); - assertEquals("The first aggregation in " + PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() + assertThat(validate(aggBuilders, new MaxBucketPipelineAggregationBuilder("name", "global>metric")), equalTo( + "Validation Failed: 1: The first aggregation in " + PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() + " must be a multi-bucket aggregation for aggregation [name] found :" + GlobalAggregationBuilder.class.getName() - + " for buckets path: global>metric", ex.getMessage()); + + " for buckets path: global>metric;")); - // Now try to point to a valid multi-bucket agg (no exception should be - // thrown) - MaxBucketPipelineAggregationBuilder builder3 = new MaxBucketPipelineAggregationBuilder("name", "terms>metric"); - builder3.validate(null, aggBuilders, Collections.emptySet()); + // Now try to point to a valid multi-bucket agg + assertThat(validate(aggBuilders, new MaxBucketPipelineAggregationBuilder("name", "terms>metric")), nullValue()); } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/MinBucketTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/MinBucketTests.java index 317f1360c7845..057e074a90cfc 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/MinBucketTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/MinBucketTests.java @@ -24,10 +24,12 @@ import org.elasticsearch.search.aggregations.bucket.terms.TermsAggregationBuilder; import org.elasticsearch.search.aggregations.support.ValueType; -import java.util.Collections; import java.util.HashSet; import java.util.Set; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.nullValue; + public class MinBucketTests extends AbstractBucketMetricsTestCase { @Override @@ -43,23 +45,18 @@ public void testValidate() { aggBuilders.add(multiBucketAgg); // First try to point to a non-existent agg - final MinBucketPipelineAggregationBuilder builder = new MinBucketPipelineAggregationBuilder("name", "invalid_agg>metric"); - IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, - () -> builder.validate(null, aggBuilders, Collections.emptySet())); - assertEquals(PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() - + " aggregation does not exist for aggregation [name]: invalid_agg>metric", ex.getMessage()); + assertThat(validate(aggBuilders, new MinBucketPipelineAggregationBuilder("name", "invalid_agg>metric")), equalTo( + "Validation Failed: 1: " + PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() + + " aggregation does not exist for aggregation [name]: invalid_agg>metric;")); // Now try to point to a single bucket agg - MinBucketPipelineAggregationBuilder builder2 = new MinBucketPipelineAggregationBuilder("name", "global>metric"); - ex = expectThrows(IllegalArgumentException.class, () -> builder2.validate(null, aggBuilders, Collections.emptySet())); - assertEquals("The first aggregation in " + PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() + assertThat(validate(aggBuilders, new MinBucketPipelineAggregationBuilder("name", "global>metric")), equalTo( + "Validation Failed: 1: The first aggregation in " + PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() + " must be a multi-bucket aggregation for aggregation [name] found :" + GlobalAggregationBuilder.class.getName() - + " for buckets path: global>metric", ex.getMessage()); + + " for buckets path: global>metric;")); - // Now try to point to a valid multi-bucket agg (no exception should be - // thrown) - MinBucketPipelineAggregationBuilder builder3 = new MinBucketPipelineAggregationBuilder("name", "terms>metric"); - builder3.validate(null, aggBuilders, Collections.emptySet()); + // Now try to point to a valid multi-bucket agg + assertThat(validate(aggBuilders, new MinBucketPipelineAggregationBuilder("name", "terms>metric")), nullValue()); } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/MovAvgIT.java b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/MovAvgIT.java index e6fff0f57886c..254e324ab9f72 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/MovAvgIT.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/MovAvgIT.java @@ -19,6 +19,29 @@ package org.elasticsearch.search.aggregations.pipeline; +import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder; +import static org.elasticsearch.search.aggregations.AggregationBuilders.avg; +import static org.elasticsearch.search.aggregations.AggregationBuilders.histogram; +import static org.elasticsearch.search.aggregations.AggregationBuilders.max; +import static org.elasticsearch.search.aggregations.AggregationBuilders.min; +import static org.elasticsearch.search.aggregations.AggregationBuilders.range; +import static org.elasticsearch.search.aggregations.PipelineAggregatorBuilders.derivative; +import static org.elasticsearch.search.aggregations.PipelineAggregatorBuilders.movingAvg; +import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertSearchResponse; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.core.IsNull.notNullValue; +import static org.hamcrest.core.IsNull.nullValue; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; +import java.util.HashMap; +import java.util.Iterator; +import java.util.List; +import java.util.Map; + +import org.elasticsearch.action.ActionRequestValidationException; import org.elasticsearch.action.bulk.BulkRequestBuilder; import org.elasticsearch.action.index.IndexRequestBuilder; import org.elasticsearch.action.search.SearchPhaseExecutionException; @@ -35,28 +58,6 @@ import org.elasticsearch.test.ESIntegTestCase; import org.hamcrest.Matchers; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.Collection; -import java.util.HashMap; -import java.util.Iterator; -import java.util.List; -import java.util.Map; - -import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder; -import static org.elasticsearch.search.aggregations.AggregationBuilders.avg; -import static org.elasticsearch.search.aggregations.AggregationBuilders.histogram; -import static org.elasticsearch.search.aggregations.AggregationBuilders.max; -import static org.elasticsearch.search.aggregations.AggregationBuilders.min; -import static org.elasticsearch.search.aggregations.AggregationBuilders.range; -import static org.elasticsearch.search.aggregations.PipelineAggregatorBuilders.derivative; -import static org.elasticsearch.search.aggregations.PipelineAggregatorBuilders.movingAvg; -import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertSearchResponse; -import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.is; -import static org.hamcrest.core.IsNull.notNullValue; -import static org.hamcrest.core.IsNull.nullValue; - @ESIntegTestCase.SuiteScopeTestCase public class MovAvgIT extends ESIntegTestCase { private static final String INTERVAL_FIELD = "l_value"; @@ -737,7 +738,7 @@ public void testBadParent() { ).get(); fail("MovingAvg should not accept non-histogram as parent"); - } catch (SearchPhaseExecutionException exception) { + } catch (ActionRequestValidationException exception) { // All good } } @@ -850,7 +851,7 @@ public void testNegativePrediction() { public void testHoltWintersNotEnoughData() { Client client = client(); - expectThrows(SearchPhaseExecutionException.class, () -> client.prepareSearch("idx") + expectThrows(IllegalArgumentException.class, () -> client.prepareSearch("idx") .addAggregation( histogram("histo").field(INTERVAL_FIELD).interval(interval) .extendedBounds(0L, interval * (numBuckets - 1)) @@ -1144,7 +1145,7 @@ public void testCheckIfNonTunableCanBeMinimized() { .minimize(true)) ).get(); fail("Simple Model cannot be minimized, but an exception was not thrown"); - } catch (SearchPhaseExecutionException e) { + } catch (ActionRequestValidationException e) { // All good } @@ -1162,7 +1163,7 @@ public void testCheckIfNonTunableCanBeMinimized() { .minimize(true)) ).get(); fail("Linear Model cannot be minimized, but an exception was not thrown"); - } catch (SearchPhaseExecutionException e) { + } catch (ActionRequestValidationException e) { // all good } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/MovAvgTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/MovAvgTests.java index 490d1f22ff329..5acd5632644f4 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/MovAvgTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/MovAvgTests.java @@ -19,18 +19,18 @@ package org.elasticsearch.search.aggregations.pipeline; +import static java.util.Collections.emptyList; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.nullValue; + +import java.io.IOException; + import org.elasticsearch.common.xcontent.json.JsonXContent; import org.elasticsearch.search.aggregations.BasePipelineAggregationTestCase; import org.elasticsearch.search.aggregations.PipelineAggregationBuilder; -import org.elasticsearch.search.aggregations.TestAggregatorFactory; import org.elasticsearch.search.aggregations.pipeline.BucketHelpers.GapPolicy; import org.elasticsearch.search.aggregations.pipeline.HoltWintersModel.SeasonalityType; -import java.io.IOException; -import java.util.Collections; -import java.util.HashSet; -import java.util.Set; - public class MovAvgTests extends BasePipelineAggregationTestCase { @Override @@ -120,11 +120,8 @@ public void testDefaultParsing() throws Exception { * The validation should verify the parent aggregation is allowed. */ public void testValidate() throws IOException { - final Set aggBuilders = new HashSet<>(); - aggBuilders.add(createTestAggregatorFactory()); - - final MovAvgPipelineAggregationBuilder builder = new MovAvgPipelineAggregationBuilder("name", "valid"); - builder.validate(PipelineAggregationHelperTests.getRandomSequentiallyOrderedParentAgg(), Collections.emptySet(), aggBuilders); + assertThat(validate(PipelineAggregationHelperTests.getRandomSequentiallyOrderedParentAgg(), + new MovAvgPipelineAggregationBuilder("name", "valid")), nullValue()); } /** @@ -133,14 +130,8 @@ public void testValidate() throws IOException { * DateHistogramAggregatorFactory or AutoDateHistogramAggregatorFactory. */ public void testValidateException() throws IOException { - final Set aggBuilders = new HashSet<>(); - aggBuilders.add(createTestAggregatorFactory()); - TestAggregatorFactory parentFactory = TestAggregatorFactory.createInstance(); - - final MovAvgPipelineAggregationBuilder builder = new MovAvgPipelineAggregationBuilder("name", "invalid_agg>metric"); - IllegalStateException ex = expectThrows(IllegalStateException.class, - () -> builder.validate(parentFactory, Collections.emptySet(), aggBuilders)); - assertEquals("moving_avg aggregation [name] must have a histogram, date_histogram or auto_date_histogram as parent", - ex.getMessage()); + assertThat(validate(emptyList(), new MovAvgPipelineAggregationBuilder("name", "invalid_agg>metric")), equalTo( + "Validation Failed: 1: moving_avg aggregation [name] must have a histogram, date_histogram " + + "or auto_date_histogram as parent but doesn't have a parent;")); } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/MovFnUnitTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/MovFnAggrgatorTests.java similarity index 79% rename from server/src/test/java/org/elasticsearch/search/aggregations/pipeline/MovFnUnitTests.java rename to server/src/test/java/org/elasticsearch/search/aggregations/pipeline/MovFnAggrgatorTests.java index ac1afee1c7919..df6435ab573a6 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/MovFnUnitTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/MovFnAggrgatorTests.java @@ -42,8 +42,6 @@ import org.elasticsearch.script.ScriptService; import org.elasticsearch.script.ScriptType; import org.elasticsearch.search.aggregations.AggregatorTestCase; -import org.elasticsearch.search.aggregations.PipelineAggregationBuilder; -import org.elasticsearch.search.aggregations.TestAggregatorFactory; import org.elasticsearch.search.aggregations.bucket.histogram.DateHistogramAggregationBuilder; import org.elasticsearch.search.aggregations.bucket.histogram.DateHistogramInterval; import org.elasticsearch.search.aggregations.bucket.histogram.Histogram; @@ -53,16 +51,14 @@ import java.io.IOException; import java.util.Arrays; import java.util.Collections; -import java.util.HashSet; import java.util.List; import java.util.Map; -import java.util.Set; import java.util.function.Consumer; import java.util.stream.Collectors; import static org.hamcrest.Matchers.equalTo; -public class MovFnUnitTests extends AggregatorTestCase { +public class MovFnAggrgatorTests extends AggregatorTestCase { private static final String DATE_FIELD = "date"; private static final String INSTANT_FIELD = "instant"; @@ -172,34 +168,4 @@ private void executeTestCase(Query query, private static long asLong(String dateTime) { return DateFormatters.from(DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER.parse(dateTime)).toInstant().toEpochMilli(); } - - /** - * The validation should verify the parent aggregation is allowed. - */ - public void testValidate() throws IOException { - final Set aggBuilders = new HashSet<>(); - Script script = new Script(Script.DEFAULT_SCRIPT_TYPE, "painless", "test", Collections.emptyMap()); - aggBuilders.add(new MovFnPipelineAggregationBuilder("mov_fn", "avg", script, 3)); - - final MovFnPipelineAggregationBuilder builder = new MovFnPipelineAggregationBuilder("name", "invalid_agg>metric", script, 1); - builder.validate(PipelineAggregationHelperTests.getRandomSequentiallyOrderedParentAgg(), Collections.emptySet(), aggBuilders); - } - - /** - * The validation should throw an IllegalArgumentException, since parent - * aggregation is not a type of HistogramAggregatorFactory, - * DateHistogramAggregatorFactory or AutoDateHistogramAggregatorFactory. - */ - public void testValidateException() throws IOException { - final Set aggBuilders = new HashSet<>(); - Script script = new Script(Script.DEFAULT_SCRIPT_TYPE, "painless", "test", Collections.emptyMap()); - aggBuilders.add(new MovFnPipelineAggregationBuilder("mov_fn", "avg", script, 3)); - TestAggregatorFactory parentFactory = TestAggregatorFactory.createInstance(); - - final MovFnPipelineAggregationBuilder builder = new MovFnPipelineAggregationBuilder("name", "invalid_agg>metric", script, 1); - IllegalStateException ex = expectThrows(IllegalStateException.class, - () -> builder.validate(parentFactory, Collections.emptySet(), aggBuilders)); - assertEquals("moving_fn aggregation [name] must have a histogram, date_histogram or auto_date_histogram as parent", - ex.getMessage()); - } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/MovFnPipelineAggregationBuilderSerializationTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/MovFnPipelineAggregationBuilderSerializationTests.java index cb1e2d5249b4a..17fbac3495c56 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/MovFnPipelineAggregationBuilderSerializationTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/MovFnPipelineAggregationBuilderSerializationTests.java @@ -19,17 +19,23 @@ package org.elasticsearch.search.aggregations.pipeline; -import org.elasticsearch.common.io.stream.Writeable; -import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.script.Script; -import org.elasticsearch.test.AbstractSerializingTestCase; +import org.elasticsearch.search.aggregations.AggregationBuilder; +import org.elasticsearch.search.aggregations.BasePipelineAggregationTestCase; import java.io.IOException; +import java.util.Collections; -public class MovFnPipelineAggregationBuilderSerializationTests extends AbstractSerializingTestCase { +import static java.util.Collections.emptyList; +import static java.util.Collections.emptyMap; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.nullValue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; +public class MovFnPipelineAggregationBuilderSerializationTests extends BasePipelineAggregationTestCase { @Override - protected MovFnPipelineAggregationBuilder createTestInstance() { + protected MovFnPipelineAggregationBuilder createTestAggregatorFactory() { MovFnPipelineAggregationBuilder builder = new MovFnPipelineAggregationBuilder( randomAlphaOfLength(10), "foo", @@ -40,19 +46,27 @@ protected MovFnPipelineAggregationBuilder createTestInstance() { return builder; } - @Override - protected Writeable.Reader instanceReader() { - return MovFnPipelineAggregationBuilder::new; + public void testValidParent() throws IOException { + Script script = new Script(Script.DEFAULT_SCRIPT_TYPE, "painless", "test", emptyMap()); + assertThat(validate(PipelineAggregationHelperTests.getRandomSequentiallyOrderedParentAgg(), + new MovFnPipelineAggregationBuilder("mov_fn", "avg", script, 3)), nullValue()); } - @Override - protected MovFnPipelineAggregationBuilder doParseInstance(XContentParser parser) throws IOException { - return MovFnPipelineAggregationBuilder.parse(parser); + public void testInvalidParent() throws IOException { + Script script = new Script(Script.DEFAULT_SCRIPT_TYPE, "painless", "test", Collections.emptyMap()); + AggregationBuilder parent = mock(AggregationBuilder.class); + when(parent.getName()).thenReturn("name"); + + assertThat(validate(parent, new MovFnPipelineAggregationBuilder("name", "invalid_agg>metric", script, 1)), equalTo( + "Validation Failed: 1: moving_fn aggregation [name] must have a histogram, date_histogram" + + " or auto_date_histogram as parent;")); } - @Override - protected boolean supportsUnknownFields() { - return false; + public void testNoParent() throws IOException { + Script script = new Script(Script.DEFAULT_SCRIPT_TYPE, "painless", "test", Collections.emptyMap()); + assertThat(validate(emptyList(), new MovFnPipelineAggregationBuilder("name", "invalid_agg>metric", script, 1)), equalTo( + "Validation Failed: 1: moving_fn aggregation [name] must have a histogram, date_histogram" + + " or auto_date_histogram as parent but doesn't have a parent;")); } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/PercentilesBucketTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/PercentilesBucketTests.java index 165312a5bdef5..1918d26e1e1af 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/PercentilesBucketTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/PercentilesBucketTests.java @@ -26,11 +26,11 @@ import org.elasticsearch.search.aggregations.bucket.terms.TermsAggregationBuilder; import org.elasticsearch.search.aggregations.support.ValueType; -import java.util.Collections; import java.util.HashSet; import java.util.Set; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.nullValue; public class PercentilesBucketTests extends AbstractBucketMetricsTestCase { @@ -72,23 +72,17 @@ public void testValidate() { aggBuilders.add(multiBucketAgg); // First try to point to a non-existent agg - final PercentilesBucketPipelineAggregationBuilder builder = new PercentilesBucketPipelineAggregationBuilder("name", - "invalid_agg>metric"); - IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, - () -> builder.validate(null, aggBuilders, Collections.emptySet())); - assertEquals(PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() - + " aggregation does not exist for aggregation [name]: invalid_agg>metric", ex.getMessage()); + assertThat(validate(aggBuilders, new PercentilesBucketPipelineAggregationBuilder("name", "invalid_agg>metric")), equalTo( + "Validation Failed: 1: " + PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() + + " aggregation does not exist for aggregation [name]: invalid_agg>metric;")); // Now try to point to a single bucket agg - PercentilesBucketPipelineAggregationBuilder builder2 = new PercentilesBucketPipelineAggregationBuilder("name", "global>metric"); - ex = expectThrows(IllegalArgumentException.class, () -> builder2.validate(null, aggBuilders, Collections.emptySet())); - assertEquals("The first aggregation in " + PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() + assertThat(validate(aggBuilders, new PercentilesBucketPipelineAggregationBuilder("name", "global>metric")), equalTo( + "Validation Failed: 1: The first aggregation in " + PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() + " must be a multi-bucket aggregation for aggregation [name] found :" + GlobalAggregationBuilder.class.getName() - + " for buckets path: global>metric", ex.getMessage()); + + " for buckets path: global>metric;")); - // Now try to point to a valid multi-bucket agg (no exception should be - // thrown) - PercentilesBucketPipelineAggregationBuilder builder3 = new PercentilesBucketPipelineAggregationBuilder("name", "terms>metric"); - builder3.validate(null, aggBuilders, Collections.emptySet()); + // Now try to point to a valid multi-bucket agg + assertThat(validate(aggBuilders, new PercentilesBucketPipelineAggregationBuilder("name", "terms>metric")), nullValue()); } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/PipelineAggregationHelperTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/PipelineAggregationHelperTests.java index 9a2d5a411d4ad..2d67735468984 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/PipelineAggregationHelperTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/PipelineAggregationHelperTests.java @@ -20,29 +20,20 @@ package org.elasticsearch.search.aggregations.pipeline; -import org.elasticsearch.common.Rounding; -import org.elasticsearch.index.query.QueryShardContext; -import org.elasticsearch.search.aggregations.AggregatorFactories; -import org.elasticsearch.search.aggregations.AggregatorFactory; -import org.elasticsearch.search.aggregations.InternalOrder; +import org.elasticsearch.search.aggregations.AggregationBuilder; import org.elasticsearch.search.aggregations.bucket.histogram.AutoDateHistogramAggregationBuilder; -import org.elasticsearch.search.aggregations.bucket.histogram.AutoDateHistogramAggregatorFactory; -import org.elasticsearch.search.aggregations.bucket.histogram.DateHistogramAggregatorFactory; -import org.elasticsearch.search.aggregations.bucket.histogram.ExtendedBounds; -import org.elasticsearch.search.aggregations.bucket.histogram.HistogramAggregatorFactory; +import org.elasticsearch.search.aggregations.bucket.histogram.DateHistogramAggregationBuilder; +import org.elasticsearch.search.aggregations.bucket.histogram.HistogramAggregationBuilder; import org.elasticsearch.search.aggregations.metrics.AvgAggregationBuilder; import org.elasticsearch.search.aggregations.metrics.MaxAggregationBuilder; import org.elasticsearch.search.aggregations.metrics.MinAggregationBuilder; import org.elasticsearch.search.aggregations.metrics.SumAggregationBuilder; import org.elasticsearch.search.aggregations.support.ValuesSourceAggregationBuilder; -import org.elasticsearch.search.aggregations.support.ValuesSourceConfig; import org.elasticsearch.test.ESTestCase; import java.io.IOException; import java.util.ArrayList; -import java.util.Collections; - -import static org.mockito.Mockito.mock; +import java.util.function.Function; /** * Provides helper methods and classes for use in PipelineAggregation tests, @@ -155,28 +146,12 @@ public static double calculateMetric(double[] values, ValuesSourceAggregationBui return 0.0; } - static AggregatorFactory getRandomSequentiallyOrderedParentAgg() throws IOException { - AggregatorFactory factory = null; - switch (randomIntBetween(0, 2)) { - case 0: - factory = new HistogramAggregatorFactory("name", mock(ValuesSourceConfig.class), 0.0d, 0.0d, - mock(InternalOrder.class), false, 0L, 0.0d, 1.0d, mock(QueryShardContext.class), null, - new AggregatorFactories.Builder(), Collections.emptyMap()); - break; - case 1: - factory = new DateHistogramAggregatorFactory("name", mock(ValuesSourceConfig.class), - mock(InternalOrder.class), false, 0L, mock(Rounding.class), mock(Rounding.class), - mock(ExtendedBounds.class), mock(QueryShardContext.class), mock(AggregatorFactory.class), - new AggregatorFactories.Builder(), Collections.emptyMap()); - break; - case 2: - default: - AutoDateHistogramAggregationBuilder.RoundingInfo[] roundings = new AutoDateHistogramAggregationBuilder.RoundingInfo[1]; - factory = new AutoDateHistogramAggregatorFactory("name", mock(ValuesSourceConfig.class), - 1, roundings, - mock(QueryShardContext.class), null, new AggregatorFactories.Builder(), Collections.emptyMap()); - } - - return factory; + static AggregationBuilder getRandomSequentiallyOrderedParentAgg() throws IOException { + @SuppressWarnings("unchecked") + Function builder = randomFrom( + HistogramAggregationBuilder::new, + DateHistogramAggregationBuilder::new, + AutoDateHistogramAggregationBuilder::new); + return builder.apply("name"); } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/SerialDifferenceTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/SerialDifferenceTests.java index ef1d13990a308..c5b09a7a5726c 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/SerialDifferenceTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/SerialDifferenceTests.java @@ -19,16 +19,21 @@ package org.elasticsearch.search.aggregations.pipeline; +import org.elasticsearch.search.aggregations.AggregationBuilder; import org.elasticsearch.search.aggregations.BasePipelineAggregationTestCase; import org.elasticsearch.search.aggregations.PipelineAggregationBuilder; -import org.elasticsearch.search.aggregations.TestAggregatorFactory; import org.elasticsearch.search.aggregations.pipeline.BucketHelpers.GapPolicy; import java.io.IOException; -import java.util.Collections; import java.util.HashSet; import java.util.Set; +import static java.util.Collections.emptyList; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.nullValue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + public class SerialDifferenceTests extends BasePipelineAggregationTestCase { @Override @@ -47,32 +52,28 @@ protected SerialDiffPipelineAggregationBuilder createTestAggregatorFactory() { } return factory; } - + /** * The validation should verify the parent aggregation is allowed. */ public void testValidate() throws IOException { - final Set aggBuilders = new HashSet<>(); - aggBuilders.add(createTestAggregatorFactory()); - - final SerialDiffPipelineAggregationBuilder builder = new SerialDiffPipelineAggregationBuilder("name", "valid"); - builder.validate(PipelineAggregationHelperTests.getRandomSequentiallyOrderedParentAgg(), Collections.emptySet(), aggBuilders); + assertThat(validate(PipelineAggregationHelperTests.getRandomSequentiallyOrderedParentAgg(), + new SerialDiffPipelineAggregationBuilder("name", "valid")), nullValue()); } - /** - * The validation should throw an IllegalArgumentException, since parent - * aggregation is not a type of HistogramAggregatorFactory, - * DateHistogramAggregatorFactory or AutoDateHistogramAggregatorFactory. - */ - public void testValidateException() throws IOException { + public void testInvalidParent() throws IOException { final Set aggBuilders = new HashSet<>(); aggBuilders.add(createTestAggregatorFactory()); - TestAggregatorFactory parentFactory = TestAggregatorFactory.createInstance(); + AggregationBuilder parent = mock(AggregationBuilder.class); + when(parent.getName()).thenReturn("name"); + assertThat(validate(parent, new SerialDiffPipelineAggregationBuilder("name", "invalid_agg>metric")), equalTo( + "Validation Failed: 1: serial_diff aggregation [name] must have a histogram, " + + "date_histogram or auto_date_histogram as parent;")); + } - final SerialDiffPipelineAggregationBuilder builder = new SerialDiffPipelineAggregationBuilder("name", "invalid_agg>metric"); - IllegalStateException ex = expectThrows(IllegalStateException.class, - () -> builder.validate(parentFactory, Collections.emptySet(), aggBuilders)); - assertEquals("serial_diff aggregation [name] must have a histogram, date_histogram or auto_date_histogram as parent", - ex.getMessage()); + public void testNoParent() { + assertThat(validate(emptyList(), new SerialDiffPipelineAggregationBuilder("name", "invalid_agg>metric")), equalTo( + "Validation Failed: 1: serial_diff aggregation [name] must have a histogram, " + + "date_histogram or auto_date_histogram as parent but doesn't have a parent;")); } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/StatsBucketTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/StatsBucketTests.java index bf2ef7615df66..aac81c19be6bd 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/StatsBucketTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/StatsBucketTests.java @@ -24,10 +24,12 @@ import org.elasticsearch.search.aggregations.bucket.terms.TermsAggregationBuilder; import org.elasticsearch.search.aggregations.support.ValueType; -import java.util.Collections; import java.util.HashSet; import java.util.Set; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.nullValue; + public class StatsBucketTests extends AbstractBucketMetricsTestCase { @Override @@ -44,23 +46,18 @@ public void testValidate() { aggBuilders.add(multiBucketAgg); // First try to point to a non-existent agg - final StatsBucketPipelineAggregationBuilder builder = new StatsBucketPipelineAggregationBuilder("name", "invalid_agg>metric"); - IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, - () -> builder.validate(null, aggBuilders, Collections.emptySet())); - assertEquals(PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() - + " aggregation does not exist for aggregation [name]: invalid_agg>metric", ex.getMessage()); + assertThat(validate(aggBuilders, new StatsBucketPipelineAggregationBuilder("name", "invalid_agg>metric")), equalTo( + "Validation Failed: 1: " + PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() + + " aggregation does not exist for aggregation [name]: invalid_agg>metric;")); // Now try to point to a single bucket agg - StatsBucketPipelineAggregationBuilder builder2 = new StatsBucketPipelineAggregationBuilder("name", "global>metric"); - ex = expectThrows(IllegalArgumentException.class, () -> builder2.validate(null, aggBuilders, Collections.emptySet())); - assertEquals("The first aggregation in " + PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() + assertThat(validate(aggBuilders, new StatsBucketPipelineAggregationBuilder("name", "global>metric")), equalTo( + "Validation Failed: 1: The first aggregation in " + PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() + " must be a multi-bucket aggregation for aggregation [name] found :" + GlobalAggregationBuilder.class.getName() - + " for buckets path: global>metric", ex.getMessage()); + + " for buckets path: global>metric;")); - // Now try to point to a valid multi-bucket agg (no exception should be - // thrown) - StatsBucketPipelineAggregationBuilder builder3 = new StatsBucketPipelineAggregationBuilder("name", "terms>metric"); - builder3.validate(null, aggBuilders, Collections.emptySet()); + // Now try to point to a valid multi-bucket agg + assertThat(validate(aggBuilders, new StatsBucketPipelineAggregationBuilder("name", "terms>metric")), nullValue()); } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/SumBucketTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/SumBucketTests.java index fdba878524146..0bcbf592458aa 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/SumBucketTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/pipeline/SumBucketTests.java @@ -24,10 +24,12 @@ import org.elasticsearch.search.aggregations.bucket.terms.TermsAggregationBuilder; import org.elasticsearch.search.aggregations.support.ValueType; -import java.util.Collections; import java.util.HashSet; import java.util.Set; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.nullValue; + public class SumBucketTests extends AbstractBucketMetricsTestCase { @Override @@ -43,23 +45,18 @@ public void testValidate() { aggBuilders.add(multiBucketAgg); // First try to point to a non-existent agg - final SumBucketPipelineAggregationBuilder builder = new SumBucketPipelineAggregationBuilder("name", "invalid_agg>metric"); - IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, - () -> builder.validate(null, aggBuilders, Collections.emptySet())); - assertEquals(PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() - + " aggregation does not exist for aggregation [name]: invalid_agg>metric", ex.getMessage()); + assertThat(validate(aggBuilders, new SumBucketPipelineAggregationBuilder("name", "invalid_agg>metric")), equalTo( + "Validation Failed: 1: " + PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() + + " aggregation does not exist for aggregation [name]: invalid_agg>metric;")); // Now try to point to a single bucket agg - SumBucketPipelineAggregationBuilder builder2 = new SumBucketPipelineAggregationBuilder("name", "global>metric"); - ex = expectThrows(IllegalArgumentException.class, () -> builder2.validate(null, aggBuilders, Collections.emptySet())); - assertEquals("The first aggregation in " + PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() + assertThat(validate(aggBuilders, new SumBucketPipelineAggregationBuilder("name", "global>metric")), equalTo( + "Validation Failed: 1: The first aggregation in " + PipelineAggregator.Parser.BUCKETS_PATH.getPreferredName() + " must be a multi-bucket aggregation for aggregation [name] found :" + GlobalAggregationBuilder.class.getName() - + " for buckets path: global>metric", ex.getMessage()); + + " for buckets path: global>metric;")); - // Now try to point to a valid multi-bucket agg (no exception should be - // thrown) - SumBucketPipelineAggregationBuilder builder3 = new SumBucketPipelineAggregationBuilder("name", "terms>metric"); - builder3.validate(null, aggBuilders, Collections.emptySet()); + // Now try to point to a valid multi-bucket agg + assertThat(validate(aggBuilders, new SumBucketPipelineAggregationBuilder("name", "terms>metric")), nullValue()); } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/BasePipelineAggregationTestCase.java b/test/framework/src/main/java/org/elasticsearch/search/aggregations/BasePipelineAggregationTestCase.java similarity index 86% rename from server/src/test/java/org/elasticsearch/search/aggregations/BasePipelineAggregationTestCase.java rename to test/framework/src/main/java/org/elasticsearch/search/aggregations/BasePipelineAggregationTestCase.java index f8958a04f7cbe..cb3b8ec27c54b 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/BasePipelineAggregationTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/search/aggregations/BasePipelineAggregationTestCase.java @@ -19,6 +19,16 @@ package org.elasticsearch.search.aggregations; +import static java.util.Collections.emptyList; +import static org.elasticsearch.test.EqualsHashCodeTestUtils.checkEqualsAndHashCode; +import static org.hamcrest.Matchers.hasSize; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; +import java.util.List; + import org.elasticsearch.cluster.metadata.MetaData; import org.elasticsearch.common.io.stream.BytesStreamOutput; import org.elasticsearch.common.io.stream.NamedWriteableAwareStreamInput; @@ -33,20 +43,13 @@ import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.env.Environment; import org.elasticsearch.indices.IndicesModule; +import org.elasticsearch.plugins.SearchPlugin; import org.elasticsearch.search.SearchModule; +import org.elasticsearch.search.aggregations.PipelineAggregationBuilder.ValidationContext; import org.elasticsearch.search.aggregations.pipeline.AbstractPipelineAggregationBuilder; import org.elasticsearch.test.AbstractQueryTestCase; import org.elasticsearch.test.ESTestCase; -import java.io.IOException; -import java.util.ArrayList; -import java.util.Collections; -import java.util.List; - -import static java.util.Collections.emptyList; -import static org.elasticsearch.test.EqualsHashCodeTestUtils.checkEqualsAndHashCode; -import static org.hamcrest.Matchers.hasSize; - public abstract class BasePipelineAggregationTestCase> extends ESTestCase { protected static final String STRING_FIELD_NAME = "mapped_string"; @@ -91,6 +94,13 @@ public void setUp() throws Exception { } } + /** + * Plugins to add to the test. + */ + protected List plugins() { + return emptyList(); + } + /** * Generic test that creates new AggregatorFactory from the test * AggregatorFactory and checks both for equality and asserts equality on @@ -198,4 +208,34 @@ public String randomNumericField() { protected NamedXContentRegistry xContentRegistry() { return xContentRegistry; } + + /** + * Helper for testing validation. + */ + protected String validate(AggregationBuilder parent, AF builder) { + return validate(ValidationContext.forInsideTree(parent, null), builder); + } + + /** + * Helper for testing validation. + */ + protected String validate(Collection siblingAggregations, AF builder) { + return validate(siblingAggregations, emptyList(), builder); + } + + /** + * Helper for testing validation. + */ + protected String validate(Collection siblingAggregations, + Collection siblingPipelineAggregations, AF builder) { + return validate(ValidationContext.forTreeRoot(siblingAggregations, siblingPipelineAggregations, null), builder); + } + + /** + * Helper for testing validation. + */ + protected String validate(ValidationContext context, AF builder) { + builder.validate(context); + return context.getValidationException() == null ? null : context.getValidationException().getMessage(); + } } diff --git a/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/cumulativecardinality/CumulativeCardinalityPipelineAggregationBuilder.java b/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/cumulativecardinality/CumulativeCardinalityPipelineAggregationBuilder.java index 844c661aadf7e..47480d98a00e2 100644 --- a/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/cumulativecardinality/CumulativeCardinalityPipelineAggregationBuilder.java +++ b/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/cumulativecardinality/CumulativeCardinalityPipelineAggregationBuilder.java @@ -10,20 +10,15 @@ import org.elasticsearch.common.xcontent.ConstructingObjectParser; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.search.DocValueFormat; -import org.elasticsearch.search.aggregations.AggregationBuilder; -import org.elasticsearch.search.aggregations.AggregatorFactory; -import org.elasticsearch.search.aggregations.PipelineAggregationBuilder; import org.elasticsearch.search.aggregations.pipeline.AbstractPipelineAggregationBuilder; import org.elasticsearch.search.aggregations.pipeline.BucketMetricsParser; import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; import java.io.IOException; -import java.util.Collection; import java.util.Map; import java.util.Objects; import static org.elasticsearch.common.xcontent.ConstructingObjectParser.constructorArg; -import static org.elasticsearch.search.aggregations.pipeline.PipelineAggregator.Parser.BUCKETS_PATH; import static org.elasticsearch.search.aggregations.pipeline.PipelineAggregator.Parser.FORMAT; public class CumulativeCardinalityPipelineAggregationBuilder @@ -90,14 +85,12 @@ protected PipelineAggregator createInternal(Map metaData) { } @Override - public void doValidate(AggregatorFactory parent, Collection aggFactories, - Collection pipelineAggregatorFactories) { + protected void validate(ValidationContext context) { if (bucketsPaths.length != 1) { - throw new IllegalStateException(BUCKETS_PATH.getPreferredName() - + " must contain a single entry for aggregation [" + name + "]"); + context.addBucketPathValidationError("must contain a single entry for aggregation [" + name + "]"); } - validateSequentiallyOrderedParentAggs(parent, NAME, name); + context.validateParentAggSequentiallyOrdered(NAME, name); } @Override @@ -105,6 +98,7 @@ protected final XContentBuilder internalXContent(XContentBuilder builder, Params if (format != null) { builder.field(BucketMetricsParser.FORMAT.getPreferredName(), format); } + builder.field(BUCKETS_PATH_FIELD.getPreferredName(), bucketsPaths[0]); return builder; } @@ -126,4 +120,9 @@ public boolean equals(Object obj) { public String getWriteableName() { return NAME; } + + @Override + protected boolean overrideBucketsPath() { + return true; + } } diff --git a/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/StubAggregatorFactory.java b/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/StubAggregatorFactory.java deleted file mode 100644 index ace86ddea6bb8..0000000000000 --- a/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/StubAggregatorFactory.java +++ /dev/null @@ -1,56 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License; - * you may not use this file except in compliance with the Elastic License. - */ -package org.elasticsearch.xpack.analytics; - -import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.common.util.BigArrays; -import org.elasticsearch.common.util.MockBigArrays; -import org.elasticsearch.common.util.MockPageCacheRecycler; -import org.elasticsearch.index.query.QueryShardContext; -import org.elasticsearch.indices.breaker.NoneCircuitBreakerService; -import org.elasticsearch.search.aggregations.Aggregator; -import org.elasticsearch.search.aggregations.AggregatorFactories; -import org.elasticsearch.search.aggregations.AggregatorFactory; -import org.elasticsearch.search.internal.SearchContext; - -import java.io.IOException; -import java.util.Collections; -import java.util.List; -import java.util.Map; - -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; - -/** - * Test implementation for AggregatorFactory. - */ -public class StubAggregatorFactory extends AggregatorFactory { - - private final Aggregator aggregator; - - private StubAggregatorFactory(QueryShardContext queryShardContext, Aggregator aggregator) throws IOException { - super("_name", queryShardContext, null, new AggregatorFactories.Builder(), Collections.emptyMap()); - this.aggregator = aggregator; - } - - @Override - protected Aggregator createInternal(SearchContext searchContext, - Aggregator parent, - boolean collectsFromSingleBucket, - List list, Map metaData) throws IOException { - return aggregator; - } - - public static StubAggregatorFactory createInstance() throws IOException { - BigArrays bigArrays = new MockBigArrays(new MockPageCacheRecycler(Settings.EMPTY), new NoneCircuitBreakerService()); - QueryShardContext queryShardContext = mock(QueryShardContext.class); - when(queryShardContext.bigArrays()).thenReturn(bigArrays); - - Aggregator aggregator = mock(Aggregator.class); - - return new StubAggregatorFactory(queryShardContext, aggregator); - } -} diff --git a/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/cumulativecardinality/CumulativeCardinalityAggregatorTests.java b/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/cumulativecardinality/CumulativeCardinalityAggregatorTests.java index 9f49588d7fc14..fd7adddd4234c 100644 --- a/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/cumulativecardinality/CumulativeCardinalityAggregatorTests.java +++ b/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/cumulativecardinality/CumulativeCardinalityAggregatorTests.java @@ -16,46 +16,27 @@ import org.apache.lucene.search.Query; import org.apache.lucene.store.Directory; import org.elasticsearch.common.CheckedConsumer; -import org.elasticsearch.common.Rounding; import org.elasticsearch.common.time.DateFormatters; import org.elasticsearch.index.mapper.DateFieldMapper; import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.index.mapper.NumberFieldMapper; -import org.elasticsearch.index.query.QueryShardContext; import org.elasticsearch.search.aggregations.AggregationBuilder; import org.elasticsearch.search.aggregations.AggregationExecutionException; -import org.elasticsearch.search.aggregations.AggregatorFactories; -import org.elasticsearch.search.aggregations.AggregatorFactory; import org.elasticsearch.search.aggregations.AggregatorTestCase; import org.elasticsearch.search.aggregations.InternalAggregation; -import org.elasticsearch.search.aggregations.InternalOrder; -import org.elasticsearch.search.aggregations.PipelineAggregationBuilder; -import org.elasticsearch.search.aggregations.bucket.histogram.AutoDateHistogramAggregationBuilder; -import org.elasticsearch.search.aggregations.bucket.histogram.AutoDateHistogramAggregatorFactory; import org.elasticsearch.search.aggregations.bucket.histogram.DateHistogramAggregationBuilder; -import org.elasticsearch.search.aggregations.bucket.histogram.DateHistogramAggregatorFactory; import org.elasticsearch.search.aggregations.bucket.histogram.DateHistogramInterval; -import org.elasticsearch.search.aggregations.bucket.histogram.ExtendedBounds; import org.elasticsearch.search.aggregations.bucket.histogram.Histogram; -import org.elasticsearch.search.aggregations.bucket.histogram.HistogramAggregatorFactory; import org.elasticsearch.search.aggregations.metrics.CardinalityAggregationBuilder; import org.elasticsearch.search.aggregations.metrics.SumAggregationBuilder; -import org.elasticsearch.search.aggregations.support.CoreValuesSourceType; import org.elasticsearch.search.aggregations.support.ValueType; -import org.elasticsearch.search.aggregations.support.ValuesSource; -import org.elasticsearch.search.aggregations.support.ValuesSourceConfig; -import org.elasticsearch.xpack.analytics.StubAggregatorFactory; import java.io.IOException; import java.util.Arrays; -import java.util.Collections; -import java.util.HashSet; import java.util.List; -import java.util.Set; import java.util.function.Consumer; import static org.hamcrest.Matchers.equalTo; -import static org.mockito.Mockito.mock; public class CumulativeCardinalityAggregatorTests extends AggregatorTestCase { @@ -115,53 +96,6 @@ public void testAllNull() throws IOException { }); } - public void testParentValidations() throws IOException { - ValuesSourceConfig valuesSourceConfig = new ValuesSourceConfig<>(CoreValuesSourceType.NUMERIC); - - // Histogram - Set aggBuilders = new HashSet<>(); - aggBuilders.add(new CumulativeCardinalityPipelineAggregationBuilder("cumulative_card", "sum")); - AggregatorFactory parent = new HistogramAggregatorFactory("name", valuesSourceConfig, 0.0d, 0.0d, - mock(InternalOrder.class), false, 0L, 0.0d, 1.0d, mock(QueryShardContext.class), null, - new AggregatorFactories.Builder(), Collections.emptyMap()); - CumulativeCardinalityPipelineAggregationBuilder builder - = new CumulativeCardinalityPipelineAggregationBuilder("name", "valid"); - builder.validate(parent, Collections.emptySet(), aggBuilders); - - // Date Histogram - aggBuilders.clear(); - aggBuilders.add(new CumulativeCardinalityPipelineAggregationBuilder("cumulative_card", "sum")); - parent = new DateHistogramAggregatorFactory("name", valuesSourceConfig, - mock(InternalOrder.class), false, 0L, mock(Rounding.class), mock(Rounding.class), - mock(ExtendedBounds.class), mock(QueryShardContext.class), mock(AggregatorFactory.class), - new AggregatorFactories.Builder(), Collections.emptyMap()); - builder = new CumulativeCardinalityPipelineAggregationBuilder("name", "valid"); - builder.validate(parent, Collections.emptySet(), aggBuilders); - - // Auto Date Histogram - ValuesSourceConfig numericVS = new ValuesSourceConfig<>(CoreValuesSourceType.NUMERIC); - aggBuilders.clear(); - aggBuilders.add(new CumulativeCardinalityPipelineAggregationBuilder("cumulative_card", "sum")); - AutoDateHistogramAggregationBuilder.RoundingInfo[] roundings = new AutoDateHistogramAggregationBuilder.RoundingInfo[1]; - parent = new AutoDateHistogramAggregatorFactory("name", numericVS, - 1, roundings, - mock(QueryShardContext.class), null, new AggregatorFactories.Builder(), Collections.emptyMap()); - builder = new CumulativeCardinalityPipelineAggregationBuilder("name", "valid"); - builder.validate(parent, Collections.emptySet(), aggBuilders); - - // Mocked "test" agg, should fail validation - aggBuilders.clear(); - aggBuilders.add(new CumulativeCardinalityPipelineAggregationBuilder("cumulative_card", "sum")); - StubAggregatorFactory parentFactory = StubAggregatorFactory.createInstance(); - - CumulativeCardinalityPipelineAggregationBuilder failBuilder - = new CumulativeCardinalityPipelineAggregationBuilder("name", "invalid_agg>metric"); - IllegalStateException ex = expectThrows(IllegalStateException.class, - () -> failBuilder.validate(parentFactory, Collections.emptySet(), aggBuilders)); - assertEquals("cumulative_cardinality aggregation [name] must have a histogram, date_histogram or auto_date_histogram as parent", - ex.getMessage()); - } - public void testNonCardinalityAgg() { Query query = new MatchAllDocsQuery(); diff --git a/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/cumulativecardinality/CumulativeCardinalityTests.java b/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/cumulativecardinality/CumulativeCardinalityTests.java new file mode 100644 index 0000000000000..d9ff43e7d06c8 --- /dev/null +++ b/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/cumulativecardinality/CumulativeCardinalityTests.java @@ -0,0 +1,73 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +package org.elasticsearch.xpack.analytics.cumulativecardinality; + +import org.elasticsearch.plugins.SearchPlugin; +import org.elasticsearch.search.aggregations.AggregationBuilder; +import org.elasticsearch.search.aggregations.BasePipelineAggregationTestCase; +import org.elasticsearch.search.aggregations.bucket.histogram.AutoDateHistogramAggregationBuilder; +import org.elasticsearch.search.aggregations.bucket.histogram.DateHistogramAggregationBuilder; +import org.elasticsearch.search.aggregations.bucket.histogram.HistogramAggregationBuilder; + +import java.io.IOException; +import java.util.List; + +import static java.util.Collections.emptyList; +import static java.util.Collections.singletonList; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.nullValue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +public class CumulativeCardinalityTests extends BasePipelineAggregationTestCase { + @Override + protected List plugins() { + return singletonList(new SearchPlugin() { + @Override + public List getPipelineAggregations() { + return singletonList(new PipelineAggregationSpec( + CumulativeCardinalityPipelineAggregationBuilder.NAME, + CumulativeCardinalityPipelineAggregationBuilder::new, + CumulativeCardinalityPipelineAggregator::new, + CumulativeCardinalityPipelineAggregationBuilder.PARSER)); + } + }); + } + + @Override + protected CumulativeCardinalityPipelineAggregationBuilder createTestAggregatorFactory() { + String name = randomAlphaOfLengthBetween(3, 20); + String bucketsPath = randomAlphaOfLengthBetween(3, 20); + CumulativeCardinalityPipelineAggregationBuilder builder = + new CumulativeCardinalityPipelineAggregationBuilder(name, bucketsPath); + if (randomBoolean()) { + builder.format(randomAlphaOfLengthBetween(1, 10)); + } + return builder; + } + + + public void testParentValidations() throws IOException { + CumulativeCardinalityPipelineAggregationBuilder builder = + new CumulativeCardinalityPipelineAggregationBuilder("name", randomAlphaOfLength(5)); + + assertThat(validate(new HistogramAggregationBuilder("name"), builder), nullValue()); + assertThat(validate(new DateHistogramAggregationBuilder("name"), builder), nullValue()); + assertThat(validate(new AutoDateHistogramAggregationBuilder("name"), builder), nullValue()); + + // Mocked "test" agg, should fail validation + AggregationBuilder stubParent = mock(AggregationBuilder.class); + when(stubParent.getName()).thenReturn("name"); + assertThat(validate(stubParent, builder), equalTo( + "Validation Failed: 1: cumulative_cardinality aggregation [name] must have a histogram, " + + "date_histogram or auto_date_histogram as parent;")); + + assertThat(validate(emptyList(), builder), equalTo( + "Validation Failed: 1: cumulative_cardinality aggregation [name] must have a histogram, " + + "date_histogram or auto_date_histogram as parent but doesn't have a parent;")); + } +}