Repository navigation
migrate does not ADD a foreign key to a pre-existing table (only creates FKs at table-creation time) #79
Description
Summary
DataProviderMigrate migrate does not emit ALTER TABLE ... ADD CONSTRAINT for a foreign key added to a table that already exists in the target DB. It creates the new column but silently skips the FK constraint. New tables get their FKs correctly at creation time; only the "add a FK to a pre-existing table" path is missing.
The bug is masked because the apply phase reports success — it is the post-apply integrity check that then fails, leaving the operator with a half-applied schema and no automated way to converge it.
Reproduction
- Have a table
conversationsalready present in the DB (created by an earlier migrate). - In
schema.yaml, add a new nullable column to it with a foreign key:
- name: end_user_id
type: Uuid # nullable
foreignKeys:
- columns: [end_user_id]
referencedTable: tenant_end_users # a NEW table created in the same run
referencedColumns: [id]
onDelete: Cascade- Run
DataProviderMigrate migrate --schema schema.yaml --provider postgres.
Actual
- Apply phase succeeds (creates
tenant_end_users, addsconversations.end_user_id, applies grants/RLS — 53 ops here). - Post-apply integrity check FAILS:
SCHEMA INTEGRITY CHECK FAILED public.conversations: missing foreign key FK_conversations_end_user_id on (end_user_id) - Re-running migrate reports "Schema is up to date — no operations needed" and still fails the same integrity check. So the apply planner never diffs/produces the missing FK on the existing table — it is not a transient ordering issue.
Expected
The planner emits ALTER TABLE public.conversations ADD CONSTRAINT "FK_conversations_end_user_id" FOREIGN KEY (end_user_id) REFERENCES public.tenant_end_users(id) ON DELETE CASCADE; so a re-run converges to zero drift.
Impact
Any FK added to an existing table never lands via the declarative pipeline. This breaks the "schema.yaml is the single source of truth, re-running is idempotent" contract for the common case of evolving an existing table. The only remediation is a manual ALTER TABLE ... ADD CONSTRAINT out of band, which defeats the declarative model.
Contrast with the FK-at-creation path, which works: tenant_end_users(tenant_id) -> tenants(id) (created in the same run) was applied correctly. The gap is specifically ADD FK on an ALTER of a pre-existing table.
Related secondary finding (integrity-check normalization, lower severity)
The same integrity check also reports a false positive on string-literal column defaults — it does not normalize Postgres's implicit ::text cast:
public.topup.status: default expected 'pending' but found 'pending'::text
'pending' and 'pending'::text are the same default; the check should normalize the cast before comparing (same class of round-trip mismatch as the filtered/expression-index issue in #65). Happy to split this into its own issue if preferred.
Environment
- DataProvider 0.9.12-beta line (same as the gaps tracked in
Nimblesite/NimblesiteAgenticPlatformdocs/specs/deployment.md/docs/plans/authorization.md). - Postgres (Supabase),
--provider postgres.
Activity
MelbourneDeveloper commentedon Jul 2, 2026
Severity escalation: this gap just cost NAP prod every single foreign key.
Sequence (2026-07-02):
migrate --allow-destructivefor the [AUTHZ-ONE-ROLE] policy cut.FK_<table>_<column>naming) and dropped all of them — 27 FKs across 17 tables.Result: NAP production currently has zero FK constraints (
select count(*) from pg_constraint where contype='f' and connamespace='public'::regnamespace→ 0). Referential integrity and everyonDelete: Cascadebehavior (tenant deletion cascade!) are silently gone. Data is still clean (0 orphan rows verified), so re-adding will validate — but only DataProvider is allowed to do it.Two asks:
Related: destructive mode also left
app_admin's table grants behind after the role was removed from the yaml (grants on 17 tables still present; role is NOLOGIN + policy-less so it's inert, but it's the same "declared state not converged" class — happy to file separately if you want it tracked on its own).MelbourneDeveloper commentedon Sep 30, 2026
NAP integration review: the current tests cover both adding an FK-bearing column and repairing a database where AddColumn was already applied. That directly addresses our upgrade failure:
processed_stripe_events.tenant_idis present but its declared FK totenants(id)is missing.The inspector change preserving composite-column order and referenced schema is useful. Please exercise it with real composite/cross-schema FKs, same-named constraints on different tables, and actual orphan-write rejection after migration. Retain named and unnamed FKs through repeated additive/destructive runs, including when the referenced table is introduced in the same upgrade. See #105 for the cleanup acceptance gate.
This is review feedback on the current local patch, not a claim that its tests have passed. #79 and #80 should share the regression coverage and release fix.
MelbourneDeveloper commentedon Sep 30, 2026
Second NAP review, 2026-09-30: local branch
fix/nap-migration-blockers, HEAD7f514faplus working changes; source review only.The new YAML/CLI regression now covers a referenced parent introduced in the upgrade, a newly added referencing column, and repair when the column already exists without its FK. Deferring existing-table FK additions until the referenced structure exists is the right ordering.
The inspector rewrite needs stronger regression coverage before release. The current added schemas use single-column, same-schema FKs, and
WithSchemarewrites every reference to the target's schema. That suite cannot exercise the new cross-schema/composite inspection logic. Add real FKs with ordered composite columns, references to a different schema, and identical constraint names on different tables; verify the inspected relationships and an unchanged diff, then prove orphan writes fail after upgrade and rerun.Also assert the second CLI migration plans no operations: catalog equality alone can pass if constraints are repeatedly dropped/recreated. NAP's concrete missing relationship remains
processed_stripe_events.tenant_id -> tenants(id). Share the fix/release coverage with #80 and the preservation checks with #105. See the new #109 comment for the current SQL Server test/CLI capability mismatch.