Scripting: Add script name to error messages

Modified ScriptEngineService to pass in a CompiledScript object
to several methods 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.

closes elastic#6653
closes elastic#11449

src/main/java/org/elasticsearch/script/CompiledScript.java

@@ -24,19 +24,41 @@
  */
 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) {
+    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;
+    }
+
     /**
      * Method to get the language.
      * @return The language of the script to be executed.
@@ -52,4 +74,13 @@ public String lang() {
     public Object compiled() {
         return compiled;
     }
+
+    /**
+     * @return A string composed of type and name to describe the CompiledScript.
+     */
+    @Override
+    public String toString() {
+        return type + " script" + (name == null ? "" : (" [" + name + "]"));
+    }
 }
+;;

src/main/java/org/elasticsearch/script/NativeScriptEngineService.java

@@ -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 {
@@ -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();
     }
 

src/main/java/org/elasticsearch/script/ScriptEngineService.java

@@ -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);
 

src/main/java/org/elasticsearch/script/ScriptService.java

@@ -27,6 +27,7 @@
 import com.google.common.collect.ImmutableMap;
 
 import org.apache.lucene.util.IOUtils;
+import org.elasticsearch.ExceptionsHelper;
 import org.elasticsearch.action.ActionListener;
 import org.elasticsearch.action.delete.DeleteRequest;
 import org.elasticsearch.action.delete.DeleteResponse;
@@ -105,7 +106,7 @@ public class ScriptService extends AbstractComponent implements Closeable {
     private Client client = null;
 
     /**
-     * @deprecated Use {@link ScriptField} instead. This should be removed in
+     * @deprecated Use {@link Script.ScriptField} instead. This should be removed in
      *             2.0
      */
     public static final ParseField SCRIPT_LANG = new ParseField("lang","script_lang");
@@ -263,22 +264,44 @@ public CompiledScript compileInternal(Script script) {
             return compiled;
         }
 
+        // Name will remain null for inline scripts.
+        String name = null;
         String code = script.getScript();
 
         if (script.getType() == ScriptType.INDEXED) {
             final IndexedScript indexedScript = new IndexedScript(lang, script.getScript());
+            name = indexedScript.id;
             code = getScriptFromIndex(indexedScript.lang, indexedScript.id);
             cacheKey = newCacheKey(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));
+            try {
+                compiled = new CompiledScript(script.getType(), name, lang, scriptEngineService.compile(code));
+            } catch (Exception e) {
+                String message = "Compilation error with " + script.getType() + " script ";
+                if (name != null) {
+                    message += "[" + name + "] ";
+                }
+                message += "using [" + lang + "]: " + ExceptionsHelper.detailedMessage(e);
+                throw new ScriptException(message, e);
+            }
             //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);
         }
+
+        // An indexed or inline script may be mistaken for the other since the cache key relies on
+        // only the code of the script. Two indexed scripts may also have the same code, but different
+        // names. If this is case, the name and type parameters must be modified accordingly.
+        // Then cache the most recent version used.
+        if (compiled.type() != script.getType() || name != null && !name.equals(compiled.name())) {
+            compiled = new CompiledScript(script.getType(), name, compiled.lang(), compiled.compiled());
+            cache.put(cacheKey, compiled);
+        }
+
         return compiled;
     }
 
@@ -320,8 +343,7 @@ private void validate(BytesReference scriptBytes, String scriptLang) {
             parser.nextToken();
             Template template = TemplateQueryParser.parse(scriptLang, parser, "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
+                //Just try and compile it without adding the script to the cache, so the name can be stored properly later
                 try {
                     //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.
@@ -406,15 +428,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) {
@@ -501,7 +523,7 @@ public void onFileInit(Path file) {
                             try(InputStreamReader reader = new InputStreamReader(Files.newInputStream(file), Charsets.UTF_8)) {
                                 String script = Streams.copyToString(reader);
                                 CacheKey cacheKey = newCacheKey(engineService, scriptNameExt.v1());
-                                staticCache.put(cacheKey, new CompiledScript(engineService.types()[0], engineService.compile(script)));
+                                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());

src/main/java/org/elasticsearch/script/expression/ExpressionScript.java

@@ -26,8 +26,11 @@
 import org.apache.lucene.queries.function.FunctionValues;
 import org.apache.lucene.queries.function.ValueSource;
 import org.apache.lucene.search.Scorer;
+import org.elasticsearch.ExceptionsHelper;
 import org.elasticsearch.common.lucene.Lucene;
+import org.elasticsearch.script.CompiledScript;
 import org.elasticsearch.script.LeafSearchScript;
+import org.elasticsearch.script.ScriptException;
 import org.elasticsearch.script.SearchScript;
 
 import java.io.IOException;
@@ -40,17 +43,17 @@
  */
 class ExpressionScript implements SearchScript {
 
-    final Expression expression;
+    final CompiledScript compiledScript;
     final SimpleBindings bindings;
     final ValueSource source;
     final ReplaceableConstValueSource specialValue; // _value
     Scorer scorer;
     int docid;
 
-    ExpressionScript(Expression e, SimpleBindings b, ReplaceableConstValueSource v) {
-        expression = e;
+    ExpressionScript(CompiledScript c, SimpleBindings b, ReplaceableConstValueSource v) {
+        compiledScript = c;
         bindings = b;
-        source = expression.getValueSource(bindings);
+        source = ((Expression)compiledScript.compiled()).getValueSource(bindings);
         specialValue = v;
     }
 
@@ -61,7 +64,11 @@ public LeafSearchScript getLeafSearchScript(final LeafReaderContext leaf) throws
             FunctionValues values = source.getValues(Collections.singletonMap("scorer", Lucene.illegalScorer("Scores are not available in the current context")), leaf);
 
             double evaluate() {
-                return values.doubleVal(docid);
+                try {
+                    return values.doubleVal(docid);
+                } catch (Exception e) {
+                    throw new ScriptException("Error with " + compiledScript + ": " + ExceptionsHelper.detailedMessage(e), e);
+                }
             }
 
             @Override
@@ -91,7 +98,7 @@ public void setScorer(Scorer s) {
                     // We have a new binding for the scorer so we need to reset the values
                     values = source.getValues(Collections.singletonMap("scorer", scorer), leaf);
                 } catch (IOException e) {
-                    throw new IllegalStateException("Can't get values", e);
+                    throw new IllegalStateException("Error with " + compiledScript + ": can't get values", e);
                 }
             }
 
@@ -109,7 +116,7 @@ public void setNextVar(String name, Object value) {
                 if (value instanceof Number) {
                     specialValue.setValue(((Number)value).doubleValue());
                 } else {
-                    throw new ExpressionScriptExecutionException("Cannot use expression with text variable");
+                    throw new ExpressionScriptExecutionException("Error with " + compiledScript + ": cannot use expression with text variable");
                 }
             }
         };