diff --git a/core/exports/export_sources.py b/core/exports/export_sources.py index 8b6fa36f..dbf5bbd8 100644 --- a/core/exports/export_sources.py +++ b/core/exports/export_sources.py @@ -16,8 +16,9 @@ writes a fresh non-cache hit back to the cache so the next export of the same so from __future__ import annotations +import json import threading -from typing import Callable, Optional, Tuple +from typing import Any, Callable, Dict, List, Optional, Tuple from utils.logging_config import get_logger @@ -121,6 +122,89 @@ def build_service_resolve_fn(service: str) -> Callable[[str, str], Tuple[Optiona return resolve_fn +def service_id_from_extra_data(track: Any, service: str) -> Optional[str]: + """The target-service track ID the DISCOVERY step already resolved for this mirrored + track, read from its ``extra_data`` blob (#945 — Boulder: "all 50 are discovered to + Deezer already, it's not using any of that"). This is free (no API call) and reliable + (it's the same id used to mirror the track). + + Only trusted when the track was discovered ON the export's target service — a + Deezer-discovered track carries a Deezer id under ``matched_data['id']``, and its + ``provider`` is the service name. A ``wing_it_fallback`` provider (the low-confidence + guess path) deliberately does NOT match here, so those fall through to the library/ + none path rather than risk a wrong track in the exported playlist.""" + raw = track.get("extra_data") if isinstance(track, dict) else None + if not raw: + return None + try: + data = json.loads(raw) if isinstance(raw, str) else raw + except Exception: + return None + if not isinstance(data, dict) or not data.get("discovered"): + return None + if str(data.get("provider") or "").lower() != str(service or "").lower(): + return None + matched = data.get("matched_data") + tid = matched.get("id") if isinstance(matched, dict) else None + return str(tid) if tid else None + + +def _track_field(track: Dict[str, Any], *names: str) -> str: + for n in names: + v = track.get(n) + if v: + return str(v) + return "" + + +def resolve_service_track_ids( + tracks: List[Dict[str, Any]], + service: str, + *, + db_fn: Optional[Callable[[str, str, str], Optional[str]]] = None, + on_progress: Optional[Callable[[int, int, Dict[str, Any]], None]] = None, +) -> Dict[str, Any]: + """Resolve a mirrored playlist's tracks to target-service track IDs for export. + + Waterfall per track: the discovery cache (``extra_data`` — free + already confidently + matched) → the library track's stored service id. A track with neither is reported + unmatched (caller skips it — never a guessed/wrong id). Returns + ``{"resolved": [{artist, title, album, service_track_id}], "stats": {...}}`` with + ``from_cache`` / ``from_library`` / ``unmatched`` tallies for the status display. + """ + db_fn = db_fn or db_service_track_id + total = len(tracks or []) + resolved: List[Dict[str, Any]] = [] + stats: Dict[str, Any] = { + "total": total, "resolved": 0, "unmatched": 0, "from_cache": 0, "from_library": 0, + } + for i, t in enumerate(tracks or []): + if not isinstance(t, dict): + t = {} + artist = _track_field(t, "artist", "artist_name", "creator") + title = _track_field(t, "title", "track_name", "name") + album = _track_field(t, "album", "album_name", "release_name") + + tid = service_id_from_extra_data(t, service) + if tid: + stats["from_cache"] += 1 + else: + tid = db_fn(artist, title, service) + if tid: + stats["from_library"] += 1 + + resolved.append({"artist": artist, "title": title, "album": album, + "service_track_id": tid or None}) + stats["resolved" if tid else "unmatched"] += 1 + if on_progress is not None: + try: + on_progress(i + 1, total, stats) + except Exception: # noqa: S110 — a progress error must never fail the export + pass + + return {"resolved": resolved, "stats": stats} + + def file_recording_mbid(artist: str, title: str) -> Optional[str]: """Recording MBID read from the matched track's file tag (set on import post-processing).""" _mbid, fpath = _db_match(artist, title) diff --git a/tests/exports/test_export_sources.py b/tests/exports/test_export_sources.py index fb6a8be2..8555903b 100644 --- a/tests/exports/test_export_sources.py +++ b/tests/exports/test_export_sources.py @@ -117,3 +117,55 @@ def test_db_service_track_id_real_sql_executes(tmp_path, monkeypatch): assert es.db_service_track_id("Kendrick Lamar", "Not Like Us", "spotify") == "spid-NLU" assert es.db_service_track_id("kendrick lamar", "not like us", "deezer") == "dz-NLU" # case-insensitive assert es.db_service_track_id("Kendrick Lamar", "Unknown Song", "spotify") is None + + +# ── discovery-cache resolution (#945: use the already-discovered IDs, no API call) ── + +import json as _json +from core.exports.export_sources import ( + service_id_from_extra_data, + resolve_service_track_ids, +) + + +def _extra(service, tid, discovered=True, provider=None): + return {'extra_data': _json.dumps({'discovered': discovered, + 'provider': provider or service, + 'matched_data': {'id': tid}})} + + +def test_extra_data_id_when_discovered_to_that_service(): + assert service_id_from_extra_data(_extra('deezer', 111), 'deezer') == '111' + # dict (not str) extra_data also works + raw = {'extra_data': {'discovered': True, 'provider': 'spotify', 'matched_data': {'id': 'spX'}}} + assert service_id_from_extra_data(raw, 'spotify') == 'spX' + + +def test_extra_data_provider_must_match_service(): + # discovered to Spotify, exporting to Deezer → don't reuse the (wrong-service) id + assert service_id_from_extra_data(_extra('spotify', 111), 'deezer') is None + + +def test_extra_data_wing_it_fallback_is_not_trusted(): + track = _extra('deezer', 111, provider='wing_it_fallback') + assert service_id_from_extra_data(track, 'deezer') is None + + +def test_extra_data_misc_none_cases(): + assert service_id_from_extra_data({}, 'deezer') is None # no extra_data + assert service_id_from_extra_data({'extra_data': 'not json{'}, 'deezer') is None # bad json + assert service_id_from_extra_data(_extra('deezer', 111, discovered=False), 'deezer') is None + + +def test_resolve_waterfall_cache_then_library_then_unmatched(): + tracks = [ + _extra('deezer', 111) | {'artist_name': 'A', 'track_name': 'Cached'}, # cache hit + {'artist_name': 'A', 'track_name': 'InLib'}, # library hit (db_fn) + {'artist_name': 'A', 'track_name': 'Nowhere'}, # unmatched + ] + db_fn = lambda a, t, s: 'lib-222' if t == 'InLib' else None + out = resolve_service_track_ids(tracks, 'deezer', db_fn=db_fn) + ids = [r['service_track_id'] for r in out['resolved']] + assert ids == ['111', 'lib-222', None] + s = out['stats'] + assert s == {'total': 3, 'resolved': 2, 'unmatched': 1, 'from_cache': 1, 'from_library': 1} diff --git a/tests/test_service_playlist_export.py b/tests/test_service_playlist_export.py index 07570a5b..790c85b5 100644 --- a/tests/test_service_playlist_export.py +++ b/tests/test_service_playlist_export.py @@ -1,7 +1,8 @@ -"""_run_service_export orchestration (#945): resolve mirrored tracks → service IDs → push → -store the target for idempotent re-export. Deps injected (fake db/client/resolve_fn) so this -needs no DB or live Spotify/Deezer.""" +"""_run_service_export orchestration (#945): resolve mirrored tracks → service IDs +(discovery cache → library) → push → store the target for idempotent re-export. Deps +injected so this needs no real DB or live Spotify/Deezer.""" +import json import web_server as ws @@ -28,48 +29,62 @@ class _FakeClient: return self.result -def _resolve(mapping): - return lambda artist, title: (mapping.get(title), 'library' if mapping.get(title) else None) +def _fake_resolver(ids): + """resolve_ids_fn stub returning the given service ids as the resolved set.""" + def fn(tracks, service, on_progress=None): + resolved = [{'artist': 'A', 'title': f't{i}', 'service_track_id': s} + for i, s in enumerate(ids)] + matched = sum(1 for s in ids if s) + return {'resolved': resolved, + 'stats': {'total': len(ids), 'resolved': matched, 'unmatched': len(ids) - matched}} + return fn -def test_success_stores_target(): +def _discovered(artist, title, service, tid): + return {'artist_name': artist, 'track_name': title, + 'extra_data': json.dumps({'discovered': True, 'provider': service, + 'matched_data': {'id': tid}})} + + +def test_success_resolves_from_discovery_cache_and_stores_target(): + """Real resolver: both tracks were discovered to Deezer, so their IDs come straight + from extra_data (the cache) with no DB/API — the gap Boulder spotted.""" job = {} - db = _FakeDB([{'artist': 'A', 'title': 'X'}, {'artist': 'A', 'title': 'Y'}]) + db = _FakeDB([_discovered('A', 'X', 'deezer', 111), _discovered('A', 'Y', 'deezer', 222)]) client = _FakeClient({'success': True, 'playlist_id': 'pl-1', 'added': 2}) - ws._run_service_export(job, db, 5, 'My PL', 'spotify', client, _resolve({'X': 'sx', 'Y': 'sy'})) + ws._run_service_export(job, db, 5, 'My PL', 'deezer', client) # real resolve_service_track_ids assert job['phase'] == 'done' - assert client.calls[0] == ('My PL', ['sx', 'sy'], None) - assert db.set_calls == [(5, 'spotify', 'pl-1')] + assert client.calls[0] == ('My PL', ['111', '222'], None) + assert db.set_calls == [(5, 'deezer', 'pl-1')] + assert job['stats']['from_cache'] == 2 and job['stats']['unmatched'] == 0 -def test_no_matched_ids_errors_no_push(): +def test_no_match_errors_no_push(): job = {} client = _FakeClient({'success': True}) - ws._run_service_export(job, _FakeDB([{'artist': 'A', 'title': 'X'}]), 5, 'PL', 'deezer', - client, _resolve({})) + ws._run_service_export(job, _FakeDB([{}]), 5, 'PL', 'deezer', client, _fake_resolver([None])) assert job['phase'] == 'error' and 'nothing to export' in job['error'] assert client.calls == [] def test_client_none_errors(): job = {} - ws._run_service_export(job, _FakeDB([{'artist': 'A', 'title': 'X'}]), 5, 'PL', 'spotify', - None, _resolve({'X': 'sx'})) + ws._run_service_export(job, _FakeDB([{}]), 5, 'PL', 'spotify', None, _fake_resolver(['sx'])) assert job['phase'] == 'error' and 'not connected' in job['error'] def test_push_failure_surfaces_error_no_target_store(): job = {} - db = _FakeDB([{'artist': 'A', 'title': 'X'}]) + db = _FakeDB([{}]) client = _FakeClient({'success': False, 'error': 'Reconnect Spotify'}) - ws._run_service_export(job, db, 5, 'PL', 'spotify', client, _resolve({'X': 'sx'})) + ws._run_service_export(job, db, 5, 'PL', 'spotify', client, _fake_resolver(['sx'])) assert job['phase'] == 'error' and job['error'] == 'Reconnect Spotify' assert db.set_calls == [] def test_reexport_passes_existing_target(): job = {} - db = _FakeDB([{'artist': 'A', 'title': 'X'}], existing='pl-old') + db = _FakeDB([{}], existing='pl-old') client = _FakeClient({'success': True, 'playlist_id': 'pl-old', 'added': 1}) - ws._run_service_export(job, db, 5, 'PL', 'deezer', client, _resolve({'X': 'dz'})) + ws._run_service_export(job, db, 5, 'PL', 'deezer', client, _fake_resolver(['dz'])) assert client.calls[0][2] == 'pl-old' diff --git a/web_server.py b/web_server.py index 413dbbe5..b555f1b3 100644 --- a/web_server.py +++ b/web_server.py @@ -27741,15 +27741,17 @@ _playlist_export_jobs = {} _playlist_export_jobs_lock = threading.Lock() -def _run_service_export(job, db, playlist_id, title, service, client, resolve_fn): +def _run_service_export(job, db, playlist_id, title, service, client, resolve_ids_fn=None): """Export a mirrored playlist back to a streaming service (Spotify/Deezer, #945). - Reuses the same resolve→push→store-target machinery the ListenBrainz export uses, - just with a service track-ID resolver and a service write client. Deps are injected - so the orchestration (resolve, unmatched-guard, push, idempotent target store, error - paths) is unit-testable without a DB or live service. + Resolves each track to a target-service ID via the discovery cache (the track's + extra_data — free + already matched) then the library's stored id, pushes the matched + set via the service write client, and stores the returned playlist id so a re-export + updates in place (idempotent, like the LB #903 fix). Deps injected so the orchestration + is unit-testable without a DB or live service. """ - from core.exports.playlist_export import resolve_playlist_tracks + from core.exports.export_sources import resolve_service_track_ids + resolve_ids_fn = resolve_ids_fn or resolve_service_track_ids tracks = db.get_mirrored_playlist_tracks(int(playlist_id)) job['total'] = len(tracks) @@ -27759,8 +27761,7 @@ def _run_service_export(job, db, playlist_id, title, service, client, resolve_fn job['done'] = done job['stats'] = dict(stats) - out = resolve_playlist_tracks(tracks, resolve_fn, on_progress=on_progress, - id_key='service_track_id') + out = resolve_ids_fn(tracks, service, on_progress=on_progress) job['stats'] = out['stats'] ids = [r['service_track_id'] for r in out['resolved'] if r.get('service_track_id')] if not ids: @@ -27792,15 +27793,13 @@ def _run_playlist_export(job_id, playlist_id, title, mode): try: # Service export (#945) — resolve to Spotify/Deezer track IDs and push. if mode in ('spotify', 'deezer'): - from core.exports.export_sources import build_service_resolve_fn db = get_database() if mode == 'spotify': client = get_spotify_client() else: from core.deezer_download_client import DeezerDownloadClient client = DeezerDownloadClient() - _run_service_export(job, db, playlist_id, title, mode, client, - build_service_resolve_fn(mode)) + _run_service_export(job, db, playlist_id, title, mode, client) return from core.exports.export_sources import build_resolve_fn