fix(database): retire a table whose shape the schema moved past
`applySchema` is CREATE ... IF NOT EXISTS and there is no migration chain, so a *changed* table never migrates: the statement silently no-ops against the old shape. Two plans had already landed on that, and neither showed up in a test because a fresh install is perfectly healthy. - 014 added `total_tracks` to explore_index and to `indexRowFields`, the projection every explore read uses, so every search, browse, artist page and album page failed with "no such column: total_tracks" on any database that already had a catalog. - 013 reshaped audio_files, so applySchema could not run at all and the app did not open. staleshape.go runs before applySchema and drops what disagrees, so the create is a create. It parses sql/schemas/ for the expectation rather than writing the column list down a second time, and it notices a changed *type* as well as a missing column — 013 moved mbid TEXT to BLOB, which no ALTER could express and which SQLite will not coerce, so a query against 16 raw bytes returns no rows rather than an error. Only Authored tables are exempt. Cache is rebuildable by definition, Owned is what a rescan rebuilds (plan 013's stated "delete and rescan"), and a table the schema no longer describes at all goes too -- 013 left seven behind plus schema_migrations. Three things in it are load-bearing, and each was a bug first: - The parser read `UNIQUE(mbid)` as a column, which made a healthy catalog look stale. That would have retired it on every launch and cost every user an artifact download per start. - The drops are one transaction with defer_foreign_keys. Those legacy tables reference each other, so any order fails on whichever goes first; turning foreign keys off instead would suppress playlist_tracks.audio_file_id's ON DELETE SET NULL and leave entries pointing at ids a rescan reissues to *different songs*. Nulled entries are empty; stale ones are wrong, and wrong quietly. - The order is sorted, so a failure reproduces. Map order is random, and the foreign-key bug passed its own regression test on two runs in three until the order was fixed. Verified against a real pre-013 install: it opens, its 22 playlists survive, 1,887 linked playlist entries become 0 rather than dangling, and the legacy tables are swept. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AfVYUVExXsx1nSWrXN8mAh
This commit is contained in:
Claude Opus 5
@@ -89,6 +89,14 @@ func NewDB(logger *slog.Logger) (*DB, error) {
|
||||
return nil, fmt.Errorf("could not apply PRAGMAs: %w", err)
|
||||
}
|
||||
|
||||
// Before the schema is applied, not after: applySchema is
|
||||
// CREATE ... IF NOT EXISTS, which no-ops against a table that
|
||||
// already exists in an older shape. Retiring the stale one first is
|
||||
// what turns that no-op into a create.
|
||||
if err := retireStaleTables(dbCtx, db, logger); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
if err := applySchema(dbCtx, db); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
@@ -0,0 +1,552 @@
|
||||
package database
|
||||
|
||||
import (
|
||||
"context"
|
||||
"database/sql"
|
||||
"fmt"
|
||||
"io/fs"
|
||||
"log/slog"
|
||||
"maps"
|
||||
"path"
|
||||
"slices"
|
||||
"strings"
|
||||
|
||||
"yellowjacket/backend/datamap"
|
||||
)
|
||||
|
||||
// This file repairs the one thing `CREATE TABLE IF NOT EXISTS` cannot.
|
||||
//
|
||||
// `sql/schemas/` is the single description of the schema and there is no
|
||||
// migration chain (plan 013): a schema change is one edit to one file.
|
||||
// That works perfectly for a *new* table, which every install then
|
||||
// creates, and not at all for a changed one -- `IF NOT EXISTS` reaches
|
||||
// an existing table only if its shape already matches, and otherwise
|
||||
// silently no-ops. The user's answer to that is "delete and rescan"
|
||||
// (plan 013, open question 1), which is free for everything a rescan
|
||||
// rebuilds.
|
||||
//
|
||||
// It is not free for the catalog. explore_index is a *downloaded
|
||||
// artifact*, not something derived from the user's files, and it is the
|
||||
// largest thing this app stores. So it went stale instead: plan 014
|
||||
// added `total_tracks` to the schema and to `indexRowFields` -- the one
|
||||
// projection every explore read uses -- and no database that already
|
||||
// existed ever grew the column. Every Explore search, browse, artist
|
||||
// page and album page on such an install fails with
|
||||
// "no such column: total_tracks", while a fresh install is perfectly
|
||||
// healthy, which is why the tests did not see it. The same databases
|
||||
// are stale a second way, from the same plan: their `mbid` columns are
|
||||
// still TEXT where the schema now declares BLOB, and SQLite does not
|
||||
// coerce between the two -- a comparison against 16 raw bytes simply
|
||||
// returns no rows.
|
||||
//
|
||||
// The repair is to notice and drop, not to migrate. A dropped catalog
|
||||
// costs one artifact download (about a minute); the alternative --
|
||||
// ALTER TABLE ADD COLUMN, which would handle `total_tracks` alone
|
||||
// cheaply -- cannot express the TEXT-to-BLOB half at all, and would
|
||||
// leave those installs quietly broken while reporting success.
|
||||
//
|
||||
// Everything except `Authored` is eligible. `Cache` is rebuildable by
|
||||
// definition; `Owned` is a projection of the user's files and a rescan
|
||||
// rebuilds it, which is plan 013's stated answer to exactly this
|
||||
// situation ("delete and rescan", open question 1); `Derived` is
|
||||
// computed from Owned. No `Authored` table is ever dropped here --
|
||||
// that is the whole point of the datamap, and it is asserted by
|
||||
// TestAuthoredTablesAreNeverRetired rather than only stated.
|
||||
//
|
||||
// What that does *not* buy is immunity for authored rows that reference
|
||||
// a retired table. `audio_files` is MIXED KIND: `play_count`,
|
||||
// `last_played` and `tag_status` are authored columns on an Owned
|
||||
// table, and they go with it. Playlists survive as playlists, and
|
||||
// their entries survive pointing at nothing. That cost was weighed and
|
||||
// accepted rather than overlooked -- the alternative is to carry the
|
||||
// authored columns across the rebuild keyed on file_path, which stays a
|
||||
// real option if this ever bites harder than it is worth.
|
||||
//
|
||||
// **This relies on foreign_keys being ON**, which applyPRAGMAs has
|
||||
// already done by the time NewDB calls it, and the dependency is not
|
||||
// cosmetic. SQLite performs an implicit DELETE before dropping a table
|
||||
// when foreign keys are enabled, so `playlist_tracks.audio_file_id` --
|
||||
// declared ON DELETE SET NULL -- is nulled. With foreign keys off, no
|
||||
// action fires and those rows keep the ids they had, which a rescan
|
||||
// then reissues starting from 1: every playlist would silently fill
|
||||
// with *different songs*. Nulled entries are merely empty; stale ones
|
||||
// are wrong, and wrong quietly. TestRetiringOwnedTablesDoesNotDangle
|
||||
// is what stops a future reordering turning one into the other.
|
||||
|
||||
// retireGroups are tables that must be retired together. A catalog
|
||||
// whose rows are gone must not keep the full-text index built over
|
||||
// them, nor the metadata claiming the import that produced them
|
||||
// finished -- that marker is exactly what stops the artifact being
|
||||
// fetched again. applySchema recreates all three empty immediately
|
||||
// afterwards, and the ordinary "no index yet" path takes over.
|
||||
var retireGroups = [][]string{
|
||||
{
|
||||
"explore_index",
|
||||
"explore_index_fts",
|
||||
"explore_index_meta",
|
||||
"explore_champion_fts",
|
||||
},
|
||||
}
|
||||
|
||||
// schemaColumn is one column as the schema file declares it.
|
||||
type schemaColumn struct {
|
||||
name string
|
||||
typ string
|
||||
}
|
||||
|
||||
// retireStaleTables drops every non-authored table whose live shape no
|
||||
// longer matches what sql/schemas/ declares, plus any table the schema
|
||||
// no longer describes at all, so applySchema can create the current
|
||||
// shape afresh. It runs before applySchema and is a no-op on a new
|
||||
// database, where the tables do not exist yet.
|
||||
func retireStaleTables(
|
||||
ctx context.Context, db *sql.DB, logger *slog.Logger,
|
||||
) error {
|
||||
declared, err := declaredTables()
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
stale := make(map[string]string)
|
||||
|
||||
for table, columns := range declared {
|
||||
entry, ok := datamap.Lookup(table)
|
||||
if !ok || entry.Kind == datamap.Authored || entry.FTS {
|
||||
continue
|
||||
}
|
||||
|
||||
reason, err := staleReason(ctx, db, table, columns)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
if reason != "" {
|
||||
stale[table] = reason
|
||||
}
|
||||
}
|
||||
|
||||
obsolete, err := obsoleteTables(ctx, db)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
maps.Copy(stale, obsolete)
|
||||
|
||||
if len(stale) == 0 {
|
||||
return nil
|
||||
}
|
||||
|
||||
return retireGroupsFor(ctx, db, logger, stale)
|
||||
}
|
||||
|
||||
// obsoleteTables are live tables the schema no longer describes at all.
|
||||
// TestCatalogCoversSchema makes the datamap a complete description of
|
||||
// the current schema, so a table it does not know is one a past version
|
||||
// created and this one does not -- plan 013 alone left seven behind
|
||||
// (recordings, release_groups, artist_credit, artist_credit_artist,
|
||||
// release_group_recordings, recording_genres) plus the
|
||||
// schema_migrations table that squashing the chain retired. They are
|
||||
// dead weight, and one of them holding a foreign key into a table being
|
||||
// rebuilt is worse than dead weight.
|
||||
//
|
||||
// SQLite's own bookkeeping and FTS shadow tables are not obsolete:
|
||||
// datamap.Lookup resolves a shadow table to its parent, and IsInternal
|
||||
// covers the rest.
|
||||
func obsoleteTables(ctx context.Context, db *sql.DB) (map[string]string, error) {
|
||||
rows, err := db.QueryContext(
|
||||
ctx, "SELECT name FROM sqlite_master WHERE type = 'table'",
|
||||
)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("could not list tables: %w", err)
|
||||
}
|
||||
|
||||
defer func() { _ = rows.Close() }()
|
||||
|
||||
out := make(map[string]string)
|
||||
|
||||
for rows.Next() {
|
||||
var name string
|
||||
|
||||
if err := rows.Scan(&name); err != nil {
|
||||
return nil, fmt.Errorf("could not scan table name: %w", err)
|
||||
}
|
||||
|
||||
if datamap.IsInternal(name) {
|
||||
continue
|
||||
}
|
||||
|
||||
if _, known := datamap.Lookup(name); !known {
|
||||
out[name] = "the schema no longer describes this table"
|
||||
}
|
||||
}
|
||||
|
||||
if err := rows.Err(); err != nil {
|
||||
return nil, fmt.Errorf("could not read table list: %w", err)
|
||||
}
|
||||
|
||||
return out, nil
|
||||
}
|
||||
|
||||
// retireGroupsFor drops each stale table along with everything its
|
||||
// retire group says must go with it.
|
||||
func retireGroupsFor(
|
||||
ctx context.Context, db *sql.DB, logger *slog.Logger,
|
||||
stale map[string]string,
|
||||
) error {
|
||||
drop := make(map[string]string)
|
||||
|
||||
for table, reason := range stale {
|
||||
drop[table] = reason
|
||||
|
||||
for _, group := range retireGroups {
|
||||
if !slices.Contains(group, table) {
|
||||
continue
|
||||
}
|
||||
|
||||
for _, member := range group {
|
||||
if _, already := drop[member]; !already {
|
||||
drop[member] = "retired with " + table
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return dropDeferred(ctx, db, logger, drop)
|
||||
}
|
||||
|
||||
// dropDeferred drops every named table in one transaction with foreign
|
||||
// key enforcement deferred to the commit.
|
||||
//
|
||||
// The deferral is required and the two obvious alternatives are both
|
||||
// wrong. These tables reference each other -- pre-013 `audio_files`
|
||||
// has a foreign key into `recordings`, which is itself being retired --
|
||||
// so dropping them one at a time in an arbitrary order fails with
|
||||
// "FOREIGN KEY constraint failed" on whichever is unlucky enough to go
|
||||
// first, and there is no order that is safe in general. Turning
|
||||
// foreign keys *off* for the duration would fix that and silently take
|
||||
// the ON DELETE SET NULL on `playlist_tracks.audio_file_id` with it,
|
||||
// leaving playlist entries pointing at ids a rescan reissues to
|
||||
// different songs -- the exact failure
|
||||
// TestRetiringOwnedTablesDoesNotDangle exists to prevent.
|
||||
//
|
||||
// Deferring keeps the actions firing while tolerating the inconsistency
|
||||
// in the middle, and the commit then checks that the end state is
|
||||
// sound. It is set inside the transaction because SQLite resets it at
|
||||
// every commit.
|
||||
func dropDeferred(
|
||||
ctx context.Context, db *sql.DB, logger *slog.Logger,
|
||||
drop map[string]string,
|
||||
) error {
|
||||
tx, err := db.BeginTx(ctx, nil)
|
||||
if err != nil {
|
||||
return fmt.Errorf("could not begin the retire transaction: %w", err)
|
||||
}
|
||||
|
||||
defer func() { _ = tx.Rollback() }()
|
||||
|
||||
if _, err := tx.ExecContext(ctx, "PRAGMA defer_foreign_keys = ON"); err != nil {
|
||||
return fmt.Errorf("could not defer foreign keys: %w", err)
|
||||
}
|
||||
|
||||
// Sorted, so a failure is reproducible. Map order is random, and a
|
||||
// bug that depends on which table happens to go first reproduces on
|
||||
// one run in three and passes review on the other two -- which is
|
||||
// exactly how the foreign-key ordering above reached a real
|
||||
// database. Sorting does not make any order *safe*; the deferral
|
||||
// does that.
|
||||
for _, table := range slices.Sorted(maps.Keys(drop)) {
|
||||
logger.Warn(
|
||||
"retiring a table the schema no longer describes",
|
||||
"table", table,
|
||||
"reason", drop[table],
|
||||
)
|
||||
|
||||
if _, err := tx.ExecContext(
|
||||
ctx, "DROP TABLE IF EXISTS "+quoteIdent(table),
|
||||
); err != nil {
|
||||
return fmt.Errorf("could not retire stale table %s: %w", table, err)
|
||||
}
|
||||
}
|
||||
|
||||
if err := tx.Commit(); err != nil {
|
||||
return fmt.Errorf("could not commit the retire: %w", err)
|
||||
}
|
||||
|
||||
return nil
|
||||
}
|
||||
|
||||
// staleReason reports why a live table disagrees with its declaration,
|
||||
// or "" when it agrees. A column the live table does not have is the
|
||||
// additive case; a column whose declared type changed is the one an
|
||||
// ALTER could not fix anyway. Columns the live table has and the
|
||||
// schema no longer declares are ignored: they cost nothing and dropping
|
||||
// the table over one would retire a healthy catalog.
|
||||
func staleReason(
|
||||
ctx context.Context, db *sql.DB, table string, columns []schemaColumn,
|
||||
) (string, error) {
|
||||
live, err := liveColumns(ctx, db, table)
|
||||
if err != nil {
|
||||
return "", err
|
||||
}
|
||||
|
||||
if len(live) == 0 {
|
||||
// Not present at all: applySchema is about to create it.
|
||||
return "", nil
|
||||
}
|
||||
|
||||
for _, col := range columns {
|
||||
liveType, present := live[col.name]
|
||||
if !present {
|
||||
return "missing column " + col.name, nil
|
||||
}
|
||||
|
||||
if !sameDeclaredType(col.typ, liveType) {
|
||||
return fmt.Sprintf(
|
||||
"column %s is %s, schema declares %s",
|
||||
col.name, liveType, col.typ,
|
||||
), nil
|
||||
}
|
||||
}
|
||||
|
||||
return "", nil
|
||||
}
|
||||
|
||||
// liveColumns returns the live table's columns and their declared types,
|
||||
// empty when the table does not exist.
|
||||
func liveColumns(
|
||||
ctx context.Context, db *sql.DB, table string,
|
||||
) (map[string]string, error) {
|
||||
rows, err := db.QueryContext(
|
||||
ctx, "SELECT name, type FROM pragma_table_info(?)", table,
|
||||
)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("could not inspect table %s: %w", table, err)
|
||||
}
|
||||
|
||||
defer func() { _ = rows.Close() }()
|
||||
|
||||
out := make(map[string]string)
|
||||
|
||||
for rows.Next() {
|
||||
var name, typ string
|
||||
|
||||
if err := rows.Scan(&name, &typ); err != nil {
|
||||
return nil, fmt.Errorf("could not scan column of %s: %w", table, err)
|
||||
}
|
||||
|
||||
out[name] = typ
|
||||
}
|
||||
|
||||
if err := rows.Err(); err != nil {
|
||||
return nil, fmt.Errorf("could not read columns of %s: %w", table, err)
|
||||
}
|
||||
|
||||
return out, nil
|
||||
}
|
||||
|
||||
// sameDeclaredType compares two SQLite type names. They are compared
|
||||
// case-insensitively and only on the leading word, so INTEGER matches
|
||||
// INTEGER and VARCHAR(20) matches VARCHAR -- SQLite's affinity rules
|
||||
// make finer distinctions meaningless, and a difference that fine is
|
||||
// not worth retiring a catalog over. An empty declared type matches
|
||||
// anything, which is what a column declared with only constraints has.
|
||||
func sameDeclaredType(declared, live string) bool {
|
||||
d := strings.ToUpper(strings.Fields(declared + " ")[0])
|
||||
l := strings.ToUpper(strings.Fields(live + " ")[0])
|
||||
|
||||
if d == "" || l == "" {
|
||||
return true
|
||||
}
|
||||
|
||||
if i := strings.IndexByte(d, '('); i >= 0 {
|
||||
d = d[:i]
|
||||
}
|
||||
|
||||
if i := strings.IndexByte(l, '('); i >= 0 {
|
||||
l = l[:i]
|
||||
}
|
||||
|
||||
return d == l
|
||||
}
|
||||
|
||||
// declaredTables parses every CREATE TABLE in sql/schemas/ into its
|
||||
// column list. Parsing the schema rather than writing the expectation
|
||||
// down a second time is the point: a second list is a second thing to
|
||||
// forget, which is the fault this whole file exists to repair.
|
||||
func declaredTables() (map[string][]schemaColumn, error) {
|
||||
dirEntries, err := schemas.ReadDir("sql/schemas")
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("could not read schemas directory: %w", err)
|
||||
}
|
||||
|
||||
out := make(map[string][]schemaColumn)
|
||||
|
||||
for _, dirEntry := range dirEntries {
|
||||
if dirEntry.IsDir() {
|
||||
continue
|
||||
}
|
||||
|
||||
content, err := fs.ReadFile(schemas, path.Join("sql/schemas", dirEntry.Name()))
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("could not read %s: %w", dirEntry.Name(), err)
|
||||
}
|
||||
|
||||
maps.Copy(out, parseCreateTables(string(content)))
|
||||
}
|
||||
|
||||
return out, nil
|
||||
}
|
||||
|
||||
// constraintKeywords begin a table constraint rather than a column.
|
||||
var constraintKeywords = map[string]bool{
|
||||
"PRIMARY": true, "FOREIGN": true, "UNIQUE": true,
|
||||
"CHECK": true, "CONSTRAINT": true,
|
||||
}
|
||||
|
||||
// parseCreateTables extracts the column names and declared types of
|
||||
// every non-virtual CREATE TABLE in one schema file.
|
||||
func parseCreateTables(content string) map[string][]schemaColumn {
|
||||
out := make(map[string][]schemaColumn)
|
||||
rest := stripLineComments(content)
|
||||
|
||||
for {
|
||||
idx := indexFold(rest, "CREATE TABLE ")
|
||||
if idx < 0 {
|
||||
return out
|
||||
}
|
||||
|
||||
rest = rest[idx+len("CREATE TABLE "):]
|
||||
|
||||
head, body, ok := splitTableBody(rest)
|
||||
if !ok {
|
||||
return out
|
||||
}
|
||||
|
||||
if name := tableName(head); name != "" {
|
||||
out[name] = parseColumns(body)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// tableName pulls the table name out of the text between "CREATE TABLE"
|
||||
// and its opening parenthesis, dropping an IF NOT EXISTS and any
|
||||
// quoting.
|
||||
func tableName(head string) string {
|
||||
head = strings.TrimSpace(head)
|
||||
head = strings.TrimPrefix(head, "IF NOT EXISTS ")
|
||||
head = strings.TrimPrefix(head, "if not exists ")
|
||||
|
||||
fields := strings.Fields(head)
|
||||
if len(fields) == 0 {
|
||||
return ""
|
||||
}
|
||||
|
||||
return strings.Trim(fields[len(fields)-1], `"'`+"`")
|
||||
}
|
||||
|
||||
// splitTableBody returns the text before the table's opening paren and
|
||||
// the balanced text inside it.
|
||||
func splitTableBody(s string) (head, body string, ok bool) {
|
||||
open := strings.IndexByte(s, '(')
|
||||
if open < 0 {
|
||||
return "", "", false
|
||||
}
|
||||
|
||||
depth := 0
|
||||
|
||||
for i := open; i < len(s); i++ {
|
||||
switch s[i] {
|
||||
case '(':
|
||||
depth++
|
||||
case ')':
|
||||
depth--
|
||||
|
||||
if depth == 0 {
|
||||
return s[:open], s[open+1 : i], true
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
return "", "", false
|
||||
}
|
||||
|
||||
// parseColumns splits a table body on its top-level commas and keeps
|
||||
// the parts that are columns rather than table constraints.
|
||||
func parseColumns(body string) []schemaColumn {
|
||||
var (
|
||||
out []schemaColumn
|
||||
depth int
|
||||
start int
|
||||
)
|
||||
|
||||
parts := make([]string, 0, 8)
|
||||
|
||||
for i := range len(body) {
|
||||
switch body[i] {
|
||||
case '(':
|
||||
depth++
|
||||
case ')':
|
||||
depth--
|
||||
case ',':
|
||||
if depth == 0 {
|
||||
parts = append(parts, body[start:i])
|
||||
start = i + 1
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
parts = append(parts, body[start:])
|
||||
|
||||
for _, part := range parts {
|
||||
fields := strings.Fields(part)
|
||||
if len(fields) == 0 {
|
||||
continue
|
||||
}
|
||||
|
||||
// A table constraint need not be followed by a space --
|
||||
// "UNIQUE(mbid)" is one field, and reading it as a column name
|
||||
// makes an entirely healthy table look stale, which retires a
|
||||
// catalog nobody asked to lose.
|
||||
head := fields[0]
|
||||
if i := strings.IndexByte(head, '('); i >= 0 {
|
||||
head = head[:i]
|
||||
}
|
||||
|
||||
if constraintKeywords[strings.ToUpper(head)] {
|
||||
continue
|
||||
}
|
||||
|
||||
col := schemaColumn{name: strings.Trim(head, `"'`+"`")}
|
||||
if len(fields) > 1 {
|
||||
col.typ = fields[1]
|
||||
}
|
||||
|
||||
out = append(out, col)
|
||||
}
|
||||
|
||||
return out
|
||||
}
|
||||
|
||||
// stripLineComments removes -- comments, which otherwise contribute
|
||||
// stray parentheses and commas to the parse.
|
||||
func stripLineComments(s string) string {
|
||||
lines := strings.Split(s, "\n")
|
||||
for i, line := range lines {
|
||||
if idx := strings.Index(line, "--"); idx >= 0 {
|
||||
lines[i] = line[:idx]
|
||||
}
|
||||
}
|
||||
|
||||
return strings.Join(lines, "\n")
|
||||
}
|
||||
|
||||
// indexFold is a case-insensitive strings.Index.
|
||||
func indexFold(s, substr string) int {
|
||||
return strings.Index(strings.ToUpper(s), strings.ToUpper(substr))
|
||||
}
|
||||
|
||||
// quoteIdent quotes a table name for interpolation into DDL, which
|
||||
// cannot take a bound parameter.
|
||||
func quoteIdent(name string) string {
|
||||
return `"` + strings.ReplaceAll(name, `"`, `""`) + `"`
|
||||
}
|
||||
@@ -0,0 +1,499 @@
|
||||
package database
|
||||
|
||||
import (
|
||||
"context"
|
||||
"database/sql"
|
||||
"log/slog"
|
||||
"path"
|
||||
"testing"
|
||||
|
||||
_ "modernc.org/sqlite"
|
||||
)
|
||||
|
||||
// testLogger discards the repair's warnings; the tests assert on the
|
||||
// database, not on the log.
|
||||
func testLogger() *slog.Logger {
|
||||
return slog.New(slog.DiscardHandler)
|
||||
}
|
||||
|
||||
// openRaw opens a scratch database file with no schema applied, so a
|
||||
// test can build an *old* shape and then let NewDB's repair meet it.
|
||||
func openRaw(t *testing.T, dir string) *sql.DB {
|
||||
t.Helper()
|
||||
|
||||
db, err := sql.Open("sqlite", path.Join(dir, "yj.db"))
|
||||
if err != nil {
|
||||
t.Fatalf("open: %v", err)
|
||||
}
|
||||
|
||||
t.Cleanup(func() { _ = db.Close() })
|
||||
|
||||
return db
|
||||
}
|
||||
|
||||
// TestRetiresIndexMissingAColumn is plan 014's bug, symptom first: an
|
||||
// explore_index created before `total_tracks` existed, met by the
|
||||
// projection every explore read uses. Before the repair this failed
|
||||
// with "no such column: total_tracks" on every install that already had
|
||||
// a catalog, while a fresh one was perfectly healthy.
|
||||
func TestRetiresIndexMissingAColumn(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
dir := t.TempDir()
|
||||
db := openRaw(t, dir)
|
||||
|
||||
// The pre-014 shape: the columns the projection needs, minus the
|
||||
// one the plan added.
|
||||
if _, err := db.ExecContext(ctx, `
|
||||
CREATE TABLE explore_index (
|
||||
id INTEGER PRIMARY KEY AUTOINCREMENT,
|
||||
entity_type INTEGER NOT NULL,
|
||||
mbid BLOB NOT NULL,
|
||||
title TEXT NOT NULL,
|
||||
artist_name TEXT NOT NULL,
|
||||
artist_mbid BLOB NOT NULL
|
||||
);
|
||||
INSERT INTO explore_index (entity_type, mbid, title, artist_name, artist_mbid)
|
||||
VALUES (1, x'00112233445566778899aabbccddeeff', 'x', 'y', x'');
|
||||
`); err != nil {
|
||||
t.Fatalf("seed: %v", err)
|
||||
}
|
||||
|
||||
if err := retireStaleTables(ctx, db, testLogger()); err != nil {
|
||||
t.Fatalf("retire: %v", err)
|
||||
}
|
||||
|
||||
if err := applySchema(ctx, db); err != nil {
|
||||
t.Fatalf("applySchema: %v", err)
|
||||
}
|
||||
|
||||
// The column the projection needs is there now.
|
||||
var n int
|
||||
if err := db.QueryRowContext(ctx,
|
||||
`SELECT COUNT(*) FROM pragma_table_info('explore_index')
|
||||
WHERE name = 'total_tracks'`,
|
||||
).Scan(&n); err != nil {
|
||||
t.Fatalf("inspect: %v", err)
|
||||
}
|
||||
|
||||
if n != 1 {
|
||||
t.Fatalf("explore_index still has no total_tracks column")
|
||||
}
|
||||
|
||||
// And the catalog really was retired rather than patched, so the
|
||||
// artifact is fetched again instead of half a catalog being served.
|
||||
if err := db.QueryRowContext(ctx,
|
||||
"SELECT COUNT(*) FROM explore_index",
|
||||
).Scan(&n); err != nil {
|
||||
t.Fatalf("count: %v", err)
|
||||
}
|
||||
|
||||
if n != 0 {
|
||||
t.Fatalf("stale rows survived the retire: %d", n)
|
||||
}
|
||||
}
|
||||
|
||||
// TestRetiresIndexWithTextMBIDs is the half an ALTER could not have
|
||||
// repaired: plan 013 changed mbid from TEXT to BLOB, and SQLite does not
|
||||
// coerce between them, so a query against 16 raw bytes returns no rows
|
||||
// rather than an error.
|
||||
func TestRetiresIndexWithTextMBIDs(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
dir := t.TempDir()
|
||||
db := openRaw(t, dir)
|
||||
|
||||
if _, err := db.ExecContext(ctx, `
|
||||
CREATE TABLE explore_index (
|
||||
id INTEGER PRIMARY KEY AUTOINCREMENT,
|
||||
entity_type TEXT NOT NULL,
|
||||
mbid TEXT NOT NULL,
|
||||
title TEXT NOT NULL,
|
||||
artist_name TEXT NOT NULL,
|
||||
artist_mbid TEXT NOT NULL,
|
||||
total_tracks INTEGER NOT NULL DEFAULT 0
|
||||
);
|
||||
`); err != nil {
|
||||
t.Fatalf("seed: %v", err)
|
||||
}
|
||||
|
||||
if err := retireStaleTables(ctx, db, testLogger()); err != nil {
|
||||
t.Fatalf("retire: %v", err)
|
||||
}
|
||||
|
||||
if err := applySchema(ctx, db); err != nil {
|
||||
t.Fatalf("applySchema: %v", err)
|
||||
}
|
||||
|
||||
var typ string
|
||||
if err := db.QueryRowContext(ctx,
|
||||
`SELECT type FROM pragma_table_info('explore_index') WHERE name = 'mbid'`,
|
||||
).Scan(&typ); err != nil {
|
||||
t.Fatalf("inspect: %v", err)
|
||||
}
|
||||
|
||||
if typ != "BLOB" {
|
||||
t.Fatalf("mbid is still %s, want BLOB", typ)
|
||||
}
|
||||
}
|
||||
|
||||
// TestRetiringTheIndexTakesItsMetaWithIt guards the thing that makes the
|
||||
// repair actually repair: the marker saying the import finished is what
|
||||
// stops the artifact being fetched again, so a catalog dropped without
|
||||
// it would stay empty forever.
|
||||
func TestRetiringTheIndexTakesItsMetaWithIt(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
dir := t.TempDir()
|
||||
db := openRaw(t, dir)
|
||||
|
||||
if _, err := db.ExecContext(ctx, `
|
||||
CREATE TABLE explore_index (
|
||||
id INTEGER PRIMARY KEY AUTOINCREMENT,
|
||||
entity_type INTEGER NOT NULL,
|
||||
mbid BLOB NOT NULL
|
||||
);
|
||||
CREATE TABLE explore_index_meta (key TEXT PRIMARY KEY, value TEXT NOT NULL);
|
||||
INSERT INTO explore_index_meta VALUES ('dump_import_done', '1');
|
||||
`); err != nil {
|
||||
t.Fatalf("seed: %v", err)
|
||||
}
|
||||
|
||||
if err := retireStaleTables(ctx, db, testLogger()); err != nil {
|
||||
t.Fatalf("retire: %v", err)
|
||||
}
|
||||
|
||||
if err := applySchema(ctx, db); err != nil {
|
||||
t.Fatalf("applySchema: %v", err)
|
||||
}
|
||||
|
||||
var n int
|
||||
if err := db.QueryRowContext(ctx,
|
||||
"SELECT COUNT(*) FROM explore_index_meta WHERE key = 'dump_import_done'",
|
||||
).Scan(&n); err != nil {
|
||||
t.Fatalf("meta: %v", err)
|
||||
}
|
||||
|
||||
if n != 0 {
|
||||
t.Fatalf("the import-done marker survived a retired catalog")
|
||||
}
|
||||
}
|
||||
|
||||
// TestHealthyDatabaseIsUntouched is the other half, and the one that
|
||||
// would make this dangerous if it failed: a current schema must survive
|
||||
// a launch with its catalog intact. A repair that retires a healthy
|
||||
// catalog costs every user an artifact download on every start.
|
||||
func TestHealthyDatabaseIsUntouched(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
dir := t.TempDir()
|
||||
db := openRaw(t, dir)
|
||||
|
||||
if err := applySchema(ctx, db); err != nil {
|
||||
t.Fatalf("applySchema: %v", err)
|
||||
}
|
||||
|
||||
if _, err := db.ExecContext(ctx, `
|
||||
INSERT INTO explore_index (entity_type, mbid, title, artist_name, artist_mbid)
|
||||
VALUES (1, x'00112233445566778899aabbccddeeff', 'x', 'y', x'')
|
||||
`); err != nil {
|
||||
t.Fatalf("seed: %v", err)
|
||||
}
|
||||
|
||||
if err := retireStaleTables(ctx, db, testLogger()); err != nil {
|
||||
t.Fatalf("retire: %v", err)
|
||||
}
|
||||
|
||||
var n int
|
||||
if err := db.QueryRowContext(ctx,
|
||||
"SELECT COUNT(*) FROM explore_index",
|
||||
).Scan(&n); err != nil {
|
||||
t.Fatalf("count: %v", err)
|
||||
}
|
||||
|
||||
if n != 1 {
|
||||
t.Fatalf("a healthy catalog was retired: %d rows left", n)
|
||||
}
|
||||
}
|
||||
|
||||
// TestAuthoredTablesAreNeverRetired states the boundary in a test rather
|
||||
// than only in a comment: this mechanism deletes data, and the only
|
||||
// thing standing between it and a user's playlists is the Kind filter.
|
||||
func TestAuthoredTablesAreNeverRetired(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
dir := t.TempDir()
|
||||
db := openRaw(t, dir)
|
||||
|
||||
// A playlists table missing most of its current columns.
|
||||
if _, err := db.ExecContext(ctx, `
|
||||
CREATE TABLE playlists (id INTEGER PRIMARY KEY, name TEXT NOT NULL);
|
||||
INSERT INTO playlists (name) VALUES ('irreplaceable');
|
||||
`); err != nil {
|
||||
t.Fatalf("seed: %v", err)
|
||||
}
|
||||
|
||||
if err := retireStaleTables(ctx, db, testLogger()); err != nil {
|
||||
t.Fatalf("retire: %v", err)
|
||||
}
|
||||
|
||||
var n int
|
||||
if err := db.QueryRowContext(ctx,
|
||||
"SELECT COUNT(*) FROM playlists",
|
||||
).Scan(&n); err != nil {
|
||||
t.Fatalf("count: %v", err)
|
||||
}
|
||||
|
||||
if n != 1 {
|
||||
t.Fatalf("an authored table was retired; rows left: %d", n)
|
||||
}
|
||||
}
|
||||
|
||||
// TestRetiresTablesTheSchemaNoLongerDescribes covers what plan 013 left
|
||||
// behind on every database that predates it: seven tables the schema
|
||||
// stopped describing, plus the schema_migrations table that squashing
|
||||
// the chain retired. They are not stale in shape — they are simply not
|
||||
// ours any more.
|
||||
func TestRetiresTablesTheSchemaNoLongerDescribes(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
db := openRaw(t, t.TempDir())
|
||||
|
||||
if _, err := db.ExecContext(ctx, `
|
||||
CREATE TABLE recordings (id INTEGER PRIMARY KEY, name TEXT);
|
||||
CREATE TABLE artist_credit (id INTEGER PRIMARY KEY, text TEXT);
|
||||
CREATE TABLE schema_migrations (version INTEGER PRIMARY KEY);
|
||||
`); err != nil {
|
||||
t.Fatalf("seed: %v", err)
|
||||
}
|
||||
|
||||
if err := retireStaleTables(ctx, db, testLogger()); err != nil {
|
||||
t.Fatalf("retire: %v", err)
|
||||
}
|
||||
|
||||
for _, table := range []string{"recordings", "artist_credit", "schema_migrations"} {
|
||||
var n int
|
||||
|
||||
if err := db.QueryRowContext(ctx,
|
||||
"SELECT COUNT(*) FROM sqlite_master WHERE type='table' AND name = ?",
|
||||
table,
|
||||
).Scan(&n); err != nil {
|
||||
t.Fatalf("inspect %s: %v", table, err)
|
||||
}
|
||||
|
||||
if n != 0 {
|
||||
t.Errorf("%s survived; the schema no longer describes it", table)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// TestFTSShadowTablesAreNotObsolete is the sweep's sharp edge: an FTS5
|
||||
// virtual table is backed by four shadow tables that appear in
|
||||
// sqlite_master under their own names and are in no schema file.
|
||||
// Dropping one destroys the index it belongs to.
|
||||
func TestFTSShadowTablesAreNotObsolete(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
db := openRaw(t, t.TempDir())
|
||||
|
||||
if err := applySchema(ctx, db); err != nil {
|
||||
t.Fatalf("applySchema: %v", err)
|
||||
}
|
||||
|
||||
obsolete, err := obsoleteTables(ctx, db)
|
||||
if err != nil {
|
||||
t.Fatalf("obsoleteTables: %v", err)
|
||||
}
|
||||
|
||||
if len(obsolete) != 0 {
|
||||
t.Fatalf("a freshly created schema reported obsolete tables: %v", obsolete)
|
||||
}
|
||||
}
|
||||
|
||||
// TestRetiringOwnedTablesDoesNotDangle pins the one behaviour that is
|
||||
// silently wrong rather than loudly broken.
|
||||
//
|
||||
// Retiring audio_files leaves playlist entries behind. With
|
||||
// foreign_keys ON — which applyPRAGMAs has done before NewDB gets here —
|
||||
// SET NULL fires and they point at nothing. With it OFF they keep ids
|
||||
// that the rescan reissues from 1, so every playlist quietly fills with
|
||||
// different songs. Nothing about the schema makes that ordering
|
||||
// obvious, so it is asserted rather than assumed.
|
||||
func TestRetiringOwnedTablesDoesNotDangle(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
db := openRaw(t, t.TempDir())
|
||||
|
||||
if _, err := db.ExecContext(ctx, "PRAGMA foreign_keys = ON"); err != nil {
|
||||
t.Fatalf("pragma: %v", err)
|
||||
}
|
||||
|
||||
if err := applySchema(ctx, db); err != nil {
|
||||
t.Fatalf("applySchema: %v", err)
|
||||
}
|
||||
|
||||
// Break audio_files' shape so it is retired, keeping a playlist
|
||||
// entry that references it.
|
||||
if _, err := db.ExecContext(ctx, `
|
||||
INSERT INTO playlists (id, name) VALUES (1, 'keepme');
|
||||
INSERT INTO libraries (id, name, path) VALUES (0, 'test', '/music');
|
||||
INSERT INTO audio_files (id, file_path, file_type_id, length_milliseconds)
|
||||
VALUES (7, '/music/a.flac', 1, 1000);
|
||||
INSERT INTO playlist_tracks (playlist_id, audio_file_id, position)
|
||||
VALUES (1, 7, 0);
|
||||
DROP VIEW IF EXISTS track_metadata;
|
||||
ALTER TABLE audio_files DROP COLUMN artist_credit;
|
||||
`); err != nil {
|
||||
t.Fatalf("seed: %v", err)
|
||||
}
|
||||
|
||||
if err := retireStaleTables(ctx, db, testLogger()); err != nil {
|
||||
t.Fatalf("retire: %v", err)
|
||||
}
|
||||
|
||||
if err := applySchema(ctx, db); err != nil {
|
||||
t.Fatalf("applySchema: %v", err)
|
||||
}
|
||||
|
||||
var dangling int
|
||||
if err := db.QueryRowContext(ctx,
|
||||
"SELECT COUNT(*) FROM playlist_tracks WHERE audio_file_id IS NOT NULL",
|
||||
).Scan(&dangling); err != nil {
|
||||
t.Fatalf("count: %v", err)
|
||||
}
|
||||
|
||||
if dangling != 0 {
|
||||
t.Fatalf(
|
||||
"%d playlist entries still point at retired audio_files ids; "+
|
||||
"a rescan will reissue those ids to different tracks",
|
||||
dangling,
|
||||
)
|
||||
}
|
||||
|
||||
// The playlist itself is authored and must be untouched.
|
||||
var playlists int
|
||||
if err := db.QueryRowContext(ctx,
|
||||
"SELECT COUNT(*) FROM playlists",
|
||||
).Scan(&playlists); err != nil {
|
||||
t.Fatalf("playlists: %v", err)
|
||||
}
|
||||
|
||||
if playlists != 1 {
|
||||
t.Fatalf("authored playlist lost: %d", playlists)
|
||||
}
|
||||
}
|
||||
|
||||
// TestRetiringInterlinkedLegacyTables is the bug the unit tests missed
|
||||
// and a real database found.
|
||||
//
|
||||
// The tables plan 013 retired reference each other -- pre-013
|
||||
// audio_files has a foreign key into recordings -- so with foreign keys
|
||||
// ON, dropping them one at a time fails with "FOREIGN KEY constraint
|
||||
// failed" on whichever goes first, and map iteration order decides
|
||||
// which that is. Every other test in this file ran with foreign keys
|
||||
// off and passed happily; the app enables them in applyPRAGMAs before
|
||||
// the repair runs, so only the real launch path showed it.
|
||||
func TestRetiringInterlinkedLegacyTables(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
db := openRaw(t, t.TempDir())
|
||||
|
||||
if _, err := db.ExecContext(ctx, "PRAGMA foreign_keys = ON"); err != nil {
|
||||
t.Fatalf("pragma: %v", err)
|
||||
}
|
||||
|
||||
// The pre-013 shape, with the reference that makes ordering matter.
|
||||
// release_group_recordings sorts *after* recordings and references
|
||||
// it, so the deterministic order retires the parent while the child
|
||||
// still holds rows pointing at it -- which is the case that fails
|
||||
// without the deferral, rather than one that fails on some runs.
|
||||
if _, err := db.ExecContext(ctx, `
|
||||
CREATE TABLE recordings (id INTEGER PRIMARY KEY, name TEXT);
|
||||
CREATE TABLE artist_credit (id INTEGER PRIMARY KEY, text TEXT);
|
||||
CREATE TABLE release_group_recordings (
|
||||
id INTEGER PRIMARY KEY,
|
||||
recording_id INTEGER NOT NULL,
|
||||
FOREIGN KEY(recording_id) REFERENCES recordings(id)
|
||||
);
|
||||
CREATE TABLE audio_files (
|
||||
id INTEGER PRIMARY KEY,
|
||||
file_path TEXT NOT NULL UNIQUE,
|
||||
recording_id INTEGER,
|
||||
FOREIGN KEY(recording_id) REFERENCES recordings(id)
|
||||
);
|
||||
INSERT INTO recordings (id, name) VALUES (1, 'x');
|
||||
INSERT INTO release_group_recordings (id, recording_id) VALUES (1, 1);
|
||||
INSERT INTO audio_files (id, file_path, recording_id)
|
||||
VALUES (1, '/music/a.flac', 1);
|
||||
`); err != nil {
|
||||
t.Fatalf("seed: %v", err)
|
||||
}
|
||||
|
||||
if err := retireStaleTables(ctx, db, testLogger()); err != nil {
|
||||
t.Fatalf("retire: %v", err)
|
||||
}
|
||||
|
||||
if err := applySchema(ctx, db); err != nil {
|
||||
t.Fatalf("applySchema: %v", err)
|
||||
}
|
||||
|
||||
for _, table := range []string{"recordings", "artist_credit"} {
|
||||
var n int
|
||||
|
||||
if err := db.QueryRowContext(ctx,
|
||||
"SELECT COUNT(*) FROM sqlite_master WHERE type='table' AND name = ?",
|
||||
table,
|
||||
).Scan(&n); err != nil {
|
||||
t.Fatalf("inspect %s: %v", table, err)
|
||||
}
|
||||
|
||||
if n != 0 {
|
||||
t.Errorf("%s survived the retire", table)
|
||||
}
|
||||
}
|
||||
|
||||
// And the rebuilt audio_files is the current shape, which is the
|
||||
// whole reason the old one had to go.
|
||||
var n int
|
||||
if err := db.QueryRowContext(ctx,
|
||||
`SELECT COUNT(*) FROM pragma_table_info('audio_files')
|
||||
WHERE name = 'artist_credit'`,
|
||||
).Scan(&n); err != nil {
|
||||
t.Fatalf("inspect audio_files: %v", err)
|
||||
}
|
||||
|
||||
if n != 1 {
|
||||
t.Fatal("audio_files was not rebuilt in the current shape")
|
||||
}
|
||||
}
|
||||
|
||||
// TestParseCreateTablesReadsTheRealSchema keeps the parser honest
|
||||
// against the files it actually runs on: a parser that silently found
|
||||
// no columns would report every table healthy and repair nothing.
|
||||
func TestParseCreateTablesReadsTheRealSchema(t *testing.T) {
|
||||
declared, err := declaredTables()
|
||||
if err != nil {
|
||||
t.Fatalf("declaredTables: %v", err)
|
||||
}
|
||||
|
||||
cols, ok := declared["explore_index"]
|
||||
if !ok {
|
||||
t.Fatal("explore_index was not parsed out of the schema files")
|
||||
}
|
||||
|
||||
want := map[string]string{
|
||||
"mbid": "BLOB",
|
||||
"total_tracks": "INTEGER",
|
||||
"artist_name": "TEXT",
|
||||
}
|
||||
|
||||
got := make(map[string]string, len(cols))
|
||||
for _, c := range cols {
|
||||
got[c.name] = c.typ
|
||||
}
|
||||
|
||||
for name, typ := range want {
|
||||
if got[name] != typ {
|
||||
t.Errorf("explore_index.%s parsed as %q, want %q", name, got[name], typ)
|
||||
}
|
||||
}
|
||||
|
||||
// A table constraint must not be mistaken for a column.
|
||||
for _, c := range cols {
|
||||
switch c.name {
|
||||
case "PRIMARY", "FOREIGN", "UNIQUE", "CHECK", "CONSTRAINT":
|
||||
t.Errorf("parsed table constraint %q as a column", c.name)
|
||||
}
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user