496 lines
28 KiB
Markdown
496 lines
28 KiB
Markdown
# Phase 2: Backend Refactor - Research
|
|
|
|
**Researched:** 2026-03-23
|
|
**Domain:** Go interface extraction, dependency injection, golang-migrate with modernc.org/sqlite
|
|
**Confidence:** HIGH
|
|
|
|
## Summary
|
|
|
|
Phase 2 replaces three package-level globals (`db`, `mu`, `webhookSecret`) with a `Server` struct that holds a `Store` interface. HTTP handlers become methods on `Server`. SQL is extracted from handlers into named `Store` methods with a concrete `SQLiteStore` implementation. Schema management moves to versioned SQL migration files run by `golang-migrate/v4` at startup via `embed.FS`.
|
|
|
|
The change is purely structural. No API contracts, no HTTP status codes, no SQL query semantics change. The test suite must pass before the phase is complete. Tests currently rely on `export_test.go` helpers (`UpdatesReset`, `GetUpdatesMap`, `ResetTags`, `ResetWebhookSecret`) that call package-level functions directly — these must be redesigned to work against the new `Server`/`Store` seam.
|
|
|
|
The critical library constraint is that `golang-migrate/v4/database/sqlite` (not `database/sqlite3`) uses `modernc.org/sqlite` — the same pure-Go driver already in use. This is the only migration path that avoids introducing CGO.
|
|
|
|
**Primary recommendation:** Extract a `Store` interface with one method per logical operation, implement `SQLiteStore` backed by `*sql.DB`, replace globals with a `Server` struct holding `Store` and `webhookSecret`, move all DDL to embedded SQL files under `migrations/sqlite/`, run migrations on startup via `golang-migrate/v4`.
|
|
|
|
<user_constraints>
|
|
## User Constraints (from CONTEXT.md)
|
|
|
|
No CONTEXT.md exists for this phase. Constraints are drawn from CLAUDE.md and STATE.md decisions.
|
|
|
|
### Locked Decisions (from STATE.md Accumulated Context)
|
|
- Backend refactor must be behavior-neutral — all existing tests must pass before PostgreSQL is introduced
|
|
- No ORM or query builder — raw SQL per store implementation; 8 operations across 3 tables is too small to justify a dependency
|
|
- `DATABASE_URL` present activates PostgreSQL; absent falls back to SQLite with `DB_PATH` — no separate `DB_DRIVER` variable (deferred to Phase 3; Store interface must accommodate it)
|
|
|
|
### Claude's Discretion
|
|
- Internal file layout within `pkg/diunwebhook/` and new sub-packages (e.g., `store/`)
|
|
- Migration file naming convention within the chosen scheme
|
|
- Whether `Server` lives in the same package as `Store` or a separate one
|
|
|
|
### Deferred Ideas (OUT OF SCOPE for Phase 2)
|
|
- PostgreSQL implementation of `Store` (Phase 3)
|
|
- Any new API endpoints or behavioral changes
|
|
- DATABASE_URL env var routing (Phase 3)
|
|
</user_constraints>
|
|
|
|
<phase_requirements>
|
|
## Phase Requirements
|
|
|
|
| ID | Description | Research Support |
|
|
|----|-------------|------------------|
|
|
| REFAC-01 | Database operations are behind a Store interface with separate SQLite and PostgreSQL implementations | Store interface design, SQLiteStore struct with `*sql.DB`, method inventory below |
|
|
| REFAC-02 | Package-level global state (db, mu, webhookSecret) is replaced with a Server struct that holds dependencies | Server struct pattern, handler-as-method pattern, export_test.go redesign |
|
|
| REFAC-03 | Schema migrations use golang-migrate with separate migration directories per dialect (sqlite/, postgres/) | golang-migrate v4.19.1, `database/sqlite` sub-package uses modernc.org/sqlite, iofs embed.FS source |
|
|
</phase_requirements>
|
|
|
|
---
|
|
|
|
## Standard Stack
|
|
|
|
### Core
|
|
| Library | Version | Purpose | Why Standard |
|
|
|---------|---------|---------|--------------|
|
|
| `github.com/golang-migrate/migrate/v4` | v4.19.1 | Versioned schema migrations | De-facto standard in Go; supports multiple DB drivers; iofs source enables single-binary deploy |
|
|
| `github.com/golang-migrate/migrate/v4/database/sqlite` | v4.19.1 (same module) | golang-migrate driver for modernc.org/sqlite | Only non-CGO sqlite driver in golang-migrate; uses pure-Go modernc.org/sqlite |
|
|
| `github.com/golang-migrate/migrate/v4/source/iofs` | v4.19.1 (same module) | Read migrations from embed.FS | Keeps migrations bundled in the binary — required for single-binary Docker deploy |
|
|
|
|
**Note on sqlite sub-package:** Use `database/sqlite` (NOT `database/sqlite3`). The `sqlite3` sub-package requires CGO via `mattn/go-sqlite3`, which violates the project's no-CGO constraint. Verified against pkg.go.dev documentation.
|
|
|
|
### Supporting (already in go.mod — no new additions for the Store/Server pattern)
|
|
| Library | Version | Purpose | When to Use |
|
|
|---------|---------|---------|-------------|
|
|
| `modernc.org/sqlite` | v1.46.1 (current) | Pure-Go SQLite driver | Already present; imported as `_ "modernc.org/sqlite"` for side-effect registration |
|
|
| Go stdlib `sync` | — | `sync.Mutex` inside SQLiteStore | Mutex moves from package-level to a field on SQLiteStore |
|
|
| Go stdlib `embed` | — | `//go:embed` for migration files | Embed SQL files into compiled binary |
|
|
|
|
### Alternatives Considered
|
|
| Instead of | Could Use | Tradeoff |
|
|
|------------|-----------|----------|
|
|
| `golang-migrate` iofs source | Raw DDL in `InitDB` (current) | Current approach blocks versioned migrations and PostgreSQL parity; golang-migrate handles ordering, locking, and checksums |
|
|
| `database/sqlite` sub-package | `database/sqlite3` | `sqlite3` requires CGO — forbidden by project constraint |
|
|
| Handler methods on `Server` | Function closures over `Server` | Methods are idiomatic Go, simpler to test, consistent with `net/http` handler signature `func(w, r)` via thin wrapper |
|
|
|
|
**Installation (new dependencies only):**
|
|
```bash
|
|
go get github.com/golang-migrate/migrate/v4@v4.19.1
|
|
go get github.com/golang-migrate/migrate/v4/database/sqlite
|
|
go get github.com/golang-migrate/migrate/v4/source/iofs
|
|
```
|
|
|
|
**Version verification:** `v4.19.1` confirmed via Go module proxy (`proxy.golang.org`) on 2026-03-23. Published 2025-11-29.
|
|
|
|
---
|
|
|
|
## Architecture Patterns
|
|
|
|
### Recommended Project Structure
|
|
|
|
```
|
|
pkg/diunwebhook/
|
|
├── diunwebhook.go # Types (DiunEvent, UpdateEntry, Tag), Server struct, handler methods
|
|
├── store.go # Store interface definition
|
|
├── sqlite_store.go # SQLiteStore — concrete implementation
|
|
├── migrate.go # RunMigrations() using golang-migrate + iofs
|
|
├── export_test.go # Test-only helpers (redesigned for Server/Store)
|
|
├── diunwebhook_test.go # Handler tests (unchanged HTTP assertions)
|
|
└── migrations/
|
|
└── sqlite/
|
|
├── 0001_initial_schema.up.sql
|
|
├── 0001_initial_schema.down.sql
|
|
└── 0002_add_acknowledged_at.up.sql # baseline migration for existing acknowledged_at column
|
|
|
|
cmd/diunwebhook/
|
|
└── main.go # Constructs SQLiteStore, calls RunMigrations, builds Server, registers routes
|
|
```
|
|
|
|
**Why keep everything in `pkg/diunwebhook/`:** CLAUDE.md says "No barrel files; single source file" — this phase is allowed to split into multiple files within the same package to keep things navigable, but a new sub-package is not required. All existing import paths (`awesomeProject/pkg/diunwebhook`) stay valid.
|
|
|
|
### Pattern 1: Store Interface
|
|
|
|
**What:** A Go interface that names every persistence operation the HTTP handlers need. One method per logical operation. No `*sql.DB` in the interface — callers never see the database type.
|
|
|
|
**When to use:** Always, for all DB access from handlers.
|
|
|
|
```go
|
|
// store.go
|
|
type Store interface {
|
|
UpsertEvent(event DiunEvent) error
|
|
GetUpdates() (map[string]UpdateEntry, error)
|
|
AcknowledgeUpdate(image string) (found bool, err error)
|
|
ListTags() ([]Tag, error)
|
|
CreateTag(name string) (Tag, error)
|
|
DeleteTag(id int) (found bool, err error)
|
|
AssignTag(image string, tagID int) error
|
|
UnassignTag(image string) error
|
|
TagExists(id int) (bool, error)
|
|
}
|
|
```
|
|
|
|
**Method count:** 9 methods covering all current SQL operations across `updates`, `tags`, and `tag_assignments`. Each method maps 1:1 to a logical DB operation that currently appears inline in a handler or in `UpdateEvent`/`GetUpdates`.
|
|
|
|
### Pattern 2: SQLiteStore
|
|
|
|
**What:** Concrete struct holding `*sql.DB` and `sync.Mutex`. Implements every method on `Store`. All SQL currently in handlers moves here.
|
|
|
|
```go
|
|
// sqlite_store.go
|
|
type SQLiteStore struct {
|
|
db *sql.DB
|
|
mu sync.Mutex
|
|
}
|
|
|
|
func NewSQLiteStore(db *sql.DB) *SQLiteStore {
|
|
return &SQLiteStore{db: db}
|
|
}
|
|
|
|
func (s *SQLiteStore) UpsertEvent(event DiunEvent) error {
|
|
s.mu.Lock()
|
|
defer s.mu.Unlock()
|
|
_, err := s.db.Exec(`INSERT INTO updates (...) ON CONFLICT ...`, ...)
|
|
return err
|
|
}
|
|
```
|
|
|
|
**Key:** The mutex moves from a package global `var mu sync.Mutex` to a `SQLiteStore` field. This enables parallel tests (each test gets its own `SQLiteStore` with its own in-memory DB).
|
|
|
|
### Pattern 3: Server Struct
|
|
|
|
**What:** Holds the `Store` interface and `webhookSecret`. Handler methods hang off `Server`. `main.go` constructs it and registers routes.
|
|
|
|
```go
|
|
// diunwebhook.go
|
|
type Server struct {
|
|
store Store
|
|
webhookSecret string
|
|
}
|
|
|
|
func NewServer(store Store, webhookSecret string) *Server {
|
|
return &Server{store: store, webhookSecret: webhookSecret}
|
|
}
|
|
|
|
func (s *Server) WebhookHandler(w http.ResponseWriter, r *http.Request) { ... }
|
|
func (s *Server) UpdatesHandler(w http.ResponseWriter, r *http.Request) { ... }
|
|
// ... etc
|
|
```
|
|
|
|
**Route registration in main.go:**
|
|
```go
|
|
srv := diun.NewServer(store, secret)
|
|
mux.HandleFunc("/webhook", srv.WebhookHandler)
|
|
mux.HandleFunc("/api/updates/", srv.DismissHandler)
|
|
// ...
|
|
```
|
|
|
|
### Pattern 4: RunMigrations with embed.FS
|
|
|
|
**What:** `RunMigrations(db *sql.DB, dialect string)` uses `golang-migrate/v4` to apply versioned SQL files embedded in the binary. Called from `main.go` before routes are registered.
|
|
|
|
```go
|
|
// migrate.go
|
|
import (
|
|
"embed"
|
|
"github.com/golang-migrate/migrate/v4"
|
|
"github.com/golang-migrate/migrate/v4/database/sqlite"
|
|
"github.com/golang-migrate/migrate/v4/source/iofs"
|
|
_ "modernc.org/sqlite"
|
|
)
|
|
|
|
//go:embed migrations/sqlite
|
|
var sqliteMigrations embed.FS
|
|
|
|
func RunMigrations(db *sql.DB) error {
|
|
src, err := iofs.New(sqliteMigrations, "migrations/sqlite")
|
|
if err != nil {
|
|
return err
|
|
}
|
|
driver, err := sqlite.WithInstance(db, &sqlite.Config{})
|
|
if err != nil {
|
|
return err
|
|
}
|
|
m, err := migrate.NewWithInstance("iofs", src, "sqlite", driver)
|
|
if err != nil {
|
|
return err
|
|
}
|
|
if err := m.Up(); err != nil && err != migrate.ErrNoChange {
|
|
return err
|
|
}
|
|
return nil
|
|
}
|
|
```
|
|
|
|
**CRITICAL:** `migrate.ErrNoChange` is not an error — it means all migrations already applied. Must not treat it as failure.
|
|
|
|
### Pattern 5: export_test.go Redesign
|
|
|
|
**What:** The current `export_test.go` calls package-level functions (`InitDB`, `db.Exec`). After the refactor, these globals are gone. Test helpers must construct a `Server` backed by a `SQLiteStore` using an in-memory DB.
|
|
|
|
```go
|
|
// export_test.go — new design
|
|
package diunwebhook
|
|
|
|
// TestServer constructs a Server with a fresh in-memory SQLiteStore.
|
|
// Used by test files to get a clean server per test.
|
|
func NewTestServer() (*Server, error) {
|
|
db, err := sql.Open("sqlite", ":memory:")
|
|
if err != nil {
|
|
return nil, err
|
|
}
|
|
if err := RunMigrations(db); err != nil {
|
|
return nil, err
|
|
}
|
|
store := NewSQLiteStore(db)
|
|
return NewServer(store, ""), nil
|
|
}
|
|
```
|
|
|
|
Tests that previously called `diun.UpdatesReset()` will call `diun.NewTestServer()` at the start of each test and operate on the returned server instance. Handler tests pass `srv.WebhookHandler` instead of `diun.WebhookHandler`.
|
|
|
|
**Impact on test signatures:** All test functions that currently call package-level handler functions will receive the server as a local variable. `TestMain` simplifies (no global reset needed — each test owns its DB).
|
|
|
|
### Anti-Patterns to Avoid
|
|
|
|
- **Direct SQL in handlers:** After REFAC-01, handlers must call `s.store.SomeMethod(...)` — never `s.store.(*SQLiteStore).db.Exec(...)`. The interface hides the DB type.
|
|
- **Single migration file containing all schema:** `InitDB`'s current DDL represents TWO logical migrations (initial schema + `acknowledged_at` column). These must become two separate numbered files so existing databases do not re-apply the already-applied column addition. Baseline migration (file 0001) represents the state of existing databases; file 0002 adds `acknowledged_at` to represent the already-run ad-hoc migration.
|
|
- **Calling `m.Up()` and treating `ErrNoChange` as fatal:** Always check `err != migrate.ErrNoChange` before returning an error from `RunMigrations`.
|
|
- **Removing `PRAGMA foreign_keys = ON` during refactor:** The SQLite connection setup must still run this pragma. Move it from `InitDB` into `NewSQLiteStore` or the connection-open step in `main.go`.
|
|
- **Replacing `db.SetMaxOpenConns(1)` with nothing:** This setting prevents concurrent write contention in SQLite. It must be preserved on the `*sql.DB` instance passed to `NewSQLiteStore`.
|
|
|
|
---
|
|
|
|
## Don't Hand-Roll
|
|
|
|
| Problem | Don't Build | Use Instead | Why |
|
|
|---------|-------------|-------------|-----|
|
|
| Versioned schema migration | Custom migration runner with version table | `golang-migrate/v4` | Migration ordering, dirty-state detection, locking, and ErrNoChange handling already solved |
|
|
| Embedding SQL files in binary | Copying SQL into string constants | Go `embed.FS` + `iofs` source | Single-binary deploy; embed handles file reading at compile time |
|
|
| Migration down-file generation | Omitting `.down.sql` files | Create stub down files | golang-migrate requires down files exist even if empty to resolve migration history |
|
|
|
|
**Key insight:** The migration machinery looks simple but has multiple edge cases (dirty state after failed migration, concurrent migration race, no-change idempotency). golang-migrate handles all of these.
|
|
|
|
---
|
|
|
|
## Common Pitfalls
|
|
|
|
### Pitfall 1: Wrong sqlite sub-package (CGO contamination)
|
|
**What goes wrong:** Developer imports `github.com/golang-migrate/migrate/v4/database/sqlite3` (the one with the `3`) — this pulls in `mattn/go-sqlite3` which requires CGO. The build succeeds on developer machines with a C compiler but fails in Alpine/cross-compilation.
|
|
**Why it happens:** The two sub-packages have nearly identical names. The `sqlite3` one appears first in search results.
|
|
**How to avoid:** Always import `database/sqlite` (no `3`). Verify with `go mod graph | grep sqlite`.
|
|
**Warning signs:** Build output mentions `gcc` or `cgo`; `go build` fails with "cgo: C compiler not found".
|
|
|
|
### Pitfall 2: ErrNoChange treated as fatal
|
|
**What goes wrong:** `RunMigrations` returns an error when the database is already at the latest migration version, causing every startup after the first to crash.
|
|
**Why it happens:** `m.Up()` returns `migrate.ErrNoChange` (a non-nil error) when no new migrations exist.
|
|
**How to avoid:** `if err := m.Up(); err != nil && !errors.Is(err, migrate.ErrNoChange) { return err }`.
|
|
**Warning signs:** App starts successfully once, crashes with "no change" on every subsequent start.
|
|
|
|
### Pitfall 3: PRAGMA foreign_keys lost during refactor
|
|
**What goes wrong:** The pragma is in `InitDB` which is being deleted. If it is not moved to the connection-open step, foreign key cascades silently stop working. The `TestDeleteTagHandler_CascadesAssignment` test catches this — but only if the pragma is active.
|
|
**Why it happens:** Refactor focuses on interface extraction and forgets the SQLite-specific connection setup.
|
|
**How to avoid:** Set `PRAGMA foreign_keys = ON` immediately after `sql.Open` and before any queries, inside `NewSQLiteStore` or via `sql.DB.Exec` in `main.go`.
|
|
|
|
### Pitfall 4: Migration baseline mismatch with existing databases
|
|
**What goes wrong:** Migration file 0001 creates the `acknowledged_at` column, but existing databases already have it (from the current ad-hoc migration). golang-migrate fails with "column already exists".
|
|
**Why it happens:** The baseline migration (0001) must represent the schema of *new* databases, while the ad-hoc migration (`ALTER TABLE updates ADD COLUMN acknowledged_at TEXT`) already ran on all existing ones.
|
|
**How to avoid:** Two migration files: `0001_initial_schema.up.sql` creates all tables including `acknowledged_at` (for fresh databases). `0002_acknowledged_at.up.sql` is a no-op or empty migration for existing databases that already ran the ALTER TABLE. Actually: since golang-migrate tracks which migrations have run, running 0001 on a new database creates the full schema; it is never run on an existing database that has already been opened by the old binary. The schema_migrations table created by golang-migrate tracks this. **The safe approach:** 0001 creates all three tables with `acknowledged_at` included from the start. Old databases that pre-exist migration tracking will need to have golang-migrate's `schema_migrations` table bootstrapped, but since `CREATE TABLE IF NOT EXISTS` is used, existing tables are not re-created.
|
|
**Warning signs:** Integration test with a pre-seeded SQLite file fails; startup error "table already exists" or "duplicate column name".
|
|
|
|
### Pitfall 5: export_test.go still references deleted globals
|
|
**What goes wrong:** After removing `var db`, `var mu`, `var webhookSecret`, the `export_test.go` that calls `db.Exec(...)` or `InitDB(":memory:")` directly fails to compile.
|
|
**Why it happens:** export_test.go provides internal access that previously relied on the globals.
|
|
**How to avoid:** Rewrite export_test.go to use `NewTestServer()` (a test-only constructor that returns a fresh `*Server` with in-memory DB). All test helpers become methods on `*Server` or use the public `Store` interface.
|
|
|
|
### Pitfall 6: INSERT OR REPLACE in TagAssignmentHandler
|
|
**What goes wrong:** The current handler uses `INSERT OR REPLACE INTO tag_assignments` — this is correct for SQLite but differs from the `ON CONFLICT DO UPDATE` pattern used in `UpdateEvent`. The `AssignTag` Store method should preserve the working behavior, not silently change semantics.
|
|
**Why it happens:** Developer unifies syntax without checking that both approaches are semantically identical for the tag_assignments table.
|
|
**How to avoid:** Keep `INSERT OR REPLACE` in `SQLiteStore.AssignTag` (it is correct — tag_assignments has `image` as PRIMARY KEY so REPLACE works). Document the intent.
|
|
|
|
---
|
|
|
|
## Code Examples
|
|
|
|
### Store interface (verified pattern)
|
|
```go
|
|
// Source: project-derived from current diunwebhook.go SQL operations audit
|
|
|
|
type Store interface {
|
|
UpsertEvent(event DiunEvent) error
|
|
GetUpdates() (map[string]UpdateEntry, error)
|
|
AcknowledgeUpdate(image string) (found bool, err error)
|
|
ListTags() ([]Tag, error)
|
|
CreateTag(name string) (Tag, error)
|
|
DeleteTag(id int) (found bool, err error)
|
|
AssignTag(image string, tagID int) error
|
|
UnassignTag(image string) error
|
|
TagExists(id int) (bool, error)
|
|
}
|
|
```
|
|
|
|
### golang-migrate with embed.FS + modernc/sqlite (verified against pkg.go.dev)
|
|
```go
|
|
// Source: pkg.go.dev/github.com/golang-migrate/migrate/v4/source/iofs
|
|
|
|
//go:embed migrations/sqlite
|
|
var sqliteMigrations embed.FS
|
|
|
|
func RunMigrations(db *sql.DB) error {
|
|
src, err := iofs.New(sqliteMigrations, "migrations/sqlite")
|
|
if err != nil {
|
|
return err
|
|
}
|
|
driver, err := sqlitemigrate.WithInstance(db, &sqlitemigrate.Config{})
|
|
if err != nil {
|
|
return err
|
|
}
|
|
m, err := migrate.NewWithInstance("iofs", src, "sqlite", driver)
|
|
if err != nil {
|
|
return err
|
|
}
|
|
if err := m.Up(); err != nil && !errors.Is(err, migrate.ErrNoChange) {
|
|
return err
|
|
}
|
|
return nil
|
|
}
|
|
```
|
|
|
|
### Migration file naming convention
|
|
```
|
|
migrations/sqlite/
|
|
0001_initial_schema.up.sql -- CREATE TABLE IF NOT EXISTS updates, tags, tag_assignments
|
|
0001_initial_schema.down.sql -- DROP TABLE tag_assignments; DROP TABLE tags; DROP TABLE updates
|
|
0002_acknowledged_at.up.sql -- (empty or no-op: column exists in 0001 baseline)
|
|
0002_acknowledged_at.down.sql -- (empty)
|
|
```
|
|
|
|
**Note on 0002:** The current `InitDB` has an ad-hoc `ALTER TABLE updates ADD COLUMN acknowledged_at TEXT`. Since 0001 will include `acknowledged_at` in the CREATE TABLE, file 0002 documents the migration history for databases that were created before this field existed but does not need to run anything — it can contain only a comment. Alternatively, since this is a greenfield migration setup, 0001 can simply include `acknowledged_at` from the start, making 0002 unnecessary. Single-file baseline (0001 only) is simpler and correct.
|
|
|
|
### Handler method on Server (verified pattern for net/http)
|
|
```go
|
|
// Source: project CLAUDE.md conventions + stdlib net/http
|
|
|
|
func (s *Server) WebhookHandler(w http.ResponseWriter, r *http.Request) {
|
|
if s.webhookSecret != "" {
|
|
auth := r.Header.Get("Authorization")
|
|
if subtle.ConstantTimeCompare([]byte(auth), []byte(s.webhookSecret)) != 1 {
|
|
http.Error(w, "unauthorized", http.StatusUnauthorized)
|
|
return
|
|
}
|
|
}
|
|
if r.Method != http.MethodPost { ... }
|
|
// ...
|
|
if err := s.store.UpsertEvent(event); err != nil {
|
|
log.Printf("WebhookHandler: failed to store event: %v", err)
|
|
http.Error(w, "internal error", http.StatusInternalServerError)
|
|
return
|
|
}
|
|
}
|
|
```
|
|
|
|
---
|
|
|
|
## SQL Operations Inventory
|
|
|
|
All current SQL in `diunwebhook.go` that must move into `SQLiteStore` methods:
|
|
|
|
| Current location | Operation | Store method |
|
|
|-----------------|-----------|--------------|
|
|
| `UpdateEvent()` | UPSERT into `updates` | `UpsertEvent` |
|
|
| `GetUpdates()` | SELECT updates JOIN tags | `GetUpdates` |
|
|
| `DismissHandler` | UPDATE `acknowledged_at` | `AcknowledgeUpdate` |
|
|
| `TagsHandler GET` | SELECT from `tags` | `ListTags` |
|
|
| `TagsHandler POST` | INSERT into `tags` | `CreateTag` |
|
|
| `TagByIDHandler DELETE` | DELETE from `tags` | `DeleteTag` |
|
|
| `TagAssignmentHandler PUT` (check) | SELECT COUNT from `tags` | `TagExists` |
|
|
| `TagAssignmentHandler PUT` (assign) | INSERT OR REPLACE into `tag_assignments` | `AssignTag` |
|
|
| `TagAssignmentHandler DELETE` | DELETE from `tag_assignments` | `UnassignTag` |
|
|
|
|
**Total: 9 Store methods.** All inline SQL moves to `SQLiteStore`. Handlers call `s.store.X(...)` only.
|
|
|
|
---
|
|
|
|
## State of the Art
|
|
|
|
| Old Approach | Current Approach | When Changed | Impact |
|
|
|--------------|------------------|--------------|--------|
|
|
| Ad-hoc DDL in application code | Versioned migration files | golang-migrate has been standard since ~2017 | Migration history tracked; dirty-state recovery available |
|
|
| Package-level globals for DB | Struct-held dependencies | Standard Go since Go 1.0; best practice since ~2016 | Enables parallel tests, multiple instances |
|
|
| CGO SQLite drivers | Pure-Go `modernc.org/sqlite` | ~2020 | No C toolchain needed; Alpine-friendly |
|
|
|
|
**Deprecated/outdated patterns in this codebase:**
|
|
- `var db *sql.DB` (package-level): replaced by `SQLiteStore.db` field
|
|
- `var mu sync.Mutex` (package-level): replaced by `SQLiteStore.mu` field
|
|
- `var webhookSecret string` (package-level): replaced by `Server.webhookSecret` field
|
|
- `SetWebhookSecret()` function: replaced by `NewServer(store, secret)` constructor
|
|
- `InitDB()` function: replaced by `RunMigrations()` + `NewSQLiteStore()`
|
|
- `export_test.go` calling `InitDB(":memory:")`: replaced by `NewTestServer()` constructor
|
|
|
|
---
|
|
|
|
## Open Questions
|
|
|
|
1. **Migration 0001 vs 0001+0002 baseline**
|
|
- What we know: The current schema has `acknowledged_at` added via an ad-hoc migration after initial creation. Two approaches exist: (a) single 0001 migration that creates all tables including `acknowledged_at` from the start; (b) 0001 creates original schema, 0002 adds `acknowledged_at`.
|
|
- What's unclear: Whether any existing deployed databases lack `acknowledged_at`. The code has `_, _ = db.Exec("ALTER TABLE ... ADD COLUMN acknowledged_at TEXT")` which silently ignores errors — meaning every database that ran this code has the column.
|
|
- Recommendation: Use a single 0001 migration with the full current schema (including `acknowledged_at`). Since this is the first time golang-migrate is introduced, all databases are either: (a) new — get full schema from 0001; (b) existing — already have `acknowledged_at`, and since `CREATE TABLE IF NOT EXISTS` is used, 0001 is a no-op for the table structures but creates the `schema_migrations` tracking table. **However**, golang-migrate does not re-run 0001 just because tables exist — it checks `schema_migrations`. On an existing DB with no `schema_migrations` table, golang-migrate will try to run 0001. If 0001 uses `CREATE TABLE IF NOT EXISTS`, it succeeds even when tables exist. This is the safe path.
|
|
|
|
2. **`TagExists` vs inline check in `AssignTag`**
|
|
- What we know: `TagAssignmentHandler` currently does a `SELECT COUNT(*)` before the INSERT. Some designs inline this into `AssignTag` and return an error code when the tag is missing.
|
|
- What's unclear: Whether the `not found` vs `internal error` distinction in the handler is best expressed as a separate `TagExists` call or a sentinel error from `AssignTag`.
|
|
- Recommendation: Keep `TagExists` as a separate method matching the current two-step pattern. This keeps the Store methods simple and the handler logic readable. A future refactor can merge them.
|
|
|
|
---
|
|
|
|
## Environment Availability
|
|
|
|
Step 2.6: SKIPPED — this phase is code/configuration-only. All changes are within the Go module already present. No new external services, CLIs, or runtimes are required beyond the existing Go 1.26 toolchain.
|
|
|
|
---
|
|
|
|
## Project Constraints (from CLAUDE.md)
|
|
|
|
The planner MUST verify all generated plans comply with these directives:
|
|
|
|
| Directive | Source | Applies To |
|
|
|-----------|--------|------------|
|
|
| No CGO — use `modernc.org/sqlite` only | CLAUDE.md Constraints | golang-migrate sub-package selection |
|
|
| Pure Go SQLite driver (`modernc.org/sqlite`) registered as `"sqlite"` | CLAUDE.md Key Dependencies | `sql.Open("sqlite", path)` — never `"sqlite3"` |
|
|
| No ORM or query builder | STATE.md Decisions | All SQLiteStore methods use raw `database/sql` |
|
|
| `go vet` runs in CI; `gofmt` enforced | CLAUDE.md Code Style | All new Go files must be gofmt-compliant |
|
|
| Handler naming pattern: `<Noun>Handler` | CLAUDE.md Naming Patterns | Handler methods on Server keep existing names |
|
|
| Test functions: `Test<FunctionName>_<Scenario>` | CLAUDE.md Naming Patterns | New test functions follow this convention |
|
|
| No barrel files; logic in `diunwebhook.go` | CLAUDE.md Module Design | New files within package are fine; no new packages required |
|
|
| Error messages lowercase: `"internal error"`, `"not found"` | CLAUDE.md Error Handling | Handler error strings must not change |
|
|
| `log.Printf` with handler name prefix on errors | CLAUDE.md Logging | e.g., `"WebhookHandler: failed to store event: %v"` |
|
|
| Single-container Docker deploy | CLAUDE.md Deployment | Migrations must run at startup from embedded files — no external migration tool |
|
|
| Backward compatible — existing SQLite users upgrade without data loss | CLAUDE.md Constraints | Migration 0001 must use `CREATE TABLE IF NOT EXISTS` |
|
|
|
|
---
|
|
|
|
## Sources
|
|
|
|
### Primary (HIGH confidence)
|
|
- `pkg.go.dev/github.com/golang-migrate/migrate/v4` — version v4.19.1 confirmed via Go module proxy on 2026-03-23
|
|
- `pkg.go.dev/github.com/golang-migrate/migrate/v4/database/sqlite` — confirmed uses `modernc.org/sqlite` (pure Go, not CGO)
|
|
- `pkg.go.dev/github.com/golang-migrate/migrate/v4/source/iofs` — `iofs.New(fsys, path)` API signature verified
|
|
- Project source: `pkg/diunwebhook/diunwebhook.go` — complete SQL operations inventory derived from direct code read
|
|
|
|
### Secondary (MEDIUM confidence)
|
|
- `github.com/golang-migrate/migrate/blob/master/database/sqlite/README.md` — confirms modernc.org/sqlite driver and pure-Go status
|
|
|
|
### Tertiary (LOW confidence)
|
|
- WebSearch results on Go Store interface patterns — general patterns verified against known stdlib conventions; no single authoritative source
|
|
|
|
---
|
|
|
|
## Metadata
|
|
|
|
**Confidence breakdown:**
|
|
- Standard stack: HIGH — golang-migrate version confirmed from Go proxy; sqlite sub-package driver verified from pkg.go.dev
|
|
- Architecture (Store interface, Server struct): HIGH — derived directly from auditing current source code; all 9 operations enumerated
|
|
- Migration design: HIGH — iofs API verified; ErrNoChange behavior documented in pkg.go.dev
|
|
- Pitfalls: HIGH — CGO pitfall verified by checking sqlite vs sqlite3 sub-packages; other pitfalls derived from code analysis
|
|
|
|
**Research date:** 2026-03-23
|
|
**Valid until:** 2026-09-23 (golang-migrate is stable; modernc.org/sqlite API is stable)
|