From 26024af1f958fcb129fbd9f6d6481a1be0e8ac30 Mon Sep 17 00:00:00 2001 From: Till Faelligen Date: Thu, 7 Jul 2022 12:43:11 +0200 Subject: [PATCH] Update comments, outdent if --- internal/sqlutil/migrate.go | 76 ++++++++++++++++---------------- internal/sqlutil/migrate_test.go | 10 ++--- 2 files changed, 44 insertions(+), 42 deletions(-) diff --git a/internal/sqlutil/migrate.go b/internal/sqlutil/migrate.go index 91dbb057e..18020a902 100644 --- a/internal/sqlutil/migrate.go +++ b/internal/sqlutil/migrate.go @@ -22,64 +22,64 @@ import ( "time" "github.com/matrix-org/dendrite/internal" + "github.com/sirupsen/logrus" ) const createDBMigrationsSQL = "" + "CREATE TABLE IF NOT EXISTS db_migrations (" + - " version TEXT PRIMARY KEY," + - " time TEXT," + - " dendrite_version TEXT" + + " version TEXT PRIMARY KEY NOT NULL," + + " time TEXT NOT NULL," + + " dendrite_version TEXT NOT NULL" + ");" const insertVersionSQL = "" + "INSERT INTO db_migrations (version, time, dendrite_version)" + - " VALUES ($1, $2, $3) " + - " ON CONFLICT(version) DO UPDATE SET dendrite_version = $4, time = $5" + " VALUES ($1, $2, $3)" const selectDBMigrationsSQL = "SELECT version FROM db_migrations" // Migration defines a migration to be run. type Migration struct { - // Version is a simple name description/name of this migration + // Version is a simple description/name of this migration. Version string - // Up defines function to execute + // Up defines the function to execute for an upgrade. Up func(ctx context.Context, txn *sql.Tx) error - // Down defines function to execute (not implemented yet) + // Down defines the function to execute for a downgrade (not implemented yet). Down func(ctx context.Context, txn *sql.Tx) error } -// Migrator the structure used by migrations +// Migrator type Migrator struct { db *sql.DB migrations []Migration - knownMigrations map[string]bool + knownMigrations map[string]struct{} mutex *sync.Mutex } -// NewMigrator creates a new DB migrator +// NewMigrator creates a new DB migrator. func NewMigrator(db *sql.DB) *Migrator { return &Migrator{ db: db, migrations: []Migration{}, - knownMigrations: make(map[string]bool), + knownMigrations: make(map[string]struct{}), mutex: &sync.Mutex{}, } } -// AddMigrations adds new migrations to the list. -// De-duplicates migrations by their version +// AddMigrations appends migrations to the list of migrations. Migrations are executed +// in the order they are added to the list. De-duplicates migrations using their Version field. func (m *Migrator) AddMigrations(migrations ...Migration) { m.mutex.Lock() defer m.mutex.Unlock() for _, mig := range migrations { - if !m.knownMigrations[mig.Version] { + if _, ok := m.knownMigrations[mig.Version]; !ok { m.migrations = append(m.migrations, mig) - m.knownMigrations[mig.Version] = true + m.knownMigrations[mig.Version] = struct{}{} } } } -// Up executes all migrations +// Up executes all migrations in order they were added. func (m *Migrator) Up(ctx context.Context) error { var ( err error @@ -95,30 +95,32 @@ func (m *Migrator) Up(ctx context.Context) error { for i := range m.migrations { now := time.Now().UTC().Format(time.RFC3339) migration := m.migrations[i] - if !executedMigrations[migration.Version] { - err = migration.Up(ctx, txn) - if err != nil { - return fmt.Errorf("unable to execute migration '%s': %w", migration.Version, err) - } - _, err = txn.ExecContext(ctx, insertVersionSQL, - migration.Version, - now, - dendriteVersion, - dendriteVersion, - now, - ) - if err != nil { - return fmt.Errorf("unable to insert executed migrations: %w", err) - } + logrus.Debugf("Executing database migration '%s'", migration.Version) + // Skip migration if it was already executed + if _, ok := executedMigrations[migration.Version]; ok { + continue + } + err = migration.Up(ctx, txn) + if err != nil { + return fmt.Errorf("unable to execute migration '%s': %w", migration.Version, err) + } + _, err = txn.ExecContext(ctx, insertVersionSQL, + migration.Version, + now, + dendriteVersion, + ) + if err != nil { + return fmt.Errorf("unable to insert executed migrations: %w", err) } } return nil }) } -// ExecutedMigrations returns a map with already executed migrations -func (m *Migrator) ExecutedMigrations(ctx context.Context) (map[string]bool, error) { - result := make(map[string]bool) +// ExecutedMigrations returns a map with already executed migrations in addition to creating the +// migrations table, if it doesn't exist. +func (m *Migrator) ExecutedMigrations(ctx context.Context) (map[string]struct{}, error) { + result := make(map[string]struct{}) _, err := m.db.ExecContext(ctx, createDBMigrationsSQL) if err != nil { return nil, fmt.Errorf("unable to create db_migrations: %w", err) @@ -127,13 +129,13 @@ func (m *Migrator) ExecutedMigrations(ctx context.Context) (map[string]bool, err if err != nil { return nil, fmt.Errorf("unable to query db_migrations: %w", err) } - defer rows.Close() // nolint: errcheck + defer internal.CloseAndLogIfError(ctx, rows, "ExecutedMigrations: rows.close() failed") var version string for rows.Next() { if err = rows.Scan(&version); err != nil { return nil, fmt.Errorf("unable to scan version: %w", err) } - result[version] = true + result[version] = struct{}{} } return result, rows.Err() diff --git a/internal/sqlutil/migrate_test.go b/internal/sqlutil/migrate_test.go index eb7571afc..d8bcae196 100644 --- a/internal/sqlutil/migrate_test.go +++ b/internal/sqlutil/migrate_test.go @@ -61,16 +61,16 @@ func Test_migrations_Up(t *testing.T) { tests := []struct { name string migrations []sqlutil.Migration - wantResult map[string]bool + wantResult map[string]struct{} wantErr bool }{ { name: "dummy migration", migrations: dummyMigrations, - wantResult: map[string]bool{ - "init": true, - "v2": true, - "multiple execs": true, + wantResult: map[string]struct{}{ + "init": {}, + "v2": {}, + "multiple execs": {}, }, }, {