Scripting: Add char position of script errors (#51069) (#51266)

Add the character position of a scripting error to error responses. The contents of the `position` field are experimental and subject to change. Currently, `offset` refers to the character location where the error was encountered, `start` and `end` define a range of characters that contain the error. eg. ``` { "error": { "root_cause": [ { "type": "script_exception", "reason": "runtime error", "script_stack": [ "y = x;", " ^---- HERE" ], "script": "def x = new ArrayList(); Map y = x;", "lang": "painless", "position": { "offset": 33, "start": 29, "end": 35 } } ``` Refs: #50993
elastic · Jan 21, 2020 · 41c15b4 · 41c15b4
1 parent 5d4bbdc
commit 41c15b4
Show file tree

Hide file tree

Showing 7 changed files with 149 additions and 21 deletions.
diff --git a/docs/painless/painless-guide/painless-debugging.asciidoc b/docs/painless/painless-guide/painless-debugging.asciidoc
@@ -50,7 +50,7 @@ Which shows that the class of `doc.first` is
    "status": 400
 }
 ---------------------------------------------------------
-// TESTRESPONSE[s/\.\.\./"script_stack": $body.error.script_stack, "script": $body.error.script, "lang": $body.error.lang, "caused_by": $body.error.caused_by, "root_cause": $body.error.root_cause, "reason": $body.error.reason/]
+// TESTRESPONSE[s/\.\.\./"script_stack": $body.error.script_stack, "script": $body.error.script, "lang": $body.error.lang, "position": $body.error.position, "caused_by": $body.error.caused_by, "root_cause": $body.error.root_cause, "reason": $body.error.reason/]
 
 You can use the same trick to see that `_source` is a `LinkedHashMap`
 in the `_update` API:
@@ -85,7 +85,7 @@ The response looks like:
 }
 ---------------------------------------------------------
 // TESTRESPONSE[s/"root_cause": \.\.\./"root_cause": $body.error.root_cause/]
-// TESTRESPONSE[s/\.\.\./"script_stack": $body.error.caused_by.script_stack, "script": $body.error.caused_by.script, "lang": $body.error.caused_by.lang, "caused_by": $body.error.caused_by.caused_by, "reason": $body.error.caused_by.reason/]
+// TESTRESPONSE[s/\.\.\./"script_stack": $body.error.caused_by.script_stack, "script": $body.error.caused_by.script, "lang": $body.error.caused_by.lang, "position": $body.error.caused_by.position, "caused_by": $body.error.caused_by.caused_by, "reason": $body.error.caused_by.reason/]
 // TESTRESPONSE[s/"to_string": ".+"/"to_string": $body.error.caused_by.to_string/]
 
 Once you have a class you can go to <<painless-api-reference>> to see a list of

diff --git a/docs/reference/scripting/using.asciidoc b/docs/reference/scripting/using.asciidoc
@@ -241,3 +241,14 @@ NOTE: The size of scripts is limited to 65,535 bytes. This can be
 changed by setting `script.max_size_in_bytes` setting to increase that soft
 limit, but if scripts are really large then a
 <<modules-scripting-engine,native script engine>> should be considered.
