From 4c8829d0bc06929c5d93f0600ab60c6132d61391 Mon Sep 17 00:00:00 2001 From: dyma solovei Date: Mon, 21 Oct 2024 15:07:15 +0200 Subject: [PATCH] refactor: remove superficial sqlschema.Operation interface Each dialect has to type-switch the operation before building a query for it. Since the migrator knows the concrete type of each operation, they are free to provide FQN in any form. Using schema.FQN field from the start simplifies the data structure later. Empty inteface is better that a superficial one. --- dialect/pgdialect/alter_table.go | 50 +++++-------- internal/dbtest/migrate_test.go | 60 +++++++++++++++ migrate/diff.go | 28 +++---- migrate/operations.go | 121 +++++++------------------------ migrate/sqlschema/migrator.go | 12 +-- 5 files changed, 119 insertions(+), 152 deletions(-) diff --git a/dialect/pgdialect/alter_table.go b/dialect/pgdialect/alter_table.go index 63e4fa38a..89bae8042 100644 --- a/dialect/pgdialect/alter_table.go +++ b/dialect/pgdialect/alter_table.go @@ -24,7 +24,7 @@ type migrator struct { var _ sqlschema.Migrator = (*migrator)(nil) -func (m *migrator) Apply(ctx context.Context, changes ...sqlschema.Operation) error { +func (m *migrator) Apply(ctx context.Context, changes ...interface{}) error { if len(changes) == 0 { return nil } @@ -41,17 +41,17 @@ func (m *migrator) Apply(ctx context.Context, changes ...sqlschema.Operation) er switch change := change.(type) { case *migrate.CreateTable: - log.Printf("create table %q", change.Name) + log.Printf("create table %q", change.FQN.Table) err = m.CreateTable(ctx, change.Model) if err != nil { - return fmt.Errorf("apply changes: create table %s: %w", change.FQN(), err) + return fmt.Errorf("apply changes: create table %s: %w", change.FQN, err) } continue case *migrate.DropTable: - log.Printf("drop table %q", change.Name) - err = m.DropTable(ctx, change.Schema, change.Name) + log.Printf("drop table %q", change.FQN.Table) + err = m.DropTable(ctx, change.FQN) if err != nil { - return fmt.Errorf("apply changes: drop table %s: %w", change.FQN(), err) + return fmt.Errorf("apply changes: drop table %s: %w", change.FQN, err) } continue case *migrate.RenameTable: @@ -88,35 +88,29 @@ func (m *migrator) Apply(ctx context.Context, changes ...sqlschema.Operation) er func (m *migrator) renameTable(fmter schema.Formatter, b []byte, rename *migrate.RenameTable) (_ []byte, err error) { b = append(b, "ALTER TABLE "...) - fqn := rename.FQN() - if b, err = fqn.AppendQuery(fmter, b); err != nil { - return b, err - } + b, _ = rename.FQN.AppendQuery(fmter, b) + b = append(b, " RENAME TO "...) - b = fmter.AppendIdent(b, rename.NewName) + b = fmter.AppendName(b, rename.NewName) return b, nil } func (m *migrator) renameColumn(fmter schema.Formatter, b []byte, rename *migrate.RenameColumn) (_ []byte, err error) { b = append(b, "ALTER TABLE "...) - fqn := rename.FQN() - if b, err = fqn.AppendQuery(fmter, b); err != nil { - return b, err - } + b, _ = rename.FQN.AppendQuery(fmter, b) b = append(b, " RENAME COLUMN "...) - b = fmter.AppendIdent(b, rename.OldName) + b = fmter.AppendName(b, rename.OldName) b = append(b, " TO "...) - b = fmter.AppendIdent(b, rename.NewName) + b = fmter.AppendName(b, rename.NewName) return b, nil } func (m *migrator) addColumn(fmter schema.Formatter, b []byte, add *migrate.AddColumn) (_ []byte, err error) { b = append(b, "ALTER TABLE "...) - fqn := add.FQN() - b, _ = fqn.AppendQuery(fmter, b) + b, _ = add.FQN.AppendQuery(fmter, b) b = append(b, " ADD COLUMN "...) b = fmter.AppendName(b, add.Column) @@ -129,8 +123,7 @@ func (m *migrator) addColumn(fmter schema.Formatter, b []byte, add *migrate.AddC func (m *migrator) dropColumn(fmter schema.Formatter, b []byte, drop *migrate.DropColumn) (_ []byte, err error) { b = append(b, "ALTER TABLE "...) - fqn := drop.FQN() - b, _ = fqn.AppendQuery(fmter, b) + b, _ = drop.FQN.AppendQuery(fmter, b) b = append(b, " DROP COLUMN "...) b = fmter.AppendName(b, drop.Column) @@ -146,10 +139,10 @@ func (m *migrator) renameConstraint(fmter schema.Formatter, b []byte, rename *mi } b = append(b, " RENAME CONSTRAINT "...) - b = fmter.AppendIdent(b, rename.OldName) + b = fmter.AppendName(b, rename.OldName) b = append(b, " TO "...) - b = fmter.AppendIdent(b, rename.NewName) + b = fmter.AppendName(b, rename.NewName) return b, nil } @@ -162,7 +155,7 @@ func (m *migrator) dropContraint(fmter schema.Formatter, b []byte, drop *migrate } b = append(b, " DROP CONSTRAINT "...) - b = fmter.AppendIdent(b, drop.ConstraintName) + b = fmter.AppendName(b, drop.ConstraintName) return b, nil } @@ -175,7 +168,7 @@ func (m *migrator) addForeignKey(fmter schema.Formatter, b []byte, add *migrate. } b = append(b, " ADD CONSTRAINT "...) - b = fmter.AppendIdent(b, add.ConstraintName) + b = fmter.AppendName(b, add.ConstraintName) b = append(b, " FOREIGN KEY ("...) if b, err = add.FK.From.Column.Safe().AppendQuery(fmter, b); err != nil { @@ -200,10 +193,7 @@ func (m *migrator) addForeignKey(fmter schema.Formatter, b []byte, add *migrate. func (m *migrator) changeColumnType(fmter schema.Formatter, b []byte, colDef *migrate.ChangeColumnType) (_ []byte, err error) { b = append(b, "ALTER TABLE "...) - fqn := colDef.FQN() - if b, err = fqn.AppendQuery(fmter, b); err != nil { - return b, err - } + b, _ = colDef.FQN.AppendQuery(fmter, b) // alterColumn never re-assigns err, so there is no need to check for err != nil after calling it var i int @@ -212,7 +202,7 @@ func (m *migrator) changeColumnType(fmter schema.Formatter, b []byte, colDef *mi b = append(b, ","...) } b = append(b, " ALTER COLUMN "...) - b = fmter.AppendIdent(b, colDef.Column) + b = fmter.AppendName(b, colDef.Column) i++ } diff --git a/internal/dbtest/migrate_test.go b/internal/dbtest/migrate_test.go index bb424b9f7..0259a498c 100644 --- a/internal/dbtest/migrate_test.go +++ b/internal/dbtest/migrate_test.go @@ -209,6 +209,7 @@ func TestAutoMigrator_Run(t *testing.T) { {testChangeColumnType_AutoCast}, {testIdentity}, {testAddDropColumn}, + // {testUnique}, } testEachDB(t, func(t *testing.T, dbName string, db *bun.DB) { @@ -753,6 +754,65 @@ func testAddDropColumn(t *testing.T, db *bun.DB) { cmpTables(t, db.Dialect().(sqlschema.InspectorDialect), wantTables, state.Tables) } +func testUnique(t *testing.T, db *bun.DB) { + type TableBefore struct { + bun.BaseModel `bun:"table:table"` + FirstName string `bun:"first_name,unique:full_name"` + LastName string `bun:"last_name,unique:full_name"` + Birthday string `bun:"birthday,unique"` + } + + type TableAfter struct { + bun.BaseModel `bun:"table:table"` + FirstName string `bun:"first_name,unique:full_name"` + MiddleName string `bun:"middle_name,unique:full_name"` // extend "full_name" unique group + LastName string `bun:"last_name,unique:full_name"` + Birthday string `bun:"birthday"` // doesn't have to be unique any more + Email string `bun:"email,unique"` // new column, unique + } + + wantTables := []sqlschema.Table{ + { + Schema: db.Dialect().DefaultSchema(), + Name: "table", + Columns: map[string]sqlschema.Column{ + "first_name": { + SQLType: sqltype.VarChar, + IsNullable: true, + }, + "middle_name": { + SQLType: sqltype.VarChar, + IsNullable: true, + }, + "last_name": { + SQLType: sqltype.VarChar, + IsNullable: true, + }, + "birthday": { + SQLType: sqltype.VarChar, + IsNullable: true, + }, + "email": { + SQLType: sqltype.VarChar, + IsNullable: true, + }, + }, + }, + } + + ctx := context.Background() + inspect := inspectDbOrSkip(t, db) + mustResetModel(t, ctx, db, (*TableBefore)(nil)) + m := newAutoMigrator(t, db, migrate.WithModel((*TableAfter)(nil))) + + // Act + err := m.Run(ctx) + require.NoError(t, err) + + // Assert + state := inspect(ctx) + cmpTables(t, db.Dialect().(sqlschema.InspectorDialect), wantTables, state.Tables) +} // // TODO: rewrite these tests into AutoMigrator tests, Diff should be moved to migrate/internal package // func TestDiff(t *testing.T) { diff --git a/migrate/diff.go b/migrate/diff.go index 1afa4dd96..30bd60b77 100644 --- a/migrate/diff.go +++ b/migrate/diff.go @@ -8,6 +8,7 @@ import ( "github.com/uptrace/bun" "github.com/uptrace/bun/migrate/sqlschema" + "github.com/uptrace/bun/schema" ) // Diff calculates the diff between the current database schema and the target state. @@ -29,8 +30,7 @@ AddedLoop: for _, removed := range removedTables.Values() { if d.canRename(removed, added) { d.changes.Add(&RenameTable{ - Schema: removed.Schema, - OldName: removed.Name, + FQN: schema.FQN{removed.Schema, removed.Name}, NewName: added.Name, }) @@ -52,9 +52,8 @@ AddedLoop: } // If a new table did not appear because of the rename operation, then it must've been created. d.changes.Add(&CreateTable{ - Schema: added.Schema, - Name: added.Name, - Model: added.Model, + FQN: schema.FQN{added.Schema, added.Name}, + Model: added.Model, }) created.Add(added) } @@ -63,8 +62,7 @@ AddedLoop: dropped := currentTables.Sub(targetTables) for _, t := range dropped.Values() { d.changes.Add(&DropTable{ - Schema: t.Schema, - Name: t.Name, + FQN: schema.FQN{t.Schema, t.Name}, }) } @@ -144,9 +142,9 @@ func (c *changeset) Add(op ...Operation) { // Func creates a MigrationFunc that applies all operations all the changeset. func (c *changeset) Func(m sqlschema.Migrator) MigrationFunc { return func(ctx context.Context, db *bun.DB) error { - var operations []sqlschema.Operation + var operations []interface{} for _, op := range c.operations { - operations = append(operations, op.(sqlschema.Operation)) + operations = append(operations, op.(interface{})) } return m.Apply(ctx, operations...) } @@ -349,8 +347,7 @@ ChangedRenamed: if cCol, ok := current.Columns[tName]; ok { if checkType && !d.equalColumns(cCol, tCol) { d.changes.Add(&ChangeColumnType{ - Schema: target.Schema, - Table: target.Name, + FQN: schema.FQN{target.Schema, target.Name}, Column: tName, From: cCol, To: d.makeTargetColDef(cCol, tCol), @@ -367,8 +364,7 @@ ChangedRenamed: continue } d.changes.Add(&RenameColumn{ - Schema: target.Schema, - Table: target.Name, + FQN: schema.FQN{target.Schema, target.Name}, OldName: cName, NewName: tName, }) @@ -379,8 +375,7 @@ ChangedRenamed: } d.changes.Add(&AddColumn{ - Schema: target.Schema, - Table: target.Name, + FQN: schema.FQN{target.Schema, target.Name}, Column: tName, ColDef: tCol, }) @@ -390,8 +385,7 @@ ChangedRenamed: for cName, cCol := range current.Columns { if _, keep := target.Columns[cName]; !keep { d.changes.Add(&DropColumn{ - Schema: target.Schema, - Table: target.Name, + FQN: schema.FQN{target.Schema, target.Name}, Column: cName, ColDef: cCol, }) diff --git a/migrate/operations.go b/migrate/operations.go index 016910edc..4b3958b5d 100644 --- a/migrate/operations.go +++ b/migrate/operations.go @@ -13,46 +13,27 @@ type Operation interface { // CreateTable type CreateTable struct { - Schema string - Name string - Model interface{} + FQN schema.FQN + Model interface{} } var _ Operation = (*CreateTable)(nil) -func (op *CreateTable) FQN() schema.FQN { - return schema.FQN{ - Schema: op.Schema, - Table: op.Name, - } -} - func (op *CreateTable) GetReverse() Operation { - return &DropTable{ - Schema: op.Schema, - Name: op.Name, - } + return &DropTable{FQN: op.FQN} } type DropTable struct { - Schema string - Name string + FQN schema.FQN } var _ Operation = (*DropTable)(nil) -func (op *DropTable) FQN() schema.FQN { - return schema.FQN{ - Schema: op.Schema, - Table: op.Name, - } -} - func (op *DropTable) DependsOn(another Operation) bool { d, ok := another.(*DropConstraint) // - return ok && ((d.FK.From.Schema == op.Schema && d.FK.From.Table == op.Name) || - (d.FK.To.Schema == op.Schema && d.FK.To.Table == op.Name)) + return ok && ((d.FK.From.Schema == op.FQN.Schema && d.FK.From.Table == op.FQN.Table) || + (d.FK.To.Schema == op.FQN.Schema && d.FK.To.Table == op.FQN.Table)) } // GetReverse for a DropTable returns a no-op migration. Logically, CreateTable is the reverse, @@ -65,51 +46,31 @@ func (op *DropTable) GetReverse() Operation { } type RenameTable struct { - Schema string - OldName string + FQN schema.FQN NewName string } var _ Operation = (*RenameTable)(nil) -var _ sqlschema.Operation = (*RenameTable)(nil) - -func (op *RenameTable) FQN() schema.FQN { - return schema.FQN{ - Schema: op.Schema, - Table: op.OldName, - } -} func (op *RenameTable) GetReverse() Operation { return &RenameTable{ - Schema: op.Schema, - OldName: op.NewName, - NewName: op.OldName, + FQN: schema.FQN{op.FQN.Schema, op.NewName}, + NewName: op.FQN.Table, } } // RenameColumn. type RenameColumn struct { - Schema string - Table string + FQN schema.FQN OldName string NewName string } var _ Operation = (*RenameColumn)(nil) -var _ sqlschema.Operation = (*RenameColumn)(nil) - -func (op *RenameColumn) FQN() schema.FQN { - return schema.FQN{ - Schema: op.Schema, - Table: op.Table, - } -} func (op *RenameColumn) GetReverse() Operation { return &RenameColumn{ - Schema: op.Schema, - Table: op.Table, + FQN: op.FQN, OldName: op.NewName, NewName: op.OldName, } @@ -117,55 +78,36 @@ func (op *RenameColumn) GetReverse() Operation { func (op *RenameColumn) DependsOn(another Operation) bool { rt, ok := another.(*RenameTable) - return ok && rt.Schema == op.Schema && rt.NewName == op.Table + return ok && rt.FQN.Schema == op.FQN.Schema && rt.NewName == op.FQN.Table } type AddColumn struct { - Schema string - Table string + FQN schema.FQN Column string ColDef sqlschema.Column } var _ Operation = (*AddColumn)(nil) -var _ sqlschema.Operation = (*AddColumn)(nil) - -func (op *AddColumn) FQN() schema.FQN { - return schema.FQN{ - Schema: op.Schema, - Table: op.Table, - } -} func (op *AddColumn) GetReverse() Operation { return &DropColumn{ - Schema: op.Schema, - Table: op.Table, + FQN: op.FQN, Column: op.Column, + ColDef: op.ColDef, } } type DropColumn struct { - Schema string - Table string + FQN schema.FQN Column string ColDef sqlschema.Column } var _ Operation = (*DropColumn)(nil) -var _ sqlschema.Operation = (*DropColumn)(nil) - -func (op *DropColumn) FQN() schema.FQN { - return schema.FQN{ - Schema: op.Schema, - Table: op.Table, - } -} func (op *DropColumn) GetReverse() Operation { return &AddColumn{ - Schema: op.Schema, - Table: op.Table, + FQN: op.FQN, Column: op.Column, ColDef: op.ColDef, } @@ -192,8 +134,8 @@ func (op *DropColumn) DependsOn(another Operation) bool { } } - return (dc.FK.From.Schema == op.Schema && dc.FK.From.Table == op.Table && fCol) || - (dc.FK.To.Schema == op.Schema && dc.FK.To.Table == op.Table && tCol) + return (dc.FK.From.Schema == op.FQN.Schema && dc.FK.From.Table == op.FQN.Table && fCol) || + (dc.FK.To.Schema == op.FQN.Schema && dc.FK.To.Table == op.FQN.Table && tCol) } return false } @@ -206,7 +148,6 @@ type RenameConstraint struct { } var _ Operation = (*RenameConstraint)(nil) -var _ sqlschema.Operation = (*RenameConstraint)(nil) func (op *RenameConstraint) FQN() schema.FQN { return schema.FQN{ @@ -217,7 +158,7 @@ func (op *RenameConstraint) FQN() schema.FQN { func (op *RenameConstraint) DependsOn(another Operation) bool { rt, ok := another.(*RenameTable) - return ok && rt.Schema == op.FK.From.Schema && rt.NewName == op.FK.From.Table + return ok && rt.FQN.Schema == op.FK.From.Schema && rt.NewName == op.FK.From.Table } func (op *RenameConstraint) GetReverse() Operation { @@ -234,7 +175,6 @@ type AddForeignKey struct { } var _ Operation = (*AddForeignKey)(nil) -var _ sqlschema.Operation = (*AddForeignKey)(nil) func (op *AddForeignKey) FQN() schema.FQN { return schema.FQN{ @@ -247,10 +187,10 @@ func (op *AddForeignKey) DependsOn(another Operation) bool { switch another := another.(type) { case *RenameTable: // TODO: provide some sort of "DependsOn" method for FK - return another.Schema == op.FK.From.Schema && another.NewName == op.FK.From.Table + return another.FQN.Schema == op.FK.From.Schema && another.NewName == op.FK.From.Table case *CreateTable: - return (another.Schema == op.FK.To.Schema && another.Name == op.FK.To.Table) || // either it's the referencing one - (another.Schema == op.FK.From.Schema && another.Name == op.FK.From.Table) // or the one being referenced + return (another.FQN.Schema == op.FK.To.Schema && another.FQN.Table == op.FK.To.Table) || // either it's the referencing one + (another.FQN.Schema == op.FK.From.Schema && another.FQN.Table == op.FK.From.Table) // or the one being referenced } return false } @@ -269,7 +209,6 @@ type DropConstraint struct { } var _ Operation = (*DropConstraint)(nil) -var _ sqlschema.Operation = (*DropConstraint)(nil) func (op *DropConstraint) FQN() schema.FQN { return schema.FQN{ @@ -287,33 +226,23 @@ func (op *DropConstraint) GetReverse() Operation { // Change column type. type ChangeColumnType struct { - Schema string - Table string + FQN schema.FQN Column string From sqlschema.Column To sqlschema.Column } var _ Operation = (*ChangeColumnType)(nil) -var _ sqlschema.Operation = (*ChangeColumnType)(nil) func (op *ChangeColumnType) GetReverse() Operation { return &ChangeColumnType{ - Schema: op.Schema, - Table: op.Table, + FQN: op.FQN, Column: op.Column, From: op.To, To: op.From, } } -func (op ChangeColumnType) FQN() schema.FQN { - return schema.FQN{ - Schema: op.Schema, - Table: op.Table, - } -} - // noop is a migration that doesn't change the schema. type noop struct{} diff --git a/migrate/sqlschema/migrator.go b/migrate/sqlschema/migrator.go index 6087a8448..e4dc5a598 100644 --- a/migrate/sqlschema/migrator.go +++ b/migrate/sqlschema/migrator.go @@ -14,7 +14,7 @@ type MigratorDialect interface { } type Migrator interface { - Apply(ctx context.Context, changes ...Operation) error + Apply(ctx context.Context, changes ...interface{}) error } // migrator is a dialect-agnostic wrapper for sqlschema.MigratorDialect. @@ -49,16 +49,10 @@ func (m *BaseMigrator) CreateTable(ctx context.Context, model interface{}) error return nil } -func (m *BaseMigrator) DropTable(ctx context.Context, schema, name string) error { - _, err := m.db.NewDropTable().TableExpr("?.?", bun.Ident(schema), bun.Ident(name)).Exec(ctx) +func (m *BaseMigrator) DropTable(ctx context.Context, fqn schema.FQN) error { + _, err := m.db.NewDropTable().TableExpr(fqn.String()).Exec(ctx) if err != nil { return err } return nil } - -// Operation is a helper interface each migrate.Operation must implement -// so an not to handle this in every dialect separately. -type Operation interface { - FQN() schema.FQN -}