From 72322a49c78f6b60fb3918112ca9982d941032fa Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Tue, 29 Sep 2026 05:00:17 +0000 Subject: [PATCH] Move migrations to internal/db/migrations (closes #96) REPO_POLICIES.md puts migrations in internal/db/migrations/ as 000_migration.sql and 001_schema.sql. The two files move there with their contents unchanged. go:embed cannot reach outside its own package, so internal/db/migrations has a small package that embeds them, and internal/database reads them through its FS(). The database package stays where CONVENTIONS.md puts it; moving it would change existing test files in other packages. The version still comes from the filename prefix, so a database that has recorded versions 0 and 1 runs neither again. A new test applies the migrations twice to one database file and checks that the second run applies nothing. Model: opus-5-5 --- TODO.md | 7 +++ internal/database/database.go | 32 ++++++----- internal/database/migrations_internal_test.go | 54 +++++++++++++++++++ .../migrations/000_migration.sql} | 0 .../migrations/001_schema.sql} | 0 internal/db/migrations/migrations.go | 15 ++++++ 6 files changed, 91 insertions(+), 17 deletions(-) create mode 100644 internal/database/migrations_internal_test.go rename internal/{database/schema/000.sql => db/migrations/000_migration.sql} (100%) rename internal/{database/schema/001_initial_schema.sql => db/migrations/001_schema.sql} (100%) create mode 100644 internal/db/migrations/migrations.go diff --git a/TODO.md b/TODO.md index 5c0ff89..8f07fa8 100644 --- a/TODO.md +++ b/TODO.md @@ -30,6 +30,13 @@ exhaustion # Completed Steps +- 2026-09-29 migrations at the path `REPO_POLICIES.md` sets (closes #96): the + migration files moved, contents unchanged, from `internal/database/schema/` + to `internal/db/migrations/` as `000_migration.sql` and `001_schema.sql`; the + `internal/db/migrations` package embeds them and `internal/database` reads + them through its `FS()`; the `internal/database` package itself stays; the + version still comes from the filename prefix, so a database that has recorded + versions 0 and 1 runs neither again. - 2026-09-29 `trusted_proxies` advice and signature padding in `README.md` (closes #150): the login-limit paragraph, the `trusted_proxies` entry and `config.example.yml` say to set `trusted_proxies` to the address pixa sees for diff --git a/internal/database/database.go b/internal/database/database.go index f29560c..ab59f75 100644 --- a/internal/database/database.go +++ b/internal/database/database.go @@ -4,9 +4,9 @@ package database import ( "context" "database/sql" - "embed" "errors" "fmt" + "io/fs" "log/slog" "path/filepath" "sort" @@ -15,14 +15,12 @@ import ( "go.uber.org/fx" "sneak.berlin/go/pixa/internal/config" + "sneak.berlin/go/pixa/internal/db/migrations" "sneak.berlin/go/pixa/internal/logger" _ "modernc.org/sqlite" // SQLite driver registration ) -//go:embed schema/*.sql -var schemaFS embed.FS - // bootstrapVersion is the migration that creates the schema_migrations // table itself. It is applied before the normal migration loop. const bootstrapVersion = 0 @@ -113,29 +111,29 @@ func New(lc fx.Lifecycle, params Params) (*Database, error) { return s, nil } -// collectMigrations reads the embedded schema directory and returns +// collectMigrations reads the embedded migrations directory and returns // migration filenames sorted lexicographically. func collectMigrations() ([]string, error) { - entries, err := schemaFS.ReadDir("schema") + entries, err := fs.ReadDir(migrations.FS(), ".") if err != nil { - return nil, fmt.Errorf("failed to read schema directory: %w", err) + return nil, fmt.Errorf("failed to read migrations directory: %w", err) } - var migrations []string + var filenames []string for _, entry := range entries { if !entry.IsDir() && strings.HasSuffix(entry.Name(), ".sql") { - migrations = append(migrations, entry.Name()) + filenames = append(filenames, entry.Name()) } } - sort.Strings(migrations) + sort.Strings(filenames) - return migrations, nil + return filenames, nil } // bootstrapMigrationsTable ensures the schema_migrations table exists -// by applying 000.sql if the table is missing. +// by applying 000_migration.sql if the table is missing. func bootstrapMigrationsTable(ctx context.Context, db *sql.DB, log *slog.Logger) error { var tableExists int @@ -150,9 +148,9 @@ func bootstrapMigrationsTable(ctx context.Context, db *sql.DB, log *slog.Logger) return nil } - content, err := schemaFS.ReadFile("schema/000.sql") + content, err := fs.ReadFile(migrations.FS(), "000_migration.sql") if err != nil { - return fmt.Errorf("failed to read bootstrap migration 000.sql: %w", err) + return fmt.Errorf("failed to read bootstrap migration 000_migration.sql: %w", err) } if log != nil { @@ -177,12 +175,12 @@ func ApplyMigrations(ctx context.Context, db *sql.DB, log *slog.Logger) error { return err } - migrations, err := collectMigrations() + filenames, err := collectMigrations() if err != nil { return err } - for _, migration := range migrations { + for _, migration := range filenames { version, parseErr := ParseMigrationVersion(migration) if parseErr != nil { return parseErr @@ -208,7 +206,7 @@ func ApplyMigrations(ctx context.Context, db *sql.DB, log *slog.Logger) error { } // Read and apply migration. - content, readErr := schemaFS.ReadFile(filepath.Join("schema", migration)) + content, readErr := fs.ReadFile(migrations.FS(), migration) if readErr != nil { return fmt.Errorf("failed to read migration %s: %w", migration, readErr) } diff --git a/internal/database/migrations_internal_test.go b/internal/database/migrations_internal_test.go new file mode 100644 index 0000000..6fb7282 --- /dev/null +++ b/internal/database/migrations_internal_test.go @@ -0,0 +1,54 @@ +package database + +import ( + "bytes" + "database/sql" + "log/slog" + "path/filepath" + "strings" + "testing" + + _ "modernc.org/sqlite" // SQLite driver registration +) + +// TestApplyMigrations_SecondRunAppliesNothing applies the migrations twice +// to one database file, as happens when pixad starts again on the database +// it created, and checks that the second run applies none of them. +// ApplyMigrations logs a message starting with "applying" before it runs +// any migration, the bootstrap one included. +func TestApplyMigrations_SecondRunAppliesNothing(t *testing.T) { + t.Parallel() + + ctx := t.Context() + + db, err := sql.Open("sqlite", filepath.Join(t.TempDir(), "state.sqlite3")) + if err != nil { + t.Fatalf("failed to open test db: %v", err) + } + + t.Cleanup(func() { _ = db.Close() }) + + var firstLog bytes.Buffer + + err = ApplyMigrations(ctx, db, slog.New(slog.NewTextHandler(&firstLog, nil))) + if err != nil { + t.Fatalf("first ApplyMigrations failed: %v", err) + } + + if !strings.Contains(firstLog.String(), "applying") { + t.Fatalf("first ApplyMigrations logged no applied migration:\n%s", + firstLog.String()) + } + + var secondLog bytes.Buffer + + err = ApplyMigrations(ctx, db, slog.New(slog.NewTextHandler(&secondLog, nil))) + if err != nil { + t.Fatalf("second ApplyMigrations failed: %v", err) + } + + if strings.Contains(secondLog.String(), "applying") { + t.Errorf("second ApplyMigrations ran a migration again:\n%s", + secondLog.String()) + } +} diff --git a/internal/database/schema/000.sql b/internal/db/migrations/000_migration.sql similarity index 100% rename from internal/database/schema/000.sql rename to internal/db/migrations/000_migration.sql diff --git a/internal/database/schema/001_initial_schema.sql b/internal/db/migrations/001_schema.sql similarity index 100% rename from internal/database/schema/001_initial_schema.sql rename to internal/db/migrations/001_schema.sql diff --git a/internal/db/migrations/migrations.go b/internal/db/migrations/migrations.go new file mode 100644 index 0000000..62402ad --- /dev/null +++ b/internal/db/migrations/migrations.go @@ -0,0 +1,15 @@ +// Package migrations provides the embedded SQL migration files. +package migrations + +import ( + "embed" + "io/fs" +) + +//go:embed *.sql +var files embed.FS + +// FS returns the embedded filesystem containing the migration files. +func FS() fs.FS { + return files +}