From ab8f82af2e45d81017c47ef9190f59a6f0c86e12 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Mon, 22 Jun 2026 22:36:29 -0700 Subject: [PATCH] #903: re-export updates the same ListenBrainz playlist in place (no duplicates) Re-running an export created a new LB playlist every time (LB keys on MBID, not name, and create always mints a new one). Now remember which LB playlist a mirror was pushed to and update it in place: - listenbrainz_client: refactor batched-add into _add_tracks_in_batches; add get_playlist_track_count, delete_playlist, update_playlist (verify exists -> clear items via item/delete -> re-add -> edit title; reports gone=True if deleted on LB), and create_or_update_playlist (update when we have a prior MBID, else create; falls back to create if the remembered one was deleted). Stable URL/MBID across re-syncs. - playlist_export_targets table + get/set_playlist_export_target: remember (mirror, target) -> LB MBID. - export job consults/stores the target so push updates in place. +6 mocked tests (clear+re-add same mbid, gone-fallback, create-or-update branches, delete). API endpoints (item/delete, playlist/edit, playlist/delete, GET count) confirmed against LB docs; live round-trip pending explicit auth. --- core/listenbrainz_client.py | 162 +++++++++++++----- database/music_database.py | 49 ++++++ .../test_listenbrainz_create_playlist.py | 74 ++++++++ web_server.py | 6 +- 4 files changed, 249 insertions(+), 42 deletions(-) diff --git a/core/listenbrainz_client.py b/core/listenbrainz_client.py index 0f21acfe..af457f3a 100644 --- a/core/listenbrainz_client.py +++ b/core/listenbrainz_client.py @@ -158,40 +158,78 @@ class ListenBrainzClient: return True + _MAX_TRACKS_PER_ADD = 100 # ListenBrainz MAX_RECORDINGS_PER_ADD + _PLAYLIST_EXT = "https://musicbrainz.org/doc/jspf#playlist" + + def _lb_headers(self) -> Dict: + return {"Authorization": f"Token {self.token}", "Content-Type": "application/json"} + + def _add_tracks_in_batches(self, playlist_mbid: str, tracks: List[Dict]) -> int: + """Add JSPF tracks to a playlist in <=100-track batches; return how many were added.""" + added = 0 + headers = self._lb_headers() + for i in range(0, len(tracks or []), self._MAX_TRACKS_PER_ADD): + batch = tracks[i:i + self._MAX_TRACKS_PER_ADD] + try: + r = self._make_request_with_retry( + "POST", f"{self.base_url}/playlist/{playlist_mbid}/item/add", + json={"playlist": {"track": batch}}, headers=headers, + ) + if r and r.status_code in (200, 201): + added += len(batch) + else: + logger.warning(f"ListenBrainz item/add batch failed: " + f"{r.status_code if r else 'no response'}") + except Exception as e: + logger.error(f"ListenBrainz item/add error: {e}") + return added + + def get_playlist_track_count(self, playlist_mbid: str): + """Current track count of an LB playlist, or None if it can't be fetched (gone/404).""" + try: + r = self._make_request_with_retry( + "GET", f"{self.base_url}/playlist/{playlist_mbid}", + params={"fetch_metadata": "false"}, + headers={"Authorization": f"Token {self.token}"}, + ) + if r and r.status_code == 200: + return len(((r.json() or {}).get("playlist") or {}).get("track", [])) + except Exception as e: + logger.debug(f"ListenBrainz get playlist count failed: {e}") + return None + + def delete_playlist(self, playlist_mbid: str) -> bool: + """Delete an LB playlist. True on success.""" + try: + r = self._make_request_with_retry( + "POST", f"{self.base_url}/playlist/{playlist_mbid}/delete", headers=self._lb_headers() + ) + return bool(r and r.status_code in (200, 201)) + except Exception as e: + logger.error(f"ListenBrainz delete playlist error: {e}") + return False + def create_playlist(self, title: str, tracks: List[Dict], public: bool = False) -> Dict: - """Create a playlist on ListenBrainz and add its tracks (#903). + """Create a NEW playlist on ListenBrainz and add its tracks (#903). ``tracks`` are JSPF track dicts — each MUST carry an ``identifier`` of the form - ``https://musicbrainz.org/recording/`` (LB rejects text-only tracks; the - caller drops un-resolved tracks via ``core.exports.jspf_export``). - - Robust for large playlists: create an empty playlist (title only) to get the - playlist MBID, then add tracks in batches of ``MAX_TRACKS_PER_ADD`` (LB's - ``MAX_RECORDINGS_PER_ADD`` cap) via the ``item/add`` endpoint — so a 1k-track - playlist doesn't blow a single-request limit. - - Returns ``{success, playlist_mbid, playlist_url, added, requested, error}``. - Never raises — failures are reported in the dict so the export job can surface them. + ``https://musicbrainz.org/recording/`` (LB rejects text-only tracks). Creates + an empty playlist for the MBID, then adds tracks in <=100 batches. Returns + ``{success, playlist_mbid, playlist_url, added, requested, error, updated}``. Never raises. """ - MAX_TRACKS_PER_ADD = 100 - PLAYLIST_EXT = "https://musicbrainz.org/doc/jspf#playlist" result = {"success": False, "playlist_mbid": None, "playlist_url": None, - "added": 0, "requested": len(tracks or []), "error": None} - + "added": 0, "requested": len(tracks or []), "error": None, "updated": False} if not self.is_authenticated(): result["error"] = "ListenBrainz not authenticated (no token/username)" return result - headers = {"Authorization": f"Token {self.token}", "Content-Type": "application/json"} - - # 1) Create the (empty) playlist — title + visibility only. create_body = {"playlist": { "title": (title or "SoulSync Export").strip() or "SoulSync Export", - "extension": {PLAYLIST_EXT: {"public": bool(public)}}, + "extension": {self._PLAYLIST_EXT: {"public": bool(public)}}, }} try: resp = self._make_request_with_retry( - "POST", f"{self.base_url}/playlist/create", json=create_body, headers=headers + "POST", f"{self.base_url}/playlist/create", json=create_body, headers=self._lb_headers() ) except Exception as e: result["error"] = f"create request failed: {e}" @@ -209,30 +247,72 @@ class ListenBrainzClient: result["playlist_mbid"] = playlist_mbid result["playlist_url"] = f"https://listenbrainz.org/playlist/{playlist_mbid}" - - # 2) Add tracks in capped batches. - added = 0 - for i in range(0, len(tracks or []), MAX_TRACKS_PER_ADD): - batch = tracks[i:i + MAX_TRACKS_PER_ADD] - add_body = {"playlist": {"track": batch}} - try: - r = self._make_request_with_retry( - "POST", f"{self.base_url}/playlist/{playlist_mbid}/item/add", - json=add_body, headers=headers, - ) - if r and r.status_code in (200, 201): - added += len(batch) - else: - logger.warning(f"ListenBrainz item/add batch failed: " - f"{r.status_code if r else 'no response'}") - except Exception as e: - logger.error(f"ListenBrainz item/add error: {e}") - - result["added"] = added - # Playlist was created even if some adds failed — report partial success honestly. + result["added"] = self._add_tracks_in_batches(playlist_mbid, tracks) result["success"] = True return result + def update_playlist(self, playlist_mbid: str, title: str, tracks: List[Dict], public: bool = False) -> Dict: + """Replace an existing LB playlist's contents IN PLACE (stable URL/MBID) (#903). + + Verifies the playlist still exists, clears its current items, re-adds the new tracks, + and updates the title. If the playlist is gone (deleted on LB), returns success=False + with ``gone=True`` so the caller can fall back to creating a fresh one. + Returns the same shape as ``create_playlist`` plus ``updated=True``. + """ + result = {"success": False, "playlist_mbid": playlist_mbid, + "playlist_url": f"https://listenbrainz.org/playlist/{playlist_mbid}", + "added": 0, "requested": len(tracks or []), "error": None, + "updated": True, "gone": False} + if not self.is_authenticated(): + result["error"] = "ListenBrainz not authenticated (no token/username)" + return result + + count = self.get_playlist_track_count(playlist_mbid) + if count is None: + result["error"] = "playlist not found on ListenBrainz" + result["gone"] = True + return result + + headers = self._lb_headers() + # Clear existing items (one range delete from the top). + if count > 0: + try: + self._make_request_with_retry( + "POST", f"{self.base_url}/playlist/{playlist_mbid}/item/delete", + json={"index": 0, "count": count}, headers=headers, + ) + except Exception as e: + logger.warning(f"ListenBrainz item/delete (clear) failed: {e}") + + result["added"] = self._add_tracks_in_batches(playlist_mbid, tracks) + + # Refresh the title (best-effort — content already replaced). + try: + self._make_request_with_retry( + "POST", f"{self.base_url}/playlist/edit/{playlist_mbid}", + json={"playlist": {"title": (title or "SoulSync Export").strip() or "SoulSync Export"}}, + headers=headers, + ) + except Exception as e: + logger.debug(f"ListenBrainz playlist title edit failed: {e}") + + result["success"] = True + return result + + def create_or_update_playlist(self, title: str, tracks: List[Dict], + existing_mbid: str = None, public: bool = False) -> Dict: + """Update the existing LB playlist in place when we've pushed this one before, else + create a fresh one — so re-exporting the same SoulSync playlist never duplicates it. + Falls back to create if the remembered playlist was deleted on LB.""" + if existing_mbid: + res = self.update_playlist(existing_mbid, title, tracks, public) + if res.get("success"): + return res + # Remembered playlist gone/failed -> create a new one instead of erroring out. + logger.info(f"ListenBrainz playlist {existing_mbid} unavailable for update " + f"({res.get('error')}); creating a new one.") + return self.create_playlist(title, tracks, public) + def get_playlists_created_for_user(self, count: int = 25, offset: int = 0) -> List[Dict]: """ Fetch playlists created FOR the user (recommendations, personalized playlists) diff --git a/database/music_database.py b/database/music_database.py index d097b0cf..1b140874 100644 --- a/database/music_database.py +++ b/database/music_database.py @@ -2097,6 +2097,19 @@ class MusicDatabase: ) """) + # Remember which external playlist a mirrored playlist was exported to, so a + # re-export UPDATES it in place instead of creating a duplicate (#903). Keyed by + # (mirrored playlist, target service) -> the target's playlist id (LB recording MBID). + cursor.execute(""" + CREATE TABLE IF NOT EXISTS playlist_export_targets ( + mirrored_playlist_id INTEGER NOT NULL, + target TEXT NOT NULL, + target_playlist_mbid TEXT NOT NULL, + updated_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, + PRIMARY KEY (mirrored_playlist_id, target) + ) + """) + # Discovery artist blacklist — artists users never want to see in discovery cursor.execute(""" CREATE TABLE IF NOT EXISTS discovery_artist_blacklist ( @@ -14167,6 +14180,42 @@ class MusicDatabase: logger.error(f"Error updating custom_name for playlist {playlist_id}: {e}") return False + def get_playlist_export_target(self, mirrored_playlist_id: int, target: str) -> Optional[str]: + """The external playlist id this mirror was last exported to (or None). #903.""" + try: + with self._get_connection() as conn: + cur = conn.cursor() + cur.execute( + "SELECT target_playlist_mbid FROM playlist_export_targets " + "WHERE mirrored_playlist_id = ? AND target = ? LIMIT 1", + (int(mirrored_playlist_id), target), + ) + row = cur.fetchone() + if row: + return (row[0] if not hasattr(row, "keys") else row["target_playlist_mbid"]) or None + except Exception as e: + logger.debug(f"get_playlist_export_target failed: {e}") + return None + + def set_playlist_export_target(self, mirrored_playlist_id: int, target: str, target_mbid: str) -> bool: + """Remember the external playlist id for this mirror (idempotent). #903.""" + if not target_mbid: + return False + try: + with self._get_connection() as conn: + cur = conn.cursor() + cur.execute( + "INSERT OR REPLACE INTO playlist_export_targets " + "(mirrored_playlist_id, target, target_playlist_mbid, updated_at) " + "VALUES (?, ?, ?, CURRENT_TIMESTAMP)", + (int(mirrored_playlist_id), target, target_mbid), + ) + conn.commit() + return True + except Exception as e: + logger.debug(f"set_playlist_export_target failed: {e}") + return False + def get_mirrored_playlist_tracks(self, playlist_id: int) -> List[Dict]: """Return all tracks for a mirrored playlist ordered by position.""" try: diff --git a/tests/exports/test_listenbrainz_create_playlist.py b/tests/exports/test_listenbrainz_create_playlist.py index a87cb440..72021e80 100644 --- a/tests/exports/test_listenbrainz_create_playlist.py +++ b/tests/exports/test_listenbrainz_create_playlist.py @@ -84,3 +84,77 @@ def test_create_ok_but_partial_add_failure(monkeypatch): assert res["success"] is True assert res["playlist_mbid"] == "PL" assert res["added"] == 0 + + +# ── update-in-place + create-or-update (#903 no-duplicate re-export) ────────── + +def _route(existing_count=None): + """Build a fake _make_request_with_retry; records calls. existing_count=None -> GET 404.""" + calls = [] + def fake(method, url, **kw): + calls.append((method, url, kw.get("json"))) + if method == "GET" and "/playlist/" in url: + if existing_count is None: + return _Resp(404, {}) + return _Resp(200, {"playlist": {"track": [{} for _ in range(existing_count)]}}) + if url.endswith("/playlist/create"): + return _Resp(200, {"playlist_mbid": "NEW-MBID"}) + return _Resp(200, {}) # item/add, item/delete, edit, delete + return fake, calls + + +def test_update_playlist_clears_then_re_adds_same_mbid(monkeypatch): + c = _client() + fake, calls = _route(existing_count=3) + monkeypatch.setattr(c, "_make_request_with_retry", fake) + res = c.update_playlist("EXIST-MBID", "New Title", _tracks(5)) + assert res["success"] is True and res["updated"] is True + assert res["playlist_mbid"] == "EXIST-MBID" # stable URL/MBID + assert res["added"] == 5 + urls = [u for _, u, _ in calls] + assert any("/item/delete" in u for u in urls) # cleared existing + assert any("/item/add" in u for u in urls) # re-added + assert any("/playlist/edit/EXIST-MBID" in u for u in urls) # title refreshed + + +def test_update_playlist_reports_gone(monkeypatch): + c = _client() + fake, _ = _route(existing_count=None) # GET 404 -> gone + monkeypatch.setattr(c, "_make_request_with_retry", fake) + res = c.update_playlist("DEAD-MBID", "T", _tracks(2)) + assert res["success"] is False + assert res["gone"] is True + + +def test_create_or_update_updates_when_existing(monkeypatch): + c = _client() + fake, _ = _route(existing_count=2) + monkeypatch.setattr(c, "_make_request_with_retry", fake) + res = c.create_or_update_playlist("T", _tracks(4), existing_mbid="EXIST") + assert res["updated"] is True and res["playlist_mbid"] == "EXIST" + + +def test_create_or_update_falls_back_to_create_when_gone(monkeypatch): + c = _client() + fake, _ = _route(existing_count=None) # remembered playlist deleted on LB + monkeypatch.setattr(c, "_make_request_with_retry", fake) + res = c.create_or_update_playlist("T", _tracks(4), existing_mbid="DEAD") + assert res["success"] is True + assert res["updated"] is False # fell back to create + assert res["playlist_mbid"] == "NEW-MBID" + + +def test_create_or_update_creates_when_no_existing(monkeypatch): + c = _client() + fake, _ = _route() + monkeypatch.setattr(c, "_make_request_with_retry", fake) + res = c.create_or_update_playlist("T", _tracks(3), existing_mbid=None) + assert res["playlist_mbid"] == "NEW-MBID" and res["updated"] is False + + +def test_delete_playlist(monkeypatch): + c = _client() + monkeypatch.setattr(c, "_make_request_with_retry", lambda *a, **k: _Resp(200, {})) + assert c.delete_playlist("MBID") is True + monkeypatch.setattr(c, "_make_request_with_retry", lambda *a, **k: _Resp(404, {})) + assert c.delete_playlist("MBID") is False diff --git a/web_server.py b/web_server.py index c2bd38b8..4ce8787f 100644 --- a/web_server.py +++ b/web_server.py @@ -27200,9 +27200,13 @@ def _run_playlist_export(job_id, playlist_id, title, mode): job['phase'] = 'pushing' from core.listenbrainz_client import ListenBrainzClient client = ListenBrainzClient() - res = client.create_playlist(title, jspf['playlist']['track']) + # Re-export updates the same LB playlist in place instead of duplicating it (#903). + existing = db.get_playlist_export_target(int(playlist_id), 'listenbrainz') + res = client.create_or_update_playlist(title, jspf['playlist']['track'], existing_mbid=existing) job['push'] = res if res.get('success'): + if res.get('playlist_mbid'): + db.set_playlist_export_target(int(playlist_id), 'listenbrainz', res['playlist_mbid']) job['phase'] = 'done' else: job['phase'] = 'error'