Fix workspace name uniqueness to be scoped per user#1007
Fix workspace name uniqueness to be scoped per user#1007derekhiggins wants to merge 1 commit intoambient-code:mainfrom
Conversation
Previously workspace names were globally unique across all users, preventing users from creating workspaces with names already taken by others (e.g., 'ambient-code'). Changes: - Add owner_user_id field to Project model - Change unique constraint from name-only to composite (owner_user_id, name) - Automatically set owner_user_id from authenticated user context - Add migration to update existing database schema - Update OpenAPI schema to include owner_user_id field - Fix ID generation to use unique IDs instead of name This allows each user to have their own workspaces with any name, with uniqueness scoped only within their own workspace collection.
WalkthroughThe changes add project ownership tracking by introducing an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/ambient-api-server/plugins/projects/handler.go`:
- Around line 40-42: The code sets projectModel.OwnerUserId from
auth.GetUsernameFromContext(ctx) without checking for an empty string, allowing
creation of projects with owner_user_id == ""; update the project creation
handler (the function that calls auth.GetUsernameFromContext and assigns
projectModel.OwnerUserId) to validate the returned username and, if it is empty,
return an appropriate error response (e.g., HTTP 401/400) and abort creation
instead of assigning an empty owner; ensure the validation occurs before any DB
insert and reference GetUsernameFromContext and projectModel.OwnerUserId when
adding the check.
In `@components/ambient-api-server/plugins/projects/migration.go`:
- Around line 41-48: The rollback block building rollbackStatements (including
`DROP INDEX IF EXISTS idx_owner_name`, `CREATE UNIQUE INDEX IF NOT EXISTS
idx_projects_name ON projects(name)`, and `ALTER TABLE projects DROP COLUMN IF
EXISTS owner_user_id`) can fail if duplicate project names exist across
different `owner_user_id` values; update the rollback in migration.go to detect
duplicates before attempting to recreate `idx_projects_name` and either (a)
abort with a clear error/warning listing offending `name` values and instructing
manual cleanup, or (b) skip/guard the `CREATE UNIQUE INDEX` step when duplicates
are present; implement this by running a pre-check query (SELECT name, COUNT(*)
FROM projects GROUP BY name HAVING COUNT(*) > 1) and branching the rollback to
log/return a helpful error or omit the index-create statement accordingly.
- Around line 32-48: The migration currently adds owner_user_id as NOT NULL with
default '' which leaves orphaned projects and can cause future uniqueness
conflicts; in ownerUserIdMigration, change the flow to add owner_user_id as
NULLABLE first, then run a backfill UPDATE (e.g., set owner_user_id = created_by
or derived from audit/logs) for rows where that data exists, then ALTER TABLE to
set NOT NULL and add DEFAULT if you want enforcement, and adjust the
migrateStatements/rollbackStatements accordingly; if no reliable backfill
exists, keep owner_user_id nullable and update the unique index logic (or use
COALESCE) to avoid grouping all empty values into one namespace.
In `@components/ambient-api-server/plugins/projects/model.go`:
- Around line 30-32: The Project.BeforeCreate method now sets d.ID = api.NewID()
instead of deriving IDs from d.Name, which is a breaking change for clients that
inferred project IDs; update the release notes to call out this change and add
migration guidance: document that Project IDs are now opaque UUIDs (referencing
Project.BeforeCreate and api.NewID()), advise clients to stop deriving IDs from
names and to fetch IDs via the Projects API, provide an example migration path
(map old name-based IDs to new IDs by querying existing projects or running a
one-time server-side migration), and note any compatibility window or
deprecation timeline for name-derived IDs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: be52b272-f212-43b0-b058-eadcda873bf4
📒 Files selected for processing (5)
components/ambient-api-server/openapi/openapi.projects.yamlcomponents/ambient-api-server/plugins/projects/handler.gocomponents/ambient-api-server/plugins/projects/migration.gocomponents/ambient-api-server/plugins/projects/model.gocomponents/ambient-api-server/plugins/projects/plugin.go
| if username := auth.GetUsernameFromContext(ctx); username != "" { | ||
| projectModel.OwnerUserId = username | ||
| } |
There was a problem hiding this comment.
Missing validation when username is unavailable.
If GetUsernameFromContext returns an empty string, the project is created with owner_user_id = "". This defeats the per-user uniqueness goal since all such projects share the empty-owner namespace. Although auth middleware should block unauthenticated requests, defensively returning an error here prevents silent creation of orphaned projects.
Proposed fix
- if username := auth.GetUsernameFromContext(ctx); username != "" {
- projectModel.OwnerUserId = username
+ username := auth.GetUsernameFromContext(ctx)
+ if username == "" {
+ return nil, errors.Unauthorized("unable to determine project owner from request context")
}
+ projectModel.OwnerUserId = username📝 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.
| if username := auth.GetUsernameFromContext(ctx); username != "" { | |
| projectModel.OwnerUserId = username | |
| } | |
| username := auth.GetUsernameFromContext(ctx) | |
| if username == "" { | |
| return nil, errors.Unauthorized("unable to determine project owner from request context") | |
| } | |
| projectModel.OwnerUserId = username |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/ambient-api-server/plugins/projects/handler.go` around lines 40 -
42, The code sets projectModel.OwnerUserId from auth.GetUsernameFromContext(ctx)
without checking for an empty string, allowing creation of projects with
owner_user_id == ""; update the project creation handler (the function that
calls auth.GetUsernameFromContext and assigns projectModel.OwnerUserId) to
validate the returned username and, if it is empty, return an appropriate error
response (e.g., HTTP 401/400) and abort creation instead of assigning an empty
owner; ensure the validation occurs before any DB insert and reference
GetUsernameFromContext and projectModel.OwnerUserId when adding the check.
| func ownerUserIdMigration() *gormigrate.Migration { | ||
| migrateStatements := []string{ | ||
| // Add owner_user_id column with a default empty string for existing rows | ||
| `ALTER TABLE projects ADD COLUMN IF NOT EXISTS owner_user_id TEXT NOT NULL DEFAULT ''`, | ||
| // Drop the old unique index on name only | ||
| `DROP INDEX IF EXISTS idx_projects_name`, | ||
| // Create composite unique index on (owner_user_id, name) | ||
| `CREATE UNIQUE INDEX IF NOT EXISTS idx_owner_name ON projects(owner_user_id, name)`, | ||
| } | ||
| rollbackStatements := []string{ | ||
| // Drop the composite unique index | ||
| `DROP INDEX IF EXISTS idx_owner_name`, | ||
| // Recreate the old unique index on name only | ||
| `CREATE UNIQUE INDEX IF NOT EXISTS idx_projects_name ON projects(name)`, | ||
| // Drop the owner_user_id column | ||
| `ALTER TABLE projects DROP COLUMN IF EXISTS owner_user_id`, | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider data backfill for existing projects.
Existing projects will have owner_user_id = '' after migration. If ownership data can be derived (e.g., from audit logs or created_by fields), consider backfilling during migration or as a follow-up task. Otherwise, these orphaned projects share a single namespace and may conflict with future user-created projects named identically.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/ambient-api-server/plugins/projects/migration.go` around lines 32
- 48, The migration currently adds owner_user_id as NOT NULL with default ''
which leaves orphaned projects and can cause future uniqueness conflicts; in
ownerUserIdMigration, change the flow to add owner_user_id as NULLABLE first,
then run a backfill UPDATE (e.g., set owner_user_id = created_by or derived from
audit/logs) for rows where that data exists, then ALTER TABLE to set NOT NULL
and add DEFAULT if you want enforcement, and adjust the
migrateStatements/rollbackStatements accordingly; if no reliable backfill
exists, keep owner_user_id nullable and update the unique index logic (or use
COALESCE) to avoid grouping all empty values into one namespace.
| rollbackStatements := []string{ | ||
| // Drop the composite unique index | ||
| `DROP INDEX IF EXISTS idx_owner_name`, | ||
| // Recreate the old unique index on name only | ||
| `CREATE UNIQUE INDEX IF NOT EXISTS idx_projects_name ON projects(name)`, | ||
| // Drop the owner_user_id column | ||
| `ALTER TABLE projects DROP COLUMN IF EXISTS owner_user_id`, | ||
| } |
There was a problem hiding this comment.
Rollback may fail if duplicate names exist.
If users create projects with identical names (under different owner_user_id values) after this migration runs, the rollback will fail at line 45 when attempting to recreate the single-column unique index on name. Document this limitation or add a check/warning in the rollback path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/ambient-api-server/plugins/projects/migration.go` around lines 41
- 48, The rollback block building rollbackStatements (including `DROP INDEX IF
EXISTS idx_owner_name`, `CREATE UNIQUE INDEX IF NOT EXISTS idx_projects_name ON
projects(name)`, and `ALTER TABLE projects DROP COLUMN IF EXISTS owner_user_id`)
can fail if duplicate project names exist across different `owner_user_id`
values; update the rollback in migration.go to detect duplicates before
attempting to recreate `idx_projects_name` and either (a) abort with a clear
error/warning listing offending `name` values and instructing manual cleanup, or
(b) skip/guard the `CREATE UNIQUE INDEX` step when duplicates are present;
implement this by running a pre-check query (SELECT name, COUNT(*) FROM projects
GROUP BY name HAVING COUNT(*) > 1) and branching the rollback to log/return a
helpful error or omit the index-create statement accordingly.
| func (d *Project) BeforeCreate(tx *gorm.DB) error { | ||
| d.ID = d.Name | ||
| d.ID = api.NewID() | ||
| return nil |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for any code that might construct project IDs from names
rg -n -C2 'project.*[Ii][Dd].*[Nn]ame|[Nn]ame.*project.*[Ii][Dd]' --type goRepository: ambient-code/platform
Length of output: 1790
🏁 Script executed:
#!/bin/bash
# Try to list files in the project to understand structure
git ls-files components/ambient-api-server/plugins/projects/ 2>/dev/null | head -20Repository: ambient-code/platform
Length of output: 1888
🏁 Script executed:
#!/bin/bash
# Search for previous version of the model.go file or any git history
# First, check what files exist and search for ID-related patterns
find . -name "*.go" -path "*projects*" -type f 2>/dev/null | head -10Repository: ambient-code/platform
Length of output: 732
🏁 Script executed:
#!/bin/bash
# Search for NewID function definition to understand its nature
rg -n "func NewID|type.*NewID" --type go 2>/dev/null | head -20Repository: ambient-code/platform
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Search for Project struct definition and BeforeCreate
rg -n "type.*Project|BeforeCreate" --type go -A 3 2>/dev/null | head -40Repository: ambient-code/platform
Length of output: 3038
🏁 Script executed:
#!/bin/bash
# List all go files in projects directory to see actual structure
ls -la components/ambient-api-server/plugins/projects/*.go 2>/dev/nullRepository: ambient-code/platform
Length of output: 1718
🏁 Script executed:
#!/bin/bash
# Search for ID assignment patterns in projects
rg -n "d\.ID\s*=" --type go 2>/dev/null | grep -i projectRepository: ambient-code/platform
Length of output: 229
🏁 Script executed:
#!/bin/bash
# Read the complete model.go file
cat -n components/ambient-api-server/plugins/projects/model.goRepository: ambient-code/platform
Length of output: 1471
🏁 Script executed:
#!/bin/bash
# Search for git history of model.go to see what changed
git log --oneline -20 -- components/ambient-api-server/plugins/projects/model.go 2>/dev/nullRepository: ambient-code/platform
Length of output: 124
🏁 Script executed:
#!/bin/bash
# Search for any documentation or tests mentioning Project ID generation
rg -n "NewID|project.*id|ID.*project" --type go -i components/ambient-api-server/plugins/projects/ 2>/dev/null | head -30Repository: ambient-code/platform
Length of output: 4225
🏁 Script executed:
#!/bin/bash
# Search for any external API contracts or client code that might use Project IDs
rg -n "project.*id|construct.*id|derive.*id" --type go -i 2>/dev/null | grep -v ".pb.go" | head -20Repository: ambient-code/platform
Length of output: 2339
🏁 Script executed:
#!/bin/bash
# Get the actual diff for the commit that touched model.go
git show f246df6 -- components/ambient-api-server/plugins/projects/model.go 2>/dev/null | head -50Repository: ambient-code/platform
Length of output: 1889
🏁 Script executed:
#!/bin/bash
# Search for any comments in the codebase about ID generation
rg -n "deterministic|construct.*id|derive.*id|from.*name" --type go -i 2>/dev/null | head -20Repository: ambient-code/platform
Length of output: 2329
🏁 Script executed:
#!/bin/bash
# Search git log more broadly for projects changes
git log --oneline -50 -- components/ambient-api-server/plugins/projects/ 2>/dev/nullRepository: ambient-code/platform
Length of output: 124
🏁 Script executed:
#!/antml/bash
# Look for any test files that might indicate ID expectations
rg -n "NewID|project.*id|SetID" --type go components/ambient-api-server/plugins/projects/ -A 2 2>/dev/null | grep -i testRepository: ambient-code/platform
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Search for release notes, changelog, or migration guides
find . -type f \( -name "CHANGELOG*" -o -name "RELEASE*" -o -name "MIGRATION*" -o -name "*.md" \) 2>/dev/null | head -20Repository: ambient-code/platform
Length of output: 812
🏁 Script executed:
#!/bin/bash
# Search for any comments or documentation about the ID change
rg -n "unique.*id|breaking.*change|project.*id" --type md -i 2>/dev/null | head -30Repository: ambient-code/platform
Length of output: 4840
🏁 Script executed:
#!/bin/bash
# Check if the commit mentions anything about documentation
git show f246df6 --stat 2>/dev/null | head -20Repository: ambient-code/platform
Length of output: 968
Project ID generation changed from deterministic to UUID—ensure release notes document this breaking change.
The commit "Fix workspace name uniqueness to be scoped per user" changed Project ID generation from d.ID = d.Name to d.ID = api.NewID(). This breaks any external integrations that relied on deriving Project IDs from names without querying the API. Document this in release notes and provide migration guidance for affected clients.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/ambient-api-server/plugins/projects/model.go` around lines 30 -
32, The Project.BeforeCreate method now sets d.ID = api.NewID() instead of
deriving IDs from d.Name, which is a breaking change for clients that inferred
project IDs; update the release notes to call out this change and add migration
guidance: document that Project IDs are now opaque UUIDs (referencing
Project.BeforeCreate and api.NewID()), advise clients to stop deriving IDs from
names and to fetch IDs via the Projects API, provide an example migration path
(map old name-based IDs to new IDs by querying existing projects or running a
one-time server-side migration), and note any compatibility window or
deprecation timeline for name-derived IDs.
|
API is a work in progress, so let's hold this until it rolls out. Imminent. |
Review Queue Status
Action needed: Address CodeRabbit Major issue on unique index migration
|
2 participants
Previously workspace names were globally unique across all users, preventing users from creating workspaces with names already taken by others (e.g., 'ambient-code').
Changes:
This allows each user to have their own workspaces with any name, with uniqueness scoped only within their own workspace collection.
--
Done in ambient-code, Needs review (I got no knowledge of this codebase)