feat(http): add IdempotencyKey in header if present #70
Conversation
WalkthroughThe pull request introduces a new idempotency key feature across multiple files in the webhooks system. The changes modify the Changes
Sequence DiagramsequenceDiagram
participant Client
participant WebhookSystem
participant TargetService
Client->>WebhookSystem: Send Webhook Request
WebhookSystem->>WebhookSystem: Generate Idempotency Key
WebhookSystem->>TargetService: Make Attempt with Idempotency Key
alt First Attempt
TargetService-->>WebhookSystem: Success
else Duplicate Attempt
TargetService-->>WebhookSystem: Ignore/Deduplicate
end
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pkg/attempt.go (1)
38-38: Add documentation for the idempotencyKey parameter.Consider adding a comment explaining the purpose and format requirements of the idempotencyKey parameter.
+// MakeAttempt creates a new webhook attempt with the given configuration. +// If idempotencyKey is not empty, it will be added as a header to ensure +// idempotent request processing on the server side. func MakeAttempt(ctx context.Context, httpClient *http.Client, retryPolicy BackoffPolicy, id, webhookID string, attemptNb int, cfg Config, idempotencyKey string, payload []byte, isTest bool) (Attempt, error) {pkg/worker/worker.go (1)
108-113: Improve error message for JSON unmarshalling.The error message could be more descriptive to help with debugging.
var ev publish.EventMessage err = json.Unmarshal([]byte(atts[0].Payload), &ev) if err != nil { - errChan <- errors.Wrap(err, "json.Unmarshal") + errChan <- errors.Wrapf(err, "failed to unmarshal payload for webhookID: %s, attempt: %s", webhookID, atts[0].ID) continue }pkg/worker/module.go (1)
133-137: Improve error handling for JSON marshalling.The error handling could be more descriptive and include context about the event being processed.
data, err := json.Marshal(ev) if err != nil { - logging.FromContext(ctx).Error(err) + logging.FromContext(ctx).Errorf("failed to marshal event message of type %s: %v", ev.Type, err) return }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/attempt.go(2 hunks)pkg/server/test.go(1 hunks)pkg/worker/module.go(1 hunks)pkg/worker/worker.go(2 hunks)
🔇 Additional comments (2)
pkg/attempt.go (1)
57-59: Consider validating the idempotency key format.The idempotency key is added to the header without validation. Consider adding basic validation to ensure it meets any format requirements.
pkg/worker/worker.go (1)
117-117: Consider handling potential nil idempotency key.The code directly uses
ev.IdempotencyKeywithout checking if it's nil or empty. Consider adding a check to handle this case gracefully.
| @@ -27,7 +27,7 @@ func (h *serverHandler) testOneConfigHandle(w http.ResponseWriter, r *http.Reque | |||
| logging.FromContext(r.Context()).Debugf("GET %s/%s%s", PathConfigs, id, PathTest) | |||
| retryPolicy := backoff.NewNoRetry() | |||
| attempt, err := webhooks.MakeAttempt(r.Context(), h.httpClient, retryPolicy, uuid.NewString(), | |||
| uuid.NewString(), 0, cfgs[0], []byte(`{"data":"test"}`), true) | |||
| uuid.NewString(), 0, cfgs[0], "ik", []byte(`{"data":"test"}`), true) | |||
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Additional test cases needed for idempotency key handling
Current implementation supports various idempotency key scenarios, but test coverage is limited to a single basic case. Based on the codebase analysis:
- Add test case with empty idempotency key in
pkg/server/test.goto verify the conditional header setting - Add test cases with different formats (UUID, timestamp-based, custom formats) to ensure proper handling
- Add test case with a long idempotency key to verify there are no length-related issues
- Consider testing header presence/absence based on the key value
🔗 Analysis chain
Improve test coverage for idempotency key scenarios.
The test uses a hard-coded "ik" value for the idempotency key. Consider adding test cases for:
- Empty idempotency key
- Different idempotency key formats
- Very long idempotency keys
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing test cases related to idempotency key
rg -A 5 'test.*[Ii]dempotency.*Key|idempotencyKey.*test'
Length of output: 58
Script:
#!/bin/bash
# Search for "ik" usage in test files and the test function context
rg -l '\.go$' | xargs rg -B 5 -A 5 '"ik"'
# Search for idempotency-related code
rg -l '\.go$' | xargs rg -B 2 -A 2 '[Ii]dempotency'
Length of output: 2307
9c6f229 to
b6f691a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/worker/worker.go (1)
108-113: Enhance error handling for better debugging.While the error handling is correct, consider adding logging and more context to the error message.
var ev publish.EventMessage err = json.Unmarshal([]byte(atts[0].Payload), &ev) if err != nil { - errChan <- errors.Wrap(err, "json.Unmarshal") + logging.FromContext(ctx).WithError(err).Error("Failed to unmarshal event message") + errChan <- errors.Wrapf(err, "failed to unmarshal payload for webhookID: %s", webhookID) continue }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/attempt.go(2 hunks)pkg/server/test.go(1 hunks)pkg/worker/module.go(1 hunks)pkg/worker/worker.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/server/test.go
- pkg/worker/module.go
- pkg/attempt.go
🔇 Additional comments (1)
pkg/worker/worker.go (1)
5-5: LGTM!The new imports are necessary for handling the event message and extracting the idempotency key.
Also applies to: 11-11
| newAttemptNb := atts[0].RetryAttempt + 1 | ||
| attempt, err := webhooks.MakeAttempt(ctx, w.httpClient, w.retryPolicy, uuid.NewString(), | ||
| webhookID, newAttemptNb, atts[0].Config, []byte(atts[0].Payload), false) | ||
| webhookID, newAttemptNb, atts[0].Config, ev.IdempotencyKey, []byte(atts[0].Payload), false) |
There was a problem hiding this comment.
💡 Codebase verification
Validation of idempotency key is missing and should be added
The codebase shows consistent validation patterns for other important fields (endpoints, secrets, event types), but lacks any validation for idempotency keys. Consider adding validation in pkg/attempt.go where MakeAttempt is called to ensure:
- The key is not empty
- The key meets any length/format requirements
- The key follows any business-specific constraints
🔗 Analysis chain
Consider validating the idempotency key.
The idempotency key is used directly from the event message without validation. Consider adding validation to ensure it meets any format or length requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/b
8000
ash
# Description: Check if idempotency key validation exists in the codebase.
# Search for any existing validation of idempotency keys
rg -A 5 'IdempotencyKey.*valid|valid.*IdempotencyKey'
# Search for any constants or types defining idempotency key requirements
ast-grep --pattern 'const (
$$$
IdempotencyKey
$$$
)'
Length of output: 117
Script:
#!/bin/bash
# Search for IdempotencyKey definition and usage
rg -A 5 "type.*IdempotencyKey|struct.*IdempotencyKey"
# Look for message/event validation patterns
rg -A 5 "func.*validate.*Event|func.*Validate.*Message"
# Find where IdempotencyKey is set/used
ast-grep --pattern 'IdempotencyKey = $_'
Length of output: 156
Script:
#!/bin/bash
# Look for method signatures containing IdempotencyKey
rg -A 5 "func.*\(.*[iI]dempotency.*\)"
# Search for string validation patterns
rg -A 5 "func.*validate.*string|len\(.*\)"
# Find event processing code
ast-grep --pattern 'func ($_) (ev Event) {
$$$
}'
Length of output: 10914
No description provided.