mirror of https://github.com/Nezreka/SoulSync.git
JohnBaumb's review: "If we're going to refactor the web_server.py soon, might as well start moving stuff away from web_server.py in our PRs. _build_source_only_artist_detail, make it a module, it's perfect." This continues the pattern the prior commit started with the source-ID lookup helpers: move the pure data-building logic to a side-effect-free core module, leave a thin wrapper in web_server.py that bridges the Flask response and the module-global clients. **core/artist_source_detail.py** — pure function that takes the artist id, name, and source plus dependency-injected per-source clients (spotify, deezer, itunes, discogs) and a Last.fm API key. Returns (payload_dict, http_status) so it isn't coupled to Flask. **web_server.py wrapper** — builds the client bag from the module globals (checks Spotify auth, constructs the Discogs client from the configured token, reads the Last.fm API key) and wraps the core return in jsonify. 147 lines of logic go away from web_server.py; the 24-line wrapper is purely glue. **tests/test_artist_source_detail.py** — 21 focused tests covering the response envelope, the source-specific ID-field stamping for all six supported sources, the dedup_variants=False contract (the behaviour that originally motivated the split of MetadataLookupOptions), per-source genre/follower extraction with safe handling of missing or throwing clients, and the Last.fm enrichment branch including the no-key and error-path cases. Runtime 0.26s.pull/361/head
Broque Thomas
1 month ago
parent
b547909604
commit
14893c85a9
@ -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/<id>`` 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/<id>/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
|
||||
@ -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"]
|
||||
Loading…
Reference in new issue