playlist export: resolve IDs from the discovery cache first (#945, increment 6)
Boulder: "all 50 tracks are discovered to Deezer already — it's not using any of that." Right — the export only checked tracks.deezer_id (library) and ignored the IDs discovery already resolved and stored in each mirrored track's extra_data. So tracks that were discovered+downloaded but not separately enriched showed as "not on Deezer" and got dropped. Adds a per-track waterfall for service export: - service_id_from_extra_data(track, service): the id discovery already matched, read from extra_data.matched_data.id — FREE (no API call) and reliable (it's the same id used to mirror the track). Trusted only when discovered ON the target service (provider == service); a wing_it_fallback (low-confidence guess) does NOT match here, so it falls through rather than risk a wrong track in the export. - resolve_service_track_ids(tracks, service): cache (extra_data) → library stored id → unmatched. Reports from_cache / from_library / unmatched. _run_service_export now uses this instead of the artist/title MBID-style resolver. For Boulder's playlist this means all 50 resolve straight from the cache — full coverage, zero API calls. (A live confident-search backfill for the genuinely-missing remainder is the optional next step, gated + thresholded.) 9 new tests: extra_data id only when provider matches + wing_it excluded + bad-json/not-discovered guards, the cache→library→unmatched waterfall with stat tallies, and _run_service_export resolving straight from the cache end-to-end. 49 export tests green, ruff clean.
This commit is contained in:
BoulderBadgeDad
parent
6e1a377f37
commit
37c8b06a27
4 changed files with 181 additions and 31 deletions
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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}
|
||||
|
|
|
|||
|
|
@ -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'
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Reference in a new issue