Scripted metric aggregations: add deprecation warning and system (#32944)

property to control legacy params (#31597)

* Scripted metric aggregations: add deprecation warning and system
property to control legacy params

Scripted metric aggregation params._agg/_aggs are replaced by
state/states context variables. By default the old params are still
present, and a deprecation warning is emitted when Scripted Metric
Aggregations are used. A new system property can be used to disable the
legacy params. This functionality will be removed in a future revision.

* Fix minor style issue and docs test failure

* Disable deprecated params._agg/_aggs in tests and revise tests to use
state/states instead

* Add integration test covering deprecated scripted metrics aggs
params._agg/_aggs access

* Disable deprecated params._agg/_aggs in docs integration tests and
revise stored scripts to use state/states instead

* Revert unnecessary migrations doc change

A relevant note should be added in the changes destined for 7.0; this
PR is going to be backported to 6.x.

* Replace deprecated _agg param bwc integration test with a couple of
unit tests

* Fix compatibility test after merge

* Rename backwards compatibility system property per code review
feedback

* Tweak deprecation warning text per review feedback

buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy
server/src/test/java/org/elasticsearch/search/aggregations/metrics/scrip
ted/ScriptedMetricAggregatorTests.java
/Users/colings86/dev/work/git/elasticsearch/.git/worktrees/elasticsearch
-6.x/CHERRY_PICK_HEAD

buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy
server/src/main/java/org/elasticsearch/script/ScriptedMetricAggContexts.
java
server/src/main/java/org/elasticsearch/search/aggregations/metrics/scrip
ted/InternalScriptedMetric.java
server/src/main/java/org/elasticsearch/search/aggregations/metrics/scrip
ted/ScriptedMetricAggregatorFactory.java
server/src/test/java/org/elasticsearch/search/aggregations/metrics/Scrip
tedMetricIT.java
server/src/test/java/org/elasticsearch/search/aggregations/metrics/scrip
ted/InternalScriptedMetricAggStateV6CompatTests.java
server/src/test/java/org/elasticsearch/search/aggregations/metrics/scrip
ted/InternalScriptedMetricTests.java
server/src/test/java/org/elasticsearch/search/aggregations/metrics/scrip
ted/ScriptedMetricAggregatorAggStateV6CompatTests.java
server/src/test/java/org/elasticsearch/search/aggregations/metrics/scrip
ted/ScriptedMetricAggregatorTests.java

Author: colings86

buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy

@@ -798,6 +798,8 @@ class BuildPlugin implements Plugin<Project> {
             systemProperty 'tests.task', path
             systemProperty 'tests.security.manager', 'true'
             systemProperty 'jna.nosys', 'true'
+            // TODO: remove this deprecation compatibility setting for 7.0
+            systemProperty 'es.aggregations.enable_scripted_metric_agg_param', 'false'
             systemProperty 'es.scripting.exception_for_missing_value', 'true'
             systemProperty 'compiler.java', project.ext.compilerJavaVersion.getMajorVersion()
             if (project.ext.inFipsJvm) {

docs/build.gradle

@@ -41,6 +41,9 @@ integTestCluster {
   // TODO: remove this for 7.0, this exists to allow the doc examples in 6.x to continue using the defaults
   systemProperty 'es.scripting.use_java_time', 'false'
   systemProperty 'es.scripting.update.ctx_in_params', 'false'
+
+  // TODO: remove this deprecation compatibility setting for 7.0
+  systemProperty 'es.aggregations.enable_scripted_metric_agg_param', 'false'
 }
 
 // remove when https://github.com/elastic/elasticsearch/issues/31305 is fixed
@@ -393,25 +396,25 @@ buildRestTests.setups['stored_scripted_metric_script'] = '''
   - do:
       put_script:
         id: "my_init_script"
-        body: { "script": { "lang": "painless", "source": "params._agg.transactions = []" } }
+        body: { "script": { "lang": "painless", "source": "state.transactions = []" } }
   - match: { acknowledged: true }
 
   - do:
       put_script:
         id: "my_map_script"
-        body: { "script": { "lang": "painless", "source": "params._agg.transactions.add(doc.type.value == 'sale' ? doc.amount.value : -1 * doc.amount.value)" } }
+        body: { "script": { "lang": "painless", "source": "state.transactions.add(doc.type.value == 'sale' ? doc.amount.value : -1 * doc.amount.value)" } }
   - match: { acknowledged: true }
 
   - do:
       put_script:
         id: "my_combine_script"
-        body: { "script": { "lang": "painless", "source": "double profit = 0;for (t in params._agg.transactions) { profit += t; } return profit" } }
+        body: { "script": { "lang": "painless", "source": "double profit = 0;for (t in state.transactions) { profit += t; } return profit" } }
   - match: { acknowledged: true }
 
   - do:
       put_script:
         id: "my_reduce_script"
-        body: { "script": { "lang": "painless", "source": "double profit = 0;for (a in params._aggs) { profit += a; } return profit" } }
+        body: { "script": { "lang": "painless", "source": "double profit = 0;for (a in states) { profit += a; } return profit" } }
   - match: { acknowledged: true }
 '''
 

server/build.gradle

@@ -351,3 +351,15 @@ if (isEclipse == false || project.path == ":server-tests") {
   check.dependsOn integTest
   integTest.mustRunAfter test
 }
+// TODO: remove these compatibility tests in 7.0
+additionalTest('testScriptedMetricAggParamsV6Compatibility') {
+  include '**/ScriptedMetricAggregatorAggStateV6CompatTests.class'
+  include '**/InternalScriptedMetricAggStateV6CompatTests.class'
+  systemProperty 'es.aggregations.enable_scripted_metric_agg_param', 'true'
+}
+
+test {
+  // these are tested explicitly in separate test tasks
+  exclude '**/ScriptedMetricAggregatorAggStateV6CompatTests.class'
+  exclude '**/InternalScriptedMetricAggStateV6CompatTests.class'
+}

server/src/main/java/org/elasticsearch/script/ScriptedMetricAggContexts.java

@@ -22,6 +22,8 @@
 import org.apache.lucene.index.LeafReaderContext;
 import org.apache.lucene.search.Scorer;
 import org.elasticsearch.ElasticsearchException;
+import org.elasticsearch.common.logging.DeprecationLogger;
+import org.elasticsearch.common.logging.Loggers;
 import org.elasticsearch.index.fielddata.ScriptDocValues;
 import org.elasticsearch.search.lookup.LeafSearchLookup;
 import org.elasticsearch.search.lookup.SearchLookup;
@@ -31,6 +33,25 @@
 import java.util.Map;
 
 public class ScriptedMetricAggContexts {
+    private static final DeprecationLogger DEPRECATION_LOGGER =
+        new DeprecationLogger(Loggers.getLogger(ScriptedMetricAggContexts.class));
+
+    // Public for access from tests
+    public static final String AGG_PARAM_DEPRECATION_WARNING =
+        "params._agg/_aggs for scripted metric aggregations are deprecated, use state/states (not in params) instead. " +
+        "Use -Des.aggregations.enable_scripted_metric_agg_param=false to disable.";
+
+    public static boolean deprecatedAggParamEnabled() {
+        boolean enabled = Boolean.parseBoolean(
+            System.getProperty("es.aggregations.enable_scripted_metric_agg_param", "true"));
+
+        if (enabled) {
+            DEPRECATION_LOGGER.deprecatedAndMaybeLog("enable_scripted_metric_agg_param", AGG_PARAM_DEPRECATION_WARNING);
+        }
+
+        return enabled;
+    }
+
     private abstract static class ParamsAndStateBase {
         private final Map<String, Object> params;
         private final Object state;

server/src/main/java/org/elasticsearch/search/aggregations/metrics/scripted/InternalScriptedMetric.java

@@ -96,7 +96,9 @@ public InternalAggregation doReduce(List<InternalAggregation> aggregations, Redu
             }
 
             // Add _aggs to params map for backwards compatibility (redundant with a context variable on the ReduceScript created below).
-            params.put("_aggs", aggregationObjects);
+            if (ScriptedMetricAggContexts.deprecatedAggParamEnabled()) {
+                params.put("_aggs", aggregationObjects);
+            }
 
             ScriptedMetricAggContexts.ReduceScript.Factory factory = reduceContext.scriptService().compile(
                 firstAggregation.reduceScript, ScriptedMetricAggContexts.ReduceScript.CONTEXT);

server/src/main/java/org/elasticsearch/search/aggregations/metrics/scripted/ScriptedMetricAggregatorFactory.java

@@ -83,10 +83,17 @@ public Aggregator createInternal(Aggregator parent, boolean collectsFromSingleBu
         // Add _agg to params map for backwards compatibility (redundant with context variables on the scripts created below).
         // When this is removed, aggState (as passed to ScriptedMetricAggregator) can be changed to Map<String, Object>, since
         // it won't be possible to completely replace it with another type as is possible when it's an entry in params.
-        if (aggParams.containsKey("_agg") == false) {
-            aggParams.put("_agg", new HashMap<String, Object>());
+        Object aggState = new HashMap<String, Object>();
+        if (ScriptedMetricAggContexts.deprecatedAggParamEnabled()) {
+            if (aggParams.containsKey("_agg") == false) {
+                // Add _agg if it wasn't added manually
+                aggParams.put("_agg", aggState);
+            } else {
+                // If it was added manually, also use it for the agg context variable to reduce the likelihood of
+                // weird behavior due to multiple different variables.
+                aggState = aggParams.get("_agg");
+            }
         }
-        Object aggState = aggParams.get("_agg");
 
         final ScriptedMetricAggContexts.InitScript initScript = this.initScript.newInstance(
             mergeParams(aggParams, initScriptParams), aggState);