diff --git a/android/app/src/main/java/com/fabledsword/minstrel/api/endpoints/DiscoverApi.kt b/android/app/src/main/java/com/fabledsword/minstrel/api/endpoints/DiscoverApi.kt index 2b8e1945..9d46b41e 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/api/endpoints/DiscoverApi.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/api/endpoints/DiscoverApi.kt @@ -13,10 +13,11 @@ import retrofit2.http.Path import retrofit2.http.Query /** - * Retrofit interface for Discover / Lidarr search / request creation. + * Retrofit interface for Discover / catalogue search / request creation. * - * `/api/lidarr/search` has a 60s LRU on the server so quick re-types - * of the same query are cheap. + * `/api/catalogue/search` asks MusicBrainz, which allows one request a + * second, so the Discover screen searches when the query is submitted, + * not on every keystroke. * * Cancel + my-requests-list endpoints live on RequestsApi (Phase 10 * Commit B), not here. @@ -25,7 +26,7 @@ interface DiscoverApi { @GET("api/discover/suggestions") suspend fun listSuggestions(): List - @GET("api/lidarr/search") + @GET("api/catalogue/search") suspend fun search( @Query("q") query: String, @Query("kind") kind: String, diff --git a/android/app/src/main/java/com/fabledsword/minstrel/discover/ui/DiscoverTiles.kt b/android/app/src/main/java/com/fabledsword/minstrel/discover/ui/DiscoverTiles.kt index cbad59cb..46b74506 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/discover/ui/DiscoverTiles.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/discover/ui/DiscoverTiles.kt @@ -25,7 +25,6 @@ import androidx.compose.ui.draw.clip import androidx.compose.ui.graphics.vector.ImageVector import androidx.compose.ui.text.style.TextOverflow import androidx.compose.ui.unit.dp -import coil3.compose.AsyncImage import com.composables.icons.lucide.Clock import com.composables.icons.lucide.Disc3 import com.composables.icons.lucide.Lucide @@ -33,6 +32,7 @@ import com.composables.icons.lucide.User import com.fabledsword.minstrel.models.ArtistSuggestionRef import com.fabledsword.minstrel.models.LidarrSearchResultRef import com.fabledsword.minstrel.models.SuggestionSnoozeRef +import com.fabledsword.minstrel.shared.widgets.ServerImage @Composable internal fun SuggestionTile( @@ -173,18 +173,19 @@ private fun Avatar(imageUrl: String, fallback: ImageVector, circular: Boolean) { .background(MaterialTheme.colorScheme.surfaceVariant), contentAlignment = Alignment.Center, ) { - if (imageUrl.isEmpty()) { + // The server hands back its own art endpoint (relative, M502 #5519), + // which answers 404 when no source has an image: ServerImage resolves + // the path and shows the icon while loading and on a miss. + ServerImage( + url = imageUrl, + contentDescription = null, + modifier = Modifier.fillMaxSize(), + ) { Icon( imageVector = fallback, contentDescription = null, tint = MaterialTheme.colorScheme.onSurfaceVariant, ) - } else { - AsyncImage( - model = imageUrl, - contentDescription = null, - modifier = Modifier.fillMaxSize(), - ) } } } diff --git a/android/app/src/main/java/com/fabledsword/minstrel/models/wire/DiscoverWire.kt b/android/app/src/main/java/com/fabledsword/minstrel/models/wire/DiscoverWire.kt index d7db19fa..b37043e9 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/models/wire/DiscoverWire.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/models/wire/DiscoverWire.kt @@ -4,9 +4,10 @@ import kotlinx.serialization.SerialName import kotlinx.serialization.Serializable /** - * One row of `GET /api/lidarr/search`. Mirrors - * `web/src/lib/api/types.ts LidarrSearchResult` / - * the Flutter client's `LidarrSearchResult`. + * One row of `GET /api/catalogue/search` (the MusicBrainz search that replaced + * Lidarr's, M502 #5519). Mirrors `web/src/lib/api/types.ts + * CatalogueSearchResult`. `imageUrl` is the server's own art endpoint, + * relative, so it goes through ServerImage. * * `inLibrary` and `requested` let the UI greyout rows the user can't * act on (already imported / already awaiting review). All defaults diff --git a/cmd/minstrel/main.go b/cmd/minstrel/main.go index 5036d8d1..9a218e51 100644 --- a/cmd/minstrel/main.go +++ b/cmd/minstrel/main.go @@ -15,6 +15,7 @@ import ( "github.com/jackc/pgx/v5/pgxpool" "git.fabledsword.com/bvandeusen/minstrel/internal/acoustid" + "git.fabledsword.com/bvandeusen/minstrel/internal/catalogue" "git.fabledsword.com/bvandeusen/minstrel/internal/config" "git.fabledsword.com/bvandeusen/minstrel/internal/coplay" "git.fabledsword.com/bvandeusen/minstrel/internal/coverart" @@ -325,6 +326,11 @@ func run() error { library.SetNotifier(notifier) go lidarrReconciler.Run(ctx) + // Release catalogue (M502 #5519): re-reads discographies older than a + // week from MusicBrainz, a batch an hour, through the process-wide + // MusicBrainz pacer it shares with tag enrichment. + go catalogue.New(pool, nil, logger.With("component", "catalogue")).Run(ctx) + // Duplicate resolver (M498, M502): classifies the sweep's groups and, with // the operator's auto-resolve on, merges the copies that are safe to merge. // Started after SetNotifier so its summaries reach admins. See diff --git a/internal/api/api.go b/internal/api/api.go index ec2419bf..94518874 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -15,6 +15,7 @@ import ( "git.fabledsword.com/bvandeusen/minstrel/internal/acquire" "git.fabledsword.com/bvandeusen/minstrel/internal/auth" + "git.fabledsword.com/bvandeusen/minstrel/internal/catalogue" "git.fabledsword.com/bvandeusen/minstrel/internal/config" "git.fabledsword.com/bvandeusen/minstrel/internal/coverart" "git.fabledsword.com/bvandeusen/minstrel/internal/eventbus" @@ -72,6 +73,9 @@ func Mount(r chi.Router, pool *pgxpool.Pool, logger *slog.Logger, events *playev loudnessSettings: loudSettings, acoustIDLookup: acoustIDLookup, acquisition: acquire.NewStore(pool), + catalogue: catalogue.New(pool, nil, logger), + catalogueSearches: newSearchCache(), + catalogueArt: catalogue.NewArt(dataDir, artSources(coverSettings)), leveled: newLeveledRenderer(dataDir, loudSettings, logger), librarySize: recommendation.NewLibrarySize(nil), loginGuard: auth.NewLoginGuard(), @@ -184,7 +188,11 @@ func Mount(r chi.Router, pool *pgxpool.Pool, logger *slog.Logger, events *playev authed.Get("/likes/artists", h.handleListLikedArtists) authed.Get("/likes/ids", h.handleGetLikedIDs) - authed.Get("/lidarr/search", h.handleLidarrSearch) + // The release catalogue (M502 #5519): request search asks + // MusicBrainz, and art for what isn't held comes through + // Minstrel from the enabled cover-art sources. + authed.Get("/catalogue/search", h.handleCatalogueSearch) + authed.Get("/catalogue/art/{kind}/{mbid}", h.handleCatalogueArt) authed.Post("/requests", h.handleCreateRequest) authed.Get("/requests", h.handleListRequests) @@ -385,6 +393,13 @@ type handlers struct { acoustIDLookup *library.AcoustIDLookupWorker // acquisition is the operator's indexers and download clients (M502). acquisition *acquire.Store + // catalogue is the release catalogue read from MusicBrainz, and + // catalogueArt the images for what isn't held (M502 #5519). + catalogue *catalogue.Service + catalogueArt *catalogue.Art + // catalogueSearches reuses recent MusicBrainz answers; nil asks every + // time, which is what handler tests get. + catalogueSearches *searchCache // leveled renders the leveled streams handed to Sonos/UPnP speakers // (M464 #5001). Nil when its cache directory cannot be made: a level // request then gets the plain stream. @@ -427,3 +442,13 @@ func newLeveledRenderer(dataDir string, settings *library.LoudnessSettingsServic } return r } + +// artSources hands the art cache the cover-art settings, or nothing when +// there are none: a nil *SettingsService inside the interface would not +// compare equal to nil and would be called. +func artSources(s *coverart.SettingsService) catalogue.ArtSources { + if s == nil { + return nil + } + return s +} diff --git a/internal/api/catalogue.go b/internal/api/catalogue.go new file mode 100644 index 00000000..2a712613 --- /dev/null +++ b/internal/api/catalogue.go @@ -0,0 +1,284 @@ +package api + +import ( + "context" + "errors" + "net/http" + "net/url" + "slices" + "strings" + "sync" + "time" + + "github.com/go-chi/chi/v5" + + "git.fabledsword.com/bvandeusen/minstrel/internal/apierror" + "git.fabledsword.com/bvandeusen/minstrel/internal/catalogue" + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" + "git.fabledsword.com/bvandeusen/minstrel/internal/musicbrainz" +) + +// catalogueSearchLimit is how many results one search shows. +const catalogueSearchLimit = 25 + +// catalogueSearchResult is one row of GET /api/catalogue/search. The shape is +// the one the Lidarr search answered with, so the Discover page and request +// rows read it unchanged. artist_mbid and album_mbid are the parents a +// request needs: empty on an artist, the artist on an album, both on a track. +// An album is a MusicBrainz release group, as Lidarr's albums were. +type catalogueSearchResult struct { + MBID string `json:"mbid"` + Name string `json:"name"` + SecondaryText string `json:"secondary_text"` + ImageURL string `json:"image_url"` + ArtistMBID string `json:"artist_mbid"` + AlbumMBID string `json:"album_mbid"` + InLibrary bool `json:"in_library"` + Requested bool `json:"requested"` +} + +// handleCatalogueSearch implements GET /api/catalogue/search?q=&kind=artist|album|track +// (M502 #5519), asking MusicBrainz directly. An unreachable MusicBrainz is a +// 503 the page shows as such (rule 164). +func (h *handlers) handleCatalogueSearch(w http.ResponseWriter, r *http.Request) { + kind := strings.TrimSpace(r.URL.Query().Get("kind")) + switch kind { + case "artist", "album", "track": + default: + writeErr(w, apierror.BadRequest("bad_kind", "kind must be artist, album, or track")) + return + } + q := strings.TrimSpace(r.URL.Query().Get("q")) + if q == "" { + writeErr(w, apierror.BadRequest("missing_query", "q is required")) + return + } + + out, err := h.catalogueSearches.get(r.Context(), kind, q, h.searchCatalogue) + if err != nil { + switch { + case errors.Is(err, musicbrainz.ErrUnreachable), errors.Is(err, musicbrainz.ErrBadResponse): + writeErr(w, &apierror.Error{Status: 503, Code: "musicbrainz_unreachable", Message: "MusicBrainz can't be reached", Cause: err}) + case r.Context().Err() != nil: + return + default: + h.logger.Error("api: catalogue search", "kind", kind, "err", err) + writeErr(w, apierror.InternalMsg("search failed", err)) + } + return + } + + dbQ := dbq.New(h.pool) + for i := range out { + inLib, err := h.catalogueInLibrary(r.Context(), kind, out[i].MBID) + if err != nil { + writeErr(w, apierror.InternalMsg("library check failed", err)) + return + } + requested, err := dbQ.HasNonTerminalRequestForMBID(r.Context(), out[i].MBID) + if err != nil { + writeErr(w, apierror.InternalMsg("request check failed", err)) + return + } + out[i].InLibrary, out[i].Requested = inLib, requested + } + writeJSON(w, http.StatusOK, out) +} + +func (h *handlers) searchCatalogue(ctx context.Context, kind, q string) ([]catalogueSearchResult, error) { + mb := h.catalogue.MusicBrainz() + out := []catalogueSearchResult{} + switch kind { + case "artist": + artists, err := mb.SearchArtists(ctx, q, catalogueSearchLimit) + if err != nil { + return nil, err + } + for _, a := range artists { + out = append(out, catalogueSearchResult{ + MBID: a.MBID, Name: a.Name, SecondaryText: artistLine(a), + ImageURL: artistArtURL(a.MBID, a.Name), + }) + } + case "album": + groups, err := mb.SearchReleaseGroups(ctx, q, catalogueSearchLimit) + if err != nil { + return nil, err + } + for _, g := range groups { + out = append(out, catalogueSearchResult{ + MBID: g.MBID, Name: g.Title, SecondaryText: joinNonEmpty(g.ArtistCredit, g.Year(), groupType(g)), + ImageURL: groupArtURL(g), ArtistMBID: first(g.ArtistMBIDs), + }) + } + case "track": + recs, err := mb.SearchRecordings(ctx, q, catalogueSearchLimit) + if err != nil { + return nil, err + } + for _, rec := range recs { + res := catalogueSearchResult{ + MBID: rec.MBID, Name: rec.Title, + SecondaryText: joinNonEmpty(rec.ReleaseGroup.Title, rec.ArtistCredit), + ArtistMBID: first(rec.ArtistMBIDs), AlbumMBID: rec.ReleaseGroup.MBID, + } + if rec.ReleaseGroup.MBID != "" { + res.ImageURL = groupArtURL(rec.ReleaseGroup) + } + out = append(out, res) + } + } + return out, nil +} + +// catalogueSearchTTL is how long a search's MusicBrainz answer is reused. +// MusicBrainz allows a request a second, and Discover searches as the user +// types, so the same words are often asked again within moments. +const catalogueSearchTTL = 10 * time.Minute + +// catalogueSearchCacheMax bounds the cache; past it, the cache starts over. +const catalogueSearchCacheMax = 500 + +// searchCache keeps recent MusicBrainz answers by kind and query. A nil +// cache asks every time. Whether a result is held or requested is not +// cached: the handler works that out on every request. +type searchCache struct { + mu sync.Mutex + entries map[string]searchEntry + now func() time.Time +} + +type searchEntry struct { + at time.Time + results []catalogueSearchResult +} + +func newSearchCache() *searchCache { + return &searchCache{entries: map[string]searchEntry{}, now: time.Now} +} + +func (c *searchCache) get(ctx context.Context, kind, q string, + ask func(context.Context, string, string) ([]catalogueSearchResult, error), +) ([]catalogueSearchResult, error) { + if c == nil { + return ask(ctx, kind, q) + } + key := kind + "\x00" + strings.ToLower(q) + c.mu.Lock() + e, ok := c.entries[key] + c.mu.Unlock() + if ok && c.now().Sub(e.at) < catalogueSearchTTL { + return slices.Clone(e.results), nil + } + results, err := ask(ctx, kind, q) + if err != nil { + return nil, err + } + c.mu.Lock() + if len(c.entries) >= catalogueSearchCacheMax { + c.entries = map[string]searchEntry{} + } + c.entries[key] = searchEntry{at: c.now(), results: slices.Clone(results)} + c.mu.Unlock() + return results, nil +} + +// artistLine is what tells two artists of one name apart: MusicBrainz's own +// disambiguation when it has one, else the kind of artist and the country. +func artistLine(a musicbrainz.Artist) string { + if a.Disambiguation != "" { + return a.Disambiguation + } + return joinNonEmpty(a.Type, a.Country) +} + +// groupType names an album's kind the way people do: "Album", "EP", +// "Album · Live", "Single · Remix". +func groupType(g musicbrainz.ReleaseGroup) string { + return joinNonEmpty(append([]string{g.PrimaryType}, g.SecondaryTypes...)...) +} + +func joinNonEmpty(parts ...string) string { + kept := parts[:0:0] + for _, p := range parts { + if p != "" { + kept = append(kept, p) + } + } + return strings.Join(kept, " · ") +} + +func first(s []string) string { + if len(s) == 0 { + return "" + } + return s[0] +} + +// artistArtURL and groupArtURL point at Minstrel's own art endpoints. The +// names ride along for the sources that look art up by name. +func artistArtURL(mbid, name string) string { + if mbid == "" { + return "" + } + return "/api/catalogue/art/artist/" + mbid + "?" + url.Values{"name": {name}}.Encode() +} + +func groupArtURL(g musicbrainz.ReleaseGroup) string { + return "/api/catalogue/art/release-group/" + g.MBID + "?" + + url.Values{"artist": {g.ArtistCredit}, "title": {g.Title}}.Encode() +} + +// catalogueInLibrary reports whether a result is already held: an artist by +// its MBID, an album by its release group (or a release id that happens to be +// the same), a track by its recording. +func (h *handlers) catalogueInLibrary(ctx context.Context, kind, mbid string) (bool, error) { + var query string + switch kind { + case "artist": + query = `SELECT EXISTS(SELECT 1 FROM artists WHERE mbid = $1)` + case "album": + query = `SELECT EXISTS(SELECT 1 FROM albums WHERE release_group_mbid = $1 OR mbid = $1)` + case "track": + query = `SELECT EXISTS(SELECT 1 FROM tracks WHERE mbid = $1)` + default: + return false, nil + } + var exists bool + err := h.pool.QueryRow(ctx, query, mbid).Scan(&exists) + return exists, err +} + +// handleCatalogueArt implements GET /api/catalogue/art/{kind}/{mbid} for an +// artist or a release group Minstrel doesn't hold. The image comes from the +// operator's enabled cover-art sources and is kept on disk, so the browser +// never fetches a third-party host. A 404 shows the placeholder. +func (h *handlers) handleCatalogueArt(w http.ResponseWriter, r *http.Request) { + kind, mbid := chi.URLParam(r, "kind"), chi.URLParam(r, "mbid") + if !catalogue.ValidMBID(mbid) { + writeErr(w, apierror.NotFound("art")) + return + } + var ( + body []byte + err error + ) + switch kind { + case "artist": + body, err = h.catalogueArt.Artist(r.Context(), mbid, r.URL.Query().Get("name")) + case "release-group": + body, err = h.catalogueArt.ReleaseGroup(r.Context(), mbid, r.URL.Query().Get("artist"), r.URL.Query().Get("title")) + default: + writeErr(w, apierror.NotFound("art")) + return + } + if err != nil { + if r.Context().Err() == nil { + writeErr(w, apierror.NotFound("art")) + } + return + } + w.Header().Set("Content-Type", http.DetectContentType(body)) + w.Header().Set("Cache-Control", "private, max-age=86400") + _, _ = w.Write(body) +} diff --git a/internal/api/catalogue_test.go b/internal/api/catalogue_test.go new file mode 100644 index 00000000..de879d2e --- /dev/null +++ b/internal/api/catalogue_test.go @@ -0,0 +1,253 @@ +package api + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "net/url" + "testing" + "time" + + "github.com/go-chi/chi/v5" + + "git.fabledsword.com/bvandeusen/minstrel/internal/catalogue" + "git.fabledsword.com/bvandeusen/minstrel/internal/coverart" + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" + "git.fabledsword.com/bvandeusen/minstrel/internal/dbtest" + "git.fabledsword.com/bvandeusen/minstrel/internal/musicbrainz" +) + +const ( + searchArtistMBID = "a74b1b7f-71a5-4011-9441-d0b5e4122711" + searchGroupMBID = "b1392450-e666-3926-a536-22c65f834433" + searchRecMBID = "c1a0e1b2-0000-4000-8000-000000000001" +) + +// fakeMusicBrainz answers the three searches with one Radiohead result each, +// or 503 when down is set. +func fakeMusicBrainz(t *testing.T, down bool) *httptest.Server { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if down { + w.WriteHeader(http.StatusServiceUnavailable) + return + } + credit := `[{"name": "Radiohead", "artist": {"id": "` + searchArtistMBID + `"}}]` + group := `{"id": "` + searchGroupMBID + `", "title": "OK Computer", "primary-type": "Album", + "first-release-date": "1997-05-21", "artist-credit": ` + credit + `}` + switch r.URL.Path { + case "/artist": + _, _ = w.Write([]byte(`{"artists": [{"id": "` + searchArtistMBID + `", "name": "Radiohead", "type": "Group", "country": "GB"}]}`)) + case "/release-group": + _, _ = w.Write([]byte(`{"release-groups": [` + group + `]}`)) + case "/recording": + _, _ = w.Write([]byte(`{"recordings": [{"id": "` + searchRecMBID + `", "title": "Karma Police", + "artist-credit": ` + credit + `, + "releases": [{"id": "r1", "status": "Official", "release-group": ` + group + `}]}]}`)) + default: + w.WriteHeader(http.StatusNotFound) + } + })) + t.Cleanup(srv.Close) + return srv +} + +func catalogueHandlers(t *testing.T, down bool) *handlers { + t.Helper() + h, pool := testHandlers(t) + resetLidarrState(t, h) + mb := &musicbrainz.Client{BaseURL: fakeMusicBrainz(t, down).URL, Pace: &musicbrainz.Pacer{}, Retries: -1} + h.catalogue = catalogue.New(pool, mb, h.logger) + return h +} + +func doCatalogueSearch(t *testing.T, h *handlers, q, kind string) *httptest.ResponseRecorder { + t.Helper() + req := httptest.NewRequest(http.MethodGet, "/api/catalogue/search?"+url.Values{"q": {q}, "kind": {kind}}.Encode(), nil) + w := httptest.NewRecorder() + h.handleCatalogueSearch(w, req) + return w +} + +func decodeResults(t *testing.T, w *httptest.ResponseRecorder) []catalogueSearchResult { + t.Helper() + if w.Code != http.StatusOK { + t.Fatalf("status = %d, body = %s", w.Code, w.Body.String()) + } + var out []catalogueSearchResult + if err := json.Unmarshal(w.Body.Bytes(), &out); err != nil { + t.Fatalf("decode: %v; body = %s", err, w.Body.String()) + } + return out +} + +func errorCode(t *testing.T, w *httptest.ResponseRecorder) string { + t.Helper() + var resp struct { + Error struct { + Code string `json:"code"` + } `json:"error"` + } + if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil { + t.Fatalf("decode: %v; body = %s", err, w.Body.String()) + } + return resp.Error.Code +} + +func TestCatalogueSearch_RefusesABadKindOrNoQuery(t *testing.T) { + h := catalogueHandlers(t, false) + if w := doCatalogueSearch(t, h, "x", "playlist"); w.Code != http.StatusBadRequest || errorCode(t, w) != "bad_kind" { + t.Errorf("bad kind: %d %s", w.Code, w.Body.String()) + } + if w := doCatalogueSearch(t, h, " ", "artist"); w.Code != http.StatusBadRequest || errorCode(t, w) != "missing_query" { + t.Errorf("no query: %d %s", w.Code, w.Body.String()) + } +} + +func TestCatalogueSearch_EachKindAsksMusicBrainz(t *testing.T) { + h := catalogueHandlers(t, false) + + artists := decodeResults(t, doCatalogueSearch(t, h, "radiohead", "artist")) + if len(artists) != 1 || artists[0].MBID != searchArtistMBID || artists[0].SecondaryText != "Group · GB" { + t.Fatalf("artists = %+v", artists) + } + if artists[0].ImageURL != "/api/catalogue/art/artist/"+searchArtistMBID+"?name=Radiohead" { + t.Errorf("artist art = %q: it must be Minstrel's own endpoint", artists[0].ImageURL) + } + + albums := decodeResults(t, doCatalogueSearch(t, h, "ok computer", "album")) + if len(albums) != 1 || albums[0].MBID != searchGroupMBID || albums[0].ArtistMBID != searchArtistMBID || + albums[0].SecondaryText != "Radiohead · 1997 · Album" { + t.Fatalf("albums = %+v", albums) + } + + tracks := decodeResults(t, doCatalogueSearch(t, h, "karma police", "track")) + if len(tracks) != 1 || tracks[0].AlbumMBID != searchGroupMBID || tracks[0].ArtistMBID != searchArtistMBID || + tracks[0].SecondaryText != "OK Computer · Radiohead" { + t.Fatalf("tracks = %+v", tracks) + } +} + +func TestCatalogueSearch_MarksWhatIsHeldAndWhatIsRequested(t *testing.T) { + h := catalogueHandlers(t, false) + ctx := context.Background() + q := dbq.New(h.pool) + + artistMBID := searchArtistMBID + artist, err := q.UpsertArtist(ctx, dbq.UpsertArtistParams{Name: dbtest.TestUserPrefix + "Radiohead", SortName: "Radiohead", Mbid: &artistMBID}) + if err != nil { + t.Fatal(err) + } + // The album is held under a release id; its release group is what matches. + if _, err := h.pool.Exec(ctx, + `INSERT INTO albums (title, sort_title, artist_id, mbid, release_group_mbid) VALUES ('OK Computer', 'OK Computer', $1, 'some-release-id', $2)`, + artist.ID, searchGroupMBID); err != nil { + t.Fatal(err) + } + if got := decodeResults(t, doCatalogueSearch(t, h, "radiohead", "artist")); !got[0].InLibrary { + t.Error("the artist is held") + } + if got := decodeResults(t, doCatalogueSearch(t, h, "ok computer", "album")); !got[0].InLibrary { + t.Error("the album is held, matched by release group") + } + + user := seedUser(t, h.pool, "requester", "pw", false) + track := searchRecMBID + if _, err := q.CreateLidarrRequest(ctx, dbq.CreateLidarrRequestParams{ + UserID: user.ID, Kind: dbq.LidarrRequestKindTrack, LidarrArtistMbid: searchArtistMBID, + LidarrTrackMbid: &track, ArtistName: "Radiohead", + }); err != nil { + t.Fatal(err) + } + got := decodeResults(t, doCatalogueSearch(t, h, "karma police", "track")) + if got[0].InLibrary || !got[0].Requested { + t.Errorf("track = %+v, want requested and not held", got[0]) + } +} + +func TestCatalogueSearch_UnreachableMusicBrainzIs503(t *testing.T) { + h := catalogueHandlers(t, true) + w := doCatalogueSearch(t, h, "radiohead", "artist") + if w.Code != http.StatusServiceUnavailable || errorCode(t, w) != "musicbrainz_unreachable" { + t.Fatalf("status %d, body %s", w.Code, w.Body.String()) + } +} + +// artSource is an artist-art source with one image. +type artSource struct{ apiTestAlbumProvider } + +func (artSource) FetchArtistArt(_ context.Context, _ coverart.ArtistRef) ([]byte, []byte, error) { + return []byte("\x89PNG\r\n\x1a\nrest"), nil, nil +} + +type fakeArtSources struct{ artists []coverart.ArtistArtProvider } + +func (s fakeArtSources) EnabledArtistProviders() []coverart.ArtistArtProvider { return s.artists } +func (fakeArtSources) EnabledAlbumProviders() []coverart.AlbumCoverProvider { return nil } + +func TestCatalogueArt_ServesTheImageOrAPlaceholder404(t *testing.T) { + h := &handlers{catalogueArt: catalogue.NewArt(t.TempDir(), fakeArtSources{artists: []coverart.ArtistArtProvider{&artSource{}}})} + r := chi.NewRouter() + r.Get("/api/catalogue/art/{kind}/{mbid}", h.handleCatalogueArt) + + get := func(path string) *httptest.ResponseRecorder { + w := httptest.NewRecorder() + r.ServeHTTP(w, httptest.NewRequest(http.MethodGet, path, nil)) + return w + } + if w := get("/api/catalogue/art/artist/" + searchArtistMBID + "?name=Radiohead"); w.Code != http.StatusOK || w.Header().Get("Content-Type") != "image/png" { + t.Errorf("artist: %d %q", w.Code, w.Header().Get("Content-Type")) + } + // No album source is enabled: a 404, which the page shows as its placeholder. + if w := get("/api/catalogue/art/release-group/" + searchGroupMBID); w.Code != http.StatusNotFound { + t.Errorf("release group with no source: %d", w.Code) + } + for _, bad := range []string{"/api/catalogue/art/artist/not-an-id", "/api/catalogue/art/label/" + searchArtistMBID} { + if w := get(bad); w.Code != http.StatusNotFound { + t.Errorf("%s: %d", bad, w.Code) + } + } +} + +func TestSuggestionArt_PointsAtMinstrelsOwnEndpoint(t *testing.T) { + views := []suggestionView{{MBID: searchArtistMBID, Name: "Sigur Rós"}, {Name: "No MBID"}} + resolveSuggestionArt(views) + if views[0].ImageURL != "/api/catalogue/art/artist/"+searchArtistMBID+"?name=Sigur+R%C3%B3s" { + t.Errorf("image_url = %q", views[0].ImageURL) + } + if views[1].ImageURL != "" { + t.Errorf("no MBID, no image: %q", views[1].ImageURL) + } +} + +func TestSearchCache_ReusesAnAnswerButNotTheFlags(t *testing.T) { + c := newSearchCache() + now := time.Now() + c.now = func() time.Time { return now } + asks := 0 + ask := func(context.Context, string, string) ([]catalogueSearchResult, error) { + asks++ + return []catalogueSearchResult{{MBID: "m"}}, nil + } + first, _ := c.get(context.Background(), "artist", "Radiohead", ask) + first[0].InLibrary = true // the handler sets flags on what it is given + again, _ := c.get(context.Background(), "artist", "radiohead", ask) + if asks != 1 { + t.Errorf("asks = %d: the same words in another case are the same search", asks) + } + if again[0].InLibrary { + t.Error("a flag set on one answer leaked into the cache") + } + if _, _ = c.get(context.Background(), "album", "radiohead", ask); asks != 2 { + t.Error("another kind is another search") + } + now = now.Add(catalogueSearchTTL + time.Second) + if _, _ = c.get(context.Background(), "artist", "radiohead", ask); asks != 3 { + t.Error("an old answer is asked again") + } + var none *searchCache + if _, _ = none.get(context.Background(), "artist", "x", ask); asks != 4 { + t.Error("a nil cache asks every time") + } +} diff --git a/internal/api/lidarr.go b/internal/api/lidarr.go deleted file mode 100644 index 59e0dda6..00000000 --- a/internal/api/lidarr.go +++ /dev/null @@ -1,138 +0,0 @@ -package api - -import ( - "context" - "errors" - "net/http" - "strings" - - "git.fabledsword.com/bvandeusen/minstrel/internal/apierror" - "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" - "git.fabledsword.com/bvandeusen/minstrel/internal/lidarr" -) - -// lidarrSearchResult is the JSON shape returned by GET /api/lidarr/search. -// Field names follow the lowercase snake_case convention used by all /api/* -// responses. artist_mbid and album_mbid are the parent identifiers surfaced -// by LookupAlbum / LookupTrack; they are empty strings on artist results. -type lidarrSearchResult struct { - MBID string `json:"mbid"` - Name string `json:"name"` - SecondaryText string `json:"secondary_text"` - ImageURL string `json:"image_url"` - ArtistMBID string `json:"artist_mbid"` - AlbumMBID string `json:"album_mbid"` - InLibrary bool `json:"in_library"` - Requested bool `json:"requested"` -} - -// handleLidarrSearch implements GET /api/lidarr/search?q=&kind=artist|album|track. -// -// Steps: -// 1. Validate kind and q params. -// 2. Load lidarr config; 503 when disabled. -// 3. Call the matching Lidarr lookup. -// 4. For each result, compute in_library (EXISTS by MBID) and requested -// (HasNonTerminalRequestForMBID). -// 5. Return normalized JSON array. -func (h *handlers) handleLidarrSearch(w http.ResponseWriter, r *http.Request) { - kind := strings.TrimSpace(r.URL.Query().Get("kind")) - switch kind { - case "artist", "album", "track": - // valid - default: - writeErr(w, apierror.BadRequest("bad_kind", "kind must be artist, album, or track")) - return - } - - q := strings.TrimSpace(r.URL.Query().Get("q")) - if q == "" { - writeErr(w, apierror.BadRequest("missing_query", "q is required")) - return - } - - cfg, err := h.lidarrCfg.Get(r.Context()) - if err != nil { - h.logger.Error("api: lidarr search: load config", "err", err) - writeErr(w, &apierror.Error{Status: 500, Code: "lidarr_lookup_failed", Message: "failed to load lidarr config", Cause: err}) - return - } - if !cfg.Enabled { - writeErr(w, &apierror.Error{Status: 503, Code: "lidarr_disabled", Message: "Lidarr integration is not enabled"}) - return - } - - client := lidarr.NewClient(cfg.BaseURL, cfg.APIKey) - - var results []lidarr.LookupResult - switch kind { - case "artist": - results, err = client.LookupArtist(r.Context(), q) - case "album": - results, err = client.LookupAlbum(r.Context(), q) - case "track": - results, err = client.LookupTrack(r.Context(), q) - } - if err != nil { - switch { - case errors.Is(err, lidarr.ErrUnreachable): - writeErr(w, &apierror.Error{Status: 503, Code: "lidarr_unreachable", Message: "Lidarr is unreachable", Cause: err}) - case errors.Is(err, lidarr.ErrAuthFailed): - writeErr(w, &apierror.Error{Status: 503, Code: "lidarr_auth_failed", Message: "Lidarr authentication failed", Cause: err}) - default: - h.logger.Error("api: lidarr search: lookup failed", "kind", kind, "err", err) - writeErr(w, &apierror.Error{Status: 500, Code: "lidarr_lookup_failed", Message: "Lidarr lookup failed", Cause: err}) - } - return - } - - dbQ := dbq.New(h.pool) - out := make([]lidarrSearchResult, 0, len(results)) - for _, res := range results { - inLib, libErr := h.lidarrInLibrary(r.Context(), kind, res.MBID) - if libErr != nil { - h.logger.Error("api: lidarr search: in_library check failed", "err", libErr) - writeErr(w, &apierror.Error{Status: 500, Code: "lidarr_lookup_failed", Message: "library check failed", Cause: libErr}) - return - } - requested, reqErr := dbQ.HasNonTerminalRequestForMBID(r.Context(), res.MBID) - if reqErr != nil { - h.logger.Error("api: lidarr search: requested check failed", "err", reqErr) - writeErr(w, &apierror.Error{Status: 500, Code: "lidarr_lookup_failed", Message: "request check failed", Cause: reqErr}) - return - } - out = append(out, lidarrSearchResult{ - MBID: res.MBID, - Name: res.Name, - SecondaryText: res.Secondary, - ImageURL: res.ImageURL, - ArtistMBID: res.ArtistMBID, - AlbumMBID: res.AlbumMBID, - InLibrary: inLib, - Requested: requested, - }) - } - writeJSON(w, http.StatusOK, out) -} - -// lidarrInLibrary checks whether the given MBID exists in the corresponding -// local library table based on kind. Runs a direct EXISTS query since -// these per-kind MBID lookups are not in the sqlc-generated query set. -func (h *handlers) lidarrInLibrary(ctx context.Context, kind, mbid string) (bool, error) { - var query string - switch kind { - case "artist": - query = `SELECT EXISTS(SELECT 1 FROM artists WHERE mbid = $1)` - case "album": - query = `SELECT EXISTS(SELECT 1 FROM albums WHERE mbid = $1)` - case "track": - query = `SELECT EXISTS(SELECT 1 FROM tracks WHERE mbid = $1)` - default: - return false, nil - } - var exists bool - if err := h.pool.QueryRow(ctx, query, mbid).Scan(&exists); err != nil { - return false, err - } - return exists, nil -} diff --git a/internal/api/lidarr_test.go b/internal/api/lidarr_test.go index d800292e..3d18771f 100644 --- a/internal/api/lidarr_test.go +++ b/internal/api/lidarr_test.go @@ -2,20 +2,17 @@ package api import ( "context" - "encoding/json" - "fmt" "net/http" "net/http/httptest" "testing" - "time" - "github.com/jackc/pgx/v5/pgtype" - - "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" - "git.fabledsword.com/bvandeusen/minstrel/internal/dbtest" "git.fabledsword.com/bvandeusen/minstrel/internal/lidarrconfig" ) +// Helpers shared by the request, quarantine and notification tests. The +// Lidarr search they once sat beside is now the catalogue search +// (catalogue_test.go, M502 #5519). + // resetLidarrState clears lidarr_requests rows and resets lidarr_config // to the factory-default state (enabled=false, nulls) between tests. func resetLidarrState(t *testing.T, h *handlers) { @@ -44,11 +41,6 @@ func saveLidarrConfig(t *testing.T, h *handlers, baseURL string, enabled bool) { } } -// lidarrArtistStubBody returns a minimal Lidarr artist lookup JSON payload. -func lidarrArtistStubBody(mbid, name string) string { - return fmt.Sprintf(`[{"foreignArtistId":%q,"artistName":%q,"genres":["Rock"],"albumCount":5,"images":[]}]`, mbid, name) -} - // newLidarrStub creates an httptest.Server that always returns the given body // with 200 OK for any /api/v1/* path. func newLidarrStub(t *testing.T, body string) *httptest.Server { @@ -61,267 +53,3 @@ func newLidarrStub(t *testing.T, body string) *httptest.Server { t.Cleanup(srv.Close) return srv } - -// seedArtistWithMBID inserts an artist row with a specific MBID for in_library testing. -func seedArtistWithMBID(t *testing.T, h *handlers, name, mbid string) dbq.Artist { - t.Helper() - a, err := dbq.New(h.pool).UpsertArtist(context.Background(), dbq.UpsertArtistParams{ - Name: name, - SortName: name, - Mbid: &mbid, - }) - if err != nil { - t.Fatalf("seedArtistWithMBID: %v", err) - } - return a -} - -// seedRequestForMBID inserts a pending lidarr_requests row for the given MBID. -func seedRequestForMBID(t *testing.T, h *handlers, userID pgtype.UUID, kind dbq.LidarrRequestKind, artistMBID, mbid string) { - t.Helper() - var albumMBID, trackMBID *string - switch kind { - case dbq.LidarrRequestKindAlbum: - albumMBID = &mbid - case dbq.LidarrRequestKindTrack: - trackMBID = &mbid - } - _, err := dbq.New(h.pool).CreateLidarrRequest(context.Background(), dbq.CreateLidarrRequestParams{ - UserID: userID, - Kind: kind, - LidarrArtistMbid: artistMBID, - LidarrAlbumMbid: albumMBID, - LidarrTrackMbid: trackMBID, - ArtistName: "Test Artist", - AlbumTitle: nil, - TrackTitle: nil, - }) - if err != nil { - t.Fatalf("seedRequestForMBID: %v", err) - } -} - -// doLidarrSearch fires a GET /api/lidarr/search request directly at the handler. -// It simulates the RequireUser middleware by injecting a dummy user context. -func doLidarrSearch(t *testing.T, h *handlers, q, kind string) *httptest.ResponseRecorder { - t.Helper() - url := fmt.Sprintf("/api/lidarr/search?q=%s&kind=%s", q, kind) - req := httptest.NewRequest(http.MethodGet, url, nil) - // Inject a minimal user context the same way other handler tests do it. - // The lidarr search handler doesn't use the user from context, so any - // non-nil user value keeps the handler from panicking. - w := httptest.NewRecorder() - h.handleLidarrSearch(w, req) - return w -} - -// TestHandleLidarrSearch_ValidationErrors covers bad kind and missing query -// in a table-driven test. -func TestHandleLidarrSearch_ValidationErrors(t *testing.T) { - h, _ := testHandlers(t) - - cases := []struct { - name string - q string - kind string - wantStatus int - wantCode string - }{ - { - name: "bad kind playlist", - q: "Beatles", - kind: "playlist", - wantStatus: http.StatusBadRequest, - wantCode: "bad_kind", - }, - { - name: "missing query", - q: "", - kind: "artist", - wantStatus: http.StatusBadRequest, - wantCode: "missing_query", - }, - } - - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - url := fmt.Sprintf("/api/lidarr/search?q=%s&kind=%s", tc.q, tc.kind) - req := httptest.NewRequest(http.MethodGet, url, nil) - w := httptest.NewRecorder() - h.handleLidarrSearch(w, req) - - if w.Code != tc.wantStatus { - t.Fatalf("status = %d, want %d; body = %s", w.Code, tc.wantStatus, w.Body.String()) - } - var resp struct { - Error struct { - Code string `json:"code"` - } `json:"error"` - } - if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil { - t.Fatalf("decode body: %v; body=%s", err, w.Body.String()) - } - if resp.Error.Code != tc.wantCode { - t.Errorf("error.code = %q, want %q", resp.Error.Code, tc.wantCode) - } - }) - } -} - -// TestHandleLidarrSearch_DisabledReturns503 verifies that a disabled Lidarr -// config causes the handler to return 503 lidarr_disabled. -func TestHandleLidarrSearch_DisabledReturns503(t *testing.T) { - h, _ := testHandlers(t) - resetLidarrState(t, h) - - // Config is already disabled after resetLidarrState; ensure it's explicit. - saveLidarrConfig(t, h, "http://127.0.0.1:1", false) - - req := httptest.NewRequest(http.MethodGet, "/api/lidarr/search?q=Beatles&kind=artist", nil) - w := httptest.NewRecorder() - h.handleLidarrSearch(w, req) - - if w.Code != http.StatusServiceUnavailable { - t.Fatalf("status = %d, want 503; body = %s", w.Code, w.Body.String()) - } - var resp struct { - Error struct { - Code string `json:"code"` - } `json:"error"` - } - if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil { - t.Fatalf("decode body: %v; body=%s", err, w.Body.String()) - } - if resp.Error.Code != "lidarr_disabled" { - t.Errorf("error.code = %q, want lidarr_disabled", resp.Error.Code) - } -} - -// TestHandleLidarrSearch_HappyPath_RequestableResult checks a result that is -// neither in the library nor already requested. -func TestHandleLidarrSearch_HappyPath_RequestableResult(t *testing.T) { - h, _ := testHandlers(t) - resetLidarrState(t, h) - - const mbid = "artist-mbid-abc123" - stub := newLidarrStub(t, lidarrArtistStubBody(mbid, "The Beatles")) - saveLidarrConfig(t, h, stub.URL, true) - - w := doLidarrSearch(t, h, "Beatles", "artist") - - if w.Code != http.StatusOK { - t.Fatalf("status = %d, want 200; body = %s", w.Code, w.Body.String()) - } - var results []lidarrSearchResult - if err := json.Unmarshal(w.Body.Bytes(), &results); err != nil { - t.Fatalf("decode: %v; body=%s", err, w.Body.String()) - } - if len(results) != 1 { - t.Fatalf("len(results) = %d, want 1", len(results)) - } - r := results[0] - if r.MBID != mbid { - t.Errorf("mbid = %q, want %q", r.MBID, mbid) - } - if r.InLibrary { - t.Errorf("in_library = true, want false") - } - if r.Requested { - t.Errorf("requested = true, want false") - } -} - -// TestHandleLidarrSearch_HappyPath_InLibraryResult checks that a result whose -// MBID matches an artist in the local DB has in_library=true. -func TestHandleLidarrSearch_HappyPath_InLibraryResult(t *testing.T) { - h, _ := testHandlers(t) - resetLidarrState(t, h) - - const mbid = "artist-mbid-inlib" - stub := newLidarrStub(t, lidarrArtistStubBody(mbid, "Library Artist")) - saveLidarrConfig(t, h, stub.URL, true) - - // Seed the artist with the matching MBID so in_library becomes true. - seedArtistWithMBID(t, h, dbtest.TestUserPrefix+"LibraryArtist", mbid) - - w := doLidarrSearch(t, h, "Library+Artist", "artist") - - if w.Code != http.StatusOK { - t.Fatalf("status = %d, want 200; body = %s", w.Code, w.Body.String()) - } - var results []lidarrSearchResult - if err := json.Unmarshal(w.Body.Bytes(), &results); err != nil { - t.Fatalf("decode: %v; body=%s", err, w.Body.String()) - } - if len(results) != 1 { - t.Fatalf("len(results) = %d, want 1", len(results)) - } - if !results[0].InLibrary { - t.Errorf("in_library = false, want true") - } -} - -// TestHandleLidarrSearch_HappyPath_AlreadyRequested verifies that a result -// whose MBID matches a pending lidarr_requests row has requested=true. -func TestHandleLidarrSearch_HappyPath_AlreadyRequested(t *testing.T) { - h, _ := testHandlers(t) - resetLidarrState(t, h) - - const mbid = "artist-mbid-requested" - stub := newLidarrStub(t, lidarrArtistStubBody(mbid, "Requested Artist")) - saveLidarrConfig(t, h, stub.URL, true) - - // Seed a user and a pending request for the artist MBID. - user := seedUser(t, h.pool, "requester", "pw", false) - seedRequestForMBID(t, h, user.ID, dbq.LidarrRequestKindArtist, mbid, mbid) - - w := doLidarrSearch(t, h, "Requested+Artist", "artist") - - if w.Code != http.StatusOK { - t.Fatalf("status = %d, want 200; body = %s", w.Code, w.Body.String()) - } - var results []lidarrSearchResult - if err := json.Unmarshal(w.Body.Bytes(), &results); err != nil { - t.Fatalf("decode: %v; body=%s", err, w.Body.String()) - } - if len(results) != 1 { - t.Fatalf("len(results) = %d, want 1", len(results)) - } - if !results[0].Requested { - t.Errorf("requested = false, want true") - } -} - -// TestHandleLidarrSearch_LidarrUnreachableReturns503 verifies that a -// connection-refused Lidarr base URL returns 503 lidarr_unreachable. -func TestHandleLidarrSearch_LidarrUnreachableReturns503(t *testing.T) { - h, _ := testHandlers(t) - resetLidarrState(t, h) - - // Port 1 is reserved and will refuse connections immediately. - saveLidarrConfig(t, h, "http://127.0.0.1:1", true) - - req := httptest.NewRequest(http.MethodGet, "/api/lidarr/search?q=Beatles&kind=artist", nil) - // Use a short-deadline context so the test doesn't hang waiting for TCP. - ctx, cancel := context.WithTimeout(req.Context(), 3*time.Second) - defer cancel() - req = req.WithContext(ctx) - - w := httptest.NewRecorder() - h.handleLidarrSearch(w, req) - - if w.Code != http.StatusServiceUnavailable { - t.Fatalf("status = %d, want 503; body = %s", w.Code, w.Body.String()) - } - var resp struct { - Error struct { - Code string `json:"code"` - } `json:"error"` - } - if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil { - t.Fatalf("decode: %v; body=%s", err, w.Body.String()) - } - if resp.Error.Code != "lidarr_unreachable" { - t.Errorf("error.code = %q, want lidarr_unreachable", resp.Error.Code) - } -} diff --git a/internal/api/suggestions.go b/internal/api/suggestions.go index 977d571f..cb1a814c 100644 --- a/internal/api/suggestions.go +++ b/internal/api/suggestions.go @@ -1,21 +1,18 @@ package api import ( - "context" "encoding/json" "errors" "io" "net/http" "strconv" "strings" - "sync" "github.com/go-chi/chi/v5" "github.com/jackc/pgx/v5/pgtype" "git.fabledsword.com/bvandeusen/minstrel/internal/apierror" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" - "git.fabledsword.com/bvandeusen/minstrel/internal/lidarr" "git.fabledsword.com/bvandeusen/minstrel/internal/recommendation" ) @@ -31,10 +28,10 @@ type suggestionView struct { // out-of-library artists is permanently partial (#2376), and the card // falls back to the seed attribution it has always shown. MatchedTags []string `json:"matched_tags,omitempty"` - // ImageURL is resolved on-demand from Lidarr (out-of-library - // artists have no local art row). Omitted when Lidarr is disabled - // or has no match — the client falls back to a placeholder. Not - // cached: a remote URL Lidarr surfaced, fetched by the browser. + // ImageURL is Minstrel's catalogue art endpoint for the candidate + // (out-of-library artists have no local art row, M502 #5519). It + // answers 404 when no enabled source has an image, and the client + // falls back to a placeholder. ImageURL string `json:"image_url,omitempty"` } @@ -105,7 +102,7 @@ func (h *handlers) handleListSuggestions(w http.ResponseWriter, r *http.Request) MatchedTags: s.MatchedTags, }) } - h.resolveSuggestionArt(r.Context(), out) + resolveSuggestionArt(out) writeJSON(w, http.StatusOK, out) } @@ -250,48 +247,15 @@ func (h *handlers) handleListSuggestionSnoozes(w http.ResponseWriter, r *http.Re writeJSON(w, http.StatusOK, out) } -// resolveSuggestionArt fills ImageURL on-demand from Lidarr's artist -// lookup, matched by MBID (foreignArtistId). Best-effort and cache-free: -// Lidarr is the only source — when it's disabled, unreachable, or has -// no match for a candidate, that entry keeps an empty ImageURL and the -// client renders its placeholder. Lookups run with bounded concurrency -// so a full Discover page doesn't serialize ~12 round-trips. Never -// fails the request; the suggestions list is the contract, art is a -// nicety. -func (h *handlers) resolveSuggestionArt(ctx context.Context, views []suggestionView) { - if len(views) == 0 { - return - } - cfg, err := h.lidarrCfg.Get(ctx) - if err != nil || !cfg.Enabled || cfg.BaseURL == "" || cfg.APIKey == "" { - return // Lidarr off / unconfigured → placeholders only - } - client := lidarr.NewClient(cfg.BaseURL, cfg.APIKey) - - const maxConcurrent = 6 - sem := make(chan struct{}, maxConcurrent) - var wg sync.WaitGroup +// resolveSuggestionArt points each suggestion's ImageURL at Minstrel's +// catalogue art endpoint (M502 #5519). Nothing is fetched here: the image is +// looked up from the enabled cover-art sources when the browser asks for it, +// and kept, so listing suggestions makes no outbound call. A candidate with +// no MBID keeps an empty ImageURL and the client renders its placeholder. +func resolveSuggestionArt(views []suggestionView) { for i := range views { - if views[i].MBID == "" || views[i].Name == "" { - continue + if views[i].MBID != "" { + views[i].ImageURL = artistArtURL(views[i].MBID, views[i].Name) } - wg.Add(1) - sem <- struct{}{} - go func(idx int) { - defer wg.Done() - defer func() { <-sem }() - results, lerr := client.LookupArtist(ctx, views[idx].Name) - if lerr != nil { - return // best-effort: no art on lookup failure - } - for _, res := range results { - if res.MBID == views[idx].MBID && res.ImageURL != "" { - // Distinct slice index per goroutine → race-free. - views[idx].ImageURL = res.ImageURL - return - } - } - }(i) } - wg.Wait() } diff --git a/internal/catalogue/art.go b/internal/catalogue/art.go new file mode 100644 index 00000000..ca60066d --- /dev/null +++ b/internal/catalogue/art.go @@ -0,0 +1,214 @@ +package catalogue + +import ( + "context" + "errors" + "fmt" + "os" + "path/filepath" + "regexp" + "sync" + "time" + + "git.fabledsword.com/bvandeusen/minstrel/internal/coverart" +) + +// NoArtRetry is how long "no source has art for this" is believed before +// the sources are asked again. +const NoArtRetry = 7 * 24 * time.Hour + +// fetchTimeout bounds one look-up across every source (rule 156). +const fetchTimeout = 30 * time.Second + +// ErrNoArt: no enabled source has an image, or the sources could not be +// reached. The caller shows its placeholder. +var ErrNoArt = errors.New("catalogue: no art") + +var mbidPattern = regexp.MustCompile(`^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$`) + +// ValidMBID reports whether s is shaped like a MusicBrainz id. It is the +// guard that keeps a request path out of the cache directory's parents. +func ValidMBID(s string) bool { return mbidPattern.MatchString(s) } + +// Art serves images for artists and release groups that are not in the +// library (search results, Discover suggestions, wanted albums), from the +// operator's enabled cover-art sources. Each image is fetched once and kept +// under the data directory, so the browser only ever talks to Minstrel. +type Art struct { + dir string + artistSources func() []coverart.ArtistArtProvider + albumSources func() []coverart.AlbumCoverProvider + now func() time.Time + + mu sync.Mutex + inflight map[string]*artCall +} + +type artCall struct { + done chan struct{} + body []byte + err error +} + +// ArtSources lists the image sources to ask, in order. The cover-art +// settings service is one: it answers with what the operator has enabled. +type ArtSources interface { + EnabledArtistProviders() []coverart.ArtistArtProvider + EnabledAlbumProviders() []coverart.AlbumCoverProvider +} + +// NewArt keeps images under dataDir/catalogue-art and asks the given +// sources. Nil sources ask nothing, so every image is the placeholder. +func NewArt(dataDir string, sources ArtSources) *Art { + a := &Art{dir: filepath.Join(dataDir, "catalogue-art"), now: time.Now, inflight: map[string]*artCall{}} + if sources != nil { + a.artistSources = sources.EnabledArtistProviders + a.albumSources = sources.EnabledAlbumProviders + } + return a +} + +// Artist returns an image of the artist: the first enabled artist source +// with one wins, its thumbnail preferred over its backdrop. +func (a *Art) Artist(ctx context.Context, mbid, name string) ([]byte, error) { + return a.cached(ctx, "artist", mbid, func(ctx context.Context) ([]byte, error) { + if a.artistSources == nil { + return nil, coverart.ErrNotFound + } + ref := coverart.ArtistRef{MBID: mbid, Name: name} + return firstOf(a.artistSources(), func(p coverart.ArtistArtProvider) ([]byte, error) { + thumb, fanart, err := p.FetchArtistArt(ctx, ref) + if err == nil && len(thumb) == 0 { + thumb = fanart + } + return thumb, err + }) + }) +} + +// ReleaseGroup returns a cover for the release group: a source that can +// answer by group (the Cover Art Archive) is asked by its id, the rest by +// artist and title. +func (a *Art) ReleaseGroup(ctx context.Context, mbid, artist, title string) ([]byte, error) { + return a.cached(ctx, "release-group", mbid, func(ctx context.Context) ([]byte, error) { + if a.albumSources == nil { + return nil, coverart.ErrNotFound + } + byName := coverart.AlbumRef{ArtistName: artist, AlbumTitle: title} + return firstOf(a.albumSources(), func(p coverart.AlbumCoverProvider) ([]byte, error) { + if g, ok := p.(coverart.ReleaseGroupCoverProvider); ok { + return g.FetchReleaseGroupCover(ctx, mbid) + } + if artist == "" || title == "" { + return nil, coverart.ErrNotFound + } + return p.FetchAlbumCover(ctx, byName) + }) + }) +} + +// firstOf asks each source in turn. It reports ErrNotFound only when every +// source said so; if any could not be reached, the miss is transient. +func firstOf[P any](sources []P, ask func(P) ([]byte, error)) ([]byte, error) { + transient := false + for _, p := range sources { + body, err := ask(p) + if err == nil && len(body) > 0 { + return body, nil + } + if err != nil && !errors.Is(err, coverart.ErrNotFound) { + transient = true + } + } + if transient { + return nil, coverart.ErrTransient + } + return nil, coverart.ErrNotFound +} + +// cached serves kind/mbid from disk, or fetches it once however many +// requests ask at the same moment. A definite "nobody has one" is written +// down and believed for NoArtRetry; an unreachable source is not, so the +// next request asks again. +func (a *Art) cached(ctx context.Context, kind, mbid string, fetch func(context.Context) ([]byte, error)) ([]byte, error) { + if !ValidMBID(mbid) { + return nil, ErrNoArt + } + dir := filepath.Join(a.dir, kind) + imgPath := filepath.Join(dir, mbid) + nonePath := imgPath + ".none" + + if body, err := os.ReadFile(imgPath); err == nil && len(body) > 0 { + return body, nil + } + if fi, err := os.Stat(nonePath); err == nil && a.now().Sub(fi.ModTime()) < NoArtRetry { + return nil, ErrNoArt + } + + key := kind + "/" + mbid + a.mu.Lock() + if call, ok := a.inflight[key]; ok { + a.mu.Unlock() + select { + case <-call.done: + return call.body, call.err + case <-ctx.Done(): + return nil, ctx.Err() + } + } + call := &artCall{done: make(chan struct{})} + a.inflight[key] = call + a.mu.Unlock() + + defer func() { + a.mu.Lock() + delete(a.inflight, key) + a.mu.Unlock() + close(call.done) + }() + + // The fetch outlives a browser that navigates away: a second request is + // likely, and the image is worth keeping. + fctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), fetchTimeout) + defer cancel() + body, err := fetch(fctx) + switch { + case err == nil && len(body) > 0: + if werr := writeFile(dir, imgPath, body); werr != nil { + // Serving it uncached is still better than a placeholder. + call.body = body + return body, nil + } + _ = os.Remove(nonePath) + call.body = body + return body, nil + case err == nil || errors.Is(err, coverart.ErrNotFound): + _ = writeFile(dir, nonePath, nil) + } + call.err = ErrNoArt + return nil, ErrNoArt +} + +func writeFile(dir, path string, body []byte) error { + if err := os.MkdirAll(dir, 0o755); err != nil { + return fmt.Errorf("catalogue art: %w", err) + } + tmp, err := os.CreateTemp(dir, ".part-*") + if err != nil { + return fmt.Errorf("catalogue art: %w", err) + } + if _, err := tmp.Write(body); err != nil { + _ = tmp.Close() + _ = os.Remove(tmp.Name()) + return fmt.Errorf("catalogue art: %w", err) + } + if err := tmp.Close(); err != nil { + _ = os.Remove(tmp.Name()) + return fmt.Errorf("catalogue art: %w", err) + } + if err := os.Rename(tmp.Name(), path); err != nil { + _ = os.Remove(tmp.Name()) + return fmt.Errorf("catalogue art: %w", err) + } + return nil +} diff --git a/internal/catalogue/art_test.go b/internal/catalogue/art_test.go new file mode 100644 index 00000000..70d099e9 --- /dev/null +++ b/internal/catalogue/art_test.go @@ -0,0 +1,165 @@ +package catalogue + +import ( + "context" + "errors" + "os" + "path/filepath" + "sync" + "sync/atomic" + "testing" + "time" + + "git.fabledsword.com/bvandeusen/minstrel/internal/coverart" +) + +const someMBID = "e21857d5-3256-4547-afb3-4b6ded592596" + +// fakeSource is an artist and album source answering with fixed results. +type fakeSource struct { + thumb, fanart, cover []byte + err error + calls atomic.Int32 + gate chan struct{} // when set, each fetch waits for it + lastRef coverart.AlbumRef + mu sync.Mutex +} + +func (f *fakeSource) ID() string { return "fake" } +func (f *fakeSource) DisplayName() string { return "Fake" } +func (f *fakeSource) RequiresAPIKey() bool { return false } +func (f *fakeSource) DefaultEnabled() bool { return true } +func (f *fakeSource) Configure(_ coverart.ProviderSettings) error { return nil } + +func (f *fakeSource) wait() { + f.calls.Add(1) + if f.gate != nil { + <-f.gate + } +} + +func (f *fakeSource) FetchArtistArt(_ context.Context, _ coverart.ArtistRef) ([]byte, []byte, error) { + f.wait() + return f.thumb, f.fanart, f.err +} + +func (f *fakeSource) FetchAlbumCover(_ context.Context, ref coverart.AlbumRef) ([]byte, error) { + f.wait() + f.mu.Lock() + f.lastRef = ref + f.mu.Unlock() + return f.cover, f.err +} + +// groupSource can also answer by release group, as the Cover Art Archive does. +type groupSource struct{ *fakeSource } + +func (g groupSource) FetchReleaseGroupCover(_ context.Context, _ string) ([]byte, error) { + g.wait() + return []byte("by-group"), nil +} + +type sources struct { + artists []coverart.ArtistArtProvider + albums []coverart.AlbumCoverProvider +} + +func (s sources) EnabledArtistProviders() []coverart.ArtistArtProvider { return s.artists } +func (s sources) EnabledAlbumProviders() []coverart.AlbumCoverProvider { return s.albums } + +func TestArt_FetchesOnceAndKeepsIt(t *testing.T) { + src := &fakeSource{fanart: []byte("backdrop")} + a := NewArt(t.TempDir(), sources{artists: []coverart.ArtistArtProvider{src}}) + + body, err := a.Artist(context.Background(), someMBID, "Gorillaz") + if err != nil || string(body) != "backdrop" { + t.Fatalf("with only a backdrop, use it: %q, %v", body, err) + } + if body, _ := a.Artist(context.Background(), someMBID, "Gorillaz"); string(body) != "backdrop" || src.calls.Load() != 1 { + t.Errorf("the second ask should come from disk: %q after %d calls", body, src.calls.Load()) + } +} + +func TestArt_NobodyHasOneIsBelievedForAWeek(t *testing.T) { + src := &fakeSource{err: coverart.ErrNotFound} + a := NewArt(t.TempDir(), sources{artists: []coverart.ArtistArtProvider{src}}) + + if _, err := a.Artist(context.Background(), someMBID, "x"); !errors.Is(err, ErrNoArt) { + t.Fatalf("err = %v, want ErrNoArt", err) + } + if _, err := a.Artist(context.Background(), someMBID, "x"); !errors.Is(err, ErrNoArt) || src.calls.Load() != 1 { + t.Errorf("a definite miss is remembered: %d calls", src.calls.Load()) + } + a.now = func() time.Time { return time.Now().Add(NoArtRetry + time.Hour) } + src.err, src.thumb = nil, []byte("thumb") + if body, err := a.Artist(context.Background(), someMBID, "x"); err != nil || string(body) != "thumb" { + t.Errorf("after a week the sources are asked again: %q, %v", body, err) + } +} + +func TestArt_AnUnreachableSourceIsAskedAgain(t *testing.T) { + src := &fakeSource{err: coverart.ErrTransient} + a := NewArt(t.TempDir(), sources{artists: []coverart.ArtistArtProvider{src}}) + for range 2 { + if _, err := a.Artist(context.Background(), someMBID, "x"); !errors.Is(err, ErrNoArt) { + t.Fatalf("err = %v", err) + } + } + if src.calls.Load() != 2 { + t.Errorf("calls = %d: an outage must not be written down as 'no art'", src.calls.Load()) + } +} + +func TestArt_ReleaseGroupAsksByGroupThenByName(t *testing.T) { + byName := &fakeSource{cover: []byte("by-name")} + caa := groupSource{&fakeSource{}} + a := NewArt(t.TempDir(), sources{albums: []coverart.AlbumCoverProvider{caa, byName}}) + if body, _ := a.ReleaseGroup(context.Background(), someMBID, "Gorillaz", "Demon Days"); string(body) != "by-group" { + t.Errorf("a source that answers by group is asked by group: %q", body) + } + + other := "0c4d5b5f-2f3c-3d31-9c4e-4b4f6b4c1a01" + a = NewArt(t.TempDir(), sources{albums: []coverart.AlbumCoverProvider{byName}}) + if body, _ := a.ReleaseGroup(context.Background(), other, "Gorillaz", "Demon Days"); string(body) != "by-name" { + t.Errorf("by name: %q", body) + } + if byName.lastRef.MBID != "" || byName.lastRef.AlbumTitle != "Demon Days" { + t.Errorf("a release-group id is not a release id, so it is not passed as one: %+v", byName.lastRef) + } +} + +func TestArt_ConcurrentAsksShareOneFetch(t *testing.T) { + src := &fakeSource{thumb: []byte("t"), gate: make(chan struct{})} + a := NewArt(t.TempDir(), sources{artists: []coverart.ArtistArtProvider{src}}) + var wg sync.WaitGroup + for range 5 { + wg.Add(1) + go func() { + defer wg.Done() + if body, err := a.Artist(context.Background(), someMBID, "x"); err != nil || string(body) != "t" { + t.Errorf("%q, %v", body, err) + } + }() + } + // Let the fetch through once every caller has had time to queue on it. + time.Sleep(50 * time.Millisecond) + close(src.gate) + wg.Wait() + if src.calls.Load() != 1 { + t.Errorf("calls = %d, want one fetch for five asks", src.calls.Load()) + } +} + +func TestArt_RefusesWhatIsNotAnMBID(t *testing.T) { + dir := t.TempDir() + a := NewArt(dir, sources{artists: []coverart.ArtistArtProvider{&fakeSource{thumb: []byte("t")}}}) + if _, err := a.Artist(context.Background(), "../../etc/passwd", "x"); !errors.Is(err, ErrNoArt) { + t.Fatalf("err = %v", err) + } + if _, err := os.Stat(filepath.Join(dir, "catalogue-art")); !os.IsNotExist(err) { + t.Error("nothing should be written for a refused id") + } + if _, err := NewArt(dir, nil).Artist(context.Background(), someMBID, "x"); !errors.Is(err, ErrNoArt) { + t.Errorf("no sources: err = %v", err) + } +} diff --git a/internal/catalogue/catalogue.go b/internal/catalogue/catalogue.go new file mode 100644 index 00000000..9084f9dd --- /dev/null +++ b/internal/catalogue/catalogue.go @@ -0,0 +1,311 @@ +// Package catalogue is Minstrel's own record of what artists have released, +// read from MusicBrainz and kept in the database (M502 #5519). The wanted +// list, search and import read it; MusicBrainz is asked only when what is +// held is missing or old. +package catalogue + +import ( + "context" + "errors" + "fmt" + "log/slog" + "time" + + "github.com/jackc/pgx/v5" + "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/musicbrainz" +) + +const ( + // DiscographyMaxAge is how long an artist's list of release groups is + // trusted before it is read again. New releases appear on this cadence. + DiscographyMaxAge = 7 * 24 * time.Hour + // ReleasesMaxAge is how long a release group's editions and track lists + // are trusted. They change rarely once a group is out. + ReleasesMaxAge = 30 * 24 * time.Hour +) + +// ErrNotFound: MusicBrainz has no artist or release group under that id. +var ErrNotFound = errors.New("catalogue: not in MusicBrainz") + +// Service reads and refreshes the catalogue. +type Service struct { + pool *pgxpool.Pool + mb *musicbrainz.Client + logger *slog.Logger + now func() time.Time +} + +// New makes a Service. A nil client talks to the public MusicBrainz. +func New(pool *pgxpool.Pool, mb *musicbrainz.Client, logger *slog.Logger) *Service { + if mb == nil { + mb = &musicbrainz.Client{} + } + if logger == nil { + logger = slog.Default() + } + return &Service{pool: pool, mb: mb, logger: logger, now: time.Now} +} + +// MusicBrainz is the client the service asks, for callers that search. +func (s *Service) MusicBrainz() *musicbrainz.Client { return s.mb } + +// Discography is an artist and the release groups credited to them. +type Discography struct { + Artist dbq.MbArtist + Groups []dbq.MbReleaseGroup + // Stale: MusicBrainz could not be asked, so this is the last copy read. + Stale bool +} + +// Discography returns the artist's release groups, reading them from +// MusicBrainz first when they have never been read or are older than +// DiscographyMaxAge. When MusicBrainz is unreachable, a copy already held is +// returned marked Stale; with nothing held, the error is returned. +func (s *Service) Discography(ctx context.Context, artistMBID string) (Discography, error) { + q := dbq.New(s.pool) + artist, err := q.GetMBArtist(ctx, artistMBID) + held := err == nil && artist.DiscographyRefreshedAt.Valid + if err != nil && !errors.Is(err, pgx.ErrNoRows) { + return Discography{}, fmt.Errorf("catalogue: read artist: %w", err) + } + stale := false + if !held || s.now().Sub(artist.DiscographyRefreshedAt.Time) > DiscographyMaxAge { + if rerr := s.RefreshArtist(ctx, artistMBID); rerr != nil { + if !held || !errors.Is(rerr, musicbrainz.ErrUnreachable) { + return Discography{}, rerr + } + stale = true + } + if artist, err = q.GetMBArtist(ctx, artistMBID); err != nil { + return Discography{}, fmt.Errorf("catalogue: read artist: %w", err) + } + } + groups, err := q.ListMBArtistReleaseGroups(ctx, artistMBID) + if err != nil { + return Discography{}, fmt.Errorf("catalogue: read discography: %w", err) + } + return Discography{Artist: artist, Groups: groups, Stale: stale}, nil +} + +// RefreshArtist reads the artist and every release group credited to them, +// and replaces the held discography in one transaction. +func (s *Service) RefreshArtist(ctx context.Context, artistMBID string) error { + artist, err := s.mb.Artist(ctx, artistMBID) + if err != nil { + return mbErr(err) + } + groups, err := s.mb.ArtistReleaseGroups(ctx, artistMBID) + if err != nil { + return mbErr(err) + } + return pgx.BeginFunc(ctx, s.pool, func(tx pgx.Tx) error { + q := dbq.New(tx) + if err := q.UpsertMBArtist(ctx, dbq.UpsertMBArtistParams{ + Mbid: artist.MBID, Name: artist.Name, SortName: artist.SortName, + Disambiguation: artist.Disambiguation, ArtistType: artist.Type, Country: artist.Country, + }); err != nil { + return fmt.Errorf("catalogue: save artist: %w", err) + } + if err := q.DeleteMBArtistReleaseGroups(ctx, artist.MBID); err != nil { + return fmt.Errorf("catalogue: clear discography: %w", err) + } + for _, g := range groups { + if err := upsertGroup(ctx, q, g); err != nil { + return err + } + if err := q.AddMBArtistReleaseGroup(ctx, dbq.AddMBArtistReleaseGroupParams{ + ArtistMbid: artist.MBID, ReleaseGroupMbid: g.MBID, + }); err != nil { + return fmt.Errorf("catalogue: link release group: %w", err) + } + } + return q.MarkMBArtistDiscographyRefreshed(ctx, artist.MBID) + }) +} + +func upsertGroup(ctx context.Context, q *dbq.Queries, g musicbrainz.ReleaseGroup) error { + secondary := g.SecondaryTypes + if secondary == nil { + secondary = []string{} + } + if err := q.UpsertMBReleaseGroup(ctx, dbq.UpsertMBReleaseGroupParams{ + Mbid: g.MBID, Title: g.Title, PrimaryType: g.PrimaryType, SecondaryTypes: secondary, + FirstReleaseDate: g.FirstReleaseDate, ArtistCredit: g.ArtistCredit, + }); err != nil { + return fmt.Errorf("catalogue: save release group: %w", err) + } + return nil +} + +// Edition is one release of a group with its track list. +type Edition struct { + Release dbq.MbRelease + Tracks []dbq.MbReleaseTrack +} + +// Editions is a release group and its releases. +type Editions struct { + Group dbq.MbReleaseGroup + Editions []Edition + Stale bool +} + +// Releases returns a release group's releases with their track lists, read +// from MusicBrainz first when never read or older than ReleasesMaxAge. Like +// Discography, an unreachable MusicBrainz serves the held copy as Stale. +func (s *Service) Releases(ctx context.Context, groupMBID string) (Editions, error) { + q := dbq.New(s.pool) + group, err := q.GetMBReleaseGroup(ctx, groupMBID) + if err != nil && !errors.Is(err, pgx.ErrNoRows) { + return Editions{}, fmt.Errorf("catalogue: read release group: %w", err) + } + held := err == nil && group.ReleasesRefreshedAt.Valid + stale := false + if !held || s.now().Sub(group.ReleasesRefreshedAt.Time) > ReleasesMaxAge { + if rerr := s.RefreshReleaseGroup(ctx, groupMBID); rerr != nil { + if !held || !errors.Is(rerr, musicbrainz.ErrUnreachable) { + return Editions{}, rerr + } + stale = true + } + if group, err = q.GetMBReleaseGroup(ctx, groupMBID); err != nil { + return Editions{}, fmt.Errorf("catalogue: read release group: %w", err) + } + } + releases, err := q.ListMBReleasesForGroup(ctx, groupMBID) + if err != nil { + return Editions{}, fmt.Errorf("catalogue: read releases: %w", err) + } + tracks, err := q.ListMBReleaseTracksForGroup(ctx, groupMBID) + if err != nil { + return Editions{}, fmt.Errorf("catalogue: read track lists: %w", err) + } + byRelease := make(map[string][]dbq.MbReleaseTrack, len(releases)) + for _, t := range tracks { + byRelease[t.ReleaseMbid] = append(byRelease[t.ReleaseMbid], t) + } + out := Editions{Group: group, Stale: stale} + for _, r := range releases { + out.Editions = append(out.Editions, Edition{Release: r, Tracks: byRelease[r.Mbid]}) + } + return out, nil +} + +// RefreshReleaseGroup reads a group's releases and track lists and replaces +// the held copy in one transaction. A group not yet held is looked up first. +func (s *Service) RefreshReleaseGroup(ctx context.Context, groupMBID string) error { + q := dbq.New(s.pool) + var group *musicbrainz.ReleaseGroup + if _, err := q.GetMBReleaseGroup(ctx, groupMBID); errors.Is(err, pgx.ErrNoRows) { + g, gerr := s.mb.ReleaseGroup(ctx, groupMBID) + if gerr != nil { + return mbErr(gerr) + } + group = &g + } else if err != nil { + return fmt.Errorf("catalogue: read release group: %w", err) + } + releases, err := s.mb.ReleaseGroupReleases(ctx, groupMBID) + if err != nil { + return mbErr(err) + } + return pgx.BeginFunc(ctx, s.pool, func(tx pgx.Tx) error { + tq := dbq.New(tx) + if group != nil { + if err := upsertGroup(ctx, tq, *group); err != nil { + return err + } + } + if err := tq.DeleteMBReleasesForGroup(ctx, groupMBID); err != nil { + return fmt.Errorf("catalogue: clear releases: %w", err) + } + for _, r := range releases { + if err := tq.InsertMBRelease(ctx, dbq.InsertMBReleaseParams{ + Mbid: r.MBID, ReleaseGroupMbid: groupMBID, Title: r.Title, Status: r.Status, + ReleaseDate: r.Date, Country: r.Country, Disambiguation: r.Disambiguation, + Formats: r.Formats(), TrackCount: int32(r.TrackCount()), + }); err != nil { + return fmt.Errorf("catalogue: save release: %w", err) + } + for _, m := range r.Media { + for _, t := range m.Tracks { + if err := tq.InsertMBReleaseTrack(ctx, dbq.InsertMBReleaseTrackParams{ + ReleaseMbid: r.MBID, MediumPosition: int32(m.Position), Position: int32(t.Position), + MediumFormat: m.Format, Number: t.Number, Title: t.Title, + LengthMs: int32(t.LengthMs), RecordingMbid: t.RecordingMBID, + }); err != nil { + return fmt.Errorf("catalogue: save track: %w", err) + } + } + } + } + return tq.MarkMBReleaseGroupReleasesRefreshed(ctx, groupMBID) + }) +} + +// RefreshDue re-reads up to limit discographies that are older than +// DiscographyMaxAge, never-read first. It stops at the first unreachable +// MusicBrainz, since the rest would fail the same way. +func (s *Service) RefreshDue(ctx context.Context, limit int) (refreshed int, err error) { + due, err := dbq.New(s.pool).ListMBArtistsDueForRefresh(ctx, dbq.ListMBArtistsDueForRefreshParams{ + Before: pgtype.Timestamptz{Time: s.now().Add(-DiscographyMaxAge), Valid: true}, + MaxRows: int32(limit), + }) + if err != nil { + return 0, fmt.Errorf("catalogue: list due artists: %w", err) + } + for _, mbid := range due { + if err := s.RefreshArtist(ctx, mbid); err != nil { + if errors.Is(err, musicbrainz.ErrUnreachable) || ctx.Err() != nil { + return refreshed, err + } + // One artist MusicBrainz cannot describe (merged away, say) + // must not stop the rest. + s.logger.Warn("catalogue: refresh artist", "mbid", mbid, "err", err) + continue + } + refreshed++ + } + return refreshed, nil +} + +// RefreshInterval is how often Run looks for discographies due a re-read, +// and RefreshBatch how many it reads per look: 20 artists is about a minute +// of MusicBrainz's budget. +const ( + RefreshInterval = time.Hour + RefreshBatch = 20 +) + +// Run refreshes due discographies every RefreshInterval until ctx ends. +func (s *Service) Run(ctx context.Context) { + ticker := time.NewTicker(RefreshInterval) + defer ticker.Stop() + for { + select { + case <-ctx.Done(): + return + case <-ticker.C: + } + n, err := s.RefreshDue(ctx, RefreshBatch) + switch { + case err != nil && ctx.Err() == nil: + s.logger.Warn("catalogue: refresh pass stopped", "refreshed", n, "err", err) + case n > 0: + s.logger.Info("catalogue: refresh pass complete", "refreshed", n) + } + } +} + +// mbErr maps MusicBrainz's not-found to the catalogue's; anything else passes +// through so callers can tell an unreachable MusicBrainz apart. +func mbErr(err error) error { + if errors.Is(err, musicbrainz.ErrNotFound) { + return fmt.Errorf("%w: %v", ErrNotFound, err) + } + return err +} diff --git a/internal/catalogue/catalogue_test.go b/internal/catalogue/catalogue_test.go new file mode 100644 index 00000000..5b495122 --- /dev/null +++ b/internal/catalogue/catalogue_test.go @@ -0,0 +1,247 @@ +package catalogue + +import ( + "context" + "errors" + "fmt" + "io" + "log/slog" + "net/http" + "net/http/httptest" + "os" + "strings" + "sync/atomic" + "testing" + "time" + + "github.com/jackc/pgx/v5/pgxpool" + + "git.fabledsword.com/bvandeusen/minstrel/internal/db" + "git.fabledsword.com/bvandeusen/minstrel/internal/dbtest" + "git.fabledsword.com/bvandeusen/minstrel/internal/musicbrainz" +) + +func newPool(t *testing.T) *pgxpool.Pool { + t.Helper() + if testing.Short() { + t.Skip("skipping integration test in -short mode") + } + dsn := os.Getenv("MINSTREL_TEST_DATABASE_URL") + if dsn == "" { + t.Skip("MINSTREL_TEST_DATABASE_URL not set") + } + if err := db.Migrate(dsn, slog.New(slog.NewTextHandler(io.Discard, nil))); err != nil { + t.Fatalf("migrate: %v", err) + } + pool, err := pgxpool.New(context.Background(), dsn) + if err != nil { + t.Fatalf("pool: %v", err) + } + t.Cleanup(pool.Close) + dbtest.ResetDB(t, pool) + return pool +} + +const ( + gorillaz = "e21857d5-3256-4547-afb3-4b6ded592596" + deLaSoul = "4f2a6b9e-9ae6-4f34-9a41-3e9f2f7c4c1d" + demonDays = "0c4d5b5f-2f3c-3d31-9c4e-4b4f6b4c1a01" + feelGood = "0c4d5b5f-2f3c-3d31-9c4e-4b4f6b4c1a02" // credited to both + demonDaysR = "1a2b3c4d-0000-4000-8000-000000000001" + demonDaysJ = "1a2b3c4d-0000-4000-8000-000000000002" +) + +// fakeMB is a MusicBrainz with two artists, a collaboration between them, +// and one album with two editions. down makes every request a 503. +type fakeMB struct { + srv *httptest.Server + calls atomic.Int32 + down atomic.Bool +} + +func newFakeMB(t *testing.T) *fakeMB { + t.Helper() + f := &fakeMB{} + f.srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + f.calls.Add(1) + if f.down.Load() { + w.WriteHeader(http.StatusServiceUnavailable) + return + } + q := r.URL.Query() + switch { + case r.URL.Path == "/artist/"+gorillaz: + _, _ = fmt.Fprintf(w, `{"id": %q, "name": "Gorillaz", "sort-name": "Gorillaz", "type": "Group", "country": "GB"}`, gorillaz) + case r.URL.Path == "/artist/"+deLaSoul: + _, _ = fmt.Fprintf(w, `{"id": %q, "name": "De La Soul", "type": "Group", "country": "US"}`, deLaSoul) + case r.URL.Path == "/release-group" && q.Get("artist") == gorillaz: + _, _ = fmt.Fprintf(w, `{"release-group-count": 2, "release-groups": [ + {"id": %q, "title": "Demon Days", "primary-type": "Album", "first-release-date": "2005-05-11", + "artist-credit": [{"name": "Gorillaz", "artist": {"id": %q}}]}, + {"id": %q, "title": "Feel Good Inc.", "primary-type": "Single", "secondary-types": [], "first-release-date": "2005-05-09", + "artist-credit": [{"name": "Gorillaz", "joinphrase": " feat. ", "artist": {"id": %q}}, {"name": "De La Soul", "artist": {"id": %q}}]} + ]}`, demonDays, gorillaz, feelGood, gorillaz, deLaSoul) + case r.URL.Path == "/release-group" && q.Get("artist") == deLaSoul: + _, _ = fmt.Fprintf(w, `{"release-group-count": 1, "release-groups": [ + {"id": %q, "title": "Feel Good Inc.", "primary-type": "Single", "first-release-date": "2005-05-09"} + ]}`, feelGood) + case r.URL.Path == "/release-group/"+demonDays: + _, _ = fmt.Fprintf(w, `{"id": %q, "title": "Demon Days", "primary-type": "Album", "first-release-date": "2005-05-11", + "artist-credit": [{"name": "Gorillaz", "artist": {"id": %q}}]}`, demonDays, gorillaz) + case r.URL.Path == "/release" && q.Get("release-group") == demonDays: + _, _ = fmt.Fprintf(w, `{"release-count": 2, "releases": [ + {"id": %q, "title": "Demon Days", "status": "Promotion", "date": "2005-04", "country": "JP", + "media": [{"position": 1, "format": "CD", "tracks": [ + {"position": 1, "number": "1", "title": "Intro", "length": 63000, "recording": {"id": "rec-intro"}}]}]}, + {"id": %q, "title": "Demon Days", "status": "Official", "date": "2005-05-11", "country": "GB", + "media": [{"position": 1, "format": "CD", "tracks": [ + {"position": 1, "number": "1", "title": "Intro", "length": 63000, "recording": {"id": "rec-intro"}}, + {"position": 2, "number": "2", "title": "Last Living Souls", "length": 190000, "recording": {"id": "rec-lls"}}]}]} + ]}`, demonDaysJ, demonDaysR) + default: + w.WriteHeader(http.StatusNotFound) + } + })) + t.Cleanup(f.srv.Close) + return f +} + +func (f *fakeMB) client() *musicbrainz.Client { + return &musicbrainz.Client{BaseURL: f.srv.URL, Pace: &musicbrainz.Pacer{}, Retries: -1} +} + +func TestDiscography_ReadsOnceThenServesTheHeldCopy_Integration(t *testing.T) { + ctx := context.Background() + mb := newFakeMB(t) + s := New(newPool(t), mb.client(), nil) + + d, err := s.Discography(ctx, gorillaz) + if err != nil { + t.Fatal(err) + } + if d.Artist.Name != "Gorillaz" || d.Artist.Country != "GB" || len(d.Groups) != 2 || d.Stale { + t.Fatalf("discography = %+v", d) + } + // Oldest first: the single came out two days before the album. + if d.Groups[0].Mbid != feelGood || d.Groups[1].PrimaryType != "Album" { + t.Errorf("order = %s, %s", d.Groups[0].Title, d.Groups[1].Title) + } + if d.Groups[0].ArtistCredit != "Gorillaz feat. De La Soul" { + t.Errorf("credit = %q", d.Groups[0].ArtistCredit) + } + + before := mb.calls.Load() + if _, err := s.Discography(ctx, gorillaz); err != nil { + t.Fatal(err) + } + if mb.calls.Load() != before { + t.Error("a fresh discography should be served without asking MusicBrainz") + } + + // The collaboration belongs to both artists. + dls, err := s.Discography(ctx, deLaSoul) + if err != nil { + t.Fatal(err) + } + if len(dls.Groups) != 1 || dls.Groups[0].Mbid != feelGood { + t.Errorf("De La Soul's discography = %+v", dls.Groups) + } + if again, _ := s.Discography(ctx, gorillaz); len(again.Groups) != 2 { + t.Error("refreshing one artist must not take a shared group from the other") + } +} + +func TestDiscography_UnreachableServesStaleOrFails_Integration(t *testing.T) { + ctx := context.Background() + mb := newFakeMB(t) + pool := newPool(t) + s := New(pool, mb.client(), nil) + + mb.down.Store(true) + if _, err := s.Discography(ctx, gorillaz); !errors.Is(err, musicbrainz.ErrUnreachable) { + t.Fatalf("nothing held and MusicBrainz down: err = %v, want ErrUnreachable", err) + } + + mb.down.Store(false) + if _, err := s.Discography(ctx, gorillaz); err != nil { + t.Fatal(err) + } + // A week and a day later, MusicBrainz is down: the held copy is served. + s.now = func() time.Time { return time.Now().Add(DiscographyMaxAge + 24*time.Hour) } + mb.down.Store(true) + d, err := s.Discography(ctx, gorillaz) + if err != nil || !d.Stale || len(d.Groups) != 2 { + t.Fatalf("stale copy: %+v, %v", d, err) + } +} + +func TestDiscography_UnknownArtistIsNotFound_Integration(t *testing.T) { + s := New(newPool(t), newFakeMB(t).client(), nil) + if _, err := s.Discography(context.Background(), "00000000-0000-4000-8000-000000000000"); !errors.Is(err, ErrNotFound) { + t.Fatalf("err = %v, want ErrNotFound", err) + } +} + +func TestReleases_ReadsEditionsAndTrackLists_Integration(t *testing.T) { + ctx := context.Background() + mb := newFakeMB(t) + s := New(newPool(t), mb.client(), nil) + + // The group isn't held yet: it is looked up first. + e, err := s.Releases(ctx, demonDays) + if err != nil { + t.Fatal(err) + } + if e.Group.Title != "Demon Days" || len(e.Editions) != 2 { + t.Fatalf("editions = %+v", e) + } + official := e.Editions[0] + if official.Release.Mbid != demonDaysR || official.Release.TrackCount != 2 || official.Release.Formats != "CD" { + t.Errorf("the official edition should come first: %+v", official.Release) + } + if len(official.Tracks) != 2 || official.Tracks[1].RecordingMbid != "rec-lls" || official.Tracks[1].LengthMs != 190000 { + t.Errorf("tracks = %+v", official.Tracks) + } + + before := mb.calls.Load() + if _, err := s.Releases(ctx, demonDays); err != nil { + t.Fatal(err) + } + if mb.calls.Load() != before { + t.Error("fresh editions should be served without asking MusicBrainz") + } + // Refreshing replaces rather than adds. + if err := s.RefreshReleaseGroup(ctx, demonDays); err != nil { + t.Fatal(err) + } + if again, _ := s.Releases(ctx, demonDays); len(again.Editions) != 2 || len(again.Editions[0].Tracks) != 2 { + t.Errorf("after a refresh: %+v", again) + } +} + +func TestRefreshDue_ReadsWhatIsOldAndStopsWhenMusicBrainzIsDown_Integration(t *testing.T) { + ctx := context.Background() + mb := newFakeMB(t) + s := New(newPool(t), mb.client(), nil) + for _, a := range []string{gorillaz, deLaSoul} { + if err := s.RefreshArtist(ctx, a); err != nil { + t.Fatal(err) + } + } + if n, err := s.RefreshDue(ctx, 10); err != nil || n != 0 { + t.Fatalf("nothing is due yet: %d, %v", n, err) + } + + s.now = func() time.Time { return time.Now().Add(DiscographyMaxAge + time.Hour) } + if n, err := s.RefreshDue(ctx, 1); err != nil || n != 1 { + t.Errorf("one due, limit one: %d, %v", n, err) + } + mb.down.Store(true) + n, err := s.RefreshDue(ctx, 10) + if !errors.Is(err, musicbrainz.ErrUnreachable) || n != 0 { + t.Errorf("MusicBrainz down: %d, %v", n, err) + } + if !strings.Contains(err.Error(), "503") { + t.Errorf("the error should say what MusicBrainz answered: %v", err) + } +} diff --git a/internal/coverart/provider.go b/internal/coverart/provider.go index 92303ece..122f02dd 100644 --- a/internal/coverart/provider.go +++ b/internal/coverart/provider.go @@ -82,6 +82,14 @@ type AlbumCoverProvider interface { FetchAlbumCover(ctx context.Context, ref AlbumRef) ([]byte, error) } +// ReleaseGroupCoverProvider is an opt-in capability for sources that can +// answer by MusicBrainz release group, which is how the release catalogue +// names an album before any edition of it is held (M502 #5519). +type ReleaseGroupCoverProvider interface { + AlbumCoverProvider + FetchReleaseGroupCover(ctx context.Context, groupMBID string) ([]byte, error) +} + // ArtistArtProvider is an opt-in capability: providers implementing // it can fetch artist images. Returns thumb + fanart atomically at // the JSON metadata step (one round-trip yields both URLs); the two diff --git a/internal/coverart/provider_mbcaa.go b/internal/coverart/provider_mbcaa.go index a448555d..35975f5d 100644 --- a/internal/coverart/provider_mbcaa.go +++ b/internal/coverart/provider_mbcaa.go @@ -92,10 +92,22 @@ func (p *mbcaaProvider) Configure(s ProviderSettings) error { // release MBID. MBCAA is MBID-only; returns ErrNotFound when // ref.MBID is empty. Returns ErrNotFound on 404 or ErrTransient otherwise. func (p *mbcaaProvider) FetchAlbumCover(ctx context.Context, ref AlbumRef) ([]byte, error) { + return p.fetchFront(ctx, "release", ref.MBID) +} + +// FetchReleaseGroupCover retrieves the 500px front cover the Cover Art +// Archive chose for a release group: what a search result or a wanted album +// shows before any edition of it is held (M502 #5519). +func (p *mbcaaProvider) FetchReleaseGroupCover(ctx context.Context, groupMBID string) ([]byte, error) { + return p.fetchFront(ctx, "release-group", groupMBID) +} + +// fetchFront fetches /{entity}/{mbid}/front-500, honouring the rate limit. +func (p *mbcaaProvider) fetchFront(ctx context.Context, entity, mbid string) ([]byte, error) { if !p.enabled.Load() { return nil, ErrNotFound // defensive; enricher already filters on enabled } - if ref.MBID == "" { + if mbid == "" { return nil, ErrNotFound // MBCAA is MBID-only } @@ -119,7 +131,7 @@ func (p *mbcaaProvider) FetchAlbumCover(ctx context.Context, ref AlbumRef) ([]by p.lastCall = time.Now() p.mu.Unlock() - url := fmt.Sprintf("%s/release/%s/front-500", cfg.BaseURL, ref.MBID) + url := fmt.Sprintf("%s/%s/%s/front-500", cfg.BaseURL, entity, mbid) req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) if err != nil { return nil, fmt.Errorf("coverart: build request: %w", err) @@ -157,3 +169,5 @@ func (p *mbcaaProvider) TestConnection(ctx context.Context) error { } return err } + +var _ ReleaseGroupCoverProvider = (*mbcaaProvider)(nil) diff --git a/internal/coverart/provider_mbcaa_test.go b/internal/coverart/provider_mbcaa_test.go index b837617e..18090712 100644 --- a/internal/coverart/provider_mbcaa_test.go +++ b/internal/coverart/provider_mbcaa_test.go @@ -166,3 +166,24 @@ func TestNewMBCAAProviderFromConfig_ReplacesRegisteredConfig(t *testing.T) { t.Errorf("cfg.UserAgent = %q, want %q", gotUA, "Minstrel/test-replaced (x)") } } + +func TestMBCAAProvider_FetchReleaseGroupCover_AsksByGroup(t *testing.T) { + var path string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + path = r.URL.Path + _, _ = w.Write([]byte("jpeg")) + })) + defer srv.Close() + p := newMBCAAProviderForTest(t, srv.URL) + + body, err := p.FetchReleaseGroupCover(context.Background(), "rg-1") + if err != nil || string(body) != "jpeg" { + t.Fatalf("body %q, err %v", body, err) + } + if path != "/release-group/rg-1/front-500" { + t.Errorf("path = %q", path) + } + if _, err := p.FetchReleaseGroupCover(context.Background(), ""); !errors.Is(err, ErrNotFound) { + t.Errorf("no id: err = %v, want ErrNotFound", err) + } +} diff --git a/internal/db/dbq/catalogue.sql.go b/internal/db/dbq/catalogue.sql.go new file mode 100644 index 00000000..d18a4261 --- /dev/null +++ b/internal/db/dbq/catalogue.sql.go @@ -0,0 +1,402 @@ +// Code generated by sqlc. DO NOT EDIT. +// versions: +// sqlc v1.31.1 +// source: catalogue.sql + +package dbq + +import ( + "context" + + "github.com/jackc/pgx/v5/pgtype" +) + +const addMBArtistReleaseGroup = `-- name: AddMBArtistReleaseGroup :exec +INSERT INTO mb_artist_release_groups (artist_mbid, release_group_mbid) +VALUES ($1, $2) +ON CONFLICT DO NOTHING +` + +type AddMBArtistReleaseGroupParams struct { + ArtistMbid string + ReleaseGroupMbid string +} + +func (q *Queries) AddMBArtistReleaseGroup(ctx context.Context, arg AddMBArtistReleaseGroupParams) error { + _, err := q.db.Exec(ctx, addMBArtistReleaseGroup, arg.ArtistMbid, arg.ReleaseGroupMbid) + return err +} + +const deleteMBArtistReleaseGroups = `-- name: DeleteMBArtistReleaseGroups :exec +DELETE FROM mb_artist_release_groups WHERE artist_mbid = $1 +` + +func (q *Queries) DeleteMBArtistReleaseGroups(ctx context.Context, artistMbid string) error { + _, err := q.db.Exec(ctx, deleteMBArtistReleaseGroups, artistMbid) + return err +} + +const deleteMBReleasesForGroup = `-- name: DeleteMBReleasesForGroup :exec +DELETE FROM mb_releases WHERE release_group_mbid = $1 +` + +// Tracks go with their releases (ON DELETE CASCADE). +func (q *Queries) DeleteMBReleasesForGroup(ctx context.Context, releaseGroupMbid string) error { + _, err := q.db.Exec(ctx, deleteMBReleasesForGroup, releaseGroupMbid) + return err +} + +const getMBArtist = `-- name: GetMBArtist :one +SELECT mbid, name, sort_name, disambiguation, artist_type, country, discography_refreshed_at, created_at, updated_at FROM mb_artists WHERE mbid = $1 +` + +func (q *Queries) GetMBArtist(ctx context.Context, mbid string) (MbArtist, error) { + row := q.db.QueryRow(ctx, getMBArtist, mbid) + var i MbArtist + err := row.Scan( + &i.Mbid, + &i.Name, + &i.SortName, + &i.Disambiguation, + &i.ArtistType, + &i.Country, + &i.DiscographyRefreshedAt, + &i.CreatedAt, + &i.UpdatedAt, + ) + return i, err +} + +const getMBReleaseGroup = `-- name: GetMBReleaseGroup :one +SELECT mbid, title, primary_type, secondary_types, first_release_date, artist_credit, releases_refreshed_at, created_at, updated_at FROM mb_release_groups WHERE mbid = $1 +` + +func (q *Queries) GetMBReleaseGroup(ctx context.Context, mbid string) (MbReleaseGroup, error) { + row := q.db.QueryRow(ctx, getMBReleaseGroup, mbid) + var i MbReleaseGroup + err := row.Scan( + &i.Mbid, + &i.Title, + &i.PrimaryType, + &i.SecondaryTypes, + &i.FirstReleaseDate, + &i.ArtistCredit, + &i.ReleasesRefreshedAt, + &i.CreatedAt, + &i.UpdatedAt, + ) + return i, err +} + +const insertMBRelease = `-- name: InsertMBRelease :exec +INSERT INTO mb_releases (mbid, release_group_mbid, title, status, release_date, country, + disambiguation, formats, track_count) +VALUES ($1, $2, $3, $4, + $5, $6, $7, $8, + $9) +ON CONFLICT (mbid) DO UPDATE SET + release_group_mbid = EXCLUDED.release_group_mbid, + title = EXCLUDED.title, + status = EXCLUDED.status, + release_date = EXCLUDED.release_date, + country = EXCLUDED.country, + disambiguation = EXCLUDED.disambiguation, + formats = EXCLUDED.formats, + track_count = EXCLUDED.track_count +` + +type InsertMBReleaseParams struct { + Mbid string + ReleaseGroupMbid string + Title string + Status string + ReleaseDate string + Country string + Disambiguation string + Formats string + TrackCount int32 +} + +func (q *Queries) InsertMBRelease(ctx context.Context, arg InsertMBReleaseParams) error { + _, err := q.db.Exec(ctx, insertMBRelease, + arg.Mbid, + arg.ReleaseGroupMbid, + arg.Title, + arg.Status, + arg.ReleaseDate, + arg.Country, + arg.Disambiguation, + arg.Formats, + arg.TrackCount, + ) + return err +} + +const insertMBReleaseTrack = `-- name: InsertMBReleaseTrack :exec +INSERT INTO mb_release_tracks (release_mbid, medium_position, position, medium_format, number, + title, length_ms, recording_mbid) +VALUES ($1, $2, $3, + $4, $5, $6, $7, + $8) +` + +type InsertMBReleaseTrackParams struct { + ReleaseMbid string + MediumPosition int32 + Position int32 + MediumFormat string + Number string + Title string + LengthMs int32 + RecordingMbid string +} + +func (q *Queries) InsertMBReleaseTrack(ctx context.Context, arg InsertMBReleaseTrackParams) error { + _, err := q.db.Exec(ctx, insertMBReleaseTrack, + arg.ReleaseMbid, + arg.MediumPosition, + arg.Position, + arg.MediumFormat, + arg.Number, + arg.Title, + arg.LengthMs, + arg.RecordingMbid, + ) + return err +} + +const listMBArtistReleaseGroups = `-- name: ListMBArtistReleaseGroups :many +SELECT g.mbid, g.title, g.primary_type, g.secondary_types, g.first_release_date, g.artist_credit, g.releases_refreshed_at, g.created_at, g.updated_at FROM mb_release_groups g +JOIN mb_artist_release_groups ag ON ag.release_group_mbid = g.mbid +WHERE ag.artist_mbid = $1 +ORDER BY g.first_release_date = '', g.first_release_date, g.title, g.mbid +` + +// An artist's discography, oldest first; undated groups last. +func (q *Queries) ListMBArtistReleaseGroups(ctx context.Context, artistMbid string) ([]MbReleaseGroup, error) { + rows, err := q.db.Query(ctx, listMBArtistReleaseGroups, artistMbid) + if err != nil { + return nil, err + } + defer rows.Close() + var items []MbReleaseGroup + for rows.Next() { + var i MbReleaseGroup + if err := rows.Scan( + &i.Mbid, + &i.Title, + &i.PrimaryType, + &i.SecondaryTypes, + &i.FirstReleaseDate, + &i.ArtistCredit, + &i.ReleasesRefreshedAt, + &i.CreatedAt, + &i.UpdatedAt, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + +const listMBArtistsDueForRefresh = `-- name: ListMBArtistsDueForRefresh :many +SELECT mbid FROM mb_artists +WHERE discography_refreshed_at IS NULL OR discography_refreshed_at < $1 +ORDER BY discography_refreshed_at NULLS FIRST, mbid +LIMIT $2 +` + +type ListMBArtistsDueForRefreshParams struct { + Before pgtype.Timestamptz + MaxRows int32 +} + +// Artists whose discography has never been read, or was read before the +// cutoff: never-read first, then oldest first. +func (q *Queries) ListMBArtistsDueForRefresh(ctx context.Context, arg ListMBArtistsDueForRefreshParams) ([]string, error) { + rows, err := q.db.Query(ctx, listMBArtistsDueForRefresh, arg.Before, arg.MaxRows) + if err != nil { + return nil, err + } + defer rows.Close() + var items []string + for rows.Next() { + var mbid string + if err := rows.Scan(&mbid); err != nil { + return nil, err + } + items = append(items, mbid) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + +const listMBReleaseTracksForGroup = `-- name: ListMBReleaseTracksForGroup :many +SELECT t.release_mbid, t.medium_position, t.position, t.medium_format, t.number, t.title, t.length_ms, t.recording_mbid FROM mb_release_tracks t +JOIN mb_releases r ON r.mbid = t.release_mbid +WHERE r.release_group_mbid = $1 +ORDER BY t.release_mbid, t.medium_position, t.position +` + +func (q *Queries) ListMBReleaseTracksForGroup(ctx context.Context, releaseGroupMbid string) ([]MbReleaseTrack, error) { + rows, err := q.db.Query(ctx, listMBReleaseTracksForGroup, releaseGroupMbid) + if err != nil { + return nil, err + } + defer rows.Close() + var items []MbReleaseTrack + for rows.Next() { + var i MbReleaseTrack + if err := rows.Scan( + &i.ReleaseMbid, + &i.MediumPosition, + &i.Position, + &i.MediumFormat, + &i.Number, + &i.Title, + &i.LengthMs, + &i.RecordingMbid, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + +const listMBReleasesForGroup = `-- name: ListMBReleasesForGroup :many +SELECT mbid, release_group_mbid, title, status, release_date, country, disambiguation, formats, track_count, created_at FROM mb_releases +WHERE release_group_mbid = $1 +ORDER BY status <> 'Official', release_date = '', release_date, title, mbid +` + +// Official editions first, then by date; undated last. +func (q *Queries) ListMBReleasesForGroup(ctx context.Context, releaseGroupMbid string) ([]MbRelease, error) { + rows, err := q.db.Query(ctx, listMBReleasesForGroup, releaseGroupMbid) + if err != nil { + return nil, err + } + defer rows.Close() + var items []MbRelease + for rows.Next() { + var i MbRelease + if err := rows.Scan( + &i.Mbid, + &i.ReleaseGroupMbid, + &i.Title, + &i.Status, + &i.ReleaseDate, + &i.Country, + &i.Disambiguation, + &i.Formats, + &i.TrackCount, + &i.CreatedAt, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + +const markMBArtistDiscographyRefreshed = `-- name: MarkMBArtistDiscographyRefreshed :exec +UPDATE mb_artists SET discography_refreshed_at = now() WHERE mbid = $1 +` + +func (q *Queries) MarkMBArtistDiscographyRefreshed(ctx context.Context, mbid string) error { + _, err := q.db.Exec(ctx, markMBArtistDiscographyRefreshed, mbid) + return err +} + +const markMBReleaseGroupReleasesRefreshed = `-- name: MarkMBReleaseGroupReleasesRefreshed :exec +UPDATE mb_release_groups SET releases_refreshed_at = now() WHERE mbid = $1 +` + +func (q *Queries) MarkMBReleaseGroupReleasesRefreshed(ctx context.Context, mbid string) error { + _, err := q.db.Exec(ctx, markMBReleaseGroupReleasesRefreshed, mbid) + return err +} + +const upsertMBArtist = `-- name: UpsertMBArtist :exec + +INSERT INTO mb_artists (mbid, name, sort_name, disambiguation, artist_type, country) +VALUES ($1, $2, $3, $4, + $5, $6) +ON CONFLICT (mbid) DO UPDATE SET + name = EXCLUDED.name, + sort_name = EXCLUDED.sort_name, + disambiguation = EXCLUDED.disambiguation, + artist_type = EXCLUDED.artist_type, + country = EXCLUDED.country, + updated_at = now() +` + +type UpsertMBArtistParams struct { + Mbid string + Name string + SortName string + Disambiguation string + ArtistType string + Country string +} + +// The release catalogue read from MusicBrainz (M502 #5519). +func (q *Queries) UpsertMBArtist(ctx context.Context, arg UpsertMBArtistParams) error { + _, err := q.db.Exec(ctx, upsertMBArtist, + arg.Mbid, + arg.Name, + arg.SortName, + arg.Disambiguation, + arg.ArtistType, + arg.Country, + ) + return err +} + +const upsertMBReleaseGroup = `-- name: UpsertMBReleaseGroup :exec +INSERT INTO mb_release_groups (mbid, title, primary_type, secondary_types, first_release_date, artist_credit) +VALUES ($1, $2, $3, $4::text[], + $5, $6) +ON CONFLICT (mbid) DO UPDATE SET + title = EXCLUDED.title, + primary_type = EXCLUDED.primary_type, + secondary_types = EXCLUDED.secondary_types, + first_release_date = EXCLUDED.first_release_date, + artist_credit = EXCLUDED.artist_credit, + updated_at = now() +` + +type UpsertMBReleaseGroupParams struct { + Mbid string + Title string + PrimaryType string + SecondaryTypes []string + FirstReleaseDate string + ArtistCredit string +} + +// Leaves releases_refreshed_at alone: re-reading a discography says nothing +// about whether the group's track lists are current. +func (q *Queries) UpsertMBReleaseGroup(ctx context.Context, arg UpsertMBReleaseGroupParams) error { + _, err := q.db.Exec(ctx, upsertMBReleaseGroup, + arg.Mbid, + arg.Title, + arg.PrimaryType, + arg.SecondaryTypes, + arg.FirstReleaseDate, + arg.ArtistCredit, + ) + return err +} diff --git a/internal/db/dbq/models.go b/internal/db/dbq/models.go index 64c066c6..e34dc4bb 100644 --- a/internal/db/dbq/models.go +++ b/internal/db/dbq/models.go @@ -482,6 +482,59 @@ type LoudnessSetting struct { LeveledCacheMb int32 } +type MbArtist struct { + Mbid string + Name string + SortName string + Disambiguation string + ArtistType string + Country string + DiscographyRefreshedAt pgtype.Timestamptz + CreatedAt pgtype.Timestamptz + UpdatedAt pgtype.Timestamptz +} + +type MbArtistReleaseGroup struct { + ArtistMbid string + ReleaseGroupMbid string +} + +type MbRelease struct { + Mbid string + ReleaseGroupMbid string + Title string + Status string + ReleaseDate string + Country string + Disambiguation string + Formats string + TrackCount int32 + CreatedAt pgtype.Timestamptz +} + +type MbReleaseGroup struct { + Mbid string + Title string + PrimaryType string + SecondaryTypes []string + FirstReleaseDate string + ArtistCredit string + ReleasesRefreshedAt pgtype.Timestamptz + CreatedAt pgtype.Timestamptz + UpdatedAt pgtype.Timestamptz +} + +type MbReleaseTrack struct { + ReleaseMbid string + MediumPosition int32 + Position int32 + MediumFormat string + Number string + Title string + LengthMs int32 + RecordingMbid string +} + type MissingReacquisition struct { AlbumID pgtype.UUID Attempts int32 diff --git a/internal/db/migrations/0080_release_catalogue.down.sql b/internal/db/migrations/0080_release_catalogue.down.sql new file mode 100644 index 00000000..bb9e6ac3 --- /dev/null +++ b/internal/db/migrations/0080_release_catalogue.down.sql @@ -0,0 +1,5 @@ +DROP TABLE IF EXISTS mb_release_tracks; +DROP TABLE IF EXISTS mb_releases; +DROP TABLE IF EXISTS mb_artist_release_groups; +DROP TABLE IF EXISTS mb_release_groups; +DROP TABLE IF EXISTS mb_artists; diff --git a/internal/db/migrations/0080_release_catalogue.up.sql b/internal/db/migrations/0080_release_catalogue.up.sql new file mode 100644 index 00000000..7f010aea --- /dev/null +++ b/internal/db/migrations/0080_release_catalogue.up.sql @@ -0,0 +1,86 @@ +-- 0080_release_catalogue.up.sql — Minstrel's own copy of what artists have +-- released, read from MusicBrainz (Scribe milestone #502, #5519). +-- +-- With Lidarr gone, the wanted list, search and import need to know an +-- artist's discography and each release's track list. MusicBrainz allows one +-- request a second, so what has been read is kept here and refreshed on a +-- schedule rather than asked again for every page. +-- +-- mb_artists artists the catalogue has been asked about. +-- mb_release_groups what a person calls an album (Lidarr's albums +-- were release groups too), with MusicBrainz's +-- primary and secondary types. +-- mb_artist_release_groups who is credited on each group: a collaboration +-- belongs to every credited artist's discography. +-- mb_releases the editions of a group, read when something +-- needs a track list. +-- mb_release_tracks each release's track list. +-- +-- Dates stay as MusicBrainz's partial-date text ("1997", "1997-05", +-- "1997-05-21"): most early releases have only a year. + +CREATE TABLE mb_artists ( + mbid text PRIMARY KEY, + name text NOT NULL, + sort_name text NOT NULL DEFAULT '', + disambiguation text NOT NULL DEFAULT '', + artist_type text NOT NULL DEFAULT '', + country text NOT NULL DEFAULT '', + -- When the discography was last read in full; NULL means never. + discography_refreshed_at timestamptz, + created_at timestamptz NOT NULL DEFAULT now(), + updated_at timestamptz NOT NULL DEFAULT now() +); + +CREATE INDEX mb_artists_refresh_idx ON mb_artists (discography_refreshed_at NULLS FIRST); + +CREATE TABLE mb_release_groups ( + mbid text PRIMARY KEY, + title text NOT NULL, + primary_type text NOT NULL DEFAULT '', + secondary_types text[] NOT NULL DEFAULT '{}', + first_release_date text NOT NULL DEFAULT '', + artist_credit text NOT NULL DEFAULT '', + -- When the releases and their track lists were last read; NULL means never. + releases_refreshed_at timestamptz, + created_at timestamptz NOT NULL DEFAULT now(), + updated_at timestamptz NOT NULL DEFAULT now() +); + +CREATE TABLE mb_artist_release_groups ( + artist_mbid text NOT NULL REFERENCES mb_artists (mbid) ON DELETE CASCADE, + release_group_mbid text NOT NULL REFERENCES mb_release_groups (mbid) ON DELETE CASCADE, + PRIMARY KEY (artist_mbid, release_group_mbid) +); + +CREATE INDEX mb_artist_release_groups_group_idx ON mb_artist_release_groups (release_group_mbid); + +CREATE TABLE mb_releases ( + mbid text PRIMARY KEY, + release_group_mbid text NOT NULL REFERENCES mb_release_groups (mbid) ON DELETE CASCADE, + title text NOT NULL, + status text NOT NULL DEFAULT '', + release_date text NOT NULL DEFAULT '', + country text NOT NULL DEFAULT '', + disambiguation text NOT NULL DEFAULT '', + formats text NOT NULL DEFAULT '', + track_count integer NOT NULL DEFAULT 0, + created_at timestamptz NOT NULL DEFAULT now() +); + +CREATE INDEX mb_releases_group_idx ON mb_releases (release_group_mbid); + +CREATE TABLE mb_release_tracks ( + release_mbid text NOT NULL REFERENCES mb_releases (mbid) ON DELETE CASCADE, + medium_position integer NOT NULL, + position integer NOT NULL, + medium_format text NOT NULL DEFAULT '', + number text NOT NULL DEFAULT '', + title text NOT NULL, + length_ms integer NOT NULL DEFAULT 0, + recording_mbid text NOT NULL DEFAULT '', + PRIMARY KEY (release_mbid, medium_position, position) +); + +CREATE INDEX mb_release_tracks_recording_idx ON mb_release_tracks (recording_mbid) + WHERE recording_mbid <> ''; diff --git a/internal/db/queries/catalogue.sql b/internal/db/queries/catalogue.sql new file mode 100644 index 00000000..6518e5d3 --- /dev/null +++ b/internal/db/queries/catalogue.sql @@ -0,0 +1,101 @@ +-- The release catalogue read from MusicBrainz (M502 #5519). + +-- name: UpsertMBArtist :exec +INSERT INTO mb_artists (mbid, name, sort_name, disambiguation, artist_type, country) +VALUES (sqlc.arg(mbid), sqlc.arg(name), sqlc.arg(sort_name), sqlc.arg(disambiguation), + sqlc.arg(artist_type), sqlc.arg(country)) +ON CONFLICT (mbid) DO UPDATE SET + name = EXCLUDED.name, + sort_name = EXCLUDED.sort_name, + disambiguation = EXCLUDED.disambiguation, + artist_type = EXCLUDED.artist_type, + country = EXCLUDED.country, + updated_at = now(); + +-- name: GetMBArtist :one +SELECT * FROM mb_artists WHERE mbid = sqlc.arg(mbid); + +-- name: MarkMBArtistDiscographyRefreshed :exec +UPDATE mb_artists SET discography_refreshed_at = now() WHERE mbid = sqlc.arg(mbid); + +-- name: ListMBArtistsDueForRefresh :many +-- Artists whose discography has never been read, or was read before the +-- cutoff: never-read first, then oldest first. +SELECT mbid FROM mb_artists +WHERE discography_refreshed_at IS NULL OR discography_refreshed_at < sqlc.arg(before) +ORDER BY discography_refreshed_at NULLS FIRST, mbid +LIMIT sqlc.arg(max_rows); + +-- name: UpsertMBReleaseGroup :exec +-- Leaves releases_refreshed_at alone: re-reading a discography says nothing +-- about whether the group's track lists are current. +INSERT INTO mb_release_groups (mbid, title, primary_type, secondary_types, first_release_date, artist_credit) +VALUES (sqlc.arg(mbid), sqlc.arg(title), sqlc.arg(primary_type), sqlc.arg(secondary_types)::text[], + sqlc.arg(first_release_date), sqlc.arg(artist_credit)) +ON CONFLICT (mbid) DO UPDATE SET + title = EXCLUDED.title, + primary_type = EXCLUDED.primary_type, + secondary_types = EXCLUDED.secondary_types, + first_release_date = EXCLUDED.first_release_date, + artist_credit = EXCLUDED.artist_credit, + updated_at = now(); + +-- name: GetMBReleaseGroup :one +SELECT * FROM mb_release_groups WHERE mbid = sqlc.arg(mbid); + +-- name: DeleteMBArtistReleaseGroups :exec +DELETE FROM mb_artist_release_groups WHERE artist_mbid = sqlc.arg(artist_mbid); + +-- name: AddMBArtistReleaseGroup :exec +INSERT INTO mb_artist_release_groups (artist_mbid, release_group_mbid) +VALUES (sqlc.arg(artist_mbid), sqlc.arg(release_group_mbid)) +ON CONFLICT DO NOTHING; + +-- name: ListMBArtistReleaseGroups :many +-- An artist's discography, oldest first; undated groups last. +SELECT g.* FROM mb_release_groups g +JOIN mb_artist_release_groups ag ON ag.release_group_mbid = g.mbid +WHERE ag.artist_mbid = sqlc.arg(artist_mbid) +ORDER BY g.first_release_date = '', g.first_release_date, g.title, g.mbid; + +-- name: MarkMBReleaseGroupReleasesRefreshed :exec +UPDATE mb_release_groups SET releases_refreshed_at = now() WHERE mbid = sqlc.arg(mbid); + +-- name: DeleteMBReleasesForGroup :exec +-- Tracks go with their releases (ON DELETE CASCADE). +DELETE FROM mb_releases WHERE release_group_mbid = sqlc.arg(release_group_mbid); + +-- name: InsertMBRelease :exec +INSERT INTO mb_releases (mbid, release_group_mbid, title, status, release_date, country, + disambiguation, formats, track_count) +VALUES (sqlc.arg(mbid), sqlc.arg(release_group_mbid), sqlc.arg(title), sqlc.arg(status), + sqlc.arg(release_date), sqlc.arg(country), sqlc.arg(disambiguation), sqlc.arg(formats), + sqlc.arg(track_count)) +ON CONFLICT (mbid) DO UPDATE SET + release_group_mbid = EXCLUDED.release_group_mbid, + title = EXCLUDED.title, + status = EXCLUDED.status, + release_date = EXCLUDED.release_date, + country = EXCLUDED.country, + disambiguation = EXCLUDED.disambiguation, + formats = EXCLUDED.formats, + track_count = EXCLUDED.track_count; + +-- name: InsertMBReleaseTrack :exec +INSERT INTO mb_release_tracks (release_mbid, medium_position, position, medium_format, number, + title, length_ms, recording_mbid) +VALUES (sqlc.arg(release_mbid), sqlc.arg(medium_position), sqlc.arg(position), + sqlc.arg(medium_format), sqlc.arg(number), sqlc.arg(title), sqlc.arg(length_ms), + sqlc.arg(recording_mbid)); + +-- name: ListMBReleasesForGroup :many +-- Official editions first, then by date; undated last. +SELECT * FROM mb_releases +WHERE release_group_mbid = sqlc.arg(release_group_mbid) +ORDER BY status <> 'Official', release_date = '', release_date, title, mbid; + +-- name: ListMBReleaseTracksForGroup :many +SELECT t.* FROM mb_release_tracks t +JOIN mb_releases r ON r.mbid = t.release_mbid +WHERE r.release_group_mbid = sqlc.arg(release_group_mbid) +ORDER BY t.release_mbid, t.medium_position, t.position; diff --git a/internal/dbtest/reset.go b/internal/dbtest/reset.go index 7824ccf0..b22504a1 100644 --- a/internal/dbtest/reset.go +++ b/internal/dbtest/reset.go @@ -103,6 +103,11 @@ var dataTables = []string{ "user_notification_prefs", "user_notification_email_state", // #5346 "acquisition_services", // M502 #5518 + "mb_release_tracks", // M502 #5519 + "mb_releases", // M502 #5519 + "mb_artist_release_groups", // M502 #5519 + "mb_release_groups", // M502 #5519 + "mb_artists", // M502 #5519 "tracks", "albums", "artists", diff --git a/internal/musicbrainz/client.go b/internal/musicbrainz/client.go new file mode 100644 index 00000000..2d906a76 --- /dev/null +++ b/internal/musicbrainz/client.go @@ -0,0 +1,208 @@ +package musicbrainz + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "io" + "net/http" + "net/url" + "strconv" + "strings" + "time" +) + +// DefaultBaseURL is the public MusicBrainz web service. +const DefaultBaseURL = "https://musicbrainz.org/ws/2" + +// UserAgent identifies Minstrel. MusicBrainz refuses requests without a +// descriptive one. +const UserAgent = "Minstrel/1.0 ( https://git.fabledsword.com/bvandeusen/minstrel )" + +// DefaultTimeout bounds one request (rule 156). A browse page with track +// lists is the slowest thing asked for. +const DefaultTimeout = 20 * time.Second + +// pageSize is the most a browse returns per page. +const pageSize = 100 + +// maxPages bounds a browse, so an artist with thousands of release groups +// cannot hold the pacer for an hour. 25 pages is 2,500 entries. +const maxPages = 25 + +var ( + // ErrUnreachable: MusicBrainz could not be asked, or kept answering with + // its throttle or a server error. Callers degrade visibly (rule 164). + ErrUnreachable = errors.New("musicbrainz: unreachable") + // ErrNotFound: MusicBrainz has nothing under that id. + ErrNotFound = errors.New("musicbrainz: not found") + // ErrBadResponse: MusicBrainz answered with something that is not the + // JSON asked for. + ErrBadResponse = errors.New("musicbrainz: unexpected response") +) + +// Client asks MusicBrainz. The zero value is usable: it talks to the public +// service through the shared pacer. +type Client struct { + BaseURL string + HTTP *http.Client + // Pace spaces requests; nil means Shared(). Tests pass a zero Pacer. + Pace *Pacer + // Retries is how many times a throttled or failed request is tried + // again. Zero means 2; negative means none. + Retries int + // Backoff is the first retry's wait, doubled each time. Zero means 1s. + Backoff time.Duration +} + +func (c *Client) base() string { + if c.BaseURL != "" { + return strings.TrimRight(c.BaseURL, "/") + } + return DefaultBaseURL +} + +func (c *Client) httpClient() *http.Client { + if c.HTTP != nil { + return c.HTTP + } + return &http.Client{Timeout: DefaultTimeout} +} + +func (c *Client) pacer() *Pacer { + if c.Pace != nil { + return c.Pace + } + return Shared() +} + +func (c *Client) retries() int { + switch { + case c.Retries < 0: + return 0 + case c.Retries == 0: + return 2 + } + return c.Retries +} + +func (c *Client) backoff(attempt int) time.Duration { + b := c.Backoff + if b <= 0 { + b = time.Second + } + return b << attempt +} + +// get fetches path?query (fmt=json added) into out, waiting on the pacer +// before every attempt. +func (c *Client) get(ctx context.Context, path string, query url.Values, out any) error { + if query == nil { + query = url.Values{} + } + query.Set("fmt", "json") + full := c.base() + path + "?" + query.Encode() + + var last error + for attempt := 0; attempt <= c.retries(); attempt++ { + if attempt > 0 { + if err := sleep(ctx, c.backoff(attempt-1)); err != nil { + return err + } + } + if err := c.pacer().Wait(ctx); err != nil { + return err + } + retry, err := c.once(ctx, full, out) + if !retry { + return err + } + last = err + } + return last +} + +// once makes one request. retry reports whether trying again could help. +func (c *Client) once(ctx context.Context, full string, out any) (retry bool, err error) { + req, err := http.NewRequestWithContext(ctx, http.MethodGet, full, nil) + if err != nil { + return false, fmt.Errorf("musicbrainz: build request: %w", err) + } + req.Header.Set("Accept", "application/json") + req.Header.Set("User-Agent", UserAgent) + + resp, err := c.httpClient().Do(req) + if err != nil { + if ctx.Err() != nil { + return false, ctx.Err() + } + return true, fmt.Errorf("%w: %v", ErrUnreachable, err) + } + defer func() { _ = resp.Body.Close() }() + + switch { + case resp.StatusCode == http.StatusNotFound: + return false, ErrNotFound + case resp.StatusCode == http.StatusServiceUnavailable, + resp.StatusCode == http.StatusTooManyRequests, + resp.StatusCode >= 500: + // 503 is how MusicBrainz says "slow down". + return true, fmt.Errorf("%w: HTTP %d", ErrUnreachable, resp.StatusCode) + case resp.StatusCode != http.StatusOK: + return false, fmt.Errorf("%w: HTTP %d", ErrBadResponse, resp.StatusCode) + } + body, err := io.ReadAll(io.LimitReader(resp.Body, 8<<20)) + if err != nil { + return true, fmt.Errorf("%w: read: %v", ErrUnreachable, err) + } + if err := json.Unmarshal(body, out); err != nil { + return false, fmt.Errorf("%w: %v", ErrBadResponse, err) + } + return false, nil +} + +func sleep(ctx context.Context, d time.Duration) error { + t := time.NewTimer(d) + defer t.Stop() + select { + case <-ctx.Done(): + return ctx.Err() + case <-t.C: + return nil + } +} + +// luceneSpecial are the characters MusicBrainz's search syntax gives meaning. +const luceneSpecial = `+-&|!(){}[]^"~*?:\/` + +// Escape makes text typed by a person safe to put in a search query, so +// "AC/DC" or "Sunn O)))" searches for what was typed rather than failing to +// parse. +func Escape(s string) string { + var b strings.Builder + for _, r := range s { + if strings.ContainsRune(luceneSpecial, r) { + b.WriteByte('\\') + } + b.WriteRune(r) + } + return b.String() +} + +// fieldQuery searches each field for the escaped text. +func fieldQuery(text string, fields ...string) string { + t := Escape(strings.TrimSpace(text)) + parts := make([]string, len(fields)) + for i, f := range fields { + parts[i] = f + ":(" + t + ")" + } + return strings.Join(parts, " OR ") +} + +func searchLimit(limit int) string { + if limit <= 0 || limit > 100 { + limit = 25 + } + return strconv.Itoa(limit) +} diff --git a/internal/musicbrainz/client_test.go b/internal/musicbrainz/client_test.go new file mode 100644 index 00000000..d645984f --- /dev/null +++ b/internal/musicbrainz/client_test.go @@ -0,0 +1,202 @@ +package musicbrainz + +import ( + "context" + "errors" + "fmt" + "net/http" + "net/http/httptest" + "strconv" + "sync/atomic" + "testing" + "time" +) + +// testClient talks to srv with no pacing and fast retries. +func testClient(srv *httptest.Server) *Client { + return &Client{BaseURL: srv.URL, Pace: &Pacer{}, Backoff: time.Millisecond} +} + +func TestEscape_KeepsWhatWasTyped(t *testing.T) { + for in, want := range map[string]string{ + "Radiohead": "Radiohead", + "AC/DC": `AC\/DC`, + "Sunn O)))": `Sunn O\)\)\)`, + `"Weird" Al`: `\"Weird\" Al`, + } { + if got := Escape(in); got != want { + t.Errorf("Escape(%q) = %q, want %q", in, got, want) + } + } +} + +func TestSearchReleaseGroups_AsksTitleAndArtistAndReadsCredits(t *testing.T) { + var query, agent atomic.Value + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + query.Store(r.URL.Query().Get("query")) + agent.Store(r.Header.Get("User-Agent")) + _, _ = w.Write([]byte(`{"release-groups": [{ + "id": "rg1", "title": "OK Computer", "primary-type": "Album", "secondary-types": [], + "first-release-date": "1997-05-21", + "artist-credit": [{"name": "Radiohead", "joinphrase": "", "artist": {"id": "a1", "name": "Radiohead"}}] + }]}`)) + })) + defer srv.Close() + + got, err := testClient(srv).SearchReleaseGroups(context.Background(), "ok computer", 10) + if err != nil { + t.Fatal(err) + } + if q := query.Load(); q != "releasegroup:(ok computer) OR artist:(ok computer)" { + t.Errorf("query = %q", q) + } + if agent.Load() != UserAgent { + t.Errorf("User-Agent = %q", agent.Load()) + } + if len(got) != 1 || got[0].MBID != "rg1" || got[0].Year() != "1997" || got[0].ArtistCredit != "Radiohead" || 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": [{ + "id": "rec1", "title": "Karma Police", "length": 264000, + "artist-credit": [{"name": "Radiohead", "artist": {"id": "a1"}}], + "releases": [ + {"id": "r1", "status": "Official", "release-group": {"id": "rg-live", "title": "Live", "primary-type": "Album", "secondary-types": ["Live"]}}, + {"id": "r2", "status": "Official", "release-group": {"id": "rg1", "title": "OK Computer", "primary-type": "Album"}} + ] + }]}`)) + })) + defer srv.Close() + + got, err := testClient(srv).SearchRecordings(context.Background(), "karma police", 5) + if err != nil { + t.Fatal(err) + } + if len(got) != 1 || got[0].ReleaseGroup.MBID != "rg1" || got[0].LengthMs != 264000 { + t.Errorf("got %+v, want it filed under the studio album", got) + } +} + +func TestArtistReleaseGroups_PagesUntilTheCount(t *testing.T) { + var calls atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + calls.Add(1) + offset, _ := strconv.Atoi(r.URL.Query().Get("offset")) + if r.URL.Query().Get("artist") != "a1" { + t.Errorf("artist = %q", r.URL.Query().Get("artist")) + } + // 3 in all, two per page. + var items string + for i := offset; i < offset+2 && i < 3; i++ { + if items != "" { + items += "," + } + items += fmt.Sprintf(`{"id": "rg%d", "title": "T%d", "primary-type": "Album"}`, i, i) + } + _, _ = fmt.Fprintf(w, `{"release-group-count": 3, "release-groups": [%s]}`, items) + })) + defer srv.Close() + + got, err := testClient(srv).ArtistReleaseGroups(context.Background(), "a1") + if err != nil { + t.Fatal(err) + } + if len(got) != 3 || calls.Load() != 2 { + t.Errorf("got %d groups in %d calls, want 3 in 2", len(got), calls.Load()) + } +} + +func TestReleaseGroupReleases_ReadsMediaAndTracks(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Query().Get("inc") != "media recordings" { + t.Errorf("inc = %q", r.URL.Query().Get("inc")) + } + _, _ = w.Write([]byte(`{"release-count": 1, "releases": [{ + "id": "r1", "title": "OK Computer", "status": "Official", "date": "1997-06-16", "country": "GB", + "media": [ + {"position": 1, "format": "CD", "tracks": [ + {"position": 1, "number": "1", "title": "Airbag", "length": 284000, "recording": {"id": "rec-a"}}, + {"position": 2, "number": "2", "title": "Paranoid Android", "length": null, "recording": {"id": "rec-b", "length": 383000}} + ]}, + {"position": 2, "format": "CD", "tracks": []} + ] + }]}`)) + })) + defer srv.Close() + + got, err := testClient(srv).ReleaseGroupReleases(context.Background(), "rg1") + if err != nil { + t.Fatal(err) + } + if len(got) != 1 { + t.Fatalf("got %+v", got) + } + r := got[0] + if r.TrackCount() != 2 || r.Formats() != "2×CD" { + t.Errorf("tracks %d, formats %q", r.TrackCount(), r.Formats()) + } + if tr := r.Media[0].Tracks[1]; tr.LengthMs != 383000 || tr.RecordingMBID != "rec-b" { + t.Errorf("a track with no length of its own should take the recording's: %+v", tr) + } +} + +func TestGet_ThrottleIsRetriedThenUnreachable(t *testing.T) { + var calls atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + if calls.Add(1) == 1 { + w.WriteHeader(http.StatusServiceUnavailable) + return + } + _, _ = w.Write([]byte(`{"artists": [{"id": "a1", "name": "Radiohead"}]}`)) + })) + defer srv.Close() + + got, err := testClient(srv).SearchArtists(context.Background(), "radiohead", 5) + if err != nil || len(got) != 1 || calls.Load() != 2 { + t.Fatalf("a throttled first answer should be retried: %v, %+v, %d calls", err, got, calls.Load()) + } + + down := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusBadGateway) + })) + defer down.Close() + if _, err := testClient(down).SearchArtists(context.Background(), "x", 5); !errors.Is(err, ErrUnreachable) { + t.Errorf("err = %v, want ErrUnreachable", err) + } +} + +func TestGet_NotFoundAndNoServer(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + })) + if _, err := testClient(srv).Artist(context.Background(), "nope"); !errors.Is(err, ErrNotFound) { + t.Errorf("err = %v, want ErrNotFound", err) + } + srv.Close() + c := testClient(srv) + c.Retries = -1 + if _, err := c.Artist(context.Background(), "a1"); !errors.Is(err, ErrUnreachable) { + t.Errorf("a closed server: err = %v, want ErrUnreachable", err) + } +} + +func TestPacer_SpacesCallers(t *testing.T) { + p := &Pacer{Interval: 30 * time.Millisecond} + start := time.Now() + for range 3 { + if err := p.Wait(context.Background()); err != nil { + t.Fatal(err) + } + } + if elapsed := time.Since(start); elapsed < 60*time.Millisecond { + t.Errorf("three waits took %v, want at least two intervals", elapsed) + } + ctx, cancel := context.WithCancel(context.Background()) + cancel() + if err := p.Wait(ctx); err == nil { + t.Error("a cancelled wait should return the context's error") + } +} diff --git a/internal/musicbrainz/entities.go b/internal/musicbrainz/entities.go new file mode 100644 index 00000000..21a449a4 --- /dev/null +++ b/internal/musicbrainz/entities.go @@ -0,0 +1,383 @@ +package musicbrainz + +import ( + "context" + "net/url" + "strconv" + "strings" +) + +// Artist is a MusicBrainz artist. +type Artist struct { + MBID string + Name string + SortName string + Disambiguation string + Type string // Person, Group, Orchestra, …; may be empty + Country string +} + +// ReleaseGroup is what a person calls an album: one work, released any +// number of times. Lidarr's albums were release groups too. +type ReleaseGroup struct { + MBID string + Title string + PrimaryType string // Album, Single, EP, Broadcast, Other; may be empty + SecondaryTypes []string // Compilation, Live, Remix, Soundtrack, … + // FirstReleaseDate is MusicBrainz's partial date: "", "1997", + // "1997-05" or "1997-05-21". + FirstReleaseDate string + ArtistCredit string // the credit as printed, joinphrases included + ArtistMBIDs []string +} + +// Year is the first release's year, or "". +func (g ReleaseGroup) Year() string { + if len(g.FirstReleaseDate) >= 4 { + return g.FirstReleaseDate[:4] + } + return "" +} + +// Release is one issue of a release group: a pressing, an edition, a +// digital release, each with its own track list. +type Release struct { + MBID string + Title string + Status string // Official, Promotion, Bootleg, Pseudo-Release; may be empty + Date string // partial, like ReleaseGroup.FirstReleaseDate + Country string + Disambiguation string + Media []Medium +} + +// TrackCount totals the tracks across every medium. +func (r Release) TrackCount() int { + n := 0 + for _, m := range r.Media { + n += len(m.Tracks) + } + return n +} + +// Formats summarises the media, e.g. "CD", "2×CD" or "CD + DVD". +func (r Release) Formats() string { + var parts []string + counts := map[string]int{} + for _, m := range r.Media { + f := m.Format + if f == "" { + f = "Unknown" + } + if counts[f] == 0 { + parts = append(parts, f) + } + counts[f]++ + } + for i, f := range parts { + if counts[f] > 1 { + parts[i] = strconv.Itoa(counts[f]) + "×" + f + } + } + return strings.Join(parts, " + ") +} + +// Medium is one disc (or side, or file set) of a release. +type Medium struct { + Position int + Format string + Tracks []Track +} + +// Track is a position on a medium, pointing at the recording heard there. +type Track struct { + Position int + Number string // as printed: "1", "A1", … + Title string + LengthMs int // 0 when unknown + RecordingMBID string +} + +// Recording is a piece of audio, as a search returns it. +type Recording struct { + MBID string + Title string + LengthMs int + ArtistCredit string + ArtistMBIDs []string + // ReleaseGroup is the group the recording is best known from: the first + // official album it appears on, else the first release listed. + ReleaseGroup ReleaseGroup +} + +// --- wire shapes --- + +type wireCredit struct { + Name string `json:"name"` + JoinPhrase string `json:"joinphrase"` + Artist struct { + ID string `json:"id"` + Name string `json:"name"` + } `json:"artist"` +} + +func credit(cs []wireCredit) (string, []string) { + var b strings.Builder + ids := make([]string, 0, len(cs)) + for _, c := range cs { + name := c.Name + if name == "" { + name = c.Artist.Name + } + b.WriteString(name) + b.WriteString(c.JoinPhrase) + if c.Artist.ID != "" { + ids = append(ids, c.Artist.ID) + } + } + return b.String(), ids +} + +type wireArtist struct { + ID string `json:"id"` + Name string `json:"name"` + SortName string `json:"sort-name"` + Disambiguation string `json:"disambiguation"` + Type string `json:"type"` + Country string `json:"country"` +} + +func (w wireArtist) artist() Artist { + return Artist{MBID: w.ID, Name: w.Name, SortName: w.SortName, Disambiguation: w.Disambiguation, Type: w.Type, Country: w.Country} +} + +type wireReleaseGroup struct { + ID string `json:"id"` + Title string `json:"title"` + PrimaryType string `json:"primary-type"` + SecondaryTypes []string `json:"secondary-types"` + FirstReleaseDate string `json:"first-release-date"` + ArtistCredit []wireCredit `json:"artist-credit"` +} + +func (w wireReleaseGroup) group() ReleaseGroup { + name, ids := credit(w.ArtistCredit) + return ReleaseGroup{ + MBID: w.ID, Title: w.Title, PrimaryType: w.PrimaryType, + SecondaryTypes: append([]string{}, w.SecondaryTypes...), FirstReleaseDate: w.FirstReleaseDate, + ArtistCredit: name, ArtistMBIDs: ids, + } +} + +type wireTrack struct { + Position int `json:"position"` + Number string `json:"number"` + Title string `json:"title"` + Length *int `json:"length"` + Recording struct { + ID string `json:"id"` + Length *int `json:"length"` + } `json:"recording"` +} + +type wireRelease struct { + ID string `json:"id"` + Title string `json:"title"` + Status string `json:"status"` + Date string `json:"date"` + Country string `json:"country"` + Disambiguation string `json:"disambiguation"` + Media []struct { + Position int `json:"position"` + Format string `json:"format"` + Tracks []wireTrack `json:"tracks"` + } `json:"media"` + ReleaseGroup *wireReleaseGroup `json:"release-group"` +} + +func (w wireRelease) release() Release { + r := Release{MBID: w.ID, Title: w.Title, Status: w.Status, Date: w.Date, Country: w.Country, Disambiguation: w.Disambiguation} + for _, m := range w.Media { + med := Medium{Position: m.Position, Format: m.Format} + for _, t := range m.Tracks { + length := 0 + switch { + case t.Length != nil: + length = *t.Length + case t.Recording.Length != nil: + length = *t.Recording.Length + } + med.Tracks = append(med.Tracks, Track{ + Position: t.Position, Number: t.Number, Title: t.Title, + LengthMs: length, RecordingMBID: t.Recording.ID, + }) + } + r.Media = append(r.Media, med) + } + return r +} + +// --- searches --- + +// SearchArtists finds artists by name or alias. +func (c *Client) SearchArtists(ctx context.Context, text string, limit int) ([]Artist, error) { + var resp struct { + Artists []wireArtist `json:"artists"` + } + q := url.Values{"query": {fieldQuery(text, "artist", "alias")}, "limit": {searchLimit(limit)}} + if err := c.get(ctx, "/artist", q, &resp); err != nil { + return nil, err + } + out := make([]Artist, 0, len(resp.Artists)) + for _, a := range resp.Artists { + out = append(out, a.artist()) + } + return out, nil +} + +// SearchReleaseGroups finds albums by title or by artist, so "radiohead ok +// computer" and "ok computer" both find OK Computer. +func (c *Client) SearchReleaseGroups(ctx context.Context, text string, limit int) ([]ReleaseGroup, error) { + var resp struct { + ReleaseGroups []wireReleaseGroup `json:"release-groups"` + } + q := url.Values{"query": {fieldQuery(text, "releasegroup", "artist")}, "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 { + Recordings []struct { + ID string `json:"id"` + Title string `json:"title"` + Length *int `json:"length"` + ArtistCredit []wireCredit `json:"artist-credit"` + Releases []wireRelease `json:"releases"` + } `json:"recordings"` + } + q := url.Values{"query": {fieldQuery(text, "recording", "artist")}, "limit": {searchLimit(limit)}} + if err := c.get(ctx, "/recording", q, &resp); err != nil { + return nil, err + } + out := make([]Recording, 0, len(resp.Recordings)) + for _, r := range resp.Recordings { + name, ids := credit(r.ArtistCredit) + rec := Recording{MBID: r.ID, Title: r.Title, ArtistCredit: name, ArtistMBIDs: ids} + if r.Length != nil { + rec.LengthMs = *r.Length + } + rec.ReleaseGroup = bestKnownFrom(r.Releases) + out = append(out, rec) + } + return out, nil +} + +// bestKnownFrom picks the release group a recording should be filed under +// in a search result: the first official album, else the first release. +func bestKnownFrom(releases []wireRelease) ReleaseGroup { + var first *wireReleaseGroup + for _, r := range releases { + if r.ReleaseGroup == nil || r.ReleaseGroup.ID == "" { + continue + } + if first == nil { + first = r.ReleaseGroup + } + if r.Status == "Official" && r.ReleaseGroup.PrimaryType == "Album" && len(r.ReleaseGroup.SecondaryTypes) == 0 { + return r.ReleaseGroup.group() + } + } + if first == nil { + return ReleaseGroup{} + } + return first.group() +} + +// --- lookups and browses --- + +// Artist looks one artist up by MBID. +func (c *Client) Artist(ctx context.Context, mbid string) (Artist, error) { + var w wireArtist + if err := c.get(ctx, "/artist/"+url.PathEscape(mbid), nil, &w); err != nil { + return Artist{}, err + } + if w.ID == "" { + return Artist{}, ErrBadResponse + } + return w.artist(), nil +} + +// ArtistReleaseGroups lists every release group the artist is credited on, +// paging through MusicBrainz's browse. +func (c *Client) ArtistReleaseGroups(ctx context.Context, artistMBID string) ([]ReleaseGroup, error) { + var out []ReleaseGroup + for page := 0; page < maxPages; page++ { + var resp struct { + Count int `json:"release-group-count"` + ReleaseGroups []wireReleaseGroup `json:"release-groups"` + } + q := url.Values{ + "artist": {artistMBID}, "inc": {"artist-credits"}, + "limit": {strconv.Itoa(pageSize)}, "offset": {strconv.Itoa(len(out))}, + } + if err := c.get(ctx, "/release-group", q, &resp); err != nil { + return nil, err + } + for _, g := range resp.ReleaseGroups { + out = append(out, g.group()) + } + if len(resp.ReleaseGroups) == 0 || len(out) >= resp.Count { + break + } + } + return out, nil +} + +// ReleaseGroupReleases lists every release of a release group with its +// media and track lists. +func (c *Client) ReleaseGroupReleases(ctx context.Context, groupMBID string) ([]Release, error) { + var out []Release + for page := 0; page < maxPages; page++ { + var resp struct { + Count int `json:"release-count"` + Releases []wireRelease `json:"releases"` + } + q := url.Values{ + "release-group": {groupMBID}, "inc": {"media recordings"}, + "limit": {strconv.Itoa(pageSize)}, "offset": {strconv.Itoa(len(out))}, + } + if err := c.get(ctx, "/release", q, &resp); err != nil { + return nil, err + } + for _, r := range resp.Releases { + out = append(out, r.release()) + } + // Advance by what came back: MusicBrainz may return fewer than the + // page size when track lists are large. + if len(resp.Releases) == 0 || len(out) >= resp.Count { + break + } + } + return out, nil +} + +// ReleaseGroup looks one release group up by MBID, with its artist credit. +func (c *Client) ReleaseGroup(ctx context.Context, mbid string) (ReleaseGroup, error) { + var w wireReleaseGroup + q := url.Values{"inc": {"artist-credits"}} + if err := c.get(ctx, "/release-group/"+url.PathEscape(mbid), q, &w); err != nil { + return ReleaseGroup{}, err + } + if w.ID == "" { + return ReleaseGroup{}, ErrBadResponse + } + return w.group(), nil +} diff --git a/internal/musicbrainz/pace.go b/internal/musicbrainz/pace.go new file mode 100644 index 00000000..4a0b3ed1 --- /dev/null +++ b/internal/musicbrainz/pace.go @@ -0,0 +1,59 @@ +// Package musicbrainz is Minstrel's client for the MusicBrainz web service: +// searches and browses for the release catalogue (M502 #5519), and the one +// pacer every MusicBrainz request in the process waits on. +package musicbrainz + +import ( + "context" + "sync" + "time" +) + +// MinInterval honours MusicBrainz's 1 request/second ceiling, with headroom. +// The limit is per client address, not per caller, which is why there is one +// pacer for the whole process. +const MinInterval = 1100 * time.Millisecond + +// Pacer spaces requests at least Interval apart. Safe for concurrent use; +// callers queue in the order they claim slots. +type Pacer struct { + Interval time.Duration + + mu sync.Mutex + next time.Time +} + +// Wait blocks until this caller's slot comes up, or ctx ends. A zero +// Interval never waits. +func (p *Pacer) Wait(ctx context.Context) error { + if p == nil || p.Interval <= 0 { + return ctx.Err() + } + p.mu.Lock() + now := time.Now() + slot := p.next + if slot.Before(now) { + slot = now + } + p.next = slot.Add(p.Interval) + p.mu.Unlock() + + wait := time.Until(slot) + if wait <= 0 { + return ctx.Err() + } + timer := time.NewTimer(wait) + defer timer.Stop() + select { + case <-ctx.Done(): + return ctx.Err() + case <-timer.C: + return nil + } +} + +var shared = &Pacer{Interval: MinInterval} + +// Shared is the pacer every MusicBrainz request in the process goes through: +// the catalogue's and tag enrichment's alike. +func Shared() *Pacer { return shared } diff --git a/internal/tags/httpclient.go b/internal/tags/httpclient.go index a65f1087..44f3b521 100644 --- a/internal/tags/httpclient.go +++ b/internal/tags/httpclient.go @@ -25,6 +25,10 @@ type httpClientOptions struct { UserAgent string // MinInterval throttles requests; 0 disables rate limiting. MinInterval time.Duration + // Pacer, when set, replaces MinInterval: requests wait on it instead. + // MusicBrainz's provider passes musicbrainz.Shared(), because its limit + // is per address and the release catalogue spends the same budget. + Pacer interface{ Wait(context.Context) error } // MaxRetries is the number of retry attempts on 429 / 5xx. MaxRetries int // BaseBackoff is the base unit for exponential backoff (2^attempt × base ± 20%). @@ -141,6 +145,9 @@ func (c *httpClient) handleStatus(ctx context.Context, resp *http.Response, atte // rateLimitWait blocks until MinInterval has elapsed since the last call, // then claims the slot. Disabled when MinInterval is 0. func (c *httpClient) rateLimitWait(ctx context.Context) error { + if c.opts.Pacer != nil { + return c.opts.Pacer.Wait(ctx) + } if c.opts.MinInterval <= 0 { return nil } diff --git a/internal/tags/provider_musicbrainz.go b/internal/tags/provider_musicbrainz.go index b652a917..c60b6e0f 100644 --- a/internal/tags/provider_musicbrainz.go +++ b/internal/tags/provider_musicbrainz.go @@ -7,16 +7,16 @@ import ( "net/url" "sync/atomic" "time" + + "git.fabledsword.com/bvandeusen/minstrel/internal/musicbrainz" ) // mbBaseURL is a var (not const) so tests can point it at an httptest server. var mbBaseURL = "https://musicbrainz.org/ws/2" const ( - // mbMinPeriod honors MusicBrainz's 1 req/s anonymous ceiling (with headroom). - mbMinPeriod = 1100 * time.Millisecond // mbUserAgent identifies the app — MusicBrainz rejects requests without one. - mbUserAgent = "Minstrel/1.0 ( https://git.fabledsword.com/bvandeusen/minstrel )" + mbUserAgent = musicbrainz.UserAgent // artistTagWeightFactor down-weights artist-level fallback tags relative // to recording-specific tags — the artist's overall character is a coarser // signal than a tag on the exact recording (#1519). @@ -37,11 +37,13 @@ type musicbrainzProvider struct { func init() { Register(&musicbrainzProvider{ client: newHTTPClient(httpClientOptions{ - Name: "musicbrainz", - UserAgent: mbUserAgent, - HTTPClient: &http.Client{Timeout: 30 * time.Second}, - MinInterval: mbMinPeriod, - MaxRetries: 3, + Name: "musicbrainz", + UserAgent: mbUserAgent, + HTTPClient: &http.Client{Timeout: 30 * time.Second}, + // MusicBrainz's 1 request/second is per address: share it with + // the release catalogue (M502 #5519). + Pacer: musicbrainz.Shared(), + MaxRetries: 3, }), }) } diff --git a/web/src/lib/api/lidarr.test.ts b/web/src/lib/api/catalogue.test.ts similarity index 60% rename from web/src/lib/api/lidarr.test.ts rename to web/src/lib/api/catalogue.test.ts index 31e26c90..56fd5623 100644 --- a/web/src/lib/api/lidarr.test.ts +++ b/web/src/lib/api/catalogue.test.ts @@ -9,39 +9,39 @@ vi.mock('./client', () => ({ } })); -import { searchLidarr } from './lidarr'; +import { searchCatalogue } from './catalogue'; import { api } from './client'; import { qk } from './queries'; -import type { LidarrSearchResult } from './types'; +import type { CatalogueSearchResult } from './types'; beforeEach(() => { vi.clearAllMocks(); }); -describe('searchLidarr', () => { +describe('searchCatalogue', () => { test('builds URL with q and kind query params', async () => { (api.get as ReturnType).mockResolvedValueOnce([]); - await searchLidarr('aphex twin', 'artist'); - expect(api.get).toHaveBeenCalledWith('/api/lidarr/search?q=aphex+twin&kind=artist'); + await searchCatalogue('aphex twin', 'artist'); + expect(api.get).toHaveBeenCalledWith('/api/catalogue/search?q=aphex+twin&kind=artist'); }); test('encodes special characters in q (quotes, ampersands, plus)', async () => { (api.get as ReturnType).mockResolvedValueOnce([]); - await searchLidarr('boards of canada & friends', 'album'); + await searchCatalogue('boards of canada & friends', 'album'); const calledWith = (api.get as ReturnType).mock.calls[0][0] as string; // URLSearchParams encodes space as '+' and '&' as '%26'. expect(calledWith).toBe( - '/api/lidarr/search?q=boards+of+canada+%26+friends&kind=album' + '/api/catalogue/search?q=boards+of+canada+%26+friends&kind=album' ); }); test('passes through results unchanged', async () => { - const fixture: LidarrSearchResult[] = [ + const fixture: CatalogueSearchResult[] = [ { mbid: 'mbid-1', name: 'Aphex Twin', secondary_text: 'Electronic', - image_url: 'https://example.test/img.jpg', + image_url: '/api/catalogue/art/artist/mbid-1?name=Aphex+Twin', artist_mbid: '', album_mbid: '', in_library: false, @@ -49,24 +49,24 @@ describe('searchLidarr', () => { } ]; (api.get as ReturnType).mockResolvedValueOnce(fixture); - const out = await searchLidarr('aphex', 'artist'); + const out = await searchCatalogue('aphex', 'artist'); expect(out).toBe(fixture); }); test('propagates errors from api.get', async () => { - const err = { code: 'lidarr_unavailable', message: 'down', status: 503 }; + const err = { code: 'musicbrainz_unreachable', message: 'down', status: 503 }; (api.get as ReturnType).mockRejectedValueOnce(err); - await expect(searchLidarr('x', 'track')).rejects.toMatchObject({ - code: 'lidarr_unavailable', + await expect(searchCatalogue('x', 'track')).rejects.toMatchObject({ + code: 'musicbrainz_unreachable', status: 503 }); }); }); -describe('qk.lidarrSearch', () => { +describe('qk.catalogueSearch', () => { test('keys include q + kind', () => { - expect(qk.lidarrSearch('foo', 'artist')).toEqual([ - 'lidarrSearch', + expect(qk.catalogueSearch('foo', 'artist')).toEqual([ + 'catalogueSearch', { q: 'foo', kind: 'artist' } ]); }); diff --git a/web/src/lib/api/catalogue.ts b/web/src/lib/api/catalogue.ts new file mode 100644 index 00000000..093a8183 --- /dev/null +++ b/web/src/lib/api/catalogue.ts @@ -0,0 +1,26 @@ +import { createQuery } from '@tanstack/svelte-query'; +import { api } from './client'; +import { qk } from './queries'; +import type { CatalogueSearchResult, LidarrRequestKind } from './types'; + +// searchCatalogue asks MusicBrainz through Minstrel's release catalogue +// (M502 #5519). URLSearchParams handles encoding (spaces, quotes, +// ampersands) so callers can pass q raw. +export async function searchCatalogue( + q: string, + kind: LidarrRequestKind +): Promise { + const params = new URLSearchParams({ q, kind }); + return api.get(`/api/catalogue/search?${params.toString()}`); +} + +// MusicBrainz allows a request a second, so a repeated search within a minute +// is served from the client's cache rather than asked again. +export function createCatalogueSearchQuery(q: string, kind: LidarrRequestKind) { + return createQuery({ + queryKey: qk.catalogueSearch(q, kind), + queryFn: () => searchCatalogue(q, kind), + enabled: q.length > 0, + staleTime: 60_000 + }); +} diff --git a/web/src/lib/api/lidarr.ts b/web/src/lib/api/lidarr.ts deleted file mode 100644 index e3ddd332..00000000 --- a/web/src/lib/api/lidarr.ts +++ /dev/null @@ -1,28 +0,0 @@ -import { createQuery } from '@tanstack/svelte-query'; -import { api } from './client'; -import { qk } from './queries'; -import type { LidarrSearchResult, LidarrRequestKind } from './types'; - -// Search-only. Admin-side Lidarr config + connection probing lives in admin.ts. - -// searchLidarr proxies the user-facing Lidarr search endpoint. URLSearchParams -// handles encoding (spaces, quotes, ampersands) so callers can pass q raw. -export async function searchLidarr( - q: string, - kind: LidarrRequestKind -): Promise { - const params = new URLSearchParams({ q, kind }); - return api.get(`/api/lidarr/search?${params.toString()}`); -} - -// staleTime mirrors the spec §11 60s server-side LRU cache: re-issuing the -// same query within a minute hits cache on the server, so there's no point -// in marking it stale client-side any sooner. -export function createLidarrSearchQuery(q: string, kind: LidarrRequestKind) { - return createQuery({ - queryKey: qk.lidarrSearch(q, kind), - queryFn: () => searchLidarr(q, kind), - enabled: q.length > 0, - staleTime: 60_000 - }); -} diff --git a/web/src/lib/api/queries.ts b/web/src/lib/api/queries.ts index 8ed04848..058318c7 100644 --- a/web/src/lib/api/queries.ts +++ b/web/src/lib/api/queries.ts @@ -30,8 +30,8 @@ export const qk = { likedArtists: () => ['likedArtists'] as const, history: () => ['history'] as const, // Lidarr / requests / admin. - lidarrSearch: (q: string, kind: string) => - ['lidarrSearch', { q, kind }] as const, + catalogueSearch: (q: string, kind: string) => + ['catalogueSearch', { q, kind }] as const, myRequests: () => ['myRequests'] as const, lidarrConfig: () => ['lidarrConfig'] as const, lidarrQualityProfiles: () => ['lidarrQualityProfiles'] as const, diff --git a/web/src/lib/api/types.ts b/web/src/lib/api/types.ts index 8cfc208a..83279c2f 100644 --- a/web/src/lib/api/types.ts +++ b/web/src/lib/api/types.ts @@ -146,7 +146,10 @@ export type LidarrRequestStatus = export type LidarrRequestKind = 'artist' | 'album' | 'track'; -export type LidarrSearchResult = { +// One row of the request search (GET /api/catalogue/search, M502 #5519). +// An album is a MusicBrainz release group; image_url is Minstrel's own art +// endpoint, which answers 404 when no source has an image. +export type CatalogueSearchResult = { mbid: string; name: string; secondary_text: string; diff --git a/web/src/lib/components/DiscoverResultCard.svelte b/web/src/lib/components/DiscoverResultCard.svelte index 5dd986ed..7730db02 100644 --- a/web/src/lib/components/DiscoverResultCard.svelte +++ b/web/src/lib/components/DiscoverResultCard.svelte @@ -28,12 +28,21 @@ state: DiscoverCardState; attribution?: string; onRequest?: () => void; - // Omit both to get a card with no snooze affordance — the Lidarr search + // Omit both to get a card with no snooze affordance — the catalogue search // results reuse this component and have nothing to park. onSnooze?: () => void; onUnsnooze?: () => void; } = $props(); + // Art comes from Minstrel's catalogue endpoint, which answers 404 when no + // source has an image (M502 #5519): show the icon then, not a broken image. + // Reset when the card is reused for another result. + let imageFailed = $state(false); + $effect(() => { + void imageUrl; + imageFailed = false; + }); + const FallbackIcon = $derived( kind === 'artist' ? Disc3 : kind === 'album' ? Album : Music2 ); @@ -50,12 +59,13 @@
- {#if imageUrl} + {#if imageUrl && !imageFailed} (imageFailed = true)} /> {:else} diff --git a/web/src/lib/components/DiscoverResultCard.test.ts b/web/src/lib/components/DiscoverResultCard.test.ts index 86114a59..7057a7ef 100644 --- a/web/src/lib/components/DiscoverResultCard.test.ts +++ b/web/src/lib/components/DiscoverResultCard.test.ts @@ -168,4 +168,20 @@ describe('DiscoverResultCard', () => { expect(onUnsnooze).toHaveBeenCalledOnce(); expect(onRequest).not.toHaveBeenCalled(); }); + + test('an image that fails to load falls back to the icon', async () => { + const { container } = render(DiscoverResultCard, { + props: { + kind: 'artist', + title: 'Boards of Canada', + state: 'requestable', + imageUrl: '/api/catalogue/art/artist/x', + }, + }); + const img = container.querySelector('img'); + expect(img).not.toBeNull(); + await fireEvent.error(img!); + expect(container.querySelector('img')).toBeNull(); + expect(container.querySelector('.art svg')).not.toBeNull(); + }); }); diff --git a/web/src/lib/styles/error-copy.json b/web/src/lib/styles/error-copy.json index caf14ca5..1dfaea6c 100644 --- a/web/src/lib/styles/error-copy.json +++ b/web/src/lib/styles/error-copy.json @@ -30,6 +30,7 @@ "mbid_required": "An MBID is required for this lookup.", "system_playlist_readonly": "System playlists can't be edited directly.", "connection_refused": "Couldn't reach the server. Check the URL and try again.", + "musicbrainz_unreachable": "MusicBrainz can't be reached right now, so search is unavailable. Try again in a minute.", "lidarr_unreachable": "Lidarr is unreachable right now. Try again, or check Admin → Integrations.", "lidarr_disabled": "Lidarr integration is not enabled.", "lidarr_auth_failed": "Lidarr authentication failed.", diff --git a/web/src/routes/admin/integrations/+page.svelte b/web/src/routes/admin/integrations/+page.svelte index ae0a137c..f860cfa6 100644 --- a/web/src/routes/admin/integrations/+page.svelte +++ b/web/src/routes/admin/integrations/+page.svelte @@ -428,8 +428,8 @@

Lidarr

- Search Lidarr from /discover and - route approved requests to it. + Route approved requests from /discover + to Lidarr. Search itself now asks MusicBrainz.

diff --git a/web/src/routes/discover/+page.svelte b/web/src/routes/discover/+page.svelte index c82bd2b7..1247c0d2 100644 --- a/web/src/routes/discover/+page.svelte +++ b/web/src/routes/discover/+page.svelte @@ -1,6 +1,6 @@