fix: resolve TODO and FIXME in utils module (#220)

crates/router/src/core/payments.rs

@@ -583,10 +583,9 @@ pub async fn list_payments(
         .into_iter()
         .map(ForeignInto::foreign_into)
         .collect();
-    utils::when(
+    utils::when(data.is_empty(), || {
-        data.is_empty(),
+        Err(errors::ApiErrorResponse::PaymentNotFound)
-        Err(errors::ApiErrorResponse::PaymentNotFound),
+    })?;
-    )?;
     Ok(services::BachResponse::Json(api::PaymentListResponse {
         size: data.len(),
         data,

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(
+    utils::when(merchant_id.ne(request_merchant_id), || {
-        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(
+                    utils::when(!amount_to_capture.le(&amount_inner.get()), || {
-                        !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(),
+            Err(report!(errors::ApiErrorResponse::PaymentUnexpectedState {
-            current_flow: "captured".to_string(),
+                field_name: "capture_method".to_string(),
-            current_value: capture_method.to_string(),
+                current_flow: "captured".to_string(),
-            states: "manual_single, manual_multiple, scheduled".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(),
+            Err(report!(errors::ApiErrorResponse::PaymentUnexpectedState {
-            current_flow: "captured".to_string(),
+                field_name: "payment.status".to_string(),
-            current_value: status.to_string(),
+                current_flow: "captured".to_string(),
-            states: "requires_capture".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(
+    utils::when(req.limit > 100 || req.limit < 1, || {
-        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(
+        (Some(req_cs), Some(pi_cs)) => utils::when(req_cs.ne(pi_cs), || {
-            req_cs.ne(pi_cs),
+            Err(errors::ApiErrorResponse::ClientSecretInvalid)
-            Err(errors::ApiErrorResponse::ClientSecretInvalid),
+        }),
-        ),
         _ => Ok(()),
     }
 }
@@ -1248,12 +1256,11 @@ pub(crate) fn validate_pm_or_token_given(
     token: &Option<String>,
     pm_data: &Option<api::PaymentMethod>,
 ) -> Result<(), errors::ApiErrorResponse> {
-    utils::when(
+    utils::when(token.is_none() && pm_data.is_none(), || {
-        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

crates/router/src/core/payments/operations/payment_confirm.rs

@@ -222,10 +222,9 @@ impl<F: Clone + Send> Domain<F, api::PaymentsRequest> for PaymentConfirm {
         )
         .await?;
 
-        utils::when(
+        utils::when(payment_method.is_none(), || {
-            payment_method.is_none(),
+            Err(errors::ApiErrorResponse::PaymentMethodNotFound)
-            Err(errors::ApiErrorResponse::PaymentMethodNotFound),
+        })?;
-        )?;
 
         Ok((op, payment_method, payment_token))
     }

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!(
+            Err(ApiErrorResponse::InvalidRequestData {
-                "cannot perform force_sync as status: {}",
+                message: format!(
-                payment_intent.status
+                    "cannot perform force_sync as status: {}",
-            ),
+                    payment_intent.status
-        }),
+                ),
+            })
+        },
     )?;
 
     let refunds = db

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(
+    utils::when(amount <= 0, || {
-        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(
+    utils::when(predicate.unwrap_or(false), || {
-        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,

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(
+    utils::when(all_refunds.len() > REFUND_MAX_ATTEMPTS, || {
-        all_refunds.len() > REFUND_MAX_ATTEMPTS,
+        Err(report!(RefundValidationError::MaxRefundCountReached))
-        Err(report!(RefundValidationError::MaxRefundCountReached)),
+    })
-    )
 }
 
 #[instrument(skip(db))]

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(
+        utils::when(ek.customer_id.ne(&customer_id), || {
-            ek.customer_id.ne(&customer_id),
+            Err(report!(errors::ApiErrorResponse::InvalidEphermeralKey))
-            Err(report!(errors::ApiErrorResponse::InvalidEphermeralKey)),
+        })?;
-        )?;
         Ok(MerchantAuthentication::MerchantId(Cow::Owned(
             ek.merchant_id,
         )))

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(
+    utils::when(resp_key_id.ne(key_id), || {
-        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)

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(
+        when(self.is_none(), || {
-            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<T> {
@@ -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<Self, errors::ValidationError>
-    where
-        Self: std::marker::Sized;
-}
-
 pub trait ValidateCall<T, F> {
     fn validate_opt(self, func: F) -> CustomResult<(), errors::ValidationError>;
 }

crates/router/src/utils/fp_utils.rs

@@ -1,48 +1,31 @@
-pub trait Kind<A> {
+pub trait Applicative<R> {
-    type Wrapped;
+    type WrappedSelf<T>;
-    type Wrapper;
+
+    fn pure(v: R) -> Self::WrappedSelf<R>;
 }
 
-pub trait Applicative<F>: Kind<F> {
+impl<R> Applicative<R> for Option<R> {
-    fn pure(v: Self::Wrapped) -> Self::Wrapper;
+    type WrappedSelf<T> = Option<T>;
-}
+    fn pure(v: R) -> Self::WrappedSelf<R> {
-
-impl<F, A> Kind<F> for Option<A> {
-    type Wrapped = A;
-    type Wrapper = Option<F>;
-}
-
-impl<F, A, B> Kind<F> for Result<A, B> {
-    type Wrapped = A;
-    type Wrapper = Result<F, B>;
-}
-
-impl<A> Applicative<A> for Option<A> {
-    fn pure(v: A) -> Self::Wrapper {
         Some(v)
     }
 }
 
-impl<A, E> Applicative<A> for Result<A, E> {
+impl<R, E> Applicative<R> for Result<R, E> {
-    fn pure(v: A) -> Self::Wrapper {
+    type WrappedSelf<T> = Result<T, E>;
+    fn pure(v: R) -> Self::WrappedSelf<R> {
         Ok(v)
     }
 }
 
-// FIXME: This method potentially encourages its users to allocate+free resources without need
+// This function allows lazy evaluation of the `f` argument
-// for example, in `check_value_present` below, this function is used as follows:
+pub fn when<W: Applicative<(), WrappedSelf<()> = W>, F>(predicate: bool, f: F) -> W
-// when(
+where
-//     self.is_none(),
+    F: FnOnce() -> W,
-//     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<F: Applicative<()> + Kind<(), Wrapped = (), Wrapper = F>>(predicate: bool, f: F) -> F {
     if predicate {
-        f
+        f()
     } else {
-        F::pure(())
+        W::pure(())
     }
 }