From e32e2e5e14e327af3b52b32a26f624bc2733fc2a Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Tue, 9 Jun 2026 16:31:05 -0700 Subject: [PATCH] =?UTF-8?q?Sync:=20append=20mode=20actually=20dedupes=20?= =?UTF-8?q?=E2=80=94=20stop=20re-adding=20the=20whole=20playlist=20(#823)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit carlosjfcasero round 2: after 6fa956d6 append stopped recreating the playlist, but every sync re-appended ALL matched tracks — every track N times. His log shows it plainly: "added 22 new tracks to 'Disney' (skipped 0 already present)" on a playlist that already had those 22. Root cause: the dedupe read `{t.id for t in get_playlist_tracks(...)}` — but JellyfinTrack only defines `ratingKey`, never `id`, so the existing-ids set was ALWAYS empty and everything looked new. NavidromeTrack is also ratingKey-only, so the Navidrome append had the identical bug. Plex (plexapi ratingKey) was fine. The existing tests were green because they mocked existing tracks as SimpleNamespace(id=...) — encoding the same wrong assumption as the code. Fix: - New pure planner plan_playlist_append(current, desired) in core/sync/playlist_edit.py (next to the reconcile planner): order-preserving, drops already-present ids, dedupes within desired, stringifies (Emby numeric vs string safe). - Jellyfin/Emby: existing ids fetched from the canonical /Playlists/{id}/Items endpoint (same as reconcile — works for Jellyfin GUIDs and Emby numeric ids), ratingKey fallback if that request fails. - Navidrome: dedupe on ratingKey (the attribute that actually exists). Tests: planner (skip-present incl. the reporter's unchanged-playlist case, desired-order, dupes-within-desired, int/str ids, empties) + the append-mode suite rewritten to pin the REAL shapes (raw Items dicts for Jellyfin, ratingKey objects for Navidrome) + a new fallback-path test. 524 playlist/sync/jellyfin/navidrome tests pass. --- core/jellyfin_client.py | 34 +++++++++++++---- core/navidrome_client.py | 19 +++++++--- core/sync/playlist_edit.py | 29 ++++++++++++++ tests/test_playlist_edit.py | 35 +++++++++++++++++ tests/test_server_playlist_append_mode.py | 46 ++++++++++++++++++----- 5 files changed, 140 insertions(+), 23 deletions(-) diff --git a/core/jellyfin_client.py b/core/jellyfin_client.py index 8ee4eb74..f24020d5 100644 --- a/core/jellyfin_client.py +++ b/core/jellyfin_client.py @@ -1566,20 +1566,40 @@ class JellyfinClient(MediaServerClient): return self.create_playlist(playlist_name, tracks) playlist_id = existing_playlist.id - existing_tracks = self.get_playlist_tracks(playlist_id) - existing_ids = { - str(t.id) for t in existing_tracks if hasattr(t, 'id') and t.id - } + # #823 round 2: the old dedupe read `t.id` off get_playlist_tracks() + # results — but JellyfinTrack only defines `ratingKey`, so the + # existing-ids set was ALWAYS empty and every sync re-appended the + # whole matched list ("added 22 ... skipped 0" on a playlist that + # already had them, every track N times). Fetch the playlist's items + # from the canonical /Playlists/{id}/Items endpoint (the same one + # reconcile uses — works on Jellyfin GUIDs and Emby numeric ids) and + # dedupe on the raw item Id; fall back to ratingKey if that fails. + existing_ids = set() + items_resp = self._make_request( + f'/Playlists/{playlist_id}/Items', {'UserId': self.user_id}) + if items_resp: + for item in items_resp.get('Items', []): + iid = str(item.get('Id') or '') + if iid: + existing_ids.add(iid) + else: + existing_ids = { + str(getattr(t, 'ratingKey', '') or '') + for t in self.get_playlist_tracks(playlist_id) + } - {''} - new_track_ids = [] + desired_ids = [] for t in tracks: tid = None if hasattr(t, 'id'): tid = str(t.id) if t.id else None elif isinstance(t, dict): tid = str(t.get('Id') or t.get('id') or '') - if tid and tid not in existing_ids and self._is_valid_guid(tid): - new_track_ids.append(tid) + if tid and self._is_valid_guid(tid): + desired_ids.append(tid) + + from core.sync.playlist_edit import plan_playlist_append + new_track_ids = plan_playlist_append(existing_ids, desired_ids) if not new_track_ids: logger.info( diff --git a/core/navidrome_client.py b/core/navidrome_client.py index 085a4257..5a0677ee 100644 --- a/core/navidrome_client.py +++ b/core/navidrome_client.py @@ -1102,12 +1102,16 @@ class NavidromeClient(MediaServerClient): return self.create_playlist(playlist_name, tracks) primary = existing_playlists[0] - existing_tracks = self.get_playlist_tracks(primary.id) + # #823 round 2: the old dedupe read `t.id` — but NavidromeTrack only + # defines `ratingKey`, so the existing-ids set was ALWAYS empty and + # every sync re-appended the whole matched list (every track N + # times). Same bug as the Jellyfin append; dedupe on ratingKey. existing_ids = { - str(t.id) for t in existing_tracks if hasattr(t, 'id') and t.id - } + str(getattr(t, 'ratingKey', '') or '') + for t in self.get_playlist_tracks(primary.id) + } - {''} - new_track_ids = [] + desired_ids = [] for t in tracks: tid = None if hasattr(t, 'ratingKey'): @@ -1116,8 +1120,11 @@ class NavidromeClient(MediaServerClient): tid = str(t.id) if t.id else None elif isinstance(t, dict): tid = str(t.get('id') or '') - if tid and tid not in existing_ids: - new_track_ids.append(tid) + if tid: + desired_ids.append(tid) + + from core.sync.playlist_edit import plan_playlist_append + new_track_ids = plan_playlist_append(existing_ids, desired_ids) if not new_track_ids: logger.info( diff --git a/core/sync/playlist_edit.py b/core/sync/playlist_edit.py index 80325238..a3423302 100644 --- a/core/sync/playlist_edit.py +++ b/core/sync/playlist_edit.py @@ -109,6 +109,34 @@ def plan_playlist_reconcile( return {"add": add, "remove": remove} +def plan_playlist_append( + current_ids: List[str], + desired_ids: List[str], +) -> List[str]: + """Plan an append: which desired ids are NOT already in the playlist. + + Used by ``sync_mode='append'`` (#823 round 2): the Jellyfin/Emby and + Navidrome appends deduped with ``{t.id for t in existing}`` — but their + track wrappers only define ``ratingKey``, never ``id``, so the existing-ids + set was ALWAYS empty and every sync re-appended the full matched list + (every track N times, "skipped 0 already present"). Pure planner so the + dedupe logic is testable; the caller fetches current ids however its + server API works and applies the returned adds. + + Order-preserving and duplicate-safe: desired order is kept, ids already + present are dropped, and duplicates WITHIN desired are emitted once. + """ + current_set = {str(t) for t in current_ids} + out: List[str] = [] + seen = set() + for d in desired_ids: + tid = str(d) + if tid and tid not in current_set and tid not in seen: + seen.add(tid) + out.append(tid) + return out + + VALID_SYNC_MODES = ("replace", "append", "reconcile") @@ -129,6 +157,7 @@ __all__ = [ "plan_playlist_add", "remove_one_occurrence", "plan_playlist_reconcile", + "plan_playlist_append", "normalize_sync_mode", "VALID_SYNC_MODES", ] diff --git a/tests/test_playlist_edit.py b/tests/test_playlist_edit.py index 66caae40..7c402992 100644 --- a/tests/test_playlist_edit.py +++ b/tests/test_playlist_edit.py @@ -161,3 +161,38 @@ def test_normalize_falls_back_for_unknown(): def test_normalize_all_real_modes_pass_through(): for m in ('replace', 'append', 'reconcile'): assert normalize_sync_mode(m, 'replace') == m + + +# ── plan_playlist_append (#823 round 2) ────────────────────────────────────── +# The Jellyfin/Emby + Navidrome appends deduped on `t.id`, but their track +# wrappers only define `ratingKey` — the existing-ids set was always empty, so +# every sync re-appended the full matched list (every track N times, +# "skipped 0 already present"). The planner is the now-testable dedupe. + +from core.sync.playlist_edit import plan_playlist_append # noqa: E402 + + +def test_append_skips_already_present(): + # carlosjfcasero's case: second sync of an unchanged playlist adds NOTHING. + assert plan_playlist_append(['a', 'b', 'c'], ['a', 'b', 'c']) == [] + + +def test_append_adds_only_new_in_desired_order(): + assert plan_playlist_append(['a', 'c'], ['a', 'b', 'c', 'd']) == ['b', 'd'] + + +def test_append_to_empty_playlist_adds_all(): + assert plan_playlist_append([], ['a', 'b']) == ['a', 'b'] + + +def test_append_dedupes_within_desired(): + assert plan_playlist_append(['x'], ['a', 'a', 'x', 'b', 'a']) == ['a', 'b'] + + +def test_append_stringifies_ids(): + # Emby numeric ids may arrive as ints on one side and strings on the other. + assert plan_playlist_append([16607838], ['16607838', '999']) == ['999'] + + +def test_append_ignores_empty_ids(): + assert plan_playlist_append(['a'], ['', 'b']) == ['b'] diff --git a/tests/test_server_playlist_append_mode.py b/tests/test_server_playlist_append_mode.py index 8624fb1a..97af0c83 100644 --- a/tests/test_server_playlist_append_mode.py +++ b/tests/test_server_playlist_append_mode.py @@ -173,13 +173,19 @@ class TestJellyfinAppendToPlaylist: mock_create.assert_called_once_with("Test", new_tracks) def test_filters_out_already_present_tracks(self): - """Reporter's exact case for Jellyfin — only new GUIDs go in.""" + """Reporter's exact case for Jellyfin — only new GUIDs go in. + + #823 round 2: existing ids now come from the canonical + /Playlists/{id}/Items request ({'Items':[{'Id': ...}]}) — the old + get_playlist_tracks path returned JellyfinTrack objects that only + define `ratingKey`, so the previous `t.id` dedupe was always empty and + every sync re-appended everything. These mocks pin the REAL shape.""" client = _make_jellyfin_client() existing_playlist = SimpleNamespace(id='pl-1') - existing_tracks = [ - SimpleNamespace(id='aaaaaaaa-bbbb-cccc-dddd-000000000001'), - SimpleNamespace(id='aaaaaaaa-bbbb-cccc-dddd-000000000002'), - ] + items_resp = {'Items': [ + {'Id': 'aaaaaaaa-bbbb-cccc-dddd-000000000001'}, + {'Id': 'aaaaaaaa-bbbb-cccc-dddd-000000000002'}, + ]} incoming = [ SimpleNamespace(id='aaaaaaaa-bbbb-cccc-dddd-000000000001'), # present SimpleNamespace(id='aaaaaaaa-bbbb-cccc-dddd-000000000003'), # NEW @@ -194,7 +200,7 @@ class TestJellyfinAppendToPlaylist: with patch.object(client, 'ensure_connection', return_value=True), \ patch.object(client, 'get_playlist_by_name', return_value=existing_playlist), \ - patch.object(client, 'get_playlist_tracks', return_value=existing_tracks), \ + patch.object(client, '_make_request', return_value=items_resp), \ patch.object(client, '_is_valid_guid', return_value=True), \ patch('core.jellyfin_client.requests.post', side_effect=fake_post): result = client.append_to_playlist("Test", incoming) @@ -206,9 +212,26 @@ class TestJellyfinAppendToPlaylist: def test_short_circuits_when_no_new_tracks(self): client = _make_jellyfin_client() existing_playlist = SimpleNamespace(id='pl-1') - existing_tracks = [SimpleNamespace(id='guid-1')] + items_resp = {'Items': [{'Id': 'guid-1'}]} with patch.object(client, 'ensure_connection', return_value=True), \ patch.object(client, 'get_playlist_by_name', return_value=existing_playlist), \ + patch.object(client, '_make_request', return_value=items_resp), \ + patch.object(client, '_is_valid_guid', return_value=True), \ + patch('core.jellyfin_client.requests.post') as mock_post: + result = client.append_to_playlist("Test", [SimpleNamespace(id='guid-1')]) + assert result is True + mock_post.assert_not_called() + + def test_falls_back_to_ratingkey_tracks_when_items_request_fails(self): + """When /Playlists/{id}/Items fails, dedupe falls back to + get_playlist_tracks — reading ratingKey (the attr that EXISTS on + JellyfinTrack), not the never-present `.id` of the original bug.""" + client = _make_jellyfin_client() + existing_playlist = SimpleNamespace(id='pl-1') + existing_tracks = [SimpleNamespace(ratingKey='guid-1')] + with patch.object(client, 'ensure_connection', return_value=True), \ + patch.object(client, 'get_playlist_by_name', return_value=existing_playlist), \ + patch.object(client, '_make_request', return_value=None), \ patch.object(client, 'get_playlist_tracks', return_value=existing_tracks), \ patch.object(client, '_is_valid_guid', return_value=True), \ patch('core.jellyfin_client.requests.post') as mock_post: @@ -221,7 +244,7 @@ class TestJellyfinAppendToPlaylist: existing_playlist = SimpleNamespace(id='pl-1') with patch.object(client, 'ensure_connection', return_value=True), \ patch.object(client, 'get_playlist_by_name', return_value=existing_playlist), \ - patch.object(client, 'get_playlist_tracks', return_value=[]), \ + patch.object(client, '_make_request', return_value={'Items': []}), \ patch.object(client, '_is_valid_guid', return_value=True), \ patch('core.jellyfin_client.requests.post', return_value=SimpleNamespace(status_code=500, text='server error')): @@ -256,9 +279,12 @@ class TestNavidromeAppendToPlaylist: mock_create.assert_called_once() def test_filters_out_already_present_tracks_and_calls_subsonic(self): + # #823 round 2: existing tracks are NavidromeTrack objects, which only + # define `ratingKey` — the old `t.id` dedupe was always empty. These + # mocks pin the REAL attribute so the dedupe is actually exercised. client = _make_navidrome_client() existing_playlists = [SimpleNamespace(id='pl-1', title='Test')] - existing_tracks = [SimpleNamespace(id='100'), SimpleNamespace(id='101')] + existing_tracks = [SimpleNamespace(ratingKey='100'), SimpleNamespace(ratingKey='101')] incoming = [ SimpleNamespace(id='100'), # present SimpleNamespace(id='102'), # NEW @@ -287,7 +313,7 @@ class TestNavidromeAppendToPlaylist: def test_short_circuits_when_no_new_tracks(self): client = _make_navidrome_client() existing_playlists = [SimpleNamespace(id='pl-1', title='Test')] - existing_tracks = [SimpleNamespace(id='100')] + existing_tracks = [SimpleNamespace(ratingKey='100')] with patch.object(client, 'ensure_connection', return_value=True), \ patch.object(client, 'get_playlists_by_name', return_value=existing_playlists), \ patch.object(client, 'get_playlist_tracks', return_value=existing_tracks), \