fix: upsert identity when creating keys in dashboard #2111
Conversation
DeepaPrasanna
requested review from
perkinsjr,
chronark and
mcstepp
as code owners
|
|
@DeepaPrasanna is attempting to deploy a commit to the Unkey Team on Vercel. A member of the Team first needs to authorize it. |
📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduced in this pull request include the addition of a new type declaration for the database in Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
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! 🙏 |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| let identity = await db.query.identities.findFirst({ | ||
| where: (table, { and, eq }) => | ||
| and(eq(table.workspaceId, workspaceId), eq(table.externalId, externalId)), | ||
| }); | ||
| if (identity) { | ||
| return identity; | ||
| } | ||
|
|
There was a problem hiding this comment.
Why are you checking this and then returning it again.
You shouldn't need to check the identity and then if it exists return it, it is already there.
There was a problem hiding this comment.
Sorry if I missed anything, but I thought we had to do an upsert here. As mentioned in the issue, I also referred to the code here
There was a problem hiding this comment.
Sorry for the delay my notifications didn't ping, if you wanted to check for upserting. Why not do and early return the other way.
You current implementation just says query the db and set to variable identity, check if the identity exists and then return the identity, but you are then further down using the upsert functionality.
let identity = await db.query.identities.findFirst({
where: (table, { and, eq }) =>
and(eq(table.workspaceId, workspaceId), eq(table.externalId, externalId)),
});
if (identity) {
return identity;
}When really could do:
let identity = await db.query.identities.findFirst({
where: (table, { and, eq }) =>
and(eq(table.workspaceId, workspaceId), eq(table.externalId, externalId)),
});
if (!identity) {
// run inserts
}or
let identity = await db.query.identities.findFirst({
where: (table, { and, eq }) =>
and(eq(table.workspaceId, workspaceId), eq(table.externalId, externalId)),
});
//handle the identity further downThere was a problem hiding this comment.
Got it! Understood what you meant. Thank you!
…Prasanna/unkey into fix/upsert-identity-keys-dashboard
…Prasanna/unkey into fix/upsert-identity-keys-dashboard
![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 (1)
apps/dashboard/lib/trpc/routers/key/create.ts (1)
144-195: Add unit tests for theupsertIdentityfunctionThe
upsertIdentityfunction plays a crucial role in ensuring that identities are correctly managed. Adding unit tests for this function will help catch any future bugs and ensure reliability.Consider writing tests that cover:
- Inserting a new identity when none exists.
- Retrieving an existing identity without creating a new one.
- Handling database errors gracefully.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- apps/dashboard/lib/trpc/routers/key/create.ts (5 hunks)
🔇 Additional comments (1)
apps/dashboard/lib/trpc/routers/key/create.ts (1)
Line range hint
103-103: Reassess the necessity of bothownerIdandidentityIdfieldsIn the key insertion, you are setting both
ownerIdandidentityId:
ownerId: input.ownerId,identityId: identity?.id,Given that
identityIdnow represents the link to the identity, andownerIdis used to create or find this identity, having both fields may be redundant. This could lead to confusion or inconsistency in data handling.Check where
ownerIdis used in the codebase and determine if it can be deprecated in favor ofidentityId. Consolidating these fields might simplify the data model.You can use the following script to find all references to
ownerId:Also applies to: 114-114
| .insert(schema.identities) | ||
| .values({ | ||
| id: newId("identity"), | ||
| createdAt: Date.now(), |
There was a problem hiding this comment.
Use new Date() instead of Date.now() for consistent date objects
In your code, you're using Date.now() to set createdAt and updatedAt, which returns a timestamp number. However, elsewhere in your code, such as in line 96~, you use new Date(), which returns a Date object. This inconsistency can lead to issues if the database expects a Date object rather than a timestamp.
Apply this diff to fix the inconsistency:
- createdAt: Date.now(),
+ createdAt: new Date(),And in the onDuplicateKeyUpdate block:
- updatedAt: Date.now(),
+ updatedAt: new Date(),Also applies to: 168-168
| let identity = await db.query.identities.findFirst({ | ||
| where: (table, { and, eq }) => | ||
| and(eq(table.workspaceId, workspaceId), eq(table.externalId, externalId)), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize upsertIdentity to avoid unnecessary database queries
Currently, if the identity does not exist, you insert it and then perform another query to retrieve the identity. This results in two database calls. You can optimize this by using the result of the insert operation or by using an UPSERT that returns the updated or inserted row if your database supports it.
Modify the upsertIdentity function to return the identity directly from the insert operation:
if (!identity) {
- await db
+ identity = await db
.insert(schema.identities)
.values({
id: newId("identity"),
- createdAt: Date.now(),
+ createdAt: new Date(),
environment: "default",
- meta: {},
+ meta: JSON.stringify({}),
externalId,
- updatedAt: null,
+ updatedAt: null,
workspaceId,
})
.onDuplicateKeyUpdate({
set: {
- updatedAt: Date.now(),
+ updatedAt: new Date(),
},
})
+ .returning()
+ .then((rows) => rows[0])
.catch((_err) => {
throw new TRPCError({
code: "INTERNAL_SERVER_ERROR",
message: "Failed to insert identity",
});
});
- identity = await db.query.identities
- .findFirst({
- where: (table, { and, eq }) =>
- and(
- eq(table.workspaceId, workspaceId),
- eq(table.externalId, externalId)
- ),
- })
- .catch((_err) => {
- throw new TRPCError({
- code: "INTERNAL_SERVER_ERROR",
- message: "Failed to read identity after upsert",
- });
- });
}This change eliminates the need for a second query, improving the efficiency of the function.
Also applies to: 178-185
| id: newId("identity"), | ||
| createdAt: Date.now(), | ||
| environment: "default", | ||
| meta: {}, |
There was a problem hiding this comment.
Ensure consistent handling of the meta field
In the identities table insertion, meta is set to an empty object {}. In contrast, when inserting into the keys table (line 96~), meta is serialized using JSON.stringify. To maintain consistency and prevent potential issues with data storage and retrieval, ensure that meta is handled the same way in both cases.
Consider serializing meta when inserting into the identities table:
- meta: {},
+ meta: JSON.stringify({}),Or, if the database schema for identities.meta accepts JSON objects directly, ensure that this is consistently handled across your codebase.
📝 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.
| meta: {}, | |
| meta: JSON.stringify({}), |
…Prasanna/unkey into fix/upsert-identity-keys-dashboard
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
apps/dashboard/lib/trpc/routers/key/create.ts (1)
80-83: LGTM: Identity upsert logic addedThe addition of the
identityvariable with conditionalupsertIdentitycall effectively addresses the issue of linking or creating an identity when a key is created with anownerId. This implementation aligns well with the PR objectives.Consider using optional chaining for a more concise expression:
const identity = input.ownerId ? await upsertIdentity(db, keyAuth.workspaceId, input.ownerId) : null;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- apps/dashboard/lib/trpc/routers/key/create.ts (4 hunks)
🔇 Additional comments (3)
apps/dashboard/lib/trpc/routers/key/create.ts (3)
1-1: LGTM: New imports added for Database and Identity typesThe addition of
DatabaseandIdentitytype imports from "@/lib/db" is consistent with the newupsertIdentityfunction introduced in this file. This change enhances type safety and improves code readability.
114-114: LGTM: Identity linked to key in databaseThe addition of the
identityIdfield in the key insertion, usingidentity?.id, effectively links the upserted identity to the newly created key in the database. This change fulfills the PR objective and ensures proper association between keys and identities.
Line range hint
1-192: Overall assessment: PR objectives achieved with minor improvements suggestedThe changes made in this file successfully address the main objectives of the PR:
- An
identityIdfield has been added to the keys table.- An identity is now upserted when creating a key with an
ownerId.- The created/retrieved identity is properly linked to the newly created key.
These modifications effectively resolve the issue where creating a key with an
ownerIddid not link or create the corresponding identity in the database.The implementation is generally sound, with only minor optimizations and consistency improvements suggested in the previous comments. Once these suggestions are addressed, the code will be more efficient and maintainable.
Great job on implementing this feature! The changes align well with the PR objectives and significantly improve the functionality of the key creation process.
| 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) { | ||
| await db | ||
| .insert(schema.identities) | ||
| .values({ | ||
| id: newId("identity"), | ||
| createdAt: Date.now(), | ||
| environment: "default", | ||
| meta: {}, | ||
| externalId, | ||
| updatedAt: null, | ||
| workspaceId, | ||
| }) | ||
| .onDuplicateKeyUpdate({ | ||
| set: { | ||
| updatedAt: Date.now(), | ||
| }, | ||
| }) | ||
| .catch((_err) => { | ||
| throw new TRPCError({ | ||
| code: "INTERNAL_SERVER_ERROR", | ||
| message: "Failed to insert identity", | ||
| }); | ||
| }); | ||
|
|
||
| identity = await db.query.identities | ||
| .findFirst({ | ||
| where: (table, { and, eq }) => | ||
| and(eq(table.workspaceId, workspaceId), eq(table.externalId, externalId)), | ||
| }) | ||
| .catch((_err) => { | ||
| throw new TRPCError({ | ||
| code: "INTERNAL_SERVER_ERROR", | ||
| message: "Failed to read identity after upsert", | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| return identity as Identity; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize upsertIdentity function and address inconsistencies
The new upsertIdentity function effectively handles the creation or retrieval of an identity. However, there are several areas for improvement:
-
Reduce database queries: The function currently performs two separate database queries when inserting a new identity. Consider using a single upsert operation that returns the inserted/updated row.
-
Improve error handling: Use more specific error codes and messages to aid in debugging and user feedback.
-
Ensure consistency in date handling: Use
new Date()instead ofDate.now()for consistency with other parts of the codebase. -
Handle
metafield consistently: Ensure themetafield is handled consistently across the codebase, either as a JSON string or an object.
Here's a suggested refactor:
async function upsertIdentity(
db: Database,
workspaceId: string,
externalId: string,
): Promise<Identity> {
try {
const result = await db
.insert(schema.identities)
.values({
id: newId("identity"),
createdAt: new Date(),
environment: "default",
meta: JSON.stringify({}),
externalId,
updatedAt: null,
workspaceId,
})
.onConflict((oc) =>
oc.constraint('identities_workspace_id_external_id_unique').merge({
updatedAt: new Date(),
})
)
.returning()
.execute();
return result[0] as Identity;
} catch (error) {
throw new TRPCError({
code: "INTERNAL_SERVER_ERROR",
message: "Failed to upsert identity",
cause: error,
});
}
}This refactored version addresses the identified issues and improves the overall efficiency and consistency of the function.
What does this PR do?
Fixes #2098
Add
identityIdin keys table and upsert identity in identities table.If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Bug Fixes