fix: transaction deadlock in trpc #2843
Conversation
Fixes transaction deadlocks around audit log buckets. This moves the workspace fetch to the beginning of the trpc handler and loads the bucket id once, to be reused. In theory we can now cache the workspace lookup but I'm not sure if Next.js does that automatically now or if we need to do it.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
|
Warning Rate limit exceeded@chronark has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 31 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. ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (31)
📝 WalkthroughWalkthroughThis change set updates audit logging across multiple components by modifying the insertAuditLogs function to require an explicit audit log bucket ID from the workspace context. Several TRPC router procedures and UI components have been refactored to directly use workspace data (ctx.workspace) while removing redundant database queries and tenant validations. New type definitions and schema modifications were introduced, and additional packages for LLM search, rate limiting, certificate management, and circuit breaking were added. GitHub workflow configuration files were also adjusted for formatting and clarity. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Context
participant Database
Client->>Context: API Request initiates context creation
Context->>Database: Query workspace by tenantId
Database-->>Context: Return workspace (with/without auditLogBucket)
alt AuditLogBucket missing
Context->>Database: Insert new auditLogBucket record
Database-->>Context: Return new bucket ID
end
Context-->>Client: Respond with context { workspace, auditLogBucket }
sequenceDiagram
participant Client
participant llmSearch
participant RateLimiter
participant OpenAI
Client->>llmSearch: Send search query
llmSearch->>RateLimiter: Enforce rate limit
RateLimiter-->>llmSearch: Permit request
llmSearch->>OpenAI: Request structured search (query)
OpenAI-->>llmSearch: Return structured result
llmSearch-->>Client: Deliver search result
Possibly related PRs
Suggested labels
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! 🙏 |
chronark
requested review from
perkinsjr,
mcstepp,
ogzhanolguncu and
MichaelUnkey
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🔭 Outside diff range comments (5)
apps/dashboard/lib/trpc/routers/rbac/connectPermissionToRole.ts (1)
Line range hint
15-31: Potential race condition between workspace fetches.The code fetches workspace data directly from the database while also relying on
ctx.workspace.auditLogBucket.idlater. This creates a potential consistency issue where the workspace in context might differ from the one fetched. Consider using the workspace from context throughout to maintain consistency and prevent deadlocks.- const workspace = await db.query.workspaces - .findFirst({ - where: (table, { and, eq, isNull }) => - and(eq(table.tenantId, ctx.tenant.id), isNull(table.deletedAt)), - with: { - roles: { - where: (table, { eq }) => eq(table.id, input.roleId), - }, - permissions: { - where: (table, { eq }) => eq(table.id, input.permissionId), - }, - }, - }) + // Use workspace from context + const workspace = ctx.workspace; + // Fetch only the required role and permission + const [role, permission] = await Promise.all([ + db.query.roles.findFirst({ + where: (table, { eq }) => eq(table.id, input.roleId), + }), + db.query.permissions.findFirst({ + where: (table, { eq }) => eq(table.id, input.permissionId), + }), + ]);apps/dashboard/lib/trpc/routers/workspace/changeName.ts (1)
The race condition concern in the audit log is valid and should be addressed.
The current implementation could lead to incorrect audit logs if concurrent name updates occur. To fix this:
- Fetch the current workspace name within the transaction before the update
- Use the fetched name in the audit log instead of
ctx.workspace.nameExample locations that need attention:
apps/dashboard/lib/trpc/routers/workspace/changeName.ts: Move workspace name retrieval inside the transaction block before the update.🔗 Analysis chain
Line range hint
31-46: Consider fetching the current workspace name within the transaction.The current implementation reads
ctx.workspace.namefrom the context, which could be stale if another transaction updates the name concurrently. Consider fetching the current name within the transaction for the audit log.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for concurrent workspace name updates rg -A 5 "update.*workspaces.*set.*name" --type tsLength of output: 51
Script:
#!/bin/bash # Search for workspace updates using multiple patterns echo "=== Searching for Prisma workspace updates ===" rg "workspace\s*\.\s*update" -A 5 --type ts echo -e "\n=== Searching for workspace name changes ===" rg "workspace.*name.*=" -A 5 --type ts echo -e "\n=== Checking transaction patterns ===" ast-grep --pattern 'prisma.$transaction(async ($$$) => { $$$ })'Length of output: 9136
Script:
#!/bin/bash # Search for Prisma workspace name updates with various patterns echo "=== Searching for Prisma workspace name updates ===" rg "prisma.*workspace.*update.*name" -A 5 --type ts echo -e "\n=== Searching for updateName implementation ===" ast-grep --pattern 'updateName = procedure .$$$ .mutation(async ({ $$$, input }) => { $$$ })' echo -e "\n=== Checking for direct name updates ===" rg "name.*=.*input\.name" -A 5 --type tsLength of output: 523
Script:
#!/bin/bash # Check the changeName.ts implementation echo "=== Checking changeName.ts implementation ===" cat apps/dashboard/lib/trpc/routers/workspace/changeName.ts echo -e "\n=== Searching for update in changeName.ts ===" rg "update" apps/dashboard/lib/trpc/routers/workspace/changeName.ts -A 5Length of output: 3338
apps/dashboard/lib/trpc/routers/rbac/updatePermission.ts (1)
Line range hint
38-46: Add error handling for permission update operation.The permission update operation inside the transaction is missing error handling, unlike similar operations in other files.
await tx .update(schema.permissions) .set({ name: input.name, description: input.description, updatedAt: new Date(), }) - .where(eq(schema.permissions.id, permission.id)); + .where(eq(schema.permissions.id, permission.id)) + .catch((_err) => { + throw new TRPCError({ + code: "INTERNAL_SERVER_ERROR", + message: "We are unable to update the permission. Please try again or contact support@unkey.dev.", + }); + });apps/dashboard/lib/trpc/routers/key/delete.ts (1)
Line range hint
14-33: Consider moving workspace fetch to use context.The separate workspace fetch could still cause deadlocks. Consider using
ctx.workspaceinstead, similar to other files in this PR.- const workspace = await db.query.workspaces - .findFirst({ - where: (table, { and, eq, isNull }) => - and(eq(table.tenantId, ctx.tenant.id), isNull(table.deletedAt)), - with: { - keys: { - where: (table, { and, inArray, isNull }) => - and(isNull(table.deletedAt), inArray(table.id, input.keyIds)), - columns: { - id: true, - }, - }, - }, - }) + const keys = await db.query.keys + .findMany({ + where: (table, { and, inArray, isNull }) => + and( + eq(table.workspaceId, ctx.workspace.id), + isNull(table.deletedAt), + inArray(table.id, input.keyIds) + ), + columns: { + id: true, + }, + })apps/dashboard/lib/trpc/routers/api/updateDeleteProtection.ts (1)
Line range hint
63-66: Fix template literal syntax error.There's an extra closing brace in the description string.
- description: `API ${api.name} delete protection is now ${ - input.enabled ? "enabled" : "disabled" - }.}`, + description: `API ${api.name} delete protection is now ${ + input.enabled ? "enabled" : "disabled" + }.`,
🧹 Nitpick comments (26)
apps/dashboard/lib/trpc/routers/rbac/connectPermissionToRole.ts (2)
Line range hint
32-48: Enhance error handling with specific error messages and proper error propagation.The current error handling:
- Uses generic error messages
- Swallows original errors in catch blocks
- Doesn't distinguish between different types of errors
Consider adding more specific error messages and proper error logging:
if (!role) { throw new TRPCError({ code: "NOT_FOUND", - message: - "We are unable to find the correct role. Please try again or contact support@unkey.dev.", + message: `Role with ID ${input.roleId} not found in workspace ${workspace.id}.`, }); } if (!permission) { throw new TRPCError({ code: "NOT_FOUND", - message: - "We are unable to find the correct permission. Please try again or contact support@unkey.dev.", + message: `Permission with ID ${input.permissionId} not found in workspace ${workspace.id}.`, }); }
Line range hint
79-98: Improve audit log description and context.While the audit logging changes address the deadlock issue by using a consistent bucket ID, the audit log description could be more informative.
await insertAuditLogs(tx, ctx.workspace.auditLogBucket.id, { workspaceId: workspace.id, actor: { type: "user", id: ctx.user.id }, event: "authorization.connect_role_and_permission", - description: `Connect role ${role.id} to ${permission.id}`, + description: `Connected role "${role.name}" (${role.id}) to permission "${permission.name}" (${permission.id})`, resources: [ { type: "role", id: role.id, + name: role.name, }, { type: "permission", id: permission.id, + name: permission.name, }, ], context: { location: ctx.audit.location, userAgent: ctx.audit.userAgent, + roleId: role.id, + permissionId: permission.id, }, });apps/dashboard/lib/trpc/routers/ratelimit/deleteNamespace.ts (2)
Line range hint
74-82: Improve error message specificity in catch blocks.The error messages in both catch blocks are identical but handle different operations (override deletion vs audit log insertion). Consider making them more specific to help with debugging.
- message: - "We are unable to delete the namespaces. Please try again or contact support@unkey.dev", + message: + "Failed to delete rate limit overrides. Please try again or contact support@unkey.dev", // And in the audit log catch block: - message: - "We are unable to delete the namespaces. Please try again or contact support@unkey.dev", + message: + "Failed to record audit logs for override deletion. Please try again or contact support@unkey.dev",Also applies to: 108-116
Line range hint
45-60: Consider extracting common audit log properties.The audit log entries for both namespace and override deletions share common properties (actor, context). Consider extracting these to reduce duplication and improve maintainability.
+ const commonAuditProps = { + workspaceId: ctx.workspace.id, + actor: { + type: "user" as const, + id: ctx.user.id, + }, + context: { + location: ctx.audit.location, + userAgent: ctx.audit.userAgent, + }, + }; await insertAuditLogs(tx, ctx.workspace.auditLogBucket.id, { - workspaceId: namespace.workspaceId, - actor: { - type: "user", - id: ctx.user.id, - }, + ...commonAuditProps, event: "ratelimitNamespace.delete", description: `Deleted ${namespace.id}`, resources: [/*...*/], - context: { - location: ctx.audit.location, - userAgent: ctx.audit.userAgent, - }, }); // Later in the overrides audit logs await insertAuditLogs( tx, ctx.workspace.auditLogBucket.id, overrides.map(({ id }) => ({ - workspaceId: ctx.workspace.id, - actor: { - type: "user", - id: ctx.user.id, - }, + ...commonAuditProps, event: "ratelimit.delete_override", description: `Deleted ${id} as part of the ${namespace.id} deletion`, resources: [/*...*/], - context: { - location: ctx.audit.location, - userAgent: ctx.audit.userAgent, - }, })), );Also applies to: 84-106
apps/dashboard/lib/trpc/routers/rbac/removePermissionFromRootKey.ts (1)
59-60: Consider schema optimization for audit logs.The current implementation correctly uses
ctx.workspace.auditLogBucket.idto prevent deadlocks, which is good. However, I notice that audit logs require bothbucket.idandworkspace.id. Consider if this dual ID requirement could be simplified in the schema to further reduce potential deadlock scenarios.apps/dashboard/lib/trpc/context.ts (1)
14-45: Be mindful of concurrency when creating a new audit log bucket.If two concurrent requests try to create the same bucket for the same workspace, you risk duplicate records or deadlocks. Consider placing a unique constraint on
(workspaceId, name)in the database and re-check if the bucket was created during the transaction to avoid potential conflicts.apps/dashboard/lib/trpc/routers/ratelimit/updateNamespaceName.ts (1)
38-38: Clarify the error message to match the missing namespace context.Current message references a workspace, which might confuse users when the real issue is a missing namespace. Consider updating the error message to reflect that the namespace was not found or is not associated with the current workspace.
apps/dashboard/lib/trpc/routers/workspace/changePlan.ts (1)
124-124: Stripe-related checks are sound, but handle non-default payment scenarios carefully.Some users might have multiple payment methods, expired cards, or upcoming invoice items. As an optional refinement, consider clarifying or logging edge cases to assist with support issues.
Also applies to: 131-131
apps/dashboard/lib/trpc/routers/rbac/disconnectRoleFromKey.ts (3)
21-24: Consider adding NOWAIT or SKIP LOCKED to prevent deadlocks.While using transactions is correct, consider adding lock timeouts or skip locked options to prevent potential deadlocks when multiple transactions compete for the same keysRoles records.
await tx .delete(schema.keysRoles) + .setLockTimeout(3000) // 3 seconds timeout .where( and( eq(schema.keysRoles.workspaceId, ctx.workspace.id), eq(schema.keysRoles.roleId, input.roleId), eq(schema.keysRoles.keyId, input.keyId), ), );
Line range hint
26-44: Consider separating audit log insertion from the main transaction.The current transaction combines both the role disconnection and audit log insertion. Consider separating these operations to reduce transaction duration and minimize contention on the audit log bucket.
Line range hint
45-52: Enhance error handling with specific error cases.The current error handling is generic. Consider adding specific error cases for common failures like:
- Role not found
- Key not found
- Permission denied
- Deadlock detected
.catch((err) => { console.error(err); + if (err.code === '40P01') { // PostgreSQL deadlock detected + throw new TRPCError({ + code: "CONFLICT", + message: "Operation conflicted with another request. Please try again.", + }); + } throw new TRPCError({ code: "INTERNAL_SERVER_ERROR", message: "We are unable to disconnect the role from the key. Please try again or contact support@unkey.dev", }); });apps/dashboard/lib/trpc/routers/ratelimit/createNamespace.ts (1)
Line range hint
1-1: Consider implementing workspace caching as mentioned in PR objectives.The changes consistently move workspace fetching to the trpc handler context, which effectively addresses the transaction deadlock issues. As mentioned in the PR objectives, there's potential for further optimization through workspace caching.
Consider implementing a caching layer for workspace data if Next.js doesn't already handle this. This could further reduce database load and improve performance. Some options to consider:
- React Query's caching mechanisms
- Next.js Server Components with built-in caching
- Redis-based caching for workspace data
apps/dashboard/lib/trpc/routers/rbac/deletePermission.ts (1)
32-33: Redundant workspace ID check in delete query.The workspace ID check in the delete query is redundant since we already verified the permission belongs to the workspace. This adds an unnecessary condition to the index scan.
.delete(schema.permissions) .where( - and( - eq(schema.permissions.id, permission.id), - eq(schema.permissions.workspaceId, ctx.workspace.id), - ), + eq(schema.permissions.id, permission.id), );apps/dashboard/lib/trpc/routers/workspace/optIntoBeta.ts (1)
14-32: Optimize switch statement using object literal.The switch statement can be simplified using an object literal mapping, making it more maintainable and less prone to errors.
+ const betaFeatureMap = { + rbac: 'rbac', + identities: 'identities', + ratelimit: 'ratelimit', + logsPage: 'logsPage' + } as const; + + ctx.workspace.betaFeatures[betaFeatureMap[input.feature]] = true; - switch (input.feature) { - case "rbac": { - ctx.workspace.betaFeatures.rbac = true; - break; - } - case "identities": { - ctx.workspace.betaFeatures.identities = true; - break; - } - case "ratelimit": { - ctx.workspace.betaFeatures.ratelimit = true; - break; - } - case "logsPage": { - ctx.workspace.betaFeatures.logsPage = true; - break; - } - }apps/dashboard/lib/trpc/routers/rbac/deleteRole.ts (1)
Line range hint
28-58: Simplify error handling to avoid nested catch blocks.The current implementation has nested catch blocks that could mask errors and complicate transaction rollback. Consider simplifying the error handling.
await db.transaction(async (tx) => { - await tx - .delete(schema.roles) - .where( - and(eq(schema.roles.id, input.roleId), eq(schema.roles.workspaceId, ctx.workspace.id)), - ) - .catch((_err) => { - throw new TRPCError({ - code: "INTERNAL_SERVER_ERROR", - message: - "We are unable to delete the role. Please try again or contact support@unkey.dev", - }); - }); + await tx.delete(schema.roles).where( + and(eq(schema.roles.id, input.roleId), eq(schema.roles.workspaceId, ctx.workspace.id)), + ); - await insertAuditLogs(tx, ctx.workspace.auditLogBucket.id, { - // ... audit log details ... - }).catch((err) => { - console.error(err); - throw new TRPCError({ - code: "INTERNAL_SERVER_ERROR", - message: - "We are unable to delete the role. Please try again or contact support@unkey.dev.", - }); - }); + await insertAuditLogs(tx, ctx.workspace.auditLogBucket.id, { + // ... audit log details ... + }); });apps/dashboard/lib/trpc/routers/key/updateEnabled.ts (1)
53-54: Consider using consistent workspace ID source.While the audit log bucket ID is correctly taken from
ctx.workspace, theworkspaceIdis still taken fromkey.workspace.id. For consistency and to prevent potential issues, consider usingctx.workspace.idfor both.await insertAuditLogs(tx, ctx.workspace.auditLogBucket.id, { - workspaceId: key.workspace.id, + workspaceId: ctx.workspace.id, actor: {apps/dashboard/lib/trpc/routers/key/updateOwnerId.ts (1)
Line range hint
61-61: Consider sanitizing ownerId in audit log message.The audit log message includes the raw
ownerIdvalue. Consider handling null case and potentially masking or formatting the ID for better readability.- description: `Changed ownerId of ${key.id} to ${input.ownerId}`, + description: `Changed ownerId of ${key.id} to ${input.ownerId ?? 'null'}`,apps/dashboard/lib/trpc/routers/ratelimit/deleteOverride.ts (2)
Line range hint
59-85: Consider consolidating error handling.The error handling for audit logs could be moved to the transaction catch block to maintain consistency and reduce code duplication.
await insertAuditLogs(tx, ctx.workspace.auditLogBucket.id, { workspaceId: ctx.workspace.id, // ... audit log details ... - }).catch((_err) => { - throw new TRPCError({ - message: - "We are unable to delete the override. Please try again or contact support@unkey.dev", - code: "INTERNAL_SERVER_ERROR", - }); - }); + }); });
Line range hint
1-1: Overall PR AssessmentThe changes effectively address the transaction deadlock issues by standardizing workspace and audit log bucket handling. However, there are a few items that need attention:
- The
delete.tsfile still performs a separate workspace fetch that could cause deadlocks- There's a syntax error in the audit log description in
updateDeleteProtection.ts- Some files have redundant error handling that could be consolidated
The overall approach of using
ctx.workspaceis solid and should prevent the deadlock issues once these items are addressed.apps/dashboard/lib/trpc/routers/api/setDefaultBytes.ts (1)
24-28: Consider reordering WHERE conditions for optimal index usage.The query's WHERE conditions should be ordered with the most selective condition first. Consider moving
eq(table.id, input.keyAuthId)before the workspace check since IDs are typically more selective.and( - eq(table.workspaceId, ctx.workspace.id), - eq(table.id, input.keyAuthId), + eq(table.id, input.keyAuthId), + eq(table.workspaceId, ctx.workspace.id), isNull(table.deletedAt), ),apps/dashboard/lib/trpc/routers/key/updateMetadata.ts (1)
32-36: Consider reordering WHERE conditions for optimal index usage.Similar to other routers, consider moving the ID check before the workspace check for better index utilization.
and( - eq(table.workspaceId, ctx.workspace.id), - eq(table.id, input.keyId), + eq(table.id, input.keyId), + eq(table.workspaceId, ctx.workspace.id), isNull(table.deletedAt), ),apps/dashboard/lib/trpc/routers/key/updateRemaining.ts (1)
31-35: Consider reordering WHERE conditions for optimal index usage.For consistency with other routers and optimal index usage, consider moving the ID check before the workspace check.
and( - eq(table.workspaceId, ctx.workspace.id), - eq(table.id, input.keyId), + eq(table.id, input.keyId), + eq(table.workspaceId, ctx.workspace.id), isNull(table.deletedAt), ),apps/dashboard/lib/trpc/routers/ratelimit/createOverride.ts (2)
23-27: Improve error message specificity.While the query changes look good, the error message could be more specific about the namespace not being found in the current workspace.
- "We are unable to find the correct namespace. Please try again or contact support@unkey.dev.", + "The namespace was not found in your workspace. Please verify the namespace ID and try again.",Also applies to: 36-42
58-59: Extract the default rate limit override value.Consider extracting the magic number
5into a named constant for better maintainability.+const DEFAULT_MAX_RATELIMIT_OVERRIDES = 5; + const max = typeof ctx.workspace.features.ratelimitOverrides === "number" ? ctx.workspace.features.ratelimitOverrides - : 5; + : DEFAULT_MAX_RATELIMIT_OVERRIDES;apps/dashboard/lib/trpc/routers/key/create.ts (1)
41-42: Improve error message clarity.The error message could be more specific about the key auth not being found in the current workspace.
- "We are unable to find the correct keyAuth. Please try again or contact support@unkey.dev", + "The key auth was not found in your workspace. Please verify the keyAuth ID and try again.",Also applies to: 54-60
apps/dashboard/lib/trpc/routers/key/updateRatelimit.ts (1)
101-102: Consider consolidating error handling.The error handling logic is duplicated between the enable and disable paths. Consider extracting it into a reusable function.
+const handleRatelimitError = (err: unknown) => { + console.error(err); + throw new TRPCError({ + code: "INTERNAL_SERVER_ERROR", + message: + "We were unable to update ratelimit on this key. Please try again or contact support@unkey.dev", + }); +}; // In the enable path -}).catch((_err) => { - throw new TRPCError({ - code: "INTERNAL_SERVER_ERROR", - message: - "We were unable to update ratelimit on this key. Please try again or contact support@unkey.dev", - }); -}); +}).catch(handleRatelimitError); // In the disable path -}).catch((err) => { - console.error(err); - throw new TRPCError({ - code: "INTERNAL_SERVER_ERROR", - message: - "We were unable to update ratelimit on this key. Please try again or contact support@unkey.dev", - }); -}); +}).catch(handleRatelimitError);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (58)
apps/dashboard/app/(app)/settings/billing/stripe/success/page.tsx(2 hunks)apps/dashboard/app/new/create-ratelimit.tsx(1 hunks)apps/dashboard/app/new/page.tsx(1 hunks)apps/dashboard/lib/audit.ts(1 hunks)apps/dashboard/lib/trpc/context.ts(1 hunks)apps/dashboard/lib/trpc/routers/api/create.ts(3 hunks)apps/dashboard/lib/trpc/routers/api/delete.ts(4 hunks)apps/dashboard/lib/trpc/routers/api/setDefaultBytes.ts(3 hunks)apps/dashboard/lib/trpc/routers/api/setDefaultPrefix.ts(3 hunks)apps/dashboard/lib/trpc/routers/api/updateDeleteProtection.ts(3 hunks)apps/dashboard/lib/trpc/routers/api/updateIpWhitelist.ts(3 hunks)apps/dashboard/lib/trpc/routers/api/updateName.ts(3 hunks)apps/dashboard/lib/trpc/routers/audit/fetch.ts(1 hunks)apps/dashboard/lib/trpc/routers/index.ts(0 hunks)apps/dashboard/lib/trpc/routers/key/create.ts(4 hunks)apps/dashboard/lib/trpc/routers/key/createRootKey.ts(6 hunks)apps/dashboard/lib/trpc/routers/key/delete.ts(2 hunks)apps/dashboard/lib/trpc/routers/key/deleteRootKey.ts(2 hunks)apps/dashboard/lib/trpc/routers/key/updateEnabled.ts(1 hunks)apps/dashboard/lib/trpc/routers/key/updateExpiration.ts(3 hunks)apps/dashboard/lib/trpc/routers/key/updateMetadata.ts(3 hunks)apps/dashboard/lib/trpc/routers/key/updateName.ts(3 hunks)apps/dashboard/lib/trpc/routers/key/updateOwnerId.ts(3 hunks)apps/dashboard/lib/trpc/routers/key/updateRatelimit.ts(4 hunks)apps/dashboard/lib/trpc/routers/key/updateRemaining.ts(2 hunks)apps/dashboard/lib/trpc/routers/key/updateRootKeyName.ts(2 hunks)apps/dashboard/lib/trpc/routers/llmGateway/create.ts(0 hunks)apps/dashboard/lib/trpc/routers/llmGateway/delete.ts(0 hunks)apps/dashboard/lib/trpc/routers/logs/llm-search.ts(2 hunks)apps/dashboard/lib/trpc/routers/logs/query-log.ts(1 hunks)apps/dashboard/lib/trpc/routers/logs/query-timeseries.ts(1 hunks)apps/dashboard/lib/trpc/routers/ratelimit/createNamespace.ts(1 hunks)apps/dashboard/lib/trpc/routers/ratelimit/createOverride.ts(5 hunks)apps/dashboard/lib/trpc/routers/ratelimit/deleteNamespace.ts(4 hunks)apps/dashboard/lib/trpc/routers/ratelimit/deleteOverride.ts(3 hunks)apps/dashboard/lib/trpc/routers/ratelimit/updateNamespaceName.ts(3 hunks)apps/dashboard/lib/trpc/routers/ratelimit/updateOverride.ts(3 hunks)apps/dashboard/lib/trpc/routers/rbac.ts(12 hunks)apps/dashboard/lib/trpc/routers/rbac/addPermissionToRootKey.ts(2 hunks)apps/dashboard/lib/trpc/routers/rbac/connectPermissionToRole.ts(1 hunks)apps/dashboard/lib/trpc/routers/rbac/connectRoleToKey.ts(1 hunks)apps/dashboard/lib/trpc/routers/rbac/createPermission.ts(1 hunks)apps/dashboard/lib/trpc/routers/rbac/createRole.ts(3 hunks)apps/dashboard/lib/trpc/routers/rbac/deletePermission.ts(2 hunks)apps/dashboard/lib/trpc/routers/rbac/deleteRole.ts(1 hunks)apps/dashboard/lib/trpc/routers/rbac/disconnectPermissionFromRole.ts(1 hunks)apps/dashboard/lib/trpc/routers/rbac/disconnectRoleFromKey.ts(1 hunks)apps/dashboard/lib/trpc/routers/rbac/removePermissionFromRootKey.ts(2 hunks)apps/dashboard/lib/trpc/routers/rbac/updatePermission.ts(2 hunks)apps/dashboard/lib/trpc/routers/rbac/updateRole.ts(2 hunks)apps/dashboard/lib/trpc/routers/rbac/upsertPermission.ts(3 hunks)apps/dashboard/lib/trpc/routers/vercel.ts(10 hunks)apps/dashboard/lib/trpc/routers/workspace/changeName.ts(1 hunks)apps/dashboard/lib/trpc/routers/workspace/changePlan.ts(6 hunks)apps/dashboard/lib/trpc/routers/workspace/create.ts(1 hunks)apps/dashboard/lib/trpc/routers/workspace/optIntoBeta.ts(2 hunks)apps/dashboard/lib/trpc/trpc.ts(1 hunks)internal/db/src/types.ts(1 hunks)
💤 Files with no reviewable changes (3)
- apps/dashboard/lib/trpc/routers/llmGateway/create.ts
- apps/dashboard/lib/trpc/routers/llmGateway/delete.ts
- apps/dashboard/lib/trpc/routers/index.ts
🧰 Additional context used
📓 Learnings (1)
apps/dashboard/lib/trpc/routers/api/setDefaultPrefix.ts (1)
Learnt from: chronark
PR: unkeyed/unkey#2146
File: apps/dashboard/lib/trpc/routers/api/setDefaultPrefix.ts:80-80
Timestamp: 2024-11-10T16:45:16.994Z
Learning: Ensure that audit log descriptions accurately reflect the action being performed, such as updating the `defaultPrefix`, and avoid incorrect references like 'name' when not applicable.
🔇 Additional comments (63)
apps/dashboard/lib/trpc/routers/rbac/connectPermissionToRole.ts (1)
Line range hint
49-78: Consider using a unique constraint for role-permission pairs.The current implementation uses
onDuplicateKeyUpdate, but it's not clear if there's a unique constraint ensuring role-permission pairs are unique within a workspace.Run this script to verify the table structure:
✅ Verification successful
Unique constraints are properly implemented ✅
The schema already has two levels of constraints ensuring uniqueness:
- A primary key on
(roleId, permissionId, workspaceId)preventing duplicates within a workspace- A unique constraint on
(permissionId, roleId)preventing duplicates globallyThe current implementation using
onDuplicateKeyUpdateis appropriate and works correctly with these constraints.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there's a unique constraint on the roles_permissions table ast-grep --pattern 'createTable("roles_permissions", (table) => { $$$ table.unique($$$) $$$ })'Length of output: 103
Script:
#!/bin/bash # Search for files containing roles_permissions table definition rg -l "roles_permissions" --type sql --type typescript # Then search for content in those files rg "roles_permissions" -A 10 -B 10 --type sql --type typescriptLength of output: 188
Script:
#!/bin/bash # Find TypeScript files containing roles_permissions fd -e ts | xargs rg "roles_permissions" -A 10 -B 10 # Specifically look for schema definitions fd -e ts | xargs rg "schema.*roles_permissions|createTable.*roles_permissions" -A 10 -B 10Length of output: 5183
apps/dashboard/lib/trpc/routers/ratelimit/deleteNamespace.ts (2)
18-22: LGTM! Query conditions are well structured.The workspace check is correctly moved to the query level, which helps prevent potential race conditions and aligns with the PR's objective of reducing deadlocks.
Line range hint
1-116: Verify transaction deadlock resolution.The changes effectively address the transaction deadlock issue by consistently using workspace context and reducing duplicate workspace fetches. However, let's verify that this is the only procedure accessing these tables to ensure we've caught all potential deadlock scenarios.
✅ Verification successful
Transaction patterns are properly implemented to prevent deadlocks
The codebase follows consistent transaction patterns across all ratelimit operations:
- Pre-transaction validations with workspace context
- Atomic operations within transactions
- Proper cascading updates (e.g., namespace deletion updating related overrides)
- No circular dependencies or nested transactions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other procedures that might access ratelimitNamespaces or ratelimitOverrides tables echo "Searching for other procedures accessing the same tables..." rg -l "ratelimitNamespaces|ratelimitOverrides" --type ts # Search for potential transaction blocks echo "Searching for transaction blocks in related files..." ast-grep --pattern 'db.transaction'Length of output: 11777
Script:
#!/bin/bash # Examine transaction patterns in ratelimit-related files echo "Examining ratelimit router transaction patterns..." for file in $(find apps/dashboard/lib/trpc/routers/ratelimit -type f -name "*.ts"); do echo "=== $file ===" rg "transaction|findFirst|findMany|update|insert|delete" "$file" -A 2 -B 2 doneLength of output: 11650
apps/dashboard/lib/trpc/routers/rbac/removePermissionFromRootKey.ts (2)
20-20: LGTM! Good optimization for deadlock prevention.Using
ctx.workspace.iddirectly instead of fetching the workspace separately is a good optimization. This aligns with the PR's objective of preventing deadlocks by moving workspace fetching to the beginning of the tRPC handler.
Line range hint
18-85: Verify transaction isolation level for deadlock prevention.The transaction structure looks good with proper atomicity and error handling. To further prevent deadlocks, verify that the database is configured with an appropriate isolation level (e.g., READ COMMITTED) for these workspace-related transactions.
Run this script to check the current transaction isolation level:
✅ Verification successful
Transaction implementation is safe with default isolation level
The current implementation is safe as it leverages PlanetScale's default REPEATABLE READ isolation level, which prevents dirty reads and non-repeatable reads. The transaction's structure follows best practices with proper error handling and atomic operations. No additional isolation configuration is needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check transaction isolation configuration in database setup # Search for isolation level configuration rg -i "isolation.*level|BEGIN\s+TRANSACTION" --type ts # Look for any custom transaction settings ast-grep --pattern 'transaction($$$)'Length of output: 8341
Script:
#!/bin/bash # Search for database configuration and transaction settings rg -i "drizzle.*config|createPool|connection.*options" --type ts # Look for any custom transaction configurations in Drizzle setup ast-grep --pattern 'drizzle($$$)'Length of output: 18013
Script:
#!/bin/bash # Check for any transaction-related configurations in the specific file rg -A 5 -B 5 "transaction" "apps/dashboard/lib/trpc/routers/rbac/removePermissionFromRootKey.ts" # Look for any transaction-related imports ast-grep --pattern 'import { $$$transaction$$$ } from $path'Length of output: 524
apps/dashboard/lib/trpc/context.ts (3)
5-6: No immediate concerns about these new imports.
11-13: Initialization of a workspace variable in the TRPC context is a good design choice.
55-55: Exposing the workspace via the returned context looks appropriate.apps/dashboard/lib/trpc/routers/ratelimit/updateNamespaceName.ts (2)
17-24: Directly fetching the namespace by ID is more efficient than going through the workspace relationship first.
57-58: Usingctx.workspace.auditLogBucket.idfor audit logs is consistent with the new workspace context approach.apps/dashboard/lib/trpc/routers/workspace/changePlan.ts (3)
33-35: Plan-change frequency check looks good but verify monthly logic.Please ensure this date-based comparison aligns with business rules about plan-change intervals. If you really want once-per-calendar-month logic, confirm that comparing just the year and month is sufficient.
44-45: Skipping further updates when the plan is already the same may cause corner cases.If a user tries to resubscribe to the same plan, the conditional logic must handle it carefully, as partly done with
planDowngradeRequest. Make sure all use-cases (e.g., canceled vs. active, reactivation, etc.) are covered.
56-57: Consistent usage ofctx.workspace.auditLogBucket.idhelps avoid mismatched log buckets.Also applies to: 100-101, 150-151
apps/dashboard/lib/trpc/trpc.ts (2)
12-14: LGTM! Early workspace validation helps prevent deadlocks.Moving the workspace validation to the auth middleware is a good approach as it ensures workspace existence is verified before any transaction begins, helping prevent potential deadlocks.
18-21: LGTM! Consistent context propagation.Properly passing the validated workspace through the context ensures consistent access throughout the request lifecycle.
apps/dashboard/lib/trpc/routers/logs/query-log.ts (1)
11-11: LGTM! Consistent use of centralized workspace.Correctly utilizing workspace ID from the context, benefiting from the early validation in the auth middleware.
internal/db/src/types.ts (1)
24-24: LGTM! Type safety for audit log buckets.Adding the AuditLogBucket type ensures type safety when working with audit log transactions, reducing potential runtime errors.
apps/dashboard/lib/trpc/routers/rbac/createPermission.ts (1)
31-31: LGTM! Consistent implementation of workspace context usage.The changes follow the same pattern of using
ctx.workspaceto prevent transaction deadlocks. The transaction ordering and error handling remain robust.Also applies to: 33-34
apps/dashboard/lib/trpc/routers/key/deleteRootKey.ts (1)
19-19: LGTM! Verify the workspace filtering logic.The changes correctly use
ctx.workspaceto prevent deadlocks. The implementation properly handles batch deletion with appropriate audit logging.Let's verify the workspace filtering logic is consistent:
Also applies to: 40-40, 42-42
✅ Verification successful
Workspace filtering implementation is correct ✅
The code properly implements dual workspace filtering:
- Uses
env().UNKEY_WORKSPACE_IDto identify internal workspace ownership- Uses
ctx.workspace.idto filter keys for the specific user workspace🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify workspace filtering in key operations ast-grep --pattern 'eq($_, env().UNKEY_WORKSPACE_ID)' -l typescriptLength of output: 184
Script:
#!/bin/bash # Check for workspace filtering patterns echo "=== Checking for workspace filtering with ctx.workspace.id ===" ast-grep --pattern 'eq($_, ctx.workspace.id)' echo -e "\n=== Checking for UNKEY_WORKSPACE_ID usage ===" rg "UNKEY_WORKSPACE_ID" -A 2 -B 2Length of output: 10360
apps/dashboard/lib/trpc/routers/ratelimit/createNamespace.ts (1)
23-23: LGTM! Well-structured transaction with proper error handling.The changes maintain the pattern of using
ctx.workspaceto prevent deadlocks while preserving the specific error handling for duplicate namespace entries.Also applies to: 27-28
apps/dashboard/lib/trpc/routers/key/updateRootKeyName.ts (1)
17-21: LGTM! Well-structured query with proper workspace scoping.The key query is properly scoped to the workspace and includes all necessary conditions (workspaceId, keyId, and deletedAt check).
apps/dashboard/lib/trpc/routers/api/create.ts (2)
15-16: LGTM: Clear validation messages.The validation messages for API names are now more specific and accurate.
24-24: Improved transaction handling by removing redundant workspace fetch.The changes effectively address the transaction deadlock issue by:
- Using
ctx.workspace.iddirectly instead of fetching workspace separately- Reusing the same workspace ID throughout the transaction
- Correctly passing the audit log bucket ID from the context
This change reduces the likelihood of deadlocks by eliminating the race condition between workspace fetch and transaction start.
Also applies to: 43-43, 57-58
apps/dashboard/lib/trpc/routers/key/updateName.ts (1)
18-22: Well-structured changes that improve security and consistency.The changes effectively:
- Add workspace validation at query level
- Simplify error handling
- Use consistent workspace ID source throughout
- Properly scope the transaction
This implementation aligns well with the PR's objective of reducing deadlocks while also improving security.
Also applies to: 31-37, 53-54
apps/dashboard/lib/trpc/routers/key/updateOwnerId.ts (1)
18-22: LGTM: Consistent implementation of workspace validation.The changes follow the same secure pattern as other files, properly validating workspace ownership at the query level and simplifying error handling.
Also applies to: 31-37
apps/dashboard/lib/trpc/routers/api/updateName.ts (2)
22-26: LGTM! Enhanced security with workspace validation.The addition of workspace ID validation in the query ensures proper authorization and prevents cross-workspace access.
57-58: Verify transaction ordering with audit logs.The change to use
ctx.workspace.auditLogBucket.idhelps prevent deadlocks by avoiding nested workspace fetches. However, let's verify there are no other transaction orderings that could still cause deadlocks.✅ Verification successful
Transaction ordering with audit logs is consistent and safe ✅
The codebase consistently performs the main operation first, followed by audit log insertion, with pre-fetched workspace data. This pattern prevents deadlocks and maintains reliable transaction ordering.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other potential transaction patterns that might cause deadlocks rg -A 5 "transaction.*async.*tx" | rg -B 5 "insertAuditLogs"Length of output: 2643
apps/dashboard/lib/trpc/routers/api/updateDeleteProtection.ts (1)
21-25: LGTM! Consistent workspace validation pattern.The workspace validation follows the same pattern as other files, ensuring proper authorization.
apps/dashboard/lib/trpc/routers/ratelimit/deleteOverride.ts (1)
17-22: LGTM! Proper workspace validation.The workspace validation ensures proper authorization and follows the consistent pattern across the codebase.
apps/dashboard/lib/trpc/routers/api/setDefaultBytes.ts (1)
59-60: LGTM! Good use of workspace context for audit logging.Using
ctx.workspace.auditLogBucket.idfrom the context helps prevent transaction deadlocks by ensuring consistent bucket ID access.apps/dashboard/lib/trpc/routers/key/updateMetadata.ts (1)
67-68: LGTM! Consistent audit log bucket handling.The audit log changes align with the deadlock prevention strategy by using the workspace context.
apps/dashboard/lib/trpc/routers/rbac/addPermissionToRootKey.ts (2)
65-68: LGTM! Good handling of multiple audit logs.The spread operator correctly combines existing audit logs with the new one, maintaining the transaction atomicity.
27-27: Verify the use of forWorkspaceId vs workspaceId.The query uses
forWorkspaceIdwhile other routers useworkspaceId. Please verify this is intentional and not a potential bug.✅ Verification successful
The use of
forWorkspaceIdis correct and intentionalThe field distinction between
workspaceIdandforWorkspaceIdfollows an established pattern in the codebase where:
workspaceIdindicates the workspace that owns/created a resourceforWorkspaceIdis specifically used for keys that are created by one workspace (usually the Unkey workspace) for use by another workspace🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check schema to verify workspace ID field naming rg -A 5 "forWorkspaceId|workspaceId" --type tsLength of output: 71100
apps/dashboard/lib/trpc/routers/key/updateRemaining.ts (1)
60-61: LGTM! Consistent audit log handling with proper error logging.Good use of workspace context for audit logging. The error handling with console.error helps with debugging while maintaining user-friendly error messages.
apps/dashboard/lib/trpc/routers/key/updateExpiration.ts (2)
39-43: Security improvement: Added workspace validation in key query.The addition of
eq(table.workspaceId, ctx.workspace.id)ensures that keys can only be accessed within their own workspace context, preventing potential cross-workspace access.
74-75: Consistent with deadlock prevention: Using workspace context for audit logs.Using
ctx.workspace.auditLogBucket.idfrom the context aligns with the PR's goal of preventing transaction deadlocks by avoiding repeated workspace fetches.apps/dashboard/lib/trpc/routers/rbac/createRole.ts (1)
34-34: Consistent workspace context usage across role creation flow.The changes consistently use
ctx.workspace.idandctx.workspace.auditLogBucket.idthroughout the role creation process, which:
- Reduces database queries by using pre-loaded workspace context
- Maintains consistency in audit logging
- Helps prevent transaction deadlocks as intended
Also applies to: 43-44, 69-69, 74-76
apps/dashboard/lib/trpc/routers/rbac/connectRoleToKey.ts (1)
79-79: LGTM! Audit logging updated correctly.The audit log insertion has been properly updated to use the workspace's audit log bucket ID from the context, which aligns with the PR's objective of reducing transaction deadlocks.
apps/dashboard/lib/trpc/routers/ratelimit/updateOverride.ts (2)
68-69: LGTM! Audit logging updated correctly.The audit log insertion has been properly updated to use the workspace context.
20-25:⚠️ Potential issueRemove duplicate workspace ID check.
The where clause contains a duplicate condition checking the workspace ID.
where: (table, { and, eq, isNull }) => and( - eq(table.workspaceId, ctx.workspace.id), eq(table.workspaceId, ctx.workspace.id), eq(table.id, input.id), isNull(table.deletedAt), ),Likely invalid or redundant comment.
apps/dashboard/lib/trpc/routers/api/updateIpWhitelist.ts (1)
84-85: LGTM! Audit logging updated correctly.The audit log insertion has been properly updated to use the workspace context.
apps/dashboard/lib/audit.ts (1)
96-96: LGTM! Function signature updated correctly.The addition of the
bucketIdparameter aligns with the PR's objective of reducing transaction deadlocks by moving workspace and bucket fetching to the beginning of the trpc handler.Let's verify that all callers have been updated to provide the bucketId parameter:
✅ Verification successful
All callers correctly provide the bucketId parameter
The codebase scan shows that all 65 calls to
insertAuditLogshave been properly updated to include the bucketId parameter, consistently using eitherctx.workspace.auditLogBucket.idor an explicitbucketId.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all insertAuditLogs calls to ensure they provide the bucketId rg "insertAuditLogs\(" -A 3Length of output: 28007
apps/dashboard/lib/trpc/routers/api/delete.ts (3)
18-22: LGTM! Query changes effectively prevent race conditions.The workspace validation is now properly handled at the query level, which helps prevent potential race conditions and reduces the risk of deadlocks.
Also applies to: 31-35
50-50: LGTM! Consistent audit log bucket usage.Using
ctx.workspace.auditLogBucket.idensures the same bucket ID is used throughout the transaction, reducing the risk of deadlocks.
82-84: LGTM! Consistent workspace context usage.Using
ctx.workspacefor both bucket ID and workspaceId maintains consistency and atomicity throughout the transaction.apps/dashboard/lib/trpc/routers/ratelimit/createOverride.ts (1)
69-69: LGTM! Consistent workspace context usage in transaction.The changes maintain consistency with the workspace context throughout the transaction, preventing potential deadlocks.
Also applies to: 78-79
apps/dashboard/lib/trpc/routers/key/create.ts (1)
77-77: LGTM! Consistent workspace context usage.The changes maintain consistency with the workspace context throughout the key creation process.
Also applies to: 93-94
apps/dashboard/lib/trpc/routers/key/updateRatelimit.ts (2)
64-65: LGTM! Consistent workspace context usage for enabling rate limits.The changes maintain consistency with the workspace context when enabling rate limits.
22-26: Consider validating workspaceId matches context.The input schema includes
workspaceId, but it's not validated againstctx.workspace.id. Consider either removing it from the input if unused or validating it matches the context.Also applies to: 35-41
apps/dashboard/app/(app)/settings/billing/stripe/success/page.tsx (1)
32-36: Add validation for audit log bucket existence.The code assumes that
auditLogBuckets[0]exists without validation. This could lead to runtime errors if no audit log bucket is found.Add a check before using the bucket ID:
with: { auditLogBuckets: { where: (table, { eq }) => eq(table.name, "unkey_mutations"), }, }, +if (!ws?.auditLogBuckets?.[0]) { + throw new Error("Audit log bucket not found"); +}apps/dashboard/lib/trpc/routers/logs/llm-search.ts (2)
84-86: LGTM! Simplified mutation logic.The workspace validation has been moved to middleware, making the code more concise and maintainable.
112-117: LGTM! Improved formatting.The example output formatting has been improved for better readability.
apps/dashboard/lib/trpc/routers/audit/fetch.ts (1)
41-41: LGTM! Using workspace from context.The change aligns with the PR objective of moving workspace fetch to the beginning of the trpc handler, which helps prevent transaction deadlocks.
apps/dashboard/lib/trpc/routers/key/createRootKey.ts (1)
27-28: LGTM! Consistent usage of workspace from context.The changes consistently use
ctx.workspace.idthroughout the file, which aligns with the PR objective of moving workspace fetch to the beginning of the trpc handler.Also applies to: 68-68, 80-80, 110-110, 114-114, 141-141
apps/dashboard/app/new/page.tsx (2)
213-221: LGTM: Audit log bucket creation looks good.The implementation correctly creates a new audit log bucket with appropriate properties:
- Unique ID generation using
newId- Proper workspace association
- Sensible defaults for retention (30 days) and delete protection
223-223: Verify audit log bucket ID usage.The audit log bucket ID is now required as a parameter for
insertAuditLogs.Run the following script to verify consistent usage of the bucket ID parameter:
✅ Verification successful
✓ Audit log bucket ID usage is consistent
All calls to
insertAuditLogscorrectly include the bucket ID as the second parameter, following the required pattern.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all calls to insertAuditLogs include a bucket ID parameter. # Test: Search for insertAuditLogs calls. Expect: All calls should have 3 parameters. rg -p "insertAuditLogs\((.*?)\)" --multilineLength of output: 305
apps/dashboard/lib/trpc/routers/vercel.ts (1)
76-78: LGTM: Consistent audit log bucket usage across all mutations.The changes correctly use
ctx.workspace.auditLogBucket.idin all audit log insertions, maintaining consistency across different operations:
- Key creation
- Vercel binding operations
- Permission updates
Also applies to: 126-126, 177-177, 257-257, 297-297, 377-377, 422-422, 463-463, 530-530, 587-587
apps/dashboard/lib/trpc/routers/rbac.ts (1)
78-78: LGTM: Consistent audit log bucket usage in RBAC operations.The changes correctly use
ctx.workspace.auditLogBucket.idin all RBAC-related audit log insertions:
- Role operations (create, update, delete)
- Permission operations (create, update, delete)
- Role-Permission connections
- Role-Key connections
Also applies to: 138-138, 213-213, 263-263, 338-338, 416-416, 447-447, 507-507, 559-559, 603-603, 667-667, 727-727
apps/dashboard/lib/trpc/routers/logs/query-timeseries.ts (1)
19-19: LGTM: Simplified workspace access.The change correctly uses
ctx.workspace.idfrom the context instead of performing a separate database query, which:
- Reduces database load
- Prevents potential transaction deadlocks
- Aligns with the PR's objective
apps/dashboard/lib/trpc/routers/api/setDefaultPrefix.ts (4)
20-24: LGTM! Well-structured query conditions.The query conditions are comprehensive and properly validate workspace ownership while checking for soft-deleted records.
Line range hint
33-39: LGTM! Clear and concise error handling.The error handling is appropriately simplified since workspace ownership is already validated in the query conditions.
Line range hint
56-77: LGTM! Comprehensive audit logging with accurate description.The audit logging implementation is thorough and follows best practices:
- Uses the workspace's audit log bucket from context
- Includes detailed metadata
- Accurately describes the prefix change with before/after values
56-57: Consider caching the workspace lookup.As mentioned in the PR objectives, there might be potential for caching the workspace lookup. This could further reduce database load and potential deadlocks.
Let's check if Next.js is already handling the caching:
…into trpc-load-workspace
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Fixes transaction deadlocks around audit log buckets.
This moves the workspace fetch to the beginning of the trpc handler and
loads the bucket id once, to be reused.
In theory we can now cache the workspace lookup but I'm not sure if
Next.js does that automatically now or if we need to do it.
Summary by CodeRabbit
New Features
Removed Features
Improvements