diff --git a/CHANGELOG.md b/CHANGELOG.md index d296f5d..2565307 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,33 @@ All notable changes to this project are documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +### Fixed +- `verify` paired each source playlist with the wrong Engine list in three ways, + every one of which invents discrepancies on a correct conversion — the failure + mode that trains you to ignore verify. Same-named playlists in different + folders collapsed onto one list (Engine's uniqueness constraint is + per-parent, so this is legal and common); duplicates within one folder all + compared against the first, because the writer renames the second to + `"Name (2)"` while both source lists keep the original name; and a missing + playlist could resolve to an unrelated one whose title merely started the + same way, so `"House"` was verified against `"House (old)"`. +- Playlists are now paired on their full folder path. Engine's + unique-name-per-folder renaming lives in one shared module used by both the + writer that applies the names and the verifier that has to predict them — + verify re-deriving that by hand is what produced all three defects. + +### Changed +- **Breaking (text output).** Playlist discrepancies are keyed by folder path + rather than bare name: `playlist[Sets/Setlist].track_order`, previously + `playlist[Setlist].track_order`. Nothing parses these keys programmatically, + but scripts grepping verify's output will need updating. +- A playlist retitled in the database is now reported as missing instead of + being silently matched when its new title resembles a duplicate suffix. This + is divergence from the source and belongs in the report; classifying it as an + external edit rather than an absence is follow-up work. + ## [0.3.2] - 2026-07-31 ### Fixed diff --git a/src/rb2engine/playlist_naming.py b/src/rb2engine/playlist_naming.py new file mode 100644 index 0000000..522fd59 --- /dev/null +++ b/src/rb2engine/playlist_naming.py @@ -0,0 +1,86 @@ +"""Sibling ordering and Engine's unique-name-per-folder renaming. + +Shared by the writer, which applies these names, and by verify, which has to +predict them in order to pair each source playlist with the engine list it +became. + +The two must travel together: the suffix a duplicate receives depends on the +sibling ordering, so a caller that re-derived only the rename would still +disagree with the writer whenever the ordering mattered. Verify re-deriving both +by hand is what produced the pairing defects this module exists to prevent. + +Paths are tuples of titles rather than a joined string because a rekordbox +playlist name may itself contain a separator character; joining is for display +only. +""" + +from __future__ import annotations + +from collections import defaultdict +from collections.abc import Sequence + +from rb2engine.ir import SourcePlaylist + +_DISPLAY_SEP = "/" + + +def sibling_order(playlists: Sequence[SourcePlaylist]) -> dict[int, list[int]]: + """``parent_rb_id`` → child rb_ids in ``(sort_order, rb_id)`` order.""" + by_rb = {pl.rb_id: pl for pl in playlists} + groups: dict[int, list[int]] = defaultdict(list) + for pl in playlists: + groups[pl.parent_rb_id].append(pl.rb_id) + for rb_ids in groups.values(): + rb_ids.sort(key=lambda r: (by_rb[r].sort_order, r)) + return dict(groups) + + +def resolve_titles(playlists: Sequence[SourcePlaylist]) -> dict[int, str]: + """``rb_id`` → the title the writer will give it. + + rekordbox permits two playlists with the same name in one folder; Engine + does not (``C_NAME_UNIQUE_FOR_PARENT``). The second and later duplicates + become ``"Name (2)"``, ``"Name (3)"``, … in sibling order, which is + deterministic, so re-runs produce identical names. + """ + by_rb = {pl.rb_id: pl for pl in playlists} + titles: dict[int, str] = {} + for rb_ids in sibling_order(playlists).values(): + seen: dict[str, int] = {} + for rb in rb_ids: + original = by_rb[rb].name + count = seen.get(original, 0) + seen[original] = count + 1 + titles[rb] = original if count == 0 else f"{original} ({count + 1})" + return titles + + +def resolve_paths( + playlists: Sequence[SourcePlaylist], +) -> dict[int, tuple[str, ...]]: + """``rb_id`` → its resolved titles from the root, root first. + + A playlist is identified by its whole path: the same title may legally + appear under several folders, since Engine's uniqueness constraint is + per-parent. + """ + by_rb = {pl.rb_id: pl for pl in playlists} + titles = resolve_titles(playlists) + paths: dict[int, tuple[str, ...]] = {} + for rb in titles: + parts: list[str] = [] + cur = rb + walked: set[int] = set() + # A parent cycle is rejected by the writer; guard anyway so verify + # cannot hang on a malformed source. + while cur != 0 and cur in by_rb and cur not in walked: + walked.add(cur) + parts.append(titles[cur]) + cur = by_rb[cur].parent_rb_id + paths[rb] = tuple(reversed(parts)) + return paths + + +def format_path(path: Sequence[str]) -> str: + """Human-readable form of a playlist path, for report field names.""" + return _DISPLAY_SEP.join(path) diff --git a/src/rb2engine/verify.py b/src/rb2engine/verify.py index a02c499..de018ed 100644 --- a/src/rb2engine/verify.py +++ b/src/rb2engine/verify.py @@ -19,6 +19,7 @@ from rb2engine.ir import SourceLibrary, SourcePlaylist, SourceTrack from rb2engine.ir_engine import artwork_content_hash from rb2engine.mapper.track import map_track +from rb2engine.playlist_naming import format_path, resolve_paths from rb2engine.reader.library import read_library from rb2engine.writer.blobs import ( decode_beat_data, @@ -537,21 +538,23 @@ def _compare_playlists( ) ) - # Title → list id (first match; Engine renames duplicates with " (N)"). - title_to_id: dict[str, int] = {} - for row in conn.execute("SELECT id, title FROM Playlist"): - title_to_id[str(row[1])] = int(row[0]) + # Pair on the whole path, not the title: the same title may legally exist + # under several folders (Engine's constraint is per-parent), and duplicates + # within one folder are renamed by the writer. Both are predicted by + # playlist_naming, which the writer uses to assign the names in the first + # place. + path_to_id = _db_playlist_paths(conn) + source_paths = resolve_paths(source.playlists) for pl in source.playlists: - list_id = title_to_id.get(pl.name) - if list_id is None: - # Renamed duplicate titles — try ordered suffix scan. - list_id = _find_playlist_id(title_to_id, pl) + path = source_paths[pl.rb_id] + label = format_path(path) + list_id = path_to_id.get(path) if list_id is None: discrepancies.append( Discrepancy( track_id=None, - field=f"playlist[{pl.name}].missing", + field=f"playlist[{label}].missing", expected="present", actual="absent", ) @@ -568,7 +571,7 @@ def _compare_playlists( discrepancies.append( Discrepancy( track_id=None, - field=f"playlist[{pl.name}].chain", + field=f"playlist[{label}].chain", expected="every row reachable from the nextEntityId chain", actual=chain_problem, ) @@ -577,23 +580,38 @@ def _compare_playlists( discrepancies.append( Discrepancy( track_id=None, - field=f"playlist[{pl.name}].track_order", + field=f"playlist[{label}].track_order", expected=expected_track_ids, actual=actual_track_ids, ) ) -def _find_playlist_id( - title_to_id: dict[str, int], pl: SourcePlaylist -) -> int | None: - if pl.name in title_to_id: - return title_to_id[pl.name] - # insert_playlists renames duplicates to "Name (2)", "Name (3)", … - for title, lid in title_to_id.items(): - if title == pl.name or title.startswith(f"{pl.name} ("): - return lid - return None +def _db_playlist_paths( + conn: sqlite3.Connection, +) -> dict[tuple[str, ...], int]: + """Engine playlist path (root first) → list id. + + Built by walking ``parentListId`` to the root, so a title is only ever + matched within its own folder. + """ + rows: dict[int, tuple[str, int]] = { + int(r[0]): (str(r[1]), int(r[2])) + for r in conn.execute("SELECT id, title, parentListId FROM Playlist") + } + paths: dict[tuple[str, ...], int] = {} + for list_id in rows: + parts: list[str] = [] + cur = list_id + walked: set[int] = set() + # Guard against a cyclic parent chain in a database we did not write. + while cur != 0 and cur in rows and cur not in walked: + walked.add(cur) + title, parent = rows[cur] + parts.append(title) + cur = parent + paths[tuple(reversed(parts))] = list_id + return paths def _expected_entity_track_ids( diff --git a/src/rb2engine/writer/playlists.py b/src/rb2engine/writer/playlists.py index 8978515..5e3b771 100644 --- a/src/rb2engine/writer/playlists.py +++ b/src/rb2engine/writer/playlists.py @@ -24,6 +24,7 @@ from rb2engine.chain import walk_entity_chain from rb2engine.ir import SourcePlaylist +from rb2engine.playlist_naming import resolve_titles # Engine / libdjinterop sentinel: tail of every next*Id chain. _NO_NEXT = 0 @@ -142,20 +143,14 @@ def insert_playlists( # happened. The suffix is assigned in the already-deterministic sibling # order (sort_order, rb_id), so re-runs produce identical names and the # determinism guarantee holds. - title_of: dict[int, str] = {} - renamed: list[tuple[str, str]] = [] - for group in siblings.values(): - seen: dict[str, int] = {} - for rb in group: # already sorted by (sort_order, rb_id) - original = by_rb[rb].name - count = seen.get(original, 0) - seen[original] = count + 1 - if count == 0: - title_of[rb] = original - else: - new_title = f"{original} ({count + 1})" - title_of[rb] = new_title - renamed.append((original, new_title)) + # The algorithm lives in playlist_naming so verify predicts exactly what we + # write; it must not be reimplemented on either side. + title_of = resolve_titles(playlists) + renamed: list[tuple[str, str]] = [ + (by_rb[rb].name, title_of[rb]) + for rb in by_rb + if title_of[rb] != by_rb[rb].name + ] if renamed: logger.warning( "renamed %d playlist(s) to satisfy Engine's unique-name-per-folder " diff --git a/tests/unit/test_playlist_naming.py b/tests/unit/test_playlist_naming.py new file mode 100644 index 0000000..b093f2f --- /dev/null +++ b/tests/unit/test_playlist_naming.py @@ -0,0 +1,95 @@ +"""Contract tests for the naming shared by the writer and verify. + +WHY these are direct rather than only exercised through both callers: the whole +point of the module is that two independent consumers agree. If either drifts, +an integration test on one side can stay green while the pair silently +disagrees — which is the class of defect the module was extracted to end. +""" + +from __future__ import annotations + +from rb2engine.ir import SourcePlaylist +from rb2engine.playlist_naming import format_path, resolve_paths, resolve_titles + + +def _pl(rb_id: int, name: str, *, parent: int = 0, sort: int = 0) -> SourcePlaylist: + return SourcePlaylist( + rb_id=rb_id, + parent_rb_id=parent, + name=name, + sort_order=sort, + is_folder=False, + track_rb_ids=[], + ) + + +def test_suffixes_follow_sibling_order_not_input_order() -> None: + """The suffix depends on (sort_order, rb_id), never on list position. + + WHY: this is why the ordering and the rename must live in one function. A + caller that re-derived only the rename would suffix whichever duplicate it + happened to see first and disagree with the writer. + """ + playlists = [ + _pl(3, "Setlist", sort=2), + _pl(1, "Setlist", sort=0), + _pl(2, "Setlist", sort=1), + ] + + titles = resolve_titles(playlists) + + assert titles[1] == "Setlist" + assert titles[2] == "Setlist (2)" + assert titles[3] == "Setlist (3)" + + +def test_duplicate_names_in_different_folders_are_not_renamed() -> None: + """Engine's constraint is per-parent, so siblings-only collisions rename.""" + playlists = [ + _pl(1, "Folder A"), + _pl(2, "Folder B", sort=1), + _pl(3, "Chill", parent=1), + _pl(4, "Chill", parent=2), + ] + + titles = resolve_titles(playlists) + + assert titles[3] == "Chill" + assert titles[4] == "Chill" + + +def test_paths_distinguish_same_named_playlists() -> None: + playlists = [ + _pl(1, "Folder A"), + _pl(2, "Folder B", sort=1), + _pl(3, "Chill", parent=1), + _pl(4, "Chill", parent=2), + ] + + paths = resolve_paths(playlists) + + assert paths[3] == ("Folder A", "Chill") + assert paths[4] == ("Folder B", "Chill") + assert paths[3] != paths[4] + + +def test_path_is_a_tuple_so_a_name_containing_the_separator_stays_distinct() -> None: + """A "/" in a playlist name must not merge two different playlists. + + WHY: rekordbox permits "/" in names. Matching on a joined string would make + a playlist literally named "A/B" collide with "B" inside folder "A"; only + the display form joins. + """ + playlists = [ + _pl(1, "A"), + _pl(2, "B", parent=1), + _pl(3, "A/B", sort=1), + ] + + paths = resolve_paths(playlists) + + assert paths[2] == ("A", "B") + assert paths[3] == ("A/B",) + assert paths[2] != paths[3] + # Display collapses them; matching does not. + assert format_path(paths[2]) == format_path(paths[3]) == "A/B" diff --git a/tests/unit/test_verify.py b/tests/unit/test_verify.py index 137ab01..b9b144a 100644 --- a/tests/unit/test_verify.py +++ b/tests/unit/test_verify.py @@ -1048,13 +1048,19 @@ def test_verify_catches_playlist_rename( assert d.actual == "absent" -def test_verify_resolves_duplicate_playlist_title_suffix( +def test_verify_reports_externally_renamed_playlist( tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: - """Engine renames collisions to 'Name (2)'; verify must still find the list. - - WHY: insert_playlists suffixes duplicates. If verify only did exact title - match, every second playlist named 'Main Set' would false-fail as missing. + """A list retitled in the database must be reported, not silently matched. + + WHY: the writer only produces a " (N)" suffix when the SOURCE holds two + playlists of that name in one folder — see + test_verify_pairs_same_folder_duplicates_with_their_own_lists, which covers + that case. With a single source playlist no rename can occur, so a database + titled "Main Set (2)" has been changed by something other than us. Treating + it as a match assumes the database is right whenever its title merely looks + like a rename, which is how "House" came to be verified against the + unrelated list "House (old)". """ from rb2engine.verify import verify_library @@ -1062,7 +1068,6 @@ def test_verify_resolves_duplicate_playlist_title_suffix( _patch_read_library(monkeypatch, lib) conn = sqlite3.connect(str(m_db)) - # Simulate Engine's duplicate-title suffix without changing entity chain. conn.execute( "UPDATE Playlist SET title = ? WHERE title = ?", ("Main Set (2)", "Main Set"), @@ -1071,10 +1076,9 @@ def test_verify_resolves_duplicate_playlist_title_suffix( conn.close() result = verify_library(drive, with_artwork=False) - # Must NOT report playlist[Main Set].missing — suffix scan finds it. - assert "playlist[Main Set].missing" not in _fields(result.discrepancies) - # Track order still comparable via the resolved list id. - assert result.ok is True + + assert result.ok is False + assert "playlist[Main Set].missing" in _fields(result.discrepancies) def test_verify_catches_extra_playlist_count( @@ -1397,3 +1401,198 @@ def test_verify_catches_empty_artwork_blob( assert not result.ok assert any(".bytes" in d.field for d in result.discrepancies) + + +# --------------------------------------------------------------------------- +# Playlist pairing — verify must compare each source list against ITS OWN +# engine list. Getting this wrong invents discrepancies on a faithful build, +# which is worse than missing one: it trains the operator to ignore verify. +# --------------------------------------------------------------------------- + + +def _build_with_playlists( + tmp_path: Path, playlists: list[SourcePlaylist] +) -> tuple[Path, SourceLibrary, Path]: + """Build a 3-track library with caller-supplied playlists.""" + drive = tmp_path / "stick" + drive.mkdir() + (drive / "Contents").mkdir() + (drive / "PIONEER").mkdir() + + tracks = { + 10: _source_track(10, drive=drive, title="Alpha", filename="a.mp3"), + 20: _source_track(20, drive=drive, title="Beta", filename="b.mp3"), + 30: _source_track(30, drive=drive, title="Gamma", filename="c.mp3"), + } + lib = SourceLibrary( + drive_root=drive, tracks=tracks, playlists=playlists, warnings=[] + ) + m_db = build_library( + lib, drive_root=drive, report=ConversionReport(), with_artwork=False + ) + return drive, lib, m_db + + +def test_verify_pairs_same_name_playlists_in_different_folders( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Same title under two different folders must not collapse to one list. + + WHY: Engine's uniqueness constraint is per-parent, so "Chill" may legally + exist in several folders. Keying the lookup on title alone makes the last + row scanned win, and every same-named list is then compared against that + one — reporting a wrong-tracks discrepancy on a build that is correct. + """ + from rb2engine.verify import verify_library + + playlists = [ + SourcePlaylist( + rb_id=1, parent_rb_id=0, name="Folder A", sort_order=0, + is_folder=True, track_rb_ids=[], + ), + SourcePlaylist( + rb_id=2, parent_rb_id=0, name="Folder B", sort_order=1, + is_folder=True, track_rb_ids=[], + ), + SourcePlaylist( + rb_id=3, parent_rb_id=1, name="Chill", sort_order=0, + is_folder=False, track_rb_ids=[10, 20], + ), + SourcePlaylist( + rb_id=4, parent_rb_id=2, name="Chill", sort_order=0, + is_folder=False, track_rb_ids=[10, 30], + ), + ] + drive, lib, _ = _build_with_playlists(tmp_path, playlists) + _patch_read_library(monkeypatch, lib) + + result = verify_library(drive, with_artwork=False) + + assert result.ok is True, ( + "faithful build reported discrepancies: " + f"{[(d.field, d.expected, d.actual) for d in result.discrepancies]}" + ) + + +def test_verify_pairs_same_folder_duplicates_with_their_own_lists( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Renamed duplicates must each verify against the list they became. + + WHY: rekordbox allows two playlists of the same name in one folder; the + writer renames the second to "Name (2)". Both source lists still carry the + original name, so an exact-title lookup resolves BOTH to the first engine + list — the second is then diffed against the first's tracks. + """ + from rb2engine.verify import verify_library + + playlists = [ + SourcePlaylist( + rb_id=1, parent_rb_id=0, name="Sets", sort_order=0, + is_folder=True, track_rb_ids=[], + ), + SourcePlaylist( + rb_id=2, parent_rb_id=1, name="Setlist", sort_order=0, + is_folder=False, track_rb_ids=[10, 20], + ), + SourcePlaylist( + rb_id=3, parent_rb_id=1, name="Setlist", sort_order=1, + is_folder=False, track_rb_ids=[10, 30], + ), + ] + drive, lib, m_db = _build_with_playlists(tmp_path, playlists) + _patch_read_library(monkeypatch, lib) + + # Precondition: the writer really did rename the second duplicate. + conn = sqlite3.connect(str(m_db)) + titles = { + str(r[0]) for r in conn.execute("SELECT title FROM Playlist").fetchall() + } + conn.close() + assert {"Setlist", "Setlist (2)"} <= titles, titles + + result = verify_library(drive, with_artwork=False) + + assert result.ok is True, ( + "faithful build reported discrepancies: " + f"{[(d.field, d.expected, d.actual) for d in result.discrepancies]}" + ) + + +def test_verify_does_not_match_unrelated_suffixed_playlist( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A missing "House" must not silently resolve to "House (old)". + + WHY: the duplicate-suffix scan matches any title starting with "House (", + which is a real, differently-named playlist — not a rename of this one. The + absent list is then reported as a track mismatch against an unrelated set + instead of as missing, pointing the operator at the wrong playlist. + """ + from rb2engine.verify import verify_library + + playlists = [ + SourcePlaylist( + rb_id=1, parent_rb_id=0, name="House", sort_order=0, + is_folder=False, track_rb_ids=[10, 20], + ), + SourcePlaylist( + rb_id=2, parent_rb_id=0, name="House (old)", sort_order=1, + is_folder=False, track_rb_ids=[30], + ), + ] + drive, lib, m_db = _build_with_playlists(tmp_path, playlists) + _patch_read_library(monkeypatch, lib) + + # Drop the "House" list so its lookup genuinely fails. + conn = sqlite3.connect(str(m_db)) + conn.execute("PRAGMA foreign_keys = ON") + conn.execute("DELETE FROM Playlist WHERE title = 'House'") + conn.commit() + conn.close() + + result = verify_library(drive, with_artwork=False) + + assert result.ok is False + fields = _fields(result.discrepancies) + assert "playlist[House].missing" in fields, fields + # It must NOT have been diffed against "House (old)". + assert "playlist[House].track_order" not in fields, fields + + +def test_db_playlist_paths_survives_a_malformed_parent_chain() -> None: + """A cyclic or orphaned parent chain must not hang or crash verify. + + WHY: these paths are walked in a database rb2engine did not necessarily + write — Engine and the hardware also modify it. Verify's job on a corrupt + library is to report, so the walk has to terminate on structures the writer + would have refused to create. + """ + from rb2engine.verify import _db_playlist_paths + + conn = sqlite3.connect(":memory:") + conn.execute( + "CREATE TABLE Playlist (id INTEGER, title TEXT, parentListId INTEGER)" + ) + conn.executemany( + "INSERT INTO Playlist (id, title, parentListId) VALUES (?, ?, ?)", + [ + (1, "Root", 0), + (2, "Child", 1), + (3, "Orphan", 99), # parent does not exist + (4, "CycleA", 5), # 4 → 5 → 4 + (5, "CycleB", 4), + ], + ) + conn.commit() + + paths = _db_playlist_paths(conn) + conn.close() + + assert paths[("Root",)] == 1 + assert paths[("Root", "Child")] == 2 + # Orphan truncates at the missing parent rather than looping forever. + assert paths[("Orphan",)] == 3 + # Both cycle members terminate; each yields a finite path. + assert any(p[-1] == "CycleA" for p in paths) + assert any(p[-1] == "CycleB" for p in paths)