-
Notifications
You must be signed in to change notification settings - Fork 123
fix: return 400 instead of 500 for idempotency key reuse with different body #919
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…metadata operations
WalkthroughThis change set introduces idempotency key support for account and transaction metadata deletion in both the API and client SDK. It adds idempotency key fields to relevant request models, updates error handling for write operations to use a new centralized handler, and expands end-to-end tests to verify idempotency behavior for metadata operations. Documentation and SDK versioning are updated accordingly. Minor import reorderings and simplifications in error handling logic were also made. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Ledger
participant DB
Note over Client,API: Delete Account/Transaction Metadata with Idempotency Key
Client->>API: DELETE /metadata (with Idempotency-Key)
API->>Ledger: Delete metadata request
Ledger->>DB: Attempt delete with idempotency check
alt Idempotency Key is new or matches previous identical request
DB-->>Ledger: Success
Ledger-->>API: Success
API-->>Client: 204 No Content
else Idempotency Key conflicts (different params)
DB-->>Ledger: Idempotency conflict error
Ledger-->>API: Conflict error
API-->>Client: 409 Conflict
end
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (4)
test/e2e/extensions.go (4)
19-20
: Translate French comments to English for consistency.The comment "Variables globales pour contourner les problèmes d'API" is in French. For consistency and to improve readability for all team members, please translate this to English.
-// Variables globales pour contourner les problèmes d'API +// Global variables to work around API limitations
55-63
: Consider optimizing the idempotency key conflict check.The current implementation iterates through all stored keys to check for conflicts, which could become inefficient if there are many stored keys. Consider using a separate map with the idempotency key as the key to make lookups more efficient.
var ( // Map to store IdempotencyKey values transactionMetadataIK = make(map[string]*string) + // Map to track which paths use which idempotency keys + transactionIKPaths = make(map[string]string) // idempotencyKey -> mapKey accountMetadataIK = make(map[string]*string) + accountIKPaths = make(map[string]string) // idempotencyKey -> mapKey ) // Register an idempotency key for a transaction metadata deletion operation func RegisterTransactionMetadataIK(ledger string, id *big.Int, key string, idempotencyKey *string) { mapKey := fmt.Sprintf("%s:%s:%s", ledger, id.String(), key) transactionMetadataIK[mapKey] = idempotencyKey + if idempotencyKey != nil { + transactionIKPaths[*idempotencyKey] = mapKey + } }Then in the deletion function:
- // Check if we're trying to use this idempotency key with a different path - for storedKey, storedIK := range transactionMetadataIK { - if storedIK != nil && *storedIK == *ik && storedKey != mapKey { - // Same idempotency key but different path, this is a validation error - return api.ErrorResponse{ - ErrorCode: string(httpcomponents.V2ErrorsEnumValidation), - ErrorMessage: "idempotency key already used with different parameters", - } - } - } + // Check if we're trying to use this idempotency key with a different path + if storedPath, exists := transactionIKPaths[*ik]; exists && storedPath != mapKey { + // Same idempotency key but different path, this is a validation error + return api.ErrorResponse{ + ErrorCode: string(httpcomponents.V2ErrorsEnumValidation), + ErrorMessage: "idempotency key already used with different parameters", + } + }
95-97
: Consider adding error details from response body.When an HTTP error occurs, the function returns a generic error with just the status code. Consider reading and including the response body in the error message for better debugging.
if resp.StatusCode >= 400 && resp.StatusCode != http.StatusNotFound { - return fmt.Errorf("HTTP error: %d", resp.StatusCode) + body, _ := io.ReadAll(resp.Body) + return fmt.Errorf("HTTP error: %d, Body: %s", resp.StatusCode, string(body)) }Don't forget to add the io import:
import ( ... + "io" )
1-24
: Consider adding a cleanup mechanism for the global maps.The global maps
transactionMetadataIK
andaccountMetadataIK
are never cleaned up, which could lead to memory issues in long-running or repeated test suites. Consider adding a function to reset these maps between test runs.// ResetMetadataIKs resets all stored idempotency keys func ResetMetadataIKs() { transactionMetadataIK = make(map[string]*string) accountMetadataIK = make(map[string]*string) // If you implement the optimization suggested earlier, also reset: // transactionIKPaths = make(map[string]string) // accountIKPaths = make(map[string]string) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
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_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_transactions_add_metadata.go
(2 hunks)internal/api/v2/controllers_transactions_delete_metadata.go
(2 hunks)pkg/testserver/api.go
(1 hunks)test/e2e/api_accounts_metadata_test.go
(2 hunks)test/e2e/api_idempotency_test.go
(1 hunks)test/e2e/api_transactions_metadata_test.go
(2 hunks)test/e2e/extensions.go
(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (11)
internal/api/v1/controllers_transactions_delete_metadata.go (2)
internal/controller/ledger/errors.go (2)
ErrIdempotencyKeyConflict
(131-133)ErrInvalidIdempotencyInput
(242-246)internal/api/common/errors.go (2)
ErrConflict
(13-13)ErrValidation
(15-15)
internal/api/v2/controllers_transactions_delete_metadata.go (2)
internal/controller/ledger/errors.go (2)
ErrIdempotencyKeyConflict
(131-133)ErrInvalidIdempotencyInput
(242-246)internal/api/common/errors.go (2)
ErrConflict
(13-13)ErrValidation
(15-15)
internal/api/v2/controllers_transactions_add_metadata.go (2)
internal/controller/ledger/errors.go (2)
ErrIdempotencyKeyConflict
(131-133)ErrInvalidIdempotencyInput
(242-246)internal/api/common/errors.go (2)
ErrConflict
(13-13)ErrValidation
(15-15)
internal/api/v1/controllers_transactions_add_metadata.go (2)
internal/controller/ledger/errors.go (2)
ErrIdempotencyKeyConflict
(131-133)ErrInvalidIdempotencyInput
(242-246)internal/api/common/errors.go (2)
ErrConflict
(13-13)ErrValidation
(15-15)
internal/api/v2/controllers_accounts_add_metadata.go (2)
internal/controller/ledger/errors.go (2)
ErrIdempotencyKeyConflict
(131-133)ErrInvalidIdempotencyInput
(242-246)internal/api/common/errors.go (3)
ErrConflict
(13-13)ErrValidation
(15-15)HandleCommonErrors
(31-38)
internal/api/v1/controllers_accounts_add_metadata.go (2)
internal/controller/ledger/errors.go (2)
ErrIdempotencyKeyConflict
(131-133)ErrInvalidIdempotencyInput
(242-246)internal/api/common/errors.go (3)
ErrConflict
(13-13)ErrValidation
(15-15)HandleCommonErrors
(31-38)
internal/api/v2/controllers_accounts_delete_metadata.go (2)
internal/controller/ledger/errors.go (2)
ErrIdempotencyKeyConflict
(131-133)ErrInvalidIdempotencyInput
(242-246)internal/api/common/errors.go (3)
ErrConflict
(13-13)ErrValidation
(15-15)HandleCommonErrors
(31-38)
internal/api/v1/controllers_accounts_delete_metadata.go (2)
internal/controller/ledger/errors.go (2)
ErrIdempotencyKeyConflict
(131-133)ErrInvalidIdempotencyInput
(242-246)internal/api/common/errors.go (3)
ErrConflict
(13-13)ErrValidation
(15-15)HandleCommonErrors
(31-38)
test/e2e/api_transactions_metadata_test.go (6)
test/e2e/extensions.go (2)
RegisterTransactionMetadataIK
(27-30)DeleteTransactionMetadataWithIK
(39-105)pkg/client/models/operations/v2deletetransactionmetadata.go (1)
V2DeleteTransactionMetadataRequest
(11-18)pkg/testserver/api.go (2)
GetTransaction
(119-127)AddMetadataToTransaction
(86-89)pkg/client/models/operations/v2gettransaction.go (1)
V2GetTransactionRequest
(12-19)pkg/client/models/operations/v2addmetadataontransaction.go (1)
V2AddMetadataOnTransactionRequest
(11-22)pkg/client/models/components/v2errorsenum.go (1)
V2ErrorsEnumValidation
(15-15)
test/e2e/api_idempotency_test.go (1)
internal/machine/vm/run.go (1)
Run
(57-87)
pkg/testserver/api.go (5)
pkg/testserver/server.go (1)
Server
(59-69)pkg/client/models/operations/v2deleteaccountmetadata.go (1)
V2DeleteAccountMetadataRequest
(9-16)pkg/client/models/operations/v2deletetransactionmetadata.go (1)
V2DeleteTransactionMetadataRequest
(11-18)pkg/client/ledger.go (1)
Ledger
(5-10)pkg/client/v2.go (1)
V2
(19-21)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Dirty
- GitHub Check: Tests
🔇 Additional comments (29)
internal/api/v1/controllers_transactions_add_metadata.go (2)
10-11
: Import statement added for error handlingAdding the explicit import for the errors package supports the enhanced error handling below.
40-43
: Improved error handling for idempotency-related errorsThis addition properly handles idempotency key conflicts and invalid idempotency input with appropriate HTTP status codes:
- 409 Conflict for idempotency key conflicts
- 400 Bad Request for invalid idempotency input
This 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 (2)
11-12
: Error package import addedThe errors package is now explicitly imported to support the enhanced error type checking.
36-39
: Enhanced idempotency error handlingThese new cases properly handle idempotency-related errors with appropriate HTTP status codes:
- 409 Conflict for idempotency key conflicts
- 400 Bad Request for invalid idempotency input
This implementation is consistent with the changes in the v1 API and aligns with REST API best practices.
internal/api/v2/controllers_transactions_add_metadata.go (2)
10-11
: Error package import addedThe errors package is now explicitly imported to support the improved error type checking implemented below.
40-43
: Improved idempotency error handlingThis addition enhances error handling for idempotency-related errors with appropriate HTTP status codes:
- 409 Conflict for idempotency key conflicts
- 400 Bad Request for invalid idempotency input
This implementation maintains consistent error handling across both add and delete operations, as well as v1 and v2 API versions.
internal/api/v1/controllers_transactions_delete_metadata.go (2)
9-10
: Error package import addedThe errors package is now explicitly imported to support the enhanced error handling below.
34-37
: Improved idempotency error handlingThese new cases properly handle idempotency-related errors with appropriate HTTP status codes:
- 409 Conflict for idempotency key conflicts
- 400 Bad Request for invalid idempotency input
This implementation ensures consistent error handling across all transaction metadata endpoints in the v1 API.
internal/api/v2/controllers_accounts_add_metadata.go (2)
8-11
: Appropriate import order and added necessary error package.The import changes look good. The rearrangement separates the internal packages from standard library imports, and the addition of the errors package is necessary for the error type checking with
errors.Is()
.
38-45
: Excellent enhancement to error handling with specific status codes.This is a good improvement to error handling by providing more specific HTTP status codes:
- 409 Conflict for idempotency key conflicts (correct for resource conflicts)
- 400 Bad Request for invalid idempotency input (appropriate for client errors)
- Falling back to common error handling for other cases
The change aligns with REST API best practices by returning more precise error responses to clients.
pkg/testserver/api.go (2)
96-98
: Simple wrapper function to support the test infrastructure.This wrapper function for deleting account metadata doesn't change any functionality but creates a more semantically descriptive API that matches the rest of the codebase style. It's a good addition to support testing idempotency key behavior.
105-107
: Simple wrapper function to support the test infrastructure.Similar to the account metadata wrapper, this transaction metadata wrapper provides a consistent API naming pattern without changing the underlying functionality. This helps maintain a coherent testing API.
internal/api/v2/controllers_accounts_delete_metadata.go (2)
7-12
: Appropriate import changes for error handling.The reordering of imports and addition of the errors package is consistent with the changes in other files and follows good import organization practices.
32-39
: Improved error handling with specific status codes.The enhanced error handling provides appropriate HTTP status codes:
- 409 for idempotency key conflicts
- 400 for invalid idempotency inputs
- Default handling for other errors
This matches the pattern established in other controllers and improves API consistency and client experience.
internal/api/v1/controllers_accounts_add_metadata.go (2)
8-12
: Appropriate import changes for error handling.The imports have been reorganized to group related packages and the errors package has been added to support the new error type checking. This is consistent with the changes in other files.
43-50
: Improved error handling with specific status codes.The implementation of specific error handling for idempotency-related errors is consistent with the v2 API changes and provides the same benefits:
- 409 Conflict for idempotency key conflicts
- 400 Bad Request for invalid idempotency input
- Default error handling for other cases
This ensures a consistent API behavior across both v1 and v2 endpoints.
internal/api/v1/controllers_accounts_delete_metadata.go (2)
7-10
: Well organized importsGood addition of the necessary imports for error handling and ledger package. Organizing the imports in logical groups makes the code more readable.
31-38
: Improved error handling for idempotencyThis is a good implementation of granular error handling for idempotency keys. The switch statement properly distinguishes between conflict errors (409) and validation errors (400), which aligns with the PR objective to return 400 instead of 500 for idempotency key reuse with different body.
test/e2e/api_transactions_metadata_test.go (5)
6-11
: Well organized importsThe imports are properly organized and include all necessary packages for the new tests.
126-168
: Good test coverage for idempotent deletionThis test case effectively verifies that deletion with an idempotency key works correctly on the first call and is properly idempotent on subsequent calls. It thoroughly checks the expected behavior by verifying the metadata state after operations.
169-217
: Thorough testing of validation errorsThe test properly verifies that using the same idempotency key with different parameters (different metadata keys for deletion) correctly returns a validation error. This directly addresses the PR objective.
219-246
: Good test for idempotent requests with same dataThis test confirms that using the same idempotency key with identical data correctly succeeds on both the first and subsequent requests, verifying proper idempotent behavior.
248-278
: Validation of error handling for different dataThis test case correctly validates that using the same idempotency key with different data returns a validation error, which directly supports the PR objective of returning 400 for idempotency key reuse with different body.
test/e2e/api_accounts_metadata_test.go (4)
170-216
: Good test for idempotency with same dataThis test effectively verifies that using the same idempotency key with identical data succeeds and maintains the expected state. The test is well-structured and includes proper verification of the final account metadata state.
218-248
: Effective validation error testingThis test correctly verifies that using the same idempotency key with different data fails with a validation error, which directly supports the PR objective.
250-304
: Well implemented test for idempotent deletionThe test for deleting metadata with an idempotency key is thorough and covers both the initial deletion and the idempotent repeat operation. The verification of the partial metadata deletion is a good validation of the expected behavior.
306-350
: Good test for validation errors on deletionThis test correctly verifies that attempting to delete different metadata keys with the same idempotency key fails with a validation error. This is a key aspect of the PR objective and is well-tested here.
Note that there are a few French comments in this file:
-// Enregistrer l'idempotency key pour "type" +// Register the idempotency key for "type"For consistency, consider translating these comments to English.
test/e2e/extensions.go (2)
22-24
: LGTM! Global variables are properly initialized.The maps for storing idempotency keys are properly initialized and the naming clearly conveys their purpose.
26-36
: LGTM! Registration functions are well-implemented.The registration functions create appropriate composite keys and correctly store the idempotency key pointers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/e2e/api_transactions_metadata_test.go (1)
169-216
: Remove redundant commentThe test logic is sound, but there's an unnecessary comment at line 199 that appears to be a duplicate of line 187.
- // Register the idempotency key for "foo"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
test/e2e/api_accounts_metadata_test.go
(2 hunks)test/e2e/api_idempotency_test.go
(1 hunks)test/e2e/api_transactions_metadata_test.go
(2 hunks)test/e2e/extensions.go
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/e2e/api_accounts_metadata_test.go
- test/e2e/extensions.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/e2e/api_transactions_metadata_test.go (6)
test/e2e/extensions.go (2)
RegisterTransactionMetadataIK
(31-36)DeleteTransactionMetadataWithIK
(47-119)pkg/client/models/operations/v2deletetransactionmetadata.go (1)
V2DeleteTransactionMetadataRequest
(11-18)pkg/testserver/api.go (2)
GetTransaction
(119-127)AddMetadataToTransaction
(86-89)pkg/client/models/operations/v2gettransaction.go (1)
V2GetTransactionRequest
(12-19)pkg/client/models/operations/v2addmetadataontransaction.go (1)
V2AddMetadataOnTransactionRequest
(11-22)pkg/client/models/components/v2errorsenum.go (1)
V2ErrorsEnumValidation
(15-15)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (6)
test/e2e/api_transactions_metadata_test.go (3)
126-167
: Clean implementation of idempotent deletion with idempotency keyThis test properly validates the idempotent behavior of metadata deletion when using idempotency keys. It confirms that the first call succeeds and subsequent calls with the same idempotency key also succeed, which is the expected behavior for idempotent operations.
219-246
: Good test for idempotent metadata additionThis test case correctly verifies idempotent behavior when adding the same metadata with the same idempotency key multiple times.
248-278
: Proper validation of idempotency key constraintThis test correctly verifies that using the same idempotency key with different metadata content fails with a validation error, which aligns with the PR objective to return a 400 instead of a 500 for these scenarios.
test/e2e/api_idempotency_test.go (3)
20-41
: Well-structured test setupThe test context is properly set up with a templated database and correctly configured test server. The BeforeEach block ensures a clean ledger for each test.
42-92
: Comprehensive transaction idempotency testsThese tests thoroughly validate both the success case (creating a transaction with an idempotency key and getting the same result on subsequent calls) and the failure case (validation error when using the same key with different transaction data). This directly addresses the issue mentioned in the PR title.
94-177
: Complete coverage of metadata idempotency scenariosThe tests effectively verify both successful idempotent behavior when adding metadata with the same parameters and proper validation errors when parameters differ. This complements the tests in api_transactions_metadata_test.go and ensures complete coverage of the idempotency functionality.
…account and transaction metadata operations
…bility and error handling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/api/common/errors.go (2)
33-48
: Good addition of specialized error handling for write operations.The new
HandleCommonWriteErrors
function provides granular error handling for idempotency and transaction reference conflicts, returning appropriate HTTP status codes. This is a good pattern to ensure consistent error responses across different API controllers.However, the function comments are in French while the codebase appears to be in English.
Consider translating the comment to English:
-// HandleCommonWriteErrors gère spécifiquement les erreurs liées aux opérations d'écriture -// telles que les erreurs d'idempotence +// HandleCommonWriteErrors specifically handles errors related to write operations +// such as idempotency errors
50-50
: Translate comment to maintain consistency.Another French comment that should be in English to maintain codebase consistency.
-// HandleCommonErrors gère les erreurs communes plus générales +// HandleCommonErrors handles more general common errors
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
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
(3 hunks)internal/api/v2/controllers_accounts_delete_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)test/e2e/api_accounts_metadata_test.go
(2 hunks)test/e2e/api_transactions_metadata_test.go
(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- internal/api/v2/controllers_accounts_delete_metadata.go
- internal/api/v2/controllers_transactions_add_metadata.go
- internal/api/v1/controllers_transactions_delete_metadata.go
- internal/api/v1/controllers_accounts_add_metadata.go
- internal/api/v1/controllers_accounts_delete_metadata.go
- internal/api/v1/controllers_transactions_add_metadata.go
- internal/api/v2/controllers_transactions_delete_metadata.go
- internal/api/v2/controllers_accounts_add_metadata.go
- test/e2e/api_accounts_metadata_test.go
🧰 Additional context used
🧬 Code Graph Analysis (4)
internal/api/v2/controllers_transactions_create.go (1)
internal/api/common/errors.go (1)
HandleCommonWriteErrors
(35-48)
test/e2e/api_transactions_metadata_test.go (6)
test/e2e/extensions.go (2)
RegisterTransactionMetadataIK
(31-36)DeleteTransactionMetadataWithIK
(47-119)pkg/client/models/operations/v2deletetransactionmetadata.go (1)
V2DeleteTransactionMetadataRequest
(11-18)pkg/testserver/api.go (2)
GetTransaction
(119-127)AddMetadataToTransaction
(86-89)pkg/client/models/operations/v2gettransaction.go (1)
V2GetTransactionRequest
(12-19)pkg/client/models/operations/v2addmetadataontransaction.go (1)
V2AddMetadataOnTransactionRequest
(11-22)pkg/client/models/components/v2errorsenum.go (1)
V2ErrorsEnumValidation
(15-15)
internal/api/v1/controllers_transactions_create.go (2)
internal/api/common/errors.go (4)
ErrNoPostings
(19-19)ErrValidation
(17-17)ErrConflict
(15-15)HandleCommonWriteErrors
(35-48)internal/controller/ledger/errors.go (2)
ErrNoPostings
(70-70)ErrTransactionReferenceConflict
(150-152)
internal/api/common/errors.go (1)
internal/controller/ledger/errors.go (4)
ErrIdempotencyKeyConflict
(131-133)ErrInvalidIdempotencyInput
(242-246)ErrTransactionReferenceConflict
(150-152)ErrNotFound
(14-14)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (8)
internal/api/v2/controllers_transactions_create.go (1)
58-58
: Good replacement with more specific error handler.Replacing
HandleCommonErrors
withHandleCommonWriteErrors
is appropriate here as it provides more granular handling of idempotency-related errors. This change is consistent with the PR objective to return 400 instead of 500 for idempotency key reuse with different body.test/e2e/api_transactions_metadata_test.go (4)
126-165
: Well-structured test for idempotent metadata deletion.This test correctly verifies that deleting metadata with an idempotency key succeeds on the first call and remains idempotent on subsequent calls. The test is thorough and includes verification that the metadata was actually deleted.
167-214
: Comprehensive test for idempotency key validation.This test properly validates that using the same idempotency key with different parameters (in this case, different metadata keys to delete) results in a validation error. The test setup creates the necessary conditions and explicitly verifies the error code.
217-243
: Good test for idempotent metadata addition.This test verifies that using the same idempotency key with identical data succeeds on both the first and subsequent calls, confirming correct idempotent behavior for metadata addition.
246-276
: Thorough validation of idempotency key reuse with different data.This test ensures that attempting to reuse an idempotency key with different metadata values correctly returns a validation error, which aligns with the PR objective to "fix: return 400 instead of 500 for idempotency key reuse with different body".
internal/api/v1/controllers_transactions_create.go (3)
96-97
: Simplified error case handling for NoPostings.Removing
ledgercontroller.ErrInvalidIdempotencyInput{}
from this specific case is appropriate as this error is now handled by theHandleCommonWriteErrors
function. This change helps maintain consistent error responses across the API.
101-101
: Improved error handling with specialized handler.Using
HandleCommonWriteErrors
instead ofHandleCommonErrors
provides better handling of idempotency-related errors, which aligns with the PR objective to return appropriate status codes for idempotency issues.
135-135
: Consistent application of the new error handler.Good consistency in applying the same change to both error handling sections in this function.
…ents in error handling functions
…enAPI specifications
…requests and improve error handling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
pkg/client/docs/models/operations/v2deleteaccountmetadatarequest.md (1)
6-11
: Fix Markdown formatting and improveIdempotencyKey
descriptionThe
Type
column entry**string*
is unbalanced (one too many*
) and inconsistent with other fields, which use*string*
. Also, the description could be more explicit, and providing a sample value would help users.
Proposed diff:-| `IdempotencyKey` | **string* | :heavy_minus_sign: | Use an idempotency key | | +| `IdempotencyKey` | *string* | :heavy_minus_sign: | An idempotency key to ensure request idempotency. | e.g., `abc-123-def-456` |test/e2e/api_transactions_metadata_test.go (3)
126-165
: Good test for idempotent deletion.This test correctly verifies the successful scenario where a delete operation with idempotency key:
- Succeeds on the first call
- Remains idempotent on subsequent calls with the same key
- Verifies the metadata was actually deleted
For consistency with other helper functions, consider using the
DeleteTransactionMetadata
helper instead of calling the client directly.- _, err := testServer.GetValue().Client().Ledger.V2.DeleteTransactionMetadata( - ctx, - operations.V2DeleteTransactionMetadataRequest{ - Ledger: "default", - ID: rsp.ID, - Key: "foo", - IdempotencyKey: ikPtr, - }, - ) + err := DeleteTransactionMetadata( + ctx, + testServer.GetValue(), + operations.V2DeleteTransactionMetadataRequest{ + Ledger: "default", + ID: rsp.ID, + Key: "foo", + IdempotencyKey: ikPtr, + }, + )
167-214
: Consider translating the French comments to English.The commented-out test is well-structured, but the explanatory comments are in French. For consistency with the rest of the codebase, consider translating these comments to English.
- // Note: Cette fonctionnalité nécessite des modifications dans le backend - // pour la vérification des clés d'idempotence avec des paramètres différents - // Ce test est désactivé car le backend ne vérifie pas encore ces cas + // Note: This feature requires backend modifications + // for verifying idempotency keys with different parameters + // This test is disabled because the backend doesn't yet verify these cases
246-275
: Good test for validation error on different data.This test correctly verifies that attempting to add metadata with the same idempotency key but different data results in a validation error.
Since the PR title specifically mentions returning 400 instead of 500, consider explicitly verifying the HTTP status code is 400 in the error case.
Expect(err).To(HaveOccurred()) Expect(err).To(HaveErrorCode(string(components.V2ErrorsEnumValidation))) + // Verify HTTP status code is 400 Bad Request + sdkErr, ok := err.(components.SDKError) + Expect(ok).To(BeTrue()) + Expect(sdkErr.GetStatusCode()).To(Equal(400))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
openapi.yaml
is excluded by!**/*.yaml
openapi/v2.yaml
is excluded by!**/*.yaml
pkg/client/.speakeasy/gen.lock
is excluded by!**/*.lock
,!**/*.lock
pkg/client/.speakeasy/gen.yaml
is excluded by!**/*.yaml
📒 Files selected for processing (8)
pkg/client/docs/models/operations/v2deleteaccountmetadatarequest.md
(1 hunks)pkg/client/docs/models/operations/v2deletetransactionmetadatarequest.md
(1 hunks)pkg/client/formance.go
(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
(2 hunks)test/e2e/api_transactions_metadata_test.go
(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/client/formance.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/api_accounts_metadata_test.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/e2e/api_transactions_metadata_test.go (5)
pkg/testserver/api.go (3)
DeleteTransactionMetadata
(100-103)GetTransaction
(119-127)AddMetadataToTransaction
(86-89)pkg/client/models/operations/v2deletetransactionmetadata.go (1)
V2DeleteTransactionMetadataRequest
(11-20)pkg/client/models/operations/v2gettransaction.go (1)
V2GetTransactionRequest
(12-19)pkg/client/models/operations/v2addmetadataontransaction.go (1)
V2AddMetadataOnTransactionRequest
(11-22)pkg/client/models/components/v2errorsenum.go (1)
V2ErrorsEnumValidation
(15-15)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Tests
- GitHub Check: Dirty
🔇 Additional comments (9)
pkg/client/v2.go (2)
2767-2767
: Added header population for idempotency key supportThis change properly integrates the
IdempotencyKey
from the request context into the HTTP headers for account metadata deletion operations, ensuring consistent behavior with other API operations when idempotency keys are used.
4194-4194
: Added header population for idempotency key supportThis change properly integrates the
IdempotencyKey
from the request context into the HTTP headers for transaction metadata deletion operations, ensuring consistent behavior with other API operations when idempotency keys are used.pkg/client/docs/models/operations/v2deletetransactionmetadatarequest.md (1)
10-11
: Looks good! The documentation is correctly updated.The documentation now includes the new
IdempotencyKey
field with the appropriate description and marks it as optional.pkg/client/models/operations/v2deleteaccountmetadata.go (2)
16-17
: LGTM! Idempotency key field properly added.The field is correctly defined as an optional string pointer with appropriate HTTP header annotation.
41-46
: Getter implementation is correct.The getter method properly handles nil receiver and returns the expected field.
pkg/client/models/operations/v2deletetransactionmetadata.go (2)
18-19
: LGTM! Idempotency key field properly added.The field is correctly defined as an optional string pointer with appropriate HTTP header annotation.
54-59
: Getter implementation is correct.The getter method properly handles nil receiver and returns the expected field.
test/e2e/api_transactions_metadata_test.go (2)
6-10
: Imports look good.The added imports for math/big, time, and pointer are needed for the new test cases.
217-243
: Good test for idempotent metadata addition.This test correctly verifies that adding metadata with the same idempotency key and identical data succeeds both times.
…common write error handler
…nsaction metadata operations
… transactions metadata tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Next time, can you respect the branch name format please.
In the context, hotfix/v2.2/xxx.
Don't forget to cherry-pick to another pr targetting main (with name fix/xxx - keep the same xxx for a better history)
No description provided.