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 c4093be5dd7..2bb3605c2da 100644 --- a/core/src/main/java/org/elasticsearch/index/query/QueryShardContext.java +++ b/core/src/main/java/org/elasticsearch/index/query/QueryShardContext.java @@ -329,7 +329,7 @@ public class QueryShardContext extends QueryRewriteContext { * Compiles (or retrieves from cache) and binds the parameters to the * provided script */ - public final SearchScript getSearchScript(Script script, ScriptContext context) { + public final SearchScript getSearchScript(Script script, ScriptContext context) { failIfFrozen(); SearchScript.Compiled compiled = scriptService.compile(script, context); return compiled.newInstance(script.getParams(), lookup()); @@ -339,7 +339,7 @@ public class QueryShardContext extends QueryRewriteContext { * parameters are available. */ public final Function, SearchScript> getLazySearchScript( - Script script, ScriptContext context) { + Script script, ScriptContext context) { // TODO: this "lazy" binding can be removed once scripted metric aggs have their own contexts, which take _agg/_aggs as a parameter failIfFrozen(); SearchScript.Compiled compiled = scriptService.compile(script, context); @@ -350,7 +350,7 @@ public class QueryShardContext extends QueryRewriteContext { * Compiles (or retrieves from cache) and binds the parameters to the * provided script */ - public final ExecutableScript getExecutableScript(Script script, ScriptContext context) { + public final ExecutableScript getExecutableScript(Script script, ScriptContext context) { failIfFrozen(); ExecutableScript.Compiled compiled = scriptService.compile(script, context); return compiled.newInstance(script.getParams()); @@ -361,7 +361,7 @@ public class QueryShardContext extends QueryRewriteContext { * parameters are available. */ public final Function, ExecutableScript> getLazyExecutableScript( - Script script, ScriptContext context) { + Script script, ScriptContext context) { // TODO: this "lazy" binding can be removed once scripted metric aggs have their own contexts, which take _agg/_aggs as a parameter failIfFrozen(); ExecutableScript.Compiled compiled = scriptService.compile(script, context); diff --git a/core/src/main/java/org/elasticsearch/script/ScriptContext.java b/core/src/main/java/org/elasticsearch/script/ScriptContext.java index 03bcb376e5a..7209673fc95 100644 --- a/core/src/main/java/org/elasticsearch/script/ScriptContext.java +++ b/core/src/main/java/org/elasticsearch/script/ScriptContext.java @@ -19,6 +19,7 @@ package org.elasticsearch.script; +import java.lang.reflect.Method; import java.util.Collections; import java.util.HashMap; import java.util.Map; @@ -31,36 +32,36 @@ import java.util.Map; *

* There are two related classes which must be supplied to construct a {@link ScriptContext}. *

- * The InstanceType is a class, possibly stateful, that is an instance of a script. Instances of - * the InstanceType may be executed multiple times by a caller with different arguments. This - * class must have an abstract method named execute which {@link ScriptEngine} implementations - * will define. - *

* The CompiledType is a factory class for constructing instances of a script. The * {@link ScriptService} returns an instance of CompiledType when compiling a script. This class * must be stateless so it is cacheable by the {@link ScriptService}. It must have an abstract method - * named newInstance which returns InstanceType which {@link ScriptEngine} implementations + * named {@code newInstance} which {@link ScriptEngine} implementations will define. + *

+ * The InstanceType is a class returned by the {@code newInstance} method of the + * CompiledType. It is an instance of a script and may be stateful. Instances of + * the InstanceType may be executed multiple times by a caller with different arguments. This + * class must have an abstract method named {@code execute} which {@link ScriptEngine} implementations * will define. */ -public final class ScriptContext { +public final class ScriptContext { - public static final ScriptContext AGGS = - new ScriptContext<>("aggs", SearchScript.class, SearchScript.Compiled.class); - public static final ScriptContext SEARCH = - new ScriptContext<>("search", SearchScript.class, SearchScript.Compiled.class); + public static final ScriptContext AGGS = + new ScriptContext<>("aggs", SearchScript.Compiled.class); + public static final ScriptContext SEARCH = + new ScriptContext<>("search", SearchScript.Compiled.class); // TODO: remove this once each agg calling scripts has its own context - public static final ScriptContext AGGS_EXECUTABLE = - new ScriptContext<>("aggs_executable", ExecutableScript.class, ExecutableScript.Compiled.class); - public static final ScriptContext UPDATE = - new ScriptContext<>("update", ExecutableScript.class, ExecutableScript.Compiled.class); - public static final ScriptContext INGEST = - new ScriptContext<>("ingest", ExecutableScript.class, ExecutableScript.Compiled.class); - public static final ScriptContext EXECUTABLE = - new ScriptContext<>("executable", ExecutableScript.class, ExecutableScript.Compiled.class); + public static final ScriptContext AGGS_EXECUTABLE = + new ScriptContext<>("aggs_executable", ExecutableScript.Compiled.class); + public static final ScriptContext UPDATE = + new ScriptContext<>("update", ExecutableScript.Compiled.class); + public static final ScriptContext INGEST = + new ScriptContext<>("ingest", ExecutableScript.Compiled.class); + public static final ScriptContext EXECUTABLE = + new ScriptContext<>("executable", ExecutableScript.Compiled.class); - public static final Map> BUILTINS; + public static final Map> BUILTINS; static { - Map> builtins = new HashMap<>(); + Map> builtins = new HashMap<>(); builtins.put(AGGS.name, AGGS); builtins.put(SEARCH.name, SEARCH); builtins.put(AGGS_EXECUTABLE.name, AGGS_EXECUTABLE); @@ -73,16 +74,30 @@ public final class ScriptContext { /** A unique identifier for this context. */ public final String name; - /** A class that is an instance of a script. */ - public final Class instanceClazz; - /** A factory class for constructing instances of a script. */ public final Class compiledClazz; + /** A class that is an instance of a script. */ + public final Class instanceClazz; + /** Construct a context with the related instance and compiled classes. */ - public ScriptContext(String name, Class instanceClazz, Class compiledClazz) { + public ScriptContext(String name, Class compiledClazz) { this.name = name; - this.instanceClazz = instanceClazz; this.compiledClazz = compiledClazz; + Method newInstanceMethod = null; + for (Method method : compiledClazz.getMethods()) { + if (method.getName().equals("newInstance")) { + if (newInstanceMethod != null) { + throw new IllegalArgumentException("Cannot have multiple newInstance methods on CompiledType class [" + + compiledClazz.getName() + "] for script context [" + name + "]"); + } + newInstanceMethod = method; + } + } + if (newInstanceMethod == null) { + throw new IllegalArgumentException("Could not find method newInstance on CompiledType class [" + + compiledClazz.getName() + "] for script context [" + name + "]"); + } + instanceClazz = newInstanceMethod.getReturnType(); } } diff --git a/core/src/main/java/org/elasticsearch/script/ScriptModule.java b/core/src/main/java/org/elasticsearch/script/ScriptModule.java index 8fd17d4e1b4..5bcc9975b8c 100644 --- a/core/src/main/java/org/elasticsearch/script/ScriptModule.java +++ b/core/src/main/java/org/elasticsearch/script/ScriptModule.java @@ -36,7 +36,7 @@ public class ScriptModule { public ScriptModule(Settings settings, List scriptPlugins) { Map engines = new HashMap<>(); - Map> contexts = new HashMap<>(ScriptContext.BUILTINS); + Map> contexts = new HashMap<>(ScriptContext.BUILTINS); for (ScriptPlugin plugin : scriptPlugins) { for (ScriptContext context : plugin.getContexts()) { ScriptContext oldContext = contexts.put(context.name, context); diff --git a/core/src/main/java/org/elasticsearch/script/ScriptService.java b/core/src/main/java/org/elasticsearch/script/ScriptService.java index 3ce1c89d04e..06f55349dec 100644 --- a/core/src/main/java/org/elasticsearch/script/ScriptService.java +++ b/core/src/main/java/org/elasticsearch/script/ScriptService.java @@ -81,7 +81,7 @@ public class ScriptService extends AbstractComponent implements Closeable, Clust private final Set contextsAllowed; private final Map engines; - private final Map> contexts; + private final Map> contexts; private final Cache cache; @@ -94,7 +94,7 @@ public class ScriptService extends AbstractComponent implements Closeable, Clust private double scriptsPerMinCounter; private double compilesAllowedPerNano; - public ScriptService(Settings settings, Map engines, Map> contexts) { + public ScriptService(Settings settings, Map engines, Map> contexts) { super(settings); Objects.requireNonNull(settings); @@ -221,7 +221,7 @@ public class ScriptService extends AbstractComponent implements Closeable, Clust * * @return a compiled script which may be used to construct instances of a script for the given context */ - public CompiledType compile(Script script, ScriptContext context) { + public CompiledType compile(Script script, ScriptContext context) { Objects.requireNonNull(script); Objects.requireNonNull(context); @@ -338,7 +338,7 @@ public class ScriptService extends AbstractComponent implements Closeable, Clust } /** Compiles a template. Note this will be moved to a separate TemplateService in the future. */ - public CompiledTemplate compileTemplate(Script script, ScriptContext scriptContext) { + public CompiledTemplate compileTemplate(Script script, ScriptContext scriptContext) { ExecutableScript.Compiled compiledScript = compile(script, scriptContext); return params -> (String)compiledScript.newInstance(params).run(); } diff --git a/core/src/test/java/org/elasticsearch/script/ScriptContextTests.java b/core/src/test/java/org/elasticsearch/script/ScriptContextTests.java new file mode 100644 index 00000000000..cb502e68eec --- /dev/null +++ b/core/src/test/java/org/elasticsearch/script/ScriptContextTests.java @@ -0,0 +1,63 @@ +/* + * 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 org.elasticsearch.test.ESTestCase; + +public class ScriptContextTests extends ESTestCase { + + public interface TwoNewInstance { + String newInstance(int foo, int bar); + String newInstance(int foo); + } + + public interface MissingNewInstance { + String typoNewInstanceMethod(int foo); + } + + public interface DummyScript { + int execute(int foo); + + interface Compiled { + DummyScript newInstance(); + } + } + + public void testTwoNewInstanceMethods() { + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> + new ScriptContext<>("test", TwoNewInstance.class)); + assertEquals("Cannot have multiple newInstance methods on CompiledType class [" + + TwoNewInstance.class.getName() + "] for script context [test]", e.getMessage()); + } + + public void testMissingNewInstanceMethod() { + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> + new ScriptContext<>("test", MissingNewInstance.class)); + assertEquals("Could not find method newInstance on CompiledType class [" + + MissingNewInstance.class.getName() + "] for script context [test]", e.getMessage()); + } + + public void testInstanceTypeReflection() { + ScriptContext context = new ScriptContext<>("test", DummyScript.Compiled.class); + assertEquals("test", context.name); + assertEquals(DummyScript.class, context.instanceClazz); + assertEquals(DummyScript.Compiled.class, context.compiledClazz); + } +} diff --git a/core/src/test/java/org/elasticsearch/script/ScriptServiceTests.java b/core/src/test/java/org/elasticsearch/script/ScriptServiceTests.java index 79241781b02..94cba8e1029 100644 --- a/core/src/test/java/org/elasticsearch/script/ScriptServiceTests.java +++ b/core/src/test/java/org/elasticsearch/script/ScriptServiceTests.java @@ -51,7 +51,7 @@ public class ScriptServiceTests extends ESTestCase { private ScriptEngine scriptEngine; private Map engines; - private Map> contexts; + private Map> contexts; private ScriptService scriptService; private Settings baseSettings; @@ -213,7 +213,7 @@ public class ScriptServiceTests extends ESTestCase { builder.put(ScriptService.SCRIPT_CACHE_SIZE_SETTING.getKey(), 1); buildScriptService(builder.build()); Script script = new Script(ScriptType.INLINE, "test", "1+1", Collections.emptyMap()); - ScriptContext context = randomFrom(contexts.values()); + ScriptContext context = randomFrom(contexts.values()); scriptService.compile(script, context); scriptService.compile(script, context); assertEquals(1L, scriptService.stats().getCompilations());