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

docs/reference/aggregations/metrics/scripted-metric-aggregation.asciidoc

@@ -26,6 +26,7 @@ POST ledger/_search?size=0
 --------------------------------------------------
 // CONSOLE
 // TEST[setup:ledger]
+// TEST[warning:params._agg/_aggs for scripted metric aggregations are deprecated, use state/states (not in params) instead. Set system property es.aggregations.enableDeprecatedScriptedMetricAggParam = false to disable this deprecated behavior.]
 
 <1> `map_script` is the only required  parameter
 
@@ -79,6 +80,7 @@ POST ledger/_search?size=0
 --------------------------------------------------
 // CONSOLE
 // TEST[setup:ledger,stored_scripted_metric_script]
+// TEST[warning:params._agg/_aggs for scripted metric aggregations are deprecated, use state/states (not in params) instead. Set system property es.aggregations.enableDeprecatedScriptedMetricAggParam = false to disable this deprecated behavior.]
 
 <1> script parameters for `init`, `map` and `combine` scripts must be specified
 in a global `params` object so that it can be shared between the scripts.

docs/reference/migration/migrate_7_0/aggregations.asciidoc

@@ -22,4 +22,5 @@ The object used to share aggregation state between the scripts in a Scripted Met
 Aggregation is now a variable called `state` available in the script context, rather than
 being provided via the `params` object as `params._agg`.
 
-The old `params._agg` variable is still available as well.
+The old `params._agg` variable is still available as well. This can be disabled by setting
+the `es.aggregations.enableDeprecatedScriptedMetricAggParam` system property to `false`.

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. " +
+        "Set system property es.aggregations.enableDeprecatedScriptedMetricAggParam = false to disable this deprecated behavior.";
+
+    public static boolean deprecatedAggParamEnabled() {
+        boolean enabled = Boolean.parseBoolean(
+            System.getProperty("es.aggregations.enableDeprecatedScriptedMetricAggParam", "true"));
+
+        if (enabled) {
+            DEPRECATION_LOGGER.deprecatedAndMaybeLog("enableDeprecatedScriptedMetricAggParams", 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);

server/src/test/java/org/elasticsearch/search/aggregations/metrics/scripted/InternalScriptedMetricTests.java

@@ -24,6 +24,7 @@
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.script.MockScriptEngine;
 import org.elasticsearch.script.Script;
+import org.elasticsearch.script.ScriptedMetricAggContexts;
 import org.elasticsearch.script.ScriptEngine;
 import org.elasticsearch.script.ScriptModule;
 import org.elasticsearch.script.ScriptService;
@@ -129,6 +130,7 @@ protected void assertReduced(InternalScriptedMetric reduced, List<InternalScript
         assertEquals(firstAgg.getMetaData(), reduced.getMetaData());
         if (hasReduceScript) {
             assertEquals(inputs.size(), reduced.aggregation());
+            assertWarnings(ScriptedMetricAggContexts.AGG_PARAM_DEPRECATION_WARNING);
         } else {
             assertEquals(inputs.size(), ((List<Object>) reduced.aggregation()).size());
         }

server/src/test/java/org/elasticsearch/search/aggregations/metrics/scripted/ScriptedMetricAggregatorTests.java

@@ -32,6 +32,7 @@
 import org.elasticsearch.indices.breaker.CircuitBreakerService;
 import org.elasticsearch.script.MockScriptEngine;
 import org.elasticsearch.script.Script;
+import org.elasticsearch.script.ScriptedMetricAggContexts;
 import org.elasticsearch.script.ScriptEngine;
 import org.elasticsearch.script.ScriptModule;
 import org.elasticsearch.script.ScriptService;
@@ -169,6 +170,8 @@ public void testNoDocs() throws IOException {
                 assertEquals(0, ((HashMap<Object, String>) scriptedMetric.aggregation()).size());
             }
         }
+
+        assertWarnings(ScriptedMetricAggContexts.AGG_PARAM_DEPRECATION_WARNING);
     }
 
     /**
@@ -195,6 +198,8 @@ public void testScriptedMetricWithoutCombine() throws IOException {
                 assertEquals(numDocs, list.size());
             }
         }
+
+        assertWarnings(ScriptedMetricAggContexts.AGG_PARAM_DEPRECATION_WARNING);
     }
 
     /**
@@ -217,6 +222,8 @@ public void testScriptedMetricWithCombine() throws IOException {
                 assertEquals(numDocs, scriptedMetric.aggregation());
             }
         }
+
+        assertWarnings(ScriptedMetricAggContexts.AGG_PARAM_DEPRECATION_WARNING);
     }
 
     /**
@@ -240,6 +247,8 @@ public void testScriptedMetricWithCombineAccessesScores() throws IOException {
                 assertEquals((double) numDocs, scriptedMetric.aggregation());
             }
         }
+
+        assertWarnings(ScriptedMetricAggContexts.AGG_PARAM_DEPRECATION_WARNING);
     }
 
     public void testScriptParamsPassedThrough() throws IOException {
@@ -259,6 +268,8 @@ public void testScriptParamsPassedThrough() throws IOException {
                 assertEquals(306, scriptedMetric.aggregation());
             }
         }
+
+        assertWarnings(ScriptedMetricAggContexts.AGG_PARAM_DEPRECATION_WARNING);
     }
 
     public void testConflictingAggAndScriptParams() throws IOException {
@@ -282,6 +293,8 @@ public void testConflictingAggAndScriptParams() throws IOException {
                     ex.getMessage());
             }
         }
+
+        assertWarnings(ScriptedMetricAggContexts.AGG_PARAM_DEPRECATION_WARNING);
     }
 
     public void testSelfReferencingAggStateAfterInit() throws IOException {
@@ -299,6 +312,8 @@ public void testSelfReferencingAggStateAfterInit() throws IOException {
                 assertEquals("Iterable object is self-referencing itself (Scripted metric aggs init script)", ex.getMessage());
             }
         }
+
+        assertWarnings(ScriptedMetricAggContexts.AGG_PARAM_DEPRECATION_WARNING);
     }
 
     public void testSelfReferencingAggStateAfterMap() throws IOException {
@@ -319,6 +334,8 @@ public void testSelfReferencingAggStateAfterMap() throws IOException {
                 assertEquals("Iterable object is self-referencing itself (Scripted metric aggs map script)", ex.getMessage());
             }
         }
+
+        assertWarnings(ScriptedMetricAggContexts.AGG_PARAM_DEPRECATION_WARNING);
     }
 
     public void testSelfReferencingAggStateAfterCombine() throws IOException {
@@ -336,6 +353,8 @@ public void testSelfReferencingAggStateAfterCombine() throws IOException {
                 assertEquals("Iterable object is self-referencing itself (Scripted metric aggs combine script)", ex.getMessage());
             }
         }
+
+        assertWarnings(ScriptedMetricAggContexts.AGG_PARAM_DEPRECATION_WARNING);
     }
 
     /**