From 812307f45ad4d2e9803f4ed642ee43f54ad93b2e Mon Sep 17 00:00:00 2001 From: dyma solovei Date: Sat, 9 Dec 2023 15:45:59 +0100 Subject: [PATCH] wip: resolve dependencies + refactor --- dialect/pgdialect/alter_table.go | 3 -- internal/dbtest/db_test.go | 14 +++--- internal/dbtest/migrate_test.go | 4 +- migrate/alt/operations.go | 80 ++++++++++++++++++++++++++++++-- migrate/auto.go | 11 ++--- migrate/diff.go | 7 ++- migrate/sqlschema/inspector.go | 7 +++ schema/sqlfmt.go | 5 ++ 8 files changed, 105 insertions(+), 26 deletions(-) diff --git a/dialect/pgdialect/alter_table.go b/dialect/pgdialect/alter_table.go index 29e467578..15034d042 100644 --- a/dialect/pgdialect/alter_table.go +++ b/dialect/pgdialect/alter_table.go @@ -4,7 +4,6 @@ import ( "context" "errors" "fmt" - "log" "github.com/uptrace/bun" "github.com/uptrace/bun/migrate/alt" @@ -25,7 +24,6 @@ type Migrator struct { var _ sqlschema.Migrator = (*Migrator)(nil) func (m *Migrator) execRaw(ctx context.Context, q *bun.RawQuery) error { - log.Print("execRaw: ", q.String()) if _, err := q.Exec(ctx); err != nil { return err } @@ -190,7 +188,6 @@ func (q *AlterTableQuery) Chain(op sqlschema.Operation) error { default: return fmt.Errorf("unsupported operation %T", op) } - return nil } func (q *AlterTableQuery) isEmpty() bool { diff --git a/internal/dbtest/db_test.go b/internal/dbtest/db_test.go index 7bbf5708a..a9a303bf6 100644 --- a/internal/dbtest/db_test.go +++ b/internal/dbtest/db_test.go @@ -45,13 +45,13 @@ const ( ) var allDBs = map[string]func(tb testing.TB) *bun.DB{ - pgName: pg, - pgxName: pgx, - mysql5Name: mysql5, - mysql8Name: mysql8, - mariadbName: mariadb, - sqliteName: sqlite, - mssql2019Name: mssql2019, + pgName: pg, + // pgxName: pgx, + // mysql5Name: mysql5, + // mysql8Name: mysql8, + // mariadbName: mariadb, + // sqliteName: sqlite, + // mssql2019Name: mssql2019, } var allDialects = []func() schema.Dialect{ diff --git a/internal/dbtest/migrate_test.go b/internal/dbtest/migrate_test.go index b82b04e9d..037ef32dc 100644 --- a/internal/dbtest/migrate_test.go +++ b/internal/dbtest/migrate_test.go @@ -477,7 +477,7 @@ func testRenamedColumns(t *testing.T, db *bun.DB) { // Database state type Original struct { bun.BaseModel `bun:"original"` - ID int64 `bun:",pk"` + ID int64 `bun:",pk"` } type Model1 struct { @@ -508,8 +508,8 @@ func testRenamedColumns(t *testing.T, db *bun.DB) { ) mustDropTableOnCleanup(t, ctx, db, (*Renamed)(nil)) m := newAutoMigrator(t, db, migrate.WithModel( - (*Renamed)(nil), (*Model2)(nil), + (*Renamed)(nil), )) // Act diff --git a/migrate/alt/operations.go b/migrate/alt/operations.go index 9d886542a..f7f1a8873 100644 --- a/migrate/alt/operations.go +++ b/migrate/alt/operations.go @@ -21,6 +21,13 @@ type CreateTable struct { 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, @@ -35,6 +42,19 @@ type DropTable struct { 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)) +} + // GetReverse for a DropTable returns a no-op migration. Logically, CreateTable is the reverse, // but DropTable does not have the table's definition to create one. // @@ -108,7 +128,7 @@ func (op *RenameColumn) DependsOn(another Operation) bool { return ok && rt.Schema == op.Schema && rt.NewName == op.Table } -// RenameConstraint +// RenameConstraint. type RenameConstraint struct { FK sqlschema.FK OldName string @@ -125,6 +145,11 @@ 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 +} + func (op *RenameConstraint) AppendQuery(fmter schema.Formatter, b []byte) ([]byte, error) { return fmter.AppendQuery(b, "RENAME CONSTRAINT ? TO ?", bun.Ident(op.OldName), bun.Ident(op.NewName)), nil } @@ -143,22 +168,67 @@ type AddForeignKey struct { } var _ Operation = (*AddForeignKey)(nil) +var _ sqlschema.Operation = (*AddForeignKey)(nil) + +func (op *AddForeignKey) FQN() schema.FQN { + return schema.FQN{ + Schema: op.FK.From.Schema, + Table: op.FK.From.Table, + } +} + +func (op *AddForeignKey) DependsOn(another Operation) bool { + switch another := another.(type) { + case *RenameTable: + return another.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 false +} + +func (op *AddForeignKey) AppendQuery(fmter schema.Formatter, b []byte) ([]byte, error) { + fqn := schema.FQN{ + Schema: op.FK.To.Schema, + Table: op.FK.To.Table, + } + b = fmter.AppendQuery(b, + "ADD CONSTRAINT ? FOREIGN KEY (?) REFERENCES ", + bun.Ident(op.ConstraintName), bun.Safe(op.FK.From.Column), + ) + b, _ = fqn.AppendQuery(fmter, b) + return fmter.AppendQuery(b, " (?)", bun.Ident(op.FK.To.Column)), nil +} func (op *AddForeignKey) GetReverse() Operation { - return &DropForeignKey{ + return &DropConstraint{ FK: op.FK, ConstraintName: op.ConstraintName, } } -type DropForeignKey struct { +// DropConstraint. +type DropConstraint struct { FK sqlschema.FK ConstraintName string } -var _ Operation = (*DropForeignKey)(nil) +var _ Operation = (*DropConstraint)(nil) +var _ sqlschema.Operation = (*DropConstraint)(nil) + +func (op *DropConstraint) FQN() schema.FQN { + return schema.FQN{ + Schema: op.FK.From.Schema, + Table: op.FK.From.Table, + } +} + +func (op *DropConstraint) AppendQuery(fmter schema.Formatter, b []byte) ([]byte, error) { + return fmter.AppendQuery(b, "DROP CONSTRAINT ?", bun.Ident(op.ConstraintName)), nil +} -func (op *DropForeignKey) GetReverse() Operation { +func (op *DropConstraint) GetReverse() Operation { return &AddForeignKey{ FK: op.FK, ConstraintName: op.ConstraintName, diff --git a/migrate/auto.go b/migrate/auto.go index c7158327f..edb8f9f77 100644 --- a/migrate/auto.go +++ b/migrate/auto.go @@ -147,7 +147,7 @@ func (am *AutoMigrator) plan(ctx context.Context) (*changeset, error) { detector := newDetector(got, want, am.diffOpts...) changes := detector.Diff() if err := changes.ResolveDependencies(); err != nil { - return nil, err + return nil, fmt.Errorf("plan migrations: %w", err) } return changes, nil } @@ -185,14 +185,11 @@ func (am *AutoMigrator) Migrate(ctx context.Context, opts ...MigrationOption) er func (am *AutoMigrator) Run(ctx context.Context) error { changes, err := am.plan(ctx) if err != nil { - return fmt.Errorf("run auto migrate: %w", err) + return fmt.Errorf("auto migrate: %w", err) } up := changes.Up(am.dbMigrator) if err := up(ctx, am.db); err != nil { - return fmt.Errorf("run auto migrate: %w", err) + return fmt.Errorf("auto migrate: %w", err) } return nil -} - -// INTERNAL ------------------------------------------------------------------- -// TODO: move to migrate/internal +} \ No newline at end of file diff --git a/migrate/diff.go b/migrate/diff.go index d2c8b247c..4c875975c 100644 --- a/migrate/diff.go +++ b/migrate/diff.go @@ -85,6 +85,7 @@ func (c *changeset) ResolveDependencies() error { case visited: return nil case current: + // TODO: add details (circle) to the error message return errors.New("detected circular dependency") } @@ -96,7 +97,9 @@ func (c *changeset) ResolveDependencies() error { }); another == op || !hasDeps || !dop.DependsOn(op) { continue } - visit(another) + if err := visit(another); err != nil { + return err + } } status[op] = visited @@ -281,7 +284,7 @@ AddedLoop: // Add DropFK migrations for removed FKs. for fk, fkName := range currentFKs { if _, ok := d.target.FKs[fk]; !ok { - d.changes.Add(&alt.DropForeignKey{ + d.changes.Add(&alt.DropConstraint{ FK: fk, ConstraintName: fkName, }) diff --git a/migrate/sqlschema/inspector.go b/migrate/sqlschema/inspector.go index 2060fef0c..53fc95a0f 100644 --- a/migrate/sqlschema/inspector.go +++ b/migrate/sqlschema/inspector.go @@ -71,6 +71,13 @@ func (si *SchemaInspector) Inspect(ctx context.Context) (State, error) { }) for _, rel := range t.Relations { + // These relations are nominal and do not need a foreign key to be declared in the current table. + // They will be either expressed as N:1 relations in an m2m mapping table, or will be referenced by the other table if it's a 1:N. + if rel.Type == schema.ManyToManyRelation || + rel.Type == schema.HasManyRelation { + continue + } + var fromCols, toCols []string for _, f := range rel.BaseFields { fromCols = append(fromCols, f.Name) diff --git a/schema/sqlfmt.go b/schema/sqlfmt.go index d0eb461cf..11eabb13b 100644 --- a/schema/sqlfmt.go +++ b/schema/sqlfmt.go @@ -1,6 +1,7 @@ package schema import ( + "fmt" "strings" "github.com/uptrace/bun/internal" @@ -50,6 +51,10 @@ func (fqn *FQN) AppendQuery(fmter Formatter, b []byte) ([]byte, error) { return fmter.AppendQuery(b, "?.?", Ident(fqn.Schema), Ident(fqn.Table)), nil } +func (fqn *FQN) String() string { + return fmt.Sprintf("%s.%s", fqn.Schema, fqn.Table) +} + //------------------------------------------------------------------------------ // Ident represents a SQL identifier, for example,