diff --git a/internal/api/admin_wanted.go b/internal/api/admin_wanted.go index b6e015a1..7ed0b292 100644 --- a/internal/api/admin_wanted.go +++ b/internal/api/admin_wanted.go @@ -91,13 +91,24 @@ type wantsPage struct { } // libraryListingView: whether the library's albums are all on the list -// yet, and how many could not be listed (nothing names them to -// MusicBrainz). +// yet, and which could not be matched to MusicBrainz (#5569). Unmatched is +// filled on the first page of the held list only. type libraryListingView struct { - ListedAt *string `json:"listed_at"` - Unlisted int32 `json:"unlisted"` + ListedAt *string `json:"listed_at"` + Unlisted int32 `json:"unlisted"` + Unmatched []unmatchedAlbumView `json:"unmatched"` } +type unmatchedAlbumView struct { + AlbumID string `json:"album_id"` + Title string `json:"title"` + Artist string `json:"artist"` + Detail string `json:"detail"` +} + +// unmatchedShown is how many unmatched albums the held list names. +const unmatchedShown = 200 + const wantsPageSize = 50 // handleListWants implements GET /api/admin/wanted?state=wanted|missing|held&offset=. @@ -137,10 +148,23 @@ func (h *handlers) handleListWants(w http.ResponseWriter, r *http.Request) { Counts: wantCounts{Wanted: counts.Wanted, Missing: counts.Missing, Held: counts.Held, Following: counts.Following, Activity: counts.Activity, Attention: counts.Attention, Blocked: counts.Blocked}, } + out.Library.Unmatched = []unmatchedAlbumView{} if listing.LibraryListedAt.Valid { at := formatTimestamp(listing.LibraryListedAt) out.Library.ListedAt = &at } + if state == "held" && offset == 0 && listing.LibraryUnlisted > 0 { + unmatched, err := q.ListUnmatchedLibraryAlbums(r.Context(), unmatchedShown) + if err != nil { + writeErrWithLog(w, h.logger, "admin wanted: unmatched albums", apierror.Internal(err)) + return + } + for _, u := range unmatched { + out.Library.Unmatched = append(out.Library.Unmatched, unmatchedAlbumView{ + AlbumID: uuidToString(u.AlbumID), Title: u.Title, Artist: u.ArtistName, Detail: u.Detail, + }) + } + } switch state { case "missing": out.Total = counts.Missing diff --git a/internal/db/dbq/models.go b/internal/db/dbq/models.go index d094ae0a..883db4df 100644 --- a/internal/db/dbq/models.go +++ b/internal/db/dbq/models.go @@ -487,6 +487,15 @@ type Grab struct { SeedReleasedAt pgtype.Timestamptz } +type LibraryAlbumMatch struct { + AlbumID pgtype.UUID + Outcome string + Method string + ReleaseGroupMbid string + Detail string + DecidedAt pgtype.Timestamptz +} + type LibraryChange struct { ID int64 EntityType string diff --git a/internal/db/dbq/wanted.sql.go b/internal/db/dbq/wanted.sql.go index dacf5e07..fbe40c14 100644 --- a/internal/db/dbq/wanted.sql.go +++ b/internal/db/dbq/wanted.sql.go @@ -125,15 +125,10 @@ func (q *Queries) DeleteWant(ctx context.Context, id pgtype.UUID) (int64, error) } const finishLibraryListing = `-- name: FinishLibraryListing :one -UPDATE acquisition_settings s +UPDATE acquisition_settings SET library_listed_at = now(), - library_unlisted = ( - SELECT COUNT(*) FROM albums a - WHERE a.created_at <= s.library_listing_started_at - AND NOT EXISTS (SELECT 1 FROM wants w - WHERE w.album_id = a.id - OR (a.release_group_mbid <> '' AND w.release_group_mbid = a.release_group_mbid))) - WHERE s.id = true AND s.library_listed_at IS NULL + library_unlisted = (SELECT COUNT(*) FROM library_album_matches WHERE outcome = 'unmatched') + WHERE id = true AND library_listed_at IS NULL RETURNING library_listed_at, library_unlisted ` @@ -142,8 +137,7 @@ type FinishLibraryListingRow struct { LibraryUnlisted int32 } -// The listing is done: count the albums from before the cutoff it could not -// list, because nothing names them to MusicBrainz. +// The listing is done: count the albums no match was found for. func (q *Queries) FinishLibraryListing(ctx context.Context) (FinishLibraryListingRow, error) { row := q.db.QueryRow(ctx, finishLibraryListing) var i FinishLibraryListingRow @@ -297,6 +291,44 @@ func (q *Queries) GetWantByReleaseGroup(ctx context.Context, releaseGroupMbid st return i, err } +const insertLibraryWant = `-- name: InsertLibraryWant :execrows +INSERT INTO wants (release_group_mbid, artist_mbid, artist_name, title, primary_type, first_release_date, + reason, album_id, wanted_at, fulfilled_at) +VALUES ($1, $2, $3, $4, + $5, $6, 'library', $7::uuid, now(), + COALESCE((SELECT MIN(t.missing_since) FROM tracks t WHERE t.album_id = $7::uuid) + - interval '1 second', now())) +ON CONFLICT (release_group_mbid) DO NOTHING +` + +type InsertLibraryWantParams struct { + ReleaseGroupMbid string + ArtistMbid string + ArtistName string + Title string + PrimaryType string + FirstReleaseDate string + AlbumID pgtype.UUID +} + +// Puts one matched album on the list as held, from just before its first +// lost track if it has one. A group already on the list is left as it is. +func (q *Queries) InsertLibraryWant(ctx context.Context, arg InsertLibraryWantParams) (int64, error) { + result, err := q.db.Exec(ctx, insertLibraryWant, + arg.ReleaseGroupMbid, + arg.ArtistMbid, + arg.ArtistName, + arg.Title, + arg.PrimaryType, + arg.FirstReleaseDate, + arg.AlbumID, + ) + if err != nil { + return 0, err + } + return result.RowsAffected(), nil +} + const libraryHoldsReleaseGroup = `-- name: LibraryHoldsReleaseGroup :one SELECT EXISTS ( SELECT 1 FROM albums a @@ -325,6 +357,103 @@ func (q *Queries) LibraryHoldsReleaseGroup(ctx context.Context, arg LibraryHolds return exists, err } +const listAlbumTracksToMatch = `-- name: ListAlbumTracksToMatch :many +SELECT title, disc_number, track_number, duration_ms, mbid + FROM tracks + WHERE album_id = $1 + ORDER BY disc_number NULLS FIRST, track_number NULLS LAST, title +` + +type ListAlbumTracksToMatchRow struct { + Title string + DiscNumber *int32 + TrackNumber *int32 + DurationMs int32 + Mbid *string +} + +// What an album's tracks say about themselves, to check a match against. +func (q *Queries) ListAlbumTracksToMatch(ctx context.Context, albumID pgtype.UUID) ([]ListAlbumTracksToMatchRow, error) { + rows, err := q.db.Query(ctx, listAlbumTracksToMatch, albumID) + if err != nil { + return nil, err + } + defer rows.Close() + var items []ListAlbumTracksToMatchRow + for rows.Next() { + var i ListAlbumTracksToMatchRow + if err := rows.Scan( + &i.Title, + &i.DiscNumber, + &i.TrackNumber, + &i.DurationMs, + &i.Mbid, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + +const listAlbumsToMatch = `-- name: ListAlbumsToMatch :many +SELECT a.id, a.title, a.mbid, a.release_group_mbid, ar.name AS artist_name, ar.mbid AS artist_mbid + FROM albums a + JOIN artists ar ON ar.id = a.artist_id + WHERE a.created_at <= $1::timestamptz + AND EXISTS (SELECT 1 FROM tracks t WHERE t.album_id = a.id) + AND NOT EXISTS (SELECT 1 FROM library_album_matches m WHERE m.album_id = a.id) + ORDER BY a.created_at, a.id + LIMIT $2 +` + +type ListAlbumsToMatchParams struct { + Cutoff pgtype.Timestamptz + MaxRows int32 +} + +type ListAlbumsToMatchRow struct { + ID pgtype.UUID + Title string + Mbid *string + ReleaseGroupMbid *string + ArtistName string + ArtistMbid *string +} + +// Library albums from before the cutoff that the listing has not decided: +// each is matched to MusicBrainz (#5569). An album row with no tracks is +// not an album in the library. +func (q *Queries) ListAlbumsToMatch(ctx context.Context, arg ListAlbumsToMatchParams) ([]ListAlbumsToMatchRow, error) { + rows, err := q.db.Query(ctx, listAlbumsToMatch, arg.Cutoff, arg.MaxRows) + if err != nil { + return nil, err + } + defer rows.Close() + var items []ListAlbumsToMatchRow + for rows.Next() { + var i ListAlbumsToMatchRow + if err := rows.Scan( + &i.ID, + &i.Title, + &i.Mbid, + &i.ReleaseGroupMbid, + &i.ArtistName, + &i.ArtistMbid, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const listFollowGroups = `-- name: ListFollowGroups :many SELECT g.mbid, g.title, g.primary_type, g.secondary_types, g.first_release_date, EXISTS (SELECT 1 FROM follow_seen_groups s @@ -506,48 +635,6 @@ func (q *Queries) ListLibraryAlbums(ctx context.Context, arg ListLibraryAlbumsPa return result.RowsAffected(), nil } -const listLibraryAlbumsToName = `-- name: ListLibraryAlbumsToName :many -SELECT a.id, a.title, a.mbid FROM albums a - WHERE a.release_group_mbid IS NULL AND a.mbid IS NOT NULL AND a.mbid <> '' - AND a.created_at <= $1::timestamptz - ORDER BY a.created_at, a.id - LIMIT $2 -` - -type ListLibraryAlbumsToNameParams struct { - Cutoff pgtype.Timestamptz - MaxRows int32 -} - -type ListLibraryAlbumsToNameRow struct { - ID pgtype.UUID - Title string - Mbid *string -} - -// Library albums from before the cutoff that carry a release id but no -// release group yet: the listing asks MusicBrainz for each. ” marks one -// MusicBrainz could not name, so it is asked once. -func (q *Queries) ListLibraryAlbumsToName(ctx context.Context, arg ListLibraryAlbumsToNameParams) ([]ListLibraryAlbumsToNameRow, error) { - rows, err := q.db.Query(ctx, listLibraryAlbumsToName, arg.Cutoff, arg.MaxRows) - if err != nil { - return nil, err - } - defer rows.Close() - var items []ListLibraryAlbumsToNameRow - for rows.Next() { - var i ListLibraryAlbumsToNameRow - if err := rows.Scan(&i.ID, &i.Title, &i.Mbid); err != nil { - return nil, err - } - items = append(items, i) - } - if err := rows.Err(); err != nil { - return nil, err - } - return items, nil -} - const listOpenWants = `-- name: ListOpenWants :many SELECT id, release_group_mbid, artist_mbid, artist_name, title, primary_type, first_release_date, reason, album_id, created_by, wanted_at, fulfilled_at, created_at, updated_at, searched_at, search_outcome, search_considered, best_release, best_verdict, search_error FROM wants WHERE fulfilled_at IS NULL @@ -645,6 +732,49 @@ func (q *Queries) ListReleaseTypeProfiles(ctx context.Context) ([]ListReleaseTyp return items, nil } +const listUnmatchedLibraryAlbums = `-- name: ListUnmatchedLibraryAlbums :many +SELECT m.album_id, a.title, ar.name AS artist_name, m.detail + FROM library_album_matches m + JOIN albums a ON a.id = m.album_id + JOIN artists ar ON ar.id = a.artist_id + WHERE m.outcome = 'unmatched' + ORDER BY ar.name, a.title + LIMIT $1 +` + +type ListUnmatchedLibraryAlbumsRow struct { + AlbumID pgtype.UUID + Title string + ArtistName string + Detail string +} + +// The albums the listing could not match, for the Held tab. +func (q *Queries) ListUnmatchedLibraryAlbums(ctx context.Context, maxRows int32) ([]ListUnmatchedLibraryAlbumsRow, error) { + rows, err := q.db.Query(ctx, listUnmatchedLibraryAlbums, maxRows) + if err != nil { + return nil, err + } + defer rows.Close() + var items []ListUnmatchedLibraryAlbumsRow + for rows.Next() { + var i ListUnmatchedLibraryAlbumsRow + if err := rows.Scan( + &i.AlbumID, + &i.Title, + &i.ArtistName, + &i.Detail, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const listWants = `-- name: ListWants :many SELECT w.id, w.release_group_mbid, w.artist_mbid, w.artist_name, w.title, w.reason, w.album_id, w.wanted_at, w.fulfilled_at, @@ -806,6 +936,52 @@ func (q *Queries) MarkGroupSeen(ctx context.Context, arg MarkGroupSeenParams) er return err } +const recordAlbumMatch = `-- name: RecordAlbumMatch :exec +INSERT INTO library_album_matches (album_id, outcome, method, release_group_mbid, detail) +VALUES ($1, $2, $3, $4, $5) +ON CONFLICT (album_id) DO UPDATE + SET outcome = EXCLUDED.outcome, method = EXCLUDED.method, + release_group_mbid = EXCLUDED.release_group_mbid, detail = EXCLUDED.detail, decided_at = now() +` + +type RecordAlbumMatchParams struct { + AlbumID pgtype.UUID + Outcome string + Method string + ReleaseGroupMbid string + Detail string +} + +func (q *Queries) RecordAlbumMatch(ctx context.Context, arg RecordAlbumMatchParams) error { + _, err := q.db.Exec(ctx, recordAlbumMatch, + arg.AlbumID, + arg.Outcome, + arg.Method, + arg.ReleaseGroupMbid, + arg.Detail, + ) + return err +} + +const recordListedAlbums = `-- name: RecordListedAlbums :exec +INSERT INTO library_album_matches (album_id, outcome, method, release_group_mbid) +SELECT a.id, 'listed', 'tags', a.release_group_mbid + FROM albums a + JOIN artists ar ON ar.id = a.artist_id + WHERE a.release_group_mbid IS NOT NULL AND a.release_group_mbid <> '' + AND ar.mbid IS NOT NULL AND ar.mbid <> '' + AND a.created_at <= $1::timestamptz + AND EXISTS (SELECT 1 FROM tracks t WHERE t.album_id = a.id) +ON CONFLICT (album_id) DO NOTHING +` + +// The albums ListLibraryAlbums takes are decided as listed by their tags, +// so the matching that follows leaves them alone (#5569). +func (q *Queries) RecordListedAlbums(ctx context.Context, cutoff pgtype.Timestamptz) error { + _, err := q.db.Exec(ctx, recordListedAlbums, cutoff) + return err +} + const repointWant = `-- name: RepointWant :exec UPDATE wants SET release_group_mbid = $2, updated_at = now() WHERE id = $1 ` @@ -822,8 +998,8 @@ func (q *Queries) RepointWant(ctx context.Context, arg RepointWantParams) error } const setAlbumReleaseGroup = `-- name: SetAlbumReleaseGroup :exec -UPDATE albums SET release_group_mbid = $1 - WHERE id = $2 AND release_group_mbid IS NULL +UPDATE albums SET release_group_mbid = $1, updated_at = now() + WHERE id = $2 AND (release_group_mbid IS NULL OR release_group_mbid = '') ` type SetAlbumReleaseGroupParams struct { @@ -831,6 +1007,7 @@ type SetAlbumReleaseGroupParams struct { ID pgtype.UUID } +// Names a matched album's release group, filling only what nothing named. func (q *Queries) SetAlbumReleaseGroup(ctx context.Context, arg SetAlbumReleaseGroupParams) error { _, err := q.db.Exec(ctx, setAlbumReleaseGroup, arg.ReleaseGroupMbid, arg.ID) return err diff --git a/internal/db/migrations/0086_library_album_matches.down.sql b/internal/db/migrations/0086_library_album_matches.down.sql new file mode 100644 index 00000000..b61ff4fa --- /dev/null +++ b/internal/db/migrations/0086_library_album_matches.down.sql @@ -0,0 +1 @@ +DROP TABLE library_album_matches; diff --git a/internal/db/migrations/0086_library_album_matches.up.sql b/internal/db/migrations/0086_library_album_matches.up.sql new file mode 100644 index 00000000..712850ee --- /dev/null +++ b/internal/db/migrations/0086_library_album_matches.up.sql @@ -0,0 +1,47 @@ +-- Every library album is matched to MusicBrainz before it goes on the list +-- (#5569, operator 2026-10-10: "they should all exist in music brainz we +-- need to use their meta data or a search system to link them up"). The +-- listing in 0085 took only albums whose tags already named their release +-- group and artist; the rest are now matched by the release their tags +-- name, the artist's discography, or a search, and checked against their +-- own tracks. +-- +-- library_album_matches one row per album the listing has decided: +-- listed (with how it was matched) or unmatched +-- (with why). It is the record that the listing +-- saw the album, so an album taken off the list +-- is never listed again. +CREATE TABLE library_album_matches ( + album_id uuid PRIMARY KEY REFERENCES albums (id) ON DELETE CASCADE, + outcome text NOT NULL + CONSTRAINT library_album_matches_outcome_check CHECK (outcome IN ('listed', 'unmatched')), + -- tags its tags named the release group. + -- release its tags named a release; MusicBrainz named the group. + -- discography its title is one of the artist's release groups. + -- search MusicBrainz's search found it by artist and title. + method text NOT NULL DEFAULT '' + CONSTRAINT library_album_matches_method_check CHECK (method IN ('', 'tags', 'release', 'discography', 'search')), + release_group_mbid text NOT NULL DEFAULT '', + detail text NOT NULL DEFAULT '', + decided_at timestamptz NOT NULL DEFAULT now() +); + +CREATE INDEX library_album_matches_unmatched_idx ON library_album_matches (decided_at) WHERE outcome = 'unmatched'; + +-- The albums 0085's listing already took, by its own test, are decided: +-- one taken off the list since stays off. +INSERT INTO library_album_matches (album_id, outcome, method, release_group_mbid) +SELECT a.id, 'listed', 'tags', a.release_group_mbid + FROM albums a + JOIN artists ar ON ar.id = a.artist_id + JOIN acquisition_settings s ON s.id = true + WHERE s.library_listing_started_at IS NOT NULL + AND a.created_at <= s.library_listing_started_at + AND a.release_group_mbid IS NOT NULL AND a.release_group_mbid <> '' + AND ar.mbid IS NOT NULL AND ar.mbid <> '' + AND EXISTS (SELECT 1 FROM tracks t WHERE t.album_id = a.id); + +-- The rest are matched by the wanted worker after boot: the listing resumes. +UPDATE acquisition_settings + SET library_listed_at = NULL, library_unlisted = 0 + WHERE library_listing_started_at IS NOT NULL; diff --git a/internal/db/queries/wanted.sql b/internal/db/queries/wanted.sql index 394b6d76..2cb967b1 100644 --- a/internal/db/queries/wanted.sql +++ b/internal/db/queries/wanted.sql @@ -258,30 +258,76 @@ SELECT DISTINCT ON (a.release_group_mbid) (SELECT COUNT(*) FROM tracks t WHERE t.album_id = a.id AND t.missing_since IS NULL) DESC, a.id ON CONFLICT (release_group_mbid) DO NOTHING; --- name: ListLibraryAlbumsToName :many --- Library albums from before the cutoff that carry a release id but no --- release group yet: the listing asks MusicBrainz for each. '' marks one --- MusicBrainz could not name, so it is asked once. -SELECT a.id, a.title, a.mbid FROM albums a - WHERE a.release_group_mbid IS NULL AND a.mbid IS NOT NULL AND a.mbid <> '' +-- name: RecordListedAlbums :exec +-- The albums ListLibraryAlbums takes are decided as listed by their tags, +-- so the matching that follows leaves them alone (#5569). +INSERT INTO library_album_matches (album_id, outcome, method, release_group_mbid) +SELECT a.id, 'listed', 'tags', a.release_group_mbid + FROM albums a + JOIN artists ar ON ar.id = a.artist_id + WHERE a.release_group_mbid IS NOT NULL AND a.release_group_mbid <> '' + AND ar.mbid IS NOT NULL AND ar.mbid <> '' AND a.created_at <= sqlc.arg(cutoff)::timestamptz + AND EXISTS (SELECT 1 FROM tracks t WHERE t.album_id = a.id) +ON CONFLICT (album_id) DO NOTHING; + +-- name: ListAlbumsToMatch :many +-- Library albums from before the cutoff that the listing has not decided: +-- each is matched to MusicBrainz (#5569). An album row with no tracks is +-- not an album in the library. +SELECT a.id, a.title, a.mbid, a.release_group_mbid, ar.name AS artist_name, ar.mbid AS artist_mbid + FROM albums a + JOIN artists ar ON ar.id = a.artist_id + WHERE a.created_at <= sqlc.arg(cutoff)::timestamptz + AND EXISTS (SELECT 1 FROM tracks t WHERE t.album_id = a.id) + AND NOT EXISTS (SELECT 1 FROM library_album_matches m WHERE m.album_id = a.id) ORDER BY a.created_at, a.id LIMIT sqlc.arg(max_rows); +-- name: ListAlbumTracksToMatch :many +-- What an album's tracks say about themselves, to check a match against. +SELECT title, disc_number, track_number, duration_ms, mbid + FROM tracks + WHERE album_id = $1 + ORDER BY disc_number NULLS FIRST, track_number NULLS LAST, title; + -- name: SetAlbumReleaseGroup :exec -UPDATE albums SET release_group_mbid = sqlc.arg(release_group_mbid) - WHERE id = sqlc.arg(id) AND release_group_mbid IS NULL; +-- Names a matched album's release group, filling only what nothing named. +UPDATE albums SET release_group_mbid = sqlc.arg(release_group_mbid), updated_at = now() + WHERE id = sqlc.arg(id) AND (release_group_mbid IS NULL OR release_group_mbid = ''); + +-- name: InsertLibraryWant :execrows +-- Puts one matched album on the list as held, from just before its first +-- lost track if it has one. A group already on the list is left as it is. +INSERT INTO wants (release_group_mbid, artist_mbid, artist_name, title, primary_type, first_release_date, + reason, album_id, wanted_at, fulfilled_at) +VALUES (sqlc.arg(release_group_mbid), sqlc.arg(artist_mbid), sqlc.arg(artist_name), sqlc.arg(title), + sqlc.arg(primary_type), sqlc.arg(first_release_date), 'library', sqlc.arg(album_id)::uuid, now(), + COALESCE((SELECT MIN(t.missing_since) FROM tracks t WHERE t.album_id = sqlc.arg(album_id)::uuid) + - interval '1 second', now())) +ON CONFLICT (release_group_mbid) DO NOTHING; + +-- name: RecordAlbumMatch :exec +INSERT INTO library_album_matches (album_id, outcome, method, release_group_mbid, detail) +VALUES (sqlc.arg(album_id), sqlc.arg(outcome), sqlc.arg(method), sqlc.arg(release_group_mbid), sqlc.arg(detail)) +ON CONFLICT (album_id) DO UPDATE + SET outcome = EXCLUDED.outcome, method = EXCLUDED.method, + release_group_mbid = EXCLUDED.release_group_mbid, detail = EXCLUDED.detail, decided_at = now(); -- name: FinishLibraryListing :one --- The listing is done: count the albums from before the cutoff it could not --- list, because nothing names them to MusicBrainz. -UPDATE acquisition_settings s +-- The listing is done: count the albums no match was found for. +UPDATE acquisition_settings SET library_listed_at = now(), - library_unlisted = ( - SELECT COUNT(*) FROM albums a - WHERE a.created_at <= s.library_listing_started_at - AND NOT EXISTS (SELECT 1 FROM wants w - WHERE w.album_id = a.id - OR (a.release_group_mbid <> '' AND w.release_group_mbid = a.release_group_mbid))) - WHERE s.id = true AND s.library_listed_at IS NULL + library_unlisted = (SELECT COUNT(*) FROM library_album_matches WHERE outcome = 'unmatched') + WHERE id = true AND library_listed_at IS NULL RETURNING library_listed_at, library_unlisted; + +-- name: ListUnmatchedLibraryAlbums :many +-- The albums the listing could not match, for the Held tab. +SELECT m.album_id, a.title, ar.name AS artist_name, m.detail + FROM library_album_matches m + JOIN albums a ON a.id = m.album_id + JOIN artists ar ON ar.id = a.artist_id + WHERE m.outcome = 'unmatched' + ORDER BY ar.name, a.title + LIMIT sqlc.arg(max_rows); diff --git a/internal/dbtest/reset.go b/internal/dbtest/reset.go index 0a6cf757..ae8b602a 100644 --- a/internal/dbtest/reset.go +++ b/internal/dbtest/reset.go @@ -110,6 +110,7 @@ var dataTables = []string{ "mb_artists", // M502 #5519 "requests", // M502 #5520 (was lidarr_requests) "download_interventions", // M502 #5523 + "library_album_matches", // #5569 "release_blocks", // M502 #5521 "grabs", // M502 #5521 "want_candidates", // M502 #5521 diff --git a/internal/importer/verify.go b/internal/importer/verify.go index e5721fc2..b33925e8 100644 --- a/internal/importer/verify.go +++ b/internal/importer/verify.go @@ -351,8 +351,8 @@ func pairUp(files []File, tracks []Track) []Pair { File: order[i], Track: i, Recording: t.RecordingMBID != "" && (f.RecordingMBID == t.RecordingMBID || slices.Contains(f.Recordings, t.RecordingMBID)), - Length: lengthFits(f.LengthMs, t.LengthMs), - Title: titleAgrees(f.Title, t.Title), + Length: release.LengthFits(f.LengthMs, t.LengthMs), + Title: release.TitleAgrees(f.Title, t.Title), } } return pairs @@ -372,40 +372,6 @@ func numbered(files []File) bool { return true } -// lengthFits allows five seconds or 3%, whichever is more: remasters and -// rips differ by a second or two, a different recording by far more. A -// track MusicBrainz has no length for says nothing against the file. -func lengthFits(fileMs, trackMs int) bool { - if trackMs <= 0 { - return true - } - if fileMs <= 0 { - return false - } - tol := max(5000, trackMs*3/100) - d := fileMs - trackMs - return d <= tol && -d <= tol -} - -// titleAgrees: the same words, or one title holding all of the other's -// ("Get Lucky (Radio Edit)" holds "Get Lucky"). -func titleAgrees(a, b string) bool { - ta, tb := release.Tokens(a), release.Tokens(b) - if len(ta) == 0 || len(tb) == 0 { - return false - } - return subset(ta, tb) || subset(tb, ta) -} - -func subset(small, big []string) bool { - for _, w := range small { - if !slices.Contains(big, w) { - return false - } - } - return true -} - // score weighs recordings above lengths above titles: a fingerprint is the // audio itself, a length is the audio's shape, a title is only a name. func score(pairs []Pair) int { diff --git a/internal/importer/verify_test.go b/internal/importer/verify_test.go index 2a25d39f..94ec335f 100644 --- a/internal/importer/verify_test.go +++ b/internal/importer/verify_test.go @@ -161,22 +161,6 @@ func TestVerify_WithoutAcoustIDTheReasonSaysSo(t *testing.T) { } } -func TestLengthFits(t *testing.T) { - for _, c := range []struct { - file, track int - ok bool - }{ - // Short tracks get the 5s floor; longer ones 3% of the track. - {120000, 125000, true}, {120000, 126000, false}, - {240000, 246000, true}, {240000, 248000, false}, {600000, 615000, true}, {600000, 620000, false}, - {0, 240000, false}, {240000, 0, true}, - } { - if got := lengthFits(c.file, c.track); got != c.ok { - t.Errorf("lengthFits(%d, %d) = %v", c.file, c.track, got) - } - } -} - func TestPlan_LidarrsNaming(t *testing.T) { e := edition("rel-cd", 2) e.Tracks[1].Title = "Within: Part 2/3?" diff --git a/internal/musicbrainz/client_test.go b/internal/musicbrainz/client_test.go index d645984f..ded2815f 100644 --- a/internal/musicbrainz/client_test.go +++ b/internal/musicbrainz/client_test.go @@ -58,6 +58,27 @@ func TestSearchReleaseGroups_AsksTitleAndArtistAndReadsCredits(t *testing.T) { } } +func TestSearchReleaseGroupsBy_AsksForBothTitleAndArtist(t *testing.T) { + var query atomic.Value + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + query.Store(r.URL.Query().Get("query")) + _, _ = w.Write([]byte(`{"release-groups": [{"id": "rg1", "title": "Highway to Hell", + "artist-credit": [{"name": "AC/DC", "artist": {"id": "a1"}}]}]}`)) + })) + defer srv.Close() + + got, err := testClient(srv).SearchReleaseGroupsBy(context.Background(), " AC/DC ", "Highway to Hell", 10) + if err != nil { + t.Fatal(err) + } + if q := query.Load(); q != `releasegroup:(Highway to Hell) AND artist:(AC\/DC)` { + t.Errorf("query = %q", q) + } + if len(got) != 1 || got[0].MBID != "rg1" || got[0].ArtistMBIDs[0] != "a1" { + t.Errorf("got %+v", got) + } +} + func TestSearchRecordings_FilesASongUnderItsAlbum(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { _, _ = w.Write([]byte(`{"recordings": [{ diff --git a/internal/musicbrainz/entities.go b/internal/musicbrainz/entities.go index 21a449a4..6b6e4e04 100644 --- a/internal/musicbrainz/entities.go +++ b/internal/musicbrainz/entities.go @@ -252,6 +252,25 @@ func (c *Client) SearchReleaseGroups(ctx context.Context, text string, limit int return out, nil } +// SearchReleaseGroupsBy finds an artist's albums by title: both must +// match, where SearchReleaseGroups takes either. It is for naming an album +// already in hand, not for a person's search box. +func (c *Client) SearchReleaseGroupsBy(ctx context.Context, artist, title string, limit int) ([]ReleaseGroup, error) { + var resp struct { + ReleaseGroups []wireReleaseGroup `json:"release-groups"` + } + query := "releasegroup:(" + Escape(strings.TrimSpace(title)) + ") AND artist:(" + Escape(strings.TrimSpace(artist)) + ")" + q := url.Values{"query": {query}, "limit": {searchLimit(limit)}} + if err := c.get(ctx, "/release-group", q, &resp); err != nil { + return nil, err + } + out := make([]ReleaseGroup, 0, len(resp.ReleaseGroups)) + for _, g := range resp.ReleaseGroups { + out = append(out, g.group()) + } + return out, nil +} + // SearchRecordings finds songs by title or by artist. func (c *Client) SearchRecordings(ctx context.Context, text string, limit int) ([]Recording, error) { var resp struct { diff --git a/internal/release/agree.go b/internal/release/agree.go new file mode 100644 index 00000000..cd2b9822 --- /dev/null +++ b/internal/release/agree.go @@ -0,0 +1,40 @@ +package release + +import "slices" + +// Comparing a file against a MusicBrainz track: shared by the importer's +// check of a download and the library listing's match of an album (#5569). + +// LengthFits allows five seconds or 3%, whichever is more: remasters and +// rips differ by a second or two, a different recording by far more. A +// track MusicBrainz has no length for says nothing against the file. +func LengthFits(fileMs, trackMs int) bool { + if trackMs <= 0 { + return true + } + if fileMs <= 0 { + return false + } + tol := max(5000, trackMs*3/100) + d := fileMs - trackMs + return d <= tol && -d <= tol +} + +// TitleAgrees: the same words, or one title holding all of the other's +// ("Get Lucky (Radio Edit)" holds "Get Lucky"). +func TitleAgrees(a, b string) bool { + ta, tb := Tokens(a), Tokens(b) + if len(ta) == 0 || len(tb) == 0 { + return false + } + return subset(ta, tb) || subset(tb, ta) +} + +func subset(small, big []string) bool { + for _, w := range small { + if !slices.Contains(big, w) { + return false + } + } + return true +} diff --git a/internal/release/agree_test.go b/internal/release/agree_test.go new file mode 100644 index 00000000..50910fe7 --- /dev/null +++ b/internal/release/agree_test.go @@ -0,0 +1,19 @@ +package release + +import "testing" + +func TestLengthFits(t *testing.T) { + for _, c := range []struct { + file, track int + ok bool + }{ + // Short tracks get the 5s floor; longer ones 3% of the track. + {120000, 125000, true}, {120000, 126000, false}, + {240000, 246000, true}, {240000, 248000, false}, {600000, 615000, true}, {600000, 620000, false}, + {0, 240000, false}, {240000, 0, true}, + } { + if got := LengthFits(c.file, c.track); got != c.ok { + t.Errorf("LengthFits(%d, %d) = %v", c.file, c.track, got) + } + } +} diff --git a/internal/wanted/library.go b/internal/wanted/library.go index cefadbc0..8642d846 100644 --- a/internal/wanted/library.go +++ b/internal/wanted/library.go @@ -4,19 +4,24 @@ import ( "context" "errors" "fmt" + "time" + + "github.com/jackc/pgx/v5" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" - "git.fabledsword.com/bvandeusen/minstrel/internal/tags" ) // ReasonLibrary: on the list because the library held it when the list // began (#5567). const ReasonLibrary = "library" -// libraryNamesPerPass bounds the MusicBrainz lookups one pass makes for -// albums whose tags named a release but not its group: about two minutes at -// MusicBrainz's one request a second, so the pass still settles its wants. -const libraryNamesPerPass = 120 +// libraryMatchBudget bounds the time one pass spends matching albums to +// MusicBrainz, at its one request a second, so the pass still settles its +// wants; the rest wait for the next pass. +const libraryMatchBudget = 2 * time.Minute + +// libraryMatchBatch is how many albums one query hands the matching. +const libraryMatchBatch = 25 // listLibrary puts the library's albums on the wanted list as held, once // (operator, 2026-10-10: "make a record saying that the import has @@ -26,13 +31,15 @@ const libraryNamesPerPass = 120 // - the first step stamps when the listing began, which is its cutoff: // an album the library gains afterwards, dropped in by hand, is never // listed; -// - every album whose release group is known goes on the list at once; -// - albums that name a release but not its group are looked up on -// MusicBrainz, a bounded number a pass, and listed as each is named; -// - once none is left to name, the listing is stamped done, with a count -// of the albums nothing names to MusicBrainz. +// - every album whose tags name its release group and artist goes on the +// list at once; +// - every other album is matched to MusicBrainz (#5569), within a time +// budget a pass, and listed as it is matched; +// - once none is left to match, the listing is stamped done, with a count +// of the albums no match was found for. // -// After that it never runs again, so an album taken off the list stays off. +// Each album decided is recorded in library_album_matches, so an album taken +// off the list stays off, and once done the listing never runs again. func (s *Service) listLibrary(ctx context.Context, res *PassResult) error { q := dbq.New(s.pool) state, err := q.GetLibraryListing(ctx) @@ -47,6 +54,9 @@ func (s *Service) listLibrary(ctx context.Context, res *PassResult) error { if cutoff, err = q.StartLibraryListing(ctx); err != nil { return fmt.Errorf("wanted: start the library listing: %w", err) } + if err := q.RecordListedAlbums(ctx, cutoff); err != nil { + return fmt.Errorf("wanted: record the library: %w", err) + } n, err := q.ListLibraryAlbums(ctx, dbq.ListLibraryAlbumsParams{Cutoff: cutoff}) if err != nil { return fmt.Errorf("wanted: list the library: %w", err) @@ -55,10 +65,11 @@ func (s *Service) listLibrary(ctx context.Context, res *PassResult) error { s.logger.Info("wanted: listing the library's albums as held", "listed", n) } + deadline := s.now().Add(libraryMatchBudget) for { - albums, err := q.ListLibraryAlbumsToName(ctx, dbq.ListLibraryAlbumsToNameParams{Cutoff: cutoff, MaxRows: libraryNamesPerPass}) + albums, err := q.ListAlbumsToMatch(ctx, dbq.ListAlbumsToMatchParams{Cutoff: cutoff, MaxRows: libraryMatchBatch}) if err != nil { - return fmt.Errorf("wanted: albums to name: %w", err) + return fmt.Errorf("wanted: albums to match: %w", err) } if len(albums) == 0 { break @@ -67,45 +78,68 @@ func (s *Service) listLibrary(ctx context.Context, res *PassResult) error { if ctx.Err() != nil { return ctx.Err() } - group, err := s.releaseGroup(ctx, deref(a.Mbid)) - switch { - case errors.Is(err, tags.ErrSwitchedOff): - // Nothing can be named while MusicBrainz is off: the rest are - // counted as unlisted rather than left waiting for ever. - return s.finishLibrary(ctx, q, res) - case errors.Is(err, tags.ErrNotFound): - group = "" // MusicBrainz has none: asked once - case err != nil: - // Unreachable or refusing for now: the next pass goes on. - s.logger.Info("wanted: naming the library's albums paused", "err", err) + if s.now().After(deadline) { + return nil // the rest next pass + } + n, err := s.listAlbum(ctx, q, a) + if errors.Is(err, errMusicBrainzAway) { + s.logger.Info("wanted: matching the library's albums paused", "err", err) return nil } - if err := q.SetAlbumReleaseGroup(ctx, dbq.SetAlbumReleaseGroupParams{ID: a.ID, ReleaseGroupMbid: &group}); err != nil { - return fmt.Errorf("wanted: name album %s: %w", a.Title, err) - } - if group == "" { - continue - } - n, err := q.ListLibraryAlbums(ctx, dbq.ListLibraryAlbumsParams{Cutoff: cutoff, AlbumID: a.ID}) if err != nil { - return fmt.Errorf("wanted: list album %s: %w", a.Title, err) + return err } - res.Listed += int(n) - } - if len(albums) == libraryNamesPerPass { - return nil // the rest next pass + res.Listed += n } } return s.finishLibrary(ctx, q, res) } +// listAlbum matches one album and lists it, or records why it could not. +func (s *Service) listAlbum(ctx context.Context, q *dbq.Queries, a dbq.ListAlbumsToMatchRow) (int, error) { + rows, err := q.ListAlbumTracksToMatch(ctx, a.ID) + if err != nil { + return 0, fmt.Errorf("wanted: read %s's tracks: %w", a.Title, err) + } + m, err := s.matchAlbum(ctx, a, tracksToMatch(rows)) + if err != nil { + return 0, err + } + if !m.OK { + s.logger.Info("wanted: no MusicBrainz match for a library album", "album", a.Title, "artist", a.ArtistName, "why", m.Detail) + return 0, q.RecordAlbumMatch(ctx, dbq.RecordAlbumMatchParams{AlbumID: a.ID, Outcome: "unmatched", Detail: m.Detail}) + } + var listed int64 + err = pgx.BeginFunc(ctx, s.pool, func(tx pgx.Tx) error { + tq := dbq.New(tx) + if err := tq.SetAlbumReleaseGroup(ctx, dbq.SetAlbumReleaseGroupParams{ID: a.ID, ReleaseGroupMbid: &m.Group}); err != nil { + return err + } + n, err := tq.InsertLibraryWant(ctx, dbq.InsertLibraryWantParams{ + ReleaseGroupMbid: m.Group, ArtistMbid: m.ArtistMBID, ArtistName: m.ArtistName, Title: m.Title, + PrimaryType: m.PrimaryType, FirstReleaseDate: m.FirstReleaseDate, AlbumID: a.ID, + }) + if err != nil { + return err + } + listed = n + return tq.RecordAlbumMatch(ctx, dbq.RecordAlbumMatchParams{ + AlbumID: a.ID, Outcome: "listed", Method: m.Method, ReleaseGroupMbid: m.Group, + }) + }) + if err != nil { + return 0, fmt.Errorf("wanted: list album %s: %w", a.Title, err) + } + return int(listed), nil +} + func (s *Service) finishLibrary(ctx context.Context, q *dbq.Queries, res *PassResult) error { done, err := q.FinishLibraryListing(ctx) if err != nil { return fmt.Errorf("wanted: finish the library listing: %w", err) } s.logger.Info("wanted: the library's albums are on the list", "listed_this_pass", res.Listed, - "unlisted", done.LibraryUnlisted) + "unmatched", done.LibraryUnlisted) return nil } diff --git a/internal/wanted/library_test.go b/internal/wanted/library_test.go new file mode 100644 index 00000000..eb865d1a --- /dev/null +++ b/internal/wanted/library_test.go @@ -0,0 +1,262 @@ +package wanted + +import ( + "context" + "fmt" + "strings" + "testing" + "time" + + "github.com/jackc/pgx/v5/pgtype" + "github.com/jackc/pgx/v5/pgxpool" + + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" + "git.fabledsword.com/bvandeusen/minstrel/internal/tags" +) + +// startListing clears the reset's "already listed", as a fresh install has it. +func startListing(t *testing.T, pool *pgxpool.Pool) { + t.Helper() + if _, err := pool.Exec(context.Background(), ` + UPDATE acquisition_settings SET library_listing_started_at = NULL, library_listed_at = NULL, library_unlisted = 0`); err != nil { + t.Fatal(err) + } +} + +// seedAlbum puts an album in the library whose tracks are titled titles, +// numbered by numbers (from 1 when none are given). An artistMBID or +// release of "" leaves that id out, as untagged files do. +func seedAlbum(t *testing.T, q *dbq.Queries, artistMBID, title, rel string, titles []string, numbers ...int32) dbq.Album { + t.Helper() + ctx := context.Background() + ap := dbq.UpsertArtistParams{Name: "Blur", SortName: "Blur"} + if artistMBID != "" { + ap.Mbid = &artistMBID + } + artist, err := q.UpsertArtist(ctx, ap) + if err != nil { + t.Fatal(err) + } + params := dbq.UpsertAlbumParams{Title: title, SortTitle: title, ArtistID: artist.ID} + if rel != "" { + params.Mbid = &rel + } + album, err := q.UpsertAlbum(ctx, params) + if err != nil { + t.Fatal(err) + } + for i, name := range titles { + num := int32(i + 1) + if i < len(numbers) { + num = numbers[i] + } + if _, err := q.UpsertTrack(ctx, dbq.UpsertTrackParams{ + Title: name, AlbumID: album.ID, ArtistID: artist.ID, TrackNumber: &num, DurationMs: 180000, + FilePath: fmt.Sprintf("/music/%s/%02d.flac", strings.ReplaceAll(title, " ", "_"), num), FileSize: 1, FileFormat: "flac", + }); err != nil { + t.Fatal(err) + } + } + return album +} + +func matchOf(t *testing.T, pool *pgxpool.Pool, album pgtype.UUID) (outcome, method, group, detail string) { + t.Helper() + if err := pool.QueryRow(context.Background(), ` + SELECT outcome, method, release_group_mbid, detail FROM library_album_matches WHERE album_id = $1`, album, + ).Scan(&outcome, &method, &group, &detail); err != nil { + t.Fatalf("no match recorded: %v", err) + } + return +} + +// The library's albums go on the list as held, once (#5567). Tags that name +// the group list it at once; a release id is named through MusicBrainz; +// what nothing matches is recorded with why. Taken off the list, an album +// stays off, and one added to the library afterwards is never listed. +func TestListLibrary_OnceAndThenTheListIsTheTruth_Integration(t *testing.T) { + pool := newPool(t) + ctx := context.Background() + q := dbq.New(pool) + s := newService(t, pool, newFakeMB(t)) + s.releaseGroup = func(_ context.Context, release string) (string, error) { + if release == "rel-ballad" { + return theBallad, nil + } + return "", tags.ErrNotFound + } + blurAlbum := seedHeld(t, q, "Blur", "rel-blur", "rg-blur", 3) + ballad := seedAlbum(t, q, blur, "The Ballad of Darren", "rel-ballad", []string{"The Ballad", "St. Charles Square"}) + bootleg := seedAlbum(t, q, blur, "Bootleg", "rel-bootleg", []string{"Intro"}) + // An album with a track gone: held from before the loss, so + // re-acquisition still fetches it back. + lost := seedHeld(t, q, "13", "rel-13", "rg-13", 2) + if _, err := pool.Exec(ctx, `UPDATE tracks SET missing_since = now() - interval '1 day' + WHERE album_id = $1 AND track_number = 1`, lost.ID); err != nil { + t.Fatal(err) + } + // An album row with no tracks is not an album in the library. + if _, err := q.UpsertAlbum(ctx, dbq.UpsertAlbumParams{Title: "Empty", SortTitle: "Empty", ArtistID: blurAlbum.ArtistID}); err != nil { + t.Fatal(err) + } + startListing(t, pool) + + if err := s.listLibrary(ctx, &PassResult{}); err != nil { + t.Fatal(err) + } + for group, album := range map[string]pgtype.UUID{"rg-blur": blurAlbum.ID, theBallad: ballad.ID, "rg-13": lost.ID} { + w, err := q.GetWantByReleaseGroup(ctx, group) + if err != nil { + t.Fatalf("%s not listed: %v", group, err) + } + if w.Reason != ReasonLibrary || !w.FulfilledAt.Valid || w.AlbumID != album || w.ArtistMbid != blur { + t.Errorf("%s = %+v: a held library want about the album", group, w) + } + } + if w, _ := q.GetWantByReleaseGroup(ctx, "rg-13"); !w.FulfilledAt.Time.Before(time.Now().Add(-23 * time.Hour)) { + t.Errorf("an album missing a track is held from %v, not from before the loss", w.FulfilledAt.Time) + } + if outcome, method, _, _ := matchOf(t, pool, ballad.ID); outcome != "listed" || method != matchRelease { + t.Errorf("ballad decided %s by %s", outcome, method) + } + if outcome, _, _, detail := matchOf(t, pool, bootleg.ID); outcome != "unmatched" || !strings.Contains(detail, `no album called "Bootleg"`) { + t.Errorf("bootleg decided %s: %q", outcome, detail) + } + state, err := q.GetLibraryListing(ctx) + if err != nil || !state.LibraryListedAt.Valid || state.LibraryUnlisted != 1 { + t.Fatalf("listing = %+v %v: done, with only the bootleg unmatched", state, err) + } + + // Off the list stays off; an album added later is never listed. + w, _ := q.GetWantByReleaseGroup(ctx, "rg-blur") + if _, err := s.RemoveWant(ctx, w.ID); err != nil { + t.Fatal(err) + } + _ = seedHeld(t, q, "Think Tank", "rel-think", "rg-think", 2) + if _, err := s.Pass(ctx); err != nil { + t.Fatal(err) + } + for _, group := range []string{"rg-blur", "rg-think"} { + if _, err := q.GetWantByReleaseGroup(ctx, group); err == nil { + t.Errorf("%s is on the list", group) + } + } +} + +// An album whose tags name nothing is matched by its title among the +// artist's release groups, and taken only when its tracks agree: a partial +// copy agrees on the tracks it has. One whose title matches but whose +// tracks do not is left unmatched, with why. +func TestListLibrary_MatchesByDiscographyWhenTheTracksAgree_Integration(t *testing.T) { + pool := newPool(t) + ctx := context.Background() + q := dbq.New(pool) + s := newService(t, pool, newFakeMB(t)) + // MusicBrainz switched off for tags does not stop matching by name. + s.releaseGroup = func(context.Context, string) (string, error) { return "", tags.ErrSwitchedOff } + partial := seedAlbum(t, q, blur, "The Ballad of Darren (Deluxe)", "rel-ballad", []string{"St Charles Square"}, 2) + wrong := seedAlbum(t, q, blur, "Parklife", "", []string{"Girls & Boys", "Tracy Jacks"}) + startListing(t, pool) + + if err := s.listLibrary(ctx, &PassResult{}); err != nil { + t.Fatal(err) + } + if outcome, method, group, _ := matchOf(t, pool, partial.ID); outcome != "listed" || method != matchDiscography || group != theBallad { + t.Errorf("partial copy decided %s by %s as %s", outcome, method, group) + } + var named *string + if err := pool.QueryRow(ctx, `SELECT release_group_mbid FROM albums WHERE id = $1`, partial.ID).Scan(&named); err != nil || named == nil || *named != theBallad { + t.Errorf("the album's release group = %v %v", named, err) + } + if outcome, _, _, detail := matchOf(t, pool, wrong.ID); outcome != "unmatched" || !strings.Contains(detail, "none has its tracks") { + t.Errorf("Parklife without Parklife's tracks decided %s: %q", outcome, detail) + } + if _, err := q.GetWantByReleaseGroup(ctx, parklife); err == nil { + t.Error("Parklife listed on its title alone") + } +} + +// An artist the tags give no id is found by MusicBrainz's search on artist +// and title, and the want is filed under the artist MusicBrainz credits. +func TestListLibrary_MatchesBySearchWhenTheArtistHasNoID_Integration(t *testing.T) { + pool := newPool(t) + ctx := context.Background() + q := dbq.New(pool) + s := newService(t, pool, newFakeMB(t)) + album := seedAlbum(t, q, "", "The Ballad of Darren", "", []string{"The Ballad", "St. Charles Square"}) + startListing(t, pool) + + if err := s.listLibrary(ctx, &PassResult{}); err != nil { + t.Fatal(err) + } + if outcome, method, _, _ := matchOf(t, pool, album.ID); outcome != "listed" || method != matchSearch { + t.Errorf("decided %s by %s", outcome, method) + } + w, err := q.GetWantByReleaseGroup(ctx, theBallad) + if err != nil || w.ArtistMbid != blur || w.Reason != ReasonLibrary || !w.FulfilledAt.Valid { + t.Errorf("want = %+v %v", w, err) + } +} + +// A MusicBrainz that does not answer pauses the matching; the album is not +// decided on half the evidence, and the next pass goes on. +func TestListLibrary_UnreachablePausesTheMatching_Integration(t *testing.T) { + pool := newPool(t) + ctx := context.Background() + q := dbq.New(pool) + f := newFakeMB(t) + s := newService(t, pool, f) + album := seedAlbum(t, q, blur, "The Ballad of Darren", "", []string{"The Ballad", "St. Charles Square"}) + startListing(t, pool) + + f.down.Store(true) + if err := s.listLibrary(ctx, &PassResult{}); err != nil { + t.Fatal(err) + } + if state, _ := q.GetLibraryListing(ctx); state.LibraryListedAt.Valid { + t.Fatal("finished while an album was still to be matched") + } + var n int + if err := pool.QueryRow(ctx, `SELECT count(*) FROM library_album_matches WHERE album_id = $1`, album.ID).Scan(&n); err != nil || n != 0 { + t.Fatalf("decided while MusicBrainz was away: %d %v", n, err) + } + f.down.Store(false) + if err := s.listLibrary(ctx, &PassResult{}); err != nil { + t.Fatal(err) + } + if _, err := q.GetWantByReleaseGroup(ctx, theBallad); err != nil { + t.Errorf("not listed once MusicBrainz answered: %v", err) + } + if state, _ := q.GetLibraryListing(ctx); !state.LibraryListedAt.Valid { + t.Error("not finished") + } +} + +func TestTrackAgreement(t *testing.T) { + edition := []editionTrack{ + {Medium: 1, Position: 1, Title: "Intro", LengthMs: 60000, Recording: "rec-1"}, + {Medium: 1, Position: 2, Title: "Song", LengthMs: 200000}, + {Medium: 2, Position: 1, Title: "Encore", LengthMs: 300000}, + } + for _, c := range []struct { + name string + tracks []libraryTrack + want int + }{ + {"all, by position", []libraryTrack{ + {Disc: 1, Number: 1, Title: "Intro", LengthMs: 60000}, {Number: 2, Title: "Song", LengthMs: 201000}, + {Disc: 2, Number: 1, Title: "Encore", LengthMs: 300000}}, 3}, + {"a partial copy", []libraryTrack{{Disc: 2, Number: 1, Title: "Encore", LengthMs: 300500}}, 1}, + {"unnumbered, by title", []libraryTrack{{Title: "Song", LengthMs: 200000}, {Title: "Encore", LengthMs: 299000}}, 2}, + {"the recording, whatever its title", []libraryTrack{{Number: 1, Title: "Untitled", LengthMs: 1, Recording: "REC-1"}}, 1}, + {"the right title at the wrong length", []libraryTrack{{Number: 2, Title: "Song", LengthMs: 400000}}, 0}, + {"a track counted once", []libraryTrack{{Title: "Song", LengthMs: 200000}, {Title: "Song", LengthMs: 200000}}, 1}, + } { + if got := trackAgreement(c.tracks, edition); got != c.want { + t.Errorf("%s: %d agree, want %d", c.name, got, c.want) + } + } + if agrees(3, 4) || !agrees(4, 5) || agrees(0, 0) { + t.Error("agrees: 80% of the album's tracks, and an album has tracks") + } +} diff --git a/internal/wanted/match.go b/internal/wanted/match.go new file mode 100644 index 00000000..bcdf3ea8 --- /dev/null +++ b/internal/wanted/match.go @@ -0,0 +1,359 @@ +package wanted + +import ( + "context" + "errors" + "fmt" + "slices" + "strings" + "time" + + "git.fabledsword.com/bvandeusen/minstrel/internal/catalogue" + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" + "git.fabledsword.com/bvandeusen/minstrel/internal/musicbrainz" + "git.fabledsword.com/bvandeusen/minstrel/internal/release" + "git.fabledsword.com/bvandeusen/minstrel/internal/tags" +) + +// Matching a library album to its MusicBrainz release group (#5569). +// +// Everything in the library came from a service that matched it against +// MusicBrainz (operator, 2026-10-10: "they should all exist in music brainz +// we need to use their meta data or a search system to link them up"), so +// every album should have a group. The ids its tags carry are taken as +// they are. Without them, the groups called what the album is called, from +// the artist's discography and then from a search, are checked against the +// album's own tracks, and the one its tracks agree with is taken. + +// Match methods, as library_album_matches records them. +const ( + matchTags = "tags" + matchRelease = "release" + matchDiscography = "discography" + matchSearch = "search" +) + +// matchAgreement is the share of an album's tracks, in tenths, an edition +// must agree with: most, not all, since a rip's track may carry a typo or a +// hidden track's length. +const matchAgreement = 8 + +// matchCandidates bounds how many groups one album is checked against: each +// costs a MusicBrainz read the first time. +const matchCandidates = 8 + +// matchDiscographyAge is how old a held discography may be and still be +// read for matching: a name does not change, and a new release is not in +// the library. +const matchDiscographyAge = 90 * 24 * time.Hour + +// errMusicBrainzAway: MusicBrainz did not answer, so the album is left for +// the next pass rather than decided on half the evidence. +var errMusicBrainzAway = errors.New("wanted: MusicBrainz did not answer") + +// libraryTrack is what one of an album's tracks says about itself. +type libraryTrack struct { + Disc, Number int + Title string + LengthMs int + Recording string +} + +// editionTrack is one track of a MusicBrainz edition. +type editionTrack struct { + Medium, Position int + Title string + LengthMs int + Recording string +} + +// candidate is a release group an album may be. +type candidate struct { + Group string + Title string + PrimaryType string + FirstReleaseDate string + ArtistMBID string + ArtistName string + Method string +} + +// albumMatch is what matching decided: a group, or why there is none. +type albumMatch struct { + candidate + OK bool + Detail string +} + +func tracksToMatch(rows []dbq.ListAlbumTracksToMatchRow) []libraryTrack { + out := make([]libraryTrack, 0, len(rows)) + for _, r := range rows { + t := libraryTrack{Title: r.Title, LengthMs: int(r.DurationMs), Recording: deref(r.Mbid)} + if r.DiscNumber != nil { + t.Disc = int(*r.DiscNumber) + } + if r.TrackNumber != nil { + t.Number = int(*r.TrackNumber) + } + out = append(out, t) + } + return out +} + +// trackAgreement counts the album's tracks an edition agrees with: the same +// recording, or a length that fits and a title that agrees. A track is +// paired by its disc and number first, then by title, so an album missing +// some tracks still pairs the ones it has. +func trackAgreement(tracks []libraryTrack, edition []editionTrack) int { + byPos := make(map[[2]int]int, len(edition)) + for i, e := range edition { + byPos[[2]int{max(e.Medium, 1), e.Position}] = i + } + agree := func(t libraryTrack, e editionTrack) bool { + if t.Recording != "" && strings.EqualFold(t.Recording, e.Recording) { + return true + } + return release.LengthFits(t.LengthMs, e.LengthMs) && release.TitleAgrees(t.Title, e.Title) + } + used := make([]bool, len(edition)) + n := 0 + for _, t := range tracks { + if t.Number > 0 { + if i, ok := byPos[[2]int{max(t.Disc, 1), t.Number}]; ok && !used[i] && agree(t, edition[i]) { + used[i] = true + n++ + continue + } + } + for i, e := range edition { + if !used[i] && agree(t, e) { + used[i] = true + n++ + break + } + } + } + return n +} + +// agrees: enough of the album's tracks agree for the edition to be it. +func agrees(n, tracks int) bool { + return tracks > 0 && n*10 >= tracks*matchAgreement +} + +// sameTitle: the same words, not merely one holding the other. +func sameTitle(a, b string) bool { + return slices.Equal(release.Tokens(a), release.Tokens(b)) +} + +// matchAlbum finds the release group a library album is. A MusicBrainz +// that does not answer is errMusicBrainzAway; anything else MusicBrainz +// says is evidence, and an album with none is an albumMatch that is not OK. +func (s *Service) matchAlbum(ctx context.Context, a dbq.ListAlbumsToMatchRow, tracks []libraryTrack) (albumMatch, error) { + // The ids the tags carry name it. + if group := deref(a.ReleaseGroupMbid); group != "" { + if c, err := s.groupCandidate(ctx, group, matchTags, a); err != nil || c.Group != "" { + return albumMatch{candidate: c, OK: err == nil}, err + } + } + if rel := deref(a.Mbid); rel != "" { + group, err := s.releaseGroup(ctx, rel) + switch { + case errors.Is(err, tags.ErrNotFound): // includes MusicBrainz switched off for tags + case err != nil: + return albumMatch{}, fmt.Errorf("%w: %v", errMusicBrainzAway, err) + default: + if c, err := s.groupCandidate(ctx, group, matchRelease, a); err != nil || c.Group != "" { + return albumMatch{candidate: c, OK: err == nil}, err + } + } + } + + // Otherwise its name, checked against its tracks: the artist's own + // discography first, then a search. + tried := map[string]bool{} + var best albumMatch + bestN, bestEdition, considered := -1, 0, 0 + check := func(cands []candidate) error { + for _, c := range cands { + if tried[c.Group] || len(tried) >= matchCandidates { + continue + } + tried[c.Group] = true + n, size, err := s.agreementWith(ctx, c.Group, tracks) + if err != nil { + return err + } + considered++ + // More tracks agreeing wins; then the same title over a longer + // one; then the edition nearest the album's length. + st, bt := sameTitle(c.Title, a.Title), sameTitle(best.Title, a.Title) + if n > bestN || (n == bestN && ((st && !bt) || (st == bt && abs(size-len(tracks)) < abs(bestEdition-len(tracks))))) { + best, bestN, bestEdition = albumMatch{candidate: c}, n, size + } + } + return nil + } + discography, err := s.discographyCandidates(ctx, a) + if err != nil { + return albumMatch{}, err + } + if err := check(discography); err != nil { + return albumMatch{}, err + } + if !agrees(bestN, len(tracks)) { + found, err := s.searchCandidates(ctx, a) + if err != nil { + return albumMatch{}, err + } + if err := check(found); err != nil { + return albumMatch{}, err + } + } + if agrees(bestN, len(tracks)) { + best.OK = true + return best, nil + } + if considered == 0 { + return albumMatch{Detail: fmt.Sprintf("MusicBrainz has no album called %q by %s.", a.Title, a.ArtistName)}, nil + } + return albumMatch{Detail: fmt.Sprintf( + "MusicBrainz has %d %s called %q by %s, but none has its tracks: the closest, %q, agrees on %d of its %d.", + considered, plural(considered, "album", "albums"), a.Title, a.ArtistName, best.Title, max(bestN, 0), len(tracks))}, nil +} + +// groupCandidate reads a group the tags name. A group MusicBrainz does not +// have is a zero candidate, so the album is matched by name instead. +func (s *Service) groupCandidate(ctx context.Context, group, method string, a dbq.ListAlbumsToMatchRow) (candidate, error) { + g, err := s.cat.MusicBrainz().ReleaseGroup(ctx, group) + switch { + case errors.Is(err, musicbrainz.ErrNotFound): + return candidate{}, nil + case err != nil: + return candidate{}, fmt.Errorf("%w: %v", errMusicBrainzAway, err) + } + artist := creditedArtist(g.ArtistMBIDs, deref(a.ArtistMbid)) + if artist == "" { + return candidate{}, nil + } + return candidate{Group: g.MBID, Title: g.Title, PrimaryType: g.PrimaryType, FirstReleaseDate: g.FirstReleaseDate, + ArtistMBID: artist, ArtistName: g.ArtistCredit, Method: method}, nil +} + +// discographyCandidates are the artist's release groups called what the +// album is called. +func (s *Service) discographyCandidates(ctx context.Context, a dbq.ListAlbumsToMatchRow) ([]candidate, error) { + artist := deref(a.ArtistMbid) + if !Followable(artist, a.ArtistName) { + return nil, nil // no artist, or Various Artists: thousands of groups + } + d, err := s.cat.DiscographyWithin(ctx, artist, matchDiscographyAge) + switch { + case errors.Is(err, catalogue.ErrNotFound): + return nil, nil + case err != nil: + return nil, fmt.Errorf("%w: %v", errMusicBrainzAway, err) + } + var out []candidate + for _, g := range d.Groups { + if release.TitleAgrees(g.Title, a.Title) { + out = append(out, candidate{Group: g.Mbid, Title: g.Title, PrimaryType: g.PrimaryType, + FirstReleaseDate: g.FirstReleaseDate, ArtistMBID: artist, ArtistName: d.Artist.Name, Method: matchDiscography}) + } + } + sortSameTitleFirst(out, a.Title) + return out, nil +} + +// searchCandidates are the groups MusicBrainz's search finds by the album's +// artist and title, whose title and artist both agree. +func (s *Service) searchCandidates(ctx context.Context, a dbq.ListAlbumsToMatchRow) ([]candidate, error) { + found, err := s.cat.MusicBrainz().SearchReleaseGroupsBy(ctx, a.ArtistName, a.Title, 10) + switch { + case errors.Is(err, musicbrainz.ErrNotFound): + return nil, nil + case errors.Is(err, musicbrainz.ErrBadResponse): + // A query MusicBrainz will not read is not a reason to wait. + s.logger.Warn("wanted: MusicBrainz refused a search", "album", a.Title, "artist", a.ArtistName, "err", err) + return nil, nil + case err != nil: + return nil, fmt.Errorf("%w: %v", errMusicBrainzAway, err) + } + var out []candidate + for _, g := range found { + artist := creditedArtist(g.ArtistMBIDs, deref(a.ArtistMbid)) + if artist == "" || !release.TitleAgrees(g.Title, a.Title) { + continue + } + if !slices.Contains(g.ArtistMBIDs, deref(a.ArtistMbid)) && !release.TitleAgrees(g.ArtistCredit, a.ArtistName) { + continue + } + out = append(out, candidate{Group: g.MBID, Title: g.Title, PrimaryType: g.PrimaryType, + FirstReleaseDate: g.FirstReleaseDate, ArtistMBID: artist, ArtistName: g.ArtistCredit, Method: matchSearch}) + } + sortSameTitleFirst(out, a.Title) + return out, nil +} + +// agreementWith is how many of the album's tracks the group's best edition +// agrees with, and that edition's length. A group MusicBrainz no longer has +// agrees with none. +func (s *Service) agreementWith(ctx context.Context, group string, tracks []libraryTrack) (n, size int, err error) { + eds, err := s.cat.Releases(ctx, group) + switch { + case errors.Is(err, catalogue.ErrNotFound): + return -1, 0, nil + case err != nil: + return 0, 0, fmt.Errorf("%w: %v", errMusicBrainzAway, err) + } + n = -1 + for _, e := range eds.Editions { + edition := make([]editionTrack, 0, len(e.Tracks)) + for _, t := range e.Tracks { + edition = append(edition, editionTrack{Medium: int(t.MediumPosition), Position: int(t.Position), + Title: t.Title, LengthMs: int(t.LengthMs), Recording: t.RecordingMbid}) + } + if got := trackAgreement(tracks, edition); got > n || (got == n && abs(len(edition)-len(tracks)) < abs(size-len(tracks))) { + n, size = got, len(edition) + } + } + return n, size, nil +} + +// creditedArtist is the artist a want is filed under: the library's own +// artist when MusicBrainz credits them, else the first credited. +func creditedArtist(credited []string, own string) string { + if own != "" && (len(credited) == 0 || slices.Contains(credited, own)) { + return own + } + if len(credited) > 0 { + return credited[0] + } + return "" +} + +func sortSameTitleFirst(cs []candidate, title string) { + slices.SortStableFunc(cs, func(x, y candidate) int { + switch sx, sy := sameTitle(x.Title, title), sameTitle(y.Title, title); { + case sx && !sy: + return -1 + case sy && !sx: + return 1 + } + return 0 + }) +} + +func abs(n int) int { + if n < 0 { + return -n + } + return n +} + +func plural(n int, one, many string) string { + if n == 1 { + return one + } + return many +} diff --git a/internal/wanted/wanted_test.go b/internal/wanted/wanted_test.go index 0b7ecd09..e0f5270e 100644 --- a/internal/wanted/wanted_test.go +++ b/internal/wanted/wanted_test.go @@ -9,6 +9,7 @@ import ( "net/http" "net/http/httptest" "os" + "strings" "sync/atomic" "testing" "time" @@ -226,6 +227,13 @@ func newFakeMB(t *testing.T) *fakeMB { {"position": 3, "number": "3", "title": "Bonus", "recording": {"id": "rec-3"}}]}]}]}`) case r.URL.Path == "/release": _, _ = fmt.Fprint(w, `{"release-count": 0, "releases": []}`) + case r.URL.Path == "/release-group" && strings.Contains(r.URL.Query().Get("query"), "releasegroup:"): + // The library listing's search by artist and title (#5569). + found := "" + if strings.Contains(r.URL.Query().Get("query"), "Ballad") { + found = group(theBallad, "The Ballad of Darren", "Album", "", "2023-07-21") + } + _, _ = fmt.Fprintf(w, `{"release-groups": [%s]}`, found) default: w.WriteHeader(http.StatusNotFound) } @@ -455,135 +463,3 @@ func seedUser(t *testing.T, pool *pgxpool.Pool) pgtype.UUID { } return u.ID } - -// startListing clears the reset's "already listed", as a fresh install has it. -func startListing(t *testing.T, pool *pgxpool.Pool) { - t.Helper() - if _, err := pool.Exec(context.Background(), ` - UPDATE acquisition_settings SET library_listing_started_at = NULL, library_listed_at = NULL, library_unlisted = 0`); err != nil { - t.Fatal(err) - } -} - -// The library's albums go on the list as held, once (#5567). An album the -// tags name by release only is named through MusicBrainz first; one nothing -// names is counted, not listed. Taken off the list, an album stays off, and -// one added to the library afterwards is never listed. -func TestListLibrary_OnceAndThenTheListIsTheTruth_Integration(t *testing.T) { - pool := newPool(t) - ctx := context.Background() - q := dbq.New(pool) - s := newService(t, pool, newFakeMB(t)) - s.releaseGroup = func(_ context.Context, release string) (string, error) { - if release == "rel-parklife" { - return "rg-parklife", nil - } - return "", tags.ErrNotFound - } - blurAlbum := seedHeld(t, q, "Blur", "rel-blur", "rg-blur", 3) - parklife := seedHeld(t, q, "Parklife", "rel-parklife", "", 2) - _ = seedHeld(t, q, "Bootleg", "rel-bootleg", "", 1) - // An album with a track gone: held from before the loss, so - // re-acquisition still fetches it back. - lost := seedHeld(t, q, "13", "rel-13", "rg-13", 2) - if _, err := pool.Exec(ctx, `UPDATE tracks SET missing_since = now() - interval '1 day' - WHERE album_id = $1 AND track_number = 1`, lost.ID); err != nil { - t.Fatal(err) - } - startListing(t, pool) - - if err := s.listLibrary(ctx, &PassResult{}); err != nil { - t.Fatal(err) - } - for group, album := range map[string]pgtype.UUID{"rg-blur": blurAlbum.ID, "rg-parklife": parklife.ID, "rg-13": lost.ID} { - w, err := q.GetWantByReleaseGroup(ctx, group) - if err != nil { - t.Fatalf("%s not listed: %v", group, err) - } - if w.Reason != ReasonLibrary || !w.FulfilledAt.Valid || w.AlbumID != album { - t.Errorf("%s = %+v: a held library want about the album", group, w) - } - } - if w, _ := q.GetWantByReleaseGroup(ctx, "rg-13"); !w.FulfilledAt.Time.Before(time.Now().Add(-23 * time.Hour)) { - t.Errorf("an album missing a track is held from %v, not from before the loss", w.FulfilledAt.Time) - } - state, err := q.GetLibraryListing(ctx) - if err != nil || !state.LibraryListedAt.Valid || state.LibraryUnlisted != 1 { - t.Fatalf("listing = %+v %v: done, with the bootleg unlisted", state, err) - } - - // Off the list stays off; an album added later is never listed. - w, _ := q.GetWantByReleaseGroup(ctx, "rg-blur") - if _, err := s.RemoveWant(ctx, w.ID); err != nil { - t.Fatal(err) - } - _ = seedHeld(t, q, "Think Tank", "rel-think", "rg-think", 2) - if _, err := s.Pass(ctx); err != nil { - t.Fatal(err) - } - for _, group := range []string{"rg-blur", "rg-think"} { - if _, err := q.GetWantByReleaseGroup(ctx, group); err == nil { - t.Errorf("%s is on the list", group) - } - } -} - -// While MusicBrainz is switched off nothing can be named, so the listing -// finishes with what it has rather than waiting for ever. -func TestListLibrary_MusicBrainzOffFinishesWithWhatIsNamed_Integration(t *testing.T) { - pool := newPool(t) - ctx := context.Background() - q := dbq.New(pool) - s := newService(t, pool, newFakeMB(t)) - s.releaseGroup = func(context.Context, string) (string, error) { return "", tags.ErrSwitchedOff } - _ = seedHeld(t, q, "Blur", "rel-blur", "rg-blur", 1) - _ = seedHeld(t, q, "Parklife", "rel-parklife", "", 1) - startListing(t, pool) - if err := s.listLibrary(ctx, &PassResult{}); err != nil { - t.Fatal(err) - } - state, _ := q.GetLibraryListing(ctx) - if !state.LibraryListedAt.Valid || state.LibraryUnlisted != 1 { - t.Errorf("listing = %+v", state) - } - var named *string - if err := pool.QueryRow(ctx, `SELECT release_group_mbid FROM albums WHERE title = 'Parklife'`).Scan(&named); err != nil { - t.Fatal(err) - } - if named != nil { - t.Errorf("an album was marked unnameable while MusicBrainz was off: %q", *named) - } -} - -// A MusicBrainz that does not answer pauses the naming; the next pass goes on. -func TestListLibrary_UnreachablePausesTheNaming_Integration(t *testing.T) { - pool := newPool(t) - ctx := context.Background() - q := dbq.New(pool) - s := newService(t, pool, newFakeMB(t)) - down := true - s.releaseGroup = func(context.Context, string) (string, error) { - if down { - return "", tags.ErrTransient - } - return "rg-parklife", nil - } - _ = seedHeld(t, q, "Parklife", "rel-parklife", "", 1) - startListing(t, pool) - if err := s.listLibrary(ctx, &PassResult{}); err != nil { - t.Fatal(err) - } - if state, _ := q.GetLibraryListing(ctx); state.LibraryListedAt.Valid { - t.Fatal("finished while an album was still to be named") - } - down = false - if err := s.listLibrary(ctx, &PassResult{}); err != nil { - t.Fatal(err) - } - if _, err := q.GetWantByReleaseGroup(ctx, "rg-parklife"); err != nil { - t.Errorf("not listed once named: %v", err) - } - if state, _ := q.GetLibraryListing(ctx); !state.LibraryListedAt.Valid { - t.Error("not finished") - } -} diff --git a/web/src/lib/api/types.ts b/web/src/lib/api/types.ts index fe1e4953..957fc4a3 100644 --- a/web/src/lib/api/types.ts +++ b/web/src/lib/api/types.ts @@ -815,7 +815,9 @@ export type ReleaseBlocksPage = { items: ReleaseBlock[]; limit: number; offset: // The one-time listing of the library's albums as held (#5567): when it // finished (null while it runs), and how many albums nothing names to // MusicBrainz, so they could not be listed. -export type LibraryListing = { listed_at: string | null; unlisted: number }; +export type UnmatchedAlbum = { album_id: string; title: string; artist: string; detail: string }; + +export type LibraryListing = { listed_at: string | null; unlisted: number; unmatched: UnmatchedAlbum[] }; export type WantsPage = Page & { counts: WantCounts; library: LibraryListing }; diff --git a/web/src/lib/components/wanted/WantList.svelte b/web/src/lib/components/wanted/WantList.svelte index ee953bb5..168f9fee 100644 --- a/web/src/lib/components/wanted/WantList.svelte +++ b/web/src/lib/components/wanted/WantList.svelte @@ -14,8 +14,9 @@ // the missing albums wanted back, or the ones held. Each open want says // what its last search found and how its download is getting on; Releases // lists every candidate with its verdict, and Search runs one now. The - // library's albums were put on the list as held once (#5567); taking one - // off leaves its files and stops it being fetched back if they go. + // library's albums were put on the list as held once (#5567), each matched to + // MusicBrainz (#5569); taking one off leaves its files and stops it being + // fetched back if they go. The ones no match was found for are named here. let { list }: { list: WantState } = $props(); @@ -26,6 +27,7 @@ const library = $derived(wants.data?.pages[0]?.library); let busy = $state(null); + let showUnmatched = $state(false); let searching = $state(null); let open = $state(null); @@ -162,16 +164,46 @@ {#if list === 'held' && library} {#if !library.listed_at}

