Conversation
chronark
requested review from
MichaelUnkey,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
📝 WalkthroughWalkthroughThis pull request introduces a new feature for hard deleting keys in the API. The changes modify the Changes
Sequence DiagramsequenceDiagram
participant Client
participant API
participant Database
Client->>API: Delete Key Request
alt Soft Delete (default)
API->>Database: Update deletedAt timestamp
else Hard Delete
API->>Database: Permanently remove key
end
API->>Client: Deletion Confirmation
Possibly related PRs
Suggested labels
Suggested reviewers
✨ 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 (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/api/src/routes/v1_keys_deleteKey.happy.test.ts (1)
47-79: Consider reducing test duplication and adding edge cases.The test implementation looks good but could be improved in a few areas:
- Consider extracting the common setup code into a helper function to reduce duplication
- Add test cases for edge scenarios:
- Attempting to permanently delete an already deleted key
- Attempting to soft delete an already deleted key
Here's a suggested refactor to reduce duplication:
async function setupTestKey(h: IntegrationHarness) { const keyId = newId("test"); const key = new KeyV1({ prefix: "test", byteLength: 16 }).toString(); await h.db.primary.insert(schema.keys).values({ id: keyId, keyAuthId: h.resources.userKeyAuth.id, hash: await sha256(key), start: key.slice(0, 8), workspaceId: h.resources.userWorkspace.id, createdAt: new Date(), }); return { keyId, key }; }apps/api/src/routes/v1_keys_deleteKey.ts (3)
27-30: Consider adding a warning about permanent deletion being irreversible.The OpenAPI description should emphasize that permanent deletion is irreversible. This helps prevent accidental permanent deletions.
permanent: z.boolean().default(false).optional().openapi({ description: - "By default Unkey soft deletes keys, so they may be recovered later. If you want to permanently delete it, set permanent=true. This might be necessary if you run into NOT_UNIQUE errors during key migration.", + "By default Unkey soft deletes keys, so they may be recovered later. If you want to permanently delete it, set permanent=true. WARNING: Permanent deletion is irreversible! This should only be used if you run into NOT_UNIQUE errors during key migration.", }),
131-138: Consider adding rate limiting for permanent deletions.Permanent deletions are destructive operations that should be rate-limited to prevent abuse or accidental mass deletions.
Consider implementing one of these approaches:
- Add a separate permission for permanent deletions
- Implement rate limiting specifically for permanent deletions
- Add a confirmation mechanism for permanent deletions
Would you like me to provide an implementation for any of these approaches?
Line range hint
154-156: Consider optimizing cache invalidation.The cache invalidation is correct but could be optimized by running the invalidations in parallel with the transaction.
- await Promise.all([cache.keyByHash.remove(key.hash), cache.keyById.remove(key.id)]); + const cacheInvalidation = Promise.all([ + cache.keyByHash.remove(key.hash), + cache.keyById.remove(key.id) + ]); + + await Promise.all([ + cacheInvalidation, + db.primary.transaction(async (tx) => { + // ... transaction code ... + }) + ]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/src/routes/v1_keys_deleteKey.happy.test.ts(1 hunks)apps/api/src/routes/v1_keys_deleteKey.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./internal/resend
- GitHub Check: Test Packages / Test ./internal/keys
- GitHub Check: Test Packages / Test ./internal/id
- GitHub Check: Test Packages / Test ./internal/hash
- GitHub Check: Test Packages / Test ./internal/encryption
- GitHub Check: Test Packages / Test ./internal/billing
- GitHub Check: Test API / API Test Local
- GitHub Check: Test GO API Local / test_agent_local
- GitHub Check: Build / Build
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
apps/api/src/routes/v1_keys_deleteKey.ts (1)
60-60: Verify error handling for already deleted keys.The current implementation might allow permanently deleting an already soft-deleted key. We should verify this behavior is intentional and properly handled.
Also applies to: 131-138
perkinsjr
left a comment
perkinsjr
left a comment
There was a problem hiding this comment.
😅 - Approved
(PTSD of one frightful day in summer)
|
I had an amazing time |
3 participants
Summary by CodeRabbit
New Features
Tests