From 45ecf2730ddb6f6d413cbfe247685008e77ea5c7 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Wed, 27 May 2026 16:47:26 -0700 Subject: [PATCH] =?UTF-8?q?Wishlist:=20harden=20Spotify=20backfill=20?= =?UTF-8?q?=E2=80=94=20poisoned=20tn=3D1=20can't=20mask=20lean=20album?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Residual per-track wishlist downloads (single tracks from different albums, below the album-bundle threshold) were producing folders without a year subfolder whenever the wishlist row carried a stale ``track_number=1`` from an older payload default. Why: ``core/downloads/candidates.py`` had a single API-fetch branch that served two concerns — resolving the track position AND hydrating the lean ``spotify_album_context`` (release_date / total_tracks / cover image) — gated entirely on track_number being unresolved. When the wishlist row's ``track_number`` happened to be 1 (a poisoned default rather than a real value), the gate short-circuited and the album hydration the same call would have done was skipped. Deezer-sourced discovery matches don't ship release_date in their search-result album shape, so without the backfill the folder lost its year. The two concerns split: - track_number resolution keeps its track_info → track object → API precedence chain. track_info defaults still win. - album hydration runs whenever release_date or total_tracks are missing, independent of where (or whether) track_number was resolved. The single API round-trip still serves both — the cost contract is preserved. The side-effect coupling is gone. Lifted into ``core/downloads/track_metadata_backfill.py`` (``hydrate_download_metadata``) so the precedence chain is pinned in isolation. 24 unit tests cover the precedence chain, the poisoned-tn=1 regression case, defensive non-dict/None inputs, the cost guard (API called at most once per invocation), and disc_number resolution. Also lands the upstream piece: ``core/wishlist/routes.py:_build_track_data`` no longer defaults ``track_number=1`` / ``disc_number=1`` / ``total_tracks=1`` / ``release_date=''`` when the library-modal add payload omits them. Missing values now flow through as ``None`` so the downstream pipeline can detect-and-recover instead of locking to a fake position. --- core/downloads/candidates.py | 74 +-- core/downloads/track_metadata_backfill.py | 175 ++++++ core/wishlist/routes.py | 34 +- .../downloads/test_track_metadata_backfill.py | 511 ++++++++++++++++++ webui/static/helper.js | 1 + 5 files changed, 743 insertions(+), 52 deletions(-) create mode 100644 core/downloads/track_metadata_backfill.py create mode 100644 tests/downloads/test_track_metadata_backfill.py diff --git a/core/downloads/candidates.py b/core/downloads/candidates.py index dd85455a..9510a0f5 100644 --- a/core/downloads/candidates.py +++ b/core/downloads/candidates.py @@ -36,6 +36,7 @@ import os from dataclasses import dataclass from typing import Any, Callable +from core.downloads.track_metadata_backfill import hydrate_download_metadata from core.runtime_state import ( download_tasks, matched_context_lock, @@ -247,55 +248,30 @@ def attempt_download_with_candidates(task_id, candidates, track, batch_id=None, enhanced_payload['artists'] = [{'name': artist} for artist in track.artists] if track.artists else [] logger.info(f"[Context] Using clean Spotify metadata - Album: '{track.album}', Title: '{track.name}'") - # Get track_number and disc_number — prefer track data we already have, - # fall back to detailed API call only if needed - got_track_number = False - - # 1. Try track_info (from frontend, has album track data) - tn = track_info.get('track_number', 0) if isinstance(track_info, dict) else 0 - dn = (track_info.get('disc_number') or 1) if isinstance(track_info, dict) else 1 - if tn and tn > 0: - enhanced_payload['track_number'] = tn - enhanced_payload['disc_number'] = dn - got_track_number = True - logger.info(f"[Context] Added track_number from track_info: {tn}, disc_number: {dn}") - - # 2. Try the track object itself (from album tracks response) - if not got_track_number and hasattr(track, 'track_number') and track.track_number: - enhanced_payload['track_number'] = track.track_number - enhanced_payload['disc_number'] = getattr(track, 'disc_number', 1) or 1 - got_track_number = True - logger.info(f"[Context] Added track_number from track object: {track.track_number}, disc_number: {enhanced_payload['disc_number']}") - - # 3. Last resort — fetch from metadata source API - if not got_track_number and hasattr(track, 'id') and track.id: - try: - detailed_track = deps.spotify_client.get_track_details(track.id) - if detailed_track and detailed_track.get('track_number'): - enhanced_payload['track_number'] = detailed_track['track_number'] - enhanced_payload['disc_number'] = detailed_track.get('disc_number') or 1 - got_track_number = True - logger.info(f"[Context] Added track_number from API: {detailed_track['track_number']}, disc_number: {enhanced_payload['disc_number']}") - - # Backfill album metadata from detailed track when context - # has incomplete data (missing release_date, total_tracks, etc.) - if isinstance(detailed_track.get('album'), dict): - dt_album = detailed_track['album'] - if not spotify_album_context.get('release_date') and dt_album.get('release_date'): - spotify_album_context['release_date'] = dt_album['release_date'] - logger.info(f"[Context] Backfilled release_date from API: {dt_album['release_date']}") - if not spotify_album_context.get('album_type') and dt_album.get('album_type'): - spotify_album_context['album_type'] = dt_album['album_type'] - if not spotify_album_context.get('total_tracks') and dt_album.get('total_tracks'): - spotify_album_context['total_tracks'] = dt_album['total_tracks'] - if not spotify_album_context.get('id') and dt_album.get('id'): - spotify_album_context['id'] = dt_album['id'] - if not spotify_album_context.get('image_url') and dt_album.get('images'): - spotify_album_context['image_url'] = dt_album['images'][0].get('url', '') - except Exception as e: - logger.error(f"[Context] API track details failed: {e}") - - if not got_track_number: + # Resolve track_number / disc_number and hydrate + # lean album context. Extracted to + # track_metadata_backfill.hydrate_download_metadata + # — see that module for the precedence chain. + # Why the extract: the inline pre-fix coupled + # album-backfill to the "track_number missing" + # branch. When wishlist payloads carried a poisoned + # default-1 track_number (older routes.py used + # ``.get('track_number', 1)``) the API call short- + # circuited and the lean album_context (no + # release_date / total_tracks for Deezer-sourced + # discovery matches) survived untouched, producing + # folders without a year subfolder. + resolved = hydrate_download_metadata( + track, track_info, spotify_album_context, deps.spotify_client, + ) + if resolved.track_number is not None: + enhanced_payload['track_number'] = resolved.track_number + enhanced_payload['disc_number'] = resolved.disc_number + logger.info( + f"[Context] Added track_number from {resolved.source}: " + f"{resolved.track_number}, disc_number: {resolved.disc_number}" + ) + else: enhanced_payload.setdefault('track_number', 0) enhanced_payload.setdefault('disc_number', 1) logger.warning("[Context] No track_number found from any source") diff --git a/core/downloads/track_metadata_backfill.py b/core/downloads/track_metadata_backfill.py new file mode 100644 index 00000000..da7fac0e --- /dev/null +++ b/core/downloads/track_metadata_backfill.py @@ -0,0 +1,175 @@ +"""Track-position resolution + album-context hydration helper. + +Lifted out of ``core/downloads/candidates.py`` to break a quiet +regression. The pre-extract code fetched detailed track data from +Spotify only when ``track_number`` was missing — the same API call +*also* backfilled the lean ``spotify_album_context`` (release_date, +total_tracks, image_url) but only as a side-effect of the +track_number branch. When a wishlist row carried a poisoned +``track_number=1`` (older payload helpers defaulted missing values +to 1), the conditional short-circuited, the API call never fired, +and the album context stayed lean — producing folders without a +year subfolder for residual per-track wishlist downloads. + +The fix splits the two concerns: ``track_number`` resolution +follows its precedence chain (track_info → track object → API), +but album hydration runs whenever ``spotify_album_context`` is +missing any of release_date / total_tracks regardless of whether +``track_number`` was already known. A single API call still serves +both — the side-effect coupling is gone but the network cost +isn't paid twice. +""" + +from __future__ import annotations + +import logging +from typing import Any, Dict, NamedTuple, Optional + +logger = logging.getLogger(__name__) + + +class ResolvedTrackMetadata(NamedTuple): + """Result of ``hydrate_download_metadata``. + + ``track_number`` is ``None`` when every source — track_info, + track object, API — failed to produce a positive integer. The + caller (candidates.py) treats ``None`` as "fall back to the + setdefault(0)" path so the existing 0-floor sentinel behaviour + is preserved. + """ + + track_number: Optional[int] + disc_number: int + source: str # 'track_info' | 'track_object' | 'api' | 'none' + + +def _positive_int(value: Any) -> Optional[int]: + if isinstance(value, bool): + return None + if isinstance(value, int): + return value if value > 0 else None + if isinstance(value, str) and value.isdigit(): + coerced = int(value) + return coerced if coerced > 0 else None + return None + + +def _album_is_lean(album_context: Any) -> bool: + """An album context is "lean" when it lacks release_date OR + total_tracks. Both fields are required downstream: + ``release_date`` drives the year folder, ``total_tracks`` drives + the TRCK tag denominator + the album-vs-single classification + in ``build_import_album_info``.""" + if not isinstance(album_context, dict): + return True + if not album_context.get('release_date'): + return True + if not album_context.get('total_tracks'): + return True + return False + + +def _backfill_album_context( + album_context: Dict[str, Any], detailed_track: Dict[str, Any] +) -> None: + """Copy missing fields from ``detailed_track['album']`` into the + in-place album_context dict. Existing values are preserved — + never overwritten — because the caller's context may carry + fields the API doesn't (e.g. enhanced search shapes can include + artists arrays the bare track endpoint omits).""" + dt_album = detailed_track.get('album') + if not isinstance(dt_album, dict) or not isinstance(album_context, dict): + return + + for key in ('release_date', 'album_type', 'total_tracks', 'id'): + if not album_context.get(key) and dt_album.get(key): + album_context[key] = dt_album[key] + + if not album_context.get('image_url'): + images = dt_album.get('images') + if isinstance(images, list) and images: + first = images[0] + if isinstance(first, dict) and first.get('url'): + album_context['image_url'] = first['url'] + + +def hydrate_download_metadata( + track: Any, + track_info: Any, + spotify_album_context: Dict[str, Any], + spotify_client: Any, +) -> ResolvedTrackMetadata: + """Resolve track position and hydrate lean album context. + + Steps: + 1. ``track_info['track_number']`` when positive + 2. ``track.track_number`` when truthy + 3. ``spotify_client.get_track_details(track.id)`` — fires when + EITHER track_number unresolved OR album_context lean. The + same call serves both concerns; only one round-trip per task. + + ``spotify_album_context`` is mutated in place when API returns + richer data. Returns the resolved track_number / disc_number / + source. ``track_number=None`` signals "no usable value found"; + the caller decides whether to floor it to 1 or leave 0. + """ + ti = track_info if isinstance(track_info, dict) else {} + + # Step 1: track_info top-level — wishlist + frontend payloads. + tn = _positive_int(ti.get('track_number')) + if tn is not None: + dn = _positive_int(ti.get('disc_number')) or 1 + source = 'track_info' + else: + tn = None + dn = 1 + source = 'none' + + # Step 2: track object — Spotify Track dataclass from search. + if tn is None: + track_tn = getattr(track, 'track_number', None) + coerced = _positive_int(track_tn) + if coerced is not None: + tn = coerced + dn = _positive_int(getattr(track, 'disc_number', None)) or 1 + source = 'track_object' + + needs_api_for_tn = tn is None + needs_api_for_album = _album_is_lean(spotify_album_context) + track_id = getattr(track, 'id', None) + + if (needs_api_for_tn or needs_api_for_album) and track_id: + try: + detailed = spotify_client.get_track_details(track_id) + except Exception as e: # noqa: BLE001 — defensive log + continue + logger.error("[Context] API track details failed: %s", e) + detailed = None + + if isinstance(detailed, dict): + if needs_api_for_tn: + api_tn = _positive_int(detailed.get('track_number')) + if api_tn is not None: + tn = api_tn + dn = _positive_int(detailed.get('disc_number')) or 1 + source = 'api' + logger.info( + "[Context] Resolved track_number=%d disc_number=%d from API", + tn, dn, + ) + + if needs_api_for_album and isinstance(spotify_album_context, dict): + _backfill_album_context(spotify_album_context, detailed) + logger.info( + "[Context] Backfilled album context from API " + "(release_date=%r, total_tracks=%r)", + spotify_album_context.get('release_date'), + spotify_album_context.get('total_tracks'), + ) + + return ResolvedTrackMetadata(track_number=tn, disc_number=dn, source=source) + + +__all__ = [ + 'ResolvedTrackMetadata', + 'hydrate_download_metadata', +] diff --git a/core/wishlist/routes.py b/core/wishlist/routes.py index 94675a68..53a36af2 100644 --- a/core/wishlist/routes.py +++ b/core/wishlist/routes.py @@ -47,6 +47,26 @@ def _build_album_images(album: Dict[str, Any]) -> list[dict[str, Any]]: def _build_track_data(track: Dict[str, Any], album: Dict[str, Any]) -> Dict[str, Any]: + """Project a wishlist-modal add payload into the canonical + wishlist track shape. + + Pre-fix this used default values (``track_number=1``, + ``disc_number=1``, ``total_tracks=1``, ``release_date=''``) when + the upstream UI omitted a field. That silently poisoned every + wishlist row added from the library "add to wishlist" modal: + track_number locked to 1 regardless of source position, year + dropped from folder paths because release_date was empty. The + library modal flow is what's used for "add this album's missing + tracks to wishlist" and "add this playlist to wishlist" bulk + actions — the most common user path, so the regression was + everywhere. + + Now preserve missing values explicitly (None for numeric + positions, omit-or-empty for release_date) so the downstream + import pipeline can detect-and-recover via + ``core/imports/track_number.py:resolve_track_number`` instead + of locking to 1. + """ album_images = _build_album_images(album) return { "id": track.get("id"), @@ -58,12 +78,20 @@ def _build_track_data(track: Dict[str, Any], album: Dict[str, Any]) -> Dict[str, "artists": album.get("artists", []), "images": album_images, "album_type": album.get("album_type", "album"), + # release_date stays as whatever the upstream sent + # (including '' when truly unknown). Path template + # gracefully omits the year when empty; we don't fake + # a date. "release_date": album.get("release_date", ""), - "total_tracks": album.get("total_tracks", 1), + # total_tracks=None preserves "we don't know"; UI uses + # this for category classification + path math. Pre-fix + # default of 1 mislabelled multi-track albums as singles. + "total_tracks": album.get("total_tracks"), }, "duration_ms": track.get("duration_ms", 0), - "track_number": track.get("track_number", 1), - "disc_number": track.get("disc_number", 1), + # Numeric positions: None when missing, not 1. + "track_number": track.get("track_number"), + "disc_number": track.get("disc_number"), "explicit": track.get("explicit", False), "popularity": track.get("popularity", 0), "preview_url": track.get("preview_url"), diff --git a/tests/downloads/test_track_metadata_backfill.py b/tests/downloads/test_track_metadata_backfill.py new file mode 100644 index 00000000..99f77351 --- /dev/null +++ b/tests/downloads/test_track_metadata_backfill.py @@ -0,0 +1,511 @@ +"""Tests for ``core/downloads/track_metadata_backfill.py``. + +Real-world regression these tests pin: wishlist rows carrying a +poisoned ``track_number=1`` (older payload helpers defaulted +missing values to 1) used to prevent the Spotify-API backfill +that hydrated lean ``spotify_album_context`` (release_date, +total_tracks). Result: residual per-track wishlist downloads +produced folders without a year subfolder when the wishlist row +came from a Deezer-sourced discovery match. + +The split-concern fix: + - ``track_number`` precedence: track_info → track object → API + - album hydration: runs whenever release_date / total_tracks + missing, INDEPENDENT of whether track_number was already known + - single API call serves both — no double round-trip +""" + +from __future__ import annotations + +from dataclasses import dataclass +from typing import Any, Optional +from unittest.mock import MagicMock + +import pytest + +from core.downloads.track_metadata_backfill import ( + ResolvedTrackMetadata, + hydrate_download_metadata, +) + + +@dataclass +class _FakeTrack: + """Stand-in for the Spotify Track dataclass — only the attrs the + backfill helper reads.""" + + id: Optional[str] = 'spotify_track_id' + track_number: Optional[int] = None + disc_number: Optional[int] = None + + +def _api_payload( + track_number: Optional[int] = 5, + disc_number: int = 1, + release_date: str = '2013-10-22', + total_tracks: int = 16, + album_id: str = 'spotify_album_id', + image_url: str = 'https://i.scdn.co/cover.jpg', + album_type: str = 'album', +) -> dict: + """Build a plausible ``spotify_client.get_track_details`` response.""" + images: list[dict[str, Any]] = [] + if image_url: + images.append({'url': image_url, 'height': 640, 'width': 640}) + return { + 'id': 'spotify_track_id', + 'name': 'Roar', + 'track_number': track_number, + 'disc_number': disc_number, + 'album': { + 'id': album_id, + 'name': 'PRISM (Deluxe)', + 'release_date': release_date, + 'total_tracks': total_tracks, + 'album_type': album_type, + 'images': images, + }, + } + + +# --------------------------------------------------------------------------- +# Track-number precedence chain. +# --------------------------------------------------------------------------- + + +def test_track_info_track_number_wins_over_track_object(): + """track_info has a real value → use it, skip lower-priority sources.""" + client = MagicMock() + album_ctx = {'release_date': '2013-10-22', 'total_tracks': 16} + + resolved = hydrate_download_metadata( + _FakeTrack(track_number=9), # would resolve to 9 if track_info empty + {'track_number': 5, 'disc_number': 2}, + album_ctx, + client, + ) + + assert resolved == ResolvedTrackMetadata(track_number=5, disc_number=2, source='track_info') + client.get_track_details.assert_not_called() + + +def test_track_object_used_when_track_info_missing(): + """track_info has no track_number → fall to track.track_number.""" + client = MagicMock() + album_ctx = {'release_date': '2013-10-22', 'total_tracks': 16} + + resolved = hydrate_download_metadata( + _FakeTrack(track_number=7, disc_number=2), + {}, # no track_number key + album_ctx, + client, + ) + + assert resolved == ResolvedTrackMetadata(track_number=7, disc_number=2, source='track_object') + client.get_track_details.assert_not_called() + + +def test_api_used_when_track_info_and_track_object_missing(): + """No local source has track_number → fire the API.""" + client = MagicMock() + client.get_track_details.return_value = _api_payload(track_number=8, disc_number=1) + album_ctx = {'release_date': '2013-10-22', 'total_tracks': 16} + + resolved = hydrate_download_metadata( + _FakeTrack(track_number=None), + {}, + album_ctx, + client, + ) + + assert resolved == ResolvedTrackMetadata(track_number=8, disc_number=1, source='api') + client.get_track_details.assert_called_once_with('spotify_track_id') + + +def test_zero_track_number_in_track_info_treated_as_missing(): + """track_info.track_number=0 is a sentinel for "missing", not a + valid position — fall through to the next source.""" + client = MagicMock() + client.get_track_details.return_value = _api_payload(track_number=3) + album_ctx = {'release_date': '2013-10-22', 'total_tracks': 16} + + resolved = hydrate_download_metadata( + _FakeTrack(track_number=None), + {'track_number': 0}, + album_ctx, + client, + ) + + assert resolved.track_number == 3 + assert resolved.source == 'api' + + +def test_string_track_number_coerced_to_int(): + """Some legacy payloads pass track_number as a string. Coerce, not reject.""" + client = MagicMock() + album_ctx = {'release_date': '2013-10-22', 'total_tracks': 16} + + resolved = hydrate_download_metadata( + _FakeTrack(), + {'track_number': '11'}, + album_ctx, + client, + ) + + assert resolved.track_number == 11 + assert resolved.source == 'track_info' + + +def test_boolean_track_number_rejected_not_treated_as_int(): + """Python ints include bools — ``True == 1`` would erroneously + pass the >0 gate. Reject explicitly so a malformed payload + falls through instead of being mistaken for track 1.""" + client = MagicMock() + client.get_track_details.return_value = _api_payload(track_number=4) + album_ctx = {'release_date': '2013-10-22', 'total_tracks': 16} + + resolved = hydrate_download_metadata( + _FakeTrack(), + {'track_number': True}, + album_ctx, + client, + ) + + assert resolved.track_number == 4 + assert resolved.source == 'api' + + +# --------------------------------------------------------------------------- +# THE REGRESSION FIX: album backfill runs even when track_number known. +# --------------------------------------------------------------------------- + + +def test_poisoned_default_track_number_does_NOT_block_album_backfill(): + """Regression pin. track_info carries the poisoned ``track_number=1`` + legacy default; album_context is lean (Deezer-sourced discovery + match, no release_date, no total_tracks). Pre-fix: API skipped + because tn>0, folder lost year. Post-fix: API still fires for + album hydration; track_number stays at 1 (caller's precedence), + but release_date / total_tracks now populate.""" + client = MagicMock() + client.get_track_details.return_value = _api_payload( + track_number=4, # the API knows the *real* track number + release_date='2007-03-12', + total_tracks=12, + ) + album_ctx = {'name': 'Welcome Interstate Managers'} # lean + + resolved = hydrate_download_metadata( + _FakeTrack(id='spotify_track_id', track_number=None), + {'track_number': 1}, # POISONED default + album_ctx, + client, + ) + + # track_number stays at 1 — track_info precedence is sacred, + # we don't second-guess it. The album fix is what matters. + assert resolved.track_number == 1 + assert resolved.source == 'track_info' + + # CRITICAL: album_context got hydrated despite track_number being "resolved". + assert album_ctx['release_date'] == '2007-03-12' + assert album_ctx['total_tracks'] == 12 + + # API call still fired — for the album, even though tn already known. + client.get_track_details.assert_called_once_with('spotify_track_id') + + +def test_rich_album_context_skips_api_when_track_number_resolved(): + """Cost guard. Both concerns satisfied locally → no API call. + Keeps the network-cost contract from the pre-extract code.""" + client = MagicMock() + album_ctx = { + 'name': 'PRISM (Deluxe)', + 'release_date': '2013-10-22', + 'total_tracks': 16, + } + + resolved = hydrate_download_metadata( + _FakeTrack(), + {'track_number': 5}, + album_ctx, + client, + ) + + assert resolved.track_number == 5 + client.get_track_details.assert_not_called() + + +def test_api_fires_for_lean_album_even_with_track_object_track_number(): + """track_number from the Track dataclass (search-result level) but + album_context still lean — API must fire for album hydration.""" + client = MagicMock() + client.get_track_details.return_value = _api_payload() + album_ctx = {'name': 'PRISM (Deluxe)'} # lean + + resolved = hydrate_download_metadata( + _FakeTrack(track_number=9, disc_number=1), + {}, + album_ctx, + client, + ) + + assert resolved.track_number == 9 + assert resolved.source == 'track_object' + assert album_ctx['release_date'] == '2013-10-22' + assert album_ctx['total_tracks'] == 16 + client.get_track_details.assert_called_once() + + +def test_album_with_only_release_date_still_triggers_backfill_for_total_tracks(): + """release_date populated but total_tracks missing → still lean → + API fires to fill total_tracks. Either missing field triggers.""" + client = MagicMock() + client.get_track_details.return_value = _api_payload() + album_ctx = {'release_date': '2013-10-22'} # total_tracks missing + + hydrate_download_metadata(_FakeTrack(), {'track_number': 5}, album_ctx, client) + + assert album_ctx['total_tracks'] == 16 + client.get_track_details.assert_called_once() + + +def test_album_with_only_total_tracks_still_triggers_backfill_for_release_date(): + """Inverse: total_tracks present but release_date missing → still lean.""" + client = MagicMock() + client.get_track_details.return_value = _api_payload() + album_ctx = {'total_tracks': 16} # release_date missing + + hydrate_download_metadata(_FakeTrack(), {'track_number': 5}, album_ctx, client) + + assert album_ctx['release_date'] == '2013-10-22' + client.get_track_details.assert_called_once() + + +# --------------------------------------------------------------------------- +# Album backfill: preserve existing values, never overwrite. +# --------------------------------------------------------------------------- + + +def test_album_backfill_does_not_overwrite_existing_release_date(): + """Caller's release_date is sacred — only fill when absent. Source + of truth may legitimately diverge between context and API (e.g. + region-specific releases). Don't second-guess the caller.""" + client = MagicMock() + client.get_track_details.return_value = _api_payload(release_date='2099-01-01') + album_ctx = {'release_date': '2013-10-22', 'total_tracks': 16, 'name': 'Real'} + + hydrate_download_metadata(_FakeTrack(), {'track_number': 1}, album_ctx, client) + + # Not lean → API never called. + client.get_track_details.assert_not_called() + assert album_ctx['release_date'] == '2013-10-22' + + +def test_album_backfill_fills_missing_image_url_from_images_array(): + """API responses use the Spotify ``images`` array shape. Helper + flattens to the ``image_url`` string the path builder reads.""" + client = MagicMock() + client.get_track_details.return_value = _api_payload( + image_url='https://i.scdn.co/cover.jpg', + ) + album_ctx = {'name': 'PRISM'} # lean + + hydrate_download_metadata(_FakeTrack(), {}, album_ctx, client) + + assert album_ctx['image_url'] == 'https://i.scdn.co/cover.jpg' + + +def test_album_backfill_handles_empty_images_array_gracefully(): + """API response with empty ``images`` array (rare but possible + on tracks the album-cover service hasn't indexed yet) must not + crash the resolver.""" + client = MagicMock() + payload = _api_payload(image_url='') # produces images=[] + client.get_track_details.return_value = payload + album_ctx = {'name': 'PRISM'} + + hydrate_download_metadata(_FakeTrack(), {}, album_ctx, client) + + # No image_url set, but other fields hydrated. + assert 'image_url' not in album_ctx + assert album_ctx['release_date'] == '2013-10-22' + + +def test_album_backfill_skips_when_detailed_album_not_dict(): + """API response with a non-dict ``album`` field (defensive — old + enrichment caches stored string album names) is safely skipped.""" + client = MagicMock() + client.get_track_details.return_value = { + 'id': 'spotify_track_id', + 'track_number': 3, + 'album': 'Just a string', + } + album_ctx = {'name': 'PRISM'} + + resolved = hydrate_download_metadata(_FakeTrack(), {}, album_ctx, client) + + # track_number resolved, album_context untouched. + assert resolved.track_number == 3 + assert 'release_date' not in album_ctx + + +# --------------------------------------------------------------------------- +# Defensive: missing IDs, exceptions, non-dict inputs. +# --------------------------------------------------------------------------- + + +def test_no_track_id_skips_api_call(): + """Without ``track.id`` the API can't be queried. Returns whatever + was resolved locally, no exception.""" + client = MagicMock() + album_ctx = {'name': 'PRISM'} # lean — but no ID to fix it + + resolved = hydrate_download_metadata( + _FakeTrack(id=None), + {}, + album_ctx, + client, + ) + + assert resolved.track_number is None + assert resolved.source == 'none' + client.get_track_details.assert_not_called() + + +def test_api_exception_does_not_propagate(): + """Network blip / Spotify 5xx must not crash the download pipeline. + Returns whatever was resolved before the failed call.""" + client = MagicMock() + client.get_track_details.side_effect = RuntimeError('429 rate limited') + album_ctx = {'name': 'PRISM'} # lean + + resolved = hydrate_download_metadata( + _FakeTrack(), + {'track_number': 5}, # already have tn from track_info + album_ctx, + client, + ) + + assert resolved.track_number == 5 + assert 'release_date' not in album_ctx # backfill failed silently + + +def test_api_returns_none_does_not_mutate_context(): + """API call that returns None (no result for this Spotify ID) is + a no-op for album hydration.""" + client = MagicMock() + client.get_track_details.return_value = None + album_ctx = {'name': 'PRISM'} + + resolved = hydrate_download_metadata(_FakeTrack(), {}, album_ctx, client) + + assert resolved.track_number is None + assert 'release_date' not in album_ctx + + +def test_non_dict_track_info_treated_as_empty(): + """Defensive — a malformed task payload with a non-dict track_info + falls through to the track object / API instead of crashing.""" + client = MagicMock() + client.get_track_details.return_value = _api_payload(track_number=6) + album_ctx = {'release_date': '2013-10-22', 'total_tracks': 16} + + resolved = hydrate_download_metadata( + _FakeTrack(), + 'not a dict', # type: ignore[arg-type] + album_ctx, + client, + ) + + assert resolved.source == 'api' + assert resolved.track_number == 6 + + +def test_non_dict_album_context_treated_as_lean(): + """Defensive — a None album_context is treated as lean, so the API + still fires but mutation is silently skipped (helper can't mutate + a non-dict in place).""" + client = MagicMock() + client.get_track_details.return_value = _api_payload() + + # Should not raise — passing None for album_context just means + # the backfill no-ops (can't mutate None) but the call signature + # is preserved. + resolved = hydrate_download_metadata( + _FakeTrack(), + {'track_number': 5}, + None, # type: ignore[arg-type] + client, + ) + + # tn resolved from track_info, no crash. + assert resolved.track_number == 5 + + +# --------------------------------------------------------------------------- +# disc_number resolution. +# --------------------------------------------------------------------------- + + +def test_disc_number_from_track_info_paired_with_track_number(): + """When track_info supplies track_number, its disc_number rides along.""" + client = MagicMock() + album_ctx = {'release_date': '2013-10-22', 'total_tracks': 16} + + resolved = hydrate_download_metadata( + _FakeTrack(), + {'track_number': 5, 'disc_number': 2}, + album_ctx, + client, + ) + + assert resolved.disc_number == 2 + + +def test_disc_number_defaults_to_1_when_only_track_number_present(): + """track_info has track_number but no disc_number → disc=1.""" + client = MagicMock() + album_ctx = {'release_date': '2013-10-22', 'total_tracks': 16} + + resolved = hydrate_download_metadata( + _FakeTrack(), + {'track_number': 5}, + album_ctx, + client, + ) + + assert resolved.disc_number == 1 + + +def test_disc_number_zero_from_api_floored_to_1(): + """API response with disc_number=0 (some niche releases) gets + floored to 1 — albums are 1-indexed.""" + client = MagicMock() + client.get_track_details.return_value = _api_payload(track_number=5, disc_number=0) + album_ctx = {'release_date': '2013-10-22', 'total_tracks': 16} + + resolved = hydrate_download_metadata(_FakeTrack(), {}, album_ctx, client) + + assert resolved.disc_number == 1 + + +# --------------------------------------------------------------------------- +# Cost guard: API called at most once. +# --------------------------------------------------------------------------- + + +def test_api_called_at_most_once_per_invocation(): + """Even when API serves both concerns (track_number AND album), + only one ``get_track_details`` call is made.""" + client = MagicMock() + client.get_track_details.return_value = _api_payload() + album_ctx = {'name': 'PRISM'} # lean + + hydrate_download_metadata(_FakeTrack(track_number=None), {}, album_ctx, client) + + assert client.get_track_details.call_count == 1 + + +if __name__ == '__main__': + pytest.main([__file__, '-v']) diff --git a/webui/static/helper.js b/webui/static/helper.js index 801d5af1..bb73a5ea 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3415,6 +3415,7 @@ function closeHelperSearch() { const WHATS_NEW = { '2.6.3': [ { unreleased: true }, + { title: 'Wishlist: harden Spotify backfill so a poisoned track_number can\'t mask a lean album', desc: 'follow-up to the earlier wishlist import-path work. residual per-track wishlist downloads (single tracks from different albums, falling below the album-bundle threshold) were producing folders without a year subfolder whenever the underlying wishlist row had a track_number=1 from an older default — the candidate dispatcher\'s "fall back to Spotify API" branch was gated entirely on track_number being missing, but the same API call was also the only thing that hydrated the lean album_context (release_date / total_tracks / cover art) when the original discovery match came from Deezer\'s search endpoint which doesn\'t carry those fields. so any row whose track_number happened to look "filled" (1 from a default) would short-circuit the API call and the year disappeared from the folder path even though the API knew it. split the two concerns: track_number resolution keeps its track_info → track object → API precedence, but album hydration now runs whenever release_date or total_tracks are missing regardless of where track_number came from. one network round-trip still serves both. lifted into core/downloads/track_metadata_backfill.py with 24 unit tests pinning every branch — including the regression: poisoned default-1 track_number does NOT block album backfill anymore.' }, { title: 'Wishlist: fix imports landing as track 01 with no year in folder name', desc: 'follow-up to the earlier wishlist album-bundle work. tracks with rich Spotify metadata were still importing as `01 - ` with no year in the folder path because three regressions stacked: the Track→dict conversion in payload helpers dropped everything except `album.name` (silently throwing away release_date / images / album_type / total_tracks), Deezer-sourced discovery matches saved their payloads without `track_number` / `disc_number` keys at all (Deezer uses `track_position` while the cache lookup read `track_number` literally), and the import pipeline only consulted `album_info.track_number` before falling to the filename — which fails for VA-collection source files like `417 Fountains of Wayne - Stacys Mom.flac` where the leading number is a playlist position not the album track. all three patched, with the track_number resolution chain lifted into `core/imports/track_number.py` as a pure function with 18 unit tests pinning every branch.' }, { title: 'Wishlist: distinguish Queued from Analyzing batches in the UI', desc: 'wishlist runs with more than 3 sub-batches used to render every batch as "Analyzing..." simultaneously — even though the download worker pool only runs 3 at a time. so a 26-album wishlist scan looked like all 26 were working in parallel when really 23 were just sitting in the executor queue waiting their turn. now batches show a distinct "Queued ⏳" state while parked in the executor queue; they flip to "Analyzing..." when a worker actually picks them up. zero behavior change — just less misleading UI.' }, { title: 'Wishlist: only engage album-bundle when several tracks from the same album are missing', desc: 'auto-wishlist + manual-wishlist runs were promoting every single-track wishlist item to a per-album bundle search — so a wishlist of "26 single tracks from 26 different albums" downloaded full albums (5-42 files each, ~85% wasted bandwidth) and hammered slskd with concurrent searches. now the bundle path only engages when an album has 2 or more missing tracks in the wishlist; single-track items take the cheaper per-track path that already works fine. fewer slskd searches per cycle, less wasted bandwidth, no more downloading the same album three times in a row when staging-match misses a single track. configurable via `wishlist.album_bundle_min_tracks` if you want the old behaviour (set to 1) or stricter (set to 3+).' },