Skip to content

Conversation

flemzord
Copy link
Member

No description provided.

@flemzord flemzord requested a review from a team as a code owner May 14, 2025 20:08
Copy link

coderabbitai bot commented May 14, 2025

Walkthrough

This 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

File(s) Change Summary
internal/api/common/errors.go Added HandleCommonWriteErrors to centralize error handling for write operations, mapping idempotency conflicts, invalid idempotency inputs, and not-found errors to appropriate HTTP responses.
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/v1/controllers_transactions_delete_metadata.go
internal/api/v2/controllers_accounts_add_metadata.go
internal/api/v2/controllers_accounts_delete_metadata.go
internal/api/v2/controllers_transactions_add_metadata.go
internal/api/v2/controllers_transactions_delete_metadata.go
internal/api/v2/controllers_ledgers_delete_metadata.go
internal/api/v2/controllers_ledgers_update_metadata.go
Updated error handling to use HandleCommonWriteErrors for write operations, replacing or simplifying previous error handling logic. Minor import reorderings. Added build constraint and explicit account address validation in v2/controllers_accounts_add_metadata.go.
internal/api/v1/controllers_transactions_create.go
internal/api/v2/controllers_transactions_create.go
Removed explicit handling for ErrInvalidIdempotencyInput in transaction creation error handling; now handled by HandleCommonWriteErrors.
pkg/client/models/operations/v2deleteaccountmetadata.go
pkg/client/models/operations/v2deletetransactionmetadata.go
Added optional IdempotencyKey field and getter method to both request structs for account and transaction metadata deletion.
pkg/client/docs/models/operations/v2deleteaccountmetadatarequest.md
pkg/client/docs/models/operations/v2deletetransactionmetadatarequest.md
Updated documentation to include the new optional IdempotencyKey field for metadata deletion requests.
pkg/client/v2.go In DeleteAccountMetadata and DeleteTransactionMetadata, added logic to populate headers (including idempotency key) from context and request data.
pkg/client/formance.go Bumped SDK and UserAgent version from "0.10.0" to "0.10.1".
pkg/testserver/api.go Added helper functions DeleteMetadataFromAccount and DeleteMetadataFromTransaction as wrappers for existing deletion functions.
test/e2e/api_accounts_metadata_test.go
test/e2e/api_transactions_metadata_test.go
test/e2e/api_idempotency_test.go
Expanded end-to-end test suites to cover idempotency key behavior for adding and deleting account and transaction metadata, including success, repeated, and conflict scenarios. Some tests for conflicting idempotency keys are commented out due to backend limitations.
docs/api/README.md Updated DELETE endpoint documentation for /v2/{ledger}/transactions/{id}/metadata/{key} to include optional Idempotency-Key HTTP header.

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
Loading

Suggested reviewers

  • paul-nicolas

Poem

🐇
With keys of idempotence, we leap ahead,
Metadata safe, no duplicate dread.
Errors now handled with clarity and care,
Docs and SDKs updated, changes everywhere!
Tests abound to keep us right—
The ledger hops forward, future bright!
🌱


📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 582ef28 and 2a37307.

📒 Files selected for processing (1)
  • docs/api/README.md (2 hunks)
✅ Files skipped from review due to trivial changes (1)
  • docs/api/README.md

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need 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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a 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 and accountMetadataIK 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

📥 Commits

Reviewing files that changed from the base of the PR and between d5d0160 and 0c47a57.

📒 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 handling

Adding the explicit import for the errors package supports the enhanced error handling below.


40-43: Improved error handling for idempotency-related errors

This 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 added

The errors package is now explicitly imported to support the enhanced error type checking.


36-39: Enhanced idempotency error handling

These 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 added

The errors package is now explicitly imported to support the improved error type checking implemented below.


40-43: Improved idempotency error handling

This 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 added

The errors package is now explicitly imported to support the enhanced error handling below.


34-37: Improved idempotency error handling

These 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 imports

Good 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 idempotency

This 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 imports

The imports are properly organized and include all necessary packages for the new tests.


126-168: Good test coverage for idempotent deletion

This 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 errors

The 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 data

This 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 data

This 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 data

This 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 testing

This 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 deletion

The 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 deletion

This 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.

Copy link

@coderabbitai coderabbitai bot left a 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 comment

The 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

📥 Commits

Reviewing files that changed from the base of the PR and between 0c47a57 and c53e923.

📒 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 key

This 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 addition

This 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 constraint

This 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 setup

The 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 tests

These 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 scenarios

The 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.

Copy link

@coderabbitai coderabbitai bot left a 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

📥 Commits

Reviewing files that changed from the base of the PR and between c53e923 and dba6222.

📒 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 with HandleCommonWriteErrors 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 the HandleCommonWriteErrors function. This change helps maintain consistent error responses across the API.


101-101: Improved error handling with specialized handler.

Using HandleCommonWriteErrors instead of HandleCommonErrors 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.

Copy link

@coderabbitai coderabbitai bot left a 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 improve IdempotencyKey description

The 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:

  1. Succeeds on the first call
  2. Remains idempotent on subsequent calls with the same key
  3. 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

📥 Commits

Reviewing files that changed from the base of the PR and between 3fa6731 and 33a06a2.

⛔ 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 support

This 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 support

This 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.

Copy link
Collaborator

@gfyrag gfyrag left a 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)

@flemzord flemzord merged commit 55d59af into release/v2.2 May 16, 2025
8 checks passed
@flemzord flemzord deleted the fix/ik branch May 16, 2025 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants