feat(api): add idempotent pledge endpoint - #717
Conversation
|
@David-Adegboyega is attempting to deploy a commit to the ritik4ever's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@David-Adegboyega Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
📝 WalkthroughWalkthroughAdds Idempotency-Key caching for pledge creation using Redis with an in-memory fallback, route middleware integration, API tests, OpenAPI and README documentation. The diff also contains an unresolved middleware import and duplicate ChangesPledge idempotency
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant idempotencyMiddleware
participant PledgeHandler
participant RedisCache
Client->>idempotencyMiddleware: POST pledge with Idempotency-Key
idempotencyMiddleware->>RedisCache: Read cached response
RedisCache-->>idempotencyMiddleware: Hit or miss
idempotencyMiddleware->>PledgeHandler: Continue on miss
PledgeHandler-->>idempotencyMiddleware: Successful pledge response
idempotencyMiddleware->>RedisCache: Cache response
idempotencyMiddleware-->>Client: Return response with HIT or MISS header
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.5)backend/src/services/cache.tsFile contains syntax errors that prevent linting: Line 39: Expected a statement but instead found 'catch'.; Line 42: Expected a statement but instead found 'catch'. 🔧 ESLint
backend/src/services/cache.tsParsing error: 'try' expected. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/api.test.ts`:
- Around line 487-491: Update all six idempotency test call sites to use the
existing postWithHeaders helper instead of post, passing each Idempotency-Key
through the headers argument. Keep unkeyed requests on post and ensure the tests
exercise the keyed idempotency path without changing their assertions.
In `@backend/src/middleware/idempotencyMiddleware.ts`:
- Around line 24-26: Update the idempotency cache-key construction in the
middleware around buildIdempotencyCacheKey to include the validated contributor
identity alongside apiKey and campaignId. Ensure contributor identity is
available from the validated request context, including for anonymous requests,
so identical idempotency keys cannot reuse another contributor’s pledge
response.
In `@backend/src/services/cache.ts`:
- Around line 38-40: Remove the redisClient = null and isConnected = false
assignments from the successful connection path after await
redisClient.connect(). Keep those state resets only in the failure/cleanup path
so successful initialization retains the connected client and connected status.
In `@backend/src/services/idempotencyCache.ts`:
- Around line 27-58: Replace the separate lookup/write flow around
getIdempotencyCacheEntry and setIdempotencyCacheEntry with an atomic claim
operation performed before the handler runs, using Redis’s conditional create
semantics (such as SET NX) and an equivalent memory-cache guard. When a key is
already claimed, wait for or replay the winning request’s stored response
instead of invoking next(); ensure the winning response is published through the
existing cache path for subsequent waiters.
In `@backend/src/validation/schemas.ts`:
- Line 5: Remove the unused CampaignStatus, CampaignSortField, and SortOrder
type imports from the schemas module while leaving all other imports unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 738db489-5446-4256-a9da-b9ef78c63100
📒 Files selected for processing (9)
CHANGELOG.mdREADME.mdbackend/src/api.test.tsbackend/src/index.tsbackend/src/middleware/idempotencyMiddleware.tsbackend/src/openapi.tsbackend/src/services/cache.tsbackend/src/services/idempotencyCache.tsbackend/src/validation/schemas.ts
| const res = await post(`/api/campaigns/${campaignId}/pledges`, { | ||
| contributor: CONTRIBUTOR_C, | ||
| amount: 100, | ||
| assetCode: 'USDC', | ||
| }, { 'Idempotency-Key': 'test-key-1' }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
These tests never send Idempotency-Key — post() takes only two arguments.
post(apiPath, body) (Lines 58-66) has no third parameter, so the { 'Idempotency-Key': ... } object is silently dropped at runtime and every one of these "idempotency" tests actually exercises the unkeyed path. This also fails tsc (TS2554: expected 2 arguments, got 3) if typecheck runs in CI. Concretely, Line 513 (secondRes.data deep-equals firstRes.data) and Line 535 (pledge count unchanged) cannot hold, since the second request creates a real second pledge.
All six call sites passing a third argument (Lines 487-491, 500-511, 519-532, 553-564, 575-587, 595-606) must use postWithHeaders instead.
🐛 Proposed fix (apply the same change to each keyed call site)
- const res = await post(`/api/campaigns/${campaignId}/pledges`, {
- contributor: CONTRIBUTOR_C,
- amount: 100,
- assetCode: 'USDC',
- }, { 'Idempotency-Key': 'test-key-1' });
+ const res = await postWithHeaders(
+ `/api/campaigns/${campaignId}/pledges`,
+ { contributor: CONTRIBUTOR_C, amount: 100, assetCode: 'USDC' },
+ { 'Idempotency-Key': 'test-key-1' },
+ );Alternatively, give post an optional headers parameter and drop postWithHeaders to avoid two near-identical helpers.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const res = await post(`/api/campaigns/${campaignId}/pledges`, { | |
| contributor: CONTRIBUTOR_C, | |
| amount: 100, | |
| assetCode: 'USDC', | |
| }, { 'Idempotency-Key': 'test-key-1' }); | |
| const res = await postWithHeaders( | |
| `/api/campaigns/${campaignId}/pledges`, | |
| { contributor: CONTRIBUTOR_C, amount: 100, assetCode: 'USDC' }, | |
| { 'Idempotency-Key': 'test-key-1' }, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/api.test.ts` around lines 487 - 491, Update all six idempotency
test call sites to use the existing postWithHeaders helper instead of post,
passing each Idempotency-Key through the headers argument. Keep unkeyed requests
on post and ensure the tests exercise the keyed idempotency path without
changing their assertions.
| const apiKey = (req as unknown as RequestWithApiKey).apiKey ?? 'anonymous'; | ||
| const campaignId = req.params.id as string; | ||
| const cacheKey = buildIdempotencyCacheKey(apiKey, campaignId, idempotencyKey); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Scope the key to the contributor contract.
The cache key omits the contributor entirely. Requests from different contributors with the same API key—or any requests using the anonymous fallback—can replay another contributor’s pledge response instead of creating the intended pledge. Include the validated contributor identity in the cache scope (or reject reuse of a key with a different request fingerprint).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/middleware/idempotencyMiddleware.ts` around lines 24 - 26, Update
the idempotency cache-key construction in the middleware around
buildIdempotencyCacheKey to include the validated contributor identity alongside
apiKey and campaignId. Ensure contributor identity is available from the
validated request context, including for anonymous requests, so identical
idempotency keys cannot reuse another contributor’s pledge response.
| export async function getIdempotencyCacheEntry( | ||
| key: string, | ||
| ): Promise<IdempotencyCacheEntry | null> { | ||
| if (isCacheAvailable()) { | ||
| const cached = await getCacheValue(key); | ||
| if (cached) { | ||
| return JSON.parse(cached) as IdempotencyCacheEntry; | ||
| } | ||
| } | ||
|
|
||
| const memoryEntry = memoryCache.get(key); | ||
| if (memoryEntry) { | ||
| return memoryEntry; | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| export async function setIdempotencyCacheEntry( | ||
| key: string, | ||
| entry: IdempotencyCacheEntry, | ||
| ): Promise<void> { | ||
| const serialized = JSON.stringify(entry); | ||
|
|
||
| if (isCacheAvailable()) { | ||
| await setCacheValue(key, serialized, IDEMPOTENCY_TTL_SECONDS).catch(() => { | ||
| // Silently fail Redis writes; memory cache still works | ||
| }); | ||
| } | ||
|
|
||
| memoryCache.set(key, entry); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Make idempotency acquisition atomic.
A lookup followed by a later write cannot prevent two concurrent requests with the same key from both missing and reaching the database. Add an atomic “claim/in-progress” operation (for Redis, e.g. SET ... NX) before invoking the handler; waiters must replay/wait for the winning response rather than call next().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/services/idempotencyCache.ts` around lines 27 - 58, Replace the
separate lookup/write flow around getIdempotencyCacheEntry and
setIdempotencyCacheEntry with an atomic claim operation performed before the
handler runs, using Redis’s conditional create semantics (such as SET NX) and an
equivalent memory-cache guard. When a key is already claimed, wait for or replay
the winning request’s stored response instead of invoking next(); ensure the
winning response is published through the existing cache path for subsequent
waiters.
|
This PR could not be merged because it has merge conflicts with the target branch. Please resolve the merge conflicts, push the updated changes, and the PR can be reviewed and merged. Thank you! |
|
@ritik4ever conflict resolved. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| contributor: CONTRIBUTOR_C, | ||
| amount: 100, | ||
| assetCode: 'USDC', | ||
| }, { 'Idempotency-Key': 'test-key-1' }); |
| contributor: CONTRIBUTOR_C, | ||
| amount: 100, | ||
| assetCode: 'USDC', | ||
| }, { 'Idempotency-Key': 'dup-key-1' }); |
| contributor: CONTRIBUTOR_C, | ||
| amount: 100, | ||
| assetCode: 'USDC', | ||
| }, { 'Idempotency-Key': 'dup-key-1' }); |
| contributor: CONTRIBUTOR_C, | ||
| amount: 100, | ||
| assetCode: 'USDC', | ||
| }, { 'Idempotency-Key': 'db-write-key' }); |
| contributor: CONTRIBUTOR_C, | ||
| amount: 100, | ||
| assetCode: 'USDC', | ||
| }, { 'Idempotency-Key': 'db-write-key' }); |
| contributor: CONTRIBUTOR_C, | ||
| amount: 100, | ||
| assetCode: 'USDC', | ||
| }, { 'Idempotency-Key': 'shared-key' }); |
| contributor: CONTRIBUTOR_D, | ||
| amount: 100, | ||
| assetCode: 'USDC', | ||
| }, { 'Idempotency-Key': 'shared-key' }); |
| contributor: CONTRIBUTOR_C, | ||
| amount: 75, | ||
| assetCode: 'USDC', | ||
| }, { 'Idempotency-Key': 'status-payload-key' }); |
| contributor: CONTRIBUTOR_C, | ||
| amount: 75, | ||
| assetCode: 'USDC', | ||
| }, { 'Idempotency-Key': 'status-payload-key' }); |
| } catch { | ||
| redisClient = null; | ||
| isConnected = false; | ||
| } catch { |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
backend/src/services/cache.ts (1)
35-44: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winRemove the duplicate handlers from the Redis initialization path.
backend/src/services/cache.ts#L35-L41has one validcatch, then an extra barecatchat line 39 and another duplicate barecatchat line 42, which prevents the service from compiling. Keep onecatchblock that logs the Redis connection failure and resets Redis state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/services/cache.ts` around lines 35 - 44, Remove the duplicate bare catch handlers from the Redis initialization path in backend/src/services/cache.ts lines 35-44, keeping the single catch that logs the failure and resets redisClient and isConnected. No direct changes are required at backend/src/index.ts line 761 or backend/src/api.test.ts lines 472, 478, and 486; they are affected sites but are corrected by the cache handler fix.Source: Linters/SAST tools
backend/src/index.ts (2)
598-598: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy liftImport
idempotencyMiddlewareand reserve idempotency keys before callingnext.
idempotencyMiddlewareis used in the pledge route but not imported, so this route will fail to compile. Even with the import, the middleware currently only reads the idempotency cache and re-writes it afterres.send, allowing concurrent requests with the same key to both miss and create duplicate pledges. Reserve the key beforenext()with an atomicSET ... NX/wait-replay path and cover it with a parallel-request test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/index.ts` at line 598, Import idempotencyMiddleware in the pledge route module, then update its implementation to atomically reserve each idempotency key with SET NX before invoking next(); when reservation fails, wait for and replay the existing result instead of proceeding. Add a parallel-request test verifying that concurrent requests with the same key produce only one pledge.
118-125: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExpose the idempotency cache header through CORS.
idempotencyMiddlewaresetsX-Idempotency-Cache, but browser cross-origin clients cannot read non-safelisted CORS response headers unless this header is added toexposedHeaders.Proposed fix
exposedHeaders: [ 'X-Total-Count', + 'X-Idempotency-Cache', 'X-RateLimit-Limit',🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/index.ts` around lines 118 - 125, Update the CORS configuration’s exposedHeaders list to include X-Idempotency-Cache, alongside the existing rate-limit and retry headers, so cross-origin clients can read the header set by idempotencyMiddleware.backend/src/openapi.ts (2)
431-434: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDo not hard-code a 64KB body limit in the API contract.
The server defaults to
16kband permitsMAX_BODY_SIZEoverrides. Describe this as the configured limit, or derive the documented value from configuration.Proposed fix
- description: 'Request body exceeds the 64KB maximum limit', + description: 'Request body exceeds the configured maximum limit.',🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/openapi.ts` around lines 431 - 434, The payloadTooLargeResponse description hard-codes a 64KB limit that may not match the server configuration. Update this response definition to describe the configured body limit, reusing the existing MAX_BODY_SIZE or body-size configuration symbol, while preserving the ApiError schema and response behavior.
636-648: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDeclare
Idempotency-Keyas an optional request header.
POST /api/campaigns/{id}/pledgesdocuments only the body in the generated OpenAPI. Add arequest.headersschema withIdempotency-Key: z.string().optional()and a short description, e.g. “An optional idempotency key for this pledge request.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/openapi.ts` around lines 636 - 648, Update the POST pledge endpoint definition near the body schema to add an optional request.headers schema containing Idempotency-Key as an optional string with a short descriptive annotation, while preserving the existing body and responses.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@backend/src/index.ts`:
- Line 598: Import idempotencyMiddleware in the pledge route module, then update
its implementation to atomically reserve each idempotency key with SET NX before
invoking next(); when reservation fails, wait for and replay the existing result
instead of proceeding. Add a parallel-request test verifying that concurrent
requests with the same key produce only one pledge.
- Around line 118-125: Update the CORS configuration’s exposedHeaders list to
include X-Idempotency-Cache, alongside the existing rate-limit and retry
headers, so cross-origin clients can read the header set by
idempotencyMiddleware.
In `@backend/src/openapi.ts`:
- Around line 431-434: The payloadTooLargeResponse description hard-codes a 64KB
limit that may not match the server configuration. Update this response
definition to describe the configured body limit, reusing the existing
MAX_BODY_SIZE or body-size configuration symbol, while preserving the ApiError
schema and response behavior.
- Around line 636-648: Update the POST pledge endpoint definition near the body
schema to add an optional request.headers schema containing Idempotency-Key as
an optional string with a short descriptive annotation, while preserving the
existing body and responses.
In `@backend/src/services/cache.ts`:
- Around line 35-44: Remove the duplicate bare catch handlers from the Redis
initialization path in backend/src/services/cache.ts lines 35-44, keeping the
single catch that logs the failure and resets redisClient and isConnected. No
direct changes are required at backend/src/index.ts line 761 or
backend/src/api.test.ts lines 472, 478, and 486; they are affected sites but are
corrected by the cache handler fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 88997596-d83b-4fdb-b51a-34c7fad0730c
📒 Files selected for processing (5)
README.mdbackend/src/api.test.tsbackend/src/index.tsbackend/src/openapi.tsbackend/src/services/cache.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
3 participants
Pull Request
What Changed
A clear and concise description of what this PR changes and why.
Related Issues
Testing Done
Describe the tests you ran and how to reproduce them.
Security Review
If this PR touches API endpoints, authentication, database queries, or contract code, check the applicable items from SECURITY_CHECKLIST.md. Paste relevant items below:
Checklist
npm test/cargo test)Screenshots (if applicable)
Add screenshots or recordings for visual changes.
Summary by CodeRabbit
New Features
Idempotency-Keyheader.X-Idempotency-Cacheresponse headers indicating cache hits or misses.Bug Fixes
Documentation