Skip to content

Commit

Permalink
Add script type and script name to error messages
Browse files Browse the repository at this point in the history 
Modified ScriptEngineService to pass in a CompiledScript object
with newly added name and type member variables.
This can in turn be used to give better scripting error messages
with the type of script used and the name of the script.

Required slight modifications to the caching mechanism.

Note that this does not enforce good behavior in that plugins will
have to write exceptions that also output the name of the script
in order to be effective. There was no way to wrap the script
methods in a try/catch block properly further up the chain because
many have script-like objects passed back that can be run at a
later time.

closes elastic#6653
closes elastic#11449
  • Loading branch information
@jdconrad
jdconrad committed Jul 11, 2015
1 parent 8790989 commit f46ba22
Show file tree
Hide file tree
Showing 21 changed files with 324 additions and 236 deletions.
36 changes: 33 additions & 3 deletions core/src/main/java/org/elasticsearch/script/CompiledScript.java
View file
Expand Up @@ -24,17 +24,39 @@
*/
public class CompiledScript {

private final ScriptService.ScriptType type;
private final String name;
private final String lang;
private final Object compiled;

/**
* Constructor for CompiledScript.
* @param type The type of script to be executed.
* @param name The name of the script to be executed.
* @param lang The language of the script to be executed.
* @param compiled The compiled script Object that is executable.
*/
public CompiledScript(String lang, Object compiled) {
this.lang = lang;
this.compiled = compiled;
public CompiledScript(ScriptService.ScriptType type, String name, String lang, Object compiled) {
this.type = type;
this.name = name;
this.lang = lang;
this.compiled = compiled;
}

/**
* Method to get the type of language.
* @return The type of language the script was compiled in.
*/
public ScriptService.ScriptType type() {
return type;
}

/**
* Method to get the name of the script.
* @return The name of the script to be executed.
*/
public String name() {
return name;
}

/**
Expand All @@ -52,4 +74,12 @@ public String lang() {
public Object compiled() {
return compiled;
}

/**
* @return A string composed of type, lang, and name to describe the CompiledScript.
*/
@Override
public String toString() {
return type + " script [" + name + "] using lang [" + lang + "]";
}
}
10 changes: 5 additions & 5 deletions core/src/main/java/org/elasticsearch/script/NativeScriptEngineService.java
View file
Expand Up @@ -71,14 +71,14 @@ public Object compile(String script) {
}

@Override
public ExecutableScript executable(Object compiledScript, @Nullable Map<String, Object> vars) {
NativeScriptFactory scriptFactory = (NativeScriptFactory) compiledScript;
public ExecutableScript executable(CompiledScript compiledScript, @Nullable Map<String, Object> vars) {
NativeScriptFactory scriptFactory = (NativeScriptFactory) compiledScript.compiled();
return scriptFactory.newScript(vars);
}

@Override
public SearchScript search(Object compiledScript, final SearchLookup lookup, @Nullable final Map<String, Object> vars) {
final NativeScriptFactory scriptFactory = (NativeScriptFactory) compiledScript;
public SearchScript search(CompiledScript compiledScript, final SearchLookup lookup, @Nullable final Map<String, Object> vars) {
final NativeScriptFactory scriptFactory = (NativeScriptFactory) compiledScript.compiled();
return new SearchScript() {
@Override
public LeafSearchScript getLeafSearchScript(LeafReaderContext context) throws IOException {
Expand All @@ -90,7 +90,7 @@ public LeafSearchScript getLeafSearchScript(LeafReaderContext context) throws IO
}

@Override
public Object execute(Object compiledScript, Map<String, Object> vars) {
public Object execute(CompiledScript compiledScript, Map<String, Object> vars) {
return executable(compiledScript, vars).run();
}

Expand Down
6 changes: 3 additions & 3 deletions core/src/main/java/org/elasticsearch/script/ScriptEngineService.java
View file
Expand Up @@ -38,11 +38,11 @@ public interface ScriptEngineService extends Closeable {

Object compile(String script);

ExecutableScript executable(Object compiledScript, @Nullable Map<String, Object> vars);
ExecutableScript executable(CompiledScript compiledScript, @Nullable Map<String, Object> vars);

SearchScript search(Object compiledScript, SearchLookup lookup, @Nullable Map<String, Object> vars);
SearchScript search(CompiledScript compiledScript, SearchLookup lookup, @Nullable Map<String, Object> vars);

Object execute(Object compiledScript, Map<String, Object> vars);
Object execute(CompiledScript compiledScript, Map<String, Object> vars);

Object unwrap(Object value);

Expand Down
83 changes: 50 additions & 33 deletions core/src/main/java/org/elasticsearch/script/ScriptService.java
View file
Expand Up @@ -249,50 +249,67 @@ public CompiledScript compile(Script script, ScriptContext scriptContext) {
}

/**
* Compiles a script straight-away, or returns the previously compiled and cached script, without checking if it can be executed based on settings.
* Compiles a script straight-away, or returns the previously compiled and cached script,
* without checking if it can be executed based on settings.
*/
public CompiledScript compileInternal(Script script) {
if (script == null) {
throw new IllegalArgumentException("The parameter script (Script) must not be null.");
}

String lang = script.getLang();
String lang = script.getLang() == null ? defaultLang : script.getLang();
ScriptType type = script.getType();
//script.getScript() could return either a name or code for a script,
//but we check for a file script name first and an indexed script name second
String name = script.getScript();

if (lang == null) {
lang = defaultLang;
}
if (logger.isTraceEnabled()) {
logger.trace("Compiling lang: [{}] type: [{}] script: {}", lang, script.getType(), script.getScript());
logger.trace("Compiling lang: [{}] type: [{}] script: {}", lang, type, name);
}

ScriptEngineService scriptEngineService = getScriptEngineServiceForLang(lang);
String cacheKey = getCacheKey(scriptEngineService, script.getScript());

if (script.getType() == ScriptType.FILE) {
CompiledScript compiled = staticCache.get(cacheKey); //On disk scripts will be loaded into the staticCache by the listener
if (compiled == null) {
throw new IllegalArgumentException("Unable to find on disk script " + script.getScript());
if (type == ScriptType.FILE) {
String cacheKey = getCacheKey(scriptEngineService, name, null);
//On disk scripts will be loaded into the staticCache by the listener
CompiledScript compiledScript = staticCache.get(cacheKey);

if (compiledScript == null) {
throw new IllegalArgumentException("Unable to find on disk file script [" + name + "] using lang [" + lang + "]");
}
return compiled;

return compiledScript;
}

//script.getScript() will be code if the script type is inline
String code = script.getScript();

if (script.getType() == ScriptType.INDEXED) {
final IndexedScript indexedScript = new IndexedScript(lang, script.getScript());
if (type == ScriptType.INDEXED) {
//The look up for an indexed script must be done every time in case
//the script has been updated in the index since the last look up.
final IndexedScript indexedScript = new IndexedScript(lang, name);
name = indexedScript.id;
code = getScriptFromIndex(indexedScript.lang, indexedScript.id);
cacheKey = getCacheKey(scriptEngineService, code);
}

CompiledScript compiled = cache.getIfPresent(cacheKey);
if (compiled == null) {
//Either an un-cached inline script or an indexed script
compiled = new CompiledScript(lang, scriptEngineService.compile(code));
//if the script type is inline the name will be the same as the code for identification in exceptions
String cacheKey = getCacheKey(scriptEngineService, type == ScriptType.INLINE ? null : name, code);
CompiledScript compiledScript = cache.getIfPresent(cacheKey);

if (compiledScript == null) {
//Either an un-cached inline script or indexed script
try {
compiledScript = new CompiledScript(type, name, lang, scriptEngineService.compile(code));
} catch (Exception exception) {
throw new ScriptException("Failed to compile " + type + " script [" + name + "] using lang [" + lang + "]", exception);
}

//Since the cache key is the script content itself we don't need to
//invalidate/check the cache if an indexed script changes.
cache.put(cacheKey, compiled);
cache.put(cacheKey, compiledScript);
}
return compiled;

return compiledScript;
}

public void queryScriptIndex(GetIndexedScriptRequest request, final ActionListener<GetResponse> listener) {
Expand Down Expand Up @@ -334,13 +351,13 @@ private void validate(BytesReference scriptBytes, String scriptLang) {
Template template = TemplateQueryParser.parse(scriptLang, parser, parseFieldMatcher, "params", "script", "template");
if (Strings.hasLength(template.getScript())) {
//Just try and compile it
//This will have the benefit of also adding the script to the cache if it compiles
try {
ScriptEngineService scriptEngineService = getScriptEngineServiceForLang(scriptLang);
//we don't know yet what the script will be used for, but if all of the operations for this lang with
//indexed scripts are disabled, it makes no sense to even compile it and cache it.
if (isAnyScriptContextEnabled(scriptLang, getScriptEngineServiceForLang(scriptLang), ScriptType.INDEXED)) {
CompiledScript compiledScript = compileInternal(template);
if (compiledScript == null) {
//indexed scripts are disabled, it makes no sense to even compile it.
if (isAnyScriptContextEnabled(scriptLang, scriptEngineService, ScriptType.INDEXED)) {
Object compiled = scriptEngineService.compile(template.getScript());
if (compiled == null) {
throw new IllegalArgumentException("Unable to parse [" + template.getScript() +
"] lang [" + scriptLang + "] (ScriptService.compile returned null)");
}
Expand Down Expand Up @@ -419,15 +436,15 @@ public ExecutableScript executable(Script script, ScriptContext scriptContext) {
* Executes a previously compiled script provided as an argument
*/
public ExecutableScript executable(CompiledScript compiledScript, Map<String, Object> vars) {
return getScriptEngineServiceForLang(compiledScript.lang()).executable(compiledScript.compiled(), vars);
return getScriptEngineServiceForLang(compiledScript.lang()).executable(compiledScript, vars);
}

/**
* Compiles (or retrieves from cache) and executes the provided search script
*/
public SearchScript search(SearchLookup lookup, Script script, ScriptContext scriptContext) {
CompiledScript compiledScript = compile(script, scriptContext);
return getScriptEngineServiceForLang(compiledScript.lang()).search(compiledScript.compiled(), lookup, script.getParams());
return getScriptEngineServiceForLang(compiledScript.lang()).search(compiledScript, lookup, script.getParams());
}

private boolean isAnyScriptContextEnabled(String lang, ScriptEngineService scriptEngineService, ScriptType scriptType) {
Expand Down Expand Up @@ -513,8 +530,8 @@ public void onFileInit(Path file) {
logger.info("compiling script file [{}]", file.toAbsolutePath());
try(InputStreamReader reader = new InputStreamReader(Files.newInputStream(file), Charsets.UTF_8)) {
String script = Streams.copyToString(reader);
String cacheKey = getCacheKey(engineService, scriptNameExt.v1());
staticCache.put(cacheKey, new CompiledScript(engineService.types()[0], engineService.compile(script)));
String cacheKey = getCacheKey(engineService, scriptNameExt.v1(), null);
staticCache.put(cacheKey, new CompiledScript(ScriptType.FILE, scriptNameExt.v1(), engineService.types()[0], engineService.compile(script)));
}
} else {
logger.warn("skipping compile of script file [{}] as all scripted operations are disabled for file scripts", file.toAbsolutePath());
Expand All @@ -538,7 +555,7 @@ public void onFileDeleted(Path file) {
ScriptEngineService engineService = getScriptEngineServiceForFileExt(scriptNameExt.v2());
assert engineService != null;
logger.info("removing script file [{}]", file.toAbsolutePath());
staticCache.remove(getCacheKey(engineService, scriptNameExt.v1()));
staticCache.remove(getCacheKey(engineService, scriptNameExt.v1(), null));
}
}

Expand Down Expand Up @@ -598,9 +615,9 @@ public String toString() {
}
}

private static String getCacheKey(ScriptEngineService scriptEngineService, String script) {
private static String getCacheKey(ScriptEngineService scriptEngineService, String name, String code) {
String lang = scriptEngineService.types()[0];
return lang + ":" + script;
return lang + ":" + (name != null ? ":" + name : "") + (code != null ? ":" + code : "");
}

private static class IndexedScript {
Expand Down
23 changes: 16 additions & 7 deletions core/src/main/java/org/elasticsearch/script/expression/ExpressionExecutableScript.java
View file
Expand Up @@ -20,6 +20,7 @@
package org.elasticsearch.script.expression;

import org.apache.lucene.expressions.Expression;
import org.elasticsearch.script.CompiledScript;
import org.elasticsearch.script.ExecutableScript;
import org.elasticsearch.script.ScriptException;

Expand All @@ -34,16 +35,18 @@ public class ExpressionExecutableScript implements ExecutableScript {

private final int NO_DOCUMENT = -1;

public final Expression expression;
public final CompiledScript compiledScript;
public final Map<String, ReplaceableConstFunctionValues> functionValuesMap;
public final ReplaceableConstFunctionValues[] functionValuesArray;

public ExpressionExecutableScript(Object compiledScript, Map<String, Object> vars) {
expression = (Expression)compiledScript;
public ExpressionExecutableScript(CompiledScript compiledScript, Map<String, Object> vars) {
this.compiledScript = compiledScript;
Expression expression = (Expression)this.compiledScript.compiled();
int functionValuesLength = expression.variables.length;

if (vars.size() != functionValuesLength) {
throw new ScriptException("The number of variables in an executable expression script [" +
throw new ScriptException("Error using " + compiledScript + ". " +
"The number of variables in an executable expression script [" +
functionValuesLength + "] must match the number of variables in the variable map" +
" [" + vars.size() + "].");
}
Expand All @@ -69,17 +72,23 @@ public void setNextVar(String name, Object value) {
double doubleValue = ((Number)value).doubleValue();
functionValuesMap.get(name).setValue(doubleValue);
} else {
throw new ScriptException("Executable expressions scripts can only process numbers." +
throw new ScriptException("Error using " + compiledScript + ". " +
"Executable expressions scripts can only process numbers." +
" The variable [" + name + "] is not a number.");
}
} else {
throw new ScriptException("The variable [" + name + "] does not exist in the executable expressions script.");
throw new ScriptException("Error using " + compiledScript + ". " +
"The variable [" + name + "] does not exist in the executable expressions script.");
}
}

@Override
public Object run() {
return expression.evaluate(NO_DOCUMENT, functionValuesArray);
try {
return ((Expression) compiledScript.compiled()).evaluate(NO_DOCUMENT, functionValuesArray);
} catch (Exception exception) {
throw new ScriptException("Error evaluating " + compiledScript, exception);
}
}

@Override
Expand Down

0 comments on commit f46ba22

Please sign in to comment.