diff --git a/.planning/REQUIREMENTS.md b/.planning/REQUIREMENTS.md index ec9af7b..898f38a 100644 --- a/.planning/REQUIREMENTS.md +++ b/.planning/REQUIREMENTS.md @@ -9,8 +9,8 @@ Requirements for this milestone. Each maps to roadmap phases. ### Data Integrity -- [ ] **DATA-01**: Webhook events use proper UPSERT (ON CONFLICT DO UPDATE) instead of INSERT OR REPLACE, preserving tag assignments when an image receives a new event -- [ ] **DATA-02**: SQLite foreign key enforcement is enabled (PRAGMA foreign_keys = ON) so tag deletion properly cascades to tag assignments +- [x] **DATA-01**: Webhook events use proper UPSERT (ON CONFLICT DO UPDATE) instead of INSERT OR REPLACE, preserving tag assignments when an image receives a new event +- [x] **DATA-02**: SQLite foreign key enforcement is enabled (PRAGMA foreign_keys = ON) so tag deletion properly cascades to tag assignments - [ ] **DATA-03**: Webhook and API endpoints enforce request body size limits (e.g., 1MB) to prevent OOM from oversized payloads - [ ] **DATA-04**: Test error handling uses t.Fatal instead of silent returns, so test failures are never swallowed @@ -91,8 +91,8 @@ Which phases cover which requirements. Updated during roadmap creation. | Requirement | Phase | Status | |-------------|-------|--------| -| DATA-01 | Phase 1 | Pending | -| DATA-02 | Phase 1 | Pending | +| DATA-01 | Phase 1 | Complete | +| DATA-02 | Phase 1 | Complete | | DATA-03 | Phase 1 | Pending | | DATA-04 | Phase 1 | Pending | | REFAC-01 | Phase 2 | Pending | diff --git a/.planning/ROADMAP.md b/.planning/ROADMAP.md index 0868a02..c96c32a 100644 --- a/.planning/ROADMAP.md +++ b/.planning/ROADMAP.md @@ -31,7 +31,7 @@ Decimal phases appear between their surrounding integers in numeric order. **Plans**: 2 plans Plans: -- [ ] 01-01-PLAN.md — Fix INSERT OR REPLACE → UPSERT in UpdateEvent(); enable PRAGMA foreign_keys = ON in InitDB(); add regression test +- [x] 01-01-PLAN.md — Fix INSERT OR REPLACE → UPSERT in UpdateEvent(); enable PRAGMA foreign_keys = ON in InitDB(); add regression test - [ ] 01-02-PLAN.md — Add http.MaxBytesReader body limits to 3 handlers (413 on oversized); replace 6 silent test returns with t.Fatalf ### Phase 2: Backend Refactor diff --git a/.planning/STATE.md b/.planning/STATE.md index 91b9ed4..0de3326 100644 --- a/.planning/STATE.md +++ b/.planning/STATE.md @@ -1,3 +1,17 @@ +--- +gsd_state_version: 1.0 +milestone: v1.0 +milestone_name: milestone +status: Ready to execute +stopped_at: Completed 01-01-PLAN.md (UPSERT + FK enforcement) +last_updated: "2026-03-23T20:16:27.362Z" +progress: + total_phases: 4 + completed_phases: 0 + total_plans: 2 + completed_plans: 1 +--- + # Project State ## Project Reference @@ -5,20 +19,17 @@ See: .planning/PROJECT.md (updated 2026-03-23) **Core value:** Reliable, persistent visibility into which services need updating — data never disappears, and the dashboard is the one place you trust to show the full picture. -**Current focus:** Phase 1 — Data Integrity +**Current focus:** Phase 01 — data-integrity ## Current Position -Phase: 1 of 4 (Data Integrity) -Plan: 0 of ? in current phase -Status: Ready to plan -Last activity: 2026-03-23 — Roadmap created; ready to begin Phase 1 planning - -Progress: [░░░░░░░░░░] 0% +Phase: 01 (data-integrity) — EXECUTING +Plan: 2 of 2 ## Performance Metrics **Velocity:** + - Total plans completed: 0 - Average duration: — - Total execution time: — @@ -30,10 +41,12 @@ Progress: [░░░░░░░░░░] 0% | - | - | - | - | **Recent Trend:** + - Last 5 plans: — - Trend: — *Updated after each plan completion* +| Phase 01 P01 | 2 | 2 tasks | 2 files | ## Accumulated Context @@ -46,6 +59,8 @@ Recent decisions affecting current work: - 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 +- [Phase 01]: Use named-column UPSERT (ON CONFLICT DO UPDATE) to preserve tag_assignments child rows on re-insert +- [Phase 01]: Enable PRAGMA foreign_keys = ON in InitDB() before DDL to activate ON DELETE CASCADE for tag deletion ### Pending Todos @@ -58,6 +73,6 @@ None yet. ## Session Continuity -Last session: 2026-03-23 -Stopped at: Roadmap created; STATE.md initialized +Last session: 2026-03-23T20:16:27.360Z +Stopped at: Completed 01-01-PLAN.md (UPSERT + FK enforcement) Resume file: None diff --git a/.planning/phases/01-data-integrity/01-01-SUMMARY.md b/.planning/phases/01-data-integrity/01-01-SUMMARY.md new file mode 100644 index 0000000..75b7c7e --- /dev/null +++ b/.planning/phases/01-data-integrity/01-01-SUMMARY.md @@ -0,0 +1,81 @@ +--- +phase: 01-data-integrity +plan: "01" +subsystem: backend/storage +tags: [sqlite, bug-fix, data-integrity, upsert, foreign-keys] +dependency_graph: + requires: [] + provides: [DATA-01, DATA-02] + affects: [pkg/diunwebhook/diunwebhook.go] +tech_stack: + added: [] + patterns: [SQLite UPSERT (ON CONFLICT DO UPDATE), PRAGMA foreign_keys = ON] +key_files: + created: [] + modified: + - pkg/diunwebhook/diunwebhook.go + - pkg/diunwebhook/diunwebhook_test.go +decisions: + - "Use named-column INSERT with ON CONFLICT(image) DO UPDATE SET (UPSERT) instead of INSERT OR REPLACE to preserve tag_assignments child rows" + - "Enable PRAGMA foreign_keys = ON immediately after SetMaxOpenConns(1) so all connections (single-connection pool) enforce FK constraints" +metrics: + duration: "2 minutes" + completed_date: "2026-03-23" + tasks_completed: 2 + files_modified: 2 +--- + +# Phase 01 Plan 01: Fix SQLite Data-Destruction Bugs (UPSERT + FK Enforcement) Summary + +**One-liner:** SQLite UPSERT replaces INSERT OR REPLACE to preserve tag_assignments on re-insert, and PRAGMA foreign_keys = ON enables ON DELETE CASCADE for tag deletion. + +## What Was Built + +Fixed two silent data-destruction bugs in the SQLite persistence layer: + +**Bug DATA-01 (INSERT OR REPLACE destroying tags):** SQLite's `INSERT OR REPLACE` is implemented as DELETE + INSERT. The DELETE fired `ON DELETE CASCADE` on `tag_assignments.image`, silently removing the tag assignment every time a new DIUN event arrived for an already-tagged image. Fixed by replacing the statement with a proper UPSERT (`INSERT INTO ... ON CONFLICT(image) DO UPDATE SET`) that only updates the non-key columns, leaving `tag_assignments` untouched. + +**Bug DATA-02 (FK enforcement disabled):** SQLite disables foreign key enforcement by default. `PRAGMA foreign_keys = ON` was never executed, so `ON DELETE CASCADE` on `tag_assignments.tag_id` did not fire when a tag was deleted. Fixed by executing the pragma immediately after `db.SetMaxOpenConns(1)` in `InitDB()`, before any DDL statements. + +## Tasks Completed + +| Task | Name | Commit | Files | +|------|------|--------|-------| +| 1 | Replace INSERT OR REPLACE with UPSERT + add PRAGMA FK enforcement | 7edbaad | pkg/diunwebhook/diunwebhook.go | +| 2 | Add regression test TestUpdateEvent_PreservesTagOnUpsert | e2d388c | pkg/diunwebhook/diunwebhook_test.go | + +## Decisions Made + +1. **Named-column UPSERT over positional INSERT OR REPLACE:** The UPSERT explicitly lists 15 columns and their `excluded.*` counterparts in the DO UPDATE SET clause, making column mapping unambiguous and safe for future schema additions. + +2. **acknowledged_at hardcoded NULL in UPSERT:** The UPSERT sets `acknowledged_at = NULL` unconditionally in both the INSERT and the ON CONFLICT update clause. This ensures a new event always resets the acknowledged state, matching the pre-existing behavior and test expectations. + +3. **PRAGMA placement before DDL:** The FK pragma is placed before all CREATE TABLE statements to ensure the enforcement is active when foreign key relationships are first defined, not just at query time. + +## Deviations from Plan + +None — plan executed exactly as written. + +## Verification Results + +- `grep -c "INSERT OR REPLACE INTO updates" pkg/diunwebhook/diunwebhook.go` → `0` (confirmed) +- `grep -c "PRAGMA foreign_keys = ON" pkg/diunwebhook/diunwebhook.go` → `1` (confirmed) +- `grep -c "ON CONFLICT(image) DO UPDATE SET" pkg/diunwebhook/diunwebhook.go` → `1` (confirmed) +- Full test suite: 29 tests pass, 0 failures, coverage 63.6% +- `TestDismissHandler_ReappearsAfterNewWebhook` — PASS +- `TestDeleteTagHandler_CascadesAssignment` — PASS +- `TestUpdateEvent_PreservesTagOnUpsert` — PASS (new regression test) + +## Known Stubs + +None. + +## Self-Check: PASSED + +Files exist: +- FOUND: pkg/diunwebhook/diunwebhook.go (modified) +- FOUND: pkg/diunwebhook/diunwebhook_test.go (modified, contains TestUpdateEvent_PreservesTagOnUpsert) + +Commits exist: +- FOUND: 7edbaad — fix(01-01): replace INSERT OR REPLACE with UPSERT and enable FK enforcement +- FOUND: e2d388c — test(01-01): add TestUpdateEvent_PreservesTagOnUpsert regression test