diff --git a/larky/src/main/java/com/verygood/security/larky/jsr223/LarkyCompiledScript.java b/larky/src/main/java/com/verygood/security/larky/jsr223/LarkyCompiledScript.java index 4a21a26a3..960debd23 100644 --- a/larky/src/main/java/com/verygood/security/larky/jsr223/LarkyCompiledScript.java +++ b/larky/src/main/java/com/verygood/security/larky/jsr223/LarkyCompiledScript.java @@ -1,6 +1,9 @@ package com.verygood.security.larky.jsr223; import com.google.common.io.CharStreams; +import java.io.IOException; +import java.io.Reader; +import java.util.Map; import com.verygood.security.larky.parser.DefaultLarkyInterpreter; import com.verygood.security.larky.parser.InMemMapBackedStarFile; @@ -9,16 +12,13 @@ import com.verygood.security.larky.parser.StarFile; import net.starlark.java.eval.EvalException; - -import java.io.IOException; -import java.io.Reader; -import java.util.Map; +import net.starlark.java.eval.Starlark; +import net.starlark.java.eval.StarlarkEvalWrapper; import javax.script.Bindings; import javax.script.CompiledScript; import javax.script.ScriptContext; import javax.script.ScriptEngine; -import javax.script.ScriptException; public class LarkyCompiledScript extends CompiledScript { @@ -44,20 +44,21 @@ public ScriptEngine getEngine() { } @Override - public Object eval(ScriptContext context) throws ScriptException { - try(Reader reader = context.getReader()) { - final StarFile script = InMemMapBackedStarFile.createStarFile(DEFAULT_SCRIPT_NAME, CharStreams.toString(reader)); - Bindings globalBindings = context.getBindings(ScriptContext.GLOBAL_SCOPE); - Bindings engineBindings = context.getBindings(ScriptContext.ENGINE_SCOPE); + public Object eval(ScriptContext context) throws LarkyEvaluationScriptException { + ParsedStarFile result; + Bindings globalBindings = context.getBindings(ScriptContext.GLOBAL_SCOPE); + Bindings engineBindings = context.getBindings(ScriptContext.ENGINE_SCOPE); + try (Reader reader = context.getReader()) { + final StarFile script = InMemMapBackedStarFile.createStarFile(DEFAULT_SCRIPT_NAME, CharStreams.toString(reader)); final DefaultLarkyInterpreter larkyInterpreter = new DefaultLarkyInterpreter(LARKY_MODE, globalBindings, engineBindings); - - ParsedStarFile result = larkyInterpreter.evaluate(script, context.getWriter()); - setBindingsValue(globalBindings, engineBindings, result.getGlobals()); - return result; - } catch (IOException | EvalException e) { - throw new ScriptException(e); + result = larkyInterpreter.evaluate(script, context.getWriter()); + } catch (IOException | StarlarkEvalWrapper.Exc.RuntimeEvalException | Starlark.UncheckedEvalException | + EvalException e) { + throw LarkyEvaluationScriptException.of(e); } + setBindingsValue(globalBindings, engineBindings, result.getGlobals()); + return result; } diff --git a/larky/src/main/java/com/verygood/security/larky/jsr223/LarkyEvaluationScriptException.java b/larky/src/main/java/com/verygood/security/larky/jsr223/LarkyEvaluationScriptException.java new file mode 100644 index 000000000..9962f026d --- /dev/null +++ b/larky/src/main/java/com/verygood/security/larky/jsr223/LarkyEvaluationScriptException.java @@ -0,0 +1,93 @@ +package com.verygood.security.larky.jsr223; + +import net.starlark.java.eval.EvalException; +import net.starlark.java.eval.Starlark; +import net.starlark.java.eval.StarlarkEvalWrapper; +import net.starlark.java.syntax.Location; + +import org.jetbrains.annotations.Contract; +import org.jetbrains.annotations.NotNull; + +import javax.script.ScriptException; + +public class LarkyEvaluationScriptException extends ScriptException { + + private final @NotNull String excToString; + + private LarkyEvaluationScriptException(Exception e) { + super(e); // do not construct this exception directly. + excToString = super.getMessage(); + } + + private LarkyEvaluationScriptException(EvalException evalException) { + this(evalException, null, -1, -1); // do not construct this exception directly. + } + + private LarkyEvaluationScriptException(@NotNull EvalException message, String fileName, int lineNumber, int columnNumber) { + super(message.getMessage(), fileName, lineNumber, columnNumber); // do not construct this exception directly. + excToString = super.getMessage() + System.lineSeparator() + message.getMessageWithStack(); + } + + @Contract("_ -> new") + public static @NotNull LarkyEvaluationScriptException of(@NotNull Exception e) { + if (e instanceof StarlarkEvalWrapper.Exc.RuntimeEvalException + || e instanceof Starlark.UncheckedEvalException) { + return onUnchecked(e); + } else if (e instanceof EvalException) { + return onEvalException((EvalException) e); + } else { + return new LarkyEvaluationScriptException(e); + } + } + + /** + * Both {@link Starlark.UncheckedEvalException} and {@link StarlarkEvalWrapper.Exc.RuntimeEvalException} may not have + * stacktraces as they are derivative of {@link RuntimeException}. + * + * As a result, {@link net.starlark.java.eval.StarlarkThread}'s stacktrace might be buried as the second frame instead + * of the first one. + */ + @Contract("_ -> new") + private static @NotNull LarkyEvaluationScriptException onUnchecked(final @NotNull Exception e) { + if ((e.getCause() instanceof EvalException)) { + final EvalException cause = (EvalException) e.getCause(); + final LarkyEvaluationScriptException scriptException = onEvalException(cause); +// scriptException.fillInLarkyStackTrace(cause); + return scriptException; + } + return new LarkyEvaluationScriptException(e); + } + + @Contract("_ -> new") + private static @NotNull LarkyEvaluationScriptException onEvalException(final @NotNull EvalException larkyException) { + final LarkyEvaluationScriptException exception; + final Location errorLoc = StarlarkEvalWrapper.Exc.getErrorLocation(larkyException); + if (errorLoc != null) { + exception = new LarkyEvaluationScriptException( + larkyException, + errorLoc.file(), + errorLoc.line(), + errorLoc.column() + ); + } else { + exception = new LarkyEvaluationScriptException(larkyException); + } + return exception; + } + + @Override + public String toString() { + return excToString; + } + + /** + * Helper method that helps fill in the stack trace from an external {@link EvalException}. This will just invoke + * {@link StarlarkEvalWrapper.Exc#fillInLarkyStackTrace(EvalException, Throwable)} with the current exception. + * + * @param larkyException - The {@link EvalException} that contains the Larky stacktrace + */ + public void fillInLarkyStackTrace(@NotNull EvalException larkyException) { + StarlarkEvalWrapper.Exc.fillInLarkyStackTrace(larkyException, this); + } + +} diff --git a/larky/src/main/java/net/starlark/java/eval/StarlarkEvalWrapper.java b/larky/src/main/java/net/starlark/java/eval/StarlarkEvalWrapper.java index ad76572aa..fe2ce31e7 100644 --- a/larky/src/main/java/net/starlark/java/eval/StarlarkEvalWrapper.java +++ b/larky/src/main/java/net/starlark/java/eval/StarlarkEvalWrapper.java @@ -2,6 +2,10 @@ import static com.google.common.base.Strings.isNullOrEmpty; +import com.google.common.collect.ImmutableList; + +import net.starlark.java.syntax.Location; + import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -63,6 +67,49 @@ public static Object getAttrFromMethodAnnotations( } public interface Exc { + + /** + * Given a {@link EvalException}, will return, if applicable, the metadata surrounding the location of the exception + * for the evaluated script. + * + * @param larkyException - The {@link EvalException} that contains the Larky stacktrace + * @return a {@link Location} detailing the filename, line, and row where the error occurred. + */ + static @Nullable Location getErrorLocation(@NotNull final EvalException larkyException) { + + final ImmutableList callStack = larkyException.getCallStack(); + final int n = callStack.size(); + if (callStack.isEmpty()) { + return null; + } + final StarlarkThread.CallStackEntry leaf = callStack.get(n - 1); + return leaf.location; + } + + /** + * Sets the given {@link Throwable}'s stack trace to a Java-style version of {@link StarlarkThread#getCallStack}. + * This is useful to expose the underlying larky callstack to the caller for a simpler way to identify Larky + * errors. + * + * @param larkyException - The {@link EvalException} that contains the Larky stacktrace + * @param throwable - The {@link Throwable} class to hoist the Larky stacktrace above the Java exception + * callstack. + */ + static void fillInLarkyStackTrace(@NotNull EvalException larkyException, @NotNull Throwable throwable) { + final ImmutableList callStack = larkyException.getCallStack(); + final int callStackSize = callStack.size(); + StackTraceElement[] trace = new StackTraceElement[callStackSize]; + for (int i = 0; i < callStackSize; i++) { + StarlarkThread.CallStackEntry frame = callStack.get(i); + trace[trace.length - i - 1] = new StackTraceElement( + /*declaringClass=*/ "", + frame.name, + frame.location.file(), + frame.location.line()); + } + throwable.setStackTrace(trace); + } + static String createUncheckedEvalMessage(Throwable cause, @Nullable StarlarkThread thread) { String msg = cause.getClass().getSimpleName() + " thrown during Starlark evaluation"; String context = null; diff --git a/larky/src/test/java/com/verygood/security/larky/jsr223/LarkyCompiledScriptTest.java b/larky/src/test/java/com/verygood/security/larky/jsr223/LarkyCompiledScriptTest.java index 0bec87e28..cf2a83805 100644 --- a/larky/src/test/java/com/verygood/security/larky/jsr223/LarkyCompiledScriptTest.java +++ b/larky/src/test/java/com/verygood/security/larky/jsr223/LarkyCompiledScriptTest.java @@ -1,12 +1,17 @@ package com.verygood.security.larky.jsr223; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + +import com.google.common.base.Throwables; +import java.io.StringWriter; import com.verygood.security.larky.parser.ParsedStarFile; import org.junit.Test; -import java.io.StringWriter; import javax.script.Bindings; import javax.script.ScriptContext; import javax.script.ScriptException; @@ -35,8 +40,8 @@ public void testEval() throws ScriptException { "" + "def main():", " return 'Hello World'", - "", - "output = main()" + "", + "output = main()" ); LarkyCompiledScript instance = (LarkyCompiledScript) engine.compile(script); @@ -45,6 +50,91 @@ public void testEval() throws ScriptException { assertEquals(expResult, result.getGlobalEnvironmentVariable("output", String.class)); } + @Test + public void testEval_withUncheckedException() { + LarkyScriptEngineFactory factory = new LarkyScriptEngineFactory(); + LarkyScriptEngine engine = (LarkyScriptEngine) factory.getScriptEngine(); + String script = String.join("\n", + "" + + "def process(input, ctx):", + " foo = 'bar'", + " # expecting error below!", + " input.body = foo", + " return input", + "", + "output = process(None, {})" + ); + + LarkyCompiledScript instance = (LarkyCompiledScript) engine.compile(script); + + ParsedStarFile result = null; + LarkyEvaluationScriptException scriptException = null; + try { + result = (ParsedStarFile) instance.eval(); + fail("should not have gotten here"); + } catch (LarkyEvaluationScriptException e) { + scriptException = e; + } catch (Exception e) { + fail("Unexpected exception thrown!"); + } + assertNotNull(scriptException); + assertTrue(scriptException.getMessage().contains("cannot set .body field of NoneType value")); + assertTrue( + Throwables.getStackTraceAsString(scriptException) + .contains( + /*substring*/ + "Traceback (most recent call last):\n" + + "\tFile \"larky.star\", line 7, column 17, in \n" + + "\tFile \"larky.star\", line 4, column 10, in process\n" + + "Error: cannot set .body field of NoneType value") + ); + } + + @Test + public void testEval_withCheckedException() { + + class OperationException extends Exception { + + public OperationException(Exception e) { + super(e); + } + } + + LarkyScriptEngineFactory factory = new LarkyScriptEngineFactory(); + LarkyScriptEngine engine = (LarkyScriptEngine) factory.getScriptEngine(); + String script = String.join("\n", + "" + + "def process(input, ctx):", + " fail('boom')", + " return input", + "", + "output = process(None, {})" + ); + + LarkyCompiledScript instance = (LarkyCompiledScript) engine.compile(script); + + OperationException operationException = null; + try { + try { + instance.eval(); + } catch (ScriptException e) { + throw new OperationException(e); + } + } catch (OperationException e) { + operationException = e; + } + assertNotNull(operationException); + assertTrue( + operationException.getMessage() + .contains( + /*substring*/ + "Traceback (most recent call last):\n" + + "\tFile \"larky.star\", line 5, column 17, in \n" + + "\tFile \"larky.star\", line 2, column 9, in process\n" + + "Error in fail: boom") + ); + } + @Test public void testEval_context() throws Exception { LarkyScriptEngineFactory factory = new LarkyScriptEngineFactory();