From 0e6d672528a7c202e2fbcb9155f1ddfc31e93538 Mon Sep 17 00:00:00 2001 From: Ryan Slade Date: Thu, 12 Dec 2024 17:51:45 +0100 Subject: [PATCH 1/7] First stab at adding foreign key constraints --- pkg/sql2pgroll/alter_table.go | 51 +++++++++++++ pkg/sql2pgroll/alter_table_test.go | 20 ++++++ pkg/sql2pgroll/expect/add_foreign_key.go | 92 ++++++++++++++++++++++++ 3 files changed, 163 insertions(+) create mode 100644 pkg/sql2pgroll/expect/add_foreign_key.go diff --git a/pkg/sql2pgroll/alter_table.go b/pkg/sql2pgroll/alter_table.go index 2d6b3d6b..241857c9 100644 --- a/pkg/sql2pgroll/alter_table.go +++ b/pkg/sql2pgroll/alter_table.go @@ -114,6 +114,8 @@ func convertAlterTableAddConstraint(stmt *pgq.AlterTableStmt, cmd *pgq.AlterTabl switch node.Constraint.GetContype() { case pgq.ConstrType_CONSTR_UNIQUE: op, err = convertAlterTableAddUniqueConstraint(stmt, node.Constraint) + case pgq.ConstrType_CONSTR_FOREIGN: + op, err = convertAlterTableAddForeignKeyConstraint(stmt, node.Constraint) default: return nil, nil } @@ -162,6 +164,55 @@ func convertAlterTableAddUniqueConstraint(stmt *pgq.AlterTableStmt, constraint * }, nil } +func convertAlterTableAddForeignKeyConstraint(stmt *pgq.AlterTableStmt, constraint *pgq.Constraint) (migrations.Operation, error) { + columns := make([]string, len(constraint.GetFkAttrs())) + for i := range columns { + columns[i] = constraint.GetFkAttrs()[i].GetString_().GetSval() + } + + foreignColumns := make([]string, len(constraint.GetPkAttrs())) + for i := range columns { + foreignColumns[i] = constraint.GetPkAttrs()[i].GetString_().GetSval() + } + + migs := make(map[string]string) + for _, column := range columns { + migs[column] = PlaceHolderSQL + } + + foreignTable := constraint.GetPktable().GetRelname() + + var onDelete migrations.ForeignKeyReferenceOnDelete + switch constraint.GetFkDelAction() { + case "a": + onDelete = migrations.ForeignKeyReferenceOnDeleteNOACTION + case "c": + onDelete = migrations.ForeignKeyReferenceOnDeleteCASCADE + case "r": + onDelete = migrations.ForeignKeyReferenceOnDeleteRESTRICT + case "d": + onDelete = migrations.ForeignKeyReferenceOnDeleteSETDEFAULT + case "n": + onDelete = migrations.ForeignKeyReferenceOnDeleteSETNULL + default: + return nil, fmt.Errorf("unknown delete action: %q", constraint.GetFkDelAction()) + } + + return &migrations.OpCreateConstraint{ + Columns: columns, + Up: migs, + Down: migs, + Name: constraint.GetConname(), + References: &migrations.OpCreateConstraintReferences{ + Columns: foreignColumns, + OnDelete: onDelete, + Table: foreignTable, + }, + Table: stmt.GetRelation().GetRelname(), + Type: migrations.OpCreateConstraintTypeForeignKey, + }, nil +} + // convertAlterTableSetColumnDefault converts SQL statements like: // // `ALTER TABLE foo COLUMN bar SET DEFAULT 'foo'` diff --git a/pkg/sql2pgroll/alter_table_test.go b/pkg/sql2pgroll/alter_table_test.go index 65c39ba8..9026e007 100644 --- a/pkg/sql2pgroll/alter_table_test.go +++ b/pkg/sql2pgroll/alter_table_test.go @@ -80,6 +80,26 @@ func TestConvertAlterTableStatements(t *testing.T) { sql: "ALTER TABLE foo DROP COLUMN bar RESTRICT ", expectedOp: expect.DropColumnOp1, }, + { + sql: "ALTER TABLE foo ADD CONSTRAINT fk_bar_cd FOREIGN KEY (a, b) REFERENCES bar (c, d);", + expectedOp: expect.AddForeignKeyOp1, + }, + { + sql: "ALTER TABLE foo ADD CONSTRAINT fk_bar_cd FOREIGN KEY (a, b) REFERENCES bar (c, d) ON DELETE NO ACTION;", + expectedOp: expect.AddForeignKeyOp1, + }, + { + sql: "ALTER TABLE foo ADD CONSTRAINT fk_bar_cd FOREIGN KEY (a, b) REFERENCES bar (c, d) ON DELETE RESTRICT;", + expectedOp: expect.AddForeignKeyOp2, + }, + { + sql: "ALTER TABLE foo ADD CONSTRAINT fk_bar_cd FOREIGN KEY (a, b) REFERENCES bar (c, d) ON DELETE SET DEFAULT ;", + expectedOp: expect.AddForeignKeyOp3, + }, + { + sql: "ALTER TABLE foo ADD CONSTRAINT fk_bar_cd FOREIGN KEY (a, b) REFERENCES bar (c, d) ON DELETE SET NULL;", + expectedOp: expect.AddForeignKeyOp4, + }, } for _, tc := range tests { diff --git a/pkg/sql2pgroll/expect/add_foreign_key.go b/pkg/sql2pgroll/expect/add_foreign_key.go new file mode 100644 index 00000000..4bdcda62 --- /dev/null +++ b/pkg/sql2pgroll/expect/add_foreign_key.go @@ -0,0 +1,92 @@ +// SPDX-License-Identifier: Apache-2.0 + +package expect + +import ( + "github.com/xataio/pgroll/pkg/migrations" + "github.com/xataio/pgroll/pkg/sql2pgroll" +) + +var AddForeignKeyOp1 = &migrations.OpCreateConstraint{ + Check: nil, + Columns: []string{"a", "b"}, + Name: "fk_bar_cd", + References: &migrations.OpCreateConstraintReferences{ + Columns: []string{"c", "d"}, + OnDelete: migrations.ForeignKeyReferenceOnDeleteNOACTION, + Table: "bar", + }, + Table: "foo", + Type: migrations.OpCreateConstraintTypeForeignKey, + Up: map[string]string{ + "a": sql2pgroll.PlaceHolderSQL, + "b": sql2pgroll.PlaceHolderSQL, + }, + Down: map[string]string{ + "a": sql2pgroll.PlaceHolderSQL, + "b": sql2pgroll.PlaceHolderSQL, + }, +} + +var AddForeignKeyOp2 = &migrations.OpCreateConstraint{ + Check: nil, + Columns: []string{"a", "b"}, + Name: "fk_bar_cd", + References: &migrations.OpCreateConstraintReferences{ + Columns: []string{"c", "d"}, + OnDelete: migrations.ForeignKeyReferenceOnDeleteRESTRICT, + Table: "bar", + }, + Table: "foo", + Type: migrations.OpCreateConstraintTypeForeignKey, + Up: map[string]string{ + "a": sql2pgroll.PlaceHolderSQL, + "b": sql2pgroll.PlaceHolderSQL, + }, + Down: map[string]string{ + "a": sql2pgroll.PlaceHolderSQL, + "b": sql2pgroll.PlaceHolderSQL, + }, +} + +var AddForeignKeyOp3 = &migrations.OpCreateConstraint{ + Check: nil, + Columns: []string{"a", "b"}, + Name: "fk_bar_cd", + References: &migrations.OpCreateConstraintReferences{ + Columns: []string{"c", "d"}, + OnDelete: migrations.ForeignKeyReferenceOnDeleteSETDEFAULT, + Table: "bar", + }, + Table: "foo", + Type: migrations.OpCreateConstraintTypeForeignKey, + Up: map[string]string{ + "a": sql2pgroll.PlaceHolderSQL, + "b": sql2pgroll.PlaceHolderSQL, + }, + Down: map[string]string{ + "a": sql2pgroll.PlaceHolderSQL, + "b": sql2pgroll.PlaceHolderSQL, + }, +} + +var AddForeignKeyOp4 = &migrations.OpCreateConstraint{ + Check: nil, + Columns: []string{"a", "b"}, + Name: "fk_bar_cd", + References: &migrations.OpCreateConstraintReferences{ + Columns: []string{"c", "d"}, + OnDelete: migrations.ForeignKeyReferenceOnDeleteSETNULL, + Table: "bar", + }, + Table: "foo", + Type: migrations.OpCreateConstraintTypeForeignKey, + Up: map[string]string{ + "a": sql2pgroll.PlaceHolderSQL, + "b": sql2pgroll.PlaceHolderSQL, + }, + Down: map[string]string{ + "a": sql2pgroll.PlaceHolderSQL, + "b": sql2pgroll.PlaceHolderSQL, + }, +} From 2a42df3a59dbd92b36ff06d626dfb5b137941a6b Mon Sep 17 00:00:00 2001 From: Ryan Slade Date: Fri, 13 Dec 2024 09:29:55 +0100 Subject: [PATCH 2/7] Factor out fixtures --- pkg/sql2pgroll/alter_table_test.go | 10 +-- pkg/sql2pgroll/expect/add_foreign_key.go | 103 +++++------------------ 2 files changed, 26 insertions(+), 87 deletions(-) diff --git a/pkg/sql2pgroll/alter_table_test.go b/pkg/sql2pgroll/alter_table_test.go index 9026e007..f4cfc76c 100644 --- a/pkg/sql2pgroll/alter_table_test.go +++ b/pkg/sql2pgroll/alter_table_test.go @@ -82,23 +82,23 @@ func TestConvertAlterTableStatements(t *testing.T) { }, { sql: "ALTER TABLE foo ADD CONSTRAINT fk_bar_cd FOREIGN KEY (a, b) REFERENCES bar (c, d);", - expectedOp: expect.AddForeignKeyOp1, + expectedOp: expect.AddForeignKeyOp1WithOnDelete(migrations.ForeignKeyReferenceOnDeleteNOACTION), }, { sql: "ALTER TABLE foo ADD CONSTRAINT fk_bar_cd FOREIGN KEY (a, b) REFERENCES bar (c, d) ON DELETE NO ACTION;", - expectedOp: expect.AddForeignKeyOp1, + expectedOp: expect.AddForeignKeyOp1WithOnDelete(migrations.ForeignKeyReferenceOnDeleteNOACTION), }, { sql: "ALTER TABLE foo ADD CONSTRAINT fk_bar_cd FOREIGN KEY (a, b) REFERENCES bar (c, d) ON DELETE RESTRICT;", - expectedOp: expect.AddForeignKeyOp2, + expectedOp: expect.AddForeignKeyOp1WithOnDelete(migrations.ForeignKeyReferenceOnDeleteRESTRICT), }, { sql: "ALTER TABLE foo ADD CONSTRAINT fk_bar_cd FOREIGN KEY (a, b) REFERENCES bar (c, d) ON DELETE SET DEFAULT ;", - expectedOp: expect.AddForeignKeyOp3, + expectedOp: expect.AddForeignKeyOp1WithOnDelete(migrations.ForeignKeyReferenceOnDeleteSETDEFAULT), }, { sql: "ALTER TABLE foo ADD CONSTRAINT fk_bar_cd FOREIGN KEY (a, b) REFERENCES bar (c, d) ON DELETE SET NULL;", - expectedOp: expect.AddForeignKeyOp4, + expectedOp: expect.AddForeignKeyOp1WithOnDelete(migrations.ForeignKeyReferenceOnDeleteSETNULL), }, } diff --git a/pkg/sql2pgroll/expect/add_foreign_key.go b/pkg/sql2pgroll/expect/add_foreign_key.go index 4bdcda62..b6604d12 100644 --- a/pkg/sql2pgroll/expect/add_foreign_key.go +++ b/pkg/sql2pgroll/expect/add_foreign_key.go @@ -7,86 +7,25 @@ import ( "github.com/xataio/pgroll/pkg/sql2pgroll" ) -var AddForeignKeyOp1 = &migrations.OpCreateConstraint{ - Check: nil, - Columns: []string{"a", "b"}, - Name: "fk_bar_cd", - References: &migrations.OpCreateConstraintReferences{ - Columns: []string{"c", "d"}, - OnDelete: migrations.ForeignKeyReferenceOnDeleteNOACTION, - Table: "bar", - }, - Table: "foo", - Type: migrations.OpCreateConstraintTypeForeignKey, - Up: map[string]string{ - "a": sql2pgroll.PlaceHolderSQL, - "b": sql2pgroll.PlaceHolderSQL, - }, - Down: map[string]string{ - "a": sql2pgroll.PlaceHolderSQL, - "b": sql2pgroll.PlaceHolderSQL, - }, -} - -var AddForeignKeyOp2 = &migrations.OpCreateConstraint{ - Check: nil, - Columns: []string{"a", "b"}, - Name: "fk_bar_cd", - References: &migrations.OpCreateConstraintReferences{ - Columns: []string{"c", "d"}, - OnDelete: migrations.ForeignKeyReferenceOnDeleteRESTRICT, - Table: "bar", - }, - Table: "foo", - Type: migrations.OpCreateConstraintTypeForeignKey, - Up: map[string]string{ - "a": sql2pgroll.PlaceHolderSQL, - "b": sql2pgroll.PlaceHolderSQL, - }, - Down: map[string]string{ - "a": sql2pgroll.PlaceHolderSQL, - "b": sql2pgroll.PlaceHolderSQL, - }, -} - -var AddForeignKeyOp3 = &migrations.OpCreateConstraint{ - Check: nil, - Columns: []string{"a", "b"}, - Name: "fk_bar_cd", - References: &migrations.OpCreateConstraintReferences{ - Columns: []string{"c", "d"}, - OnDelete: migrations.ForeignKeyReferenceOnDeleteSETDEFAULT, - Table: "bar", - }, - Table: "foo", - Type: migrations.OpCreateConstraintTypeForeignKey, - Up: map[string]string{ - "a": sql2pgroll.PlaceHolderSQL, - "b": sql2pgroll.PlaceHolderSQL, - }, - Down: map[string]string{ - "a": sql2pgroll.PlaceHolderSQL, - "b": sql2pgroll.PlaceHolderSQL, - }, -} - -var AddForeignKeyOp4 = &migrations.OpCreateConstraint{ - Check: nil, - Columns: []string{"a", "b"}, - Name: "fk_bar_cd", - References: &migrations.OpCreateConstraintReferences{ - Columns: []string{"c", "d"}, - OnDelete: migrations.ForeignKeyReferenceOnDeleteSETNULL, - Table: "bar", - }, - Table: "foo", - Type: migrations.OpCreateConstraintTypeForeignKey, - Up: map[string]string{ - "a": sql2pgroll.PlaceHolderSQL, - "b": sql2pgroll.PlaceHolderSQL, - }, - Down: map[string]string{ - "a": sql2pgroll.PlaceHolderSQL, - "b": sql2pgroll.PlaceHolderSQL, - }, +func AddForeignKeyOp1WithOnDelete(onDelete migrations.ForeignKeyReferenceOnDelete) *migrations.OpCreateConstraint { + return &migrations.OpCreateConstraint{ + Check: nil, + Columns: []string{"a", "b"}, + Name: "fk_bar_cd", + References: &migrations.OpCreateConstraintReferences{ + Columns: []string{"c", "d"}, + OnDelete: onDelete, + Table: "bar", + }, + Table: "foo", + Type: migrations.OpCreateConstraintTypeForeignKey, + Up: map[string]string{ + "a": sql2pgroll.PlaceHolderSQL, + "b": sql2pgroll.PlaceHolderSQL, + }, + Down: map[string]string{ + "a": sql2pgroll.PlaceHolderSQL, + "b": sql2pgroll.PlaceHolderSQL, + }, + } } From 32735dff8cd5947dcee1e93e0d6ef44605a5aaf9 Mon Sep 17 00:00:00 2001 From: Ryan Slade Date: Fri, 13 Dec 2024 09:39:48 +0100 Subject: [PATCH 3/7] Add single column examples --- pkg/sql2pgroll/alter_table_test.go | 4 ++++ pkg/sql2pgroll/expect/add_foreign_key.go | 19 ++++++++++++++++++- 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/pkg/sql2pgroll/alter_table_test.go b/pkg/sql2pgroll/alter_table_test.go index f4cfc76c..179f506d 100644 --- a/pkg/sql2pgroll/alter_table_test.go +++ b/pkg/sql2pgroll/alter_table_test.go @@ -100,6 +100,10 @@ func TestConvertAlterTableStatements(t *testing.T) { sql: "ALTER TABLE foo ADD CONSTRAINT fk_bar_cd FOREIGN KEY (a, b) REFERENCES bar (c, d) ON DELETE SET NULL;", expectedOp: expect.AddForeignKeyOp1WithOnDelete(migrations.ForeignKeyReferenceOnDeleteSETNULL), }, + { + sql: "ALTER TABLE foo ADD CONSTRAINT fk_bar_c FOREIGN KEY (a) REFERENCES bar (c);", + expectedOp: expect.AddForeignKeyOp2, + }, } for _, tc := range tests { diff --git a/pkg/sql2pgroll/expect/add_foreign_key.go b/pkg/sql2pgroll/expect/add_foreign_key.go index b6604d12..2e539f33 100644 --- a/pkg/sql2pgroll/expect/add_foreign_key.go +++ b/pkg/sql2pgroll/expect/add_foreign_key.go @@ -9,7 +9,6 @@ import ( func AddForeignKeyOp1WithOnDelete(onDelete migrations.ForeignKeyReferenceOnDelete) *migrations.OpCreateConstraint { return &migrations.OpCreateConstraint{ - Check: nil, Columns: []string{"a", "b"}, Name: "fk_bar_cd", References: &migrations.OpCreateConstraintReferences{ @@ -29,3 +28,21 @@ func AddForeignKeyOp1WithOnDelete(onDelete migrations.ForeignKeyReferenceOnDelet }, } } + +var AddForeignKeyOp2 = &migrations.OpCreateConstraint{ + Columns: []string{"a"}, + Name: "fk_bar_c", + References: &migrations.OpCreateConstraintReferences{ + Columns: []string{"c"}, + OnDelete: migrations.ForeignKeyReferenceOnDeleteNOACTION, + Table: "bar", + }, + Table: "foo", + Type: migrations.OpCreateConstraintTypeForeignKey, + Up: map[string]string{ + "a": sql2pgroll.PlaceHolderSQL, + }, + Down: map[string]string{ + "a": sql2pgroll.PlaceHolderSQL, + }, +} From 373611876bccf922a2c4a8192a1bc607a1da4b5a Mon Sep 17 00:00:00 2001 From: Ryan Slade Date: Fri, 13 Dec 2024 10:24:28 +0100 Subject: [PATCH 4/7] Handle qualified table names --- pkg/sql2pgroll/alter_table.go | 14 +++++++++++--- pkg/sql2pgroll/alter_table_test.go | 4 ++++ pkg/sql2pgroll/expect/add_foreign_key.go | 18 ++++++++++++++++++ 3 files changed, 33 insertions(+), 3 deletions(-) diff --git a/pkg/sql2pgroll/alter_table.go b/pkg/sql2pgroll/alter_table.go index 241857c9..7a86bda8 100644 --- a/pkg/sql2pgroll/alter_table.go +++ b/pkg/sql2pgroll/alter_table.go @@ -180,8 +180,6 @@ func convertAlterTableAddForeignKeyConstraint(stmt *pgq.AlterTableStmt, constrai migs[column] = PlaceHolderSQL } - foreignTable := constraint.GetPktable().GetRelname() - var onDelete migrations.ForeignKeyReferenceOnDelete switch constraint.GetFkDelAction() { case "a": @@ -198,6 +196,16 @@ func convertAlterTableAddForeignKeyConstraint(stmt *pgq.AlterTableStmt, constrai return nil, fmt.Errorf("unknown delete action: %q", constraint.GetFkDelAction()) } + tableName := stmt.GetRelation().GetRelname() + if stmt.GetRelation().GetSchemaname() != "" { + tableName = stmt.GetRelation().GetSchemaname() + "." + tableName + } + + foreignTable := constraint.GetPktable().GetRelname() + if constraint.GetPktable().GetSchemaname() != "" { + foreignTable = constraint.GetPktable().GetSchemaname() + "." + foreignTable + } + return &migrations.OpCreateConstraint{ Columns: columns, Up: migs, @@ -208,7 +216,7 @@ func convertAlterTableAddForeignKeyConstraint(stmt *pgq.AlterTableStmt, constrai OnDelete: onDelete, Table: foreignTable, }, - Table: stmt.GetRelation().GetRelname(), + Table: tableName, Type: migrations.OpCreateConstraintTypeForeignKey, }, nil } diff --git a/pkg/sql2pgroll/alter_table_test.go b/pkg/sql2pgroll/alter_table_test.go index 179f506d..5945fe46 100644 --- a/pkg/sql2pgroll/alter_table_test.go +++ b/pkg/sql2pgroll/alter_table_test.go @@ -104,6 +104,10 @@ func TestConvertAlterTableStatements(t *testing.T) { sql: "ALTER TABLE foo ADD CONSTRAINT fk_bar_c FOREIGN KEY (a) REFERENCES bar (c);", expectedOp: expect.AddForeignKeyOp2, }, + { + sql: "ALTER TABLE schema_a.foo ADD CONSTRAINT fk_bar_c FOREIGN KEY (a) REFERENCES schema_a.bar (c);", + expectedOp: expect.AddForeignKeyOp3, + }, } for _, tc := range tests { diff --git a/pkg/sql2pgroll/expect/add_foreign_key.go b/pkg/sql2pgroll/expect/add_foreign_key.go index 2e539f33..590a20ad 100644 --- a/pkg/sql2pgroll/expect/add_foreign_key.go +++ b/pkg/sql2pgroll/expect/add_foreign_key.go @@ -46,3 +46,21 @@ var AddForeignKeyOp2 = &migrations.OpCreateConstraint{ "a": sql2pgroll.PlaceHolderSQL, }, } + +var AddForeignKeyOp3 = &migrations.OpCreateConstraint{ + Columns: []string{"a"}, + Name: "fk_bar_c", + References: &migrations.OpCreateConstraintReferences{ + Columns: []string{"c"}, + OnDelete: migrations.ForeignKeyReferenceOnDeleteNOACTION, + Table: "schema_a.bar", + }, + Table: "schema_a.foo", + Type: migrations.OpCreateConstraintTypeForeignKey, + Up: map[string]string{ + "a": sql2pgroll.PlaceHolderSQL, + }, + Down: map[string]string{ + "a": sql2pgroll.PlaceHolderSQL, + }, +} From edfed1cdd01fc0f79c1ec6f5024037bd737e651e Mon Sep 17 00:00:00 2001 From: Ryan Slade Date: Fri, 13 Dec 2024 11:04:20 +0100 Subject: [PATCH 5/7] Fall back to raw SQL if update actions are added --- pkg/sql2pgroll/alter_table.go | 13 +++++++++++++ pkg/sql2pgroll/alter_table_test.go | 10 ++++++++++ 2 files changed, 23 insertions(+) diff --git a/pkg/sql2pgroll/alter_table.go b/pkg/sql2pgroll/alter_table.go index 7a86bda8..b2bb4557 100644 --- a/pkg/sql2pgroll/alter_table.go +++ b/pkg/sql2pgroll/alter_table.go @@ -165,6 +165,10 @@ func convertAlterTableAddUniqueConstraint(stmt *pgq.AlterTableStmt, constraint * } func convertAlterTableAddForeignKeyConstraint(stmt *pgq.AlterTableStmt, constraint *pgq.Constraint) (migrations.Operation, error) { + if !canConvertAlterTableAddForeignKeyConstraint(constraint) { + return nil, nil + } + columns := make([]string, len(constraint.GetFkAttrs())) for i := range columns { columns[i] = constraint.GetFkAttrs()[i].GetString_().GetSval() @@ -221,6 +225,15 @@ func convertAlterTableAddForeignKeyConstraint(stmt *pgq.AlterTableStmt, constrai }, nil } +func canConvertAlterTableAddForeignKeyConstraint(constraint *pgq.Constraint) bool { + switch constraint.GetFkUpdAction() { + case "r", "c", "n", "d": + // We ignore "a" above since this is NO ACTION, the default + return false + } + return true +} + // convertAlterTableSetColumnDefault converts SQL statements like: // // `ALTER TABLE foo COLUMN bar SET DEFAULT 'foo'` diff --git a/pkg/sql2pgroll/alter_table_test.go b/pkg/sql2pgroll/alter_table_test.go index 5945fe46..e41ac584 100644 --- a/pkg/sql2pgroll/alter_table_test.go +++ b/pkg/sql2pgroll/alter_table_test.go @@ -104,6 +104,10 @@ func TestConvertAlterTableStatements(t *testing.T) { sql: "ALTER TABLE foo ADD CONSTRAINT fk_bar_c FOREIGN KEY (a) REFERENCES bar (c);", expectedOp: expect.AddForeignKeyOp2, }, + { + sql: "ALTER TABLE foo ADD CONSTRAINT fk_bar_c FOREIGN KEY (a) REFERENCES bar (c) NOT VALID;", + expectedOp: expect.AddForeignKeyOp2, + }, { sql: "ALTER TABLE schema_a.foo ADD CONSTRAINT fk_bar_c FOREIGN KEY (a) REFERENCES schema_a.bar (c);", expectedOp: expect.AddForeignKeyOp3, @@ -144,6 +148,12 @@ func TestUnconvertableAlterTableStatements(t *testing.T) { // Non literal default values "ALTER TABLE foo ALTER COLUMN bar SET DEFAULT now()", + + // Unsupported foreign key statements + "ALTER TABLE foo ADD CONSTRAINT fk_bar_cd FOREIGN KEY (a, b) REFERENCES bar (c, d) ON UPDATE RESTRICT;", + "ALTER TABLE foo ADD CONSTRAINT fk_bar_cd FOREIGN KEY (a, b) REFERENCES bar (c, d) ON UPDATE CASCADE;", + "ALTER TABLE foo ADD CONSTRAINT fk_bar_cd FOREIGN KEY (a, b) REFERENCES bar (c, d) ON UPDATE SET NULL;", + "ALTER TABLE foo ADD CONSTRAINT fk_bar_cd FOREIGN KEY (a, b) REFERENCES bar (c, d) ON UPDATE SET DEFAULT;", } for _, sql := range tests { From 7abd3668c5c7200c9a337bbbaadae257db0cc4af Mon Sep 17 00:00:00 2001 From: Ryan Slade Date: Fri, 13 Dec 2024 11:29:18 +0100 Subject: [PATCH 6/7] Add more fallback cases --- pkg/sql2pgroll/alter_table.go | 13 ++++++++++++- pkg/sql2pgroll/alter_table_test.go | 3 +++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/pkg/sql2pgroll/alter_table.go b/pkg/sql2pgroll/alter_table.go index b2bb4557..414169c2 100644 --- a/pkg/sql2pgroll/alter_table.go +++ b/pkg/sql2pgroll/alter_table.go @@ -228,8 +228,19 @@ func convertAlterTableAddForeignKeyConstraint(stmt *pgq.AlterTableStmt, constrai func canConvertAlterTableAddForeignKeyConstraint(constraint *pgq.Constraint) bool { switch constraint.GetFkUpdAction() { case "r", "c", "n", "d": - // We ignore "a" above since this is NO ACTION, the default + // RESTRICT, CASCADE, SET NULL, SET DEFAULT return false + case "a": + // NO ACTION, the default + break + } + switch constraint.GetFkMatchtype() { + case "f": + // FULL + return false + case "s": + // SIMPLE, the default + break } return true } diff --git a/pkg/sql2pgroll/alter_table_test.go b/pkg/sql2pgroll/alter_table_test.go index e41ac584..f3665aa2 100644 --- a/pkg/sql2pgroll/alter_table_test.go +++ b/pkg/sql2pgroll/alter_table_test.go @@ -154,6 +154,9 @@ func TestUnconvertableAlterTableStatements(t *testing.T) { "ALTER TABLE foo ADD CONSTRAINT fk_bar_cd FOREIGN KEY (a, b) REFERENCES bar (c, d) ON UPDATE CASCADE;", "ALTER TABLE foo ADD CONSTRAINT fk_bar_cd FOREIGN KEY (a, b) REFERENCES bar (c, d) ON UPDATE SET NULL;", "ALTER TABLE foo ADD CONSTRAINT fk_bar_cd FOREIGN KEY (a, b) REFERENCES bar (c, d) ON UPDATE SET DEFAULT;", + "ALTER TABLE foo ADD CONSTRAINT fk_bar_cd FOREIGN KEY (a, b) REFERENCES bar (c, d) MATCH FULL;", + // MATCH PARTIAL is not implemented in the actual parser yet + //"ALTER TABLE foo ADD CONSTRAINT fk_bar_cd FOREIGN KEY (a, b) REFERENCES bar (c, d) MATCH PARTIAL;", } for _, sql := range tests { From 21eebb14f05e2c6ef1f47008579e8f567fc064fe Mon Sep 17 00:00:00 2001 From: Ryan Slade Date: Fri, 13 Dec 2024 14:00:08 +0100 Subject: [PATCH 7/7] Update doc comment --- pkg/sql2pgroll/alter_table.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/sql2pgroll/alter_table.go b/pkg/sql2pgroll/alter_table.go index 414169c2..6f59825b 100644 --- a/pkg/sql2pgroll/alter_table.go +++ b/pkg/sql2pgroll/alter_table.go @@ -98,11 +98,13 @@ func convertAlterTableAlterColumnType(stmt *pgq.AlterTableStmt, cmd *pgq.AlterTa }, nil } -// convertAlterTableAddConstraint converts SQL statements like: +// convertAlterTableAddConstraint converts SQL statements that add UNIQUE or FOREIGN KEY constraints, +// for example: // // `ALTER TABLE foo ADD CONSTRAINT bar UNIQUE (a)` +// `ALTER TABLE foo ADD CONSTRAINT fk_bar_c FOREIGN KEY (a) REFERENCES bar (c);` // -// To an OpCreateConstraint operation. +// An OpCreateConstraint operation is returned. func convertAlterTableAddConstraint(stmt *pgq.AlterTableStmt, cmd *pgq.AlterTableCmd) (migrations.Operation, error) { node, ok := cmd.GetDef().Node.(*pgq.Node_Constraint) if !ok {