Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: upsert identity when creating keys in dashboard #2111

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

fix: upsert identity when creating keys in dashboard #2111

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
aef29c2
fix: upsert identity when creating keys in dashboard
DeepaPrasanna Sep 18, 2024
caf3f87
Merge branch 'unkeyed:main' into fix/upsert-identity-keys-dashboard
DeepaPrasanna Sep 18, 2024
db49abd
fix: add error handling
DeepaPrasanna Sep 20, 2024
fd7369c
Merge branch 'main' into fix/upsert-identity-keys-dashboard
DeepaPrasanna Sep 20, 2024
fe706b7
Merge branch 'main' into fix/upsert-identity-keys-dashboard
DeepaPrasanna Sep 25, 2024
ed476fd
refactor: write the upsert identity in a better way
DeepaPrasanna Oct 2, 2024
bece6ec
Merge branch 'fix/upsert-identity-keys-dashboard' of github.com:Deepa…
DeepaPrasanna Oct 2, 2024
37dd148
Merge branch 'fix/upsert-identity-keys-dashboard' of github.com:Deepa…
DeepaPrasanna Oct 2, 2024
b3797a4
refactor: run pnpm fmt
DeepaPrasanna Oct 2, 2024
b4f9273
Merge branch 'fix/upsert-identity-keys-dashboard' of github.com:Deepa…
DeepaPrasanna Oct 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion apps/dashboard/lib/db.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { dbEnv } from "@/lib/env";
import { Client } from "@planetscale/database";
import { drizzle, schema } from "@unkey/db";
DeepaPrasanna marked this conversation as resolved.
Show resolved Hide resolved
import { type PlanetScaleDatabase, drizzle, schema } from "@unkey/db";
export type Database = PlanetScaleDatabase<typeof schema>;

export const db = drizzle(
new Client({
Expand Down
50 changes: 49 additions & 1 deletion apps/dashboard/lib/trpc/routers/key/create.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { db, schema } from "@/lib/db";
import { type Database, type Identity, db, schema } from "@/lib/db";
import { ingestAuditLogs } from "@/lib/tinybird";
import { TRPCError } from "@trpc/server";
import { newId } from "@unkey/id";
Expand Down Expand Up @@ -62,6 +62,10 @@ export const createKey = t.procedure
});
}

const identity = input.ownerId
? await upsertIdentity(db, keyAuth.workspaceId, input.ownerId)
: null;

const keyId = newId("key");
const { key, hash, start } = await newKey({
prefix: input.prefix,
Expand Down Expand Up @@ -92,6 +96,7 @@ export const createKey = t.procedure
deletedAt: null,
enabled: input.enabled,
environment: input.environment,
identityId: identity?.id,
})
.catch((_err) => {
throw new TRPCError({
Expand Down Expand Up @@ -120,3 +125,46 @@ export const createKey = t.procedure

return { keyId, key };
});

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)),
});
Comment on lines +149 to +152
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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

if (identity) {
return identity;
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@perkinsjr perkinsjr Oct 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 down

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it! Understood what you meant. Thank you!

await db
.insert(schema.identities)
.values({
id: newId("identity"),
createdAt: Date.now(),
environment: "default",
meta: {},
externalId,
updatedAt: null,
workspaceId,
})
.onDuplicateKeyUpdate({
set: {
updatedAt: Date.now(),
},
});
DeepaPrasanna marked this conversation as resolved.
Show resolved Hide resolved

identity = await db.query.identities.findFirst({
where: (table, { and, eq }) =>
and(eq(table.workspaceId, workspaceId), eq(table.externalId, externalId)),
});
DeepaPrasanna marked this conversation as resolved.
Show resolved Hide resolved
if (!identity) {
throw new TRPCError({
code: "INTERNAL_SERVER_ERROR",
message: "Failed to read identity after upsert",
});
}
return identity;
}