From f92dbede22e8d1b241893da9c9173fa16083a872 Mon Sep 17 00:00:00 2001 From: Benjamin Ross Date: Fri, 26 Jun 2026 10:03:17 -0600 Subject: [PATCH 1/8] feat(testdb): add fast test database creation for Postgres and Spanner Postgres: NewPostgresDatabaseFromTemplate creates isolated test databases by cloning a content-addressed template database that has already been migrated. The template is created once per unique migration set (SHA256 of migration filenames and contents) and reused across all tests in the process. Uses pg_advisory_lock for cross-process safety and marks the template with IS_TEMPLATE=true, ALLOW_CONNECTIONS=false. Spanner: NewSpannerDatabaseFromMigrations and CreateSpannerDatabaseFromMigrations create a Spanner test database with all migrations applied in a single batched DDL update via UpdateDatabaseDdl, instead of running each migration file individually through golang-migrate. The spanner_schema_migrations table is pre-populated with the latest version so subsequent RunMigrations calls are no-ops (ErrNoChange). Falls back to normal migration if DDL parsing fails. Shared helpers: migrationFiles, migrationHash, and migrationVersion utilities for walking embedded migration FS, computing content-addressed hashes, and extracting version numbers from filenames. --- database/testdb/migrations.go | 68 +++++++++++ database/testdb/postgres_template.go | 171 +++++++++++++++++++++++++++ database/testdb/spanner_fast.go | 157 ++++++++++++++++++++++++ 3 files changed, 396 insertions(+) create mode 100644 database/testdb/migrations.go create mode 100644 database/testdb/postgres_template.go create mode 100644 database/testdb/spanner_fast.go diff --git a/database/testdb/migrations.go b/database/testdb/migrations.go new file mode 100644 index 00000000..8d5d76a0 --- /dev/null +++ b/database/testdb/migrations.go @@ -0,0 +1,68 @@ +package testdb + +import ( + "crypto/sha256" + "encoding/hex" + "fmt" + "io" + "io/fs" + "sort" + "strconv" + "strings" +) + +// migrationFiles walks the embedded migrations FS, filters by suffix, sorts by +// filename (which starts with a zero-padded version number), and returns the +// sorted filenames and their contents. +func migrationFiles(migrations fs.FS, suffix string) ([]string, []string, error) { + entries, err := fs.ReadDir(migrations, "migrations") + if err != nil { + return nil, nil, fmt.Errorf("reading migrations directory: %w", err) + } + + var names []string + for _, e := range entries { + if e.IsDir() || !strings.HasSuffix(e.Name(), suffix) { + continue + } + names = append(names, e.Name()) + } + sort.Strings(names) + + contents := make([]string, len(names)) + for i, name := range names { + data, err := fs.ReadFile(migrations, "migrations/"+name) + if err != nil { + return nil, nil, fmt.Errorf("reading migration %s: %w", name, err) + } + contents[i] = string(data) + } + + return names, contents, nil +} + +// migrationHash computes a SHA256 hash of migration filenames and contents. +// The hash is content-addressed: it changes when any migration is added or +// modified, which automatically invalidates caches keyed by the hash. +func migrationHash(names, contents []string) string { + h := sha256.New() + for i := range names { + io.WriteString(h, names[i]) + io.WriteString(h, contents[i]) + } + return hex.EncodeToString(h.Sum(nil)) +} + +// migrationVersion extracts the version number from the last migration filename. +// Filenames follow the pattern NNN_description.up.{suffix}.sql +func migrationVersion(names []string) (int, error) { + if len(names) == 0 { + return 0, nil + } + parts := strings.SplitN(names[len(names)-1], "_", 2) + v, err := strconv.Atoi(parts[0]) + if err != nil { + return 0, fmt.Errorf("parsing migration version from %s: %w", names[len(names)-1], err) + } + return v, nil +} diff --git a/database/testdb/postgres_template.go b/database/testdb/postgres_template.go new file mode 100644 index 00000000..7cf85e57 --- /dev/null +++ b/database/testdb/postgres_template.go @@ -0,0 +1,171 @@ +package testdb + +import ( + "database/sql" + "fmt" + "io/fs" + "sync" + "testing" + + "github.com/jackc/pgx/v5" + "github.com/moov-io/base" + "github.com/moov-io/base/database" + "github.com/moov-io/base/log" +) + +// templateRegistry tracks template databases created within a process. +// The template is content-addressed by migration hash so it auto-invalidates +// when migrations change. The template is created once and reused for all +// subsequent tests in the same process. +var ( + templateMu sync.Mutex + templatesCreated = make(map[string]bool) +) + +// NewPostgresDatabaseFromTemplate creates an isolated test database by cloning +// a template database that has already been migrated. The template is created +// once per unique migration set (content-addressed by SHA256) and reused across +// all tests in the process. This avoids running migrations for every test. +// +// The template database is marked IS_TEMPLATE=true and ALLOW_CONNECTIONS=false +// so no test can accidentally connect to it. Each test gets its own clone with +// full isolation. +func NewPostgresDatabaseFromTemplate( + tb testing.TB, + logger log.Logger, + cfg database.DatabaseConfig, + migrations fs.FS, +) (database.DatabaseConfig, func()) { + tb.Helper() + + if cfg.Postgres == nil { + tb.Fatal("NewPostgresDatabaseFromTemplate: postgres config not defined") + } + + names, contents, err := migrationFiles(migrations, ".up.postgres.sql") + if err != nil { + tb.Fatal(fmt.Errorf("reading postgres migrations: %w", err)) + } + + hash := migrationHash(names, contents) + templateName := fmt.Sprintf("tmpl_%s", hash[:16]) + + // Connect to an admin database to manage templates. + // Try "moov" first (the standard Moov docker-compose DB), then "postgres". + adminDb := openAdminDb(tb, cfg.Postgres) + + // Serialize template creation across processes using an advisory lock + // keyed by the migration hash. + lockKey := hashToLockKey(hash) + if _, err := adminDb.Exec("SELECT pg_advisory_lock($1)", lockKey); err != nil { + tb.Fatal(fmt.Errorf("acquiring advisory lock: %w", err)) + } + defer adminDb.Exec("SELECT pg_advisory_unlock($1)", lockKey) + + // Check if the template already exists and is marked as a template. + templateMu.Lock() + needCreate := !templatesCreated[hash] + templateMu.Unlock() + + if needCreate { + var exists bool + err = adminDb.QueryRow( + "SELECT EXISTS(SELECT 1 FROM pg_database WHERE datname = $1 AND datistemplate = true)", + templateName, + ).Scan(&exists) + if err != nil { + tb.Fatal(fmt.Errorf("checking template existence: %w", err)) + } + + if !exists { + // Create the template database. + _, err = adminDb.Exec(fmt.Sprintf("CREATE DATABASE %s", pgx.Identifier{templateName}.Sanitize())) + if err != nil { + // Maybe a concurrent process created it — re-check. + err2 := adminDb.QueryRow( + "SELECT EXISTS(SELECT 1 FROM pg_database WHERE datname = $1 AND datistemplate = true)", + templateName, + ).Scan(&exists) + if err2 != nil || !exists { + tb.Fatal(fmt.Errorf("creating template database: %w", err)) + } + } else { + // Run migrations on the template. + templateCfg := database.DatabaseConfig{ + DatabaseName: templateName, + Postgres: cfg.Postgres, + } + if err := database.RunMigrations(logger, templateCfg, database.WithEmbeddedMigrations(migrations)); err != nil { + adminDb.Exec(fmt.Sprintf("DROP DATABASE IF EXISTS %s", pgx.Identifier{templateName}.Sanitize())) + tb.Fatal(fmt.Errorf("running migrations on template: %w", err)) + } + + // Mark as template and disallow direct connections. + _, err = adminDb.Exec(fmt.Sprintf("ALTER DATABASE %s IS_TEMPLATE true", pgx.Identifier{templateName}.Sanitize())) + if err != nil { + tb.Fatal(fmt.Errorf("marking template: %w", err)) + } + _, err = adminDb.Exec(fmt.Sprintf("ALTER DATABASE %s ALLOW_CONNECTIONS false", pgx.Identifier{templateName}.Sanitize())) + if err != nil { + tb.Fatal(fmt.Errorf("disallowing connections to template: %w", err)) + } + } + } + + templateMu.Lock() + templatesCreated[hash] = true + templateMu.Unlock() + } + + // Create the per-test database from the template. + testDbName := "test" + base.ID()[0:26] + _, err = adminDb.Exec(fmt.Sprintf("CREATE DATABASE %s TEMPLATE %s", + pgx.Identifier{testDbName}.Sanitize(), + pgx.Identifier{templateName}.Sanitize())) + if err != nil { + tb.Fatal(fmt.Errorf("creating test database from template: %w", err)) + } + + dropFn := func() { + _, err := adminDb.Exec(fmt.Sprintf("DROP DATABASE %s", pgx.Identifier{testDbName}.Sanitize())) + if err != nil { + tb.Logf("cleanup: drop database %s: %v", testDbName, err) + } + adminDb.Close() + } + + cfg.DatabaseName = testDbName + return cfg, dropFn +} + +// openAdminDb connects to an admin database for managing templates. +// Tries "moov" first, then falls back to "postgres". +func openAdminDb(tb testing.TB, pgCfg *database.PostgresConfig) *sql.DB { + tb.Helper() + + for _, adminDb := range []string{"moov", "postgres"} { + db, err := sql.Open("pgx", fmt.Sprintf("postgres://%s:%s@%s/%s", + pgCfg.User, pgCfg.Password, pgCfg.Address, adminDb)) + if err != nil { + continue + } + if err := db.Ping(); err != nil { + db.Close() + continue + } + return db + } + + tb.Fatal("could not connect to admin database (tried 'moov' and 'postgres')") + return nil +} + +// hashToLockKey converts the first 8 bytes of a hex hash string into an int64 +// for use with pg_advisory_lock. +func hashToLockKey(hash string) int64 { + var key int64 + for i := 0; i < 8 && i < len(hash); i++ { + key = key<<8 | int64(hash[i]) + } + return key +} diff --git a/database/testdb/spanner_fast.go b/database/testdb/spanner_fast.go new file mode 100644 index 00000000..a552510f --- /dev/null +++ b/database/testdb/spanner_fast.go @@ -0,0 +1,157 @@ +package testdb + +import ( + "context" + "fmt" + "io/fs" + "testing" + + "cloud.google.com/go/spanner" + spannerdb "cloud.google.com/go/spanner/admin/database/apiv1" + "cloud.google.com/go/spanner/admin/database/apiv1/databasepb" + "cloud.google.com/go/spanner/spansql" + "github.com/moov-io/base/database" + "github.com/moov-io/base/log" +) + +// NewSpannerDatabaseFromMigrations creates a Spanner test database with all +// migrations applied in a single batched DDL update, instead of running each +// migration file individually through golang-migrate. The +// spanner_schema_migrations table is pre-populated with the latest version so +// subsequent RunMigrations calls are no-ops (ErrNoChange). +// +// If the batched DDL fails or cannot be parsed, the function falls back to +// creating a fresh empty database and lets the caller run migrations normally. +func NewSpannerDatabaseFromMigrations( + tb testing.TB, + logger log.Logger, + cfg database.DatabaseConfig, + migrations fs.FS, +) (database.DatabaseConfig, func()) { + tb.Helper() + cfg2, dropFn, err := CreateSpannerDatabaseFromMigrations(logger, cfg, migrations) + if err != nil { + tb.Fatal(err) + } + return cfg2, dropFn +} + +// CreateSpannerDatabaseFromMigrations is the error-returning variant of +// NewSpannerDatabaseFromMigrations. It is safe to call from contexts that +// cannot use tb.Fatal (e.g. inside sync.Once.Do). +func CreateSpannerDatabaseFromMigrations( + logger log.Logger, + cfg database.DatabaseConfig, + migrations fs.FS, +) (database.DatabaseConfig, func(), error) { + cfg, err := NewSpannerDatabase(cfg.DatabaseName, cfg.Spanner) + if err != nil { + return cfg, nil, fmt.Errorf("creating spanner database: %w", err) + } + + dropFn := func() { dropSpannerDBByCfg(cfg) } + + names, contents, err := migrationFiles(migrations, ".up.spanner.sql") + if err != nil { + return cfg, dropFn, fmt.Errorf("reading spanner migrations: %w", err) + } + + if len(names) == 0 { + return cfg, dropFn, nil + } + + version, err := migrationVersion(names) + if err != nil { + return cfg, dropFn, err + } + + var allStmts []string + for i, content := range contents { + ddl, err := spansql.ParseDDL(names[i], content) + if err != nil { + logger.Info().Logf("spanner fast create: DDL parse failed for %s, falling back: %v", names[i], err) + return fallbackSpannerE(cfg) + } + for _, stmt := range ddl.List { + allStmts = append(allStmts, stmt.SQL()) + } + } + + schemaMigrationsDDL := `CREATE TABLE spanner_schema_migrations ( + Version INT64 NOT NULL, + Dirty BOOL NOT NULL +) PRIMARY KEY(Version)` + allStmts = append([]string{schemaMigrationsDDL}, allStmts...) + + ctx := context.Background() + adminClient, err := spannerdb.NewDatabaseAdminClient(ctx) + if err != nil { + return cfg, dropFn, fmt.Errorf("creating spanner admin client: %w", err) + } + defer adminClient.Close() + + dbPath := fmt.Sprintf("projects/%s/instances/%s/databases/%s", + cfg.Spanner.Project, cfg.Spanner.Instance, cfg.DatabaseName) + + op, err := adminClient.UpdateDatabaseDdl(ctx, &databasepb.UpdateDatabaseDdlRequest{ + Database: dbPath, + Statements: allStmts, + }) + if err != nil { + logger.Info().Logf("spanner fast create: batched DDL request failed, falling back: %v", err) + return fallbackSpannerE(cfg) + } + if err := op.Wait(ctx); err != nil { + logger.Info().Logf("spanner fast create: batched DDL operation failed, falling back: %v", err) + return fallbackSpannerE(cfg) + } + + dataClient, err := spanner.NewClient(ctx, dbPath) + if err != nil { + return cfg, dropFn, fmt.Errorf("creating spanner data client for version insert: %w", err) + } + _, err = dataClient.ReadWriteTransaction(ctx, func(ctx context.Context, txn *spanner.ReadWriteTransaction) error { + return txn.BufferWrite([]*spanner.Mutation{ + spanner.Delete("spanner_schema_migrations", spanner.AllKeys()), + spanner.Insert("spanner_schema_migrations", + []string{"Version", "Dirty"}, + []interface{}{version, false}, + ), + }) + }) + dataClient.Close() + if err != nil { + return cfg, dropFn, fmt.Errorf("inserting migration version: %w", err) + } + + logger.Info().Logf("spanner fast create: applied %d migration files as %d DDL statements (version %d)", len(names), len(allStmts)-1, version) + return cfg, dropFn, nil +} + +// fallbackSpannerE drops the current database, creates a fresh empty one, and +// returns it so the caller can run migrations normally via LoadDatabase. +func fallbackSpannerE(cfg database.DatabaseConfig) (database.DatabaseConfig, func(), error) { + dropSpannerDBByCfg(cfg) + cfg2, err := NewSpannerDatabase(cfg.DatabaseName, cfg.Spanner) + if err != nil { + return cfg, nil, fmt.Errorf("fallback: creating fresh spanner database: %w", err) + } + return cfg2, func() { dropSpannerDBByCfg(cfg2) }, nil +} + +// dropSpannerDBByCfg drops a Spanner database described by a DatabaseConfig. +func dropSpannerDBByCfg(cfg database.DatabaseConfig) { + ctx := context.Background() + adminClient, err := spannerdb.NewDatabaseAdminClient(ctx) + if err != nil { + return + } + defer adminClient.Close() + + dbPath := fmt.Sprintf("projects/%s/instances/%s/databases/%s", + cfg.Spanner.Project, cfg.Spanner.Instance, cfg.DatabaseName) + + _ = adminClient.DropDatabase(ctx, &databasepb.DropDatabaseRequest{ + Database: dbPath, + }) +} From d7dc9779a9631bfbb4c6620a5cc144eb2a1300a0 Mon Sep 17 00:00:00 2001 From: Benjamin Ross Date: Fri, 26 Jun 2026 10:09:29 -0600 Subject: [PATCH 2/8] fix(deps): bump golang.org/x/net to v0.55.0 for GO-2026-5026 Pre-existing govulncheck failure in admin.TestAdmin__AddHandler via idna.ToASCII. Not related to the testdb changes but blocking CI. --- go.mod | 4 ++-- go.sum | 20 ++++---------------- 2 files changed, 6 insertions(+), 18 deletions(-) diff --git a/go.mod b/go.mod index 9ead87e7..5db6a5c9 100644 --- a/go.mod +++ b/go.mod @@ -93,10 +93,10 @@ require ( go.yaml.in/yaml/v2 v2.4.4 // indirect go.yaml.in/yaml/v3 v3.0.4 // indirect golang.org/x/crypto v0.51.0 // indirect - golang.org/x/net v0.54.0 // indirect + golang.org/x/net v0.55.0 // indirect golang.org/x/oauth2 v0.36.0 // indirect golang.org/x/sync v0.20.0 // indirect - golang.org/x/sys v0.44.0 // indirect + golang.org/x/sys v0.45.0 // indirect golang.org/x/text v0.37.0 // indirect google.golang.org/api v0.278.0 // indirect google.golang.org/genproto v0.0.0-20260504160031-60b97b32f348 // indirect diff --git a/go.sum b/go.sum index a0194c29..5cfeeaee 100644 --- a/go.sum +++ b/go.sum @@ -5,8 +5,6 @@ cloud.google.com/go v0.123.0 h1:2NAUJwPR47q+E35uaJeYoNhuNEM9kM8SjgRgdeOJUSE= cloud.google.com/go v0.123.0/go.mod h1:xBoMV08QcqUGuPW65Qfm1o9Y4zKZBpGS+7bImXLTAZU= cloud.google.com/go/alloydb v1.26.0 h1:UTzyumJ8tEo0CqwzLkV4WMGnCxvvhw3BDy1nXfCt9KE= cloud.google.com/go/alloydb v1.26.0/go.mod h1:oqHGc/Xb5fWtH+wIDpu2wcPJX9oML/fGJuH/sp8ysyo= -cloud.google.com/go/alloydbconn v1.18.2 h1:zxxUnSU50d1sS/nswGBldpZsaxF3emirbO+xF+Rtgig= -cloud.google.com/go/alloydbconn v1.18.2/go.mod h1:0vfrUSwleLoK13ycnoaZ1A4yCfg3r7rig7Xm5vKlEtM= cloud.google.com/go/alloydbconn v1.18.3 h1:7A8QN5DtPkyGToqekWSm4+Ryrx2+xYWkvRYh0A0fAVg= cloud.google.com/go/alloydbconn v1.18.3/go.mod h1:cbwUQHP9eLp3Y66xZyQZrKmlcvqLnlb91qKc6kXV46Q= cloud.google.com/go/auth v0.20.0 h1:kXTssoVb4azsVDoUiF8KvxAqrsQcQtB53DcSgta74CA= @@ -280,8 +278,6 @@ go.yaml.in/yaml/v3 v3.0.4 h1:tfq32ie2Jv2UxXFdLJdh3jXuOzWiL1fo0bu/FbuKpbc= go.yaml.in/yaml/v3 v3.0.4/go.mod h1:DhzuOOF2ATzADvBadXxruRBLzYTpT36CKvDb3+aBEFg= golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w= golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto= -golang.org/x/crypto v0.50.0 h1:zO47/JPrL6vsNkINmLoo/PH1gcxpls50DNogFvB5ZGI= -golang.org/x/crypto v0.50.0/go.mod h1:3muZ7vA7PBCE6xgPX7nkzzjiUq87kRItoJQM1Yo8S+Q= golang.org/x/crypto v0.51.0 h1:IBPXwPfKxY7cWQZ38ZCIRPI50YLeevDLlLnyC5wRGTI= golang.org/x/crypto v0.51.0/go.mod h1:8AdwkbraGNABw2kOX6YFPs3WM22XqI4EXEd8g+x7Oc8= golang.org/x/exp v0.0.0-20190121172915-509febef88a4/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA= @@ -294,10 +290,8 @@ golang.org/x/net v0.0.0-20190213061140-3a22650c66bd/go.mod h1:mL1N/T3taQHkDXs73r golang.org/x/net v0.0.0-20190311183353-d8887717615a/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg= golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg= golang.org/x/net v0.0.0-20201110031124-69a78807bb2b/go.mod h1:sp8m0HH+o8qH0wwXwYZr8TS3Oi6o0r6Gce1SSxlDquU= -golang.org/x/net v0.53.0 h1:d+qAbo5L0orcWAr0a9JweQpjXF19LMXJE8Ey7hwOdUA= -golang.org/x/net v0.53.0/go.mod h1:JvMuJH7rrdiCfbeHoo3fCQU24Lf5JJwT9W3sJFulfgs= -golang.org/x/net v0.54.0 h1:2zJIZAxAHV/OHCDTCOHAYehQzLfSXuf/5SoL/Dv6w/w= -golang.org/x/net v0.54.0/go.mod h1:Sj4oj8jK6XmHpBZU/zWHw3BV3abl4Kvi+Ut7cQcY+cQ= +golang.org/x/net v0.55.0 h1:bcvxaJn3e1U6InsFWt1JUq1aSjnRxLzT2rtD2KfkDF8= +golang.org/x/net v0.55.0/go.mod h1:L5U2KuzuOe1lY7Z+aWVIKK6qEeJXnXV9yzGA+WCHJww= golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U= golang.org/x/oauth2 v0.36.0 h1:peZ/1z27fi9hUOFCAZaHyrpWG5lwe0RJEEEeH0ThlIs= golang.org/x/oauth2 v0.36.0/go.mod h1:YDBUJMTkDnJS+A4BP4eZBjCqtokkg1hODuPjwiGPO7Q= @@ -310,12 +304,10 @@ golang.org/x/sys v0.0.0-20180830151530-49385e6e1522/go.mod h1:STP8DvDyc/dI5b8T5h golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20200930185726-fdedc70b468f/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= -golang.org/x/sys v0.44.0 h1:ildZl3J4uzeKP07r2F++Op7E9B29JRUy+a27EibtBTQ= -golang.org/x/sys v0.44.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= +golang.org/x/sys v0.45.0 h1:dO4czNzziLiiXplLQgBCEpCvXQ3dnkn0SdaZSYdQ+FY= +golang.org/x/sys v0.45.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= -golang.org/x/text v0.36.0 h1:JfKh3XmcRPqZPKevfXVpI1wXPTqbkE5f7JA92a55Yxg= -golang.org/x/text v0.36.0/go.mod h1:NIdBknypM8iqVmPiuco0Dh6P5Jcdk8lJL0CUebqK164= golang.org/x/text v0.37.0 h1:Cqjiwd9eSg8e0QAkyCaQTNHFIIzWtidPahFWR83rTrc= golang.org/x/text v0.37.0/go.mod h1:a5sjxXGs9hsn/AJVwuElvCAo9v8QYLzvavO5z2PiM38= golang.org/x/time v0.15.0 h1:bbrp8t3bGUeFOx08pvsMYRTCVSMk89u4tKbNOZbp88U= @@ -339,8 +331,6 @@ google.golang.org/genproto v0.0.0-20260504160031-60b97b32f348 h1:JjVGDZYWkJWZcxv google.golang.org/genproto v0.0.0-20260504160031-60b97b32f348/go.mod h1:95PqD4xM+AdOcBGsmgfaofXsiA37uXDtDufVbntT3TU= google.golang.org/genproto/googleapis/api v0.0.0-20260504160031-60b97b32f348 h1:U8orV30l6KpDsi9dxU0CoJZGbjS8EEpw+6ba+XwGPQA= google.golang.org/genproto/googleapis/api v0.0.0-20260504160031-60b97b32f348/go.mod h1:Yzdzr5OOZFgSsEV2D/Xi9NL3bszpXFAg0hFJiRohcD8= -google.golang.org/genproto/googleapis/rpc v0.0.0-20260504160031-60b97b32f348 h1:pfIbyB44sWzHiCpRqIen67ZQnVXSfIxWrqUMk1qwODE= -google.golang.org/genproto/googleapis/rpc v0.0.0-20260504160031-60b97b32f348/go.mod h1:4Hqkh8ycfw05ld/3BWL7rJOSfebL2Q+DVDeRgYgxUU8= google.golang.org/genproto/googleapis/rpc v0.0.0-20260511170946-3700d4141b60 h1:seT2EwLWM78plQ7wcDfuWBc/4FAEAXDDiaSol4ku4qo= google.golang.org/genproto/googleapis/rpc v0.0.0-20260511170946-3700d4141b60/go.mod h1:4Hqkh8ycfw05ld/3BWL7rJOSfebL2Q+DVDeRgYgxUU8= google.golang.org/grpc v1.19.0/go.mod h1:mqu4LbDTu4XGKhr4mRzUsmM4RtVoemTSY81AxZiDr8c= @@ -348,8 +338,6 @@ google.golang.org/grpc v1.23.0/go.mod h1:Y5yQAOtifL1yxbo5wqy6BxZv8vAUGQwXBOALyac google.golang.org/grpc v1.25.1/go.mod h1:c3i+UQWmh7LiEpx4sFZnkU36qjEYZ0imhYfXVyQciAY= google.golang.org/grpc v1.27.0/go.mod h1:qbnxyOmOxrQa7FizSgH+ReBfzJrCY1pSN7KXBS8abTk= google.golang.org/grpc v1.33.2/go.mod h1:JMHMWHQWaTccqQQlmk3MJZS+GWXOdAesneDmEnv2fbc= -google.golang.org/grpc v1.81.0 h1:W3G9N3KQf3BU+YuCtGKJk0CmxQNbAISICD/9AORxLIw= -google.golang.org/grpc v1.81.0/go.mod h1:xGH9GfzOyMTGIOXBJmXt+BX/V0kcdQbdcuwQ/zNw42I= google.golang.org/grpc v1.81.1 h1:VnnIIZ88UzOOKLukQi+ImGz8O1Wdp8nAGGnvOfEIWQQ= google.golang.org/grpc v1.81.1/go.mod h1:xGH9GfzOyMTGIOXBJmXt+BX/V0kcdQbdcuwQ/zNw42I= google.golang.org/protobuf v0.0.0-20200109180630-ec00e32a8dfd/go.mod h1:DFci5gLYBciE7Vtevhsrf46CRTquxDuWsQurQQe4oz8= From 2918f3a748f961463da04a261add4f6aa76a78b6 Mon Sep 17 00:00:00 2001 From: Benjamin Ross Date: Fri, 26 Jun 2026 10:17:08 -0600 Subject: [PATCH 3/8] test(testdb): add unit tests for migration helper functions Covers migrationFiles, migrationHash, and migrationVersion with embedded test fixture migrations. Brings testdb package coverage above 0% to satisfy CI coverage threshold. --- database/testdb/migrations_test.go | 102 ++++++++++++++++++ .../001_create_items.up.spanner.sql | 3 + .../001_create_users.up.postgres.sql | 1 + .../migrations/002_add_email.up.postgres.sql | 1 + .../migrations/002_add_price.up.spanner.sql | 1 + 5 files changed, 108 insertions(+) create mode 100644 database/testdb/migrations_test.go create mode 100644 database/testdb/testdata/migrations/001_create_items.up.spanner.sql create mode 100644 database/testdb/testdata/migrations/001_create_users.up.postgres.sql create mode 100644 database/testdb/testdata/migrations/002_add_email.up.postgres.sql create mode 100644 database/testdb/testdata/migrations/002_add_price.up.spanner.sql diff --git a/database/testdb/migrations_test.go b/database/testdb/migrations_test.go new file mode 100644 index 00000000..1d3cbc31 --- /dev/null +++ b/database/testdb/migrations_test.go @@ -0,0 +1,102 @@ +package testdb + +import ( + "embed" + "io/fs" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +//go:embed all:testdata +var testEmbedFS embed.FS + +// testMigrationsFS wraps the embedded files so that migrationFiles can call +// fs.ReadDir(migrations, "migrations"). We embed testdata/ which contains +// a migrations/ subdirectory, matching the convention used by +// database.WithEmbeddedMigrations (iofs.New(f, "migrations")). +var testMigrationsFS fs.FS + +func init() { + sub, err := fs.Sub(testEmbedFS, "testdata") + if err != nil { + panic(err) + } + testMigrationsFS = sub +} + +func TestMigrationFiles_postgres(t *testing.T) { + names, contents, err := migrationFiles(testMigrationsFS, ".up.postgres.sql") + require.NoError(t, err) + assert.Len(t, names, 2) + assert.Equal(t, "001_create_users.up.postgres.sql", names[0]) + assert.Equal(t, "002_add_email.up.postgres.sql", names[1]) + assert.Equal(t, "CREATE TABLE users (id TEXT PRIMARY KEY);\n", contents[0]) + assert.Equal(t, "ALTER TABLE users ADD COLUMN email TEXT;\n", contents[1]) +} + +func TestMigrationFiles_spanner(t *testing.T) { + names, contents, err := migrationFiles(testMigrationsFS, ".up.spanner.sql") + require.NoError(t, err) + assert.Len(t, names, 2) + assert.Equal(t, "001_create_items.up.spanner.sql", names[0]) + assert.Equal(t, "002_add_price.up.spanner.sql", names[1]) + assert.Contains(t, contents[0], "CREATE TABLE items") + assert.Contains(t, contents[1], "ADD COLUMN Price") +} + +func TestMigrationFiles_wrongSuffix(t *testing.T) { + names, _, err := migrationFiles(testMigrationsFS, ".up.mysql.sql") + require.NoError(t, err) + assert.Empty(t, names) +} + +func TestMigrationHash_stable(t *testing.T) { + names := []string{"001_a.up.sql", "002_b.up.sql"} + contents := []string{"-- a\n", "-- b\n"} + h1 := migrationHash(names, contents) + h2 := migrationHash(names, contents) + assert.Equal(t, h1, h2) + assert.Len(t, h1, 64) +} + +func TestMigrationHash_changesOnContent(t *testing.T) { + names := []string{"001_a.up.sql"} + h1 := migrationHash(names, []string{"-- a\n"}) + h2 := migrationHash(names, []string{"-- b\n"}) + assert.NotEqual(t, h1, h2) +} + +func TestMigrationHash_changesOnName(t *testing.T) { + contents := []string{"-- a\n"} + h1 := migrationHash([]string{"001_a.up.sql"}, contents) + h2 := migrationHash([]string{"001_c.up.sql"}, contents) + assert.NotEqual(t, h1, h2) +} + +func TestMigrationVersion(t *testing.T) { + tests := []struct { + name string + names []string + want int + wantErr bool + }{ + {"empty", nil, 0, false}, + {"single", []string{"001_a.up.sql"}, 1, false}, + {"multi", []string{"001_a.up.sql", "010_b.up.sql", "003_c.up.sql"}, 3, false}, + {"large", []string{"001_a.up.sql", "999_final.up.sql"}, 999, false}, + {"bad", []string{"abc.up.sql"}, 0, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := migrationVersion(tt.names) + if tt.wantErr { + assert.Error(t, err) + } else { + require.NoError(t, err) + assert.Equal(t, tt.want, got) + } + }) + } +} diff --git a/database/testdb/testdata/migrations/001_create_items.up.spanner.sql b/database/testdb/testdata/migrations/001_create_items.up.spanner.sql new file mode 100644 index 00000000..27d39e2d --- /dev/null +++ b/database/testdb/testdata/migrations/001_create_items.up.spanner.sql @@ -0,0 +1,3 @@ +CREATE TABLE items ( + ItemID STRING(36) NOT NULL, +) PRIMARY KEY(ItemID); diff --git a/database/testdb/testdata/migrations/001_create_users.up.postgres.sql b/database/testdb/testdata/migrations/001_create_users.up.postgres.sql new file mode 100644 index 00000000..d8ed438a --- /dev/null +++ b/database/testdb/testdata/migrations/001_create_users.up.postgres.sql @@ -0,0 +1 @@ +CREATE TABLE users (id TEXT PRIMARY KEY); diff --git a/database/testdb/testdata/migrations/002_add_email.up.postgres.sql b/database/testdb/testdata/migrations/002_add_email.up.postgres.sql new file mode 100644 index 00000000..79fc1d86 --- /dev/null +++ b/database/testdb/testdata/migrations/002_add_email.up.postgres.sql @@ -0,0 +1 @@ +ALTER TABLE users ADD COLUMN email TEXT; diff --git a/database/testdb/testdata/migrations/002_add_price.up.spanner.sql b/database/testdb/testdata/migrations/002_add_price.up.spanner.sql new file mode 100644 index 00000000..c1bc3fbb --- /dev/null +++ b/database/testdb/testdata/migrations/002_add_price.up.spanner.sql @@ -0,0 +1 @@ +ALTER TABLE items ADD COLUMN Price INT64; From 6b0f278d8eefe435107658ad274d4a6102ae4b46 Mon Sep 17 00:00:00 2001 From: Benjamin Ross Date: Fri, 26 Jun 2026 10:21:26 -0600 Subject: [PATCH 4/8] fix(testdb): use Contains instead of Equal for line-ending portability Windows CI reads embedded files with CRLF line endings, causing exact string equality assertions to fail. --- database/testdb/migrations_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/database/testdb/migrations_test.go b/database/testdb/migrations_test.go index 1d3cbc31..7de5deff 100644 --- a/database/testdb/migrations_test.go +++ b/database/testdb/migrations_test.go @@ -32,8 +32,8 @@ func TestMigrationFiles_postgres(t *testing.T) { assert.Len(t, names, 2) assert.Equal(t, "001_create_users.up.postgres.sql", names[0]) assert.Equal(t, "002_add_email.up.postgres.sql", names[1]) - assert.Equal(t, "CREATE TABLE users (id TEXT PRIMARY KEY);\n", contents[0]) - assert.Equal(t, "ALTER TABLE users ADD COLUMN email TEXT;\n", contents[1]) + assert.Contains(t, contents[0], "CREATE TABLE users (id TEXT PRIMARY KEY)") + assert.Contains(t, contents[1], "ALTER TABLE users ADD COLUMN email TEXT") } func TestMigrationFiles_spanner(t *testing.T) { From c2db40e01e0129417ac4738dd89f6c5bce440d86 Mon Sep 17 00:00:00 2001 From: Benjamin Ross Date: Fri, 26 Jun 2026 10:37:41 -0600 Subject: [PATCH 5/8] fix(testdb): ensure service database exists as side effect Some tests (e.g. TestEnvironment in card-gateway) call service.NewEnvironment directly without going through CreateTestDatabase. The old CreateTestDatabase created the service database as a side effect via openOrCreateDatabase. NewPostgresDatabaseFromTemplate now preserves that behavior by creating the service database if it doesn't exist. Also fix nil pointer panic by guarding testEnv.Shutdown in card-gateway. --- database/testdb/postgres_template.go | 33 ++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/database/testdb/postgres_template.go b/database/testdb/postgres_template.go index 7cf85e57..e7e64ff8 100644 --- a/database/testdb/postgres_template.go +++ b/database/testdb/postgres_template.go @@ -54,6 +54,13 @@ func NewPostgresDatabaseFromTemplate( // Try "moov" first (the standard Moov docker-compose DB), then "postgres". adminDb := openAdminDb(tb, cfg.Postgres) + // Ensure the service database exists. Some tests (e.g. TestEnvironment) + // call service.NewEnvironment directly without going through + // CreateTestDatabase, relying on the service database existing as a + // side effect of prior test setup. The old CreateTestDatabase created + // it via openOrCreateDatabase; we preserve that behavior here. + ensureServiceDatabase(tb, adminDb, cfg.DatabaseName) + // Serialize template creation across processes using an advisory lock // keyed by the migration hash. lockKey := hashToLockKey(hash) @@ -169,3 +176,29 @@ func hashToLockKey(hash string) int64 { } return key } + +// ensureServiceDatabase creates the service database if it doesn't exist. +// This preserves the side-effect behavior of the old CreateTestDatabase, +// which created the service database via openOrCreateDatabase. Some tests +// call service.NewEnvironment directly without CreateTestDatabase and rely +// on the service database existing. +func ensureServiceDatabase(tb testing.TB, adminDb *sql.DB, dbName string) { + tb.Helper() + if dbName == "" { + return + } + var exists bool + err := adminDb.QueryRow( + "SELECT EXISTS(SELECT 1 FROM pg_database WHERE datname = $1)", + dbName, + ).Scan(&exists) + if err != nil { + tb.Fatal(fmt.Errorf("checking service database existence: %w", err)) + } + if !exists { + _, err = adminDb.Exec(fmt.Sprintf("CREATE DATABASE %s", pgx.Identifier{dbName}.Sanitize())) + if err != nil { + tb.Fatal(fmt.Errorf("creating service database %s: %w", dbName, err)) + } + } +} From 30cb4bf9c09f7b35799be2e76aa8231376d5b741 Mon Sep 17 00:00:00 2001 From: Benjamin Ross Date: Fri, 26 Jun 2026 16:13:05 -0600 Subject: [PATCH 6/8] fix(testdb): address template database review feedback Use a dedicated Postgres connection for advisory lock acquisition and release so the session-level lock is not leaked across pooled connections. Clean up Spanner databases on post-create failures and guard nil migration filesystems before reading migrations. --- database/testdb/postgres_template.go | 57 +++++++++++++++++++++------- database/testdb/spanner_fast.go | 19 +++++++--- 2 files changed, 58 insertions(+), 18 deletions(-) diff --git a/database/testdb/postgres_template.go b/database/testdb/postgres_template.go index e7e64ff8..26ad331c 100644 --- a/database/testdb/postgres_template.go +++ b/database/testdb/postgres_template.go @@ -1,6 +1,7 @@ package testdb import ( + "context" "database/sql" "fmt" "io/fs" @@ -41,6 +42,9 @@ func NewPostgresDatabaseFromTemplate( if cfg.Postgres == nil { tb.Fatal("NewPostgresDatabaseFromTemplate: postgres config not defined") } + if migrations == nil { + tb.Fatal("NewPostgresDatabaseFromTemplate: migrations FS is nil") + } names, contents, err := migrationFiles(migrations, ".up.postgres.sql") if err != nil { @@ -61,13 +65,29 @@ func NewPostgresDatabaseFromTemplate( // it via openOrCreateDatabase; we preserve that behavior here. ensureServiceDatabase(tb, adminDb, cfg.DatabaseName) - // Serialize template creation across processes using an advisory lock - // keyed by the migration hash. + ctx := context.Background() + conn, err := adminDb.Conn(ctx) + if err != nil { + tb.Fatal(fmt.Errorf("acquiring admin connection: %w", err)) + } + defer func() { + if err := conn.Close(); err != nil { + tb.Logf("cleanup: close admin connection: %v", err) + } + }() + + // Serialize template creation across processes using a session-level advisory + // lock keyed by the migration hash. The lock and unlock must use the same + // physical connection. lockKey := hashToLockKey(hash) - if _, err := adminDb.Exec("SELECT pg_advisory_lock($1)", lockKey); err != nil { + if _, err := conn.ExecContext(ctx, "SELECT pg_advisory_lock($1)", lockKey); err != nil { tb.Fatal(fmt.Errorf("acquiring advisory lock: %w", err)) } - defer adminDb.Exec("SELECT pg_advisory_unlock($1)", lockKey) + defer func() { + if _, err := conn.ExecContext(ctx, "SELECT pg_advisory_unlock($1)", lockKey); err != nil { + tb.Logf("cleanup: release advisory lock: %v", err) + } + }() // Check if the template already exists and is marked as a template. templateMu.Lock() @@ -76,7 +96,8 @@ func NewPostgresDatabaseFromTemplate( if needCreate { var exists bool - err = adminDb.QueryRow( + err = conn.QueryRowContext( + ctx, "SELECT EXISTS(SELECT 1 FROM pg_database WHERE datname = $1 AND datistemplate = true)", templateName, ).Scan(&exists) @@ -86,10 +107,11 @@ func NewPostgresDatabaseFromTemplate( if !exists { // Create the template database. - _, err = adminDb.Exec(fmt.Sprintf("CREATE DATABASE %s", pgx.Identifier{templateName}.Sanitize())) + _, err = conn.ExecContext(ctx, fmt.Sprintf("CREATE DATABASE %s", pgx.Identifier{templateName}.Sanitize())) if err != nil { // Maybe a concurrent process created it — re-check. - err2 := adminDb.QueryRow( + err2 := conn.QueryRowContext( + ctx, "SELECT EXISTS(SELECT 1 FROM pg_database WHERE datname = $1 AND datistemplate = true)", templateName, ).Scan(&exists) @@ -103,16 +125,18 @@ func NewPostgresDatabaseFromTemplate( Postgres: cfg.Postgres, } if err := database.RunMigrations(logger, templateCfg, database.WithEmbeddedMigrations(migrations)); err != nil { - adminDb.Exec(fmt.Sprintf("DROP DATABASE IF EXISTS %s", pgx.Identifier{templateName}.Sanitize())) + if _, dropErr := conn.ExecContext(ctx, fmt.Sprintf("DROP DATABASE IF EXISTS %s", pgx.Identifier{templateName}.Sanitize())); dropErr != nil { + tb.Logf("cleanup: drop template database %s: %v", templateName, dropErr) + } tb.Fatal(fmt.Errorf("running migrations on template: %w", err)) } // Mark as template and disallow direct connections. - _, err = adminDb.Exec(fmt.Sprintf("ALTER DATABASE %s IS_TEMPLATE true", pgx.Identifier{templateName}.Sanitize())) + _, err = conn.ExecContext(ctx, fmt.Sprintf("ALTER DATABASE %s IS_TEMPLATE true", pgx.Identifier{templateName}.Sanitize())) if err != nil { tb.Fatal(fmt.Errorf("marking template: %w", err)) } - _, err = adminDb.Exec(fmt.Sprintf("ALTER DATABASE %s ALLOW_CONNECTIONS false", pgx.Identifier{templateName}.Sanitize())) + _, err = conn.ExecContext(ctx, fmt.Sprintf("ALTER DATABASE %s ALLOW_CONNECTIONS false", pgx.Identifier{templateName}.Sanitize())) if err != nil { tb.Fatal(fmt.Errorf("disallowing connections to template: %w", err)) } @@ -126,7 +150,7 @@ func NewPostgresDatabaseFromTemplate( // Create the per-test database from the template. testDbName := "test" + base.ID()[0:26] - _, err = adminDb.Exec(fmt.Sprintf("CREATE DATABASE %s TEMPLATE %s", + _, err = conn.ExecContext(ctx, fmt.Sprintf("CREATE DATABASE %s TEMPLATE %s", pgx.Identifier{testDbName}.Sanitize(), pgx.Identifier{templateName}.Sanitize())) if err != nil { @@ -138,7 +162,9 @@ func NewPostgresDatabaseFromTemplate( if err != nil { tb.Logf("cleanup: drop database %s: %v", testDbName, err) } - adminDb.Close() + if err := adminDb.Close(); err != nil { + tb.Logf("cleanup: close admin database: %v", err) + } } cfg.DatabaseName = testDbName @@ -198,7 +224,12 @@ func ensureServiceDatabase(tb testing.TB, adminDb *sql.DB, dbName string) { if !exists { _, err = adminDb.Exec(fmt.Sprintf("CREATE DATABASE %s", pgx.Identifier{dbName}.Sanitize())) if err != nil { - tb.Fatal(fmt.Errorf("creating service database %s: %w", dbName, err)) + if checkErr := adminDb.QueryRow( + "SELECT EXISTS(SELECT 1 FROM pg_database WHERE datname = $1)", + dbName, + ).Scan(&exists); checkErr != nil || !exists { + tb.Fatal(fmt.Errorf("creating service database %s: %w", dbName, err)) + } } } } diff --git a/database/testdb/spanner_fast.go b/database/testdb/spanner_fast.go index a552510f..00bbeabc 100644 --- a/database/testdb/spanner_fast.go +++ b/database/testdb/spanner_fast.go @@ -44,6 +44,10 @@ func CreateSpannerDatabaseFromMigrations( cfg database.DatabaseConfig, migrations fs.FS, ) (database.DatabaseConfig, func(), error) { + if migrations == nil { + return cfg, nil, fmt.Errorf("migrations FS is nil") + } + cfg, err := NewSpannerDatabase(cfg.DatabaseName, cfg.Spanner) if err != nil { return cfg, nil, fmt.Errorf("creating spanner database: %w", err) @@ -53,7 +57,8 @@ func CreateSpannerDatabaseFromMigrations( names, contents, err := migrationFiles(migrations, ".up.spanner.sql") if err != nil { - return cfg, dropFn, fmt.Errorf("reading spanner migrations: %w", err) + dropSpannerDBByCfg(cfg) + return cfg, nil, fmt.Errorf("reading spanner migrations: %w", err) } if len(names) == 0 { @@ -62,7 +67,8 @@ func CreateSpannerDatabaseFromMigrations( version, err := migrationVersion(names) if err != nil { - return cfg, dropFn, err + dropSpannerDBByCfg(cfg) + return cfg, nil, err } var allStmts []string @@ -86,7 +92,8 @@ func CreateSpannerDatabaseFromMigrations( ctx := context.Background() adminClient, err := spannerdb.NewDatabaseAdminClient(ctx) if err != nil { - return cfg, dropFn, fmt.Errorf("creating spanner admin client: %w", err) + dropSpannerDBByCfg(cfg) + return cfg, nil, fmt.Errorf("creating spanner admin client: %w", err) } defer adminClient.Close() @@ -108,7 +115,8 @@ func CreateSpannerDatabaseFromMigrations( dataClient, err := spanner.NewClient(ctx, dbPath) if err != nil { - return cfg, dropFn, fmt.Errorf("creating spanner data client for version insert: %w", err) + dropSpannerDBByCfg(cfg) + return cfg, nil, fmt.Errorf("creating spanner data client for version insert: %w", err) } _, err = dataClient.ReadWriteTransaction(ctx, func(ctx context.Context, txn *spanner.ReadWriteTransaction) error { return txn.BufferWrite([]*spanner.Mutation{ @@ -121,7 +129,8 @@ func CreateSpannerDatabaseFromMigrations( }) dataClient.Close() if err != nil { - return cfg, dropFn, fmt.Errorf("inserting migration version: %w", err) + dropSpannerDBByCfg(cfg) + return cfg, nil, fmt.Errorf("inserting migration version: %w", err) } logger.Info().Logf("spanner fast create: applied %d migration files as %d DDL statements (version %d)", len(names), len(allStmts)-1, version) From c3e7229e524f5f4a5448ceb8d3ed48065fe5493f Mon Sep 17 00:00:00 2001 From: Benjamin Ross Date: Fri, 26 Jun 2026 16:18:53 -0600 Subject: [PATCH 7/8] test(testdb): cover helper edge cases for short CI Adds unit coverage for service database creation side effects, content-addressed lock keys, and nil Spanner migration files so short CI coverage stays above threshold. --- database/testdb/migrations_test.go | 77 ++++++++++++++++++++++++++++++ go.mod | 1 + go.sum | 3 ++ 3 files changed, 81 insertions(+) diff --git a/database/testdb/migrations_test.go b/database/testdb/migrations_test.go index 7de5deff..5d79c5ad 100644 --- a/database/testdb/migrations_test.go +++ b/database/testdb/migrations_test.go @@ -5,8 +5,12 @@ import ( "io/fs" "testing" + "github.com/DATA-DOG/go-sqlmock" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/moov-io/base/database" + "github.com/moov-io/base/log" ) //go:embed all:testdata @@ -100,3 +104,76 @@ func TestMigrationVersion(t *testing.T) { }) } } + +func TestHashToLockKey(t *testing.T) { + assert.NotZero(t, hashToLockKey("0123456789abcdef")) + assert.Equal(t, int64('a'), hashToLockKey("a")) +} + +func TestEnsureServiceDatabase(t *testing.T) { + t.Run("empty name", func(t *testing.T) { + db, mock, err := sqlmock.New() + require.NoError(t, err) + defer db.Close() + + ensureServiceDatabase(t, db, "") + + require.NoError(t, mock.ExpectationsWereMet()) + }) + + t.Run("already exists", func(t *testing.T) { + db, mock, err := sqlmock.New() + require.NoError(t, err) + defer db.Close() + + mock.ExpectQuery("SELECT EXISTS"). + WithArgs("svc"). + WillReturnRows(sqlmock.NewRows([]string{"exists"}).AddRow(true)) + + ensureServiceDatabase(t, db, "svc") + + require.NoError(t, mock.ExpectationsWereMet()) + }) + + t.Run("create succeeds", func(t *testing.T) { + db, mock, err := sqlmock.New() + require.NoError(t, err) + defer db.Close() + + mock.ExpectQuery("SELECT EXISTS"). + WithArgs("svc"). + WillReturnRows(sqlmock.NewRows([]string{"exists"}).AddRow(false)) + mock.ExpectExec("CREATE DATABASE"). + WillReturnResult(sqlmock.NewResult(0, 1)) + + ensureServiceDatabase(t, db, "svc") + + require.NoError(t, mock.ExpectationsWereMet()) + }) + + t.Run("create loses race", func(t *testing.T) { + db, mock, err := sqlmock.New() + require.NoError(t, err) + defer db.Close() + + mock.ExpectQuery("SELECT EXISTS"). + WithArgs("svc"). + WillReturnRows(sqlmock.NewRows([]string{"exists"}).AddRow(false)) + mock.ExpectExec("CREATE DATABASE"). + WillReturnError(assert.AnError) + mock.ExpectQuery("SELECT EXISTS"). + WithArgs("svc"). + WillReturnRows(sqlmock.NewRows([]string{"exists"}).AddRow(true)) + + ensureServiceDatabase(t, db, "svc") + + require.NoError(t, mock.ExpectationsWereMet()) + }) +} + +func TestCreateSpannerDatabaseFromMigrations_nilMigrations(t *testing.T) { + cfg, dropFn, err := CreateSpannerDatabaseFromMigrations(log.NewNopLogger(), database.DatabaseConfig{}, nil) + require.Error(t, err) + assert.Nil(t, dropFn) + assert.Empty(t, cfg.DatabaseName) +} diff --git a/go.mod b/go.mod index 5db6a5c9..6b48a913 100644 --- a/go.mod +++ b/go.mod @@ -42,6 +42,7 @@ require ( cloud.google.com/go/longrunning v0.13.0 // indirect cloud.google.com/go/monitoring v1.29.0 // indirect filippo.io/edwards25519 v1.2.0 // indirect + github.com/DATA-DOG/go-sqlmock v1.5.2 // indirect github.com/GoogleCloudPlatform/grpc-gcp-go/grpcgcp v1.6.0 // indirect github.com/GoogleCloudPlatform/opentelemetry-operations-go/detectors/gcp v1.32.0 // indirect github.com/GoogleCloudPlatform/opentelemetry-operations-go/exporter/metric v0.56.0 // indirect diff --git a/go.sum b/go.sum index 5cfeeaee..ad2a3b99 100644 --- a/go.sum +++ b/go.sum @@ -30,6 +30,8 @@ filippo.io/edwards25519 v1.2.0/go.mod h1:xzAOLCNug/yB62zG1bQ8uziwrIqIuxhctzJT18Q github.com/Azure/go-ansiterm v0.0.0-20230124172434-306776ec8161 h1:L/gRVlceqvL25UVaW/CKtUDjefjrs0SPonmDGUVOYP0= github.com/Azure/go-ansiterm v0.0.0-20230124172434-306776ec8161/go.mod h1:xomTg63KZ2rFqZQzSB4Vz2SUXa1BpHTVz9L5PTmPC4E= github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU= +github.com/DATA-DOG/go-sqlmock v1.5.2 h1:OcvFkGmslmlZibjAjaHm3L//6LiuBgolP7OputlJIzU= +github.com/DATA-DOG/go-sqlmock v1.5.2/go.mod h1:88MAG/4G7SMwSE3CeA0ZKzrT5CiOU3OJ+JlNzwDqpNU= github.com/GoogleCloudPlatform/grpc-gcp-go/grpcgcp v1.6.0 h1:BzsL0qE7LvtTEtXG7Dt5NS1EP0CQwI21HZfj9aGghhw= github.com/GoogleCloudPlatform/grpc-gcp-go/grpcgcp v1.6.0/go.mod h1:I7kE2kM3qCr9QPT4cU4cCFYkEpVyVr16YOGUHzy+nR0= github.com/GoogleCloudPlatform/opentelemetry-operations-go/detectors/gcp v1.32.0 h1:rIkQfkCOVKc1OiRCNcSDD8ml5RJlZbH/Xsq7lbpynwc= @@ -166,6 +168,7 @@ github.com/jackc/pgx/v5 v5.9.2 h1:3ZhOzMWnR4yJ+RW1XImIPsD1aNSz4T4fyP7zlQb56hw= github.com/jackc/pgx/v5 v5.9.2/go.mod h1:mal1tBGAFfLHvZzaYh77YS/eC6IX9OWbRV1QIIM0Jn4= github.com/jackc/puddle/v2 v2.2.2 h1:PR8nw+E/1w0GLuRFSmiioY6UooMp6KJv0/61nB7icHo= github.com/jackc/puddle/v2 v2.2.2/go.mod h1:vriiEXHvEE654aYKXXjOvZM39qJ0q+azkZFrfEOc3H4= +github.com/kisielk/sqlstruct v0.0.0-20201105191214-5f3e10d3ab46/go.mod h1:yyMNCyc/Ib3bDTKd379tNMpB/7/H5TjM2Y9QJ5THLbE= github.com/klauspost/compress v1.18.0 h1:c/Cqfb0r+Yi+JtIEq73FWXVkRonBlf0CRNYc8Zttxdo= github.com/klauspost/compress v1.18.0/go.mod h1:2Pp+KzxcywXVXMr50+X0Q/Lsb43OQHYWRCY2AiWywWQ= github.com/kr/pretty v0.1.0/go.mod h1:dAy3ld7l9f0ibDNOQOHHMYYIIbhfbHSm3C4ZsoJORNo= From 80895c15d2f589340fdef9cfcaef5ff345530ac0 Mon Sep 17 00:00:00 2001 From: Benjamin Ross Date: Mon, 29 Jun 2026 08:43:06 -0600 Subject: [PATCH 8/8] fix(testdb): make Postgres template lock cover creation path Use the advisory lock only for the template lifecycle: check whether the content-addressed template exists, create and migrate it if missing, then mark it as a template before unlocking. The per-test clone happens after unlocking so clone creation is not unnecessarily serialized. --- database/testdb/postgres_template.go | 148 ++++++++++++--------------- 1 file changed, 67 insertions(+), 81 deletions(-) diff --git a/database/testdb/postgres_template.go b/database/testdb/postgres_template.go index 26ad331c..1af2511c 100644 --- a/database/testdb/postgres_template.go +++ b/database/testdb/postgres_template.go @@ -5,7 +5,6 @@ import ( "database/sql" "fmt" "io/fs" - "sync" "testing" "github.com/jackc/pgx/v5" @@ -14,15 +13,6 @@ import ( "github.com/moov-io/base/log" ) -// templateRegistry tracks template databases created within a process. -// The template is content-addressed by migration hash so it auto-invalidates -// when migrations change. The template is created once and reused for all -// subsequent tests in the same process. -var ( - templateMu sync.Mutex - templatesCreated = make(map[string]bool) -) - // NewPostgresDatabaseFromTemplate creates an isolated test database by cloning // a template database that has already been migrated. The template is created // once per unique migration set (content-addressed by SHA256) and reused across @@ -65,6 +55,44 @@ func NewPostgresDatabaseFromTemplate( // it via openOrCreateDatabase; we preserve that behavior here. ensureServiceDatabase(tb, adminDb, cfg.DatabaseName) + ensurePostgresTemplate(tb, logger, adminDb, cfg, migrations, hash, templateName) + + // Create the per-test database from the template. The template creation lock + // is intentionally released before this point: the lock protects the template + // definition, not every clone created from it. + testDbName := "test" + base.ID()[0:26] + _, err = adminDb.Exec(fmt.Sprintf("CREATE DATABASE %s TEMPLATE %s", + pgx.Identifier{testDbName}.Sanitize(), + pgx.Identifier{templateName}.Sanitize())) + if err != nil { + tb.Fatal(fmt.Errorf("creating test database from template: %w", err)) + } + + dropFn := func() { + _, err := adminDb.Exec(fmt.Sprintf("DROP DATABASE %s", pgx.Identifier{testDbName}.Sanitize())) + if err != nil { + tb.Logf("cleanup: drop database %s: %v", testDbName, err) + } + if err := adminDb.Close(); err != nil { + tb.Logf("cleanup: close admin database: %v", err) + } + } + + cfg.DatabaseName = testDbName + return cfg, dropFn +} + +func ensurePostgresTemplate( + tb testing.TB, + logger log.Logger, + adminDb *sql.DB, + cfg database.DatabaseConfig, + migrations fs.FS, + hash string, + templateName string, +) { + tb.Helper() + ctx := context.Background() conn, err := adminDb.Conn(ctx) if err != nil { @@ -89,86 +117,44 @@ func NewPostgresDatabaseFromTemplate( } }() - // Check if the template already exists and is marked as a template. - templateMu.Lock() - needCreate := !templatesCreated[hash] - templateMu.Unlock() - - if needCreate { - var exists bool - err = conn.QueryRowContext( - ctx, - "SELECT EXISTS(SELECT 1 FROM pg_database WHERE datname = $1 AND datistemplate = true)", - templateName, - ).Scan(&exists) - if err != nil { - tb.Fatal(fmt.Errorf("checking template existence: %w", err)) - } - - if !exists { - // Create the template database. - _, err = conn.ExecContext(ctx, fmt.Sprintf("CREATE DATABASE %s", pgx.Identifier{templateName}.Sanitize())) - if err != nil { - // Maybe a concurrent process created it — re-check. - err2 := conn.QueryRowContext( - ctx, - "SELECT EXISTS(SELECT 1 FROM pg_database WHERE datname = $1 AND datistemplate = true)", - templateName, - ).Scan(&exists) - if err2 != nil || !exists { - tb.Fatal(fmt.Errorf("creating template database: %w", err)) - } - } else { - // Run migrations on the template. - templateCfg := database.DatabaseConfig{ - DatabaseName: templateName, - Postgres: cfg.Postgres, - } - if err := database.RunMigrations(logger, templateCfg, database.WithEmbeddedMigrations(migrations)); err != nil { - if _, dropErr := conn.ExecContext(ctx, fmt.Sprintf("DROP DATABASE IF EXISTS %s", pgx.Identifier{templateName}.Sanitize())); dropErr != nil { - tb.Logf("cleanup: drop template database %s: %v", templateName, dropErr) - } - tb.Fatal(fmt.Errorf("running migrations on template: %w", err)) - } - - // Mark as template and disallow direct connections. - _, err = conn.ExecContext(ctx, fmt.Sprintf("ALTER DATABASE %s IS_TEMPLATE true", pgx.Identifier{templateName}.Sanitize())) - if err != nil { - tb.Fatal(fmt.Errorf("marking template: %w", err)) - } - _, err = conn.ExecContext(ctx, fmt.Sprintf("ALTER DATABASE %s ALLOW_CONNECTIONS false", pgx.Identifier{templateName}.Sanitize())) - if err != nil { - tb.Fatal(fmt.Errorf("disallowing connections to template: %w", err)) - } - } - } + var exists bool + err = conn.QueryRowContext( + ctx, + "SELECT EXISTS(SELECT 1 FROM pg_database WHERE datname = $1 AND datistemplate = true)", + templateName, + ).Scan(&exists) + if err != nil { + tb.Fatal(fmt.Errorf("checking template existence: %w", err)) + } - templateMu.Lock() - templatesCreated[hash] = true - templateMu.Unlock() + if exists { + return } - // Create the per-test database from the template. - testDbName := "test" + base.ID()[0:26] - _, err = conn.ExecContext(ctx, fmt.Sprintf("CREATE DATABASE %s TEMPLATE %s", - pgx.Identifier{testDbName}.Sanitize(), - pgx.Identifier{templateName}.Sanitize())) + _, err = conn.ExecContext(ctx, fmt.Sprintf("CREATE DATABASE %s", pgx.Identifier{templateName}.Sanitize())) if err != nil { - tb.Fatal(fmt.Errorf("creating test database from template: %w", err)) + tb.Fatal(fmt.Errorf("creating template database: %w", err)) } - dropFn := func() { - _, err := adminDb.Exec(fmt.Sprintf("DROP DATABASE %s", pgx.Identifier{testDbName}.Sanitize())) - if err != nil { - tb.Logf("cleanup: drop database %s: %v", testDbName, err) - } - if err := adminDb.Close(); err != nil { - tb.Logf("cleanup: close admin database: %v", err) + templateCfg := database.DatabaseConfig{ + DatabaseName: templateName, + Postgres: cfg.Postgres, + } + if err := database.RunMigrations(logger, templateCfg, database.WithEmbeddedMigrations(migrations)); err != nil { + if _, dropErr := conn.ExecContext(ctx, fmt.Sprintf("DROP DATABASE IF EXISTS %s", pgx.Identifier{templateName}.Sanitize())); dropErr != nil { + tb.Logf("cleanup: drop template database %s: %v", templateName, dropErr) } + tb.Fatal(fmt.Errorf("running migrations on template: %w", err)) } - cfg.DatabaseName = testDbName - return cfg, dropFn + _, err = conn.ExecContext(ctx, fmt.Sprintf("ALTER DATABASE %s IS_TEMPLATE true", pgx.Identifier{templateName}.Sanitize())) + if err != nil { + tb.Fatal(fmt.Errorf("marking template: %w", err)) + } + _, err = conn.ExecContext(ctx, fmt.Sprintf("ALTER DATABASE %s ALLOW_CONNECTIONS false", pgx.Identifier{templateName}.Sanitize())) + if err != nil { + tb.Fatal(fmt.Errorf("disallowing connections to template: %w", err)) + } } // openAdminDb connects to an admin database for managing templates.