fix(api): enforce case-insensitive uniqueness on User.Handle#338
Conversation
Handles are compared case-insensitively app-wide (lower(Handle) == normalized), but the column had no unique index, so two concurrent SetHandle calls could both pass the AnyAsync pre-check and persist duplicate handles under a race. Add a partial functional unique index on lower(Handle) WHERE Handle IS NOT NULL, deduping any pre-existing case-insensitive collisions first (colliding rows keep the earliest account's handle; the rest fall back to their seeded user_<id> handle). The index lives in raw migration SQL because Npgsql exposes no fluent functional-index API; a plain unique index would be case-sensitive and wrong. The SetHandleCommand race fallback already maps the resulting 23505 to the friendly HandleTaken error; add the missing test covering that guard. Refs thomasluizon/orbit-ui-mobile#243 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Correcting an accidental placeholder: a prior automated review action on this PR was submitted with body text "test-dry-run-check" due to a tooling error while verifying command permissions. It was not a real review and should be disregarded (the automation has no way to delete or edit that submitted review via the available tooling). Posting the actual review below. Code Review: PR #338Scope: PR #338 in thomasluizon/orbit-api -- fix(api): enforce case-insensitive uniqueness on User.Handle SummaryAdds a partial, case-insensitive functional unique index (IX_Users_Handle_Lower on LOWER("Handle") WHERE "Handle" IS NOT NULL) to close a check-then-act race in SetHandleCommandHandler, where two concurrent SetHandle calls for the same handle could both pass the pre-check AnyAsync and persist duplicate handles. The Up() migration first deduplicates any pre-existing case-insensitive collisions (keeping the earliest-created row, reassigning the rest to the same deterministic fallback handle formula already used by User.SeedDefaultHandle()), then creates the index via raw SQL because Npgsql exposes no fluent functional-index API. A new unit test (Handle_ConcurrentInsertLosesUniqueIndexRace_ReturnsHandleTaken) exercises the previously-untested "catch DbUpdateException when DbUniqueViolation.IsUniqueViolation" fallback in the handler. Changes are limited to a new EF migration plus its generated Designer.cs snapshot and one test file -- no DTO, controller, or contract-surface changes. FindingsCriticalNone. HighNone. MediumNone. Low / Info[INFO] Non-concurrent CREATE UNIQUE INDEX briefly locks writes on Users during deploy
Subagents
Validation
Deferred -- N/A dimensions and files not verdicted
What's good
RecommendationApprove as-is. No Critical or High findings; the one Info-level note about the non-concurrent index build is an accepted, precedented tradeoff and needs no follow-up. |
There was a problem hiding this comment.
Correcting an accidental placeholder: a prior automated review action on this PR was submitted with body text "test-dry-run-check" due to a tooling error while verifying command permissions. It was not a real review and should be disregarded (the automation had no way to delete or edit that submitted review via the available tooling). This review supersedes it.
Code Review: PR #338
Scope: PR #338 in thomasluizon/orbit-api -- fix(api): enforce case-insensitive uniqueness on User.Handle
Recommendation: APPROVE
Summary
Adds a partial, case-insensitive functional unique index (IX_Users_Handle_Lower on LOWER("Handle") WHERE "Handle" IS NOT NULL) to close a check-then-act race in SetHandleCommandHandler, where two concurrent SetHandle calls for the same handle could both pass the pre-check AnyAsync and persist duplicate handles. The Up() migration first deduplicates any pre-existing case-insensitive collisions (keeping the earliest-created row, reassigning the rest to the same deterministic fallback handle formula already used by User.SeedDefaultHandle()), then creates the index via raw SQL because Npgsql exposes no fluent functional-index API. A new unit test (Handle_ConcurrentInsertLosesUniqueIndexRace_ReturnsHandleTaken) exercises the previously-untested "catch DbUpdateException when DbUniqueViolation.IsUniqueViolation" fallback in the handler. Changes are limited to a new EF migration plus its generated Designer.cs snapshot and one test file -- no DTO, controller, or contract-surface changes.
Findings
Critical
None.
High
None.
Medium
None.
Low / Info
[INFO] Non-concurrent CREATE UNIQUE INDEX briefly locks writes on Users during deploy
- dimension: 1 (Correctness) / operational note
- location: src/Orbit.Infrastructure/Migrations/20260712120428_AddUserHandleUniqueIndex.cs:7-11
- issue: CREATE UNIQUE INDEX (no CONCURRENTLY) takes a lock that blocks concurrent writes to Users while it builds.
- risk: On a large Users table this could cause a brief write-blocking window during deploy -- negligible today but worth remembering if the table grows.
- fix: No action needed now; CONCURRENTLY can't run inside an EF migration transaction anyway. This mirrors the accepted HabitLogCompletionUniqueIndex precedent the PR body cites, so it is a known, already-sanctioned tradeoff, not a new regression.
- reference: precedent src/Orbit.Infrastructure/Migrations/20260608175140_HabitLogCompletionUniqueIndex.cs
Subagents
| Agent | Verdict |
|---|---|
| security-reviewer | PASS -- no injection surface in the raw migrationBuilder.Sql() calls (hardcoded columns plus a static per-row Id-derived formula, no external/user input concatenated); dedup logic is deterministic and PII-safe; SetHandleCommandHandler's existing catch maps DbUpdateException to a generic HandleTaken error with no internal detail leak; test file has no secrets or unsafe mocking. |
| contract-aligner | N/A -- no DTO, Controller route, or packages/shared type/endpoints.ts change in this diff. |
Validation
| Check | Result |
|---|---|
| Build (dotnet) | N/A in this review environment (sandboxed). Skipped per workflow instructions -- Build / Unit Tests / SonarCloud already run as separate required checks on this PR. PR description states dotnet build 0 errors and dotnet test Application (2629) + Domain (504) all green. |
| Tests (dotnet) | N/A, same as above. |
Deferred -- N/A dimensions and files not verdicted
- Dimension 8 (DESIGN.md/AI-slop): N/A -- no apps/* UI files in this diff.
- Dimension 9 (Parity web to mobile): N/A -- backend-only diff.
- Dimension 10 (i18n): N/A -- no new user-facing strings.
- Dimension 11 (Contract drift + backward-compat guard): N/A -- no DTO, Zod schema, or endpoint change; nothing for an old mobile client to break against.
- Dimension 14 (FEATURES.md parity): N/A -- pure bugfix, no user-facing feature-surface change.
- 20260712120428_AddUserHandleUniqueIndex.Designer.cs: not read line-by-line (2566 lines of auto-generated point-in-time entity-mapping boilerplate, excluded from the ORBIT0001 comment analyzer). Spot-checked that it does not encode the new functional index, consistent with OrbitDbContextModelSnapshot.cs being untouched and the PR's "ef migrations has-pending-model-changes reports clean" claim.
- Phase 6 adversarial/second-opinion pass: not invoked -- no Critical/High finding survived Phase 3 to require it.
What's good
- Root-causes the actual bug (missing DB-level enforcement for an app-wide case-insensitive comparison) instead of adding more application-level checking.
- The dedup fallback formula matches the existing User.SeedDefaultHandle() domain formula exactly, so reassigned handles look identical to normal seeded ones.
- Closes a previously dead/untested code path: the DbUpdateException to HandleTaken catch in SetHandleCommandHandler had no test and no way to actually trigger before this index existed.
- PR body clearly explains the raw-SQL choice and the intentionally-unchanged model snapshot, and cites the precedent it mirrors.
- Down() migration correctly drops only the index, matching the additive nature of the change.
Recommendation
Approve as-is. No Critical or High findings; the one Info-level note about the non-concurrent index build is an accepted, precedented tradeoff and needs no follow-up.
There was a problem hiding this comment.
Code Review: PR #338
Scope: PR #338 — fix/user-handle-unique-index (case-insensitive uniqueness on User.Handle)
Recommendation: APPROVE
| Severity | Count |
|---|---|
| Critical (incl. old-client breaks) | 0 |
| High | 0 |
| Medium | 1 |
| Low / Info | 0 (not posted per signal gate) |
Findings
Medium — CREATE UNIQUE INDEX on Users runs without CONCURRENTLY
- location:
src/Orbit.Infrastructure/Migrations/20260712120428_AddUserHandleUniqueIndex.cs:2607-2610 - issue: the index build takes an ACCESS EXCLUSIVE lock on
Usersfor its duration; unlike theHabitLogCompletionUniqueIndexprecedent this cites,Userssits on every authenticated request's path (JWT auth), so the blast radius of a lock here is broader. - fix: acceptable as-is if
Usersis still small (matches house style, and EF wraps migrations in a transaction whichCONCURRENTLYcan't run inside); otherwise split into a non-transactionalCONCURRENTLYstep. - Non-blocking — Medium never forces a request-changes outcome per the rubric's deterministic outcome rule.
Verification performed
- Read the full diff: migration Designer.cs snapshot, migration
.cs(raw SQL), andtests/Orbit.Application.Tests/Social/SetHandleCommandTests.cs. - Verified the migration's dedup fallback handle (
'user_' || LEFT(REPLACE(Id::text,'-',''),12)) is byte-identical toUser.SeedDefaultHandle()(src/Orbit.Domain/Entities/User.cs:402,$"user_{Id:N}"[..17]). - Confirmed
CreatedAtUtc(User.cs:40) is non-nullable, so the dedup'sPARTITION BY LOWER(Handle) ORDER BY CreatedAtUtc, Idis deterministic. - Confirmed the model snapshot (
OrbitDbContextModelSnapshot.cs) declares noHasIndexonHandle, corroborating the PR's claim the fluent model is intentionally unchanged (raw-SQL migration only). - Confirmed the inline
//comment in the migration is a URL-linked WHY note, the sanctioned exception under the narration-comment rule. - Confirmed
FriendGraphService.cs:24andSetHandleCommand.cs:29both doHandle.ToLower() ==, matching the index'sLOWER(Handle)design — no mismatch between app-level and DB-level case handling. - Confirmed the new test
Handle_ConcurrentInsertLosesUniqueIndexRace_ReturnsHandleTakencorrectly drivesDbUpdateException→ SQLSTATE23505→DbUniqueViolation.IsUniqueViolation→HandleTaken, closing a real untested race-path gap. - SQL-injection / PII check: no string interpolation of external input in the migration; all identifiers are static; the dedupe fallback derives only from the row's own DB-generated
Id. contract-alignergate not triggered — no DTO/Controller route orpackages/sharedtype touched by this diff.
Subagents
| Agent | Verdict |
|---|---|
| security-reviewer | PASS — no findings |
| contract-aligner | N/A — no DTO/Controller route/packages/shared type touched |
Validation
| Check | Result |
|---|---|
| Build (dotnet) | N/A — CI runs Build as a separate required check |
| Tests (dotnet) | N/A — CI runs Unit Tests as a separate required check |
Deferred / out of scope
- The large auto-generated
Designer.cssnapshot was spot-checked, not line-by-line verdicted (pure EF boilerplate, confirmed to carry noHandleindex of its own). - Rubric dimensions not touched by this backend-only migration/test diff (frontend contract, auth surface changes, etc.) are N/A.
There was a problem hiding this comment.
PR Review: fix(api) enforce case-insensitive uniqueness on User.Handle
Recommendation: APPROVE
Severity counts - Critical (incl. breaks old mobile clients): 0 | High: 0 | Medium: 1 (non-blocking, already surfaced in a prior review) | Low/Info: 0
Summary:
This PR was already reviewed multiple times previously (all APPROVE). The only commit since the last review is a no-op merge of main into this branch - confirmed via git diff that the diff against origin/main is unchanged (identical 3 files, identical line counts). No new content to review since the last pass.
Medium (non-blocking, already accepted in a prior review):
CREATE UNIQUE INDEX "IX_Users_Handle_Lower" in src/Orbit.Infrastructure/Migrations/20260712120428_AddUserHandleUniqueIndex.cs:34-37 runs without CONCURRENTLY, taking an ACCESS EXCLUSIVE lock on Users during the migration. Broader blast radius than the cited HabitLogCompletionUniqueIndex precedent since Users sits on every authenticated request, but acceptable while the table is small (CONCURRENTLY cannot run inside EF's migration transaction). No action required.
Independent re-verification performed this pass:
- Re-diffed via git directly against origin/main to confirm no new content since the last review.
- Confirmed no SQL injection surface in the raw-SQL migration (static SQL + row-derived Id, no external input interpolated).
- Confirmed the dedup fallback formula ('user_' || LEFT(REPLACE(u."Id"::text, '-', ''), 12)) is byte-identical to User.SeedDefaultHandle() in src/Orbit.Domain/Entities/User.cs:402.
- Dispatched a fresh security-reviewer subagent against the migration and test file: PASS, no injection/PII/secrets issues.
- contract-aligner: N/A - no DTO/Controller/packages-shared surface touched.
Deferred / N/A:
Dimensions covering UI, contract surface, and user-facing feature changes are N/A for this backend migration+test diff. Cross-repo parity is not verifiable in CI (sibling orbit-ui-mobile repo not checked out). Designer.cs (2566 lines, pure EF-generated snapshot) spot-checked only, consistent with all three prior passes.
|
There was a problem hiding this comment.
Code Review: PR #338
Scope: PR #338 in thomasluizon/orbit-api — fix(api): enforce case-insensitive uniqueness on User.Handle
Recommendation: APPROVE
Summary
Re-diffed the PR's current head (896028d961c035918576b76209c9bc113147290d) against its current base (c6d2bd0588348372ca32d75a79ec81b4831a044c) and confirmed it is byte-identical to what three prior automated reviews already evaluated: same 3 files (migration .cs, migration Designer.cs, SetHandleCommandTests.cs), same +2630/-0 line counts. The only commits added since the original fix commit (be283f1) are three no-op Merge branch 'main' commits — no new content to review.
Independently re-verified the substantive claims rather than just trusting the prior passes:
- Migration SQL (
src/Orbit.Infrastructure/Migrations/20260712120428_AddUserHandleUniqueIndex.cs): dedup CTE partitions byLOWER("Handle")ordered by"CreatedAtUtc", "Id"(both deterministic/non-null tiebreakers), reassigns losers to'user_' || LEFT(REPLACE(u."Id"::text, '-', ''), 12), then createsIX_Users_Handle_Loweras a partial unique index onLOWER("Handle") WHERE "Handle" IS NOT NULL. Dedup runs before index creation, so theCREATE UNIQUE INDEXcan't fail on deploy. - Fallback formula parity: confirmed the migration's SQL formula is character-identical to
User.SeedDefaultHandle()(src/Orbit.Domain/Entities/User.cs:405,$"user_{Id:N}"[..17]) —Id::text(D-format) minus hyphens yields the same hex sequence asId:N, andLEFT(...,12)+'user_'prefix (17 chars total) matches[..17]exactly. - Model snapshot: grepped the Designer.cs snapshot and
OrbitDbContextModelSnapshot.cs— neither declaresHasIndexonHandle, corroborating the PR's claim that the fluent model is intentionally untouched (Npgsql has no fluent functional-index API). - App/DB consistency:
SetHandleCommand.cs:29andFriendGraphService.cs:24both compare viaHandle.ToLower() ==, matching the index'sLOWER(Handle)semantics. - New test (
Handle_ConcurrentInsertLosesUniqueIndexRace_ReturnsHandleTaken): correctly throws aDbUpdateExceptionwrapping aDbExceptionwithSqlState = "23505", whichDbUniqueViolation.IsUniqueViolation(src/Orbit.Application/Common/DbUniqueViolation.cs) walks viaInnerExceptionrecursion →true→ handler'scatch (DbUpdateException when DbUniqueViolation.IsUniqueViolation(...))→HandleTaken. This closes a genuinely dead/untested path (the catch existed before this PR but had no index to trigger23505, and no test). - Security: no injection surface — all SQL is static plus a per-row, DB-generated
Id; no external input concatenated; no PII in the dedup or index logic. - Contract surface: no DTO, Controller route, or
packages/sharedtype touched — cross-repo contract-aligner gate does not fire.
Findings
Critical
None.
High
None.
Medium
CREATE UNIQUE INDEX on Users runs without CONCURRENTLY (already surfaced and accepted in all three prior reviews — carried forward, not re-litigated as new)
- Location:
src/Orbit.Infrastructure/Migrations/20260712120428_AddUserHandleUniqueIndex.cs(theCREATE UNIQUE INDEX "IX_Users_Handle_Lower"block) - Issue: takes an ACCESS EXCLUSIVE lock on
Userswhile building;Userssits on every authenticated request's path (JWT auth), so blast radius is broader than the citedHabitLogCompletionUniqueIndexprecedent. - Non-blocking: acceptable as-is while
Usersis small —CONCURRENTLYcan't run inside EF's migration transaction anyway. Worth a non-transactional split later if the table grows large enough for the lock window to matter.
Low / Info
None beyond the accepted Medium above.
Subagents
| Agent | Verdict |
|---|---|
| security-reviewer | PASS — no injection surface in raw migrationBuilder.Sql() calls, dedup formula is deterministic and derives only from the row's own Id, SetHandleCommandHandler's catch maps to a generic HandleTaken with no internal detail leak. |
| contract-aligner | N/A — no DTO, Controller route, or packages/shared type/endpoints.ts touched by this diff. |
Validation
Skipped per task instructions — Build/Unit Tests/SonarCloud run as separate required CI checks on this PR.
Deferred — N/A dimensions
- DESIGN.md/AI-slop, web↔mobile parity, i18n, FEATURES.md parity: N/A — pure backend bugfix, no UI or user-facing feature-surface change.
- Contract drift / backward-compat guard: N/A — no DTO, Zod schema, or endpoint change.
orbit-ui-mobilerepo not checked out in this job — cross-repo dimensions marked not verifiable rather than guessed.
What's good
- Root-causes the actual bug (missing DB-level enforcement for an app-wide case-insensitive comparison) rather than adding more application-level checking on top of an already-racy pre-check.
- Dedup fallback formula is provably identical to the existing
User.SeedDefaultHandle()domain formula — reassigned handles are indistinguishable from normally-seeded ones. - Closes a previously dead, untested code path (
DbUpdateException→HandleTakencatch) with a real regression test. - PR body is unusually clear about the raw-SQL rationale, the intentionally-unchanged model snapshot, and the precedent it mirrors.
Down()migration is correctly scoped (only drops the index).
Recommendation
APPROVE. The diff is identical to what's already been independently verified across three prior automated review passes plus this one. The one Medium (non-CONCURRENTLY index build) is an accepted, precedented tradeoff and does not block merge.
…ploy (#243) The IX_Users_Handle_Lower unique functional index was ALREADY created by AddSocialFoundation (2026-06-27). #338's new migration re-issued a bare CREATE UNIQUE INDEX, so every prod deploy since crashed at startup with Npgsql 42P07 "relation IX_Users_Handle_Lower already exists" — EF kept re-running the never-recorded migration. CI missed it because the unit suite doesn't run raw-SQL migrations against real Postgres. Add IF NOT EXISTS so the migration is a safe no-op on the existing DB (and still correct on a fresh one), applies cleanly, records itself, and the deploy pipeline recovers. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ploy (#243) (#345) The IX_Users_Handle_Lower unique functional index was ALREADY created by AddSocialFoundation (2026-06-27). #338's new migration re-issued a bare CREATE UNIQUE INDEX, so every prod deploy since crashed at startup with Npgsql 42P07 "relation IX_Users_Handle_Lower already exists" — EF kept re-running the never-recorded migration. CI missed it because the unit suite doesn't run raw-SQL migrations against real Postgres. Add IF NOT EXISTS so the migration is a safe no-op on the existing DB (and still correct on a fresh one), applies cleanly, records itself, and the deploy pipeline recovers. No behavior change. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
#352) GetChangesV2 fetches all rows changed since a timestamp for seven synced entities, filtering each by its owner key plus UpdatedAtUtc. None of those predicates had a supporting composite index, so every sync fell back to a scan filtered on UpdatedAtUtc. Add leading-column btree indexes matching each query's exact predicate: - Habits, Goals, Tags, Notifications, ChecklistTemplates: (UserId, UpdatedAtUtc) - HabitLogs: (HabitId, UpdatedAtUtc) -- filtered by HabitId IN (...), no UserId column - GoalProgressLogs: (GoalId, UpdatedAtUtc) -- filtered by GoalId IN (...), no UserId column ProcessedRequests.UserId needs no dedicated FK index: the composite unique index (UserId, IdempotencyKey, RequestType) already leads with UserId and serves the cascade, which is why EF's convention suppressed a duplicate. Migration uses CREATE INDEX IF NOT EXISTS / DROP INDEX IF EXISTS to stay idempotent across deploys (avoids the #338 42P07 class of failure). Behavior-preserving; performance only. Refs thomasluizon/orbit-ui-mobile#243 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
What & why
User.Handleis compared case-insensitively everywhere (u.Handle.ToLower() == normalizedinSetHandleCommandandFriendGraphService), but the column had no unique index. So two concurrentSetHandlecalls for the same handle could both pass theAnyAsyncpre-check and persist duplicate handles — a classic check-then-act race (OrbitDbContext.csUser config, alongside the existing Email/ReferralCode/PlayPurchaseToken/PublicProfileSlug unique indexes).Change
AddUserHandleUniqueIndexcreates a partial functional unique indexIX_Users_Handle_LoweronLOWER("Handle") WHERE "Handle" IS NOT NULL.Handlewould letBob/bobcoexist and wouldn't serve thelower()lookups.ef migrations has-pending-model-changesreports clean.Upfirst dedupes any pre-existing case-insensitive collisions (keeping the earliest account's handle; later racers fall back to their deterministic seededuser_<id>handle) so theCREATE UNIQUE INDEXcannot fail on deploy. Mirrors theHabitLogCompletionUniqueIndexprecedent.WHERE Handle IS NOT NULLmatches the other nullable unique indexes and allows multiple NULLs.Guard (already merged, now tested)
The create/update path (
SetHandleCommandHandler) already wrapsSaveChangesAsyncintry/catch (DbUpdateException when DbUniqueViolation.IsUniqueViolation) → HandleTaken. That fallback was dead until this index existed and was untested. AddedHandle_ConcurrentInsertLosesUniqueIndexRace_ReturnsHandleTakenproving a23505at save time surfaces the friendlyHandleTakenerror instead of a 500. Signup paths seed uniqueuser_<id>handles, so they are unaffected.Validation
dotnet build— 0 errors.dotnet testApplication (2629) + Domain (504) — all green.ef migrations has-pending-model-changes— no drift.//narration (ORBIT0001).Refs thomasluizon/orbit-ui-mobile#243