From 3d345c4b815a18a5dfe681a01a1c896dd570a8a0 Mon Sep 17 00:00:00 2001 From: Michael Fridman Date: Sun, 21 May 2023 08:20:29 -0400 Subject: [PATCH] fix: up and up -allow-missing behaviour (#524) --- tests/e2e/allow_missing_test.go | 36 +++++++++ up.go | 130 ++++++++------------------------ up_test.go | 4 +- 3 files changed, 70 insertions(+), 100 deletions(-) diff --git a/tests/e2e/allow_missing_test.go b/tests/e2e/allow_missing_test.go index cc18f29c4..26c24f58d 100644 --- a/tests/e2e/allow_missing_test.go +++ b/tests/e2e/allow_missing_test.go @@ -315,6 +315,42 @@ func TestMigrateAllowMissingDown(t *testing.T) { } } +func TestWithWithoutAllowMissing(t *testing.T) { + // Test for https://github.com/pressly/goose/issues/521 + + // Apply 1,2,4,3 then run up without allow missing. If the next requested migration is + // 4 then it should not raise an error because it has already been applied. + // If goose attempts to apply 4 again then it will raise an error (SQLSTATE 42701) because it + // has already been applied. Specifically it will raise a error. + db := setupTestDB(t, 2) + + migrations, err := goose.CollectMigrations(migrationsDir, 0, 4) + check.NoError(t, err) + err = migrations[3].Up(db) // version 4 + check.NoError(t, err) + err = migrations[2].Up(db) // version 3 + check.NoError(t, err) + + err = goose.UpTo(db, migrationsDir, 4) + check.NoError(t, err) + err = goose.UpTo(db, migrationsDir, 4, goose.WithAllowMissing()) + check.NoError(t, err) + + // Rollback migration 3 because it is the last applied migration. + // But, we want to change this behaviour to apply rollback migration 4. + // See these issues for more details: + // https://github.com/pressly/goose/issues/523 + // https://github.com/pressly/goose/issues/402 + // + // Adding this test to ensure the behaviour is updated and captured in a test. + err = goose.Down(db, migrationsDir) + check.NoError(t, err) + + version, err := goose.GetDBVersion(db) + check.NoError(t, err) + check.Number(t, version, 4) +} + // setupTestDB is helper to setup a DB and apply migrations // up to the specified version. func setupTestDB(t *testing.T, version int64) *sql.DB { diff --git a/up.go b/up.go index 66ba2d6fe..ab5a30630 100644 --- a/up.go +++ b/up.go @@ -3,7 +3,6 @@ package goose import ( "context" "database/sql" - "errors" "fmt" "sort" "strings" @@ -64,8 +63,14 @@ func UpTo(db *sql.DB, dir string, version int64, opts ...OptionsFunc) error { if err != nil { return err } + dbMaxVersion := dbMigrations[len(dbMigrations)-1].Version + // lookupAppliedInDB is a map of all applied migrations in the database. + lookupAppliedInDB := make(map[int64]bool) + for _, m := range dbMigrations { + lookupAppliedInDB[m.Version] = true + } - missingMigrations := findMissingMigrations(dbMigrations, foundMigrations) + missingMigrations := findMissingMigrations(dbMigrations, foundMigrations, dbMaxVersion) // feature(mf): It is very possible someone may want to apply ONLY new migrations // and skip missing migrations altogether. At the moment this is not supported, @@ -79,37 +84,38 @@ func UpTo(db *sql.DB, dir string, version int64, opts ...OptionsFunc) error { return fmt.Errorf("error: found %d missing migrations:\n\t%s", len(missingMigrations), strings.Join(collected, "\n\t")) } - + var migrationsToApply Migrations if option.allowMissing { - return upWithMissing( - db, - missingMigrations, - foundMigrations, - dbMigrations, - option, - ) + migrationsToApply = missingMigrations + } + // filter all migrations with a version greater than the supplied version (min) and less than or + // equal to the requested version (max). Note, we do not need to filter out missing migrations + // because we are only appending "new" migrations that have a higher version than the current + // database max version, which inevitably means they are not "missing". + for _, m := range foundMigrations { + if lookupAppliedInDB[m.Version] { + continue + } + if m.Version > dbMaxVersion && m.Version <= version { + migrationsToApply = append(migrationsToApply, m) + } } var current int64 - for { - var err error - current, err = GetDBVersion(db) - if err != nil { - return err - } - next, err := foundMigrations.Next(current) - if err != nil { - if errors.Is(err, ErrNoNextVersion) { - break - } - return fmt.Errorf("failed to find next migration: %v", err) - } - if err := next.Up(db); err != nil { + for _, m := range migrationsToApply { + if err := m.Up(db); err != nil { return err } if option.applyUpByOne { return nil } + current = m.Version + } + if len(migrationsToApply) == 0 && option.applyUpByOne { + current, err = GetDBVersion(db) + if err != nil { + return err + } } // At this point there are no more migrations to apply. But we need to maintain // the following behaviour: @@ -140,77 +146,6 @@ func upToNoVersioning(db *sql.DB, migrations Migrations, version int64) error { return nil } -func upWithMissing( - db *sql.DB, - missingMigrations Migrations, - foundMigrations Migrations, - dbMigrations Migrations, - option *options, -) error { - lookupApplied := make(map[int64]bool) - for _, found := range dbMigrations { - lookupApplied[found.Version] = true - } - - // Apply all missing migrations first. - for _, missing := range missingMigrations { - if err := missing.Up(db); err != nil { - return err - } - // Apply one migration and return early. - if option.applyUpByOne { - return nil - } - // TODO(mf): do we need this check? It's a bit redundant, but we may - // want to keep it as a safe-guard. Maybe we should instead have - // the underlying query (if possible) return the current version as - // part of the same transaction. - current, err := GetDBVersion(db) - if err != nil { - return err - } - if current == missing.Version { - lookupApplied[missing.Version] = true - continue - } - return fmt.Errorf("error: missing migration:%d does not match current db version:%d", - current, missing.Version) - } - - // We can no longer rely on the database version_id to be sequential because - // missing (out-of-order) migrations get applied before newer migrations. - - for _, found := range foundMigrations { - // TODO(mf): instead of relying on this lookup, consider hitting - // the database directly? - // Alternatively, we can skip a bunch migrations and start the cursor - // at a version that represents 100% applied migrations. But this is - // risky, and we should aim to keep this logic simple. - if lookupApplied[found.Version] { - continue - } - if err := found.Up(db); err != nil { - return err - } - if option.applyUpByOne { - return nil - } - } - current, err := GetDBVersion(db) - if err != nil { - return err - } - // At this point there are no more migrations to apply. But we need to maintain - // the following behaviour: - // UpByOne returns an error to signifying there are no more migrations. - // Up and UpTo return nil - log.Printf("goose: no migrations to run. current version: %d\n", current) - if option.applyUpByOne { - return ErrNoNextVersion - } - return nil -} - // Up applies all available migrations. func Up(db *sql.DB, dir string, opts ...OptionsFunc) error { return UpTo(db, dir, maxVersion, opts...) @@ -246,15 +181,14 @@ func listAllDBVersions(ctx context.Context, db *sql.DB) (Migrations, error) { // findMissingMigrations migrations returns all missing migrations. // A migrations is considered missing if it has a version less than the // current known max version. -func findMissingMigrations(knownMigrations, newMigrations Migrations) Migrations { - max := knownMigrations[len(knownMigrations)-1].Version +func findMissingMigrations(knownMigrations, newMigrations Migrations, dbMaxVersion int64) Migrations { existing := make(map[int64]bool) for _, known := range knownMigrations { existing[known.Version] = true } var missing Migrations for _, new := range newMigrations { - if !existing[new.Version] && new.Version < max { + if !existing[new.Version] && new.Version < dbMaxVersion { missing = append(missing, new) } } diff --git a/up_test.go b/up_test.go index cb491ca72..63c8f2eeb 100644 --- a/up_test.go +++ b/up_test.go @@ -10,7 +10,7 @@ func TestFindMissingMigrations(t *testing.T) { {Version: 3}, {Version: 4}, {Version: 5}, - {Version: 7}, + {Version: 7}, // <-- database max version_id } new := Migrations{ {Version: 1}, @@ -22,7 +22,7 @@ func TestFindMissingMigrations(t *testing.T) { {Version: 7}, // <-- database max version_id {Version: 8}, // new migration } - got := findMissingMigrations(known, new) + got := findMissingMigrations(known, new, 7) if len(got) != 2 { t.Fatalf("invalid migration count: got:%d want:%d", len(got), 2) }