fix: enforce case-insensitive PAT title uniqueness#1489
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughIntroduces a PostgreSQL migration to modify the uniqueness constraint on user PAT titles. The up migration replaces the case-sensitive unique index with a case-insensitive version using the LOWER() function, while the down migration reverts this change. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~4 minutes Suggested reviewers
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/store/postgres/migrations/20260330100000_user_pats_title_case_insensitive.up.sql (1)
2-3: The drop-then-create pattern is safe due to implicit transaction wrapping, but a safer approach is still recommended.In this codebase, migrations using golang-migrate/v4 with PostgreSQL execute multiple semicolon-separated statements within an implicit single transaction. The two statements in this migration (DROP and CREATE) will be atomic, so the index will not be left in an unprotected state if the CREATE fails.
However, the suggested approach (CREATE with a temporary name first, then DROP the old index, then RENAME) remains preferable because it:
- Validates the new index can be built before removing the old one
- Eliminates the brief intermediate state where the constraint doesn't exist, even if transactional
- Is more resilient to future framework or implementation changes
The actual risk here is data validation: if existing data contains case-insensitive duplicates (e.g., records with titles "Test" and "test" for the same user_id and org_id), the CREATE UNIQUE INDEX will fail and the migration will be marked dirty. Verify no such duplicates exist before deploying.
Safer migration ordering
--- Replace case-sensitive unique index with case-insensitive one -DROP INDEX IF EXISTS idx_user_pats_unique_title; -CREATE UNIQUE INDEX idx_user_pats_unique_title ON user_pats(user_id, org_id, LOWER(title)) WHERE deleted_at IS NULL; +CREATE UNIQUE INDEX idx_user_pats_unique_title_ci + ON user_pats(user_id, org_id, LOWER(title)) + WHERE deleted_at IS NULL; +DROP INDEX IF EXISTS idx_user_pats_unique_title; +ALTER INDEX idx_user_pats_unique_title_ci RENAME TO idx_user_pats_unique_title;
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f7a12ad3-b652-4464-a653-c199c91aff39
📒 Files selected for processing (2)
internal/store/postgres/migrations/20260330100000_user_pats_title_case_insensitive.down.sqlinternal/store/postgres/migrations/20260330100000_user_pats_title_case_insensitive.up.sql
internal/store/postgres/migrations/20260330100000_user_pats_title_case_insensitive.down.sql
Show resolved
Hide resolved
Pull Request Test Coverage Report for Build 23743518746Details
💛 - Coveralls |
Description:
Summary
LOWER(title)CheckCurrentUserPATTitleRPC which already checks case-insensitivelyProblem
The PAT title uniqueness has a mismatch between the availability check and the database constraint:
CheckCurrentUserPATTitleRPC): UsesLOWER(title)— treats "Token" and "token" as the same titletitle— treats "Token" and "token" as different titlesThis means:
The fix aligns the DB constraint with the availability check by making both case-insensitive.
Pre-migration check
Run before applying to ensure no existing case-insensitive duplicates:
If any results, manually rename duplicates before running the migration.
Changes
Test plan