Scripting: Replace Update Context (#32096) (#32752)

original-brownbear · web-flow · commit 6f3a3e19327b · 2018-08-12T12:32:36.000+02:00
* SCRIPTING: Move Update Scripts to their own context
* Added system property for backwards compatibility of change to `ctx.params`
diff --git a/buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy b/buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy
@@ -820,9 +820,12 @@ class BuildPlugin implements Plugin<Project> {
                 }
             }
 
-            // TODO: remove this once joda time is removed from scriptin in 7.0
+            // TODO: remove this once joda time is removed from scripting in 7.0
             systemProperty 'es.scripting.use_java_time', 'true'
 
+            // TODO: remove this once ctx isn't added to update script params in 7.0
+            systemProperty 'es.scripting.update.ctx_in_params', 'false'
+
             // Set the system keystore/truststore password if we're running tests in a FIPS-140 JVM
             if (project.inFipsJvm) {
                 systemProperty 'javax.net.ssl.trustStorePassword', 'password'
diff --git a/docs/build.gradle b/docs/build.gradle
@@ -40,6 +40,7 @@ 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'
 }
 
 // remove when https://github.com/elastic/elasticsearch/issues/31305 is fixed
diff --git a/modules/lang-painless/build.gradle b/modules/lang-painless/build.gradle
@@ -25,6 +25,7 @@ esplugin {
 integTestCluster {
   module project.project(':modules:mapper-extras')
   systemProperty 'es.scripting.use_java_time', 'true'
+  systemProperty 'es.scripting.update.ctx_in_params', 'false'
 }
 
 dependencies {
diff --git a/modules/lang-painless/src/test/resources/rest-api-spec/test/painless/15_update.yml b/modules/lang-painless/src/test/resources/rest-api-spec/test/painless/15_update.yml
@@ -132,7 +132,7 @@
           body:
             script:
               lang:   painless
-              source: "for (def key : params.keySet()) { ctx._source[key] = params[key]}"
+              source: "ctx._source.ctx = ctx"
               params: { bar: 'xxx' }
 
   - match: { error.root_cause.0.type: "remote_transport_exception" }
diff --git a/modules/reindex/src/main/java/org/elasticsearch/index/reindex/AbstractAsyncBulkByScrollAction.java b/modules/reindex/src/main/java/org/elasticsearch/index/reindex/AbstractAsyncBulkByScrollAction.java
@@ -49,9 +49,9 @@
 import org.elasticsearch.index.mapper.TypeFieldMapper;
 import org.elasticsearch.index.mapper.VersionFieldMapper;
 import org.elasticsearch.index.reindex.ScrollableHitSource.SearchFailure;
-import org.elasticsearch.script.ExecutableScript;
 import org.elasticsearch.script.Script;
 import org.elasticsearch.script.ScriptService;
+import org.elasticsearch.script.UpdateScript;
 import org.elasticsearch.search.sort.SortBuilder;
 import org.elasticsearch.threadpool.ThreadPool;
 
@@ -772,7 +772,7 @@ public abstract static class ScriptApplier implements BiFunction<RequestWrapper<
         private final Script script;
         private final Map<String, Object> params;
 
-        private ExecutableScript executable;
+        private UpdateScript executable;
         private Map<String, Object> context;
 
         public ScriptApplier(WorkerBulkByScrollTaskState taskWorker,
@@ -792,7 +792,7 @@ public RequestWrapper<?> apply(RequestWrapper<?> request, ScrollableHitSource.Hi
                 return request;
             }
             if (executable == null) {
-                ExecutableScript.Factory factory = scriptService.compile(script, ExecutableScript.UPDATE_CONTEXT);
+                UpdateScript.Factory factory = scriptService.compile(script, UpdateScript.CONTEXT);
                 executable = factory.newInstance(params);
             }
             if (context == null) {
@@ -815,8 +815,7 @@ public RequestWrapper<?> apply(RequestWrapper<?> request, ScrollableHitSource.Hi
             OpType oldOpType = OpType.INDEX;
             context.put("op", oldOpType.toString());
 
-            executable.setNextVar("ctx", context);
-            executable.run();
+            executable.execute(context);
 
             String newOp = (String) context.remove("op");
             if (newOp == null) {
diff --git a/modules/reindex/src/test/java/org/elasticsearch/index/reindex/AbstractAsyncBulkByScrollActionScriptTestCase.java b/modules/reindex/src/test/java/org/elasticsearch/index/reindex/AbstractAsyncBulkByScrollActionScriptTestCase.java
@@ -26,8 +26,10 @@
 import org.elasticsearch.action.index.IndexRequest;
 import org.elasticsearch.script.ExecutableScript;
 import org.elasticsearch.script.ScriptService;
+import org.elasticsearch.script.UpdateScript;
 import org.junit.Before;
 
+import java.util.Collections;
 import java.util.Map;
 import java.util.function.Consumer;
 
@@ -54,10 +56,16 @@ public void setupScriptService() {
     protected <T extends ActionRequest> T applyScript(Consumer<Map<String, Object>> scriptBody) {
         IndexRequest index = new IndexRequest("index", "type", "1").source(singletonMap("foo", "bar"));
         ScrollableHitSource.Hit doc = new ScrollableHitSource.BasicHit("test", "type", "id", 0);
-        ExecutableScript executableScript = new SimpleExecutableScript(scriptBody);
-        ExecutableScript.Factory factory = params -> executableScript;
-        when(scriptService.compile(any(), eq(ExecutableScript.CONTEXT))).thenReturn(factory);
-        when(scriptService.compile(any(), eq(ExecutableScript.UPDATE_CONTEXT))).thenReturn(factory);
+        UpdateScript updateScript = new UpdateScript(Collections.emptyMap()) {
+            @Override
+            public void execute(Map<String, Object> ctx) {
+                scriptBody.accept(ctx);
+            }
+        };
+        UpdateScript.Factory factory = params -> updateScript;
+        ExecutableScript simpleExecutableScript = new SimpleExecutableScript(scriptBody);
+        when(scriptService.compile(any(), eq(ExecutableScript.CONTEXT))).thenReturn(params -> simpleExecutableScript);
+        when(scriptService.compile(any(), eq(UpdateScript.CONTEXT))).thenReturn(factory);
         AbstractAsyncBulkByScrollAction<Request> action = action(scriptService, request().setScript(mockScript("")));
         RequestWrapper<?> result = action.buildScriptApplier().apply(AbstractAsyncBulkByScrollAction.wrap(index), doc);
         return (result != null) ? (T) result.self() : null;
diff --git a/server/src/main/java/org/elasticsearch/action/update/UpdateHelper.java b/server/src/main/java/org/elasticsearch/action/update/UpdateHelper.java
@@ -19,6 +19,10 @@
 
 package org.elasticsearch.action.update;
 
+import java.io.IOException;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.function.LongSupplier;
 import org.apache.logging.log4j.Logger;
 import org.elasticsearch.ElasticsearchException;
 import org.elasticsearch.action.DocWriteResponse;
@@ -44,21 +48,23 @@
 import org.elasticsearch.index.mapper.RoutingFieldMapper;
 import org.elasticsearch.index.shard.IndexShard;
 import org.elasticsearch.index.shard.ShardId;
-import org.elasticsearch.script.ExecutableScript;
 import org.elasticsearch.script.Script;
 import org.elasticsearch.script.ScriptService;
+import org.elasticsearch.script.UpdateScript;
 import org.elasticsearch.search.lookup.SourceLookup;
 
-import java.io.IOException;
 import java.util.ArrayList;
-import java.util.HashMap;
-import java.util.Map;
-import java.util.function.LongSupplier;
+import static org.elasticsearch.common.Booleans.parseBoolean;
 
 /**
  * Helper for translating an update request to an index, delete request or update response.
  */
 public class UpdateHelper extends AbstractComponent {
+
+    /** Whether scripts should add the ctx variable to the params map. */
+    private static final boolean CTX_IN_PARAMS =
+        parseBoolean(System.getProperty("es.scripting.update.ctx_in_params"), true);
+
     private final ScriptService scriptService;
 
     public UpdateHelper(Settings settings, ScriptService scriptService) {
@@ -297,10 +303,18 @@ Result prepareUpdateScriptRequest(ShardId shardId, UpdateRequest request, GetRes
     private Map<String, Object> executeScript(Script script, Map<String, Object> ctx) {
         try {
             if (scriptService != null) {
-                ExecutableScript.Factory factory = scriptService.compile(script, ExecutableScript.UPDATE_CONTEXT);
-                ExecutableScript executableScript = factory.newInstance(script.getParams());
-                executableScript.setNextVar(ContextFields.CTX, ctx);
-                executableScript.run();
+                UpdateScript.Factory factory = scriptService.compile(script, UpdateScript.CONTEXT);
+                final Map<String, Object> params;
+                if (CTX_IN_PARAMS) {
+                    params = new HashMap<>(script.getParams());
+                    params.put(ContextFields.CTX, ctx);
+                    deprecationLogger.deprecated("Using `ctx` via `params.ctx` is deprecated. " +
+                        "Use -Des.scripting.update.ctx_in_params=false to enforce non-deprecated usage.");
+                } else {
+                    params = script.getParams();
+                }
+                UpdateScript executableScript = factory.newInstance(params);
+                executableScript.execute(ctx);
             }
         } catch (Exception e) {
             throw new IllegalArgumentException("failed to execute script", e);
diff --git a/server/src/main/java/org/elasticsearch/script/ExecutableScript.java b/server/src/main/java/org/elasticsearch/script/ExecutableScript.java
@@ -46,7 +46,4 @@ interface Factory {
     }
 
     ScriptContext<Factory> CONTEXT = new ScriptContext<>("executable", Factory.class);
-
-    // TODO: remove these once each has its own script interface
-    ScriptContext<Factory> UPDATE_CONTEXT = new ScriptContext<>("update", Factory.class);
 }
diff --git a/server/src/main/java/org/elasticsearch/script/ScriptModule.java b/server/src/main/java/org/elasticsearch/script/ScriptModule.java
@@ -49,10 +49,10 @@ public class ScriptModule {
             SearchScript.SCRIPT_SORT_CONTEXT,
             SearchScript.TERMS_SET_QUERY_CONTEXT,
             ExecutableScript.CONTEXT,
+            UpdateScript.CONTEXT,
             BucketAggregationScript.CONTEXT,
             BucketAggregationSelectorScript.CONTEXT,
             SignificantTermsHeuristicScoreScript.CONTEXT,
-            ExecutableScript.UPDATE_CONTEXT,
             IngestScript.CONTEXT,
             FilterScript.CONTEXT,
             SimilarityScript.CONTEXT,
diff --git a/server/src/main/java/org/elasticsearch/script/ScriptService.java b/server/src/main/java/org/elasticsearch/script/ScriptService.java
@@ -285,7 +285,7 @@ public <FactoryType> FactoryType compile(Script script, ScriptContext<FactoryTyp
         // TODO: fix this through some API or something, that's wrong
         // special exception to prevent expressions from compiling as update or mapping scripts
         boolean expression = "expression".equals(lang);
-        boolean notSupported = context.name.equals(ExecutableScript.UPDATE_CONTEXT.name);
+        boolean notSupported = context.name.equals(UpdateScript.CONTEXT.name);
         if (expression && notSupported) {
             throw new UnsupportedOperationException("scripts of type [" + script.getType() + "]," +
                 " operation [" + context.name + "] and lang [" + lang + "] are not supported");
diff --git a/server/src/main/java/org/elasticsearch/script/UpdateScript.java b/server/src/main/java/org/elasticsearch/script/UpdateScript.java
@@ -0,0 +1,52 @@
+
+/*
+ * Licensed to Elasticsearch under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.elasticsearch.script;
+
+import java.util.Map;
+
+/**
+ * An update script.
+ */
+public abstract class UpdateScript {
+
+    public static final String[] PARAMETERS = { "ctx" };
+
+    /** The context used to compile {@link UpdateScript} factories. */
+    public static final ScriptContext<Factory> CONTEXT = new ScriptContext<>("update", Factory.class);
+
+    /** The generic runtime parameters for the script. */
+    private final Map<String, Object> params;
+
+    public UpdateScript(Map<String, Object> params) {
+        this.params = params;
+    }
+
+    /** Return the parameters for this script. */
+    public Map<String, Object> getParams() {
+        return params;
+    }
+
+    public abstract void execute(Map<String, Object> ctx);
+
+    public interface Factory {
+        UpdateScript newInstance(Map<String, Object> params);
+    }
+}
diff --git a/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java b/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java
@@ -167,7 +167,7 @@ public void testAllowAllScriptContextSettings() throws IOException {
 
         assertCompileAccepted("painless", "script", ScriptType.INLINE, SearchScript.CONTEXT);
         assertCompileAccepted("painless", "script", ScriptType.INLINE, SearchScript.AGGS_CONTEXT);
-        assertCompileAccepted("painless", "script", ScriptType.INLINE, ExecutableScript.UPDATE_CONTEXT);
+        assertCompileAccepted("painless", "script", ScriptType.INLINE, UpdateScript.CONTEXT);
         assertCompileAccepted("painless", "script", ScriptType.INLINE, IngestScript.CONTEXT);
     }
 
@@ -187,7 +187,7 @@ public void testAllowSomeScriptContextSettings() throws IOException {
 
         assertCompileAccepted("painless", "script", ScriptType.INLINE, SearchScript.CONTEXT);
         assertCompileAccepted("painless", "script", ScriptType.INLINE, SearchScript.AGGS_CONTEXT);
-        assertCompileRejected("painless", "script", ScriptType.INLINE, ExecutableScript.UPDATE_CONTEXT);
+        assertCompileRejected("painless", "script", ScriptType.INLINE, UpdateScript.CONTEXT);
     }
 
     public void testAllowNoScriptTypeSettings() throws IOException {
diff --git a/server/src/test/java/org/elasticsearch/update/UpdateIT.java b/server/src/test/java/org/elasticsearch/update/UpdateIT.java
@@ -94,6 +94,7 @@ protected Map<String, Function<Map<String, Object>, Object>> pluginScripts() {
                 }
 
                 Map<String, Object> source = (Map<String, Object>) ctx.get("_source");
+                params.remove("ctx");
                 source.putAll(params);
 
                 return ctx;
diff --git a/test/framework/src/main/java/org/elasticsearch/script/MockScriptEngine.java b/test/framework/src/main/java/org/elasticsearch/script/MockScriptEngine.java
@@ -96,6 +96,18 @@ public void execute(Map<String, Object> ctx) {
                 }
             };
             return context.factoryClazz.cast(factory);
+        } else if (context.instanceClazz.equals(UpdateScript.class)) {
+            UpdateScript.Factory factory = parameters -> new UpdateScript(parameters) {
+                @Override
+                public void execute(Map<String, Object> ctx) {
+                    final Map<String, Object> vars = new HashMap<>();
+                    vars.put("ctx", ctx);
+                    vars.put("params", parameters);
+                    vars.putAll(parameters);
+                    script.apply(vars);
+                }
+            };
+            return context.factoryClazz.cast(factory);
         } else if (context.instanceClazz.equals(BucketAggregationScript.class)) {
             BucketAggregationScript.Factory factory = parameters -> new BucketAggregationScript(parameters) {
                 @Override

Original file line number	Diff line number	Diff line change
`@@ -820,9 +820,12 @@ class BuildPlugin implements Plugin<Project> {`
`820`	`820`	`}`
`821`	`821`	`}`
`822`	`822`
`823`		`- // TODO: remove this once joda time is removed from scriptin in 7.0`
	`823`	`+ // TODO: remove this once joda time is removed from scripting in 7.0`
`824`	`824`	`systemProperty 'es.scripting.use_java_time', 'true'`
`825`	`825`
	`826`	`+ // TODO: remove this once ctx isn't added to update script params in 7.0`
	`827`	`+ systemProperty 'es.scripting.update.ctx_in_params', 'false'`
	`828`	`+`
`826`	`829`	`// Set the system keystore/truststore password if we're running tests in a FIPS-140 JVM`
`827`	`830`	`if (project.inFipsJvm) {`
`828`	`831`	`systemProperty 'javax.net.ssl.trustStorePassword', 'password'`
Original file line number	Diff line number	Diff line change
`@@ -40,6 +40,7 @@ integTestCluster {`
`40`	`40`
`41`	`41`	`// TODO: remove this for 7.0, this exists to allow the doc examples in 6.x to continue using the defaults`
`42`	`42`	`systemProperty 'es.scripting.use_java_time', 'false'`
	`43`	`+ systemProperty 'es.scripting.update.ctx_in_params', 'false'`
`43`	`44`	`}`
`44`	`45`
`45`	`46`	`// remove when https://github.com/elastic/elasticsearch/issues/31305 is fixed`
Original file line number	Diff line number	Diff line change
`@@ -25,6 +25,7 @@ esplugin {`
`25`	`25`	`integTestCluster {`
`26`	`26`	`module project.project(':modules:mapper-extras')`
`27`	`27`	`systemProperty 'es.scripting.use_java_time', 'true'`
	`28`	`+ systemProperty 'es.scripting.update.ctx_in_params', 'false'`
`28`	`29`	`}`
`29`	`30`
`30`	`31`	`dependencies {`
Original file line number	Diff line number	Diff line change
`@@ -46,7 +46,4 @@ interface Factory {`
`46`	`46`	`}`
`47`	`47`
`48`	`48`	`ScriptContext<Factory> CONTEXT = new ScriptContext<>("executable", Factory.class);`
`49`		`-`
`50`		`- // TODO: remove these once each has its own script interface`
`51`		`- ScriptContext<Factory> UPDATE_CONTEXT = new ScriptContext<>("update", Factory.class);`
`52`	`49`	`}`