From d1c8899487cef874860393a46a4f8460bff98280 Mon Sep 17 00:00:00 2001 From: ChrisBeWithYou Date: Sat, 4 Jul 2026 16:08:10 -0500 Subject: [PATCH] =?UTF-8?q?feat(library):=20metadata=20override=20+=20lock?= =?UTF-8?q?=20store,=20enforced=20by=20enrichment=20(popup=20slices=201?= =?UTF-8?q?=E2=80=932)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Backend foundation for the Fix-metadata popup. Not yet surfaced in the UI (the display + 3-tab popup are the next slices); no PR until it's user-visible. Slice 1 — the store: - `song_field_override(filename, field, value, locked)` table: a reversible DISPLAY overlay (never written to the pack), filename-keyed so it survives a rescan (never purged by delete_missing) and is dropped only with the song. - DB methods (partial upsert that drops empty+unlocked rows; batch map) + `GET`/`PUT /api/song/{fn}/overrides` (field allowlist title/artist/album/ year/genre; clearing rides PUT since DELETE /api/song/{path} shadows sub- routes; PUT demo-blocked). Slice 2 — locks respected by enrichment: - The auto-matcher composes a per-song `_compose_lock_filter` onto the global apply-filter, so a match still applies IDENTITY (mbid/release → art) but never re-canonicalizes a LOCKED display field. - Gap-fill (write-to-file) skips locked album/year/genre — writing the matched value would be exactly the clobber the lock exists to prevent. - Review/manual picks bypass the filter (an explicit confirm overrides a lock). Tests: store semantics + rescan-survival + API; the lock filter + reader; an auto-match leaving a locked field un-canonicalized; gap-fill excluding locked keys. Co-Authored-By: Claude Opus 4.8 (1M context) --- server.py | 191 +++++++++++++++++++++++++++++++++- tests/test_field_overrides.py | 148 ++++++++++++++++++++++++++ tests/test_gap_fill.py | 13 +++ tests/test_mb_enrichment.py | 16 +++ 4 files changed, 365 insertions(+), 3 deletions(-) create mode 100644 tests/test_field_overrides.py diff --git a/server.py b/server.py index c511ee6..2250354 100644 --- a/server.py +++ b/server.py @@ -199,6 +199,7 @@ _DEMO_BLOCKED: list[tuple[str, re.Pattern]] = [ ("DELETE", re.compile(r"^/api/audio-effects/active-mapping$")), ("POST", re.compile(r"^/api/song/.*/meta$")), ("POST", re.compile(r"^/api/song/.*/art/upload$")), + ("PUT", re.compile(r"^/api/song/.+/overrides$")), ("GET", re.compile(r"^/api/plugins/updates$")), ("POST", re.compile(r"^/api/plugins/[^/]+/update$")), ("POST", re.compile(r"^/api/plugins/editor/save$")), @@ -667,6 +668,26 @@ class MetadataDB: ) """) self.conn.execute("CREATE INDEX IF NOT EXISTS idx_song_tags_tag ON song_tags(tag COLLATE NOCASE)") + # Per-field metadata OVERRIDES + LOCKS (the Fix-metadata popup). A + # reversible DISPLAY overlay, never written to the pack: `value` is the + # user's corrected value for a catalog field (title/artist/album/year/ + # genre), `locked=1` pins the field so a metadata refresh / auto-match + # never changes what's shown for it (Plex-style field lock). Effective + # display value = override → matched-MusicBrainz → pack → derived. + # Filename-keyed → purged with the song on delete_song, NEVER on a + # rescan (delete_missing), so an edit survives re-import like every other + # local layer. + self.conn.execute(""" + CREATE TABLE IF NOT EXISTS song_field_override ( + filename TEXT NOT NULL, + field TEXT NOT NULL, -- title|artist|album|year|genre + value TEXT, -- corrected value (NULL = lock only, no override) + locked INTEGER NOT NULL DEFAULT 0, + updated_at TEXT, + PRIMARY KEY (filename, field) + ) + """) + self.conn.execute("CREATE INDEX IF NOT EXISTS idx_field_override_fn ON song_field_override(filename)") # Artist-name aliases (P4): "ACDC" → "AC/DC", "the beatles" → "The Beatles". # A CANONICALIZATION OVERRIDE applied AT DISPLAY only — the scanner-derived # `songs.artist` and the feedpak files are never rewritten (a rescan can't @@ -1168,6 +1189,77 @@ class MetadataDB: self.conn.commit() return self.get_song_user_meta(filename) + # ── Per-field metadata overrides + locks (Fix-metadata popup) ───────────── + def get_song_overrides(self, filename: str) -> dict: + """{field: {"value": str|None, "locked": bool}} for one song.""" + rows = self.conn.execute( + "SELECT field, value, locked FROM song_field_override WHERE filename = ?", + (filename,)).fetchall() + return {r[0]: {"value": r[1], "locked": bool(r[2])} for r in rows} + + def set_song_override(self, filename: str, field: str, *, + value="__keep__", locked="__keep__") -> dict: + """Partial upsert of one field's override value and/or lock. Pass a + value/locked to set it or leave the sentinel to keep the current one. A + row with neither a value nor a lock is dropped (no empty shell). Returns + the song's full override map.""" + with self._lock: + cur = self.conn.execute( + "SELECT value, locked FROM song_field_override WHERE filename = ? AND field = ?", + (filename, field)).fetchone() + new_val = (cur[0] if cur else None) if value == "__keep__" else value + new_lock = (bool(cur[1]) if cur else False) if locked == "__keep__" else bool(locked) + new_val = (new_val or "").strip() or None + if new_val is None and not new_lock: + self.conn.execute( + "DELETE FROM song_field_override WHERE filename = ? AND field = ?", + (filename, field)) + else: + self.conn.execute( + "INSERT INTO song_field_override (filename, field, value, locked, updated_at) " + "VALUES (?, ?, ?, ?, datetime('now')) " + "ON CONFLICT(filename, field) DO UPDATE SET " + "value = excluded.value, locked = excluded.locked, updated_at = excluded.updated_at", + (filename, field, new_val, 1 if new_lock else 0)) + self.conn.commit() + return self.get_song_overrides(filename) + + def locked_fields(self, filename: str) -> set: + """The catalog fields the user LOCKED for a song (Fix-metadata popup). + An automatic match must never (re)canonicalize these, and gap-fill must + never write them to the file. Locked read (the enrichment worker calls + it), minimal projection.""" + with self._lock: + return {r[0] for r in self.conn.execute( + "SELECT field FROM song_field_override WHERE filename = ? AND locked = 1", + (filename,)).fetchall()} + + def clear_song_override(self, filename: str, field: str) -> dict: + """Remove a field's override + lock entirely (revert to the resolved + pack/matched value).""" + with self._lock: + self.conn.execute( + "DELETE FROM song_field_override WHERE filename = ? AND field = ?", + (filename, field)) + self.conn.commit() + return self.get_song_overrides(filename) + + def overrides_map(self, filenames) -> dict: + """{filename: {field: {value, locked}}} for a batch — feeds the grid's + effective-value resolution (display slice). Chunked under SQLite's + variable limit.""" + fns = list(filenames) + out: dict = {} + for i in range(0, len(fns), 400): + chunk = fns[i:i + 400] + if not chunk: + break + q = ("SELECT filename, field, value, locked FROM song_field_override " + "WHERE filename IN (%s)" % ",".join("?" * len(chunk))) + for fn, field, value, locked in self.conn.execute(q, chunk).fetchall(): + out.setdefault(fn, {})[field] = {"value": value, "locked": bool(locked)} + return out + def set_song_tags(self, filename: str, tags) -> list: """Replace ALL of a song's tags with the given set (each normalized; blanks + case-dupes dropped). Full-replace so the whole personal-meta @@ -1232,6 +1324,7 @@ class MetadataDB: INSIDE the caller's `meta_db._lock` — must not re-acquire the lock.""" self.conn.execute("DELETE FROM song_user_meta WHERE filename = ?", (filename,)) self.conn.execute("DELETE FROM song_tags WHERE filename = ?", (filename,)) + self.conn.execute("DELETE FROM song_field_override WHERE filename = ?", (filename,)) def batch_user_meta(self, filenames, *, set_difficulty="__keep__", add_tags=None, remove_tags=None) -> int: @@ -6986,6 +7079,34 @@ def _artist_title_from_filename(filename: str) -> dict | None: return {"artist": artist, "title": title} +# A per-song LOCK (Fix-metadata popup) → the candidate display keys it +# suppresses on an AUTOMATIC match. Identity keys (recording/release/artist ids, +# isrc) are deliberately absent: a locked DISPLAY field still gets matched for +# art + future re-match, it just isn't re-canonicalized behind the user's back. +_LOCK_FIELD_TO_CAND = { + "artist": ("artist", "artist_sort"), + "title": ("title",), + "album": ("album",), + "year": ("year",), + "genre": ("genres",), +} + + +def _compose_lock_filter(base_filter, locked_fields): + """Wrap the pass's global per-field apply-filter with a per-song filter that + also strips the song's LOCKED display fields, so an automatic match never + re-canonicalizes a field the user pinned. Returns base_filter unchanged when + the song has no relevant lock (the common path).""" + blocked = {ck for f in locked_fields for ck in _LOCK_FIELD_TO_CAND.get(f, ())} + if not blocked: + return base_filter + + def lock_filter(cand): + c = base_filter(cand) if base_filter else cand + return {k: v for k, v in c.items() if k not in blocked} + return lock_filter + + def _enrich_one(row: dict, auto_min: float | None = None, field_filter=None, apply_mask: str = "") -> None: """The matcher (P8; replaces P7's no-op). Precedence per design §5: @@ -7011,6 +7132,14 @@ def _enrich_one(row: dict, auto_min: float | None = None, field_filter=None, siblings. Network errors raise EnrichTransportError so the pass pauses instead of burning attempts while offline.""" fn, chash = row["filename"], row["content_hash"] + # Respect per-song field LOCKS (Fix-metadata popup): an automatic match must + # not re-canonicalize a field the user pinned. Compose the lock filter onto + # the pass's global apply-filter — both the cache-copy and text-match auto + # paths run their candidate through it. (Review/manual picks bypass the + # filter, so confirming a match in the modal is an explicit override.) + locked = meta_db.locked_fields(fn) + if locked: + field_filter = _compose_lock_filter(field_filter, locked) cached = meta_db.enrichment_cache_lookup(chash, exclude_filename=fn) if cached: @@ -9034,6 +9163,56 @@ def put_song_user_meta(filename: str, data: dict): return meta_db.get_song_user_meta(key) +# Catalog fields the Fix-metadata popup may override/lock — the intersection of +# "displayable identity" and "safe to correct locally". Guitar/practice facts +# and personal fields are never overrides. +_OVERRIDE_FIELDS = frozenset({"title", "artist", "album", "year", "genre"}) + + +@app.get("/api/song/{filename:path}/overrides") +def get_song_overrides(filename: str): + """Per-field metadata overrides + locks for one song (Fix-metadata popup): + {"overrides": {field: {"value": str|null, "locked": bool}}}.""" + return {"overrides": meta_db.get_song_overrides( + meta_db._canonical_song_filename(filename))} + + +@app.put("/api/song/{filename:path}/overrides") +def put_song_overrides(filename: str, data: dict): + """Set/clear per-field overrides + locks. Body: + `{"overrides": {field: {"value": str|null, "locked": bool}}}`. Only catalog + fields (title/artist/album/year/genre) are accepted. A field left with no + value and unlocked is removed. Returns the merged override map. + + Clearing rides this PUT (send value:null, locked:false) rather than a DELETE + sub-route, because `DELETE /api/song/{filename:path}` already owns every + DELETE under /api/song and would shadow it (same reason as tags).""" + ov = (data or {}).get("overrides") + if not isinstance(ov, dict) or not ov: + return JSONResponse({"error": "overrides must be a non-empty object"}, 400) + bad = sorted(f for f in ov if f not in _OVERRIDE_FIELDS) + if bad: + return JSONResponse({"error": "unknown field(s): " + ", ".join(bad)}, 400) + key = meta_db._canonical_song_filename(filename) + for field, spec in ov.items(): + if not isinstance(spec, dict): + return JSONResponse({"error": f"'{field}' must be an object with value/locked"}, 400) + kwargs: dict = {} + if "value" in spec: + v = spec["value"] + if v is None: + kwargs["value"] = None + elif isinstance(v, (str, int, float)) and not isinstance(v, bool): + kwargs["value"] = str(v).strip()[:500] + else: + return JSONResponse({"error": f"'{field}' value must be a string or null"}, 400) + if "locked" in spec: + kwargs["locked"] = bool(spec["locked"]) + if kwargs: + meta_db.set_song_override(key, field, **kwargs) + return {"overrides": meta_db.get_song_overrides(key)} + + @app.post("/api/songs/user-meta/batch") def batch_song_user_meta(data: dict): """Bulk personal-meta edit over a selection — one request instead of N×2 @@ -12202,15 +12381,21 @@ def _gap_fill_proposals(cache_key: str, resolved) -> tuple[dict, str]: manifest = sloppak_mod.load_manifest(resolved) or {} except Exception: return {}, "not-sloppak" + # A LOCKED field (Fix-metadata popup) is never gap-filled — the user pinned + # it away from the matched value, so writing that value to the file would + # be exactly the clobber the lock exists to prevent. (The lock field name is + # `genre`; the manifest/gap-fill key is `genres`.) + locked = meta_db.locked_fields(cache_key) out = {} album = (row.get("canon_album") or "").strip() - if album and _gap_fill_manifest_absent(manifest, "album"): + if album and "album" not in locked and _gap_fill_manifest_absent(manifest, "album"): out["album"] = album year = (row.get("canon_year") or "").strip() - if year.isdigit() and int(year) and _gap_fill_manifest_absent(manifest, "year"): + if (year.isdigit() and int(year) and "year" not in locked + and _gap_fill_manifest_absent(manifest, "year")): out["year"] = int(year) genres = [str(g) for g in (row.get("genres") or []) if isinstance(g, str) and g.strip()] - if genres and _gap_fill_manifest_absent(manifest, "genres"): + if genres and "genre" not in locked and _gap_fill_manifest_absent(manifest, "genres"): out["genres"] = genres # Identity keys (feedpak spec 1.14.0) — written in canonical form only. mbid = (row.get("mb_recording_id") or "").strip().lower() diff --git a/tests/test_field_overrides.py b/tests/test_field_overrides.py new file mode 100644 index 0000000..dfeda73 --- /dev/null +++ b/tests/test_field_overrides.py @@ -0,0 +1,148 @@ +"""Tests for the per-field metadata override + lock store (Fix-metadata popup). + +A reversible DISPLAY overlay, never written to the pack: filename-keyed, so it +survives a rescan (never purged by delete_missing) and is dropped only with the +song (delete_song). Locks pin a field against a later auto-match. +""" + +import importlib +import sys + +import pytest +from fastapi.testclient import TestClient + + +@pytest.fixture() +def server(tmp_path, monkeypatch, isolate_logging): + monkeypatch.setenv("CONFIG_DIR", str(tmp_path)) + monkeypatch.setenv("FEEDBACK_SKIP_STARTUP_TASKS", "1") + sys.modules.pop("server", None) + srv = importlib.import_module("server") + try: + yield srv + finally: + conn = getattr(getattr(srv, "meta_db", None), "conn", None) + if conn is not None: + getattr(sys.modules.get("server"), "_join_background_db_threads", lambda: None)() + conn.close() + sys.modules.pop("server", None) + + +@pytest.fixture() +def client(server): + return TestClient(server.app) + + +def _put(server, fn, **meta): + base = {"title": "Song", "artist": "Artist", "album": "", "duration": 100, + "arrangements": [{"name": "Lead", "index": 0}]} + base.update(meta) + server.meta_db.put(fn, 0, 0, base) + + +# ── store semantics ─────────────────────────────────────────────────────────── + +def test_set_get_and_partial_upsert(server): + db = server.meta_db + assert db.get_song_overrides("a.archive") == {} + db.set_song_override("a.archive", "artist", value="AC/DC") + assert db.get_song_overrides("a.archive") == {"artist": {"value": "AC/DC", "locked": False}} + # partial: lock without touching the value + db.set_song_override("a.archive", "artist", locked=True) + assert db.get_song_overrides("a.archive")["artist"] == {"value": "AC/DC", "locked": True} + # partial: change the value, keep the lock + db.set_song_override("a.archive", "artist", value="AC/DC (fixed)") + assert db.get_song_overrides("a.archive")["artist"] == {"value": "AC/DC (fixed)", "locked": True} + + +def test_lock_only_row_persists_without_a_value(server): + db = server.meta_db + db.set_song_override("a.archive", "year", locked=True) + # a pure lock (no override value) is a valid, kept row + assert db.get_song_overrides("a.archive") == {"year": {"value": None, "locked": True}} + + +def test_empty_and_unlocked_drops_the_row(server): + db = server.meta_db + db.set_song_override("a.archive", "album", value="X", locked=True) + db.set_song_override("a.archive", "album", value="", locked=False) + assert db.get_song_overrides("a.archive") == {} # no empty shell + + +def test_clear_one_field_leaves_others(server): + db = server.meta_db + db.set_song_override("a.archive", "title", value="T") + db.set_song_override("a.archive", "artist", value="A") + db.clear_song_override("a.archive", "title") + assert set(db.get_song_overrides("a.archive")) == {"artist"} + + +# ── lifecycle: rescan survival vs explicit delete ───────────────────────────── + +def test_rescan_never_purges_overrides_delete_does(server): + _put(server, "a.archive") + server.meta_db.set_song_override("a.archive", "artist", value="AC/DC", locked=True) + server.meta_db.delete_missing(set()) # file vanished from a scan + assert server.meta_db.get_song_overrides("a.archive")["artist"]["value"] == "AC/DC" + server.meta_db.purge_song_user_data("a.archive") # the delete_song purge + assert server.meta_db.get_song_overrides("a.archive") == {} + + +def test_overrides_map_batches(server): + db = server.meta_db + db.set_song_override("a.archive", "artist", value="A") + db.set_song_override("b.archive", "title", value="B", locked=True) + m = db.overrides_map(["a.archive", "b.archive", "missing.archive"]) + assert m["a.archive"]["artist"]["value"] == "A" + assert m["b.archive"]["title"] == {"value": "B", "locked": True} + assert "missing.archive" not in m + assert db.overrides_map([]) == {} + + +# ── API ─────────────────────────────────────────────────────────────────────── + +def test_api_put_get_and_clear(client, server): + _put(server, "a.archive") + r = client.put("/api/song/a.archive/overrides", + json={"overrides": {"artist": {"value": "AC/DC", "locked": True}, + "year": {"value": "1979"}}}) + assert r.status_code == 200 + ov = r.json()["overrides"] + assert ov["artist"] == {"value": "AC/DC", "locked": True} + assert ov["year"] == {"value": "1979", "locked": False} + assert client.get("/api/song/a.archive/overrides").json()["overrides"]["artist"]["value"] == "AC/DC" + # clear via PUT (value null + unlocked) — DELETE is shadowed by /api/song/{path} + client.put("/api/song/a.archive/overrides", + json={"overrides": {"artist": {"value": None, "locked": False}}}) + assert "artist" not in client.get("/api/song/a.archive/overrides").json()["overrides"] + + +def test_api_rejects_unknown_field(client, server): + _put(server, "a.archive") + r = client.put("/api/song/a.archive/overrides", + json={"overrides": {"tuning": {"value": "Drop D"}}}) + assert r.status_code == 400 + assert "unknown field" in r.json()["error"] + + +# ── lock enforcement (slice 2) ──────────────────────────────────────────────── + +def test_locked_fields_reader(server): + db = server.meta_db + db.set_song_override("a.archive", "artist", value="X", locked=True) + db.set_song_override("a.archive", "title", value="Y") # override, not locked + db.set_song_override("a.archive", "year", locked=True) # lock only + assert db.locked_fields("a.archive") == {"artist", "year"} + + +def test_compose_lock_filter_strips_locked_cand_keys(server): + f = server._compose_lock_filter(None, {"artist", "year"}) + cand = {"recording_id": "r", "artist": "X", "artist_sort": "X", "title": "T", + "year": "1990", "album": "A", "genres": ["rock"]} + out = f(cand) + # locked display keys stripped (artist maps to artist + artist_sort)… + assert not ({"artist", "artist_sort", "year"} & set(out)) + # …identity + unlocked display fields survive + assert out["recording_id"] == "r" and out["title"] == "T" and out["album"] == "A" + # no locks → base filter returned unchanged (zero-copy common path) + assert server._compose_lock_filter(None, set()) is None diff --git a/tests/test_gap_fill.py b/tests/test_gap_fill.py index a870cf4..dc9cbbd 100644 --- a/tests/test_gap_fill.py +++ b/tests/test_gap_fill.py @@ -110,6 +110,19 @@ def test_preview_excludes_author_set_keys(server, client): assert {"genres", "mbid", "isrc"} <= got +def test_preview_excludes_locked_fields(server, client): + """A field LOCKED in the Fix-metadata popup is never gap-filled — writing + the matched value would be exactly the clobber the lock prevents — even + though the match has a value and the manifest lacks it.""" + make_dir_sloppak(server, "a.sloppak") + seed_match(server, "a.sloppak") + server.meta_db.set_song_override("a.sloppak", "album", locked=True) + server.meta_db.set_song_override("a.sloppak", "year", locked=True) + got = {m["key"] for m in client.get("/api/song/a.sloppak/gap-fill").json()["missing"]} + assert "album" not in got and "year" not in got + assert {"genres", "mbid", "isrc"} <= got # unlocked keys still offered + + def test_preview_excludes_present_but_empty_keys(server, client): """Gap-fill is append-only, so a present-but-empty value (album: '', year: 0) is NOT a gap the writer can fill — appending would duplicate the diff --git a/tests/test_mb_enrichment.py b/tests/test_mb_enrichment.py index 43c42df..d0937ca 100644 --- a/tests/test_mb_enrichment.py +++ b/tests/test_mb_enrichment.py @@ -185,6 +185,22 @@ def test_enrich_auto_matches_japanese_primary_via_alias(server, monkeypatch): assert row["mb_recording_id"] == "rec-jp" +# ── per-song field locks respected by the auto-matcher ─────────────────────── + +def test_locked_field_not_canonicalized_by_auto_match(server, monkeypatch): + _put(server, "x.sloppak") # title "Thunderstruck (v2)", artist "ACDC" + server.meta_db.set_song_override("x.sloppak", "artist", locked=True) + monkeypatch.setattr(server, "_mb_http_get", + lambda path, params: {"recordings": [mb_doc()]}) + monkeypatch.setattr(server, "_enrich_network_enabled", lambda: True) + server._background_enrich() + row = server.meta_db.get_enrichment("x.sloppak") + assert row["match_state"] == "matched" # still matches (identity applies)… + assert row["canon_artist"] is None # …but the LOCKED artist isn't canonicalized + assert row["canon_title"] == "Thunderstruck" # unlocked display fields still apply + assert row["mb_recording_id"] # identity keys still stored (art needs them) + + # ── offline safety (the pytest-never-hits-network contract) ────────────────── def test_offline_default_skips_matching(server, monkeypatch):