diff --git a/core/metadata/canonical_version.py b/core/metadata/canonical_version.py index ccc07451..9871d89e 100644 --- a/core/metadata/canonical_version.py +++ b/core/metadata/canonical_version.py @@ -207,4 +207,24 @@ def pick_canonical_release( return best, best_score -__all__ = ["score_release_against_files", "pick_canonical_release"] +# Album sources the canonical system reads (mirror of +# core.library_reorganize._ALBUM_ID_COLUMNS — a test pins them in sync). A manual +# match on any of these should pin/lock the canonical version (#758); a match on +# a source the canonical tools don't read (e.g. lastfm) has no version to pin. +CANONICAL_ALBUM_SOURCES = frozenset({'spotify', 'itunes', 'deezer', 'discogs', 'hydrabase'}) + + +def should_pin_manual_canonical(entity_type: str, source: str) -> bool: + """Whether a manual match should also pin (and lock) the canonical album + version (#758). True only for an ALBUM match on a canonical-recognised + source — so the user's chosen edition becomes the authority every downstream + tool (track-number repair, reorganize, missing-tracks) reads, and the auto + resolve job won't override it. + """ + return entity_type == 'album' and source in CANONICAL_ALBUM_SOURCES + + +__all__ = [ + "score_release_against_files", "pick_canonical_release", + "CANONICAL_ALBUM_SOURCES", "should_pin_manual_canonical", +] diff --git a/database/music_database.py b/database/music_database.py index 43935ead..71c7aa23 100644 --- a/database/music_database.py +++ b/database/music_database.py @@ -1019,6 +1019,10 @@ class MusicDatabase: 'canonical_album_id': 'TEXT DEFAULT NULL', 'canonical_score': 'REAL DEFAULT NULL', 'canonical_resolved_at': 'TIMESTAMP DEFAULT NULL', + # #758 — set when the user MANUALLY pins an album version. The + # auto resolve job (and any re-resolution) must never overwrite + # a locked pin, so a manual match stays put across cycles. + 'canonical_locked': 'INTEGER DEFAULT 0', } for _col, _typedef in _canonical_cols.items(): if album_cols and _col not in album_cols: @@ -1027,17 +1031,27 @@ class MusicDatabase: except Exception as e: logger.error("Error repairing core media schema columns: %s", e) - def set_album_canonical(self, album_id, source: str, canonical_album_id: str, score: float) -> bool: + def set_album_canonical(self, album_id, source: str, canonical_album_id: str, + score: float, locked: bool = False) -> bool: """Persist the resolved canonical (source, album_id, score) for an album - (#765 Stage 2). Returns True if a row was updated.""" + (#765 Stage 2). Returns True if a row was updated. + + ``locked=True`` marks a MANUAL pin (#758): the user explicitly chose this + album version. A manual write always wins (overwrites any existing pin). + An AUTO write (``locked=False``, the resolve job) will NOT overwrite a + locked pin — the guard is in the WHERE clause so it's atomic. + """ conn = self._get_connection() try: cursor = conn.cursor() + # Auto writes can't clobber a manual lock; manual writes always apply. + guard = "" if locked else " AND (canonical_locked IS NULL OR canonical_locked = 0)" cursor.execute( "UPDATE albums SET canonical_source = ?, canonical_album_id = ?, " - "canonical_score = ?, canonical_resolved_at = CURRENT_TIMESTAMP " - "WHERE id = ?", - (source, str(canonical_album_id), float(score), album_id), + "canonical_score = ?, canonical_locked = ?, " + "canonical_resolved_at = CURRENT_TIMESTAMP " + f"WHERE id = ?{guard}", + (source, str(canonical_album_id), float(score), 1 if locked else 0, album_id), ) conn.commit() return cursor.rowcount > 0 @@ -1048,15 +1062,16 @@ class MusicDatabase: conn.close() def get_album_canonical(self, album_id) -> Optional[dict]: - """Return ``{'source','album_id','score','resolved_at'}`` for an album's - pinned canonical release, or ``None`` when unresolved (#765 Stage 2). - Consumers treat ``None`` as 'fall back to today's behavior'.""" + """Return ``{'source','album_id','score','resolved_at','locked'}`` for an + album's pinned canonical release, or ``None`` when unresolved (#765 Stage + 2). ``locked`` is True for a manual pin (#758). Consumers treat ``None`` + as 'fall back to today's behavior'.""" conn = self._get_connection() try: cursor = conn.cursor() cursor.execute( "SELECT canonical_source, canonical_album_id, canonical_score, " - "canonical_resolved_at FROM albums WHERE id = ?", + "canonical_resolved_at, canonical_locked FROM albums WHERE id = ?", (album_id,), ) row = cursor.fetchone() @@ -1067,6 +1082,7 @@ class MusicDatabase: 'album_id': row[1], 'score': row[2], 'resolved_at': row[3], + 'locked': bool(row[4]), } except Exception as e: logger.error("Error reading album canonical for %s: %s", album_id, e) diff --git a/tests/test_canonical_manual_lock.py b/tests/test_canonical_manual_lock.py new file mode 100644 index 00000000..ad9fd99e --- /dev/null +++ b/tests/test_canonical_manual_lock.py @@ -0,0 +1,99 @@ +"""#758 — a manual album match pins (and LOCKS) the canonical album version, so +re-resolution / the auto canonical job can't drag it back to the deluxe edition. + +Two seams: + - should_pin_manual_canonical (pure): when a manual match should pin canonical. + - set_album_canonical / get_album_canonical (DB): the lock can't be overwritten + by an auto write, but a new manual write still wins. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from core.metadata.canonical_version import ( + CANONICAL_ALBUM_SOURCES, + should_pin_manual_canonical, +) +from database.music_database import MusicDatabase + + +# --------------------------------------------------------------------------- +# should_pin_manual_canonical — pure +# --------------------------------------------------------------------------- + +@pytest.mark.parametrize('source', ['spotify', 'itunes', 'deezer', 'discogs', 'hydrabase']) +def test_pins_album_on_recognised_source(source): + assert should_pin_manual_canonical('album', source) is True + + +@pytest.mark.parametrize('entity', ['artist', 'track']) +def test_does_not_pin_non_album(entity): + assert should_pin_manual_canonical(entity, 'spotify') is False + + +@pytest.mark.parametrize('source', ['lastfm', 'genius', 'musicbrainz', 'audiodb', 'tidal']) +def test_does_not_pin_source_canonical_cant_read(source): + # No album-version data the canonical tools read → nothing to pin. + assert should_pin_manual_canonical('album', source) is False + + +def test_sources_stay_in_sync_with_album_id_columns(): + # The set must mirror the canonical reader's column map; if a source is + # added there, this fails until CANONICAL_ALBUM_SOURCES is updated. + from core.library_reorganize import _ALBUM_ID_COLUMNS + assert CANONICAL_ALBUM_SOURCES == set(_ALBUM_ID_COLUMNS) + + +# --------------------------------------------------------------------------- +# set_album_canonical / get_album_canonical — the lock (DB) +# --------------------------------------------------------------------------- + +def _insert_album(db, album_id): + conn = db._get_connection() + conn.execute("INSERT OR IGNORE INTO artists (id, name) VALUES ('ar1', 'Artist')") + conn.execute("INSERT INTO albums (id, artist_id, title) VALUES (?, 'ar1', 'Album')", (album_id,)) + conn.commit() + conn.close() + + +@pytest.fixture +def db(tmp_path: Path) -> MusicDatabase: + d = MusicDatabase(database_path=str(tmp_path / "ml.db")) + _insert_album(d, 'al1') + return d + + +def test_manual_lock_set_and_read(db): + assert db.set_album_canonical('al1', 'spotify', 'REG', 1.0, locked=True) is True + c = db.get_album_canonical('al1') + assert c['source'] == 'spotify' and c['album_id'] == 'REG' and c['locked'] is True + + +def test_auto_cannot_overwrite_manual_lock(db): + db.set_album_canonical('al1', 'spotify', 'REG', 1.0, locked=True) + # The auto resolve job tries to re-pin the deluxe — must be refused. + assert db.set_album_canonical('al1', 'spotify', 'DELUXE', 0.9, locked=False) is False + c = db.get_album_canonical('al1') + assert c['album_id'] == 'REG' and c['locked'] is True # unchanged + + +def test_new_manual_match_overrides_existing_pin(db): + db.set_album_canonical('al1', 'spotify', 'OLD', 0.8, locked=False) # auto pin + # User manually picks a different edition — manual always wins. + assert db.set_album_canonical('al1', 'itunes', 'NEW', 1.0, locked=True) is True + c = db.get_album_canonical('al1') + assert c['source'] == 'itunes' and c['album_id'] == 'NEW' and c['locked'] is True + + +def test_auto_overwrites_auto(db): + db.set_album_canonical('al1', 'spotify', 'A', 0.8, locked=False) + assert db.set_album_canonical('al1', 'spotify', 'B', 0.9, locked=False) is True + assert db.get_album_canonical('al1')['album_id'] == 'B' + + +def test_unresolved_album_returns_none(db): + _insert_album(db, 'al2') + assert db.get_album_canonical('al2') is None diff --git a/web_server.py b/web_server.py index 05a40160..f8587878 100644 --- a/web_server.py +++ b/web_server.py @@ -11316,6 +11316,18 @@ def library_manual_match(): if cursor.rowcount == 0: return jsonify({"success": False, "error": "Entity not found"}), 404 + # #758 — a manual ALBUM match also pins (and LOCKS) the canonical album + # version to the chosen release, so the auto resolve job and every tool + # that reads the canonical pin (track-number repair, reorganize, + # missing-tracks) honor the user's edition instead of re-resolving back + # to the deluxe. The lock survives future enrichment/resolution cycles. + try: + from core.metadata.canonical_version import should_pin_manual_canonical + if should_pin_manual_canonical(entity_type, service): + database.set_album_canonical(entity_id, service, service_id, 1.0, locked=True) + except Exception as e: + logger.warning("Manual canonical pin failed for album %s: %s", entity_id, e) + # Re-fetch fresh data artist_id = data.get('artist_id', entity_id) if entity_type != 'artist':