fix: return 400 instead of 500 for idempotency key reuse with different body #921
Conversation
WalkthroughThis update introduces enhanced idempotency support for account and transaction metadata operations, including documentation, client model, and API controller changes. A new error handler, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Controller
participant Ledger
Client->>API: DELETE /v2/{ledger}/transactions/{id}/metadata/{key} (Idempotency-Key)
API->>Controller: deleteTransactionMetadata (with Idempotency-Key)
Controller->>Ledger: DeleteTransactionMetadata
Ledger-->>Controller: Success or Error (idempotency conflict, not found, etc.)
Controller->>API: HandleCommonWriteErrors (maps error to HTTP response)
API-->>Client: 204 No Content / 409 Conflict / 400 Bad Request / 404 Not Found
sequenceDiagram
participant Test
participant API
participant Controller
participant Ledger
Test->>API: POST/DELETE Metadata with Idempotency-Key
API->>Controller: Add/Delete Metadata (Idempotency-Key)
Controller->>Ledger: Save/Delete Metadata
Ledger-->>Controller: Success or Idempotency Error
Controller->>API: HandleCommonWriteErrors
API-->>Test: Response (Success or Error)
Possibly related PRs
Suggested reviewers
Poem
✨ 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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #921 +/- ##
==========================================
+ Coverage 81.96% 82.42% +0.45%
==========================================
Files 141 141
Lines 7764 7766 +2
==========================================
+ Hits 6364 6401 +37
+ Misses 1074 1045 -29
+ Partials 326 320 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
internal/api/common/errors.go (2)
42-53: Exported function misses Go-doc comment
HandleCommonWriteErrorsis exported but lacks a leading doc comment.
This will triggergolint/staticcheckwarnings and makes the public API slightly harder to discover.+// HandleCommonWriteErrors translates write-time domain errors (idempotency, +// validation, not-found) into the appropriate HTTP responses. It should be used +// by controllers that mutate ledger state. func HandleCommonWriteErrors(w http.ResponseWriter, r *http.Request, err error) {
44-53: Consider grouping the validation branch for clarityMinor style: the 400 path currently uses
api.BadRequest, while the 404 path
usesapi.NotFound; both are validations of user input.
You could make intent clearer and slightly reduce duplication:case errors.Is(err, ledgercontroller.ErrInvalidIdempotencyInput{}), errors.Is(err, ledgercontroller.ErrNotFound): api.BadRequest(w, ErrValidation, err)Not essential, but it keeps the happy-path short and the switch symmetrical.
test/e2e/api_accounts_metadata_test.go (1)
216-244: Nit: reuse idempotency-key constants to improve readabilityYou repeatedly build the same string literal via
pointer.For("account-meta-key-2").
Defining a local constant (e.g.const ik = "account-meta-key-2") and re-using
pointer.For(ik)makes the intent clearer and prevents typos when the key is used many times.test/e2e/api_transactions_metadata_test.go (2)
168-208: Potential flakiness due to sharedtxIDacross specsAll specs in this
When("adding a metadata")block reuse the sametxID
initialised once in the outerBeforeEach.
Because individual specs mutate the transaction’s metadata
(AddMetadataOnTransaction,DeleteTransactionMetadata), they are order-insensitive today,
but any future spec that depends on initial metadata could sporadically fail.Consider moving the transaction creation into each inner
BeforeEachor using
JustBeforeEachto guarantee a clean slate per spec.
211-265: Minor duplication – factor outconst idempKey = "test-idempotency-key"Similar to the account tests, extracting the idempotency key string into a
constant improves readability and reduces the chance of mismatched literals.test/e2e/api_idempotency_test.go (2)
89-99: Verify the HTTP status code explicitlyThe test correctly checks for a validation error, but it would be beneficial to explicitly verify the HTTP status code is 400 (not 500) to directly validate the PR objective.
_, err = Wait(specContext, DeferClient(testServer)).Ledger.V2.CreateTransaction(ctx, txRequest) Expect(err).To(HaveOccurred()) Expect(err).To(HaveErrorCode(string(components.V2ErrorsEnumValidation))) + Expect(err).To(HaveHTTPStatusCode(400))
25-186: Consider adding a test case without idempotency keysWhile the existing tests thoroughly cover idempotency behavior, it might be valuable to include a test case that verifies operations work correctly without idempotency keys. This would provide complete coverage of possible usage patterns.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
openapi.yamlis excluded by!**/*.yamlopenapi/v2.yamlis excluded by!**/*.yamlpkg/client/.speakeasy/gen.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (24)
.gitignore(1 hunks)docs/api/README.md(2 hunks)internal/api/common/errors.go(2 hunks)internal/api/v1/controllers_accounts_add_metadata.go(2 hunks)internal/api/v1/controllers_accounts_delete_metadata.go(2 hunks)internal/api/v1/controllers_transactions_add_metadata.go(2 hunks)internal/api/v1/controllers_transactions_create.go(3 hunks)internal/api/v1/controllers_transactions_delete_metadata.go(2 hunks)internal/api/v2/controllers_accounts_add_metadata.go(2 hunks)internal/api/v2/controllers_accounts_delete_metadata.go(2 hunks)internal/api/v2/controllers_ledgers_delete_metadata.go(2 hunks)internal/api/v2/controllers_ledgers_update_metadata.go(2 hunks)internal/api/v2/controllers_transactions_add_metadata.go(2 hunks)internal/api/v2/controllers_transactions_create.go(2 hunks)internal/api/v2/controllers_transactions_delete_metadata.go(1 hunks)internal/api/v2/controllers_transactions_revert.go(1 hunks)pkg/client/docs/models/operations/v2deleteaccountmetadatarequest.md(1 hunks)pkg/client/docs/models/operations/v2deletetransactionmetadatarequest.md(1 hunks)pkg/client/models/operations/v2deleteaccountmetadata.go(2 hunks)pkg/client/models/operations/v2deletetransactionmetadata.go(2 hunks)pkg/client/v2.go(2 hunks)test/e2e/api_accounts_metadata_test.go(4 hunks)test/e2e/api_idempotency_test.go(1 hunks)test/e2e/api_transactions_metadata_test.go(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (16)
internal/api/v1/controllers_accounts_add_metadata.go (1)
internal/api/common/errors.go (1)
HandleCommonWriteErrors(42-53)
internal/api/v1/controllers_accounts_delete_metadata.go (1)
internal/api/common/errors.go (1)
HandleCommonWriteErrors(42-53)
internal/api/v1/controllers_transactions_create.go (1)
internal/api/common/errors.go (1)
HandleCommonWriteErrors(42-53)
internal/api/v2/controllers_ledgers_update_metadata.go (1)
internal/api/common/errors.go (1)
HandleCommonWriteErrors(42-53)
internal/api/v2/controllers_accounts_add_metadata.go (1)
internal/api/common/errors.go (1)
HandleCommonWriteErrors(42-53)
internal/api/v1/controllers_transactions_add_metadata.go (1)
internal/api/common/errors.go (1)
HandleCommonWriteErrors(42-53)
internal/api/v2/controllers_transactions_delete_metadata.go (1)
internal/api/common/errors.go (1)
HandleCommonWriteErrors(42-53)
internal/api/v2/controllers_accounts_delete_metadata.go (1)
internal/api/common/errors.go (1)
HandleCommonWriteErrors(42-53)
internal/api/v2/controllers_ledgers_delete_metadata.go (1)
internal/api/common/errors.go (1)
HandleCommonWriteErrors(42-53)
internal/api/v2/controllers_transactions_add_metadata.go (1)
internal/api/common/errors.go (1)
HandleCommonWriteErrors(42-53)
internal/api/v2/controllers_transactions_create.go (1)
internal/api/common/errors.go (1)
HandleCommonWriteErrors(42-53)
internal/api/v1/controllers_transactions_delete_metadata.go (1)
internal/api/common/errors.go (1)
HandleCommonWriteErrors(42-53)
internal/api/common/errors.go (1)
internal/controller/ledger/errors.go (3)
ErrIdempotencyKeyConflict(112-114)ErrInvalidIdempotencyInput(223-227)ErrNotFound(14-14)
test/e2e/api_transactions_metadata_test.go (5)
test/e2e/suite_test.go (1)
UseTemplatedDatabase(114-116)pkg/client/models/operations/v2gettransaction.go (1)
V2GetTransactionRequest(12-19)pkg/client/models/operations/v2deletetransactionmetadata.go (1)
V2DeleteTransactionMetadataRequest(11-20)pkg/client/models/operations/v2addmetadataontransaction.go (1)
V2AddMetadataOnTransactionRequest(11-22)pkg/client/models/components/v2errorsenum.go (1)
V2ErrorsEnumValidation(15-15)
test/e2e/api_accounts_metadata_test.go (4)
pkg/client/models/operations/v2addmetadatatoaccount.go (1)
V2AddMetadataToAccountRequest(9-24)pkg/client/models/operations/v2getaccount.go (1)
V2GetAccountRequest(11-22)pkg/client/models/components/v2errorsenum.go (1)
V2ErrorsEnumValidation(15-15)pkg/client/models/operations/v2deleteaccountmetadata.go (1)
V2DeleteAccountMetadataRequest(9-18)
test/e2e/api_idempotency_test.go (7)
test/e2e/suite_test.go (1)
UseTemplatedDatabase(114-116)pkg/client/models/components/v2transaction.go (1)
V2Transaction(11-24)pkg/client/models/operations/v2createtransaction.go (1)
V2CreateTransactionRequest(9-23)pkg/client/models/components/v2posttransaction.go (1)
V2PostTransaction(58-67)pkg/client/models/components/v2posting.go (1)
V2Posting(10-15)pkg/client/models/components/v2errorsenum.go (1)
V2ErrorsEnumValidation(15-15)pkg/client/models/operations/v2addmetadataontransaction.go (1)
V2AddMetadataOnTransactionRequest(11-22)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Tests
- GitHub Check: Dirty
🔇 Additional comments (39)
.gitignore (1)
12-12: Approve ignoring coverage.txt.Adding
coverage.txtto.gitignorehelps prevent committing coverage report artifacts produced by the new end-to-end tests, keeping the repo clean.internal/api/v2/controllers_transactions_revert.go (1)
10-10: Style improvement: Whitespace organization.The addition of a blank line after the "errors" import helps maintain consistent formatting throughout the codebase, separating the standard library imports from third-party imports.
internal/api/v1/controllers_transactions_add_metadata.go (2)
11-11: Style improvement: Whitespace organization.The addition of a blank line after the "errors" import helps maintain consistent formatting throughout the codebase, separating the standard library imports from third-party imports.
41-41: Improved error handling for idempotency key conflicts.The switch from
HandleCommonErrorstoHandleCommonWriteErrorsensures that idempotency key conflicts and invalid idempotency inputs are properly handled with appropriate HTTP status codes (400 instead of 500), providing better error responses to clients.internal/api/v2/controllers_transactions_add_metadata.go (2)
11-11: Style improvement: Whitespace organization.The addition of a blank line after the "errors" import helps maintain consistent formatting throughout the codebase, separating the standard library imports from third-party imports.
41-41: Improved error handling for idempotency key conflicts.The switch from
HandleCommonErrorstoHandleCommonWriteErrorsensures that idempotency key conflicts and invalid idempotency inputs are properly handled with appropriate HTTP status codes (400 instead of 500), providing better error responses to clients.internal/api/v2/controllers_accounts_add_metadata.go (3)
8-9: Style improvement: Import organization.The reordering of imports maintains a clean separation between standard library and third-party imports while preserving the same functionality.
11-11: Style improvement: Whitespace organization.The addition of a blank line after the "errors" import helps maintain consistent formatting throughout the codebase, separating the standard library imports from third-party imports.
38-38: Improved error handling for idempotency key conflicts.The switch to
HandleCommonWriteErrorsensures consistent handling of write-related errors, particularly for idempotency key conflicts. This change returns HTTP 400 instead of 500 when the same idempotency key is used with different request bodies, improving the API's reliability and client experience.internal/api/v1/controllers_accounts_delete_metadata.go (2)
7-8: Minor code organization improvement.Reordering the import statements to group them more logically helps with code readability.
29-29: Improved error handling for idempotency key conflicts.Replacing
HandleCommonErrorswithHandleCommonWriteErrorsensures that idempotency-related errors are handled with appropriate HTTP status codes, specifically:
ErrIdempotencyKeyConflictreturns 409 Conflict instead of 500ErrInvalidIdempotencyInputreturns 400 Bad Request instead of 500This change directly addresses the PR objective of improving error responses for idempotency key reuse scenarios.
pkg/client/docs/models/operations/v2deleteaccountmetadatarequest.md (1)
6-11: Documentation updated with idempotency key support.Good addition of the
IdempotencyKeyfield to the request model documentation. The formatting is consistent with other fields, and it clearly indicates that this is an optional field used for idempotency purposes.internal/api/v1/controllers_transactions_create.go (2)
104-104: Enhanced error handling for transaction creation with postings.Implementing the
HandleCommonWriteErrorshandler ensures that idempotency key conflicts during transaction creation with postings are properly handled with appropriate HTTP status codes (400 for invalid input, 409 for conflicts) rather than falling through to a generic error handler that might return 500.
139-139: Enhanced error handling for transaction creation with scripts.Similar to the previous change, this ensures that idempotency key conflicts during transaction creation with scripts are properly handled with appropriate HTTP status codes rather than potentially returning a 500 error.
internal/api/v1/controllers_accounts_add_metadata.go (2)
8-10: Minor code organization improvement.Reordering the import statements to group related packages together improves code readability and organization.
43-43: Improved error handling for metadata operations.Using
HandleCommonWriteErrorsinstead ofHandleCommonErrorsensures proper handling of idempotency-related errors when adding account metadata. This change is consistent with the other API endpoints and directly addresses the PR objective of returning appropriate status codes (400/409) for idempotency key conflicts instead of 500 errors.internal/api/v2/controllers_accounts_delete_metadata.go (1)
30-30: Good error handling improvementThis change improves error handling by using
HandleCommonWriteErrorsinstead ofHandleCommonErrors. The specialized handler properly returns appropriate HTTP status codes (400, 404, 409) for idempotency-related errors, which aligns with the PR objective to return 400 instead of 500 for idempotency key reuse with different body.internal/api/v2/controllers_transactions_delete_metadata.go (1)
31-31: Simplified and improved error handlingGood change that replaces specific error handling logic with the centralized
HandleCommonWriteErrorsfunction. This improves the handling of idempotency-related errors by returning the appropriate status codes (particularly 400 for invalid idempotency input instead of 500) while keeping the code DRY.internal/api/v2/controllers_ledgers_update_metadata.go (1)
27-27: Better error handling for write operationsThis change appropriately replaces
HandleCommonErrorswithHandleCommonWriteErrorsfor better handling of idempotency-related errors. This ensures consistent error responses across all API endpoints, particularly returning 400 for invalid idempotency input rather than 500.internal/api/v1/controllers_transactions_delete_metadata.go (1)
31-31: Consistent error handling improvementGood improvement that aligns this v1 API handler with the same changes made to v2 handlers. Using
HandleCommonWriteErrorsprovides proper handling for idempotency-related errors, ensuring appropriate status codes (400 instead of 500 for invalid idempotency input) across both API versions.pkg/client/v2.go (2)
2345-2346: Enhanced idempotency support for DeleteAccountMetadataThe addition of
utils.PopulateHeaders(ctx, req, request, nil)allows the client to properly send idempotency keys in HTTP headers for account metadata deletion operations, supporting idempotent requests.
3772-3773: Enhanced idempotency support for DeleteTransactionMetadataThe addition of
utils.PopulateHeaders(ctx, req, request, nil)enables the client to properly send idempotency keys in HTTP headers for transaction metadata deletion operations, maintaining consistent behavior with other API operations.pkg/client/docs/models/operations/v2deletetransactionmetadatarequest.md (1)
10-11: Documentation updated with IdempotencyKey fieldThe request model documentation is properly updated to include the new
IdempotencyKeyfield, clearly indicating it's optional and describing its purpose for idempotent operations.internal/api/v2/controllers_transactions_create.go (2)
7-8: Import reorderingSimple formatting change to the imports order, maintaining the same dependencies.
60-60: Improved error handling for idempotency keysChanged from generic error handling to specialized write operation error handling that properly handles idempotency-related errors. This ensures that idempotency key conflicts return appropriate HTTP status codes (400 for invalid input, 409 for conflicts) instead of 500 errors, providing better client feedback.
This change aligns with the PR objective to properly handle idempotency key reuse with different bodies, returning 400 Bad Request instead of 500 Internal Server Error.
internal/api/v2/controllers_ledgers_delete_metadata.go (1)
6-7: Improved error handling for idempotency in write operations.The change from
HandleCommonErrorstoHandleCommonWriteErrorsproperly handles idempotency-related errors with appropriate HTTP status codes instead of returning 500 errors.Also applies to: 17-17
docs/api/README.md (1)
969-984: Documentation update for idempotency support.The addition of the
Idempotency-Keyheader in the API documentation correctly reflects the new functionality for supporting idempotent delete operations on transaction metadata.pkg/client/models/operations/v2deleteaccountmetadata.go (2)
16-18: Added idempotency key support for account metadata deletion.The new
IdempotencyKeyfield with proper header annotation enables client applications to perform idempotent account metadata deletion operations.
41-46: Added getter method for the new IdempotencyKey field.The new getter method follows the pattern of other fields in the struct and properly handles nil receiver checks.
pkg/client/models/operations/v2deletetransactionmetadata.go (2)
18-19: Added idempotency key support for transaction metadata deletion.The new
IdempotencyKeyfield with proper header annotation enables client applications to perform idempotent transaction metadata deletion operations.
54-59: Added getter method for the new IdempotencyKey field.The new getter method follows the pattern of other fields in the struct and properly handles nil receiver checks.
internal/api/common/errors.go (1)
44-51:✅ Verification successful
Double-check
errors.Isbehaviour with value-receiver errorsYou match the targets with value literals (
ledgercontroller.ErrIdempotencyKeyConflict{}…).
That relies on the concrete error type implementing anIs(target error) boolmethod; otherwiseerrors.Is
will fallback to==comparison on the (possibly non-comparable) struct and never match.Please ensure both
ErrIdempotencyKeyConflictandErrInvalidIdempotencyInputimplementIs.
If they don’t, the handler will silently fall through toInternalServerError, returning 500 – exactly what the PR aims to avoid.
🏁 Script executed:
#!/usr/bin/env bash # Verify the error types implement an Is(target error) bool method. rg -n "type Err(IdempotencyKeyConflict|InvalidIdempotencyInput)" -A3 rg -n "func .*ErrIdempotencyKeyConflict.* Is(" || true rg -n "func .*ErrInvalidIdempotencyInput.* Is(" || trueLength of output: 2497
🏁 Script executed:
#!/usr/bin/env bash # Verify that both error types implement an Is(target error) bool method. # Check ErrIdempotencyKeyConflict rg -n '^func \(e ErrIdempotencyKeyConflict\) Is\(' -A3 internal/controller/ledger/errors.go # Check ErrInvalidIdempotencyInput rg -n '^func \(e ErrInvalidIdempotencyInput\) Is\(' -A3 internal/controller/ledger/errors.goLength of output: 444
Verified
errors.Isbehavior for idempotency errorsI confirmed in internal/controller/ledger/errors.go that both error types define:
func (e ErrIdempotencyKeyConflict) Is(err error) bool { … } func (e ErrInvalidIdempotencyInput) Is(err error) bool { … }This ensures
errors.Is(err, ledgercontroller.Err…)will match as intended. No further action needed.test/e2e/api_accounts_metadata_test.go (2)
171-214: Great end-to-end coverage for idempotency key success pathThe scenario exercises the same key / same body contract and asserts both the absence of errors and the persisted state – nice!
246-336: Solid negative-path assertionsThe tests correctly verify 409/400 behaviour for conflicting idempotency parameters – this will prevent regressions. 👍
test/e2e/api_transactions_metadata_test.go (1)
128-167: 👍 Idempotent delete scenarios thoroughly exercisedThe two-step delete with the same idempotency key and the state assertion afterwards
nicely prove correct handler wiring.test/e2e/api_idempotency_test.go (4)
1-48: Good test setup structureThe test environment is properly configured with templated database, NATS messaging, and appropriate instrumentation. The initial setup creates a default ledger before each test, which provides a clean starting point for the scenarios.
49-100: Well-implemented idempotency tests for transaction creationThe transaction creation tests thoroughly verify idempotency behavior:
- Successful reuse of the same key with identical data
- Validation error when attempting to reuse a key with different data
This aligns well with the PR objective to return 400 instead of 500 for idempotency key reuse with different body.
102-161: Effective idempotency testing for metadata operationsThe tests for adding metadata with idempotency keys appropriately verify:
- Initial operation succeeds
- The metadata is correctly added
- A subsequent identical operation succeeds
This is a thorough verification of the happy path for idempotent operations.
162-184: Good validation of error case for metadata operationsThe test properly verifies that attempting to use the same idempotency key with different metadata results in a validation error. This is a critical part of ensuring the system handles idempotency conflicts correctly.
Similar to the transaction creation test, consider explicitly verifying the HTTP status code is 400:
Expect(err).To(HaveOccurred()) Expect(err).To(HaveErrorCode(string(components.V2ErrorsEnumValidation))) + Expect(err).To(HaveHTTPStatusCode(400))
No description provided.