From 884429b9406feca3c56c6787659cd3802e9f67a6 Mon Sep 17 00:00:00 2001 From: Andrew Farries Date: Mon, 15 Jan 2024 09:28:35 +0000 Subject: [PATCH 01/11] Preserve default value when duplicating a column --- pkg/migrations/duplicate.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/pkg/migrations/duplicate.go b/pkg/migrations/duplicate.go index d35b3f80..39c0af4f 100644 --- a/pkg/migrations/duplicate.go +++ b/pkg/migrations/duplicate.go @@ -42,14 +42,22 @@ func (d *Duplicator) WithType(t string) *Duplicator { func (d *Duplicator) Duplicate(ctx context.Context) error { const ( cAlterTableSQL = `ALTER TABLE %s ADD COLUMN %s %s` + cSetDefaultSQL = `ALTER COLUMN %s SET DEFAULT %s` cAddForeignKeySQL = `ADD CONSTRAINT %s FOREIGN KEY (%s) REFERENCES %s (%s)` ) + // Generate SQL to duplicate the column's name and type sql := fmt.Sprintf(cAlterTableSQL, pq.QuoteIdentifier(d.table.Name), pq.QuoteIdentifier(d.asName), d.withType) + // Generate SQL to duplicate the column's default value + if d.column.Default != nil { + sql += fmt.Sprintf(", "+cSetDefaultSQL, d.asName, *d.column.Default) + } + + // Generate SQL to duplicate any foreign key constraints on the column for _, fk := range d.table.ForeignKeys { if slices.Contains(fk.Columns, d.column.Name) { sql += fmt.Sprintf(", "+cAddForeignKeySQL, From 2838f67d0e4bf531ef3f1f0a1af69be334368da5 Mon Sep 17 00:00:00 2001 From: Andrew Farries Date: Mon, 15 Jan 2024 11:53:17 +0000 Subject: [PATCH 02/11] Add test: ensure default preserved on type change Check that changing a column's type preserves a default value defined on the column. --- pkg/migrations/op_change_type_test.go | 65 +++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/pkg/migrations/op_change_type_test.go b/pkg/migrations/op_change_type_test.go index b8208048..cea81bbc 100644 --- a/pkg/migrations/op_change_type_test.go +++ b/pkg/migrations/op_change_type_test.go @@ -222,6 +222,71 @@ func TestChangeColumnType(t *testing.T) { ConstraintMustExist(t, db, "public", "employees", "fk_employee_department") }, }, + { + name: "changing column type preserves any defaults on the column", + migrations: []migrations.Migration{ + { + Name: "01_add_table", + Operations: migrations.Operations{ + &migrations.OpCreateTable{ + Name: "users", + Columns: []migrations.Column{ + { + Name: "id", + Type: "integer", + Pk: true, + }, + { + Name: "username", + Type: "text", + Default: ptr("'alice'"), + Nullable: true, + }, + }, + }, + }, + }, + { + Name: "02_change_type", + Operations: migrations.Operations{ + &migrations.OpAlterColumn{ + Table: "users", + Column: "username", + Type: "varchar(255)", + Up: "username", + Down: "username", + }, + }, + }, + }, + afterStart: func(t *testing.T, db *sql.DB) { + // A row can be inserted into the new version of the table. + MustInsert(t, db, "public", "02_change_type", "users", map[string]string{ + "id": "1", + }) + + // The newly inserted row respects the default value of the column. + rows := MustSelect(t, db, "public", "02_change_type", "users") + assert.Equal(t, []map[string]any{ + {"id": 1, "username": "alice"}, + }, rows) + }, + afterRollback: func(t *testing.T, db *sql.DB) { + }, + afterComplete: func(t *testing.T, db *sql.DB) { + // A row can be inserted into the new version of the table. + MustInsert(t, db, "public", "02_change_type", "users", map[string]string{ + "id": "2", + }) + + // The newly inserted row respects the default value of the column. + rows := MustSelect(t, db, "public", "02_change_type", "users") + assert.Equal(t, []map[string]any{ + {"id": 1, "username": "alice"}, + {"id": 2, "username": "alice"}, + }, rows) + }, + }, }) } From 661b6a7c9ce56e3da678a9ba65192c2d9cab0cd6 Mon Sep 17 00:00:00 2001 From: Andrew Farries Date: Mon, 15 Jan 2024 16:46:02 +0000 Subject: [PATCH 03/11] Add test: check defaults preserved on set NOT NULL --- pkg/migrations/op_set_notnull_test.go | 64 +++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/pkg/migrations/op_set_notnull_test.go b/pkg/migrations/op_set_notnull_test.go index eec21abc..0f37880f 100644 --- a/pkg/migrations/op_set_notnull_test.go +++ b/pkg/migrations/op_set_notnull_test.go @@ -299,6 +299,70 @@ func TestSetNotNull(t *testing.T) { ConstraintMustExist(t, db, "public", "employees", "fk_employee_department") }, }, + { + name: "setting a nullable column to not null retains any default defined on the column", + migrations: []migrations.Migration{ + { + Name: "01_add_table", + Operations: migrations.Operations{ + &migrations.OpCreateTable{ + Name: "users", + Columns: []migrations.Column{ + { + Name: "id", + Type: "integer", + Pk: true, + }, + { + Name: "name", + Type: "text", + Nullable: true, + Default: ptr("'anonymous'"), + }, + }, + }, + }, + }, + { + Name: "02_set_not_null", + Operations: migrations.Operations{ + &migrations.OpAlterColumn{ + Table: "users", + Column: "name", + Nullable: ptr(false), + Up: "(SELECT CASE WHEN name IS NULL THEN 'anonymous' ELSE name END)", + }, + }, + }, + }, + afterStart: func(t *testing.T, db *sql.DB) { + // A row can be inserted into the new version of the table. + MustInsert(t, db, "public", "02_set_not_null", "users", map[string]string{ + "id": "1", + }) + + // The newly inserted row respects the default value of the column. + rows := MustSelect(t, db, "public", "02_set_not_null", "users") + assert.Equal(t, []map[string]any{ + {"id": 1, "name": "anonymous"}, + }, rows) + }, + afterRollback: func(t *testing.T, db *sql.DB) { + }, + afterComplete: func(t *testing.T, db *sql.DB) { + // A row can be inserted into the new version of the table. + MustInsert(t, db, "public", "02_set_not_null", "users", map[string]string{ + "id": "2", + }) + + // The newly inserted row respects the default value of the column. + rows := MustSelect(t, db, "public", "02_set_not_null", "users") + assert.Equal(t, []map[string]any{ + {"id": 1, "name": "anonymous"}, + {"id": 2, "name": "anonymous"}, + }, rows) + }, + }, }) } From 8d7e99f39116fb1c61a1a5f8bc2db82ddc266d8d Mon Sep 17 00:00:00 2001 From: Andrew Farries Date: Mon, 15 Jan 2024 17:19:53 +0000 Subject: [PATCH 04/11] Preserve column properties when adding `CHECK` --- pkg/migrations/op_set_check.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/pkg/migrations/op_set_check.go b/pkg/migrations/op_set_check.go index 2dce1426..497db2f8 100644 --- a/pkg/migrations/op_set_check.go +++ b/pkg/migrations/op_set_check.go @@ -27,7 +27,8 @@ func (o *OpSetCheckConstraint) Start(ctx context.Context, conn *sql.DB, stateSch column := table.GetColumn(o.Column) // Create a copy of the column on the underlying table. - if err := duplicateColumn(ctx, conn, table, *column); err != nil { + d := NewColumnDuplicator(conn, table, column) + if err := d.Duplicate(ctx); err != nil { return fmt.Errorf("failed to duplicate column: %w", err) } @@ -112,11 +113,9 @@ func (o *OpSetCheckConstraint) Complete(ctx context.Context, conn *sql.DB, s *sc } // Rename the new column to the old column name - _, err = conn.ExecContext(ctx, fmt.Sprintf("ALTER TABLE IF EXISTS %s RENAME COLUMN %s TO %s", - pq.QuoteIdentifier(o.Table), - pq.QuoteIdentifier(TemporaryName(o.Column)), - pq.QuoteIdentifier(o.Column))) - if err != nil { + table := s.GetTable(o.Table) + column := table.GetColumn(o.Column) + if err := RenameDuplicatedColumn(ctx, conn, table, column); err != nil { return err } From 830e07aec1f177e6b93645f5ecbf301c51411829 Mon Sep 17 00:00:00 2001 From: Andrew Farries Date: Mon, 15 Jan 2024 17:20:44 +0000 Subject: [PATCH 05/11] Add test: `DEFAULT` preserved when adding `CHECK` --- pkg/migrations/op_set_check_test.go | 293 +++++++++++++++++----------- 1 file changed, 181 insertions(+), 112 deletions(-) diff --git a/pkg/migrations/op_set_check_test.go b/pkg/migrations/op_set_check_test.go index 7d745f41..9ba00f90 100644 --- a/pkg/migrations/op_set_check_test.go +++ b/pkg/migrations/op_set_check_test.go @@ -13,129 +13,198 @@ import ( func TestSetCheckConstraint(t *testing.T) { t.Parallel() - ExecuteTests(t, TestCases{{ - name: "add check constraint", - migrations: []migrations.Migration{ - { - Name: "01_add_table", - Operations: migrations.Operations{ - &migrations.OpCreateTable{ - Name: "posts", - Columns: []migrations.Column{ - { - Name: "id", - Type: "serial", - Pk: true, + ExecuteTests(t, TestCases{ + { + name: "add check constraint", + migrations: []migrations.Migration{ + { + Name: "01_add_table", + Operations: migrations.Operations{ + &migrations.OpCreateTable{ + Name: "posts", + Columns: []migrations.Column{ + { + Name: "id", + Type: "serial", + Pk: true, + }, + { + Name: "title", + Type: "text", + }, }, - { - Name: "title", - Type: "text", + }, + }, + }, + { + Name: "02_add_check_constraint", + Operations: migrations.Operations{ + &migrations.OpAlterColumn{ + Table: "posts", + Column: "title", + Check: &migrations.CheckConstraint{ + Name: "check_title_length", + Constraint: "length(title) > 3", }, + Up: "(SELECT CASE WHEN length(title) <= 3 THEN LPAD(title, 4, '-') ELSE title END)", + Down: "title", }, }, }, }, - { - Name: "02_add_check_constraint", - Operations: migrations.Operations{ - &migrations.OpAlterColumn{ - Table: "posts", - Column: "title", - Check: &migrations.CheckConstraint{ - Name: "check_title_length", - Constraint: "length(title) > 3", + afterStart: func(t *testing.T, db *sql.DB) { + // The new (temporary) `title` column should exist on the underlying table. + ColumnMustExist(t, db, "public", "posts", migrations.TemporaryName("title")) + + // Inserting a row that meets the check constraint into the old view works. + MustInsert(t, db, "public", "01_add_table", "posts", map[string]string{ + "title": "post by alice", + }) + + // Inserting a row that does not meet the check constraint into the old view also works. + MustInsert(t, db, "public", "01_add_table", "posts", map[string]string{ + "title": "b", + }) + + // Both rows have been backfilled into the new view; the short title has + // been rewritten using `up` SQL to meet the length constraint. + rows := MustSelect(t, db, "public", "02_add_check_constraint", "posts") + assert.Equal(t, []map[string]any{ + {"id": 1, "title": "post by alice"}, + {"id": 2, "title": "---b"}, + }, rows) + + // Inserting a row that meets the check constraint into the new view works. + MustInsert(t, db, "public", "02_add_check_constraint", "posts", map[string]string{ + "title": "post by carl", + }) + + // Inserting a row that does not meet the check constraint into the new view fails. + MustNotInsert(t, db, "public", "02_add_check_constraint", "posts", map[string]string{ + "title": "d", + }) + + // The row that was inserted into the new view has been backfilled into the old view. + rows = MustSelect(t, db, "public", "01_add_table", "posts") + assert.Equal(t, []map[string]any{ + {"id": 1, "title": "post by alice"}, + {"id": 2, "title": "b"}, + {"id": 3, "title": "post by carl"}, + }, rows) + }, + afterRollback: func(t *testing.T, db *sql.DB) { + // The new (temporary) `title` column should not exist on the underlying table. + ColumnMustNotExist(t, db, "public", "posts", migrations.TemporaryName("title")) + + // The up function no longer exists. + FunctionMustNotExist(t, db, "public", migrations.TriggerFunctionName("posts", "title")) + // The down function no longer exists. + FunctionMustNotExist(t, db, "public", migrations.TriggerFunctionName("posts", migrations.TemporaryName("title"))) + + // The up trigger no longer exists. + TriggerMustNotExist(t, db, "public", "posts", migrations.TriggerName("posts", "title")) + // The down trigger no longer exists. + TriggerMustNotExist(t, db, "public", "posts", migrations.TriggerName("posts", migrations.TemporaryName("title"))) + }, + afterComplete: func(t *testing.T, db *sql.DB) { + // Inserting a row that meets the check constraint into the new view works. + MustInsert(t, db, "public", "02_add_check_constraint", "posts", map[string]string{ + "title": "post by dana", + }) + + // Inserting a row that does not meet the check constraint into the new view fails. + MustNotInsert(t, db, "public", "02_add_check_constraint", "posts", map[string]string{ + "title": "e", + }) + + // The data in the new `posts` view is as expected. + rows := MustSelect(t, db, "public", "02_add_check_constraint", "posts") + assert.Equal(t, []map[string]any{ + {"id": 1, "title": "post by alice"}, + {"id": 2, "title": "---b"}, + {"id": 3, "title": "post by carl"}, + {"id": 5, "title": "post by dana"}, + }, rows) + + // The up function no longer exists. + FunctionMustNotExist(t, db, "public", migrations.TriggerFunctionName("posts", "title")) + // The down function no longer exists. + FunctionMustNotExist(t, db, "public", migrations.TriggerFunctionName("posts", migrations.TemporaryName("title"))) + + // The up trigger no longer exists. + TriggerMustNotExist(t, db, "public", "posts", migrations.TriggerName("posts", "title")) + // The down trigger no longer exists. + TriggerMustNotExist(t, db, "public", "posts", migrations.TriggerName("posts", migrations.TemporaryName("title"))) + }, + }, + { + name: "column defaults are preserved when adding a check constraint", + migrations: []migrations.Migration{ + { + Name: "01_add_table", + Operations: migrations.Operations{ + &migrations.OpCreateTable{ + Name: "posts", + Columns: []migrations.Column{ + { + Name: "id", + Type: "serial", + Pk: true, + }, + { + Name: "title", + Type: "text", + Default: ptr("'untitled'"), + }, + }, + }, + }, + }, + { + Name: "02_add_check_constraint", + Operations: migrations.Operations{ + &migrations.OpAlterColumn{ + Table: "posts", + Column: "title", + Check: &migrations.CheckConstraint{ + Name: "check_title_length", + Constraint: "length(title) > 3", + }, + Up: "(SELECT CASE WHEN length(title) <= 3 THEN LPAD(title, 4, '-') ELSE title END)", + Down: "title", }, - Up: "(SELECT CASE WHEN length(title) <= 3 THEN LPAD(title, 4, '-') ELSE title END)", - Down: "title", }, }, }, + afterStart: func(t *testing.T, db *sql.DB) { + // A row can be inserted into the new version of the table. + MustInsert(t, db, "public", "02_add_check_constraint", "posts", map[string]string{ + "id": "1", + }) + + // The newly inserted row respects the default value of the column. + rows := MustSelect(t, db, "public", "02_add_check_constraint", "posts") + assert.Equal(t, []map[string]any{ + {"id": 1, "title": "untitled"}, + }, rows) + }, + afterRollback: func(t *testing.T, db *sql.DB) { + }, + afterComplete: func(t *testing.T, db *sql.DB) { + // A row can be inserted into the new version of the table. + MustInsert(t, db, "public", "02_add_check_constraint", "posts", map[string]string{ + "id": "2", + }) + + // The newly inserted row respects the default value of the column. + rows := MustSelect(t, db, "public", "02_add_check_constraint", "posts") + assert.Equal(t, []map[string]any{ + {"id": 1, "title": "untitled"}, + {"id": 2, "title": "untitled"}, + }, rows) + }, }, - afterStart: func(t *testing.T, db *sql.DB) { - // The new (temporary) `title` column should exist on the underlying table. - ColumnMustExist(t, db, "public", "posts", migrations.TemporaryName("title")) - - // Inserting a row that meets the check constraint into the old view works. - MustInsert(t, db, "public", "01_add_table", "posts", map[string]string{ - "title": "post by alice", - }) - - // Inserting a row that does not meet the check constraint into the old view also works. - MustInsert(t, db, "public", "01_add_table", "posts", map[string]string{ - "title": "b", - }) - - // Both rows have been backfilled into the new view; the short title has - // been rewritten using `up` SQL to meet the length constraint. - rows := MustSelect(t, db, "public", "02_add_check_constraint", "posts") - assert.Equal(t, []map[string]any{ - {"id": 1, "title": "post by alice"}, - {"id": 2, "title": "---b"}, - }, rows) - - // Inserting a row that meets the check constraint into the new view works. - MustInsert(t, db, "public", "02_add_check_constraint", "posts", map[string]string{ - "title": "post by carl", - }) - - // Inserting a row that does not meet the check constraint into the new view fails. - MustNotInsert(t, db, "public", "02_add_check_constraint", "posts", map[string]string{ - "title": "d", - }) - - // The row that was inserted into the new view has been backfilled into the old view. - rows = MustSelect(t, db, "public", "01_add_table", "posts") - assert.Equal(t, []map[string]any{ - {"id": 1, "title": "post by alice"}, - {"id": 2, "title": "b"}, - {"id": 3, "title": "post by carl"}, - }, rows) - }, - afterRollback: func(t *testing.T, db *sql.DB) { - // The new (temporary) `title` column should not exist on the underlying table. - ColumnMustNotExist(t, db, "public", "posts", migrations.TemporaryName("title")) - - // The up function no longer exists. - FunctionMustNotExist(t, db, "public", migrations.TriggerFunctionName("posts", "title")) - // The down function no longer exists. - FunctionMustNotExist(t, db, "public", migrations.TriggerFunctionName("posts", migrations.TemporaryName("title"))) - - // The up trigger no longer exists. - TriggerMustNotExist(t, db, "public", "posts", migrations.TriggerName("posts", "title")) - // The down trigger no longer exists. - TriggerMustNotExist(t, db, "public", "posts", migrations.TriggerName("posts", migrations.TemporaryName("title"))) - }, - afterComplete: func(t *testing.T, db *sql.DB) { - // Inserting a row that meets the check constraint into the new view works. - MustInsert(t, db, "public", "02_add_check_constraint", "posts", map[string]string{ - "title": "post by dana", - }) - - // Inserting a row that does not meet the check constraint into the new view fails. - MustNotInsert(t, db, "public", "02_add_check_constraint", "posts", map[string]string{ - "title": "e", - }) - - // The data in the new `posts` view is as expected. - rows := MustSelect(t, db, "public", "02_add_check_constraint", "posts") - assert.Equal(t, []map[string]any{ - {"id": 1, "title": "post by alice"}, - {"id": 2, "title": "---b"}, - {"id": 3, "title": "post by carl"}, - {"id": 5, "title": "post by dana"}, - }, rows) - - // The up function no longer exists. - FunctionMustNotExist(t, db, "public", migrations.TriggerFunctionName("posts", "title")) - // The down function no longer exists. - FunctionMustNotExist(t, db, "public", migrations.TriggerFunctionName("posts", migrations.TemporaryName("title"))) - - // The up trigger no longer exists. - TriggerMustNotExist(t, db, "public", "posts", migrations.TriggerName("posts", "title")) - // The down trigger no longer exists. - TriggerMustNotExist(t, db, "public", "posts", migrations.TriggerName("posts", migrations.TemporaryName("title"))) - }, - }}) + }) } func TestSetCheckConstraintValidation(t *testing.T) { From 67aa67f65e7ea2ca29ca50d34d95cb8352748d7e Mon Sep 17 00:00:00 2001 From: Andrew Farries Date: Mon, 15 Jan 2024 17:38:25 +0000 Subject: [PATCH 06/11] Add test: FKs preserved when adding `CHECK` --- pkg/migrations/op_set_check_test.go | 80 +++++++++++++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/pkg/migrations/op_set_check_test.go b/pkg/migrations/op_set_check_test.go index 9ba00f90..8baae581 100644 --- a/pkg/migrations/op_set_check_test.go +++ b/pkg/migrations/op_set_check_test.go @@ -204,6 +204,86 @@ func TestSetCheckConstraint(t *testing.T) { }, rows) }, }, + { + name: "foreign keys are preserved when adding a check constraint", + migrations: []migrations.Migration{ + { + Name: "01_add_departments_table", + Operations: migrations.Operations{ + &migrations.OpCreateTable{ + Name: "departments", + Columns: []migrations.Column{ + { + Name: "id", + Type: "serial", + Pk: true, + }, + { + Name: "name", + Type: "text", + Nullable: false, + }, + }, + }, + }, + }, + { + Name: "02_add_employees_table", + Operations: migrations.Operations{ + &migrations.OpCreateTable{ + Name: "employees", + Columns: []migrations.Column{ + { + Name: "id", + Type: "serial", + Pk: true, + }, + { + Name: "name", + Type: "text", + Nullable: false, + }, + { + Name: "department_id", + Type: "integer", + Nullable: true, + References: &migrations.ForeignKeyReference{ + Name: "fk_employee_department", + Table: "departments", + Column: "id", + }, + }, + }, + }, + }, + }, + { + Name: "03_add_check_constraint", + Operations: migrations.Operations{ + &migrations.OpAlterColumn{ + Table: "employees", + Column: "department_id", + Check: &migrations.CheckConstraint{ + Name: "check_valid_department_id", + Constraint: "department_id > 1", + }, + Up: "(SELECT CASE WHEN department_id <= 1 THEN 2 ELSE department_id END)", + Down: "department_id", + }, + }, + }, + }, + afterStart: func(t *testing.T, db *sql.DB) { + // A temporary FK constraint has been created on the temporary column + ConstraintMustExist(t, db, "public", "employees", migrations.TemporaryName("fk_employee_department")) + }, + afterRollback: func(t *testing.T, db *sql.DB) { + }, + afterComplete: func(t *testing.T, db *sql.DB) { + // The foreign key constraint still exists on the column + ConstraintMustExist(t, db, "public", "employees", "fk_employee_department") + }, + }, }) } From 2ad348055bca72df10f7a82f9351e2f23b5e4597 Mon Sep 17 00:00:00 2001 From: Andrew Farries Date: Tue, 16 Jan 2024 08:23:38 +0000 Subject: [PATCH 07/11] Correct test name --- pkg/migrations/op_set_unique_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/migrations/op_set_unique_test.go b/pkg/migrations/op_set_unique_test.go index 4d483fd3..05ddff53 100644 --- a/pkg/migrations/op_set_unique_test.go +++ b/pkg/migrations/op_set_unique_test.go @@ -116,7 +116,7 @@ func TestSetColumnUnique(t *testing.T) { }, }, { - name: "set unique with default user supplied down sql", + name: "set unique with user supplied down sql", migrations: []migrations.Migration{ { Name: "01_add_table", From efdb80b22e9f16498efaaee719a59f189d4511d7 Mon Sep 17 00:00:00 2001 From: Andrew Farries Date: Tue, 16 Jan 2024 08:22:14 +0000 Subject: [PATCH 08/11] Preserve column properties in set `UNIQUE` op --- pkg/migrations/op_set_unique.go | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/pkg/migrations/op_set_unique.go b/pkg/migrations/op_set_unique.go index 76a3f145..3b61494f 100644 --- a/pkg/migrations/op_set_unique.go +++ b/pkg/migrations/op_set_unique.go @@ -26,7 +26,8 @@ func (o *OpSetUnique) Start(ctx context.Context, conn *sql.DB, stateSchema strin column := table.GetColumn(o.Column) // create a copy of the column on the underlying table. - if err := duplicateColumn(ctx, conn, table, *column); err != nil { + d := NewColumnDuplicator(conn, table, column) + if err := d.Duplicate(ctx); err != nil { return fmt.Errorf("failed to duplicate column: %w", err) } @@ -115,10 +116,11 @@ func (o *OpSetUnique) Complete(ctx context.Context, conn *sql.DB, s *schema.Sche } // Rename the new column to the old column name - _, err = conn.ExecContext(ctx, fmt.Sprintf("ALTER TABLE IF EXISTS %s RENAME COLUMN %s TO %s", - pq.QuoteIdentifier(o.Table), - pq.QuoteIdentifier(TemporaryName(o.Column)), - pq.QuoteIdentifier(o.Column))) + table := s.GetTable(o.Table) + column := table.GetColumn(o.Column) + if err := RenameDuplicatedColumn(ctx, conn, table, column); err != nil { + return err + } return err } From 9cf5f7b96bb48d5f5e20f24ceb3458ea8e226661 Mon Sep 17 00:00:00 2001 From: Andrew Farries Date: Tue, 16 Jan 2024 08:52:11 +0000 Subject: [PATCH 09/11] Add `MustDelete` test helper method --- pkg/migrations/op_common_test.go | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/pkg/migrations/op_common_test.go b/pkg/migrations/op_common_test.go index 164511b8..20025a74 100644 --- a/pkg/migrations/op_common_test.go +++ b/pkg/migrations/op_common_test.go @@ -496,6 +496,36 @@ func insert(t *testing.T, db *sql.DB, schema, version, table string, record map[ return err } +func MustDelete(t *testing.T, db *sql.DB, schema, version, table string, record map[string]string) { + t.Helper() + + if err := delete(t, db, schema, version, table, record); err != nil { + t.Fatal(err) + } +} + +func delete(t *testing.T, db *sql.DB, schema, version, table string, record map[string]string) error { + t.Helper() + versionSchema := roll.VersionedSchemaName(schema, version) + + cols := maps.Keys(record) + slices.Sort(cols) + + recordStr := "" + for i, c := range cols { + if i > 0 { + recordStr += " AND " + } + recordStr += fmt.Sprintf("%s = '%s'", c, record[c]) + } + + //nolint:gosec // this is a test so we don't care about SQL injection + stmt := fmt.Sprintf("DELETE FROM %s.%s WHERE %s", versionSchema, table, recordStr) + + _, err := db.Exec(stmt) + return err +} + func MustSelect(t *testing.T, db *sql.DB, schema, version, table string) []map[string]any { t.Helper() versionSchema := roll.VersionedSchemaName(schema, version) From a329f893951147b813673f86cf3acd07bf8ae1bd Mon Sep 17 00:00:00 2001 From: Andrew Farries Date: Tue, 16 Jan 2024 08:37:57 +0000 Subject: [PATCH 10/11] Add test for preservation of column default value --- pkg/migrations/op_set_unique_test.go | 80 ++++++++++++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/pkg/migrations/op_set_unique_test.go b/pkg/migrations/op_set_unique_test.go index 05ddff53..b1f5d86b 100644 --- a/pkg/migrations/op_set_unique_test.go +++ b/pkg/migrations/op_set_unique_test.go @@ -179,5 +179,85 @@ func TestSetColumnUnique(t *testing.T) { afterComplete: func(t *testing.T, db *sql.DB) { }, }, + { + name: "column defaults are preserved when adding a unique constraint", + migrations: []migrations.Migration{ + { + Name: "01_add_table", + Operations: migrations.Operations{ + &migrations.OpCreateTable{ + Name: "reviews", + Columns: []migrations.Column{ + { + Name: "id", + Type: "serial", + Pk: true, + }, + { + Name: "username", + Type: "text", + Default: ptr("'anonymous'"), + }, + { + Name: "product", + Type: "text", + }, + { + Name: "review", + Type: "text", + }, + }, + }, + }, + }, + { + Name: "02_set_unique", + Operations: migrations.Operations{ + &migrations.OpAlterColumn{ + Table: "reviews", + Column: "username", + Unique: &migrations.UniqueConstraint{ + Name: "reviews_username_unique", + }, + Up: "username", + Down: "username", + }, + }, + }, + }, + afterStart: func(t *testing.T, db *sql.DB) { + // A row can be inserted into the new version of the table. + MustInsert(t, db, "public", "02_set_unique", "reviews", map[string]string{ + "product": "apple", "review": "awesome", + }) + + // The newly inserted row respects the default value of the column. + rows := MustSelect(t, db, "public", "02_set_unique", "reviews") + assert.Equal(t, []map[string]any{ + {"id": 1, "username": "anonymous", "product": "apple", "review": "awesome"}, + }, rows) + }, + afterRollback: func(t *testing.T, db *sql.DB) { + }, + afterComplete: func(t *testing.T, db *sql.DB) { + // Delete the row that was inserted in the `afterStart` hook to + // ensure that another row with a default 'username' can be inserted + // without violating the UNIQUE constraint on the column. + MustDelete(t, db, "public", "02_set_unique", "reviews", map[string]string{ + "id": "1", + }) + + // A row can be inserted into the new version of the table. + MustInsert(t, db, "public", "02_set_unique", "reviews", map[string]string{ + "product": "banana", "review": "bent", + }) + + // The newly inserted row respects the default value of the column. + rows := MustSelect(t, db, "public", "02_set_unique", "reviews") + assert.Equal(t, []map[string]any{ + {"id": 2, "username": "anonymous", "product": "banana", "review": "bent"}, + }, rows) + }, + }, }) } From edc378ca192b74dd70466ab8b241c85a2e9a3f50 Mon Sep 17 00:00:00 2001 From: Andrew Farries Date: Tue, 16 Jan 2024 08:43:26 +0000 Subject: [PATCH 11/11] Add test for preservation of FK constraints --- pkg/migrations/op_set_unique_test.go | 79 ++++++++++++++++++++++++++++ 1 file changed, 79 insertions(+) diff --git a/pkg/migrations/op_set_unique_test.go b/pkg/migrations/op_set_unique_test.go index b1f5d86b..d660c0f1 100644 --- a/pkg/migrations/op_set_unique_test.go +++ b/pkg/migrations/op_set_unique_test.go @@ -259,5 +259,84 @@ func TestSetColumnUnique(t *testing.T) { }, rows) }, }, + { + name: "foreign keys defined on the column are preserved when adding a unique constraint", + migrations: []migrations.Migration{ + { + Name: "01_add_departments_table", + Operations: migrations.Operations{ + &migrations.OpCreateTable{ + Name: "departments", + Columns: []migrations.Column{ + { + Name: "id", + Type: "serial", + Pk: true, + }, + { + Name: "name", + Type: "text", + Nullable: false, + }, + }, + }, + }, + }, + { + Name: "02_add_employees_table", + Operations: migrations.Operations{ + &migrations.OpCreateTable{ + Name: "employees", + Columns: []migrations.Column{ + { + Name: "id", + Type: "serial", + Pk: true, + }, + { + Name: "name", + Type: "text", + Nullable: false, + }, + { + Name: "department_id", + Type: "integer", + Nullable: true, + References: &migrations.ForeignKeyReference{ + Name: "fk_employee_department", + Table: "departments", + Column: "id", + }, + }, + }, + }, + }, + }, + { + Name: "03_set_unique", + Operations: migrations.Operations{ + &migrations.OpAlterColumn{ + Table: "employees", + Column: "department_id", + Unique: &migrations.UniqueConstraint{ + Name: "employees_department_id_unique", + }, + Up: "department_id", + Down: "department_id", + }, + }, + }, + }, + afterStart: func(t *testing.T, db *sql.DB) { + // A temporary FK constraint has been created on the temporary column + ConstraintMustExist(t, db, "public", "employees", migrations.TemporaryName("fk_employee_department")) + }, + afterRollback: func(t *testing.T, db *sql.DB) { + }, + afterComplete: func(t *testing.T, db *sql.DB) { + // The foreign key constraint still exists on the column + ConstraintMustExist(t, db, "public", "employees", "fk_employee_department") + }, + }, }) }