Refactor up implementation

up.go

@@ -3,7 +3,6 @@ package goose
 import (
 	"context"
 	"database/sql"
-	"errors"
 	"fmt"
 	"sort"
 	"strings"
@@ -64,6 +63,12 @@ func UpTo(db *sql.DB, dir string, version int64, opts ...OptionsFunc) error {
 	if err != nil {
 		return err
 	}
+	currentVersion := 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)
 
@@ -79,37 +84,26 @@ 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"))
 	}
+	migrationsToApply := missingMigrations
 
-	if option.allowMissing {
-		return upWithMissing(
-			db,
-			missingMigrations,
-			foundMigrations,
-			dbMigrations,
-			option,
-		)
+	for _, m := range foundMigrations {
+		if lookupAppliedInDB[m.Version] {
+			continue
+		}
+		if m.Version > currentVersion && 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
 	}
 	// At this point there are no more migrations to apply. But we need to maintain
 	// the following behaviour:
@@ -140,77 +134,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...)