From 5686ddc4335ff2cb3a88d8e3450f5e3c6f571a15 Mon Sep 17 00:00:00 2001 From: Andrew Farries Date: Tue, 6 Feb 2024 07:39:17 +0000 Subject: [PATCH] Strengthen assertions for tests that check preservation of `UNIQUE` constraints (#277) Strengthen the tests that check for preservation of `UNIQUE` constraints when a column is duplicated so that they can check for the failure case in #273. It's not enough to test that the column does not accept duplicate values after the migration completes. Both a unique index and a unique constraint will have that effect but we need to ensure that the **constraint** is present on the table once the migration completes. Duplicating a `UNIQUE` constraint is a two-step process, first creating a `UNIQUE` index on migration start and then upgrading the index to a constraint on migration completion. #273 occurs when this upgrade fails and the column is left with the unique index but not the constraint. Subsequent migrations will then fail to duplicate the non-existent unique constraint. Part of #273 --- pkg/migrations/op_change_type_test.go | 30 ++++++++++++++------ pkg/migrations/op_common_test.go | 27 ++++++++++++++++++ pkg/migrations/op_drop_constraint_test.go | 28 ++++++++++++++----- pkg/migrations/op_drop_not_null_test.go | 26 +++++++++++++---- pkg/migrations/op_set_check_test.go | 30 ++++++++++++++------ pkg/migrations/op_set_fk_test.go | 34 ++++++++++++++++------- pkg/migrations/op_set_notnull_test.go | 26 +++++++++++++---- 7 files changed, 156 insertions(+), 45 deletions(-) diff --git a/pkg/migrations/op_change_type_test.go b/pkg/migrations/op_change_type_test.go index b6ccc126..ab9b0f0f 100644 --- a/pkg/migrations/op_change_type_test.go +++ b/pkg/migrations/op_change_type_test.go @@ -411,16 +411,27 @@ func TestChangeColumnType(t *testing.T) { Pk: ptr(true), }, { - Name: "username", - Type: "text", - Unique: ptr(true), + Name: "username", + Type: "text", }, }, }, }, }, { - Name: "02_change_type", + Name: "02_set_unique", + Operations: migrations.Operations{ + &migrations.OpAlterColumn{ + Table: "users", + Column: "username", + Unique: &migrations.UniqueConstraint{Name: "unique_username"}, + Up: ptr("username"), + Down: ptr("username"), + }, + }, + }, + { + Name: "03_change_type", Operations: migrations.Operations{ &migrations.OpAlterColumn{ Table: "users", @@ -434,25 +445,28 @@ func TestChangeColumnType(t *testing.T) { }, afterStart: func(t *testing.T, db *sql.DB, schema string) { // Inserting an initial row succeeds - MustInsert(t, db, schema, "02_change_type", "users", map[string]string{ + MustInsert(t, db, schema, "03_change_type", "users", map[string]string{ "username": "alice", }) // Inserting a row with a duplicate `username` value fails - MustNotInsert(t, db, schema, "02_change_type", "users", map[string]string{ + MustNotInsert(t, db, schema, "03_change_type", "users", map[string]string{ "username": "alice", }, testutils.UniqueViolationErrorCode) }, afterRollback: func(t *testing.T, db *sql.DB, schema string) { }, afterComplete: func(t *testing.T, db *sql.DB, schema string) { + // The table has a unique constraint defined on it + UniqueConstraintMustExist(t, db, schema, "users", "unique_username") + // Inserting a row with a duplicate `username` value fails - MustNotInsert(t, db, schema, "02_change_type", "users", map[string]string{ + MustNotInsert(t, db, schema, "03_change_type", "users", map[string]string{ "username": "alice", }, testutils.UniqueViolationErrorCode) // Inserting a row with a different `username` value succeeds - MustInsert(t, db, schema, "02_change_type", "users", map[string]string{ + MustInsert(t, db, schema, "03_change_type", "users", map[string]string{ "username": "bob", }) }, diff --git a/pkg/migrations/op_common_test.go b/pkg/migrations/op_common_test.go index 48d43229..289a9a26 100644 --- a/pkg/migrations/op_common_test.go +++ b/pkg/migrations/op_common_test.go @@ -197,6 +197,13 @@ func CheckConstraintMustExist(t *testing.T, db *sql.DB, schema, table, constrain } } +func UniqueConstraintMustExist(t *testing.T, db *sql.DB, schema, table, constraint string) { + t.Helper() + if !uniqueConstraintExists(t, db, schema, table, constraint) { + t.Fatalf("Expected unique constraint %q to exist", constraint) + } +} + func ValidatedForeignKeyMustExist(t *testing.T, db *sql.DB, schema, table, constraint string) { t.Helper() if !foreignKeyExists(t, db, schema, table, constraint, true) { @@ -286,6 +293,26 @@ func checkConstraintExists(t *testing.T, db *sql.DB, schema, table, constraint s return exists } +func uniqueConstraintExists(t *testing.T, db *sql.DB, schema, table, constraint string) bool { + t.Helper() + + var exists bool + err := db.QueryRow(` + SELECT EXISTS ( + SELECT 1 + FROM pg_catalog.pg_constraint + WHERE conrelid = $1::regclass + AND conname = $2 + AND contype = 'u' + )`, + fmt.Sprintf("%s.%s", schema, table), constraint).Scan(&exists) + if err != nil { + t.Fatal(err) + } + + return exists +} + func foreignKeyExists(t *testing.T, db *sql.DB, schema, table, constraint string, validated bool) bool { t.Helper() diff --git a/pkg/migrations/op_drop_constraint_test.go b/pkg/migrations/op_drop_constraint_test.go index b762abff..d67fddf6 100644 --- a/pkg/migrations/op_drop_constraint_test.go +++ b/pkg/migrations/op_drop_constraint_test.go @@ -690,14 +690,25 @@ func TestDropConstraint(t *testing.T) { Name: "title", Type: "text", Nullable: ptr(true), - Unique: ptr(true), }, }, }, }, }, { - Name: "02_add_check_constraint", + Name: "02_set_unique", + Operations: migrations.Operations{ + &migrations.OpAlterColumn{ + Table: "posts", + Column: "title", + Unique: &migrations.UniqueConstraint{Name: "unique_title"}, + Up: ptr("title"), + Down: ptr("title"), + }, + }, + }, + { + Name: "03_add_check_constraint", Operations: migrations.Operations{ &migrations.OpAlterColumn{ Table: "posts", @@ -712,7 +723,7 @@ func TestDropConstraint(t *testing.T) { }, }, { - Name: "03_drop_check_constraint", + Name: "04_drop_check_constraint", Operations: migrations.Operations{ &migrations.OpDropConstraint{ Table: "posts", @@ -726,25 +737,28 @@ func TestDropConstraint(t *testing.T) { }, afterStart: func(t *testing.T, db *sql.DB, schema string) { // Inserting an initial row into the `posts` table succeeds - MustInsert(t, db, schema, "03_drop_check_constraint", "posts", map[string]string{ + MustInsert(t, db, schema, "04_drop_check_constraint", "posts", map[string]string{ "title": "post by alice", }) // Inserting another row with a duplicate `title` value fails - MustNotInsert(t, db, schema, "03_drop_check_constraint", "posts", map[string]string{ + MustNotInsert(t, db, schema, "04_drop_check_constraint", "posts", map[string]string{ "title": "post by alice", }, testutils.UniqueViolationErrorCode) }, afterRollback: func(t *testing.T, db *sql.DB, schema string) { }, afterComplete: func(t *testing.T, db *sql.DB, schema string) { + // The table has a unique constraint defined on it + UniqueConstraintMustExist(t, db, schema, "posts", "unique_title") + // Inserting a row with a duplicate `title` value fails - MustNotInsert(t, db, schema, "03_drop_check_constraint", "posts", map[string]string{ + MustNotInsert(t, db, schema, "04_drop_check_constraint", "posts", map[string]string{ "title": "post by alice", }, testutils.UniqueViolationErrorCode) // Inserting a row with a different `title` value succeeds - MustInsert(t, db, schema, "03_drop_check_constraint", "posts", map[string]string{ + MustInsert(t, db, schema, "04_drop_check_constraint", "posts", map[string]string{ "title": "post by bob", }) }, diff --git a/pkg/migrations/op_drop_not_null_test.go b/pkg/migrations/op_drop_not_null_test.go index dfbc1f5b..45a19c26 100644 --- a/pkg/migrations/op_drop_not_null_test.go +++ b/pkg/migrations/op_drop_not_null_test.go @@ -434,14 +434,25 @@ func TestDropNotNull(t *testing.T) { Name: "name", Type: "text", Nullable: ptr(false), - Unique: ptr(true), }, }, }, }, }, { - Name: "02_set_not_null", + Name: "02_set_unique", + Operations: migrations.Operations{ + &migrations.OpAlterColumn{ + Table: "users", + Column: "name", + Unique: &migrations.UniqueConstraint{Name: "unique_name"}, + Up: ptr("name"), + Down: ptr("name"), + }, + }, + }, + { + Name: "03_set_not_null", Operations: migrations.Operations{ &migrations.OpAlterColumn{ Table: "users", @@ -455,25 +466,28 @@ func TestDropNotNull(t *testing.T) { }, afterStart: func(t *testing.T, db *sql.DB, schema string) { // Inserting an initial row succeeds - MustInsert(t, db, schema, "02_set_not_null", "users", map[string]string{ + MustInsert(t, db, schema, "03_set_not_null", "users", map[string]string{ "name": "alice", }) // Inserting a row with a duplicate `name` value fails - MustNotInsert(t, db, schema, "02_set_not_null", "users", map[string]string{ + MustNotInsert(t, db, schema, "03_set_not_null", "users", map[string]string{ "name": "alice", }, testutils.UniqueViolationErrorCode) }, afterRollback: func(t *testing.T, db *sql.DB, schema string) { }, afterComplete: func(t *testing.T, db *sql.DB, schema string) { + // The table has a unique constraint defined on it + UniqueConstraintMustExist(t, db, schema, "users", "unique_name") + // Inserting a row with a duplicate `name` value fails - MustNotInsert(t, db, schema, "02_set_not_null", "users", map[string]string{ + MustNotInsert(t, db, schema, "03_set_not_null", "users", map[string]string{ "name": "alice", }, testutils.UniqueViolationErrorCode) // Inserting a row with a different `name` value succeeds - MustInsert(t, db, schema, "02_set_not_null", "users", map[string]string{ + MustInsert(t, db, schema, "03_set_not_null", "users", map[string]string{ "name": "bob", }) }, diff --git a/pkg/migrations/op_set_check_test.go b/pkg/migrations/op_set_check_test.go index deba0056..093b5e76 100644 --- a/pkg/migrations/op_set_check_test.go +++ b/pkg/migrations/op_set_check_test.go @@ -428,16 +428,27 @@ func TestSetCheckConstraint(t *testing.T) { Pk: ptr(true), }, { - Name: "title", - Type: "text", - Unique: ptr(true), + Name: "title", + Type: "text", }, }, }, }, }, { - Name: "02_add_check_constraint", + Name: "02_set_unique", + Operations: migrations.Operations{ + &migrations.OpAlterColumn{ + Table: "posts", + Column: "title", + Unique: &migrations.UniqueConstraint{Name: "unique_title"}, + Up: ptr("title"), + Down: ptr("title"), + }, + }, + }, + { + Name: "03_add_check_constraint", Operations: migrations.Operations{ &migrations.OpAlterColumn{ Table: "posts", @@ -454,25 +465,28 @@ func TestSetCheckConstraint(t *testing.T) { }, afterStart: func(t *testing.T, db *sql.DB, schema string) { // Inserting an initial row succeeds - MustInsert(t, db, schema, "02_add_check_constraint", "posts", map[string]string{ + MustInsert(t, db, schema, "03_add_check_constraint", "posts", map[string]string{ "title": "post by alice", }) // Inserting a row with a duplicate `title` value fails - MustNotInsert(t, db, schema, "02_add_check_constraint", "posts", map[string]string{ + MustNotInsert(t, db, schema, "03_add_check_constraint", "posts", map[string]string{ "title": "post by alice", }, testutils.UniqueViolationErrorCode) }, afterRollback: func(t *testing.T, db *sql.DB, schema string) { }, afterComplete: func(t *testing.T, db *sql.DB, schema string) { + // The table has a unique constraint defined on it + UniqueConstraintMustExist(t, db, schema, "posts", "unique_title") + // Inserting a row with a duplicate `title` value fails - MustNotInsert(t, db, schema, "02_add_check_constraint", "posts", map[string]string{ + MustNotInsert(t, db, schema, "03_add_check_constraint", "posts", map[string]string{ "title": "post by alice", }, testutils.UniqueViolationErrorCode) // Inserting a row with a different `title` value succeeds - MustInsert(t, db, schema, "02_add_check_constraint", "posts", map[string]string{ + MustInsert(t, db, schema, "03_add_check_constraint", "posts", map[string]string{ "title": "post by bob", }) }, diff --git a/pkg/migrations/op_set_fk_test.go b/pkg/migrations/op_set_fk_test.go index 8a6f4b4a..45f7b092 100644 --- a/pkg/migrations/op_set_fk_test.go +++ b/pkg/migrations/op_set_fk_test.go @@ -552,16 +552,27 @@ func TestSetForeignKey(t *testing.T) { Type: "text", }, { - Name: "user_id", - Type: "integer", - Unique: ptr(true), + Name: "user_id", + Type: "integer", }, }, }, }, }, { - Name: "02_add_fk_constraint", + Name: "02_set_unique", + Operations: migrations.Operations{ + &migrations.OpAlterColumn{ + Table: "posts", + Column: "user_id", + Unique: &migrations.UniqueConstraint{Name: "unique_user_id"}, + Up: ptr("user_id"), + Down: ptr("user_id"), + }, + }, + }, + { + Name: "03_add_fk_constraint", Operations: migrations.Operations{ &migrations.OpAlterColumn{ Table: "posts", @@ -579,19 +590,19 @@ func TestSetForeignKey(t *testing.T) { }, afterStart: func(t *testing.T, db *sql.DB, schema string) { // Set up the users table with a reference row - MustInsert(t, db, schema, "02_add_fk_constraint", "users", map[string]string{ + MustInsert(t, db, schema, "03_add_fk_constraint", "users", map[string]string{ "name": "alice", "id": "1", }) // Inserting an initial row succeeds - MustInsert(t, db, schema, "02_add_fk_constraint", "posts", map[string]string{ + MustInsert(t, db, schema, "03_add_fk_constraint", "posts", map[string]string{ "title": "post by alice", "user_id": "1", }) // Inserting a row with a duplicate `user_id` fails. - MustNotInsert(t, db, schema, "02_add_fk_constraint", "posts", map[string]string{ + MustNotInsert(t, db, schema, "03_add_fk_constraint", "posts", map[string]string{ "title": "post by alice 2", "user_id": "1", }, testutils.UniqueViolationErrorCode) @@ -599,20 +610,23 @@ func TestSetForeignKey(t *testing.T) { afterRollback: func(t *testing.T, db *sql.DB, schema string) { }, afterComplete: func(t *testing.T, db *sql.DB, schema string) { + // The 'posts' table has a unique constraint defined on it + UniqueConstraintMustExist(t, db, schema, "posts", "unique_user_id") + // Inserting a row with a duplicate `user_id` fails - MustNotInsert(t, db, schema, "02_add_fk_constraint", "posts", map[string]string{ + MustNotInsert(t, db, schema, "03_add_fk_constraint", "posts", map[string]string{ "title": "post by alice 3", "user_id": "1", }, testutils.UniqueViolationErrorCode) // Set up the users table with another reference row - MustInsert(t, db, schema, "02_add_fk_constraint", "users", map[string]string{ + MustInsert(t, db, schema, "03_add_fk_constraint", "users", map[string]string{ "name": "bob", "id": "2", }) // Inserting a row with a different `user_id` succeeds - MustInsert(t, db, schema, "02_add_fk_constraint", "posts", map[string]string{ + MustInsert(t, db, schema, "03_add_fk_constraint", "posts", map[string]string{ "title": "post by bob", "user_id": "2", }) diff --git a/pkg/migrations/op_set_notnull_test.go b/pkg/migrations/op_set_notnull_test.go index 36571097..7ff4eae8 100644 --- a/pkg/migrations/op_set_notnull_test.go +++ b/pkg/migrations/op_set_notnull_test.go @@ -438,14 +438,25 @@ func TestSetNotNull(t *testing.T) { Name: "name", Type: "text", Nullable: ptr(true), - Unique: ptr(true), }, }, }, }, }, { - Name: "02_set_not_null", + Name: "02_set_unique", + Operations: migrations.Operations{ + &migrations.OpAlterColumn{ + Table: "users", + Column: "name", + Unique: &migrations.UniqueConstraint{Name: "unique_name"}, + Up: ptr("name"), + Down: ptr("name"), + }, + }, + }, + { + Name: "03_set_not_null", Operations: migrations.Operations{ &migrations.OpAlterColumn{ Table: "users", @@ -458,25 +469,28 @@ func TestSetNotNull(t *testing.T) { }, afterStart: func(t *testing.T, db *sql.DB, schema string) { // Inserting an initial row succeeds - MustInsert(t, db, schema, "02_set_not_null", "users", map[string]string{ + MustInsert(t, db, schema, "03_set_not_null", "users", map[string]string{ "name": "alice", }) // Inserting a row with a duplicate `name` value fails - MustNotInsert(t, db, schema, "02_set_not_null", "users", map[string]string{ + MustNotInsert(t, db, schema, "03_set_not_null", "users", map[string]string{ "name": "alice", }, testutils.UniqueViolationErrorCode) }, afterRollback: func(t *testing.T, db *sql.DB, schema string) { }, afterComplete: func(t *testing.T, db *sql.DB, schema string) { + // The table has a unique constraint defined on it + UniqueConstraintMustExist(t, db, schema, "users", "unique_name") + // Inserting a row with a duplicate `name` value fails - MustNotInsert(t, db, schema, "02_set_not_null", "users", map[string]string{ + MustNotInsert(t, db, schema, "03_set_not_null", "users", map[string]string{ "name": "alice", }, testutils.UniqueViolationErrorCode) // Inserting a row with a different `name` value succeeds - MustInsert(t, db, schema, "02_set_not_null", "users", map[string]string{ + MustInsert(t, db, schema, "03_set_not_null", "users", map[string]string{ "name": "bob", }) },