318 lines
14 KiB
Markdown
318 lines
14 KiB
Markdown
---
|
|
phase: 06-sql-consolidation-code-quality
|
|
plan: 03
|
|
type: execute
|
|
wave: 2
|
|
depends_on: [06-01]
|
|
files_modified:
|
|
- backend/database/sql/queries/audio_files.sql
|
|
- backend/database/sql/sqlcgen/audio_files.sql.go
|
|
- backend/database/sql/sqlcgen/models.go
|
|
- backend/queue/persistence.go
|
|
- backend/database/search.go
|
|
- backend/library/library.go
|
|
- backend/library/rescan.go
|
|
autonomous: true
|
|
requirements: [QUAL-03, QUAL-04]
|
|
|
|
must_haves:
|
|
truths:
|
|
- "lookupChunk no longer uses fmt.Sprintf for IN clause construction — it calls a sqlc-generated query via the track_metadata VIEW"
|
|
- "Every hand-crafted SQL statement that bypasses sqlc has a // SAFETY: comment with two parts: why sqlc can't handle it AND what makes it safe"
|
|
- "All 12 identified hand-crafted SQL statements have SAFETY comments"
|
|
- "Queue tests and database tests pass unchanged after the migration"
|
|
artifacts:
|
|
- path: "backend/database/sql/queries/audio_files.sql"
|
|
provides: "sqlc query for batch track metadata lookup"
|
|
contains: "LookupTrackMetaByPaths"
|
|
- path: "backend/queue/persistence.go"
|
|
provides: "Updated lookupChunk using sqlc-generated query"
|
|
contains: "SAFETY"
|
|
- path: "backend/database/search.go"
|
|
provides: "SAFETY comments on all FTS5 queries"
|
|
contains: "SAFETY"
|
|
- path: "backend/library/library.go"
|
|
provides: "SAFETY comments on FTS5 insert/delete operations"
|
|
contains: "SAFETY"
|
|
- path: "backend/library/rescan.go"
|
|
provides: "SAFETY comments on FTS5 delete operation"
|
|
contains: "SAFETY"
|
|
key_links:
|
|
- from: "backend/queue/persistence.go"
|
|
to: "backend/database/sql/sqlcgen/"
|
|
via: "sqlc-generated LookupTrackMetaByPaths query"
|
|
pattern: "LookupTrackMetaByPaths"
|
|
- from: "backend/database/sql/queries/audio_files.sql"
|
|
to: "track_metadata VIEW"
|
|
via: "SELECT FROM track_metadata WHERE file_path IN (sqlc.slice)"
|
|
pattern: "sqlc.slice"
|
|
---
|
|
|
|
<objective>
|
|
Migrate the queue's `lookupChunk` from hand-crafted SQL with `fmt.Sprintf` to a sqlc-generated query using the `track_metadata` VIEW and `sqlc.slice()`, then add `// SAFETY:` comments to all remaining hand-crafted SQL statements.
|
|
|
|
Purpose: Replace the only hand-crafted SQL that CAN be migrated to sqlc (lookupChunk), and document all intentional exceptions so future maintainers understand why each hand-crafted statement exists.
|
|
|
|
Output: sqlc query file, regenerated code, updated persistence.go, SAFETY comments on all 12 hand-crafted SQL statements.
|
|
</objective>
|
|
|
|
<execution_context>
|
|
@/home/caleb/.config/opencode/get-shit-done/workflows/execute-plan.md
|
|
@/home/caleb/.config/opencode/get-shit-done/templates/summary.md
|
|
</execution_context>
|
|
|
|
<context>
|
|
@.planning/PROJECT.md
|
|
@.planning/ROADMAP.md
|
|
@.planning/STATE.md
|
|
@.planning/phases/06-sql-consolidation-code-quality/06-RESEARCH.md
|
|
@.planning/phases/06-sql-consolidation-code-quality/06-01-SUMMARY.md
|
|
|
|
@backend/queue/persistence.go
|
|
@backend/database/search.go
|
|
@backend/library/library.go
|
|
@backend/library/rescan.go
|
|
@backend/database/sql/queries/audio_files.sql
|
|
@backend/database/sqlc.yaml
|
|
|
|
<interfaces>
|
|
<!-- Key types the executor needs -->
|
|
|
|
From backend/queue/persistence.go:
|
|
```go
|
|
type trackMeta struct {
|
|
AudioFileID int64
|
|
FilePath string
|
|
Title string
|
|
Artist string
|
|
}
|
|
|
|
// lookupTrackMetaBatch — chunks at maxSQLiteVars (900) and calls lookupChunk per chunk
|
|
// lookupChunk — hand-crafted SELECT with fmt.Sprintf IN clause (TARGET for sqlc migration)
|
|
// insertTrackBatch — multi-row INSERT with variable VALUES count (STAYS hand-crafted)
|
|
const maxSQLiteVars = 900
|
|
```
|
|
|
|
From backend/database/search.go (after Plan 01 consolidation):
|
|
- SearchFTS — FTS5 MATCH query using track_metadata VIEW
|
|
- SearchFTSByFilename — FTS5 MATCH query using track_metadata VIEW
|
|
- InsertSearchIndex — single-row INSERT INTO search_index
|
|
- DeleteSearchIndex — DELETE FROM search_index WHERE rowid = ?
|
|
- ClearSearchIndex — DELETE FROM search_index
|
|
- RebuildSearchIndex — INSERT INTO search_index SELECT FROM track_metadata
|
|
- SearchFTSTracks — FTS5 MATCH query using track_metadata VIEW
|
|
|
|
From backend/library/library.go:
|
|
- commitNewAudioFile (~line 798) — INSERT INTO search_index VALUES (single row)
|
|
- updateAudioFileMetadata (~line 879) — DELETE FROM search_index WHERE rowid = ?
|
|
- updateAudioFileMetadata (~line 893) — INSERT INTO search_index VALUES (single row)
|
|
|
|
From backend/library/rescan.go:
|
|
- clearAllLibraryData (~line 165) — DELETE FROM search_index
|
|
|
|
Complete SAFETY comment inventory (12 statements):
|
|
| # | File | Function | Operation | Why hand-crafted |
|
|
|---|------|----------|-----------|-----------------|
|
|
| 1 | search.go | SearchFTS | FTS5 MATCH | FTS5 unsupported by sqlc |
|
|
| 2 | search.go | SearchFTSByFilename | FTS5 MATCH | FTS5 unsupported by sqlc |
|
|
| 3 | search.go | InsertSearchIndex | FTS5 INSERT | FTS5 virtual table |
|
|
| 4 | search.go | DeleteSearchIndex | FTS5 DELETE | FTS5 virtual table |
|
|
| 5 | search.go | ClearSearchIndex | FTS5 DELETE | FTS5 virtual table |
|
|
| 6 | search.go | RebuildSearchIndex | FTS5 INSERT SELECT | FTS5 virtual table |
|
|
| 7 | search.go | SearchFTSTracks | FTS5 MATCH | FTS5 unsupported by sqlc |
|
|
| 8 | library.go | commitNewAudioFile | FTS5 INSERT | FTS5 virtual table |
|
|
| 9 | library.go | updateAudioFileMetadata | FTS5 DELETE | FTS5 virtual table |
|
|
| 10 | library.go | updateAudioFileMetadata | FTS5 INSERT | FTS5 virtual table |
|
|
| 11 | rescan.go | clearAllLibraryData | FTS5 DELETE | FTS5 virtual table |
|
|
| 12 | persistence.go | insertTrackBatch | Variable-count multi-row INSERT | sqlc can't generate variable-length batch INSERTs |
|
|
</interfaces>
|
|
</context>
|
|
|
|
<tasks>
|
|
|
|
<task type="auto">
|
|
<name>Task 1: Migrate lookupChunk to sqlc with sqlc.slice()</name>
|
|
<files>
|
|
backend/database/sql/queries/audio_files.sql
|
|
backend/database/sql/sqlcgen/audio_files.sql.go
|
|
backend/database/sql/sqlcgen/models.go
|
|
backend/queue/persistence.go
|
|
</files>
|
|
<action>
|
|
1. Add the sqlc query to `backend/database/sql/queries/audio_files.sql`:
|
|
```sql
|
|
-- name: LookupTrackMetaByPaths :many
|
|
SELECT id, file_path, title, artist_name
|
|
FROM track_metadata
|
|
WHERE file_path IN (sqlc.slice('paths'));
|
|
```
|
|
This uses the `track_metadata` VIEW created by Plan 01. The VIEW's columns `title` and `artist_name` match the data lookupChunk currently fetches via its inline JOIN.
|
|
|
|
2. Run `go tool sqlc generate` from `backend/database/` to generate the Go code.
|
|
|
|
3. Update `backend/queue/persistence.go`:
|
|
|
|
a. Replace the `lookupChunk` method body. Instead of building `fmt.Sprintf` placeholders, call the sqlc-generated `LookupTrackMetaByPaths` method:
|
|
```go
|
|
func (q *Queue) lookupChunk(
|
|
paths []string,
|
|
result map[string]trackMeta,
|
|
) {
|
|
if len(paths) == 0 {
|
|
return
|
|
}
|
|
|
|
rows, err := q.db.Queries.LookupTrackMetaByPaths(q.db.Ctx, paths)
|
|
if err != nil {
|
|
q.logger.Error("Batch metadata lookup failed", "err", err)
|
|
return
|
|
}
|
|
|
|
for _, row := range rows {
|
|
result[row.FilePath] = trackMeta{
|
|
AudioFileID: row.ID,
|
|
FilePath: row.FilePath,
|
|
Title: row.Title,
|
|
Artist: row.ArtistName,
|
|
}
|
|
}
|
|
}
|
|
```
|
|
|
|
b. The `lookupTrackMetaBatch` function stays unchanged — it still chunks at `maxSQLiteVars` and calls `lookupChunk` per chunk. The chunking is still necessary because `sqlc.slice()` does NOT auto-chunk.
|
|
|
|
c. Remove the now-unused imports: `"fmt"` and `"strings"` may become unused if `insertTrackBatch` is the only remaining user. Check import usage — `fmt` is still needed for `insertTrackBatch` (line ~200 `fmt.Errorf`), and `strings` is still needed for `insertTrackBatch` (line ~196 `strings.Join`). Keep both if still referenced.
|
|
|
|
4. Verify the field name mapping is correct:
|
|
- VIEW column `id` → sqlc field `ID` → `trackMeta.AudioFileID`
|
|
- VIEW column `file_path` → sqlc field `FilePath` → `trackMeta.FilePath`
|
|
- VIEW column `title` → sqlc field `Title` → `trackMeta.Title`
|
|
- VIEW column `artist_name` → sqlc field `ArtistName` → `trackMeta.Artist`
|
|
|
|
5. Run queue tests to verify the migration doesn't break metadata resolution.
|
|
</action>
|
|
<verify>
|
|
<automated>cd backend/database && go tool sqlc generate && cd ../.. && go test -tags webkit2_41 -race -count=1 -timeout 60s ./backend/queue/...</automated>
|
|
</verify>
|
|
<done>
|
|
- sqlc query `LookupTrackMetaByPaths` exists in audio_files.sql
|
|
- lookupChunk uses the sqlc-generated query instead of fmt.Sprintf
|
|
- lookupTrackMetaBatch still chunks at maxSQLiteVars (900)
|
|
- All queue tests pass with -race (29 tests)
|
|
- No hand-crafted SQL remains in lookupChunk
|
|
</done>
|
|
</task>
|
|
|
|
<task type="auto">
|
|
<name>Task 2: Add SAFETY comments to all hand-crafted SQL</name>
|
|
<files>
|
|
backend/database/search.go
|
|
backend/library/library.go
|
|
backend/library/rescan.go
|
|
backend/queue/persistence.go
|
|
</files>
|
|
<action>
|
|
Add `// SAFETY:` comments to all 12 hand-crafted SQL statements. Each comment has two parts: (1) WHY sqlc can't handle it, and (2) what makes the query safe. Cross-reference related operations where applicable.
|
|
|
|
**backend/database/search.go** (7 statements):
|
|
|
|
1. Before SearchFTS query (~line 34):
|
|
`// SAFETY: FTS5 MATCH syntax unsupported by sqlc. Query is parameterized; no string interpolation.`
|
|
|
|
2. Before SearchFTSByFilename query (~line 92):
|
|
`// SAFETY: FTS5 MATCH syntax unsupported by sqlc. Query is parameterized; no string interpolation.`
|
|
|
|
3. Before InsertSearchIndex query (~line 133):
|
|
`// SAFETY: FTS5 virtual table INSERT unsupported by sqlc. All values are parameterized.`
|
|
|
|
4. Before DeleteSearchIndex query (~line 143):
|
|
`// SAFETY: FTS5 virtual table DELETE unsupported by sqlc. Rowid is parameterized.`
|
|
|
|
5. Before ClearSearchIndex query (~line 152):
|
|
`// SAFETY: FTS5 virtual table DELETE unsupported by sqlc. No parameters; unconditional delete.`
|
|
|
|
6. Before RebuildSearchIndex query (~line 168):
|
|
`// SAFETY: FTS5 virtual table INSERT unsupported by sqlc. All values sourced from track_metadata VIEW; no user input.`
|
|
|
|
7. Before SearchFTSTracks query (~line 232):
|
|
`// SAFETY: FTS5 MATCH syntax unsupported by sqlc. Query is parameterized; no string interpolation.`
|
|
|
|
**backend/library/library.go** (3 statements):
|
|
|
|
8. Before commitNewAudioFile FTS INSERT (~line 798):
|
|
`// SAFETY: FTS5 virtual table, see search.go:InsertSearchIndex. All values parameterized.`
|
|
|
|
9. Before updateAudioFileMetadata FTS DELETE (~line 879):
|
|
`// SAFETY: FTS5 virtual table, see search.go:DeleteSearchIndex. Rowid parameterized.`
|
|
|
|
10. Before updateAudioFileMetadata FTS INSERT (~line 893):
|
|
`// SAFETY: FTS5 virtual table, see search.go:InsertSearchIndex. All values parameterized.`
|
|
|
|
**backend/library/rescan.go** (1 statement):
|
|
|
|
11. Before clearAllLibraryData FTS DELETE (~line 165):
|
|
`// SAFETY: FTS5 virtual table, see search.go:ClearSearchIndex. No parameters; unconditional delete.`
|
|
|
|
**backend/queue/persistence.go** (1 statement):
|
|
|
|
12. Before insertTrackBatch query (~line 195):
|
|
`// SAFETY: Multi-row INSERT with variable row count unsupported by sqlc. Placeholder count matches args length; no string interpolation.`
|
|
|
|
**Rules:**
|
|
- Place each SAFETY comment on the line immediately before the SQL string literal (the query variable or inline string)
|
|
- Use the exact `// SAFETY:` prefix (capital, colon, space)
|
|
- Two-part format: reason + safety assurance
|
|
- Cross-reference related operations in library.go/rescan.go back to search.go
|
|
</action>
|
|
<verify>
|
|
<automated>test $(grep -r "// SAFETY:" backend/database/search.go backend/library/library.go backend/library/rescan.go backend/queue/persistence.go | wc -l) -eq 12 && echo "All 12 SAFETY comments present" && go build -tags webkit2_41 ./...</automated>
|
|
</verify>
|
|
<done>
|
|
- All 12 hand-crafted SQL statements have SAFETY comments
|
|
- Comments follow two-part format (why + safety assurance)
|
|
- Cross-references link library.go/rescan.go back to search.go
|
|
- Code compiles without errors
|
|
- No SAFETY comments on migration DDL (migration2, migration3, migration4)
|
|
</done>
|
|
</task>
|
|
|
|
</tasks>
|
|
|
|
<verification>
|
|
```bash
|
|
# 1. sqlc generates cleanly
|
|
cd backend/database && go tool sqlc generate
|
|
|
|
# 2. All queue tests pass
|
|
go test -tags webkit2_41 -race -count=1 -timeout 60s ./backend/queue/...
|
|
|
|
# 3. All database tests pass
|
|
go test -tags webkit2_41 -race -count=1 -timeout 60s ./backend/database/...
|
|
|
|
# 4. All library tests pass
|
|
go test -tags webkit2_41 -race -count=1 -timeout 60s ./backend/library/...
|
|
|
|
# 5. Verify all 12 SAFETY comments exist
|
|
grep -r "// SAFETY:" backend/database/search.go backend/library/library.go backend/library/rescan.go backend/queue/persistence.go | wc -l # Should be 12
|
|
|
|
# 6. Verify no fmt.Sprintf remains in lookupChunk
|
|
grep -A5 "func.*lookupChunk" backend/queue/persistence.go | grep -c "fmt.Sprintf" # Should be 0
|
|
|
|
# 7. Full build
|
|
go build -tags webkit2_41 ./...
|
|
```
|
|
</verification>
|
|
|
|
<success_criteria>
|
|
- lookupChunk uses sqlc-generated `LookupTrackMetaByPaths` query against track_metadata VIEW
|
|
- fmt.Sprintf placeholder construction eliminated from lookupChunk
|
|
- Chunking logic preserved (maxSQLiteVars = 900)
|
|
- All 12 hand-crafted SQL statements documented with // SAFETY: comments
|
|
- All existing tests pass (queue: 29, database: 15, library: 13)
|
|
- Full project builds without errors
|
|
</success_criteria>
|
|
|
|
<output>
|
|
After completion, create `.planning/phases/06-sql-consolidation-code-quality/06-03-SUMMARY.md`
|
|
</output>
|