Skip to content
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.

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

feat(python): add error differentiation to Python. #3772

Merged
merged 41 commits into from
Sep 28, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
5586bbc
framework errors
comcalvi Sep 1, 2022
db512f8
tmp
comcalvi Sep 7, 2022
d8510a7
playback tests pass
comcalvi Sep 8, 2022
770c120
remove comment
comcalvi Sep 8, 2022
8b81f62
js exception added
comcalvi Sep 9, 2022
3c0dc76
rename
comcalvi Sep 12, 2022
3ed3b27
rename JsiiE
comcalvi Sep 12, 2022
d6975d8
need to ensure that JsErrors are thrown correctly, the writeError() m…
comcalvi Sep 12, 2022
6ed003b
playback fix
comcalvi Sep 13, 2022
428c95e
force js error from kernel host...pretty sure this is the wrong appro…
comcalvi Sep 13, 2022
8dbb87f
working userland errors
comcalvi Sep 13, 2022
3486573
remove old commment
comcalvi Sep 13, 2022
42a4f31
remove commented code
comcalvi Sep 13, 2022
a32e01d
test fix
comcalvi Sep 14, 2022
f116ddb
removed old comment
comcalvi Sep 14, 2022
feecb62
updated expected exception type to JsiiException
comcalvi Sep 14, 2022
da967df
updated type to name
comcalvi Sep 14, 2022
855085d
updated comment
comcalvi Sep 15, 2022
53aa68d
updated comment
comcalvi Sep 15, 2022
3fc61f8
final comment update
comcalvi Sep 15, 2022
dc7562c
rename JsError to JSError
comcalvi Sep 15, 2022
fb8a93f
have to delete the file to change the capitalization...why????
comcalvi Sep 15, 2022
a468d90
final rename
comcalvi Sep 15, 2022
373cae2
Made JsiiException the base error class in Java. JsiiError now repres…
comcalvi Sep 19, 2022
636516d
comment update
comcalvi Sep 19, 2022
fa5398e
review comments
comcalvi Sep 20, 2022
7ccc170
use RuntimeError instead of Exception
comcalvi Sep 21, 2022
0ec3270
need JS error passing...
comcalvi Sep 21, 2022
8ada609
tmp
comcalvi Sep 21, 2022
ea450bb
merge from main; ensure that the kernel has the new error types.
comcalvi Sep 21, 2022
220d903
some py tests are failing...not clear why yet
comcalvi Sep 22, 2022
acd0a55
changed the type of some exceptions from JsiiFault to RuntimeError in
comcalvi Sep 22, 2022
2dee5e2
fixed Java bugs. Leaving print statements to investigate final bug in…
comcalvi Sep 24, 2022
d8be24a
added name to complete response in java and kernel
comcalvi Sep 26, 2022
1da9987
logging removed
comcalvi Sep 26, 2022
6d3a097
remove last print
comcalvi Sep 26, 2022
699f025
change formatting of ternary operator in JavaRuntime
comcalvi Sep 26, 2022
6d825a0
remove old import
comcalvi Sep 26, 2022
20857e2
review comments
comcalvi Sep 27, 2022
bf39336
formatting
comcalvi Sep 27, 2022
4258bd6
Merge branch 'main' into py-errors
mergify[bot] Sep 28, 2022
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
Original file line number Diff line number Diff line change
Expand Up @@ -333,9 +333,12 @@ public void exceptions() {
assertEquals(23, calc3.getValue());
boolean thrown = false;
try {
calc3.add(10);
calc3.add(10);
} catch (JsiiException e) {
// We expect a RuntimeException that is NOT a JsiiException.
throw e;
} catch (RuntimeException e) {
thrown = true;
thrown = true;
}
assertTrue(thrown);
calc3.setMaxValue(40);
Expand Down Expand Up @@ -450,6 +453,8 @@ public java.lang.Number overrideMe(java.lang.Number mult) {
boolean thrown = false;
try {
obj.callMe();
} catch (JsiiException e) {
throw e;
} catch (RuntimeException e) {
assertTrue(e.getMessage().contains( "Thrown by native code"));
thrown = true;
Expand Down Expand Up @@ -519,6 +524,8 @@ public String getTheProperty() {
boolean thrown = false;
try {
so.retrieveValueOfTheProperty();
} catch (JsiiException e) {
throw e;
} catch (RuntimeException e) {
assertTrue(e.getMessage().contains("Oh no, this is bad"));
thrown = true;
Expand All @@ -537,6 +544,8 @@ public void setTheProperty(String value) {
boolean thrown = false;
try {
so.modifyValueOfTheProperty("Hii");
} catch (JsiiException e) {
throw e;
} catch (RuntimeException e) {
assertTrue(e.getMessage().contains("Exception from overloaded setter"));
thrown = true;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ public void asyncMethodOverrides() {
assertEquals(obj.getObjId(), JsiiObjectRef.parse(first.getInvoke().getObjref()).getObjId());

// now complete the callback with some override value
client.completeCallback(first, null, toSandbox(999));
client.completeCallback(first, null, null, toSandbox(999));

// end the async invocation, but now we expect the value to be different since we override the method.
JsonNode result = client.endAsyncMethod(promise);
Expand Down Expand Up @@ -146,19 +146,55 @@ public void asyncMethodOverridesThrow() {
assertEquals(obj.getObjId(), JsiiObjectRef.parse(first.getInvoke().getObjref()).getObjId());

// now complete the callback with an error
client.completeCallback(first, "Hello, Error", null);
client.completeCallback(first, "Hello, Error", null, null);

// end the async invocation, but now we expect the value to be different since we override the method.
boolean thrown = false;
try {
client.endAsyncMethod(promise);
} catch (JsiiException e) {
} catch (RuntimeException e) {
assertEquals(RuntimeException.class, e.getClass());
assertTrue(e.getMessage().contains("Hello, Error"));
thrown = true;
}
assertTrue(thrown);
}

@Test
public void asyncMethodOverridesThrowWithFault() {
JsiiObjectRef obj = client.createObject("jsii-calc.AsyncVirtualMethods", Arrays.asList(), methodOverride("overrideMe", "myCookie"), Arrays.asList());

// begin will return a promise
JsiiPromise promise = client.beginAsyncMethod(obj, "callMe", toSandboxArray());
assertFalse(promise.getPromiseId().isEmpty());

// now we expect to see a callback to "overrideMe" in the pending callbacks queue

List<Callback> callbacks = client.pendingCallbacks();

assertEquals(1, callbacks.size());

Callback first = callbacks.get(0);
assertEquals("overrideMe", first.getInvoke().getMethod());
assertEquals("myCookie", first.getCookie());
assertEquals(1, first.getInvoke().getArgs().size());
assertEquals(JsiiObjectMapper.valueToTree(10), first.getInvoke().getArgs().get(0));
assertEquals(obj.getObjId(), JsiiObjectRef.parse(first.getInvoke().getObjref()).getObjId());

// now complete the callback with an error
client.completeCallback(first, "Hello, Fault", "@jsii/kernel.Fault", null);

// end the async invocation, but now we expect the value to be different since we override the method.
boolean thrown = false;
try {
client.endAsyncMethod(promise);
} catch (JsiiException e) {
assertTrue(e.getMessage().contains("Hello, Fault"));
thrown = true;
}
assertTrue(thrown);
}

@Test
public void syncVirtualMethods() {
JsiiObjectRef obj = client.createObject("jsii-calc.SyncVirtualMethods", Arrays.asList(), methodOverride("virtualMethod","myCookie"), Arrays.asList());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -243,12 +243,14 @@ public List<Callback> pendingCallbacks() {
* Completes a callback.
* @param callback The callback to complete.
* @param error Error information (or null).
* @param name Error type (or null).
* @param result Result (or null).
*/
public void completeCallback(final Callback callback, final String error, final JsonNode result) {
public void completeCallback(final Callback callback, final String error, final String name, final JsonNode result) {
ObjectNode req = makeRequest("complete");
req.put("cbid", callback.getCbid());
req.put("err", error);
req.put("name", name);
req.set("result", result);

this.runtime.requestResponse(req);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,15 @@ private Object invokeMethod(final Object obj, final Method method, final Object.
throw e;
}
} catch (InvocationTargetException e) {
throw new JsiiError(e.getTargetException());
if (e.getTargetException() instanceof JsiiError){
throw (JsiiError)(e.getTargetException());
} else if (e.getTargetException() instanceof RuntimeException) {
// can rethrow without wrapping here
throw (RuntimeException)(e.getTargetException());
} else {
// Can't just throw a checked error without wrapping it :(
throw new RuntimeException(e.getTargetException());
}
} catch (IllegalAccessException e) {
throw new JsiiError(e);
} finally {
Expand All @@ -502,9 +510,12 @@ private Object invokeMethod(final Object obj, final Method method, final Object.
private void processCallback(final Callback callback) {
try {
JsonNode result = handleCallback(callback);
this.getClient().completeCallback(callback, null, result);
} catch (JsiiError e) {
this.getClient().completeCallback(callback, e.getMessage(), null);
this.getClient().completeCallback(callback, null, null, result);
} catch (Exception e) {
String name = e instanceof JsiiException
? JsiiException.Type.JSII_FAULT.toString()
: JsiiException.Type.RUNTIME_ERROR.toString();
this.getClient().completeCallback(callback, e.getMessage(), name, null);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ public abstract class JsiiException extends RuntimeException {

static enum Type {
JSII_FAULT("@jsii/kernel.Fault"),
RUNTIME_EXCEPTION("@jsii/kernel.RuntimeException");
RUNTIME_ERROR("@jsii/kernel.RuntimeError");

private final String errorType;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ private JsonNode processErrorResponse(final JsonNode resp) {
errorMessage += "\n" + resp.get("stack").asText();
}

if (errorName.equals(JsiiException.Type.RUNTIME_EXCEPTION.toString())) {
if (errorName.equals(JsiiException.Type.RUNTIME_ERROR.toString())) {
throw new RuntimeException(errorMessage);
}

Expand All @@ -146,6 +146,7 @@ private JsonNode processCallbackResponse(final JsonNode resp) {

JsonNode result = null;
String error = null;
String name = null;
try {
result = this.callbackHandler.handleCallback(callback);
} catch (Exception e) {
Expand All @@ -154,12 +155,17 @@ private JsonNode processCallbackResponse(final JsonNode resp) {
} else {
error = e.getMessage();
}

name = e instanceof JsiiError
? JsiiException.Type.JSII_FAULT.toString()
: JsiiException.Type.RUNTIME_ERROR.toString();
}

ObjectNode completeResponse = JsonNodeFactory.instance.objectNode();
completeResponse.put("cbid", callback.getCbid());
if (error != null) {
completeResponse.put("err", error);
completeResponse.put("name", name);
}
if (result != null) {
completeResponse.set("result", result);
Expand Down
1 change: 1 addition & 0 deletions packages/@jsii/kernel/src/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ export interface CallbacksResponse {
export interface CompleteRequest {
readonly cbid: string;
readonly err?: string;
readonly name?: string;
readonly result?: any;
}

Expand Down
18 changes: 14 additions & 4 deletions packages/@jsii/kernel/src/kernel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -470,9 +470,15 @@ export class Kernel {
try {
result = await promise;
this._debug('promise result:', result);
} catch (e) {
} catch (e: any) {
this._debug('promise error:', e);
throw new JsiiFault((e as any).message);
if (e.name === JsiiErrorType.JSII_FAULT) {
throw new JsiiFault(e.message);
}

// default to RuntimeError, since non-kernel errors may not
// have their `name` field defined
throw new RuntimeError(e);
}

return {
Expand Down Expand Up @@ -505,7 +511,7 @@ export class Kernel {
}

public complete(req: api.CompleteRequest): api.CompleteResponse {
const { cbid, err, result } = req;
const { cbid, err, result, name } = req;

this._debug('complete', cbid, err, result);

Expand All @@ -516,7 +522,11 @@ export class Kernel {

if (err) {
this._debug('completed with error:', err);
cb.fail(new Error(err));
cb.fail(
name === JsiiErrorType.JSII_FAULT
? new JsiiFault(err)
: new RuntimeError(err),
);
} else {
const sandoxResult = this._toSandbox(
result,
Expand Down
2 changes: 1 addition & 1 deletion packages/@jsii/kernel/src/recording.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ export function recordInteraction(kernel: Kernel, inputOutputLogPath: string) {
return ret;
} catch (e: any) {
logOutput({ error: e.message, name: e.name });
if (e.type === JsiiErrorType.RUNTIME_ERROR) {
if (e.name === JsiiErrorType.RUNTIME_ERROR) {
throw new RuntimeError(e.message);
}
throw new JsiiFault(e.message);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@
CompleteRequest,
CompleteResponse,
)
from ...errors import JSIIError, JavaScriptError
from ...errors import ErrorType, JSIIError, JavaScriptError


@attr.s(auto_attribs=True, frozen=True, slots=True)
Expand All @@ -83,10 +83,11 @@ class _OkayResponse:


@attr.s(auto_attribs=True, frozen=True, slots=True)
class _ErrorRespose:
class _ErrorResponse:

error: str
stack: str
name: str


@attr.s(auto_attribs=True, frozen=True, slots=True)
Expand All @@ -101,7 +102,7 @@ class _CompleteRequest:
complete: CompleteRequest


_ProcessResponse = Union[_OkayResponse, _ErrorRespose, _CallbackResponse]
_ProcessResponse = Union[_OkayResponse, _ErrorResponse, _CallbackResponse]


def _with_api_key(api_name, asdict):
Expand Down Expand Up @@ -326,6 +327,8 @@ def send(
elif isinstance(resp, _CallbackResponse):
return resp.callback
else:
if resp.name == ErrorType.RUNTIME_ERROR.value:
raise RuntimeError(resp.error) from JavaScriptError(resp.stack)
raise JSIIError(resp.error) from JavaScriptError(resp.stack)


Expand Down
6 changes: 6 additions & 0 deletions packages/@jsii/python-runtime/src/jsii/errors.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import textwrap
from enum import Enum


class JSIIError(Exception):
Expand All @@ -11,3 +12,8 @@ def __init__(self, stack):

def __str__(self):
return "\n" + textwrap.indent(self.stack, " ", lambda line: bool(line))


class ErrorType(Enum):
JSII_FAULT = "@jsii/kernel.Fault"
RUNTIME_ERROR = "@jsii/kernel.RuntimeError"
14 changes: 7 additions & 7 deletions packages/@jsii/python-runtime/tests/test_compliance.py
Original file line number Diff line number Diff line change
Expand Up @@ -438,7 +438,7 @@ def test_exceptions():

assert calc3.value == 23

with pytest.raises(Exception):
with pytest.raises(RuntimeError):
calc3.add(10)

calc3.max_value = 40
Expand Down Expand Up @@ -544,7 +544,7 @@ def override_me(self, mult):

obj = ThrowingAsyncVirtualMethods()

with pytest.raises(Exception, match="Thrown by native code"):
with pytest.raises(RuntimeError, match="Thrown by native code"):
obj.call_me()


Expand Down Expand Up @@ -623,7 +623,7 @@ def the_property(self, value):

so = ThrowingSyncVirtualMethods()

with pytest.raises(Exception, match="Oh no, this is bad"):
with pytest.raises(RuntimeError, match="Oh no, this is bad"):
so.retrieve_value_of_the_property()


Expand All @@ -639,7 +639,7 @@ def the_property(self, value):

so = ThrowingSyncVirtualMethods()

with pytest.raises(Exception, match="Exception from overloaded setter"):
with pytest.raises(RuntimeError, match="Exception from overloaded setter"):
so.modify_value_of_the_property("Hii")


Expand Down Expand Up @@ -704,7 +704,7 @@ def test_fail_syncOverrides_callsDoubleAsync_method():
obj.call_async = True

# TODO: Error Handling
with pytest.raises(Exception):
with pytest.raises(RuntimeError):
obj.caller_is_method()


Expand All @@ -713,7 +713,7 @@ def test_fail_syncOverrides_callsDoubleAsync_propertyGetter():
obj.call_async = True

# TODO: Error Handling
with pytest.raises(Exception):
with pytest.raises(RuntimeError):
obj.caller_is_property


Expand All @@ -723,7 +723,7 @@ def test_fail_syncOverrides_callsDoubleAsync_propertySetter():
obj.call_async = True

# TODO: Error Handling
with pytest.raises(Exception):
with pytest.raises(RuntimeError):
obj.caller_is_property = 12


Expand Down
Loading