fix: handle sqlite legacy budget column drops#2876
fix: handle sqlite legacy budget column drops#2876danpiths wants to merge 1 commit intographite-base/2876from
Conversation
|
|
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdded SQLite-aware migration logic to remove legacy Changes
Sequence DiagramsequenceDiagram
participant Migr as Migration
participant SQLite as SQLiteDB
participant GORM as GORMMigrator
participant Dump as DumpTable
Migr->>SQLite: PRAGMA table_info(table)\n(check for budget_id)
SQLite-->>Migr: column metadata
alt budget_id exists (SQLite path)
Migr->>SQLite: CREATE TABLE __dump ... (exclude budget_id)
Migr->>SQLite: INSERT INTO __dump SELECT ... (non-budget_id cols)
SQLite-->>Migr: data copied
Migr->>SQLite: DROP TABLE original
Migr->>SQLite: CREATE TABLE original (from GORM model)
Migr->>SQLite: INSERT INTO original SELECT ... FROM __dump
Migr->>SQLite: DROP TABLE __dump
Migr->>SQLite: PRAGMA table_info(original)\n(verify budget_id gone)
SQLite-->>Migr: verification result
else non-SQLite (GORM path)
Migr->>GORM: DropColumn(table, "budget_id")
GORM-->>Migr: operation result
Migr->>GORM: Verify column absent
GORM-->>Migr: verification result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 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 docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
Confidence Score: 5/5Safe to merge; the fix is well-reasoned, the PRAGMA/connection-pool approach is correct, and all remaining findings are minor style suggestions. All issues found are P2: a single-slash comment typo and missing idempotency test coverage for the new regression tests. The core logic — dump table strategy, PRAGMA foreign_keys OFF with SetMaxOpenConns(1), correct ordering of drops before CreateConstraint — is sound and well-tested. No files require special attention. Important Files Changed
Reviews (2): Last reviewed commit: "fix: handle sqlite legacy budget column ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@framework/configstore/migrations_test.go`:
- Around line 2091-2093: The test currently adds plain budget_id columns but
must model the legacy FK; change the three ALTER TABLE statements in
migrations_test.go to add budget_id VARCHAR(255) REFERENCES
governance_budgets(id) so the fixture mirrors the broken production schema, then
after running the migration query PRAGMA foreign_key_list for each table (use a
small fkRow struct with fields Table/From/To), e.g. run db.Raw(`PRAGMA
foreign_key_list(governance_virtual_keys)`).Scan(&fks) and assert the result
contains fkRow{Table: "governance_budgets", From: "budget_id", To: "id"} (repeat
for the other two tables) to validate post-migration FK metadata still points at
governance_budgets.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e4afafd4-ef4b-435b-b6f7-490d2f62cbbe
📒 Files selected for processing (2)
framework/configstore/migrations.goframework/configstore/migrations_test.go
danpiths
force-pushed
the
fix/04-21-fix_handle_sqlite_legacy_budget_column_drops
branch
from
535e6c7 to
7df8625
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
framework/configstore/migrations.go (2)
6319-6351:⚠️ Potential issue | 🟠 MajorAdd preflight check for duplicate team references to the same budget before backfill.
The scalar subquery at line 6344 (
SELECT id FROM governance_teams WHERE governance_teams.budget_id = governance_budgets.id) assumes each budget is referenced by at most one team. If multiple teams reference the same budget in legacy data, PostgreSQL will fail the migration with a "more than one row" error, and SQLite will silently pick one team, losing the other associations when thebudget_idcolumn is dropped. Add a GROUP BY/HAVING preflight check alongside the existing cross-owner check:Suggested preflight query
SELECT COUNT(*) FROM governance_teams t GROUP BY t.budget_id HAVING COUNT(*) > 1If duplicates exist, fail the migration with a clear error message requiring manual resolution.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/migrations.go` around lines 6319 - 6351, Add a preflight duplicate-team-per-budget check before the UPDATE backfill: run a grouped query against governance_teams (GROUP BY budget_id HAVING COUNT(*) > 1) using tx.Raw (similar to the existing conflictCount query), scan the result into a count/variable and, if >0, return an error that clearly states duplicate team references to the same budget exist and must be resolved manually; place this check before the tx.Exec(...) UPDATE that uses the scalar subquery so the migration fails fast instead of producing a SQL error or silently losing associations.
6172-6200:⚠️ Potential issue | 🟠 MajorAdd duplicate-reference preflights to all three legacy budget_id backfills.
Lines 6174 (VirtualKey), 6189 (ProviderConfig), and 6350 (Team) use scalar subqueries that assume each legacy
budget_idmaps to exactly one owner. If stale data has the same budget referenced by multiple VirtualKeys, multiple ProviderConfigs, or multiple Teams—or by any combination—PostgreSQL will fail with "more than one row returned by a subquery" while SQLite may silently pick one row, leavinggovernance_budgetsrows with arbitrary or missing ownership after the legacy columns drop.Add a preflight for each that checks for duplicates within the same table (
GROUP BY budget_id HAVING COUNT(*) > 1) and aborts before the backfill runs. The team migration already has a cross-owner conflict check (line 6330); extend it with a same-owner duplicate check. Apply the same pattern to VirtualKey and ProviderConfig backfills.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/migrations.go` around lines 6172 - 6200, The three legacy budget_id backfills (the tx.Exec blocks that update governance_budgets using scalar subqueries over governance_virtual_keys, governance_virtual_key_provider_configs, and governance_teams) must each run a preflight duplicate check to avoid "more than one row returned" errors: before running the UPDATE, run a query against the source table (for VirtualKey: governance_virtual_keys / TableVirtualKey; for ProviderConfig: governance_virtual_key_provider_configs / TableVirtualKeyProviderConfig; for Team: governance_teams) that SELECTs budget_id GROUP BY budget_id HAVING COUNT(*) > 1 and if any rows are returned abort the migration with a clear error message; also extend the existing team preflight (the cross-owner conflict check around the team backfill) to include this same-table duplicate check so same-owner duplicates are caught. Ensure the checks run only when the legacy budget_id column exists (same mg.HasColumn guards) and return a descriptive fmt.Errorf if duplicates are found.
🧹 Nitpick comments (1)
framework/configstore/migrations_test.go (1)
2280-2337: Consider adding a brief comment noting these tests are SQLite-specific.Both migration tests run exclusively against SQLite (via
setupLegacyBudgetOwnerMigrationDB). While this is appropriate given the bug is SQLite-specific and Postgres uses GORM's nativeDropColumn, a brief comment would help future maintainers understand the scope.📝 Suggested documentation
+// TestMigrationAddMultiBudgetTables_DropsLegacyBudgetColumnsAndBackfillsOwners +// exercises the SQLite-specific table rebuild path for dropping legacy budget_id +// columns. Postgres uses GORM's native DropColumn and is covered by existing tests. func TestMigrationAddMultiBudgetTables_DropsLegacyBudgetColumnsAndBackfillsOwners(t *testing.T) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/configstore/migrations_test.go` around lines 2280 - 2337, The two migration tests TestMigrationAddMultiBudgetTables_DropsLegacyBudgetColumnsAndBackfillsOwners and TestMigrationAddTeamBudgetsToBudgetsTable_DropsLegacyBudgetColumnAndBackfillsOwners run against a SQLite-specific test DB created by setupLegacyBudgetOwnerMigrationDB; add a one-line comment above each test (or above the shared setup call) stating that these tests target SQLite only because the bug is SQLite rename/DropColumn-specific and Postgres uses GORM's native DropColumn behavior, so maintainers understand the DB scope when reading these tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@framework/configstore/migrations.go`:
- Around line 6319-6351: Add a preflight duplicate-team-per-budget check before
the UPDATE backfill: run a grouped query against governance_teams (GROUP BY
budget_id HAVING COUNT(*) > 1) using tx.Raw (similar to the existing
conflictCount query), scan the result into a count/variable and, if >0, return
an error that clearly states duplicate team references to the same budget exist
and must be resolved manually; place this check before the tx.Exec(...) UPDATE
that uses the scalar subquery so the migration fails fast instead of producing a
SQL error or silently losing associations.
- Around line 6172-6200: The three legacy budget_id backfills (the tx.Exec
blocks that update governance_budgets using scalar subqueries over
governance_virtual_keys, governance_virtual_key_provider_configs, and
governance_teams) must each run a preflight duplicate check to avoid "more than
one row returned" errors: before running the UPDATE, run a query against the
source table (for VirtualKey: governance_virtual_keys / TableVirtualKey; for
ProviderConfig: governance_virtual_key_provider_configs /
TableVirtualKeyProviderConfig; for Team: governance_teams) that SELECTs
budget_id GROUP BY budget_id HAVING COUNT(*) > 1 and if any rows are returned
abort the migration with a clear error message; also extend the existing team
preflight (the cross-owner conflict check around the team backfill) to include
this same-table duplicate check so same-owner duplicates are caught. Ensure the
checks run only when the legacy budget_id column exists (same mg.HasColumn
guards) and return a descriptive fmt.Errorf if duplicates are found.
---
Nitpick comments:
In `@framework/configstore/migrations_test.go`:
- Around line 2280-2337: The two migration tests
TestMigrationAddMultiBudgetTables_DropsLegacyBudgetColumnsAndBackfillsOwners and
TestMigrationAddTeamBudgetsToBudgetsTable_DropsLegacyBudgetColumnAndBackfillsOwners
run against a SQLite-specific test DB created by
setupLegacyBudgetOwnerMigrationDB; add a one-line comment above each test (or
above the shared setup call) stating that these tests target SQLite only because
the bug is SQLite rename/DropColumn-specific and Postgres uses GORM's native
DropColumn behavior, so maintainers understand the DB scope when reading these
tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9be99ad3-94d7-46d2-b8c5-5457a46b26ae
📒 Files selected for processing (2)
framework/configstore/migrations.goframework/configstore/migrations_test.go
akshaydeo
changed the base branch from
04-21-v1.5.0-prerelease4_cut
to
graphite-base/2876
3 participants
Summary
Fixes SQLite startup failures and runtime 500s caused by legacy
budget_idcolumns lingering on
governance_virtual_keys,governance_virtual_key_provider_configs, andgovernance_teamsafter themulti-budget ownership migration.
The V1.5.0 migrations (
add_multi_budget_tables,add_team_budgets_to_budgets_table) moved budget ownership from single-FKcolumns (
budget_id) on parent tables to multi-budget ownership viagovernance_budgets.{virtual_key_id, provider_config_id, team_id}. However, thelegacy
budget_idcolumns were dropped with a fire-and-forgetALTER TABLE ... DROP COLUMN IF EXISTSthat silently failed on SQLite when thecolumn was referenced by a foreign key definition. This left stale columns and
FK constraints in the schema, causing
FOREIGN KEY constraint failederrors atrequest time when budgets were reconciled during team/VK updates.
Changes
sqliteDropLegacyBudgetColumn):Uses a dump-data / drop-original / create-clean / restore-data strategy to
remove
budget_idwithout ever renaming the original table. This avoids aSQLite behavior where
ALTER TABLE RENAMEpropagates into FK references inother tables, corrupting them.
dropLegacyBudgetColumn): Routes to theSQLite rebuild path or standard GORM
DropColumnfor Postgres. Verifies thecolumn is actually gone afterward and fails the migration if not.
constraint creation (
CreateConstraint). On SQLite,CreateConstrainttriggers internal table rebuilds; doing it after the legacy column is gone
avoids FK reference corruption from rename propagation.
_ = tx.Exec("ALTER TABLE ... DROP COLUMN IF EXISTS budget_id")calls arereplaced with
dropLegacyBudgetColumn(tx, tableName)which fails themigration on error instead of silently swallowing it.
(
TestMigrationAddMultiBudgetTables_DropsLegacyBudgetColumnsAndBackfillsOwners,TestMigrationAddTeamBudgetsToBudgetsTable_DropsLegacyBudgetColumnAndBackfillsOwners)simulate legacy DBs with
budget_idcolumns still present and verify both thecolumn drop and the ownership backfill.
setupLegacyBudgetOwnerMigrationDB,insertProviderConfigRaw,insertTeamRawfor constructing legacy-shaped testdatabases.
Type of change
Affected areas
How to test
Manual validation with a pre-V1.5.0 SQLite database:
config.dbbackup from before the multi-budget migrationshould succeed without FK errors
Screenshots/Recordings
N/A — backend-only migration fix.
Breaking changes
Related issues
N/A
Security considerations
None. This change only affects database schema migration logic during startup.
No auth, secrets, PII, or sandboxing changes.
Checklist
docs/contributing/README.mdand followed the guidelines