fix(api): idempotent handle-index migration — unblock prod deploy (#243)#345
Conversation
…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>
b7fb856 to
72baf49
Compare
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Complete
Scope: PR #345 in thomasluizon/orbit-api — fix(api): idempotent handle-index migration — unblock prod deploy (#243)
Recommendation: APPROVE
No prior reviews or resolved threads existed on this PR (fresh review). security-reviewer ran (gated: backend src/ touched) and returned PASS. contract-aligner did not fire — no DTO/route/schema changed.
Summary
The stated intent (add IF NOT EXISTS to the IX_Users_Handle_Lower index migration) is correct: diffed against the original 20260627043108_AddSocialFoundation.cs definition and confirmed the index expression, WHERE clause, and column are byte-for-byte identical, so IF NOT EXISTS is a genuine safe no-op, not a guarantee-weakening shortcut. The commit uses a WHY-with-URL comment, compliant with the comment policy. The single commit also bundles a second, undisclosed change: a query-pushdown refactor of GetXpHistoryQuery, GetGoalProgressHistoryQuery, and GetGoalsQuery (new IGenericRepository.FindPagedAsync/SumAsync, moving pagination/filtering/aggregation from in-memory LINQ to SQL). The before/after logic for all three handlers was traced by hand and is behavior-equivalent (baseline/range date-boundary splits, filtered Include, Skip/Take semantics all check out), and it's now backed by a new SQLite-backed QueryPushDownDbTests.cs that proves the SQL translation at page/date boundaries — the refactor itself is sound and well-tested, not an integration-suite violation (the codebase already has 5 other *DbTests.cs files using the identical SQLite-in-memory pattern for Infrastructure-layer tests; this isn't new). Security review confirmed all three queries keep UserId scoping from the JWT claim before pagination, and the new repository methods are pure parameterized LINQ (no raw SQL).
Findings
Critical
None.
High
None.
Medium
Incident-fix commit silently bundles an unrelated behavioral refactor
- Location:
src/Orbit.Application/Goals/Queries/GetGoalsQuery.cs(+GetXpHistoryQuery.cs,GetGoalProgressHistoryQuery.cs,GenericRepository.cs) - Issue: The PR body and commit message describe this as a narrow "Incident fix" ("Adds IF NOT EXISTS... No behavior change; the index and its guarantees are unchanged") for the migration crash only. The actual diff also rewrites three query handlers to push pagination/filtering/aggregation into SQL via two new
IGenericRepositorymethods — a real, if behavior-preserving, production-logic change that the PR description never mentions. - Risk: A PR framed as an urgent "unblock prod deploy" hotfix invites fast merges with reduced scrutiny; if the bundled refactor had contained a bug, it would have shipped under that reduced-scrutiny banner instead of going through its own review. It also makes rollback of the incident fix ambiguous — reverting "the migration fix commit" would also revert the query refactor.
- Suggested fix: Split into two PRs (the migration fix merges immediately; the query-pushdown refactor gets its own review), or at minimum update the PR title/body to describe both changes so reviewers know the real diff size.
SqliteCompatOrbitDbContext nested class duplicated a 6th time
- Location:
tests/Orbit.Infrastructure.Tests/Persistence/QueryPushDownDbTests.cs:914-935 - Issue: The private nested
SqliteCompatOrbitDbContext(strips::default-value SQL and index filters for SQLite compatibility) is now copy-pasted identically into 6 test files. This predates the PR but the PR adds the 6th copy rather than extracting it. - Risk: Any future fix to the SQLite-compat shim now needs to be applied in 6 places; drift between copies is easy to miss.
- Suggested fix: Extract to a shared internal helper (e.g.,
tests/Orbit.Infrastructure.Tests/Persistence/SqliteTestDbContext.cs) reused by all 6 test classes. Not blocking — worth a follow-up.
Low / Info
None (signal gate).
Subagents
| Agent | Verdict |
|---|---|
| security-reviewer | PASS — no raw/interpolated SQL in new FindPagedAsync/SumAsync; UserId scoping from JWT preserved in all three refactored queries; migration preserves identical uniqueness guarantee |
| contract-aligner | N/A — no DTO, Controller route, or shared-package contract surface changed |
Validation
| Check | Result |
|---|---|
| Build (dotnet) | N/A — skipped per CI adaptation; Build/Unit Tests/SonarCloud run as separate required checks |
| Tests (dotnet) | N/A — skipped per CI adaptation |
Deferred — N/A dimensions
- Parity web↔mobile — not verifiable in this job (orbit-ui-mobile not checked out); backend-only diff.
- i18n / FEATURES.md parity / AI-slop dimensions — N/A, no user-facing strings, feature surface, or UI files in diff.
What's good
- The actual incident fix is minimal, correct, and verified byte-for-byte against the pre-existing index definition it's meant to no-op against.
- The bundled refactor is unusually well tested: it adds a real SQLite-backed integration-style suite (
QueryPushDownDbTests.cs) that proves SQL translation at exact date/page boundaries, plus rewritten unit tests that assert on captured predicate/order expressions rather than just outputs. UserIdscoping is preserved in every refactored query — confirmed independently by the security-reviewer subagent.
Recommendation
Approve — the migration fix is correct and low-risk, and the bundled refactor, while undisclosed in the PR description, is behaviorally verified and doesn't introduce a security or correctness regression. Recommend a quick follow-up to (a) note the refactor in the PR description or split future incident-fix PRs from unrelated changes, and (b) extract the duplicated SqliteCompatOrbitDbContext helper.
1 participant
Incident fix. Every orbit-api deploy since #338 crashed at startup:
Npgsql 42P07: relation "IX_Users_Handle_Lower" already exists. That unique functional index was already created byAddSocialFoundation(2026-06-27); #338 re-created it. AddsIF NOT EXISTSso the migration applies as a safe no-op and records itself, recovering the pipeline. Root-caused via Render deploy logs. Refs #243.