Skip to content

Fix function dispatch failure error messages to be properly formatted #946

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
copybara-service wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix function dispatch failure error messages to be properly formatted #946

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions runtime/src/main/java/dev/cel/runtime/planner/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,10 +199,10 @@ java_library(
name = "eval_zero_arity",
srcs = ["EvalZeroArity.java"],
deps = [
":eval_helpers",
":execution_frame",
":planned_interpretable",
"//common/values",
"//common/values:cel_value",
"//runtime:evaluation_exception",
"//runtime:interpretable",
"//runtime:resolved_overload",
Expand All @@ -217,7 +217,6 @@ java_library(
":execution_frame",
":planned_interpretable",
"//common/values",
"//common/values:cel_value",
"//runtime:evaluation_exception",
"//runtime:interpretable",
"//runtime:resolved_overload",
Expand All @@ -232,7 +231,6 @@ java_library(
":execution_frame",
":planned_interpretable",
"//common/values",
"//common/values:cel_value",
"//runtime:evaluation_exception",
"//runtime:interpretable",
"//runtime:resolved_overload",
Expand All @@ -248,7 +246,6 @@ java_library(
":planned_interpretable",
"//common/exceptions:overload_not_found",
"//common/values",
"//common/values:cel_value",
"//runtime:evaluation_exception",
"//runtime:interpretable",
"//runtime:resolved_overload",
Expand Down Expand Up @@ -377,7 +374,11 @@ java_library(
"//common:error_codes",
"//common/exceptions:runtime_exception",
"//common/values",
"//common/values:cel_value",
"//runtime:evaluation_exception",
"//runtime:interpretable",
"//runtime:resolved_overload",
"@maven//:com_google_guava_guava",
],
)

Expand Down
26 changes: 26 additions & 0 deletions runtime/src/main/java/dev/cel/runtime/planner/EvalHelpers.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,14 @@

package dev.cel.runtime.planner;

import com.google.common.base.Joiner;
import dev.cel.common.CelErrorCode;
import dev.cel.common.exceptions.CelRuntimeException;
import dev.cel.common.values.CelValue;
import dev.cel.common.values.CelValueConverter;
import dev.cel.common.values.ErrorValue;
import dev.cel.runtime.CelEvaluationException;
import dev.cel.runtime.CelResolvedOverload;
import dev.cel.runtime.GlobalResolver;

final class EvalHelpers {
Expand Down Expand Up @@ -51,5 +56,26 @@ static Object evalStrictly(
}
}

static Object dispatch(CelResolvedOverload overload, CelValueConverter valueConverter, Object[] args) throws CelEvaluationException {
try {
Object result = overload.getDefinition().apply(args);
Object runtimeValue = valueConverter.toRuntimeValue(result);
if (runtimeValue instanceof CelValue) {
return valueConverter.unwrap((CelValue) runtimeValue);
}

return runtimeValue;
} catch (CelRuntimeException e) {
// Function dispatch failure that's already been handled -- just propagate.
throw e;
} catch (RuntimeException e) {
// Unexpected function dispatch failure.
throw new IllegalArgumentException(String.format(
"Function '%s' failed with arg(s) '%s'",
overload.getOverloadId(), Joiner.on(", ").join(args)),
e);
}
}

private EvalHelpers() {}
}
8 changes: 1 addition & 7 deletions runtime/src/main/java/dev/cel/runtime/planner/EvalLateBoundCall.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@

import com.google.common.collect.ImmutableList;
import dev.cel.common.exceptions.CelOverloadNotFoundException;
import dev.cel.common.values.CelValue;
import dev.cel.common.values.CelValueConverter;
import dev.cel.runtime.CelEvaluationException;
import dev.cel.runtime.CelResolvedOverload;
Expand Down Expand Up @@ -48,12 +47,7 @@ public Object eval(GlobalResolver resolver, ExecutionFrame frame) throws CelEval
.findOverload(functionName, overloadIds, argVals)
.orElseThrow(() -> new CelOverloadNotFoundException(functionName, overloadIds));

Object result = resolvedOverload.getDefinition().apply(argVals);
Object runtimeValue = celValueConverter.toRuntimeValue(result);
if (runtimeValue instanceof CelValue) {
return celValueConverter.unwrap((CelValue) runtimeValue);
}
return runtimeValue;
return EvalHelpers.dispatch(resolvedOverload, celValueConverter, argVals);
}

static EvalLateBoundCall create(
Expand Down
8 changes: 1 addition & 7 deletions runtime/src/main/java/dev/cel/runtime/planner/EvalUnary.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
import static dev.cel.runtime.planner.EvalHelpers.evalNonstrictly;
import static dev.cel.runtime.planner.EvalHelpers.evalStrictly;

import dev.cel.common.values.CelValue;
import dev.cel.common.values.CelValueConverter;
import dev.cel.runtime.CelEvaluationException;
import dev.cel.runtime.CelResolvedOverload;
Expand All @@ -37,12 +36,7 @@ public Object eval(GlobalResolver resolver, ExecutionFrame frame) throws CelEval
: evalNonstrictly(arg, resolver, frame);
Object[] arguments = new Object[] {argVal};

Object result = resolvedOverload.getDefinition().apply(arguments);
Object runtimeValue = celValueConverter.toRuntimeValue(result);
if (runtimeValue instanceof CelValue) {
return celValueConverter.unwrap((CelValue) runtimeValue);
}
return runtimeValue;
return EvalHelpers.dispatch(resolvedOverload, celValueConverter, arguments);
}

static EvalUnary create(
Expand Down
8 changes: 1 addition & 7 deletions runtime/src/main/java/dev/cel/runtime/planner/EvalVarArgsCall.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
import static dev.cel.runtime.planner.EvalHelpers.evalNonstrictly;
import static dev.cel.runtime.planner.EvalHelpers.evalStrictly;

import dev.cel.common.values.CelValue;
import dev.cel.common.values.CelValueConverter;
import dev.cel.runtime.CelEvaluationException;
import dev.cel.runtime.CelResolvedOverload;
Expand All @@ -43,12 +42,7 @@ public Object eval(GlobalResolver resolver, ExecutionFrame frame) throws CelEval
: evalNonstrictly(arg, resolver, frame);
}

Object result = resolvedOverload.getDefinition().apply(argVals);
Object runtimeValue = celValueConverter.toRuntimeValue(result);
if (runtimeValue instanceof CelValue) {
return celValueConverter.unwrap((CelValue) runtimeValue);
}
return runtimeValue;
return EvalHelpers.dispatch(resolvedOverload, celValueConverter, argVals);
}

static EvalVarArgsCall create(
Expand Down
8 changes: 1 addition & 7 deletions runtime/src/main/java/dev/cel/runtime/planner/EvalZeroArity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@

package dev.cel.runtime.planner;

import dev.cel.common.values.CelValue;
import dev.cel.common.values.CelValueConverter;
import dev.cel.runtime.CelEvaluationException;
import dev.cel.runtime.CelResolvedOverload;
Expand All @@ -28,12 +27,7 @@ final class EvalZeroArity extends PlannedInterpretable {

@Override
public Object eval(GlobalResolver resolver, ExecutionFrame frame) throws CelEvaluationException {
Object result = resolvedOverload.getDefinition().apply(EMPTY_ARRAY);
Object runtimeValue = celValueConverter.toRuntimeValue(result);
if (runtimeValue instanceof CelValue) {
return celValueConverter.unwrap((CelValue) runtimeValue);
}
return runtimeValue;
return EvalHelpers.dispatch(resolvedOverload, celValueConverter, EMPTY_ARRAY);
}

static EvalZeroArity create(
Expand Down
6 changes: 5 additions & 1 deletion runtime/src/main/java/dev/cel/runtime/planner/PlannedProgram.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,11 @@ private CelEvaluationException newCelEvaluationException(long exprId, Exception
} else if (e instanceof CelRuntimeException) {
builder = CelEvaluationExceptionBuilder.newBuilder((CelRuntimeException) e);
} else {
builder = CelEvaluationExceptionBuilder.newBuilder(e.getMessage()).setCause(e);
// Unhandled function dispatch failures wraps the original exception with a descriptive message
// (e.g: "Function foo failed with...")
// We need to unwrap the cause here to preserve the original exception message and its cause.
Throwable cause = e.getCause() != null ? e.getCause() : e;
builder = CelEvaluationExceptionBuilder.newBuilder(e.getMessage()).setCause(cause);
}

return builder.setMetadata(metadata(), exprId).build();
Expand Down
6 changes: 0 additions & 6 deletions runtime/src/test/java/dev/cel/runtime/PlannerInterpreterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,6 @@ public void typeComparisons() {
skipBaselineVerification();
}

@Override
public void optional_errors() {
// TODO: Fix error message for function dispatch failures
skipBaselineVerification();
}

@Override
public void jsonFieldNames() throws Exception {
// TODO: Support JSON field names for planner
Expand Down
6 changes: 4 additions & 2 deletions runtime/src/test/java/dev/cel/runtime/planner/ProgramPlannerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,10 +397,12 @@ public void plan_call_zeroArgs() throws Exception {
public void plan_call_throws() throws Exception {
CelAbstractSyntaxTree ast = compile("error()");
Program program = PLANNER.plan(ast);
String expectedOverloadId = isParseOnly ? "error" : "error_overload";

CelEvaluationException e = assertThrows(CelEvaluationException.class, program::eval);
assertThat(e).hasMessageThat().contains("evaluation error at <input>:5: Intentional error");
assertThat(e).hasCauseThat().isInstanceOf(IllegalArgumentException.class);
assertThat(e).hasMessageThat().contains("evaluation error at <input>:5: Function '" + expectedOverloadId + "' failed with arg(s) ''");
assertThat(e.getCause()).isInstanceOf(IllegalArgumentException.class);
assertThat(e.getCause().getMessage()).contains("Intentional error");
}

@Test
Expand Down
Loading