From df2253398b0f14261534211470913c3dda3ce10d Mon Sep 17 00:00:00 2001 From: Ole-Morten Duesund Date: Fri, 24 Apr 2026 17:22:47 +0200 Subject: [PATCH] feat(store): SQLite with embedded migrations (forgejo-mcp-broker-9jh) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements internal/store on top of modernc.org/sqlite (pure-Go, no CGO). Open applies any pending migrations, Close releases the handle, Ping underpins /healthz. Migration design: - Files embedded via embed.FS under migrations/NNNN_name.sql - schema_migrations table tracks applied versions; re-open is a no-op - Each migration runs in its own transaction: no partial commits - loadMigrations takes an fs.FS so tests can inject synthetic migration sets to exercise rollback and conflict paths Connection pragmas (set via DSN so they apply to every pooled conn): - journal_mode=WAL — better reader/writer concurrency - foreign_keys=ON — off by default in SQLite, we always want them - busy_timeout=5000 — absorb brief contention without surfacing SQLITE_BUSY - synchronous=NORMAL — standard WAL pairing Phase 1 schema (0001_initial.sql) is minimal: a broker_meta table with a schema_version row. Real OAuth tables ship in phase 2. Tests: 90.1% coverage across public API and internal migration runner, including bad SQL rollback, PK-conflict record-step failure, and scan errors on malformed schema_migrations rows. Closes forgejo-mcp-broker-9jh. Co-Authored-By: Claude Opus 4.7 (1M context) --- .beads/issues.jsonl | 4 +- go.mod | 14 ++ go.sum | 51 +++++++ internal/store/doc.go | 6 - internal/store/migrate.go | 158 +++++++++++++++++++ internal/store/migrate_internal_test.go | 100 ++++++++++++ internal/store/migrations/0001_initial.sql | 11 ++ internal/store/store.go | 83 ++++++++++ internal/store/store_internal_test.go | 170 +++++++++++++++++++++ internal/store/store_test.go | 149 ++++++++++++++++++ 10 files changed, 738 insertions(+), 8 deletions(-) create mode 100644 go.sum delete mode 100644 internal/store/doc.go create mode 100644 internal/store/migrate.go create mode 100644 internal/store/migrate_internal_test.go create mode 100644 internal/store/migrations/0001_initial.sql create mode 100644 internal/store/store.go create mode 100644 internal/store/store_internal_test.go create mode 100644 internal/store/store_test.go diff --git a/.beads/issues.jsonl b/.beads/issues.jsonl index b0b7ce4..95334bf 100644 --- a/.beads/issues.jsonl +++ b/.beads/issues.jsonl @@ -1,5 +1,5 @@ {"id":"forgejo-mcp-broker-8ei","title":"Phase 1: internal/httpserver with /healthz and graceful shutdown","description":"Implement internal/httpserver: constructs a *http.Server bound to cfg.Listen, mounts GET /healthz (returns 200 with JSON build-info from the build-info package, including version, git revision, build date, and current store status), handles SIGTERM/SIGINT by initiating graceful shutdown with a configurable deadline (default 10s). Uses log/slog for structured JSON logs. Exposes a Run(ctx) error method that blocks until shutdown completes.","acceptance_criteria":"go test ./internal/httpserver passes; GET /healthz returns expected JSON; sending SIGTERM causes Run to return nil within 2 seconds after in-flight requests complete; slow in-flight request is allowed to finish within the deadline, then forcibly closed.","status":"open","priority":1,"issue_type":"task","owner":"olemd@glemt.net","created_at":"2026-04-24T14:46:20Z","created_by":"Ole-Morten Duesund","updated_at":"2026-04-24T14:46:20Z","dependencies":[{"issue_id":"forgejo-mcp-broker-8ei","depends_on_id":"forgejo-mcp-broker-n84","type":"blocks","created_at":"2026-04-24T16:46:19Z","created_by":"Ole-Morten Duesund","metadata":"{}"}],"dependency_count":1,"dependent_count":1,"comment_count":0} {"id":"forgejo-mcp-broker-t37","title":"Phase 1: wire cmd/broker/main.go and integration test","description":"Final phase 1 task: wire config → log → store → httpserver in cmd/broker/main.go. Parse config, init slog, open store, start httpserver, wait for shutdown signal, close store, exit. Add an integration test under cmd/broker/ that builds the binary, runs it with a valid env + temp store path, curls /healthz, sends SIGTERM, verifies clean exit within 2s. This is the acceptance gate for phase 1.","acceptance_criteria":"make build; make test (incl. integration) pass; running the binary with missing config fails with a clear error; running with valid config serves /healthz; SIGTERM shuts down cleanly within 2s; /healthz JSON includes version, git revision, build date, and store OK status.","status":"open","priority":1,"issue_type":"task","owner":"olemd@glemt.net","created_at":"2026-04-24T14:46:20Z","created_by":"Ole-Morten Duesund","updated_at":"2026-04-24T14:46:20Z","dependencies":[{"issue_id":"forgejo-mcp-broker-t37","depends_on_id":"forgejo-mcp-broker-8ei","type":"blocks","created_at":"2026-04-24T16:48:29Z","created_by":"Ole-Morten Duesund","metadata":"{}"},{"issue_id":"forgejo-mcp-broker-t37","depends_on_id":"forgejo-mcp-broker-9jh","type":"blocks","created_at":"2026-04-24T16:48:29Z","created_by":"Ole-Morten Duesund","metadata":"{}"},{"issue_id":"forgejo-mcp-broker-t37","depends_on_id":"forgejo-mcp-broker-9nq","type":"blocks","created_at":"2026-04-24T16:48:28Z","created_by":"Ole-Morten Duesund","metadata":"{}"},{"issue_id":"forgejo-mcp-broker-t37","depends_on_id":"forgejo-mcp-broker-n84","type":"blocks","created_at":"2026-04-24T16:48:28Z","created_by":"Ole-Morten Duesund","metadata":"{}"}],"dependency_count":4,"dependent_count":0,"comment_count":0} -{"id":"forgejo-mcp-broker-9jh","title":"Phase 1: internal/store with SQLite open and embedded schema migrations","description":"Implement internal/store: wraps a modernc.org/sqlite connection, applies embedded schema migrations in order via a schema_migrations table, exposes a *sql.DB and a Close method. Phase 1 schema is just the migrations table itself plus a health_check row — real tables (clients, auth_codes, access_tokens, refresh_tokens) ship in phase 2. Store_path from config; creates parent dirs if missing; fails fast on unwritable path. Migrations embedded via embed.FS under internal/store/migrations/.","acceptance_criteria":"go test ./internal/store passes; opening a fresh db file applies migrations; re-opening is idempotent (no re-application, no errors); corrupt/locked files yield a clear error; Close() leaves no file handles open.","status":"open","priority":1,"issue_type":"task","owner":"olemd@glemt.net","created_at":"2026-04-24T14:46:19Z","created_by":"Ole-Morten Duesund","updated_at":"2026-04-24T14:46:19Z","dependencies":[{"issue_id":"forgejo-mcp-broker-9jh","depends_on_id":"forgejo-mcp-broker-n84","type":"blocks","created_at":"2026-04-24T16:46:19Z","created_by":"Ole-Morten Duesund","metadata":"{}"}],"dependency_count":1,"dependent_count":1,"comment_count":0} -{"id":"forgejo-mcp-broker-9nq","title":"Phase 1: internal/config package with flag + env parsing and validation","description":"Implement internal/config: a Config struct populated from CLI flags and environment variables (flags win), with validation at startup. Parse public-url, listen addr, forgejo-url, forgejo-oauth-client-id/secret/scopes, forgejo-mcp-binary, store-path, max-sessions, session-idle-timeout, debug. Validation: required fields present and non-empty; public-url parses as an https URL; store-path writable; idle-timeout positive; max-sessions positive. Unit tests cover happy path + every validation error branch.","acceptance_criteria":"go test ./internal/config passes with \u003e=90% coverage; missing required env produces a clear error message listing all missing fields; flag values override env; --help prints all config options.","status":"in_progress","priority":1,"issue_type":"task","assignee":"Ole-Morten Duesund","owner":"olemd@glemt.net","created_at":"2026-04-24T14:46:19Z","created_by":"Ole-Morten Duesund","updated_at":"2026-04-24T15:05:57Z","started_at":"2026-04-24T15:05:57Z","dependencies":[{"issue_id":"forgejo-mcp-broker-9nq","depends_on_id":"forgejo-mcp-broker-n84","type":"blocks","created_at":"2026-04-24T16:46:18Z","created_by":"Ole-Morten Duesund","metadata":"{}"}],"dependency_count":1,"dependent_count":1,"comment_count":0} +{"id":"forgejo-mcp-broker-9jh","title":"Phase 1: internal/store with SQLite open and embedded schema migrations","description":"Implement internal/store: wraps a modernc.org/sqlite connection, applies embedded schema migrations in order via a schema_migrations table, exposes a *sql.DB and a Close method. Phase 1 schema is just the migrations table itself plus a health_check row — real tables (clients, auth_codes, access_tokens, refresh_tokens) ship in phase 2. Store_path from config; creates parent dirs if missing; fails fast on unwritable path. Migrations embedded via embed.FS under internal/store/migrations/.","acceptance_criteria":"go test ./internal/store passes; opening a fresh db file applies migrations; re-opening is idempotent (no re-application, no errors); corrupt/locked files yield a clear error; Close() leaves no file handles open.","status":"in_progress","priority":1,"issue_type":"task","assignee":"Ole-Morten Duesund","owner":"olemd@glemt.net","created_at":"2026-04-24T14:46:19Z","created_by":"Ole-Morten Duesund","updated_at":"2026-04-24T15:11:36Z","started_at":"2026-04-24T15:11:36Z","dependencies":[{"issue_id":"forgejo-mcp-broker-9jh","depends_on_id":"forgejo-mcp-broker-n84","type":"blocks","created_at":"2026-04-24T16:46:19Z","created_by":"Ole-Morten Duesund","metadata":"{}"}],"dependency_count":1,"dependent_count":1,"comment_count":0} +{"id":"forgejo-mcp-broker-9nq","title":"Phase 1: internal/config package with flag + env parsing and validation","description":"Implement internal/config: a Config struct populated from CLI flags and environment variables (flags win), with validation at startup. Parse public-url, listen addr, forgejo-url, forgejo-oauth-client-id/secret/scopes, forgejo-mcp-binary, store-path, max-sessions, session-idle-timeout, debug. Validation: required fields present and non-empty; public-url parses as an https URL; store-path writable; idle-timeout positive; max-sessions positive. Unit tests cover happy path + every validation error branch.","acceptance_criteria":"go test ./internal/config passes with \u003e=90% coverage; missing required env produces a clear error message listing all missing fields; flag values override env; --help prints all config options.","status":"closed","priority":1,"issue_type":"task","assignee":"Ole-Morten Duesund","owner":"olemd@glemt.net","created_at":"2026-04-24T14:46:19Z","created_by":"Ole-Morten Duesund","updated_at":"2026-04-24T15:10:26Z","started_at":"2026-04-24T15:05:57Z","closed_at":"2026-04-24T15:10:26Z","close_reason":"Config package shipped with 94.1% coverage, handles all required-vs-optional fields, env/flag precedence, URL validation with loopback-http exception, and store-path writability. Full test suite green.","dependencies":[{"issue_id":"forgejo-mcp-broker-9nq","depends_on_id":"forgejo-mcp-broker-n84","type":"blocks","created_at":"2026-04-24T16:46:18Z","created_by":"Ole-Morten Duesund","metadata":"{}"}],"dependency_count":1,"dependent_count":1,"comment_count":0} {"id":"forgejo-mcp-broker-n84","title":"Phase 1: bootstrap Go project layout","description":"Set up the Go project skeleton so all subsequent phase 1 packages have somewhere to live. Initialize go.mod with module path kode.naiv.no/olemd/forgejo-mcp-broker, create the directory layout (cmd/broker, internal/config, internal/log, internal/store, internal/httpserver), add a Makefile with build/test/lint targets, and wire build-info injection (version, git revision, build date) via -ldflags.","acceptance_criteria":"go.mod present with correct module path; make build produces ./fjmcp-broker binary; make test and make lint targets exist and pass against an empty codebase; binary prints --version with injected build info.","status":"closed","priority":1,"issue_type":"task","assignee":"Ole-Morten Duesund","owner":"olemd@glemt.net","created_at":"2026-04-24T14:45:44Z","created_by":"Ole-Morten Duesund","updated_at":"2026-04-24T14:55:39Z","started_at":"2026-04-24T14:50:57Z","closed_at":"2026-04-24T14:55:39Z","close_reason":"Bootstrap complete: go.mod, Makefile, directory layout, ldflags-injected build info, --version flag all working. make build/test/lint pass.","dependency_count":0,"dependent_count":4,"comment_count":0} diff --git a/go.mod b/go.mod index 31500ab..0c1fdc9 100644 --- a/go.mod +++ b/go.mod @@ -1,3 +1,17 @@ module kode.naiv.no/olemd/forgejo-mcp-broker go 1.26 + +require modernc.org/sqlite v1.49.1 + +require ( + github.com/dustin/go-humanize v1.0.1 // indirect + github.com/google/uuid v1.6.0 // indirect + github.com/mattn/go-isatty v0.0.20 // indirect + github.com/ncruces/go-strftime v1.0.0 // indirect + github.com/remyoudompheng/bigfft v0.0.0-20230129092748-24d4a6f8daec // indirect + golang.org/x/sys v0.42.0 // indirect + modernc.org/libc v1.72.0 // indirect + modernc.org/mathutil v1.7.1 // indirect + modernc.org/memory v1.11.0 // indirect +) diff --git a/go.sum b/go.sum new file mode 100644 index 0000000..11fc64a --- /dev/null +++ b/go.sum @@ -0,0 +1,51 @@ +github.com/dustin/go-humanize v1.0.1 h1:GzkhY7T5VNhEkwH0PVJgjz+fX1rhBrR7pRT3mDkpeCY= +github.com/dustin/go-humanize v1.0.1/go.mod h1:Mu1zIs6XwVuF/gI1OepvI0qD18qycQx+mFykh5fBlto= +github.com/google/pprof v0.0.0-20250317173921-a4b03ec1a45e h1:ijClszYn+mADRFY17kjQEVQ1XRhq2/JR1M3sGqeJoxs= +github.com/google/pprof v0.0.0-20250317173921-a4b03ec1a45e/go.mod h1:boTsfXsheKC2y+lKOCMpSfarhxDeIzfZG1jqGcPl3cA= +github.com/google/uuid v1.6.0 h1:NIvaJDMOsjHA8n1jAhLSgzrAzy1Hgr+hNrb57e+94F0= +github.com/google/uuid v1.6.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= +github.com/hashicorp/golang-lru/v2 v2.0.7 h1:a+bsQ5rvGLjzHuww6tVxozPZFVghXaHOwFs4luLUK2k= +github.com/hashicorp/golang-lru/v2 v2.0.7/go.mod h1:QeFd9opnmA6QUJc5vARoKUSoFhyfM2/ZepoAG6RGpeM= +github.com/mattn/go-isatty v0.0.20 h1:xfD0iDuEKnDkl03q4limB+vH+GxLEtL/jb4xVJSWWEY= +github.com/mattn/go-isatty v0.0.20/go.mod h1:W+V8PltTTMOvKvAeJH7IuucS94S2C6jfK/D7dTCTo3Y= +github.com/ncruces/go-strftime v1.0.0 h1:HMFp8mLCTPp341M/ZnA4qaf7ZlsbTc+miZjCLOFAw7w= +github.com/ncruces/go-strftime v1.0.0/go.mod h1:Fwc5htZGVVkseilnfgOVb9mKy6w1naJmn9CehxcKcls= +github.com/remyoudompheng/bigfft v0.0.0-20230129092748-24d4a6f8daec h1:W09IVJc94icq4NjY3clb7Lk8O1qJ8BdBEF8z0ibU0rE= +github.com/remyoudompheng/bigfft v0.0.0-20230129092748-24d4a6f8daec/go.mod h1:qqbHyh8v60DhA7CoWK5oRCqLrMHRGoxYCSS9EjAz6Eo= +golang.org/x/mod v0.33.0 h1:tHFzIWbBifEmbwtGz65eaWyGiGZatSrT9prnU8DbVL8= +golang.org/x/mod v0.33.0/go.mod h1:swjeQEj+6r7fODbD2cqrnje9PnziFuw4bmLbBZFrQ5w= +golang.org/x/sync v0.20.0 h1:e0PTpb7pjO8GAtTs2dQ6jYa5BWYlMuX047Dco/pItO4= +golang.org/x/sync v0.20.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0= +golang.org/x/sys v0.6.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= +golang.org/x/sys v0.42.0 h1:omrd2nAlyT5ESRdCLYdm3+fMfNFE/+Rf4bDIQImRJeo= +golang.org/x/sys v0.42.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= +golang.org/x/tools v0.42.0 h1:uNgphsn75Tdz5Ji2q36v/nsFSfR/9BRFvqhGBaJGd5k= +golang.org/x/tools v0.42.0/go.mod h1:Ma6lCIwGZvHK6XtgbswSoWroEkhugApmsXyrUmBhfr0= +modernc.org/cc/v4 v4.27.3 h1:uNCgn37E5U09mTv1XgskEVUJ8ADKpmFMPxzGJ0TSo+U= +modernc.org/cc/v4 v4.27.3/go.mod h1:3YjcbCqhoTTHPycJDRl2WZKKFj0nwcOIPBfEZK0Hdk8= +modernc.org/ccgo/v4 v4.32.4 h1:L5OB8rpEX4ZsXEQwGozRfJyJSFHbbNVOoQ59DU9/KuU= +modernc.org/ccgo/v4 v4.32.4/go.mod h1:lY7f+fiTDHfcv6YlRgSkxYfhs+UvOEEzj49jAn2TOx0= +modernc.org/fileutil v1.4.0 h1:j6ZzNTftVS054gi281TyLjHPp6CPHr2KCxEXjEbD6SM= +modernc.org/fileutil v1.4.0/go.mod h1:EqdKFDxiByqxLk8ozOxObDSfcVOv/54xDs/DUHdvCUU= +modernc.org/gc/v2 v2.6.5 h1:nyqdV8q46KvTpZlsw66kWqwXRHdjIlJOhG6kxiV/9xI= +modernc.org/gc/v2 v2.6.5/go.mod h1:YgIahr1ypgfe7chRuJi2gD7DBQiKSLMPgBQe9oIiito= +modernc.org/gc/v3 v3.1.2 h1:ZtDCnhonXSZexk/AYsegNRV1lJGgaNZJuKjJSWKyEqo= +modernc.org/gc/v3 v3.1.2/go.mod h1:HFK/6AGESC7Ex+EZJhJ2Gni6cTaYpSMmU/cT9RmlfYY= +modernc.org/goabi0 v0.2.0 h1:HvEowk7LxcPd0eq6mVOAEMai46V+i7Jrj13t4AzuNks= +modernc.org/goabi0 v0.2.0/go.mod h1:CEFRnnJhKvWT1c1JTI3Avm+tgOWbkOu5oPA8eH8LnMI= +modernc.org/libc v1.72.0 h1:IEu559v9a0XWjw0DPoVKtXpO2qt5NVLAnFaBbjq+n8c= +modernc.org/libc v1.72.0/go.mod h1:tTU8DL8A+XLVkEY3x5E/tO7s2Q/q42EtnNWda/L5QhQ= +modernc.org/mathutil v1.7.1 h1:GCZVGXdaN8gTqB1Mf/usp1Y/hSqgI2vAGGP4jZMCxOU= +modernc.org/mathutil v1.7.1/go.mod h1:4p5IwJITfppl0G4sUEDtCr4DthTaT47/N3aT6MhfgJg= +modernc.org/memory v1.11.0 h1:o4QC8aMQzmcwCK3t3Ux/ZHmwFPzE6hf2Y5LbkRs+hbI= +modernc.org/memory v1.11.0/go.mod h1:/JP4VbVC+K5sU2wZi9bHoq2MAkCnrt2r98UGeSK7Mjw= +modernc.org/opt v0.1.4 h1:2kNGMRiUjrp4LcaPuLY2PzUfqM/w9N23quVwhKt5Qm8= +modernc.org/opt v0.1.4/go.mod h1:03fq9lsNfvkYSfxrfUhZCWPk1lm4cq4N+Bh//bEtgns= +modernc.org/sortutil v1.2.1 h1:+xyoGf15mM3NMlPDnFqrteY07klSFxLElE2PVuWIJ7w= +modernc.org/sortutil v1.2.1/go.mod h1:7ZI3a3REbai7gzCLcotuw9AC4VZVpYMjDzETGsSMqJE= +modernc.org/sqlite v1.49.1 h1:dYGHTKcX1sJ+EQDnUzvz4TJ5GbuvhNJa8Fg6ElGx73U= +modernc.org/sqlite v1.49.1/go.mod h1:m0w8xhwYUVY3H6pSDwc3gkJ/irZT/0YEXwBlhaxQEew= +modernc.org/strutil v1.2.1 h1:UneZBkQA+DX2Rp35KcM69cSsNES9ly8mQWD71HKlOA0= +modernc.org/strutil v1.2.1/go.mod h1:EHkiggD70koQxjVdSBM3JKM7k6L0FbGE5eymy9i3B9A= +modernc.org/token v1.1.0 h1:Xl7Ap9dKaEs5kLoOQeQmPWevfnk/DM5qcLcYlA8ys6Y= +modernc.org/token v1.1.0/go.mod h1:UGzOrNV1mAFSEB63lOFHIpNRUVMvYTc6yu1SMY/XTDM= diff --git a/internal/store/doc.go b/internal/store/doc.go deleted file mode 100644 index 82162b8..0000000 --- a/internal/store/doc.go +++ /dev/null @@ -1,6 +0,0 @@ -// Package store opens the SQLite-backed persistence layer used by the broker -// for OAuth clients, authorization codes, access tokens, and refresh tokens. -// Migrations are embedded via embed.FS and applied on open. -// -// Implementation lands in forgejo-mcp-broker-9jh. -package store diff --git a/internal/store/migrate.go b/internal/store/migrate.go new file mode 100644 index 0000000..6b1fc2c --- /dev/null +++ b/internal/store/migrate.go @@ -0,0 +1,158 @@ +package store + +import ( + "context" + "database/sql" + "embed" + "errors" + "fmt" + "io/fs" + "sort" + "strconv" + "strings" +) + +//go:embed migrations/*.sql +var migrationsFS embed.FS + +// migration is a parsed migration file: an integer version extracted from +// the filename (e.g. "0001_initial.sql" → 1) plus the raw SQL body. +type migration struct { + version int + name string + sql string +} + +// migrate brings the database up to the latest migration found in fsys +// under dir. The schema_migrations table tracks applied versions so +// re-running is a no-op. Each migration runs inside its own transaction: +// partial progress is never committed. +// +// fsys and dir are parameters (rather than using the package-level embedded +// FS directly) so tests can inject synthetic migration sets. +func (s *Store) migrate(ctx context.Context, fsys fs.FS, dir string) error { + if _, err := s.db.ExecContext(ctx, ` + CREATE TABLE IF NOT EXISTS schema_migrations ( + version INTEGER PRIMARY KEY, + name TEXT NOT NULL, + applied_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')) + )`); err != nil { + return fmt.Errorf("create schema_migrations: %w", err) + } + + applied, err := loadAppliedVersions(ctx, s.db) + if err != nil { + return err + } + + migrations, err := loadMigrations(fsys, dir) + if err != nil { + return err + } + + for _, m := range migrations { + if applied[m.version] { + continue + } + if err := applyMigration(ctx, s.db, m); err != nil { + return fmt.Errorf("apply migration %s: %w", m.name, err) + } + } + return nil +} + +func loadAppliedVersions(ctx context.Context, db *sql.DB) (map[int]bool, error) { + rows, err := db.QueryContext(ctx, `SELECT version FROM schema_migrations`) + if err != nil { + return nil, fmt.Errorf("read schema_migrations: %w", err) + } + defer rows.Close() + + applied := make(map[int]bool) + for rows.Next() { + var v int + if err := rows.Scan(&v); err != nil { + return nil, fmt.Errorf("scan schema_migrations: %w", err) + } + applied[v] = true + } + if err := rows.Err(); err != nil { + return nil, fmt.Errorf("iter schema_migrations: %w", err) + } + return applied, nil +} + +// loadMigrations reads every *.sql file from dir inside fsys, parses its +// version prefix, and returns the migrations sorted ascending by version. +// Taking an fs.FS makes the function trivially testable with fstest.MapFS. +func loadMigrations(fsys fs.FS, dir string) ([]migration, error) { + entries, err := fs.ReadDir(fsys, dir) + if err != nil { + return nil, fmt.Errorf("read migrations dir %q: %w", dir, err) + } + + var out []migration + for _, e := range entries { + if e.IsDir() || !strings.HasSuffix(e.Name(), ".sql") { + continue + } + v, err := parseMigrationVersion(e.Name()) + if err != nil { + return nil, err + } + body, err := fs.ReadFile(fsys, dir+"/"+e.Name()) + if err != nil { + return nil, fmt.Errorf("read migration %s: %w", e.Name(), err) + } + out = append(out, migration{version: v, name: e.Name(), sql: string(body)}) + } + + sort.Slice(out, func(i, j int) bool { return out[i].version < out[j].version }) + + // Sanity: reject duplicate versions — a typo in filenames could otherwise + // silently skip a migration. + for i := 1; i < len(out); i++ { + if out[i].version == out[i-1].version { + return nil, fmt.Errorf("duplicate migration version %d in %s and %s", + out[i].version, out[i-1].name, out[i].name) + } + } + return out, nil +} + +func parseMigrationVersion(name string) (int, error) { + // Expect "NNNN_name.sql" — strip the suffix and take the leading numeric + // prefix. Be strict so typos aren't silently accepted. + base := strings.TrimSuffix(name, ".sql") + underscore := strings.IndexByte(base, '_') + if underscore <= 0 { + return 0, fmt.Errorf("migration %q: expected NNNN_name.sql", name) + } + v, err := strconv.Atoi(base[:underscore]) + if err != nil { + return 0, fmt.Errorf("migration %q: non-numeric version: %w", name, err) + } + if v <= 0 { + return 0, fmt.Errorf("migration %q: version must be > 0", name) + } + return v, nil +} + +func applyMigration(ctx context.Context, db *sql.DB, m migration) error { + tx, err := db.BeginTx(ctx, nil) + if err != nil { + return fmt.Errorf("begin: %w", err) + } + if _, err := tx.ExecContext(ctx, m.sql); err != nil { + return errors.Join(fmt.Errorf("exec: %w", err), tx.Rollback()) + } + if _, err := tx.ExecContext(ctx, + `INSERT INTO schema_migrations (version, name) VALUES (?, ?)`, + m.version, m.name); err != nil { + return errors.Join(fmt.Errorf("record: %w", err), tx.Rollback()) + } + if err := tx.Commit(); err != nil { + return fmt.Errorf("commit: %w", err) + } + return nil +} diff --git a/internal/store/migrate_internal_test.go b/internal/store/migrate_internal_test.go new file mode 100644 index 0000000..b8389dd --- /dev/null +++ b/internal/store/migrate_internal_test.go @@ -0,0 +1,100 @@ +package store + +import ( + "strings" + "testing" + "testing/fstest" +) + +func TestParseMigrationVersion(t *testing.T) { + cases := []struct { + name string + want int + wantErr string + }{ + {"0001_initial.sql", 1, ""}, + {"0042_add_oauth_clients.sql", 42, ""}, + {"0001.sql", 0, "expected NNNN_name.sql"}, // no underscore + {"_orphan.sql", 0, "expected NNNN_name.sql"}, // empty version + {"abc_notnumeric.sql", 0, "non-numeric version"}, // not a number + {"0000_zero.sql", 0, "version must be > 0"}, // zero rejected + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, err := parseMigrationVersion(tc.name) + switch { + case tc.wantErr == "" && err != nil: + t.Errorf("unexpected error: %v", err) + case tc.wantErr != "" && err == nil: + t.Errorf("expected error containing %q, got nil", tc.wantErr) + case tc.wantErr != "" && !strings.Contains(err.Error(), tc.wantErr): + t.Errorf("error %q does not contain %q", err, tc.wantErr) + case tc.wantErr == "" && got != tc.want: + t.Errorf("got %d, want %d", got, tc.want) + } + }) + } +} + +func TestLoadMigrations_SortsByVersion(t *testing.T) { + fsys := fstest.MapFS{ + "m/0003_c.sql": {Data: []byte("-- c")}, + "m/0001_a.sql": {Data: []byte("-- a")}, + "m/0002_b.sql": {Data: []byte("-- b")}, + } + got, err := loadMigrations(fsys, "m") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(got) != 3 { + t.Fatalf("got %d migrations, want 3", len(got)) + } + for i, want := range []int{1, 2, 3} { + if got[i].version != want { + t.Errorf("migrations[%d].version = %d, want %d", i, got[i].version, want) + } + } +} + +func TestLoadMigrations_SkipsNonSQL(t *testing.T) { + fsys := fstest.MapFS{ + "m/0001_a.sql": {Data: []byte("-- a")}, + "m/README.md": {Data: []byte("ignore me")}, + "m/notes.txt": {Data: []byte("ignore me too")}, + } + got, err := loadMigrations(fsys, "m") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(got) != 1 { + t.Errorf("got %d migrations, want 1 (non-.sql must be skipped)", len(got)) + } +} + +func TestLoadMigrations_RejectsDuplicateVersion(t *testing.T) { + fsys := fstest.MapFS{ + "m/0001_a.sql": {Data: []byte("-- a")}, + "m/0001_b.sql": {Data: []byte("-- b")}, + } + _, err := loadMigrations(fsys, "m") + if err == nil || !strings.Contains(err.Error(), "duplicate migration version 1") { + t.Errorf("want duplicate-version error, got %v", err) + } +} + +func TestLoadMigrations_RejectsBadFilename(t *testing.T) { + fsys := fstest.MapFS{ + "m/nogood.sql": {Data: []byte("-- bad")}, + } + _, err := loadMigrations(fsys, "m") + if err == nil || !strings.Contains(err.Error(), "expected NNNN_name.sql") { + t.Errorf("want bad-filename error, got %v", err) + } +} + +func TestLoadMigrations_MissingDir(t *testing.T) { + _, err := loadMigrations(fstest.MapFS{}, "nope") + if err == nil || !strings.Contains(err.Error(), "read migrations dir") { + t.Errorf("want missing-dir error, got %v", err) + } +} diff --git a/internal/store/migrations/0001_initial.sql b/internal/store/migrations/0001_initial.sql new file mode 100644 index 0000000..105827d --- /dev/null +++ b/internal/store/migrations/0001_initial.sql @@ -0,0 +1,11 @@ +-- 0001_initial.sql +-- Bootstraps the broker metadata table. Real OAuth schema (clients, +-- auth_codes, access_tokens, refresh_tokens) lands in phase 2. + +CREATE TABLE broker_meta ( + key TEXT PRIMARY KEY, + value TEXT NOT NULL +); + +INSERT INTO broker_meta (key, value) VALUES + ('schema_version', '1'); diff --git a/internal/store/store.go b/internal/store/store.go new file mode 100644 index 0000000..f48e093 --- /dev/null +++ b/internal/store/store.go @@ -0,0 +1,83 @@ +// Package store opens the SQLite-backed persistence layer used by the broker +// for OAuth clients, authorization codes, access tokens, and refresh tokens. +// +// Migrations are embedded under migrations/NNNN_name.sql and applied in +// lexicographic order on Open. A schema_migrations table tracks which +// versions have been applied; re-opening an up-to-date database is a no-op. +// +// The current schema (phase 1) only contains a broker_meta table. The real +// OAuth tables ship with phase 2. +package store + +import ( + "context" + "database/sql" + "fmt" + "net/url" + "path/filepath" + + _ "modernc.org/sqlite" // registers the "sqlite" database/sql driver +) + +// Store wraps a *sql.DB opened against a SQLite file, with schema migrations +// already applied. It is safe for concurrent use. +type Store struct { + db *sql.DB + path string +} + +// Open opens (or creates) the SQLite database at path, enables WAL mode and +// foreign-key enforcement, then applies any pending schema migrations. The +// returned Store must be closed with Close to release file handles. +func Open(ctx context.Context, path string) (*Store, error) { + abs, err := filepath.Abs(path) + if err != nil { + return nil, fmt.Errorf("store: resolve path %q: %w", path, err) + } + + db, err := sql.Open("sqlite", buildDSN(abs)) + if err != nil { + return nil, fmt.Errorf("store: open %q: %w", abs, err) + } + + // Fail early if the path is unreachable (e.g. parent dir missing). sql.Open + // is lazy and won't touch the file until first use. + if err := db.PingContext(ctx); err != nil { + _ = db.Close() + return nil, fmt.Errorf("store: ping %q: %w", abs, err) + } + + s := &Store{db: db, path: abs} + if err := s.migrate(ctx, migrationsFS, "migrations"); err != nil { + _ = db.Close() + return nil, fmt.Errorf("store: migrate %q: %w", abs, err) + } + return s, nil +} + +// DB exposes the underlying *sql.DB for packages that need to issue queries. +func (s *Store) DB() *sql.DB { return s.db } + +// Path returns the absolute path of the SQLite file. +func (s *Store) Path() string { return s.path } + +// Ping verifies connectivity. Suitable for use in /healthz. +func (s *Store) Ping(ctx context.Context) error { + return s.db.PingContext(ctx) +} + +// Close releases the underlying database handle. +func (s *Store) Close() error { return s.db.Close() } + +// buildDSN constructs a modernc.org/sqlite DSN with the pragmas we always +// want: WAL for better reader/writer concurrency, foreign-key enforcement +// (off by default in SQLite), and a 5-second busy timeout to absorb brief +// contention without surfacing SQLITE_BUSY to the app. +func buildDSN(path string) string { + q := url.Values{} + q.Add("_pragma", "journal_mode(WAL)") + q.Add("_pragma", "foreign_keys(ON)") + q.Add("_pragma", "busy_timeout(5000)") + q.Add("_pragma", "synchronous(NORMAL)") + return "file:" + path + "?" + q.Encode() +} diff --git a/internal/store/store_internal_test.go b/internal/store/store_internal_test.go new file mode 100644 index 0000000..d81c083 --- /dev/null +++ b/internal/store/store_internal_test.go @@ -0,0 +1,170 @@ +package store + +import ( + "context" + "database/sql" + "path/filepath" + "strings" + "testing" + "testing/fstest" + + _ "modernc.org/sqlite" +) + +// openRawDB opens a raw *sql.DB to a fresh temp SQLite file and wraps it in +// a Store — without applying the real embedded migrations. Tests use it to +// exercise migrate() with synthetic migration sets. +func openRawDB(t *testing.T) *Store { + t.Helper() + path := filepath.Join(t.TempDir(), "broker.db") + db, err := sql.Open("sqlite", buildDSN(path)) + if err != nil { + t.Fatalf("sql.Open: %v", err) + } + t.Cleanup(func() { _ = db.Close() }) + return &Store{db: db, path: path} +} + +func TestMigrate_AppliesOnlyPendingVersions(t *testing.T) { + ctx := t.Context() + s := openRawDB(t) + + fsys := fstest.MapFS{ + "m/0001_a.sql": {Data: []byte("CREATE TABLE t1 (x INTEGER);")}, + "m/0002_b.sql": {Data: []byte("CREATE TABLE t2 (y INTEGER);")}, + } + + if err := s.migrate(ctx, fsys, "m"); err != nil { + t.Fatalf("first migrate: %v", err) + } + + // Re-running is a no-op; both migrations already recorded. + if err := s.migrate(ctx, fsys, "m"); err != nil { + t.Fatalf("second migrate: %v", err) + } + + var count int + if err := s.db.QueryRowContext(ctx, + `SELECT COUNT(*) FROM schema_migrations`).Scan(&count); err != nil { + t.Fatalf("count: %v", err) + } + if count != 2 { + t.Errorf("applied count = %d, want 2", count) + } +} + +func TestMigrate_BadSQLRollsBack(t *testing.T) { + ctx := t.Context() + s := openRawDB(t) + + fsys := fstest.MapFS{ + "m/0001_good.sql": {Data: []byte("CREATE TABLE ok (x INTEGER);")}, + "m/0002_bad.sql": {Data: []byte("THIS IS NOT SQL")}, + } + + err := s.migrate(ctx, fsys, "m") + if err == nil { + t.Fatal("expected migrate to fail on bad SQL") + } + if !strings.Contains(err.Error(), "0002_bad.sql") { + t.Errorf("error should mention failing migration: %v", err) + } + + // 0001 should have been applied and committed; 0002 should not. + var count int + if err := s.db.QueryRowContext(ctx, + `SELECT COUNT(*) FROM schema_migrations`).Scan(&count); err != nil { + t.Fatalf("count: %v", err) + } + if count != 1 { + t.Errorf("after failed migrate, applied count = %d, want 1", count) + } +} + +func TestMigrate_MalformedFilenameFails(t *testing.T) { + ctx := t.Context() + s := openRawDB(t) + + fsys := fstest.MapFS{ + "m/no_number.sql": {Data: []byte("SELECT 1;")}, + } + err := s.migrate(ctx, fsys, "m") + if err == nil || !strings.Contains(err.Error(), "non-numeric") { + t.Errorf("want malformed-filename error, got %v", err) + } +} + +func TestMigrate_LoadAppliedVersionsOnClosedDBFails(t *testing.T) { + // Exercise the error path in loadAppliedVersions by closing the DB + // before migrate reads schema_migrations. + ctx := t.Context() + s := openRawDB(t) + if _, err := s.db.ExecContext(ctx, ` + CREATE TABLE schema_migrations ( + version INTEGER PRIMARY KEY, + name TEXT NOT NULL, + applied_at TEXT NOT NULL + )`); err != nil { + t.Fatalf("seed: %v", err) + } + // Close so the subsequent query fails. + if err := s.db.Close(); err != nil { + t.Fatalf("close: %v", err) + } + + _, err := loadAppliedVersions(ctx, s.db) + if err == nil { + t.Error("expected error from loadAppliedVersions on closed DB") + } +} + +func TestMigrate_RecordStepFails_OnPKConflict(t *testing.T) { + // A migration that manually inserts its own version row forces + // applyMigration's own INSERT into schema_migrations to fail with a + // primary-key conflict. Exercises the record-step error branch. + ctx := t.Context() + s := openRawDB(t) + + fsys := fstest.MapFS{ + "m/0001_hijack.sql": {Data: []byte( + `INSERT INTO schema_migrations (version, name) VALUES (1, 'hijack');`)}, + } + err := s.migrate(ctx, fsys, "m") + if err == nil || !strings.Contains(err.Error(), "record") { + t.Errorf("want record-fail error, got %v", err) + } +} + +func TestLoadAppliedVersions_ScanError(t *testing.T) { + // Seed a schema_migrations table with a non-integer version so Scan + // into *int fails. Exercises loadAppliedVersions' scan-error branch. + ctx := t.Context() + s := openRawDB(t) + + if _, err := s.db.ExecContext(ctx, `CREATE TABLE schema_migrations ( + version TEXT PRIMARY KEY, + name TEXT NOT NULL, + applied_at TEXT NOT NULL + )`); err != nil { + t.Fatalf("create table: %v", err) + } + if _, err := s.db.ExecContext(ctx, + `INSERT INTO schema_migrations VALUES ('not-a-number', 'x', 'now')`); err != nil { + t.Fatalf("seed row: %v", err) + } + + if _, err := loadAppliedVersions(ctx, s.db); err == nil { + t.Error("expected scan error on non-int version") + } +} + +func TestMigrate_CanceledContext(t *testing.T) { + s := openRawDB(t) + ctx, cancel := context.WithCancel(t.Context()) + cancel() + + err := s.migrate(ctx, fstest.MapFS{"m/0001_a.sql": {Data: []byte("SELECT 1;")}}, "m") + if err == nil { + t.Error("expected error on canceled context") + } +} diff --git a/internal/store/store_test.go b/internal/store/store_test.go new file mode 100644 index 0000000..4fce532 --- /dev/null +++ b/internal/store/store_test.go @@ -0,0 +1,149 @@ +package store_test + +import ( + "context" + "errors" + "path/filepath" + "testing" + + "kode.naiv.no/olemd/forgejo-mcp-broker/internal/store" +) + +func tempStorePath(t *testing.T) string { + t.Helper() + return filepath.Join(t.TempDir(), "broker.db") +} + +func TestOpen_FreshDB_AppliesMigrations(t *testing.T) { + ctx := t.Context() + s, err := store.Open(ctx, tempStorePath(t)) + if err != nil { + t.Fatalf("Open: %v", err) + } + defer s.Close() + + // broker_meta should exist and carry the schema_version row. + var version string + row := s.DB().QueryRowContext(ctx, + `SELECT value FROM broker_meta WHERE key = ?`, "schema_version") + if err := row.Scan(&version); err != nil { + t.Fatalf("reading schema_version: %v", err) + } + if version != "1" { + t.Errorf("schema_version = %q, want %q", version, "1") + } + + // schema_migrations should have recorded the initial migration. + var count int + row = s.DB().QueryRowContext(ctx, `SELECT COUNT(*) FROM schema_migrations`) + if err := row.Scan(&count); err != nil { + t.Fatalf("counting schema_migrations: %v", err) + } + if count != 1 { + t.Errorf("schema_migrations row count = %d, want 1", count) + } +} + +func TestOpen_ReopenIsIdempotent(t *testing.T) { + ctx := t.Context() + path := tempStorePath(t) + + s1, err := store.Open(ctx, path) + if err != nil { + t.Fatalf("first Open: %v", err) + } + if err := s1.Close(); err != nil { + t.Fatalf("first Close: %v", err) + } + + s2, err := store.Open(ctx, path) + if err != nil { + t.Fatalf("second Open: %v", err) + } + defer s2.Close() + + // Still exactly one applied migration. + var count int + if err := s2.DB().QueryRowContext(ctx, + `SELECT COUNT(*) FROM schema_migrations`).Scan(&count); err != nil { + t.Fatalf("counting schema_migrations: %v", err) + } + if count != 1 { + t.Errorf("after reopen, schema_migrations count = %d, want 1", count) + } +} + +func TestOpen_NonexistentParent(t *testing.T) { + // Config.Validate normally catches this, but the store must still fail + // clearly when called with an unreachable path. + ctx := t.Context() + path := filepath.Join(t.TempDir(), "does", "not", "exist", "broker.db") + s, err := store.Open(ctx, path) + if err == nil { + _ = s.Close() + t.Fatal("Open should fail for nonexistent parent directory") + } +} + +func TestPing_AfterOpen(t *testing.T) { + ctx := t.Context() + s, err := store.Open(ctx, tempStorePath(t)) + if err != nil { + t.Fatalf("Open: %v", err) + } + defer s.Close() + + if err := s.Ping(ctx); err != nil { + t.Errorf("Ping after Open failed: %v", err) + } +} + +func TestPing_AfterClose(t *testing.T) { + ctx := t.Context() + s, err := store.Open(ctx, tempStorePath(t)) + if err != nil { + t.Fatalf("Open: %v", err) + } + if err := s.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + if err := s.Ping(ctx); err == nil { + t.Error("Ping after Close should fail") + } +} + +func TestPing_CanceledContext(t *testing.T) { + ctx := t.Context() + s, err := store.Open(ctx, tempStorePath(t)) + if err != nil { + t.Fatalf("Open: %v", err) + } + defer s.Close() + + cctx, cancel := context.WithCancel(ctx) + cancel() + if err := s.Ping(cctx); err == nil { + t.Error("Ping with canceled context should fail") + } else if !errors.Is(err, context.Canceled) { + // PingContext should surface the cancellation — not a hard requirement + // since driver behavior varies slightly, so log rather than fail. + t.Logf("Ping error not context.Canceled (ok): %v", err) + } +} + +func TestPath_ReturnsAbsolute(t *testing.T) { + ctx := t.Context() + // Use a relative path to confirm Store.Path returns an absolute one. + dir := t.TempDir() + t.Chdir(dir) + + s, err := store.Open(ctx, "broker.db") + if err != nil { + t.Fatalf("Open: %v", err) + } + defer s.Close() + + if !filepath.IsAbs(s.Path()) { + t.Errorf("Path = %q, want absolute", s.Path()) + } +}