+
+[float]
+[[modules-scripting-errors]]
+=== Script errors
+Elasticsearch returns error details when there is a compliation or runtime
+exception.  The contents of this response are useful for tracking down the
+problem.
+
+experimental[]
+
+The contents of `position` are experimental and subject to change.
diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessScript.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/PainlessScript.java
@@ -58,14 +58,15 @@ public interface PainlessScript {
     default ScriptException convertToScriptException(Throwable t, Map<String, List<String>> extraMetadata) {
         // create a script stack: this is just the script portion
         List<String> scriptStack = new ArrayList<>();
+        ScriptException.Position pos = null;
         for (StackTraceElement element : t.getStackTrace()) {
             if (WriterConstants.CLASS_NAME.equals(element.getClassName())) {
                 // found the script portion
-                int offset = element.getLineNumber();
-                if (offset == -1) {
+                int originalOffset = element.getLineNumber();
+                if (originalOffset == -1) {
                     scriptStack.add("<<< unknown portion of script >>>");
                 } else {
-                    offset--; // offset is 1 based, line numbers must be!
+                    int offset = --originalOffset; // offset is 1 based, line numbers must be!
                     int startOffset = getPreviousStatement(offset);
                     if (startOffset == -1) {
                         assert false; // should never happen unless we hit exc in ctor prologue...
@@ -84,14 +85,15 @@ default ScriptException convertToScriptException(Throwable t, Map<String, List<S
                     }
                     pointer.append("^---- HERE");
                     scriptStack.add(pointer.toString());
+                    pos = new ScriptException.Position(originalOffset, startOffset, endOffset);
                 }
                 break;
             // but filter our own internal stacks (e.g. indy bootstrap)
             } else if (!shouldFilter(element)) {
                 scriptStack.add(element.toString());
             }
         }
-        ScriptException scriptException = new ScriptException("runtime error", t, scriptStack, getName(), PainlessScriptEngine.NAME);
+        ScriptException scriptException = new ScriptException("runtime error", t, scriptStack, getName(), PainlessScriptEngine.NAME, pos);
         for (Map.Entry<String, List<String>> entry : extraMetadata.entrySet()) {
             scriptException.addMetadata(entry.getKey(), entry.getValue());
         }

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
@@ -439,14 +439,15 @@ private CompilerSettings buildCompilerSettings(Map<String, String> params) {
     private ScriptException convertToScriptException(String scriptSource, Throwable t) {
         // create a script stack: this is just the script portion
         List<String> scriptStack = new ArrayList<>();
+        ScriptException.Position pos = null;
         for (StackTraceElement element : t.getStackTrace()) {
             if (WriterConstants.CLASS_NAME.equals(element.getClassName())) {
                 // found the script portion
-                int offset = element.getLineNumber();
-                if (offset == -1) {
+                int originalOffset = element.getLineNumber();
+                if (originalOffset == -1) {
                     scriptStack.add("<<< unknown portion of script >>>");
                 } else {
-                    offset--; // offset is 1 based, line numbers must be!
+                    int offset = --originalOffset; // offset is 1 based, line numbers must be!
                     int startOffset = getPreviousStatement(offset);
                     int endOffset = getNextStatement(scriptSource, offset);
                     StringBuilder snippet = new StringBuilder();
@@ -467,11 +468,12 @@ private ScriptException convertToScriptException(String scriptSource, Throwable
                     }
                     pointer.append("^---- HERE");
                     scriptStack.add(pointer.toString());
+                    pos = new ScriptException.Position(originalOffset, startOffset, endOffset);
                 }
                 break;
             }
         }
-        throw new ScriptException("compile error", t, scriptStack, scriptSource, PainlessScriptEngine.NAME);
+        throw new ScriptException("compile error", t, scriptStack, scriptSource, PainlessScriptEngine.NAME, pos);
     }
 
     // very simple heuristic: +/- 25 chars. can be improved later.

diff --git a/modules/lang-painless/src/test/resources/rest-api-spec/test/painless/17_update_error.yml b/modules/lang-painless/src/test/resources/rest-api-spec/test/painless/17_update_error.yml
@@ -0,0 +1,15 @@
+---
+"Script errors contain position":
+  - skip:
+      version: " - 7.7.0"
+      reason: "position introduced in 7.7"
+
+  - do:
+      catch: /compile error/
+      put_script:
+        id: "1"
+        context: "score"
+        body: { "script": {"lang": "painless", "source": "_score * foo bar + doc['myParent.weight'].value"} }
+  - match: { error.root_cause.0.position.offset: 13 }
+  - match: { error.root_cause.0.position.start: 0 }
+  - match: { error.root_cause.0.position.end: 38 }
diff --git a/server/src/main/java/org/elasticsearch/script/ScriptException.java b/server/src/main/java/org/elasticsearch/script/ScriptException.java
@@ -1,6 +1,7 @@
 package org.elasticsearch.script;
 
 import org.elasticsearch.ElasticsearchException;
+import org.elasticsearch.Version;
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
@@ -51,6 +52,7 @@ public class ScriptException extends ElasticsearchException {
     private final List<String> scriptStack;
     private final String script;
     private final String lang;
+    private final Position pos;
 
     /**
      * Create a new ScriptException.
@@ -61,13 +63,22 @@ public class ScriptException extends ElasticsearchException {
      *                Must not be {@code null}, but can be empty (though this should be avoided if possible).
      * @param script Identifier for which script failed. Must not be {@code null}.
      * @param lang Scripting engine language, such as "painless". Must not be {@code null}.
-     * @throws NullPointerException if any parameters are {@code null}.
+     * @param pos Position of error within script, may be {@code null}.
+     * @throws NullPointerException if any parameters are {@code null} except pos.
      */
-    public ScriptException(String message, Throwable cause, List<String> scriptStack, String script, String lang) {
+    public ScriptException(String message, Throwable cause, List<String> scriptStack, String script, String lang, Position pos) {
         super(Objects.requireNonNull(message), Objects.requireNonNull(cause));
         this.scriptStack = Collections.unmodifiableList(Objects.requireNonNull(scriptStack));
         this.script = Objects.requireNonNull(script);
         this.lang = Objects.requireNonNull(lang);
+        this.pos = pos;
+    }
+
+    /**
+     * Create a new ScriptException with null Position.
+     */
+    public ScriptException(String message, Throwable cause, List<String> scriptStack, String script, String lang) {
+        this(message, cause, scriptStack, script, lang, null);
     }
 
     /**
@@ -78,6 +89,11 @@ public ScriptException(StreamInput in) throws IOException {
         scriptStack = Arrays.asList(in.readStringArray());
         script = in.readString();
         lang = in.readString();
+        if (in.getVersion().onOrAfter(Version.V_7_7_0) && in.readBoolean()) {
+            pos = new Position(in);
+        } else {
+            pos = null;
+        }
     }
 
     @Override
@@ -86,13 +102,24 @@ public void writeTo(StreamOutput out) throws IOException {
         out.writeStringArray(scriptStack.toArray(new String[0]));
         out.writeString(script);
         out.writeString(lang);
+        if (out.getVersion().onOrAfter(Version.V_7_7_0)) {
+            if (pos == null) {
+                out.writeBoolean(false);
+            } else {
+                out.writeBoolean(true);
+                pos.writeTo(out);
+            }
+        }
     }
 
     @Override
     protected void metadataToXContent(XContentBuilder builder, Params params) throws IOException {
         builder.field("script_stack", scriptStack);
         builder.field("script", script);
         builder.field("lang", lang);
+        if (pos != null) {
+            pos.toXContent(builder, params);
+        }
     }
 
     /**
@@ -119,6 +146,13 @@ public String getLang() {
         return lang;
     }
 
+    /**
+     * Returns the position of the error.
+     */
+    public Position getPos() {
+        return pos;
+    }
+
     /**
      * Returns a JSON version of this exception for debugging.
      */
@@ -138,4 +172,51 @@ public String toJsonString() {
     public RestStatus status() {
         return RestStatus.BAD_REQUEST;
     }
+
+    public static class Position {
+        public final int offset;
+        public final int start;
+        public final int end;
+
+        public Position(int offset, int start, int end) {
+            this.offset = offset;
+            this.start = start;
+            this.end = end;
+        }
+
+        Position(StreamInput in) throws IOException {
+            offset = in.readInt();
+            start = in.readInt();
+            end = in.readInt();
+        }
+
+        void writeTo(StreamOutput out) throws IOException {
+            out.writeInt(offset);
+            out.writeInt(start);
+            out.writeInt(end);
+        }
+
+        void toXContent(XContentBuilder builder, Params params) throws IOException {
+            builder.startObject("position");
+            builder.field("offset", offset);
+            builder.field("start", start);
+            builder.field("end", end);
+            builder.endObject();
+        }
+
+        @Override
+        public boolean equals(Object o) {
+            if (this == o)
+                return true;
+            if (o == null || getClass() != o.getClass())
+                return false;
+            Position position = (Position) o;
+            return offset == position.offset && start == position.start && end == position.end;
+        }
+
+        @Override
+        public int hashCode() {
+            return Objects.hash(offset, start, end);
+        }
+    }
 }
diff --git a/server/src/test/java/org/elasticsearch/script/ScriptExceptionTests.java b/server/src/test/java/org/elasticsearch/script/ScriptExceptionTests.java
@@ -35,38 +35,55 @@
 
 /** Simple tests for {@link ScriptException} */
 public class ScriptExceptionTests extends ESTestCase {
-    
+
     /** ensure we can round trip in serialization */
     public void testRoundTrip() throws IOException {
-        ScriptException e = new ScriptException("messageData", new Exception("causeData"), Arrays.asList("stack1", "stack2"), 
+        ScriptException e = new ScriptException("messageData", new Exception("causeData"), Arrays.asList("stack1", "stack2"),
                                                 "sourceData", "langData");
-        
+
         ByteArrayOutputStream bytes = new ByteArrayOutputStream();
         StreamOutput output = new DataOutputStreamOutput(new DataOutputStream(bytes));
         e.writeTo(output);
         output.close();
-        
+
         StreamInput input = new InputStreamStreamInput(new ByteArrayInputStream(bytes.toByteArray()));
         ScriptException e2 = new ScriptException(input);
         input.close();
-        
+
         assertEquals(e.getMessage(), e2.getMessage());
         assertEquals(e.getScriptStack(), e2.getScriptStack());
         assertEquals(e.getScript(), e2.getScript());
         assertEquals(e.getLang(), e2.getLang());
+        assertNull(e.getPos());
+
+        // Ensure non-null position also works
+        e = new ScriptException(e.getMessage(), e.getCause(), e.getScriptStack(), e.getScript(), e.getLang(),
+            new ScriptException.Position(1, 0, 2));
+        bytes = new ByteArrayOutputStream();
+        output = new DataOutputStreamOutput(new DataOutputStream(bytes));
+        e.writeTo(output);
+        output.close();
+
+        input = new InputStreamStreamInput(new ByteArrayInputStream(bytes.toByteArray()));
+        e2 = new ScriptException(input);
+        input.close();
+        assertEquals(e.getPos(), e2.getPos());
     }
-    
+
     /** Test that our elements are present in the json output */
     public void testJsonOutput() {
-        ScriptException e = new ScriptException("messageData", new Exception("causeData"), Arrays.asList("stack1", "stack2"), 
-                                                "sourceData", "langData");
+        ScriptException e = new ScriptException("messageData", new Exception("causeData"), Arrays.asList("stack1", "stack2"),
+                                                "sourceData", "langData", new ScriptException.Position(2, 1, 3));
         String json = e.toJsonString();
         assertTrue(json.contains(e.getMessage()));
         assertTrue(json.contains(e.getCause().getMessage()));
         assertTrue(json.contains("stack1"));
         assertTrue(json.contains("stack2"));
         assertTrue(json.contains(e.getScript()));
         assertTrue(json.contains(e.getLang()));
+        assertTrue(json.contains("1"));
+        assertTrue(json.contains("2"));
+        assertTrue(json.contains("3"));
     }
 
     /** ensure the script stack is immutable */
@@ -78,7 +95,7 @@ public void testImmutableStack() {
         });
     }
 
-    /** ensure no parameters can be null */
+    /** ensure no parameters can be null except pos*/
     public void testNoLeniency() {
         expectThrows(NullPointerException.class, () -> {
            new ScriptException(null, new Exception(), Collections.emptyList(), "a", "b");