From a2184f56741cebd2763a147e899639f8eee8d31b Mon Sep 17 00:00:00 2001 From: Gursharan Singh <3442979+G8XSU@users.noreply.github.com> Date: Thu, 14 Sep 2023 14:25:19 -0700 Subject: [PATCH 1/2] Add NoSuchKeyError as possible errorcode Motivation: Such checks are best performed in backend than having client checking empty or null value. This is also in-line with more critical requirement for functioning of NotFound introduced by MonitorUpdatingPersister Design. Even though there are some popular KVStore's which do not throw such exception on keyNotFound such as AWS-DDB (arguably the most popular one), we can still figure out that key didn't exist if value was returned as null or by similar means in other stores. --- build.rs | 2 +- src/error.rs | 15 +++++++++++++++ src/types.rs | 17 ++++++++++++----- tests/tests.rs | 24 ++++++++++++++++++++++++ 4 files changed, 52 insertions(+), 6 deletions(-) diff --git a/build.rs b/build.rs index 09e2db9..3ffff30 100644 --- a/build.rs +++ b/build.rs @@ -14,7 +14,7 @@ fn main() { #[cfg(feature = "genproto")] fn generate_protos() { download_file( - "https://raw.githubusercontent.com/lightningdevkit/vss-server/e1a88afd61f56d7e8e90a32036ca12389e36fe44/app/src/main/proto/vss.proto", + "https://raw.githubusercontent.com/lightningdevkit/vss-server/62e888e1bd3305d23b15da857edffaf527163048/app/src/main/proto/vss.proto", "src/proto/vss.proto", ).unwrap(); diff --git a/src/error.rs b/src/error.rs index d07aa77..07aa32d 100644 --- a/src/error.rs +++ b/src/error.rs @@ -10,9 +10,20 @@ use std::fmt::{Display, Formatter}; /// information regarding each error code and corresponding use-cases. #[derive(Debug)] pub enum VssError { + /// Please refer to [`ErrorCode::NoSuchKeyException`]. + NoSuchKeyError(String), + + /// Please refer to [`ErrorCode::InvalidRequestException`]. InvalidRequestError(String), + + /// Please refer to [`ErrorCode::ConflictException`]. ConflictError(String), + + /// Please refer to [`ErrorCode::InternalServerException`]. InternalServerError(String), + + /// There is an unknown error, it could be a client-side bug, unrecognized error-code, network error + /// or something else. InternalError(String), } @@ -33,6 +44,9 @@ impl VssError { impl Display for VssError { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { match self { + VssError::NoSuchKeyError(message) => { + write!(f, "Requested key does not exist: {}", message) + } VssError::InvalidRequestError(message) => { write!(f, "Request sent to VSS Storage was invalid: {}", message) } @@ -54,6 +68,7 @@ impl Error for VssError {} impl From for VssError { fn from(error_response: ErrorResponse) -> Self { match error_response.error_code() { + ErrorCode::NoSuchKeyException => VssError::NoSuchKeyError(error_response.message), ErrorCode::InvalidRequestException => VssError::InvalidRequestError(error_response.message), ErrorCode::ConflictException => VssError::ConflictError(error_response.message), ErrorCode::InternalServerException => VssError::InternalServerError(error_response.message), diff --git a/src/types.rs b/src/types.rs index 252e3aa..0967782 100644 --- a/src/types.rs +++ b/src/types.rs @@ -10,7 +10,10 @@ pub struct GetObjectRequest { /// Authorization and billing can also be performed at the `store_id` level. #[prost(string, tag = "1")] pub store_id: ::prost::alloc::string::String, - /// `Key` for which the value is to be fetched. + /// The key of the value to be fetched. + /// + /// If the specified `key` does not exist, returns `ErrorCode.NO_SUCH_KEY_EXCEPTION` in the + /// the `ErrorResponse`. /// /// Consistency Guarantee: /// Get(read) operations against a `key` are consistent reads and will reflect all previous writes, @@ -287,17 +290,19 @@ pub struct KeyValue { pub enum ErrorCode { /// Default protobuf Enum value. Will not be used as `ErrorCode` by server. Unknown = 0, - /// CONFLICT_EXCEPTION is used when the request contains mismatched version (either key or global) + /// Used when the request contains mismatched version (either key or global) /// in `PutObjectRequest`. For more info refer `PutObjectRequest`. ConflictException = 1, - /// INVALID_REQUEST_EXCEPTION is used in the following cases: + /// Used in the following cases: /// - The request was missing a required argument. /// - The specified argument was invalid, incomplete or in the wrong format. /// - The request body of api cannot be deserialized into corresponding protobuf object. InvalidRequestException = 2, - /// An internal server error occurred, client is probably at no fault and can safely retry this - /// error with exponential backoff. + /// Used when an internal server error occurred, client is probably at no fault and can safely retry + /// this error with exponential backoff. InternalServerException = 3, + /// Used when the specified `key` in a `GetObjectRequest` does not exist. + NoSuchKeyException = 4, } impl ErrorCode { /// String value of the enum field names used in the ProtoBuf definition. @@ -310,6 +315,7 @@ impl ErrorCode { ErrorCode::ConflictException => "CONFLICT_EXCEPTION", ErrorCode::InvalidRequestException => "INVALID_REQUEST_EXCEPTION", ErrorCode::InternalServerException => "INTERNAL_SERVER_EXCEPTION", + ErrorCode::NoSuchKeyException => "NO_SUCH_KEY_EXCEPTION", } } /// Creates an enum from field names used in the ProtoBuf definition. @@ -319,6 +325,7 @@ impl ErrorCode { "CONFLICT_EXCEPTION" => Some(Self::ConflictException), "INVALID_REQUEST_EXCEPTION" => Some(Self::InvalidRequestException), "INTERNAL_SERVER_EXCEPTION" => Some(Self::InternalServerException), + "NO_SUCH_KEY_EXCEPTION" => Some(Self::NoSuchKeyException), _ => None, } } diff --git a/tests/tests.rs b/tests/tests.rs index 610d988..d5f6d62 100644 --- a/tests/tests.rs +++ b/tests/tests.rs @@ -149,6 +149,30 @@ mod tests { mock_server.expect(1).assert(); } + #[tokio::test] + async fn test_no_such_key_err_handling() { + let base_url = mockito::server_url(); + let vss_client = VssClient::new(&base_url); + + // NoSuchKeyError + let error_response = ErrorResponse { + error_code: ErrorCode::NoSuchKeyException.into(), + message: "NoSuchKeyException".to_string(), + }; + let mock_server = mockito::mock("POST", GET_OBJECT_ENDPOINT) + .with_status(409) + .with_body(&error_response.encode_to_vec()) + .create(); + + let get_result = vss_client + .get_object(&GetObjectRequest { store_id: "store".to_string(), key: "non_existent_key".to_string() }) + .await; + assert!(matches!(get_result.unwrap_err(), VssError::NoSuchKeyError { .. })); + + // Verify 1 request hit the server + mock_server.expect(1).assert(); + } + #[tokio::test] async fn test_invalid_request_err_handling() { let base_url = mockito::server_url(); From 318ab965421841536a7b46f294458eaf14883992 Mon Sep 17 00:00:00 2001 From: Gursharan Singh <3442979+G8XSU@users.noreply.github.com> Date: Mon, 18 Sep 2023 17:38:27 -0700 Subject: [PATCH 2/2] Downgrade prost-build to support for lower msrv --- Cargo.toml | 2 +- src/types.rs | 112 +++++++++++++++++---------------------------------- 2 files changed, 39 insertions(+), 75 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index a7a03d4..d9a1044 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -16,7 +16,7 @@ prost = "0.11.6" reqwest = { version = "0.11.13", features = ["rustls-tls"] } [build-dependencies] -prost-build = { version = "0.11.3" } +prost-build = { version = "0.8.0" } reqwest = { version = "0.11.13", features = ["blocking"] } [dev-dependencies] diff --git a/src/types.rs b/src/types.rs index 0967782..542e173 100644 --- a/src/types.rs +++ b/src/types.rs @@ -1,9 +1,8 @@ /// Request payload to be used for `GetObject` API call to server. -#[allow(clippy::derive_partial_eq_without_eq)] #[derive(Clone, PartialEq, ::prost::Message)] pub struct GetObjectRequest { /// `store_id` is a keyspace identifier. - /// Ref: ) + /// Ref: https://en.wikipedia.org/wiki/Keyspace_(distributed_data_store) /// All APIs operate within a single `store_id`. /// It is up to clients to use single or multiple stores for their use-case. /// This can be used for client-isolation/ rate-limiting / throttling on the server-side. @@ -21,12 +20,11 @@ pub struct GetObjectRequest { /// /// Read Isolation: /// Get/Read operations against a `key` are ensured to have read-committed isolation. - /// Ref: )#Read_committed + /// Ref: https://en.wikipedia.org/wiki/Isolation_(database_systems)#Read_committed #[prost(string, tag = "2")] pub key: ::prost::alloc::string::String, } /// Server response for `GetObject` API. -#[allow(clippy::derive_partial_eq_without_eq)] #[derive(Clone, PartialEq, ::prost::Message)] pub struct GetObjectResponse { /// Fetched `value` and `version` along with the corresponding `key` in the request. @@ -34,11 +32,10 @@ pub struct GetObjectResponse { pub value: ::core::option::Option, } /// Request payload to be used for `PutObject` API call to server. -#[allow(clippy::derive_partial_eq_without_eq)] #[derive(Clone, PartialEq, ::prost::Message)] pub struct PutObjectRequest { /// `store_id` is a keyspace identifier. - /// Ref: ) + /// Ref: https://en.wikipedia.org/wiki/Keyspace_(distributed_data_store) /// All APIs operate within a single `store_id`. /// It is up to clients to use single or multiple stores for their use-case. /// This can be used for client-isolation/ rate-limiting / throttling on the server-side. @@ -75,25 +72,25 @@ pub struct PutObjectRequest { /// All Items in a single `PutObjectRequest` must have distinct keys. /// /// Key-level versioning (Conditional Write): - /// Clients are expected to store a `version` against every `key`. - /// The write will succeed if the current DB version against the `key` is the same as in the request. - /// When initiating a `PutObjectRequest`, the request should contain their client-side `version` - /// for that key-value. + /// Clients are expected to store a `version` against every `key`. + /// The write will succeed if the current DB version against the `key` is the same as in the request. + /// When initiating a `PutObjectRequest`, the request should contain their client-side `version` + /// for that key-value. /// - /// For the first write of any `key`, the `version` should be '0'. If the write succeeds, the client - /// must increment their corresponding key versions (client-side) by 1. - /// The server increments key versions (server-side) for every successful write, hence this - /// client-side increment is required to ensure matching versions. These updated key versions should - /// be used in subsequent `PutObjectRequest`s for the keys. + /// For the first write of any `key`, the `version` should be '0'. If the write succeeds, the client + /// must increment their corresponding key versions (client-side) by 1. + /// The server increments key versions (server-side) for every successful write, hence this + /// client-side increment is required to ensure matching versions. These updated key versions should + /// be used in subsequent `PutObjectRequest`s for the keys. /// - /// Requests with a conflicting/mismatched version will fail with `CONFLICT_EXCEPTION` as ErrorCode - /// for conditional writes. + /// Requests with a conflicting/mismatched version will fail with `CONFLICT_EXCEPTION` as ErrorCode + /// for conditional writes. /// /// Skipping key-level versioning (Non-conditional Write): - /// If you wish to skip key-level version checks, set the `version` against the `key` to '-1'. - /// This will perform a non-conditional write query, after which the `version` against the `key` - /// is reset to '1'. Hence, the next `PutObjectRequest` for the `key` can be either - /// a non-conditional write or a conditional write with `version` set to `1`. + /// If you wish to skip key-level version checks, set the `version` against the `key` to '-1'. + /// This will perform a non-conditional write query, after which the `version` against the `key` + /// is reset to '1'. Hence, the next `PutObjectRequest` for the `key` can be either + /// a non-conditional write or a conditional write with `version` set to `1`. /// /// Considerations for transactions: /// Transaction writes of multiple items have a performance overhead, hence it is recommended to use @@ -112,18 +109,18 @@ pub struct PutObjectRequest { /// Each item in the `delete_items` field consists of a `key` and its corresponding `version`. /// /// Key-Level Versioning (Conditional Delete): - /// The `version` is used to perform a version check before deleting the item. - /// The delete will only succeed if the current database version against the `key` is the same as - /// the `version` specified in the request. + /// The `version` is used to perform a version check before deleting the item. + /// The delete will only succeed if the current database version against the `key` is the same as + /// the `version` specified in the request. /// /// Skipping key-level versioning (Non-conditional Delete): - /// If you wish to skip key-level version checks, set the `version` against the `key` to '-1'. - /// This will perform a non-conditional delete query. + /// If you wish to skip key-level version checks, set the `version` against the `key` to '-1'. + /// This will perform a non-conditional delete query. /// /// Fails with `CONFLICT_EXCEPTION` as the ErrorCode if: - /// * The requested item does not exist. - /// * The requested item does exist but there is a version-number mismatch (in conditional delete) - /// with the one in the database. + /// * The requested item does not exist. + /// * The requested item does exist but there is a version-number mismatch (in conditional delete) + /// with the one in the database. /// /// Multiple items in the `delete_items` field, along with the `transaction_items`, are written in a /// database transaction in an all-or-nothing fashion. @@ -133,15 +130,13 @@ pub struct PutObjectRequest { pub delete_items: ::prost::alloc::vec::Vec, } /// Server response for `PutObject` API. -#[allow(clippy::derive_partial_eq_without_eq)] #[derive(Clone, PartialEq, ::prost::Message)] pub struct PutObjectResponse {} /// Request payload to be used for `DeleteObject` API call to server. -#[allow(clippy::derive_partial_eq_without_eq)] #[derive(Clone, PartialEq, ::prost::Message)] pub struct DeleteObjectRequest { /// `store_id` is a keyspace identifier. - /// Ref: ) + /// Ref: https://en.wikipedia.org/wiki/Keyspace_(distributed_data_store) /// All APIs operate within a single `store_id`. /// It is up to clients to use single or multiple stores for their use-case. /// This can be used for client-isolation/ rate-limiting / throttling on the server-side. @@ -153,12 +148,12 @@ pub struct DeleteObjectRequest { /// An item consists of a `key` and its corresponding `version`. /// /// Key-level Versioning (Conditional Delete): - /// The item is only deleted if the current database version against the `key` is the same as - /// the `version` specified in the request. + /// The item is only deleted if the current database version against the `key` is the same as + /// the `version` specified in the request. /// /// Skipping key-level versioning (Non-conditional Delete): - /// If you wish to skip key-level version checks, set the `version` against the `key` to '-1'. - /// This will perform a non-conditional delete query. + /// If you wish to skip key-level version checks, set the `version` against the `key` to '-1'. + /// This will perform a non-conditional delete query. /// /// This operation is idempotent, that is, multiple delete calls for the same item will not fail. /// @@ -168,15 +163,13 @@ pub struct DeleteObjectRequest { pub key_value: ::core::option::Option, } /// Server response for `DeleteObject` API. -#[allow(clippy::derive_partial_eq_without_eq)] #[derive(Clone, PartialEq, ::prost::Message)] pub struct DeleteObjectResponse {} /// Request payload to be used for `ListKeyVersions` API call to server. -#[allow(clippy::derive_partial_eq_without_eq)] #[derive(Clone, PartialEq, ::prost::Message)] pub struct ListKeyVersionsRequest { /// `store_id` is a keyspace identifier. - /// Ref: ) + /// Ref: https://en.wikipedia.org/wiki/Keyspace_(distributed_data_store) /// All APIs operate within a single `store_id`. /// It is up to clients to use single or multiple stores for their use-case. /// This can be used for client-isolation/ rate-limiting / throttling on the server-side. @@ -209,7 +202,6 @@ pub struct ListKeyVersionsRequest { pub page_token: ::core::option::Option<::prost::alloc::string::String>, } /// Server response for `ListKeyVersions` API. -#[allow(clippy::derive_partial_eq_without_eq)] #[derive(Clone, PartialEq, ::prost::Message)] pub struct ListKeyVersionsResponse { /// Fetched keys and versions. @@ -238,9 +230,9 @@ pub struct ListKeyVersionsResponse { /// /// In case of refreshing the complete key-version view on the client-side, correct usage for /// the returned `global_version` is as following: - /// 1. Read `global_version` from the first page of paginated response and save it as local variable. - /// 2. Update all the `key_versions` on client-side from all the pages of paginated response. - /// 3. Update `global_version` on client_side from the local variable saved in step-1. + /// 1. Read `global_version` from the first page of paginated response and save it as local variable. + /// 2. Update all the `key_versions` on client-side from all the pages of paginated response. + /// 3. Update `global_version` on client_side from the local variable saved in step-1. /// This ensures that on client-side, all current `key_versions` were stored at `global_version` or later. /// This guarantee is helpful for ensuring the versioning correctness if using the `global_version` /// in `PutObject` API and can help avoid the race conditions related to it. @@ -249,7 +241,6 @@ pub struct ListKeyVersionsResponse { } /// When HttpStatusCode is not ok (200), the response `content` contains a serialized `ErrorResponse` /// with the relevant `ErrorCode` and `message` -#[allow(clippy::derive_partial_eq_without_eq)] #[derive(Clone, PartialEq, ::prost::Message)] pub struct ErrorResponse { /// The error code uniquely identifying an error condition. @@ -264,7 +255,6 @@ pub struct ErrorResponse { pub message: ::prost::alloc::string::String, } /// Represents a key-value pair to be stored or retrieved. -#[allow(clippy::derive_partial_eq_without_eq)] #[derive(Clone, PartialEq, ::prost::Message)] pub struct KeyValue { /// Key against which the value is stored. @@ -294,9 +284,9 @@ pub enum ErrorCode { /// in `PutObjectRequest`. For more info refer `PutObjectRequest`. ConflictException = 1, /// Used in the following cases: - /// - The request was missing a required argument. - /// - The specified argument was invalid, incomplete or in the wrong format. - /// - The request body of api cannot be deserialized into corresponding protobuf object. + /// - The request was missing a required argument. + /// - The specified argument was invalid, incomplete or in the wrong format. + /// - The request body of api cannot be deserialized into corresponding protobuf object. InvalidRequestException = 2, /// Used when an internal server error occurred, client is probably at no fault and can safely retry /// this error with exponential backoff. @@ -304,29 +294,3 @@ pub enum ErrorCode { /// Used when the specified `key` in a `GetObjectRequest` does not exist. NoSuchKeyException = 4, } -impl ErrorCode { - /// String value of the enum field names used in the ProtoBuf definition. - /// - /// The values are not transformed in any way and thus are considered stable - /// (if the ProtoBuf definition does not change) and safe for programmatic use. - pub fn as_str_name(&self) -> &'static str { - match self { - ErrorCode::Unknown => "UNKNOWN", - ErrorCode::ConflictException => "CONFLICT_EXCEPTION", - ErrorCode::InvalidRequestException => "INVALID_REQUEST_EXCEPTION", - ErrorCode::InternalServerException => "INTERNAL_SERVER_EXCEPTION", - ErrorCode::NoSuchKeyException => "NO_SUCH_KEY_EXCEPTION", - } - } - /// Creates an enum from field names used in the ProtoBuf definition. - pub fn from_str_name(value: &str) -> ::core::option::Option { - match value { - "UNKNOWN" => Some(Self::Unknown), - "CONFLICT_EXCEPTION" => Some(Self::ConflictException), - "INVALID_REQUEST_EXCEPTION" => Some(Self::InvalidRequestException), - "INTERNAL_SERVER_EXCEPTION" => Some(Self::InternalServerException), - "NO_SUCH_KEY_EXCEPTION" => Some(Self::NoSuchKeyException), - _ => None, - } - } -}