Fixed duplicate insert in posts table #746
Conversation
WalkthroughA new scenario was introduced in the Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code Graph Analysis (1)src/app.ts (2)
🔇 Additional comments (4)
✨ Finishing Touches
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/post/post.repository.knex.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build, Test and Push
🔇 Additional comments (1)
src/post/post.repository.knex.ts (1)
688-715: Good approach to handling concurrency issues.The addition of
posts.uuidduplicate detection effectively addresses the concurrency problem described in the PR objectives. The overall error handling structure is well-designed, using transactions appropriately and providing clear feedback to callers through theisDuplicateflag.
src/post/post.repository.knex.ts
Outdated
| (err.message.includes('ap_id_hash') || | ||
| err.message.includes('posts.uuid')) |
There was a problem hiding this comment.
Fix logical inconsistency in duplicate error handling.
The error handling logic checks for duplicate entry errors on either ap_id_hash or posts.uuid, but the recovery logic (lines 699-704) only searches for the existing post by ap_id_hash. This creates an inconsistency where UUID-based duplicates might not be properly handled.
Consider this approach to handle both cases:
if (
err instanceof Error &&
'code' in err &&
err.code === 'ER_DUP_ENTRY' &&
(err.message.includes('ap_id_hash') ||
err.message.includes('posts.uuid'))
) {
- const row = await transaction('posts')
- .whereRaw('ap_id_hash = UNHEX(SHA2(?, 256))', [
- post.apId.href,
- ])
- .select('id')
- .first();
+ // Try to find by ap_id_hash first (most common case)
+ let row = await transaction('posts')
+ .whereRaw('ap_id_hash = UNHEX(SHA2(?, 256))', [
+ post.apId.href,
+ ])
+ .select('id')
+ .first();
+
+ // If not found by ap_id_hash, try by UUID
+ if (!row) {
+ row = await transaction('posts')
+ .where('uuid', post.uuid)
+ .select('id')
+ .first();
+ }
if (row) {
return {
id: row.id,
isDuplicate: true,
};
}
}📝 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.
| (err.message.includes('ap_id_hash') || | |
| err.message.includes('posts.uuid')) | |
| if ( | |
| err instanceof Error && | |
| 'code' in err && | |
| err.code === 'ER_DUP_ENTRY' && | |
| (err.message.includes('ap_id_hash') || | |
| err.message.includes('posts.uuid')) | |
| ) { | |
| // Try to find by ap_id_hash first (most common case) | |
| let row = await transaction('posts') | |
| .whereRaw('ap_id_hash = UNHEX(SHA2(?, 256))', [ | |
| post.apId.href, | |
| ]) | |
| .select('id') | |
| .first(); | |
| // If not found by ap_id_hash, try by UUID | |
| if (!row) { | |
| row = await transaction('posts') | |
| .where('uuid', post.uuid) | |
| .select('id') | |
| .first(); | |
| } | |
| if (row) { | |
| return { | |
| id: row.id, | |
| isDuplicate: true, | |
| }; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/post/post.repository.knex.ts around lines 696 to 697, the error handling
checks for duplicates on both ap_id_hash and posts.uuid, but the recovery logic
only handles ap_id_hash duplicates. To fix this, update the recovery logic to
distinguish which duplicate key caused the error and query the existing post
accordingly—use ap_id_hash if that caused the error, or use posts.uuid if that
was the duplicate key—ensuring both cases are properly handled.
allouis
force-pushed
the
fix-dup-entry-posts
branch
2 times, most recently
from
053eab1 to
1f63a9e
Compare
allouis
force-pushed
the
fix-dup-entry-posts
branch
from
77dded2 to
1786516
Compare
closes https://linear.app/ghost/issue/PROD-1905
When we recieve a second webhook for the same post, we attempt to save another post with the same
uuid- which throws a duplicate key error.We should really be doing some kind of update on the post, but we don't current support that and it's out of scope of fixing the error.
The fix here moves the logic for handling incoming ghost posts to the PostService, and we check if the post has already been processed, returning an error if it has.
This allows the HTTP handler to respond with a 400 instead of a 500