From 47cbae9b265c3ad86a9e8de28d8b416539354e6b Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Fri, 28 Sep 2018 17:13:08 -0700 Subject: [PATCH] Scripting: Remove ExecutableScript (#34154) This commit removes the legacy ExecutableScript, which was no longer used except in tests. All uses have previously been converted to script contexts. --- .../ExpressionExecutableScript.java | 88 ----------------- .../expression/ExpressionScriptEngine.java | 4 - .../expression/MoreExpressionTests.java | 65 ------------- .../painless/PainlessScriptEngine.java | 20 +--- .../elasticsearch/painless/ScriptImpl.java | 3 +- .../elasticsearch/painless/BasicAPITests.java | 3 +- .../painless/BasicStatementTests.java | 6 +- .../elasticsearch/painless/BindingsTests.java | 55 ++++++----- .../painless/NeedsScoreTests.java | 2 - .../painless/ReservedWordTests.java | 96 ------------------- .../elasticsearch/painless/ScoreTests.java | 72 -------------- .../painless/ScriptEngineTests.java | 36 ------- .../painless/ScriptTestCase.java | 22 ++--- .../elasticsearch/painless/StringTests.java | 2 +- .../painless/WhenThingsGoWrongTests.java | 6 +- ...AsyncBulkByScrollActionScriptTestCase.java | 7 +- .../index/reindex/SimpleExecutableScript.java | 51 ---------- .../script/ExecutableScript.java | 49 ---------- .../elasticsearch/script/ScriptModule.java | 1 - .../elasticsearch/script/SearchScript.java | 4 +- .../script/MockScriptEngine.java | 69 ++++--------- .../test/MockPainlessScriptEngine.java | 5 +- 22 files changed, 71 insertions(+), 595 deletions(-) delete mode 100644 modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionExecutableScript.java delete mode 100644 modules/lang-painless/src/test/java/org/elasticsearch/painless/ReservedWordTests.java delete mode 100644 modules/lang-painless/src/test/java/org/elasticsearch/painless/ScoreTests.java delete mode 100644 modules/reindex/src/test/java/org/elasticsearch/index/reindex/SimpleExecutableScript.java delete mode 100644 server/src/main/java/org/elasticsearch/script/ExecutableScript.java diff --git a/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionExecutableScript.java b/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionExecutableScript.java deleted file mode 100644 index f9cdae40457..00000000000 --- a/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionExecutableScript.java +++ /dev/null @@ -1,88 +0,0 @@ -/* - * 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.expression; - -import org.apache.lucene.expressions.Expression; -import org.elasticsearch.script.ExecutableScript; -import org.elasticsearch.script.GeneralScriptException; - -import java.util.HashMap; -import java.util.Map; - -/** - * A bridge to evaluate an {@link Expression} against a map of variables in the context - * of an {@link ExecutableScript}. - */ -public class ExpressionExecutableScript implements ExecutableScript { - public final Expression expression; - public final Map functionValuesMap; - public final ReplaceableConstDoubleValues[] functionValuesArray; - - public ExpressionExecutableScript(Expression expression, Map vars) { - this.expression = expression; - int functionValuesLength = expression.variables.length; - - if (vars.size() != functionValuesLength) { - throw new GeneralScriptException("Error using " + expression + ". " + - "The number of variables in an executable expression script [" + - functionValuesLength + "] must match the number of variables in the variable map" + - " [" + vars.size() + "]."); - } - - functionValuesArray = new ReplaceableConstDoubleValues[functionValuesLength]; - functionValuesMap = new HashMap<>(); - - for (int functionValuesIndex = 0; functionValuesIndex < functionValuesLength; ++functionValuesIndex) { - String variableName = expression.variables[functionValuesIndex]; - functionValuesArray[functionValuesIndex] = new ReplaceableConstDoubleValues(); - functionValuesMap.put(variableName, functionValuesArray[functionValuesIndex]); - } - - for (String varsName : vars.keySet()) { - setNextVar(varsName, vars.get(varsName)); - } - } - - @Override - public void setNextVar(String name, Object value) { - if (functionValuesMap.containsKey(name)) { - if (value instanceof Number) { - double doubleValue = ((Number)value).doubleValue(); - functionValuesMap.get(name).setValue(doubleValue); - } else { - throw new GeneralScriptException("Error using " + expression + ". " + - "Executable expressions scripts can only process numbers." + - " The variable [" + name + "] is not a number."); - } - } else { - throw new GeneralScriptException("Error using " + expression + ". " + - "The variable [" + name + "] does not exist in the executable expressions script."); - } - } - - @Override - public Object run() { - try { - return expression.evaluate(functionValuesArray); - } catch (Exception exception) { - throw new GeneralScriptException("Error evaluating " + expression, exception); - } - } -} diff --git a/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionScriptEngine.java b/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionScriptEngine.java index 55f8deb0592..9d305a5f2d9 100644 --- a/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionScriptEngine.java +++ b/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionScriptEngine.java @@ -41,7 +41,6 @@ import org.elasticsearch.index.mapper.MapperService; import org.elasticsearch.script.BucketAggregationScript; import org.elasticsearch.script.BucketAggregationSelectorScript; import org.elasticsearch.script.ClassPermission; -import org.elasticsearch.script.ExecutableScript; import org.elasticsearch.script.FilterScript; import org.elasticsearch.script.ScoreScript; import org.elasticsearch.script.ScriptContext; @@ -112,9 +111,6 @@ public class ExpressionScriptEngine extends AbstractComponent implements ScriptE if (context.instanceClazz.equals(SearchScript.class)) { SearchScript.Factory factory = (p, lookup) -> newSearchScript(expr, lookup, p); return context.factoryClazz.cast(factory); - } else if (context.instanceClazz.equals(ExecutableScript.class)) { - ExecutableScript.Factory factory = (p) -> new ExpressionExecutableScript(expr, p); - return context.factoryClazz.cast(factory); } else if (context.instanceClazz.equals(BucketAggregationScript.class)) { return context.factoryClazz.cast(newBucketAggregationScriptFactory(expr)); } else if (context.instanceClazz.equals(BucketAggregationSelectorScript.class)) { diff --git a/modules/lang-expression/src/test/java/org/elasticsearch/script/expression/MoreExpressionTests.java b/modules/lang-expression/src/test/java/org/elasticsearch/script/expression/MoreExpressionTests.java index 6d7ab1d2595..11310710769 100644 --- a/modules/lang-expression/src/test/java/org/elasticsearch/script/expression/MoreExpressionTests.java +++ b/modules/lang-expression/src/test/java/org/elasticsearch/script/expression/MoreExpressionTests.java @@ -19,8 +19,6 @@ package org.elasticsearch.script.expression; -import org.apache.lucene.expressions.Expression; -import org.apache.lucene.expressions.js.JavascriptCompiler; import org.elasticsearch.action.search.SearchPhaseExecutionException; import org.elasticsearch.action.search.SearchRequestBuilder; import org.elasticsearch.action.search.SearchResponse; @@ -33,7 +31,6 @@ import org.elasticsearch.index.query.QueryBuilders; import org.elasticsearch.index.query.functionscore.ScoreFunctionBuilder; import org.elasticsearch.index.query.functionscore.ScoreFunctionBuilders; import org.elasticsearch.plugins.Plugin; -import org.elasticsearch.script.GeneralScriptException; import org.elasticsearch.script.Script; import org.elasticsearch.script.ScriptType; import org.elasticsearch.search.SearchHits; @@ -504,68 +501,6 @@ public class MoreExpressionTests extends ESIntegTestCase { message.contains("text variable"), equalTo(true)); } - // series of unit test for using expressions as executable scripts - public void testExecutableScripts() throws Exception { - assumeTrue("test creates classes directly, cannot run with security manager", System.getSecurityManager() == null); - Map vars = new HashMap<>(); - vars.put("a", 2.5); - vars.put("b", 3); - vars.put("xyz", -1); - - Expression expr = JavascriptCompiler.compile("a+b+xyz"); - - ExpressionExecutableScript ees = new ExpressionExecutableScript(expr, vars); - assertEquals((Double) ees.run(), 4.5, 0.001); - - ees.setNextVar("b", -2.5); - assertEquals((Double) ees.run(), -1, 0.001); - - ees.setNextVar("a", -2.5); - ees.setNextVar("b", -2.5); - ees.setNextVar("xyz", -2.5); - assertEquals((Double) ees.run(), -7.5, 0.001); - - String message; - - try { - vars = new HashMap<>(); - vars.put("a", 1); - ees = new ExpressionExecutableScript(expr, vars); - ees.run(); - fail("An incorrect number of variables were allowed to be used in an expression."); - } catch (GeneralScriptException se) { - message = se.getMessage(); - assertThat(message + " should have contained number of variables", message.contains("number of variables"), equalTo(true)); - } - - try { - vars = new HashMap<>(); - vars.put("a", 1); - vars.put("b", 3); - vars.put("c", -1); - ees = new ExpressionExecutableScript(expr, vars); - ees.run(); - fail("A variable was allowed to be set that does not exist in the expression."); - } catch (GeneralScriptException se) { - message = se.getMessage(); - assertThat(message + " should have contained does not exist in", message.contains("does not exist in"), equalTo(true)); - } - - try { - vars = new HashMap<>(); - vars.put("a", 1); - vars.put("b", 3); - vars.put("xyz", "hello"); - ees = new ExpressionExecutableScript(expr, vars); - ees.run(); - fail("A non-number was allowed to be use in the expression."); - } catch (GeneralScriptException se) { - message = se.getMessage(); - assertThat(message + " should have contained process numbers", message.contains("process numbers"), equalTo(true)); - } - - } - // test to make sure expressions are not allowed to be used as update scripts public void testInvalidUpdateScript() throws Exception { try { diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessScriptEngine.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessScriptEngine.java index 1f583174a3f..5ed305751c8 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessScriptEngine.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessScriptEngine.java @@ -26,7 +26,6 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.painless.Compiler.Loader; import org.elasticsearch.painless.lookup.PainlessLookupBuilder; import org.elasticsearch.painless.spi.Whitelist; -import org.elasticsearch.script.ExecutableScript; import org.elasticsearch.script.ScriptContext; import org.elasticsearch.script.ScriptEngine; import org.elasticsearch.script.ScriptException; @@ -102,7 +101,7 @@ public final class PainlessScriptEngine extends AbstractComponent implements Scr for (Map.Entry, List> entry : contexts.entrySet()) { ScriptContext context = entry.getKey(); - if (context.instanceClazz.equals(SearchScript.class) || context.instanceClazz.equals(ExecutableScript.class)) { + if (context.instanceClazz.equals(SearchScript.class)) { contextsToCompilers.put(context, new Compiler(GenericElasticsearchScript.class, null, null, PainlessLookupBuilder.buildFromWhitelists(entry.getValue()))); } else { @@ -154,23 +153,6 @@ public final class PainlessScriptEngine extends AbstractComponent implements Scr return needsScore; } }; - return context.factoryClazz.cast(factory); - } else if (context.instanceClazz.equals(ExecutableScript.class)) { - Constructor constructor = compile(compiler, scriptName, scriptSource, params); - - ExecutableScript.Factory factory = new ExecutableScript.Factory() { - @Override - public ExecutableScript newInstance(Map params) { - try { - // a new instance is required for the class bindings model to work correctly - GenericElasticsearchScript newInstance = (GenericElasticsearchScript)constructor.newInstance(); - return new ScriptImpl(newInstance, params, null, null); - } catch (InstantiationException | IllegalAccessException | InvocationTargetException e) { - throw new IllegalArgumentException("internal error"); - } - } - }; - return context.factoryClazz.cast(factory); } else { // Check we ourselves are not being called by unprivileged code. diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/ScriptImpl.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/ScriptImpl.java index 067bf38cb36..2f31694ff3c 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/ScriptImpl.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/ScriptImpl.java @@ -20,7 +20,6 @@ package org.elasticsearch.painless; import org.apache.lucene.index.LeafReaderContext; -import org.elasticsearch.script.ExecutableScript; import org.elasticsearch.script.SearchScript; import org.elasticsearch.search.lookup.LeafSearchLookup; import org.elasticsearch.search.lookup.SearchLookup; @@ -31,7 +30,7 @@ import java.util.function.DoubleSupplier; import java.util.function.Function; /** - * ScriptImpl can be used as either an {@link ExecutableScript} or a {@link SearchScript} + * ScriptImpl can be used as a {@link SearchScript} * to run a previously compiled Painless script. */ final class ScriptImpl extends SearchScript { diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/BasicAPITests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/BasicAPITests.java index 9863db0b21e..c5cc723ca84 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/BasicAPITests.java +++ b/modules/lang-painless/src/test/java/org/elasticsearch/painless/BasicAPITests.java @@ -67,7 +67,8 @@ public class BasicAPITests extends ScriptTestCase { ctx.put("_source", _source); params.put("ctx", ctx); - assertEquals("testvalue", exec("ctx._source['load'].5 = ctx._source['load'].remove('load5')", params, true)); + assertEquals("testvalue", exec("params.ctx._source['load'].5 = params.ctx._source['load'].remove('load5')", + params, true)); } /** Test loads and stores with a list */ diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/BasicStatementTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/BasicStatementTests.java index 0f5844c6599..6f632b5df48 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/BasicStatementTests.java +++ b/modules/lang-painless/src/test/java/org/elasticsearch/painless/BasicStatementTests.java @@ -262,19 +262,19 @@ public class BasicStatementTests extends ScriptTestCase { "for (int i = 0; i < array.length; i++) { sum += array[i] } return sum", Collections.emptyMap(), Collections.singletonMap(CompilerSettings.MAX_LOOP_COUNTER, "0"), - null, true + true )); assertEquals(6L, exec("long sum = 0; long[] array = new long[] { 1, 2, 3 };" + "int i = 0; while (i < array.length) { sum += array[i++] } return sum", Collections.emptyMap(), Collections.singletonMap(CompilerSettings.MAX_LOOP_COUNTER, "0"), - null, true + true )); assertEquals(6L, exec("long sum = 0; long[] array = new long[] { 1, 2, 3 };" + "int i = 0; do { sum += array[i++] } while (i < array.length); return sum", Collections.emptyMap(), Collections.singletonMap(CompilerSettings.MAX_LOOP_COUNTER, "0"), - null, true + true )); } diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/BindingsTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/BindingsTests.java index 4bcc557d3dc..167deb3a20b 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/BindingsTests.java +++ b/modules/lang-painless/src/test/java/org/elasticsearch/painless/BindingsTests.java @@ -19,46 +19,51 @@ package org.elasticsearch.painless; -import org.elasticsearch.script.ExecutableScript; +import org.elasticsearch.painless.spi.Whitelist; +import org.elasticsearch.script.ScriptContext; import java.util.Collections; -import java.util.HashMap; +import java.util.List; import java.util.Map; public class BindingsTests extends ScriptTestCase { + public abstract static class BindingsTestScript { + public static final String[] PARAMETERS = { "test", "bound" }; + public abstract int execute(int test, int bound); + public interface Factory { + BindingsTestScript newInstance(); + } + public static final ScriptContext CONTEXT = new ScriptContext<>("bindings_test", Factory.class); + } + + @Override + protected Map, List> scriptContexts() { + Map, List> contexts = super.scriptContexts(); + contexts.put(BindingsTestScript.CONTEXT, Whitelist.BASE_WHITELISTS); + return contexts; + } + public void testBasicBinding() { assertEquals(15, exec("testAddWithState(4, 5, 6, 0.0)")); } public void testRepeatedBinding() { - String script = "testAddWithState(4, 5, params.test, 0.0)"; - Map params = new HashMap<>(); - ExecutableScript.Factory factory = scriptEngine.compile(null, script, ExecutableScript.CONTEXT, Collections.emptyMap()); - ExecutableScript executableScript = factory.newInstance(params); + String script = "testAddWithState(4, 5, test, 0.0)"; + BindingsTestScript.Factory factory = scriptEngine.compile(null, script, BindingsTestScript.CONTEXT, Collections.emptyMap()); + BindingsTestScript executableScript = factory.newInstance(); - executableScript.setNextVar("test", 5); - assertEquals(14, executableScript.run()); - - executableScript.setNextVar("test", 4); - assertEquals(13, executableScript.run()); - - executableScript.setNextVar("test", 7); - assertEquals(16, executableScript.run()); + assertEquals(14, executableScript.execute(5, 0)); + assertEquals(13, executableScript.execute(4, 0)); + assertEquals(16, executableScript.execute(7, 0)); } public void testBoundBinding() { - String script = "testAddWithState(4, params.bound, params.test, 0.0)"; - Map params = new HashMap<>(); - ExecutableScript.Factory factory = scriptEngine.compile(null, script, ExecutableScript.CONTEXT, Collections.emptyMap()); - ExecutableScript executableScript = factory.newInstance(params); + String script = "testAddWithState(4, bound, test, 0.0)"; + BindingsTestScript.Factory factory = scriptEngine.compile(null, script, BindingsTestScript.CONTEXT, Collections.emptyMap()); + BindingsTestScript executableScript = factory.newInstance(); - executableScript.setNextVar("test", 5); - executableScript.setNextVar("bound", 1); - assertEquals(10, executableScript.run()); - - executableScript.setNextVar("test", 4); - executableScript.setNextVar("bound", 2); - assertEquals(9, executableScript.run()); + assertEquals(10, executableScript.execute(5, 1)); + assertEquals(9, executableScript.execute(4, 2)); } } diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/NeedsScoreTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/NeedsScoreTests.java index 50a377b8818..86f2af32d16 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/NeedsScoreTests.java +++ b/modules/lang-painless/src/test/java/org/elasticsearch/painless/NeedsScoreTests.java @@ -23,7 +23,6 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.index.IndexService; import org.elasticsearch.index.query.QueryShardContext; import org.elasticsearch.painless.spi.Whitelist; -import org.elasticsearch.script.ExecutableScript; import org.elasticsearch.script.ScriptContext; import org.elasticsearch.script.SearchScript; import org.elasticsearch.search.lookup.SearchLookup; @@ -45,7 +44,6 @@ public class NeedsScoreTests extends ESSingleNodeTestCase { Map, List> contexts = new HashMap<>(); contexts.put(SearchScript.CONTEXT, Whitelist.BASE_WHITELISTS); - contexts.put(ExecutableScript.CONTEXT, Whitelist.BASE_WHITELISTS); PainlessScriptEngine service = new PainlessScriptEngine(Settings.EMPTY, contexts); QueryShardContext shardContext = index.newQueryShardContext(0, null, () -> 0, null); diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/ReservedWordTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/ReservedWordTests.java deleted file mode 100644 index 08b78b1c708..00000000000 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/ReservedWordTests.java +++ /dev/null @@ -1,96 +0,0 @@ -/* - * 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.painless; - -import java.util.Collections; -import java.util.HashMap; - -/** Tests for special reserved words such as _score */ -public class ReservedWordTests extends ScriptTestCase { - - /** check that we can't declare a variable of _score, its really reserved! */ - public void testScoreVar() { - IllegalArgumentException expected = expectScriptThrows(IllegalArgumentException.class, () -> { - exec("int _score = 5; return _score;"); - }); - assertTrue(expected.getMessage().contains("Variable [_score] is already defined")); - } - - /** check that we can't write to _score, its read-only! */ - public void testScoreStore() { - IllegalArgumentException expected = expectScriptThrows(IllegalArgumentException.class, () -> { - exec("_score = 5; return _score;"); - }); - assertTrue(expected.getMessage().contains("Variable [_score] is read-only")); - } - - /** check that we can't declare a variable of doc, its really reserved! */ - public void testDocVar() { - IllegalArgumentException expected = expectScriptThrows(IllegalArgumentException.class, () -> { - exec("int doc = 5; return doc;"); - }); - assertTrue(expected.getMessage().contains("Variable [doc] is already defined")); - } - - /** check that we can't write to doc, its read-only! */ - public void testDocStore() { - IllegalArgumentException expected = expectScriptThrows(IllegalArgumentException.class, () -> { - exec("doc = 5; return doc;"); - }); - assertTrue(expected.getMessage().contains("Variable [doc] is read-only")); - } - - /** check that we can't declare a variable of ctx, its really reserved! */ - public void testCtxVar() { - IllegalArgumentException expected = expectScriptThrows(IllegalArgumentException.class, () -> { - exec("int ctx = 5; return ctx;"); - }); - assertTrue(expected.getMessage().contains("Variable [ctx] is already defined")); - } - - /** check that we can't write to ctx, its read-only! */ - public void testCtxStore() { - IllegalArgumentException expected = expectScriptThrows(IllegalArgumentException.class, () -> { - exec("ctx = 5; return ctx;"); - }); - assertTrue(expected.getMessage().contains("Variable [ctx] is read-only")); - } - - /** check that we can modify its contents though */ - public void testCtxStoreMap() { - assertEquals(5, exec("ctx.foo = 5; return ctx.foo;", Collections.singletonMap("ctx", new HashMap()), true)); - } - - /** check that we can't declare a variable of _value, its really reserved! */ - public void testAggregationValueVar() { - IllegalArgumentException expected = expectScriptThrows(IllegalArgumentException.class, () -> { - exec("int _value = 5; return _value;"); - }); - assertTrue(expected.getMessage().contains("Variable [_value] is already defined")); - } - - /** check that we can't write to _value, its read-only! */ - public void testAggregationValueStore() { - IllegalArgumentException expected = expectScriptThrows(IllegalArgumentException.class, () -> { - exec("_value = 5; return _value;"); - }); - assertTrue(expected.getMessage().contains("Variable [_value] is read-only")); - } -} diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/ScoreTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/ScoreTests.java deleted file mode 100644 index 3d19dedd3b0..00000000000 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/ScoreTests.java +++ /dev/null @@ -1,72 +0,0 @@ -/* - * 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.painless; - -import org.apache.lucene.search.Scorable; - -import java.util.Collections; - -public class ScoreTests extends ScriptTestCase { - - /** Most of a dummy scorer impl that requires overriding just score(). */ - abstract class MockScorer extends Scorable { - @Override - public int docID() { - return 0; - } - } - - public void testScoreWorks() { - assertEquals(2.5, exec("_score", Collections.emptyMap(), Collections.emptyMap(), - new MockScorer() { - @Override - public float score() { - return 2.5f; - } - }, - true)); - } - - public void testScoreNotUsed() { - assertEquals(3.5, exec("3.5", Collections.emptyMap(), Collections.emptyMap(), - new MockScorer() { - @Override - public float score() { - throw new AssertionError("score() should not be called"); - } - }, - true)); - } - - public void testScoreCached() { - assertEquals(9.0, exec("_score + _score", Collections.emptyMap(), Collections.emptyMap(), - new MockScorer() { - private boolean used = false; - @Override - public float score() { - if (used == false) { - return 4.5f; - } - throw new AssertionError("score() should not be called twice"); - } - }, - true)); - } -} diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/ScriptEngineTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/ScriptEngineTests.java index 37b1ff68ec5..6594225842e 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/ScriptEngineTests.java +++ b/modules/lang-painless/src/test/java/org/elasticsearch/painless/ScriptEngineTests.java @@ -19,10 +19,7 @@ package org.elasticsearch.painless; -import org.elasticsearch.script.ExecutableScript; - import java.util.Arrays; -import java.util.Collections; import java.util.HashMap; import java.util.Map; @@ -73,37 +70,4 @@ public class ScriptEngineTests extends ScriptTestCase { assertEquals("value1", exec("return params.l.3.prop1;", vars, true)); } - - public void testChangingVarsCrossExecution1() { - Map vars = new HashMap<>(); - Map ctx = new HashMap<>(); - vars.put("ctx", ctx); - - ExecutableScript.Factory factory = - scriptEngine.compile(null, "return ctx.value;", ExecutableScript.CONTEXT, Collections.emptyMap()); - ExecutableScript script = factory.newInstance(vars); - - ctx.put("value", 1); - Object o = script.run(); - assertEquals(1, ((Number) o).intValue()); - - ctx.put("value", 2); - o = script.run(); - assertEquals(2, ((Number) o).intValue()); - } - - public void testChangingVarsCrossExecution2() { - Map vars = new HashMap<>(); - ExecutableScript.Factory factory = - scriptEngine.compile(null, "return params['value'];", ExecutableScript.CONTEXT, Collections.emptyMap()); - ExecutableScript script = factory.newInstance(vars); - - script.setNextVar("value", 1); - Object value = script.run(); - assertEquals(1, ((Number)value).intValue()); - - script.setNextVar("value", 2); - value = script.run(); - assertEquals(2, ((Number)value).intValue()); - } } diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/ScriptTestCase.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/ScriptTestCase.java index 577b120fc90..e69a1ad5dcf 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/ScriptTestCase.java +++ b/modules/lang-painless/src/test/java/org/elasticsearch/painless/ScriptTestCase.java @@ -20,24 +20,23 @@ package org.elasticsearch.painless; import junit.framework.AssertionFailedError; -import org.apache.lucene.search.Scorable; -import org.elasticsearch.common.lucene.ScorerAware; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.painless.antlr.Walker; import org.elasticsearch.painless.lookup.PainlessLookup; import org.elasticsearch.painless.lookup.PainlessLookupBuilder; import org.elasticsearch.painless.spi.Whitelist; -import org.elasticsearch.script.ExecutableScript; import org.elasticsearch.script.ScriptContext; import org.elasticsearch.script.ScriptException; import org.elasticsearch.script.SearchScript; import org.elasticsearch.test.ESTestCase; import org.junit.Before; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; +import static org.elasticsearch.painless.PainlessExecuteAction.PainlessTestScript; import static org.elasticsearch.painless.node.SSource.MainMethodReserved; import static org.hamcrest.Matchers.hasSize; @@ -69,7 +68,7 @@ public abstract class ScriptTestCase extends ESTestCase { protected Map, List> scriptContexts() { Map, List> contexts = new HashMap<>(); contexts.put(SearchScript.CONTEXT, Whitelist.BASE_WHITELISTS); - contexts.put(ExecutableScript.CONTEXT, Whitelist.BASE_WHITELISTS); + contexts.put(PainlessTestScript.CONTEXT, Whitelist.BASE_WHITELISTS); return contexts; } @@ -87,11 +86,11 @@ public abstract class ScriptTestCase extends ESTestCase { public Object exec(String script, Map vars, boolean picky) { Map compilerSettings = new HashMap<>(); compilerSettings.put(CompilerSettings.INITIAL_CALL_SITE_DEPTH, random().nextBoolean() ? "0" : "10"); - return exec(script, vars, compilerSettings, null, picky); + return exec(script, vars, compilerSettings, picky); } /** Compiles and returns the result of {@code script} with access to {@code vars} and compile-time parameters */ - public Object exec(String script, Map vars, Map compileParams, Scorable scorer, boolean picky) { + public Object exec(String script, Map vars, Map compileParams, boolean picky) { // test for ambiguity errors before running the actual script if picky is true if (picky) { ScriptClassInfo scriptClassInfo = new ScriptClassInfo(PAINLESS_LOOKUP, GenericElasticsearchScript.class); @@ -99,15 +98,12 @@ public abstract class ScriptTestCase extends ESTestCase { pickySettings.setPicky(true); pickySettings.setRegexesEnabled(CompilerSettings.REGEX_ENABLED.get(scriptEngineSettings())); Walker.buildPainlessTree( - scriptClassInfo, new MainMethodReserved(), getTestName(), script, pickySettings, PAINLESS_LOOKUP, null); + scriptClassInfo, new MainMethodReserved(), getTestName(), script, pickySettings, PAINLESS_LOOKUP, null); } // test actual script execution - ExecutableScript.Factory factory = scriptEngine.compile(null, script, ExecutableScript.CONTEXT, compileParams); - ExecutableScript executableScript = factory.newInstance(vars); - if (scorer != null) { - ((ScorerAware)executableScript).setScorer(scorer); - } - return executableScript.run(); + PainlessTestScript.Factory factory = scriptEngine.compile(null, script, PainlessTestScript.CONTEXT, compileParams); + PainlessTestScript testScript = factory.newInstance(vars == null ? Collections.emptyMap() : vars); + return testScript.execute(); } /** diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/StringTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/StringTests.java index 55cce62b819..c73d6c2071a 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/StringTests.java +++ b/modules/lang-painless/src/test/java/org/elasticsearch/painless/StringTests.java @@ -233,7 +233,7 @@ public class StringTests extends ScriptTestCase { ctx.put("_id", "somerandomid"); params.put("ctx", ctx); - assertEquals("somerandomid.somerandomid", exec("ctx._id += '.' + ctx._id", params, false)); + assertEquals("somerandomid.somerandomid", exec("params.ctx._id += '.' + params.ctx._id", params, false)); assertEquals("somerandomid.somerandomid", exec("String x = 'somerandomid'; x += '.' + x")); assertEquals("somerandomid.somerandomid", exec("def x = 'somerandomid'; x += '.' + x")); } diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/WhenThingsGoWrongTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/WhenThingsGoWrongTests.java index 79d2fe0c53d..32d74d0837c 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/WhenThingsGoWrongTests.java +++ b/modules/lang-painless/src/test/java/org/elasticsearch/painless/WhenThingsGoWrongTests.java @@ -130,7 +130,7 @@ public class WhenThingsGoWrongTests extends ScriptTestCase { public void testBogusParameter() { IllegalArgumentException expected = expectThrows(IllegalArgumentException.class, () -> { - exec("return 5;", null, Collections.singletonMap("bogusParameterKey", "bogusParameterValue"), null, true); + exec("return 5;", null, Collections.singletonMap("bogusParameterKey", "bogusParameterValue"), true); }); assertTrue(expected.getMessage().contains("Unrecognized compile-time parameter")); } @@ -253,7 +253,7 @@ public class WhenThingsGoWrongTests extends ScriptTestCase { public void testRCurlyNotDelim() { IllegalArgumentException e = expectScriptThrows(IllegalArgumentException.class, () -> { // We don't want PICKY here so we get the normal error message - exec("def i = 1} return 1", emptyMap(), emptyMap(), null, false); + exec("def i = 1} return 1", emptyMap(), emptyMap(), false); }); assertEquals("unexpected token ['}'] was expecting one of [{, ';'}].", e.getMessage()); } @@ -285,7 +285,7 @@ public class WhenThingsGoWrongTests extends ScriptTestCase { public void testCanNotOverrideRegexEnabled() { IllegalArgumentException e = expectThrows(IllegalArgumentException.class, - () -> exec("", null, singletonMap(CompilerSettings.REGEX_ENABLED.getKey(), "true"), null, false)); + () -> exec("", null, singletonMap(CompilerSettings.REGEX_ENABLED.getKey(), "true"), false)); assertEquals("[painless.regex.enabled] can only be set on node startup.", e.getMessage()); } diff --git a/modules/reindex/src/test/java/org/elasticsearch/index/reindex/AbstractAsyncBulkByScrollActionScriptTestCase.java b/modules/reindex/src/test/java/org/elasticsearch/index/reindex/AbstractAsyncBulkByScrollActionScriptTestCase.java index 68cc6bc39e0..e838b89eb38 100644 --- a/modules/reindex/src/test/java/org/elasticsearch/index/reindex/AbstractAsyncBulkByScrollActionScriptTestCase.java +++ b/modules/reindex/src/test/java/org/elasticsearch/index/reindex/AbstractAsyncBulkByScrollActionScriptTestCase.java @@ -20,11 +20,10 @@ package org.elasticsearch.index.reindex; import org.elasticsearch.action.ActionRequest; -import org.elasticsearch.index.reindex.AbstractAsyncBulkByScrollAction.OpType; -import org.elasticsearch.index.reindex.AbstractAsyncBulkByScrollAction.RequestWrapper; import org.elasticsearch.action.delete.DeleteRequest; import org.elasticsearch.action.index.IndexRequest; -import org.elasticsearch.script.ExecutableScript; +import org.elasticsearch.index.reindex.AbstractAsyncBulkByScrollAction.OpType; +import org.elasticsearch.index.reindex.AbstractAsyncBulkByScrollAction.RequestWrapper; import org.elasticsearch.script.ScriptService; import org.elasticsearch.script.UpdateScript; import org.junit.Before; @@ -62,8 +61,6 @@ public abstract class AbstractAsyncBulkByScrollActionScriptTestCase< scriptBody.accept(ctx); } };; - ExecutableScript simpleExecutableScript = new SimpleExecutableScript(scriptBody); - when(scriptService.compile(any(), eq(ExecutableScript.CONTEXT))).thenReturn(params -> simpleExecutableScript); when(scriptService.compile(any(), eq(UpdateScript.CONTEXT))).thenReturn(factory); AbstractAsyncBulkByScrollAction action = action(scriptService, request().setScript(mockScript(""))); RequestWrapper result = action.buildScriptApplier().apply(AbstractAsyncBulkByScrollAction.wrap(index), doc); diff --git a/modules/reindex/src/test/java/org/elasticsearch/index/reindex/SimpleExecutableScript.java b/modules/reindex/src/test/java/org/elasticsearch/index/reindex/SimpleExecutableScript.java deleted file mode 100644 index be661282df7..00000000000 --- a/modules/reindex/src/test/java/org/elasticsearch/index/reindex/SimpleExecutableScript.java +++ /dev/null @@ -1,51 +0,0 @@ -/* - * 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.index.reindex; - -import org.elasticsearch.script.ExecutableScript; - -import java.util.Map; -import java.util.function.Consumer; - - -public class SimpleExecutableScript implements ExecutableScript { - private final Consumer> script; - private Map ctx; - - public SimpleExecutableScript(Consumer> script) { - this.script = script; - } - - @Override - public Object run() { - script.accept(ctx); - return null; - } - - @Override - @SuppressWarnings("unchecked") - public void setNextVar(String name, Object value) { - if ("ctx".equals(name)) { - ctx = (Map) value; - } else { - throw new IllegalArgumentException("Unsupported var [" + name + "]"); - } - } -} diff --git a/server/src/main/java/org/elasticsearch/script/ExecutableScript.java b/server/src/main/java/org/elasticsearch/script/ExecutableScript.java deleted file mode 100644 index d0d8020371b..00000000000 --- a/server/src/main/java/org/elasticsearch/script/ExecutableScript.java +++ /dev/null @@ -1,49 +0,0 @@ -/* - * 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; - -/** - * An executable script, can't be used concurrently. - */ -public interface ExecutableScript { - - /** - * Sets a runtime script parameter. - *

- * Note that this method may be slow, involving put() and get() calls - * to a hashmap or similar. - * @param name parameter name - * @param value parameter value - */ - void setNextVar(String name, Object value); - - /** - * Executes the script. - */ - Object run(); - - interface Factory { - ExecutableScript newInstance(Map params); - } - - ScriptContext CONTEXT = new ScriptContext<>("executable", Factory.class); -} diff --git a/server/src/main/java/org/elasticsearch/script/ScriptModule.java b/server/src/main/java/org/elasticsearch/script/ScriptModule.java index 968bc143ba8..d98ed62f602 100644 --- a/server/src/main/java/org/elasticsearch/script/ScriptModule.java +++ b/server/src/main/java/org/elasticsearch/script/ScriptModule.java @@ -45,7 +45,6 @@ public class ScriptModule { ScoreScript.CONTEXT, SearchScript.SCRIPT_SORT_CONTEXT, TermsSetQueryScript.CONTEXT, - ExecutableScript.CONTEXT, UpdateScript.CONTEXT, BucketAggregationScript.CONTEXT, BucketAggregationSelectorScript.CONTEXT, diff --git a/server/src/main/java/org/elasticsearch/script/SearchScript.java b/server/src/main/java/org/elasticsearch/script/SearchScript.java index cdf5c98ec62..693d0dd8a3d 100644 --- a/server/src/main/java/org/elasticsearch/script/SearchScript.java +++ b/server/src/main/java/org/elasticsearch/script/SearchScript.java @@ -41,7 +41,7 @@ import java.util.Map; *

  • Call one of the {@code run} methods: {@link #run()}, {@link #runAsDouble()}, or {@link #runAsLong()}
    • * */ -public abstract class SearchScript implements ScorerAware, ExecutableScript { +public abstract class SearchScript implements ScorerAware { /** The generic runtime parameters for the script. */ private final Map params; @@ -112,7 +112,6 @@ public abstract class SearchScript implements ScorerAware, ExecutableScript { setNextVar("_value", value); } - @Override public void setNextVar(String field, Object value) {} /** Return the result as a long. This is used by aggregation scripts over long fields. */ @@ -120,7 +119,6 @@ public abstract class SearchScript implements ScorerAware, ExecutableScript { throw new UnsupportedOperationException("runAsLong is not implemented"); } - @Override public Object run() { return runAsDouble(); } 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 e81c9f95ba2..76031dca84f 100644 --- a/test/framework/src/main/java/org/elasticsearch/script/MockScriptEngine.java +++ b/test/framework/src/main/java/org/elasticsearch/script/MockScriptEngine.java @@ -105,16 +105,14 @@ public class MockScriptEngine implements ScriptEngine { } }; return context.factoryClazz.cast(factory); - } else if (context.instanceClazz.equals(ExecutableScript.class)) { - ExecutableScript.Factory factory = mockCompiled::createExecutableScript; - return context.factoryClazz.cast(factory); } else if (context.instanceClazz.equals(IngestScript.class)) { - IngestScript.Factory factory = parameters -> new IngestScript(parameters) { - @Override - public void execute(Map ctx) { - script.apply(ctx); - } - }; + IngestScript.Factory factory = vars -> + new IngestScript(vars) { + @Override + public void execute(Map ctx) { + script.apply(ctx); + } + }; return context.factoryClazz.cast(factory); } else if (context.instanceClazz.equals(IngestConditionalScript.class)) { IngestConditionalScript.Factory factory = parameters -> new IngestConditionalScript(parameters) { @@ -167,16 +165,17 @@ public class MockScriptEngine implements ScriptEngine { 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 - // this is just a shim to act as an executable script just as before - ExecutableScript execScript = mockCompiled.createExecutableScript(vars); - return new TemplateScript(vars) { - @Override - public String execute() { - return (String) execScript.run(); - } - }; + Map varsWithParams = new HashMap<>(); + if (vars != null) { + varsWithParams.put("params", vars); + } + return new TemplateScript(vars) { + @Override + public String execute() { + return (String) script.apply(varsWithParams); + } }; + }; return context.factoryClazz.cast(factory); } else if (context.instanceClazz.equals(FilterScript.class)) { FilterScript.Factory factory = mockCompiled::createFilterScript; @@ -231,19 +230,6 @@ public class MockScriptEngine implements ScriptEngine { return name; } - public ExecutableScript createExecutableScript(Map params) { - Map context = new HashMap<>(); - if (options != null) { - context.putAll(options); // TODO: remove this once scripts know to look for options under options key - context.put("options", options); - } - if (params != null) { - context.putAll(params); // TODO: remove this once scripts know to look for params under params key - context.put("params", params); - } - return new MockExecutableScript(context, script != null ? script : ctx -> source); - } - public SearchScript.LeafFactory createSearchScript(Map params, SearchLookup lookup) { Map context = new HashMap<>(); if (options != null) { @@ -294,27 +280,6 @@ public class MockScriptEngine implements ScriptEngine { } } - public class MockExecutableScript implements ExecutableScript { - - private final Function, Object> script; - private final Map vars; - - public MockExecutableScript(Map vars, Function, Object> script) { - this.vars = vars; - this.script = script; - } - - @Override - public void setNextVar(String name, Object value) { - vars.put(name, value); - } - - @Override - public Object run() { - return script.apply(vars); - } - } - public class MockSearchScript implements SearchScript.LeafFactory { private final Function, Object> script; diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/monitoring/test/MockPainlessScriptEngine.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/monitoring/test/MockPainlessScriptEngine.java index 6c0e5e156ca..5aeeb47db7e 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/monitoring/test/MockPainlessScriptEngine.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/monitoring/test/MockPainlessScriptEngine.java @@ -6,7 +6,6 @@ package org.elasticsearch.xpack.core.monitoring.test; import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.script.ExecutableScript; import org.elasticsearch.script.MockScriptEngine; import org.elasticsearch.script.MockScriptPlugin; import org.elasticsearch.script.ScriptContext; @@ -45,9 +44,7 @@ public class MockPainlessScriptEngine extends MockScriptEngine { @Override public T compile(String name, String script, ScriptContext context, Map options) { MockCompiledScript compiledScript = new MockCompiledScript(name, options, script, p -> script); - if (context.instanceClazz.equals(ExecutableScript.class)) { - return context.factoryClazz.cast((ExecutableScript.Factory) compiledScript::createExecutableScript); - } else if (context.instanceClazz.equals(SearchScript.class)) { + if (context.instanceClazz.equals(SearchScript.class)) { return context.factoryClazz.cast((SearchScript.Factory) compiledScript::createSearchScript); } throw new IllegalArgumentException("mock painless does not know how to handle context [" + context.name + "]");