From 87cedc296e06ecd007e5a43c8d454ba4767edf5f Mon Sep 17 00:00:00 2001 From: Nishant Joshi Date: Fri, 23 Dec 2022 00:56:09 +0530 Subject: [PATCH] fix: resolve `TODO` and `FIXME` in `utils` module (#220) --- crates/router/src/core/payments.rs | 7 +- crates/router/src/core/payments/helpers.rs | 95 ++++++++++--------- .../payments/operations/payment_confirm.rs | 7 +- .../payments/operations/payment_status.rs | 14 +-- crates/router/src/core/refunds.rs | 20 ++-- crates/router/src/core/refunds/validator.rs | 17 ++-- crates/router/src/services/api.rs | 7 +- crates/router/src/services/encryption.rs | 7 +- crates/router/src/utils/ext_traits.rs | 14 +-- crates/router/src/utils/fp_utils.rs | 51 ++++------ 10 files changed, 110 insertions(+), 129 deletions(-) diff --git a/crates/router/src/core/payments.rs b/crates/router/src/core/payments.rs index 9d8af0fffe..dae9dc7e11 100644 --- a/crates/router/src/core/payments.rs +++ b/crates/router/src/core/payments.rs @@ -583,10 +583,9 @@ pub async fn list_payments( .into_iter() .map(ForeignInto::foreign_into) .collect(); - utils::when( - data.is_empty(), - Err(errors::ApiErrorResponse::PaymentNotFound), - )?; + utils::when(data.is_empty(), || { + Err(errors::ApiErrorResponse::PaymentNotFound) + })?; Ok(services::BachResponse::Json(api::PaymentListResponse { size: data.len(), data, diff --git a/crates/router/src/core/payments/helpers.rs b/crates/router/src/core/payments/helpers.rs index 7b0487ac0b..cac54faa9b 100644 --- a/crates/router/src/core/payments/helpers.rs +++ b/crates/router/src/core/payments/helpers.rs @@ -212,14 +212,13 @@ pub fn validate_merchant_id( let request_merchant_id = request_merchant_id.unwrap_or(merchant_id); - utils::when( - merchant_id.ne(request_merchant_id), + utils::when(merchant_id.ne(request_merchant_id), || { Err(report!(errors::ApiErrorResponse::PreconditionFailed { message: format!( "Invalid `merchant_id`: {request_merchant_id} not found in merchant account" ) - })), - ) + })) + }) } #[instrument(skip_all)] @@ -235,15 +234,14 @@ pub fn validate_request_amount_and_amount_to_capture( api::Amount::Value(amount_inner) => { // If both amount and amount to capture is present // then amount to be capture should be less than or equal to request amount - utils::when( - !amount_to_capture.le(&amount_inner.get()), + utils::when(!amount_to_capture.le(&amount_inner.get()), || { Err(report!(errors::ApiErrorResponse::PreconditionFailed { message: format!( "amount_to_capture is greater than amount capture_amount: {:?} request_amount: {:?}", amount_to_capture, amount ) - })), - ) + })) + }) } api::Amount::Zero => { // If the amount is Null but still amount_to_capture is passed this is invalid and @@ -370,9 +368,11 @@ pub fn verify_mandate_details( .mandate_amount .map(|mandate_amount| request_amount > mandate_amount) .unwrap_or(true), - Err(report!(errors::ApiErrorResponse::MandateValidationFailed { - reason: "request amount is greater than mandate amount".to_string() - })), + || { + Err(report!(errors::ApiErrorResponse::MandateValidationFailed { + reason: "request amount is greater than mandate amount".to_string() + })) + }, ), storage::enums::MandateType::MultiUse => utils::when( mandate @@ -381,9 +381,11 @@ pub fn verify_mandate_details( (mandate.amount_captured.unwrap_or(0) + request_amount) > mandate_amount }) .unwrap_or(false), - Err(report!(errors::ApiErrorResponse::MandateValidationFailed { - reason: "request amount is greater than mandate amount".to_string() - })), + || { + Err(report!(errors::ApiErrorResponse::MandateValidationFailed { + reason: "request amount is greater than mandate amount".to_string() + })) + }, ), }?; utils::when( @@ -391,9 +393,11 @@ pub fn verify_mandate_details( .mandate_currency .map(|mandate_currency| mandate_currency.to_string() != request_currency) .unwrap_or(false), - Err(report!(errors::ApiErrorResponse::MandateValidationFailed { - reason: "cross currency mandates not supported".to_string() - })), + || { + Err(report!(errors::ApiErrorResponse::MandateValidationFailed { + reason: "cross currency mandates not supported".to_string() + })) + }, ) } @@ -916,12 +920,14 @@ pub(crate) fn validate_capture_method( ) -> RouterResult<()> { utils::when( capture_method == storage_enums::CaptureMethod::Automatic, - Err(report!(errors::ApiErrorResponse::PaymentUnexpectedState { - field_name: "capture_method".to_string(), - current_flow: "captured".to_string(), - current_value: capture_method.to_string(), - states: "manual_single, manual_multiple, scheduled".to_string() - })), + || { + Err(report!(errors::ApiErrorResponse::PaymentUnexpectedState { + field_name: "capture_method".to_string(), + current_flow: "captured".to_string(), + current_value: capture_method.to_string(), + states: "manual_single, manual_multiple, scheduled".to_string() + })) + }, ) } @@ -929,12 +935,14 @@ pub(crate) fn validate_capture_method( pub(crate) fn validate_status(status: storage_enums::IntentStatus) -> RouterResult<()> { utils::when( status != storage_enums::IntentStatus::RequiresCapture, - Err(report!(errors::ApiErrorResponse::PaymentUnexpectedState { - field_name: "payment.status".to_string(), - current_flow: "captured".to_string(), - current_value: status.to_string(), - states: "requires_capture".to_string() - })), + || { + Err(report!(errors::ApiErrorResponse::PaymentUnexpectedState { + field_name: "payment.status".to_string(), + current_flow: "captured".to_string(), + current_value: status.to_string(), + states: "requires_capture".to_string() + })) + }, ) } @@ -945,9 +953,11 @@ pub(crate) fn validate_amount_to_capture( ) -> RouterResult<()> { utils::when( amount_to_capture.is_some() && (Some(amount) < amount_to_capture), - Err(report!(errors::ApiErrorResponse::InvalidRequestData { - message: "amount_to_capture is greater than amount".to_string() - })), + || { + Err(report!(errors::ApiErrorResponse::InvalidRequestData { + message: "amount_to_capture is greater than amount".to_string() + })) + }, ) } @@ -983,12 +993,11 @@ pub(super) async fn filter_by_constraints( pub(super) fn validate_payment_list_request( req: &api::PaymentListConstraints, ) -> CustomResult<(), errors::ApiErrorResponse> { - utils::when( - req.limit > 100 || req.limit < 1, + utils::when(req.limit > 100 || req.limit < 1, || { Err(errors::ApiErrorResponse::InvalidRequestData { message: "limit should be in between 1 and 100".to_string(), - }), - )?; + }) + })?; Ok(()) } @@ -1236,10 +1245,9 @@ pub(crate) fn authenticate_client_secret( payment_intent_client_secret: Option<&String>, ) -> Result<(), errors::ApiErrorResponse> { match (request_client_secret, payment_intent_client_secret) { - (Some(req_cs), Some(pi_cs)) => utils::when( - req_cs.ne(pi_cs), - Err(errors::ApiErrorResponse::ClientSecretInvalid), - ), + (Some(req_cs), Some(pi_cs)) => utils::when(req_cs.ne(pi_cs), || { + Err(errors::ApiErrorResponse::ClientSecretInvalid) + }), _ => Ok(()), } } @@ -1248,12 +1256,11 @@ pub(crate) fn validate_pm_or_token_given( token: &Option, pm_data: &Option, ) -> Result<(), errors::ApiErrorResponse> { - utils::when( - token.is_none() && pm_data.is_none(), + utils::when(token.is_none() && pm_data.is_none(), || { Err(errors::ApiErrorResponse::InvalidRequestData { message: "A payment token or payment method data is required".to_string(), - }), - ) + }) + }) } // A function to perform database lookup and then verify the client secret diff --git a/crates/router/src/core/payments/operations/payment_confirm.rs b/crates/router/src/core/payments/operations/payment_confirm.rs index 9ea8a2dfaf..d84db93877 100644 --- a/crates/router/src/core/payments/operations/payment_confirm.rs +++ b/crates/router/src/core/payments/operations/payment_confirm.rs @@ -222,10 +222,9 @@ impl Domain for PaymentConfirm { ) .await?; - utils::when( - payment_method.is_none(), - Err(errors::ApiErrorResponse::PaymentMethodNotFound), - )?; + utils::when(payment_method.is_none(), || { + Err(errors::ApiErrorResponse::PaymentMethodNotFound) + })?; Ok((op, payment_method, payment_token)) } diff --git a/crates/router/src/core/payments/operations/payment_status.rs b/crates/router/src/core/payments/operations/payment_status.rs index b177340d9d..d0682df3c8 100644 --- a/crates/router/src/core/payments/operations/payment_status.rs +++ b/crates/router/src/core/payments/operations/payment_status.rs @@ -251,12 +251,14 @@ async fn get_tracker_for_sync< utils::when( request.force_sync && !helpers::can_call_connector(payment_intent.status), - Err(ApiErrorResponse::InvalidRequestData { - message: format!( - "cannot perform force_sync as status: {}", - payment_intent.status - ), - }), + || { + Err(ApiErrorResponse::InvalidRequestData { + message: format!( + "cannot perform force_sync as status: {}", + payment_intent.status + ), + }) + }, )?; let refunds = db diff --git a/crates/router/src/core/refunds.rs b/crates/router/src/core/refunds.rs index fd64a31fd8..e044d16421 100644 --- a/crates/router/src/core/refunds.rs +++ b/crates/router/src/core/refunds.rs @@ -48,14 +48,13 @@ pub async fn refund_create_core( amount = req.amount.unwrap_or(payment_attempt.amount); // FIXME: Need to that capture amount //TODO: Can we change the flow based on some workflow idea - utils::when( - amount <= 0, + utils::when(amount <= 0, || { Err(report!(errors::ApiErrorResponse::InvalidDataFormat { field_name: "amount".to_string(), expected_format: "positive integer".to_string() }) - .attach_printable("amount less than zero")), - )?; + .attach_printable("amount less than zero")) + })?; payment_intent = db .find_payment_intent_by_payment_id_merchant_id( @@ -68,8 +67,10 @@ pub async fn refund_create_core( utils::when( payment_intent.status != enums::IntentStatus::Succeeded, - Err(report!(errors::ApiErrorResponse::PaymentNotSucceeded) - .attach_printable("unable to refund for a unsuccessful payment intent")), + || { + Err(report!(errors::ApiErrorResponse::PaymentNotSucceeded) + .attach_printable("unable to refund for a unsuccessful payment intent")) + }, )?; validate_and_create_refund( @@ -363,14 +364,13 @@ pub async fn validate_and_create_refund( .as_ref() .map(|merchant_id| merchant_id != &merchant_account.merchant_id); - utils::when( - predicate.unwrap_or(false), + utils::when(predicate.unwrap_or(false), || { Err(report!(errors::ApiErrorResponse::InvalidDataFormat { field_name: "merchant_id".to_string(), expected_format: "merchant_id from merchant account".to_string() }) - .attach_printable("invalid merchant_id in request")), - )?; + .attach_printable("invalid merchant_id in request")) + })?; let refund = match validator::validate_uniqueness_of_refund_id_against_merchant_id( db, diff --git a/crates/router/src/core/refunds/validator.rs b/crates/router/src/core/refunds/validator.rs index 03d9277c58..593269b60f 100644 --- a/crates/router/src/core/refunds/validator.rs +++ b/crates/router/src/core/refunds/validator.rs @@ -60,9 +60,11 @@ pub fn validate_refund_amount( utils::when( refund_amount > (payment_attempt_amount - total_refunded_amount), - Err(report!( - RefundValidationError::RefundAmountExceedsPaymentAmount - )), + || { + Err(report!( + RefundValidationError::RefundAmountExceedsPaymentAmount + )) + }, ) } @@ -74,7 +76,7 @@ pub fn validate_payment_order_age( utils::when( (current_time - *created_at).whole_days() > REFUND_MAX_AGE, - Err(report!(RefundValidationError::OrderExpired)), + || Err(report!(RefundValidationError::OrderExpired)), ) } @@ -83,10 +85,9 @@ pub fn validate_maximum_refund_against_payment_attempt( all_refunds: &[storage::Refund], ) -> CustomResult<(), RefundValidationError> { // TODO: Make this configurable - utils::when( - all_refunds.len() > REFUND_MAX_ATTEMPTS, - Err(report!(RefundValidationError::MaxRefundCountReached)), - ) + utils::when(all_refunds.len() > REFUND_MAX_ATTEMPTS, || { + Err(report!(RefundValidationError::MaxRefundCountReached)) + }) } #[instrument(skip(db))] diff --git a/crates/router/src/services/api.rs b/crates/router/src/services/api.rs index 1da2bd8a3a..c69105df6b 100644 --- a/crates/router/src/services/api.rs +++ b/crates/router/src/services/api.rs @@ -618,10 +618,9 @@ pub(crate) async fn authenticate_eph_key<'a>( .get_ephemeral_key(api_key) .await .change_context(errors::ApiErrorResponse::Unauthorized)?; - utils::when( - ek.customer_id.ne(&customer_id), - Err(report!(errors::ApiErrorResponse::InvalidEphermeralKey)), - )?; + utils::when(ek.customer_id.ne(&customer_id), || { + Err(report!(errors::ApiErrorResponse::InvalidEphermeralKey)) + })?; Ok(MerchantAuthentication::MerchantId(Cow::Owned( ek.merchant_id, ))) diff --git a/crates/router/src/services/encryption.rs b/crates/router/src/services/encryption.rs index b2da15d15f..fe69f76839 100644 --- a/crates/router/src/services/encryption.rs +++ b/crates/router/src/services/encryption.rs @@ -293,11 +293,10 @@ pub async fn decrypt_jwe( .into_report() .change_context(errors::EncryptionError) .attach_printable("Error getting Decrypted jwe")?; - utils::when( - resp_key_id.ne(key_id), + utils::when(resp_key_id.ne(key_id), || { Err(report!(errors::EncryptionError).attach_printable("Missing ciphertext blob")) - .attach_printable("key_id mismatch, Error authenticating response"), - )?; + .attach_printable("key_id mismatch, Error authenticating response") + })?; let resp = String::from_utf8(dst_payload) .into_report() .change_context(errors::EncryptionError) diff --git a/crates/router/src/utils/ext_traits.rs b/crates/router/src/utils/ext_traits.rs index f708d8680a..e686751bcc 100644 --- a/crates/router/src/utils/ext_traits.rs +++ b/crates/router/src/utils/ext_traits.rs @@ -31,13 +31,12 @@ where T: std::fmt::Debug, { fn check_value_present(&self, field_name: &str) -> RouterResult<()> { - when( - self.is_none(), + when(self.is_none(), || { Err(Report::new(ApiErrorResponse::MissingRequiredField { field_name: field_name.to_string(), }) - .attach_printable(format!("Missing required field {field_name} in {self:?}"))), - ) + .attach_printable(format!("Missing required field {field_name} in {self:?}"))) + }) } fn get_required_value(self, field_name: &str) -> RouterResult { @@ -105,13 +104,6 @@ pub(crate) fn merge_json_values(a: &mut serde_json::Value, b: &serde_json::Value } } -// TODO: change Name -pub trait ValidateVar { - fn validate(self) -> CustomResult - where - Self: std::marker::Sized; -} - pub trait ValidateCall { fn validate_opt(self, func: F) -> CustomResult<(), errors::ValidationError>; } diff --git a/crates/router/src/utils/fp_utils.rs b/crates/router/src/utils/fp_utils.rs index 65d7dda428..f38574a41c 100644 --- a/crates/router/src/utils/fp_utils.rs +++ b/crates/router/src/utils/fp_utils.rs @@ -1,48 +1,31 @@ -pub trait Kind { - type Wrapped; - type Wrapper; +pub trait Applicative { + type WrappedSelf; + + fn pure(v: R) -> Self::WrappedSelf; } -pub trait Applicative: Kind { - fn pure(v: Self::Wrapped) -> Self::Wrapper; -} - -impl Kind for Option { - type Wrapped = A; - type Wrapper = Option; -} - -impl Kind for Result { - type Wrapped = A; - type Wrapper = Result; -} - -impl Applicative for Option { - fn pure(v: A) -> Self::Wrapper { +impl Applicative for Option { + type WrappedSelf = Option; + fn pure(v: R) -> Self::WrappedSelf { Some(v) } } -impl Applicative for Result { - fn pure(v: A) -> Self::Wrapper { +impl Applicative for Result { + type WrappedSelf = Result; + fn pure(v: R) -> Self::WrappedSelf { Ok(v) } } -// FIXME: This method potentially encourages its users to allocate+free resources without need -// for example, in `check_value_present` below, this function is used as follows: -// when( -// self.is_none(), -// Err(Report::new(ValidateError) -// .attach_printable(format!("In {self:?} {key} has not found"))), -// ) -// This code allocates a `String` because of format! macro, and potentially allocates inside an error. -// The it should either replaced with `if` or the alternate argument should be a callback. -// Maybe there are other places with extra allocation? -pub fn when + Kind<(), Wrapped = (), Wrapper = F>>(predicate: bool, f: F) -> F { +// This function allows lazy evaluation of the `f` argument +pub fn when = W>, F>(predicate: bool, f: F) -> W +where + F: FnOnce() -> W, +{ if predicate { - f + f() } else { - F::pure(()) + W::pure(()) } }