From 7571ca437a37859b721cc6eca1d2d16d55dfeff4 Mon Sep 17 00:00:00 2001 From: Alexander Reelsen Date: Mon, 16 Mar 2020 18:07:14 +0100 Subject: [PATCH] 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 --- .../watcher/common/text/TextTemplate.java | 18 ++++++++----- .../common/text/TextTemplateEngine.java | 2 +- .../notification/email/EmailTemplate.java | 2 +- .../common/text/TextTemplateTests.java | 26 +++++++++++++++++++ 4 files changed, 40 insertions(+), 8 deletions(-) diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/text/TextTemplate.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/text/TextTemplate.java index 224dcc7140d..a261e6d01af 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/text/TextTemplate.java +++ b/x-pack/plugin/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 class TextTemplate implements ToXContent { 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 class TextTemplate implements ToXContent { 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/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/text/TextTemplateEngine.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/text/TextTemplateEngine.java index 2f2d3d7b9f3..fed102f1d5c 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/text/TextTemplateEngine.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/text/TextTemplateEngine.java @@ -32,7 +32,7 @@ public class TextTemplateEngine { String mediaType = compileParams(detectContentType(template)); template = trimContentType(textTemplate); - if (textTemplate.isUsingMustache() == false) { + if (textTemplate.mayRequireCompilation() == false) { return template; } diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/email/EmailTemplate.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/email/EmailTemplate.java index dc48dd2b9f2..2735ad9fc93 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/email/EmailTemplate.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/email/EmailTemplate.java @@ -455,7 +455,7 @@ public class EmailTemplate implements ToXContentObject { 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/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/common/text/TextTemplateTests.java b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/common/text/TextTemplateTests.java index 2ebaccc6180..a563695732a 100644 --- a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/common/text/TextTemplateTests.java +++ b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/common/text/TextTemplateTests.java @@ -225,6 +225,32 @@ public class TextTemplateTests extends ESTestCase { 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())); }