From c9d4b02a0204318a3d94f1334f990a1236398002 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Thu, 14 May 2026 11:10:51 -0700 Subject: [PATCH] Fix Deezer contributors tagging silently dropping for cache-polluted tracks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #588. Contributing-artist tagging worked for some tracks but silently dropped them for others — most reproducibly when the album had been fetched before the per-track post-process ran. Trace: get_track_details cache check used `track_position in cached` as the "full payload" sentinel. Both `/track/` AND `/album//tracks` set track_position. Only `/track/` sets the `contributors` array. When album-tracks data hit the cache first, get_track_details returned the partial record → _build_enhanced_track found no contributors → metadata-source contributors-upgrade silently fell back to single-artist. Reporter's case (Andrea Botez - Sacrifice): the album fetch logged "Retrieved 4 tracks for album 673558211" before the post-process, which cached all 4 tracks as partial records. The contributors- upgrade then hit the partial cache and the upgrade log line never fired because len(upgraded) was never > 1. Lifted cache-validity to a pure helper `_is_full_track_payload` that requires BOTH `track_position` AND `contributors` key presence. Empty list `[]` is valid — single-artist tracks fetched via `/track/` carry it explicitly. Partial cache hits fall through to a fresh `/track/` fetch, which writes the full payload back to cache. 11 boundary tests pin every shape: full payload, single-artist with empty contributors list, partial album-tracks shape, search-result shape, none/non-dict, and the cache-hit/cache-miss/api-failure paths on get_track_details (including the exact reporter-scenario regression). Full suite: 3021 passed. --- core/deezer_client.py | 38 +++- .../test_deezer_track_cache_validity.py | 208 ++++++++++++++++++ webui/static/helper.js | 1 + 3 files changed, 239 insertions(+), 8 deletions(-) create mode 100644 tests/metadata/test_deezer_track_cache_validity.py diff --git a/core/deezer_client.py b/core/deezer_client.py index e564663c..07162228 100644 --- a/core/deezer_client.py +++ b/core/deezer_client.py @@ -89,6 +89,33 @@ def _upgrade_deezer_cover_url(url: str, target_size: int = _DEEZER_MAX_COVER_SIZ return _DEEZER_CDN_SIZE_PATTERN.sub(f'/{target_size}x{target_size}-', url, count=1) +def _is_full_track_payload(payload: Optional[Dict[str, Any]]) -> bool: + """Distinguish a full `/track/` cache hit from partial album-tracks data. + + Three Deezer endpoints feed the per-track cache: + - `/track/` — full record, includes both `track_position` AND + `contributors` (the multi-artist list the contributors-upgrade + path reads). + - `/album//tracks` — partial; includes `track_position` but + omits `contributors`. + - `/search/track` — minimal; lacks `track_position`. + + Pre-fix `get_track_details` only checked `track_position`, so + partial album-tracks payloads were treated as full hits and the + contributors-upgrade silently fell back to single-artist tagging + whenever an album had been fetched before its individual tracks + were post-processed (issue #588). + + `contributors` key presence is the load-bearing distinction — + `[]` is a valid value for genuinely single-artist tracks fetched + via the per-track endpoint, so test for key membership not + truthiness. + """ + if not isinstance(payload, dict): + return False + return 'track_position' in payload and 'contributors' in payload + + # ==================== Dataclasses (match iTunesClient / SpotifyClient format) ==================== @dataclass @@ -546,14 +573,9 @@ class DeezerClient: """Get detailed track info — returns Spotify-compatible dict (metadata source interface)""" cache = get_metadata_cache() cached = cache.get_entity('deezer', 'track', str(track_id)) - if cached and cached.get('title'): - # Search results are cached with minimal data (no track_position). - # Only use cache if it has track_position — the key field from /track/{id}. - # Search results include 'isrc' and 'release_date' but NOT track_position, - # so those fields alone are not sufficient to distinguish full from partial data. - if 'track_position' in cached: - return self._build_enhanced_track(cached) - # Otherwise fall through to fetch full data from API + if cached and cached.get('title') and _is_full_track_payload(cached): + return self._build_enhanced_track(cached) + # Otherwise fall through to fetch full data from API data = self._api_get(f'track/{track_id}') if not data: diff --git a/tests/metadata/test_deezer_track_cache_validity.py b/tests/metadata/test_deezer_track_cache_validity.py new file mode 100644 index 00000000..8ff107de --- /dev/null +++ b/tests/metadata/test_deezer_track_cache_validity.py @@ -0,0 +1,208 @@ +"""Pin the Deezer per-track cache validity check. + +Issue #588: contributors tagging worked for some tracks and not others. +Root cause was cache pollution — `/album//tracks` cached partial +records under the same key as `/track/`, and `get_track_details` +was using `track_position` alone as the "full payload" sentinel. Both +endpoints set track_position; only `/track/` sets contributors. + +These tests pin the corrected sentinel (`_is_full_track_payload`) so +the regression can't silently come back. +""" + +from unittest.mock import MagicMock, patch + +import pytest + +from core.deezer_client import _is_full_track_payload + + +# ──────────────────────────────────────────────────────────────────── +# Pure helper — payload-shape classification +# ──────────────────────────────────────────────────────────────────── + +def test_full_track_endpoint_payload_is_valid(): + payload = { + 'id': 12345, + 'title': 'Erased', + 'track_position': 1, + 'contributors': [{'name': 'Whipped Cream'}, {'name': 'Andrea Botez'}], + 'artist': {'name': 'Whipped Cream'}, + 'album': {'id': 1, 'title': 'Erased'}, + } + assert _is_full_track_payload(payload) is True + + +def test_full_track_with_empty_contributors_list_is_valid(): + # Single-artist track from /track/ still emits contributors=[] + # The KEY presence is what matters, not truthiness. + payload = { + 'id': 12345, + 'title': 'Solo Track', + 'track_position': 1, + 'contributors': [], + 'artist': {'name': 'Solo Artist'}, + } + assert _is_full_track_payload(payload) is True + + +def test_album_tracks_payload_missing_contributors_is_partial(): + # The exact shape /album//tracks returns per item — has + # track_position but no contributors. Pre-fix this passed the + # `track_position in cached` check; post-fix it correctly falls + # through to a fresh /track/ fetch. + payload = { + 'id': 12345, + 'title': 'Sacrifice', + 'track_position': 1, + 'duration': 180, + 'artist': {'name': 'Andrea Botez'}, + } + assert _is_full_track_payload(payload) is False + + +def test_search_payload_without_track_position_is_partial(): + payload = { + 'id': 12345, + 'title': 'Sacrifice', + 'artist': {'name': 'Andrea Botez'}, + 'isrc': 'XX1234567890', + } + assert _is_full_track_payload(payload) is False + + +def test_none_or_non_dict_payload_is_partial(): + assert _is_full_track_payload(None) is False + assert _is_full_track_payload([]) is False + assert _is_full_track_payload('string') is False + assert _is_full_track_payload(0) is False + + +def test_empty_dict_is_partial(): + assert _is_full_track_payload({}) is False + + +# ──────────────────────────────────────────────────────────────────── +# get_track_details — cache + fetch interaction +# ──────────────────────────────────────────────────────────────────── + +@pytest.fixture +def deezer_client(): + """Build a DeezerClient with mocked HTTP + cache. Bypasses __init__ + auth/config requirements.""" + from core.deezer_client import DeezerClient + client = DeezerClient.__new__(DeezerClient) + client._api_get = MagicMock() + return client + + +def _patch_cache(cached_payload): + """Patch the module-level cache lookup. Returns the patched cache + mock so callers can assert on store_entity calls.""" + cache = MagicMock() + cache.get_entity.return_value = cached_payload + cache.store_entity = MagicMock() + return patch('core.deezer_client.get_metadata_cache', return_value=cache), cache + + +def test_cache_hit_with_full_payload_skips_api_call(deezer_client): + full = { + 'id': 12345, + 'title': 'Erased', + 'track_position': 1, + 'contributors': [{'name': 'Whipped Cream'}, {'name': 'Andrea Botez'}], + 'artist': {'name': 'Whipped Cream'}, + 'album': {'id': 1, 'title': 'Erased', 'nb_tracks': 1}, + } + cache_patch, cache = _patch_cache(full) + with cache_patch: + result = deezer_client.get_track_details('12345') + + assert result is not None + assert result['artists'] == ['Whipped Cream', 'Andrea Botez'] + deezer_client._api_get.assert_not_called() + cache.store_entity.assert_not_called() + + +def test_cache_hit_with_partial_album_tracks_payload_refetches(deezer_client): + """The bug from #588 — partial album-tracks data should NOT be + treated as a full hit. Post-fix the client re-fetches.""" + partial = { + 'id': 12345, + 'title': 'Sacrifice', + 'track_position': 1, + 'artist': {'name': 'Andrea Botez'}, + } + fresh = { + 'id': 12345, + 'title': 'Sacrifice', + 'track_position': 1, + 'contributors': [{'name': 'Andrea Botez'}, {'name': 'Grabbitz'}], + 'artist': {'name': 'Andrea Botez'}, + 'album': {'id': 1, 'title': 'Sacrifice', 'nb_tracks': 1}, + } + cache_patch, cache = _patch_cache(partial) + deezer_client._api_get.return_value = fresh + with cache_patch: + result = deezer_client.get_track_details('12345') + + assert result is not None + assert result['artists'] == ['Andrea Botez', 'Grabbitz'] + deezer_client._api_get.assert_called_once_with('track/12345') + cache.store_entity.assert_called_once_with('deezer', 'track', '12345', fresh) + + +def test_cache_miss_fetches_fresh(deezer_client): + cache_patch, cache = _patch_cache(None) + fresh = { + 'id': 12345, + 'title': 'Sacrifice', + 'track_position': 1, + 'contributors': [{'name': 'Andrea Botez'}, {'name': 'Grabbitz'}], + 'artist': {'name': 'Andrea Botez'}, + 'album': {'id': 1, 'title': 'Sacrifice', 'nb_tracks': 1}, + } + deezer_client._api_get.return_value = fresh + with cache_patch: + result = deezer_client.get_track_details('12345') + + assert result is not None + assert result['artists'] == ['Andrea Botez', 'Grabbitz'] + deezer_client._api_get.assert_called_once_with('track/12345') + cache.store_entity.assert_called_once() + + +def test_cache_hit_with_search_shape_refetches(deezer_client): + """Search results lack track_position — same fall-through path as + partial album-tracks data.""" + search_shape = { + 'id': 12345, + 'title': 'Sacrifice', + 'artist': {'name': 'Andrea Botez'}, + 'isrc': 'XX1234567890', + } + fresh = { + 'id': 12345, + 'title': 'Sacrifice', + 'track_position': 1, + 'contributors': [{'name': 'Andrea Botez'}, {'name': 'Grabbitz'}], + 'artist': {'name': 'Andrea Botez'}, + 'album': {'id': 1, 'title': 'Sacrifice', 'nb_tracks': 1}, + } + cache_patch, _ = _patch_cache(search_shape) + deezer_client._api_get.return_value = fresh + with cache_patch: + result = deezer_client.get_track_details('12345') + + assert result is not None + assert result['artists'] == ['Andrea Botez', 'Grabbitz'] + deezer_client._api_get.assert_called_once() + + +def test_api_failure_returns_none(deezer_client): + cache_patch, _ = _patch_cache(None) + deezer_client._api_get.return_value = None + with cache_patch: + result = deezer_client.get_track_details('12345') + + assert result is None diff --git a/webui/static/helper.js b/webui/static/helper.js index 1b56a672..c69feb7e 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3416,6 +3416,7 @@ const WHATS_NEW = { '2.5.2': [ // --- May 13, 2026 — 2.5.2 release --- { date: 'May 13, 2026 — 2.5.2 release' }, + { title: 'Deezer: Contributing Artist Tagging Now Consistent', desc: 'github issue #588: contributors tagging worked for some tracks but silently dropped them for others — most reproducibly for tracks whose ALBUM was fetched before the per-track post-process ran. trace: `core/deezer_client.py:get_track_details` cache check used `track_position` as the "full payload" sentinel, but BOTH `/track/` AND `/album//tracks` set that field. only `/track/` sets the `contributors` array. when album-tracks data hit the cache first, `get_track_details` returned the partial record → `_build_enhanced_track` found no contributors → the metadata-source contributors-upgrade silently fell back to single-artist. fix: lifted cache-validity to a pure helper `_is_full_track_payload` that requires BOTH `track_position` AND `contributors` key presence (empty list `[]` is valid — single-artist tracks fetched via `/track/` carry it explicitly). partial cache hits now fall through to a fresh `/track/` fetch. 11 boundary tests pin every shape: full payload, single-artist with empty contributors list, partial album-tracks shape, search-result shape, none/non-dict, cache-hit/cache-miss/api-failure paths.', page: 'downloads' }, { title: 'Server Playlists: Find & Add Now Persists As A Permanent Match', desc: 'github issue #585: when a spotify track name had a versioned suffix not present in the local file (e.g. "Iron Man - 2012 - Remaster" vs "Iron Man") the auto-matcher missed the pair. user could click Find & Add to manually pick the right local file — that worked, file got added to the plex playlist — but the source spotify track stayed in Missing while the added file showed up under Extra, because the matcher had no record of the user-confirmed pairing. on the next sync the source track would re-quarantine and try to download all over again. fix: every Find & Add selection now writes a `(spotify_track_id → server_track_id)` override into `sync_match_cache` at confidence=1.0. the matching algorithm runs an override pass BEFORE the existing exact and fuzzy passes, so any user-confirmed pair short-circuits straight to "matched" without going through normalization at all. covers every kind of mismatch — dash-suffix remasters, covers / karaoke versions, alt masters, cross-language titles, typo\'d local files, anything. logic lifted to `core/sync/match_overrides.py` (pure helpers `resolve_match_overrides` + `record_manual_match`). 18 boundary tests pin: cache-hit pairs, cache-miss falls through, stale-cache (server track removed) handled gracefully, two sources pointing at same server track (UNIQUE-violation defense), str/int id coercion, partial cache hits, defensive against non-dict inputs and DB exceptions. legacy entries without `source_track_id` (non-mirrored playlists) just skip the override path. works across plex / jellyfin / navidrome.', page: 'sync' }, { title: 'Quarantine Management — See, Approve, Delete Files Without Touching The Filesystem', desc: 'github issue #584: quarantined files used to just sit in `ss_quarantine/` with a thin sidecar — no UI, no recovery, no way to see what got dropped or why. new **Quarantine** tab on the existing Library History modal (downloads page → Download History button) lists every quarantined file with the same row chrome as the Downloads + Server Imports tabs: thumb placeholder, expected track + artist, original filename, trigger badge (Duration / AcoustID / Bit Depth), relative time, expandable details panel showing the full failure reason. three per-row actions: **Approve** (restores the file, re-runs post-processing with ONLY the failing check skipped, lands in your library with full tags + lyrics + scan), **Recover** (legacy fallback for entries quarantined before this PR with thin sidecars — moves to Staging so you finish via Import flow), **Delete** (permanent removal of file + sidecar). all three use the themed soulsync confirm modal + toast feedback (no native browser alert / confirm). per-check bypass means approving a duration-mismatch file still runs AcoustID; approving an AcoustID failure still runs bit-depth — other quality gates stay live so you can only override one trigger at a time. files that fail a different check after approval get re-quarantined with the new trigger label so you can decide again. sidecar now persists the full json-safe context so approve has everything the pipeline needs to re-process. download modal status differentiates "🛡️ Quarantined" from "❌ Failed" so recoverable files are visible at a glance. logic lifted to pure helpers in `core/imports/quarantine.py` (list / delete / approve / recover_to_staging / serialize_quarantine_context) with 27 boundary tests covering orphan files / orphan sidecars / corrupt sidecars / collision-safe filename restoration / full-context vs thin-sidecar dispatch / json round-trip safety. four new endpoints. pipeline change is per-check conditionals at the existing quarantine sites — no blanket skip-all flag.', page: 'downloads' }, { title: 'Configurable Duration Tolerance For Quarantined Tracks', desc: 'discord question: tracks were quarantining when their actual length drifted by a few seconds from what spotify/musicbrainz reported (3s tolerance hardcoded, 5s for tracks >10min). live recordings, alternate masterings, and some legitimate uploads routinely drift more than that. new setting on settings → metadata → post-processing: "duration tolerance (seconds)". `0 = auto` (preserves the existing 3s/5s defaults). raise it to 10 / 15 / 20 if your library has a lot of drift-prone material. capped at 60s — past that the check is effectively off. applies to ALL matched downloads (soulseek / tidal / qobuz / hifi / youtube / deezer-direct) since they all flow through the same post-process integrity check. logic lifted to a pure helper `core/imports/file_integrity.py:resolve_duration_tolerance` that coerces the config value (none / empty / 0 / negative / unparseable / above-cap) to either a float override or `None` for the auto-scaled default. 12 tests pin every input shape.', page: 'settings' },