- Your library's albums are being added to this list as held. Albums whose tags name only a - release are looked up on MusicBrainz first, about one a second. + Your library's albums are being added to this list as held. Albums whose tags don't name them + are matched on MusicBrainz first, by their artist, title and tracks, about one a second.

{:else if library.unlisted > 0} -

- {library.unlisted === 1 ? '1 album' : `${library.unlisted} albums`} in your library - {library.unlisted === 1 ? "isn't" : "aren't"} on this list: nothing in {library.unlisted === 1 ? 'its' : 'their'} - tags names {library.unlisted === 1 ? 'it' : 'them'} to MusicBrainz. Those are never fetched back if - their files go missing. -

+
+

+ {library.unlisted === 1 ? '1 album' : `${library.unlisted} albums`} in your library + {library.unlisted === 1 ? "isn't" : "aren't"} on this list: no MusicBrainz album matched + {library.unlisted === 1 ? 'its' : 'their'} artist, title and tracks. Until + {library.unlisted === 1 ? "it's" : "they're"} matched, {library.unlisted === 1 ? "it isn't" : "they aren't"} + fetched back if {library.unlisted === 1 ? 'its' : 'their'} files go missing. +

+ {#if library.unmatched.length > 0} + + {#if showUnmatched} +
    + {#each library.unmatched as u (u.album_id)} +
  • + {u.title} + · {u.artist} +
    {u.detail}
    +
    • + {/each} +
+ {#if library.unmatched.length < library.unlisted} +

+ The first {library.unmatched.length} of {library.unlisted}. +

+ {/if} + {/if} + {/if} +
{/if} {/if} diff --git a/web/src/lib/components/wanted/WantList.test.ts b/web/src/lib/components/wanted/WantList.test.ts index b4e5b8a3..e7d7c283 100644 --- a/web/src/lib/components/wanted/WantList.test.ts +++ b/web/src/lib/components/wanted/WantList.test.ts @@ -1,39 +1,25 @@ -import { afterEach, describe, expect, test, vi } from "vitest"; -import { fireEvent, render, screen, waitFor } from "@testing-library/svelte"; -import { mockInfiniteQuery, mockQuery } from "../../../test-utils/query"; -import type { - LibraryListing, - Want, - WantGrab, - WantsPage, -} from "#lib/api/types.js"; +import { afterEach, describe, expect, test, vi } from 'vitest'; +import { fireEvent, render, screen, waitFor } from '@testing-library/svelte'; +import { mockInfiniteQuery, mockQuery } from '../../../test-utils/query'; +import type { LibraryListing, Want, WantGrab, WantsPage } from '#lib/api/types.js'; -vi.mock("#lib/api/wanted.js", () => ({ +vi.mock('#lib/api/wanted.js', () => ({ createWantsQuery: vi.fn(), removeWant: vi.fn().mockResolvedValue(undefined), searchWant: vi.fn().mockResolvedValue({ - outcome: "nothing", - considered: 0, - best_release: "", - best_verdict: "", - narrowed: [], + outcome: 'nothing', considered: 0, best_release: '', best_verdict: '', narrowed: [] }), createCandidatesQuery: vi.fn(), grabCandidate: vi.fn(), blockCandidate: vi.fn(), - unblockRelease: vi.fn(), + unblockRelease: vi.fn() })); -vi.mock("#lib/stores/toast.svelte.js", () => ({ pushToast: vi.fn() })); +vi.mock('#lib/stores/toast.svelte.js', () => ({ pushToast: vi.fn() })); -import WantList from "./WantList.svelte"; -import { pushToast } from "#lib/stores/toast.svelte.js"; -import { - createCandidatesQuery, - createWantsQuery, - removeWant, - searchWant, -} from "#lib/api/wanted.js"; -import type { WantState } from "#lib/api/types.js"; +import WantList from './WantList.svelte'; +import { pushToast } from '#lib/stores/toast.svelte.js'; +import { createCandidatesQuery, createWantsQuery, removeWant, searchWant } from '#lib/api/wanted.js'; +import type { WantState } from '#lib/api/types.js'; const DAY = 24 * 3_600_000; const ago = (ms: number) => new Date(Date.now() - ms).toISOString(); @@ -42,305 +28,203 @@ function want(id: string, extra: Partial = {}): Want { return { id, release_group_mbid: `rg-${id}`, - title: "The Ballad of Darren", - artist_mbid: "blur", - artist_name: "Blur", - artist_id: "art-1", - primary_type: "Album", + title: 'The Ballad of Darren', + artist_mbid: 'blur', + artist_name: 'Blur', + artist_id: 'art-1', + primary_type: 'Album', secondary_types: [], - year: "2023", - reason: "follow", + year: '2023', + reason: 'follow', wanted_at: ago(2 * DAY), held_at: null, - image_url: "", - ...extra, + image_url: '', + ...extra }; } function page( items: Want[], - library: LibraryListing = { listed_at: ago(DAY), unlisted: 0 }, + library: LibraryListing = { listed_at: ago(DAY), unlisted: 0, unmatched: [] } ): WantsPage { return { items, total: items.length, limit: 50, offset: 0, - counts: { - wanted: 2, - missing: 1, - held: 4, - following: 1, - activity: 1, - attention: 0, - blocked: 0, - }, - library, + counts: { wanted: 2, missing: 1, held: 4, following: 1, activity: 1, attention: 0, blocked: 0 }, + library }; } function grab(extra: Partial = {}): WantGrab { return { - id: "g1", - name: "Blur - Blur (1997) [FLAC]", + id: 'g1', + name: 'Blur - Blur (1997) [FLAC]', grabbed_at: ago(60_000), - origin: "schedule", - quality: "lossless", + origin: 'schedule', + quality: 'lossless', live: true, - end_reason: "", + end_reason: '', progress: 0, - import_state: "", - import_reason: "", - end_detail: "", + import_state: '', + import_reason: '', + end_detail: '', imported_at: null, imported_files: 0, - ...extra, + ...extra }; } afterEach(() => vi.clearAllMocks()); function setup( - items: Want[] = [ - want("w1"), - want("w2", { reason: "request", requested_by: "sam", title: "Parklife" }), - ], - state: WantState = "wanted", - library?: LibraryListing, + items: Want[] = [want('w1'), want('w2', { reason: 'request', requested_by: 'sam', title: 'Parklife' })], + state: WantState = 'wanted', + library?: LibraryListing ) { vi.mocked(createWantsQuery).mockReturnValue( - mockInfiniteQuery({ - pages: [page(items, library)], - }) as unknown as ReturnType, + mockInfiniteQuery({ pages: [page(items, library)] }) as unknown as ReturnType ); vi.mocked(createCandidatesQuery).mockReturnValue( - mockQuery({ data: [] }) as unknown as ReturnType< - typeof createCandidatesQuery - >, + mockQuery({ data: [] }) as unknown as ReturnType ); return render(WantList, { list: state }); } -describe("WantList", () => { - test("asks for its own state", () => { - setup([], "missing"); - expect(createWantsQuery).toHaveBeenCalledWith("missing"); +describe('WantList', () => { + test('asks for its own state', () => { + setup([], 'missing'); + expect(createWantsQuery).toHaveBeenCalledWith('missing'); expect(screen.getByText(/no missing albums are wanted back/i)).toBeTruthy(); }); - test("a want says why it is wanted", () => { + test('a want says why it is wanted', () => { setup(); - const why = screen - .getAllByTestId("want-why") - .map((e) => e.textContent ?? ""); + const why = screen.getAllByTestId('want-why').map((e) => e.textContent ?? ''); expect(why[0]).toMatch(/new from a followed artist/i); expect(why[1]).toMatch(/requested by sam/i); }); - test("remove takes a want off the list", async () => { + test('remove takes a want off the list', async () => { setup(); - await fireEvent.click( - screen.getByRole("button", { name: /stop wanting parklife/i }), - ); - await waitFor(() => expect(removeWant).toHaveBeenCalledWith("w2")); + await fireEvent.click(screen.getByRole('button', { name: /stop wanting parklife/i })); + await waitFor(() => expect(removeWant).toHaveBeenCalledWith('w2')); }); - test("a held album can be taken off the list, and is not searched for", async () => { - setup( - [want("w1", { held_at: ago(DAY), album_id: "al-1", reason: "library" })], - "held", - ); - expect( - screen.queryByRole("button", { name: /stop wanting|search for/i }), - ).toBeNull(); - expect( - screen - .getByRole("link", { name: /the ballad of darren/i }) - .getAttribute("href"), - ).toBe("/albums/al-1"); - expect(screen.getByTestId("want-why").textContent).toMatch( - /in your library when the list began/i, - ); - await fireEvent.click( - screen.getByRole("button", { - name: /take the ballad of darren off the list/i, - }), - ); - await waitFor(() => expect(removeWant).toHaveBeenCalledWith("w1")); + test('a held album can be taken off the list, and is not searched for', async () => { + setup([want('w1', { held_at: ago(DAY), album_id: 'al-1', reason: 'library' })], 'held'); + expect(screen.queryByRole('button', { name: /stop wanting|search for/i })).toBeNull(); + expect(screen.getByRole('link', { name: /the ballad of darren/i }).getAttribute('href')).toBe('/albums/al-1'); + expect(screen.getByTestId('want-why').textContent).toMatch(/in your library when the list began/i); + await fireEvent.click(screen.getByRole('button', { name: /take the ballad of darren off the list/i })); + await waitFor(() => expect(removeWant).toHaveBeenCalledWith('w1')); }); - test("the held tab says while the library is being listed, and what could not be", () => { - setup([], "held", { listed_at: null, unlisted: 0 }); - expect(screen.getByTestId("library-listing").textContent).toMatch( - /being added to this list/i, - ); + test('the held tab says while the library is being listed', () => { + setup([], 'held', { listed_at: null, unlisted: 0, unmatched: [] }); + expect(screen.getByTestId('library-listing').textContent).toMatch(/being added to this list/i); }); - test("the held tab names how many albums nothing names to MusicBrainz", () => { - setup([want("w1", { held_at: ago(DAY) })], "held", { + test('the held tab names the albums no MusicBrainz album matched, on request', async () => { + setup([want('w1', { held_at: ago(DAY) })], 'held', { listed_at: ago(DAY), unlisted: 12, + unmatched: [ + { album_id: 'al-9', title: 'Demo Tape', artist: 'Blur', detail: 'MusicBrainz has no album called "Demo Tape" by Blur.' } + ] }); - expect(screen.getByTestId("library-listing").textContent).toMatch( - /12 albums in your library\s+aren't on this list/i, - ); + expect(screen.getByTestId('library-listing').textContent).toMatch(/12 albums in your library\s+aren't on this list/i); + expect(screen.queryByTestId('unmatched-albums')).toBeNull(); + await fireEvent.click(screen.getByRole('button', { name: /show them/i })); + const list = screen.getByTestId('unmatched-albums'); + expect(list.textContent).toMatch(/Demo Tape/); + expect(list.textContent).toMatch(/has no album called/); + expect(screen.getByRole('link', { name: 'Demo Tape' }).getAttribute('href')).toBe('/albums/al-9'); + expect(screen.getByText(/the first 1 of 12/i)).toBeTruthy(); }); - test("an empty list explains what lands there", () => { + test('an empty list explains what lands there', () => { setup([]); expect(screen.getByText(/nothing is wanted/i)).toBeTruthy(); }); - test("each open want says what its last search found", () => { + test('each open want says what its last search found', () => { setup([ - want("w1"), - want("w2", { - title: "Parklife", - search: { - searched_at: ago(3_600_000), - outcome: "nothing", - considered: 4, - best_release: "x", - best_verdict: "rejected", - error: "", - }, - }), - want("w3", { title: "Blur", grab: grab() }), - want("w4", { - title: "13", - grab: grab({ import_state: "downloading", progress: 0.42 }), + want('w1'), + want('w2', { + title: 'Parklife', + search: { searched_at: ago(3_600_000), outcome: 'nothing', considered: 4, best_release: 'x', best_verdict: 'rejected', error: '' } }), + want('w3', { title: 'Blur', grab: grab() }), + want('w4', { title: '13', grab: grab({ import_state: 'downloading', progress: 0.42 }) }) ]); - const lines = screen - .getAllByTestId("want-search") - .map((e) => e.textContent ?? ""); + const lines = screen.getAllByTestId('want-search').map((e) => e.textContent ?? ''); expect(lines[0]).toMatch(/not searched yet/i); expect(lines[1]).toMatch(/4 found, none of them this album/i); - expect(lines[2]).toMatch( - /downloading blur - blur \(1997\) \[flac\], taken by the schedule/i, - ); + expect(lines[2]).toMatch(/downloading blur - blur \(1997\) \[flac\], taken by the schedule/i); expect(lines[3]).toMatch(/downloading 42%/i); }); - test("a download waiting on a person says why and points to Activity", () => { + test('a download waiting on a person says why and points to Activity', () => { setup([ - want("w1", { - grab: grab({ - import_state: "review", - import_reason: "Only the lengths agree. A person decides.", - }), - }), + want('w1', { grab: grab({ import_state: 'review', import_reason: 'Only the lengths agree. A person decides.' }) }) ]); - const line = screen.getByTestId("want-search"); + const line = screen.getByTestId('want-search'); expect(line.textContent).toMatch(/needs you: only the lengths agree/i); - expect( - screen - .getByRole("link", { name: /decide on activity/i }) - .getAttribute("href"), - ).toBe("/wanted/activity"); + expect(screen.getByRole('link', { name: /decide on activity/i }).getAttribute('href')).toBe('/wanted/activity'); // A live download can't be searched for again. - expect( - ( - screen.getByRole("button", { - name: /search for the ballad of darren now/i, - }) as HTMLButtonElement - ).disabled, - ).toBe(true); + expect((screen.getByRole('button', { name: /search for the ballad of darren now/i }) as HTMLButtonElement).disabled).toBe(true); }); - test("a refused download is named beside the search that follows it", () => { + test('a refused download is named beside the search that follows it', () => { setup([ - want("w1", { - search: { - searched_at: ago(3_600_000), - outcome: "nothing", - considered: 0, - best_release: "", - best_verdict: "", - error: "", - }, - grab: grab({ - live: false, - end_reason: "refused", - import_state: "refused", - import_reason: "Another album.", - }), - }), + want('w1', { + search: { searched_at: ago(3_600_000), outcome: 'nothing', considered: 0, best_release: '', best_verdict: '', error: '' }, + grab: grab({ live: false, end_reason: 'refused', import_state: 'refused', import_reason: 'Another album.' }) + }) ]); - expect(screen.getByTestId("want-search").textContent).toMatch( - /last download refused: another album/i, - ); + expect(screen.getByTestId('want-search').textContent).toMatch(/last download refused: another album/i); }); - test("a download given up on is named beside the search that follows it", () => { + test('a download given up on is named beside the search that follows it', () => { setup([ - want("w1", { - search: { - searched_at: ago(3_600_000), - outcome: "waiting", - considered: 4, - best_release: "", - best_verdict: "", - error: "", - }, - grab: grab({ - live: false, - end_reason: "stalled", - import_state: "downloading", - end_detail: "Nobody was sharing it.", - }), - }), + want('w1', { + search: { searched_at: ago(3_600_000), outcome: 'waiting', considered: 4, best_release: '', best_verdict: '', error: '' }, + grab: grab({ live: false, end_reason: 'stalled', import_state: 'downloading', end_detail: 'Nobody was sharing it.' }) + }) ]); - expect(screen.getByTestId("want-search").textContent).toMatch( - /last download given up: nobody was sharing it/i, - ); + expect(screen.getByTestId('want-search').textContent).toMatch(/last download given up: nobody was sharing it/i); }); - test("a held album says how it arrived", () => { + test('a held album says how it arrived', () => { setup( [ - want("w1", { + want('w1', { held_at: ago(DAY), - album_id: "al-1", - grab: grab({ - live: false, - end_reason: "imported", - import_state: "imported", - imported_at: ago(DAY), - imported_files: 11, - }), - }), + album_id: 'al-1', + grab: grab({ live: false, end_reason: 'imported', import_state: 'imported', imported_at: ago(DAY), imported_files: 11 }) + }) ], - "held", - ); - expect(screen.getByTestId("want-arrived").textContent).toMatch( - /imported 11 tracks .* from blur - blur/i, + 'held' ); + expect(screen.getByTestId('want-arrived').textContent).toMatch(/imported 11 tracks .* from blur - blur/i); }); - test("Search runs one now and says what came of it", async () => { + test('Search runs one now and says what came of it', async () => { setup(); - await fireEvent.click( - screen.getByRole("button", { name: /search for parklife now/i }), - ); - await waitFor(() => expect(searchWant).toHaveBeenCalledWith("w2")); - await waitFor(() => - expect(pushToast).toHaveBeenCalledWith( - "Nothing found for Parklife.", - "info", - ), - ); + await fireEvent.click(screen.getByRole('button', { name: /search for parklife now/i })); + await waitFor(() => expect(searchWant).toHaveBeenCalledWith('w2')); + await waitFor(() => expect(pushToast).toHaveBeenCalledWith('Nothing found for Parklife.', 'info')); }); - test("Releases opens what the last search found", async () => { + test('Releases opens what the last search found', async () => { setup(); - expect(screen.queryByTestId("want-candidates")).toBeNull(); - const toggle = screen.getByRole("button", { - name: /releases found for parklife/i, - }); + expect(screen.queryByTestId('want-candidates')).toBeNull(); + const toggle = screen.getByRole('button', { name: /releases found for parklife/i }); await fireEvent.click(toggle); - expect(toggle.getAttribute("aria-expanded")).toBe("true"); - expect(screen.getByTestId("want-candidates")).toBeTruthy(); - expect(createCandidatesQuery).toHaveBeenCalledWith("w2"); + expect(toggle.getAttribute('aria-expanded')).toBe('true'); + expect(screen.getByTestId('want-candidates')).toBeTruthy(); + expect(createCandidatesQuery).toHaveBeenCalledWith('w2'); }); }); diff --git a/web/src/routes/wanted/wanted.test.ts b/web/src/routes/wanted/wanted.test.ts index 547b9c1a..4b1bb38d 100644 --- a/web/src/routes/wanted/wanted.test.ts +++ b/web/src/routes/wanted/wanted.test.ts @@ -1,35 +1,23 @@ -import { afterEach, describe, expect, test, vi } from "vitest"; -import { render, screen } from "@testing-library/svelte"; -import { createRawSnippet } from "svelte"; -import { mockInfiniteQuery } from "../../test-utils/query"; -import { pageUrlModule } from "#test-utils/mocks/appState.js"; -import type { WantCounts, WantsPage } from "#lib/api/types.js"; +import { afterEach, describe, expect, test, vi } from 'vitest'; +import { render, screen } from '@testing-library/svelte'; +import { createRawSnippet } from 'svelte'; +import { mockInfiniteQuery } from '../../test-utils/query'; +import { pageUrlModule } from '#test-utils/mocks/appState.js'; +import type { WantCounts, WantsPage } from '#lib/api/types.js'; -const state = vi.hoisted(() => ({ - pageUrl: new URL("http://localhost/wanted"), -})); -vi.mock("$app/state", () => pageUrlModule(state)); -vi.mock("#lib/api/wanted.js", () => ({ createWantsQuery: vi.fn() })); +const state = vi.hoisted(() => ({ pageUrl: new URL('http://localhost/wanted') })); +vi.mock('$app/state', () => pageUrlModule(state)); +vi.mock('#lib/api/wanted.js', () => ({ createWantsQuery: vi.fn() })); -import WantedLayout from "./+layout.svelte"; -import { createWantsQuery } from "#lib/api/wanted.js"; +import WantedLayout from './+layout.svelte'; +import { createWantsQuery } from '#lib/api/wanted.js'; -const counts: WantCounts = { - wanted: 5, - missing: 1, - held: 9, - following: 2, - activity: 3, - attention: 0, - blocked: 4, -}; -const children = createRawSnippet(() => ({ - render: () => `

body

`, -})); +const counts: WantCounts = { wanted: 5, missing: 1, held: 9, following: 2, activity: 3, attention: 0, blocked: 4 }; +const children = createRawSnippet(() => ({ render: () => `

body

` })); afterEach(() => vi.clearAllMocks()); -function setup(c: WantCounts = counts, path = "/wanted") { +function setup(c: WantCounts = counts, path = '/wanted') { state.pageUrl = new URL(`http://localhost${path}`); const page: WantsPage = { items: [], @@ -37,58 +25,44 @@ function setup(c: WantCounts = counts, path = "/wanted") { limit: 50, offset: 0, counts: c, - library: { listed_at: null, unlisted: 0 }, + library: { listed_at: null, unlisted: 0, unmatched: [] } }; vi.mocked(createWantsQuery).mockReturnValue( - mockInfiniteQuery({ pages: [page] }) as unknown as ReturnType< - typeof createWantsQuery - >, + mockInfiniteQuery({ pages: [page] }) as unknown as ReturnType ); // eslint-disable-next-line @typescript-eslint/no-explicit-any return render(WantedLayout, { children } as any); } -describe("/wanted layout", () => { - test("tabs are links carrying their counts, the current one marked", () => { - setup(counts, "/wanted/activity"); - const nav = screen.getByRole("navigation", { name: /wanted sections/i }); - const links = Array.from(nav.querySelectorAll("a")).map((a) => [ - a.getAttribute("href"), - a.textContent?.replace(/\s+/g, " ").trim(), - ]); +describe('/wanted layout', () => { + test('tabs are links carrying their counts, the current one marked', () => { + setup(counts, '/wanted/activity'); + const nav = screen.getByRole('navigation', { name: /wanted sections/i }); + const links = Array.from(nav.querySelectorAll('a')).map((a) => [a.getAttribute('href'), a.textContent?.replace(/\s+/g, ' ').trim()]); expect(links).toEqual([ - ["/wanted", "Wanted 5"], - ["/wanted/activity", "Activity 3"], - ["/wanted/missing", "Missing files 1"], - ["/wanted/held", "Held 9"], - ["/wanted/following", "Following 2"], - ["/wanted/blocked", "Blocked 4"], + ['/wanted', 'Wanted 5'], + ['/wanted/activity', 'Activity 3'], + ['/wanted/missing', 'Missing files 1'], + ['/wanted/held', 'Held 9'], + ['/wanted/following', 'Following 2'], + ['/wanted/blocked', 'Blocked 4'] ]); - expect(screen.getByRole("link", { name: /activity/i })).toHaveAttribute( - "aria-current", - "page", - ); - expect(screen.getByRole("link", { name: /^wanted/i })).not.toHaveAttribute( - "aria-current", - ); - expect(screen.getByTestId("tab-body")).toBeTruthy(); + expect(screen.getByRole('link', { name: /activity/i })).toHaveAttribute('aria-current', 'page'); + expect(screen.getByRole('link', { name: /^wanted/i })).not.toHaveAttribute('aria-current'); + expect(screen.getByTestId('tab-body')).toBeTruthy(); }); - test("when nothing needs a person the verdict says so", () => { + test('when nothing needs a person the verdict says so', () => { setup(); - expect(screen.getByTestId("wanted-verdict").textContent).toMatch( - /5 albums wanted, 3 downloading\. Nothing needs you\./, - ); - expect(screen.queryByTestId("tab-attention")).toBeNull(); + expect(screen.getByTestId('wanted-verdict').textContent).toMatch(/5 albums wanted, 3 downloading\. Nothing needs you\./); + expect(screen.queryByTestId('tab-attention')).toBeNull(); }); - test("downloads waiting on a person lead the page and mark Activity", () => { + test('downloads waiting on a person lead the page and mark Activity', () => { setup({ ...counts, attention: 2 }); - const verdict = screen.getByTestId("wanted-verdict"); + const verdict = screen.getByTestId('wanted-verdict'); expect(verdict.textContent).toMatch(/2 downloads need you/); - expect( - screen.getByRole("link", { name: /see activity/i }).getAttribute("href"), - ).toBe("/wanted/activity"); - expect(screen.getByTestId("tab-attention").textContent).toBe("2"); + expect(screen.getByRole('link', { name: /see activity/i }).getAttribute('href')).toBe('/wanted/activity'); + expect(screen.getByTestId('tab-attention').textContent).toBe('2'); }); });