From 9675de1a8a925a8cfa1a1cba892cdd87c31df64b Mon Sep 17 00:00:00 2001 From: Mees van Dongen Date: Fri, 12 Jun 2026 14:21:15 +0200 Subject: [PATCH] fix: order modified foreign keys after added unique constraints --- internal/diff/topological.go | 30 +++++++++------ internal/diff/topological_test.go | 37 ++++++++++++++++++ .../issue_modified_fk_to_new_unique/diff.sql | 9 +++++ .../issue_modified_fk_to_new_unique/new.sql | 20 ++++++++++ .../issue_modified_fk_to_new_unique/old.sql | 19 ++++++++++ .../issue_modified_fk_to_new_unique/plan.json | 38 +++++++++++++++++++ .../issue_modified_fk_to_new_unique/plan.sql | 9 +++++ .../issue_modified_fk_to_new_unique/plan.txt | 24 ++++++++++++ .../diff.sql | 9 +++++ .../new.sql | 20 ++++++++++ .../old.sql | 19 ++++++++++ .../plan.json | 38 +++++++++++++++++++ .../plan.sql | 9 +++++ .../plan.txt | 24 ++++++++++++ 14 files changed, 293 insertions(+), 12 deletions(-) create mode 100644 testdata/diff/dependency/issue_modified_fk_to_new_unique/diff.sql create mode 100644 testdata/diff/dependency/issue_modified_fk_to_new_unique/new.sql create mode 100644 testdata/diff/dependency/issue_modified_fk_to_new_unique/old.sql create mode 100644 testdata/diff/dependency/issue_modified_fk_to_new_unique/plan.json create mode 100644 testdata/diff/dependency/issue_modified_fk_to_new_unique/plan.sql create mode 100644 testdata/diff/dependency/issue_modified_fk_to_new_unique/plan.txt create mode 100644 testdata/diff/dependency/issue_modified_fk_to_new_unique_reverse_order/diff.sql create mode 100644 testdata/diff/dependency/issue_modified_fk_to_new_unique_reverse_order/new.sql create mode 100644 testdata/diff/dependency/issue_modified_fk_to_new_unique_reverse_order/old.sql create mode 100644 testdata/diff/dependency/issue_modified_fk_to_new_unique_reverse_order/plan.json create mode 100644 testdata/diff/dependency/issue_modified_fk_to_new_unique_reverse_order/plan.sql create mode 100644 testdata/diff/dependency/issue_modified_fk_to_new_unique_reverse_order/plan.txt diff --git a/internal/diff/topological.go b/internal/diff/topological.go index 3602b5f0..4f7163e4 100644 --- a/internal/diff/topological.go +++ b/internal/diff/topological.go @@ -512,9 +512,9 @@ func topologicallySortFunctions(functions []*ir.Function) []*ir.Function { return sortedFunctions } -// topologicallySortModifiedTables sorts modified tables based on constraint dependencies -// Tables with added UNIQUE/PK constraints that are referenced by other tables' added FKs -// will come before those tables +// topologicallySortModifiedTables sorts modified tables based on constraint dependencies. +// Tables with added UNIQUE/PK constraints that are referenced by other tables' added OR +// modified FKs will come before those tables. func topologicallySortModifiedTables(tableDiffs []*tableDiff) []*tableDiff { if len(tableDiffs) <= 1 { return tableDiffs @@ -539,12 +539,12 @@ func topologicallySortModifiedTables(tableDiffs []*tableDiff) []*tableDiff { adjList[key] = []string{} } - // Build edges: if tableA adds a FK to tableB's newly-added UNIQUE/PK, add edge tableB -> tableA + // Build edges: if tableA adds or modifies a FK to tableB's newly-added UNIQUE/PK, + // add edge tableB -> tableA. for keyA, tdA := range tableDiffMap { - // Look at FK constraints being added to tableA - for _, fkConstraint := range tdA.AddedConstraints { - if fkConstraint.Type != ir.ConstraintTypeForeignKey { - continue + processFKDependency := func(fkConstraint *ir.Constraint) { + if fkConstraint == nil || fkConstraint.Type != ir.ConstraintTypeForeignKey { + return } // Build referenced table key @@ -557,7 +557,7 @@ func topologicallySortModifiedTables(tableDiffs []*tableDiff) []*tableDiff { // Check if referenced table exists in our modified tables set tdB, exists := tableDiffMap[keyB] if !exists || keyA == keyB { - continue + return } // Check if tableB is adding a UNIQUE or PK constraint that matches the FK reference @@ -566,15 +566,21 @@ func topologicallySortModifiedTables(tableDiffs []*tableDiff) []*tableDiff { continue } - // Check if this constraint matches the FK's referenced columns if constraintMatchesFKReference(constraint, fkConstraint) { - // Add edge: tableB (with new UNIQUE/PK) -> tableA (with new FK) adjList[keyB] = append(adjList[keyB], keyA) inDegree[keyA]++ - break + return } } } + + for _, fkConstraint := range tdA.AddedConstraints { + processFKDependency(fkConstraint) + } + + for _, constraintDiff := range tdA.ModifiedConstraints { + processFKDependency(constraintDiff.New) + } } // Kahn's algorithm with deterministic cycle breaking diff --git a/internal/diff/topological_test.go b/internal/diff/topological_test.go index adc145a0..52791108 100644 --- a/internal/diff/topological_test.go +++ b/internal/diff/topological_test.go @@ -439,3 +439,40 @@ func TestBuildFunctionBodyDependenciesWithTopologicalSort(t *testing.T) { t.Errorf("expected a_helper before z_wrapper, got order: %v", order) } } + +func TestTopologicallySortModifiedTables_ModifiedForeignKeyDependsOnAddedUnique(t *testing.T) { + referenced := &tableDiff{ + Table: &ir.Table{Schema: "public", Name: "organization_role"}, + AddedConstraints: []*ir.Constraint{{ + Name: "organization_role_organization_id_id_key", + Type: ir.ConstraintTypeUnique, + Columns: []*ir.ConstraintColumn{ + {Name: "organization_id", Position: 1}, + {Name: "id", Position: 2}, + }, + }}, + } + + dependent := &tableDiff{ + Table: &ir.Table{Schema: "public", Name: "member_role"}, + ModifiedConstraints: []*ConstraintDiff{{ + New: &ir.Constraint{ + Name: "member_role_organization_id_role_id_fkey", + Type: ir.ConstraintTypeForeignKey, + ReferencedTable: "organization_role", + ReferencedColumns: []*ir.ConstraintColumn{ + {Name: "organization_id", Position: 1}, + {Name: "id", Position: 2}, + }, + }, + }}, + } + + sorted := topologicallySortModifiedTables([]*tableDiff{dependent, referenced}) + if len(sorted) != 2 { + t.Fatalf("expected 2 modified tables, got %d", len(sorted)) + } + if sorted[0].Table.Name != "organization_role" || sorted[1].Table.Name != "member_role" { + t.Fatalf("expected referenced table before dependent modified FK table, got [%s %s]", sorted[0].Table.Name, sorted[1].Table.Name) + } +} diff --git a/testdata/diff/dependency/issue_modified_fk_to_new_unique/diff.sql b/testdata/diff/dependency/issue_modified_fk_to_new_unique/diff.sql new file mode 100644 index 00000000..b0a99136 --- /dev/null +++ b/testdata/diff/dependency/issue_modified_fk_to_new_unique/diff.sql @@ -0,0 +1,9 @@ +ALTER TABLE parent_variants +ADD CONSTRAINT parent_variants_parent_entity_id_id_key UNIQUE (parent_entity_id, id); + +ALTER TABLE child_links DROP CONSTRAINT child_links_parent_variant_fkey; + +ALTER TABLE child_links +ADD CONSTRAINT child_links_parent_variant_fkey FOREIGN KEY (parent_entity_id, parent_variant_id) REFERENCES parent_variants (parent_entity_id, id) ON DELETE CASCADE NOT VALID; + +ALTER TABLE child_links VALIDATE CONSTRAINT child_links_parent_variant_fkey; diff --git a/testdata/diff/dependency/issue_modified_fk_to_new_unique/new.sql b/testdata/diff/dependency/issue_modified_fk_to_new_unique/new.sql new file mode 100644 index 00000000..e1729078 --- /dev/null +++ b/testdata/diff/dependency/issue_modified_fk_to_new_unique/new.sql @@ -0,0 +1,20 @@ +CREATE TABLE parent_entities ( + id integer PRIMARY KEY +); + +CREATE TABLE parent_variants ( + id integer PRIMARY KEY, + parent_entity_id integer NOT NULL, + CONSTRAINT parent_variants_parent_entity_id_fkey FOREIGN KEY (parent_entity_id) + REFERENCES parent_entities (id) + ON DELETE CASCADE, + CONSTRAINT parent_variants_parent_entity_id_id_key UNIQUE (parent_entity_id, id) +); + +CREATE TABLE child_links ( + parent_variant_id integer NOT NULL, + parent_entity_id integer NOT NULL, + CONSTRAINT child_links_parent_variant_fkey FOREIGN KEY (parent_entity_id, parent_variant_id) + REFERENCES parent_variants (parent_entity_id, id) + ON DELETE CASCADE +); diff --git a/testdata/diff/dependency/issue_modified_fk_to_new_unique/old.sql b/testdata/diff/dependency/issue_modified_fk_to_new_unique/old.sql new file mode 100644 index 00000000..500aed62 --- /dev/null +++ b/testdata/diff/dependency/issue_modified_fk_to_new_unique/old.sql @@ -0,0 +1,19 @@ +CREATE TABLE parent_entities ( + id integer PRIMARY KEY +); + +CREATE TABLE parent_variants ( + id integer PRIMARY KEY, + parent_entity_id integer NOT NULL, + CONSTRAINT parent_variants_parent_entity_id_fkey FOREIGN KEY (parent_entity_id) + REFERENCES parent_entities (id) + ON DELETE CASCADE +); + +CREATE TABLE child_links ( + parent_variant_id integer NOT NULL, + parent_entity_id integer NOT NULL, + CONSTRAINT child_links_parent_variant_fkey FOREIGN KEY (parent_variant_id) + REFERENCES parent_variants (id) + ON DELETE CASCADE +); diff --git a/testdata/diff/dependency/issue_modified_fk_to_new_unique/plan.json b/testdata/diff/dependency/issue_modified_fk_to_new_unique/plan.json new file mode 100644 index 00000000..0fdc88ea --- /dev/null +++ b/testdata/diff/dependency/issue_modified_fk_to_new_unique/plan.json @@ -0,0 +1,38 @@ +{ + "version": "1.0.0", + "pgschema_version": "1.11.0", + "created_at": "1970-01-01T00:00:00Z", + "source_fingerprint": { + "hash": "cf492aa24c7a6ee879a81c2015c4b84a55981752bb21cc45ad7b02df4efe9f20" + }, + "groups": [ + { + "steps": [ + { + "sql": "ALTER TABLE parent_variants\nADD CONSTRAINT parent_variants_parent_entity_id_id_key UNIQUE (parent_entity_id, id);", + "type": "table.constraint", + "operation": "create", + "path": "public.parent_variants.parent_variants_parent_entity_id_id_key" + }, + { + "sql": "ALTER TABLE child_links DROP CONSTRAINT child_links_parent_variant_fkey;", + "type": "table.constraint", + "operation": "drop", + "path": "public.child_links.child_links_parent_variant_fkey" + }, + { + "sql": "ALTER TABLE child_links\nADD CONSTRAINT child_links_parent_variant_fkey FOREIGN KEY (parent_entity_id, parent_variant_id) REFERENCES parent_variants (parent_entity_id, id) ON DELETE CASCADE NOT VALID;", + "type": "table.constraint", + "operation": "create", + "path": "public.child_links.child_links_parent_variant_fkey" + }, + { + "sql": "ALTER TABLE child_links VALIDATE CONSTRAINT child_links_parent_variant_fkey;", + "type": "table.constraint", + "operation": "create", + "path": "public.child_links.child_links_parent_variant_fkey" + } + ] + } + ] +} diff --git a/testdata/diff/dependency/issue_modified_fk_to_new_unique/plan.sql b/testdata/diff/dependency/issue_modified_fk_to_new_unique/plan.sql new file mode 100644 index 00000000..b0a99136 --- /dev/null +++ b/testdata/diff/dependency/issue_modified_fk_to_new_unique/plan.sql @@ -0,0 +1,9 @@ +ALTER TABLE parent_variants +ADD CONSTRAINT parent_variants_parent_entity_id_id_key UNIQUE (parent_entity_id, id); + +ALTER TABLE child_links DROP CONSTRAINT child_links_parent_variant_fkey; + +ALTER TABLE child_links +ADD CONSTRAINT child_links_parent_variant_fkey FOREIGN KEY (parent_entity_id, parent_variant_id) REFERENCES parent_variants (parent_entity_id, id) ON DELETE CASCADE NOT VALID; + +ALTER TABLE child_links VALIDATE CONSTRAINT child_links_parent_variant_fkey; diff --git a/testdata/diff/dependency/issue_modified_fk_to_new_unique/plan.txt b/testdata/diff/dependency/issue_modified_fk_to_new_unique/plan.txt new file mode 100644 index 00000000..2fbe67c8 --- /dev/null +++ b/testdata/diff/dependency/issue_modified_fk_to_new_unique/plan.txt @@ -0,0 +1,24 @@ +Plan: 2 to modify. + +Summary by type: + tables: 2 to modify + +Tables: + ~ child_links + - child_links_parent_variant_fkey (constraint) + + child_links_parent_variant_fkey (constraint) + ~ parent_variants + + parent_variants_parent_entity_id_id_key (constraint) + +DDL to be executed: +-------------------------------------------------- + +ALTER TABLE parent_variants +ADD CONSTRAINT parent_variants_parent_entity_id_id_key UNIQUE (parent_entity_id, id); + +ALTER TABLE child_links DROP CONSTRAINT child_links_parent_variant_fkey; + +ALTER TABLE child_links +ADD CONSTRAINT child_links_parent_variant_fkey FOREIGN KEY (parent_entity_id, parent_variant_id) REFERENCES parent_variants (parent_entity_id, id) ON DELETE CASCADE NOT VALID; + +ALTER TABLE child_links VALIDATE CONSTRAINT child_links_parent_variant_fkey; diff --git a/testdata/diff/dependency/issue_modified_fk_to_new_unique_reverse_order/diff.sql b/testdata/diff/dependency/issue_modified_fk_to_new_unique_reverse_order/diff.sql new file mode 100644 index 00000000..4aa320df --- /dev/null +++ b/testdata/diff/dependency/issue_modified_fk_to_new_unique_reverse_order/diff.sql @@ -0,0 +1,9 @@ +ALTER TABLE z_parent_variants +ADD CONSTRAINT z_parent_variants_parent_entity_id_id_key UNIQUE (parent_entity_id, id); + +ALTER TABLE a_child_links DROP CONSTRAINT a_child_links_parent_variant_fkey; + +ALTER TABLE a_child_links +ADD CONSTRAINT a_child_links_parent_variant_fkey FOREIGN KEY (parent_entity_id, parent_variant_id) REFERENCES z_parent_variants (parent_entity_id, id) ON DELETE CASCADE NOT VALID; + +ALTER TABLE a_child_links VALIDATE CONSTRAINT a_child_links_parent_variant_fkey; diff --git a/testdata/diff/dependency/issue_modified_fk_to_new_unique_reverse_order/new.sql b/testdata/diff/dependency/issue_modified_fk_to_new_unique_reverse_order/new.sql new file mode 100644 index 00000000..7a86e777 --- /dev/null +++ b/testdata/diff/dependency/issue_modified_fk_to_new_unique_reverse_order/new.sql @@ -0,0 +1,20 @@ +CREATE TABLE z_parent_entities ( + id integer PRIMARY KEY +); + +CREATE TABLE z_parent_variants ( + id integer PRIMARY KEY, + parent_entity_id integer NOT NULL, + CONSTRAINT z_parent_variants_parent_entity_id_fkey FOREIGN KEY (parent_entity_id) + REFERENCES z_parent_entities (id) + ON DELETE CASCADE, + CONSTRAINT z_parent_variants_parent_entity_id_id_key UNIQUE (parent_entity_id, id) +); + +CREATE TABLE a_child_links ( + parent_variant_id integer NOT NULL, + parent_entity_id integer NOT NULL, + CONSTRAINT a_child_links_parent_variant_fkey FOREIGN KEY (parent_entity_id, parent_variant_id) + REFERENCES z_parent_variants (parent_entity_id, id) + ON DELETE CASCADE +); diff --git a/testdata/diff/dependency/issue_modified_fk_to_new_unique_reverse_order/old.sql b/testdata/diff/dependency/issue_modified_fk_to_new_unique_reverse_order/old.sql new file mode 100644 index 00000000..bf8bad13 --- /dev/null +++ b/testdata/diff/dependency/issue_modified_fk_to_new_unique_reverse_order/old.sql @@ -0,0 +1,19 @@ +CREATE TABLE z_parent_entities ( + id integer PRIMARY KEY +); + +CREATE TABLE z_parent_variants ( + id integer PRIMARY KEY, + parent_entity_id integer NOT NULL, + CONSTRAINT z_parent_variants_parent_entity_id_fkey FOREIGN KEY (parent_entity_id) + REFERENCES z_parent_entities (id) + ON DELETE CASCADE +); + +CREATE TABLE a_child_links ( + parent_variant_id integer NOT NULL, + parent_entity_id integer NOT NULL, + CONSTRAINT a_child_links_parent_variant_fkey FOREIGN KEY (parent_variant_id) + REFERENCES z_parent_variants (id) + ON DELETE CASCADE +); diff --git a/testdata/diff/dependency/issue_modified_fk_to_new_unique_reverse_order/plan.json b/testdata/diff/dependency/issue_modified_fk_to_new_unique_reverse_order/plan.json new file mode 100644 index 00000000..6240bd1f --- /dev/null +++ b/testdata/diff/dependency/issue_modified_fk_to_new_unique_reverse_order/plan.json @@ -0,0 +1,38 @@ +{ + "version": "1.0.0", + "pgschema_version": "1.11.0", + "created_at": "1970-01-01T00:00:00Z", + "source_fingerprint": { + "hash": "e0b0e7a0420b2e22309e50c6a79beb8a8467358c23e0c8b4fb4f6d30cb1de8f3" + }, + "groups": [ + { + "steps": [ + { + "sql": "ALTER TABLE z_parent_variants\nADD CONSTRAINT z_parent_variants_parent_entity_id_id_key UNIQUE (parent_entity_id, id);", + "type": "table.constraint", + "operation": "create", + "path": "public.z_parent_variants.z_parent_variants_parent_entity_id_id_key" + }, + { + "sql": "ALTER TABLE a_child_links DROP CONSTRAINT a_child_links_parent_variant_fkey;", + "type": "table.constraint", + "operation": "drop", + "path": "public.a_child_links.a_child_links_parent_variant_fkey" + }, + { + "sql": "ALTER TABLE a_child_links\nADD CONSTRAINT a_child_links_parent_variant_fkey FOREIGN KEY (parent_entity_id, parent_variant_id) REFERENCES z_parent_variants (parent_entity_id, id) ON DELETE CASCADE NOT VALID;", + "type": "table.constraint", + "operation": "create", + "path": "public.a_child_links.a_child_links_parent_variant_fkey" + }, + { + "sql": "ALTER TABLE a_child_links VALIDATE CONSTRAINT a_child_links_parent_variant_fkey;", + "type": "table.constraint", + "operation": "create", + "path": "public.a_child_links.a_child_links_parent_variant_fkey" + } + ] + } + ] +} diff --git a/testdata/diff/dependency/issue_modified_fk_to_new_unique_reverse_order/plan.sql b/testdata/diff/dependency/issue_modified_fk_to_new_unique_reverse_order/plan.sql new file mode 100644 index 00000000..4aa320df --- /dev/null +++ b/testdata/diff/dependency/issue_modified_fk_to_new_unique_reverse_order/plan.sql @@ -0,0 +1,9 @@ +ALTER TABLE z_parent_variants +ADD CONSTRAINT z_parent_variants_parent_entity_id_id_key UNIQUE (parent_entity_id, id); + +ALTER TABLE a_child_links DROP CONSTRAINT a_child_links_parent_variant_fkey; + +ALTER TABLE a_child_links +ADD CONSTRAINT a_child_links_parent_variant_fkey FOREIGN KEY (parent_entity_id, parent_variant_id) REFERENCES z_parent_variants (parent_entity_id, id) ON DELETE CASCADE NOT VALID; + +ALTER TABLE a_child_links VALIDATE CONSTRAINT a_child_links_parent_variant_fkey; diff --git a/testdata/diff/dependency/issue_modified_fk_to_new_unique_reverse_order/plan.txt b/testdata/diff/dependency/issue_modified_fk_to_new_unique_reverse_order/plan.txt new file mode 100644 index 00000000..88fd3708 --- /dev/null +++ b/testdata/diff/dependency/issue_modified_fk_to_new_unique_reverse_order/plan.txt @@ -0,0 +1,24 @@ +Plan: 2 to modify. + +Summary by type: + tables: 2 to modify + +Tables: + ~ a_child_links + - a_child_links_parent_variant_fkey (constraint) + + a_child_links_parent_variant_fkey (constraint) + ~ z_parent_variants + + z_parent_variants_parent_entity_id_id_key (constraint) + +DDL to be executed: +-------------------------------------------------- + +ALTER TABLE z_parent_variants +ADD CONSTRAINT z_parent_variants_parent_entity_id_id_key UNIQUE (parent_entity_id, id); + +ALTER TABLE a_child_links DROP CONSTRAINT a_child_links_parent_variant_fkey; + +ALTER TABLE a_child_links +ADD CONSTRAINT a_child_links_parent_variant_fkey FOREIGN KEY (parent_entity_id, parent_variant_id) REFERENCES z_parent_variants (parent_entity_id, id) ON DELETE CASCADE NOT VALID; + +ALTER TABLE a_child_links VALIDATE CONSTRAINT a_child_links_parent_variant_fkey;