From a4222d8f8c2326ea2213fa9dd95844679d9b02f4 Mon Sep 17 00:00:00 2001 From: Andrew Farries Date: Mon, 22 Apr 2024 12:11:15 +0100 Subject: [PATCH] Support multiple 'alter column' sub operations (#338) Allow 'alter column' operations to include multiple sub-operations. This means migrations like this one are now possible: ```json { "name": "35_alter_column_multiple", "operations": [ { "alter_column": { "table": "events", "column": "name", "name": "event_name", "type": "text", "nullable": false, "unique": { "name": "events_event_name_unique" }, "check": { "name": "event_name_length", "constraint": "length(name) > 3" }, "up": "(SELECT CASE WHEN name IS NULL THEN 'placeholder' ELSE name END)", "down": "name" } } ] } ``` This 'alter column' operation: * Renames a column * Changes its type * Sets it `NOT NULL` * Adds a unique constraint * Adds a check constraint Previously, this would have required 5 different operations. Builds on https://github.com/xataio/pgroll/pull/337. Fixes https://github.com/xataio/pgroll/issues/336 --- docs/README.md | 2 + examples/34_create_events_table.json | 22 ++ examples/35_alter_column_multiple.json | 23 ++ pkg/jsonschema/testdata/alter-column-3.txtar | 2 +- pkg/jsonschema/testdata/alter-column-4.txtar | 2 +- pkg/jsonschema/testdata/alter-column-5.txtar | 2 +- pkg/jsonschema/testdata/alter-column-6.txtar | 4 +- pkg/jsonschema/testdata/alter-column-7.txtar | 4 +- pkg/jsonschema/testdata/alter-column-8.txtar | 4 +- pkg/jsonschema/testdata/alter-column-9.txtar | 18 + pkg/migrations/errors.go | 17 +- pkg/migrations/op_alter_column.go | 231 ++++++----- pkg/migrations/op_alter_column_test.go | 390 ++++++++++++++++++- pkg/migrations/op_rename_column.go | 50 --- schema.json | 13 +- 15 files changed, 583 insertions(+), 201 deletions(-) create mode 100644 examples/34_create_events_table.json create mode 100644 examples/35_alter_column_multiple.json create mode 100644 pkg/jsonschema/testdata/alter-column-9.txtar delete mode 100644 pkg/migrations/op_rename_column.go diff --git a/docs/README.md b/docs/README.md index 9b868192..8658604e 100644 --- a/docs/README.md +++ b/docs/README.md @@ -738,6 +738,8 @@ Example **add column** migrations: An alter column operation alters the properties of a column. The operation supports several sub-operations, described below. +An alter column operation may contain multiple sub-operations. For example, a single alter column operation may rename a column, change its type, and add a check constraint. + #### Rename column A rename column operation renames a column. diff --git a/examples/34_create_events_table.json b/examples/34_create_events_table.json new file mode 100644 index 00000000..232995e9 --- /dev/null +++ b/examples/34_create_events_table.json @@ -0,0 +1,22 @@ +{ + "name": "34_create_events_table", + "operations": [ + { + "create_table": { + "name": "events", + "columns": [ + { + "name": "id", + "type": "serial", + "pk": true + }, + { + "name": "name", + "type": "varchar(255)", + "nullable": true + } + ] + } + } + ] +} diff --git a/examples/35_alter_column_multiple.json b/examples/35_alter_column_multiple.json new file mode 100644 index 00000000..89c59eb4 --- /dev/null +++ b/examples/35_alter_column_multiple.json @@ -0,0 +1,23 @@ +{ + "name": "35_alter_column_multiple", + "operations": [ + { + "alter_column": { + "table": "events", + "column": "name", + "name": "event_name", + "type": "text", + "nullable": false, + "unique": { + "name": "events_event_name_unique" + }, + "check": { + "name": "event_name_length", + "constraint": "length(name) > 3" + }, + "up": "(SELECT CASE WHEN name IS NULL OR LENGTH(name) <= 3 THEN 'placeholder' ELSE name END)", + "down": "name" + } + } + ] +} diff --git a/pkg/jsonschema/testdata/alter-column-3.txtar b/pkg/jsonschema/testdata/alter-column-3.txtar index f93b8179..1472f72d 100644 --- a/pkg/jsonschema/testdata/alter-column-3.txtar +++ b/pkg/jsonschema/testdata/alter-column-3.txtar @@ -1,5 +1,5 @@ This is an invalid 'alter_column' migration. -It sets `name` but also sets `up` and `down`. +It sets only `name` but also sets `up` and `down`. -- alter_column.json -- { diff --git a/pkg/jsonschema/testdata/alter-column-4.txtar b/pkg/jsonschema/testdata/alter-column-4.txtar index fe878c58..3f380760 100644 --- a/pkg/jsonschema/testdata/alter-column-4.txtar +++ b/pkg/jsonschema/testdata/alter-column-4.txtar @@ -1,5 +1,5 @@ This is an invalid 'alter_column' migration. -It sets `name` but also sets `up`. +It sets only `name` but also sets `up`. -- alter_column.json -- { diff --git a/pkg/jsonschema/testdata/alter-column-5.txtar b/pkg/jsonschema/testdata/alter-column-5.txtar index 46408812..f6117f3d 100644 --- a/pkg/jsonschema/testdata/alter-column-5.txtar +++ b/pkg/jsonschema/testdata/alter-column-5.txtar @@ -1,5 +1,5 @@ This is an invalid 'alter_column' migration. -It sets `name` but also sets `down`. +It sets only `name` but also sets `down`. -- alter_column.json -- { diff --git a/pkg/jsonschema/testdata/alter-column-6.txtar b/pkg/jsonschema/testdata/alter-column-6.txtar index 9c80f90d..68b54f83 100644 --- a/pkg/jsonschema/testdata/alter-column-6.txtar +++ b/pkg/jsonschema/testdata/alter-column-6.txtar @@ -1,4 +1,4 @@ -This is an invalid 'alter_column' migration. +This is a valid 'alter_column' migration. It sets both `name` and `nullable`. -- alter_column.json -- @@ -19,4 +19,4 @@ It sets both `name` and `nullable`. } -- valid -- -false +true diff --git a/pkg/jsonschema/testdata/alter-column-7.txtar b/pkg/jsonschema/testdata/alter-column-7.txtar index d2509592..6b3c233f 100644 --- a/pkg/jsonschema/testdata/alter-column-7.txtar +++ b/pkg/jsonschema/testdata/alter-column-7.txtar @@ -1,5 +1,5 @@ -This is an invalid 'alter_column' migration. -It sets both `name` and `nullable`. +This is a invalid 'alter_column' migration. +It sets both `name` and `nullable` but does not set `up` or `down`. -- alter_column.json -- { diff --git a/pkg/jsonschema/testdata/alter-column-8.txtar b/pkg/jsonschema/testdata/alter-column-8.txtar index cb9a54fb..4fe6ba3e 100644 --- a/pkg/jsonschema/testdata/alter-column-8.txtar +++ b/pkg/jsonschema/testdata/alter-column-8.txtar @@ -1,4 +1,4 @@ -This is an invalid 'alter_column' migration. +This is a valid 'alter_column' migration. It sets both `type` and `nullable`. -- alter_column.json -- @@ -19,4 +19,4 @@ It sets both `type` and `nullable`. } -- valid -- -false +true diff --git a/pkg/jsonschema/testdata/alter-column-9.txtar b/pkg/jsonschema/testdata/alter-column-9.txtar new file mode 100644 index 00000000..234a9a59 --- /dev/null +++ b/pkg/jsonschema/testdata/alter-column-9.txtar @@ -0,0 +1,18 @@ +This is an invalid 'alter_column' migration. +It specifies a table and column but no changes. + +-- alter_column.json -- +{ + "name": "migration_name", + "operations": [ + { + "alter_column": { + "table": "reviews", + "column": "review" + } + } + ] +} + +-- valid -- +false diff --git a/pkg/migrations/errors.go b/pkg/migrations/errors.go index 45012c4c..f0232848 100644 --- a/pkg/migrations/errors.go +++ b/pkg/migrations/errors.go @@ -158,14 +158,6 @@ func (e NoDownSQLAllowedError) Error() string { return "down SQL is not allowed for this operation" } -type MultipleAlterColumnChangesError struct { - Changes int -} - -func (e MultipleAlterColumnChangesError) Error() string { - return fmt.Sprintf("alter column operations require exactly one change, found %d", e.Changes) -} - type BackfillNotPossibleError struct { Table string } @@ -199,3 +191,12 @@ func (e InvalidOnDeleteSettingError) Error() string { e.Setting, ) } + +type AlterColumnNoChangesError struct { + Table string + Column string +} + +func (e AlterColumnNoChangesError) Error() string { + return fmt.Sprintf("alter column %q on table %q requires at least one change", e.Column, e.Table) +} diff --git a/pkg/migrations/op_alter_column.go b/pkg/migrations/op_alter_column.go index d076fccf..a4780d59 100644 --- a/pkg/migrations/op_alter_column.go +++ b/pkg/migrations/op_alter_column.go @@ -16,27 +16,27 @@ var _ Operation = (*OpAlterColumn)(nil) func (o *OpAlterColumn) Start(ctx context.Context, conn *sql.DB, stateSchema string, tr SQLTransformer, s *schema.Schema, cbs ...CallbackFn) (*schema.Table, error) { table := s.GetTable(o.Table) column := table.GetColumn(o.Column) + ops := o.subOperations() - op := o.innerOperation() - - if _, ok := op.(*OpRenameColumn); !ok { - // Duplicate the column on the underlying table. - d := duplicatorForOperation(o.innerOperation(), conn, table, column) + // Duplicate the column on the underlying table. + if !o.isRenameOnly() { + d := duplicatorForOperations(ops, conn, table, column) if err := d.Duplicate(ctx); err != nil { return nil, fmt.Errorf("failed to duplicate column: %w", err) } } // perform any operation specific start steps - tbl, err := op.Start(ctx, conn, stateSchema, tr, s, cbs...) - if err != nil { - return nil, err + for _, op := range ops { + if _, err := op.Start(ctx, conn, stateSchema, tr, s, cbs...); err != nil { + return nil, err + } } // Add a trigger to copy values from the old column to the new, rewriting values using the `up` SQL. // Rename column operations do not require this trigger. - if _, ok := op.(*OpRenameColumn); !ok { - err = createTrigger(ctx, conn, tr, triggerConfig{ + if !o.isRenameOnly() { + err := createTrigger(ctx, conn, tr, triggerConfig{ Name: TriggerName(o.Table, o.Column), Direction: TriggerDirectionUp, Columns: table.Columns, @@ -44,7 +44,7 @@ func (o *OpAlterColumn) Start(ctx context.Context, conn *sql.DB, stateSchema str TableName: o.Table, PhysicalColumn: TemporaryName(o.Column), StateSchema: stateSchema, - SQL: o.upSQLForOperation(op), + SQL: o.upSQLForOperations(ops), }) if err != nil { return nil, fmt.Errorf("failed to create up trigger: %w", err) @@ -66,25 +66,35 @@ func (o *OpAlterColumn) Start(ctx context.Context, conn *sql.DB, stateSchema str TableName: o.Table, PhysicalColumn: o.Column, StateSchema: stateSchema, - SQL: o.downSQLForOperation(op), + SQL: o.downSQLForOperations(ops), }) if err != nil { return nil, fmt.Errorf("failed to create down trigger: %w", err) } } - return tbl, nil + // rename the column in the virtual schema if required + if o.Name != nil { + table.RenameColumn(o.Column, *o.Name) + } + + if o.isRenameOnly() { + return nil, nil + } + return table, nil } func (o *OpAlterColumn) Complete(ctx context.Context, conn *sql.DB, tr SQLTransformer, s *schema.Schema) error { - op := o.innerOperation() + ops := o.subOperations() // Perform any operation specific completion steps - if err := op.Complete(ctx, conn, tr, s); err != nil { - return err + for _, op := range ops { + if err := op.Complete(ctx, conn, tr, s); err != nil { + return err + } } - if _, ok := op.(*OpRenameColumn); !ok { + if !o.isRenameOnly() { // Drop the old column _, err := conn.ExecContext(ctx, fmt.Sprintf("ALTER TABLE IF EXISTS %s DROP COLUMN IF EXISTS %s", pq.QuoteIdentifier(o.Table), @@ -115,18 +125,31 @@ func (o *OpAlterColumn) Complete(ctx context.Context, conn *sql.DB, tr SQLTransf } } + // rename the column in the underlying table if required + if o.Name != nil { + _, err := conn.ExecContext(ctx, fmt.Sprintf("ALTER TABLE %s RENAME COLUMN %s TO %s", + pq.QuoteIdentifier(o.Table), + pq.QuoteIdentifier(o.Column), + pq.QuoteIdentifier(*o.Name))) + if err != nil { + return err + } + } + return nil } func (o *OpAlterColumn) Rollback(ctx context.Context, conn *sql.DB, tr SQLTransformer) error { - op := o.innerOperation() + ops := o.subOperations() // Perform any operation specific rollback steps - if err := op.Rollback(ctx, conn, tr); err != nil { - return err + for _, ops := range ops { + if err := ops.Rollback(ctx, conn, tr); err != nil { + return err + } } - if _, ok := op.(*OpRenameColumn); !ok { + if !o.isRenameOnly() { // Drop the new column _, err := conn.ExecContext(ctx, fmt.Sprintf("ALTER TABLE %s DROP COLUMN IF EXISTS %s", pq.QuoteIdentifier(o.Table), @@ -157,11 +180,6 @@ func (o *OpAlterColumn) Rollback(ctx context.Context, conn *sql.DB, tr SQLTransf } func (o *OpAlterColumn) Validate(ctx context.Context, s *schema.Schema) error { - // Ensure that the operation describes only one change to the column - if cnt := o.numChanges(); cnt != 1 { - return MultipleAlterColumnChangesError{Changes: cnt} - } - // Validate that the table and column exist table := s.GetTable(o.Table) if table == nil { @@ -177,9 +195,23 @@ func (o *OpAlterColumn) Validate(ctx context.Context, s *schema.Schema) error { return err } - // Apply any special validation rules for the inner operation - op := o.innerOperation() - if _, ok := op.(*OpRenameColumn); ok { + // If the column is being renamed, ensure that the target column name does + // not already exist. + if o.Name != nil { + if table.GetColumn(*o.Name) != nil { + return ColumnAlreadyExistsError{Table: o.Table, Name: *o.Name} + } + } + + ops := o.subOperations() + + // Ensure that at least one sub-operation or rename is present + if len(ops) == 0 && o.Name == nil { + return AlterColumnNoChangesError{Table: o.Table, Column: o.Column} + } + + // Rename-only operations are not allowed to have `up` or `down` SQL + if o.isRenameOnly() { if o.Up != "" { return NoUpSQLAllowedError{} } @@ -188,139 +220,124 @@ func (o *OpAlterColumn) Validate(ctx context.Context, s *schema.Schema) error { } } - // Validate the inner operation in isolation - return op.Validate(ctx, s) + // Validate the sub-operations in isolation + for _, op := range ops { + if err := op.Validate(ctx, s); err != nil { + return err + } + } + + return nil } -func (o *OpAlterColumn) innerOperation() Operation { - switch { - case o.Name != nil: - return &OpRenameColumn{ - Table: o.Table, - From: o.Column, - To: *o.Name, - } +func (o *OpAlterColumn) subOperations() []Operation { + var ops []Operation - case o.Type != nil: - return &OpChangeType{ + if o.Type != nil { + ops = append(ops, &OpChangeType{ Table: o.Table, Column: o.Column, Type: *o.Type, Up: o.Up, Down: o.Down, - } - - case o.Check != nil: - return &OpSetCheckConstraint{ + }) + } + if o.Check != nil { + ops = append(ops, &OpSetCheckConstraint{ Table: o.Table, Column: o.Column, Check: *o.Check, Up: o.Up, Down: o.Down, - } - - case o.References != nil: - return &OpSetForeignKey{ + }) + } + if o.References != nil { + ops = append(ops, &OpSetForeignKey{ Table: o.Table, Column: o.Column, References: *o.References, Up: o.Up, Down: o.Down, - } - - case o.Nullable != nil && !*o.Nullable: - return &OpSetNotNull{ + }) + } + if o.Nullable != nil && !*o.Nullable { + ops = append(ops, &OpSetNotNull{ Table: o.Table, Column: o.Column, Up: o.Up, Down: o.Down, - } - - case o.Nullable != nil && *o.Nullable: - return &OpDropNotNull{ + }) + } + if o.Nullable != nil && *o.Nullable { + ops = append(ops, &OpDropNotNull{ Table: o.Table, Column: o.Column, Up: o.Up, Down: o.Down, - } - - case o.Unique != nil: - return &OpSetUnique{ + }) + } + if o.Unique != nil { + ops = append(ops, &OpSetUnique{ Table: o.Table, Column: o.Column, Name: o.Unique.Name, Up: o.Up, Down: o.Down, - } - } - return nil -} - -// numChanges returns the number of kinds of change that one 'alter column' -// operation represents. -func (o *OpAlterColumn) numChanges() int { - fieldsSet := 0 - - if o.Name != nil { - fieldsSet++ - } - if o.Type != nil { - fieldsSet++ - } - if o.Check != nil { - fieldsSet++ - } - if o.References != nil { - fieldsSet++ - } - if o.Nullable != nil { - fieldsSet++ - } - if o.Unique != nil { - fieldsSet++ + }) } - return fieldsSet + return ops } -// duplicatorForOperation returns a Duplicator for the given operation. -func duplicatorForOperation(op Operation, conn *sql.DB, table *schema.Table, column *schema.Column) *Duplicator { +// duplicatorForOperations returns a Duplicator for the given operations +func duplicatorForOperations(ops []Operation, conn *sql.DB, table *schema.Table, column *schema.Column) *Duplicator { d := NewColumnDuplicator(conn, table, column) - switch op := (op).(type) { - case *OpDropNotNull: - d = d.WithoutNotNull() - case *OpChangeType: - d = d.WithType(op.Type) + for _, op := range ops { + switch op := (op).(type) { + case *OpDropNotNull: + d = d.WithoutNotNull() + case *OpChangeType: + d = d.WithType(op.Type) + } } return d } -// downSQLForOperation returns the down SQL for the given operation, applying -// an appropriate default if none is provided. -func (o *OpAlterColumn) downSQLForOperation(op Operation) string { +// downSQLForOperations returns the `down` SQL for the given operations, applying +// an appropriate default if no `down` SQL is provided. +func (o *OpAlterColumn) downSQLForOperations(ops []Operation) string { if o.Down != "" { return o.Down } - switch (op).(type) { - case *OpSetUnique, *OpSetNotNull: - return pq.QuoteIdentifier(o.Column) + for _, op := range ops { + switch (op).(type) { + case *OpSetUnique, *OpSetNotNull: + return pq.QuoteIdentifier(o.Column) + } } return "" } -// upSQLForOperation returns the up SQL for the given operation, applying -// an appropriate default if none is provided. -func (o *OpAlterColumn) upSQLForOperation(op Operation) string { +// upSQLForOperations returns the `up` SQL for the given operations, applying +// an appropriate default if no `up` SQL is provided. +func (o *OpAlterColumn) upSQLForOperations(ops []Operation) string { if o.Up != "" { return o.Up } - if _, ok := op.(*OpDropNotNull); ok { - return pq.QuoteIdentifier(o.Column) + for _, op := range ops { + if _, ok := op.(*OpDropNotNull); ok { + return pq.QuoteIdentifier(o.Column) + } } return "" } + +// isRenameOnly returns true if the operation is a rename column operation only. +func (o *OpAlterColumn) isRenameOnly() bool { + return len(o.subOperations()) == 0 && o.Name != nil +} diff --git a/pkg/migrations/op_alter_column_test.go b/pkg/migrations/op_alter_column_test.go index c165fa7e..9c1fcafd 100644 --- a/pkg/migrations/op_alter_column_test.go +++ b/pkg/migrations/op_alter_column_test.go @@ -3,11 +3,381 @@ package migrations_test import ( + "database/sql" "testing" + "github.com/stretchr/testify/assert" "github.com/xataio/pgroll/pkg/migrations" + "github.com/xataio/pgroll/pkg/testutils" ) +func TestAlterColumnMultipleSubOperations(t *testing.T) { + t.Parallel() + + ExecuteTests(t, TestCases{ + { + name: "can alter a column: set not null, change type, rename and add check constraint", + migrations: []migrations.Migration{ + { + Name: "01_create_table", + Operations: migrations.Operations{ + &migrations.OpCreateTable{ + Name: "events", + Columns: []migrations.Column{ + { + Name: "id", + Type: "serial", + Pk: ptr(true), + }, + { + Name: "name", + Type: "varchar(255)", + Nullable: ptr(true), + }, + }, + }, + }, + }, + { + Name: "02_alter_column", + Operations: migrations.Operations{ + &migrations.OpAlterColumn{ + Table: "events", + Column: "name", + Up: "(SELECT CASE WHEN name IS NULL OR LENGTH(name) <= 3 THEN 'placeholder' ELSE name END)", + Down: "name", + Name: ptr("event_name"), + Type: ptr("text"), + Nullable: ptr(false), + Check: &migrations.CheckConstraint{ + Name: "event_name_length", + Constraint: "length(name) > 3", + }, + }, + }, + }, + }, + afterStart: func(t *testing.T, db *sql.DB, schema string) { + // Inserting a NULL into the new `event_name` column should fail + MustNotInsert(t, db, schema, "02_alter_column", "events", map[string]string{ + "id": "1", + }, testutils.CheckViolationErrorCode) + + // Inserting a non-NULL value into the new `event_name` column should succeed + MustInsert(t, db, schema, "02_alter_column", "events", map[string]string{ + "id": "2", + "event_name": "apples", + }) + + // The value inserted into the new `event_name` column has been backfilled into the + // old `name` column. + rows := MustSelect(t, db, schema, "01_create_table", "events") + assert.Equal(t, []map[string]any{ + {"id": 2, "name": "apples"}, + }, rows) + + // Inserting a NULL value into the old `name` column should succeed + MustInsert(t, db, schema, "01_create_table", "events", map[string]string{ + "id": "3", + }) + + // The NULL value inserted into the old `name` column has been written into + // the new `event_name` column using the `up` SQL. + rows = MustSelect(t, db, schema, "02_alter_column", "events") + assert.Equal(t, []map[string]any{ + {"id": 2, "event_name": "apples"}, + {"id": 3, "event_name": "placeholder"}, + }, rows) + + // Inserting a non-NULL value into the old `name` column should succeed + MustInsert(t, db, schema, "01_create_table", "events", map[string]string{ + "id": "4", + "name": "bananas", + }) + + // The non-NULL value inserted into the old `name` column has been copied + // unchanged into the new `event_name` column. + rows = MustSelect(t, db, schema, "02_alter_column", "events") + assert.Equal(t, []map[string]any{ + {"id": 2, "event_name": "apples"}, + {"id": 3, "event_name": "placeholder"}, + {"id": 4, "event_name": "bananas"}, + }, rows) + + // Inserting a row into the new `event_name` column that violates the + // check constraint should fail + MustNotInsert(t, db, schema, "02_alter_column", "events", map[string]string{ + "id": "5", + "event_name": "x", + }, testutils.CheckViolationErrorCode) + + // Inserting a row into the old `name` column that violates the + // check constraint should succeed. + MustInsert(t, db, schema, "01_create_table", "events", map[string]string{ + "id": "5", + "name": "x", + }) + + // The value that didn't meet the check constraint has been rewritten + // into the new `event_name` column using the `up` SQL. + rows = MustSelect(t, db, schema, "02_alter_column", "events") + assert.Equal(t, []map[string]any{ + {"id": 2, "event_name": "apples"}, + {"id": 3, "event_name": "placeholder"}, + {"id": 4, "event_name": "bananas"}, + {"id": 5, "event_name": "placeholder"}, + }, rows) + + // The type of the new column in the underlying table should be `text` + ColumnMustHaveType(t, db, schema, "events", migrations.TemporaryName("name"), "text") + }, + afterRollback: func(t *testing.T, db *sql.DB, schema string) { + // The new (temporary) `name` column should not exist on the underlying table. + ColumnMustNotExist(t, db, schema, "events", migrations.TemporaryName("name")) + }, + afterComplete: func(t *testing.T, db *sql.DB, schema string) { + // Inserting a NULL into the new `event_name` column should fail + MustNotInsert(t, db, schema, "02_alter_column", "events", map[string]string{ + "id": "6", + }, testutils.NotNullViolationErrorCode) + + // Inserting a row into the new `event_name` column that violates the + // check constraint should fail + MustNotInsert(t, db, schema, "02_alter_column", "events", map[string]string{ + "id": "6", + "event_name": "x", + }, testutils.CheckViolationErrorCode) + + // The type of the new column in the underlying table should be `text` + ColumnMustHaveType(t, db, schema, "events", "event_name", "text") + + // The column in the underlying table should be `event_name` + ColumnMustExist(t, db, schema, "events", "event_name") + + // The table contains the expected rows + rows := MustSelect(t, db, schema, "02_alter_column", "events") + assert.Equal(t, []map[string]any{ + {"id": 2, "event_name": "apples"}, + {"id": 3, "event_name": "placeholder"}, + {"id": 4, "event_name": "bananas"}, + {"id": 5, "event_name": "placeholder"}, + }, rows) + }, + }, + { + name: "can alter a column: rename and add a unique constraint", + migrations: []migrations.Migration{ + { + Name: "01_create_table", + Operations: migrations.Operations{ + &migrations.OpCreateTable{ + Name: "events", + Columns: []migrations.Column{ + { + Name: "id", + Type: "serial", + Pk: ptr(true), + }, + { + Name: "name", + Type: "varchar(255)", + Nullable: ptr(true), + }, + }, + }, + }, + }, + { + Name: "02_alter_column", + Operations: migrations.Operations{ + &migrations.OpAlterColumn{ + Table: "events", + Column: "name", + Up: "name || '-' || random()*999::int", + Down: "name", + Name: ptr("event_name"), + Unique: &migrations.UniqueConstraint{ + Name: "events_event_name_unique", + }, + }, + }, + }, + }, + afterStart: func(t *testing.T, db *sql.DB, schema string) { + // Can insert a row into the new `event_name` column + MustInsert(t, db, schema, "02_alter_column", "events", map[string]string{ + "id": "1", + "event_name": "apples", + }) + + // Inserting a row with the same value into the new `event_name` column fails + MustNotInsert(t, db, schema, "02_alter_column", "events", map[string]string{ + "id": "2", + "event_name": "apples", + }, testutils.UniqueViolationErrorCode) + + // Inserting a row with a duplicate value into the old `name` column succeeds + MustInsert(t, db, schema, "01_create_table", "events", map[string]string{ + "id": "2", + "name": "apples", + }) + }, + afterRollback: func(t *testing.T, db *sql.DB, schema string) { + }, + afterComplete: func(t *testing.T, db *sql.DB, schema string) { + // Inserting a row with the same value into the new `event_name` column fails + MustNotInsert(t, db, schema, "02_alter_column", "events", map[string]string{ + "id": "2", + "event_name": "apples", + }, testutils.UniqueViolationErrorCode) + + // The column in the underlying table has been renamed to `event_name` + ColumnMustExist(t, db, schema, "events", "event_name") + }, + }, + { + name: "can alter a column: add an FK constraint and set not null", + migrations: []migrations.Migration{ + { + Name: "01_create_table", + Operations: migrations.Operations{ + &migrations.OpCreateTable{ + Name: "events", + Columns: []migrations.Column{ + { + Name: "id", + Type: "serial", + Pk: ptr(true), + }, + { + Name: "name", + Type: "varchar(255)", + Nullable: ptr(true), + }, + }, + }, + &migrations.OpCreateTable{ + Name: "people", + Columns: []migrations.Column{ + { + Name: "id", + Type: "serial", + Pk: ptr(true), + }, + { + Name: "name", + Type: "varchar(255)", + Nullable: ptr(true), + }, + { + Name: "manages", + Type: "integer", + Nullable: ptr(true), + }, + }, + }, + }, + }, + { + Name: "02_alter_column", + Operations: migrations.Operations{ + &migrations.OpAlterColumn{ + Table: "people", + Column: "manages", + Up: "(SELECT CASE WHEN manages IS NULL THEN 1 ELSE manages END)", + Down: "manages", + Nullable: ptr(false), + References: &migrations.ForeignKeyReference{ + Table: "events", + Column: "id", + Name: "person_manages_event_fk", + }, + }, + }, + }, + }, + afterStart: func(t *testing.T, db *sql.DB, schema string) { + // Can insert an initial event into the `events` table + MustInsert(t, db, schema, "02_alter_column", "events", map[string]string{ + "id": "1", + "name": "event1", + }) + + // Inserting a row into the `people` table with a NULL `manages` value fails + MustNotInsert(t, db, schema, "02_alter_column", "people", map[string]string{ + "id": "1", + "name": "alice", + }, testutils.CheckViolationErrorCode) + + // Inserting a row into the `people` table with a `manages` field that + // violates the FK constraint fails + MustNotInsert(t, db, schema, "02_alter_column", "people", map[string]string{ + "id": "1", + "name": "alice", + "manages": "2", + }, testutils.FKViolationErrorCode) + + // Inserting a row into the `people` table with a valid `manages` field + // succeeds + MustInsert(t, db, schema, "02_alter_column", "people", map[string]string{ + "id": "1", + "name": "alice", + "manages": "1", + }) + + // Inserting a row into the old version of the `people` table with a + // NULL `manages` value succeeds + MustInsert(t, db, schema, "01_create_table", "people", map[string]string{ + "id": "2", + "name": "bob", + }) + + // The version of the `people` table in the new schema has the expected rows. + // In particular, the `manages` field has been backfilled using the + // `up` SQL for `bob`. + rows := MustSelect(t, db, schema, "02_alter_column", "people") + assert.Equal(t, []map[string]any{ + {"id": 1, "name": "alice", "manages": 1}, + {"id": 2, "name": "bob", "manages": 1}, + }, rows) + }, + afterRollback: func(t *testing.T, db *sql.DB, schema string) { + }, + afterComplete: func(t *testing.T, db *sql.DB, schema string) { + // Inserting a row into the `people` table with a NULL `manages` value fails + MustNotInsert(t, db, schema, "02_alter_column", "people", map[string]string{ + "id": "1", + "name": "carl", + }, testutils.NotNullViolationErrorCode) + + // Inserting a row into the `people` table with a `manages` field that + // violates the FK constraint fails + MustNotInsert(t, db, schema, "02_alter_column", "people", map[string]string{ + "id": "3", + "name": "carl", + "manages": "2", + }, testutils.FKViolationErrorCode) + + // Inserting a row into the `people` table with a valid `manages` field + // succeeds + MustInsert(t, db, schema, "02_alter_column", "people", map[string]string{ + "id": "3", + "name": "carl", + "manages": "1", + }) + + // The `people` table has the expected rows. + rows := MustSelect(t, db, schema, "02_alter_column", "people") + assert.Equal(t, []map[string]any{ + {"id": 1, "name": "alice", "manages": 1}, + {"id": 2, "name": "bob", "manages": 1}, + {"id": 3, "name": "carl", "manages": 1}, + }, rows) + }, + }, + }) +} + func TestAlterColumnValidation(t *testing.T) { t.Parallel() @@ -134,25 +504,7 @@ func TestAlterColumnValidation(t *testing.T) { }, }, }, - wantStartErr: migrations.MultipleAlterColumnChangesError{Changes: 0}, - }, - { - name: "only one change at at time", - migrations: []migrations.Migration{ - createTablesMigration, - { - Name: "01_alter_column", - Operations: migrations.Operations{ - &migrations.OpAlterColumn{ - Table: "posts", - Column: "title", - Name: ptr("renamed_title"), - Type: ptr("varchar(255)"), - }, - }, - }, - }, - wantStartErr: migrations.MultipleAlterColumnChangesError{Changes: 2}, + wantStartErr: migrations.AlterColumnNoChangesError{Table: "posts", Column: "title"}, }, { name: "table must have a primary key on exactly one column", diff --git a/pkg/migrations/op_rename_column.go b/pkg/migrations/op_rename_column.go deleted file mode 100644 index 0b102595..00000000 --- a/pkg/migrations/op_rename_column.go +++ /dev/null @@ -1,50 +0,0 @@ -// SPDX-License-Identifier: Apache-2.0 - -package migrations - -import ( - "context" - "database/sql" - "fmt" - - "github.com/lib/pq" - "github.com/xataio/pgroll/pkg/schema" -) - -type OpRenameColumn struct { - Table string `json:"table"` - From string `json:"from"` - To string `json:"to"` -} - -var _ Operation = (*OpRenameColumn)(nil) - -func (o *OpRenameColumn) Start(ctx context.Context, conn *sql.DB, stateSchema string, tr SQLTransformer, s *schema.Schema, cbs ...CallbackFn) (*schema.Table, error) { - table := s.GetTable(o.Table) - table.RenameColumn(o.From, o.To) - return nil, nil -} - -func (o *OpRenameColumn) Complete(ctx context.Context, conn *sql.DB, tr SQLTransformer, s *schema.Schema) error { - // rename the column in the underlying table - _, err := conn.ExecContext(ctx, fmt.Sprintf("ALTER TABLE %s RENAME COLUMN %s TO %s", - pq.QuoteIdentifier(o.Table), - pq.QuoteIdentifier(o.From), - pq.QuoteIdentifier(o.To))) - return err -} - -func (o *OpRenameColumn) Rollback(ctx context.Context, conn *sql.DB, tr SQLTransformer) error { - // no-op - return nil -} - -func (o *OpRenameColumn) Validate(ctx context.Context, s *schema.Schema) error { - table := s.GetTable(o.Table) - - if table.GetColumn(o.To) != nil { - return ColumnAlreadyExistsError{Table: o.Table, Name: o.To} - } - - return nil -} diff --git a/schema.json b/schema.json index 6c0f5550..f4623722 100644 --- a/schema.json +++ b/schema.json @@ -162,29 +162,26 @@ "required": ["table", "column"], "oneOf": [ { - "required": ["up", "down"], - "oneOf": [ + "anyOf": [ { "required": ["check"] }, { "required": ["type"] }, { "required": ["nullable"] }, { "required": ["unique"] }, { "required": ["references"] } ], - "not": { - "required": ["name"] - } + "required": ["up", "down"] }, { "required": ["name"], "not": { "anyOf": [ - { "required": ["up"] }, - { "required": ["down"] }, { "required": ["check"] }, { "required": ["type"] }, { "required": ["nullable"] }, { "required": ["unique"] }, - { "required": ["references"] } + { "required": ["references"] }, + { "required": ["up"] }, + { "required": ["down"] } ] } }