From a95dc801b7fcd96f312fc57dd14b6d05d1f5547d Mon Sep 17 00:00:00 2001 From: Andrew Farries Date: Wed, 20 Mar 2024 15:28:41 +0000 Subject: [PATCH 1/3] Improve test failure message from assertion If the column had no comment the error message if this assertion failed would be a scan error attempting to scan a NULL into a string. Use a pointer to a string so that columns that fail the assertion due to having a NULL comment have a meaningful failure message. --- pkg/migrations/op_common_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/migrations/op_common_test.go b/pkg/migrations/op_common_test.go index 993b1b5c..11a8bd81 100644 --- a/pkg/migrations/op_common_test.go +++ b/pkg/migrations/op_common_test.go @@ -484,7 +484,7 @@ func columnHasType(t *testing.T, db *sql.DB, schema, table, column, expectedType func columnHasComment(t *testing.T, db *sql.DB, schema, table, column, expectedComment string) bool { t.Helper() - var actualComment string + var actualComment *string err := db.QueryRow(fmt.Sprintf(` SELECT col_description( %[1]s::regclass, @@ -497,7 +497,7 @@ func columnHasComment(t *testing.T, db *sql.DB, schema, table, column, expectedC t.Fatal(err) } - return expectedComment == actualComment + return actualComment != nil && *actualComment == expectedComment } func tableHasComment(t *testing.T, db *sql.DB, schema, table, expectedComment string) bool { From 0ed3313d289709705050bcc881272f2a00149a9a Mon Sep 17 00:00:00 2001 From: Andrew Farries Date: Wed, 20 Mar 2024 15:18:24 +0000 Subject: [PATCH 2/3] Preserve column comments on duplication --- pkg/migrations/duplicate.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/pkg/migrations/duplicate.go b/pkg/migrations/duplicate.go index 0173b026..7bcd58da 100644 --- a/pkg/migrations/duplicate.go +++ b/pkg/migrations/duplicate.go @@ -58,6 +58,7 @@ func (d *Duplicator) Duplicate(ctx context.Context) error { cAddForeignKeySQL = `ADD CONSTRAINT %s FOREIGN KEY (%s) REFERENCES %s (%s) ON DELETE %s` cAddCheckConstraintSQL = `ADD CONSTRAINT %s %s NOT VALID` cCreateUniqueIndexSQL = `CREATE UNIQUE INDEX CONCURRENTLY %s ON %s (%s)` + cCommentOnColumnSQL = `COMMENT ON COLUMN %s.%s IS %s` ) // Generate SQL to duplicate the column's name and type @@ -116,6 +117,20 @@ func (d *Duplicator) Duplicate(ctx context.Context) error { return err } + // Generate SQL to duplicate the column's comment + if d.column.Comment != "" { + sql = fmt.Sprintf(cCommentOnColumnSQL, + pq.QuoteIdentifier(d.table.Name), + pq.QuoteIdentifier(d.asName), + pq.QuoteLiteral(d.column.Comment), + ) + + _, err = d.conn.ExecContext(ctx, sql) + if err != nil { + return err + } + } + // Generate SQL to duplicate any unique constraints on the column // The constraint is duplicated by adding a unique index on the column concurrently. // The index is converted into a unique constraint on migration completion. From 6515f952f02f34659f80ed6815304dd293e9d332 Mon Sep 17 00:00:00 2001 From: Andrew Farries Date: Wed, 20 Mar 2024 15:52:02 +0000 Subject: [PATCH 3/3] Add tests for comment preservation --- pkg/migrations/op_change_type_test.go | 47 +++++++++++++++ pkg/migrations/op_drop_constraint_test.go | 49 ++++++++++++++++ pkg/migrations/op_drop_not_null_test.go | 48 ++++++++++++++++ pkg/migrations/op_set_check_test.go | 50 ++++++++++++++++ pkg/migrations/op_set_fk_test.go | 69 +++++++++++++++++++++++ pkg/migrations/op_set_notnull_test.go | 47 +++++++++++++++ pkg/migrations/op_set_unique_test.go | 58 +++++++++++++++++++ 7 files changed, 368 insertions(+) diff --git a/pkg/migrations/op_change_type_test.go b/pkg/migrations/op_change_type_test.go index ff7871f5..8cef798d 100644 --- a/pkg/migrations/op_change_type_test.go +++ b/pkg/migrations/op_change_type_test.go @@ -472,6 +472,53 @@ func TestChangeColumnType(t *testing.T) { }) }, }, + { + name: "changing column type preserves any comments on the column", + migrations: []migrations.Migration{ + { + Name: "01_add_table", + Operations: migrations.Operations{ + &migrations.OpCreateTable{ + Name: "users", + Columns: []migrations.Column{ + { + Name: "id", + Type: "serial", + Pk: ptr(true), + }, + { + Name: "username", + Type: "text", + Comment: ptr("the name of the user"), + }, + }, + }, + }, + }, + { + Name: "02_change_type", + Operations: migrations.Operations{ + &migrations.OpAlterColumn{ + Table: "users", + Column: "username", + Type: ptr("varchar(255)"), + Up: ptr("username"), + Down: ptr("username"), + }, + }, + }, + }, + afterStart: func(t *testing.T, db *sql.DB, schema string) { + // The duplicated column has a comment defined on it + ColumnMustHaveComment(t, db, schema, "users", migrations.TemporaryName("username"), "the name of the user") + }, + afterRollback: func(t *testing.T, db *sql.DB, schema string) { + }, + afterComplete: func(t *testing.T, db *sql.DB, schema string) { + // The final column has a comment defined on it + ColumnMustHaveComment(t, db, schema, "users", "username", "the name of the user") + }, + }, }) } diff --git a/pkg/migrations/op_drop_constraint_test.go b/pkg/migrations/op_drop_constraint_test.go index d67fddf6..6ef0bb73 100644 --- a/pkg/migrations/op_drop_constraint_test.go +++ b/pkg/migrations/op_drop_constraint_test.go @@ -815,6 +815,55 @@ func TestDropConstraint(t *testing.T) { }, testutils.NotNullViolationErrorCode) }, }, + { + name: "dropping a unique constraint preserves any comment on the column", + migrations: []migrations.Migration{ + { + Name: "01_add_table", + Operations: migrations.Operations{ + &migrations.OpCreateTable{ + Name: "posts", + Columns: []migrations.Column{ + { + Name: "id", + Type: "serial", + Pk: ptr(true), + }, + { + Name: "title", + Type: "text", + Unique: ptr(true), + Nullable: ptr(false), + Comment: ptr("the title of the post"), + }, + }, + }, + }, + }, + { + Name: "02_drop_unique_constraint", + Operations: migrations.Operations{ + &migrations.OpDropConstraint{ + Table: "posts", + Column: "title", + Name: "_pgroll_new_posts_title_key", + Up: "title", + Down: "title", + }, + }, + }, + }, + afterStart: func(t *testing.T, db *sql.DB, schema string) { + // The duplicated column has a comment defined on it + ColumnMustHaveComment(t, db, schema, "posts", migrations.TemporaryName("title"), "the title of the post") + }, + afterRollback: func(t *testing.T, db *sql.DB, schema string) { + }, + afterComplete: func(t *testing.T, db *sql.DB, schema string) { + // The final column has a comment defined on it + ColumnMustHaveComment(t, db, schema, "posts", "title", "the title of the post") + }, + }, }) } diff --git a/pkg/migrations/op_drop_not_null_test.go b/pkg/migrations/op_drop_not_null_test.go index 45a19c26..0b91aa4e 100644 --- a/pkg/migrations/op_drop_not_null_test.go +++ b/pkg/migrations/op_drop_not_null_test.go @@ -492,6 +492,54 @@ func TestDropNotNull(t *testing.T) { }) }, }, + { + name: "dropping not null retains any comments defined on the column", + migrations: []migrations.Migration{ + { + Name: "01_add_table", + Operations: migrations.Operations{ + &migrations.OpCreateTable{ + Name: "users", + Columns: []migrations.Column{ + { + Name: "id", + Type: "serial", + Pk: ptr(true), + }, + { + Name: "name", + Type: "text", + Nullable: ptr(false), + Comment: ptr("the name of the user"), + }, + }, + }, + }, + }, + { + Name: "02_set_not_null", + Operations: migrations.Operations{ + &migrations.OpAlterColumn{ + Table: "users", + Column: "name", + Nullable: ptr(true), + Up: ptr("name"), + Down: ptr("(SELECT CASE WHEN name IS NULL THEN 'anonymous' ELSE name END)"), + }, + }, + }, + }, + afterStart: func(t *testing.T, db *sql.DB, schema string) { + // The duplicated column has a comment defined on it + ColumnMustHaveComment(t, db, schema, "users", migrations.TemporaryName("name"), "the name of the user") + }, + afterRollback: func(t *testing.T, db *sql.DB, schema string) { + }, + afterComplete: func(t *testing.T, db *sql.DB, schema string) { + // The final column has a comment defined on it + ColumnMustHaveComment(t, db, schema, "users", "name", "the name of the user") + }, + }, }) } diff --git a/pkg/migrations/op_set_check_test.go b/pkg/migrations/op_set_check_test.go index 6c03434f..cc397abf 100644 --- a/pkg/migrations/op_set_check_test.go +++ b/pkg/migrations/op_set_check_test.go @@ -492,6 +492,56 @@ func TestSetCheckConstraint(t *testing.T) { }) }, }, + { + name: "comments 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: ptr(true), + }, + { + Name: "title", + Type: "text", + Comment: ptr("the title of the post"), + }, + }, + }, + }, + }, + { + 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: ptr("(SELECT CASE WHEN length(title) <= 3 THEN LPAD(title, 4, '-') ELSE title END)"), + Down: ptr("title"), + }, + }, + }, + }, + afterStart: func(t *testing.T, db *sql.DB, schema string) { + // The duplicated column has a comment defined on it + ColumnMustHaveComment(t, db, schema, "posts", migrations.TemporaryName("title"), "the title of the post") + }, + afterRollback: func(t *testing.T, db *sql.DB, schema string) { + }, + afterComplete: func(t *testing.T, db *sql.DB, schema string) { + // The final column has a comment defined on it + ColumnMustHaveComment(t, db, schema, "posts", "title", "the title of the post") + }, + }, }) } diff --git a/pkg/migrations/op_set_fk_test.go b/pkg/migrations/op_set_fk_test.go index f8834c73..52e89476 100644 --- a/pkg/migrations/op_set_fk_test.go +++ b/pkg/migrations/op_set_fk_test.go @@ -1050,6 +1050,75 @@ func TestSetForeignKey(t *testing.T) { }) }, }, + { + name: "comments are preserved when adding a foreign key constraint", + migrations: []migrations.Migration{ + { + Name: "01_add_tables", + Operations: migrations.Operations{ + &migrations.OpCreateTable{ + Name: "users", + Columns: []migrations.Column{ + { + Name: "id", + Type: "serial", + Pk: ptr(true), + }, + { + Name: "name", + Type: "text", + }, + }, + }, + &migrations.OpCreateTable{ + Name: "posts", + Columns: []migrations.Column{ + { + Name: "id", + Type: "serial", + Pk: ptr(true), + }, + { + Name: "title", + Type: "text", + }, + { + Name: "user_id", + Type: "integer", + Comment: ptr("the id of the author"), + }, + }, + }, + }, + }, + { + Name: "02_add_fk_constraint", + Operations: migrations.Operations{ + &migrations.OpAlterColumn{ + Table: "posts", + Column: "user_id", + References: &migrations.ForeignKeyReference{ + Name: "fk_users_id", + Table: "users", + Column: "id", + }, + Up: ptr("(SELECT CASE WHEN EXISTS (SELECT 1 FROM users WHERE users.id = user_id) THEN user_id ELSE NULL END)"), + Down: ptr("user_id"), + }, + }, + }, + }, + afterStart: func(t *testing.T, db *sql.DB, schema string) { + // The duplicated column has a comment defined on it + ColumnMustHaveComment(t, db, schema, "posts", migrations.TemporaryName("user_id"), "the id of the author") + }, + afterRollback: func(t *testing.T, db *sql.DB, schema string) { + }, + afterComplete: func(t *testing.T, db *sql.DB, schema string) { + // The final column has a comment defined on it + ColumnMustHaveComment(t, db, schema, "posts", "user_id", "the id of the author") + }, + }, }) } diff --git a/pkg/migrations/op_set_notnull_test.go b/pkg/migrations/op_set_notnull_test.go index e1902162..6c0535ee 100644 --- a/pkg/migrations/op_set_notnull_test.go +++ b/pkg/migrations/op_set_notnull_test.go @@ -496,6 +496,53 @@ func TestSetNotNull(t *testing.T) { }) }, }, + { + name: "setting a column to not null retains any comment defined on the column", + migrations: []migrations.Migration{ + { + Name: "01_add_table", + Operations: migrations.Operations{ + &migrations.OpCreateTable{ + Name: "users", + Columns: []migrations.Column{ + { + Name: "id", + Type: "serial", + Pk: ptr(true), + }, + { + Name: "name", + Type: "text", + Nullable: ptr(true), + Comment: ptr("the name of the user"), + }, + }, + }, + }, + }, + { + Name: "02_set_not_null", + Operations: migrations.Operations{ + &migrations.OpAlterColumn{ + Table: "users", + Column: "name", + Nullable: ptr(false), + Up: ptr("(SELECT CASE WHEN name IS NULL THEN 'anonymous' ELSE name END)"), + }, + }, + }, + }, + afterStart: func(t *testing.T, db *sql.DB, schema string) { + // The duplicated column has a comment defined on it + ColumnMustHaveComment(t, db, schema, "users", migrations.TemporaryName("name"), "the name of the user") + }, + afterRollback: func(t *testing.T, db *sql.DB, schema string) { + }, + afterComplete: func(t *testing.T, db *sql.DB, schema string) { + // The final column has a comment defined on it + ColumnMustHaveComment(t, db, schema, "users", "name", "the name of the user") + }, + }, }) } diff --git a/pkg/migrations/op_set_unique_test.go b/pkg/migrations/op_set_unique_test.go index b18d294b..9e60b650 100644 --- a/pkg/migrations/op_set_unique_test.go +++ b/pkg/migrations/op_set_unique_test.go @@ -463,6 +463,64 @@ func TestSetColumnUnique(t *testing.T) { }, testutils.NotNullViolationErrorCode) }, }, + { + name: "comments 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: ptr(true), + }, + { + Name: "username", + Type: "text", + Nullable: ptr(false), + Comment: ptr("the name of the user"), + }, + { + 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: ptr("username"), + Down: ptr("username"), + }, + }, + }, + }, + afterStart: func(t *testing.T, db *sql.DB, schema string) { + // The duplicated column has a comment defined on it + ColumnMustHaveComment(t, db, schema, "reviews", migrations.TemporaryName("username"), "the name of the user") + }, + afterRollback: func(t *testing.T, db *sql.DB, schema string) { + }, + afterComplete: func(t *testing.T, db *sql.DB, schema string) { + // The final column has a comment defined on it + ColumnMustHaveComment(t, db, schema, "reviews", "username", "the name of the user") + }, + }, // It should be possible to add multiple unique constraints to a column // once unique constraints covering multiple columns are supported. //