feat(js-connectors): Proper JS error propagation #4186
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The way it is implemented: 1. On TS side, we catch all errors from connector methods. In case we encounterd an error, we store it on per-connector error registry and return it's numeric id to the engine. 2. Engine propogates that numeric id all the way throuhg quaint up to query-engine-node-api, where it becomes new kind of error, `ExternalError` with a code of P2036 and error id in the meta field. 3. Client is then expected to pick the error from the registry and re-throw it. This PR implements this for smoke-tests, similar implementation needs to be done in the client. Implementation is done this way, rather than propogating `napi::Error` instance, to avoid dependencies on napi types in all intermediate crates other than `js-connectors` and `query-engine-node-api`. To better accomodate this pattern, `Result` type was introduced to the connectors. It allows to explicitly indicate if particular call succeeded or failed via return value. On Rust side, those values get parsed and converted into `std::result::Result` values. On Rust side, `AsyncJsFunction` wrapper over `ThreadsafeFunction` is introduced. It handles promises and JS result type conversions automatically and simplifies `Proxy` code. This also lays the foundation for supporting "Known" errors - JsError type could be extended in a future with new error types, that could be converted into standard prisma errors with normal error codes. Fix #prisma/team-orm#260
|
I am also happy to discuss naming of some of the things this PR introduces. In particuallary, I am not 100% with those names:
|
SevInf
commented
Aug 30, 2023
Comment on lines
+310
to
+315
| if (error.error_code === 'P2036') { | ||
| const jsError = this.connector.errorRegistry.consumeError(error.meta.id) | ||
| if (!jsError) { | ||
| throw new Error(`Something went wrong. Engine reported external error with id ${error.meta.id}, but it was not registered.`) | ||
| } | ||
| throw jsError.error |
There was a problem hiding this comment.
This is rougly what Prisma client has to do to get errors too.
SevInf
commented
Aug 30, 2023
| ArgType: ToNapiValue + 'static, | ||
| ReturnType: FromNapiValue + 'static, | ||
| { | ||
| unsafe fn from_napi_value(napi_env: napi::sys::napi_env, napi_val: napi::sys::napi_value) -> napi::Result<Self> { |
There was a problem hiding this comment.
This PR is pretty heavy on the usage of this trait because it plays nicely with napi-rs build-in methods for automatic conversions and there is no safe alternative. In all cases we immediately wrap it into safe wrappers and work with them afterwards. I beleive safe alternative could be added to napi-rs pretty easily, so if you think it's a problem I can try to send them a PR.
millsp
changed the title
jkomyno
reviewed
Aug 30, 2023
CodSpeed Performance ReportMerging #4186 will not alter performanceComparing Summary
|
SevInf
commented
Aug 31, 2023
SevInf
commented
Aug 31, 2023
SevInf
commented
Aug 31, 2023
SevInf
commented
Aug 31, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The way it is implemented:
encounterd an error, we store it on per-connector error registry and
return it's numeric id to the engine.
query-engine-node-api, where it becomes new kind of error,
ExternalErrorwith a code of P2036 and error id in the meta field.re-throw it. This PR implements this for smoke-tests, similar
implementation needs to be done in the client.
Implementation is done this way, rather than propogating
napi::Errorinstance, to avoid dependencies on napi types in all intermediate crates
other than
js-connectorsandquery-engine-node-api.To better accomodate this pattern,
Resulttype was introduced to theconnectors. It allows to explicitly indicate if particular call
succeeded or failed via return value. On Rust side, those values get
parsed and converted into
std::result::Resultvalues.On Rust side,
AsyncJsFunctionwrapper overThreadsafeFunctionisintroduced. It handles promises and JS result type conversions
automatically and simplifies
Proxycode.This also lays the foundation for supporting "Known" errors - JsError
type could be extended in a future with new error types, that could be
converted into standard prisma errors with normal error codes.
Fix #prisma/team-orm#260