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: Errors handling between Rust and JS #80

Merged

Conversation

fmarek-kindred
Copy link
Contributor

@fmarek-kindred fmarek-kindred commented Sep 18, 2023

The main ideas:

Throwing error

  • Rust code should always "throw" some Talos error, in our case all public method are throwing ClientError so, there was no need to do any changes here.
  • Create simple "napi-managed" enum SdkErrorKind in RS and map its values to ClientError.kind, in our case that is ClientErrorKind.rs enum.
  • In the napi adapter layer "cohort_sdk_js" declare a simple container class SdkErrorContainer" whose purpose is to collect attributes from ClientError` and make them a) serialisable to JSON b) transferrable to JS layer
  • When cohort_sdk_js is handling rust error it packs data into SdkErrorContainer, serialises it to json + string and sets into napi::Error.message

Receiving and mapping error

  • JS caller will receive this error as GenericError with message looking like JSON object.
  • Introduce cohort_sdk_client JS project which wraps every public service class from cohort_sdk_js. For example, cohort_sdk_js exposes logical service "initiator". We program this service as cohort_sdk_js::InternalInitiator and do not let JS client to use it directly. Instead, we wrap this class into "cohort_sdk_client::Initiator" so that every public method is a delegate with try .. catch. When catching error we catch a normal JS error, use it's message attribute to parse into SdkErrorContainer. Then we re-throw resulting info as "official" TalosSdkError. This is JS class extending JS Error with enum inside it.

The end client only aware of:

  • cohort_sdk_client as dependency
  • cohort_sdk_client::Initiator as service class,
  • cohort_sdk_client::TalosSdkError as error which will be thown from service classes. It has TalosSdkError.kind as SdkErrorKind enum.

@@ -25,13 +25,14 @@ export class BankingApp {

async init() {
this.initiator = await Initiator.init(sdkConfig)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If kafka is down the error will bubble into main method. There is an example how we can handle typed errors.

@@ -71,6 +111,16 @@ new Promise(async (resolve) => {
queue,
fnFinish,
)
await app.init()
try {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is example where caller can deal with specific errors. I skipped else statement here.

}
}

export {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Export only what we allow client to use.

import { isSdkError } from "./internal"
import { TalosSdkError } from "."

export class Initiator {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrapper with error handler.

@@ -0,0 +1,5 @@
import { SDK_CONTAINER_TYPE } from "cohort_sdk_js"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The value is managed in the Rust layer.


// println!("Initiator.certify(): after cohort.certify(...)");
Ok("Success".to_string())
let _res = self.cohort.certify(&make_new_request, &ooo_impl).await.map_err(map_error)?;
Copy link
Contributor Author

@fmarek-kindred fmarek-kindred Sep 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's where we use our error handling and packaging logic.

@codecov
Copy link

codecov bot commented Sep 18, 2023

Codecov Report

❗ No coverage uploaded for pull request base (feature/cohort_replicator_js@f261ebf). Click here to learn what that means.
Patch has no changes to coverable lines.

❗ Current head d0bb3dd differs from pull request most recent head d66ea6a. Consider uploading reports for the commit d66ea6a to get more accurate results

Additional details and impacted files
@@                       Coverage Diff                       @@
##             feature/cohort_replicator_js      #80   +/-   ##
===============================================================
  Coverage                                ?   58.16%           
===============================================================
  Files                                   ?      103           
  Lines                                   ?     5314           
  Branches                                ?        0           
===============================================================
  Hits                                    ?     3091           
  Misses                                  ?     2223           
  Partials                                ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fmarek-kindred fmarek-kindred marked this pull request as ready for review September 20, 2023 06:13
* feat: Add wrapper and error handler for Replicator JS.

* feat: Add ReplicatorError and map it to JS layer.

* fix: Move dev dependencies into relevant section in the package.json
@fmarek-kindred fmarek-kindred changed the title feat: Initial draft for errors handling between Rust and JS feat: Errors handling between Rust and JS Sep 21, 2023
Copy link
Collaborator

@gk-kindred gk-kindred left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 👍

@fmarek-kindred fmarek-kindred merged commit 4cecb9f into feature/cohort_replicator_js Sep 25, 2023
2 checks passed
@fmarek-kindred fmarek-kindred deleted the feature/errors_mapping_rust_to_js branch September 25, 2023 05:35
fmarek-kindred added a commit that referenced this pull request Sep 26, 2023
* fix: Refactor strucutre to define re-usabe elements.

* fix: Move replicator callback traits into callbacks.rs

* feat: Add Replicator JS wrapper

* feat: Cohort Replicator JS

* feat: Use launch parameters to configure runtime settings.

* feat: Use i64 when converting numbers from JS to Rust.

* chore: Remove unused file.

* fix: Use better error handling inside replicator install()

* chore: Add logging to Replicator JS

* fix: Convert snapshot version to number in JS

* feat: Add missing DB migrartions; Organise Makefile commands. (#79)

* feat: Errors handling between Rust and JS (#80)

* feat: Initial draft for errors handling between Rust and JS

* fix: Remove cohort_sdk_js dependency, instead declare optional dependencies on images.

* fix: Inject using serde annotation _typ during serialisation into JSON.

* fix: Improve error handling when processig callback calls between JS and RS.

* feat: Add ReplicatorError and map it to JS layer. (#81)

* feat: Add wrapper and error handler for Replicator JS.

* feat: Add ReplicatorError and map it to JS layer.

* fix: Move dev dependencies into relevant section in the package.json
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants