From 74d41681e7750f1cc7b7690c689e6cf6a30a526c Mon Sep 17 00:00:00 2001 From: dracarys18 Date: Wed, 29 Nov 2023 20:46:04 +0530 Subject: [PATCH 1/2] fix: throw bad request while pushing duplicate data to redis --- crates/router/src/core/errors/utils.rs | 22 +++++++++++++++++++ crates/router/src/db/refund.rs | 6 ++--- crates/router/src/db/reverse_lookup.rs | 3 ++- crates/router/src/utils/db_utils.rs | 8 +++++-- crates/storage_impl/src/errors.rs | 20 +++++++++++++++++ .../src/payments/payment_attempt.rs | 3 ++- .../src/payments/payment_intent.rs | 5 +++-- crates/storage_impl/src/utils.rs | 5 +++-- 8 files changed, 61 insertions(+), 11 deletions(-) diff --git a/crates/router/src/core/errors/utils.rs b/crates/router/src/core/errors/utils.rs index b62abd0e336e..f00948b887e1 100644 --- a/crates/router/src/core/errors/utils.rs +++ b/crates/router/src/core/errors/utils.rs @@ -480,3 +480,25 @@ impl ConnectorErrorExt for error_stack::Result } } } + +pub trait RedisErrorExt { + #[track_caller] + fn to_redis_failed_response(self, key: &str) -> error_stack::Report; +} + +impl RedisErrorExt for error_stack::Report { + fn to_redis_failed_response(self, key: &str) -> error_stack::Report { + match self.current_context() { + errors::RedisError::NotFound => self.change_context( + errors::StorageError::ValueNotFound(format!("Data does not exist for key {key}",)), + ), + errors::RedisError::SetNxFailed => { + self.change_context(errors::StorageError::DuplicateValue { + entity: "redis", + key: Some(key.to_string()), + }) + } + _ => self.change_context(errors::StorageError::KVError), + } + } +} diff --git a/crates/router/src/db/refund.rs b/crates/router/src/db/refund.rs index 8ac8bd106eff..5a1cac6cc938 100644 --- a/crates/router/src/db/refund.rs +++ b/crates/router/src/db/refund.rs @@ -275,7 +275,7 @@ mod storage { use super::RefundInterface; use crate::{ connection, - core::errors::{self, CustomResult}, + core::errors::{self, utils::RedisErrorExt, CustomResult}, db::reverse_lookup::ReverseLookupInterface, logger, services::Store, @@ -392,7 +392,7 @@ mod storage { &key, ) .await - .change_context(errors::StorageError::KVError)? + .map_err(|err| err.to_redis_failed_response(&key))? .try_into_hsetnx() { Ok(HsetnxReply::KeyNotSet) => Err(errors::StorageError::DuplicateValue { @@ -550,7 +550,7 @@ mod storage { &key, ) .await - .change_context(errors::StorageError::KVError)? + .map_err(|err| err.to_redis_failed_response(&key))? .try_into_hset() .change_context(errors::StorageError::KVError)?; diff --git a/crates/router/src/db/reverse_lookup.rs b/crates/router/src/db/reverse_lookup.rs index 445e171fa277..344077f3ec0f 100644 --- a/crates/router/src/db/reverse_lookup.rs +++ b/crates/router/src/db/reverse_lookup.rs @@ -69,6 +69,7 @@ mod storage { use super::{ReverseLookupInterface, Store}; use crate::{ connection, + core::errors::utils::RedisErrorExt, errors::{self, CustomResult}, types::storage::{ enums, kv, @@ -109,7 +110,7 @@ mod storage { format!("reverse_lookup_{}", &created_rev_lookup.lookup_id), ) .await - .change_context(errors::StorageError::KVError)? + .map_err(|err| err.to_redis_failed_response(&created_rev_lookup.lookup_id))? .try_into_setnx() { Ok(SetnxReply::KeySet) => Ok(created_rev_lookup), diff --git a/crates/router/src/utils/db_utils.rs b/crates/router/src/utils/db_utils.rs index febc226c0202..219b6f9777f9 100644 --- a/crates/router/src/utils/db_utils.rs +++ b/crates/router/src/utils/db_utils.rs @@ -1,4 +1,7 @@ -use crate::{core::errors, routes::metrics}; +use crate::{ + core::errors::{self, utils::RedisErrorExt}, + routes::metrics, +}; /// Generates hscan field pattern. Suppose the field is pa_1234_ref_1211 it will generate /// pa_1234_ref_* @@ -28,7 +31,8 @@ where metrics::KV_MISS.add(&metrics::CONTEXT, 1, &[]); database_call_closure().await } - _ => Err(redis_error.change_context(errors::StorageError::KVError)), + // Keeping the key empty here since the error would never go here. + _ => Err(redis_error.to_redis_failed_response("")), }, } } diff --git a/crates/storage_impl/src/errors.rs b/crates/storage_impl/src/errors.rs index bc68986cb8ea..04bfe47a4cec 100644 --- a/crates/storage_impl/src/errors.rs +++ b/crates/storage_impl/src/errors.rs @@ -158,6 +158,26 @@ impl StorageError { } } +pub trait RedisErrorExt { + #[track_caller] + fn to_redis_failed_response(self, key: &str) -> error_stack::Report; +} + +impl RedisErrorExt for error_stack::Report { + fn to_redis_failed_response(self, key: &str) -> error_stack::Report { + match self.current_context() { + RedisError::NotFound => self.change_context(DataStorageError::ValueNotFound(format!( + "Data does not exist for key {key}", + ))), + RedisError::SetNxFailed => self.change_context(DataStorageError::DuplicateValue { + entity: "redis", + key: Some(key.to_string()), + }), + _ => self.change_context(DataStorageError::KVError), + } + } +} + impl_error_type!(EncryptionError, "Encryption error"); #[derive(Debug, thiserror::Error)] diff --git a/crates/storage_impl/src/payments/payment_attempt.rs b/crates/storage_impl/src/payments/payment_attempt.rs index e86119e41af6..891ad39cecbc 100644 --- a/crates/storage_impl/src/payments/payment_attempt.rs +++ b/crates/storage_impl/src/payments/payment_attempt.rs @@ -29,6 +29,7 @@ use router_env::{instrument, tracing}; use crate::{ diesel_error_to_data_error, + errors::RedisErrorExt, lookup::ReverseLookupInterface, redis::kv_store::{kv_wrapper, KvOperation}, utils::{pg_connection_read, pg_connection_write, try_redis_get_else_try_database_get}, @@ -409,7 +410,7 @@ impl PaymentAttemptInterface for KVRouterStore { &key, ) .await - .change_context(errors::StorageError::KVError)? + .map_err(|err| err.to_redis_failed_response(&key))? .try_into_hsetnx() { Ok(HsetnxReply::KeyNotSet) => Err(errors::StorageError::DuplicateValue { diff --git a/crates/storage_impl/src/payments/payment_intent.rs b/crates/storage_impl/src/payments/payment_intent.rs index c3b3d22ffe35..5b859274766e 100644 --- a/crates/storage_impl/src/payments/payment_intent.rs +++ b/crates/storage_impl/src/payments/payment_intent.rs @@ -38,6 +38,7 @@ use router_env::{instrument, tracing}; use crate::connection; use crate::{ diesel_error_to_data_error, + errors::RedisErrorExt, redis::kv_store::{kv_wrapper, KvOperation}, utils::{self, pg_connection_read, pg_connection_write}, DataModelExt, DatabaseStore, KVRouterStore, @@ -114,7 +115,7 @@ impl PaymentIntentInterface for KVRouterStore { &key, ) .await - .change_context(StorageError::KVError)? + .map_err(|err| err.to_redis_failed_response(&key))? .try_into_hsetnx() { Ok(HsetnxReply::KeyNotSet) => Err(StorageError::DuplicateValue { @@ -175,7 +176,7 @@ impl PaymentIntentInterface for KVRouterStore { &key, ) .await - .change_context(StorageError::KVError)? + .map_err(|err| err.to_redis_failed_response(&key))? .try_into_hset() .change_context(StorageError::KVError)?; diff --git a/crates/storage_impl/src/utils.rs b/crates/storage_impl/src/utils.rs index 6d6e1cd5402b..6d69f02593fd 100644 --- a/crates/storage_impl/src/utils.rs +++ b/crates/storage_impl/src/utils.rs @@ -3,7 +3,7 @@ use data_models::errors::StorageError; use diesel::PgConnection; use error_stack::{IntoReport, ResultExt}; -use crate::{metrics, DatabaseStore}; +use crate::{errors::RedisErrorExt, metrics, DatabaseStore}; pub async fn pg_connection_read( store: &T, @@ -64,7 +64,8 @@ where metrics::KV_MISS.add(&metrics::CONTEXT, 1, &[]); database_call_closure().await } - _ => Err(redis_error.change_context(StorageError::KVError)), + // Keeping the key empty here since the error would never go here. + _ => Err(redis_error.to_redis_failed_response("")), }, } } From 9c2fd78bfd988df77704c62a2c690b269f8976c5 Mon Sep 17 00:00:00 2001 From: dracarys18 Date: Thu, 30 Nov 2023 00:18:44 +0530 Subject: [PATCH 2/2] fix: add to reverse lookup --- crates/storage_impl/src/lookup.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/storage_impl/src/lookup.rs b/crates/storage_impl/src/lookup.rs index bd045fedd379..c96e24515772 100644 --- a/crates/storage_impl/src/lookup.rs +++ b/crates/storage_impl/src/lookup.rs @@ -11,6 +11,7 @@ use redis_interface::SetnxReply; use crate::{ diesel_error_to_data_error, + errors::RedisErrorExt, redis::kv_store::{kv_wrapper, KvOperation}, utils::{self, try_redis_get_else_try_database_get}, DatabaseStore, KVRouterStore, RouterStore, @@ -97,7 +98,7 @@ impl ReverseLookupInterface for KVRouterStore { format!("reverse_lookup_{}", &created_rev_lookup.lookup_id), ) .await - .change_context(errors::StorageError::KVError)? + .map_err(|err| err.to_redis_failed_response(&created_rev_lookup.lookup_id))? .try_into_setnx() { Ok(SetnxReply::KeySet) => Ok(created_rev_lookup),