diff --git a/core/artist_source_detail.py b/core/artist_source_detail.py new file mode 100644 index 00000000..c62a5293 --- /dev/null +++ b/core/artist_source_detail.py @@ -0,0 +1,186 @@ +"""Synthesize an artist-detail response for an artist that isn't in the library. + +Extracted from ``web_server.py`` so the logic is importable at test time. +The route handler in ``web_server.py`` is now a thin wrapper that builds the +per-source clients (which live as module globals there), calls this function, +and wraps the return value in ``jsonify``. + +Used by ``/api/artist-detail/`` when the URL is called with a ``source`` +query parameter and the library DB lookup misses. Enriches the response with +whatever metadata we can pull on demand: + + * Image URL (via ``metadata_service.get_artist_image_url``) + * Source-specific artist info — genres + follower count from the named + source's ``get_artist`` / ``get_artist_info`` helper + * Last.fm bio + listeners + playcount + URL (by artist name) + * Discography from the named source, with variant dedup disabled so every + release surfaces + +All per-source clients are passed in explicitly. Callers that can't or don't +want to provide a given client pass ``None`` — the corresponding enrichment +branch becomes a no-op. +""" + +from __future__ import annotations + +import logging +from typing import Any, Dict, Optional, Tuple + +from core.artist_source_lookup import SOURCE_ID_FIELD + +logger = logging.getLogger("artist_source_detail") + + +def build_source_only_artist_detail( + artist_id: str, + artist_name: str, + source: str, + *, + spotify_client: Optional[Any] = None, + deezer_client: Optional[Any] = None, + itunes_client: Optional[Any] = None, + discogs_client: Optional[Any] = None, + lastfm_api_key: Optional[str] = None, +) -> Tuple[Dict[str, Any], int]: + """Build the artist-detail payload for a source-only artist. + + Returns ``(payload_dict, http_status)``. Callers wrap the dict in + ``jsonify`` or equivalent. Status is 200 on success, 404 when the + source's discography lookup returned no releases. + """ + # Deferred import — keeps the top-level module importable in test rigs + # that stub out only what they need (same pattern `_find_library_artist` + # uses). + from core.metadata_service import ( + MetadataLookupOptions, + get_artist_detail_discography, + get_artist_image_url, + ) + + resolved_name = (artist_name or artist_id or "").strip() + + # 1. Image URL via the same helper /api/artist//image uses. + image_url: Optional[str] = None + try: + image_url = get_artist_image_url(artist_id, source_override=source) + except Exception as e: + logger.debug(f"Artist image lookup failed for {source}:{artist_id}: {e}") + + # 2. Source-side artist info (image, genres, followers depending on source). + source_genres: list = [] + source_followers: Optional[int] = None + try: + if source == "spotify" and spotify_client is not None: + sp_artist = spotify_client.get_artist(artist_id, allow_fallback=False) + if sp_artist: + source_genres = sp_artist.get("genres") or [] + source_followers = (sp_artist.get("followers") or {}).get("total") + if not image_url and sp_artist.get("images"): + image_url = sp_artist["images"][0].get("url") + elif source == "deezer" and deezer_client is not None: + dz_artist = deezer_client.get_artist_info(artist_id) + if dz_artist: + source_genres = dz_artist.get("genres") or [] + source_followers = (dz_artist.get("followers") or {}).get("total") + elif source == "itunes" and itunes_client is not None: + it_artist = itunes_client.get_artist(artist_id) + if it_artist: + source_genres = it_artist.get("genres") or [] + elif source == "discogs" and discogs_client is not None: + dc_artist = discogs_client.get_artist(artist_id) + if dc_artist: + source_genres = dc_artist.get("genres") or [] + except Exception as e: + logger.debug(f"Source-side artist info lookup failed for {source}:{artist_id}: {e}") + + # 3. Last.fm enrichment by artist name. + lastfm_bio: Optional[str] = None + lastfm_listeners: Optional[int] = None + lastfm_playcount: Optional[int] = None + lastfm_url: Optional[str] = None + if resolved_name and lastfm_api_key: + try: + from core.lastfm_client import LastFMClient + lastfm = LastFMClient(api_key=lastfm_api_key) + lf_info = lastfm.get_artist_info(resolved_name) + if lf_info: + bio_obj = lf_info.get("bio") or {} + lastfm_bio = bio_obj.get("content") or bio_obj.get("summary") + stats_obj = lf_info.get("stats") or {} + if stats_obj.get("listeners"): + try: + lastfm_listeners = int(stats_obj["listeners"]) + except (ValueError, TypeError): + pass + if stats_obj.get("playcount"): + try: + lastfm_playcount = int(stats_obj["playcount"]) + except (ValueError, TypeError): + pass + lastfm_url = lf_info.get("url") + except Exception as e: + logger.debug(f"Last.fm enrichment failed for {resolved_name}: {e}") + + # 4. Discography from the specified source. Skip variant dedup so the + # page shows every release the source returns — matches the inline + # Artists-page behaviour that this view was modelled after. + discography_result = get_artist_detail_discography( + artist_id, + artist_name=resolved_name or artist_id, + options=MetadataLookupOptions( + source_override=source, + allow_fallback=True, + skip_cache=False, + max_pages=0, + limit=50, + artist_source_ids={source: artist_id}, + dedup_variants=False, + ), + ) + + if not discography_result.get("success"): + return { + "success": False, + "error": discography_result.get("error", "Could not load discography"), + "source": source, + }, 404 + + artist_info: Dict[str, Any] = { + "id": artist_id, + "name": resolved_name or artist_id, + "image_url": image_url, + "server_source": None, # not in library + "genres": source_genres, + } + + # Stamp the source-specific ID so the correct service badge renders on the + # hero (e.g. source=deezer -> deezer_id; source=spotify -> spotify_artist_id). + source_id_field = SOURCE_ID_FIELD.get(source) + if source_id_field: + artist_info[source_id_field] = artist_id + + if source_followers is not None: + artist_info["followers"] = source_followers + if lastfm_bio: + artist_info["lastfm_bio"] = lastfm_bio + if lastfm_listeners is not None: + artist_info["lastfm_listeners"] = lastfm_listeners + if lastfm_playcount is not None: + artist_info["lastfm_playcount"] = lastfm_playcount + if lastfm_url: + artist_info["lastfm_url"] = lastfm_url + + logger.info( + f"Source-only artist-detail: {artist_info['name']} from {source} — " + f"albums={len(discography_result.get('albums', []))}, " + f"eps={len(discography_result.get('eps', []))}, " + f"singles={len(discography_result.get('singles', []))}, " + f"genres={len(source_genres)}, lastfm_bio={'yes' if lastfm_bio else 'no'}" + ) + + return { + "success": True, + "artist": artist_info, + "discography": discography_result, + "enrichment_coverage": {}, + }, 200 diff --git a/tests/test_artist_source_detail.py b/tests/test_artist_source_detail.py new file mode 100644 index 00000000..400bf7fe --- /dev/null +++ b/tests/test_artist_source_detail.py @@ -0,0 +1,274 @@ +"""Tests for ``core.artist_source_detail.build_source_only_artist_detail``. + +The function used to live inline inside ``web_server.py``; a prior version of +these tests AST-parsed the function body to assert on response keys because +``web_server.py`` couldn't be imported at test time. Now that the logic lives +in a side-effect-free core module with dependency-injected clients, the tests +just call it directly with mocks. +""" + +from __future__ import annotations + +from types import SimpleNamespace + +import pytest + +from core import artist_source_detail +from core.artist_source_detail import build_source_only_artist_detail + + +# --------------------------------------------------------------------------- +# Fixtures — stubs for the metadata_service helpers the function calls +# --------------------------------------------------------------------------- + +def _success_discography(**overrides): + result = { + "success": True, + "albums": [{"id": "a1", "title": "Album One"}], + "eps": [], + "singles": [{"id": "s1", "title": "Single One"}], + } + result.update(overrides) + return result + + +def _empty_discography(): + return { + "success": False, + "error": "No releases found for artist", + } + + +@pytest.fixture +def _stub_metadata(monkeypatch): + """Replace the metadata_service imports with controllable stubs. + + The function imports ``get_artist_image_url`` and + ``get_artist_detail_discography`` lazily inside its body (deferred import), + so we patch on the metadata_service module directly. + """ + from core import metadata_service + + state = { + "image_url": None, + "discography": _success_discography(), + "last_options": None, + "last_discog_call": None, + } + + def fake_get_artist_image_url(artist_id, source_override=None): + return state["image_url"] + + def fake_get_artist_detail_discography(artist_id, artist_name="", options=None): + state["last_options"] = options + state["last_discog_call"] = (artist_id, artist_name) + return state["discography"] + + monkeypatch.setattr(metadata_service, "get_artist_image_url", fake_get_artist_image_url) + monkeypatch.setattr(metadata_service, "get_artist_detail_discography", fake_get_artist_detail_discography) + + return state + + +# --------------------------------------------------------------------------- +# Group A — Success-path response shape + source-specific ID stamping +# --------------------------------------------------------------------------- + +class TestResponseShape: + def test_success_returns_expected_envelope(self, _stub_metadata): + payload, status = build_source_only_artist_detail( + "dz-123", "Artist One", "deezer", + ) + assert status == 200 + assert payload["success"] is True + assert payload["discography"] == _stub_metadata["discography"] + assert payload["enrichment_coverage"] == {} + assert payload["artist"]["id"] == "dz-123" + assert payload["artist"]["name"] == "Artist One" + assert payload["artist"]["server_source"] is None + assert "genres" in payload["artist"] + + def test_empty_artist_name_falls_back_to_id(self, _stub_metadata): + payload, status = build_source_only_artist_detail( + "dz-123", "", "deezer", + ) + assert status == 200 + assert payload["artist"]["name"] == "dz-123" + + def test_failure_returns_404(self, _stub_metadata): + _stub_metadata["discography"] = _empty_discography() + payload, status = build_source_only_artist_detail( + "dz-missing", "Unknown Artist", "deezer", + ) + assert status == 404 + assert payload["success"] is False + assert payload["source"] == "deezer" + assert "error" in payload + + @pytest.mark.parametrize("source,expected_field", [ + ("spotify", "spotify_artist_id"), + ("itunes", "itunes_artist_id"), + ("deezer", "deezer_id"), + ("discogs", "discogs_id"), + ("hydrabase", "soul_id"), + ("musicbrainz", "musicbrainz_id"), + ]) + def test_source_specific_id_field_is_stamped(self, _stub_metadata, source, expected_field): + payload, _ = build_source_only_artist_detail("the-id", "Artist", source) + assert payload["artist"][expected_field] == "the-id" + + +# --------------------------------------------------------------------------- +# Group B — Discography options contract (the bug that motivated the extract) +# --------------------------------------------------------------------------- + +class TestDiscographyOptions: + def test_dedup_variants_disabled(self, _stub_metadata): + """Source-only view must show every release variant, matching the + retired inline Artists page behaviour.""" + build_source_only_artist_detail("dz-1", "Artist", "deezer") + opts = _stub_metadata["last_options"] + assert opts is not None + assert opts.dedup_variants is False + + def test_passes_source_override_and_artist_source_ids(self, _stub_metadata): + build_source_only_artist_detail("sp-999", "Artist", "spotify") + opts = _stub_metadata["last_options"] + assert opts.source_override == "spotify" + assert opts.artist_source_ids == {"spotify": "sp-999"} + + +# --------------------------------------------------------------------------- +# Group C — Per-source enrichment +# --------------------------------------------------------------------------- + +class TestPerSourceEnrichment: + def test_spotify_extracts_genres_followers_and_image_fallback(self, _stub_metadata): + spotify = SimpleNamespace( + get_artist=lambda aid, allow_fallback=False: { + "genres": ["alt rock", "emo"], + "followers": {"total": 12345}, + "images": [{"url": "https://sp/img.jpg"}], + } + ) + payload, _ = build_source_only_artist_detail( + "sp-1", "Artist", "spotify", spotify_client=spotify, + ) + assert payload["artist"]["genres"] == ["alt rock", "emo"] + assert payload["artist"]["followers"] == 12345 + # image_url falls back to Spotify's image when metadata_service returned None + assert payload["artist"]["image_url"] == "https://sp/img.jpg" + + def test_deezer_extracts_genres_and_followers(self, _stub_metadata): + deezer = SimpleNamespace( + get_artist_info=lambda aid: { + "genres": ["pop"], + "followers": {"total": 500}, + } + ) + payload, _ = build_source_only_artist_detail( + "dz-1", "Artist", "deezer", deezer_client=deezer, + ) + assert payload["artist"]["genres"] == ["pop"] + assert payload["artist"]["followers"] == 500 + + def test_itunes_extracts_genres_only(self, _stub_metadata): + itunes = SimpleNamespace(get_artist=lambda aid: {"genres": ["rock"]}) + payload, _ = build_source_only_artist_detail( + "it-1", "Artist", "itunes", itunes_client=itunes, + ) + assert payload["artist"]["genres"] == ["rock"] + assert "followers" not in payload["artist"] + + def test_discogs_extracts_genres_only(self, _stub_metadata): + discogs = SimpleNamespace(get_artist=lambda aid: {"genres": ["jazz"]}) + payload, _ = build_source_only_artist_detail( + "dc-1", "Artist", "discogs", discogs_client=discogs, + ) + assert payload["artist"]["genres"] == ["jazz"] + + def test_client_none_is_safe(self, _stub_metadata): + """Missing client for the requested source is a no-op, not a crash.""" + payload, status = build_source_only_artist_detail( + "dz-1", "Artist", "deezer", deezer_client=None, + ) + assert status == 200 + assert payload["artist"]["genres"] == [] + + def test_client_exception_does_not_propagate(self, _stub_metadata): + """A failing source client should log and move on; the response still builds.""" + def _boom(_): + raise RuntimeError("deezer down") + + deezer = SimpleNamespace(get_artist_info=_boom) + payload, status = build_source_only_artist_detail( + "dz-1", "Artist", "deezer", deezer_client=deezer, + ) + assert status == 200 + assert payload["artist"]["genres"] == [] + + +# --------------------------------------------------------------------------- +# Group D — Last.fm enrichment +# --------------------------------------------------------------------------- + +class _FakeLastFM: + def __init__(self, api_key=None): + self.api_key = api_key + + def get_artist_info(self, name): + return { + "bio": {"content": "Long bio text", "summary": "Short summary"}, + "stats": {"listeners": "5000", "playcount": "99999"}, + "url": "https://last.fm/artist", + } + + +class TestLastFmEnrichment: + def _patch_lastfm(self, monkeypatch, cls=_FakeLastFM): + import core.lastfm_client as lastfm_module + monkeypatch.setattr(lastfm_module, "LastFMClient", cls) + + def test_lastfm_fields_populated_when_api_key_present(self, _stub_metadata, monkeypatch): + self._patch_lastfm(monkeypatch) + payload, _ = build_source_only_artist_detail( + "dz-1", "Artist", "deezer", lastfm_api_key="LFM_KEY", + ) + artist = payload["artist"] + assert artist["lastfm_bio"] == "Long bio text" + assert artist["lastfm_listeners"] == 5000 + assert artist["lastfm_playcount"] == 99999 + assert artist["lastfm_url"] == "https://last.fm/artist" + + def test_no_lastfm_when_api_key_missing(self, _stub_metadata, monkeypatch): + self._patch_lastfm(monkeypatch) + payload, _ = build_source_only_artist_detail( + "dz-1", "Artist", "deezer", lastfm_api_key=None, + ) + assert "lastfm_bio" not in payload["artist"] + assert "lastfm_listeners" not in payload["artist"] + + def test_summary_used_when_bio_content_missing(self, _stub_metadata, monkeypatch): + class _SummaryOnly(_FakeLastFM): + def get_artist_info(self, name): + return { + "bio": {"summary": "Just a summary"}, + "stats": {}, + "url": "", + } + self._patch_lastfm(monkeypatch, _SummaryOnly) + payload, _ = build_source_only_artist_detail( + "dz-1", "Artist", "deezer", lastfm_api_key="LFM_KEY", + ) + assert payload["artist"]["lastfm_bio"] == "Just a summary" + + def test_lastfm_exception_does_not_propagate(self, _stub_metadata, monkeypatch): + class _Broken(_FakeLastFM): + def get_artist_info(self, name): + raise RuntimeError("last.fm rate limited") + self._patch_lastfm(monkeypatch, _Broken) + payload, status = build_source_only_artist_detail( + "dz-1", "Artist", "deezer", lastfm_api_key="LFM_KEY", + ) + assert status == 200 + assert "lastfm_bio" not in payload["artist"] diff --git a/web_server.py b/web_server.py index a6302e66..05d577c4 100644 --- a/web_server.py +++ b/web_server.py @@ -11335,159 +11335,60 @@ def _find_library_artist_for_source(database, source, source_artist_id, artist_n def _build_source_only_artist_detail(artist_id, artist_name, source): - """Synthesize an artist-detail response from a single metadata source for an - artist that isn't in the local library. Used when `/api/artist-detail/` - is called with a `source` param and the library DB lookup misses. - - Enriches the response with whatever metadata we can pull on demand: - - Image URL via metadata_service helper - - Source-specific ID field (so the source's own service badge renders) - - Genres from the source's get_artist_info if available - - Last.fm bio + listeners + playcount + URL by artist name - """ - from core.metadata_service import ( - MetadataLookupOptions, - get_artist_detail_discography as _get_artist_detail_discography, - get_artist_image_url as _get_artist_image_url, - ) - - resolved_name = (artist_name or artist_id or '').strip() - - # 1. Image URL via the same helper /api/artist//image uses. - image_url = None - try: - image_url = _get_artist_image_url(artist_id, source_override=source) - except Exception as e: - logger.debug(f"Artist image lookup failed for {source}:{artist_id}: {e}") - - # 2. Source-side artist info (image, genres, followers depending on source). - source_genres = [] - source_followers = None - try: - if source == 'spotify' and spotify_client and spotify_client.is_spotify_authenticated(): - sp_artist = spotify_client.get_artist(artist_id, allow_fallback=False) - if sp_artist: - source_genres = sp_artist.get('genres') or [] - source_followers = (sp_artist.get('followers') or {}).get('total') - if not image_url and sp_artist.get('images'): - image_url = sp_artist['images'][0].get('url') - elif source == 'deezer': - dz = _get_deezer_client() - if dz: - dz_artist = dz.get_artist_info(artist_id) - if dz_artist: - source_genres = dz_artist.get('genres') or [] - source_followers = (dz_artist.get('followers') or {}).get('total') - elif source == 'itunes': - it = _get_itunes_client() - if it: - it_artist = it.get_artist(artist_id) - if it_artist: - source_genres = it_artist.get('genres') or [] - elif source == 'discogs': - token = config_manager.get('discogs.token', '') - if token: - dc = _get_discogs_client(token) - if dc: - dc_artist = dc.get_artist(artist_id) - if dc_artist: - source_genres = dc_artist.get('genres') or [] - except Exception as e: - logger.debug(f"Source-side artist info lookup failed for {source}:{artist_id}: {e}") - - # 3. Last.fm enrichment by artist name — bio + listeners + playcount + url. - lastfm_bio = None - lastfm_listeners = None - lastfm_playcount = None - lastfm_url = None - if resolved_name: - try: - lastfm_api_key = config_manager.get('lastfm.api_key', '') or '' - if lastfm_api_key: - from core.lastfm_client import LastFMClient - lastfm = LastFMClient(api_key=lastfm_api_key) - lf_info = lastfm.get_artist_info(resolved_name) - if lf_info: - bio_obj = lf_info.get('bio') or {} - lastfm_bio = bio_obj.get('content') or bio_obj.get('summary') - stats_obj = lf_info.get('stats') or {} - if stats_obj.get('listeners'): - try: - lastfm_listeners = int(stats_obj['listeners']) - except (ValueError, TypeError): - pass - if stats_obj.get('playcount'): - try: - lastfm_playcount = int(stats_obj['playcount']) - except (ValueError, TypeError): - pass - lastfm_url = lf_info.get('url') - except Exception as e: - logger.debug(f"Last.fm enrichment failed for {resolved_name}: {e}") - - # 4. Discography from the specified source. Skip the variant-dedup so the - # page shows every release the source returns — matches the inline - # Artists-page behaviour the user is comparing against. - discography_result = _get_artist_detail_discography( - artist_id, - artist_name=resolved_name or artist_id, - options=MetadataLookupOptions( - source_override=source, - allow_fallback=True, - skip_cache=False, - max_pages=0, - limit=50, - artist_source_ids={source: artist_id}, - dedup_variants=False, - ), - ) + """Thin wrapper around ``core.artist_source_detail.build_source_only_artist_detail``. - if not discography_result.get('success'): - return jsonify({ - "success": False, - "error": discography_result.get('error', 'Could not load discography'), - "source": source, - }), 404 + Builds the per-source client bag from web_server's module globals (each + source's module-level client + Last.fm api key), forwards to the pure + implementation in ``core/``, and wraps the (dict, status) return in + ``jsonify``. + """ + from core.artist_source_detail import build_source_only_artist_detail - artist_info = { - "id": artist_id, - "name": resolved_name or artist_id, - "image_url": image_url, - "server_source": None, # not in library - "genres": source_genres, - } + # Resolve the per-source clients defensively — the original inline code + # wrapped the whole source-side lookup in try/except so a failing + # client helper (e.g. Spotify auth probe during a rate-limit ban, + # Discogs client init error) would degrade gracefully to empty + # enrichment instead of 500-ing the request. Preserve that. + sp = None + dz = None + it = None + dc = None + try: + if spotify_client and spotify_client.is_spotify_authenticated(): + sp = spotify_client + except Exception as e: + logger.debug(f"Spotify client resolution failed: {e}") + try: + dz = _get_deezer_client() + except Exception as e: + logger.debug(f"Deezer client resolution failed: {e}") + try: + it = _get_itunes_client() + except Exception as e: + logger.debug(f"iTunes client resolution failed: {e}") + try: + discogs_token = config_manager.get('discogs.token', '') or '' + if discogs_token: + dc = _get_discogs_client(discogs_token) + except Exception as e: + logger.debug(f"Discogs client resolution failed: {e}") - # Set the source-specific ID so the corresponding service badge renders on - # the hero (e.g. source=deezer -> deezer_id; source=spotify -> spotify_artist_id). - source_id_field = _SOURCE_ID_FIELD.get(source) - if source_id_field: - artist_info[source_id_field] = artist_id - - if source_followers is not None: - artist_info['followers'] = source_followers - if lastfm_bio: - artist_info['lastfm_bio'] = lastfm_bio - if lastfm_listeners is not None: - artist_info['lastfm_listeners'] = lastfm_listeners - if lastfm_playcount is not None: - artist_info['lastfm_playcount'] = lastfm_playcount - if lastfm_url: - artist_info['lastfm_url'] = lastfm_url + try: + lastfm_api_key = config_manager.get('lastfm.api_key', '') or None + except Exception: + lastfm_api_key = None - logger.info( - f"Source-only artist-detail: {artist_info['name']} from {source} — " - f"albums={len(discography_result.get('albums', []))}, " - f"eps={len(discography_result.get('eps', []))}, " - f"singles={len(discography_result.get('singles', []))}, " - f"genres={len(source_genres)}, lastfm_bio={'yes' if lastfm_bio else 'no'}" + payload, status = build_source_only_artist_detail( + artist_id, + artist_name, + source, + spotify_client=sp, + deezer_client=dz, + itunes_client=it, + discogs_client=dc, + lastfm_api_key=lastfm_api_key, ) - - return jsonify({ - "success": True, - "artist": artist_info, - "discography": discography_result, - "enrichment_coverage": {}, - }) + return jsonify(payload), status @app.route('/api/artist-detail/')