diff --git a/.planning/plans/active/015-multi-artist-credits.md b/.planning/plans/active/015-multi-artist-credits.md new file mode 100644 index 0000000..7ac3c23 --- /dev/null +++ b/.planning/plans/active/015-multi-artist-credits.md @@ -0,0 +1,337 @@ +# 015 — Multi-artist credits, navigable + +## The problem + +A track credited to more than one artist has exactly one navigable +artist in this app, and the others are punctuation. + +`audio_files` carries `artist_credit` (the credit as tagged, for +display) and `artist_id` (one artist, for grouping and browsing). +`primaryArtist()` (`backend/library/artistcredit.go:53`) resolves that +one artist by *string-parsing* the credit: it strips a " feat. " +clause, and deliberately does not split on `&`, `x`, `with` or `,` +because those appear inside real artist names. So "Lana Del Rey ft. +Sean Lennon" stores Lana Del Rey and discards Sean Lennon entirely, +and "Alina Baraz & Galimatias" stores one artist whose name is the +whole credit. + +### What the measurement says + +Measured 2026-08-16 against a real 26,069-file library (19,840 mp3, +6,229 flac; 57 unreadable, m4a/ogg not examined), plus an 80+80 +MusicBrainz `inc=artist-credits` sample. + +- **13%** of a random sample of the library's recordings have more + than one credited artist in MusicBrainz (10 of 79 resolved). + Extrapolates to ~3,250 of the 24,989 files carrying a recording + MBID. +- **0.86%** of files (224) carry any structured multi-artist signal in + their own tags. mp3 carries **zero** files with multiple + `MUSICBRAINZ_ARTISTID` values across 19,840 files; flac has 87. +- **1,286** files say "feat." in `ARTIST`; **1,159 of them (90%)** + have nothing structured behind it. A sample of 80 such files was + multi-artist in MB **80 of 80 times**. + +CLAUDE.md currently justifies plan 013's removal of `artist_credit` / +`artist_credit_artist` with "3 credits of 2,823 listed more than one +artist". That figure measured **our own writer**, not the library: +`cachedLinkArtist` was called exactly once per credit +(`e7748f1^:backend/library/library.go:1842`), so a collaboration could +never have been recorded, and the three were resolution collisions on +shared credit text. Dropping the join table was still correct — it only +ever held one row, so it was pure join cost — but the stated evidence +does not support "multi-artist is rare". Correcting that claim is part +of this plan. + +### Why the tags cannot answer it + +Deriving the decomposition locally, with no network, works **79% of the +time** (169 of 215 files with a multi-value `ARTISTS` tag: mp3 69/105, +flac 100/110), and the failures are systematic rather than random: + +``` +ARTIST = '2Pac feat. Snoop Dogg, Nate Dogg, Hussein Fatal & Yaki Kadafi' +ARTISTS = ['2Pac', 'Snoop Doggy Dogg', 'Nate Dogg', 'Fatal', 'Yaki Kadafi'] +``` + +`ARTISTS` holds **canonical** artist names; `ARTIST` holds +**as-credited** names. Locating one inside the other fails on +"Snoop Doggy Dogg" vs "Snoop Dogg", on "Fatal" vs "Hussein Fatal", and +on Unicode (`Michel'le` vs `Michel’le`, `K-Ci` vs `K‐Ci` — U+2010, not +a hyphen). That distinction is precisely what a join phrase encodes, +and it is why this cannot be a tag-parsing feature. + +Two format details that will mislead anyone re-running the probe: +Picard writes `ARTISTS` **slash-joined into one TXXX frame** on mp3 and +as **true repeated Vorbis keys** on flac, so a probe splitting only on +NUL undercounts mp3 to zero. + +## The shape + +MusicBrainz models a credit as ordered parts, and the credit *string* +is derived from them — `artist_credit.name` is a cached render, nothing +more. Each participant is `(position, artist, name, join_phrase)`, +where `artist` is the MBID (canonical, what you navigate to) and `name` +is the credited spelling (what you display). + +**Join phrases are assembly instructions, not disassembly +instructions.** Rendering is a concatenation, never a search: + +``` +for each (position, artist_mbid, credited_name, join_phrase): + emit link(credited_name -> artist_mbid) + emit text(join_phrase) +``` + +The link positions are known **by construction**. This is load-bearing: +if we instead located each `credited_name` inside the stored +`artist_credit` text, we would reintroduce the mismatch above — the +stored string may have come from the tags while the parts come from the +catalog, and those **disagree for ~1 in 3 multi-artist files** (61 of +90 sampled credits rendered exactly equal to the tag string). +Divergences seen: `'Skrillex feat. Swae Lee'` tagged vs +`'Skrillex & Swae Lee'` in MB; `'STRFKR'` vs `'Starfucker'`; +`'Zedd feat. Hayley Williams'` vs `'... of Paramore'`. Either MB was +edited after tagging or Picard versions differ; either way the search +would miss or match the wrong span. + +So `audio_files.artist_credit` stops being the source of truth and +becomes the **fallback**, used only where there are no parts. + +## Where the data comes from + +The catalog carries the decomposition; no user ever makes a +per-recording call. Two sources were ruled out first, both cheaply: + +- **The canonical dump — which is what CI already pulls + (`dumpimport.go:84-85`) — does not have it.** + `canonical_musicbrainz_data.csv` gives `artist_mbids` (ordered list) + and `artist_credit_name`, but that last column is the *rendered* + string. Splitting it on CI needs the as-credited names, so CI would + fail exactly the way a local parse does. +- **The JSON dumps do not cover the catalog.** + `json-dumps/recording.tar.xz` is 31 MB / 368 MB uncompressed and + holds **153,691 recordings**, not ~35M. Measured against the test + library's 24,885 recording MBIDs: **0.00% overlap, zero rows**. It is + some other subset and is not usable. + +That leaves the core dump, **`mbdump.tar.bz2`** (7.1 GB compressed at +the 20260815 export), from +`https://data.metabrainz.org/pub/musicbrainz/data/fullexport/`. Four +members are needed: + +| member | why | approx rows | +| --- | --- | --- | +| `mbdump/artist_credit_name` | `(artist_credit, position, artist, name, join_phrase)` — the payload | ~4M | +| `mbdump/artist` | `id -> gid`, since the above references artist *row ids* | ~2.6M | +| `mbdump/recording` | `gid -> artist_credit`, to key credits by recording MBID | ~35M | +| `mbdump/release_group` | same, for album credits | ~2M | + +### Coverage is not a concern + +Of 24,885 distinct recording MBIDs in the test library, **24,808 +(99.7%)** already have an `explore_index` recording row, measured +against a database at 2,052,200 rows — i.e. shipped-artifact coverage, +not a local build's. The popularity filter does not strand the long +tail here. + +## Status + +- **Phase 1 — done.** `backend/explore/dumpcredits.go` + + `dumpcreditswrite.go`, wired into `dumpimport.go`'s `run` behind its + own `credits_import_done` marker. +- **Phase 2 — done.** `cmd/indexexport` writes the two tables; + `artifactimport.go` reads them behind `artifactHasCredits()`. +- **Phase 4 — done, and it does not need Phase 3.** `explore.GetCredits` + reads the catalog tables keyed on the *recording* MBID, which both + sides of the app already carry — a catalog row has one and so does a + local file (`library.Track.RecordingMBID`). So one binding serves the + Explore pages and the library's own lists, and all ten artist-link + call sites render credits today without a local table. +- **Phase 3 (`file_artists`) — not started, and now an + offline-resilience task rather than a prerequisite.** The table is + deliberately *not* declared yet: nothing writes or reads it, and a + schema file plus a datamap note describing behaviour that does not + exist is a claim the code cannot back. Its remaining + value is that credits currently vanish when the catalog is absent or + still downloading, which is precisely the `no-index` state + `ShelfPage.State` exists to describe. Materialising into + `file_artists` is what makes a library stand on its own. + +**Nothing renders yet in practice**, because no published artifact +carries credit tables — every credit falls back to its single link +until an index build with Phase 1 runs and is exported. + +**Column layouts are verified against the real 20260815 export**, not +taken from the schema docs — `artist(id, gid, …)`, +`artist_credit(id, name, artist_count, …)`, +`artist_credit_name(credit, position, artist, name, join_phrase)` and +`recording(id, gid, name, artist_credit, …)` were each read out of the +dump. `release_group` shares `recording`'s first four columns and is +the one layout still taken on trust; `ErrDumpShape` turns a wrong guess +into a loud failure rather than a quietly wrong catalog. + +**Still unrun: the ingest against the real 7.1 GB dump.** Everything is +covered by tests over a synthetic tar, which cannot catch a surprise in +the other ~35M rows. + +### Phase 1 — Ingest credits on CI + +New dump stage in `cmd/indexbuild`, behind the `indexbuild` tag with +the rest of `dumpimport.go`'s stages. + +**Constraint from `b98840e`:** `cmd/indexbuild` is built +`CGO_ENABLED=0` in a plain `golang` container and must not reach the +Wails `application` package — `TestIndexToolsDoNotImportWails` walks +`go list -deps -tags indexbuild`. Nothing here should need it, but a +new `ServiceStartup` hook on a package this imports is how it comes +back. Go's `compress/bzip2` is pure Go and decompress-only, which is +all this needs. + +**Measured, 20260815 export.** Tar members are **alphabetical**, and +that is favourable: `artist` (435 MB), `artist_credit` (414 MB) and +`artist_credit_name` (237 MB) all fall inside the first ~900 MB +compressed, while `recording` and `release_group` come later. So the +maps are complete before the rows that consume them arrive, and no +recording data is ever buffered. + +Pure-Go `compress/bzip2` decompresses at **26 MB/s uncompressed / +8.7 MB/s compressed** (measured on a 250 MB prefix, 3.01x ratio) — +**~13.7 min** for the whole file single-threaded, and less because the +stream can stop after `release_group` rather than reading the +`series`/`tag`/`track`/`url`/`work` tail. The 2 MB/s origin throttle +dominates, as it already does for every other dump here. + +Do not, however, *depend* on the ordering: assert it and fall back to +buffering if a future export reorders, rather than silently emitting +nothing. + +- `artist` -> `map[int32]uuid16` (~2.6M x ~20 B = ~60 MB) +- `artist_credit_name` -> `map[int32][]creditPart` (~4M x ~40 B = + ~200 MB) +- `recording` / `release_group` -> emit `gid -> credit_id` **only for + MBIDs already in `explore_index`** (the kept set is ~1.4M x 16 B = + ~22 MB), which is what keeps 35M rows from being held + +Peak ~300 MB, one sequential pass. + +**Only multi-artist credits are stored.** A single-artist credit is +`(name, "")` and is already fully described by `explore_index`'s +`artist_name` / `artist_mbid`; storing it would triple the table for +nothing. Post-filter after loading, once the row count per credit is +known. + +New tables (and `datamap` entries, or `TestCatalogCoversSchema` fails +the build — both are `Cache`, matching `explore_index`): + +``` +artist_credit_part(credit_id, position, artist_mbid, credited_name, join_phrase) +``` + +with `explore_index.artist_credit_id` as the link. Credits are +**shared** — an album's twelve tracks by one artist share one credit +row — which is the opposite of 013's local verdict, and correctly so: +1:1 in a local library, genuinely many-to-one at 2M-row catalog scale. + +### Phase 2 — Ship them in the artifact + +`cmd/indexexport` currently creates exactly two tables in the artifact +(`explore_index`, `artifact_meta`, at `cmd/indexexport/*.go:147,170`), +so this is a structural addition, not a column. + +Estimated size: ~13% of 1.4M recordings, deduplicated by shared credit, +at ~2.3 parts each — order 400k rows, ~18 MB uncompressed. Against a +~0.6 GB install that is acceptable; it must be measured rather than +assumed before merge. + +`artifactimport.go` must read it **only if present**, on the writer +handle where `core` is attached — the `artifactHasTotals()` / +`artifactStoresText()` pattern (`artifactimport.go:145-175`), one step +up from a column to a table. An artifact published before this exists +is still a perfectly good catalog and must import as one that declines +to answer. Adding this to the importer's SELECT list without the probe +is how every already-published artifact starts failing. + +`artifactCatalogColumns` gains `artist_credit_id`; it is kept in sync +with the exporter by `TestArtifactColumnsMatchExporter`. + +### Phase 3 — Materialize locally + +``` +file_artists(audio_file_id, position, artist_id, credited_name, join_phrase) +``` + +`credited_name` is stored **per row**, not looked up from +`artists.name` — that is the Snoop-Doggy-Dogg distinction, and it is +the whole point. + +Filled at scan/import time by joining `audio_files.recording_mbid` +against the catalog. **Materialized rather than resolved live**, +because the catalog is a downloaded artifact that can be absent or +still arriving — that is why `ShelfPage.State` has a `no-index` value — +and a library whose track rows lose their artists when the catalog is +missing is worse than today. + +That implies a backfill for the case where the catalog arrives *after* +the library was scanned. It registers with `jobs` (progress, cancel) +like every other long pass, and takes a **distinct kind** from +`index-build`, since `job-controls.ts` keys its "you will discard hours +of downloading" confirmation on that kind. + +`artists` gains rows for guests who own no files. **This changes what +the artists grid shows** and is an open question below. + +### Phase 4 — Render + +`utils/explore-link.ts` gains a credit-rendering entry point taking +ordered parts and returning a `TemplateResult`. Every row and detail +view already renders artist names through it, so they inherit +multi-artist links without individually knowing credits exist — the +property that made centralising it worthwhile. + +Its existing fallback philosophy already covers the no-parts case: "a +list where some rows are clickable and others silently are not reads as +a bug, not as a statement about metadata." Where there are no parts +(no recording MBID, or no catalog row — ~4% of the test library) render +today's behaviour: the flat `artist_credit` string with one link to the +primary artist. **Do not split the string there.** There is genuinely +no information to split on, and that is the one place the temptation +returns. + +`primaryArtist()` stays exactly as it is. It remains the fallback and +is still what `artist_id` means. + +## Open questions + +1. **Catalog credit vs tagged credit, when they disagree** (~1 in 3 + multi-artist files). Rendering the catalog's decomposition is what + makes names navigable; preserving the file's is what makes the app + reflect the user's files. Leaning toward: render the catalog + decomposition, keep `artist_credit` as the fallback string. Wants a + deliberate decision, not an accident. +2. **Do guest artists appear in the artists grid?** Phase 3 creates + `artists` rows for people who own no files. The grid currently means + "artists in your library" and joins `audio_files`. A guest on one + track is arguably in the library and arguably not. Whichever way, + the ownership question stays "is there a file" — that rule does not + bend. +3. **`release_group` credits** are ingested in the same pass for + nearly nothing, but album-artist rendering is a separate surface. + Ship the data in phase 1, render in a follow-up rather than widening + phase 4. +4. **Our own `tagwriter`** does not write `ARTISTS` or multiple + `MUSICBRAINZ_ARTISTID` frames, so autotagging a folder degrades the + very field this rests on — the same shape as the existing + track-totals note. Out of scope here; worth recording. + +## Verification + +- Coverage: re-run the library probe and assert `file_artists` is + populated for ~13% of files, not ~0.9%. +- `TestCatalogCoversSchema` / `TestLifetimesMatchSchema` for the new + tables. +- `TestIndexToolsDoNotImportWails` still passes with the new stage. +- An artifact **without** the credits table imports cleanly (the + `artifactHasTotals` regression shape). +- Round-trip: a known multi-artist recording renders each name as a + separate link with the correct join phrases between them. diff --git a/backend/database/sql/schemas/artist_credit_part.sql b/backend/database/sql/schemas/artist_credit_part.sql new file mode 100644 index 0000000..e12de1a --- /dev/null +++ b/backend/database/sql/schemas/artist_credit_part.sql @@ -0,0 +1,56 @@ +-- The decomposition of a multi-artist credit, from the MusicBrainz +-- dump. One row per credited artist, in credit order. +-- +-- A credit is ordered parts, and the credit *string* is derived from +-- them -- MusicBrainz's own `artist_credit.name` is a cached render and +-- nothing more. Rendering is a concatenation: +-- +-- for each part in position order: +-- emit link(credited_name -> artist_mbid) +-- emit text(join_phrase) +-- +-- so the link boundaries are known by construction. That is the whole +-- reason this table exists, and it is why nothing may reconstruct a +-- credit by *searching* for a name inside a credit string: the stored +-- string may have come from a file's tags while the parts come from the +-- catalog, and measured on a real library those disagree for about one +-- in three multi-artist credits ("Skrillex feat. Swae Lee" tagged +-- against "Skrillex & Swae Lee" upstream). A search would miss, or +-- match the wrong span. +-- +-- `credited_name` is the name *as credited*, which is not the artist's +-- canonical name: MusicBrainz credits "Snoop Dogg" on a track by the +-- artist whose name is "Snoop Doggy Dogg". It is stored per row rather +-- than joined from an artist table for exactly that reason. +-- +-- Only *multi-artist* credits are stored. A single-artist credit is +-- (name, "") and is already fully described by explore_index's +-- artist_name and artist_mbid; storing those would roughly triple the +-- table to say nothing new. +-- +-- Credits are shared: an album's twelve tracks by one artist reference +-- one credit_id. That is the opposite of the local library's verdict +-- in plan 013, and correctly so -- credit sharing is 1:1 in one +-- person's files and genuinely many-to-one across a 2M-row catalog. +-- +-- MBIDs are the same 16 raw bytes explore_index stores, for the same +-- size reason and with the same CHECK, so a stringly write fails at the +-- insert that made it rather than reading back as no rows at all. See +-- backend/explore/mbid.go. +CREATE TABLE IF NOT EXISTS artist_credit_part ( + credit_id INTEGER NOT NULL, + position INTEGER NOT NULL, + artist_mbid BLOB NOT NULL CHECK(length(artist_mbid) = 16), + + -- The name as credited on this release, which may differ from the + -- artist's canonical name. Display uses this; navigation uses the + -- MBID above. + credited_name TEXT NOT NULL, + + -- The literal connector that follows this part -- " feat. ", " & ", + -- ", ", or "" on the last part. Rendered as plain text between two + -- links. + join_phrase TEXT NOT NULL DEFAULT '', + + PRIMARY KEY (credit_id, position) +) WITHOUT ROWID; diff --git a/backend/database/sql/schemas/artist_credit_ref.sql b/backend/database/sql/schemas/artist_credit_ref.sql new file mode 100644 index 0000000..8a06c80 --- /dev/null +++ b/backend/database/sql/schemas/artist_credit_ref.sql @@ -0,0 +1,30 @@ +-- Which credit a catalog entity is credited to. One row per recording +-- or release group whose credit names more than one artist. +-- +-- This is a table rather than an `explore_index.artist_credit_id` +-- column, and that is a deliberate consequence of how this app applies +-- its schema. `applySchema` is CREATE ... IF NOT EXISTS and there is +-- no migration chain (plan 013), so a *column* added to an existing +-- table never reaches a database that already has it -- while a new +-- *table* is created on every install, old or new, for free. +-- explore_index is the one table nobody can afford to drop and rebuild +-- on a schema change: it is the artifact users download rather than +-- derive. +-- +-- Only multi-artist credits are referenced here, matching +-- artist_credit_part. An entity with no row is credited to exactly one +-- artist, which explore_index's own artist_name and artist_mbid already +-- describe -- so absence is the common case and means "nothing to +-- decompose", not "unknown". +-- +-- `credit_id` is opaque and is only meaningful against the +-- artist_credit_part rows built or imported alongside it. The two are +-- always written together; nothing persists a credit_id anywhere else. +-- The local library stores resolved parts, never this id. +CREATE TABLE IF NOT EXISTS artist_credit_ref ( + mbid BLOB NOT NULL PRIMARY KEY CHECK(length(mbid) = 16), + credit_id INTEGER NOT NULL +) WITHOUT ROWID; + +CREATE INDEX IF NOT EXISTS idx_artist_credit_ref_credit + ON artist_credit_ref(credit_id); diff --git a/backend/database/sql/sqlcgen/models.go b/backend/database/sql/sqlcgen/models.go index 5be6ad8..c210de6 100644 --- a/backend/database/sql/sqlcgen/models.go +++ b/backend/database/sql/sqlcgen/models.go @@ -27,6 +27,19 @@ type Artist struct { Mbid sql.NullString } +type ArtistCreditPart struct { + CreditID int64 + Position int64 + ArtistMbid []byte + CreditedName string + JoinPhrase string +} + +type ArtistCreditRef struct { + Mbid []byte + CreditID int64 +} + type ArtistEnrichment struct { ArtistMbid string BrowsedAt sql.NullTime diff --git a/backend/datamap/datamap.go b/backend/datamap/datamap.go index 3978747..d77215a 100644 --- a/backend/datamap/datamap.go +++ b/backend/datamap/datamap.go @@ -185,6 +185,21 @@ var tables = []Table{ Note: "Full-text index over the champion entities of the " + "MusicBrainz dump. Rebuilt only by a full index build.", }, + { + Name: "artist_credit_part", Kind: Cache, Lifetime: Retained, + Note: "The decomposition of a multi-artist credit, from the " + + "MusicBrainz dump: one row per credited artist, with the " + + "name as credited and the join phrase that follows it. " + + "Arrives with the downloaded artifact, so rebuilding it " + + "costs a dump stream and it is never swept.", + }, + { + Name: "artist_credit_ref", Kind: Cache, Lifetime: Retained, + Note: "Which credit a catalog recording or release group is " + + "credited to. Present only for multi-artist credits; " + + "absence means one artist, which explore_index already " + + "describes. Ships and dies with artist_credit_part.", + }, { Name: "explore_index", Kind: Cache, Lifetime: Retained, Note: "The offline MusicBrainz search index. Rebuilding costs a " + diff --git a/backend/explore/artifactimport.go b/backend/explore/artifactimport.go index 8a5d854..49f92ca 100644 --- a/backend/explore/artifactimport.go +++ b/backend/explore/artifactimport.go @@ -283,6 +283,9 @@ func (si *SearchIndex) importCoreArtifact(ctx context.Context, path string) erro } merged, mergeErr := si.mergeArtifactRows(ctx, info.rows) + if mergeErr == nil { + si.mergeArtifactCredits(ctx) + } if ftsSuspended { start := time.Now() @@ -473,3 +476,75 @@ func (si *SearchIndex) removeArtifactFile(path string) { si.logger.Warn("core artifact: cleanup failed", "path", path, "error", err) } } + +// artifactHasCredits reports whether the attached artifact carries the +// multi-artist credit tables. +// +// The same shape, and the same handle, as artifactHasTotals above: an +// artifact published before credits existed is still a perfectly good +// catalog, and there is one already out there. Selecting from a table +// that is not in it would fail an import that should have succeeded, so +// it is asked rather than assumed -- on the *writer*, because `core` is +// attached to that one connection and the read pool cannot see it. +func (si *SearchIndex) artifactHasCredits() bool { + var n int + + err := si.db.QueryRowWriter( + `SELECT COUNT(*) FROM core.sqlite_master + WHERE type = 'table' AND name IN ('artist_credit_part', 'artist_credit_ref')`, + ).Scan(&n) + + return err == nil && n == 2 +} + +// mergeArtifactCredits copies the credit decomposition out of the +// attached artifact. +// +// Credits are replaced wholesale rather than merged: they are derived +// entirely from one dump build, they are keyed by ids that are only +// meaningful within the artifact that carried them, and a half-updated +// credit renders as the wrong artists rather than as missing ones. +// +// A failure here is logged and not returned. The catalog has already +// merged at this point, and a catalog without credits is the catalog +// this app had before them -- every credit falls back to its single +// artist, which is the same fallback an untagged file already gets. +func (si *SearchIndex) mergeArtifactCredits(ctx context.Context) { + if !si.artifactHasCredits() { + si.logger.Info("core artifact: no credit tables, keeping single-artist credits") + + return + } + + start := time.Now() + + for _, stmt := range []string{ + "DELETE FROM artist_credit_part", + "DELETE FROM artist_credit_ref", + `INSERT OR REPLACE INTO artist_credit_part + (credit_id, position, artist_mbid, credited_name, join_phrase) + SELECT credit_id, position, artist_mbid, credited_name, join_phrase + FROM core.artist_credit_part`, + `INSERT OR REPLACE INTO artist_credit_ref (mbid, credit_id) + SELECT mbid, credit_id FROM core.artist_credit_ref`, + } { + if err := ctx.Err(); err != nil { + return + } + + if _, err := si.db.ExecContext(stmt); err != nil { + si.logger.Warn("core artifact: credit merge failed", "error", err) + + return + } + } + + var refs int + + _ = si.db.QueryRowWriter("SELECT COUNT(*) FROM artist_credit_ref").Scan(&refs) + + si.logger.Info("core artifact: credits merged", + "entities", refs, + "elapsed", time.Since(start).Round(time.Millisecond), + ) +} diff --git a/backend/explore/artifactimport_test.go b/backend/explore/artifactimport_test.go index 09564ad..6a8184e 100644 --- a/backend/explore/artifactimport_test.go +++ b/backend/explore/artifactimport_test.go @@ -3,6 +3,7 @@ package explore import ( "context" "database/sql" + "encoding/hex" "os" "path/filepath" "strings" @@ -597,3 +598,153 @@ func TestImportCoreArtifactReadsTotalsWhenPresent(t *testing.T) { t.Errorf("TotalTracks = %d, want 0 (the catalog does not say)", old.TotalTracks) } } + +// addArtifactCredits gives an artifact file the credit tables the +// exporter now writes, so the import path can be exercised against one +// that has them. +func addArtifactCredits(t *testing.T, path string) { + t.Helper() + + db, err := sql.Open("sqlite", "file:"+path) + if err != nil { + t.Fatalf("open artifact: %v", err) + } + + defer func() { _ = db.Close() }() + + for _, stmt := range []string{ + `CREATE TABLE artist_credit_part ( + credit_id INTEGER NOT NULL, + position INTEGER NOT NULL, + artist_mbid BLOB NOT NULL, + credited_name TEXT NOT NULL, + join_phrase TEXT NOT NULL DEFAULT '', + PRIMARY KEY (credit_id, position) + ) WITHOUT ROWID`, + `CREATE TABLE artist_credit_ref ( + mbid BLOB NOT NULL PRIMARY KEY, + credit_id INTEGER NOT NULL + ) WITHOUT ROWID`, + } { + if _, err := db.Exec(stmt); err != nil { + t.Fatalf("create credit tables: %v", err) + } + } + + // The packed form the catalog stores. uuid16/parseUUID live behind + // the indexbuild tag, so this file decodes for itself. + pack := func(mbid string) []byte { + raw, err := hex.DecodeString(strings.ReplaceAll(mbid, "-", "")) + if err != nil || len(raw) != 16 { + t.Fatalf("fixture MBID %q is not a UUID: %v", mbid, err) + } + + return raw + } + + a, b, rec := pack(artA), pack(artB), pack(recA) + + for _, part := range [][]any{ + {7, 0, a, "Artist A", " feat. "}, + {7, 1, b, "Artist B", ""}, + } { + if _, err := db.Exec(`INSERT INTO artist_credit_part + (credit_id, position, artist_mbid, credited_name, join_phrase) + VALUES (?, ?, ?, ?, ?)`, part...); err != nil { + t.Fatalf("insert part: %v", err) + } + } + + if _, err := db.Exec( + "INSERT INTO artist_credit_ref (mbid, credit_id) VALUES (?, ?)", rec, 7, + ); err != nil { + t.Fatalf("insert ref: %v", err) + } +} + +// TestImportCoreArtifactMergesCredits is the positive half of the +// compatibility pair: an artifact that carries credits delivers them, +// rendering back to the credit string they decompose. +func TestImportCoreArtifactMergesCredits(t *testing.T) { + db := database.NewTestDB(t) + si := NewSearchIndex(db, nil, nil, testLogger()) + + path := writeTestArtifact(t, validMeta(), []artifactRow{ + {"recording", recA, "Song A", "Artist A feat. Artist B", artA, 2000}, + }) + + addArtifactCredits(t, path) + + if err := si.importCoreArtifact(context.Background(), path); err != nil { + t.Fatalf("importCoreArtifact: %v", err) + } + + rows, err := db.QueryContext( + `SELECT p.credited_name, p.join_phrase + FROM artist_credit_ref r + JOIN artist_credit_part p ON p.credit_id = r.credit_id + ORDER BY p.position`, + ) + if err != nil { + t.Fatalf("query credits: %v", err) + } + + defer func() { _ = rows.Close() }() + + var rendered strings.Builder + + for rows.Next() { + var name, join string + + if err := rows.Scan(&name, &join); err != nil { + t.Fatalf("scan: %v", err) + } + + rendered.WriteString(name) + rendered.WriteString(join) + } + + if got := rendered.String(); got != "Artist A feat. Artist B" { + t.Errorf("rendered credit = %q, want %q", got, "Artist A feat. Artist B") + } +} + +// TestImportCoreArtifactWithoutCredits is the regression that matters +// most here: an artifact published before credits existed cannot be +// re-cut retroactively, so it must import as a catalog that declines to +// answer rather than failing outright. writeTestArtifact deliberately +// builds one without the tables. +func TestImportCoreArtifactWithoutCredits(t *testing.T) { + db := database.NewTestDB(t) + si := NewSearchIndex(db, nil, nil, testLogger()) + + path := writeTestArtifact(t, validMeta(), []artifactRow{ + {"recording", recA, "Song A", "Artist A", artA, 2000}, + }) + + if err := si.importCoreArtifact(context.Background(), path); err != nil { + t.Fatalf("an artifact without credit tables must still import: %v", err) + } + + var rows int + if err := db.QueryRowWriter( + "SELECT COUNT(*) FROM explore_index", + ).Scan(&rows); err != nil { + t.Fatalf("count: %v", err) + } + + if rows != 1 { + t.Errorf("catalog rows = %d, want 1", rows) + } + + var refs int + if err := db.QueryRowWriter( + "SELECT COUNT(*) FROM artist_credit_ref", + ).Scan(&refs); err != nil { + t.Fatalf("count refs: %v", err) + } + + if refs != 0 { + t.Errorf("credit refs = %d, want 0", refs) + } +} diff --git a/backend/explore/credits.go b/backend/explore/credits.go new file mode 100644 index 0000000..9daae4f --- /dev/null +++ b/backend/explore/credits.go @@ -0,0 +1,132 @@ +package explore + +import ( + "fmt" + "strings" +) + +// Reading multi-artist credits back out of the catalog. +// +// The tables are filled centrally (backend/explore/dumpcredits.go, and +// the artifact import) and hold only credits naming more than one +// artist: an entity with no rows here is credited to one artist, which +// explore_index's own artist_name and artist_mbid already describe. +// Absence is the common case and means "nothing to decompose", never +// "unknown". +// +// The lookup is keyed on the *recording* MBID, which both sides of the +// app already have -- a catalog row carries it and so does a local +// file (library.Track.RecordingMBID) -- so one query serves the Explore +// pages and the library's own lists without either needing to know +// where the other gets its rows. + +// CreditPart is one credited artist within a credit, in credit order. +// +// CreditedName is the name *as credited*, which is not the artist's own +// name: MusicBrainz credits "Snoop Dogg" on a track by the artist +// called "Snoop Doggy Dogg". Display uses it; navigation uses +// ArtistMBID. JoinPhrase is the literal connector that follows this +// part, so a credit renders by concatenation and never by searching a +// name inside a credit string. +type CreditPart struct { + Position int `json:"position"` + ArtistMBID string `json:"artistMbid"` + CreditedName string `json:"creditedName"` + JoinPhrase string `json:"joinPhrase"` +} + +// creditLookupBatch bounds how many MBIDs go into one IN clause. A +// tracklist is the caller here, so the realistic ceiling is a few +// hundred; the bound exists so a 50,000-row selection cannot build a +// statement SQLite refuses to parse. +const creditLookupBatch = 500 + +// GetCredits returns the decomposition of every multi-artist credit +// among the given entity MBIDs, keyed by MBID. +// +// MBIDs with a single-artist credit are simply absent from the result, +// which is what the caller wants: it renders its existing single link +// for those, and that is the same answer it would have rendered anyway. +func (si *SearchIndex) GetCredits(mbids []string) (map[string][]CreditPart, error) { + out := make(map[string][]CreditPart) + + for start := 0; start < len(mbids); start += creditLookupBatch { + end := min(start+creditLookupBatch, len(mbids)) + + if err := si.appendCredits(mbids[start:end], out); err != nil { + return nil, err + } + } + + return out, nil +} + +// appendCredits runs one batch into the accumulating result. +func (si *SearchIndex) appendCredits( + mbids []string, out map[string][]CreditPart, +) error { + args := make([]any, 0, len(mbids)) + holders := make([]string, 0, len(mbids)) + + for _, mbid := range mbids { + if mbid == "" { + continue + } + + args = append(args, dbMBID(mbid)) + holders = append(holders, "?") + } + + if len(args) == 0 { + return nil + } + + // Ordered by position because that ordering *is* the credit's + // meaning; the caller concatenates in the order it receives. + rows, err := si.db.QueryContext( + `SELECT r.mbid, p.position, p.artist_mbid, p.credited_name, p.join_phrase + FROM artist_credit_ref r + JOIN artist_credit_part p ON p.credit_id = r.credit_id + WHERE r.mbid IN (`+strings.Join(holders, ",")+`) + ORDER BY r.mbid, p.position`, + args..., + ) + if err != nil { + return fmt.Errorf("read artist credits: %w", err) + } + + defer func() { _ = rows.Close() }() + + for rows.Next() { + var ( + entity dbMBID + artist dbMBID + part CreditPart + ) + + if err := rows.Scan( + &entity, &part.Position, &artist, &part.CreditedName, &part.JoinPhrase, + ); err != nil { + return fmt.Errorf("scan artist credit: %w", err) + } + + part.ArtistMBID = string(artist) + out[string(entity)] = append(out[string(entity)], part) + } + + if err := rows.Err(); err != nil { + return fmt.Errorf("read artist credits: %w", err) + } + + return nil +} + +// GetCredits is the bound form: the frontend asks for a tracklist's +// worth of MBIDs at once rather than one per row. +// +// Batched for the reason every other per-row backend question here is: +// asking on hover or on render turns a list into N IPC round trips, and +// this one is asked about every row of every list in the app. +func (e *Service) GetCredits(mbids []string) (map[string][]CreditPart, error) { + return e.index.GetCredits(mbids) +} diff --git a/backend/explore/credits_test.go b/backend/explore/credits_test.go new file mode 100644 index 0000000..2843133 --- /dev/null +++ b/backend/explore/credits_test.go @@ -0,0 +1,128 @@ +package explore + +import ( + "encoding/hex" + "fmt" + "strings" + "testing" + + "yellowjacket/backend/database" +) + +// seedCredit writes one multi-artist credit and points an entity at it, +// the way the dump import and the artifact import both do. +func seedCredit(t *testing.T, db *database.DB, entity string, id int, parts []CreditPart) { + t.Helper() + + pack := func(mbid string) []byte { + raw, err := hex.DecodeString(strings.ReplaceAll(mbid, "-", "")) + if err != nil || len(raw) != 16 { + t.Fatalf("bad fixture mbid %q: %v", mbid, err) + } + + return raw + } + + if _, err := db.ExecContext( + "INSERT INTO artist_credit_ref (mbid, credit_id) VALUES (?, ?)", + pack(entity), id, + ); err != nil { + t.Fatalf("seed ref: %v", err) + } + + for _, p := range parts { + if _, err := db.ExecContext( + `INSERT INTO artist_credit_part + (credit_id, position, artist_mbid, credited_name, join_phrase) + VALUES (?, ?, ?, ?, ?)`, + id, p.Position, pack(p.ArtistMBID), p.CreditedName, p.JoinPhrase, + ); err != nil { + t.Fatalf("seed part: %v", err) + } + } +} + +// TestGetCreditsDecomposes: the parts come back in position order and +// concatenate to the credit they describe. +func TestGetCreditsDecomposes(t *testing.T) { + db := database.NewTestDB(t) + si := NewSearchIndex(db, nil, nil, testLogger()) + + rec := testMBID("rec-1") + a, b := testMBID("artist-a"), testMBID("artist-b") + + seedCredit(t, db, rec, 7, []CreditPart{ + {Position: 0, ArtistMBID: a, CreditedName: "2Pac", JoinPhrase: " feat. "}, + {Position: 1, ArtistMBID: b, CreditedName: "Snoop Dogg"}, + }) + + got, err := si.GetCredits([]string{rec}) + if err != nil { + t.Fatalf("GetCredits: %v", err) + } + + parts := got[rec] + if len(parts) != 2 { + t.Fatalf("parts = %d, want 2", len(parts)) + } + + var rendered strings.Builder + for _, p := range parts { + rendered.WriteString(p.CreditedName) + rendered.WriteString(p.JoinPhrase) + } + + if rendered.String() != "2Pac feat. Snoop Dogg" { + t.Errorf("rendered = %q, want %q", rendered.String(), "2Pac feat. Snoop Dogg") + } + + // Dashed on the way out: a blob reaching the frontend is sixteen + // bytes of mojibake, and nothing above mbid.go speaks that. + if parts[0].ArtistMBID != a { + t.Errorf("artist mbid = %q, want %q", parts[0].ArtistMBID, a) + } +} + +// TestGetCreditsOmitsSingleArtist: absence is the common case and means +// "nothing to decompose", so the caller renders its existing one link. +func TestGetCreditsOmitsSingleArtist(t *testing.T) { + db := database.NewTestDB(t) + si := NewSearchIndex(db, nil, nil, testLogger()) + + got, err := si.GetCredits([]string{testMBID("untagged"), ""}) + if err != nil { + t.Fatalf("GetCredits: %v", err) + } + + if len(got) != 0 { + t.Errorf("got %d credits, want none", len(got)) + } +} + +// TestGetCreditsBatches: the lookup is asked about whole tracklists, so +// it must not build one statement per row or one SQLite refuses to +// parse. +func TestGetCreditsBatches(t *testing.T) { + db := database.NewTestDB(t) + si := NewSearchIndex(db, nil, nil, testLogger()) + + mbids := make([]string, 0, creditLookupBatch*2+7) + for i := range creditLookupBatch*2 + 7 { + mbids = append(mbids, testMBID(fmt.Sprintf("batch-%d", i))) + } + + // One real credit somewhere past the first batch boundary. + seedCredit(t, db, mbids[creditLookupBatch+3], 9, []CreditPart{ + {Position: 0, ArtistMBID: testMBID("a"), CreditedName: "A", JoinPhrase: " & "}, + {Position: 1, ArtistMBID: testMBID("b"), CreditedName: "B"}, + }) + + got, err := si.GetCredits(mbids) + if err != nil { + t.Fatalf("GetCredits: %v", err) + } + + if len(got[mbids[creditLookupBatch+3]]) != 2 { + t.Errorf("a credit past the first batch boundary was not returned") + } +} diff --git a/backend/explore/dumpcredits.go b/backend/explore/dumpcredits.go new file mode 100644 index 0000000..92fb24d --- /dev/null +++ b/backend/explore/dumpcredits.go @@ -0,0 +1,637 @@ +//go:build indexbuild + +package explore + +import ( + "archive/tar" + "bufio" + "compress/bzip2" + "context" + "errors" + "fmt" + "io" + "path" + "regexp" + "strconv" + "strings" +) + +// Multi-artist credits, from the core MusicBrainz dump. +// +// A credit is ordered parts and the credit *string* is derived from +// them; MusicBrainz's own artist_credit.name is a cached render. What +// this pass extracts is the decomposition: for each catalog recording +// and release group whose credit names more than one artist, the +// credited artists in order, each with the name *as credited* and the +// join phrase that follows it. See artist_credit_part.sql for why that +// is stored rather than derived, and why nothing may reconstruct a +// credit by searching a name inside a credit string. +// +// It is a separate dump from everything else here, and it has to be. +// The canonical dump this importer already streams gives artist_mbids +// (an ordered list) and artist_credit_name (the *rendered* string) -- +// no join phrases, and no per-artist as-credited names. Splitting the +// rendered string using canonical artist names fails on exactly the +// credits that matter: measured on a real library, 21% of multi-artist +// credits name an artist differently from the artist's own name +// ("Snoop Dogg" credited on a track by "Snoop Doggy Dogg"), so the +// substring is simply not there. The JSON dumps were checked too and +// cover 153,691 recordings of ~35M, with zero overlap against a real +// library. This dump is the only source. +// +// Cost, measured on the 20260815 export: 7.1 GB compressed, decompressed +// by pure-Go compress/bzip2 at ~26 MB/s uncompressed (~13.7 min for the +// whole file, single-threaded). cmd/indexbuild is built CGO_ENABLED=0, +// so the stdlib decompressor is what there is -- and it is fine, because +// the 2 MB/s origin throttle dominates, as it does for every other dump +// here. + +const ( + // defaultMBDumpBaseURL is the core MusicBrainz export. Only + // mbdump.tar.bz2 is fetched; the other tarballs there hold data this + // app has no use for. + defaultMBDumpBaseURL = "https://data.metabrainz.org/pub/musicbrainz/data/fullexport/" +) + +var ( + mbdumpDirRe = regexp.MustCompile(`^\d{8}-\d+$`) + mbdumpFileRe = regexp.MustCompile(`^mbdump\.tar\.bz2$`) + + // ErrDumpShape is returned when a dump member does not have the + // columns this code was written against. It is deliberately fatal: + // reading the wrong column silently produces a catalog whose credits + // are subtly wrong, which is far worse than a failed build. + ErrDumpShape = errors.New("musicbrainz dump member has an unexpected shape") +) + +// Column positions in the Postgres COPY output, verified against the +// 20260815 export. There is no header row to read them from, so they +// are asserted instead -- see checkShape. +const ( + artistColID = 0 + artistColGID = 1 + artistColMin = 2 + + creditColID = 0 + creditColArtistCount = 2 + creditColMin = 3 + + partColCredit = 0 + partColPosition = 1 + partColArtist = 2 + partColName = 3 + partColJoin = 4 + partColMin = 5 + + // recording and release_group share a layout in the columns this + // pass reads: id, gid, name, artist_credit, ... + entityColGID = 1 + entityColCredit = 3 + entityColMin = 4 +) + +// creditPart is one credited artist within a credit. +type creditPart struct { + position int + artistID int32 + name string + join string +} + +// creditScan is what one pass over the dump collects. +type creditScan struct { + // artistGIDs maps an artist row id to its MBID. artist_credit_name + // references artists by row id, and the tar orders `artist` before + // it, so this is complete by the time it is read. + artistGIDs map[int32]uuid16 + + // multiCredits are the credit ids naming more than one artist, from + // artist_credit.artist_count. Taking the count from the dump rather + // than counting parts means a credit can be rejected before its + // parts are stored. + multiCredits map[int32]struct{} + + // parts are the decompositions of multiCredits, keyed by credit id. + parts map[int32][]creditPart + + // refs maps a kept catalog entity to its credit. Only entities in + // explore_index and only multi-artist credits: everything else is + // already described by explore_index's own artist_name/artist_mbid. + refs map[uuid16]int32 + + // used are the credits some ref actually points at, which is a small + // fraction of multiCredits -- the catalog keeps ~1.8M entities of + // MusicBrainz's tens of millions. + used map[int32]struct{} + + skippedUnknownArtist int +} + +// creditsImportDoneKey marks in explore_index_meta that the credit pass +// has run against the current catalog. +// +// It is its own marker rather than part of the import's stage state for +// a resume reason: the credit pass runs *after* the catalog is +// assembled, and a failure in it must not send the next run back +// through the ~205 GB it just finished. Marking separately means a +// retry retries only this. +const creditsImportDoneKey = "credits_import_done" + +// ensureArtistCredits runs the credit pass unless it has already run +// against this catalog, reporting whether it newly populated them. +// +// Called from both of run's paths -- the full import and the resume +// that finds the rows already assembled -- and from the maintenance +// entry point below, since a catalog built before credits existed is +// otherwise never offered a chance to gain them: the index job picks +// its mode from the index's own state, and a complete import means +// "refresh", which never enters run() at all. +// +// The return value is what tells the job there is something new worth +// publishing. A refresh otherwise reports "changed" only when the +// listens series advanced, so credits would sit in the CI database and +// never reach an artifact. +func (imp *dumpImporter) ensureArtistCredits(ctx context.Context) bool { + if imp.si.hasMeta(creditsImportDoneKey) { + return false + } + + url, err := discoverDumpFile( + ctx, imp.httpClient, imp.mbdumpBaseURL, mbdumpDirRe, mbdumpFileRe, + ) + if err != nil { + imp.logger.Warn("credit import: could not find the dump", "error", err) + + return false + } + + if err := imp.importArtistCredits(ctx, url); err != nil { + // A catalog without credits is the catalog this app shipped + // before them: every credit falls back to its single artist. + // That is worth far less than failing an import that otherwise + // succeeded. + imp.logger.Warn("credit import: failed", "error", err) + + return false + } + + imp.si.setMeta(creditsImportDoneKey, "1") + + return true +} + +// EnsureArtistCredits tops up the credit tables outside a full import. +// +// It exists because the index job's modes are decided from the index's +// own state: a cache holding a completed import chooses `refresh`, +// which folds in incremental listens and never enters the dump +// importer. Without this, a catalog built before the credit pass +// existed could only gain credits from a `rebuild` -- and a rebuild +// re-downloads ~205 GB to reproduce rows it already has, to add +// something that costs 7 GB on its own. +// +// Reports whether credits were newly populated, so the caller knows +// there is a new artifact worth publishing. +func (e *Service) EnsureArtistCredits(ctx context.Context) bool { + imp, err := newDumpImporter(e.index, e.lb) + if err != nil { + e.index.logger.Warn("credit import: could not start", "error", err) + + return false + } + + return imp.ensureArtistCredits(ctx) +} + +// importArtistCredits streams the core MusicBrainz dump and fills +// artist_credit_part and artist_credit_ref for the entities the catalog +// kept. +// +// It runs after assembleIndex because it asks explore_index which +// entities those are: the popularity filter decides what is worth +// carrying credits for, and asking the table rather than the kept sets +// means this stays correct if that filter changes. +func (imp *dumpImporter) importArtistCredits(ctx context.Context, url string) error { + kept, err := imp.keptEntityMBIDs(ctx) + if err != nil { + return err + } + + if len(kept) == 0 { + imp.logger.Warn("credit import: no catalog entities, skipping") + + return nil + } + + imp.logger.Info("credit import: starting", "url", url, "entities", len(kept)) + imp.logJob("Streaming MusicBrainz dump for artist credits") + + scan, err := imp.scanCreditDump(ctx, url, kept) + if err != nil { + return err + } + + imp.logger.Info("credit import: scanned", + "multiArtistCredits", len(scan.multiCredits), + "entitiesWithMultiArtistCredit", len(scan.refs), + "creditsUsed", len(scan.used), + ) + + return imp.writeCredits(ctx, scan) +} + +// keptEntityMBIDs is every recording and release group in the catalog. +// Artists are excluded: an artist is not credited to a credit. +func (imp *dumpImporter) keptEntityMBIDs(ctx context.Context) (map[uuid16]struct{}, error) { + rows, err := imp.si.db.QueryContextWith(ctx, + `SELECT mbid FROM explore_index + WHERE entity_type IN (2 /* release_group */, 3 /* recording */)`, + ) + if err != nil { + return nil, fmt.Errorf("credit import: read catalog entities: %w", err) + } + + defer func() { _ = rows.Close() }() + + out := make(map[uuid16]struct{}) + + for rows.Next() { + var raw []byte + + if err := rows.Scan(&raw); err != nil { + return nil, fmt.Errorf("credit import: scan mbid: %w", err) + } + + if len(raw) != len(uuid16{}) { + continue + } + + var id uuid16 + + copy(id[:], raw) + + out[id] = struct{}{} + } + + if err := rows.Err(); err != nil { + return nil, fmt.Errorf("credit import: read catalog entities: %w", err) + } + + return out, nil +} + +// scanCreditDump makes one sequential pass over mbdump.tar.bz2. +// +// The tar's members are alphabetical, which is what makes a single pass +// possible without buffering the big ones: `artist` and +// `artist_credit_name` both arrive before `recording` and +// `release_group`, so by the time an entity names a credit, that +// credit's parts and their artists' MBIDs are already known and the +// entity can be resolved and dropped. 35M recording rows are never +// held. +// +// The order is not depended on blindly: an entity naming a credit that +// has not been seen is counted and reported rather than silently +// producing an empty catalog, which is what a reordered export would +// otherwise look like. +func (imp *dumpImporter) scanCreditDump( + ctx context.Context, url string, kept map[uuid16]struct{}, +) (*creditScan, error) { + stream := imp.openDumpStream(ctx, url, 0) + + defer func() { _ = stream.Close() }() + + return imp.scanCreditTar( + ctx, + tar.NewReader(bzip2.NewReader(bufio.NewReaderSize(stream, 1<<20))), + kept, + ) +} + +// scanCreditTar is the parse, separated from the fetch so it can be +// driven by a tar built in a test. compress/bzip2 is decompress-only, +// so a test cannot produce the real container. +func (imp *dumpImporter) scanCreditTar( + ctx context.Context, tr *tar.Reader, kept map[uuid16]struct{}, +) (*creditScan, error) { + scan := &creditScan{ + artistGIDs: make(map[int32]uuid16), + multiCredits: make(map[int32]struct{}), + parts: make(map[int32][]creditPart), + refs: make(map[uuid16]int32), + used: make(map[int32]struct{}), + } + + for { + if err := ctx.Err(); err != nil { + return nil, err + } + + hdr, err := tr.Next() + if errors.Is(err, io.EOF) { + break + } + + if err != nil { + return nil, fmt.Errorf("credit import: tar: %w", err) + } + + if hdr.Typeflag != tar.TypeReg { + continue + } + + done, err := imp.scanCreditMember(ctx, hdr.Name, tr, kept, scan) + if err != nil { + return nil, err + } + + if done { + // Everything this pass needs has been read; the rest of the + // tarball is other entities' data and decompressing it would + // cost minutes for nothing. + break + } + } + + if scan.skippedUnknownArtist > 0 { + imp.logger.Warn("credit import: credits dropped for unknown artists", + "count", scan.skippedUnknownArtist, + ) + } + + return scan, nil +} + +// scanCreditMember dispatches one tar member, reporting whether the +// pass has everything it needs. +func (imp *dumpImporter) scanCreditMember( + ctx context.Context, name string, r io.Reader, + kept map[uuid16]struct{}, scan *creditScan, +) (bool, error) { + switch path.Base(name) { + case "artist": + return false, imp.scanArtists(ctx, r, scan) + case "artist_credit": + return false, imp.scanCredits(ctx, r, scan) + case "artist_credit_name": + return false, imp.scanCreditParts(ctx, r, scan) + case "recording", "release_group": + if err := imp.scanCreditedEntities(ctx, r, kept, scan); err != nil { + return false, err + } + + // release_group sorts after recording, so the pass is complete + // once it has been read. + return path.Base(name) == "release_group", nil + default: + return false, nil + } +} + +// scanArtists records every artist's MBID by row id. +func (imp *dumpImporter) scanArtists( + ctx context.Context, r io.Reader, scan *creditScan, +) error { + return scanTSV(ctx, r, artistColMin, "artist", func(fields []string) error { + id, ok := parseInt32(fields[artistColID]) + if !ok { + return nil + } + + var gid uuid16 + + if !parseUUID(fields[artistColGID], gid[:]) { + return fmt.Errorf("%w: artist.gid is not a UUID: %q", + ErrDumpShape, truncate(fields[artistColGID])) + } + + scan.artistGIDs[id] = gid + + return nil + }) +} + +// scanCredits records which credits name more than one artist. +func (imp *dumpImporter) scanCredits( + ctx context.Context, r io.Reader, scan *creditScan, +) error { + return scanTSV(ctx, r, creditColMin, "artist_credit", func(fields []string) error { + id, ok := parseInt32(fields[creditColID]) + if !ok { + return nil + } + + count, ok := parseInt32(fields[creditColArtistCount]) + if !ok { + return fmt.Errorf("%w: artist_credit.artist_count is not a number: %q", + ErrDumpShape, truncate(fields[creditColArtistCount])) + } + + if count > 1 { + scan.multiCredits[id] = struct{}{} + } + + return nil + }) +} + +// scanCreditParts records the decomposition of every multi-artist +// credit. +func (imp *dumpImporter) scanCreditParts( + ctx context.Context, r io.Reader, scan *creditScan, +) error { + return scanTSV(ctx, r, partColMin, "artist_credit_name", func(fields []string) error { + credit, ok := parseInt32(fields[partColCredit]) + if !ok { + return nil + } + + if _, multi := scan.multiCredits[credit]; !multi { + return nil + } + + position, ok := parseInt32(fields[partColPosition]) + if !ok { + return nil + } + + artist, ok := parseInt32(fields[partColArtist]) + if !ok { + return nil + } + + scan.parts[credit] = append(scan.parts[credit], creditPart{ + position: int(position), + artistID: artist, + name: fields[partColName], + join: fields[partColJoin], + }) + + return nil + }) +} + +// scanCreditedEntities resolves recordings and release groups against +// the catalog, keeping only those the catalog holds and whose credit +// names more than one artist. +func (imp *dumpImporter) scanCreditedEntities( + ctx context.Context, r io.Reader, kept map[uuid16]struct{}, scan *creditScan, +) error { + return scanTSV(ctx, r, entityColMin, "recording/release_group", + func(fields []string) error { + var gid uuid16 + + if !parseUUID(fields[entityColGID], gid[:]) { + return fmt.Errorf("%w: entity gid is not a UUID: %q", + ErrDumpShape, truncate(fields[entityColGID])) + } + + if _, want := kept[gid]; !want { + return nil + } + + credit, ok := parseInt32(fields[entityColCredit]) + if !ok { + return fmt.Errorf("%w: entity artist_credit is not a number: %q", + ErrDumpShape, truncate(fields[entityColCredit])) + } + + if _, multi := scan.multiCredits[credit]; !multi { + return nil + } + + scan.refs[gid] = credit + scan.used[credit] = struct{}{} + + return nil + }) +} + +// scanTSV reads Postgres COPY output a line at a time, unescaping each +// field and handing the row to fn. +// +// The shape is asserted on the first row rather than trusted: this dump +// has no header, so a column that moved would otherwise be read as a +// neighbouring one and produce a catalog that is quietly wrong. +func scanTSV( + ctx context.Context, r io.Reader, minCols int, member string, + fn func(fields []string) error, +) error { + sc := bufio.NewScanner(r) + sc.Buffer(make([]byte, 0, 1<<20), 1<<24) + + checked := false + rows := 0 + + for sc.Scan() { + rows++ + + if rows%(1<<20) == 0 { + if err := ctx.Err(); err != nil { + return err + } + } + + line := sc.Text() + if line == "" { + continue + } + + fields := strings.Split(line, "\t") + if len(fields) < minCols { + if !checked { + return fmt.Errorf("%w: %s has %d columns, need at least %d", + ErrDumpShape, member, len(fields), minCols) + } + + continue + } + + checked = true + + for i := range fields { + fields[i] = unescapeCopy(fields[i]) + } + + if err := fn(fields); err != nil { + return err + } + } + + if err := sc.Err(); err != nil { + return fmt.Errorf("credit import: read %s: %w", member, err) + } + + return nil +} + +// unescapeCopy undoes Postgres COPY's text escaping. A NULL (\N) is +// returned as an empty string: every field this pass reads is either a +// number it will reject or a name whose absence means the same as +// empty. +func unescapeCopy(s string) string { + if s == `\N` { + return "" + } + + if !strings.ContainsRune(s, '\\') { + return s + } + + var b strings.Builder + + b.Grow(len(s)) + + for i := 0; i < len(s); i++ { + if s[i] != '\\' || i+1 >= len(s) { + b.WriteByte(s[i]) + + continue + } + + i++ + + switch s[i] { + case 'n': + b.WriteByte('\n') + case 't': + b.WriteByte('\t') + case 'r': + b.WriteByte('\r') + case 'b': + b.WriteByte('\b') + case 'f': + b.WriteByte('\f') + case 'v': + b.WriteByte('\v') + case '\\': + b.WriteByte('\\') + default: + b.WriteByte('\\') + b.WriteByte(s[i]) + } + } + + return b.String() +} + +func parseInt32(s string) (int32, bool) { + n, err := strconv.ParseInt(s, 10, 32) + if err != nil { + return 0, false + } + + return int32(n), true +} + +// truncate bounds an error message built from dump data, which is +// attacker-free but can be long. +func truncate(s string) string { + const limit = 64 + + if len(s) <= limit { + return s + } + + return s[:limit] + "..." +} diff --git a/backend/explore/dumpcredits_test.go b/backend/explore/dumpcredits_test.go new file mode 100644 index 0000000..adc2740 --- /dev/null +++ b/backend/explore/dumpcredits_test.go @@ -0,0 +1,406 @@ +//go:build indexbuild + +package explore + +import ( + "archive/tar" + "bytes" + "context" + "errors" + "strings" + "testing" + + "yellowjacket/backend/database" +) + +// tarOf builds an uncompressed tar of the named members, in the order +// given. Order is the point of several of these tests: the real dump's +// members are alphabetical, which is what lets one pass resolve an +// entity's credit without buffering 35M recordings. +func tarOf(t *testing.T, members ...[2]string) *tar.Reader { + t.Helper() + + var buf bytes.Buffer + + tw := tar.NewWriter(&buf) + + for _, m := range members { + body := []byte(m[1]) + + if err := tw.WriteHeader(&tar.Header{ + Name: "mbdump/" + m[0], + Mode: 0o644, + Size: int64(len(body)), + Typeflag: tar.TypeReg, + }); err != nil { + t.Fatalf("tar header: %v", err) + } + + if _, err := tw.Write(body); err != nil { + t.Fatalf("tar write: %v", err) + } + } + + if err := tw.Close(); err != nil { + t.Fatalf("tar close: %v", err) + } + + return tar.NewReader(&buf) +} + +func tsv(rows ...[]string) string { + var b strings.Builder + + for _, r := range rows { + b.WriteString(strings.Join(r, "\t")) + b.WriteByte('\n') + } + + return b.String() +} + +// mustMBID is testMBID in the packed form the catalog stores. +func mustMBID(label string) uuid16 { + var u uuid16 + + if !parseUUID(testMBID(label), u[:]) { + panic("testMBID did not produce a UUID for " + label) + } + + return u +} + +// The two artists of the worked example, and the entities they credit. +var ( + creditRecMBID = mustMBID("recording-1") + creditRGMBID = mustMBID("release-group-1") +) + +// sampleDump is the shape verified against the 20260815 export: +// artist(id, gid, ...), artist_credit(id, name, artist_count, ...), +// artist_credit_name(credit, position, artist, name, join_phrase), +// recording/release_group(id, gid, name, artist_credit, ...). +func sampleDump(t *testing.T) *tar.Reader { + t.Helper() + + return tarOf(t, + [2]string{"artist", tsv( + []string{"11", testMBID("artist-a"), "Snoop Doggy Dogg", "Snoop Doggy Dogg"}, + []string{"22", testMBID("artist-b"), "2Pac", "2Pac"}, + )}, + [2]string{"artist_credit", tsv( + []string{"900", "2Pac feat. Snoop Dogg", "2", "1", "", "0", ""}, + []string{"901", "Solo Artist", "1", "1", "", "0", ""}, + )}, + [2]string{"artist_credit_name", tsv( + // Deliberately out of position order: the dump is not + // obliged to emit them sorted and the credit's meaning is + // the order, not the file's. + []string{"900", "1", "11", "Snoop Dogg", ""}, + []string{"900", "0", "22", "2Pac", " feat. "}, + []string{"901", "0", "11", "Solo Artist", ""}, + )}, + [2]string{"recording", tsv( + []string{"1", testMBID("recording-1"), "Some Song", "900", "180000"}, + []string{"2", testMBID("not-kept"), "Other", "900", "1"}, + []string{"3", testMBID("solo"), "Solo", "901", "1"}, + )}, + [2]string{"release_group", tsv( + []string{"5", testMBID("release-group-1"), "Some Album", "900", "1"}, + )}, + ) +} + +func creditTestImporter(t *testing.T) *dumpImporter { + t.Helper() + + db := database.NewTestDB(t) + + return &dumpImporter{ + si: NewSearchIndex(db, nil, nil, testLogger()), + logger: testLogger(), + } +} + +// TestScanCreditDumpDecomposes is the worked example end to end: the +// credit's parts come back in position order, with the *credited* +// names and the join phrase between them. +func TestScanCreditDumpDecomposes(t *testing.T) { + imp := creditTestImporter(t) + + kept := map[uuid16]struct{}{ + creditRecMBID: {}, + creditRGMBID: {}, + } + + scan, err := imp.scanCreditTar(context.Background(), sampleDump(t), kept) + if err != nil { + t.Fatalf("scan: %v", err) + } + + if got := len(scan.refs); got != 2 { + t.Fatalf("refs = %d, want 2 (the recording and the release group)", got) + } + + if scan.refs[creditRecMBID] != 900 { + t.Errorf("recording credit = %d, want 900", scan.refs[creditRecMBID]) + } + + parts := scan.parts[900] + if len(parts) != 2 { + t.Fatalf("parts = %d, want 2", len(parts)) + } + + // Sorting happens on write, so assert the pieces are all present + // and let the render test below check the order. + byPos := map[int]creditPart{} + for _, p := range parts { + byPos[p.position] = p + } + + if byPos[0].name != "2Pac" || byPos[0].join != " feat. " { + t.Errorf("position 0 = %q/%q, want \"2Pac\"/\" feat. \"", + byPos[0].name, byPos[0].join) + } + + // The credited name, not the artist's own name: this is the whole + // reason credited_name is stored per row. + if byPos[1].name != "Snoop Dogg" { + t.Errorf("position 1 credited name = %q, want \"Snoop Dogg\"", byPos[1].name) + } +} + +// TestSingleArtistCreditsAreNotStored: a one-artist credit is already +// described by explore_index's artist_name/artist_mbid, and storing it +// would roughly triple the table to say nothing new. +func TestSingleArtistCreditsAreNotStored(t *testing.T) { + imp := creditTestImporter(t) + + solo := mustMBID("solo") + kept := map[uuid16]struct{}{solo: {}} + + scan, err := imp.scanCreditTar(context.Background(), sampleDump(t), kept) + if err != nil { + t.Fatalf("scan: %v", err) + } + + if len(scan.refs) != 0 { + t.Fatalf("a single-artist credit was referenced: %v", scan.refs) + } + + if _, ok := scan.multiCredits[901]; ok { + t.Error("credit 901 has artist_count 1 and should not be multi") + } +} + +// TestOnlyKeptEntitiesAreReferenced: the catalog's popularity filter +// decides what is worth carrying credits for, and an entity outside it +// must not produce a row pointing at nothing. +func TestOnlyKeptEntitiesAreReferenced(t *testing.T) { + imp := creditTestImporter(t) + + kept := map[uuid16]struct{}{creditRecMBID: {}} + + scan, err := imp.scanCreditTar(context.Background(), sampleDump(t), kept) + if err != nil { + t.Fatalf("scan: %v", err) + } + + if _, ok := scan.refs[mustMBID("not-kept")]; ok { + t.Error("an entity outside the catalog was referenced") + } + + if len(scan.used) != 1 { + t.Errorf("used credits = %d, want 1", len(scan.used)) + } +} + +// TestWriteCreditsRoundTrips checks what the frontend will actually +// read: parts in position order, dashed MBIDs out of the 16 raw bytes, +// and a rendered credit that reassembles to the tagged string. +func TestWriteCreditsRoundTrips(t *testing.T) { + imp := creditTestImporter(t) + + kept := map[uuid16]struct{}{creditRecMBID: {}, creditRGMBID: {}} + + scan, err := imp.scanCreditTar(context.Background(), sampleDump(t), kept) + if err != nil { + t.Fatalf("scan: %v", err) + } + + if err := imp.writeCredits(context.Background(), scan); err != nil { + t.Fatalf("writeCredits: %v", err) + } + + rows, err := imp.si.db.QueryContext( + `SELECT p.position, p.artist_mbid, p.credited_name, p.join_phrase + FROM artist_credit_ref r + JOIN artist_credit_part p ON p.credit_id = r.credit_id + WHERE r.mbid = ? + ORDER BY p.position`, + creditRecMBID[:], + ) + if err != nil { + t.Fatalf("query: %v", err) + } + + defer func() { _ = rows.Close() }() + + var rendered strings.Builder + + names := []string{} + + for rows.Next() { + var ( + pos int + mbid []byte + name string + join string + ) + + if err := rows.Scan(&pos, &mbid, &name, &join); err != nil { + t.Fatalf("scan row: %v", err) + } + + if len(mbid) != 16 { + t.Fatalf("artist_mbid is %d bytes, want 16", len(mbid)) + } + + names = append(names, name) + + rendered.WriteString(name) + rendered.WriteString(join) + } + + if err := rows.Err(); err != nil { + t.Fatalf("rows: %v", err) + } + + // Concatenation is the contract: names in order, join phrases + // between them, and no searching a name inside a credit string. + if got := rendered.String(); got != "2Pac feat. Snoop Dogg" { + t.Errorf("rendered credit = %q, want %q", got, "2Pac feat. Snoop Dogg") + } + + if len(names) != 2 || names[0] != "2Pac" { + t.Errorf("parts came back out of position order: %v", names) + } +} + +// TestCreditRefsNeverDangle: a ref whose parts were not stored renders +// as a credit with no artists at all, which is worse than the +// single-artist fallback it replaced. +func TestCreditRefsNeverDangle(t *testing.T) { + imp := creditTestImporter(t) + + kept := map[uuid16]struct{}{creditRecMBID: {}} + + scan, err := imp.scanCreditTar(context.Background(), sampleDump(t), kept) + if err != nil { + t.Fatalf("scan: %v", err) + } + + // An artist the dump never named: the credit cannot be navigated to + // and must be dropped whole, taking its ref with it. + scan.artistGIDs = map[int32]uuid16{} + + if err := imp.writeCredits(context.Background(), scan); err != nil { + t.Fatalf("writeCredits: %v", err) + } + + var refs, parts int + + if err := imp.si.db.QueryRowWriter( + "SELECT COUNT(*) FROM artist_credit_ref", + ).Scan(&refs); err != nil { + t.Fatalf("count refs: %v", err) + } + + if err := imp.si.db.QueryRowWriter( + "SELECT COUNT(*) FROM artist_credit_part", + ).Scan(&parts); err != nil { + t.Fatalf("count parts: %v", err) + } + + if refs != 0 || parts != 0 { + t.Fatalf("refs=%d parts=%d, want 0/0 when the artists are unknown", refs, parts) + } +} + +// TestCreditDumpShapeIsAsserted: the dump has no header row, so a +// column that moved would be read as its neighbour and produce a +// catalog that is quietly wrong. Loud is the requirement. +func TestCreditDumpShapeIsAsserted(t *testing.T) { + imp := creditTestImporter(t) + + short := tarOf(t, [2]string{"artist", tsv([]string{"11", "only-two-columns"})}) + + _, err := imp.scanCreditTar(context.Background(), short, map[uuid16]struct{}{}) + if err == nil { + t.Fatal("a member with a non-UUID gid was accepted") + } + + if !errors.Is(err, ErrDumpShape) { + t.Errorf("error = %v, want ErrDumpShape", err) + } +} + +// TestUnescapeCopy covers Postgres COPY's text escaping, which reaches +// artist names routinely -- a tab or backslash in a name would +// otherwise shift every field after it. +func TestUnescapeCopy(t *testing.T) { + tests := []struct{ in, want string }{ + {`plain`, `plain`}, + {`\N`, ``}, + {`a\tb`, "a\tb"}, + {`a\nb`, "a\nb"}, + {`back\\slash`, `back\slash`}, + {`AC\/DC`, `AC\/DC`}, + {`trailing\`, `trailing\`}, + } + + for _, tt := range tests { + if got := unescapeCopy(tt.in); got != tt.want { + t.Errorf("unescapeCopy(%q) = %q, want %q", tt.in, got, tt.want) + } + } +} + +// TestEnsureArtistCreditsIsIdempotent pins what the index job depends +// on to decide whether to publish. +// +// The pass runs on every mode, including the `refresh` that a complete +// catalog always chooses — so it must be free when there is nothing to +// do, and it must say so. A `true` here republishes the artifact; a +// `true` on every run would republish an identical one weekly, and a +// permanent `false` would mean a catalog that never gains credits at +// all. +func TestEnsureArtistCreditsIsIdempotent(t *testing.T) { + imp := creditTestImporter(t) + + // The marker is what "already done" means; with it set, the pass + // must not reach the network or report a change. + imp.si.setMeta(creditsImportDoneKey, "1") + + if imp.ensureArtistCredits(context.Background()) { + t.Fatal("a second run reported new credits; the artifact would republish forever") + } +} + +// TestEnsureArtistCreditsReportsFailureAsNoChange: a dump that cannot be +// reached leaves the catalog exactly as it was, and must not claim +// otherwise — publishing on it would ship an artifact with no credits +// and mark the work done. +func TestEnsureArtistCreditsReportsFailureAsNoChange(t *testing.T) { + imp := creditTestImporter(t) + imp.httpClient = newDumpHTTPClient() + imp.mbdumpBaseURL = "http://127.0.0.1:1/nonexistent/" + + if imp.ensureArtistCredits(context.Background()) { + t.Fatal("an unreachable dump reported new credits") + } + + if imp.si.hasMeta(creditsImportDoneKey) { + t.Error("a failed pass marked itself done; it would never retry") + } +} diff --git a/backend/explore/dumpcreditswrite.go b/backend/explore/dumpcreditswrite.go new file mode 100644 index 0000000..63cc69e --- /dev/null +++ b/backend/explore/dumpcreditswrite.go @@ -0,0 +1,164 @@ +//go:build indexbuild + +package explore + +import ( + "context" + "database/sql" + "fmt" + "sort" +) + +// writeCredits persists the scanned decompositions. +// +// Only credits some catalog entity actually points at are written: the +// dump has millions of multi-artist credits and the catalog keeps ~1.8M +// entities, so storing every credit would be most of a table nothing +// can reach. +// +// The two tables are written in one transaction, because a ref pointing +// at parts that are not there renders as a credit with no artists -- +// worse than the single-artist fallback it replaced. +func (imp *dumpImporter) writeCredits(ctx context.Context, scan *creditScan) error { + tx, err := imp.si.db.BeginTx() + if err != nil { + return fmt.Errorf("credit import: begin: %w", err) + } + + defer func() { _ = tx.Rollback() }() + + // A rebuild replaces the previous pass wholesale. These are Cache + // tables derived entirely from the dump, so there is nothing to + // merge and a stale row is a wrong credit. + for _, table := range []string{"artist_credit_part", "artist_credit_ref"} { + if _, err := tx.ExecContext(ctx, "DELETE FROM "+table); err != nil { + return fmt.Errorf("credit import: clear %s: %w", table, err) + } + } + + written, err := imp.writeCreditParts(ctx, tx, scan) + if err != nil { + return err + } + + refs, err := imp.writeCreditRefs(ctx, tx, scan, written) + if err != nil { + return err + } + + if err := tx.Commit(); err != nil { + return fmt.Errorf("credit import: commit: %w", err) + } + + imp.logger.Info("credit import: complete", + "credits", len(written), + "refs", refs, + ) + + return nil +} + +// writeCreditParts inserts the parts of every used credit and returns +// the set of credits that were actually stored. +// +// A credit is stored whole or not at all. If any of its artists has no +// MBID -- which should not happen, the dump being self-consistent, but +// would leave a part that cannot be navigated to -- the credit is +// dropped and the entity falls back to explore_index's single artist, +// which is a worse answer rather than a broken one. +func (imp *dumpImporter) writeCreditParts( + ctx context.Context, tx *sql.Tx, scan *creditScan, +) (map[int32]struct{}, error) { + stmt, err := tx.PrepareContext(ctx, + `INSERT INTO artist_credit_part + (credit_id, position, artist_mbid, credited_name, join_phrase) + VALUES (?, ?, ?, ?, ?)`, + ) + if err != nil { + return nil, fmt.Errorf("credit import: prepare part insert: %w", err) + } + + defer func() { _ = stmt.Close() }() + + written := make(map[int32]struct{}, len(scan.used)) + + for credit := range scan.used { + parts := scan.parts[credit] + if len(parts) < 2 { + // artist_credit said more than one artist and + // artist_credit_name did not deliver them. Nothing to + // decompose, so leave the entity to its single artist. + continue + } + + // Position order is the credit's meaning, and the dump is not + // obliged to emit it sorted. + sort.Slice(parts, func(i, j int) bool { + return parts[i].position < parts[j].position + }) + + resolved := make([][]any, 0, len(parts)) + ok := true + + for _, part := range parts { + gid, found := scan.artistGIDs[part.artistID] + if !found { + scan.skippedUnknownArtist++ + ok = false + + break + } + + resolved = append(resolved, []any{ + credit, part.position, gid[:], part.name, part.join, + }) + } + + if !ok { + continue + } + + for _, args := range resolved { + if _, err := stmt.ExecContext(ctx, args...); err != nil { + return nil, fmt.Errorf("credit import: insert part: %w", err) + } + } + + written[credit] = struct{}{} + } + + return written, nil +} + +// writeCreditRefs points each kept entity at its credit, skipping any +// whose credit was not stored so a ref never dangles. +func (imp *dumpImporter) writeCreditRefs( + ctx context.Context, tx *sql.Tx, scan *creditScan, written map[int32]struct{}, +) (int, error) { + stmt, err := tx.PrepareContext(ctx, + "INSERT OR REPLACE INTO artist_credit_ref (mbid, credit_id) VALUES (?, ?)", + ) + if err != nil { + return 0, fmt.Errorf("credit import: prepare ref insert: %w", err) + } + + defer func() { _ = stmt.Close() }() + + count := 0 + + for mbid, credit := range scan.refs { + if _, stored := written[credit]; !stored { + continue + } + + id := mbid + + if _, err := stmt.ExecContext(ctx, id[:], credit); err != nil { + return 0, fmt.Errorf("credit import: insert ref: %w", err) + } + + count++ + } + + return count, nil +} diff --git a/backend/explore/dumpimport.go b/backend/explore/dumpimport.go index d0acee0..e14f0fc 100644 --- a/backend/explore/dumpimport.go +++ b/backend/explore/dumpimport.go @@ -104,6 +104,7 @@ type dumpImporter struct { canonicalBaseURL string listensBaseURL string + mbdumpBaseURL string // Disk safety floors (fields so tests can relax them). minStartFreeBytes uint64 @@ -144,6 +145,7 @@ func newDumpImporter(si *SearchIndex, lb *ListenBrainzClient) (*dumpImporter, er stagingDir: stagingDir, canonicalBaseURL: defaultCanonicalBaseURL, listensBaseURL: defaultListensBaseURL, + mbdumpBaseURL: defaultMBDumpBaseURL, minStartFreeBytes: dumpMinStartFreeBytes, abortFreeBytes: dumpAbortFreeBytes, }, nil @@ -171,6 +173,7 @@ func (imp *dumpImporter) run(ctx context.Context) error { // Fast path: rows already assembled, only patch passes remain. if state.Stage == dumpStageAssembled { imp.si.MarkReadyIfPopulated() + imp.ensureArtistCredits(ctx) imp.runPatchPasses(ctx) if err := ctx.Err(); err != nil { @@ -305,6 +308,11 @@ func (imp *dumpImporter) run(ctx context.Context) error { imp.si.MarkReadyIfPopulated() imp.si.refreshStatusCounts() + // Multi-artist credits, from a different dump. After the catalog, + // because it asks explore_index which entities are worth carrying + // credits for. + imp.ensureArtistCredits(ctx) + // Stage 4: API patch passes (idempotent). imp.runPatchPasses(ctx) diff --git a/cmd/indexbuild/main.go b/cmd/indexbuild/main.go index dd5f286..b586a49 100644 --- a/cmd/indexbuild/main.go +++ b/cmd/indexbuild/main.go @@ -177,12 +177,25 @@ func run(o opts) error { complete := svc.IndexImportComplete() && !errors.Is(err, errIncomplete) + // Credits are maintenance, not part of any one mode. They come from + // a different dump, they are keyed on entities the catalog already + // holds, and a catalog built before the pass existed would otherwise + // only gain them from a rebuild — which re-downloads ~205 GB to + // re-derive rows it already has. Skipped when the import is not + // complete, because there is nothing to key them against yet. + creditsAdded := false + if complete { + creditsAdded = svc.EnsureArtistCredits(context.Background()) + } + // "Changed" means there is something new worth publishing, so it is // only ever true for a finished import: a build stamps the listens // series early, long before its rows are assembled, and reporting a // change off that would be a lie about a half-built index. changed := complete && - (svc.IndexBaselineSeries() != seriesBefore || chosen != modeRefresh) + (svc.IndexBaselineSeries() != seriesBefore || + chosen != modeRefresh || + creditsAdded) report(logger, svc, chosen, complete, changed) diff --git a/cmd/indexexport/main.go b/cmd/indexexport/main.go index e97c667..4831dfa 100644 --- a/cmd/indexexport/main.go +++ b/cmd/indexexport/main.go @@ -171,6 +171,26 @@ func createSchema(db *sql.DB) error { key TEXT PRIMARY KEY, value TEXT NOT NULL )`, + // Multi-artist credits. Shipped as their own tables rather than + // as an explore_index column because a credit is a variable + // number of ordered parts, and because credits are *shared* -- + // an album's tracks by one artist reference one credit, which is + // what keeps this to a few hundred thousand rows. + // + // An importer that predates these reads an artifact without + // them; artifactHasCredits is what asks. + `CREATE TABLE core.artist_credit_part ( + credit_id INTEGER NOT NULL, + position INTEGER NOT NULL, + artist_mbid BLOB NOT NULL, + credited_name TEXT NOT NULL, + join_phrase TEXT NOT NULL DEFAULT '', + PRIMARY KEY (credit_id, position) + ) WITHOUT ROWID`, + `CREATE TABLE core.artist_credit_ref ( + mbid BLOB NOT NULL PRIMARY KEY, + credit_id INTEGER NOT NULL + ) WITHOUT ROWID`, } for _, stmt := range stmts { @@ -243,6 +263,63 @@ func copyRows(db *sql.DB, artists, perArtistRGs, perArtistRecs int) error { fmt.Printf(" %-15s %d\n", sel.label+":", n) } + return copyCredits(db) +} + +// copyCredits ships the credit decomposition for the entities that made +// it into the artifact, and only those. +// +// The refs go first and the parts follow *from* the refs, so a credit is +// carried only if something in the artifact points at it. The source +// index holds credits for every catalog entity, while the artifact is a +// windowed subset -- copying all of them would carry a large table most +// of which nothing in the artifact can reach. +// +// A source index built before the credit pass simply has no rows here, +// which is not an error: the artifact then carries the tables empty, and +// every credit falls back to its single artist exactly as before. +func copyCredits(db *sql.DB) error { + // Asked, not assumed. A source index built before the credit pass + // has no such table, and "no such table" would fail an export whose + // catalog is otherwise complete. + for _, table := range []string{"artist_credit_ref", "artist_credit_part"} { + var n int + + if err := db.QueryRow( + `SELECT COUNT(*) FROM main.sqlite_master + WHERE type = 'table' AND name = ?`, table, + ).Scan(&n); err != nil { + return fmt.Errorf("probe %s: %w", table, err) + } + + if n == 0 { + fmt.Printf(" %-15s none in source\n", "credits:") + + return nil + } + } + + refs, err := insertSelect(db, ` + INSERT INTO core.artist_credit_ref (mbid, credit_id) + SELECT r.mbid, r.credit_id + FROM main.artist_credit_ref r + WHERE r.mbid IN (SELECT mbid FROM core.explore_index)`) + if err != nil { + return err + } + + parts, err := insertSelect(db, ` + INSERT INTO core.artist_credit_part + (credit_id, position, artist_mbid, credited_name, join_phrase) + SELECT p.credit_id, p.position, p.artist_mbid, p.credited_name, p.join_phrase + FROM main.artist_credit_part p + WHERE p.credit_id IN (SELECT credit_id FROM core.artist_credit_ref)`) + if err != nil { + return err + } + + fmt.Printf(" %-15s %d refs, %d parts\n", "credits:", refs, parts) + return nil } diff --git a/frontend/bindings/yellowjacket/backend/explore/index.ts b/frontend/bindings/yellowjacket/backend/explore/index.ts index 7c49266..f6187cf 100644 --- a/frontend/bindings/yellowjacket/backend/explore/index.ts +++ b/frontend/bindings/yellowjacket/backend/explore/index.ts @@ -12,6 +12,7 @@ export { export type { AlbumCompleteFunc, + CreditPart, IndexStatus, LBSimilarArtist, LBTopRecording, diff --git a/frontend/bindings/yellowjacket/backend/explore/models.ts b/frontend/bindings/yellowjacket/backend/explore/models.ts index 3693835..54646db 100644 --- a/frontend/bindings/yellowjacket/backend/explore/models.ts +++ b/frontend/bindings/yellowjacket/backend/explore/models.ts @@ -7,6 +7,23 @@ */ export type AlbumCompleteFunc = any; +/** + * CreditPart is one credited artist within a credit, in credit order. + * + * CreditedName is the name *as credited*, which is not the artist's own + * name: MusicBrainz credits "Snoop Dogg" on a track by the artist + * called "Snoop Doggy Dogg". Display uses it; navigation uses + * ArtistMBID. JoinPhrase is the literal connector that follows this + * part, so a credit renders by concatenation and never by searching a + * name inside a credit string. + */ +export interface CreditPart { + "position": number; + "artistMbid": string; + "creditedName": string; + "joinPhrase": string; +} + /** * IndexStatus is the full index build status, exposed to the frontend. */ diff --git a/frontend/bindings/yellowjacket/backend/explore/service.ts b/frontend/bindings/yellowjacket/backend/explore/service.ts index b04bcdc..bb723ae 100644 --- a/frontend/bindings/yellowjacket/backend/explore/service.ts +++ b/frontend/bindings/yellowjacket/backend/explore/service.ts @@ -225,6 +225,18 @@ export function GetCandidateThumbnail(releaseMBID: string, releaseGroupMBID: str return $Call.ByID(1946932424, releaseMBID, releaseGroupMBID); } +/** + * GetCredits is the bound form: the frontend asks for a tracklist's + * worth of MBIDs at once rather than one per row. + * + * Batched for the reason every other per-row backend question here is: + * asking on hover or on render turns a list into N IPC round trips, and + * this one is asked about every row of every list in the app. + */ +export function GetCredits(mbids: string[] | null): $CancellablePromise<{ [_ in string]?: $models.CreditPart[] | null } | null> { + return $Call.ByID(225964099, mbids); +} + /** * GetExploreShelves builds the page Explore shows before a query. * diff --git a/lefthook.yml b/lefthook.yml index a89a2ec..eb6614c 100644 --- a/lefthook.yml +++ b/lefthook.yml @@ -26,7 +26,11 @@ pre-commit: go generate ./... if [ -n "$(git diff --name-only)" ]; then echo "Generated code is out of date. Run 'make generate' and stage the changes." - git diff --stat + # --no-pager, or this blocks forever on `less` waiting for a + # keypress that a hook run without a tty will never get: the + # commit hangs at exactly the moment it is trying to tell you + # why it failed. + git --no-pager diff --stat exit 1 fi