From 0338fe451f87b738e92eea318423a6ca06caef31 Mon Sep 17 00:00:00 2001 From: Thomas Holst Date: Wed, 4 Feb 2026 16:53:20 +0100 Subject: [PATCH 01/15] Add target-only before migration and docs --- MIGRATION_APPROACHES.md | 125 ++++++++++++++++++++++++++++++++++++ pkg/db/gorm.go | 24 +++++-- pkg/migrate/README.md | 9 +++ pkg/migrate/migrate.go | 106 ++++++++++++++++++++++++++++++ pkg/migrate/migrate_test.go | 65 +++++++++++++++++++ 5 files changed, 323 insertions(+), 6 deletions(-) create mode 100644 MIGRATION_APPROACHES.md diff --git a/MIGRATION_APPROACHES.md b/MIGRATION_APPROACHES.md new file mode 100644 index 0000000..9a91cd1 --- /dev/null +++ b/MIGRATION_APPROACHES.md @@ -0,0 +1,125 @@ +# Migration Approaches + +This repository provides a migration flow that combines GORM AutoMigrate with +versioned SQL migrations powered by `golang-migrate`. This document describes +what is supported today, what is not supported, and the extension approach we +chose for "before AutoMigrate" use cases. + +## Current Flow (Baseline) + +For a service using `pkg/db`: + +1. Optional `setup.sql` is executed by `pkg/migrate` when migrations are run. +2. GORM AutoMigrate runs once (migrates to latest model definitions in code). +3. `golang-migrate` runs numbered `*.up.sql` migrations to the target version. +4. Optional `fdw.up.sql` / `fdw.down.sql` scripts run around the numbered steps. + +Key properties: +- AutoMigrate is **not version-aware** and always migrates to the latest code. +- SQL migrations are **versioned** and ordered by filename version. +- `setup.sql` must be **idempotent** because it runs on every migration call. + +## Supported + +- Versioned SQL migrations: `{version}_{name}.up.sql` / `{version}_{name}.down.sql` +- Optional `setup.sql` and FDW scripts +- Migrating to a specific target version (`MigrateDB(ctx, version, ...)`) + +## Not Supported (by design) + +- Per-version interleaving of AutoMigrate with SQL steps +- Version-specific AutoMigrate (GORM does not support this) +- Non-idempotent pre-AutoMigrate scripts without tracking + +## Extension: Target-Version Before Script + +Use case: GORM AutoMigrate cannot perform a change you need before the current +target version. To support this while keeping AutoMigrate, we allow a **single +target-version before script** that runs immediately before AutoMigrate. + +Naming: +- `{version}_{name}.before.up.sql` + +Rules: +- The before script runs **only** when `{version}` matches the target version. +- It is **not tracked**; it must be **idempotent**. +- All `.before.` files are excluded from the post-AutoMigrate `golang-migrate` + run to prevent accidental execution as a normal migration. + +This is intentionally limited to avoid interleaving issues with AutoMigrate. + +## Examples (File Lists and Execution Sequences) + +### Example A: Single-Version Upgrade (v6 -> v7) + +Files: +``` +006_previous.up.sql +007_add_column.up.sql +007_add_column.before.up.sql +``` + +Target version: `7` + +Execution sequence: +1. `007_add_column.before.up.sql` (idempotent, target-only) +2. GORM AutoMigrate (to latest schema, i.e., v7) +3. `006_previous.up.sql` +4. `007_add_column.up.sql` + +### Example B: Multi-Version Jump (v2 -> v5) + +Files: +``` +003_add_user.up.sql +004_add_order.up.sql +005_add_invoice.up.sql +005_add_invoice.before.up.sql +``` + +Target version: `5` + +Execution sequence: +1. `005_add_invoice.before.up.sql` (target-only) +2. GORM AutoMigrate (to latest schema, i.e., v5) +3. `003_add_user.up.sql` +4. `004_add_order.up.sql` +5. `005_add_invoice.up.sql` + +Note why we do **not** run `003.before.up.sql` or `004.before.up.sql`: +AutoMigrate cannot be constrained to v3 or v4, so interleaving would be +incorrect. The model is intentionally limited to the **target-only** before +script. + +### Example C: No Before Script + +Files: +``` +001_init.up.sql +002_add_user.up.sql +``` + +Target version: `2` + +Execution sequence: +1. GORM AutoMigrate +2. `001_init.up.sql` +3. `002_add_user.up.sql` + +## Extension Options (Tradeoffs) + +1. **Full SQL migrations** (no AutoMigrate) + - Pros: Deterministic, version-aware ordering, supports complex changes. + - Cons: More SQL to write and maintain; slower for rapid dev. + +2. **Separate migration streams** (before/after tables) + - Pros: Versioned before + after with tracking. + - Cons: Still cannot interleave AutoMigrate by version. + +3. **Version-aware migrations only** + - Requires a migration system that can apply model changes incrementally, + which GORM AutoMigrate does not provide. + +The target-only before script is the pragmatic compromise: it preserves +backwards compatibility, maintains the existing flow, and supports critical +pre-AutoMigrate fixes without re-architecting the migration system. diff --git a/pkg/db/gorm.go b/pkg/db/gorm.go index 4d4b6ab..caecd41 100644 --- a/pkg/db/gorm.go +++ b/pkg/db/gorm.go @@ -154,21 +154,33 @@ func runMigration(conn *gorm.DB, migFn MigrationFunc, migrationVersion uint, sta logging.LogErrorf(ErrDBConnection, "MigrateDB() - db handle is nil") return ErrDBConnection } - // Run GORM automigrations as supplied by service - err := migFn(conn) + sqlDB, err := conn.DB() if err != nil { + logging.LogErrorf(err, "error getting sql DB") return err } - - sqlDB, err := conn.DB() + if migrationVersion > 0 { + migration := migrate.NewMigration(sqlDB, migrationsSource, migrationsTable, logging.Logger()) + if _, err := migration.ExecuteTargetBeforeUp(context.Background(), migrationVersion); err != nil { + return err + } + } + // Run GORM automigrations as supplied by service + err = migFn(conn) if err != nil { - logging.LogErrorf(err, "error getting sql DB") return err } // Run manual migrations defined in sql scripts if needed if migrationVersion > 0 { - migration := migrate.NewMigration(sqlDB, migrationsSource, migrationsTable, logging.Logger()) + afterSource, cleanup, err := migrate.CreateAfterSourceFolder(migrationsSource) + if err != nil { + return err + } + if cleanup != nil { + defer cleanup() + } + migration := migrate.NewMigration(sqlDB, afterSource, migrationsTable, logging.Logger()) err = migration.MigrateDB(context.Background(), migrationVersion, startFromZero) } diff --git a/pkg/migrate/README.md b/pkg/migrate/README.md index 1ebbf49..6f47865 100644 --- a/pkg/migrate/README.md +++ b/pkg/migrate/README.md @@ -23,6 +23,15 @@ The setup script must be idempotent, as it will be run for every migration (unli The scripts are optional and must be called `fdw.up.sql` and `fdw.down.sql` and be placed in the same folder as the other sql scripts. The placeholders can be used like this well-known notation within the scripts: `{{.LocalUser}}`. The main use case for the scripts is to prepare the database for some foreign data migration like described in [Postgres FDW](https://www.postgresql.org/docs/12/postgres-fdw.html). +## Target-Version Before Script + +`go-pg-migrate` supports an optional target-version before script that runs once per migration invocation before GORM AutoMigrate. The script is **not tracked** and must be **idempotent**. + +- Naming: `{version}_{name}.before.up.sql` (example: `007_add_index.before.up.sql`) +- The before script is only considered when `{version}` matches the target version passed to `MigrateDB`. +- If no matching file exists, the before phase is skipped. +- Files with `.before.` are excluded from the post‑AutoMigrate migration run. + ## Migration Table `golang-migrate` needs a table that will contain the migration metadata (current version and the dirty status). This table will be created by the library with the given table name. diff --git a/pkg/migrate/migrate.go b/pkg/migrate/migrate.go index 90fc64e..4be8d25 100644 --- a/pkg/migrate/migrate.go +++ b/pkg/migrate/migrate.go @@ -5,6 +5,8 @@ import ( "database/sql" "fmt" "os" + "path/filepath" + "strconv" "strings" "text/template" @@ -21,6 +23,8 @@ const ( setupScriptName = "setup.sql" fdwUpScriptName = "fdw.up.sql" fdwDownScriptName = "fdw.down.sql" + beforeUpSuffix = ".before.up.sql" + beforeDownSuffix = ".before.down.sql" ) // Migration is the struct that holds the information needed for migrating a database. @@ -181,6 +185,52 @@ func (m *Migration) execute(ctx context.Context, filename string, templateData i return err } +// ExecuteTargetBeforeUp runs a target-version before migration if present. +// The script is expected to be idempotent because it is not tracked. +func (m *Migration) ExecuteTargetBeforeUp(ctx context.Context, migrationVersion uint) (bool, error) { + filename, err := findBeforeUpFile(m.sourceFolder, migrationVersion) + if err != nil { + return false, errors.Wrap(err, "could not scan for before migration") + } + if filename == "" { + _ = m.log.InfoGeneric(ctx, fmt.Sprintf("no before migration found for version %d - skipped", migrationVersion)) + return false, nil + } + if err := m.execute(ctx, filename, nil); err != nil { + return false, errors.Wrap(err, fmt.Sprintf("could not run before migration %q", filename)) + } + return true, nil +} + +// CreateAfterSourceFolder returns a temp folder containing only non-before migrations. +func CreateAfterSourceFolder(sourceFolder string) (string, func(), error) { + entries, err := os.ReadDir(sourceFolder) + if err != nil { + return "", nil, errors.Wrap(err, "could not read migrations folder") + } + tempDir, err := os.MkdirTemp("", "migrate-after-*") + if err != nil { + return "", nil, errors.Wrap(err, "could not create temp folder") + } + cleanup := func() { + _ = os.RemoveAll(tempDir) + } + for _, entry := range entries { + if entry.IsDir() { + continue + } + name := entry.Name() + if isBeforeMigrationFile(name) { + continue + } + if err := copyFile(filepath.Join(sourceFolder, name), filepath.Join(tempDir, name)); err != nil { + cleanup() + return "", nil, err + } + } + return tempDir, cleanup, nil +} + func fileExists(path string) (bool, error) { _, err := os.Stat(path) if err == nil { @@ -193,3 +243,59 @@ func fileExists(path string) (bool, error) { // file may exists but os.Stat fails for other reasons (eg. permission, failing disk) return false, err } + +func isBeforeMigrationFile(filename string) bool { + return strings.HasSuffix(filename, beforeUpSuffix) || strings.HasSuffix(filename, beforeDownSuffix) +} + +func findBeforeUpFile(sourceFolder string, migrationVersion uint) (string, error) { + entries, err := os.ReadDir(sourceFolder) + if err != nil { + return "", err + } + for _, entry := range entries { + if entry.IsDir() { + continue + } + name := entry.Name() + if !strings.HasSuffix(name, beforeUpSuffix) { + continue + } + version, ok := parseMigrationVersion(name) + if !ok { + continue + } + if version == migrationVersion { + return name, nil + } + } + return "", nil +} + +func parseMigrationVersion(filename string) (uint, bool) { + base := filepath.Base(filename) + sep := strings.Index(base, "_") + if sep <= 0 { + return 0, false + } + versionStr := base[:sep] + if len(versionStr) == 0 { + return 0, false + } + parsed, err := strconv.ParseUint(versionStr, 10, 32) + if err != nil { + return 0, false + } + return uint(parsed), true +} + +func copyFile(src, dest string) error { + data, err := os.ReadFile(src) + if err != nil { + return errors.Wrap(err, fmt.Sprintf("could not read %q", src)) + } + if err := os.WriteFile(dest, data, 0o644); err != nil { + return errors.Wrap(err, fmt.Sprintf("could not write %q", dest)) + } + return nil +} diff --git a/pkg/migrate/migrate_test.go b/pkg/migrate/migrate_test.go index ca7071b..d7cafe8 100644 --- a/pkg/migrate/migrate_test.go +++ b/pkg/migrate/migrate_test.go @@ -3,6 +3,8 @@ package migrate import ( "context" "log" + "os" + "path/filepath" "testing" _ "github.com/golang-migrate/migrate/v4/source/file" @@ -102,6 +104,61 @@ func TestMigration_parseFile(t *testing.T) { } } +func TestFindBeforeUpFile(t *testing.T) { + dir := t.TempDir() + writeMigrationFile(t, dir, "001_init.up.sql") + writeMigrationFile(t, dir, "002_add.before.up.sql") + writeMigrationFile(t, dir, "002_add.before.down.sql") + writeMigrationFile(t, dir, "003_other.before.up.sql") + + found, err := findBeforeUpFile(dir, 2) + if err != nil { + t.Fatalf("findBeforeUpFile() error = %v", err) + } + if found != "002_add.before.up.sql" { + t.Fatalf("findBeforeUpFile() got = %q, want %q", found, "002_add.before.up.sql") + } + + found, err = findBeforeUpFile(dir, 4) + if err != nil { + t.Fatalf("findBeforeUpFile() error = %v", err) + } + if found != "" { + t.Fatalf("findBeforeUpFile() got = %q, want empty", found) + } +} + +func TestCreateAfterSourceFolder_excludesBefore(t *testing.T) { + dir := t.TempDir() + writeMigrationFile(t, dir, "001_init.up.sql") + writeMigrationFile(t, dir, "002_add.before.up.sql") + writeMigrationFile(t, dir, "002_add.before.down.sql") + writeMigrationFile(t, dir, "setup.sql") + + afterDir, cleanup, err := CreateAfterSourceFolder(dir) + if err != nil { + t.Fatalf("CreateAfterSourceFolder() error = %v", err) + } + if cleanup != nil { + defer cleanup() + } + + entries, err := os.ReadDir(afterDir) + if err != nil { + t.Fatalf("ReadDir() error = %v", err) + } + found := map[string]bool{} + for _, entry := range entries { + found[entry.Name()] = true + } + if found["002_add.before.up.sql"] || found["002_add.before.down.sql"] { + t.Fatalf("CreateAfterSourceFolder() should exclude before files") + } + if !found["001_init.up.sql"] || !found["setup.sql"] { + t.Fatalf("CreateAfterSourceFolder() should keep non-before files") + } +} + var wantParsedFdwUp = `BEGIN; CREATE SERVER IF NOT EXISTS keymgmt_server FOREIGN DATA WRAPPER postgres_fdw OPTIONS (host 'myHostname', dbname 'myDBName', port '42'); @@ -116,3 +173,11 @@ var wantParsedSetup = `CREATE TABLE IF NOT EXISTS test_setup.testtable ( PRIMARY KEY (id) ); ` + +func writeMigrationFile(t *testing.T, dir, name string) { + t.Helper() + path := filepath.Join(dir, name) + if err := os.WriteFile(path, []byte("SELECT 1;"), 0o644); err != nil { + t.Fatalf("WriteFile(%q) error = %v", path, err) + } +} From 7dd2597f4a80b82249548200dfe8df103083bca0 Mon Sep 17 00:00:00 2001 From: Thomas Holst Date: Thu, 5 Feb 2026 21:50:10 +0100 Subject: [PATCH 02/15] Update migration diagrams and docs --- MIGRATION_APPROACHES.md | 179 ++++++------------ docs/migration-legacy-example-v5-to-v7.mmd | 10 + docs/migration-legacy.mmd | 10 + docs/migration-versioned-example-v5-to-v7.mmd | 16 ++ docs/migration-versioned.mmd | 11 ++ pkg/migrate/README.md | 47 ++--- 6 files changed, 122 insertions(+), 151 deletions(-) create mode 100644 docs/migration-legacy-example-v5-to-v7.mmd create mode 100644 docs/migration-legacy.mmd create mode 100644 docs/migration-versioned-example-v5-to-v7.mmd create mode 100644 docs/migration-versioned.mmd diff --git a/MIGRATION_APPROACHES.md b/MIGRATION_APPROACHES.md index 9a91cd1..82c73fd 100644 --- a/MIGRATION_APPROACHES.md +++ b/MIGRATION_APPROACHES.md @@ -1,125 +1,58 @@ # Migration Approaches -This repository provides a migration flow that combines GORM AutoMigrate with -versioned SQL migrations powered by `golang-migrate`. This document describes -what is supported today, what is not supported, and the extension approach we -chose for "before AutoMigrate" use cases. - -## Current Flow (Baseline) - -For a service using `pkg/db`: - -1. Optional `setup.sql` is executed by `pkg/migrate` when migrations are run. -2. GORM AutoMigrate runs once (migrates to latest model definitions in code). -3. `golang-migrate` runs numbered `*.up.sql` migrations to the target version. -4. Optional `fdw.up.sql` / `fdw.down.sql` scripts run around the numbered steps. - -Key properties: -- AutoMigrate is **not version-aware** and always migrates to the latest code. -- SQL migrations are **versioned** and ordered by filename version. -- `setup.sql` must be **idempotent** because it runs on every migration call. - -## Supported - -- Versioned SQL migrations: `{version}_{name}.up.sql` / `{version}_{name}.down.sql` -- Optional `setup.sql` and FDW scripts -- Migrating to a specific target version (`MigrateDB(ctx, version, ...)`) - -## Not Supported (by design) - -- Per-version interleaving of AutoMigrate with SQL steps -- Version-specific AutoMigrate (GORM does not support this) -- Non-idempotent pre-AutoMigrate scripts without tracking - -## Extension: Target-Version Before Script - -Use case: GORM AutoMigrate cannot perform a change you need before the current -target version. To support this while keeping AutoMigrate, we allow a **single -target-version before script** that runs immediately before AutoMigrate. - -Naming: -- `{version}_{name}.before.up.sql` - -Rules: -- The before script runs **only** when `{version}` matches the target version. -- It is **not tracked**; it must be **idempotent**. -- All `.before.` files are excluded from the post-AutoMigrate `golang-migrate` - run to prevent accidental execution as a normal migration. - -This is intentionally limited to avoid interleaving issues with AutoMigrate. - -## Examples (File Lists and Execution Sequences) - -### Example A: Single-Version Upgrade (v6 -> v7) - -Files: -``` -006_previous.up.sql -007_add_column.up.sql -007_add_column.before.up.sql -``` - -Target version: `7` - -Execution sequence: -1. `007_add_column.before.up.sql` (idempotent, target-only) -2. GORM AutoMigrate (to latest schema, i.e., v7) -3. `006_previous.up.sql` -4. `007_add_column.up.sql` - -### Example B: Multi-Version Jump (v2 -> v5) - -Files: -``` -003_add_user.up.sql -004_add_order.up.sql -005_add_invoice.up.sql -005_add_invoice.before.up.sql -``` - -Target version: `5` - -Execution sequence: -1. `005_add_invoice.before.up.sql` (target-only) -2. GORM AutoMigrate (to latest schema, i.e., v5) -3. `003_add_user.up.sql` -4. `004_add_order.up.sql` -5. `005_add_invoice.up.sql` - -Note why we do **not** run `003.before.up.sql` or `004.before.up.sql`: -AutoMigrate cannot be constrained to v3 or v4, so interleaving would be -incorrect. The model is intentionally limited to the **target-only** before -script. - -### Example C: No Before Script - -Files: -``` -001_init.up.sql -002_add_user.up.sql -``` - -Target version: `2` - -Execution sequence: -1. GORM AutoMigrate -2. `001_init.up.sql` -3. `002_add_user.up.sql` - -## Extension Options (Tradeoffs) - -1. **Full SQL migrations** (no AutoMigrate) - - Pros: Deterministic, version-aware ordering, supports complex changes. - - Cons: More SQL to write and maintain; slower for rapid dev. - -2. **Separate migration streams** (before/after tables) - - Pros: Versioned before + after with tracking. - - Cons: Still cannot interleave AutoMigrate by version. - -3. **Version-aware migrations only** - - Requires a migration system that can apply model changes incrementally, - which GORM AutoMigrate does not provide. - -The target-only before script is the pragmatic compromise: it preserves -backwards compatibility, maintains the existing flow, and supports critical -pre-AutoMigrate fixes without re-architecting the migration system. +This repository supports **two migration logics** for services using `pkg/db`. +The *legacy* logic mirrors what is on `main` today. The *versioned* logic is +new and enables interleaving per migration version. + +## 1) Legacy Migration Logic (as on main) + +Use when you want a single AutoMigrate call and minimal changes to existing +services. + +**API** +- Provide `WithMigrationFunc(func(*gorm.DB) error)` +- Optionally set `WithMigrationVersion(version)` + +**Behavior (main)** +- Executes a **target‑only** before script if present: + - `{version}_{name}.before.up.sql` +- Runs **one** AutoMigrate (latest models). +- Runs `setup.sql` (idempotent), then `fdw.up.sql` (optional). +- Runs numbered SQL migrations via `golang-migrate`: + - `{version}_{name}.up.sql` / `{version}_{name}.down.sql` +- Runs `fdw.down.sql` (optional). + +## 2) Versioned Migration Logic (new) + +Use when you need **interleaving** per migration version. + +**API** +- Provide `WithVersionedMigrationFunc(func(*gorm.DB, uint) error)` +- Optionally set `WithMigrationVersion(version)` + +**Behavior** +- Runs `setup.sql` once (idempotent). +- Runs `fdw.up.sql` / `fdw.down.sql` once (optional). +- For each version from current+1 to target: + 1. `{version}_{name}.before.up.sql` (optional) + 2. `AutoMigrate(version)` (service implementation) + 3. `{version}_{name}.after.up.sql` (optional) + +**Notes** +- Missing before/after scripts are skipped. +- If no after script exists for a version, a no‑op migration is applied so the + migration table advances. + +## File Naming Summary + +Legacy (main): +- Before: `{version}_{name}.before.up.sql` +- After: `{version}_{name}.up.sql` + +Versioned (new): +- Before: `{version}_{name}.before.up.sql` +- After: `{version}_{name}.after.up.sql` + +Shared: +- Setup: `setup.sql` +- FDW: `fdw.up.sql`, `fdw.down.sql` diff --git a/docs/migration-legacy-example-v5-to-v7.mmd b/docs/migration-legacy-example-v5-to-v7.mmd new file mode 100644 index 0000000..2b3e589 --- /dev/null +++ b/docs/migration-legacy-example-v5-to-v7.mmd @@ -0,0 +1,10 @@ +flowchart TB + %% Legacy Example: Current v5 -> Target v7 + + A0["Current DB version: 5"] --> A1["Target version: 7"] + A1 --> A2["AutoMigrate (once, latest models)"] + A2 --> A3["setup.sql (optional, idempotent)"] + A3 --> A4["fdw.up.sql (optional)"] + A4 --> A5["006_fix_constraint.up.sql"] + A5 --> A6["007_add_feature.up.sql"] + A6 --> A7["fdw.down.sql (optional)"] diff --git a/docs/migration-legacy.mmd b/docs/migration-legacy.mmd new file mode 100644 index 0000000..98e0709 --- /dev/null +++ b/docs/migration-legacy.mmd @@ -0,0 +1,10 @@ +flowchart TB + %% Legacy Migration Logic (as on main) + + A0["Start migrations"] --> A1["AutoMigrate (once, latest models)"] + A1 --> A2["setup.sql (optional, idempotent)"] + A2 --> A3["fdw.up.sql (optional)"] + A3 --> L1["For each version v = current+1 .. target"] + L1 --> L2["{version}_{name}.up.sql"] + L2 -.-> L1 + L2 --> A4["fdw.down.sql (optional)"] diff --git a/docs/migration-versioned-example-v5-to-v7.mmd b/docs/migration-versioned-example-v5-to-v7.mmd new file mode 100644 index 0000000..b184dbe --- /dev/null +++ b/docs/migration-versioned-example-v5-to-v7.mmd @@ -0,0 +1,16 @@ +flowchart TB + %% Versioned Example: Current v5 -> Target v7 + + B0["Current DB version: 5"] --> B1["Target version: 7"] + B1 --> B2["setup.sql (optional, idempotent)"] + B2 --> B3["fdw.up.sql (optional)"] + + B3 --> V1["006_fix_constraint.before.up.sql (optional)"] + V1 --> V2["AutoMigrate(6)"] + V2 --> V3["006_fix_constraint.after.up.sql (optional)"] + + V3 --> V4["007_add_feature.before.up.sql (optional)"] + V4 --> V5["AutoMigrate(7)"] + V5 --> V6["007_add_feature.after.up.sql (optional)"] + + V6 --> B4["fdw.down.sql (optional)"] diff --git a/docs/migration-versioned.mmd b/docs/migration-versioned.mmd new file mode 100644 index 0000000..9f51f02 --- /dev/null +++ b/docs/migration-versioned.mmd @@ -0,0 +1,11 @@ +flowchart TB + %% Versioned Migration Logic (WithVersionedMigrationFunc) + + B0["Start migrations"] --> B1["setup.sql (optional, idempotent)"] + B1 --> B2["fdw.up.sql (optional)"] + B2 --> V1["For each version v = current+1 .. target"] + V1 --> V2["{version}_{name}.before.up.sql (optional)"] + V2 --> V3["AutoMigrate(version)"] + V3 --> V4["{version}_{name}.after.up.sql (optional)"] + V4 -.-> V1 + V4 --> B3["fdw.down.sql (optional)"] diff --git a/pkg/migrate/README.md b/pkg/migrate/README.md index 6f47865..eab774c 100644 --- a/pkg/migrate/README.md +++ b/pkg/migrate/README.md @@ -1,38 +1,29 @@ # go-pg-migrate -Library for migrating the Postgres Database of PHDP services. It uses [golang-migrate V4](https://github.com/golang-migrate/migrate) for the migration. +Library for migrating Postgres databases in services using `pkg/db`. It is +built on top of `golang-migrate` and supports **two migration logics** used by +`go-svc`. -## Setup Script +## Shared Scripts -`go-pg-migrate` allows to run a setup script before the migration steps that will be handled by `golang-migrate`. -The script is optional and must be called `setup.sql` and be placed in the same folder as the other sql scripts. -The main use case for the setup script is creating an schema that will be used by `golang-migrate` for the migration table itself. -The setup script must be idempotent, as it will be run for every migration (unlike the migration steps that are skipped if the version is already present). +These scripts are optional and run regardless of which migration logic is used. -## Postgres foreign-data wrapper +- `setup.sql` (idempotent, runs once) +- `fdw.up.sql` and `fdw.down.sql` (optional, runs once around all versions) -`go-pg-migrate` allows to run additional scripts before and after the migration which are golang templated by a ForeignDatabase struct and the following fields: +## Legacy Logic (as on main) -- LocalUser string -- DBName string -- Hostname string -- Port uint -- User string -- Password string +- Single AutoMigrate (latest models). +- Target‑only before script: + - `{version}_{name}.before.up.sql` +- SQL migrations executed via `golang-migrate`: + - `{version}_{name}.up.sql` / `{version}_{name}.down.sql` -The scripts are optional and must be called `fdw.up.sql` and `fdw.down.sql` and be placed in the same folder as the other sql scripts. The placeholders can be used like this well-known notation within the scripts: `{{.LocalUser}}`. -The main use case for the scripts is to prepare the database for some foreign data migration like described in [Postgres FDW](https://www.postgresql.org/docs/12/postgres-fdw.html). +## Versioned Logic (new) -## Target-Version Before Script +- Interleaves per migration version: + 1. `{version}_{name}.before.up.sql` (optional) + 2. AutoMigrate(version) (service implementation) + 3. `{version}_{name}.after.up.sql` (optional) -`go-pg-migrate` supports an optional target-version before script that runs once per migration invocation before GORM AutoMigrate. The script is **not tracked** and must be **idempotent**. - -- Naming: `{version}_{name}.before.up.sql` (example: `007_add_index.before.up.sql`) -- The before script is only considered when `{version}` matches the target version passed to `MigrateDB`. -- If no matching file exists, the before phase is skipped. -- Files with `.before.` are excluded from the post‑AutoMigrate migration run. - -## Migration Table - -`golang-migrate` needs a table that will contain the migration metadata (current version and the dirty status). This table will be created by the library with the given table name. -However, the schema where the table is created is not configurable for postgres as of version 4 of `golang-migrate`. Instead, the `golang-migrate` library will create the table with the unqualified name, which will have the effect of creating the table in the current schema. Therefore, if the table is intended to be created in a particular schema, that schema needs to be set as the current schema (first element in the search path). +Missing before/after scripts are skipped. From bdf5e94dd27a979dc53d7cb35915d430110673a5 Mon Sep 17 00:00:00 2001 From: Thomas Holst Date: Fri, 6 Feb 2026 00:53:04 +0100 Subject: [PATCH 03/15] Refine versioned migration flow --- MIGRATION_APPROACHES.md | 20 +- docs/migration-versioned-example-v5-to-v7.mmd | 12 +- docs/migration-versioned.mmd | 9 +- pkg/db/gorm.go | 87 ++++++-- pkg/db/options.go | 15 ++ pkg/db/options_test.go | 33 +++ pkg/db/testing.go | 4 +- pkg/migrate/README.md | 11 +- pkg/migrate/migrate.go | 188 +++++++++++++++++- pkg/migrate/migrate_test.go | 78 ++++++++ 10 files changed, 408 insertions(+), 49 deletions(-) create mode 100644 pkg/db/options_test.go diff --git a/MIGRATION_APPROACHES.md b/MIGRATION_APPROACHES.md index 82c73fd..0f3023a 100644 --- a/MIGRATION_APPROACHES.md +++ b/MIGRATION_APPROACHES.md @@ -14,8 +14,6 @@ services. - Optionally set `WithMigrationVersion(version)` **Behavior (main)** -- Executes a **target‑only** before script if present: - - `{version}_{name}.before.up.sql` - Runs **one** AutoMigrate (latest models). - Runs `setup.sql` (idempotent), then `fdw.up.sql` (optional). - Runs numbered SQL migrations via `golang-migrate`: @@ -34,24 +32,26 @@ Use when you need **interleaving** per migration version. - Runs `setup.sql` once (idempotent). - Runs `fdw.up.sql` / `fdw.down.sql` once (optional). - For each version from current+1 to target: - 1. `{version}_{name}.before.up.sql` (optional) + 1. Versioned before script (optional) 2. `AutoMigrate(version)` (service implementation) - 3. `{version}_{name}.after.up.sql` (optional) + 3. Versioned after script (optional) + 4. Record version **after** the full sequence completes **Notes** - Missing before/after scripts are skipped. -- If no after script exists for a version, a no‑op migration is applied so the - migration table advances. +- The version bump represents completion of before + AutoMigrate + after. +- **Supported naming (versioned path only):** + - Before: `{version}_{name}.before.sql` **or** `{version}_{name}.before.up.sql` + - After: `{version}_{name}.after.sql` **or** `{version}_{name}.after.up.sql` ## File Naming Summary Legacy (main): -- Before: `{version}_{name}.before.up.sql` -- After: `{version}_{name}.up.sql` +- SQL migrations: `{version}_{name}.up.sql` Versioned (new): -- Before: `{version}_{name}.before.up.sql` -- After: `{version}_{name}.after.up.sql` +- Before: `{version}_{name}.before.sql` or `{version}_{name}.before.up.sql` +- After: `{version}_{name}.after.sql` or `{version}_{name}.after.up.sql` Shared: - Setup: `setup.sql` diff --git a/docs/migration-versioned-example-v5-to-v7.mmd b/docs/migration-versioned-example-v5-to-v7.mmd index b184dbe..51cbc07 100644 --- a/docs/migration-versioned-example-v5-to-v7.mmd +++ b/docs/migration-versioned-example-v5-to-v7.mmd @@ -5,12 +5,14 @@ flowchart TB B1 --> B2["setup.sql (optional, idempotent)"] B2 --> B3["fdw.up.sql (optional)"] - B3 --> V1["006_fix_constraint.before.up.sql (optional)"] + B3 --> V1["006_fix_constraint.before.sql (optional)"] V1 --> V2["AutoMigrate(6)"] - V2 --> V3["006_fix_constraint.after.up.sql (optional)"] + V2 --> V3["006_fix_constraint.after.sql (optional)"] + V3 --> V3b["Record version 6"] - V3 --> V4["007_add_feature.before.up.sql (optional)"] + V3b --> V4["007_add_feature.before.sql (optional)"] V4 --> V5["AutoMigrate(7)"] - V5 --> V6["007_add_feature.after.up.sql (optional)"] + V5 --> V6["007_add_feature.after.sql (optional)"] + V6 --> V6b["Record version 7"] - V6 --> B4["fdw.down.sql (optional)"] + V6b --> B4["fdw.down.sql (optional)"] diff --git a/docs/migration-versioned.mmd b/docs/migration-versioned.mmd index 9f51f02..4d66d1f 100644 --- a/docs/migration-versioned.mmd +++ b/docs/migration-versioned.mmd @@ -4,8 +4,9 @@ flowchart TB B0["Start migrations"] --> B1["setup.sql (optional, idempotent)"] B1 --> B2["fdw.up.sql (optional)"] B2 --> V1["For each version v = current+1 .. target"] - V1 --> V2["{version}_{name}.before.up.sql (optional)"] + V1 --> V2["{version}_{name}.before.sql or .before.up.sql (optional)"] V2 --> V3["AutoMigrate(version)"] - V3 --> V4["{version}_{name}.after.up.sql (optional)"] - V4 -.-> V1 - V4 --> B3["fdw.down.sql (optional)"] + V3 --> V4["{version}_{name}.after.sql or .after.up.sql (optional)"] + V4 --> V5["Record version v"] + V5 -.-> V1 + V5 --> B3["fdw.down.sql (optional)"] diff --git a/pkg/db/gorm.go b/pkg/db/gorm.go index caecd41..2513b0b 100644 --- a/pkg/db/gorm.go +++ b/pkg/db/gorm.go @@ -2,6 +2,7 @@ package db import ( "context" + stderrors "errors" "time" "github.com/pkg/errors" @@ -59,7 +60,7 @@ func Initialize(runCtx context.Context, opts *ConnectionOptions) <-chan struct{} defer logging.LogInfof("database connection closed") }() - err = runMigration(conn, opts.MigrationFunc, opts.MigrationVersion, opts.MigrationStartFromZero) + err = runMigration(conn, opts.VersionedMigrationFunc, opts.MigrationVersion, opts.MigrationStartFromZero) if err != nil { if opts.MigrationHaltOnError { logging.LogErrorf(err, "database migration failed - aborting") @@ -149,7 +150,7 @@ func retryExponential(runCtx context.Context, attempts uint, waitPeriod time.Dur } // runMigration Executes Migrations on the database -func runMigration(conn *gorm.DB, migFn MigrationFunc, migrationVersion uint, startFromZero bool) error { +func runMigration(conn *gorm.DB, migFn VersionedMigrationFunc, migrationVersion uint, startFromZero bool) error { if conn == nil { logging.LogErrorf(ErrDBConnection, "MigrateDB() - db handle is nil") return ErrDBConnection @@ -159,30 +160,82 @@ func runMigration(conn *gorm.DB, migFn MigrationFunc, migrationVersion uint, sta logging.LogErrorf(err, "error getting sql DB") return err } - if migrationVersion > 0 { - migration := migrate.NewMigration(sqlDB, migrationsSource, migrationsTable, logging.Logger()) - if _, err := migration.ExecuteTargetBeforeUp(context.Background(), migrationVersion); err != nil { - return err + if migrationVersion == 0 { + if migFn != nil { + return migFn(conn, 0) } + return nil + } + + ctx := context.Background() + migration := migrate.NewMigration(sqlDB, migrationsSource, migrationsTable, logging.Logger()) + + if err := migration.ExecuteSetup(ctx); err != nil { + return err + } + if err := migration.ExecuteFdwUp(ctx); err != nil { + return err } - // Run GORM automigrations as supplied by service - err = migFn(conn) + defer func() { + if err := migration.ExecuteFdwDown(ctx); err != nil { + logging.LogErrorf(err, "error executing fdw down script") + } + }() + + mpg, err := migration.MigrateInstance() if err != nil { return err } - // Run manual migrations defined in sql scripts if needed - if migrationVersion > 0 { - afterSource, cleanup, err := migrate.CreateAfterSourceFolder(migrationsSource) - if err != nil { + currentVersionRaw, dirty, err := mpg.Version() + if err != nil { + if stderrors.Is(err, migrate.ErrNilVersion) { + if startFromZero { + currentVersionRaw = 0 + } else { + // no migration info and startFromZero disabled -> treat as already at target + // nolint: gosec + if err := migrate.SetVersion(mpg, migrationVersion); err != nil { + return err + } + return nil + } + } else { return err } - if cleanup != nil { - defer cleanup() + } + currentVersion := uint(currentVersionRaw) + if dirty { + return errors.Errorf("database migration is dirty at version %d", currentVersion) + } + + for version := currentVersion + 1; version <= migrationVersion; version++ { + if _, err := migration.ExecuteBeforeUp(ctx, version); err != nil { + logging.LogErrorf(err, "error running before migration for version %d", version) + return err } - migration := migrate.NewMigration(sqlDB, afterSource, migrationsTable, logging.Logger()) - err = migration.MigrateDB(context.Background(), migrationVersion, startFromZero) + + if migFn != nil { + if err := migFn(conn, version); err != nil { + logging.LogErrorf(err, "error running auto migration for version %d", version) + return err + } + } + + if _, err := migration.ExecuteAfterUp(ctx, version); err != nil { + logging.LogErrorf(err, "error running after migration for version %d", version) + return err + } + + // Record version after full before/auto/after sequence. + // nolint: gosec + if err := migrate.SetVersion(mpg, version); err != nil { + logging.LogErrorf(err, "error setting migration version to %d", version) + return err + } + + logging.LogInfof("migration for version %d executed successfully", version) } - return err + return nil } diff --git a/pkg/db/options.go b/pkg/db/options.go index 948d8a7..0d5b718 100644 --- a/pkg/db/options.go +++ b/pkg/db/options.go @@ -18,6 +18,7 @@ import ( const SSLVerifyFull = "verify-full" type MigrationFunc func(do *gorm.DB) error +type VersionedMigrationFunc func(do *gorm.DB, version uint) error type DriverFunc func(connectString string, opts *ConnectionOptions) (*gorm.DB, error) func NewConnection(opts ...ConnectionOption) *ConnectionOptions { @@ -50,6 +51,7 @@ type ConnectionOptions struct { // The cert is provided by Jenkins on build under default path "/root.ca.pem" SSLRootCertPath string MigrationFunc MigrationFunc + VersionedMigrationFunc VersionedMigrationFunc DriverFunc DriverFunc EnableInstrumentation bool LoggerConfig logger.Config @@ -144,6 +146,19 @@ func WithSSLRootCertPath(value string) ConnectionOption { func WithMigrationFunc(fn MigrationFunc) ConnectionOption { return func(c *ConnectionOptions) { c.MigrationFunc = fn + // Backward compatibility: wrap legacy migration into versioned flow. + c.VersionedMigrationFunc = func(do *gorm.DB, version uint) error { + if version == c.MigrationVersion { + return fn(do) + } + return nil + } + } +} + +func WithVersionedMigrationFunc(fn VersionedMigrationFunc) ConnectionOption { + return func(c *ConnectionOptions) { + c.VersionedMigrationFunc = fn } } diff --git a/pkg/db/options_test.go b/pkg/db/options_test.go new file mode 100644 index 0000000..e93ad4b --- /dev/null +++ b/pkg/db/options_test.go @@ -0,0 +1,33 @@ +package db + +import ( + "testing" + + "gorm.io/gorm" +) + +func TestWithMigrationFuncWrapsVersioned(t *testing.T) { + calls := 0 + fn := func(_ *gorm.DB) error { + calls++ + return nil + } + + opts := NewConnection( + WithMigrationVersion(2), + WithMigrationFunc(fn), + ) + if opts.VersionedMigrationFunc == nil { + t.Fatalf("expected VersionedMigrationFunc to be set") + } + + _ = opts.VersionedMigrationFunc(nil, 1) + if calls != 0 { + t.Fatalf("expected no calls for version 1, got %d", calls) + } + + _ = opts.VersionedMigrationFunc(nil, 2) + if calls != 1 { + t.Fatalf("expected one call for version 2, got %d", calls) + } +} diff --git a/pkg/db/testing.go b/pkg/db/testing.go index 128dd57..750a02a 100644 --- a/pkg/db/testing.go +++ b/pkg/db/testing.go @@ -38,8 +38,8 @@ func InitializeTestPostgres(opts *ConnectionOptions) { logging.LogErrorf(err, "error connecting to testing postgres") db = nil } - if opts.MigrationFunc != nil { - if err = runMigration(conn, opts.MigrationFunc, opts.MigrationVersion, true); err != nil { + if opts.VersionedMigrationFunc != nil { + if err = runMigration(conn, opts.VersionedMigrationFunc, opts.MigrationVersion, true); err != nil { logging.LogErrorf(err, "test DB migration error") } } diff --git a/pkg/migrate/README.md b/pkg/migrate/README.md index eab774c..a60a577 100644 --- a/pkg/migrate/README.md +++ b/pkg/migrate/README.md @@ -14,16 +14,19 @@ These scripts are optional and run regardless of which migration logic is used. ## Legacy Logic (as on main) - Single AutoMigrate (latest models). -- Target‑only before script: - - `{version}_{name}.before.up.sql` - SQL migrations executed via `golang-migrate`: - `{version}_{name}.up.sql` / `{version}_{name}.down.sql` ## Versioned Logic (new) - Interleaves per migration version: - 1. `{version}_{name}.before.up.sql` (optional) + 1. Versioned before script (optional) 2. AutoMigrate(version) (service implementation) - 3. `{version}_{name}.after.up.sql` (optional) + 3. Versioned after script (optional) + 4. Record version after the full sequence completes Missing before/after scripts are skipped. + +**Supported naming (versioned path only):** +- Before: `{version}_{name}.before.sql` or `{version}_{name}.before.up.sql` +- After: `{version}_{name}.after.sql` or `{version}_{name}.after.up.sql` diff --git a/pkg/migrate/migrate.go b/pkg/migrate/migrate.go index 4be8d25..7ba60db 100644 --- a/pkg/migrate/migrate.go +++ b/pkg/migrate/migrate.go @@ -25,8 +25,14 @@ const ( fdwDownScriptName = "fdw.down.sql" beforeUpSuffix = ".before.up.sql" beforeDownSuffix = ".before.down.sql" + afterUpSuffix = ".after.up.sql" + beforeSuffix = ".before.sql" + afterSuffix = ".after.sql" ) +// ErrNilVersion is returned when no migration version is set in the database. +var ErrNilVersion = migrate.ErrNilVersion + // Migration is the struct that holds the information needed for migrating a database. type Migration struct { db *sql.DB @@ -87,11 +93,48 @@ func (m *Migration) MigrateDB(ctx context.Context, migrationVersion uint, startF return errors.Wrap(err, "could not run the fdw.up script") } + if err := m.MigrateToVersion(ctx, migrationVersion, startFromZero); err != nil { + return err + } + + if err := m.execute(ctx, fdwDownScriptName, m.foreignDatabase); err != nil { // execute fdw.down + return errors.Wrap(err, "could not run the fdw.down script") + } + + return nil +} + +// ExecuteSetup runs the setup.sql script if present. +func (m *Migration) ExecuteSetup(ctx context.Context) error { + if err := m.execute(ctx, setupScriptName, nil); err != nil { + return errors.Wrap(err, "could not run the setup script") + } + return nil +} + +// ExecuteFdwUp runs the fdw.up.sql script if present. +func (m *Migration) ExecuteFdwUp(ctx context.Context) error { + if err := m.execute(ctx, fdwUpScriptName, m.foreignDatabase); err != nil { + return errors.Wrap(err, "could not run the fdw.up script") + } + return nil +} + +// ExecuteFdwDown runs the fdw.down.sql script if present. +func (m *Migration) ExecuteFdwDown(ctx context.Context) error { + if err := m.execute(ctx, fdwDownScriptName, m.foreignDatabase); err != nil { + return errors.Wrap(err, "could not run the fdw.down script") + } + return nil +} + +// MigrateInstance creates a golang-migrate instance for the current source folder. +func (m *Migration) MigrateInstance() (*migrate.Migrate, error) { driver, err := postgres.WithInstance(m.db, &postgres.Config{ MigrationsTable: m.migrationTable, }) if err != nil { - return errors.Wrap(err, "error creating database driver") + return nil, errors.Wrap(err, "error creating database driver") } mpg, err := migrate.NewWithDatabaseInstance( @@ -100,7 +143,30 @@ func (m *Migration) MigrateDB(ctx context.Context, migrationVersion uint, startF driver, ) if err != nil { - return errors.Wrap(err, "error creating migrate instance") + return nil, errors.Wrap(err, "error creating migrate instance") + } + return mpg, nil +} + +// CurrentVersion returns the current migration version. +func (m *Migration) CurrentVersion(ctx context.Context) (uint, bool, error) { + mpg, err := m.MigrateInstance() + if err != nil { + return 0, false, err + } + + version, dirty, err := mpg.Version() + if err != nil { + return 0, false, err + } + return uint(version), dirty, nil +} + +// MigrateToVersion runs golang-migrate without setup/fdw scripts. +func (m *Migration) MigrateToVersion(ctx context.Context, migrationVersion uint, startFromZero bool) error { + mpg, err := m.MigrateInstance() + if err != nil { + return err } _, _, err = mpg.Version() @@ -127,10 +193,15 @@ func (m *Migration) MigrateDB(ctx context.Context, migrationVersion uint, startF return errors.Wrap(err, fmt.Sprintf("error migrating database to v%d", migrationVersion)) } - if err := m.execute(ctx, fdwDownScriptName, m.foreignDatabase); err != nil { // execute fdw.down - return errors.Wrap(err, "could not run the fdw.down script") - } + return nil +} +// SetVersion records the current version in the migrations table using an existing migrate instance. +func SetVersion(mpg *migrate.Migrate, migrationVersion uint) error { + // nolint: gosec + if err := mpg.Force(int(migrationVersion)); err != nil { + return errors.Wrap(err, "error setting migration version") + } return nil } @@ -202,6 +273,38 @@ func (m *Migration) ExecuteTargetBeforeUp(ctx context.Context, migrationVersion return true, nil } +// ExecuteBeforeUp runs a versioned before migration if present. +func (m *Migration) ExecuteBeforeUp(ctx context.Context, migrationVersion uint) (bool, error) { + filename, err := findBeforeUpFile(m.sourceFolder, migrationVersion) + if err != nil { + return false, errors.Wrap(err, "could not scan for before migration") + } + if filename == "" { + _ = m.log.InfoGeneric(ctx, fmt.Sprintf("no before migration found for version %d - skipped", migrationVersion)) + return false, nil + } + if err := m.execute(ctx, filename, nil); err != nil { + return false, errors.Wrap(err, fmt.Sprintf("could not run before migration %q", filename)) + } + return true, nil +} + +// ExecuteAfterUp runs a versioned after migration if present. +func (m *Migration) ExecuteAfterUp(ctx context.Context, migrationVersion uint) (bool, error) { + filename, err := findAfterUpFile(m.sourceFolder, migrationVersion) + if err != nil { + return false, errors.Wrap(err, "could not scan for after migration") + } + if filename == "" { + _ = m.log.InfoGeneric(ctx, fmt.Sprintf("no after migration found for version %d - skipped", migrationVersion)) + return false, nil + } + if err := m.execute(ctx, filename, nil); err != nil { + return false, errors.Wrap(err, fmt.Sprintf("could not run after migration %q", filename)) + } + return true, nil +} + // CreateAfterSourceFolder returns a temp folder containing only non-before migrations. func CreateAfterSourceFolder(sourceFolder string) (string, func(), error) { entries, err := os.ReadDir(sourceFolder) @@ -231,6 +334,49 @@ func CreateAfterSourceFolder(sourceFolder string) (string, func(), error) { return tempDir, cleanup, nil } +// CreateAfterSourceFolderForVersion returns a temp folder with the after migration for a single version. +func CreateAfterSourceFolderForVersion(sourceFolder string, migrationVersion uint) (string, func(), error) { + entries, err := os.ReadDir(sourceFolder) + if err != nil { + return "", nil, errors.Wrap(err, "could not read migrations folder") + } + tempDir, err := os.MkdirTemp("", "migrate-after-*") + if err != nil { + return "", nil, errors.Wrap(err, "could not create temp folder") + } + cleanup := func() { + _ = os.RemoveAll(tempDir) + } + copied := false + for _, entry := range entries { + if entry.IsDir() { + continue + } + name := entry.Name() + if !strings.HasSuffix(name, afterUpSuffix) { + continue + } + version, ok := parseMigrationVersion(name) + if !ok || version != migrationVersion { + continue + } + targetName := strings.TrimSuffix(name, afterUpSuffix) + ".up.sql" + if err := copyFile(filepath.Join(sourceFolder, name), filepath.Join(tempDir, targetName)); err != nil { + cleanup() + return "", nil, err + } + copied = true + } + if !copied { + noopName := fmt.Sprintf("%d_noop.up.sql", migrationVersion) + if err := os.WriteFile(filepath.Join(tempDir, noopName), []byte("SELECT 1;"), 0o644); err != nil { + cleanup() + return "", nil, errors.Wrap(err, fmt.Sprintf("could not write %q", noopName)) + } + } + return tempDir, cleanup, nil +} + func fileExists(path string) (bool, error) { _, err := os.Stat(path) if err == nil { @@ -245,7 +391,11 @@ func fileExists(path string) (bool, error) { } func isBeforeMigrationFile(filename string) bool { - return strings.HasSuffix(filename, beforeUpSuffix) || strings.HasSuffix(filename, beforeDownSuffix) + return strings.HasSuffix(filename, beforeUpSuffix) || strings.HasSuffix(filename, beforeDownSuffix) || strings.HasSuffix(filename, beforeSuffix) +} + +func isAfterMigrationFile(filename string) bool { + return strings.HasSuffix(filename, afterUpSuffix) || strings.HasSuffix(filename, afterSuffix) } func findBeforeUpFile(sourceFolder string, migrationVersion uint) (string, error) { @@ -258,7 +408,31 @@ func findBeforeUpFile(sourceFolder string, migrationVersion uint) (string, error continue } name := entry.Name() - if !strings.HasSuffix(name, beforeUpSuffix) { + if !strings.HasSuffix(name, beforeUpSuffix) && !strings.HasSuffix(name, beforeSuffix) { + continue + } + version, ok := parseMigrationVersion(name) + if !ok { + continue + } + if version == migrationVersion { + return name, nil + } + } + return "", nil +} + +func findAfterUpFile(sourceFolder string, migrationVersion uint) (string, error) { + entries, err := os.ReadDir(sourceFolder) + if err != nil { + return "", err + } + for _, entry := range entries { + if entry.IsDir() { + continue + } + name := entry.Name() + if !strings.HasSuffix(name, afterUpSuffix) && !strings.HasSuffix(name, afterSuffix) { continue } version, ok := parseMigrationVersion(name) diff --git a/pkg/migrate/migrate_test.go b/pkg/migrate/migrate_test.go index d7cafe8..114ddbf 100644 --- a/pkg/migrate/migrate_test.go +++ b/pkg/migrate/migrate_test.go @@ -128,6 +128,29 @@ func TestFindBeforeUpFile(t *testing.T) { } } +func TestFindAfterUpFile(t *testing.T) { + dir := t.TempDir() + writeMigrationFile(t, dir, "001_init.after.up.sql") + writeMigrationFile(t, dir, "002_add.after.up.sql") + writeMigrationFile(t, dir, "003_other.after.up.sql") + + found, err := findAfterUpFile(dir, 2) + if err != nil { + t.Fatalf("findAfterUpFile() error = %v", err) + } + if found != "002_add.after.up.sql" { + t.Fatalf("findAfterUpFile() got = %q, want %q", found, "002_add.after.up.sql") + } + + found, err = findAfterUpFile(dir, 4) + if err != nil { + t.Fatalf("findAfterUpFile() error = %v", err) + } + if found != "" { + t.Fatalf("findAfterUpFile() got = %q, want empty", found) + } +} + func TestCreateAfterSourceFolder_excludesBefore(t *testing.T) { dir := t.TempDir() writeMigrationFile(t, dir, "001_init.up.sql") @@ -159,6 +182,61 @@ func TestCreateAfterSourceFolder_excludesBefore(t *testing.T) { } } +func TestCreateAfterSourceFolderForVersion(t *testing.T) { + dir := t.TempDir() + writeMigrationFile(t, dir, "001_init.after.up.sql") + writeMigrationFile(t, dir, "002_add.after.up.sql") + writeMigrationFile(t, dir, "003_other.after.up.sql") + + afterDir, cleanup, err := CreateAfterSourceFolderForVersion(dir, 2) + if err != nil { + t.Fatalf("CreateAfterSourceFolderForVersion() error = %v", err) + } + if cleanup != nil { + defer cleanup() + } + if afterDir == "" { + t.Fatalf("CreateAfterSourceFolderForVersion() returned empty dir") + } + + entries, err := os.ReadDir(afterDir) + if err != nil { + t.Fatalf("ReadDir() error = %v", err) + } + found := map[string]bool{} + for _, entry := range entries { + found[entry.Name()] = true + } + if !found["002_add.up.sql"] { + t.Fatalf("CreateAfterSourceFolderForVersion() should include renamed after file") + } + if found["001_init.up.sql"] || found["003_other.up.sql"] { + t.Fatalf("CreateAfterSourceFolderForVersion() should only include the target version") + } + + afterDir, cleanup, err = CreateAfterSourceFolderForVersion(dir, 4) + if err != nil { + t.Fatalf("CreateAfterSourceFolderForVersion() error = %v", err) + } + if cleanup != nil { + defer cleanup() + } + if afterDir == "" { + t.Fatalf("CreateAfterSourceFolderForVersion() got empty dir") + } + entries, err = os.ReadDir(afterDir) + if err != nil { + t.Fatalf("ReadDir() error = %v", err) + } + found = map[string]bool{} + for _, entry := range entries { + found[entry.Name()] = true + } + if !found["4_noop.up.sql"] { + t.Fatalf("CreateAfterSourceFolderForVersion() should create noop migration") + } +} + var wantParsedFdwUp = `BEGIN; CREATE SERVER IF NOT EXISTS keymgmt_server FOREIGN DATA WRAPPER postgres_fdw OPTIONS (host 'myHostname', dbname 'myDBName', port '42'); From 8231515c2fe94a0c889ecf12a7ceb8a2f7d2f1cc Mon Sep 17 00:00:00 2001 From: Thomas Holst Date: Fri, 6 Feb 2026 10:50:48 +0100 Subject: [PATCH 04/15] Address lint issues --- pkg/db/gorm.go | 110 +++++++++++++++++++++++------------- pkg/migrate/migrate.go | 24 ++++---- pkg/migrate/migrate_test.go | 2 +- 3 files changed, 86 insertions(+), 50 deletions(-) diff --git a/pkg/db/gorm.go b/pkg/db/gorm.go index 2513b0b..fb93304 100644 --- a/pkg/db/gorm.go +++ b/pkg/db/gorm.go @@ -155,17 +155,15 @@ func runMigration(conn *gorm.DB, migFn VersionedMigrationFunc, migrationVersion logging.LogErrorf(ErrDBConnection, "MigrateDB() - db handle is nil") return ErrDBConnection } + if migrationVersion == 0 { + return runMigrationNoSQL(conn, migFn) + } + sqlDB, err := conn.DB() if err != nil { logging.LogErrorf(err, "error getting sql DB") return err } - if migrationVersion == 0 { - if migFn != nil { - return migFn(conn, 0) - } - return nil - } ctx := context.Background() migration := migrate.NewMigration(sqlDB, migrationsSource, migrationsTable, logging.Logger()) @@ -182,59 +180,93 @@ func runMigration(conn *gorm.DB, migFn VersionedMigrationFunc, migrationVersion } }() + return runMigrationVersions(ctx, conn, migration, migFn, migrationVersion, startFromZero) +} + +func runMigrationNoSQL(conn *gorm.DB, migFn VersionedMigrationFunc) error { + if migFn != nil { + return migFn(conn, 0) + } + return nil +} + +func runMigrationVersions( + ctx context.Context, + conn *gorm.DB, + migration *migrate.Migration, + migFn VersionedMigrationFunc, + migrationVersion uint, + startFromZero bool, +) error { mpg, err := migration.MigrateInstance() if err != nil { return err } - currentVersionRaw, dirty, err := mpg.Version() + currentVersion, dirty, err := currentMigrationVersion(mpg, migrationVersion, startFromZero) if err != nil { - if stderrors.Is(err, migrate.ErrNilVersion) { - if startFromZero { - currentVersionRaw = 0 - } else { - // no migration info and startFromZero disabled -> treat as already at target - // nolint: gosec - if err := migrate.SetVersion(mpg, migrationVersion); err != nil { - return err - } - return nil - } - } else { - return err - } + return err } - currentVersion := uint(currentVersionRaw) if dirty { return errors.Errorf("database migration is dirty at version %d", currentVersion) } for version := currentVersion + 1; version <= migrationVersion; version++ { - if _, err := migration.ExecuteBeforeUp(ctx, version); err != nil { - logging.LogErrorf(err, "error running before migration for version %d", version) + if err := applyMigrationVersion(ctx, conn, migration, migFn, mpg, version); err != nil { return err } + logging.LogInfof("migration for version %d executed successfully", version) + } - if migFn != nil { - if err := migFn(conn, version); err != nil { - logging.LogErrorf(err, "error running auto migration for version %d", version) - return err - } - } + return nil +} - if _, err := migration.ExecuteAfterUp(ctx, version); err != nil { - logging.LogErrorf(err, "error running after migration for version %d", version) - return err - } +func currentMigrationVersion(mpg migrate.VersionSetter, migrationVersion uint, startFromZero bool) (uint, bool, error) { + currentVersion, dirty, err := mpg.Version() + if err == nil { + return currentVersion, dirty, nil + } + if !stderrors.Is(err, migrate.ErrNilVersion) { + return 0, false, err + } + if startFromZero { + return 0, false, nil + } + if err := migrate.SetVersion(mpg, migrationVersion); err != nil { + return 0, false, err + } + return migrationVersion, false, nil +} - // Record version after full before/auto/after sequence. - // nolint: gosec - if err := migrate.SetVersion(mpg, version); err != nil { - logging.LogErrorf(err, "error setting migration version to %d", version) +func applyMigrationVersion( + ctx context.Context, + conn *gorm.DB, + migration *migrate.Migration, + migFn VersionedMigrationFunc, + mpg migrate.VersionSetter, + version uint, +) error { + if _, err := migration.ExecuteBeforeUp(ctx, version); err != nil { + logging.LogErrorf(err, "error running before migration for version %d", version) + return err + } + + if migFn != nil { + if err := migFn(conn, version); err != nil { + logging.LogErrorf(err, "error running auto migration for version %d", version) return err } + } - logging.LogInfof("migration for version %d executed successfully", version) + if _, err := migration.ExecuteAfterUp(ctx, version); err != nil { + logging.LogErrorf(err, "error running after migration for version %d", version) + return err + } + + // Record version after full before/auto/after sequence. + if err := migrate.SetVersion(mpg, version); err != nil { + logging.LogErrorf(err, "error setting migration version to %d", version) + return err } return nil diff --git a/pkg/migrate/migrate.go b/pkg/migrate/migrate.go index 7ba60db..4cee634 100644 --- a/pkg/migrate/migrate.go +++ b/pkg/migrate/migrate.go @@ -149,7 +149,7 @@ func (m *Migration) MigrateInstance() (*migrate.Migrate, error) { } // CurrentVersion returns the current migration version. -func (m *Migration) CurrentVersion(ctx context.Context) (uint, bool, error) { +func (m *Migration) CurrentVersion() (uint, bool, error) { mpg, err := m.MigrateInstance() if err != nil { return 0, false, err @@ -159,7 +159,7 @@ func (m *Migration) CurrentVersion(ctx context.Context) (uint, bool, error) { if err != nil { return 0, false, err } - return uint(version), dirty, nil + return version, dirty, nil } // MigrateToVersion runs golang-migrate without setup/fdw scripts. @@ -197,7 +197,7 @@ func (m *Migration) MigrateToVersion(ctx context.Context, migrationVersion uint, } // SetVersion records the current version in the migrations table using an existing migrate instance. -func SetVersion(mpg *migrate.Migrate, migrationVersion uint) error { +func SetVersion(mpg VersionSetter, migrationVersion uint) error { // nolint: gosec if err := mpg.Force(int(migrationVersion)); err != nil { return errors.Wrap(err, "error setting migration version") @@ -205,6 +205,12 @@ func SetVersion(mpg *migrate.Migrate, migrationVersion uint) error { return nil } +// VersionSetter abstracts a migrate instance that can report and set version. +type VersionSetter interface { + Version() (uint, bool, error) + Force(int) error +} + func (m *Migration) parseFile(ctx context.Context, filename string, templateData interface{}) (string, error) { path := m.sourceFolder + "/" + filename @@ -369,7 +375,7 @@ func CreateAfterSourceFolderForVersion(sourceFolder string, migrationVersion uin } if !copied { noopName := fmt.Sprintf("%d_noop.up.sql", migrationVersion) - if err := os.WriteFile(filepath.Join(tempDir, noopName), []byte("SELECT 1;"), 0o644); err != nil { + if err := os.WriteFile(filepath.Join(tempDir, noopName), []byte("SELECT 1;"), 0o600); err != nil { cleanup() return "", nil, errors.Wrap(err, fmt.Sprintf("could not write %q", noopName)) } @@ -391,11 +397,9 @@ func fileExists(path string) (bool, error) { } func isBeforeMigrationFile(filename string) bool { - return strings.HasSuffix(filename, beforeUpSuffix) || strings.HasSuffix(filename, beforeDownSuffix) || strings.HasSuffix(filename, beforeSuffix) -} - -func isAfterMigrationFile(filename string) bool { - return strings.HasSuffix(filename, afterUpSuffix) || strings.HasSuffix(filename, afterSuffix) + return strings.HasSuffix(filename, beforeUpSuffix) || + strings.HasSuffix(filename, beforeDownSuffix) || + strings.HasSuffix(filename, beforeSuffix) } func findBeforeUpFile(sourceFolder string, migrationVersion uint) (string, error) { @@ -468,7 +472,7 @@ func copyFile(src, dest string) error { if err != nil { return errors.Wrap(err, fmt.Sprintf("could not read %q", src)) } - if err := os.WriteFile(dest, data, 0o644); err != nil { + if err := os.WriteFile(dest, data, 0o600); err != nil { return errors.Wrap(err, fmt.Sprintf("could not write %q", dest)) } return nil diff --git a/pkg/migrate/migrate_test.go b/pkg/migrate/migrate_test.go index 114ddbf..a6b695e 100644 --- a/pkg/migrate/migrate_test.go +++ b/pkg/migrate/migrate_test.go @@ -255,7 +255,7 @@ var wantParsedSetup = `CREATE TABLE IF NOT EXISTS test_setup.testtable ( func writeMigrationFile(t *testing.T, dir, name string) { t.Helper() path := filepath.Join(dir, name) - if err := os.WriteFile(path, []byte("SELECT 1;"), 0o644); err != nil { + if err := os.WriteFile(path, []byte("SELECT 1;"), 0o600); err != nil { t.Fatalf("WriteFile(%q) error = %v", path, err) } } From 7c26edc899d4c7ad2a6f365cf56aebd573a32828 Mon Sep 17 00:00:00 2001 From: Thomas Holst Date: Fri, 6 Feb 2026 10:53:37 +0100 Subject: [PATCH 05/15] Move migration documentation to docs --- .../migration-approaches.md | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) rename MIGRATION_APPROACHES.md => docs/migration-approaches.md (69%) diff --git a/MIGRATION_APPROACHES.md b/docs/migration-approaches.md similarity index 69% rename from MIGRATION_APPROACHES.md rename to docs/migration-approaches.md index 0f3023a..50d161d 100644 --- a/MIGRATION_APPROACHES.md +++ b/docs/migration-approaches.md @@ -1,32 +1,32 @@ # Migration Approaches -This repository supports **two migration logics** for services using `pkg/db`. -The *legacy* logic mirrors what is on `main` today. The *versioned* logic is -new and enables interleaving per migration version. +This repository supports **two migration flows** for services using `pkg/db`. +Choose the legacy flow for a single AutoMigrate pass, or the versioned flow if +you need per‑version interleaving. -## 1) Legacy Migration Logic (as on main) +## 1) Legacy Migration Flow Use when you want a single AutoMigrate call and minimal changes to existing services. **API** -- Provide `WithMigrationFunc(func(*gorm.DB) error)` -- Optionally set `WithMigrationVersion(version)` +- `WithMigrationFunc(func(*gorm.DB) error)` +- Optional: `WithMigrationVersion(version)` -**Behavior (main)** +**Behavior** - Runs **one** AutoMigrate (latest models). - Runs `setup.sql` (idempotent), then `fdw.up.sql` (optional). - Runs numbered SQL migrations via `golang-migrate`: - `{version}_{name}.up.sql` / `{version}_{name}.down.sql` - Runs `fdw.down.sql` (optional). -## 2) Versioned Migration Logic (new) +## 2) Versioned Migration Flow Use when you need **interleaving** per migration version. **API** -- Provide `WithVersionedMigrationFunc(func(*gorm.DB, uint) error)` -- Optionally set `WithMigrationVersion(version)` +- `WithVersionedMigrationFunc(func(*gorm.DB, uint) error)` +- Optional: `WithMigrationVersion(version)` **Behavior** - Runs `setup.sql` once (idempotent). @@ -40,16 +40,16 @@ Use when you need **interleaving** per migration version. **Notes** - Missing before/after scripts are skipped. - The version bump represents completion of before + AutoMigrate + after. -- **Supported naming (versioned path only):** +- **Supported naming (versioned flow only):** - Before: `{version}_{name}.before.sql` **or** `{version}_{name}.before.up.sql` - After: `{version}_{name}.after.sql` **or** `{version}_{name}.after.up.sql` ## File Naming Summary -Legacy (main): +Legacy flow: - SQL migrations: `{version}_{name}.up.sql` -Versioned (new): +Versioned flow: - Before: `{version}_{name}.before.sql` or `{version}_{name}.before.up.sql` - After: `{version}_{name}.after.sql` or `{version}_{name}.after.up.sql` From 09b9225fd0e46c855bf68a5177a4e478ac7866e1 Mon Sep 17 00:00:00 2001 From: Thomas Holst Date: Fri, 6 Feb 2026 10:58:54 +0100 Subject: [PATCH 06/15] Embed migration diagrams in docs --- docs/migration-approaches.md | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/docs/migration-approaches.md b/docs/migration-approaches.md index 50d161d..5fb1836 100644 --- a/docs/migration-approaches.md +++ b/docs/migration-approaches.md @@ -20,6 +20,20 @@ services. - `{version}_{name}.up.sql` / `{version}_{name}.down.sql` - Runs `fdw.down.sql` (optional). +**Diagram** +```mermaid +flowchart TB + %% Legacy Migration Logic + + A0["Start migrations"] --> A1["AutoMigrate (once, latest models)"] + A1 --> A2["setup.sql (optional, idempotent)"] + A2 --> A3["fdw.up.sql (optional)"] + A3 --> L1["For each version v = current+1 .. target"] + L1 --> L2["{version}_{name}.up.sql"] + L2 -.-> L1 + L2 --> A4["fdw.down.sql (optional)"] +``` + ## 2) Versioned Migration Flow Use when you need **interleaving** per migration version. @@ -44,6 +58,22 @@ Use when you need **interleaving** per migration version. - Before: `{version}_{name}.before.sql` **or** `{version}_{name}.before.up.sql` - After: `{version}_{name}.after.sql` **or** `{version}_{name}.after.up.sql` +**Diagram** +```mermaid +flowchart TB + %% Versioned Migration Logic + + B0["Start migrations"] --> B1["setup.sql (optional, idempotent)"] + B1 --> B2["fdw.up.sql (optional)"] + B2 --> V1["For each version v = current+1 .. target"] + V1 --> V2["{version}_{name}.before.sql or .before.up.sql (optional)"] + V2 --> V3["AutoMigrate(version)"] + V3 --> V4["{version}_{name}.after.sql or .after.up.sql (optional)"] + V4 --> V5["Record version v"] + V5 -.-> V1 + V5 --> B3["fdw.down.sql (optional)"] +``` + ## File Naming Summary Legacy flow: From 5fff2d5e5eacb6a6b890961495ee8c9e090c2fd7 Mon Sep 17 00:00:00 2001 From: Thomas Holst Date: Fri, 6 Feb 2026 11:02:06 +0100 Subject: [PATCH 07/15] Move migration docs into docs/migration --- README.md | 1 + docs/{migration-approaches.md => migration/README.md} | 0 2 files changed, 1 insertion(+) rename docs/{migration-approaches.md => migration/README.md} (100%) diff --git a/README.md b/README.md index 7af27c7..8199f2c 100644 --- a/README.md +++ b/README.md @@ -16,6 +16,7 @@ Prometheus instrumentation, HTTP client/server utilities, and more. - `pkg/standard`: Opinionated server/gateway wiring - `pkg/ticket`: Lightweight JWT ticket verification/claims - `pkg/transport`: Composable RoundTripper chain (retry, timeout, auth, trace) +- Migration approaches: `docs/migration/README.md` ### Prometheus namespace diff --git a/docs/migration-approaches.md b/docs/migration/README.md similarity index 100% rename from docs/migration-approaches.md rename to docs/migration/README.md From 4cf4ddb3db0814d694f17681a6551bf4623bc18a Mon Sep 17 00:00:00 2001 From: Thomas Holst Date: Fri, 6 Feb 2026 11:02:54 +0100 Subject: [PATCH 08/15] Move migration diagrams into docs/migration --- docs/{ => migration}/migration-legacy-example-v5-to-v7.mmd | 0 docs/{ => migration}/migration-legacy.mmd | 0 docs/{ => migration}/migration-versioned-example-v5-to-v7.mmd | 0 docs/{ => migration}/migration-versioned.mmd | 0 4 files changed, 0 insertions(+), 0 deletions(-) rename docs/{ => migration}/migration-legacy-example-v5-to-v7.mmd (100%) rename docs/{ => migration}/migration-legacy.mmd (100%) rename docs/{ => migration}/migration-versioned-example-v5-to-v7.mmd (100%) rename docs/{ => migration}/migration-versioned.mmd (100%) diff --git a/docs/migration-legacy-example-v5-to-v7.mmd b/docs/migration/migration-legacy-example-v5-to-v7.mmd similarity index 100% rename from docs/migration-legacy-example-v5-to-v7.mmd rename to docs/migration/migration-legacy-example-v5-to-v7.mmd diff --git a/docs/migration-legacy.mmd b/docs/migration/migration-legacy.mmd similarity index 100% rename from docs/migration-legacy.mmd rename to docs/migration/migration-legacy.mmd diff --git a/docs/migration-versioned-example-v5-to-v7.mmd b/docs/migration/migration-versioned-example-v5-to-v7.mmd similarity index 100% rename from docs/migration-versioned-example-v5-to-v7.mmd rename to docs/migration/migration-versioned-example-v5-to-v7.mmd diff --git a/docs/migration-versioned.mmd b/docs/migration/migration-versioned.mmd similarity index 100% rename from docs/migration-versioned.mmd rename to docs/migration/migration-versioned.mmd From 55851b9bc30abab9f652bf158596cc7bdbc3bb0b Mon Sep 17 00:00:00 2001 From: Thomas Holst Date: Fri, 6 Feb 2026 11:05:39 +0100 Subject: [PATCH 09/15] Fix README migration doc link --- README.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/README.md b/README.md index 8199f2c..202b950 100644 --- a/README.md +++ b/README.md @@ -11,12 +11,11 @@ Prometheus instrumentation, HTTP client/server utilities, and more. - `pkg/log`: Structured logging, audit logs, HTTP request/response logging - `pkg/logging`: Global logger facade for convenience - `pkg/middlewares`: Auth, tenant, tracing, URL filter middlewares -- `pkg/migrate`: Migration runner for PostgreSQL (SQL files) +- `pkg/migrate`: Migration runner for PostgreSQL (SQL files). See [Documentation](docs/migration/README.md) - `pkg/prom`: Prometheus metrics utilities for HTTP client/server - `pkg/standard`: Opinionated server/gateway wiring - `pkg/ticket`: Lightweight JWT ticket verification/claims - `pkg/transport`: Composable RoundTripper chain (retry, timeout, auth, trace) -- Migration approaches: `docs/migration/README.md` ### Prometheus namespace From 7100e475ed98c332f5286a26184156f50bf941bc Mon Sep 17 00:00:00 2001 From: Thomas Holst Date: Fri, 6 Feb 2026 11:08:27 +0100 Subject: [PATCH 10/15] Reorder migration docs and clarify version record --- docs/migration/README.md | 76 +++++++++++++++++++++------------------- 1 file changed, 39 insertions(+), 37 deletions(-) diff --git a/docs/migration/README.md b/docs/migration/README.md index 5fb1836..bff0e60 100644 --- a/docs/migration/README.md +++ b/docs/migration/README.md @@ -1,40 +1,10 @@ # Migration Approaches This repository supports **two migration flows** for services using `pkg/db`. -Choose the legacy flow for a single AutoMigrate pass, or the versioned flow if -you need per‑version interleaving. +Choose the versioned flow if you need per‑version interleaving, or the legacy +flow for a single AutoMigrate pass. -## 1) Legacy Migration Flow - -Use when you want a single AutoMigrate call and minimal changes to existing -services. - -**API** -- `WithMigrationFunc(func(*gorm.DB) error)` -- Optional: `WithMigrationVersion(version)` - -**Behavior** -- Runs **one** AutoMigrate (latest models). -- Runs `setup.sql` (idempotent), then `fdw.up.sql` (optional). -- Runs numbered SQL migrations via `golang-migrate`: - - `{version}_{name}.up.sql` / `{version}_{name}.down.sql` -- Runs `fdw.down.sql` (optional). - -**Diagram** -```mermaid -flowchart TB - %% Legacy Migration Logic - - A0["Start migrations"] --> A1["AutoMigrate (once, latest models)"] - A1 --> A2["setup.sql (optional, idempotent)"] - A2 --> A3["fdw.up.sql (optional)"] - A3 --> L1["For each version v = current+1 .. target"] - L1 --> L2["{version}_{name}.up.sql"] - L2 -.-> L1 - L2 --> A4["fdw.down.sql (optional)"] -``` - -## 2) Versioned Migration Flow +## 1) Versioned Migration Flow Use when you need **interleaving** per migration version. @@ -54,6 +24,8 @@ Use when you need **interleaving** per migration version. **Notes** - Missing before/after scripts are skipped. - The version bump represents completion of before + AutoMigrate + after. +- **Recording a version** means updating the `migrations` table used by + `golang-migrate` with the new version number. - **Supported naming (versioned flow only):** - Before: `{version}_{name}.before.sql` **or** `{version}_{name}.before.up.sql` - After: `{version}_{name}.after.sql` **or** `{version}_{name}.after.up.sql` @@ -69,20 +41,50 @@ flowchart TB V1 --> V2["{version}_{name}.before.sql or .before.up.sql (optional)"] V2 --> V3["AutoMigrate(version)"] V3 --> V4["{version}_{name}.after.sql or .after.up.sql (optional)"] - V4 --> V5["Record version v"] + V4 --> V5["Record version v (migrations table)"] V5 -.-> V1 V5 --> B3["fdw.down.sql (optional)"] ``` -## File Naming Summary +## 2) Legacy Migration Flow -Legacy flow: -- SQL migrations: `{version}_{name}.up.sql` +Use when you want a single AutoMigrate call and minimal changes to existing +services. + +**API** +- `WithMigrationFunc(func(*gorm.DB) error)` +- Optional: `WithMigrationVersion(version)` + +**Behavior** +- Runs **one** AutoMigrate (latest models). +- Runs `setup.sql` (idempotent), then `fdw.up.sql` (optional). +- Runs numbered SQL migrations via `golang-migrate`: + - `{version}_{name}.up.sql` / `{version}_{name}.down.sql` +- Runs `fdw.down.sql` (optional). + +**Diagram** +```mermaid +flowchart TB + %% Legacy Migration Logic + + A0["Start migrations"] --> A1["AutoMigrate (once, latest models)"] + A1 --> A2["setup.sql (optional, idempotent)"] + A2 --> A3["fdw.up.sql (optional)"] + A3 --> L1["For each version v = current+1 .. target"] + L1 --> L2["{version}_{name}.up.sql"] + L2 -.-> L1 + L2 --> A4["fdw.down.sql (optional)"] +``` + +## File Naming Summary Versioned flow: - Before: `{version}_{name}.before.sql` or `{version}_{name}.before.up.sql` - After: `{version}_{name}.after.sql` or `{version}_{name}.after.up.sql` +Legacy flow: +- SQL migrations: `{version}_{name}.up.sql` + Shared: - Setup: `setup.sql` - FDW: `fdw.up.sql`, `fdw.down.sql` From 7944b8a92427e4e19eceb645076fd0808718e9a1 Mon Sep 17 00:00:00 2001 From: Thomas Holst Date: Fri, 6 Feb 2026 11:37:00 +0100 Subject: [PATCH 11/15] Add migration flow tests --- pkg/db/migration_version_test.go | 82 +++++++++++++++++ pkg/migrate/migrate_test.go | 18 ++++ test/migration_versioned_test.go | 147 +++++++++++++++++++++++++++++++ 3 files changed, 247 insertions(+) create mode 100644 pkg/db/migration_version_test.go create mode 100644 test/migration_versioned_test.go diff --git a/pkg/db/migration_version_test.go b/pkg/db/migration_version_test.go new file mode 100644 index 0000000..7af16ef --- /dev/null +++ b/pkg/db/migration_version_test.go @@ -0,0 +1,82 @@ +package db + +import ( + "errors" + "testing" + + "github.com/d4l-data4life/go-svc/pkg/migrate" +) + +type fakeVersionSetter struct { + version uint + dirty bool + err error + forced []uint + forceErr error +} + +func (f *fakeVersionSetter) Version() (uint, bool, error) { + return f.version, f.dirty, f.err +} + +func (f *fakeVersionSetter) Force(v int) error { + f.forced = append(f.forced, uint(v)) + return f.forceErr +} + +func TestCurrentMigrationVersion_StartFromZero(t *testing.T) { + setter := &fakeVersionSetter{err: migrate.ErrNilVersion} + version, dirty, err := currentMigrationVersion(setter, 5, true) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if dirty { + t.Fatalf("expected dirty=false") + } + if version != 0 { + t.Fatalf("expected version 0, got %d", version) + } + if len(setter.forced) != 0 { + t.Fatalf("expected no Force calls, got %v", setter.forced) + } +} + +func TestCurrentMigrationVersion_RecordsTarget(t *testing.T) { + setter := &fakeVersionSetter{err: migrate.ErrNilVersion} + version, dirty, err := currentMigrationVersion(setter, 7, false) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if dirty { + t.Fatalf("expected dirty=false") + } + if version != 7 { + t.Fatalf("expected version 7, got %d", version) + } + if len(setter.forced) != 1 || setter.forced[0] != 7 { + t.Fatalf("expected Force(7), got %v", setter.forced) + } +} + +func TestCurrentMigrationVersion_PropagatesDirty(t *testing.T) { + setter := &fakeVersionSetter{version: 3, dirty: true} + version, dirty, err := currentMigrationVersion(setter, 7, false) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !dirty { + t.Fatalf("expected dirty=true") + } + if version != 3 { + t.Fatalf("expected version 3, got %d", version) + } +} + +func TestCurrentMigrationVersion_PropagatesError(t *testing.T) { + boom := errors.New("boom") + setter := &fakeVersionSetter{err: boom} + _, _, err := currentMigrationVersion(setter, 7, false) + if !errors.Is(err, boom) { + t.Fatalf("expected error %v, got %v", boom, err) + } +} diff --git a/pkg/migrate/migrate_test.go b/pkg/migrate/migrate_test.go index a6b695e..460fc8f 100644 --- a/pkg/migrate/migrate_test.go +++ b/pkg/migrate/migrate_test.go @@ -110,6 +110,7 @@ func TestFindBeforeUpFile(t *testing.T) { writeMigrationFile(t, dir, "002_add.before.up.sql") writeMigrationFile(t, dir, "002_add.before.down.sql") writeMigrationFile(t, dir, "003_other.before.up.sql") + writeMigrationFile(t, dir, "004_new.before.sql") found, err := findBeforeUpFile(dir, 2) if err != nil { @@ -126,6 +127,14 @@ func TestFindBeforeUpFile(t *testing.T) { if found != "" { t.Fatalf("findBeforeUpFile() got = %q, want empty", found) } + + found, err = findBeforeUpFile(dir, 4) + if err != nil { + t.Fatalf("findBeforeUpFile() error = %v", err) + } + if found != "004_new.before.sql" { + t.Fatalf("findBeforeUpFile() got = %q, want %q", found, "004_new.before.sql") + } } func TestFindAfterUpFile(t *testing.T) { @@ -133,6 +142,7 @@ func TestFindAfterUpFile(t *testing.T) { writeMigrationFile(t, dir, "001_init.after.up.sql") writeMigrationFile(t, dir, "002_add.after.up.sql") writeMigrationFile(t, dir, "003_other.after.up.sql") + writeMigrationFile(t, dir, "004_new.after.sql") found, err := findAfterUpFile(dir, 2) if err != nil { @@ -149,6 +159,14 @@ func TestFindAfterUpFile(t *testing.T) { if found != "" { t.Fatalf("findAfterUpFile() got = %q, want empty", found) } + + found, err = findAfterUpFile(dir, 4) + if err != nil { + t.Fatalf("findAfterUpFile() error = %v", err) + } + if found != "004_new.after.sql" { + t.Fatalf("findAfterUpFile() got = %q, want %q", found, "004_new.after.sql") + } } func TestCreateAfterSourceFolder_excludesBefore(t *testing.T) { diff --git a/test/migration_versioned_test.go b/test/migration_versioned_test.go new file mode 100644 index 0000000..a1cc68e --- /dev/null +++ b/test/migration_versioned_test.go @@ -0,0 +1,147 @@ +package test + +import ( + "context" + "fmt" + "os" + "path/filepath" + "strconv" + "testing" + + "github.com/d4l-data4life/go-svc/pkg/db" + "github.com/d4l-data4life/go-svc/pkg/migrate" + "gorm.io/gorm" +) + +func TestVersionedMigrationFlow(t *testing.T) { + cfg, err := parseEnv() + if err != nil { + t.Fatal(err) + } + + sqlDB, err := connectToDB(cfg) + if err != nil { + t.Fatal(err) + } + defer sqlDB.Close() + + ctx := context.Background() + _ = cleanTable(ctx, sqlDB, "migration_steps") + _ = cleanTable(ctx, sqlDB, "migrations") + defer func() { + _ = cleanTable(ctx, sqlDB, "migration_steps") + _ = cleanTable(ctx, sqlDB, "migrations") + }() + + tmpDir := t.TempDir() + sqlDir := filepath.Join(tmpDir, "sql") + if err := os.MkdirAll(sqlDir, 0o755); err != nil { + t.Fatal(err) + } + + writeSQL(t, sqlDir, "001_init.before.sql", ` +CREATE TABLE IF NOT EXISTS migration_steps ( + seq SERIAL PRIMARY KEY, + step TEXT NOT NULL +); +INSERT INTO migration_steps (step) VALUES ('before-1'); +`) + writeSQL(t, sqlDir, "001_init.after.sql", ` +INSERT INTO migration_steps (step) VALUES ('after-1'); +`) + writeSQL(t, sqlDir, "002_add.before.sql", ` +INSERT INTO migration_steps (step) VALUES ('before-2'); +`) + writeSQL(t, sqlDir, "002_add.after.sql", ` +INSERT INTO migration_steps (step) VALUES ('after-2'); +`) + writeSQL(t, sqlDir, "003_more.before.up.sql", ` +INSERT INTO migration_steps (step) VALUES ('before-3'); +`) + writeSQL(t, sqlDir, "003_more.after.up.sql", ` +INSERT INTO migration_steps (step) VALUES ('after-3'); +`) + + cwd, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + if err := os.Chdir(tmpDir); err != nil { + t.Fatal(err) + } + defer func() { + _ = os.Chdir(cwd) + }() + + migFn := func(conn *gorm.DB, version uint) error { + return conn.Exec(fmt.Sprintf("INSERT INTO migration_steps (step) VALUES ('auto-%d')", version)).Error + } + + opts := db.NewConnection( + db.WithHost(cfg.PGHost), + db.WithPort(strconv.Itoa(int(cfg.PGPort))), + db.WithDatabaseName(cfg.PGName), + db.WithUser(cfg.PGUser), + db.WithPassword(cfg.PGPassword), + db.WithSSLMode("disable"), + db.WithMigrationStartFromZero(true), + db.WithMigrationVersion(4), + db.WithVersionedMigrationFunc(migFn), + ) + + db.InitializeTestPostgres(opts) + conn := db.Get() + if conn == nil { + t.Fatal("db handle is nil") + } + + type row struct { + Seq int + Step string + } + rows := []row{} + if err := conn.Raw("SELECT seq, step FROM migration_steps ORDER BY seq").Scan(&rows).Error; err != nil { + t.Fatalf("query steps: %v", err) + } + + want := []string{ + "before-1", + "auto-1", + "after-1", + "before-2", + "auto-2", + "after-2", + "before-3", + "auto-3", + "after-3", + "auto-4", + } + if len(rows) != len(want) { + t.Fatalf("got %d steps, want %d", len(rows), len(want)) + } + for i, w := range want { + if rows[i].Step != w { + t.Fatalf("step %d: got %q, want %q", i, rows[i].Step, w) + } + } + + migration := migrate.NewMigration(sqlDB, sqlDir, "migrations", &testLog{}) + version, dirty, err := migration.CurrentVersion() + if err != nil { + t.Fatalf("current version: %v", err) + } + if dirty { + t.Fatalf("expected clean migrations table") + } + if version != 4 { + t.Fatalf("expected version 4, got %d", version) + } +} + +func writeSQL(t *testing.T, dir, name, content string) { + t.Helper() + path := filepath.Join(dir, name) + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatalf("WriteFile(%q) error = %v", path, err) + } +} From 4d8c4fe28db15b8088bf84d7e953da06d4e57e0e Mon Sep 17 00:00:00 2001 From: Thomas Holst Date: Fri, 6 Feb 2026 11:42:11 +0100 Subject: [PATCH 12/15] Stabilize migration tests and lint --- pkg/db/gorm.go | 5 ++- pkg/log/audit.go | 1 + pkg/logging/logger.go | 8 +++- pkg/migrate/migrate.go | 66 ++++++++++++++++++++++++++++++++ pkg/migrate/migrate_test.go | 4 +- test/migration_versioned_test.go | 9 ++++- 6 files changed, 88 insertions(+), 5 deletions(-) diff --git a/pkg/db/gorm.go b/pkg/db/gorm.go index fb93304..b7576b6 100644 --- a/pkg/db/gorm.go +++ b/pkg/db/gorm.go @@ -198,10 +198,13 @@ func runMigrationVersions( migrationVersion uint, startFromZero bool, ) error { - mpg, err := migration.MigrateInstance() + mpg, cleanup, err := migration.MigrateInstanceForVersionTracking() if err != nil { return err } + if cleanup != nil { + defer cleanup() + } currentVersion, dirty, err := currentMigrationVersion(mpg, migrationVersion, startFromZero) if err != nil { diff --git a/pkg/log/audit.go b/pkg/log/audit.go index fbe6fab..14e6275 100644 --- a/pkg/log/audit.go +++ b/pkg/log/audit.go @@ -68,6 +68,7 @@ func (l *Logger) createBaseAuditLog(ctx context.Context, logType AuditLogType) b // The expected context keys are "trace-id" and "user-id". // This is the log type to use when a message should be accompanied // with an object relevant for auditing, e.g., new set of permissions. +// // Deprecated: use AuditSecurity instead. func (l *Logger) Audit( ctx context.Context, diff --git a/pkg/logging/logger.go b/pkg/logging/logger.go index 4cac7f7..4a1758f 100644 --- a/pkg/logging/logger.go +++ b/pkg/logging/logger.go @@ -174,7 +174,13 @@ func LogWarningfCtx(ctx context.Context, err error, format string, fields ...int // LogAudit logs a generic audit event containing of a message along with an object pertaining to the message. func LogAudit(ctx context.Context, message string, object any) { - if err := Logger().Audit(ctx, message, object); err != nil { + if err := Logger().AuditSecurity( + ctx, + "audit", + true, + golog.Message(message), + golog.AdditionalData(object), + ); err != nil { fmt.Printf("Logging error (LogAudit): %s\n", err.Error()) } } diff --git a/pkg/migrate/migrate.go b/pkg/migrate/migrate.go index 4cee634..d6b72ee 100644 --- a/pkg/migrate/migrate.go +++ b/pkg/migrate/migrate.go @@ -148,6 +148,35 @@ func (m *Migration) MigrateInstance() (*migrate.Migrate, error) { return mpg, nil } +// MigrateInstanceForVersionTracking creates a migrate instance that excludes before/after scripts. +func (m *Migration) MigrateInstanceForVersionTracking() (*migrate.Migrate, func(), error) { + sourceFolder, cleanup, err := CreateVersionSourceFolder(m.sourceFolder) + if err != nil { + return nil, nil, err + } + driver, err := postgres.WithInstance(m.db, &postgres.Config{ + MigrationsTable: m.migrationTable, + }) + if err != nil { + if cleanup != nil { + cleanup() + } + return nil, nil, errors.Wrap(err, "error creating database driver") + } + mpg, err := migrate.NewWithDatabaseInstance( + "file://"+sourceFolder, + "postgres", + driver, + ) + if err != nil { + if cleanup != nil { + cleanup() + } + return nil, nil, errors.Wrap(err, "error creating migrate instance") + } + return mpg, cleanup, nil +} + // CurrentVersion returns the current migration version. func (m *Migration) CurrentVersion() (uint, bool, error) { mpg, err := m.MigrateInstance() @@ -340,6 +369,39 @@ func CreateAfterSourceFolder(sourceFolder string) (string, func(), error) { return tempDir, cleanup, nil } +// CreateVersionSourceFolder returns a temp folder with only tracked migration files. +// It excludes before/after scripts to avoid duplicate version conflicts. +func CreateVersionSourceFolder(sourceFolder string) (string, func(), error) { + entries, err := os.ReadDir(sourceFolder) + if err != nil { + return "", nil, errors.Wrap(err, "could not read migrations folder") + } + tempDir, err := os.MkdirTemp("", "migrate-version-*") + if err != nil { + return "", nil, errors.Wrap(err, "could not create temp folder") + } + cleanup := func() { + _ = os.RemoveAll(tempDir) + } + for _, entry := range entries { + if entry.IsDir() { + continue + } + name := entry.Name() + if isBeforeMigrationFile(name) || isAfterMigrationFile(name) { + continue + } + if !strings.HasSuffix(name, ".up.sql") && !strings.HasSuffix(name, ".down.sql") { + continue + } + if err := copyFile(filepath.Join(sourceFolder, name), filepath.Join(tempDir, name)); err != nil { + cleanup() + return "", nil, err + } + } + return tempDir, cleanup, nil +} + // CreateAfterSourceFolderForVersion returns a temp folder with the after migration for a single version. func CreateAfterSourceFolderForVersion(sourceFolder string, migrationVersion uint) (string, func(), error) { entries, err := os.ReadDir(sourceFolder) @@ -402,6 +464,10 @@ func isBeforeMigrationFile(filename string) bool { strings.HasSuffix(filename, beforeSuffix) } +func isAfterMigrationFile(filename string) bool { + return strings.HasSuffix(filename, afterUpSuffix) || strings.HasSuffix(filename, afterSuffix) +} + func findBeforeUpFile(sourceFolder string, migrationVersion uint) (string, error) { entries, err := os.ReadDir(sourceFolder) if err != nil { diff --git a/pkg/migrate/migrate_test.go b/pkg/migrate/migrate_test.go index 460fc8f..6f24dea 100644 --- a/pkg/migrate/migrate_test.go +++ b/pkg/migrate/migrate_test.go @@ -120,7 +120,7 @@ func TestFindBeforeUpFile(t *testing.T) { t.Fatalf("findBeforeUpFile() got = %q, want %q", found, "002_add.before.up.sql") } - found, err = findBeforeUpFile(dir, 4) + found, err = findBeforeUpFile(dir, 5) if err != nil { t.Fatalf("findBeforeUpFile() error = %v", err) } @@ -152,7 +152,7 @@ func TestFindAfterUpFile(t *testing.T) { t.Fatalf("findAfterUpFile() got = %q, want %q", found, "002_add.after.up.sql") } - found, err = findAfterUpFile(dir, 4) + found, err = findAfterUpFile(dir, 5) if err != nil { t.Fatalf("findAfterUpFile() error = %v", err) } diff --git a/test/migration_versioned_test.go b/test/migration_versioned_test.go index a1cc68e..b9cce56 100644 --- a/test/migration_versioned_test.go +++ b/test/migration_versioned_test.go @@ -126,7 +126,14 @@ INSERT INTO migration_steps (step) VALUES ('after-3'); } migration := migrate.NewMigration(sqlDB, sqlDir, "migrations", &testLog{}) - version, dirty, err := migration.CurrentVersion() + mpg, cleanup, err := migration.MigrateInstanceForVersionTracking() + if err != nil { + t.Fatalf("current version: %v", err) + } + if cleanup != nil { + defer cleanup() + } + version, dirty, err := mpg.Version() if err != nil { t.Fatalf("current version: %v", err) } From 4532724abe1bc41b2a0146a841566be3130e2141 Mon Sep 17 00:00:00 2001 From: Thomas Holst Date: Fri, 6 Feb 2026 13:08:07 +0100 Subject: [PATCH 13/15] Fix gosec integer conversion warnings --- pkg/db/migration_version_test.go | 4 ++-- test/migration_versioned_test.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/db/migration_version_test.go b/pkg/db/migration_version_test.go index 7af16ef..87b17ec 100644 --- a/pkg/db/migration_version_test.go +++ b/pkg/db/migration_version_test.go @@ -11,7 +11,7 @@ type fakeVersionSetter struct { version uint dirty bool err error - forced []uint + forced []int forceErr error } @@ -20,7 +20,7 @@ func (f *fakeVersionSetter) Version() (uint, bool, error) { } func (f *fakeVersionSetter) Force(v int) error { - f.forced = append(f.forced, uint(v)) + f.forced = append(f.forced, v) return f.forceErr } diff --git a/test/migration_versioned_test.go b/test/migration_versioned_test.go index b9cce56..4888e20 100644 --- a/test/migration_versioned_test.go +++ b/test/migration_versioned_test.go @@ -79,7 +79,7 @@ INSERT INTO migration_steps (step) VALUES ('after-3'); opts := db.NewConnection( db.WithHost(cfg.PGHost), - db.WithPort(strconv.Itoa(int(cfg.PGPort))), + db.WithPort(strconv.FormatUint(uint64(cfg.PGPort), 10)), db.WithDatabaseName(cfg.PGName), db.WithUser(cfg.PGUser), db.WithPassword(cfg.PGPassword), From 7ccb9b5008fad90e352e4a74a8a86b8d936a220c Mon Sep 17 00:00:00 2001 From: Thomas Holst Date: Fri, 6 Feb 2026 13:21:47 +0100 Subject: [PATCH 14/15] Restore migrate README details --- pkg/migrate/README.md | 52 ++++++++++++++++++++++++++++++++++--------- 1 file changed, 42 insertions(+), 10 deletions(-) diff --git a/pkg/migrate/README.md b/pkg/migrate/README.md index a60a577..eab13b6 100644 --- a/pkg/migrate/README.md +++ b/pkg/migrate/README.md @@ -1,23 +1,55 @@ # go-pg-migrate -Library for migrating Postgres databases in services using `pkg/db`. It is -built on top of `golang-migrate` and supports **two migration logics** used by -`go-svc`. +Library for migrating Postgres databases in services using `pkg/db`. It is built +on top of [golang-migrate V4](https://github.com/golang-migrate/migrate). -## Shared Scripts +## Setup Script -These scripts are optional and run regardless of which migration logic is used. +`go-pg-migrate` allows running a setup script before the migration steps handled +by `golang-migrate`. The script is optional and must be called `setup.sql` and +placed in the same folder as the other SQL scripts. -- `setup.sql` (idempotent, runs once) -- `fdw.up.sql` and `fdw.down.sql` (optional, runs once around all versions) +The main use case is creating a schema that will be used by `golang-migrate` for +the migration table itself. The setup script must be idempotent, as it will be +run for every migration invocation (unlike migration steps which are skipped if +the version is already present). -## Legacy Logic (as on main) +## Postgres Foreign Data Wrapper (FDW) + +`go-pg-migrate` can run additional scripts before and after the migration which +are templated by a `ForeignDatabase` struct: + +- LocalUser string +- DBName string +- Hostname string +- Port uint +- User string +- Password string + +The scripts are optional and must be called `fdw.up.sql` and `fdw.down.sql`, and +placed in the same folder as the other SQL scripts. Placeholders can be used via +`{{.LocalUser}}` syntax. The main use case is preparing the database for foreign +data migration (see Postgres FDW docs). + +## Migration Table + +`golang-migrate` uses a table that contains migration metadata (current version +and dirty status). The table is created with the given name. For Postgres, the +schema is not configurable as of v4, so the table is created in the current +schema (first element in the search path). If the table must live in a specific +schema, that schema must be in the search path. + +## Migration Flows + +`pkg/db` supports two flows: legacy and versioned. + +### Legacy Flow - Single AutoMigrate (latest models). - SQL migrations executed via `golang-migrate`: - `{version}_{name}.up.sql` / `{version}_{name}.down.sql` -## Versioned Logic (new) +### Versioned Flow - Interleaves per migration version: 1. Versioned before script (optional) @@ -27,6 +59,6 @@ These scripts are optional and run regardless of which migration logic is used. Missing before/after scripts are skipped. -**Supported naming (versioned path only):** +**Supported naming (versioned flow only):** - Before: `{version}_{name}.before.sql` or `{version}_{name}.before.up.sql` - After: `{version}_{name}.after.sql` or `{version}_{name}.after.up.sql` From 3be72645347834295e8d78bfabc254eff9044911 Mon Sep 17 00:00:00 2001 From: Thomas Holst Date: Sun, 15 Feb 2026 14:26:12 +0100 Subject: [PATCH 15/15] Fix migration flow compatibility - Restore legacy MigrationFunc + golang-migrate path alongside versioned flow - Guard against configuring both legacy and versioned functions - Tighten before/after script discovery (conflicts error) - Lint/test fixes --- pkg/db/gorm.go | 92 ++++++++++++---- pkg/db/gorm_migration_guard_test.go | 17 +++ pkg/db/migration_version_test.go | 26 +++-- pkg/db/options.go | 7 -- pkg/db/options_test.go | 22 ++-- pkg/db/testing.go | 4 +- pkg/migrate/migrate.go | 158 ++++++++++++++++++---------- pkg/migrate/migrate_test.go | 46 ++++++++ 8 files changed, 264 insertions(+), 108 deletions(-) create mode 100644 pkg/db/gorm_migration_guard_test.go diff --git a/pkg/db/gorm.go b/pkg/db/gorm.go index b7576b6..9135ef5 100644 --- a/pkg/db/gorm.go +++ b/pkg/db/gorm.go @@ -2,6 +2,7 @@ package db import ( "context" + "database/sql" stderrors "errors" "time" @@ -60,7 +61,7 @@ func Initialize(runCtx context.Context, opts *ConnectionOptions) <-chan struct{} defer logging.LogInfof("database connection closed") }() - err = runMigration(conn, opts.VersionedMigrationFunc, opts.MigrationVersion, opts.MigrationStartFromZero) + err = runMigration(conn, opts.MigrationFunc, opts.VersionedMigrationFunc, opts.MigrationVersion, opts.MigrationStartFromZero) if err != nil { if opts.MigrationHaltOnError { logging.LogErrorf(err, "database migration failed - aborting") @@ -150,15 +151,61 @@ func retryExponential(runCtx context.Context, attempts uint, waitPeriod time.Dur } // runMigration Executes Migrations on the database -func runMigration(conn *gorm.DB, migFn VersionedMigrationFunc, migrationVersion uint, startFromZero bool) error { +func runMigration( + conn *gorm.DB, + legacyFn MigrationFunc, + versionedFn VersionedMigrationFunc, + migrationVersion uint, + startFromZero bool, +) error { if conn == nil { logging.LogErrorf(ErrDBConnection, "MigrateDB() - db handle is nil") return ErrDBConnection } + if legacyFn != nil && versionedFn != nil { + return errors.New("both MigrationFunc (legacy) and VersionedMigrationFunc are set; please configure only one migration flow") + } + if migrationVersion == 0 { + // No SQL migrations; run whichever automigration function is provided. + if versionedFn != nil { + return versionedFn(conn, 0) + } + if legacyFn != nil { + return legacyFn(conn) + } + return nil + } + + // Prefer explicit versioned flow when configured. + if versionedFn != nil { + return runMigrationVersioned(conn, versionedFn, migrationVersion, startFromZero) + } + + sqlDB, err := conn.DB() + if err != nil { + logging.LogErrorf(err, "error getting sql DB") + return err + } + + return runMigrationLegacy(sqlDB, conn, legacyFn, migrationVersion, startFromZero) +} + +func runMigrationLegacy(sqlDB *sql.DB, conn *gorm.DB, legacyFn MigrationFunc, migrationVersion uint, startFromZero bool) error { + // Preserve legacy behavior: AutoMigrate once, then SQL migrations via golang-migrate. + if legacyFn != nil { + if err := legacyFn(conn); err != nil { + return err + } + } + if migrationVersion == 0 { - return runMigrationNoSQL(conn, migFn) + return nil } + migration := migrate.NewMigration(sqlDB, migrationsSource, migrationsTable, logging.Logger()) + return migration.MigrateDB(context.Background(), migrationVersion, startFromZero) +} +func runMigrationVersioned(conn *gorm.DB, migFn VersionedMigrationFunc, migrationVersion uint, startFromZero bool) error { sqlDB, err := conn.DB() if err != nil { logging.LogErrorf(err, "error getting sql DB") @@ -183,13 +230,6 @@ func runMigration(conn *gorm.DB, migFn VersionedMigrationFunc, migrationVersion return runMigrationVersions(ctx, conn, migration, migFn, migrationVersion, startFromZero) } -func runMigrationNoSQL(conn *gorm.DB, migFn VersionedMigrationFunc) error { - if migFn != nil { - return migFn(conn, 0) - } - return nil -} - func runMigrationVersions( ctx context.Context, conn *gorm.DB, @@ -206,7 +246,7 @@ func runMigrationVersions( defer cleanup() } - currentVersion, dirty, err := currentMigrationVersion(mpg, migrationVersion, startFromZero) + currentVersion, dirty, needsRecordTarget, err := currentMigrationVersion(mpg, migrationVersion, startFromZero) if err != nil { return err } @@ -214,6 +254,22 @@ func runMigrationVersions( return errors.Errorf("database migration is dirty at version %d", currentVersion) } + // Legacy behavior: when the database has no version info and startFromZero is false, + // run AutoMigrate once, then record the target version without running per-version hooks. + if needsRecordTarget { + if migFn != nil { + if err := migFn(conn, migrationVersion); err != nil { + logging.LogErrorf(err, "error running auto migration for version %d", migrationVersion) + return err + } + } + if err := migrate.SetVersion(mpg, migrationVersion); err != nil { + logging.LogErrorf(err, "error setting migration version to %d", migrationVersion) + return err + } + return nil + } + for version := currentVersion + 1; version <= migrationVersion; version++ { if err := applyMigrationVersion(ctx, conn, migration, migFn, mpg, version); err != nil { return err @@ -224,21 +280,19 @@ func runMigrationVersions( return nil } -func currentMigrationVersion(mpg migrate.VersionSetter, migrationVersion uint, startFromZero bool) (uint, bool, error) { +func currentMigrationVersion(mpg migrate.VersionSetter, migrationVersion uint, startFromZero bool) (uint, bool, bool, error) { currentVersion, dirty, err := mpg.Version() if err == nil { - return currentVersion, dirty, nil + return currentVersion, dirty, false, nil } if !stderrors.Is(err, migrate.ErrNilVersion) { - return 0, false, err + return 0, false, false, err } if startFromZero { - return 0, false, nil - } - if err := migrate.SetVersion(mpg, migrationVersion); err != nil { - return 0, false, err + return 0, false, false, nil } - return migrationVersion, false, nil + // Caller should run a single AutoMigrate and then record the target version. + return migrationVersion, false, true, nil } func applyMigrationVersion( diff --git a/pkg/db/gorm_migration_guard_test.go b/pkg/db/gorm_migration_guard_test.go new file mode 100644 index 0000000..c7e8b52 --- /dev/null +++ b/pkg/db/gorm_migration_guard_test.go @@ -0,0 +1,17 @@ +package db + +import ( + "testing" + + "gorm.io/gorm" +) + +func TestRunMigrationRejectsBothLegacyAndVersionedFuncs(t *testing.T) { + legacyFn := func(_ *gorm.DB) error { return nil } + versionedFn := func(_ *gorm.DB, _ uint) error { return nil } + + err := runMigration(&gorm.DB{}, legacyFn, versionedFn, 1, true) + if err == nil { + t.Fatalf("expected error") + } +} diff --git a/pkg/db/migration_version_test.go b/pkg/db/migration_version_test.go index 87b17ec..ba85baf 100644 --- a/pkg/db/migration_version_test.go +++ b/pkg/db/migration_version_test.go @@ -26,13 +26,16 @@ func (f *fakeVersionSetter) Force(v int) error { func TestCurrentMigrationVersion_StartFromZero(t *testing.T) { setter := &fakeVersionSetter{err: migrate.ErrNilVersion} - version, dirty, err := currentMigrationVersion(setter, 5, true) + version, dirty, needsRecordTarget, err := currentMigrationVersion(setter, 5, true) if err != nil { t.Fatalf("unexpected error: %v", err) } if dirty { t.Fatalf("expected dirty=false") } + if needsRecordTarget { + t.Fatalf("expected needsRecordTarget=false") + } if version != 0 { t.Fatalf("expected version 0, got %d", version) } @@ -41,32 +44,38 @@ func TestCurrentMigrationVersion_StartFromZero(t *testing.T) { } } -func TestCurrentMigrationVersion_RecordsTarget(t *testing.T) { +func TestCurrentMigrationVersion_NeedsRecordTarget(t *testing.T) { setter := &fakeVersionSetter{err: migrate.ErrNilVersion} - version, dirty, err := currentMigrationVersion(setter, 7, false) + version, dirty, needsRecordTarget, err := currentMigrationVersion(setter, 7, false) if err != nil { t.Fatalf("unexpected error: %v", err) } if dirty { t.Fatalf("expected dirty=false") } + if !needsRecordTarget { + t.Fatalf("expected needsRecordTarget=true") + } if version != 7 { t.Fatalf("expected version 7, got %d", version) } - if len(setter.forced) != 1 || setter.forced[0] != 7 { - t.Fatalf("expected Force(7), got %v", setter.forced) + if len(setter.forced) != 0 { + t.Fatalf("expected no Force calls, got %v", setter.forced) } } func TestCurrentMigrationVersion_PropagatesDirty(t *testing.T) { setter := &fakeVersionSetter{version: 3, dirty: true} - version, dirty, err := currentMigrationVersion(setter, 7, false) + version, dirty, needsRecordTarget, err := currentMigrationVersion(setter, 7, false) if err != nil { t.Fatalf("unexpected error: %v", err) } if !dirty { t.Fatalf("expected dirty=true") } + if needsRecordTarget { + t.Fatalf("expected needsRecordTarget=false") + } if version != 3 { t.Fatalf("expected version 3, got %d", version) } @@ -75,7 +84,10 @@ func TestCurrentMigrationVersion_PropagatesDirty(t *testing.T) { func TestCurrentMigrationVersion_PropagatesError(t *testing.T) { boom := errors.New("boom") setter := &fakeVersionSetter{err: boom} - _, _, err := currentMigrationVersion(setter, 7, false) + version, dirty, needsRecordTarget, err := currentMigrationVersion(setter, 7, false) + _ = version + _ = dirty + _ = needsRecordTarget if !errors.Is(err, boom) { t.Fatalf("expected error %v, got %v", boom, err) } diff --git a/pkg/db/options.go b/pkg/db/options.go index 0d5b718..cdf6af9 100644 --- a/pkg/db/options.go +++ b/pkg/db/options.go @@ -146,13 +146,6 @@ func WithSSLRootCertPath(value string) ConnectionOption { func WithMigrationFunc(fn MigrationFunc) ConnectionOption { return func(c *ConnectionOptions) { c.MigrationFunc = fn - // Backward compatibility: wrap legacy migration into versioned flow. - c.VersionedMigrationFunc = func(do *gorm.DB, version uint) error { - if version == c.MigrationVersion { - return fn(do) - } - return nil - } } } diff --git a/pkg/db/options_test.go b/pkg/db/options_test.go index e93ad4b..adbd671 100644 --- a/pkg/db/options_test.go +++ b/pkg/db/options_test.go @@ -6,28 +6,18 @@ import ( "gorm.io/gorm" ) -func TestWithMigrationFuncWrapsVersioned(t *testing.T) { - calls := 0 - fn := func(_ *gorm.DB) error { - calls++ - return nil - } +func TestWithMigrationFuncDoesNotSetVersionedMigrationFunc(t *testing.T) { + fn := func(_ *gorm.DB) error { return nil } opts := NewConnection( WithMigrationVersion(2), WithMigrationFunc(fn), ) - if opts.VersionedMigrationFunc == nil { - t.Fatalf("expected VersionedMigrationFunc to be set") - } - _ = opts.VersionedMigrationFunc(nil, 1) - if calls != 0 { - t.Fatalf("expected no calls for version 1, got %d", calls) + if opts.MigrationFunc == nil { + t.Fatalf("expected MigrationFunc to be set") } - - _ = opts.VersionedMigrationFunc(nil, 2) - if calls != 1 { - t.Fatalf("expected one call for version 2, got %d", calls) + if opts.VersionedMigrationFunc != nil { + t.Fatalf("expected VersionedMigrationFunc to be nil when only WithMigrationFunc is used") } } diff --git a/pkg/db/testing.go b/pkg/db/testing.go index 750a02a..4f3cb97 100644 --- a/pkg/db/testing.go +++ b/pkg/db/testing.go @@ -38,8 +38,8 @@ func InitializeTestPostgres(opts *ConnectionOptions) { logging.LogErrorf(err, "error connecting to testing postgres") db = nil } - if opts.VersionedMigrationFunc != nil { - if err = runMigration(conn, opts.VersionedMigrationFunc, opts.MigrationVersion, true); err != nil { + if opts.MigrationFunc != nil || opts.VersionedMigrationFunc != nil || opts.MigrationVersion > 0 { + if err = runMigration(conn, opts.MigrationFunc, opts.VersionedMigrationFunc, opts.MigrationVersion, true); err != nil { logging.LogErrorf(err, "test DB migration error") } } diff --git a/pkg/migrate/migrate.go b/pkg/migrate/migrate.go index d6b72ee..8838b59 100644 --- a/pkg/migrate/migrate.go +++ b/pkg/migrate/migrate.go @@ -3,6 +3,7 @@ package migrate import ( "context" "database/sql" + stderrors "errors" "fmt" "os" "path/filepath" @@ -199,7 +200,7 @@ func (m *Migration) MigrateToVersion(ctx context.Context, migrationVersion uint, } _, _, err = mpg.Version() - if err == migrate.ErrNilVersion && !startFromZero { + if stderrors.Is(err, migrate.ErrNilVersion) && !startFromZero { // no migration information in the database, so it's a fresh database // and the data model is already the latest one set up Gorm automigrations // nolint: gosec @@ -291,23 +292,6 @@ func (m *Migration) execute(ctx context.Context, filename string, templateData i return err } -// ExecuteTargetBeforeUp runs a target-version before migration if present. -// The script is expected to be idempotent because it is not tracked. -func (m *Migration) ExecuteTargetBeforeUp(ctx context.Context, migrationVersion uint) (bool, error) { - filename, err := findBeforeUpFile(m.sourceFolder, migrationVersion) - if err != nil { - return false, errors.Wrap(err, "could not scan for before migration") - } - if filename == "" { - _ = m.log.InfoGeneric(ctx, fmt.Sprintf("no before migration found for version %d - skipped", migrationVersion)) - return false, nil - } - if err := m.execute(ctx, filename, nil); err != nil { - return false, errors.Wrap(err, fmt.Sprintf("could not run before migration %q", filename)) - } - return true, nil -} - // ExecuteBeforeUp runs a versioned before migration if present. func (m *Migration) ExecuteBeforeUp(ctx context.Context, migrationVersion uint) (bool, error) { filename, err := findBeforeUpFile(m.sourceFolder, migrationVersion) @@ -415,34 +399,53 @@ func CreateAfterSourceFolderForVersion(sourceFolder string, migrationVersion uin cleanup := func() { _ = os.RemoveAll(tempDir) } + copied, err := copyAfterMigrationForVersion(entries, sourceFolder, tempDir, migrationVersion) + if err != nil { + cleanup() + return "", nil, err + } + if !copied { + noopName := fmt.Sprintf("%d_noop.up.sql", migrationVersion) + if err := os.WriteFile(filepath.Join(tempDir, noopName), []byte("SELECT 1;"), 0o600); err != nil { + cleanup() + return "", nil, errors.Wrap(err, fmt.Sprintf("could not write %q", noopName)) + } + } + return tempDir, cleanup, nil +} + +func copyAfterMigrationForVersion(entries []os.DirEntry, sourceFolder, tempDir string, migrationVersion uint) (bool, error) { copied := false for _, entry := range entries { if entry.IsDir() { continue } name := entry.Name() - if !strings.HasSuffix(name, afterUpSuffix) { + targetName, ok := afterMigrationTargetName(name) + if !ok { continue } version, ok := parseMigrationVersion(name) if !ok || version != migrationVersion { continue } - targetName := strings.TrimSuffix(name, afterUpSuffix) + ".up.sql" if err := copyFile(filepath.Join(sourceFolder, name), filepath.Join(tempDir, targetName)); err != nil { - cleanup() - return "", nil, err + return false, err } copied = true } - if !copied { - noopName := fmt.Sprintf("%d_noop.up.sql", migrationVersion) - if err := os.WriteFile(filepath.Join(tempDir, noopName), []byte("SELECT 1;"), 0o600); err != nil { - cleanup() - return "", nil, errors.Wrap(err, fmt.Sprintf("could not write %q", noopName)) - } + return copied, nil +} + +func afterMigrationTargetName(filename string) (string, bool) { + switch { + case strings.HasSuffix(filename, afterUpSuffix): + return strings.TrimSuffix(filename, afterUpSuffix) + ".up.sql", true + case strings.HasSuffix(filename, afterSuffix): + return strings.TrimSuffix(filename, afterSuffix) + ".up.sql", true + default: + return "", false } - return tempDir, cleanup, nil } func fileExists(path string) (bool, error) { @@ -469,51 +472,92 @@ func isAfterMigrationFile(filename string) bool { } func findBeforeUpFile(sourceFolder string, migrationVersion uint) (string, error) { - entries, err := os.ReadDir(sourceFolder) - if err != nil { - return "", err - } - for _, entry := range entries { - if entry.IsDir() { - continue - } - name := entry.Name() - if !strings.HasSuffix(name, beforeUpSuffix) && !strings.HasSuffix(name, beforeSuffix) { - continue - } - version, ok := parseMigrationVersion(name) - if !ok { - continue - } - if version == migrationVersion { - return name, nil - } - } - return "", nil + return findHookFile(sourceFolder, migrationVersion, beforeUpSuffix, beforeSuffix, "before") } func findAfterUpFile(sourceFolder string, migrationVersion uint) (string, error) { + return findHookFile(sourceFolder, migrationVersion, afterUpSuffix, afterSuffix, "after") +} + +type hookFileKind uint8 + +const ( + hookFileNone hookFileKind = iota + hookFileUp + hookFilePlain +) + +func findHookFile(sourceFolder string, migrationVersion uint, suffixUp, suffixPlain, hookName string) (string, error) { entries, err := os.ReadDir(sourceFolder) if err != nil { return "", err } + foundUp, foundPlain, err := scanHookFiles(entries, migrationVersion, suffixUp, suffixPlain, hookName) + if err != nil { + return "", err + } + if foundUp != "" && foundPlain != "" { + return "", fmt.Errorf("conflicting %s migrations found for version %d: %q and %q", hookName, migrationVersion, foundUp, foundPlain) + } + if foundUp != "" { + return foundUp, nil + } + return foundPlain, nil +} + +func scanHookFiles(entries []os.DirEntry, migrationVersion uint, suffixUp, suffixPlain, hookName string) (string, string, error) { + var foundUp string + var foundPlain string for _, entry := range entries { if entry.IsDir() { continue } name := entry.Name() - if !strings.HasSuffix(name, afterUpSuffix) && !strings.HasSuffix(name, afterSuffix) { - continue - } - version, ok := parseMigrationVersion(name) + kind, ok := classifyHookFile(name, migrationVersion, suffixUp, suffixPlain) if !ok { continue } - if version == migrationVersion { - return name, nil + switch kind { + case hookFileUp: + if foundUp != "" && foundUp != name { + return "", "", fmt.Errorf( + "multiple %s migrations found for version %d: %q and %q", + hookName, + migrationVersion, + foundUp, + name, + ) + } + foundUp = name + case hookFilePlain: + if foundPlain != "" && foundPlain != name { + return "", "", fmt.Errorf( + "multiple %s migrations found for version %d: %q and %q", + hookName, + migrationVersion, + foundPlain, + name, + ) + } + foundPlain = name + default: + continue } } - return "", nil + return foundUp, foundPlain, nil +} + +func classifyHookFile(filename string, migrationVersion uint, suffixUp, suffixPlain string) (hookFileKind, bool) { + switch { + case strings.HasSuffix(filename, suffixUp): + version, ok := parseMigrationVersion(filename) + return hookFileUp, ok && version == migrationVersion + case strings.HasSuffix(filename, suffixPlain): + version, ok := parseMigrationVersion(filename) + return hookFilePlain, ok && version == migrationVersion + default: + return hookFileNone, false + } } func parseMigrationVersion(filename string) (uint, bool) { diff --git a/pkg/migrate/migrate_test.go b/pkg/migrate/migrate_test.go index 6f24dea..a774b46 100644 --- a/pkg/migrate/migrate_test.go +++ b/pkg/migrate/migrate_test.go @@ -137,6 +137,17 @@ func TestFindBeforeUpFile(t *testing.T) { } } +func TestFindBeforeUpFile_Conflicts(t *testing.T) { + dir := t.TempDir() + writeMigrationFile(t, dir, "004_new.before.sql") + writeMigrationFile(t, dir, "004_new.before.up.sql") + + _, err := findBeforeUpFile(dir, 4) + if err == nil { + t.Fatalf("expected conflict error") + } +} + func TestFindAfterUpFile(t *testing.T) { dir := t.TempDir() writeMigrationFile(t, dir, "001_init.after.up.sql") @@ -169,6 +180,17 @@ func TestFindAfterUpFile(t *testing.T) { } } +func TestFindAfterUpFile_Conflicts(t *testing.T) { + dir := t.TempDir() + writeMigrationFile(t, dir, "004_new.after.sql") + writeMigrationFile(t, dir, "004_new.after.up.sql") + + _, err := findAfterUpFile(dir, 4) + if err == nil { + t.Fatalf("expected conflict error") + } +} + func TestCreateAfterSourceFolder_excludesBefore(t *testing.T) { dir := t.TempDir() writeMigrationFile(t, dir, "001_init.up.sql") @@ -255,6 +277,30 @@ func TestCreateAfterSourceFolderForVersion(t *testing.T) { } } +func TestCreateAfterSourceFolderForVersion_afterDotSql(t *testing.T) { + dir := t.TempDir() + writeMigrationFile(t, dir, "004_new.after.sql") + + afterDir, cleanup, err := CreateAfterSourceFolderForVersion(dir, 4) + if err != nil { + t.Fatalf("CreateAfterSourceFolderForVersion() error = %v", err) + } + if cleanup != nil { + defer cleanup() + } + entries, err := os.ReadDir(afterDir) + if err != nil { + t.Fatalf("ReadDir() error = %v", err) + } + found := map[string]bool{} + for _, entry := range entries { + found[entry.Name()] = true + } + if !found["004_new.up.sql"] { + t.Fatalf("CreateAfterSourceFolderForVersion() should include renamed after.sql file") + } +} + var wantParsedFdwUp = `BEGIN; CREATE SERVER IF NOT EXISTS keymgmt_server FOREIGN DATA WRAPPER postgres_fdw OPTIONS (host 'myHostname', dbname 'myDBName', port '42');