From 3059fdb6351379e0329c026c95d64fc7908f982b Mon Sep 17 00:00:00 2001 From: ZuzaOsa Date: Tue, 13 May 2025 10:13:27 +0200 Subject: [PATCH 1/2] Move error handling to a separate file --- src/errors.rs | 63 ++++++++++++++++++++++++++++ src/lib.rs | 1 + src/paging.rs | 2 +- src/requests/parameter_wrappers.rs | 3 +- src/result.rs | 2 +- src/session.rs | 7 ++-- src/tests/error_throwing_tests.rs | 2 +- src/tests/utils_tests.rs | 7 ++-- src/types/inet.rs | 2 +- src/types/local_date.rs | 3 +- src/types/local_time.rs | 3 +- src/types/type_hints.rs | 2 +- src/utils.rs | 66 ++---------------------------- 13 files changed, 85 insertions(+), 78 deletions(-) create mode 100644 src/errors.rs diff --git a/src/errors.rs b/src/errors.rs new file mode 100644 index 00000000..56d8ec2d --- /dev/null +++ b/src/errors.rs @@ -0,0 +1,63 @@ +use std::{ + error::Error, + fmt::{self, Display}, +}; + +use napi::Status; + +/// Enum representing possible JavaScript error types. +/// Error, RangeError, ReferenceError, SyntaxError, TypeError +/// are native JavaScript error types and the rest are custom +/// Datastax driver error types. +pub enum ErrorType { + ArgumentError, + AuthenticationError, + BusyConnectionError, // TODO: Add suport for fields of this error + DriverError, + DriverInternalError, + NoHostAvailableError, // TODO: Add suport for fields of this error + NotSupportedError, + OperationTimedOutError, // TODO: Add suport for fields of this error + ResponseError, // TODO: Add suport for fields of this error + Error, + RangeError, + ReferenceError, + SyntaxError, + TypeError, +} + +impl Display for ErrorType { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str(match self { + ErrorType::ArgumentError => "ArgumentError", + ErrorType::AuthenticationError => "AuthenticationError", + ErrorType::BusyConnectionError => "BusyConnectionError", + ErrorType::DriverError => "DriverError", + ErrorType::DriverInternalError => "DriverInternalError", + ErrorType::NoHostAvailableError => "NoHostAvailableError", + ErrorType::NotSupportedError => "NotSupportedError", + ErrorType::OperationTimedOutError => "OperationTimedOutError", + ErrorType::ResponseError => "ResponseError", + ErrorType::Error => "Error", + ErrorType::RangeError => "RangeError", + ErrorType::ReferenceError => "ReferenceError", + ErrorType::SyntaxError => "SyntaxError", + ErrorType::TypeError => "TypeError", + }) + } +} + +/// Convert rust error to napi::Error +pub(crate) fn err_to_napi(e: T) -> napi::Error { + js_error(e) +} + +/// Create napi::Error from a message +pub(crate) fn js_error(e: T) -> napi::Error { + js_typed_error(e, ErrorType::Error) +} + +/// Create napi::Error from a message and error type +pub(crate) fn js_typed_error(e: T, error_type: ErrorType) -> napi::Error { + napi::Error::new(Status::GenericFailure, format!("{}#{}", error_type, e)) +} diff --git a/src/lib.rs b/src/lib.rs index 350c4abb..16afe89c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -3,6 +3,7 @@ extern crate napi_derive; // Link other files pub mod auth; +pub mod errors; pub mod options; pub mod paging; pub mod requests; diff --git a/src/paging.rs b/src/paging.rs index d4200188..46996da8 100644 --- a/src/paging.rs +++ b/src/paging.rs @@ -1,7 +1,7 @@ use napi::bindgen_prelude::{Buffer, ToNapiValue}; use scylla::response::{PagingState, PagingStateResponse}; -use crate::{result::QueryResultWrapper, utils::js_error}; +use crate::{errors::js_error, result::QueryResultWrapper}; #[napi] pub struct PagingStateWrapper { diff --git a/src/requests/parameter_wrappers.rs b/src/requests/parameter_wrappers.rs index 5a33e722..d4267fd8 100644 --- a/src/requests/parameter_wrappers.rs +++ b/src/requests/parameter_wrappers.rs @@ -2,11 +2,12 @@ use napi::bindgen_prelude::{BigInt, Buffer}; use scylla::value::{Counter, CqlTimestamp, CqlTimeuuid, CqlValue, MaybeUnset}; use crate::{ + errors::js_error, types::{ duration::DurationWrapper, inet::InetAddressWrapper, local_date::LocalDateWrapper, local_time::LocalTimeWrapper, uuid::UuidWrapper, }, - utils::{bigint_to_i64, js_error}, + utils::bigint_to_i64, }; #[napi] diff --git a/src/result.rs b/src/result.rs index 96b88477..d65e9c1b 100644 --- a/src/result.rs +++ b/src/result.rs @@ -1,13 +1,13 @@ use std::sync::Arc; use crate::{ + errors::err_to_napi, types::{ local_date::LocalDateWrapper, time_uuid::TimeUuidWrapper, type_wrappers::{ComplexType, CqlType, CqlTypeClass}, uuid::UuidWrapper, }, - utils::err_to_napi, }; use napi::bindgen_prelude::{BigInt, Buffer, ToNapiValue}; use scylla::{ diff --git a/src/session.rs b/src/session.rs index 0da6901f..1092dd4b 100644 --- a/src/session.rs +++ b/src/session.rs @@ -7,14 +7,13 @@ use scylla::statement::prepared::PreparedStatement; use scylla::statement::{Consistency, SerialConsistency, Statement}; use scylla::value::{CqlValue, MaybeUnset}; +use crate::errors::{err_to_napi, js_error}; use crate::options; use crate::paging::{PagingResult, PagingStateWrapper}; use crate::requests::parameter_wrappers::MaybeUnsetQueryParameterWrapper; use crate::requests::request::QueryOptionsWrapper; -use crate::utils::{bigint_to_i64, js_error}; -use crate::{ - requests::request::PreparedStatementWrapper, result::QueryResultWrapper, utils::err_to_napi, -}; +use crate::utils::bigint_to_i64; +use crate::{requests::request::PreparedStatementWrapper, result::QueryResultWrapper}; const DEFAULT_CACHE_SIZE: u32 = 512; diff --git a/src/tests/error_throwing_tests.rs b/src/tests/error_throwing_tests.rs index aaaede0b..76b121e1 100644 --- a/src/tests/error_throwing_tests.rs +++ b/src/tests/error_throwing_tests.rs @@ -1,4 +1,4 @@ -use crate::utils::{js_typed_error, ErrorType}; +use crate::errors::{js_typed_error, ErrorType}; #[napi] /// Test function that throws specified error with a custom message. diff --git a/src/tests/utils_tests.rs b/src/tests/utils_tests.rs index 2f094140..a6a019b5 100644 --- a/src/tests/utils_tests.rs +++ b/src/tests/utils_tests.rs @@ -1,13 +1,14 @@ use napi::bindgen_prelude::BigInt; -use crate::utils::{self, bigint_to_i64}; +use crate::errors::js_error; +use crate::utils::bigint_to_i64; #[napi] pub fn tests_bigint_to_i64(value: BigInt, case_id: Option) -> napi::Result<()> { let case_id = match case_id { Some(case_id) => case_id, None => { - return utils::bigint_to_i64(value, "Overflow expected").map(|_| ()); + return bigint_to_i64(value, "Overflow expected").map(|_| ()); } }; @@ -28,7 +29,7 @@ pub fn tests_bigint_to_i64(value: BigInt, case_id: Option) -> napi::Result< if v == expected { Ok(()) } else { - Err(utils::js_error(format!("Got {}, expected{}", v, expected))) + Err(js_error(format!("Got {}, expected{}", v, expected))) } } Err(e) => Err(e), diff --git a/src/types/inet.rs b/src/types/inet.rs index 3a8ef19f..dba757d2 100644 --- a/src/types/inet.rs +++ b/src/types/inet.rs @@ -1,4 +1,4 @@ -use crate::utils::{js_typed_error, ErrorType}; +use crate::errors::{js_typed_error, ErrorType}; use napi::bindgen_prelude::{Buffer, BufferSlice}; use std::net::IpAddr; diff --git a/src/types/local_date.rs b/src/types/local_date.rs index a4e329cd..38d22228 100644 --- a/src/types/local_date.rs +++ b/src/types/local_date.rs @@ -1,4 +1,5 @@ -use crate::utils::{js_error, CharCounter}; +use crate::errors::js_error; +use crate::utils::CharCounter; use regex::Regex; use scylla::value::CqlDate; use std::sync::LazyLock; diff --git a/src/types/local_time.rs b/src/types/local_time.rs index 9aa63ffc..4719269f 100644 --- a/src/types/local_time.rs +++ b/src/types/local_time.rs @@ -1,4 +1,5 @@ -use crate::utils::{bigint_to_i64, js_error, CharCounter}; +use crate::errors::js_error; +use crate::utils::{bigint_to_i64, CharCounter}; use napi::bindgen_prelude::BigInt; use scylla::value::CqlTime; use std::fmt::{self, Write}; diff --git a/src/types/type_hints.rs b/src/types/type_hints.rs index eae26772..a248b975 100644 --- a/src/types/type_hints.rs +++ b/src/types/type_hints.rs @@ -1,4 +1,4 @@ -use crate::utils::js_error; +use crate::errors::js_error; use super::type_wrappers::{ComplexType, CqlType}; diff --git a/src/utils.rs b/src/utils.rs index 396f8454..40b44202 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -1,66 +1,6 @@ -use std::{ - error::Error, - fmt::{self, Display}, -}; - -use napi::{bindgen_prelude::BigInt, Status}; - -/// Enum representing possible JavaScript error types. -/// Error, RangeError, ReferenceError, SyntaxError, TypeError -/// are native JavaScript error types and the rest are custom -/// Datastax driver error types. -pub enum ErrorType { - ArgumentError, - AuthenticationError, - BusyConnectionError, // TODO: Add suport for fields of this error - DriverError, - DriverInternalError, - NoHostAvailableError, // TODO: Add suport for fields of this error - NotSupportedError, - OperationTimedOutError, // TODO: Add suport for fields of this error - ResponseError, // TODO: Add suport for fields of this error - Error, - RangeError, - ReferenceError, - SyntaxError, - TypeError, -} - -impl Display for ErrorType { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.write_str(match self { - ErrorType::ArgumentError => "ArgumentError", - ErrorType::AuthenticationError => "AuthenticationError", - ErrorType::BusyConnectionError => "BusyConnectionError", - ErrorType::DriverError => "DriverError", - ErrorType::DriverInternalError => "DriverInternalError", - ErrorType::NoHostAvailableError => "NoHostAvailableError", - ErrorType::NotSupportedError => "NotSupportedError", - ErrorType::OperationTimedOutError => "OperationTimedOutError", - ErrorType::ResponseError => "ResponseError", - ErrorType::Error => "Error", - ErrorType::RangeError => "RangeError", - ErrorType::ReferenceError => "ReferenceError", - ErrorType::SyntaxError => "SyntaxError", - ErrorType::TypeError => "TypeError", - }) - } -} - -/// Convert rust error to napi::Error -pub(crate) fn err_to_napi(e: T) -> napi::Error { - js_error(e) -} - -/// Create napi::Error from a message -pub(crate) fn js_error(e: T) -> napi::Error { - js_typed_error(e, ErrorType::Error) -} - -/// Create napi::Error from a message and error type -pub(crate) fn js_typed_error(e: T, error_type: ErrorType) -> napi::Error { - napi::Error::new(Status::GenericFailure, format!("{}#{}", error_type, e)) -} +use crate::errors::js_error; +use napi::bindgen_prelude::BigInt; +use std::fmt::{self, Display}; /// Convert napi bigint to i64. Returns napi::Error if value doesn't fit in i64. pub(crate) fn bigint_to_i64(value: BigInt, error_msg: impl Display) -> napi::Result { From 7bc90aef4f18f46fce040511ac210a0e3a86996b Mon Sep 17 00:00:00 2001 From: ZuzaOsa Date: Thu, 15 May 2025 17:44:08 +0200 Subject: [PATCH 2/2] Add description of the error throwing issue --- docs/src/internal/error_throwing.md | 74 +++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+) create mode 100644 docs/src/internal/error_throwing.md diff --git a/docs/src/internal/error_throwing.md b/docs/src/internal/error_throwing.md new file mode 100644 index 00000000..ee3f26d7 --- /dev/null +++ b/docs/src/internal/error_throwing.md @@ -0,0 +1,74 @@ +# Errors + +Napi-rs provides only a way to throw a JavaScript `Error` object. +Both DataStax and ScyllaDB drivers utilize multiple error classes to represent different issues. + +## Errors in the DataStax driver + +The DataStax driver utilizes error classes native to Node.js: + +- `Error`, +- `TypeError`, +- `RangeError`, +- `ReferenceError`, +- `SyntaxError`, + +custom error classes without additional arguments: + +- `ArgumentError`, +- `AuthenticationError`, +- `DriverError`, +- `DriverInternalError`, +- `NotSupportedError`, + +and custom error classes with additional arguments: + +- `BusyConnectionError`, +- `NoHostAvailableError`, +- `OperationTimedOutError`, +- `ResponseError`. + +Some of the errors arguments would be easy to pass by adding logic to the function implemented in +[#118](https://github.com/scylladb-zpp-2024-javascript-driver/scylladb-javascript-driver/pull/118), +but some of them are not easy to pass and even not easily available in the Rust driver. +For example, `NoHostAvailableError` has `innerErrors` parameter, +which is an object map containing the error message for each host. +This is available in the Rust driver only when using query execution history, +otherwise only the error for the last host is available. + +## Errors in the ScyllaDB driver + +The ScyllaDB driver has multiple custom error classes that can be found in `scylla/src/error.rs` file. +Some types of errors are enums, nesting more specific errors inside. + +## Comparison + +There are multiple errors in ScyllaDB driver that cannot be easily mapped to the DataStax driver errors. +Some of them are only relevant to ScyllaDB, because of different database and driver designs. +In the DataStax driver, the errors are more generic +and most of them are classified under `ResponseError` +with `code` parameter used to identify the nature of the error +(a list of error codes can be found in `lib/types/index.js` file). + +## Possible solutions + +### 1. Use the DataStax driver errors + +Use the old architecture of the DataStax driver and throw existing errors. +Adding support for passing additional parameters of the errors would need to be implemented. +All Rust errors would need to be mapped, possibly losing some of the information +or encountering an issue that two cases are treated as the same error in the Rust driver +but different errors in the DataStax driver. +However, this would keep the interface closer to the DataStax driver +if some users are expecting specific error types. + +### 2. Use the ScyllaDB driver errors + +Use the ScyllaDB driver errors and throw them. +This would require implementing all of the error classes in JavaScript, +taking into account the fact that some of them are enums (probably by using class inheritance). +This would also need some work to pass the parameters of the errors. +This approach would lose less information about the errors and add support for ScyllaDB specific errors. +However, this would make the interface of possible errors different from the DataStax driver, +resulting in users needing to reimplement some of the error handling logic - +especially if they are testing for specific error types.