feat: update externalId to (dis-)connect identities #2128
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
|
Warning Rate limit exceeded@chronark has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 37 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughWalkthroughThe changes involve updates to key management functionality within the API. The Changes
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (2)
apps/api/src/routes/v1_keys_createKey.ts (2)
Line range hint
572-599: Consider enhancing error handling and consistency inupsertIdentityThe
upsertIdentityfunction looks good overall, but there are a few areas for improvement:
- Error handling: Consider wrapping the database operations in a try-catch block to handle potential database errors explicitly.
- Consistency in timestamp fields: The
updatedAtfield is set tonullfor new identities. Consider setting it to the current timestamp, similar tocreatedAt.- Error message specificity: The error message "Failed to read identity after upsert" could be more informative. Consider including the
workspaceIdandexternalIdin the error message.Here's a suggested refactor:
export async function upsertIdentity( db: Database, workspaceId: string, externalId: string, ): Promise<Identity> { - let identity = await db.query.identities.findFirst({ - where: (table, { and, eq }) => - and(eq(table.workspaceId, workspaceId), eq(table.externalId, externalId)), - }); - if (identity) { - return identity; - } + try { + let identity = await db.query.identities.findFirst({ + where: (table, { and, eq }) => + and(eq(table.workspaceId, workspaceId), eq(table.externalId, externalId)), + }); + if (identity) { + return identity; + } - await db - .insert(schema.identities) - .values({ - id: newId("identity"), - createdAt: Date.now(), - environment: "default", - meta: {}, - externalId, - updatedAt: null, - workspaceId, - }) - .onDuplicateKeyUpdate({ - set: { - updatedAt: Date.now(), - }, - }); + const now = Date.now(); + await db + .insert(schema.identities) + .values({ + id: newId("identity"), + createdAt: now, + environment: "default", + meta: {}, + externalId, + updatedAt: now, + workspaceId, + }) + .onDuplicateKeyUpdate({ + set: { + updatedAt: now, + }, + }); - identity = await db.query.identities.findFirst({ - where: (table, { and, eq }) => - and(eq(table.workspaceId, workspaceId), eq(table.externalId, externalId)), - }); - if (!identity) { + identity = await db.query.identities.findFirst({ + where: (table, { and, eq }) => + and(eq(table.workspaceId, workspaceId), eq(table.externalId, externalId)), + }); + if (!identity) { + throw new UnkeyApiError({ + code: "INTERNAL_SERVER_ERROR", + message: `Failed to read identity after upsert for workspaceId: ${workspaceId}, externalId: ${externalId}`, + }); + } + return identity; + } catch (error) { throw new UnkeyApiError({ code: "INTERNAL_SERVER_ERROR", - message: "Failed to read identity after upsert", + message: `Error upserting identity: ${error.message}`, }); } - return identity; }This refactor addresses the mentioned points and improves the overall robustness of the function.
Line range hint
225-229: Consider deprecatingownerIdin favor ofexternalIdThe code currently uses both
externalIdandownerId, withownerIdserving as a fallback. To improve consistency and align with the newexternalIdfield, consider deprecatingownerIdand usingexternalIdexclusively.Update the code as follows:
- const externalId = req.externalId ?? req.ownerId; + const externalId = req.externalId; + if (req.ownerId && !req.externalId) { + logger.warn("ownerId is deprecated, please use externalId instead", { + workspaceId: authorizedWorkspaceId, + apiId: api.id, + }); + externalId = req.ownerId; + } const [permissionIds, roleIds, identity] = await Promise.all([ getPermissionIds(db.readonly, authorizedWorkspaceId, req.permissions ?? []), getRoleIds(db.readonly, authorizedWorkspaceId, req.roles ?? []), - externalId + externalId ? upsertIdentity(db.primary, authorizedWorkspaceId, externalId) : Promise.resolve(null), ]);This change maintains backwards compatibility while encouraging the use of
externalIdand warning about the deprecatedownerId.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- apps/api/src/routes/v1_keys_createKey.ts (1 hunks)
- apps/api/src/routes/v1_keys_updateKey.happy.test.ts (1 hunks)
- apps/api/src/routes/v1_keys_updateKey.ts (3 hunks)
Additional comments not posted (3)
apps/api/src/routes/v1_keys_updateKey.ts (3)
9-9: Correct import ofupsertIdentityfunctionThe import statement correctly brings in the
upsertIdentityfunction from./v1_keys_createKey, which is necessary for handling identity upserts.
341-348: Correct implementation ofidentityIdassignmentThe logic for assigning
identityIdbased onexternalIdandownerIdis correctly implemented, handlingundefinedandnullvalues appropriately. This ensures backward compatibility while encouraging the use ofexternalId.
355-355: AddingidentityIdto key update operation
identityIdis correctly included in the key update operation, which associates the key with the appropriate identity in the database.
… 'clickhouse' host
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
… found variable refactor(v1_identities_updateIdentity.ts): remove unnecessary empty line
| @@ -124,6 +124,7 @@ test("sets new ratelimits", async (t) => { | |||
| where: (table, { eq }) => eq(table.identityId, identity.id), | |||
| }); | |||
|
|
|||
| console.log({ found }); | |||
There was a problem hiding this comment.
no that's on purpose, I just added it, cause this test only fails on github, not local
|
Ah then approved 👍 |
Summary by CodeRabbit
New Features
externalIdfield for updating keys, replacing the deprecatedownerIdfield.externalIdwhen connecting keys to identities.Bug Fixes
externalIdandownerId.Documentation
externalIdandownerId.