docs(phase-01): research data integrity phase
Investigates SQLite UPSERT semantics, FK enforcement per-connection requirement, http.MaxBytesReader behavior, and t.Fatal test patterns. All four DATA-0x bugs confirmed with authoritative sources and line numbers. No open blockers; ready for planning. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
491
.planning/phases/01-data-integrity/01-RESEARCH.md
Normal file
491
.planning/phases/01-data-integrity/01-RESEARCH.md
Normal file
@@ -0,0 +1,491 @@
|
||||
# Phase 1: Data Integrity — Research
|
||||
|
||||
**Researched:** 2026-03-23
|
||||
**Domain:** Go / SQLite — UPSERT semantics, FK enforcement, HTTP body limits, test correctness
|
||||
**Confidence:** HIGH (all four bugs confirmed via direct code analysis and authoritative sources)
|
||||
|
||||
---
|
||||
|
||||
## Summary
|
||||
|
||||
Phase 1 fixes four concrete, active bugs in `pkg/diunwebhook/diunwebhook.go` and its test file. None of these changes alter the public API, the database schema, or the HTTP route surface. They are surgical line-level fixes to existing functions.
|
||||
|
||||
Bug 1 (DATA-01) is the most damaging: `INSERT OR REPLACE` in `UpdateEvent()` at line 109 performs a DELETE + INSERT on conflict, which cascades to delete any `tag_assignments` row that references the image being updated. Every new DIUN event for an already-tagged image silently destroys the tag. The fix is a one-statement replacement: `INSERT INTO updates (...) VALUES (...) ON CONFLICT(image) DO UPDATE SET ...` using the `excluded.` qualifier for new values.
|
||||
|
||||
Bug 2 (DATA-02) is directly related: even with the UPSERT fix in place, the `ON DELETE CASCADE` constraint on `tag_assignments.tag_id` cannot fire during a tag delete because `PRAGMA foreign_keys = ON` is never executed. SQLite disables FK enforcement by default at the connection level. The fix is one `db.Exec` call immediately after `sql.Open` in `InitDB()`. Since the codebase already uses `db.SetMaxOpenConns(1)`, the single-connection constraint makes this safe without needing DSN parameters or connection hooks.
|
||||
|
||||
Bug 3 (DATA-03) is a security/reliability issue: `json.NewDecoder(r.Body).Decode(&event)` in `WebhookHandler` reads an unbounded body. The fix is `r.Body = http.MaxBytesReader(w, r.Body, 1<<20)` before the decode, plus an `errors.As(err, &maxBytesError)` check in the decode error path to return 413. The same pattern applies to the POST body in `TagsHandler` and the PUT/DELETE body in `TagAssignmentHandler`.
|
||||
|
||||
Bug 4 (DATA-04) is in the test file: six call sites use `if err != nil { return }` instead of `t.Fatalf(...)`, causing test setup failures to appear as passing tests. These are pure test-file changes with no production impact.
|
||||
|
||||
**Primary recommendation:** Fix all four bugs in order (DATA-01 through DATA-04) as separate commits. Each fix is independent and can be verified by its own targeted test.
|
||||
|
||||
---
|
||||
|
||||
## Project Constraints (from CLAUDE.md)
|
||||
|
||||
| Directive | Category |
|
||||
|-----------|----------|
|
||||
| No CGO — uses `modernc.org/sqlite` (pure Go) | Dependency constraint |
|
||||
| Run tests: `go test -v -coverprofile=coverage.out -coverpkg=./... ./...` | Test command |
|
||||
| Run single test: `go test -v -run TestWebhookHandler ./pkg/diunwebhook/` | Test command |
|
||||
| CI warns (but does not fail) when coverage drops below 80% | Coverage policy |
|
||||
| No ORM or query builder — raw SQL only | SQL constraint |
|
||||
| Module name is `awesomeProject` — do not rename in this phase | Scope constraint |
|
||||
|
||||
---
|
||||
|
||||
<phase_requirements>
|
||||
## Phase Requirements
|
||||
|
||||
| ID | Description | Research Support |
|
||||
|----|-------------|------------------|
|
||||
| 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 | SQLite 3.24+ UPSERT syntax confirmed; `excluded.` qualifier for column update values documented; fix is line 109 of diunwebhook.go |
|
||||
| DATA-02 | SQLite foreign key enforcement is enabled (PRAGMA foreign_keys = ON) so tag deletion properly cascades to tag assignments | FK enforcement is per-connection; with SetMaxOpenConns(1) a single db.Exec after Open is sufficient; modernc.org/sqlite also supports DSN `_pragma=foreign_keys(1)` as a future-proof alternative |
|
||||
| DATA-03 | Webhook and API endpoints enforce request body size limits (e.g., 1MB) to prevent OOM from oversized payloads | `http.MaxBytesReader` wraps r.Body before decode; `errors.As(err, &maxBytesError)` detects limit exceeded; caller must explicitly return 413 — the reader does not set it automatically |
|
||||
| DATA-04 | Test error handling uses t.Fatal instead of silent returns, so test failures are never swallowed | Six call sites identified in diunwebhook_test.go (lines 38-40, 153-154, 228-231, 287-289, 329-331, 350-351); all follow the same `if err != nil { return }` pattern |
|
||||
</phase_requirements>
|
||||
|
||||
---
|
||||
|
||||
## Standard Stack
|
||||
|
||||
### Core (no new dependencies required)
|
||||
|
||||
| Library | Version | Purpose | Why Standard |
|
||||
|---------|---------|---------|--------------|
|
||||
| `modernc.org/sqlite` | v1.46.1 (current) | SQLite driver (pure Go, no CGO) | Already used; UPSERT and PRAGMA support confirmed |
|
||||
| `database/sql` | stdlib | SQL connection and query interface | Already used |
|
||||
| `net/http` | stdlib | `http.MaxBytesReader`, `http.MaxBytesError` | Available since Go 1.19; Go module specifies 1.26 |
|
||||
| `errors` | stdlib | `errors.As` for typed error detection | Already imported in test file |
|
||||
|
||||
No new `go.mod` entries are needed for this phase. All required functionality is in the existing standard library and the already-present `modernc.org/sqlite` driver.
|
||||
|
||||
### Alternatives Considered
|
||||
|
||||
| Instead of | Could Use | Tradeoff |
|
||||
|------------|-----------|----------|
|
||||
| `db.Exec("PRAGMA foreign_keys = ON")` after Open | DSN `?_pragma=foreign_keys(1)` | DSN approach applies to every future connection including pooled ones; direct Exec is sufficient given `SetMaxOpenConns(1)` but DSN is more robust if pooling ever changes |
|
||||
| `errors.As(err, &maxBytesError)` | `strings.Contains(err.Error(), "http: request body too large")` | String matching is fragile and not API-stable; `errors.As` with `*http.MaxBytesError` is the documented pattern |
|
||||
|
||||
---
|
||||
|
||||
## Architecture Patterns
|
||||
|
||||
### Existing Code Structure (not changing in Phase 1)
|
||||
|
||||
Phase 1 does NOT restructure the package. All fixes are line-level edits within the existing `pkg/diunwebhook/diunwebhook.go` and `pkg/diunwebhook/diunwebhook_test.go` files. The package-level global state, handler functions, and overall architecture are left for Phase 2.
|
||||
|
||||
### Pattern 1: SQLite UPSERT with excluded. qualifier
|
||||
|
||||
**What:** Replace `INSERT OR REPLACE INTO updates VALUES (...)` with a proper UPSERT that only updates event fields, never touching the row's relationship to `tag_assignments`.
|
||||
|
||||
**When to use:** Any time an INSERT must update an existing row without deleting it — which is the always-correct choice when foreign key children must survive.
|
||||
|
||||
**Why INSERT OR REPLACE is wrong:** SQLite implements `INSERT OR REPLACE` as DELETE + INSERT. The DELETE fires the `ON DELETE CASCADE` on `tag_assignments.image`, destroying the child row. Even if FK enforcement is OFF, the row is physically deleted and reinserted with a new rowid, making the FK relationship stale.
|
||||
|
||||
**Example:**
|
||||
```go
|
||||
// Source: https://sqlite.org/lang_upsert.html
|
||||
// Replace line 109 in UpdateEvent():
|
||||
_, err := db.Exec(`
|
||||
INSERT INTO updates (
|
||||
image, diun_version, hostname, status, provider,
|
||||
hub_link, mime_type, digest, created, platform,
|
||||
ctn_name, ctn_id, ctn_state, ctn_status,
|
||||
received_at, acknowledged_at
|
||||
) VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,NULL)
|
||||
ON CONFLICT(image) DO UPDATE SET
|
||||
diun_version = excluded.diun_version,
|
||||
hostname = excluded.hostname,
|
||||
status = excluded.status,
|
||||
provider = excluded.provider,
|
||||
hub_link = excluded.hub_link,
|
||||
mime_type = excluded.mime_type,
|
||||
digest = excluded.digest,
|
||||
created = excluded.created,
|
||||
platform = excluded.platform,
|
||||
ctn_name = excluded.ctn_name,
|
||||
ctn_id = excluded.ctn_id,
|
||||
ctn_state = excluded.ctn_state,
|
||||
ctn_status = excluded.ctn_status,
|
||||
received_at = excluded.received_at,
|
||||
acknowledged_at = NULL`,
|
||||
event.Image, event.DiunVersion, ...
|
||||
)
|
||||
```
|
||||
|
||||
Key points:
|
||||
- `excluded.column_name` refers to the value that would have been inserted (the new value)
|
||||
- `acknowledged_at = NULL` explicitly resets the acknowledged state on each new event — this matches the test `TestDismissHandler_ReappearsAfterNewWebhook`
|
||||
- `tag_assignments` is untouched because the UPDATE path never deletes the `updates` row
|
||||
|
||||
### Pattern 2: PRAGMA foreign_keys = ON placement
|
||||
|
||||
**What:** Execute `PRAGMA foreign_keys = ON` immediately after `sql.Open`, before any schema creation.
|
||||
|
||||
**When to use:** Every SQLite database that defines FK constraints with `ON DELETE CASCADE`.
|
||||
|
||||
**Why it must be immediate:** SQLite FK enforcement is a connection-level setting, not a database-level setting. It resets to OFF when the connection closes. With `db.SetMaxOpenConns(1)`, there is exactly one connection and it lives for the process lifetime, so one `db.Exec` call is sufficient.
|
||||
|
||||
**Example:**
|
||||
```go
|
||||
// Source: https://sqlite.org/foreignkeys.html
|
||||
// Add in InitDB() after sql.Open, before schema creation:
|
||||
func InitDB(path string) error {
|
||||
var err error
|
||||
db, err = sql.Open("sqlite", path)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
db.SetMaxOpenConns(1)
|
||||
|
||||
// Enable FK enforcement — must be first SQL executed on this connection
|
||||
if _, err = db.Exec(`PRAGMA foreign_keys = ON`); err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
// ... CREATE TABLE IF NOT EXISTS ...
|
||||
}
|
||||
```
|
||||
|
||||
The error from `db.Exec("PRAGMA foreign_keys = ON")` must NOT be swallowed. If the pragma fails (which is extremely unlikely with `modernc.org/sqlite`), returning the error prevents silent misconfiguration.
|
||||
|
||||
**Future-proof alternative (if SetMaxOpenConns(1) is ever removed):**
|
||||
```go
|
||||
db, err = sql.Open("sqlite", path+"?_pragma=foreign_keys(1)")
|
||||
```
|
||||
The `_pragma` DSN parameter in `modernc.org/sqlite` applies the pragma on every new connection, making it pool-safe.
|
||||
|
||||
### Pattern 3: http.MaxBytesReader with typed error detection
|
||||
|
||||
**What:** Wrap `r.Body` before JSON decoding; check for `*http.MaxBytesError` to return 413.
|
||||
|
||||
**When to use:** Any handler that reads a request body from untrusted clients.
|
||||
|
||||
**Example:**
|
||||
```go
|
||||
// Source: https://pkg.go.dev/net/http#MaxBytesReader
|
||||
// Source: https://www.alexedwards.net/blog/how-to-properly-parse-a-json-request-body
|
||||
const maxBodyBytes = 1 << 20 // 1 MB
|
||||
|
||||
func WebhookHandler(w http.ResponseWriter, r *http.Request) {
|
||||
// ... auth check, method check ...
|
||||
|
||||
r.Body = http.MaxBytesReader(w, r.Body, maxBodyBytes)
|
||||
|
||||
var event DiunEvent
|
||||
if err := json.NewDecoder(r.Body).Decode(&event); err != nil {
|
||||
var maxBytesErr *http.MaxBytesError
|
||||
if errors.As(err, &maxBytesErr) {
|
||||
http.Error(w, "request body too large", http.StatusRequestEntityTooLarge)
|
||||
return
|
||||
}
|
||||
log.Printf("WebhookHandler: failed to decode request: %v", err)
|
||||
http.Error(w, "bad request", http.StatusBadRequest)
|
||||
return
|
||||
}
|
||||
// ...
|
||||
}
|
||||
```
|
||||
|
||||
Critical details:
|
||||
- `http.MaxBytesReader` does NOT automatically set the 413 status. The caller must detect `*http.MaxBytesError` via `errors.As` and call `http.Error(w, ..., 413)`.
|
||||
- `maxBodyBytes` should be defined as a package-level constant so all three handlers share the same limit.
|
||||
- Apply to: `WebhookHandler` (POST /webhook), `TagsHandler` POST branch, `TagAssignmentHandler` PUT and DELETE branches.
|
||||
|
||||
### Pattern 4: t.Fatalf in test setup paths
|
||||
|
||||
**What:** Replace `if err != nil { return }` with `t.Fatalf("...: %v", err)` in test setup code.
|
||||
|
||||
**When to use:** Any `t.Test*` function where an error in setup (not the system under test) would make subsequent assertions meaningless.
|
||||
|
||||
**Example:**
|
||||
```go
|
||||
// Before (silently swallows test setup failure — test appears to pass):
|
||||
err := diun.UpdateEvent(event)
|
||||
if err != nil {
|
||||
return
|
||||
}
|
||||
|
||||
// After (test is marked failed, execution stops, CI catches the failure):
|
||||
if err := diun.UpdateEvent(event); err != nil {
|
||||
t.Fatalf("UpdateEvent setup failed: %v", err)
|
||||
}
|
||||
```
|
||||
|
||||
**Distinction from `t.Errorf`:** Use `t.Fatal`/`t.Fatalf` when the test cannot proceed meaningfully after the failure (setup failure). Use `t.Errorf` for the assertion being tested (allows collecting multiple failures in one run).
|
||||
|
||||
### Anti-Patterns to Avoid
|
||||
|
||||
- **`INSERT OR REPLACE` for any table with FK children:** Always use `ON CONFLICT DO UPDATE` when child rows in related tables must survive the conflict resolution.
|
||||
- **`_, _ = db.Exec("PRAGMA ...")`:** Never swallow errors on PRAGMA execution. FK enforcement silently failing means the test `TestDeleteTagHandler_CascadesAssignment` appears to pass while the bug exists in production.
|
||||
- **`strings.Contains(err.Error(), "request body too large")`:** The error message string is not part of the stable Go API. Use `errors.As(err, &maxBytesError)` instead.
|
||||
- **Sharing the `maxBodyBytes` constant as a magic number:** Define it once (`const maxBodyBytes = 1 << 20`) so all three handlers use the same value.
|
||||
|
||||
---
|
||||
|
||||
## Don't Hand-Roll
|
||||
|
||||
| Problem | Don't Build | Use Instead | Why |
|
||||
|---------|-------------|-------------|-----|
|
||||
| SQLite UPSERT | A "check if exists, then INSERT or UPDATE" two-step | `INSERT ... ON CONFLICT DO UPDATE` | Two-step is non-atomic; concurrent writes between the SELECT and INSERT/UPDATE can create duplicates or miss updates |
|
||||
| Request body size limit | Manual `io.ReadAll` with size check | `http.MaxBytesReader` | `MaxBytesReader` also signals the server to close the connection after the limit, preventing slow clients from holding connections open |
|
||||
| Typed error detection | `err.Error() == "http: request body too large"` | `errors.As(err, &maxBytesError)` | String comparison is fragile; `MaxBytesError` is a stable exported type since Go 1.19 |
|
||||
|
||||
---
|
||||
|
||||
## Common Pitfalls
|
||||
|
||||
### Pitfall 1: PRAGMA foreign_keys = ON placed after schema creation
|
||||
|
||||
**What goes wrong:** If `PRAGMA foreign_keys = ON` is placed after `CREATE TABLE IF NOT EXISTS tag_assignments (... REFERENCES tags(id) ON DELETE CASCADE)`, on an in-memory database the tables may already have orphaned rows from a prior test run (via `UpdatesReset()` which calls `InitDB(":memory:")`). The pragma is correctly placed but the tables were created in an FK-off state. This is fine because the pragma affects enforcement of future writes, not table creation syntax.
|
||||
|
||||
**Why it matters:** The ordering within `InitDB()` is: Open → PRAGMA → CREATE TABLE. If PRAGMA is after CREATE TABLE, it still works for enforcement purposes (FK enforcement applies at write time, not table creation time). However, putting PRAGMA first is cleaner and avoids any ambiguity.
|
||||
|
||||
**How to avoid:** Place `db.Exec("PRAGMA foreign_keys = ON")` as the very first SQL statement after `sql.Open` — before any schema DDL.
|
||||
|
||||
### Pitfall 2: ON CONFLICT UPSERT must list columns explicitly
|
||||
|
||||
**What goes wrong:** `INSERT OR REPLACE INTO updates VALUES (?,?,?,...)` uses positional VALUES with no column list. The replacement `INSERT INTO updates (...) VALUES (...) ON CONFLICT(image) DO UPDATE SET` must explicitly name every column in the VALUES list. If a column is added to the schema later (e.g., another migration), the VALUES list must be updated too.
|
||||
|
||||
**Why it happens:** The current `INSERT OR REPLACE` implicitly inserts into all columns by position. The UPSERT syntax requires an explicit conflict target column (`image`) which means the column list must be explicit.
|
||||
|
||||
**How to avoid:** The explicit column list in the UPSERT is actually safer — column additions to the schema won't silently insert NULL into unmentioned columns.
|
||||
|
||||
### Pitfall 3: MaxBytesReader must wrap r.Body before any read
|
||||
|
||||
**What goes wrong:** `http.MaxBytesReader` wraps the reader; it does not inspect an already-partially-read body. If any code reads from `r.Body` before `MaxBytesReader` is applied (e.g., a middleware that logs the request), the limit applies only to the remaining bytes. In the current codebase this is not a problem — no reads happen before the JSON decode.
|
||||
|
||||
**How to avoid:** Apply `r.Body = http.MaxBytesReader(w, r.Body, maxBodyBytes)` as the first operation in each handler body, before any reads.
|
||||
|
||||
### Pitfall 4: TestDeleteTagHandler_CascadesAssignment currently passes for the wrong reason
|
||||
|
||||
**What goes wrong:** This test passes today even though `PRAGMA foreign_keys = ON` is not set. The reason: `GetUpdates()` uses a `LEFT JOIN tag_assignments ta ON u.image = ta.image`. When `INSERT OR REPLACE` deletes the `tag_assignments` row as a side effect (either via the FK cascade on a different code path, or by direct `tag_assignments` cleanup), the LEFT JOIN simply returns NULL for the tag columns — and the test checks `m["nginx:latest"].Tag != nil`. So the test correctly detects the absence of a tag, but for the wrong reason.
|
||||
|
||||
**Warning sign:** After fixing DATA-01 (UPSERT), if DATA-02 (FK enforcement) is not also fixed, `TestDeleteTagHandler_CascadesAssignment` may start failing because tag assignments now survive the UPSERT but FK cascades still do not fire on tag deletion.
|
||||
|
||||
**How to avoid:** Fix DATA-01 and DATA-02 together, not separately. The regression test for DATA-02 must assert that deleting a tag removes its assignments.
|
||||
|
||||
### Pitfall 5: Silent errors in test helpers (export_test.go)
|
||||
|
||||
**What goes wrong:** `ResetTags()` in `export_test.go` calls `db.Exec(...)` twice with no error checking. If the DELETE fails (e.g., FK violation because FK enforcement is now ON and there is a constraint preventing the delete), the reset silently leaves stale data.
|
||||
|
||||
**How to avoid:** After fixing DATA-02, verify that `ResetTags()` in `export_test.go` does not need `PRAGMA foreign_keys = OFF` temporarily, or that the DELETE cascade order is correct (delete `tag_assignments` first, then `tags`). The current order is correct — `DELETE FROM tag_assignments` first, then `DELETE FROM tags`. With FK enforcement ON, deleting from `tag_assignments` first and then `tags` will succeed cleanly.
|
||||
|
||||
---
|
||||
|
||||
## Code Examples
|
||||
|
||||
Verified patterns from official sources:
|
||||
|
||||
### DATA-01: Full UPSERT replacement for UpdateEvent()
|
||||
|
||||
```go
|
||||
// Source: https://sqlite.org/lang_upsert.html
|
||||
func UpdateEvent(event DiunEvent) error {
|
||||
mu.Lock()
|
||||
defer mu.Unlock()
|
||||
_, err := db.Exec(`
|
||||
INSERT INTO updates (
|
||||
image, diun_version, hostname, status, provider,
|
||||
hub_link, mime_type, digest, created, platform,
|
||||
ctn_name, ctn_id, ctn_state, ctn_status,
|
||||
received_at, acknowledged_at
|
||||
) VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,NULL)
|
||||
ON CONFLICT(image) DO UPDATE SET
|
||||
diun_version = excluded.diun_version,
|
||||
hostname = excluded.hostname,
|
||||
status = excluded.status,
|
||||
provider = excluded.provider,
|
||||
hub_link = excluded.hub_link,
|
||||
mime_type = excluded.mime_type,
|
||||
digest = excluded.digest,
|
||||
created = excluded.created,
|
||||
platform = excluded.platform,
|
||||
ctn_name = excluded.ctn_name,
|
||||
ctn_id = excluded.ctn_id,
|
||||
ctn_state = excluded.ctn_state,
|
||||
ctn_status = excluded.ctn_status,
|
||||
received_at = excluded.received_at,
|
||||
acknowledged_at = NULL`,
|
||||
event.Image, event.DiunVersion, event.Hostname, event.Status, event.Provider,
|
||||
event.HubLink, event.MimeType, event.Digest,
|
||||
event.Created.Format(time.RFC3339), event.Platform,
|
||||
event.Metadata.ContainerName, event.Metadata.ContainerID,
|
||||
event.Metadata.State, event.Metadata.Status,
|
||||
time.Now().Format(time.RFC3339),
|
||||
)
|
||||
return err
|
||||
}
|
||||
```
|
||||
|
||||
### DATA-02: PRAGMA placement in InitDB()
|
||||
|
||||
```go
|
||||
// Source: https://sqlite.org/foreignkeys.html
|
||||
func InitDB(path string) error {
|
||||
var err error
|
||||
db, err = sql.Open("sqlite", path)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
db.SetMaxOpenConns(1)
|
||||
|
||||
// Enable FK enforcement on the single connection before any schema work
|
||||
if _, err = db.Exec(`PRAGMA foreign_keys = ON`); err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
// ... existing CREATE TABLE statements unchanged ...
|
||||
}
|
||||
```
|
||||
|
||||
### DATA-03: MaxBytesReader + typed error check
|
||||
|
||||
```go
|
||||
// Source: https://pkg.go.dev/net/http#MaxBytesReader
|
||||
// Source: https://www.alexedwards.net/blog/how-to-properly-parse-a-json-request-body
|
||||
|
||||
const maxBodyBytes = 1 << 20 // 1 MB — package-level constant, shared by all handlers
|
||||
|
||||
// In WebhookHandler, after method and auth checks:
|
||||
r.Body = http.MaxBytesReader(w, r.Body, maxBodyBytes)
|
||||
var event DiunEvent
|
||||
if err := json.NewDecoder(r.Body).Decode(&event); err != nil {
|
||||
var maxBytesErr *http.MaxBytesError
|
||||
if errors.As(err, &maxBytesErr) {
|
||||
http.Error(w, "request body too large", http.StatusRequestEntityTooLarge)
|
||||
return
|
||||
}
|
||||
log.Printf("WebhookHandler: failed to decode request: %v", err)
|
||||
http.Error(w, "bad request", http.StatusBadRequest)
|
||||
return
|
||||
}
|
||||
```
|
||||
|
||||
### DATA-04: t.Fatalf replacements
|
||||
|
||||
```go
|
||||
// Before — silent test pass on setup failure:
|
||||
err := diun.UpdateEvent(event)
|
||||
if err != nil {
|
||||
return
|
||||
}
|
||||
|
||||
// After — test fails loudly, CI catches the failure:
|
||||
if err := diun.UpdateEvent(event); err != nil {
|
||||
t.Fatalf("test setup: UpdateEvent failed: %v", err)
|
||||
}
|
||||
```
|
||||
|
||||
### DATA-04: New regression test for DATA-01 (tag survives new event)
|
||||
|
||||
This test does not exist yet and must be added as part of DATA-01:
|
||||
|
||||
```go
|
||||
func TestUpdateEvent_PreservesTagOnUpsert(t *testing.T) {
|
||||
diun.UpdatesReset()
|
||||
|
||||
// Insert image and assign a tag
|
||||
if err := diun.UpdateEvent(diun.DiunEvent{Image: "nginx:latest", Status: "new"}); err != nil {
|
||||
t.Fatalf("first UpdateEvent failed: %v", err)
|
||||
}
|
||||
tagID := postTagAndGetID(t, "webservers")
|
||||
body, _ := json.Marshal(map[string]interface{}{"image": "nginx:latest", "tag_id": tagID})
|
||||
req := httptest.NewRequest(http.MethodPut, "/api/tag-assignments", bytes.NewReader(body))
|
||||
rec := httptest.NewRecorder()
|
||||
diun.TagAssignmentHandler(rec, req)
|
||||
if rec.Code != http.StatusNoContent {
|
||||
t.Fatalf("tag assignment failed: %d", rec.Code)
|
||||
}
|
||||
|
||||
// Receive a second event for the same image (simulates DIUN re-notification)
|
||||
if err := diun.UpdateEvent(diun.DiunEvent{Image: "nginx:latest", Status: "update"}); err != nil {
|
||||
t.Fatalf("second UpdateEvent failed: %v", err)
|
||||
}
|
||||
|
||||
// Tag must survive the second event
|
||||
m := diun.GetUpdatesMap()
|
||||
if m["nginx:latest"].Tag == nil {
|
||||
t.Error("tag was lost after second UpdateEvent — INSERT OR REPLACE bug not fixed")
|
||||
}
|
||||
if m["nginx:latest"].Tag != nil && m["nginx:latest"].Tag.ID != tagID {
|
||||
t.Errorf("tag ID changed: expected %d, got %d", tagID, m["nginx:latest"].Tag.ID)
|
||||
}
|
||||
// Acknowledged state should be reset
|
||||
if m["nginx:latest"].Acknowledged {
|
||||
t.Error("acknowledged state should be reset by new event")
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## State of the Art
|
||||
|
||||
| Old Approach | Current Approach | When Changed | Impact |
|
||||
|--------------|------------------|--------------|--------|
|
||||
| `INSERT OR REPLACE` (DELETE+INSERT) | `INSERT ... ON CONFLICT DO UPDATE` | SQLite 3.24 (2018-06-04) | Preserves FK child rows; row identity unchanged |
|
||||
| Manual PRAGMA per session | DSN `?_pragma=foreign_keys(1)` | modernc.org/sqlite (current) | Pool-safe; applies to every future connection automatically |
|
||||
| `io.LimitReader` for body limits | `http.MaxBytesReader` | Go 1.0+ (always) | Signals connection close; returns typed `MaxBytesError` |
|
||||
| `*http.MaxBytesError` type assertion | `errors.As(err, &maxBytesErr)` | Go 1.19 (MaxBytesError exported) | Type-safe; works with wrapped errors |
|
||||
|
||||
**Deprecated/outdated:**
|
||||
- `INSERT OR REPLACE`: Still valid SQLite syntax but semantically wrong for tables with FK children. Use `ON CONFLICT DO UPDATE` instead.
|
||||
- String-matching on error messages: `strings.Contains(err.Error(), "request body too large")` — not API-stable. `errors.As` with `*http.MaxBytesError` is the correct pattern since Go 1.19.
|
||||
|
||||
---
|
||||
|
||||
## Open Questions
|
||||
|
||||
1. **Does `PRAGMA foreign_keys = ON` interfere with `UpdatesReset()` calling `InitDB(":memory:")`?**
|
||||
- What we know: `UpdatesReset()` in `export_test.go` calls `InitDB(":memory:")` which re-runs the full schema creation on a fresh in-memory database. The PRAGMA will be set on the new connection.
|
||||
- What's unclear: Whether setting the PRAGMA on `:memory:` changes any test behavior for existing passing tests.
|
||||
- Recommendation: Run the full test suite immediately after adding the PRAGMA. If any test regresses, inspect whether it was relying on FK non-enforcement. This is unlikely since the existing tests do not create FK-violation scenarios intentionally.
|
||||
|
||||
2. **Should `TagAssignmentHandler`'s `INSERT OR REPLACE INTO tag_assignments` (line 352) also be changed to a proper UPSERT?**
|
||||
- What we know: `tag_assignments` has `image TEXT PRIMARY KEY`, so `INSERT OR REPLACE` on it also deletes and reinserts. Since `tag_assignments` has no FK children, the delete+insert is functionally harmless here.
|
||||
- What's unclear: Whether this is in scope for Phase 1 or Phase 2.
|
||||
- Recommendation: Include it in Phase 1 for consistency and to eliminate all `INSERT OR REPLACE` occurrences. The fix is trivial: `INSERT INTO tag_assignments (image, tag_id) VALUES (?, ?) ON CONFLICT(image) DO UPDATE SET tag_id = excluded.tag_id`.
|
||||
|
||||
---
|
||||
|
||||
## Environment Availability
|
||||
|
||||
Step 2.6: SKIPPED — Phase 1 is code-only edits to existing Go source files and test files. No external tools, services, runtimes, databases, or CLIs beyond the existing project toolchain are required.
|
||||
|
||||
---
|
||||
|
||||
## Sources
|
||||
|
||||
### Primary (HIGH confidence)
|
||||
- [SQLite UPSERT documentation](https://sqlite.org/lang_upsert.html) — ON CONFLICT DO UPDATE syntax, `excluded.` qualifier behavior, availability since SQLite 3.24
|
||||
- [SQLite Foreign Key Support](https://sqlite.org/foreignkeys.html) — per-connection enforcement, must enable with PRAGMA, not stored in DB file
|
||||
- [Go net/http package — MaxBytesReader](https://pkg.go.dev/net/http) — function signature, MaxBytesError type, behavior on limit exceeded
|
||||
- [modernc.org/sqlite package](https://pkg.go.dev/modernc.org/sqlite) — DSN `_pragma` parameter, RegisterConnectionHook API
|
||||
- Direct code analysis: `pkg/diunwebhook/diunwebhook.go` lines 58-118, 179, 277, 340, 352 — HIGH confidence (source of truth)
|
||||
- Direct code analysis: `pkg/diunwebhook/diunwebhook_test.go` lines 38-40, 153-154, 228-231, 287-289, 329-331, 350-351 — HIGH confidence (source of truth)
|
||||
- Direct code analysis: `pkg/diunwebhook/export_test.go` — HIGH confidence
|
||||
|
||||
### Secondary (MEDIUM confidence)
|
||||
- [Alex Edwards — How to properly parse a JSON request body](https://www.alexedwards.net/blog/how-to-properly-parse-a-json-request-body) — MaxBytesReader + errors.As pattern, verified against pkg.go.dev
|
||||
- [TIL: SQLite Foreign Key Support with Go](https://www.rockyourcode.com/til-sqlite-foreign-key-support-with-go/) — per-connection requirement, connection pool implications
|
||||
- `.planning/codebase/CONCERNS.md` — pre-existing bug audit (lines 37-47) — HIGH (prior analysis by same team)
|
||||
- `.planning/research/PITFALLS.md` — Pitfall 2 (INSERT OR REPLACE) — HIGH (direct codebase evidence cited)
|
||||
|
||||
### Tertiary (LOW confidence)
|
||||
- None
|
||||
|
||||
---
|
||||
|
||||
## Metadata
|
||||
|
||||
**Confidence breakdown:**
|
||||
- DATA-01 fix (UPSERT): HIGH — SQLite official docs confirm syntax, codebase confirms bug location at line 109
|
||||
- DATA-02 fix (FK enforcement): HIGH — SQLite official docs confirm per-connection behavior, modernc.org/sqlite docs confirm DSN approach, SetMaxOpenConns(1) makes simple Exec sufficient
|
||||
- DATA-03 fix (MaxBytesReader): HIGH — Go stdlib docs confirm API, MaxBytesError exported since Go 1.19, module requires Go 1.26
|
||||
- DATA-04 fix (t.Fatal): HIGH — Direct test file analysis, standard Go testing idiom
|
||||
|
||||
**Research date:** 2026-03-23
|
||||
**Valid until:** 2026-06-23 (SQLite and Go stdlib APIs are extremely stable; UPSERT syntax has not changed since 3.24 in 2018)
|
||||
Reference in New Issue
Block a user