chore: Make workspace slug not null after migrate#3916
Conversation
MichaelUnkey
requested review from
Flo4604,
chronark,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
|
|
Warning Rate limit exceeded@chronark has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 13 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 selected for processing (1)
📝 WalkthroughWalkthroughMake Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Dashboard
participant DB
User->>Dashboard: GET /gateway-new
Dashboard->>DB: Find workspace by org
alt no workspace found
Dashboard->>DB: Insert workspace { name: "Personal Workspace", slug: "personal-workspace-<rand>" }
Dashboard->>DB: Insert quotas for workspace
Dashboard-->>User: Redirect to new workspace page
else workspace exists
Dashboard-->>User: Redirect to existing workspace page
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (1 passed, 2 warnings)❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/dashboard/app/(app)/gateway-new/page.tsx (1)
27-42: Race on upsert + fragile slug entropy; handle duplicates and widen suffix space
- TOCTOU: Two concurrent requests can both see no workspace and then one insert fails on orgId (UNIQUE) or slug (UNIQUE). Wrap insert in try/catch and refetch, or use an idempotent upsert.
- Collisions: randomInt(100000) gives only 1e5 possibilities; at scale this risks hitting UNIQUE(slug). Use a wider suffix (e.g., fixed-length base36) and/or retry on duplicate.
Apply this minimal, defensive change:
@@ if (!ws) { const id = newId("workspace"); - await db.insert(schema.workspaces).values({ - id, - name: "Personal Workspace", - slug: `personal-workspace-${randomInt(100000)}`, - orgId, - betaFeatures: {}, - features: {}, - }); + // 8-char base36 suffix (~2.8e12 space), total slug length << 64 + const suffix = randomInt(36 ** 8).toString(36).padStart(8, "0"); + const slug = `personal-workspace-${suffix}`; + try { + await db.insert(schema.workspaces).values({ + id, + name: "Personal Workspace", + slug, + orgId, + betaFeatures: {}, + features: {}, + }); + } catch (e: any) { + // Ignore duplicate errors (orgId/slug) from a concurrent creator. + if (e?.code !== "ER_DUP_ENTRY" && e?.errno !== 1062) throw e; + } - await db.insert(schema.quotas).values({ - workspaceId: id, - ...freeTierQuotas, - }); + // Ensure quotas exist; refetch actual workspace id if our insert lost the race. + const current = await db.query.workspaces.findFirst({ + where: (table, { eq, isNull, and }) => and(eq(table.orgId, orgId), isNull(table.deletedAtM)), + }); + if (current) { + try { + await db.insert(schema.quotas).values({ + workspaceId: current.id, + ...freeTierQuotas, + }); + } catch (e: any) { + // Ignore duplicate if quotas already created in the competing request. + if (e?.code !== "ER_DUP_ENTRY" && e?.errno !== 1062) throw e; + } + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/api/src/routes/v1_keys_getVerifications.security.test.ts(1 hunks)apps/dashboard/app/(app)/gateway-new/page.tsx(2 hunks)internal/db/src/schema/workspaces.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Packages / Test
- GitHub Check: Build / Build
🔇 Additional comments (2)
apps/dashboard/app/(app)/gateway-new/page.tsx (1)
11-11: Import is fine for server runtimerandomInt from node:crypto is OK in this server-only route. No issues.
apps/api/src/routes/v1_keys_getVerifications.security.test.ts (1)
78-90: Cap test-generated slug to schema limit
- In
apps/api/src/routes/v1_keys_getVerifications.security.test.ts, append.slice(0, 64)after the regex chain to match the workspace.slug (varchar(64)) limit and avoid brittle failures:- .replace(/^-|-$/g, ""), + .replace(/^-|-$/g, "") + .slice(0, 64),
- Optionally extract a shared
slugifyutil (e.g.,utils/slug.ts) for reuse in tests and client code to keep slug logic consistent.- Verify the
workspace.slugcolumn is defined as varchar(64) in your DB migrations/schema.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/dashboard/app/(app)/gateway-new/page.tsx (1)
27-42: Harden the “upsert” against races and make it idempotent.Current check-then-insert can race on concurrent gateway.new hits (unique orgId/slug). Either use DB upsert or catch-and-ignore duplicate key.
Minimal, driver-agnostic approach:
- await db.insert(schema.workspaces).values({ + try { + await db.insert(schema.workspaces).values({ id, name: "Personal Workspace", slug: `personal-workspace-${randomUUID().slice(0, 8)}`, orgId, betaFeatures: {}, features: {}, - }); + }); + } catch (e: any) { + // MySQL: 1062 ER_DUP_ENTRY; ignore races on orgId/slug + if (e?.code !== "ER_DUP_ENTRY" && e?.errno !== 1062) throw e; + } @@ - await db.insert(schema.quotas).values({ + try { + await db.insert(schema.quotas).values({ workspaceId: id, ...freeTierQuotas, - }); + }); + } catch (e: any) { + if (e?.code !== "ER_DUP_ENTRY" && e?.errno !== 1062) throw e; + }
♻️ Duplicate comments (1)
go/pkg/db/schema.sql (1)
185-185: Migration safety: backfill before enforcing NOT NULL.Ensure a two-step rollout:
- Backfill all NULL slugs with a de-duped, <=64 value (in batches to avoid locks).
- Then ALTER COLUMN to NOT NULL (online DDL) and keep UNIQUE(slug).
If any seed/test writers still omit slug, the migration will fail. Refer to the earlier verification agent’s repository-wide scan; rerun it after the latest changes.
I can open a thorough tracking issue (Summary/Impact/Repro/AC/Validation/References) per imeyer’s preference—say the word and I’ll draft it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
apps/api/src/routes/v1_keys_getVerifications.security.test.ts(1 hunks)apps/dashboard/app/(app)/gateway-new/page.tsx(2 hunks)go/pkg/db/schema.sql(1 hunks)internal/db/src/schema/workspaces.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: imeyer
PR: unkeyed/unkey#3755
File: .github/workflows/runbook-freshness-check.yaml:157-173
Timestamp: 2025-08-08T14:59:52.283Z
Learning: Repo unkeyed/unkey: When a CI/workflow fix is deferred, imeyer prefers a thorough GitHub issue be opened with sections (Summary, Impact, Where, Repro, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References) and assigned to imeyer, including backlinks to the originating PR and comment.
Learnt from: imeyer
PR: unkeyed/unkey#3755
File: .github/workflows/job_detect_changes.yaml:111-114
Timestamp: 2025-08-08T16:10:00.224Z
Learning: Repo: unkeyed/unkey — When imeyer writes “make me an issue” on a PR thread, automatically open a thorough GitHub issue (Summary, Impact, Where, Repro, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References), include backlinks to the PR and comment, and assign to imeyer.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test API / API Test Local
🔇 Additional comments (1)
apps/api/src/routes/v1_keys_getVerifications.security.test.ts (1)
78-84: LGTM: test aligned to NOT NULL slug.Using a UUID for both name and slug satisfies length and uniqueness constraints for the fixture.
|
Tests are currently failing and the changes also have to be done for the go schema and any place where the go code inserts workspaces |
|
yeah fixed it already |
|
👌 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/resend/emails/api_v1_migration.tsx (2)
26-28: Fix Tailwind class typo (“text-semibold” → “font-semibold”).Ensures the heading actually renders semibold.
- <Heading className="font-sans text-3xl text-semibold text-center"> + <Heading className="font-sans text-3xl font-semibold text-center">
37-39: Avoid hard-coding the EOL date; centralize/configure it.Move the date to a shared constant or env so updates don’t require code edits.
Example:
// top-level or shared config const API_V1_EOL_DISPLAY = "January 1, 2026"; ... <Text className="font-semibold text-red-600"> Important: <span className="font-mono">api.unkey.dev/v1</span> will be discontinued on {API_V1_EOL_DISPLAY}. </Text>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
internal/resend/emails/api_v1_migration.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test Packages / Test
…unkey into workspace-slug-not-null
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
go/apps/api/routes/v2_identities_list_identities/200_test.go (1)
54-87: Compile-time bug: cannot range over int (totalIdentities)for i := range totalIdentities is invalid; use a classic for loop.
- const totalIdentities = 15 - var externalIDs []string - for i := range totalIdentities { + const totalIdentities = 15 + var externalIDs []string + for i := 0; i < totalIdentities; i++ {Optionally preallocate for fewer allocs:
- var externalIDs []string + externalIDs := make([]string, 0, totalIdentities)go/apps/api/routes/v2_identities_list_identities/400_test.go (1)
82-114: Use testutil.CallRaw for malformed JSON test
The test builds a rawhttp.Requestwith invalid JSON but then callsCallRoute, which re-marshals a valid struct—so the malformed body is never sent. Replace theCallRouteinvocation with:res := testutil.CallRaw[openapi.BadRequestErrorResponse](h, req)to exercise the JSON parse error.
go/pkg/testutil/seed/seed.go (2)
49-55: Optional: derive a readable slug from the name for seed data.Using uid.New("slug") is fine; for debuggability you could slugify the name (lowercase, hyphenated). Example:
name := uid.New("test_name") slug := strings.ToLower(strings.ReplaceAll(name, " ", "-")) params := db.InsertWorkspaceParams{ ID: uid.New("test_ws"), OrgID: uid.New("test_org"), Name: name, Slug: slug, CreatedAt: time.Now().UnixMilli(), }Remember to
import "strings".
430-433: Bug: incorrect assertions validate WorkspaceID instead of Name/Slug.These checks won’t catch empty Name/Slug. Fix as below.
- require.NoError(s.t, assert.NotEmpty(req.WorkspaceID, "Permission Name must be set")) - require.NoError(s.t, assert.NotEmpty(req.WorkspaceID, "Permission Slug must be set")) + require.NoError(s.t, assert.NotEmpty(req.Name, "Permission Name must be set")) + require.NoError(s.t, assert.NotEmpty(req.Slug, "Permission Slug must be set"))go/pkg/db/models_generated.go (1)
770-776: Include Slug in all InsertWorkspaceParams literals
Tests and seed data (e.g., go/pkg/testutil/seed/seed.go, go/pkg/db/retry_test.go, and various apps/api/routes/* tests) must set the new non-nullable Slug field.go/pkg/db/queries/workspace_insert.sql (1)
20-23: Tier literal casing could diverge from code enums.Insert uses 'Free' while WorkspacesPlan constants appear lower-case ('free'). Aligning avoids surprises in comparisons.
- 'Free', + 'free',go/pkg/db/querier_generated.go (1)
1186-1210: Add missing backfill migration for slug
No migration was found that populatesslugon existing workspaces—add anUPDATE workspaces SET slug=…(e.g. derived fromname) before enforcingNOT NULL.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (14)
go/apps/api/routes/v2_identities_list_identities/200_test.go(1 hunks)go/apps/api/routes/v2_identities_list_identities/400_test.go(2 hunks)go/apps/api/routes/v2_identities_list_identities/cross_workspace_test.go(1 hunks)go/apps/api/routes/v2_keys_add_permissions/404_test.go(1 hunks)go/apps/api/routes/v2_keys_remove_permissions/403_test.go(1 hunks)go/apps/api/routes/v2_keys_remove_permissions/404_test.go(2 hunks)go/apps/api/routes/v2_keys_set_roles/404_test.go(2 hunks)go/pkg/db/bulk_workspace_insert.sql.go(3 hunks)go/pkg/db/models_generated.go(1 hunks)go/pkg/db/querier_generated.go(2 hunks)go/pkg/db/queries/workspace_insert.sql(2 hunks)go/pkg/db/retry_test.go(1 hunks)go/pkg/db/workspace_insert.sql_generated.go(6 hunks)go/pkg/testutil/seed/seed.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
go/pkg/testutil/seed/seed.go (1)
go/pkg/uid/uid.go (1)
New(97-127)
go/apps/api/routes/v2_keys_remove_permissions/403_test.go (1)
go/pkg/testutil/seed/seed.go (1)
New(39-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build / Build
- GitHub Check: Test Packages / Test
- GitHub Check: Test API / API Test Local
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (14)
go/apps/api/routes/v2_keys_set_roles/404_test.go (1)
151-157: Slug added to InsertWorkspace — LGTMSupplying Slug is required post-migration; using uid.New("slug") avoids uniqueness collisions in parallel tests.
Unable to automatically verify all InsertWorkspaceParams initializations include a Slug field due to environment limitations—please manually confirm no instances remain missing a Slug field.go/apps/api/routes/v2_keys_add_permissions/404_test.go (1)
80-86: Populate slug on workspace insert — looks correctMatches the NOT NULL schema. No further action.
go/apps/api/routes/v2_identities_list_identities/200_test.go (1)
292-298: Slug added to single-workspace setup — OKConsistent with new schema requirements.
go/apps/api/routes/v2_keys_remove_permissions/403_test.go (1)
112-119: Slug supplied on other workspace insert — goodAligns tests with NOT NULL slug constraint.
go/apps/api/routes/v2_identities_list_identities/cross_workspace_test.go (1)
45-52: Slug passed on workspace insert: looks good.Providing Slug in InsertWorkspaceParams unblocks the NOT NULL constraint and keeps this test self-contained.
go/pkg/db/retry_test.go (1)
142-149: Populate slug in integration setup: good catch.Including Slug in InsertWorkspaceParams aligns the test with the new schema and generated types.
go/apps/api/routes/v2_keys_remove_permissions/404_test.go (1)
114-124: Added slug on workspace inserts in tests: all set.Both inserts now satisfy the NOT NULL slug constraint.
Also applies to: 186-196
go/pkg/db/queries/workspace_insert.sql (1)
2-9: Remove migration/backfill verification—slugalready exists as NOT NULL with a UNIQUE constraint.Likely an incorrect or invalid review comment.
go/pkg/db/bulk_workspace_insert.sql.go (3)
11-13: Bulk insert column list updated: good.Slug added to the column list; matches generated single-row insert.
24-25: Values placeholder count matches column count.Now five positional args before literals — correct.
32-37: Args collection includes Slug in correct order.Order aligns with VALUES tuple. Looks good.
go/pkg/db/workspace_insert.sql_generated.go (2)
39-45: InsertWorkspaceParams adds Slug: good.Public API now enforces the invariant at compile-time.
73-80: Parameter binding order correct.Slug is passed between Name and CreatedAt consistently with the SQL.
go/pkg/db/querier_generated.go (1)
1186-1210: Slug added to InsertWorkspace: looks correct.Column list and VALUES placeholders now include slug; counts align; constants remain unchanged. No issues on the generated side.
3 participants
What does this PR do?
Fixes # (issue)
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
Chores
UI/Content
Tests