Repository: amacneil/dbmate
Pull request #386 (2023-02-20 06:23:15)

Always wait when creating driver
- Always wait for database (if `db.WaitBefore` is true) during driver creation
- Simplify some code by creating driver multiple times instead of passing `drv` around


Patch:
diff --git a/main.go b/main.go
index 04a777f..b32c594 100644
--- a/main.go
+++ b/main.go
@@ -33,6 +33,7 @@ func NewApp() *cli.App {
 	app.Usage = "A lightweight, framework-independent database migration tool."
 	app.Version = dbmate.Version
 
+	defaultDB := dbmate.New(nil)
 	app.Flags = []cli.Flag{
 		&cli.StringFlag{
 			Name:    "url",
@@ -49,20 +50,20 @@ func NewApp() *cli.App {
 			Name:    "migrations-dir",
 			Aliases: []string{"d"},
 			EnvVars: []string{"DBMATE_MIGRATIONS_DIR"},
-			Value:   dbmate.DefaultMigrationsDir,
+			Value:   defaultDB.MigrationsDir,
 			Usage:   "specify the directory containing migration files",
 		},
 		&cli.StringFlag{
 			Name:    "migrations-table",
 			EnvVars: []string{"DBMATE_MIGRATIONS_TABLE"},
-			Value:   dbmate.DefaultMigrationsTableName,
+			Value:   defaultDB.MigrationsTableName,
 			Usage:   "specify the database table to record migrations in",
 		},
 		&cli.StringFlag{
 			Name:    "schema-file",
 			Aliases: []string{"s"},
 			EnvVars: []string{"DBMATE_SCHEMA_FILE"},
-			Value:   dbmate.DefaultSchemaFile,
+			Value:   defaultDB.SchemaFile,
 			Usage:   "specify the schema file location",
 		},
 		&cli.BoolFlag{
@@ -79,7 +80,7 @@ func NewApp() *cli.App {
 			Name:    "wait-timeout",
 			EnvVars: []string{"DBMATE_WAIT_TIMEOUT"},
 			Usage:   "timeout for --wait flag",
-			Value:   dbmate.DefaultWaitTimeout,
+			Value:   defaultDB.WaitTimeout,
 		},
 	}
 
@@ -231,9 +232,9 @@ func action(f func(*dbmate.DB, *cli.Context) error) cli.ActionFunc {
 		db.MigrationsTableName = c.String("migrations-table")
 		db.SchemaFile = c.String("schema-file")
 		db.WaitBefore = c.Bool("wait")
-		overrideTimeout := c.Duration("wait-timeout")
-		if overrideTimeout != 0 {
-			db.WaitTimeout = overrideTimeout
+		waitTimeout := c.Duration("wait-timeout")
+		if waitTimeout != 0 {
+			db.WaitTimeout = waitTimeout
 		}
 
 		return f(db, c)
diff --git a/pkg/dbmate/db.go b/pkg/dbmate/db.go
index 5c1c3a2..5d47c10 100644
--- a/pkg/dbmate/db.go
+++ b/pkg/dbmate/db.go
@@ -15,21 +15,6 @@ import (
 	"github.com/amacneil/dbmate/pkg/dbutil"
 )
 
-// DefaultMigrationsDir specifies default directory to find migration files
-const DefaultMigrationsDir = "./db/migrations"
-
-// DefaultMigrationsTableName specifies default database tables to record migraitons in
-const DefaultMigrationsTableName = "schema_migrations"
-
-// DefaultSchemaFile specifies default location for schema.sql
-const DefaultSchemaFile = "./db/schema.sql"
-
-// DefaultWaitInterval specifies length of time between connection attempts
-const DefaultWaitInterval = time.Second
-
-// DefaultWaitTimeout specifies maximum time for connection attempts
-const DefaultWaitTimeout = 60 * time.Second
-
 // Error codes
 var (
 	ErrNoMigrationFiles      = errors.New("no migration files found")
@@ -44,23 +29,33 @@ var (
 	ErrCreateDirectory       = errors.New("unable to create directory")
 )
 
+// migrationFileRegexp pattern for valid migration files
+var migrationFileRegexp = regexp.MustCompile(`^\d.*\.sql$`)
+
 // DB allows dbmate actions to be performed on a specified database
 type DB struct {
-	AutoDumpSchema      bool
-	DatabaseURL         *url.URL
-	MigrationsDir       string
+	// AutoDumpSchema generates schema.sql after each action
+	AutoDumpSchema bool
+	// DatabaseURL is the database connection string
+	DatabaseURL *url.URL
+	// Log is the interface to write stdout
+	Log io.Writer
+	// MigrationsDir specifies the directory to find migration files
+	MigrationsDir string
+	// MigrationsTableName specifies the database table to record migrations in
 	MigrationsTableName string
-	SchemaFile          string
-	Verbose             bool
-	WaitBefore          bool
-	WaitInterval        time.Duration
-	WaitTimeout         time.Duration
-	Log                 io.Writer
+	// SchemaFile specifies the location for schema.sql file
+	SchemaFile string
+	// Verbose prints the result of each statement execution
+	Verbose bool
+	// WaitBefore will wait for database to become available before running any actions
+	WaitBefore bool
+	// WaitInterval specifies length of time between connection attempts
+	WaitInterval time.Duration
+	// WaitTimeout specifies maximum time for connection attempts
+	WaitTimeout time.Duration
 }
 
-// migrationFileRegexp pattern for valid migration files
-var migrationFileRegexp = regexp.MustCompile(`^\d.*\.sql$`)
-
 // StatusResult represents an available migration status
 type StatusResult struct {
 	Filename string
@@ -72,18 +67,18 @@ func New(databaseURL *url.URL) *DB {
 	return &DB{
 		AutoDumpSchema:      true,
 		DatabaseURL:         databaseURL,
-		MigrationsDir:       DefaultMigrationsDir,
-		MigrationsTableName: DefaultMigrationsTableName,
-		SchemaFile:          DefaultSchemaFile,
+		MigrationsDir:       "./db/migrations",
+		MigrationsTableName: "schema_migrations",
+		SchemaFile:          "./db/schema.sql",
 		WaitBefore:          false,
-		WaitInterval:        DefaultWaitInterval,
-		WaitTimeout:         DefaultWaitTimeout,
+		WaitInterval:        time.Second,
+		WaitTimeout:         60 * time.Second,
 		Log:                 os.Stdout,
 	}
 }
 
-// GetDriver initializes the appropriate database driver
-func (db *DB) GetDriver() (Driver, error) {
+// Driver initializes the appropriate database driver
+func (db *DB) Driver() (Driver, error) {
 	if db.DatabaseURL == nil || db.DatabaseURL.Scheme == "" {
 		return nil, ErrInvalidURL
 	}
@@ -95,22 +90,18 @@ func (db *DB) GetDriver() (Driver, error) {
 
 	config := DriverConfig{
 		DatabaseURL:         db.DatabaseURL,
-		MigrationsTableName: db.MigrationsTableName,
 		Log:                 db.Log,
+		MigrationsTableName: db.MigrationsTableName,
 	}
+	drv := driverFunc(config)
 
-	return driverFunc(config), nil
-}
-
-// Wait blocks until the database server is available. It does not verify that
-// the specified database exists, only that the host is ready to accept connections.
-func (db *DB) Wait() error {
-	drv, err := db.GetDriver()
-	if err != nil {
-		return err
+	if db.WaitBefore {
+		if err := db.wait(drv); err != nil {
+			return nil, err
+		}
 	}
 
-	return db.wait(drv)
+	return drv, nil
 }
 
 func (db *DB) wait(drv Driver) error {
@@ -140,18 +131,23 @@ func (db *DB) wait(drv Driver) error {
 	return fmt.Errorf("%w: %s", ErrCantConnect, err)
 }
 
-// CreateAndMigrate creates the database (if necessary) and runs migrations
-func (db *DB) CreateAndMigrate() error {
-	drv, err := db.GetDriver()
+// Wait blocks until the database server is available. It does not verify that
+// the specified database exists, only that the host is ready to accept connections.
+func (db *DB) Wait() error {
+	drv, err := db.Driver()
 	if err != nil {
 		return err
 	}
 
-	if db.WaitBefore {
-		err := db.wait(drv)
-		if err != nil {
-			return err
-		}
+	// if db.WaitBefore is true, wait() will get called twice, no harm
+	return db.wait(drv)
+}
+
+// CreateAndMigrate creates the database (if necessary) and runs migrations
+func (db *DB) CreateAndMigrate() error {
+	drv, err := db.Driver()
+	if err != nil {
+		return err
 	}
 
 	// create database if it does not already exist
@@ -165,61 +161,36 @@ func (db *DB) CreateAndMigrate() error {
 	}
 
 	// migrate
-	return db.migrate(drv)
+	return db.Migrate()
 }
 
 // Create creates the current database
 func (db *DB) Create() error {
-	drv, err := db.GetDriver()
+	drv, err := db.Driver()
 	if err != nil {
 		return err
 	}
 
-	if db.WaitBefore {
-		err := db.wait(drv)
-		if err != nil {
-			return err
-		}
-	}
-
 	return drv.CreateDatabase()
 }
 
 // Drop drops the current database (if it exists)
 func (db *DB) Drop() error {
-	drv, err := db.GetDriver()
+	drv, err := db.Driver()
 	if err != nil {
 		return err
 	}
 
-	if db.WaitBefore {
-		err := db.wait(drv)
-		if err != nil {
-			return err
-		}
-	}
-
 	return drv.DropDatabase()
 }
 
 // DumpSchema writes the current database schema to a file
 func (db *DB) DumpSchema() error {
-	drv, err := db.GetDriver()
+	drv, err := db.Driver()
 	if err != nil {
 		return err
 	}
 
-	return db.dumpSchema(drv)
-}
-
-func (db *DB) dumpSchema(drv Driver) error {
-	if db.WaitBefore {
-		err := db.wait(drv)
-		if err != nil {
-			return err
-		}
-	}
-
 	sqlDB, err := db.openDatabaseForMigration(drv)
 	if err != nil {
 		return err
@@ -319,15 +290,6 @@ func (db *DB) openDatabaseForMigration(drv Driver) (*sql.DB, error) {
 
 // Migrate migrates database to the latest version
 func (db *DB) Migrate() error {
-	drv, err := db.GetDriver()
-	if err != nil {
-		return err
-	}
-
-	return db.migrate(drv)
-}
-
-func (db *DB) migrate(drv Driver) error {
 	files, err := findMigrationFiles(db.MigrationsDir, migrationFileRegexp)
 	if err != nil {
 		return err
@@ -337,11 +299,9 @@ func (db *DB) migrate(drv Driver) error {
 		return ErrNoMigrationFiles
 	}
 
-	if db.WaitBefore {
-		err := db.wait(drv)
-		if err != nil {
-			return err
-		}
+	drv, err := db.Driver()
+	if err != nil {
+		return err
 	}
 
 	sqlDB, err := db.openDatabaseForMigration(drv)
@@ -397,7 +357,7 @@ func (db *DB) migrate(drv Driver) error {
 
 	// automatically update schema file, silence errors
 	if db.AutoDumpSchema {
-		_ = db.dumpSchema(drv)
+		_ = db.DumpSchema()
 	}
 
 	return nil
@@ -465,18 +425,11 @@ func migrationVersion(filename string) string {
 
 // Rollback rolls back the most recent migration
 func (db *DB) Rollback() error {
-	drv, err := db.GetDriver()
+	drv, err := db.Driver()
 	if err != nil {
 		return err
 	}
 
-	if db.WaitBefore {
-		err := db.wait(drv)
-		if err != nil {
-			return err
-		}
-	}
-
 	sqlDB, err := db.openDatabaseForMigration(drv)
 	if err != nil {
 		return err
@@ -536,7 +489,7 @@ func (db *DB) Rollback() error {
 
 	// automatically update schema file, silence errors
 	if db.AutoDumpSchema {
-		_ = db.dumpSchema(drv)
+		_ = db.DumpSchema()
 	}
 
 	return nil
@@ -544,12 +497,7 @@ func (db *DB) Rollback() error {
 
 // Status shows the status of all migrations
 func (db *DB) Status(quiet bool) (int, error) {
-	drv, err := db.GetDriver()
-	if err != nil {
-		return -1, err
-	}
-
-	results, err := db.CheckMigrationsStatus(drv)
+	results, err := db.CheckMigrationsStatus()
 	if err != nil {
 		return -1, err
 	}
@@ -580,7 +528,12 @@ func (db *DB) Status(quiet bool) (int, error) {
 }
 
 // CheckMigrationsStatus returns the status of all available mgirations
-func (db *DB) CheckMigrationsStatus(drv Driver) ([]StatusResult, error) {
+func (db *DB) CheckMigrationsStatus() ([]StatusResult, error) {
+	drv, err := db.Driver()
+	if err != nil {
+		return nil, err
+	}
+
 	files, err := findMigrationFiles(db.MigrationsDir, migrationFileRegexp)
 	if err != nil {
 		return nil, err
diff --git a/pkg/dbmate/driver.go b/pkg/dbmate/driver.go
index 730a29b..4ad2b74 100644
--- a/pkg/dbmate/driver.go
+++ b/pkg/dbmate/driver.go
@@ -26,8 +26,8 @@ type Driver interface {
 // DriverConfig holds configuration passed to driver constructors
 type DriverConfig struct {
 	DatabaseURL         *url.URL
-	MigrationsTableName string
 	Log                 io.Writer
+	MigrationsTableName string
 }
 
 // DriverFunc represents a driver constructor


Test patch:
diff --git a/pkg/dbmate/db_test.go b/pkg/dbmate/db_test.go
index b3a0a79..cab7421 100644
--- a/pkg/dbmate/db_test.go
+++ b/pkg/dbmate/db_test.go
@@ -52,21 +52,21 @@ func TestNew(t *testing.T) {
 func TestGetDriver(t *testing.T) {
 	t.Run("missing URL", func(t *testing.T) {
 		db := dbmate.New(nil)
-		drv, err := db.GetDriver()
+		drv, err := db.Driver()
 		require.Nil(t, drv)
 		require.EqualError(t, err, "invalid url, have you set your --url flag or DATABASE_URL environment variable?")
 	})
 
 	t.Run("missing schema", func(t *testing.T) {
 		db := dbmate.New(dbutil.MustParseURL("//hi"))
-		drv, err := db.GetDriver()
+		drv, err := db.Driver()
 		require.Nil(t, drv)
 		require.EqualError(t, err, "invalid url, have you set your --url flag or DATABASE_URL environment variable?")
 	})
 
 	t.Run("invalid driver", func(t *testing.T) {
 		db := dbmate.New(dbutil.MustParseURL("foo://bar"))
-		drv, err := db.GetDriver()
+		drv, err := db.Driver()
 		require.EqualError(t, err, "unsupported driver: foo")
 		require.Nil(t, drv)
 	})
@@ -260,7 +260,7 @@ func TestMigrate(t *testing.T) {
 	for _, u := range testURLs() {
 		t.Run(u.Scheme, func(t *testing.T) {
 			db := newTestDB(t, u)
-			drv, err := db.GetDriver()
+			drv, err := db.Driver()
 			require.NoError(t, err)
 
 			// drop and recreate database
@@ -295,7 +295,7 @@ func TestUp(t *testing.T) {
 	for _, u := range testURLs() {
 		t.Run(u.Scheme, func(t *testing.T) {
 			db := newTestDB(t, u)
-			drv, err := db.GetDriver()
+			drv, err := db.Driver()
 			require.NoError(t, err)
 
 			// drop database
@@ -328,7 +328,7 @@ func TestRollback(t *testing.T) {
 	for _, u := range testURLs() {
 		t.Run(u.Scheme, func(t *testing.T) {
 			db := newTestDB(t, u)
-			drv, err := db.GetDriver()
+			drv, err := db.Driver()
 			require.NoError(t, err)
 
 			// drop, recreate, and migrate database
@@ -373,7 +373,7 @@ func TestStatus(t *testing.T) {
 	for _, u := range testURLs() {
 		t.Run(u.Scheme, func(t *testing.T) {
 			db := newTestDB(t, u)
-			drv, err := db.GetDriver()
+			drv, err := db.Driver()
 			require.NoError(t, err)
 
 			// drop, recreate, and migrate database
@@ -388,7 +388,7 @@ func TestStatus(t *testing.T) {
 			defer dbutil.MustClose(sqlDB)
 
 			// two pending
-			results, err := db.CheckMigrationsStatus(drv)
+			results, err := db.CheckMigrationsStatus()
 			require.NoError(t, err)
 			require.Len(t, results, 2)
 			require.False(t, results[0].Applied)
@@ -402,7 +402,7 @@ func TestStatus(t *testing.T) {
 			require.NoError(t, err)
 
 			// two applied
-			results, err = db.CheckMigrationsStatus(drv)
+			results, err = db.CheckMigrationsStatus()
 			require.NoError(t, err)
 			require.Len(t, results, 2)
 			require.True(t, results[0].Applied)
@@ -413,7 +413,7 @@ func TestStatus(t *testing.T) {
 			require.NoError(t, err)
 
 			// one applied, one pending
-			results, err = db.CheckMigrationsStatus(drv)
+			results, err = db.CheckMigrationsStatus()
 			require.NoError(t, err)
 			require.Len(t, results, 2)
 			require.True(t, results[0].Applied)
diff --git a/pkg/driver/clickhouse/clickhouse_test.go b/pkg/driver/clickhouse/clickhouse_test.go
index 8f1e53e..bfb4717 100644
--- a/pkg/driver/clickhouse/clickhouse_test.go
+++ b/pkg/driver/clickhouse/clickhouse_test.go
@@ -14,7 +14,7 @@ import (
 
 func testClickHouseDriver(t *testing.T) *Driver {
 	u := dbutil.MustParseURL(os.Getenv("CLICKHOUSE_TEST_URL"))
-	drv, err := dbmate.New(u).GetDriver()
+	drv, err := dbmate.New(u).Driver()
 	require.NoError(t, err)
 
 	return drv.(*Driver)
@@ -40,7 +40,7 @@ func prepTestClickHouseDB(t *testing.T) *sql.DB {
 
 func TestGetDriver(t *testing.T) {
 	db := dbmate.New(dbutil.MustParseURL("clickhouse://"))
-	drvInterface, err := db.GetDriver()
+	drvInterface, err := db.Driver()
 	require.NoError(t, err)
 
 	// driver should have URL and default migrations table set
diff --git a/pkg/driver/mysql/mysql_test.go b/pkg/driver/mysql/mysql_test.go
index 46d3f51..70a3113 100644
--- a/pkg/driver/mysql/mysql_test.go
+++ b/pkg/driver/mysql/mysql_test.go
@@ -14,7 +14,7 @@ import (
 
 func testMySQLDriver(t *testing.T) *Driver {
 	u := dbutil.MustParseURL(os.Getenv("MYSQL_TEST_URL"))
-	drv, err := dbmate.New(u).GetDriver()
+	drv, err := dbmate.New(u).Driver()
 	require.NoError(t, err)
 
 	return drv.(*Driver)
@@ -40,7 +40,7 @@ func prepTestMySQLDB(t *testing.T) *sql.DB {
 
 func TestGetDriver(t *testing.T) {
 	db := dbmate.New(dbutil.MustParseURL("mysql://"))
-	drvInterface, err := db.GetDriver()
+	drvInterface, err := db.Driver()
 	require.NoError(t, err)
 
 	// driver should have URL and default migrations table set
diff --git a/pkg/driver/postgres/postgres_test.go b/pkg/driver/postgres/postgres_test.go
index 3b699e5..fd29f00 100644
--- a/pkg/driver/postgres/postgres_test.go
+++ b/pkg/driver/postgres/postgres_test.go
@@ -15,7 +15,7 @@ import (
 
 func testPostgresDriver(t *testing.T) *Driver {
 	u := dbutil.MustParseURL(os.Getenv("POSTGRES_TEST_URL"))
-	drv, err := dbmate.New(u).GetDriver()
+	drv, err := dbmate.New(u).Driver()
 	require.NoError(t, err)
 
 	return drv.(*Driver)
@@ -41,7 +41,7 @@ func prepTestPostgresDB(t *testing.T) *sql.DB {
 
 func TestGetDriver(t *testing.T) {
 	db := dbmate.New(dbutil.MustParseURL("postgres://"))
-	drvInterface, err := db.GetDriver()
+	drvInterface, err := db.Driver()
 	require.NoError(t, err)
 
 	// driver should have URL and default migrations table set
diff --git a/pkg/driver/sqlite/sqlite_test.go b/pkg/driver/sqlite/sqlite_test.go
index 858957b..8ee357b 100644
--- a/pkg/driver/sqlite/sqlite_test.go
+++ b/pkg/driver/sqlite/sqlite_test.go
@@ -16,7 +16,7 @@ import (
 
 func testSQLiteDriver(t *testing.T) *Driver {
 	u := dbutil.MustParseURL(os.Getenv("SQLITE_TEST_URL"))
-	drv, err := dbmate.New(u).GetDriver()
+	drv, err := dbmate.New(u).Driver()
 	require.NoError(t, err)
 
 	return drv.(*Driver)
@@ -42,7 +42,7 @@ func prepTestSQLiteDB(t *testing.T) *sql.DB {
 
 func TestGetDriver(t *testing.T) {
 	db := dbmate.New(dbutil.MustParseURL("sqlite://"))
-	drvInterface, err := db.GetDriver()
+	drvInterface, err := db.Driver()
 	require.NoError(t, err)
 
 	// driver should have URL and default migrations table set