From 473e98981bcbac246ade6f27da13aa0238b0c8f5 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Fri, 21 Apr 2017 17:53:03 -0700 Subject: [PATCH] Scripts: Remove unnecessary executable shortcut (#24264) ScriptService has two executable methods, one which takes a CompiledScript, which is similar to search, and one that takes a raw Script and both compiles and returns an ExecutableScript for it. The latter is not needed, and the call sites which used one or the other were mixed. This commit removes the extra executable method in favor of callers first calling compile, then executable. --- .../elasticsearch/action/update/UpdateHelper.java | 4 +++- .../index/query/QueryRewriteContext.java | 4 +++- .../index/query/QueryShardContext.java | 3 ++- .../org/elasticsearch/script/ScriptService.java | 7 ------- .../significant/heuristics/ScriptHeuristic.java | 4 +++- .../org/elasticsearch/script/NativeScriptTests.java | 5 +++-- .../elasticsearch/script/ScriptServiceTests.java | 13 ++++++++----- .../ingest/common/ScriptProcessor.java | 4 +++- .../ingest/common/ScriptProcessorTests.java | 3 ++- .../mustache/TransportSearchTemplateAction.java | 4 +++- 10 files changed, 30 insertions(+), 21 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/action/update/UpdateHelper.java b/core/src/main/java/org/elasticsearch/action/update/UpdateHelper.java index b1621bc118e..09fc0bccbf3 100644 --- a/core/src/main/java/org/elasticsearch/action/update/UpdateHelper.java +++ b/core/src/main/java/org/elasticsearch/action/update/UpdateHelper.java @@ -43,6 +43,7 @@ import org.elasticsearch.index.mapper.ParentFieldMapper; import org.elasticsearch.index.mapper.RoutingFieldMapper; import org.elasticsearch.index.shard.IndexShard; import org.elasticsearch.index.shard.ShardId; +import org.elasticsearch.script.CompiledScript; import org.elasticsearch.script.ExecutableScript; import org.elasticsearch.script.Script; import org.elasticsearch.script.ScriptContext; @@ -213,7 +214,8 @@ public class UpdateHelper extends AbstractComponent { private Map executeScript(Script script, Map ctx) { try { if (scriptService != null) { - ExecutableScript executableScript = scriptService.executable(script, ScriptContext.Standard.UPDATE); + CompiledScript compiledScript = scriptService.compile(script, ScriptContext.Standard.UPDATE); + ExecutableScript executableScript = scriptService.executable(compiledScript, script.getParams()); executableScript.setNextVar("ctx", ctx); executableScript.run(); } diff --git a/core/src/main/java/org/elasticsearch/index/query/QueryRewriteContext.java b/core/src/main/java/org/elasticsearch/index/query/QueryRewriteContext.java index 80726496a73..183ab690ce2 100644 --- a/core/src/main/java/org/elasticsearch/index/query/QueryRewriteContext.java +++ b/core/src/main/java/org/elasticsearch/index/query/QueryRewriteContext.java @@ -25,6 +25,7 @@ import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.index.IndexSettings; import org.elasticsearch.index.mapper.MapperService; +import org.elasticsearch.script.CompiledScript; import org.elasticsearch.script.ExecutableScript; import org.elasticsearch.script.Script; import org.elasticsearch.script.ScriptContext; @@ -104,7 +105,8 @@ public class QueryRewriteContext { } public BytesReference getTemplateBytes(Script template) { - ExecutableScript executable = scriptService.executable(template, ScriptContext.Standard.SEARCH); + CompiledScript compiledTemplate = scriptService.compile(template, ScriptContext.Standard.SEARCH); + ExecutableScript executable = scriptService.executable(compiledTemplate, template.getParams()); return (BytesReference) executable.run(); } } diff --git a/core/src/main/java/org/elasticsearch/index/query/QueryShardContext.java b/core/src/main/java/org/elasticsearch/index/query/QueryShardContext.java index 2b5e69947f3..a6a7108f7a9 100644 --- a/core/src/main/java/org/elasticsearch/index/query/QueryShardContext.java +++ b/core/src/main/java/org/elasticsearch/index/query/QueryShardContext.java @@ -351,7 +351,8 @@ public class QueryShardContext extends QueryRewriteContext { */ public final ExecutableScript getExecutableScript(Script script, ScriptContext context) { failIfFrozen(); - return scriptService.executable(script, context); + CompiledScript compiledScript = scriptService.compile(script, context); + return scriptService.executable(compiledScript, script.getParams()); } /** diff --git a/core/src/main/java/org/elasticsearch/script/ScriptService.java b/core/src/main/java/org/elasticsearch/script/ScriptService.java index 692e081a7ba..e0c7b3c63de 100644 --- a/core/src/main/java/org/elasticsearch/script/ScriptService.java +++ b/core/src/main/java/org/elasticsearch/script/ScriptService.java @@ -469,13 +469,6 @@ public class ScriptService extends AbstractComponent implements Closeable, Clust } } - /** - * Compiles (or retrieves from cache) and executes the provided script - */ - public ExecutableScript executable(Script script, ScriptContext scriptContext) { - return executable(compile(script, scriptContext), script.getParams()); - } - /** * Executes a previously compiled script provided as an argument */ diff --git a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/heuristics/ScriptHeuristic.java b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/heuristics/ScriptHeuristic.java index b73f2f0987e..7618839d496 100644 --- a/core/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/heuristics/ScriptHeuristic.java +++ b/core/src/main/java/org/elasticsearch/search/aggregations/bucket/significant/heuristics/ScriptHeuristic.java @@ -28,6 +28,7 @@ import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.index.query.QueryParseContext; import org.elasticsearch.index.query.QueryShardException; +import org.elasticsearch.script.CompiledScript; import org.elasticsearch.script.ExecutableScript; import org.elasticsearch.script.Script; import org.elasticsearch.script.ScriptContext; @@ -92,7 +93,8 @@ public class ScriptHeuristic extends SignificanceHeuristic { @Override public SignificanceHeuristic rewrite(InternalAggregation.ReduceContext context) { - return new ExecutableScriptHeuristic(script, context.scriptService().executable(script, ScriptContext.Standard.AGGS)); + CompiledScript compiledScript = context.scriptService().compile(script, ScriptContext.Standard.AGGS); + return new ExecutableScriptHeuristic(script, context.scriptService().executable(compiledScript, script.getParams())); } @Override diff --git a/core/src/test/java/org/elasticsearch/script/NativeScriptTests.java b/core/src/test/java/org/elasticsearch/script/NativeScriptTests.java index bf5c7e0daa7..aa2e260c7c2 100644 --- a/core/src/test/java/org/elasticsearch/script/NativeScriptTests.java +++ b/core/src/test/java/org/elasticsearch/script/NativeScriptTests.java @@ -52,8 +52,9 @@ public class NativeScriptTests extends ESTestCase { List> scriptSettings = scriptModule.getSettings(); scriptSettings.add(InternalSettingsPlugin.VERSION_CREATED); - ExecutableScript executable = scriptModule.getScriptService().executable( - new Script(ScriptType.INLINE, NativeScriptEngineService.NAME, "my", Collections.emptyMap()), ScriptContext.Standard.SEARCH); + Script script = new Script(ScriptType.INLINE, NativeScriptEngineService.NAME, "my", Collections.emptyMap()); + CompiledScript compiledScript = scriptModule.getScriptService().compile(script, ScriptContext.Standard.SEARCH); + ExecutableScript executable = scriptModule.getScriptService().executable(compiledScript, script.getParams()); assertThat(executable.run().toString(), equalTo("test")); } diff --git a/core/src/test/java/org/elasticsearch/script/ScriptServiceTests.java b/core/src/test/java/org/elasticsearch/script/ScriptServiceTests.java index da205a9292e..6c2bd6f1be2 100644 --- a/core/src/test/java/org/elasticsearch/script/ScriptServiceTests.java +++ b/core/src/test/java/org/elasticsearch/script/ScriptServiceTests.java @@ -346,7 +346,9 @@ public class ScriptServiceTests extends ESTestCase { public void testExecutableCountedInCompilationStats() throws IOException { buildScriptService(Settings.EMPTY); - scriptService.executable(new Script(ScriptType.INLINE, "test", "1+1", Collections.emptyMap()), randomFrom(scriptContexts)); + Script script = new Script(ScriptType.INLINE, "test", "1+1", Collections.emptyMap()); + CompiledScript compiledScript = scriptService.compile(script, randomFrom(scriptContexts)); + scriptService.executable(compiledScript, script.getParams()); assertEquals(1L, scriptService.stats().getCompilations()); } @@ -371,8 +373,9 @@ public class ScriptServiceTests extends ESTestCase { builder.put(ScriptService.SCRIPT_CACHE_SIZE_SETTING.getKey(), 1); builder.put("script.inline", "true"); buildScriptService(builder.build()); - scriptService.executable(new Script(ScriptType.INLINE, "test", "1+1", Collections.emptyMap()), randomFrom(scriptContexts)); - scriptService.executable(new Script(ScriptType.INLINE, "test", "1+1", Collections.emptyMap()), randomFrom(scriptContexts)); + Script script = new Script(ScriptType.INLINE, "test", "1+1", Collections.emptyMap()); + scriptService.compile(script, randomFrom(scriptContexts)); + scriptService.compile(script, randomFrom(scriptContexts)); assertEquals(1L, scriptService.stats().getCompilations()); } @@ -394,8 +397,8 @@ public class ScriptServiceTests extends ESTestCase { builder.put(ScriptService.SCRIPT_CACHE_SIZE_SETTING.getKey(), 1); builder.put("script.inline", "true"); buildScriptService(builder.build()); - scriptService.executable(new Script(ScriptType.INLINE, "test", "1+1", Collections.emptyMap()), randomFrom(scriptContexts)); - scriptService.executable(new Script(ScriptType.INLINE, "test", "2+2", Collections.emptyMap()), randomFrom(scriptContexts)); + scriptService.compile(new Script(ScriptType.INLINE, "test", "1+1", Collections.emptyMap()), randomFrom(scriptContexts)); + scriptService.compile(new Script(ScriptType.INLINE, "test", "2+2", Collections.emptyMap()), randomFrom(scriptContexts)); assertEquals(2L, scriptService.stats().getCompilations()); assertEquals(1L, scriptService.stats().getCacheEvictions()); } diff --git a/modules/ingest-common/src/main/java/org/elasticsearch/ingest/common/ScriptProcessor.java b/modules/ingest-common/src/main/java/org/elasticsearch/ingest/common/ScriptProcessor.java index 33c06a3804a..a5bc8034027 100644 --- a/modules/ingest-common/src/main/java/org/elasticsearch/ingest/common/ScriptProcessor.java +++ b/modules/ingest-common/src/main/java/org/elasticsearch/ingest/common/ScriptProcessor.java @@ -23,6 +23,7 @@ import org.elasticsearch.common.Strings; import org.elasticsearch.ingest.AbstractProcessor; import org.elasticsearch.ingest.IngestDocument; import org.elasticsearch.ingest.Processor; +import org.elasticsearch.script.CompiledScript; import org.elasticsearch.script.ExecutableScript; import org.elasticsearch.script.Script; import org.elasticsearch.script.ScriptContext; @@ -70,7 +71,8 @@ public final class ScriptProcessor extends AbstractProcessor { */ @Override public void execute(IngestDocument document) { - ExecutableScript executableScript = scriptService.executable(script, ScriptContext.Standard.INGEST); + CompiledScript compiledScript = scriptService.compile(script, ScriptContext.Standard.INGEST); + ExecutableScript executableScript = scriptService.executable(compiledScript, script.getParams()); executableScript.setNextVar("ctx", document.getSourceAndMetadata()); executableScript.run(); } diff --git a/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/ScriptProcessorTests.java b/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/ScriptProcessorTests.java index 94430622d1a..e76f3016dda 100644 --- a/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/ScriptProcessorTests.java +++ b/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/ScriptProcessorTests.java @@ -24,6 +24,7 @@ import java.util.Map; import org.elasticsearch.ingest.IngestDocument; import org.elasticsearch.ingest.RandomDocumentPicks; +import org.elasticsearch.script.CompiledScript; import org.elasticsearch.script.ExecutableScript; import org.elasticsearch.script.Script; import org.elasticsearch.script.ScriptService; @@ -46,7 +47,7 @@ public class ScriptProcessorTests extends ESTestCase { ScriptService scriptService = mock(ScriptService.class); Script script = new Script("_script"); ExecutableScript executableScript = mock(ExecutableScript.class); - when(scriptService.executable(any(Script.class), any())).thenReturn(executableScript); + when(scriptService.executable(any(CompiledScript.class), any())).thenReturn(executableScript); Map document = new HashMap<>(); document.put("bytes_in", randomInt()); diff --git a/modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/TransportSearchTemplateAction.java b/modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/TransportSearchTemplateAction.java index d7b04062382..61f099f6c24 100644 --- a/modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/TransportSearchTemplateAction.java +++ b/modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/TransportSearchTemplateAction.java @@ -34,6 +34,7 @@ import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.index.query.QueryParseContext; +import org.elasticsearch.script.CompiledScript; import org.elasticsearch.script.ExecutableScript; import org.elasticsearch.script.Script; import org.elasticsearch.script.ScriptService; @@ -71,7 +72,8 @@ public class TransportSearchTemplateAction extends HandledTransportAction