From 807b3b22ffe55769b697c76634aa184695dc4cde Mon Sep 17 00:00:00 2001 From: Armin Date: Thu, 12 Jul 2018 13:47:40 +0200 Subject: [PATCH 1/7] Replace Ingest ScriptContext with Custom Interface --- .../ingest/common/ScriptProcessor.java | 10 ++-- .../ingest/common/ScriptProcessorTests.java | 11 +++-- .../script/ExecutableScript.java | 1 - .../elasticsearch/script/IngestScript.java | 49 +++++++++++++++++++ .../elasticsearch/script/ScriptModule.java | 2 +- .../script/ScriptServiceTests.java | 8 +-- .../script/MockScriptEngine.java | 10 ++++ 7 files changed, 74 insertions(+), 17 deletions(-) create mode 100644 server/src/main/java/org/elasticsearch/script/IngestScript.java 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 ddb284b9c890d..74c68fd5c2638 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 @@ -31,7 +31,7 @@ import org.elasticsearch.ingest.AbstractProcessor; import org.elasticsearch.ingest.IngestDocument; import org.elasticsearch.ingest.Processor; -import org.elasticsearch.script.ExecutableScript; +import org.elasticsearch.script.IngestScript; import org.elasticsearch.script.Script; import org.elasticsearch.script.ScriptException; import org.elasticsearch.script.ScriptService; @@ -73,10 +73,8 @@ public final class ScriptProcessor extends AbstractProcessor { */ @Override public void execute(IngestDocument document) { - ExecutableScript.Factory factory = scriptService.compile(script, ExecutableScript.INGEST_CONTEXT); - ExecutableScript executableScript = factory.newInstance(script.getParams()); - executableScript.setNextVar("ctx", document.getSourceAndMetadata()); - executableScript.run(); + IngestScript.Factory factory = scriptService.compile(script, IngestScript.CONTEXT); + factory.newInstance(script.getParams()).execute(document.getSourceAndMetadata()); } @Override @@ -108,7 +106,7 @@ public ScriptProcessor create(Map registry, String pr // verify script is able to be compiled before successfully creating processor. try { - scriptService.compile(script, ExecutableScript.INGEST_CONTEXT); + scriptService.compile(script, IngestScript.CONTEXT); } catch (ScriptException e) { throw newConfigurationException(TYPE, processorTag, null, e); } 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 1004a41bcc592..ef1a89f212098 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,13 +24,14 @@ import org.elasticsearch.ingest.IngestDocument; import org.elasticsearch.ingest.RandomDocumentPicks; -import org.elasticsearch.script.ExecutableScript; +import org.elasticsearch.script.IngestScript; import org.elasticsearch.script.Script; import org.elasticsearch.script.ScriptService; import org.elasticsearch.test.ESTestCase; import static org.hamcrest.Matchers.hasKey; import static org.hamcrest.core.Is.is; +import static org.mockito.Matchers.anyMapOf; import static org.mockito.Mockito.any; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; @@ -45,9 +46,9 @@ public void testScripting() throws Exception { ScriptService scriptService = mock(ScriptService.class); Script script = mockScript("_script"); - ExecutableScript.Factory factory = mock(ExecutableScript.Factory.class); - ExecutableScript executableScript = mock(ExecutableScript.class); - when(scriptService.compile(script, ExecutableScript.INGEST_CONTEXT)).thenReturn(factory); + IngestScript.Factory factory = mock(IngestScript.Factory.class); + IngestScript executableScript = mock(IngestScript.class); + when(scriptService.compile(script, IngestScript.CONTEXT)).thenReturn(factory); when(factory.newInstance(any())).thenReturn(executableScript); Map document = new HashMap<>(); @@ -58,7 +59,7 @@ public void testScripting() throws Exception { doAnswer(invocationOnMock -> { ingestDocument.setFieldValue("bytes_total", randomBytesTotal); return null; - }).when(executableScript).run(); + }).when(executableScript).execute(anyMapOf(String.class, Object.class)); ScriptProcessor processor = new ScriptProcessor(randomAlphaOfLength(10), script, scriptService); diff --git a/server/src/main/java/org/elasticsearch/script/ExecutableScript.java b/server/src/main/java/org/elasticsearch/script/ExecutableScript.java index e87b7cdf3890a..2f7a01c37980d 100644 --- a/server/src/main/java/org/elasticsearch/script/ExecutableScript.java +++ b/server/src/main/java/org/elasticsearch/script/ExecutableScript.java @@ -50,5 +50,4 @@ interface Factory { // TODO: remove these once each has its own script interface ScriptContext AGGS_CONTEXT = new ScriptContext<>("aggs_executable", Factory.class); ScriptContext UPDATE_CONTEXT = new ScriptContext<>("update", Factory.class); - ScriptContext INGEST_CONTEXT = new ScriptContext<>("ingest", Factory.class); } diff --git a/server/src/main/java/org/elasticsearch/script/IngestScript.java b/server/src/main/java/org/elasticsearch/script/IngestScript.java new file mode 100644 index 0000000000000..44db2e2853c2e --- /dev/null +++ b/server/src/main/java/org/elasticsearch/script/IngestScript.java @@ -0,0 +1,49 @@ + +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.script; + +import java.util.Map; + +public abstract class IngestScript { + + public static final String[] PARAMETERS = { "ctx" }; + + /** The context used to compile {@link IngestScript} factories. */ + public static final ScriptContext CONTEXT = new ScriptContext<>("ingest", Factory.class); + + /** The generic runtime parameters for the script. */ + private final Map params; + + public IngestScript(Map params) { + this.params = params; + } + + /** Return the parameters for this script. */ + public Map getParams() { + return params; + } + + public abstract void execute(Map ctx); + + public interface Factory { + IngestScript newInstance(Map params); + } +} diff --git a/server/src/main/java/org/elasticsearch/script/ScriptModule.java b/server/src/main/java/org/elasticsearch/script/ScriptModule.java index 042953117c5a5..bf4bd9c57cef0 100644 --- a/server/src/main/java/org/elasticsearch/script/ScriptModule.java +++ b/server/src/main/java/org/elasticsearch/script/ScriptModule.java @@ -51,7 +51,7 @@ public class ScriptModule { ExecutableScript.CONTEXT, ExecutableScript.AGGS_CONTEXT, ExecutableScript.UPDATE_CONTEXT, - ExecutableScript.INGEST_CONTEXT, + IngestScript.CONTEXT, FilterScript.CONTEXT, SimilarityScript.CONTEXT, SimilarityWeightScript.CONTEXT, diff --git a/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java b/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java index b35fcbcc03c17..585f860165160 100644 --- a/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java +++ b/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java @@ -168,7 +168,7 @@ public void testAllowAllScriptContextSettings() throws IOException { assertCompileAccepted("painless", "script", ScriptType.INLINE, SearchScript.CONTEXT); assertCompileAccepted("painless", "script", ScriptType.INLINE, SearchScript.AGGS_CONTEXT); assertCompileAccepted("painless", "script", ScriptType.INLINE, ExecutableScript.UPDATE_CONTEXT); - assertCompileAccepted("painless", "script", ScriptType.INLINE, ExecutableScript.INGEST_CONTEXT); + assertCompileAccepted("painless", "script", ScriptType.INLINE, IngestScript.CONTEXT); } public void testAllowSomeScriptTypeSettings() throws IOException { @@ -209,13 +209,13 @@ public void testAllowNoScriptContextSettings() throws IOException { } public void testCompileNonRegisteredContext() throws IOException { - contexts.remove(ExecutableScript.INGEST_CONTEXT.name); + contexts.remove(IngestScript.CONTEXT.name); buildScriptService(Settings.EMPTY); String type = scriptEngine.getType(); IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> - scriptService.compile(new Script(ScriptType.INLINE, type, "test", Collections.emptyMap()), ExecutableScript.INGEST_CONTEXT)); - assertThat(e.getMessage(), containsString("script context [" + ExecutableScript.INGEST_CONTEXT.name + "] not supported")); + scriptService.compile(new Script(ScriptType.INLINE, type, "test", Collections.emptyMap()), IngestScript.CONTEXT)); + assertThat(e.getMessage(), containsString("script context [" + IngestScript.CONTEXT.name + "] not supported")); } public void testCompileCountedInCompilationStats() throws IOException { diff --git a/test/framework/src/main/java/org/elasticsearch/script/MockScriptEngine.java b/test/framework/src/main/java/org/elasticsearch/script/MockScriptEngine.java index e608bd13d2559..57ac82a040778 100644 --- a/test/framework/src/main/java/org/elasticsearch/script/MockScriptEngine.java +++ b/test/framework/src/main/java/org/elasticsearch/script/MockScriptEngine.java @@ -88,6 +88,16 @@ public T compile(String name, String source, ScriptContext context, Map new IngestScript(parameters) { + @Override + public void execute(Map ctx) { + if (script != null) { + script.apply(ctx); + } + } + }; + return context.factoryClazz.cast(factory); } else if (context.instanceClazz.equals(TemplateScript.class)) { TemplateScript.Factory factory = vars -> { // TODO: need a better way to implement all these new contexts From ada26d7dfeafc47d4ca663a889b1c5d9e9aa8190 Mon Sep 17 00:00:00 2001 From: Armin Date: Thu, 12 Jul 2018 15:09:35 +0200 Subject: [PATCH 2/7] Remove unnecessary null check --- .../main/java/org/elasticsearch/script/MockScriptEngine.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/test/framework/src/main/java/org/elasticsearch/script/MockScriptEngine.java b/test/framework/src/main/java/org/elasticsearch/script/MockScriptEngine.java index 57ac82a040778..8e40e4bcf1468 100644 --- a/test/framework/src/main/java/org/elasticsearch/script/MockScriptEngine.java +++ b/test/framework/src/main/java/org/elasticsearch/script/MockScriptEngine.java @@ -92,9 +92,7 @@ public T compile(String name, String source, ScriptContext context, Map new IngestScript(parameters) { @Override public void execute(Map ctx) { - if (script != null) { - script.apply(ctx); - } + script.apply(ctx); } }; return context.factoryClazz.cast(factory); From 89874c9eb53b6b07a770f47fa3dfd32e44bc49d4 Mon Sep 17 00:00:00 2001 From: Armin Date: Thu, 12 Jul 2018 15:17:10 +0200 Subject: [PATCH 3/7] Make org.elasticsearch.ingest.common.ScriptProcessorTests#testScripting more precise --- .../org/elasticsearch/ingest/common/ScriptProcessorTests.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) 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 ef1a89f212098..6957a2788a1a6 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 @@ -57,7 +57,9 @@ public void testScripting() throws Exception { IngestDocument ingestDocument = RandomDocumentPicks.randomIngestDocument(random(), document); doAnswer(invocationOnMock -> { - ingestDocument.setFieldValue("bytes_total", randomBytesTotal); + @SuppressWarnings("unchecked") + Map ctx = (Map) invocationOnMock.getArguments()[0]; + ctx.put("bytes_total", randomBytesTotal); return null; }).when(executableScript).execute(anyMapOf(String.class, Object.class)); From fb9647dc57b7b02005b4b20c8f78c3a2bcff6219 Mon Sep 17 00:00:00 2001 From: Armin Date: Thu, 12 Jul 2018 16:13:31 +0200 Subject: [PATCH 4/7] Don't mock script factory in ScriptProcessorTests --- .../ingest/common/ScriptProcessorTests.java | 41 ++++++++++--------- 1 file changed, 21 insertions(+), 20 deletions(-) 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 6957a2788a1a6..72bc337e9c9f7 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 @@ -19,23 +19,22 @@ package org.elasticsearch.ingest.common; +import java.util.Collections; import java.util.HashMap; import java.util.Map; +import org.elasticsearch.common.settings.Settings; import org.elasticsearch.ingest.IngestDocument; import org.elasticsearch.ingest.RandomDocumentPicks; -import org.elasticsearch.script.IngestScript; +import org.elasticsearch.script.MockScriptEngine; import org.elasticsearch.script.Script; +import org.elasticsearch.script.ScriptModule; import org.elasticsearch.script.ScriptService; +import org.elasticsearch.script.ScriptType; import org.elasticsearch.test.ESTestCase; import static org.hamcrest.Matchers.hasKey; import static org.hamcrest.core.Is.is; -import static org.mockito.Matchers.anyMapOf; -import static org.mockito.Mockito.any; -import static org.mockito.Mockito.doAnswer; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; public class ScriptProcessorTests extends ESTestCase { @@ -43,26 +42,28 @@ public void testScripting() throws Exception { int randomBytesIn = randomInt(); int randomBytesOut = randomInt(); int randomBytesTotal = randomBytesIn + randomBytesOut; - - ScriptService scriptService = mock(ScriptService.class); - Script script = mockScript("_script"); - IngestScript.Factory factory = mock(IngestScript.Factory.class); - IngestScript executableScript = mock(IngestScript.class); - when(scriptService.compile(script, IngestScript.CONTEXT)).thenReturn(factory); - when(factory.newInstance(any())).thenReturn(executableScript); + String scriptName = "script"; + ScriptService scriptService = new ScriptService(Settings.builder().build(), + Collections.singletonMap( + Script.DEFAULT_SCRIPT_LANG, new MockScriptEngine( + Script.DEFAULT_SCRIPT_LANG, + Collections.singletonMap( + scriptName, ctx -> { + ctx.put("bytes_total", randomBytesTotal); + return null; + } + ) + ) + ), + new HashMap<>(ScriptModule.CORE_CONTEXTS) + ); + Script script = new Script(ScriptType.INLINE, Script.DEFAULT_SCRIPT_LANG, scriptName, Collections.emptyMap()); Map document = new HashMap<>(); document.put("bytes_in", randomInt()); document.put("bytes_out", randomInt()); IngestDocument ingestDocument = RandomDocumentPicks.randomIngestDocument(random(), document); - doAnswer(invocationOnMock -> { - @SuppressWarnings("unchecked") - Map ctx = (Map) invocationOnMock.getArguments()[0]; - ctx.put("bytes_total", randomBytesTotal); - return null; - }).when(executableScript).execute(anyMapOf(String.class, Object.class)); - ScriptProcessor processor = new ScriptProcessor(randomAlphaOfLength(10), script, scriptService); processor.execute(ingestDocument); From b1f7d4385fc2fe909dda839e42541d902d91348a Mon Sep 17 00:00:00 2001 From: Armin Date: Thu, 12 Jul 2018 16:40:47 +0200 Subject: [PATCH 5/7] Adjust mock script plugin in IT for new API --- .../java/org/elasticsearch/ingest/common/IngestRestartIT.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/IngestRestartIT.java b/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/IngestRestartIT.java index 69236144007bc..0fa615a825434 100644 --- a/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/IngestRestartIT.java +++ b/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/IngestRestartIT.java @@ -60,7 +60,7 @@ public static class CustomScriptPlugin extends MockScriptPlugin { protected Map, Object>> pluginScripts() { return Collections.singletonMap("my_script", script -> { @SuppressWarnings("unchecked") - Map ctx = (Map) script.get("ctx"); + Map ctx = script; ctx.put("z", 0); return null; }); From c35c213ac57098d71fd7a8e65bb6743dc122d7e9 Mon Sep 17 00:00:00 2001 From: Armin Date: Thu, 12 Jul 2018 16:51:36 +0200 Subject: [PATCH 6/7] Adjust mock script plugin in IT for new API --- .../java/org/elasticsearch/ingest/common/IngestRestartIT.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/IngestRestartIT.java b/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/IngestRestartIT.java index 0fa615a825434..8c3976d2b175c 100644 --- a/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/IngestRestartIT.java +++ b/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/IngestRestartIT.java @@ -58,9 +58,7 @@ protected boolean ignoreExternalCluster() { public static class CustomScriptPlugin extends MockScriptPlugin { @Override protected Map, Object>> pluginScripts() { - return Collections.singletonMap("my_script", script -> { - @SuppressWarnings("unchecked") - Map ctx = script; + return Collections.singletonMap("my_script", ctx -> { ctx.put("z", 0); return null; }); From 762d2f7b7fa47a1fe42669ea529a2abe4090c148 Mon Sep 17 00:00:00 2001 From: Armin Date: Fri, 13 Jul 2018 10:40:10 +0200 Subject: [PATCH 7/7] add class level javadoc --- .../src/main/java/org/elasticsearch/script/IngestScript.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/server/src/main/java/org/elasticsearch/script/IngestScript.java b/server/src/main/java/org/elasticsearch/script/IngestScript.java index 44db2e2853c2e..f357394ed31f0 100644 --- a/server/src/main/java/org/elasticsearch/script/IngestScript.java +++ b/server/src/main/java/org/elasticsearch/script/IngestScript.java @@ -22,6 +22,9 @@ import java.util.Map; +/** + * A script used by the Ingest Script Processor. + */ public abstract class IngestScript { public static final String[] PARAMETERS = { "ctx" };