Disable Watcher script optimization for stored scripts (#53497)

The watcher TextTemplateEngine uses a fast path mechanism where it checks for the existence of `{{` to decide if a mustache script required compilation. This does not work for stored script, as the field that is checked contains the id of the script, which means, the name of the script is returned as its value. This commit checks for the script type and does not involve this fast path check if a stored script is used. Closes #40212
elastic · Mar 16, 2020 · c68c71f · c68c71f
1 parent d0b8535
commit c68c71f
Show file tree

Hide file tree

Showing 4 changed files with 40 additions and 8 deletions.
diff --git a/...lugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/text/TextTemplate.java b/...lugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/text/TextTemplate.java
@@ -29,12 +29,12 @@ public class TextTemplate implements ToXContent {
 
     private final Script script;
     private final String inlineTemplate;
-    private final boolean isUsingMustache;
+    private final boolean mayRequireCompilation;
 
     public TextTemplate(String template) {
         this.script = null;
         this.inlineTemplate = template;
-        this.isUsingMustache = template.contains("{{");
+        this.mayRequireCompilation = template.contains("{{");
     }
 
     public TextTemplate(String template, @Nullable XContentType contentType, ScriptType type,
@@ -50,14 +50,14 @@ public TextTemplate(String template, @Nullable XContentType contentType, ScriptT
             params = new HashMap<>();
         }
         this.script = new Script(type, type == ScriptType.STORED ? null : Script.DEFAULT_TEMPLATE_LANG, template, options, params);
-        this.isUsingMustache = template.contains("{{");
         this.inlineTemplate = null;
+        this.mayRequireCompilation = script.getType() == ScriptType.STORED || script.getIdOrCode().contains("{{");
     }
 
     public TextTemplate(Script script) {
         this.script = script;
         this.inlineTemplate = null;
-        this.isUsingMustache = script.getIdOrCode().contains("{{");
+        this.mayRequireCompilation = script.getType() == ScriptType.STORED || script.getIdOrCode().contains("{{");
     }
 
     public Script getScript() {
@@ -68,8 +68,14 @@ public String getTemplate() {
         return script != null ? script.getIdOrCode() : inlineTemplate;
     }
 
-    public boolean isUsingMustache() {
-        return isUsingMustache;
+    /**
+     * Check if compilation may be required.
+     * If a stored script is used, we cannot tell at this stage, so we always assume
+     * that stored scripts require compilation.
+     * If an inline script is used, we checked for the mustache opening brackets
+     */
+    public boolean mayRequireCompilation() {
+        return mayRequireCompilation;
     }
 
     public XContentType getContentType() {

diff --git a/...watcher/src/main/java/org/elasticsearch/xpack/watcher/common/text/TextTemplateEngine.java b/...watcher/src/main/java/org/elasticsearch/xpack/watcher/common/text/TextTemplateEngine.java
@@ -32,7 +32,7 @@ public String render(TextTemplate textTemplate, Map<String, Object> model) {
         String mediaType = compileParams(detectContentType(template));
         template = trimContentType(textTemplate);
 
-        if (textTemplate.isUsingMustache() == false) {
+        if (textTemplate.mayRequireCompilation() == false) {
             return template;
         }
 

diff --git a/...tcher/src/main/java/org/elasticsearch/xpack/watcher/notification/email/EmailTemplate.java b/...tcher/src/main/java/org/elasticsearch/xpack/watcher/notification/email/EmailTemplate.java
@@ -455,7 +455,7 @@ public boolean handle(String fieldName, XContentParser parser) throws IOExceptio
         static void validateEmailAddresses(TextTemplate ... emails) {
             for (TextTemplate emailTemplate : emails) {
                 // no mustache, do validation
-                if (emailTemplate.isUsingMustache() == false) {
+                if (emailTemplate.mayRequireCompilation() == false) {
                     String email = emailTemplate.getTemplate();
                     try {
                         for (Email.Address address : Email.AddressList.parse(email)) {

diff --git a/.../watcher/src/test/java/org/elasticsearch/xpack/watcher/common/text/TextTemplateTests.java b/.../watcher/src/test/java/org/elasticsearch/xpack/watcher/common/text/TextTemplateTests.java
@@ -225,6 +225,32 @@ public void testParserInvalidMissingText() throws Exception {
         assertEquals("[1:2] [script] unknown field [type]", ex.getMessage());
     }
 
+    public void testMustacheTemplateRequiresCompilation() {
+        final TextTemplate inlineTemplateRequiresCompilation = createTextTemplate(ScriptType.INLINE, "{{foo.bar}}");
+        assertThat(inlineTemplateRequiresCompilation.mayRequireCompilation(), is(true));
+
+        final TextTemplate inlineTemplateNoRequiresCompilation = createTextTemplate(ScriptType.INLINE, "script without mustache");
+        assertThat(inlineTemplateNoRequiresCompilation.mayRequireCompilation(), is(false));
+
+        final TextTemplate storedScriptTemplate = createTextTemplate(ScriptType.STORED, "stored_script_id");
+        assertThat(storedScriptTemplate.mayRequireCompilation(), is(true));
+    }
+
+    private TextTemplate createTextTemplate(ScriptType type, String idOrCode) {
+        final TextTemplate template;
+        if (randomBoolean()) {
+            if (type == ScriptType.STORED) {
+                template = new TextTemplate(new Script(type, null, idOrCode, null, Collections.emptyMap()));
+            } else {
+                template = new TextTemplate(new Script(type, lang, idOrCode, Collections.emptyMap(), Collections.emptyMap()));
+            }
+        } else {
+            template = new TextTemplate(idOrCode, null, type,  null);
+        }
+
+        return template;
+    }
+
     public void testNullObject() throws Exception {
         assertThat(engine.render(null ,new HashMap<>()), is(nullValue()));
     }