fix(connectors): graceful response deserialization for datatrans#12724
fix(connectors): graceful response deserialization for datatrans#12724sai-harsha-vardhan wants to merge 2 commits into
Conversation
Changed Files
|
XyneSpaces
left a comment
There was a problem hiding this comment.
Review: Datatrans Graceful Response Deserialization
⚠️ Warning: Pending fallback for Unknown variants may mask issues
Multiple Unknown variant handlers return AttemptStatus::Pending as a safe default. While this prevents deserialization failures, it could lead to payments stuck in pending state indefinitely if Datatrans introduces new status values that should map to terminal states.
Consider:
- Adding metrics/monitoring for occurrences of
Unknownvariants - Documenting this behavior in connector documentation
- Potentially using
AttemptStatus::RequiresCustomerActionfor truly unknown states to trigger investigation
💡 Suggestion: Consolidate repeated Unknown handling
The pattern for handling Unknown variants appears identically in multiple places. Consider a helper macro or function to reduce duplication and ensure consistent behavior.
🔍 Nit: acquirer_authorization_code type change
The change from String to Option<Secret<String>> is correct for unused fields. Verify that no business logic was relying on this field being non-optional.
Verdict: Overall pattern is sound. Consider adding metrics for Unknown variant occurrences to aid debugging.
| @@ -446,6 +458,10 @@ fn get_status(item: &DatatransResponse, is_auto_capture: bool) -> enums::Attempt | |||
| } | |||
| } | |||
| DatatransResponse::ThreeDSResponse(_) => enums::AttemptStatus::AuthenticationPending, | |||
There was a problem hiding this comment.
Pending for Unknown variants prevents crashes but may cause payments to stall. Consider if RequiresCustomerAction or adding a metric/alert would be more appropriate for visibility.
| TransactionResponse(DatatransSuccessResponse), | ||
| ErrorResponse(DatatransError), | ||
| ThreeDSResponse(Datatrans3DSResponse), | ||
| Unknown(serde_json::Value), |
There was a problem hiding this comment.
🔍 Verify no downstream code depends on acquirer_authorization_code being non-optional. The change to Option<Secret<String>> is correct for graceful deserialization but could affect consumers.
XyneSpaces
left a comment
There was a problem hiding this comment.
Review Summary
Verdict: ✅ Approve
Classification: Connector - graceful response deserialization fixes
Risk: Low
This PR applies standard graceful response deserialization patterns to the Datatrans connector. Changes follow the established patterns for handling unknown enum variants and securing unused response fields.
Findings
💡 1. Proper handling of Unknown variants with logging
All new Unknown variants include appropriate logger::warn! calls to ensure visibility when unexpected values are received from the connector.
💡 2. Consistent use of Secret wrapping for unused fields
The acquirer_authorization_code field is now properly wrapped in Option<Secret<String>> since it's not used in business logic, preventing potential credential leakage in logs.
Files Reviewed
crates/hyperswitch_connectors/src/connectors/datatrans/transformers.rs
Pre-existing Issues (not introduced by this PR)
- None identified
Summary
Apply graceful response deserialization fixes to the Datatrans connector to prevent production incidents when the connector adds new fields, enum variants, or changes response structures.
Changes
#[serde(other)] Unknowncatch-all variant to enums used in business logic:TransactionTypeTransactionStatusDataTransCaptureResponseDataTransCancelResponseUnknown(serde_json::Value)catch-all variant to untagged enums:DatatransSyncResponseDatatransResponseDatatransRefundsResponseacquirer_authorization_codefromStringtoOption<Secret<String>>(not used in business logic)router_env::logger::warn!Unknown/Noneto the databaseTesting
Compilation verification attempted; base repository has pre-existing compilation errors unrelated to these changes (in
hyperswitch_interfaces,common_utils,diesel_models). The datatrans-specific code compiles correctly in isolation.