From a09ff0c048a0f850e0b298b7639fcca83077d571 Mon Sep 17 00:00:00 2001 From: Kian Parvin Date: Wed, 18 Dec 2024 18:41:36 +0200 Subject: [PATCH] chore: additional migration tests --- internal/db/db.go | 35 +++++++------ internal/db/db_test.go | 52 +++++++++++++++++++ internal/db/export_test.go | 9 ++++ .../invalidmigrations/001_initial.up.sql | 6 +++ .../002_add_invalid_table.up.sql | 6 +++ .../003_add_another_table.up.sql | 6 +++ .../validmigrations/001_initial.up.sql | 6 +++ .../validmigrations/002_add_table.up.sql | 5 ++ .../003_add_another_table.up.sql | 6 +++ internal/dbmodel/sql/migrations.md | 18 +++++++ 10 files changed, 134 insertions(+), 15 deletions(-) create mode 100644 internal/db/testdata/invalidmigrations/001_initial.up.sql create mode 100644 internal/db/testdata/invalidmigrations/002_add_invalid_table.up.sql create mode 100644 internal/db/testdata/invalidmigrations/003_add_another_table.up.sql create mode 100644 internal/db/testdata/validmigrations/001_initial.up.sql create mode 100644 internal/db/testdata/validmigrations/002_add_table.up.sql create mode 100644 internal/db/testdata/validmigrations/003_add_another_table.up.sql create mode 100644 internal/dbmodel/sql/migrations.md diff --git a/internal/db/db.go b/internal/db/db.go index 63efcc018..9e258c30e 100644 --- a/internal/db/db.go +++ b/internal/db/db.go @@ -6,6 +6,7 @@ package db import ( "context" "database/sql" + "embed" stderr "errors" "fmt" "path" @@ -26,7 +27,7 @@ import ( // Use a custom table name so that we don't run into collisions when OpenFGA or other tools // are using the same DB as JIMM in our Docker Compose setup. const ( - migrationTableName = "jimm_schema_migrations" + MigrationTableName = "jimm_schema_migrations" ) // A Database provides access to the database model. A Database instance @@ -87,46 +88,50 @@ func (d *Database) Migrate(ctx context.Context) error { if d == nil || d.DB == nil { return errors.E(op, errors.CodeServerConfiguration, "database not configured") } - db := d.DB.WithContext(ctx) + return d.migrateFromSource(ctx, dbmodel.SQL, path.Join("sql", d.DB.Name())) +} + +func (d *Database) migrateFromSource(ctx context.Context, fs embed.FS, sqlPath string) error { + sqlDir, err := iofs.New(fs, sqlPath) + if err != nil { + return fmt.Errorf("unable to create new sql filesys: %w", err) + } + + db := d.DB.WithContext(ctx) sqlDB, err := db.DB() if err != nil { - return errors.E(op, fmt.Errorf("unable to obtain raw DB: %w", err)) + return fmt.Errorf("failed to obtain raw DB: %w", err) } conn, err := sqlDB.Conn(ctx) if err != nil { return fmt.Errorf("failed to obtain DB conn: %w", err) } - sqlDir, err := iofs.New(dbmodel.SQL, path.Join("sql", db.Name())) - if err != nil { - return errors.E(op, fmt.Errorf("unable to create new sql filesys: %w", err)) - } - driver, err := postgres.WithConnection(ctx, conn, &postgres.Config{MigrationsTable: MigrationTableName}) if err != nil { - return errors.E(op, fmt.Errorf("unable to create new driver instance: %w", err)) + return fmt.Errorf("unable to create new driver instance: %w", err) } // DB name is left blank because it is contained in the driver/DB connection. m, err := migrate.NewWithInstance("iofs", sqlDir, "", driver) if err != nil { - return errors.E(op, fmt.Errorf("unable to create new migrator: %w", err)) + return fmt.Errorf("unable to create new migrator: %w", err) } defer m.Close() // Setup custom logger for consistent output. - logger := migrationLogger{logger: zapctx.Logger(ctx), verbose: false} + logger := migrationLogger{logger: zapctx.Logger(ctx), verbose: true} m.Log = logger if err := d.handleDeprecatedMigrations(ctx, m); err != nil { - return errors.E(op, fmt.Errorf("failed to handle deprecated migrations: %w", err)) + return fmt.Errorf("failed to handle deprecated migrations: %w", err) } v, dirty, err := m.Version() if err != nil { if !stderr.Is(err, migrate.ErrNilVersion) { - return errors.E(op, fmt.Errorf("failed to get db version: %w", err)) + return fmt.Errorf("failed to get db version: %w", err) } } @@ -135,13 +140,13 @@ func (d *Database) Migrate(ctx context.Context) error { workingVersion := int(v) - 1 zapctx.Info(ctx, "dirty database, reverting version", zap.Int("version", workingVersion)) if err := m.Force(workingVersion); err != nil { - return errors.E(op, fmt.Errorf("failed to fix dirty db version: %w", err)) + return fmt.Errorf("failed to fix dirty db version: %w", err) } } if err := m.Up(); err != nil { if !stderr.Is(err, migrate.ErrNoChange) { - return errors.E(op, fmt.Errorf("failed to migrate db: %w", err)) + return fmt.Errorf("failed to migrate db: %w", err) } } diff --git a/internal/db/db_test.go b/internal/db/db_test.go index 2b25df6ed..4b27ccace 100644 --- a/internal/db/db_test.go +++ b/internal/db/db_test.go @@ -4,15 +4,42 @@ package db_test import ( "context" + "embed" "testing" qt "github.com/frankban/quicktest" + "github.com/golang-migrate/migrate/v4" + "github.com/golang-migrate/migrate/v4/database/postgres" + "github.com/golang-migrate/migrate/v4/source/iofs" "github.com/canonical/jimm/v3/internal/db" "github.com/canonical/jimm/v3/internal/dbmodel" "github.com/canonical/jimm/v3/internal/errors" ) +//go:embed testdata/invalidmigrations/*.sql +var invalidSQL embed.FS + +//go:embed testdata/validmigrations/*.sql +var validSQL embed.FS + +func newTestMigrator(c *qt.C, d *db.Database, fs embed.FS, sqlPath string) *migrate.Migrate { + sqlDir, err := iofs.New(fs, sqlPath) + c.Assert(err, qt.IsNil) + + sqlDB, err := d.DB.DB() + c.Assert(err, qt.IsNil) + + driver, err := postgres.WithInstance(sqlDB, &postgres.Config{MigrationsTable: db.MigrationTableName}) + c.Assert(err, qt.IsNil) + + m, err := migrate.NewWithInstance("iofs", sqlDir, "", driver) + c.Assert(err, qt.IsNil) + c.Cleanup(func() { m.Close() }) + + return m +} + // dbSuite contains a suite of database tests that are run against // different database engines. type dbSuite struct { @@ -30,6 +57,31 @@ func (s *dbSuite) TestMigrate(c *qt.C) { c.Assert(err, qt.IsNil) } +// TestFailedMigration verifies a failed migration will cause a dirty migration +// that when fixed should automatically work. +func (s *dbSuite) TestFailedMigration(c *qt.C) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + err := s.Database.MigrateFromSource(ctx, invalidSQL, "testdata/invalidmigrations") + c.Assert(err, qt.Not(qt.IsNil)) + + m := newTestMigrator(c, s.Database, invalidSQL, "testdata/invalidmigrations") + v, dirty, err := m.Version() + c.Assert(err, qt.IsNil) + c.Assert(dirty, qt.IsTrue) + c.Assert(v, qt.Equals, uint(2)) + + err = s.Database.MigrateFromSource(ctx, validSQL, "testdata/validmigrations") + c.Assert(err, qt.IsNil) + + m = newTestMigrator(c, s.Database, validSQL, "testdata/validmigrations") + v, dirty, err = m.Version() + c.Assert(err, qt.IsNil) + c.Assert(dirty, qt.IsFalse) + c.Assert(v, qt.Equals, uint(3)) +} + func TestMigrateUnconfiguredDatabase(t *testing.T) { c := qt.New(t) diff --git a/internal/db/export_test.go b/internal/db/export_test.go index d3a632218..cdc8b8049 100644 --- a/internal/db/export_test.go +++ b/internal/db/export_test.go @@ -2,6 +2,11 @@ package db +import ( + "context" + "embed" +) + var ( JwksKind = jwksKind JwksPublicKeyTag = jwksPublicKeyTag @@ -12,3 +17,7 @@ var ( OAuthSessionStoreSecretTag = oauthSessionStoreSecretTag NewUUID = &newUUID ) + +func (d *Database) MigrateFromSource(ctx context.Context, fs embed.FS, sqlPath string) error { + return d.migrateFromSource(ctx, fs, sqlPath) +} diff --git a/internal/db/testdata/invalidmigrations/001_initial.up.sql b/internal/db/testdata/invalidmigrations/001_initial.up.sql new file mode 100644 index 000000000..7e9086820 --- /dev/null +++ b/internal/db/testdata/invalidmigrations/001_initial.up.sql @@ -0,0 +1,6 @@ +-- 1_1.sql initialises an empty database. + +CREATE TABLE IF NOT EXISTS test ( + id BIGSERIAL PRIMARY KEY, + time TIMESTAMP WITH TIME ZONE +); diff --git a/internal/db/testdata/invalidmigrations/002_add_invalid_table.up.sql b/internal/db/testdata/invalidmigrations/002_add_invalid_table.up.sql new file mode 100644 index 000000000..0ad8c4e52 --- /dev/null +++ b/internal/db/testdata/invalidmigrations/002_add_invalid_table.up.sql @@ -0,0 +1,6 @@ +-- 1_2.sql is a migration that adds an invalid table. + +CREATE TABLE IF NOT EXISTS invalid ( + id BIGSERIAL PRIMARY KEY, + time INVALIDTYPE +); diff --git a/internal/db/testdata/invalidmigrations/003_add_another_table.up.sql b/internal/db/testdata/invalidmigrations/003_add_another_table.up.sql new file mode 100644 index 000000000..083e65225 --- /dev/null +++ b/internal/db/testdata/invalidmigrations/003_add_another_table.up.sql @@ -0,0 +1,6 @@ +-- 1_3.sql is a migration that adds an controller table. + +CREATE TABLE IF NOT EXISTS controller ( + id BIGSERIAL PRIMARY KEY, + name TEXT +); diff --git a/internal/db/testdata/validmigrations/001_initial.up.sql b/internal/db/testdata/validmigrations/001_initial.up.sql new file mode 100644 index 000000000..7e9086820 --- /dev/null +++ b/internal/db/testdata/validmigrations/001_initial.up.sql @@ -0,0 +1,6 @@ +-- 1_1.sql initialises an empty database. + +CREATE TABLE IF NOT EXISTS test ( + id BIGSERIAL PRIMARY KEY, + time TIMESTAMP WITH TIME ZONE +); diff --git a/internal/db/testdata/validmigrations/002_add_table.up.sql b/internal/db/testdata/validmigrations/002_add_table.up.sql new file mode 100644 index 000000000..fa3a95a7a --- /dev/null +++ b/internal/db/testdata/validmigrations/002_add_table.up.sql @@ -0,0 +1,5 @@ +-- 1_2.sql is a migration that adds a valid table. + +CREATE TABLE IF NOT EXISTS valid ( + id BIGSERIAL PRIMARY KEY +); diff --git a/internal/db/testdata/validmigrations/003_add_another_table.up.sql b/internal/db/testdata/validmigrations/003_add_another_table.up.sql new file mode 100644 index 000000000..083e65225 --- /dev/null +++ b/internal/db/testdata/validmigrations/003_add_another_table.up.sql @@ -0,0 +1,6 @@ +-- 1_3.sql is a migration that adds an controller table. + +CREATE TABLE IF NOT EXISTS controller ( + id BIGSERIAL PRIMARY KEY, + name TEXT +); diff --git a/internal/dbmodel/sql/migrations.md b/internal/dbmodel/sql/migrations.md new file mode 100644 index 000000000..50cb00c94 --- /dev/null +++ b/internal/dbmodel/sql/migrations.md @@ -0,0 +1,18 @@ +# Notes + +Migrations are applied using [golang-migrate](https://github.com/golang-migrate/migrate). +Previously migrations were applied using a home-grown solution and a `versions` table. +The switch to golang-migrate was done to simplify our code. + +To cater for existing deployments, we handle the case that the `versions` table still +exists and "force" the new migration tool to align with the old. + +No "down" migrations are used currently. We aim to work with the philosophy that application +changes should be done such that we deprecate the use of any tables/columns, deploy these changes +and then later create a migration to make permanent changes to the DB. Ideally always moving +migrations forwards and never backwards. + +By default, golang-migrate does not run migrations in a transactions. +**But**, the [postgres](https://github.com/golang-migrate/migrate/blob/master/database/postgres/README.md#multi-statement-mode) driver has slightly unique behavior - "running multiple SQL statements in one Exec executes them inside a transaction". +So each migration file is in fact run in a transaction when using PostgreSQL. To be more explicit, +one can wrap the migration file with BEGIN/COMMIT instructions.