#901: give file-import playlist tracks a stable id so manual matches stick
A Find & Add on a file-import (CSV/M3U/TXT) playlist track was silently dropped and the track re-appeared as 'extra' (radoslav-orlov). Root cause: unlike Spotify/YouTube (native ids), file-import + iTunes-only tracks arrive with an EMPTY source_track_id — and the whole manual-match system keys on it. _persist_find_and_add_match is a no-op on an empty id, and find_manual_library_match_by_source_track_id returns None for one, so the match can be neither recorded nor looked up. That's the youtube-vs-file difference the reporter noticed. Fix: stable_source_track_id() derives a DETERMINISTIC 'file:<hash>' id from the track identity (artist|title|album, normalized) when there's no native id; mirror_playlist assigns it so the SAME song gets the SAME id across re-imports/discovery — exactly what the match lookup needs. Native ids are used verbatim; bonus: discovery extra_data now survives a re-import for these tracks too. Tests: helper (native passthrough, deterministic + case/field-insensitive, distinct per song, empty-on-no-title, file: prefix); mirror_playlist integration (file tracks get stable distinct ids, stable across re-import, native ids untouched). 319 playlist/ sync/discovery/mirrored tests green.
This commit is contained in:
parent
0df1d55ed6
commit
6e622d30f1
4 changed files with 115 additions and 5 deletions
|
|
@ -19,6 +19,35 @@ from urllib.parse import parse_qs, urlparse
|
|||
|
||||
_SPOTIFY_ID_RE = re.compile(r"^[A-Za-z0-9]{16,32}$")
|
||||
|
||||
|
||||
def stable_source_track_id(track: Mapping, existing: Optional[str] = None) -> str:
|
||||
"""A stable per-track id for a mirrored-playlist track.
|
||||
|
||||
Spotify / YouTube / Deezer tracks carry a native id. File-import (CSV / M3U /
|
||||
TXT) and iTunes-only sources don't — they arrive with an empty
|
||||
``source_track_id``. The whole manual-match system (Find & Add ↔ sync) keys on
|
||||
``source_track_id``, and an empty key can neither be recorded (the persist is a
|
||||
no-op) nor looked up — so a manual match on a file-import track is silently
|
||||
dropped and the track re-appears as "extra" (#901).
|
||||
|
||||
When a native id is present it's used verbatim. Otherwise we derive a
|
||||
DETERMINISTIC id from the track's identity (artist|title|album, normalized) so
|
||||
the SAME song gets the SAME id across re-imports and discovery passes — which
|
||||
is exactly what the match lookup needs. Prefixed ``file:`` so it's recognizable
|
||||
and never collides with a real upstream id. Returns '' only when there's no
|
||||
usable identity at all (no title)."""
|
||||
native = (existing if existing is not None else track.get("source_track_id")) or ""
|
||||
native = str(native).strip()
|
||||
if native:
|
||||
return native
|
||||
title = str(track.get("track_name") or track.get("name") or "").strip().lower()
|
||||
if not title:
|
||||
return ""
|
||||
artist = str(track.get("artist_name") or track.get("artist") or "").strip().lower()
|
||||
album = str(track.get("album_name") or track.get("album") or "").strip().lower()
|
||||
digest = hashlib.md5(f"{artist}|{title}|{album}".encode("utf-8")).hexdigest()[:16]
|
||||
return f"file:{digest}"
|
||||
|
||||
# Synthetic batch playlist_id prefixes that wrap a mirrored_playlists PK.
|
||||
# Download/discovery flows build a batch playlist_id as f"{prefix}{pk}" — e.g.
|
||||
# auto_mirror_<pk> (core/automation/handlers/sync_playlist.py), youtube_mirrored_<pk>
|
||||
|
|
|
|||
|
|
@ -13933,16 +13933,20 @@ class MusicDatabase:
|
|||
logger.debug("Failed to preserve mirrored playlist extra_data: %s", e)
|
||||
|
||||
# Replace all tracks
|
||||
from core.playlists.source_refs import stable_source_track_id
|
||||
cursor.execute("DELETE FROM mirrored_playlist_tracks WHERE playlist_id=?", (playlist_id,))
|
||||
for i, t in enumerate(tracks):
|
||||
# File-import / iTunes-only tracks arrive with no native id; give
|
||||
# them a DETERMINISTIC one so a Find & Add manual match can be
|
||||
# recorded and found (it keys on source_track_id) instead of being
|
||||
# silently dropped and re-appearing as "extra" (#901).
|
||||
sid = stable_source_track_id(t)
|
||||
extra = t.get('extra_data')
|
||||
if extra and not isinstance(extra, str):
|
||||
extra = json.dumps(extra)
|
||||
# Restore preserved discovery data if the incoming track doesn't have its own
|
||||
if not extra:
|
||||
sid = t.get('source_track_id')
|
||||
if sid and sid in old_extra_map:
|
||||
extra = old_extra_map[sid]
|
||||
if not extra and sid and sid in old_extra_map:
|
||||
extra = old_extra_map[sid]
|
||||
cursor.execute("""
|
||||
INSERT INTO mirrored_playlist_tracks
|
||||
(playlist_id, position, track_name, artist_name, album_name, duration_ms, image_url, source_track_id, extra_data)
|
||||
|
|
@ -13951,7 +13955,7 @@ class MusicDatabase:
|
|||
playlist_id, i + 1,
|
||||
t.get('track_name', ''), t.get('artist_name', ''),
|
||||
t.get('album_name', ''), t.get('duration_ms', 0),
|
||||
t.get('image_url'), t.get('source_track_id'), extra
|
||||
t.get('image_url'), sid or None, extra
|
||||
))
|
||||
conn.commit()
|
||||
logger.info(f"Mirrored playlist '{name}' ({source}) with {len(tracks)} tracks")
|
||||
|
|
|
|||
|
|
@ -60,3 +60,35 @@ def test_mirror_playlist_refresh_preserves_existing_description(tmp_path):
|
|||
assert refreshed_id == playlist_id
|
||||
playlist = db.get_mirrored_playlist(playlist_id)
|
||||
assert playlist["description"] == "https://open.spotify.com/playlist/abc"
|
||||
|
||||
|
||||
def test_file_import_tracks_get_a_stable_source_track_id(tmp_path):
|
||||
# #901: file-import tracks arrive with no source_track_id; mirror_playlist must
|
||||
# assign a deterministic one so a Find & Add manual match can key on it (and so
|
||||
# discovery extra_data survives a re-import).
|
||||
db = MusicDatabase(str(tmp_path / "music.db"))
|
||||
file_tracks = [
|
||||
{"track_name": "Slow Ride", "artist_name": "Foghat", "album_name": "Fool for the City"},
|
||||
{"track_name": "I Gotta Feeling", "artist_name": "The Black Eyed Peas"},
|
||||
]
|
||||
pid = db.mirror_playlist(source="file", source_playlist_id="myfile", name="From File",
|
||||
tracks=file_tracks, profile_id=1)
|
||||
rows = db.get_mirrored_playlist_tracks(pid)
|
||||
ids = [r["source_track_id"] for r in rows]
|
||||
assert all(i and i.startswith("file:") for i in ids) # no empty ids
|
||||
assert len(set(ids)) == 2 # distinct per song
|
||||
|
||||
# Re-import the SAME file → SAME ids (stable), so a recorded match still keys.
|
||||
db.mirror_playlist(source="file", source_playlist_id="myfile", name="From File",
|
||||
tracks=list(file_tracks), profile_id=1)
|
||||
rows2 = db.get_mirrored_playlist_tracks(pid)
|
||||
assert [r["source_track_id"] for r in rows2] == ids
|
||||
|
||||
|
||||
def test_native_ids_still_used_verbatim(tmp_path):
|
||||
db = MusicDatabase(str(tmp_path / "music.db"))
|
||||
pid = db.mirror_playlist(source="spotify", source_playlist_id="sp", name="Sp",
|
||||
tracks=[{"track_name": "S", "artist_name": "A", "source_track_id": "spotify123"}],
|
||||
profile_id=1)
|
||||
rows = db.get_mirrored_playlist_tracks(pid)
|
||||
assert rows[0]["source_track_id"] == "spotify123" # native id untouched
|
||||
|
|
|
|||
45
tests/playlists/test_stable_source_track_id.py
Normal file
45
tests/playlists/test_stable_source_track_id.py
Normal file
|
|
@ -0,0 +1,45 @@
|
|||
"""#901: a manual match (Find & Add) on a file-import playlist track was silently
|
||||
dropped and the track re-appeared as "extra". Root cause: file-import / iTunes-only
|
||||
tracks arrive with an EMPTY source_track_id, and the whole manual-match system keys
|
||||
on it — an empty key can't be persisted (no-op) or looked up. Fix: derive a stable
|
||||
deterministic id from the track's identity so matches stick like they do for
|
||||
Spotify/YouTube (which carry native ids).
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from core.playlists.source_refs import stable_source_track_id
|
||||
|
||||
|
||||
def test_native_id_is_used_verbatim():
|
||||
assert stable_source_track_id({"source_track_id": "2fdfsGuqb6SBX5ocoBWHUd"}) == "2fdfsGuqb6SBX5ocoBWHUd"
|
||||
# explicit existing wins over the dict
|
||||
assert stable_source_track_id({"source_track_id": "x"}, existing="y") == "y"
|
||||
|
||||
|
||||
def test_file_track_gets_a_deterministic_prefixed_id():
|
||||
t = {"track_name": "Slow Ride", "artist_name": "Foghat", "album_name": "Fool for the City"}
|
||||
a = stable_source_track_id(t)
|
||||
assert a.startswith("file:") and len(a) == len("file:") + 16
|
||||
# SAME song → SAME id across calls/re-imports (what the match lookup needs)
|
||||
assert stable_source_track_id(dict(t)) == a
|
||||
|
||||
|
||||
def test_identity_is_case_and_field_insensitive_but_distinguishes_songs():
|
||||
base = {"track_name": "Slow Ride", "artist_name": "Foghat", "album_name": "Fool for the City"}
|
||||
same = {"name": "slow ride", "artist": "FOGHAT", "album": "fool for the city"} # alt field names + case
|
||||
assert stable_source_track_id(base) == stable_source_track_id(same)
|
||||
# a different song gets a different id
|
||||
assert stable_source_track_id(base) != stable_source_track_id(
|
||||
{"track_name": "I Just Want to Make Love to You", "artist_name": "Foghat"})
|
||||
|
||||
|
||||
def test_empty_id_when_no_title():
|
||||
assert stable_source_track_id({"artist_name": "Foghat"}) == ""
|
||||
assert stable_source_track_id({}) == ""
|
||||
|
||||
|
||||
def test_never_collides_with_a_real_upstream_id():
|
||||
# the file: prefix keeps synthetic ids out of the spotify/youtube id space
|
||||
fid = stable_source_track_id({"track_name": "x", "artist_name": "y"})
|
||||
assert ":" in fid and not fid.replace("file:", "").startswith("file")
|
||||
Loading…
Reference in a new issue