Scripted metric aggregations: add deprecation warning and system prop…

…erty 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

@@ -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 'compiler.java', project.ext.compilerJavaVersion.getMajorVersion()
             if (project.ext.inFipsJvm) {
                 systemProperty 'runtime.java', project.ext.runtimeJavaVersion.getMajorVersion() + "FIPS"

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
@@ -400,25 +403,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

@@ -342,3 +342,15 @@ if (isEclipse == false || project.path == ":server-tests") {
   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);