fix(payment-initiations): return CONFLICT error when multiple payments are initiated with the same reference #368
Conversation
WalkthroughThe pull request refactors the payment initiation creation flow by replacing the use of Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Service as PaymentInitiationsCreate
participant Storage
participant DB
Client->>Service: Create Payment Initiation Request
Service->>Storage: Call PaymentInitiationsInsert
Storage->>DB: Begin transaction & insert record
alt Insert Successful
DB-->>Storage: Success
Storage-->>Service: Return success
else Duplicate Key Error
DB-->>Storage: Return error
Storage->>Storage: rollbackOnTxError(tx, error)
Storage-->>Service: Return error
end
Service-->>Client: Respond with result/error
Assessment against linked issues
Possibly related PRs
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #368 +/- ##
==========================================
- Coverage 73.43% 72.87% -0.56%
==========================================
Files 539 530 -9
Lines 26654 25812 -842
==========================================
- Hits 19573 18811 -762
+ Misses 6064 5985 -79
+ Partials 1017 1016 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
.codecov.ymlis excluded by!**/*.ymlopenapi.yamlis excluded by!**/*.yamlopenapi/v1-2/v1-2.yamlis excluded by!**/*.yaml
📒 Files selected for processing (26)
internal/api/services/payment_initiations_create.go(1 hunks)internal/api/services/payment_initiations_create_test.go(2 hunks)internal/api/v2/handler_transfer_initiations_create.go(0 hunks)internal/api/v2/handler_transfer_initiations_create_test.go(2 hunks)internal/api/v3/handler_payment_initiations_create_test.go(3 hunks)internal/connectors/engine/plugins/plugin_generated.go(1 hunks)internal/connectors/plugins/public/atlar/client/client_generated.go(1 hunks)internal/storage/accounts_test.go(4 hunks)internal/storage/balances_test.go(4 hunks)internal/storage/bank_accounts_test.go(6 hunks)internal/storage/connector_tasks_tree_test.go(3 hunks)internal/storage/connectors_test.go(6 hunks)internal/storage/error.go(2 hunks)internal/storage/events_test.go(4 hunks)internal/storage/payment_initiation_reversals_test.go(7 hunks)internal/storage/payment_initiations.go(1 hunks)internal/storage/payment_initiations_test.go(15 hunks)internal/storage/payments_test.go(8 hunks)internal/storage/pools_test.go(7 hunks)internal/storage/schedules_test.go(5 hunks)internal/storage/states_test.go(3 hunks)internal/storage/storage.go(1 hunks)internal/storage/storage_generated.go(1 hunks)internal/storage/tasks_test.go(3 hunks)internal/storage/webhooks_test.go(3 hunks)internal/storage/workflow_instances_test.go(4 hunks)
💤 Files with no reviewable changes (1)
- internal/api/v2/handler_transfer_initiations_create.go
🧰 Additional context used
🧬 Code Definitions (14)
internal/storage/connectors_test.go (1)
internal/storage/storage.go (1) (1)
store(144-151)
internal/storage/connector_tasks_tree_test.go (1)
internal/storage/storage.go (1) (1)
store(144-151)
internal/storage/error.go (1)
internal/storage/payment_initiations.go (3) (3)
tx(70-70)err(236-236)err(470-470)
internal/storage/events_test.go (1)
internal/storage/storage.go (1) (1)
store(144-151)
internal/storage/accounts_test.go (1)
internal/storage/storage.go (1) (1)
store(144-151)
internal/storage/schedules_test.go (1)
internal/storage/storage.go (1) (1)
store(144-151)
internal/storage/bank_accounts_test.go (1)
internal/storage/storage.go (1) (1)
store(144-151)
internal/storage/pools_test.go (1)
internal/storage/storage.go (1) (1)
store(144-151)
internal/api/v2/handler_transfer_initiations_create_test.go (2)
internal/api/v2/handler_transfer_initiations_create.go (1) (1)
CreateTransferInitiationRequest(19-31)internal/api/v2/handler_test.go (2) (2)
prepareJSONRequest(33-38)assertExpectedResponse(24-31)
internal/storage/payments_test.go (1)
internal/storage/storage.go (1) (1)
store(144-151)
internal/storage/tasks_test.go (1)
internal/storage/storage.go (1) (1)
store(144-151)
internal/storage/payment_initiations.go (4)
internal/storage/payment_initiation_reversals.go (9) (9)
s(52-88)s(90-102)s(104-114)s(128-159)s(161-203)s(205-217)s(219-231)s(245-272)err(165-165)internal/models/payment_initiations.go (4) (4)
pi(68-108)pi(110-172)pi(197-246)PaymentInitiation(35-66)internal/storage/payments.go (1) (1)
err(311-311)internal/storage/error.go (2) (2)
e(21-40)rollbackOnTxError(43-51)
internal/storage/payment_initiation_reversals_test.go (1)
internal/storage/storage.go (1) (1)
store(144-151)
internal/storage/payment_initiations_test.go (1)
internal/storage/payment_initiations.go (4) (4)
err(236-236)err(470-470)pi(112-112)pi(143-143)
🪛 GitHub Check: codecov/patch
internal/storage/error.go
[warning] 49-50: internal/storage/error.go#L49-L50
Added lines #L49 - L50 were not covered by tests
🔇 Additional comments (53)
internal/storage/connectors_test.go (1)
64-64: Good resource management practice added.Adding
defer store.Close()after each store initialization ensures proper cleanup of database connections when test functions complete. This prevents potential resource leaks and follows best practices for resource management in Go.Also applies to: 107-107, 136-136, 183-183, 211-211, 241-241
internal/storage/error.go (1)
4-4: Required imports for new rollback functionality.These imports are necessary for the new transaction rollback function.
Also applies to: 8-8, 11-11
internal/connectors/engine/plugins/plugin_generated.go (1)
24-24: Auto-generated gomock field.The
isgomockfield is automatically added by the gomock code generator and serves as a marker to indicate that this struct is a mock implementation. No manual changes should be made to this generated file.internal/storage/connector_tasks_tree_test.go (1)
80-80: Good resource management practice added.Adding
defer store.Close()after each store initialization ensures proper cleanup of database connections when test functions complete. This is part of a consistent pattern applied across multiple test files in this PR to improve resource management.Also applies to: 106-106, 131-131
internal/storage/accounts_test.go (4)
138-138: Good resource management practice added.Adding
defer store.Close()ensures that the database connections and resources are properly cleaned up after the test completes, regardless of whether it passes or fails. This prevents potential resource leaks during testing.
210-210: Consistent resource cleanup implementation.Adding
defer store.Close()right after store initialization follows the same pattern as in other test functions, ensuring proper resource management throughout the test suite.
241-241: Proper resource cleanup added.The
defer store.Close()statement ensures database connections are properly released after test completion, which is essential for preventing resource exhaustion during test runs.
293-293: Good resource management practice added.Adding
defer store.Close()ensures consistent resource cleanup across all test functions, which is particularly important in this test that creates multiple database records.internal/storage/states_test.go (3)
52-52: Proper resource cleanup added.Adding
defer store.Close()ensures that database connections are properly released after test execution, preventing potential resource leaks in the test suite.
97-97: Good resource management practice added.The
defer store.Close()statement ensures proper cleanup of database resources after the test completes, following the consistent pattern established throughout the test suite.
126-126: Consistent resource cleanup implementation.Adding
defer store.Close()right after store initialization ensures proper resource management and follows the same pattern implemented in other test functions.internal/storage/schedules_test.go (5)
47-47: Good resource management practice added.Adding
defer store.Close()ensures that database connections are properly released after test completion, preventing potential resource leaks during test execution.
87-87: Proper resource cleanup added.The
defer store.Close()statement ensures consistent resource management across all test functions, following best practices for database connection handling in tests.
123-123: Consistent resource cleanup implementation.Adding
defer store.Close()right after store initialization ensures proper cleanup of database resources even if the test fails or panics.
154-154: Good resource management practice added.The
defer store.Close()statement ensures proper cleanup of database connections, which is particularly important in test functions that perform multiple database operations.
179-179: Proper resource cleanup added.Adding
defer store.Close()follows the consistent pattern implemented throughout the test suite, ensuring proper database connection management during test execution.internal/storage/events_test.go (4)
62-62: Good resource management practice added.Adding
defer store.Close()ensures that database connections are properly released after test completion, preventing potential resource leaks during testing.
111-111: Proper resource cleanup added.The
defer store.Close()statement ensures consistent resource management and cleanup, following the pattern implemented throughout the test suite.
141-141: Consistent resource cleanup implementation.Adding
defer store.Close()right after store initialization ensures proper cleanup of database resources, even if the test fails or panics.
170-170: Good resource management practice added.The
defer store.Close()statement ensures proper cleanup of database connections, which is essential for preventing resource exhaustion during the test suite execution.internal/storage/pools_test.go (2)
57-57: Improved resource management with defer statement.Adding
defer store.Close()ensures proper cleanup of the store resource after test completion, preventing potential resource leaks.
113-113: Consistent resource cleanup pattern throughout test suite.These defer statements ensure proper cleanup of database connections across all test functions in the file, following Go best practices for resource management.
Also applies to: 142-142, 181-181, 218-218, 253-253, 284-284
internal/storage/workflow_instances_test.go (1)
57-57: Proper resource cleanup with defer statement.The addition of
defer store.Close()statements ensures database connections are properly closed after each test completes, regardless of test outcome.Also applies to: 105-105, 149-149, 190-190
internal/storage/bank_accounts_test.go (1)
69-69: Enhanced resource management with deferred cleanup.The addition of
defer store.Close()statements ensures that database connections are properly cleaned up, even in case of test failure, preventing resource leaks.Also applies to: 132-132, 233-233, 309-309, 599-599, 691-691
internal/storage/payments_test.go (1)
251-251: Improved test hygiene with consistent resource cleanup.Adding
defer store.Close()statements throughout the test functions ensures proper cleanup of database connections, preventing potential resource leaks during test execution.Also applies to: 585-585, 602-602, 657-657, 686-686, 751-751, 786-786, 860-860
internal/storage/tasks_test.go (3)
77-77: Great addition of resource cleanup!Adding the
defer store.Close()statement ensures proper database connection cleanup after the test completes, which helps prevent potential resource leaks.
140-140: Good resource management practice!Adding
defer store.Close()ensures the store is properly closed after test execution regardless of success or failure outcomes.
166-166: Consistent resource cleanup implementation!This completes the pattern of adding proper resource cleanup across all test functions in this file.
internal/storage/webhooks_test.go (3)
49-49: Good addition of resource cleanup!Adding
defer store.Close()ensures proper resource management in the test function.
87-87: Proper resource management!Adding
defer store.Close()after store initialization maintains consistent cleanup patterns across tests.
113-113: Consistent resource cleanup implementation!This completes the pattern of proper store resource cleanup across all test functions in this file.
internal/api/services/payment_initiations_create.go (2)
27-27: Consistent update to insert instead of upser 8000 tThis change completes the replacement of
PaymentInitiationsUpsertwithPaymentInitiationsInsertthroughout the function, ensuring consistent behavior.
24-24:✅ Verification successful
Critical fix: Changed from upsert to insert for conflict detection
This change from
PaymentInitiationsUpserttoPaymentInitiationsInsertis key to ensuring duplicate payment references return a conflict error rather than silently overwriting existing records.Let's verify that the implementation in the storage layer can indeed return a conflict error:
🏁 Script executed:
#!/bin/bash # Search for the implementation of the PaymentInitiationsInsert method rg "func.*PaymentInitiationsInsert" -A 10Length of output: 3569
Conflict Error Handling Verified
The storage layer now uses
PaymentInitiationsInsertto ensure that attempts to create duplicate payment references trigger a conflict error rather than overwriting an existing record. The search confirms that:
- The
PaymentInitiationsInsertimplementation is present ininternal/storage/payment_initiations.goand wraps the insert operation within a transaction.- There is a dedicated test (
TestPaymentInitiationsInsertininternal/storage/payment_initiations_test.go) that covers this functionality.Note: Although the helper error message still references “upsert” (i.e.,
e("upsert payment initiations", err)), this does not affect the conflict-detection behavior. Consider updating the error text for clarity if needed.internal/storage/balances_test.go (4)
73-73: Improved resource management!Adding
defer store.Close()ensures proper database connection cleanup after test completion.
290-290: Good resource cleanup practice!Adding
defer store.Close()maintains consistent resource management across tests.
327-327: Proper store cleanup added!This ensures the database connection is properly closed after test execution.
580-580: Consistent resource management implementation!Adding
defer store.Close()completes the pattern of proper cleanup across all test functions in this file.internal/storage/payment_initiation_reversals_test.go (1)
86-86: Good resource management with deferred closures.Adding
defer store.Close()statements in all test functions ensures proper cleanup of database resources, preventing potential resource leaks during test execution.Also applies to: 132-132, 162-162, 198-198, 578-578, 627-627, 656-656
internal/storage/storage.go (1)
68-68: Method signature change aligns with the PR objective.Changing
PaymentInitiationsUpserttoPaymentInitiationsInsertaccurately reflects the new behavior of returning a conflict error rather than silently handling duplicates. This interface change is a key part of implementing the PR's objective to return CONFLICT errors for duplicate payment references.internal/api/v3/handler_payment_initiations_create_test.go (1)
78-95: Good test coverage for the new duplicate reference handling.This test case verifies that when a duplicate key error occurs during payment initiation creation, the API properly returns a CONFLICT error message with a BadRequest status code. This test is essential for validating the PR's objective to handle duplicate payment references correctly.
internal/storage/payment_initiations.go (2)
69-77: Improved error handling with named return and deferred rollback.The function has been refactored to use a named return variable and a deferred function for transaction rollback, which is a more robust pattern for handling database transactions. This ensures that transaction resources are properly released if an error occurs.
85-87: Key change: Removed conflict resolution clause to detect duplicate references.Removing the
On("CONFLICT (id) DO NOTHING")clause is the core change that implements the PR objective. Now, when a duplicate payment reference is inserted, the database will raise a constraint violation error instead of silently ignoring the conflict. This error is then propagated to the caller, allowing the API to return a CONFLICT response to the client.internal/api/services/payment_initiations_create_test.go (2)
80-85: Good test coverage for duplicate key error handling.This new test case appropriately validates the handling of duplicate key errors in the payment initiations creation flow, ensuring they are correctly wrapped with descriptive error messages.
96-96: Correctly updated mock expectation from upsert to insert.The mock expectation has been properly updated to use
PaymentInitiationsInsertinstead ofPaymentInitiationsUpsert, aligning with the storage layer changes.internal/api/v2/handler_transfer_initiations_create_test.go (3)
5-5: Correctly added fmt import.The added import is necessary for using
fmt.Errorfin the new test case.
13-13: Appropriately imported storage package.The storage package import is required to access the
ErrDuplicateKeyValueconstant.
71-88: Well-implemented conflict test case for duplicate payment references.This test case properly verifies that a CONFLICT error is returned when attempting to create payment initiations with duplicate references. The test constructs an error with the storage.ErrDuplicateKeyValue wrapped inside it, which aligns with the PR's objective to handle duplicate payment references properly.
internal/storage/payment_initiations_test.go (4)
88-88: Function updated to use insert instead of upsert.The change properly reflects the transition from upsert to insert semantics in the storage layer.
93-93: Test function name updated to match implementation change.The test function name has been correctly updated from
TestPaymentInitiationsUpserttoTestPaymentInitiationsInsertto reflect the functional change.
98-98: Improved resource cleanup with defer store.Close().Adding
defer store.Close()statements to all test functions ensures proper cleanup of database resources, which is an important practice to prevent resource leaks during testing.Also applies to: 147-147, 201-201, 230-230, 259-259, 294-294, 738-738, 809-809, 953-953, 998-999, 1058-1058, 1086-1086
117-139: Properly updated test case for insert behavior with duplicate key detection.The test case has been updated to verify that attempting to insert an entity with a duplicate ID results in a
ErrDuplicateKeyValueerror, ensuring the conflict detection mechanism works as expected.internal/storage/storage_generated.go (1)
784-801: Added mock implementation for PaymentInitiationsInsert.The mock implementation has been properly added to support the transition from upsert to insert semantics in the storage layer. The implementation correctly handles variadic arguments for adjustments and properly records method calls.
internal/connectors/plugins/public/atlar/client/client_generated.go (1)
216-252: MockErrorCodeReader implementation looks good for testing error handlingThe addition of the
MockErrorCodeReaderimplementation provides essential mocking capabilities for testing components that interact with theErrorCodeReaderinterface. This aligns with the PR objective of handling CONFLICT errors when multiple payments are initiated with the same reference.The mock follows standard gomock patterns and maintains consistency with the existing
MockClientimplementation. TheCode()method will allow tests to simulate specific error codes, which is crucial for verifying the correct handling of conflict scenarios.
| // meant to be called in defer block | ||
| func rollbackOnTxError(ctx context.Context, tx bun.Tx, err error) { | ||
| if err == nil { | ||
| return | ||
| } | ||
|
|
||
| if rollbackErr := tx.Rollback(); rollbackErr != nil { | ||
| logging.FromContext(ctx).WithField("original_error", err.Error()).Errorf("failed to rollback transaction: %w", rollbackErr) | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Centralized transaction rollback function improves error handling.
The new rollbackOnTxError function centralizes transaction rollback logic, ensuring consistent error handling across the codebase when database operations fail. This supports the PR objective of properly handling conflicts when duplicate payment references are encountered.
Note: The error logging in this function is not covered by tests.
Consider adding test coverage for the rollback error scenario at lines 48-50.
🏁 Script executed:
#!/bin/bash
# Find other places where this function is now being used
rg -A 2 "rollbackOnTxError" --type goLength of output: 372
Action Required: Enhance Test Coverage for Rollback Error Logging
The new rollbackOnTxError function does centralize transaction rollback logic and is used in both internal/storage/error.go and internal/storage/payment_initiations.go. However, the error logging path (lines 48–50 in internal/storage/error.go) is currently untested.
- Please add tests that simulate a rollback failure scenario to verify that the error is logged as expected.
- Confirm that calling
rollbackOnTxErrorin both contexts behaves correctly when rollback errors occur.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 49-50: internal/storage/error.go#L49-L50
Added lines #L49 - L50 were not covered by tests
…s are initiated with the same reference (#368) * Return duplicate key error if two payment initiation creation attempts share a primary key * Return error with CONFLICT code when payment initiation already exists * Remove deprecated (v1) provider parameter from payment initiation request * Add generated connector files to codecov exclusion list * Make generic DB rollback error * Hold parallel db connections in storage unit tests only as long as needed
Fixes: PMNT-64