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 all 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
57 changes: 56 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 { rateLimitedProcedure, ratelimit } from "@/lib/trpc/ratelimitProcedure";
import { TRPCError } from "@trpc/server";
Expand Down Expand Up @@ -77,6 +77,10 @@ export const createKey = rateLimitedProcedure(ratelimit.create)
});
}

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 @@ -107,6 +111,7 @@ export const createKey = rateLimitedProcedure(ratelimit.create)
deletedAt: null,
enabled: input.enabled,
environment: input.environment,
identityId: identity?.id,
})
.catch((_err) => {
throw new TRPCError({
Expand Down Expand Up @@ -135,3 +140,53 @@ export const createKey = rateLimitedProcedure(ratelimit.create)

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) {
await db
.insert(schema.identities)
.values({
id: newId("identity"),
createdAt: Date.now(),
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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

environment: "default",
meta: {},
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
meta: {},
meta: JSON.stringify({}),

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;
}
Comment on lines +144 to +192
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

⚠️ Potential issue

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:

  1. 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.

  2. Improve error handling: Use more specific error codes and messages to aid in debugging and user feedback.

  3. Ensure consistency in date handling: Use new Date() instead of Date.now() for consistency with other parts of the codebase.

  4. Handle meta field consistently: Ensure the meta field 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.