fix: fixes customer FK column issue - #4292
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds a startup migration that, when the legacy ChangesLegacy Budget ID Column Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/configstore/migrations.go`:
- Around line 9813-9829: After the defensive backfill that sets
governance_budgets.customer_id from governance_customers.budget_id and clears
governance_customers.budget_id, recompute and persist the customers' config hash
so config_hash stays in sync with GenerateCustomerHash: identify the affected
governance_customers rows (those whose budget_id was non-NULL / whose id was
used to set governance_budgets.customer_id), call GenerateCustomerHash for each
affected customer and update governance_customers.config_hash accordingly
(perform the selection before clearing budget_id or capture affected IDs in the
same transaction), and ensure these updates occur inside the same migration
transaction so migration/runtime hash generation parity is preserved.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 06530fa2-b2ad-4fe3-ba14-ae5931bcbd7a
📒 Files selected for processing (1)
framework/configstore/migrations.go
Confidence Score: 5/5The migration correctly resolves the FK violation without dropping the column or constraint, and all three steps operate on the right rows. The backfill uses EXISTS + correlated subquery consistently, the hash refresh targets only customers whose budget ownership actually changes, and clearing the legacy budget_id column does not affect config_hash because BudgetID is gorm:"-" in the Go struct. No locking, deadlock, or data-loss risks were identified. framework/configstore/migrations.go — the new migration function is untested; adding a test covering the three key scenarios would close the remaining gap. Important Files Changed
Reviews (3): Last reviewed commit: "fix: fixes customer FK column issue" | Re-trigger Greptile |
330a1ee to
066504c
Compare
Merge activity
|
066504c to
8865f7c
Compare
## Summary Fixes a bug where `DeleteCustomer` fails with a foreign key constraint violation when attempting to `DELETE FROM governance_budgets WHERE customer_id = ?`. The root cause is that `governance_customers.budget_id` rows were left populated by a previous migration (`migrationAddCustomerBudgetsToBudgetsTable`), causing the FK check (`fk_governance_customers_budget`) to block deletion of the referenced budget rows. Since budget ownership now lives on `governance_budgets.customer_id`, the legacy `budget_id` values on `governance_customers` can be safely nulled — a `NULL` reference satisfies the FK unconditionally. ## Changes - Adds a new migration `migrationNullLegacyCustomerBudgetID` that: - Performs a defensive backfill to ensure any `governance_budgets` rows that still lack a `customer_id` (e.g. written by an older instance in a mixed-version cluster) are claimed before the legacy references are cleared. - Nulls all non-null `governance_customers.budget_id` values, resolving the FK conflict without dropping the column or constraint (deferred to a major release). - Includes a best-effort rollback that repopulates `budget_id` from `governance_budgets.customer_id`, picking the oldest budget for customers with multiple budgets. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./framework/configstore/... ``` 1. Ensure a customer with an associated budget exists in the database (with `governance_customers.budget_id` populated). 2. Run the migration and confirm `governance_customers.budget_id` is `NULL` for all rows. 3. Confirm `governance_budgets.customer_id` is correctly populated. 4. Attempt to delete the customer and verify no FK constraint error occurs. ## Breaking changes - [ ] Yes - [x] No The `governance_customers.budget_id` column and its FK are retained; only the values are cleared. ## Related issues Closes the FK violation bug introduced by the `migrationAddCustomerBudgetsToBudgetsTable` migration leaving legacy `budget_id` references intact. ## Security considerations None. This migration only modifies internal budget ownership references and does not affect auth, secrets, or PII. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Added a database migration to clean up legacy customer→budget links: claims unowned budgets safely, refreshes affected customer configuration, and clears deprecated legacy references to improve data consistency. * **Bug Fixes** * Improved rollback behavior to best-effort restore legacy customer→budget links when possible, preferring the oldest matching budget. <!-- end of auto-generated comment: release notes by coderabbit.ai -->

Summary
Fixes a bug where
DeleteCustomerfails with a foreign key constraint violation when attempting toDELETE FROM governance_budgets WHERE customer_id = ?. The root cause is thatgovernance_customers.budget_idrows were left populated by a previous migration (migrationAddCustomerBudgetsToBudgetsTable), causing the FK check (fk_governance_customers_budget) to block deletion of the referenced budget rows. Since budget ownership now lives ongovernance_budgets.customer_id, the legacybudget_idvalues ongovernance_customerscan be safely nulled — aNULLreference satisfies the FK unconditionally.Changes
migrationNullLegacyCustomerBudgetIDthat:governance_budgetsrows that still lack acustomer_id(e.g. written by an older instance in a mixed-version cluster) are claimed before the legacy references are cleared.governance_customers.budget_idvalues, resolving the FK conflict without dropping the column or constraint (deferred to a major release).budget_idfromgovernance_budgets.customer_id, picking the oldest budget for customers with multiple budgets.Type of change
Affected areas
How to test
go test ./framework/configstore/...governance_customers.budget_idpopulated).governance_customers.budget_idisNULLfor all rows.governance_budgets.customer_idis correctly populated.Breaking changes
The
governance_customers.budget_idcolumn and its FK are retained; only the values are cleared.Related issues
Closes the FK violation bug introduced by the
migrationAddCustomerBudgetsToBudgetsTablemigration leaving legacybudget_idreferences intact.Security considerations
None. This migration only modifies internal budget ownership references and does not affect auth, secrets, or PII.
Checklist
docs/contributing/README.mdand followed the guidelinesSummary by CodeRabbit