From bcd69c8baa8be29a0eb0558007d09db54aedc460 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Tue, 9 Jun 2026 17:10:03 -0700 Subject: [PATCH] =?UTF-8?q?Multi-artist=20tags:=20Search=20=E2=86=92=20Dow?= =?UTF-8?q?nload=20Now=20finally=20knows=20its=20metadata=20source=20(Nett?= =?UTF-8?q?i93)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third round of the multi-artist report. The earlier fixes (Deezer contributors upgrade, _artists_list, feat_in_title/artist_separator) were all in place and correct — but gated on source == 'deezer', and on the real Search → Download Now path NOTHING carried the source: core/search/sources.py serialized tracks with no source field, search.js's enrichedTrack didn't add one, so get_import_source() resolved '' and the whole Deezer-specific block silently skipped. Files were tagged with only the primary artist until a Retag (which rebuilds context with the source set — exactly why retagging always fixed it). The earlier tests passed because they set context['source'] directly — the one field the real flow never had (same mock-drift as the #823 append tests). Reproduced with Netti93's exact track (deezer 3966840171) through the real extract_source_metadata: before — source '', artists ['August Burns Red']; after — source 'deezer', contributors fetched, artists ['August Burns Red', 'Polaris'], title 'Sonic Salvation (feat. Polaris)' per feat_in_title. Fix, three layers: - core/search/sources.py: serialized tracks/albums/artists carry "source" (the canonical name the orchestrator already passes; '' when unnamed). - core/imports/context.py get_import_source: also reads '_source' from the nested dicts (track_info/original_search/album/artist) — additionally fixes the discography/wishlist flows, which always passed '_source' that nothing read. - search.js: enrichedTrack + the album-download path carry source through to the download task. Tests: real-payload staging-shaped contexts (source in track_info, '_source' shape, and the pre-fix sourceless shape staying safe — mocked Deezer client), serializer source-field tests, resolver fallback tests; exact-shape serializer tests updated for the new key. 1977 import/metadata/search tests pass (the only 2 failures are the known soundcloud ones). --- core/imports/context.py | 16 ++-- core/search/sources.py | 17 ++++ tests/imports/test_import_context.py | 17 ++++ .../test_multi_artist_tag_settings.py | 90 +++++++++++++++++++ tests/search/test_search_sources.py | 45 ++++++++++ webui/static/search.js | 14 ++- 6 files changed, 193 insertions(+), 6 deletions(-) diff --git a/core/imports/context.py b/core/imports/context.py index 73459949..f718f014 100644 --- a/core/imports/context.py +++ b/core/imports/context.py @@ -129,30 +129,36 @@ def get_import_search_result(context: Optional[Dict[str, Any]]) -> Dict[str, Any def get_import_source(context: Optional[Dict[str, Any]]) -> str: + # Several track payloads carry the metadata source under "_source" rather + # than "source" (the discography/wishlist dicts, frontend search results). + # Only the context-level "_source" was honored (normalize_import_context); + # the nested dicts were checked for "source" alone, so a Deezer-sourced + # Download Now resolved to '' and source-specific metadata logic (the + # Deezer contributors upgrade for multi-artist tags) never ran (Netti93). if not isinstance(context, dict): return "" - source = context.get("source") + source = context.get("source") or context.get("_source") if source: return str(source) track_info = get_import_track_info(context) - source = _first_value(track_info, "source", default="") + source = _first_value(track_info, "source", "_source", default="") if source: return str(source) original_search = get_import_original_search(context) - source = _first_value(original_search, "source", default="") + source = _first_value(original_search, "source", "_source", default="") if source: return str(source) album = get_import_context_album(context) - source = _first_value(album, "source", default="") + source = _first_value(album, "source", "_source", default="") if source: return str(source) artist = get_import_context_artist(context) - source = _first_value(artist, "source", default="") + source = _first_value(artist, "source", "_source", default="") return str(source) if source else "" diff --git a/core/search/sources.py b/core/search/sources.py index 006bf8f0..e5d66805 100644 --- a/core/search/sources.py +++ b/core/search/sources.py @@ -35,6 +35,7 @@ def search_kind(client, query: str, kind: str, source_name: Optional[str] = None artists.append({ "id": artist.id, "name": artist.name, + "source": source_name or "", "image_url": artist.image_url, "external_urls": artist.external_urls or {}, }) @@ -52,6 +53,7 @@ def search_kind(client, query: str, kind: str, source_name: Optional[str] = None "id": album.id, "name": album.name, "artist": artist_name, + "source": source_name or "", "image_url": album.image_url, "release_date": album.release_date, "total_tracks": album.total_tracks, @@ -78,6 +80,21 @@ def search_kind(client, query: str, kind: str, source_name: Optional[str] = None "id": track.id, "name": track.name, "artist": artist_name, + # The REAL artist list, not the joined display string above. + # Spotify/Tidal/iTunes searches return collabs as a list; + # collapsing them to one "A, B" string made the import + # pipeline tag downloads with a single combined artist + # (resolve_track_artists saw one value). The frontend keeps + # using "artist" for display. + "artists": list(track.artists or []), + # Which metadata source this result came from. Travels with + # the payload through Download Now -> download task -> + # import context, where extract_source_metadata needs it to + # run source-specific logic (the Deezer contributors + # upgrade for multi-artist tags — Netti93's report: without + # it get_import_source() resolved '' and collab tracks + # were tagged with only the primary artist until a Retag). + "source": source_name or "", "album": track.album, "duration_ms": track.duration_ms, "image_url": track.image_url, diff --git a/tests/imports/test_import_context.py b/tests/imports/test_import_context.py index e71584b7..5ce80835 100644 --- a/tests/imports/test_import_context.py +++ b/tests/imports/test_import_context.py @@ -338,3 +338,20 @@ def test_detect_album_info_web_forces_album_when_track_and_artist_differ(): assert album_info["album_name"] == "Album One" assert album_info["track_number"] == 4 assert album_info["disc_number"] == 2 + + +def test_get_import_source_reads_underscore_source_from_nested_dicts(): + """Netti93 multi-artist fix: many track payloads carry '_source' (the + discography/wishlist dicts) or 'source' only inside track_info (search + results). get_import_source must resolve all of them — previously only + the context-level keys worked, so direct downloads resolved '' and + source-specific metadata logic never ran.""" + from core.imports.context import get_import_source + + assert get_import_source({"track_info": {"source": "deezer"}}) == "deezer" + assert get_import_source({"track_info": {"_source": "deezer"}}) == "deezer" + assert get_import_source({"original_search_result": {"_source": "itunes"}}) == "itunes" + assert get_import_source({"_source": "tidal"}) == "tidal" + # context-level 'source' still wins over nested + assert get_import_source({"source": "spotify", "track_info": {"_source": "deezer"}}) == "spotify" + assert get_import_source({}) == "" diff --git a/tests/metadata/test_multi_artist_tag_settings.py b/tests/metadata/test_multi_artist_tag_settings.py index b8de390f..dc835bed 100644 --- a/tests/metadata/test_multi_artist_tag_settings.py +++ b/tests/metadata/test_multi_artist_tag_settings.py @@ -561,3 +561,93 @@ class TestDeezerDirectDownloadFlow: assert meta["_artists_list"] == ["FAYAN", "Dalton"] assert meta["artist"] == "FAYAN;Dalton" assert meta["title"] == "VERLIEBT IN MICH" + + +class TestSourceResolutionFromRealDownloadPayload: + """Netti93 round 3: the prior tests set context['source'] — a field the + REAL Search → Download Now flow never had. The serialized search results + carried no source at all, so get_import_source() resolved '' and the + Deezer contributors upgrade never ran on direct downloads (single-artist + tags until a Retag). These pin the actual payload shapes end to end.""" + + def _staging_context(self, track_info): + # Shape built by core/downloads/staging.py:457 for a stream download. + return { + "track_info": track_info, + "spotify_artist": {"name": "August Burns Red", "id": None}, + "spotify_album": {"name": "Death Below"}, + "original_search_result": { + "username": "tidal", "filename": "/x.flac", + "title": "Sonic Salvation", "artist": "August Burns Red", + "spotify_clean_title": "Sonic Salvation", + "spotify_clean_album": "Death Below", + "spotify_clean_artist": "August Burns Red", + "track_number": 1, "disc_number": 1, + }, + "is_album_download": False, + "staging_source": True, + } + + def _fake_deezer(self): + return SimpleNamespace(get_track_details=MagicMock(return_value={ + "id": "3966840171", "name": "Sonic Salvation", + "artists": ["August Burns Red", "Polaris"], + })) + + def test_source_in_track_info_triggers_upgrade(self): + """The fixed flow: serialized search result carries source='deezer' + inside track_info (no context-level source) → upgrade fires.""" + from core.metadata import source as src_module + + track_info = { + "id": "3966840171", "name": "Sonic Salvation", + "source": "deezer", + "artists": ["August Burns Red"], + "album": {"name": "Death Below", "id": None}, + } + fake_deezer = self._fake_deezer() + with patch.object(src_module, "get_config_manager", + return_value=_make_cfg({"metadata_enhancement.tags.write_multi_artist": True})), \ + patch("core.metadata.get_deezer_client", return_value=fake_deezer): + meta = src_module.extract_source_metadata( + self._staging_context(track_info), {"name": "August Burns Red", "id": ""}, {}) + + assert meta["_artists_list"] == ["August Burns Red", "Polaris"] + fake_deezer.get_track_details.assert_called_once_with("3966840171") + + def test_underscore_source_shape_also_triggers_upgrade(self): + """Discography/wishlist payloads carry '_source' instead of 'source' — + get_import_source must honor that shape too.""" + from core.metadata import source as src_module + + track_info = { + "id": "3966840171", "name": "Sonic Salvation", + "_source": "deezer", + "artists": ["August Burns Red"], + } + fake_deezer = self._fake_deezer() + with patch.object(src_module, "get_config_manager", return_value=_make_cfg()), \ + patch("core.metadata.get_deezer_client", return_value=fake_deezer): + meta = src_module.extract_source_metadata( + self._staging_context(track_info), {"name": "August Burns Red", "id": ""}, {}) + + assert meta["_artists_list"] == ["August Burns Red", "Polaris"] + fake_deezer.get_track_details.assert_called_once_with("3966840171") + + def test_sourceless_payload_still_no_upgrade(self): + """A payload with no source anywhere (pre-fix shape) must not crash — + and must not call the Deezer API blindly.""" + from core.metadata import source as src_module + + track_info = { + "id": "3966840171", "name": "Sonic Salvation", + "artists": ["August Burns Red"], + } + fake_deezer = self._fake_deezer() + with patch.object(src_module, "get_config_manager", return_value=_make_cfg()), \ + patch("core.metadata.get_deezer_client", return_value=fake_deezer): + meta = src_module.extract_source_metadata( + self._staging_context(track_info), {"name": "August Burns Red", "id": ""}, {}) + + assert meta["_artists_list"] == ["August Burns Red"] + fake_deezer.get_track_details.assert_not_called() diff --git a/tests/search/test_search_sources.py b/tests/search/test_search_sources.py index f6949027..eb5fb7fd 100644 --- a/tests/search/test_search_sources.py +++ b/tests/search/test_search_sources.py @@ -84,6 +84,7 @@ def test_search_kind_artists_returns_normalized_dicts(): assert result == [{ 'id': 'id1', 'name': 'Pink Floyd', + 'source': 'spotify', 'image_url': 'thumb.jpg', 'external_urls': {'spotify': 'url'}, }] @@ -137,6 +138,8 @@ def test_search_kind_tracks_returns_full_shape(): 'id': 't1', 'name': 'Money', 'artist': 'Pink Floyd', + 'artists': ['Pink Floyd'], + 'source': '', 'album': 'DSOTM', 'duration_ms': 383000, 'image_url': 'm.jpg', @@ -201,3 +204,45 @@ def test_search_source_all_fail_returns_empty_lists(): client = _Client(fail={'artists', 'albums', 'tracks'}) result = sources.search_source('q', client, 'spotify') assert result == {'artists': [], 'albums': [], 'tracks': [], 'available': True} + + +# ── source field on serialized results (Netti93 multi-artist fix) ─────────── +# Search results must carry which metadata source they came from: the payload +# travels Download Now → download task → import context, where the Deezer +# contributors upgrade (multi-artist tags) is gated on source == 'deezer'. +# Without it get_import_source() resolved '' and collab tracks were tagged +# with only the primary artist until a Retag. + +def _full_client(): + return _Client( + artists=[_Artist('id1', 'A')], + albums=[_Album('a1', 'Album', artists=['A'])], + tracks=[_Track('t1', 'T', artists=['A'], album='Album')], + ) + + +def test_serialized_tracks_carry_source(): + out = sources.search_kind(_full_client(), "q", "tracks", source_name="deezer") + assert out and all(t["source"] == "deezer" for t in out) + + +def test_serialized_albums_and_artists_carry_source(): + albums = sources.search_kind(_full_client(), "q", "albums", source_name="deezer") + artists = sources.search_kind(_full_client(), "q", "artists", source_name="deezer") + assert albums and all(a["source"] == "deezer" for a in albums) + assert artists and all(a["source"] == "deezer" for a in artists) + + +def test_serialized_source_empty_when_unnamed(): + # hydrabase path calls without a source_name — emit '' not the class name. + out = sources.search_kind(_full_client(), "q", "tracks") + assert out and all(t["source"] == "" for t in out) + + +def test_serialized_tracks_carry_real_artists_list(): + """Collabs must survive as a LIST — the joined "A, B" display string made + downloads tag a single combined artist (Marcus's report).""" + client = _Client(tracks=[_Track('t1', 'Collab', artists=['Artist A', 'Artist B'], album='X')]) + out = sources.search_kind(client, 'q', 'tracks', source_name='spotify') + assert out[0]['artists'] == ['Artist A', 'Artist B'] + assert out[0]['artist'] == 'Artist A, Artist B' # display string unchanged diff --git a/webui/static/search.js b/webui/static/search.js index 54110af1..721df256 100644 --- a/webui/static/search.js +++ b/webui/static/search.js @@ -750,6 +750,9 @@ function initializeSearchModeToggle() { // Enrich each track with full album object (needed for wishlist functionality) const enrichedTracks = albumData.tracks.map(track => ({ ...track, + // Carry the metadata source for source-specific import logic + // (Deezer contributors upgrade for multi-artist tags). + source: track.source || album.source || null, album: { name: albumData.name, id: albumData.id, @@ -902,7 +905,16 @@ function initializeSearchModeToggle() { const enrichedTrack = { id: track.id, name: track.name, - artists: [track.artist], // Convert string to array for modal compatibility + // Carry the metadata source so the import pipeline can run + // source-specific logic (Deezer contributors upgrade for + // multi-artist tags) on the downloaded file. + source: track.source || null, + // Prefer the real artist list (Spotify/Tidal collabs) over the + // joined "A, B" display string, so downloads get proper + // multi-artist tags instead of one combined artist. + artists: (Array.isArray(track.artists) && track.artists.length) + ? track.artists + : [track.artist], album: { name: track.album, id: null,