Fix adding extra unique constraint to columns that are already unique (#579)

Previously, `pgroll` failed to add unique constraint to columns that
were already included in another unique constraint in some cases. The
bug is triggered when the name of the column that is not yet unique
comes earlier than the already unique column.

The error message we get:

```
Failed to complete migration: unable to execute complete operation: failed to create unique constraint from index "_pgroll_dup_fruits_name_key": pq: index "fruits_name_key" does not exist
```

The issue is during renaming phase. `pgroll` checked if the unique index
is duplicated (thus, has to be renamed). Then `pgroll` checked if the
column (being renamed) is included in the index. If it was part of the
index, `pgroll` renamed the index to the final name. If that unique
index happened to be a unique constraint `pgroll` transformed the index
into a unique constraint. Good, yay.

However, if the unique index is duplicated, but the column (that was
being renamed) was not part of the index definition, `pgroll` did not
rename the index. If this unique index is a constraint, `pgroll` still
tried to transform the index with the original name into a constraint.
So we got the error message above.

Now this problem is fixed.

Related to #227

Author: kvch

pkg/migrations/op_create_constraint_test.go

@@ -495,6 +495,106 @@ func TestCreateConstraint(t *testing.T) {
 				}, rows)
 			},
 		},
+		{
+			name: "create unique constraint on a unique column and another column",
+			migrations: []migrations.Migration{
+				{
+					Name: "01_add_table",
+					Operations: migrations.Operations{
+						&migrations.OpCreateTable{
+							Name: "users",
+							Columns: []migrations.Column{
+								{
+									Name: "id",
+									Type: "serial",
+									Pk:   true,
+								},
+								{
+									Name:     "name",
+									Type:     "varchar(255)",
+									Nullable: false,
+								},
+								{
+									Name:     "email",
+									Type:     "varchar(255)",
+									Nullable: true,
+								},
+							},
+							Constraints: []migrations.Constraint{
+								{
+									Name:    "unique_name",
+									Type:    "unique",
+									Columns: []string{"name"},
+								},
+							},
+						},
+					},
+				},
+				{
+					Name: "02_create_constraint",
+					Operations: migrations.Operations{
+						&migrations.OpCreateConstraint{
+							Name:    "unique_name_email",
+							Table:   "users",
+							Type:    "unique",
+							Columns: []string{"email", "name"},
+							Up: map[string]string{
+								"name":  "name || random()",
+								"email": "email || random()",
+							},
+							Down: map[string]string{
+								"name":  "name",
+								"email": "email",
+							},
+						},
+					},
+				},
+			},
+			afterStart: func(t *testing.T, db *sql.DB, schema string) {
+				// The index has been created on the underlying table.
+				IndexMustExist(t, db, schema, "users", "unique_name")
+				IndexMustExist(t, db, schema, "users", "unique_name_email")
+
+				// Inserting values into the old schema that violate uniqueness should succeed.
+				MustInsert(t, db, schema, "01_add_table", "users", map[string]string{
+					"name":  "alice",
+					"email": "email",
+				})
+				MustInsert(t, db, schema, "01_add_table", "users", map[string]string{
+					"name":  "bob",
+					"email": "email",
+				})
+
+				// Inserting values into the new schema that violate uniqueness should fail.
+				MustInsert(t, db, schema, "02_create_constraint", "users", map[string]string{
+					"name":  "cat",
+					"email": "email",
+				})
+				MustNotInsert(t, db, schema, "02_create_constraint", "users", map[string]string{
+					"name":  "cat",
+					"email": "email",
+				}, testutils.UniqueViolationErrorCode)
+			},
+			afterRollback: func(t *testing.T, db *sql.DB, schema string) {
+				// The index has been dropped from the the underlying table.
+				IndexMustNotExist(t, db, schema, "users", "unique_name_email")
+
+				// Functions, triggers and temporary columns are dropped.
+				TableMustBeCleanedUp(t, db, schema, "users", "name", "email")
+			},
+			afterComplete: func(t *testing.T, db *sql.DB, schema string) {
+				// Functions, triggers and temporary columns are dropped.
+				TableMustBeCleanedUp(t, db, schema, "users", "name", "email")
+
+				// Inserting values into the new schema that violate uniqueness should fail.
+				MustInsert(t, db, schema, "02_create_constraint", "users", map[string]string{
+					"name": "carol",
+				})
+				MustNotInsert(t, db, schema, "02_create_constraint", "users", map[string]string{
+					"name": "carol",
+				}, testutils.UniqueViolationErrorCode)
+			},
+		},
 	})
 }
 

pkg/migrations/rename.go

@@ -123,26 +123,24 @@ func RenameDuplicatedColumn(ctx context.Context, conn db.DB, table *schema.Table
 	// Rename any indexes on the duplicated column and use unique indexes to
 	// create `UNIQUE` constraints.
 	for _, idx := range table.Indexes {
-		if !IsDuplicatedName(idx.Name) {
+		if !IsDuplicatedName(idx.Name) || !slices.Contains(idx.Columns, TemporaryName(column.Name)) {
 			continue
 		}
 
-		if slices.Contains(idx.Columns, TemporaryName(column.Name)) {
-			// Rename the index to its original name
-			renameIndexSQL := fmt.Sprintf(cRenameIndexSQL,
-				pq.QuoteIdentifier(idx.Name),
-				pq.QuoteIdentifier(StripDuplicationPrefix(idx.Name)),
-			)
+		// Rename the index to its original name
+		renameIndexSQL := fmt.Sprintf(cRenameIndexSQL,
+			pq.QuoteIdentifier(idx.Name),
+			pq.QuoteIdentifier(StripDuplicationPrefix(idx.Name)),
+		)
 
-			_, err = conn.ExecContext(ctx, renameIndexSQL)
-			if err != nil {
-				return fmt.Errorf("failed to rename index %q: %w", idx.Name, err)
-			}
-
-			// Index no longer exists, remove it from the table
-			delete(table.Indexes, idx.Name)
+		_, err = conn.ExecContext(ctx, renameIndexSQL)
+		if err != nil {
+			return fmt.Errorf("failed to rename index %q: %w", idx.Name, err)
 		}
 
+		// Index no longer exists, remove it from the table
+		delete(table.Indexes, idx.Name)
+
 		if _, ok := table.UniqueConstraints[StripDuplicationPrefix(idx.Name)]; idx.Unique && ok {
 			// Create a unique constraint using the unique index
 			createUniqueConstraintSQL := fmt.Sprintf(cCreateUniqueConstraintSQL,
@@ -156,7 +154,6 @@ func RenameDuplicatedColumn(ctx context.Context, conn db.DB, table *schema.Table
 				return fmt.Errorf("failed to create unique constraint from index %q: %w", idx.Name, err)
 			}
 		}
-
 	}
 
 	return nil