From 09779359893b1ada2e343f4dffd091bc9b9ae62e Mon Sep 17 00:00:00 2001 From: Jack Conradson Date: Thu, 10 Nov 2016 09:58:37 -0800 Subject: [PATCH] Clean up of Script. Closes elastic/elasticsearch#3982 Original commit: elastic/x-pack-elasticsearch@96c94ae8d5afc5e3120cb0b63e8054b182d8dde5 --- .../xpack/common/text/TextTemplate.java | 28 +++++++++++++++---- .../xpack/common/text/TextTemplateEngine.java | 6 +++- .../SecurityIndexSearcherWrapper.java | 4 +-- .../search/WatcherSearchTemplateRequest.java | 9 +++--- .../search/WatcherSearchTemplateService.java | 4 +-- .../xcontent/WatcherXContentParser.java | 10 +++++++ .../support/xcontent/XContentSource.java | 2 +- .../script/LatchScriptEngine.java | 2 +- .../xpack/common/text/TextTemplateTests.java | 21 +++++++------- .../xpack/graph/test/GraphTests.java | 4 +-- ...SecurityIndexSearcherWrapperUnitTests.java | 11 ++++---- .../condition/ScriptConditionTests.java | 11 ++++---- .../execution/ManualExecutionTests.java | 4 +-- .../history/HistoryActionConditionTests.java | 2 +- .../watcher/support/WatcherUtilsTests.java | 10 +++---- .../WatcherSearchTemplateRequestTests.java | 2 +- .../test/integration/BasicWatcherTests.java | 4 ++- .../integration/SearchTransformTests.java | 3 +- .../transform/TransformIntegrationTests.java | 5 ++-- .../script/ScriptTransformTests.java | 8 +++--- 20 files changed, 93 insertions(+), 57 deletions(-) diff --git a/elasticsearch/src/main/java/org/elasticsearch/xpack/common/text/TextTemplate.java b/elasticsearch/src/main/java/org/elasticsearch/xpack/common/text/TextTemplate.java index 4657916e838..0230685f3a2 100644 --- a/elasticsearch/src/main/java/org/elasticsearch/xpack/common/text/TextTemplate.java +++ b/elasticsearch/src/main/java/org/elasticsearch/xpack/common/text/TextTemplate.java @@ -15,6 +15,7 @@ import org.elasticsearch.script.Script; import org.elasticsearch.script.ScriptType; import java.io.IOException; +import java.util.HashMap; import java.util.Map; import java.util.Objects; @@ -27,8 +28,6 @@ import java.util.Objects; */ public class TextTemplate implements ToXContent { - public static final String DEFAULT_TEMPLATE_LANG = "mustache"; - private final Script script; private final String inlineTemplate; @@ -39,7 +38,14 @@ public class TextTemplate implements ToXContent { public TextTemplate(String template, @Nullable XContentType contentType, ScriptType type, @Nullable Map params) { - this.script = new Script(template, type, DEFAULT_TEMPLATE_LANG, params, contentType); + Map options = new HashMap<>(); + if (contentType != null) { + options.put(Script.CONTENT_TYPE_OPTION, contentType.mediaType()); + } + if (params == null) { + params = new HashMap<>(); + } + this.script = new Script(type, Script.DEFAULT_TEMPLATE_LANG, template, options, params); this.inlineTemplate = null; } @@ -53,11 +59,21 @@ public class TextTemplate implements ToXContent { } public String getTemplate() { - return script != null ? script.getScript() : inlineTemplate; + return script != null ? script.getIdOrCode() : inlineTemplate; } public XContentType getContentType() { - return script != null ? script.getContentType() : null; + if (script == null) { + return null; + } + + String mediaType = script.getOptions().get(Script.CONTENT_TYPE_OPTION); + + if (mediaType == null) { + return null; + } + + return XContentType.fromMediaTypeOrFormat(mediaType); } public ScriptType getType() { @@ -97,7 +113,7 @@ public class TextTemplate implements ToXContent { if (parser.currentToken() == XContentParser.Token.VALUE_STRING) { return new TextTemplate(parser.text()); } else { - return new TextTemplate(Script.parse(parser, ParseFieldMatcher.STRICT, DEFAULT_TEMPLATE_LANG)); + return new TextTemplate(Script.parse(parser, ParseFieldMatcher.STRICT, Script.DEFAULT_TEMPLATE_LANG)); } } } diff --git a/elasticsearch/src/main/java/org/elasticsearch/xpack/common/text/TextTemplateEngine.java b/elasticsearch/src/main/java/org/elasticsearch/xpack/common/text/TextTemplateEngine.java index fa60aa00759..96c18ef757b 100644 --- a/elasticsearch/src/main/java/org/elasticsearch/xpack/common/text/TextTemplateEngine.java +++ b/elasticsearch/src/main/java/org/elasticsearch/xpack/common/text/TextTemplateEngine.java @@ -44,7 +44,11 @@ public class TextTemplateEngine extends AbstractComponent { } mergedModel.putAll(model); - Script script = new Script(template, textTemplate.getType(), "mustache", mergedModel, textTemplate.getContentType()); + Map options = new HashMap<>(); + if (textTemplate.getContentType() != null) { + options.put(Script.CONTENT_TYPE_OPTION, textTemplate.getContentType().mediaType()); + } + Script script = new Script(textTemplate.getType(), "mustache", template, options, mergedModel); CompiledScript compiledScript = service.compile(script, Watcher.SCRIPT_CONTEXT, compileParams); ExecutableScript executable = service.executable(compiledScript, model); Object result = executable.run(); diff --git a/elasticsearch/src/main/java/org/elasticsearch/xpack/security/authz/accesscontrol/SecurityIndexSearcherWrapper.java b/elasticsearch/src/main/java/org/elasticsearch/xpack/security/authz/accesscontrol/SecurityIndexSearcherWrapper.java index 716403eebf7..216cc7e6fc9 100644 --- a/elasticsearch/src/main/java/org/elasticsearch/xpack/security/authz/accesscontrol/SecurityIndexSearcherWrapper.java +++ b/elasticsearch/src/main/java/org/elasticsearch/xpack/security/authz/accesscontrol/SecurityIndexSearcherWrapper.java @@ -317,8 +317,8 @@ public class SecurityIndexSearcherWrapper extends IndexSearcherWrapper { userModel.put("metadata", Collections.unmodifiableMap(user.metadata())); params.put("_user", userModel); // Always enforce mustache script lang: - script = new Script(script.getScript(), script.getType(), "mustache", params, script.getContentType()); - ExecutableScript executable = scriptService.executable(script, ScriptContext.Standard.SEARCH, Collections.emptyMap()); + script = new Script(script.getType(), "mustache", script.getIdOrCode(), script.getOptions(), params); + ExecutableScript executable = scriptService.executable(script, ScriptContext.Standard.SEARCH); return (BytesReference) executable.run(); } else { return querySource; diff --git a/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/support/search/WatcherSearchTemplateRequest.java b/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/support/search/WatcherSearchTemplateRequest.java index 753d8cf76f4..4ba3df4b7d6 100644 --- a/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/support/search/WatcherSearchTemplateRequest.java +++ b/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/support/search/WatcherSearchTemplateRequest.java @@ -28,6 +28,7 @@ import org.elasticsearch.xpack.common.text.TextTemplate; import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.List; import java.util.Locale; import java.util.Objects; @@ -54,13 +55,13 @@ public class WatcherSearchTemplateRequest implements ToXContent { this.indicesOptions = indicesOptions; // Here we convert a watch search request body into an inline search template, // this way if any Watcher related context variables are used, they will get resolved. - this.template = new Script(searchSource.utf8ToString(), ScriptType.INLINE, TextTemplate.DEFAULT_TEMPLATE_LANG, null); + this.template = new Script(ScriptType.INLINE, Script.DEFAULT_TEMPLATE_LANG, searchSource.utf8ToString(), Collections.emptyMap()); this.searchSource = null; } public WatcherSearchTemplateRequest(String[] indices, String[] types, SearchType searchType, IndicesOptions indicesOptions, Script template) { - assert template == null || TextTemplate.DEFAULT_TEMPLATE_LANG.equals(template.getLang()); + assert template == null || Script.DEFAULT_TEMPLATE_LANG.equals(template.getLang()); this.indices = indices; this.types = types; this.searchType = searchType; @@ -118,7 +119,7 @@ public class WatcherSearchTemplateRequest implements ToXContent { if (template != null) { return template; } else { - return new Script(searchSource.utf8ToString(), ScriptType.INLINE, TextTemplate.DEFAULT_TEMPLATE_LANG, null); + return new Script(ScriptType.INLINE, Script.DEFAULT_TEMPLATE_LANG, searchSource.utf8ToString(), Collections.emptyMap()); } } @@ -271,7 +272,7 @@ public class WatcherSearchTemplateRequest implements ToXContent { indicesOptions = IndicesOptions.fromOptions(ignoreUnavailable, allowNoIndices, expandOpen, expandClosed, DEFAULT_INDICES_OPTIONS); } else if (ParseFieldMatcher.STRICT.match(currentFieldName, TEMPLATE_FIELD)) { - template = Script.parse(parser, ParseFieldMatcher.STRICT, TextTemplate.DEFAULT_TEMPLATE_LANG); + template = Script.parse(parser, ParseFieldMatcher.STRICT, Script.DEFAULT_TEMPLATE_LANG); } else { throw new ElasticsearchParseException("could not read search request. unexpected object field [" + currentFieldName + "]"); diff --git a/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/support/search/WatcherSearchTemplateService.java b/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/support/search/WatcherSearchTemplateService.java index ecb7b682bc6..559f18d8a11 100644 --- a/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/support/search/WatcherSearchTemplateService.java +++ b/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/support/search/WatcherSearchTemplateService.java @@ -55,8 +55,8 @@ public class WatcherSearchTemplateService extends AbstractComponent { watcherContextParams.putAll(source.getParams()); } // Templates are always of lang mustache: - Script template = new Script(source.getScript(), source.getType(), "mustache", watcherContextParams, - source.getContentType()); + Script template = new Script(source.getType(), "mustache", source.getIdOrCode(), source.getOptions(), watcherContextParams + ); CompiledScript compiledScript = scriptService.compile(template, Watcher.SCRIPT_CONTEXT, Collections.emptyMap()); return (BytesReference) scriptService.executable(compiledScript, template.getParams()).run(); } diff --git a/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/support/xcontent/WatcherXContentParser.java b/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/support/xcontent/WatcherXContentParser.java index b4818f00010..c93babb5bf3 100644 --- a/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/support/xcontent/WatcherXContentParser.java +++ b/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/support/xcontent/WatcherXContentParser.java @@ -109,6 +109,16 @@ public class WatcherXContentParser implements XContentParser { return parser.mapOrdered(); } + @Override + public Map mapStrings() throws IOException { + return parser.mapStrings(); + } + + @Override + public Map mapStringsOrdered() throws IOException { + return parser.mapStringsOrdered(); + } + @Override public List list() throws IOException { return parser.list(); diff --git a/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/support/xcontent/XContentSource.java b/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/support/xcontent/XContentSource.java index 63c35b26d5d..72c36a50cfe 100644 --- a/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/support/xcontent/XContentSource.java +++ b/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/support/xcontent/XContentSource.java @@ -118,7 +118,7 @@ public class XContentSource implements ToXContent { public static void writeTo(XContentSource source, StreamOutput out) throws IOException { out.writeBytesReference(source.bytes); - XContentType.writeTo(source.contentType, out); + source.contentType.writeTo(out); } private Object data() { diff --git a/elasticsearch/src/test/java/org/elasticsearch/script/LatchScriptEngine.java b/elasticsearch/src/test/java/org/elasticsearch/script/LatchScriptEngine.java index 714ae16ae14..f9262f81710 100644 --- a/elasticsearch/src/test/java/org/elasticsearch/script/LatchScriptEngine.java +++ b/elasticsearch/src/test/java/org/elasticsearch/script/LatchScriptEngine.java @@ -86,7 +86,7 @@ public class LatchScriptEngine implements ScriptEngineService { } public static Script latchScript() { - return new Script("", ScriptType.INLINE, NAME, Collections.emptyMap()); + return new Script(ScriptType.INLINE, NAME, "", Collections.emptyMap()); } @Override diff --git a/elasticsearch/src/test/java/org/elasticsearch/xpack/common/text/TextTemplateTests.java b/elasticsearch/src/test/java/org/elasticsearch/xpack/common/text/TextTemplateTests.java index b9350ee3284..7ae93368376 100644 --- a/elasticsearch/src/test/java/org/elasticsearch/xpack/common/text/TextTemplateTests.java +++ b/elasticsearch/src/test/java/org/elasticsearch/xpack/common/text/TextTemplateTests.java @@ -5,7 +5,7 @@ */ package org.elasticsearch.xpack.common.text; -import org.elasticsearch.ElasticsearchParseException; +import org.elasticsearch.common.ParsingException; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.xcontent.XContentBuilder; @@ -27,6 +27,7 @@ import java.util.Map; import static java.util.Collections.singletonMap; import static java.util.Collections.unmodifiableMap; import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.notNullValue; @@ -58,7 +59,7 @@ public class TextTemplateTests extends ESTestCase { ScriptType type = randomFrom(ScriptType.values()); CompiledScript compiledScript = mock(CompiledScript.class); - when(service.compile(new Script(templateText, type, lang, merged), Watcher.SCRIPT_CONTEXT, + when(service.compile(new Script(type, lang, templateText, merged), Watcher.SCRIPT_CONTEXT, Collections.singletonMap("content_type", "text/plain"))).thenReturn(compiledScript); when(service.executable(compiledScript, model)).thenReturn(script); when(script.run()).thenReturn("rendered_text"); @@ -74,7 +75,7 @@ public class TextTemplateTests extends ESTestCase { ScriptType scriptType = randomFrom(ScriptType.values()); CompiledScript compiledScript = mock(CompiledScript.class); - when(service.compile(new Script(templateText, scriptType, lang, model), Watcher.SCRIPT_CONTEXT, + when(service.compile(new Script(scriptType, lang, templateText, model), Watcher.SCRIPT_CONTEXT, Collections.singletonMap("content_type", "text/plain"))).thenReturn(compiledScript); when(service.executable(compiledScript, model)).thenReturn(script); when(script.run()).thenReturn("rendered_text"); @@ -88,7 +89,7 @@ public class TextTemplateTests extends ESTestCase { Map model = singletonMap("key", "model_val"); CompiledScript compiledScript = mock(CompiledScript.class); - when(service.compile(new Script(templateText, ScriptType.INLINE, lang, model), Watcher.SCRIPT_CONTEXT, + when(service.compile(new Script(ScriptType.INLINE, lang, templateText, model), Watcher.SCRIPT_CONTEXT, Collections.singletonMap("content_type", "text/plain"))).thenReturn(compiledScript); when(service.executable(compiledScript, model)).thenReturn(script); when(script.run()).thenReturn("rendered_text"); @@ -143,8 +144,8 @@ public class TextTemplateTests extends ESTestCase { try { TextTemplate.parse(parser); fail("expected parse exception when encountering an unknown field"); - } catch (ElasticsearchParseException e) { - assertThat(e.getMessage(), is("unexpected field [unknown_field]")); + } catch (IllegalArgumentException e) { + assertThat(e.getMessage(), containsString("[script] unknown field [unknown_field], parser not found")); } } @@ -160,8 +161,8 @@ public class TextTemplateTests extends ESTestCase { try { TextTemplate.parse(parser); fail("expected parse exception when script type is unknown"); - } catch (ElasticsearchParseException e) { - assertThat(e.getMessage(), is("unexpected field [template]")); + } catch (IllegalArgumentException e) { + assertThat(e.getMessage(), is("[script] unknown field [template], parser not found")); } } @@ -176,8 +177,8 @@ public class TextTemplateTests extends ESTestCase { try { TextTemplate.parse(parser); fail("expected parse exception when template text is missing"); - } catch (ElasticsearchParseException e) { - assertThat(e.getMessage(), is("unexpected field [type]")); + } catch (IllegalArgumentException e) { + assertThat(e.getMessage(), containsString("[script] unknown field [type], parser not found")); } } diff --git a/elasticsearch/src/test/java/org/elasticsearch/xpack/graph/test/GraphTests.java b/elasticsearch/src/test/java/org/elasticsearch/xpack/graph/test/GraphTests.java index 889c9cc2447..42e46f80854 100644 --- a/elasticsearch/src/test/java/org/elasticsearch/xpack/graph/test/GraphTests.java +++ b/elasticsearch/src/test/java/org/elasticsearch/xpack/graph/test/GraphTests.java @@ -211,8 +211,8 @@ public class GraphTests extends ESSingleNodeTestCase { //00s friends of beatles grb.createNextHop(QueryBuilders.termQuery("decade", "00s")).addVertexRequest("people").size(100).minDocCount(1); // A query that should cause a timeout - ScriptQueryBuilder timeoutQuery = QueryBuilders.scriptQuery(new Script(NativeTestScriptedTimeout.TEST_NATIVE_SCRIPT_TIMEOUT, - ScriptType.INLINE, "native", null)); + ScriptQueryBuilder timeoutQuery = QueryBuilders.scriptQuery(new Script(ScriptType.INLINE, "native", + NativeTestScriptedTimeout.TEST_NATIVE_SCRIPT_TIMEOUT, Collections.emptyMap())); grb.createNextHop(timeoutQuery).addVertexRequest("people").size(100).minDocCount(1); GraphExploreResponse response = grb.get(); diff --git a/elasticsearch/src/test/java/org/elasticsearch/xpack/security/authz/accesscontrol/SecurityIndexSearcherWrapperUnitTests.java b/elasticsearch/src/test/java/org/elasticsearch/xpack/security/authz/accesscontrol/SecurityIndexSearcherWrapperUnitTests.java index 1ef212073f2..e142eef4f23 100644 --- a/elasticsearch/src/test/java/org/elasticsearch/xpack/security/authz/accesscontrol/SecurityIndexSearcherWrapperUnitTests.java +++ b/elasticsearch/src/test/java/org/elasticsearch/xpack/security/authz/accesscontrol/SecurityIndexSearcherWrapperUnitTests.java @@ -587,24 +587,23 @@ public class SecurityIndexSearcherWrapperUnitTests extends ESTestCase { }; ExecutableScript executableScript = mock(ExecutableScript.class); - when(scriptService.executable(any(Script.class), eq(ScriptContext.Standard.SEARCH), eq(Collections.emptyMap()))) - .thenReturn(executableScript); + when(scriptService.executable(any(Script.class), eq(ScriptContext.Standard.SEARCH))).thenReturn(executableScript); XContentBuilder builder = jsonBuilder(); String query = new TermQueryBuilder("field", "{{_user.username}}").toXContent(builder, ToXContent.EMPTY_PARAMS).string(); - Script script = new Script(query, ScriptType.INLINE, null, Collections.singletonMap("custom", "value")); + Script script = new Script(ScriptType.INLINE, "mustache", query, Collections.singletonMap("custom", "value")); builder = jsonBuilder().startObject().field("template"); script.toXContent(builder, ToXContent.EMPTY_PARAMS); BytesReference querySource = builder.endObject().bytes(); securityIndexSearcherWrapper.evaluateTemplate(querySource); ArgumentCaptor