Watcher: Compile scripts on each invocation (elastic/elasticsearch#4239)

Transform and condition scripts were only compiled on its initial creation, so
when a new watch is created or when the master node loads all the watches. However
changing a script (like a stored one) did not lead to any changes in the in memory
watch store and thus the old script was executed again.

We do however have a mechanism in Elasticsearch's ScriptService that already does some
caching, and should reuse that one.

Closes elastic/elasticsearch#4237

Original commit: elastic/x-pack-elasticsearch@477548e237

elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/condition/ScriptCondition.java

@@ -32,21 +32,20 @@ public final class ScriptCondition extends Condition {
     private static final Result UNMET = new Result(null, TYPE, false);
 
     private final ScriptService scriptService;
-    private final CompiledScript compiledScript;
     private final Script script;
 
     public ScriptCondition(Script script) {
         super(TYPE);
         this.script = script;
         scriptService = null;
-        compiledScript = null;
     }
 
     ScriptCondition(Script script, ScriptService scriptService) {
         super(TYPE);
         this.scriptService = scriptService;
         this.script = script;
-        compiledScript = scriptService.compile(script, Watcher.SCRIPT_CONTEXT, Collections.emptyMap());
+        // try to compile so we catch syntax errors early
+        scriptService.compile(script, Watcher.SCRIPT_CONTEXT, Collections.emptyMap());
     }
 
     public Script getScript() {
@@ -73,6 +72,7 @@ public final class ScriptCondition extends Condition {
         if (script.getParams() != null && !script.getParams().isEmpty()) {
             parameters.putAll(script.getParams());
         }
+        CompiledScript compiledScript = scriptService.compile(script, Watcher.SCRIPT_CONTEXT, Collections.emptyMap());
         ExecutableScript executable = scriptService.executable(compiledScript, parameters);
         Object value = executable.run();
         if (value instanceof Boolean) {

elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/transform/script/ExecutableScriptTransform.java

@@ -28,13 +28,13 @@ import static org.elasticsearch.xpack.watcher.transform.script.ScriptTransform.T
 public class ExecutableScriptTransform extends ExecutableTransform<ScriptTransform, ScriptTransform.Result> {
 
     private final ScriptService scriptService;
-    private final CompiledScript compiledScript;
 
     public ExecutableScriptTransform(ScriptTransform transform, Logger logger, ScriptService scriptService) {
         super(transform, logger);
         this.scriptService = scriptService;
         Script script = transform.getScript();
-        compiledScript = scriptService.compile(script, Watcher.SCRIPT_CONTEXT, Collections.emptyMap());
+        // try to compile so we catch syntax errors early
+        scriptService.compile(script, Watcher.SCRIPT_CONTEXT, Collections.emptyMap());
     }
 
     @Override
@@ -47,7 +47,6 @@ public class ExecutableScriptTransform extends ExecutableTransform<ScriptTransfo
         }
     }
 
-
     ScriptTransform.Result doExecute(WatchExecutionContext ctx, Payload payload) throws IOException {
         Script script = transform.getScript();
         Map<String, Object> model = new HashMap<>();
@@ -55,6 +54,7 @@ public class ExecutableScriptTransform extends ExecutableTransform<ScriptTransfo
             model.putAll(script.getParams());
         }
         model.putAll(createCtxModel(ctx, payload));
+        CompiledScript compiledScript = scriptService.compile(script, Watcher.SCRIPT_CONTEXT, Collections.emptyMap());
         ExecutableScript executable = scriptService.executable(compiledScript, model);
         Object value = executable.run();
         if (value instanceof Map) {

elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/transform/script/ScriptTransformFactory.java

@@ -9,19 +9,16 @@ import org.elasticsearch.common.logging.Loggers;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.script.ScriptService;
-import org.elasticsearch.script.ScriptSettings;
 import org.elasticsearch.xpack.watcher.transform.TransformFactory;
 
 import java.io.IOException;
 
 public class ScriptTransformFactory extends TransformFactory<ScriptTransform, ScriptTransform.Result, ExecutableScriptTransform> {
 
-    private final Settings settings;
     private final ScriptService scriptService;
 
     public ScriptTransformFactory(Settings settings, ScriptService scriptService) {
         super(Loggers.getLogger(ExecutableScriptTransform.class, settings));
-        this.settings = settings;
         this.scriptService = scriptService;
     }
 

elasticsearch/src/test/java/org/elasticsearch/xpack/watcher/transform/script/ScriptTransformTests.java

@@ -25,7 +25,6 @@ import org.elasticsearch.xpack.watcher.support.Variables;
 import org.elasticsearch.xpack.watcher.transform.Transform;
 import org.elasticsearch.xpack.watcher.watch.Payload;
 import org.junit.After;
-import org.junit.Before;
 
 import java.util.Arrays;
 import java.util.Collections;
@@ -50,12 +49,8 @@ import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
 public class ScriptTransformTests extends ESTestCase {
-    ThreadPool tp = null;
 
-    @Before
+    private final ThreadPool tp = new TestThreadPool(ThreadPool.Names.SAME);
-    public void init() {
-        tp = new TestThreadPool(ThreadPool.Names.SAME);
-    }
 
     @After
     public void cleanup() {

qa/smoke-test-watcher-with-painless/src/test/resources/rest-api-spec/test/watcher_painless/50_update_scripts.yaml

@@ -0,0 +1,136 @@
+# When a script is specified in a watch, updates should be taken into account
+# See https://github.com/elastic/x-plugins/issues/4237
+---
+"Test transform scripts are updated on execution":
+  - do:
+    cluster.health:
+      wait_for_status: yellow
+
+  - do:
+    put_script:
+      id: transform-script
+      lang: painless
+      body: >
+        {
+          "script":"return [ 'email': 'foo@bar.org' ]"
+        }
+
+  - do:
+      xpack.watcher.put_watch:
+        id: "my_watch"
+        body:  >
+          {
+            "trigger": {
+              "schedule": {
+                "interval": "1h"
+              }
+            },
+            "input": {
+              "simple": {}
+            },
+            "transform": {
+              "script": {
+                "stored": "transform-script"
+              }
+            },
+            "actions": {
+              "my_log": {
+                "logging": {
+                  "text": "{{ctx}}"
+                }
+              }
+            }
+          }
+
+  - match: { _id: "my_watch" }
+
+  - do:
+      xpack.watcher.execute_watch:
+        id: "my_watch"
+
+  - match: { "watch_record.watch_id": "my_watch" }
+  - match: { "watch_record.result.transform.payload.email": "foo@bar.org" }
+
+  - do:
+    put_script:
+      id: transform-script
+      lang: painless
+      body: >
+        {
+          "script":"return [ 'email': 'foo@example.org' ]"
+        }
+
+  - do:
+      xpack.watcher.execute_watch:
+        id: "my_watch"
+
+  - match: { "watch_record.watch_id": "my_watch" }
+  - match: { "watch_record.result.transform.payload.email": "foo@example.org" }
+
+---
+"Test condition scripts are updated on execution":
+  - do:
+    cluster.health:
+      wait_for_status: yellow
+
+  - do:
+    put_script:
+      id: condition-script
+      lang: painless
+      body: >
+        {
+          "script":"return false"
+        }
+
+  - do:
+      xpack.watcher.put_watch:
+        id: "my_watch"
+        body:  >
+          {
+            "trigger": {
+              "schedule": {
+                "interval": "1h"
+              }
+            },
+            "input": {
+              "simple": {}
+            },
+            "condition": {
+              "script": {
+                "stored": "condition-script"
+              }
+            },
+            "actions": {
+              "my_log": {
+                "logging": {
+                  "text": "{{ctx}}"
+                }
+              }
+            }
+          }
+
+  - match: { _id: "my_watch" }
+
+  - do:
+      xpack.watcher.execute_watch:
+        id: "my_watch"
+
+  - match: { "watch_record.watch_id": "my_watch" }
+  - match: { "watch_record.result.condition.met": false }
+
+  - do:
+    put_script:
+      id: condition-script
+      lang: painless
+      body: >
+        {
+          "script":"return true"
+        }
+
+  - do:
+      xpack.watcher.execute_watch:
+        id: "my_watch"
+
+  - match: { "watch_record.watch_id": "my_watch" }
+  - match: { "watch_record.result.condition.met": true }
+