fix: up and up -allow-missing behaviour (#524)

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 {

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)
 		}
 	}

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)
 	}