From fdfa9d64600d57c88897513bb116f027c232c483 Mon Sep 17 00:00:00 2001 From: Ibrahim Serdar Acikgoz Date: Wed, 12 Jan 2022 19:25:22 +0300 Subject: [PATCH 1/5] morph.go: ban global variables --- morph.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/morph.go b/morph.go index a8837a1..39ccb83 100644 --- a/morph.go +++ b/morph.go @@ -42,10 +42,6 @@ type Config struct { type EngineOption func(*Morph) -var defaultConfig = &Config{ - Logger: log.New(os.Stderr, "", log.LstdFlags), // add default logger -} - func WithLogger(logger *log.Logger) EngineOption { return func(m *Morph) { m.config.Logger = logger @@ -82,7 +78,9 @@ func WithLock(key string) EngineOption { // New creates a new instance of the migrations engine from an existing db instance and a migrations source. func New(ctx context.Context, driver drivers.Driver, source sources.Source, options ...EngineOption) (*Morph, error) { engine := &Morph{ - config: defaultConfig, + config: &Config{ + Logger: log.New(os.Stderr, "", log.LstdFlags), // add default logger + }, source: source, driver: driver, } From 9196be862ca15a300b2a734b22ac9dd27c37d80f Mon Sep 17 00:00:00 2001 From: Ibrahim Serdar Acikgoz Date: Wed, 12 Jan 2022 19:49:44 +0300 Subject: [PATCH 2/5] more globals --- drivers/mysql/mysql.go | 6 +++--- drivers/postgres/postgres.go | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/mysql/mysql.go b/drivers/mysql/mysql.go index d1d7e1d..795a2b5 100644 --- a/drivers/mysql/mysql.go +++ b/drivers/mysql/mysql.go @@ -69,7 +69,7 @@ func Open(connURL string) (drivers.Driver, error) { return nil, &drivers.AppError{Driver: driverName, OrigErr: err, Message: "failed to sanitize url from custom parameters"} } - driverConfig, err := mergeConfigWithParams(customParams, defaultConfig) + driverConfig, err := mergeConfigWithParams(customParams, *defaultConfig) if err != nil { return nil, &drivers.AppError{Driver: driverName, OrigErr: err, Message: "failed to merge custom params to driver config"} } @@ -368,7 +368,7 @@ func mergeConfigs(config *Config, defaultConfig *Config) *Config { return config } -func mergeConfigWithParams(params map[string]string, config *Config) (*Config, error) { +func mergeConfigWithParams(params map[string]string, config Config) (*Config, error) { var err error for _, configKey := range configParams { @@ -388,7 +388,7 @@ func mergeConfigWithParams(params map[string]string, config *Config) (*Config, e } } - return config, nil + return &config, nil } func (driver *mysql) addMigrationQuery(migration *models.Migration) string { diff --git a/drivers/postgres/postgres.go b/drivers/postgres/postgres.go index 5aab684..f272761 100644 --- a/drivers/postgres/postgres.go +++ b/drivers/postgres/postgres.go @@ -77,7 +77,7 @@ func Open(connURL string) (drivers.Driver, error) { return nil, &drivers.AppError{Driver: driverName, OrigErr: err, Message: "failed to sanitize url from custom parameters"} } - driverConfig, err := mergeConfigWithParams(customParams, defaultConfig) + driverConfig, err := mergeConfigWithParams(customParams, *defaultConfig) if err != nil { return nil, &drivers.AppError{Driver: driverName, OrigErr: err, Message: "failed to merge custom params to driver config"} } @@ -128,7 +128,7 @@ func currentSchema(conn *sql.Conn, config *Config) (string, error) { return schemaName, nil } -func mergeConfigWithParams(params map[string]string, config *Config) (*Config, error) { +func mergeConfigWithParams(params map[string]string, config Config) (*Config, error) { var err error for _, configKey := range configParams { @@ -148,7 +148,7 @@ func mergeConfigWithParams(params map[string]string, config *Config) (*Config, e } } - return config, nil + return &config, nil } func mergeConfigs(config, defaultConfig *Config) *Config { From 1ef943e5d2c782d90049975209f5e04d230820f7 Mon Sep 17 00:00:00 2001 From: Ibrahim Serdar Acikgoz Date: Wed, 12 Jan 2022 20:13:33 +0300 Subject: [PATCH 3/5] fix tests --- drivers/mysql/mysql_test.go | 12 ++++++++++-- drivers/postgres/postgres_test.go | 13 ++++++++++++- 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/drivers/mysql/mysql_test.go b/drivers/mysql/mysql_test.go index 3b94007..06f9ac4 100644 --- a/drivers/mysql/mysql_test.go +++ b/drivers/mysql/mysql_test.go @@ -95,11 +95,19 @@ func (suite *MysqlTestSuite) TestOpen() { }) suite.T().Run("when connURL is valid and bare uses default configuration", func(t *testing.T) { - connectedDriver, teardown := suite.InitializeDriver(testConnURL) + connectedDriver, teardown := suite.InitializeDriver(defaultConnURL) defer teardown() + cfg := &Config{ + Config: drivers.Config{ + MigrationsTable: defaultConfig.MigrationsTable, + StatementTimeoutInSecs: defaultConfig.StatementTimeoutInSecs, + MigrationMaxSize: defaultConfig.MigrationMaxSize, + }, + closeDBonClose: true, // we have created DB from DSN + } mysqlDriver := connectedDriver.(*mysql) - suite.Assert().EqualValues(defaultConfig, mysqlDriver.config) + suite.Assert().EqualValues(cfg, mysqlDriver.config) }) suite.T().Run("when connURL is valid can override migrations table", func(t *testing.T) { diff --git a/drivers/postgres/postgres_test.go b/drivers/postgres/postgres_test.go index 9596c85..69e4514 100644 --- a/drivers/postgres/postgres_test.go +++ b/drivers/postgres/postgres_test.go @@ -91,8 +91,19 @@ func (suite *PostgresTestSuite) TestOpen() { connectedDriver, teardown := suite.InitializeDriver(testConnURL) defer teardown() + cfg := &Config{ + Config: drivers.Config{ + MigrationsTable: defaultConfig.MigrationsTable, + StatementTimeoutInSecs: defaultConfig.StatementTimeoutInSecs, + MigrationMaxSize: defaultConfig.MigrationMaxSize, + }, + databaseName: databaseName, + schemaName: "public", + closeDBonClose: true, // we have created DB from DSN + } + pgDriver := connectedDriver.(*postgres) - suite.Assert().EqualValues(defaultConfig, pgDriver.config) + suite.Assert().EqualValues(cfg, pgDriver.config) }) suite.T().Run("when connURL is valid can override migrations table", func(t *testing.T) { From bfc8a2ffe0e56c561a02ca12624da1a5db3a25bd Mon Sep 17 00:00:00 2001 From: Ibrahim Serdar Acikgoz Date: Wed, 12 Jan 2022 20:56:03 +0300 Subject: [PATCH 4/5] clone configs --- drivers/mysql/mysql.go | 21 +++++++++++++++------ drivers/postgres/postgres.go | 22 ++++++++++++++++------ 2 files changed, 31 insertions(+), 12 deletions(-) diff --git a/drivers/mysql/mysql.go b/drivers/mysql/mysql.go index 795a2b5..58e0b1d 100644 --- a/drivers/mysql/mysql.go +++ b/drivers/mysql/mysql.go @@ -69,7 +69,7 @@ func Open(connURL string) (drivers.Driver, error) { return nil, &drivers.AppError{Driver: driverName, OrigErr: err, Message: "failed to sanitize url from custom parameters"} } - driverConfig, err := mergeConfigWithParams(customParams, *defaultConfig) + driverConfig, err := mergeConfigWithParams(customParams, defaultConfig) if err != nil { return nil, &drivers.AppError{Driver: driverName, OrigErr: err, Message: "failed to merge custom params to driver config"} } @@ -368,27 +368,36 @@ func mergeConfigs(config *Config, defaultConfig *Config) *Config { return config } -func mergeConfigWithParams(params map[string]string, config Config) (*Config, error) { +func mergeConfigWithParams(params map[string]string, config *Config) (*Config, error) { var err error + cfg := &Config{ + Config: drivers.Config{ + MigrationsTable: config.MigrationsTable, + StatementTimeoutInSecs: config.StatementTimeoutInSecs, + MigrationMaxSize: config.MigrationMaxSize, + }, + databaseName: config.databaseName, + closeDBonClose: config.closeDBonClose, + } for _, configKey := range configParams { if v, ok := params[configKey]; ok { switch configKey { case "x-migration-max-size": - if config.MigrationMaxSize, err = strconv.Atoi(v); err != nil { + if cfg.MigrationMaxSize, err = strconv.Atoi(v); err != nil { return nil, errors.New(fmt.Sprintf("failed to cast config param %s of %s", configKey, v)) } case "x-migrations-table": - config.MigrationsTable = v + cfg.MigrationsTable = v case "x-statement-timeout": - if config.StatementTimeoutInSecs, err = strconv.Atoi(v); err != nil { + if cfg.StatementTimeoutInSecs, err = strconv.Atoi(v); err != nil { return nil, errors.New(fmt.Sprintf("failed to cast config param %s of %s", configKey, v)) } } } } - return &config, nil + return cfg, nil } func (driver *mysql) addMigrationQuery(migration *models.Migration) string { diff --git a/drivers/postgres/postgres.go b/drivers/postgres/postgres.go index f272761..2ec8dcd 100644 --- a/drivers/postgres/postgres.go +++ b/drivers/postgres/postgres.go @@ -77,7 +77,7 @@ func Open(connURL string) (drivers.Driver, error) { return nil, &drivers.AppError{Driver: driverName, OrigErr: err, Message: "failed to sanitize url from custom parameters"} } - driverConfig, err := mergeConfigWithParams(customParams, *defaultConfig) + driverConfig, err := mergeConfigWithParams(customParams, defaultConfig) if err != nil { return nil, &drivers.AppError{Driver: driverName, OrigErr: err, Message: "failed to merge custom params to driver config"} } @@ -128,27 +128,37 @@ func currentSchema(conn *sql.Conn, config *Config) (string, error) { return schemaName, nil } -func mergeConfigWithParams(params map[string]string, config Config) (*Config, error) { +func mergeConfigWithParams(params map[string]string, config *Config) (*Config, error) { var err error + cfg := &Config{ + Config: drivers.Config{ + MigrationsTable: config.MigrationsTable, + StatementTimeoutInSecs: config.StatementTimeoutInSecs, + MigrationMaxSize: config.MigrationMaxSize, + }, + databaseName: config.databaseName, + schemaName: config.schemaName, + closeDBonClose: config.closeDBonClose, + } for _, configKey := range configParams { if v, ok := params[configKey]; ok { switch configKey { case "x-migration-max-size": - if config.MigrationMaxSize, err = strconv.Atoi(v); err != nil { + if cfg.MigrationMaxSize, err = strconv.Atoi(v); err != nil { return nil, errors.New(fmt.Sprintf("failed to cast config param %s of %s", configKey, v)) } case "x-migrations-table": - config.MigrationsTable = v + cfg.MigrationsTable = v case "x-statement-timeout": - if config.StatementTimeoutInSecs, err = strconv.Atoi(v); err != nil { + if cfg.StatementTimeoutInSecs, err = strconv.Atoi(v); err != nil { return nil, errors.New(fmt.Sprintf("failed to cast config param %s of %s", configKey, v)) } } } } - return &config, nil + return cfg, nil } func mergeConfigs(config, defaultConfig *Config) *Config { From f12a829dcbf90fca22d901dfe1e30cc123eb00d2 Mon Sep 17 00:00:00 2001 From: Ibrahim Serdar Acikgoz Date: Wed, 12 Jan 2022 21:03:08 +0300 Subject: [PATCH 5/5] reflect review comments --- drivers/mysql/mysql.go | 28 ++++++---------------------- drivers/mysql/mysql_test.go | 11 +++++++++++ drivers/mysql/utils.go | 11 +++++++++++ drivers/postgres/postgres.go | 31 +++++++------------------------ drivers/postgres/postgres_test.go | 7 +++++++ drivers/postgres/utils.go | 16 +++++++++++++++- 6 files changed, 57 insertions(+), 47 deletions(-) diff --git a/drivers/mysql/mysql.go b/drivers/mysql/mysql.go index 58e0b1d..860c185 100644 --- a/drivers/mysql/mysql.go +++ b/drivers/mysql/mysql.go @@ -16,13 +16,6 @@ import ( const driverName = "mysql" const defaultMigrationMaxSize = 10 * 1 << 20 // 10 MB -var defaultConfig = &Config{ - Config: drivers.Config{ - MigrationsTable: "db_migrations", - StatementTimeoutInSecs: 60, - MigrationMaxSize: defaultMigrationMaxSize, - }, -} // add here any custom driver configuration var configParams = []string{ @@ -44,7 +37,7 @@ type mysql struct { } func WithInstance(dbInstance *sql.DB, config *Config) (drivers.Driver, error) { - driverConfig := mergeConfigs(config, defaultConfig) + driverConfig := mergeConfigs(config, getDefaultConfig()) conn, err := dbInstance.Conn(context.Background()) if err != nil { @@ -69,7 +62,7 @@ func Open(connURL string) (drivers.Driver, error) { return nil, &drivers.AppError{Driver: driverName, OrigErr: err, Message: "failed to sanitize url from custom parameters"} } - driverConfig, err := mergeConfigWithParams(customParams, defaultConfig) + driverConfig, err := mergeConfigWithParams(customParams, getDefaultConfig()) if err != nil { return nil, &drivers.AppError{Driver: driverName, OrigErr: err, Message: "failed to merge custom params to driver config"} } @@ -370,34 +363,25 @@ func mergeConfigs(config *Config, defaultConfig *Config) *Config { func mergeConfigWithParams(params map[string]string, config *Config) (*Config, error) { var err error - cfg := &Config{ - Config: drivers.Config{ - MigrationsTable: config.MigrationsTable, - StatementTimeoutInSecs: config.StatementTimeoutInSecs, - MigrationMaxSize: config.MigrationMaxSize, - }, - databaseName: config.databaseName, - closeDBonClose: config.closeDBonClose, - } for _, configKey := range configParams { if v, ok := params[configKey]; ok { switch configKey { case "x-migration-max-size": - if cfg.MigrationMaxSize, err = strconv.Atoi(v); err != nil { + if config.MigrationMaxSize, err = strconv.Atoi(v); err != nil { return nil, errors.New(fmt.Sprintf("failed to cast config param %s of %s", configKey, v)) } case "x-migrations-table": - cfg.MigrationsTable = v + config.MigrationsTable = v case "x-statement-timeout": - if cfg.StatementTimeoutInSecs, err = strconv.Atoi(v); err != nil { + if config.StatementTimeoutInSecs, err = strconv.Atoi(v); err != nil { return nil, errors.New(fmt.Sprintf("failed to cast config param %s of %s", configKey, v)) } } } } - return cfg, nil + return config, nil } func (driver *mysql) addMigrationQuery(migration *models.Migration) string { diff --git a/drivers/mysql/mysql_test.go b/drivers/mysql/mysql_test.go index 06f9ac4..0b4530a 100644 --- a/drivers/mysql/mysql_test.go +++ b/drivers/mysql/mysql_test.go @@ -98,6 +98,7 @@ func (suite *MysqlTestSuite) TestOpen() { connectedDriver, teardown := suite.InitializeDriver(defaultConnURL) defer teardown() + defaultConfig := getDefaultConfig() cfg := &Config{ Config: drivers.Config{ MigrationsTable: defaultConfig.MigrationsTable, @@ -144,6 +145,8 @@ func (suite *MysqlTestSuite) TestOpen() { } func (suite *MysqlTestSuite) TestCreateSchemaTableIfNotExists() { + defaultConfig := getDefaultConfig() + suite.T().Run("it errors when connection is missing", func(t *testing.T) { driver := &mysql{} @@ -194,6 +197,8 @@ func (suite *MysqlTestSuite) TestCreateSchemaTableIfNotExists() { } func (suite *MysqlTestSuite) TestLock() { + defaultConfig := getDefaultConfig() + connectedDriver, teardown := suite.InitializeDriver(testConnURL) defer teardown() @@ -211,6 +216,8 @@ func (suite *MysqlTestSuite) TestLock() { } func (suite *MysqlTestSuite) TestUnlock() { + defaultConfig := getDefaultConfig() + connectedDriver, teardown := suite.InitializeDriver(testConnURL) defer teardown() @@ -230,6 +237,8 @@ func (suite *MysqlTestSuite) TestUnlock() { } func (suite *MysqlTestSuite) TestAppliedMigrations() { + defaultConfig := getDefaultConfig() + connectedDriver, teardown := suite.InitializeDriver(testConnURL) defer teardown() @@ -252,6 +261,8 @@ func (suite *MysqlTestSuite) TestAppliedMigrations() { } func (suite *MysqlTestSuite) TestApply() { + defaultConfig := getDefaultConfig() + testData := []struct { Scenario string PendingMigrations []*models.Migration diff --git a/drivers/mysql/utils.go b/drivers/mysql/utils.go index c4360cb..d23be18 100644 --- a/drivers/mysql/utils.go +++ b/drivers/mysql/utils.go @@ -1,6 +1,7 @@ package mysql import ( + "github.com/go-morph/morph/drivers" mysqlDriver "github.com/go-sql-driver/mysql" ) @@ -21,3 +22,13 @@ func extractDatabaseNameFromURL(conn string) (string, error) { return cfg.DBName, nil } + +func getDefaultConfig() *Config { + return &Config{ + Config: drivers.Config{ + MigrationsTable: "db_migrations", + StatementTimeoutInSecs: 60, + MigrationMaxSize: defaultMigrationMaxSize, + }, + } +} diff --git a/drivers/postgres/postgres.go b/drivers/postgres/postgres.go index 2ec8dcd..631f3e5 100644 --- a/drivers/postgres/postgres.go +++ b/drivers/postgres/postgres.go @@ -14,14 +14,7 @@ import ( ) var ( - driverName = "postgres" - defaultConfig = &Config{ - Config: drivers.Config{ - MigrationsTable: "db_migrations", - StatementTimeoutInSecs: 60, - MigrationMaxSize: defaultMigrationMaxSize, - }, - } + driverName = "postgres" defaultMigrationMaxSize = 10 * 1 << 20 // 10 MB configParams = []string{ "x-migration-max-size", @@ -44,7 +37,7 @@ type postgres struct { } func WithInstance(dbInstance *sql.DB, config *Config) (drivers.Driver, error) { - driverConfig := mergeConfigs(config, defaultConfig) + driverConfig := mergeConfigs(config, getDefaultConfig()) conn, err := dbInstance.Conn(context.Background()) if err != nil { @@ -77,7 +70,7 @@ func Open(connURL string) (drivers.Driver, error) { return nil, &drivers.AppError{Driver: driverName, OrigErr: err, Message: "failed to sanitize url from custom parameters"} } - driverConfig, err := mergeConfigWithParams(customParams, defaultConfig) + driverConfig, err := mergeConfigWithParams(customParams, getDefaultConfig()) if err != nil { return nil, &drivers.AppError{Driver: driverName, OrigErr: err, Message: "failed to merge custom params to driver config"} } @@ -130,35 +123,25 @@ func currentSchema(conn *sql.Conn, config *Config) (string, error) { func mergeConfigWithParams(params map[string]string, config *Config) (*Config, error) { var err error - cfg := &Config{ - Config: drivers.Config{ - MigrationsTable: config.MigrationsTable, - StatementTimeoutInSecs: config.StatementTimeoutInSecs, - MigrationMaxSize: config.MigrationMaxSize, - }, - databaseName: config.databaseName, - schemaName: config.schemaName, - closeDBonClose: config.closeDBonClose, - } for _, configKey := range configParams { if v, ok := params[configKey]; ok { switch configKey { case "x-migration-max-size": - if cfg.MigrationMaxSize, err = strconv.Atoi(v); err != nil { + if config.MigrationMaxSize, err = strconv.Atoi(v); err != nil { return nil, errors.New(fmt.Sprintf("failed to cast config param %s of %s", configKey, v)) } case "x-migrations-table": - cfg.MigrationsTable = v + config.MigrationsTable = v case "x-statement-timeout": - if cfg.StatementTimeoutInSecs, err = strconv.Atoi(v); err != nil { + if config.StatementTimeoutInSecs, err = strconv.Atoi(v); err != nil { return nil, errors.New(fmt.Sprintf("failed to cast config param %s of %s", configKey, v)) } } } } - return cfg, nil + return config, nil } func mergeConfigs(config, defaultConfig *Config) *Config { diff --git a/drivers/postgres/postgres_test.go b/drivers/postgres/postgres_test.go index 69e4514..c2772a8 100644 --- a/drivers/postgres/postgres_test.go +++ b/drivers/postgres/postgres_test.go @@ -91,6 +91,7 @@ func (suite *PostgresTestSuite) TestOpen() { connectedDriver, teardown := suite.InitializeDriver(testConnURL) defer teardown() + defaultConfig := getDefaultConfig() cfg := &Config{ Config: drivers.Config{ MigrationsTable: defaultConfig.MigrationsTable, @@ -141,6 +142,8 @@ func (suite *PostgresTestSuite) TestOpen() { } func (suite *PostgresTestSuite) TestCreateSchemaTableIfNotExists() { + defaultConfig := getDefaultConfig() + suite.T().Run("it errors when connection is missing", func(t *testing.T) { driver := &postgres{} @@ -236,6 +239,8 @@ func (suite *PostgresTestSuite) TestUnlock() { } func (suite *PostgresTestSuite) TestAppliedMigrations() { + defaultConfig := getDefaultConfig() + connectedDriver, teardown := suite.InitializeDriver(testConnURL) defer teardown() @@ -258,6 +263,8 @@ func (suite *PostgresTestSuite) TestAppliedMigrations() { } func (suite *PostgresTestSuite) TestApply() { + defaultConfig := getDefaultConfig() + testData := []struct { Scenario string PendingMigrations []*models.Migration diff --git a/drivers/postgres/utils.go b/drivers/postgres/utils.go index f4f18fe..7a686e9 100644 --- a/drivers/postgres/utils.go +++ b/drivers/postgres/utils.go @@ -1,6 +1,10 @@ package postgres -import "net/url" +import ( + "net/url" + + "github.com/go-morph/morph/drivers" +) func extractDatabaseNameFromURL(URL string) (string, error) { uri, err := url.Parse(URL) @@ -10,3 +14,13 @@ func extractDatabaseNameFromURL(URL string) (string, error) { return uri.Path[1:], nil } + +func getDefaultConfig() *Config { + return &Config{ + Config: drivers.Config{ + MigrationsTable: "db_migrations", + StatementTimeoutInSecs: 60, + MigrationMaxSize: defaultMigrationMaxSize, + }, + } +}