From d50883c46d353cc15ad8007273b21114b3a5f1da Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Wed, 19 Jan 2022 14:13:46 -0500 Subject: [PATCH 1/5] Add back a nice-r error message for moving_avg But only when using compatible-with=7. --- .../elasticsearch/search/SearchModule.java | 10 +++ .../MovAvgPipelineAggregationBuilder.java | 66 +++++++++++++++++++ 2 files changed, 76 insertions(+) create mode 100644 server/src/main/java/org/elasticsearch/search/aggregations/pipeline/MovAvgPipelineAggregationBuilder.java diff --git a/server/src/main/java/org/elasticsearch/search/SearchModule.java b/server/src/main/java/org/elasticsearch/search/SearchModule.java index 7b7fa2e8b4abf..7f85d59642cbc 100644 --- a/server/src/main/java/org/elasticsearch/search/SearchModule.java +++ b/server/src/main/java/org/elasticsearch/search/SearchModule.java @@ -201,6 +201,7 @@ import org.elasticsearch.search.aggregations.pipeline.InternalStatsBucket; import org.elasticsearch.search.aggregations.pipeline.MaxBucketPipelineAggregationBuilder; import org.elasticsearch.search.aggregations.pipeline.MinBucketPipelineAggregationBuilder; +import org.elasticsearch.search.aggregations.pipeline.MovAvgPipelineAggregationBuilder; import org.elasticsearch.search.aggregations.pipeline.MovFnPipelineAggregationBuilder; import org.elasticsearch.search.aggregations.pipeline.PercentilesBucketPipelineAggregationBuilder; import org.elasticsearch.search.aggregations.pipeline.SerialDiffPipelineAggregationBuilder; @@ -783,6 +784,15 @@ private void registerPipelineAggregations(List plugins) { MovFnPipelineAggregationBuilder.PARSER ) ); + if (RestApiVersion.minimumSupported() == RestApiVersion.V_7) { + registerPipelineAggregation( + new PipelineAggregationSpec( + MovAvgPipelineAggregationBuilder.NAME_V7, + MovAvgPipelineAggregationBuilder::new, + MovAvgPipelineAggregationBuilder.PARSER + ) + ); + } registerFromPlugin(plugins, SearchPlugin::getPipelineAggregations, this::registerPipelineAggregation); } 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 new file mode 100644 index 0000000000000..59447cfc0818c --- /dev/null +++ b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/MovAvgPipelineAggregationBuilder.java @@ -0,0 +1,66 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.search.aggregations.pipeline; + +import org.elasticsearch.common.ParsingException; +import org.elasticsearch.common.io.stream.StreamInput; +import org.elasticsearch.common.io.stream.StreamOutput; +import org.elasticsearch.common.logging.DeprecationLogger; +import org.elasticsearch.core.RestApiVersion; +import org.elasticsearch.index.query.CommonTermsQueryBuilder; +import org.elasticsearch.xcontent.ContextParser; +import org.elasticsearch.xcontent.ParseField; +import org.elasticsearch.xcontent.XContentBuilder; + +import java.io.IOException; +import java.util.Map; + +public class MovAvgPipelineAggregationBuilder extends AbstractPipelineAggregationBuilder { + private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(CommonTermsQueryBuilder.class); + public static final String MOVING_AVG_AGG_DEPRECATION_MSG = "Moving Average aggregation usage is not supported. " + + "Use the [moving_fn] aggregation instead."; + + public static ParseField NAME_V7 = new ParseField("moving_avg").withAllDeprecated(MOVING_AVG_AGG_DEPRECATION_MSG) + .forRestApiVersion(RestApiVersion.equalTo(RestApiVersion.V_7)); + + public static final ContextParser PARSER = (parser, name) -> { + deprecationLogger.compatibleCritical("moving_avg_aggregation", MOVING_AVG_AGG_DEPRECATION_MSG); + throw new ParsingException(parser.getTokenLocation(), MOVING_AVG_AGG_DEPRECATION_MSG); + }; + + public MovAvgPipelineAggregationBuilder(StreamInput in) throws IOException { + super(in, NAME_V7.getPreferredName()); + throw new UnsupportedOperationException("moving_avg is not meant to be used."); + } + + @Override + protected void doWriteTo(StreamOutput out) throws IOException { + throw new UnsupportedOperationException("moving_avg is not meant to be used."); + } + + @Override + protected PipelineAggregator createInternal(Map metadata) { + throw new UnsupportedOperationException("moving_avg is not meant to be used."); + } + + @Override + protected XContentBuilder internalXContent(XContentBuilder builder, Params params) throws IOException { + throw new UnsupportedOperationException("moving_avg is not meant to be used."); + } + + @Override + protected void validate(ValidationContext context) { + throw new UnsupportedOperationException("moving_avg is not meant to be used."); + } + + @Override + public final String getWriteableName() { + return null; + } +} From 638ca766f519b0282aef32012097ffbcbdebeb96 Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Wed, 19 Jan 2022 16:19:33 -0500 Subject: [PATCH 2/5] And a yamlRestTestV7Compat test --- .../test/search.aggregation/10_moving_avg.yml | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100644 rest-api-spec/src/yamlRestTestV7Compat/resources/rest-api-spec/test/search.aggregation/10_moving_avg.yml diff --git a/rest-api-spec/src/yamlRestTestV7Compat/resources/rest-api-spec/test/search.aggregation/10_moving_avg.yml b/rest-api-spec/src/yamlRestTestV7Compat/resources/rest-api-spec/test/search.aggregation/10_moving_avg.yml new file mode 100644 index 0000000000000..98ff9065dd472 --- /dev/null +++ b/rest-api-spec/src/yamlRestTestV7Compat/resources/rest-api-spec/test/search.aggregation/10_moving_avg.yml @@ -0,0 +1,30 @@ +--- +setup: + - skip: + version: "9.0.0 - " + reason: "compatible from 8.x to 7.x" + features: + - "headers" + +--- +moving_avg agg throws exception: + - do: + catch: "/Moving Average aggregation usage is not supported. Use the \\[moving_fn\\] aggregation instead./" + search: + rest_total_hits_as_int: true + body: + aggs: + the_histo: + date_histogram: + field: "date" + calendar_interval: "1d" + aggs: + the_avg: + avg: + field: "value_field" + the_movavg: + moving_avg: + buckets_path: "the_avg" + headers: + Content-Type: "application/vnd.elasticsearch+json;compatible-with=7" + Accept: "application/vnd.elasticsearch+json;compatible-with=7" From 5f9eb6977fd2957b5fc0ae8cdfd9bdaa1f233ffa Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Wed, 19 Jan 2022 16:53:21 -0500 Subject: [PATCH 3/5] Need to filter out this aggregation in the test, too --- .../java/org/elasticsearch/search/SearchModuleTests.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/server/src/test/java/org/elasticsearch/search/SearchModuleTests.java b/server/src/test/java/org/elasticsearch/search/SearchModuleTests.java index 148e3768ce126..7de59898a7c79 100644 --- a/server/src/test/java/org/elasticsearch/search/SearchModuleTests.java +++ b/server/src/test/java/org/elasticsearch/search/SearchModuleTests.java @@ -36,6 +36,7 @@ import org.elasticsearch.search.aggregations.pipeline.AbstractPipelineAggregationBuilder; import org.elasticsearch.search.aggregations.pipeline.DerivativePipelineAggregationBuilder; import org.elasticsearch.search.aggregations.pipeline.InternalDerivative; +import org.elasticsearch.search.aggregations.pipeline.MovAvgPipelineAggregationBuilder; import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; import org.elasticsearch.search.aggregations.support.AggregationContext; import org.elasticsearch.search.aggregations.support.CoreValuesSourceType; @@ -478,6 +479,8 @@ public CheckedBiConsumer getReque private static final String[] REST_COMPATIBLE_QUERIES = new String[] { TypeQueryV7Builder.NAME_V7.getPreferredName(), CommonTermsQueryBuilder.NAME_V7.getPreferredName() }; + private static final String[] REST_COMPATIBLE_AGGREGATIONS = new String[] { + MovAvgPipelineAggregationBuilder.NAME_V7.getPreferredName() }; /** * Dummy test {@link AggregationBuilder} used to test registering aggregation builders. @@ -765,7 +768,7 @@ public List> getQueries() { .filter(e -> RestApiVersion.current().matches(e.restApiCompatibility)) .collect(toSet()), // -1 because of the registered in the test - hasSize(searchModule.getNamedXContents().size() - REST_COMPATIBLE_QUERIES.length - 1) + hasSize(searchModule.getNamedXContents().size() - REST_COMPATIBLE_QUERIES.length - REST_COMPATIBLE_AGGREGATIONS.length - 1) ); final List compatEntry = searchModule.getNamedXContents() From 9f5663257d6562bab2e4adec7aece987d64fef5c Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Wed, 19 Jan 2022 16:20:54 -0500 Subject: [PATCH 4/5] Move this nearby test foo/bar paths are unusual for these tests, while foo.bar directories are very usual. --- .../test/{search/sort => search.sort}/10_nested_path_filter.yml | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename rest-api-spec/src/yamlRestTestV7Compat/resources/rest-api-spec/test/{search/sort => search.sort}/10_nested_path_filter.yml (100%) diff --git a/rest-api-spec/src/yamlRestTestV7Compat/resources/rest-api-spec/test/search/sort/10_nested_path_filter.yml b/rest-api-spec/src/yamlRestTestV7Compat/resources/rest-api-spec/test/search.sort/10_nested_path_filter.yml similarity index 100% rename from rest-api-spec/src/yamlRestTestV7Compat/resources/rest-api-spec/test/search/sort/10_nested_path_filter.yml rename to rest-api-spec/src/yamlRestTestV7Compat/resources/rest-api-spec/test/search.sort/10_nested_path_filter.yml From c1d7a35563cd9105954fb7f81bafd63e3af43fff Mon Sep 17 00:00:00 2001 From: Joe Gallo Date: Thu, 20 Jan 2022 16:04:19 -0500 Subject: [PATCH 5/5] Deprecate it, and add an explanation --- .../pipeline/MovAvgPipelineAggregationBuilder.java | 9 +++++++++ 1 file changed, 9 insertions(+) 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 59447cfc0818c..55c62768885c0 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 @@ -21,6 +21,15 @@ import java.io.IOException; import java.util.Map; +/** + * The actual moving_avg aggregation was removed as a breaking change in 8.0. This class exists to provide a friendlier error message + * if somebody attempts to use the moving_avg aggregation via the compatible-with=7 mechanism. + * + * We can remove this class entirely when v7 rest api compatibility is dropped. + * + * @deprecated Only for 7.x rest compat + */ +@Deprecated public class MovAvgPipelineAggregationBuilder extends AbstractPipelineAggregationBuilder { private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(CommonTermsQueryBuilder.class); public static final String MOVING_AVG_AGG_DEPRECATION_MSG = "Moving Average aggregation usage is not supported. "