idempotency key support implemented#282
Conversation
|
@feyishola is attempting to deploy a commit to the olufunbiik's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdded idempotency key support to the tips API by introducing an optional Changes
Sequence DiagramsequenceDiagram
participant Client
participant Controller
participant Service
participant Repository
participant Database
Client->>Controller: POST /tips with Idempotency-Key header
Controller->>Controller: Extract & map idempotency-key to DTO
Controller->>Service: create(userId, createTipDto)
Service->>Repository: findOne(idempotencyKey)
Repository->>Database: SELECT * FROM tips WHERE idempotencyKey = ?
alt Idempotency Key Match Found
Database-->>Repository: Return existing Tip
Repository-->>Service: Return cached Tip
Service->>Service: Log replay message
Service-->>Controller: Return existing Tip (no processing)
else No Match - New Request
Service->>Service: Validate & process tip
Service->>Service: Stellar transaction checks
Service->>Service: Moderation & fees
Service->>Repository: create(tip with idempotencyKey)
Repository->>Database: INSERT INTO tips (...)
Database-->>Repository: Return persisted Tip
Repository-->>Service: Return new Tip
Service->>Service: Emit events & notifications
Service-->>Controller: Return new Tip
end
Controller-->>Client: Tip response (200 OK)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 4
🧹 Nitpick comments (2)
backend/src/tips/entities/tip.entity.ts (1)
50-52: Redundant@Index()decorator whenunique: trueis already set.The
unique: trueconstraint on the column automatically creates a unique index in the database. The explicit@Index()decorator creates an additional non-unique index, which is redundant and wastes storage.Suggested fix
- `@Column`({ length: 128, nullable: true, unique: true }) - `@Index`() - idempotencyKey?: string; + `@Column`({ length: 128, nullable: true, unique: true }) + idempotencyKey?: string;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/tips/entities/tip.entity.ts` around lines 50 - 52, The idempotencyKey column currently has both `@Column`({ length: 128, nullable: true, unique: true }) and an explicit `@Index`() which is redundant; remove the `@Index`() decorator for the idempotencyKey property so the unique: true on the Column creates the single unique index and avoids creating an extra non-unique index (locate the idempotencyKey field in the Tip entity to apply this change).backend/migrations/1769950000000-AddIdempotencyKeyToTips.ts (1)
5-23: Redundant unique index creation.The column is created with
isUnique: true(line 12), which already creates a unique constraint on the column. Creating an additional unique index viacreateIndex(lines 16-23) is redundant and creates duplicate enforcement of the same constraint.Remove either the
isUnique: truefrom the column definition or the separate index creation.Option 1: Keep column-level constraint only
await queryRunner.addColumn( 'tips', new TableColumn({ name: 'idempotencyKey', type: 'varchar', length: '128', isNullable: true, isUnique: true, }), ); - - await queryRunner.createIndex( - 'tips', - new TableIndex({ - name: 'IDX_tips_idempotencyKey', - columnNames: ['idempotencyKey'], - isUnique: true, - }), - ); } public async down(queryRunner: QueryRunner): Promise<void> { - await queryRunner.dropIndex('tips', 'IDX_tips_idempotencyKey'); await queryRunner.dropColumn('tips', 'idempotencyKey'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/migrations/1769950000000-AddIdempotencyKeyToTips.ts` around lines 5 - 23, The migration currently adds a unique constraint twice: once via the TableColumn option isUnique: true on the idempotencyKey column (in the queryRunner.addColumn call creating new TableColumn with name 'idempotencyKey') and again by creating a unique index ('IDX_tips_idempotencyKey') with queryRunner.createIndex/new TableIndex. Remove the redundancy by deleting the separate createIndex block (queryRunner.createIndex(... new TableIndex { name: 'IDX_tips_idempotencyKey', ... })) so the single column-level unique constraint remains; alternatively, if you prefer an explicit index, set isUnique: false on the TableColumn and keep the createIndex—ensure the up/down migration changes remain consistent with whichever option you choose.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/src/tips/tips.controller.ts`:
- Around line 69-72: The idempotencyKeyHeader is being assigned to
createTipDto.idempotencyKey after the DTO has already passed ValidationPipe, so
the header bypasses the `@MaxLength`(128) check; fix by validating the header
before or as part of DTO validation: either set createTipDto.idempotencyKey from
idempotencyKeyHeader prior to running ValidationPipe (so class-validator
enforces `@MaxLength`(128)) or perform an explicit check on idempotencyKeyHeader
(e.g., if (idempotencyKeyHeader.length > 128) throw new
BadRequestException(...)) before assigning it to createTipDto; reference
createTipDto, idempotencyKeyHeader, ValidationPipe, and the `@MaxLength`(128)
constraint when making the change.
In `@backend/src/tips/tips.service.ts`:
- Around line 71-81: Concurrent requests using the same idempotencyKey can both
pass the tipRepository.findOne check and then one insert will succeed while the
other throws a QueryFailedError (unique constraint violation); modify the
create/save path that persists the tip (where you call this.tipRepository.save /
insert after checking idempotencyKey) to wrap the save in a try-catch, detect
the DB unique-constraint error (QueryFailedError or specific SQL error code),
and on that error re-query tipRepository.findOne({ where: { idempotencyKey } })
and return the existing record instead of letting the exception bubble; apply
the same try-catch pattern to the other similar block referenced at lines
182-185.
- Around line 70-80: The idempotency lookup returns any record matching
idempotencyKey and can leak tip data across users; update the logic in the
idempotency block (the call to this.tipRepository.findOne and the subsequent use
of existing/return existing) to verify ownership before replaying: either
include the current user's identifier in the findOne query (e.g., where: {
idempotencyKey, fromUser: currentUserId }) or, if you can't change the query,
check existing.fromUser === currentUserId and only log/return existing when they
match; alternatively plan a schema change to enforce a compound unique
constraint on (fromUser, idempotencyKey) to make the key per-user.
- Around line 67-68: The Tip entity's required fromUser field is missing when
creating a new Tip in the create method; update the tipRepository.create(...)
call inside the async create(userId: string, createTipDto: CreateTipDto):
Promise<Tip> function to include fromUser: userId (alongside senderAddress,
artistId, trackId, stellarTxHash, message, idempotencyKey) so the created Tip
has the required fromUser relation populated.
---
Nitpick comments:
In `@backend/migrations/1769950000000-AddIdempotencyKeyToTips.ts`:
- Around line 5-23: The migration currently adds a unique constraint twice: once
via the TableColumn option isUnique: true on the idempotencyKey column (in the
queryRunner.addColumn call creating new TableColumn with name 'idempotencyKey')
and again by creating a unique index ('IDX_tips_idempotencyKey') with
queryRunner.createIndex/new TableIndex. Remove the redundancy by deleting the
separate createIndex block (queryRunner.createIndex(... new TableIndex { name:
'IDX_tips_idempotencyKey', ... })) so the single column-level unique constraint
remains; alternatively, if you prefer an explicit index, set isUnique: false on
the TableColumn and keep the createIndex—ensure the up/down migration changes
remain consistent with whichever option you choose.
In `@backend/src/tips/entities/tip.entity.ts`:
- Around line 50-52: The idempotencyKey column currently has both `@Column`({
length: 128, nullable: true, unique: true }) and an explicit `@Index`() which is
redundant; remove the `@Index`() decorator for the idempotencyKey property so the
unique: true on the Column creates the single unique index and avoids creating
an extra non-unique index (locate the idempotencyKey field in the Tip entity to
apply this change).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bb685032-12db-4923-88c2-96d8c5590648
📒 Files selected for processing (5)
backend/migrations/1769950000000-AddIdempotencyKeyToTips.tsbackend/src/tips/create-tips.dto.tsbackend/src/tips/entities/tip.entity.tsbackend/src/tips/tips.controller.tsbackend/src/tips/tips.service.ts
| // Header takes precedence over body field; merge into DTO | ||
| if (idempotencyKeyHeader) { | ||
| createTipDto.idempotencyKey = idempotencyKeyHeader; | ||
| } |
There was a problem hiding this comment.
Header value bypasses DTO validation.
The idempotencyKeyHeader is assigned to createTipDto.idempotencyKey after the DTO has already been validated by the ValidationPipe. This means the header value bypasses @MaxLength(128) validation—a header longer than 128 characters will pass through and could cause a database error or truncation.
Suggested fix: Add explicit validation
// Header takes precedence over body field; merge into DTO
if (idempotencyKeyHeader) {
+ if (idempotencyKeyHeader.length > 128) {
+ throw new BadRequestException('Idempotency-Key header must not exceed 128 characters');
+ }
createTipDto.idempotencyKey = idempotencyKeyHeader;
}📝 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.
| // Header takes precedence over body field; merge into DTO | |
| if (idempotencyKeyHeader) { | |
| createTipDto.idempotencyKey = idempotencyKeyHeader; | |
| } | |
| // Header takes precedence over body field; merge into DTO | |
| if (idempotencyKeyHeader) { | |
| if (idempotencyKeyHeader.length > 128) { | |
| throw new BadRequestException('Idempotency-Key header must not exceed 128 characters'); | |
| } | |
| createTipDto.idempotencyKey = idempotencyKeyHeader; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/tips/tips.controller.ts` around lines 69 - 72, The
idempotencyKeyHeader is being assigned to createTipDto.idempotencyKey after the
DTO has already passed ValidationPipe, so the header bypasses the
`@MaxLength`(128) check; fix by validating the header before or as part of DTO
validation: either set createTipDto.idempotencyKey from idempotencyKeyHeader
prior to running ValidationPipe (so class-validator enforces `@MaxLength`(128)) or
perform an explicit check on idempotencyKeyHeader (e.g., if
(idempotencyKeyHeader.length > 128) throw new BadRequestException(...)) before
assigning it to createTipDto; reference createTipDto, idempotencyKeyHeader,
ValidationPipe, and the `@MaxLength`(128) constraint when making the change.
| async create(userId: string, createTipDto: CreateTipDto): Promise<Tip> { | ||
| const { artistId, trackId, stellarTxHash, message } = createTipDto; | ||
| const { artistId, trackId, stellarTxHash, message, idempotencyKey } = createTipDto; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if fromUser is set anywhere during tip creation
ast-grep --pattern 'fromUser: $_'Repository: OlufunbiIK/tip-tune
Length of output: 45
🏁 Script executed:
fd -e "\.ts$" tips | grep -E "(entity|service)" | head -20Repository: OlufunbiIK/tip-tune
Length of output: 45
🏁 Script executed:
cat -n backend/src/tips/tips.entity.ts | head -100Repository: OlufunbiIK/tip-tune
Length of output: 126
🏁 Script executed:
cat -n backend/src/tips/tips.service.ts | sed -n '60,90p'Repository: OlufunbiIK/tip-tune
Length of output: 1363
🏁 Script executed:
cat -n backend/src/tips/tips.service.ts | sed -n '160,185p'Repository: OlufunbiIK/tip-tune
Length of output: 985
🏁 Script executed:
find . -type f -name "*entity*" | grep -i tipRepository: OlufunbiIK/tip-tune
Length of output: 152
🏁 Script executed:
find . -type f -name "*.ts" | xargs grep -l "fromUser" | head -10Repository: OlufunbiIK/tip-tune
Length of output: 426
🏁 Script executed:
rg "class Tip" -A 30Repository: OlufunbiIK/tip-tune
Length of output: 21374
🏁 Script executed:
cat -n backend/src/tips/entities/tip.entity.tsRepository: OlufunbiIK/tip-tune
Length of output: 4776
Set fromUser field during tip creation.
The Tip entity has a required fromUser field (line 70), but it's not being set in the tipRepository.create() call (lines 168-183). The userId parameter is currently only used to fetch the user and set senderAddress, leaving fromUser undefined. Add fromUser: userId to the repository create call.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/tips/tips.service.ts` around lines 67 - 68, The Tip entity's
required fromUser field is missing when creating a new Tip in the create method;
update the tipRepository.create(...) call inside the async create(userId:
string, createTipDto: CreateTipDto): Promise<Tip> function to include fromUser:
userId (alongside senderAddress, artistId, trackId, stellarTxHash, message,
idempotencyKey) so the created Tip has the required fromUser relation populated.
| // --- Idempotency key check: replay the original response if key already seen --- | ||
| if (idempotencyKey) { | ||
| const existing = await this.tipRepository.findOne({ | ||
| where: { idempotencyKey }, | ||
| }); | ||
| if (existing) { | ||
| this.logger.log( | ||
| `Idempotency replay for key=${idempotencyKey}, tipId=${existing.id}`, | ||
| ); | ||
| return existing; | ||
| } |
There was a problem hiding this comment.
Security: Idempotency replay leaks tip data across users.
The lookup queries only by idempotencyKey without verifying ownership. If User A creates a tip with key "abc", and User B later submits a request with the same key, User B receives User A's tip record—exposing senderAddress, amount, message, and other sensitive fields.
Either scope the idempotency key per-user (requires schema change to a compound unique constraint on (fromUser, idempotencyKey)) or validate ownership before returning the cached response.
Minimal fix: Validate ownership before replay
if (idempotencyKey) {
const existing = await this.tipRepository.findOne({
where: { idempotencyKey },
});
if (existing) {
+ // Verify the cached tip belongs to the requesting user
+ if (existing.fromUser !== userId) {
+ throw new ConflictException(
+ 'Idempotency key is already in use by another request',
+ );
+ }
this.logger.log(
`Idempotency replay for key=${idempotencyKey}, tipId=${existing.id}`,
);
return existing;
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/tips/tips.service.ts` around lines 70 - 80, The idempotency
lookup returns any record matching idempotencyKey and can leak tip data across
users; update the logic in the idempotency block (the call to
this.tipRepository.findOne and the subsequent use of existing/return existing)
to verify ownership before replaying: either include the current user's
identifier in the findOne query (e.g., where: { idempotencyKey, fromUser:
currentUserId }) or, if you can't change the query, check existing.fromUser ===
currentUserId and only log/return existing when they match; alternatively plan a
schema change to enforce a compound unique constraint on (fromUser,
idempotencyKey) to make the key per-user.
| if (idempotencyKey) { | ||
| const existing = await this.tipRepository.findOne({ | ||
| where: { idempotencyKey }, | ||
| }); | ||
| if (existing) { | ||
| this.logger.log( | ||
| `Idempotency replay for key=${idempotencyKey}, tipId=${existing.id}`, | ||
| ); | ||
| return existing; | ||
| } | ||
| } |
There was a problem hiding this comment.
Race condition: Concurrent requests with the same new key cause unhandled exception.
Two concurrent requests with the same (new) idempotencyKey can both pass the findOne check (TOCTOU), then both attempt to insert. One succeeds; the other throws a QueryFailedError (unique constraint violation) that bubbles up as a 500 error instead of returning the already-created tip.
Wrap the save in a try-catch to handle the duplicate key error gracefully.
Suggested fix: Catch unique constraint violation and retry lookup
- const savedTip = await this.tipRepository.save(newTip);
+ let savedTip: Tip;
+ try {
+ savedTip = await this.tipRepository.save(newTip);
+ } catch (error: any) {
+ // Handle race condition: another request inserted the same idempotencyKey
+ if (
+ idempotencyKey &&
+ error.code === '23505' // PostgreSQL unique violation
+ ) {
+ const existing = await this.tipRepository.findOne({
+ where: { idempotencyKey },
+ });
+ if (existing && existing.fromUser === userId) {
+ this.logger.log(
+ `Idempotency replay (race recovery) for key=${idempotencyKey}, tipId=${existing.id}`,
+ );
+ return existing;
+ }
+ throw new ConflictException('Idempotency key collision');
+ }
+ throw error;
+ }Also applies to: 182-185
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/tips/tips.service.ts` around lines 71 - 81, Concurrent requests
using the same idempotencyKey can both pass the tipRepository.findOne check and
then one insert will succeed while the other throws a QueryFailedError (unique
constraint violation); modify the create/save path that persists the tip (where
you call this.tipRepository.save / insert after checking idempotencyKey) to wrap
the save in a try-catch, detect the DB unique-constraint error (QueryFailedError
or specific SQL error code), and on that error re-query tipRepository.findOne({
where: { idempotencyKey } }) and return the existing record instead of letting
the exception bubble; apply the same try-catch pattern to the other similar
block referenced at lines 182-185.
|
LGTM |
2 participants
closes #254
closes #256
Summary by CodeRabbit
Idempotency-Keyheader when creating tips. Resubmitting a request with the same key will return the previously created tip instead of creating a duplicate, ensuring safe retry semantics during network failures.