From 4039ec3573c33ad8374644224c701ea2cad7bd3e Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Fri, 5 Jun 2026 20:07:03 -0700 Subject: [PATCH] #800: Write Tags must not overwrite a real file value with a placeholder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A mis-grouped library track sits under a 'Various Artists' / '[Unknown Album]' record while the file itself is correctly tagged. Write Tags reads the DB and stamped that junk over the file — destroying the correct tags. (Rematching never helped: a match only stores a source-ID pointer, it never changes the local name/title the writer reads.) Guard: never replace a real file value with a placeholder. Added at both seams so preview and write agree: - build_tag_diff marks such a field protected/no-change (the preview no longer shows a wrong overwrite, and has_changes reflects reality), - write_tags_to_file reads the file's current values and skips the placeholder-over-real fields, preserving the file. Field-agnostic and direction-safe: the guard fires ONLY when the DB value is a placeholder AND the file holds a real one. A legitimate value still writes — including a genuine 'Various Artists' album artist on a real compilation, where the file has no conflicting real value, so the guard doesn't fire. Every write_tags_to_file caller writes DB->file as a correction, so blocking placeholder-over-real is correct for all of them (the download post-process uses a different path, embed_source_ids). 23 new tests (placeholder detection, the guard fn, build_tag_diff on the screenshot-#2 scenario, end-to-end FLAC write preserving real values and still overwriting with real ones). 113 existing repair/retag/tag tests pass. --- core/tag_writer.py | 68 +++++++++- tests/test_tag_writer_placeholder_guard.py | 143 +++++++++++++++++++++ 2 files changed, 210 insertions(+), 1 deletion(-) create mode 100644 tests/test_tag_writer_placeholder_guard.py diff --git a/core/tag_writer.py b/core/tag_writer.py index 02aa8d1e..707454ac 100644 --- a/core/tag_writer.py +++ b/core/tag_writer.py @@ -136,10 +136,49 @@ def read_file_tags(file_path: str) -> Dict[str, Any]: return result +# Known placeholder / "we don't really know" metadata values. A match in the +# DB never warrants overwriting a real value already in the file (issue #800: +# a mis-grouped track sits under a "Various Artists" / "[Unknown Album]" record, +# and Write Tags would otherwise stamp that junk over the file's correct tags). +_PLACEHOLDER_META_VALUES = frozenset({ + 'various artists', 'various artist', + 'unknown artist', 'unknown album', 'unknown', + '[unknown album]', '[unknown artist]', '[unknown]', + 'untitled album', +}) + + +def is_placeholder_meta(value: Any) -> bool: + """True for empty or known-placeholder metadata strings (case-insensitive).""" + if value is None: + return True + s = str(value).strip().lower() + return s == '' or s in _PLACEHOLDER_META_VALUES + + +def guard_placeholder_overwrite(db_val: Any, file_val: Any) -> Any: + """#800 guard: never replace a real file value with a placeholder. + + Returns ``None`` (skip the write → preserve the file's value) ONLY when the + DB value is a placeholder/empty AND the file already holds a real, + non-placeholder value. Otherwise returns ``db_val`` unchanged — so a + legitimate value still writes, including a genuine ``Various Artists`` album + artist on a real compilation (there the file has no conflicting real value, + so the guard doesn't fire). + """ + if is_placeholder_meta(db_val) and not is_placeholder_meta(file_val): + return None + return db_val + + def build_tag_diff(file_tags: Dict[str, Any], db_data: Dict[str, Any]) -> List[Dict[str, Any]]: """ Compare file tags against DB metadata. Returns a list of diffs: - [{ field, file_value, db_value, changed }] + [{ field, file_value, db_value, changed, protected }] + + ``protected`` marks a field the #800 guard is holding back: the DB value is + a placeholder and the file's real value would be preserved, so it's shown + as no-change rather than a wrong overwrite. """ fields = [ ('title', 'title', 'Title'), @@ -179,12 +218,22 @@ def build_tag_diff(file_tags: Dict[str, Any], db_data: Dict[str, Any]) -> List[D # Only mark as changed if DB has a value AND it differs from file # (writer skips fields where DB value is empty, so don't show them as diffs) changed = bool(db_str) and file_str != db_str + + # #800 — if the change would replace a real file value with a + # placeholder (Various Artists / [Unknown Album] / …), hold it back: + # the writer preserves the file's value, so show it as no-change. + protected = False + if changed and guard_placeholder_overwrite(db_val, file_val) is None: + changed = False + protected = True + diffs.append({ 'field': label, 'file_key': file_key, 'file_value': str(file_val) if file_val is not None else '', 'db_value': str(db_val) if db_val is not None else '', 'changed': changed, + 'protected': protected, }) # Cover art — special row @@ -253,6 +302,23 @@ def write_tags_to_file(file_path: str, db_data: Dict[str, Any], artist = db_data.get('track_artist') or db_data.get('artist_name') # Per-track artist for compilations/DJ mixes album = db_data.get('album_title') album_artist = db_data.get('artist_name') # Album artist stays as the album-level artist + + # #800 — never overwrite a real value already in the file with a + # placeholder DB value (Various Artists / [Unknown Album] / …). A + # mis-grouped track sits under such a record; writing it would destroy + # the file's correct tags. Reads the file's current values and skips + # only the placeholder-over-real fields (legit values, incl. a genuine + # compilation's Various Artists, still write — see guard docstring). + try: + _current = read_file_tags(file_path) + except Exception: + _current = {} + if not _current.get('error'): + title = guard_placeholder_overwrite(title, _current.get('title')) + artist = guard_placeholder_overwrite(artist, _current.get('artist')) + album = guard_placeholder_overwrite(album, _current.get('album')) + album_artist = guard_placeholder_overwrite(album_artist, _current.get('album_artist')) + year = db_data.get('year') genres = db_data.get('genres') track_num = db_data.get('track_number') diff --git a/tests/test_tag_writer_placeholder_guard.py b/tests/test_tag_writer_placeholder_guard.py new file mode 100644 index 00000000..d5b63ad6 --- /dev/null +++ b/tests/test_tag_writer_placeholder_guard.py @@ -0,0 +1,143 @@ +"""#800 — Write Tags must never overwrite a real file value with a placeholder. + +A mis-grouped track sits under a 'Various Artists' / '[Unknown Album]' record, +while the file itself is correctly tagged. Previously Write Tags stamped that +junk over the good file (data loss). These pin the guard at both seams: the +preview (build_tag_diff) and the actual write (write_tags_to_file). +""" + +from __future__ import annotations + +import os +import tempfile + +import pytest +from mutagen.flac import FLAC + +from core.tag_writer import ( + build_tag_diff, + guard_placeholder_overwrite, + is_placeholder_meta, + write_tags_to_file, +) + + +# --------------------------------------------------------------------------- +# pure helpers +# --------------------------------------------------------------------------- + +@pytest.mark.parametrize('val', [ + None, '', ' ', 'Various Artists', 'various artists', 'VARIOUS ARTIST', + 'Unknown Artist', 'Unknown Album', '[Unknown Album]', 'Unknown', 'Untitled Album', +]) +def test_placeholder_values_detected(val): + assert is_placeholder_meta(val) is True + + +@pytest.mark.parametrize('val', ['OneRepublic', 'Native (Deluxe)', 'Counting Stars', 'VA Movement']) +def test_real_values_not_placeholder(val): + assert is_placeholder_meta(val) is False + + +def test_guard_blocks_placeholder_over_real(): + assert guard_placeholder_overwrite('Various Artists', 'OneRepublic') is None + assert guard_placeholder_overwrite('[Unknown Album]', 'Native (Deluxe)') is None + + +def test_guard_allows_real_over_anything(): + assert guard_placeholder_overwrite('OneRepublic', 'Old Wrong Name') == 'OneRepublic' + assert guard_placeholder_overwrite('OneRepublic', '') == 'OneRepublic' + + +def test_guard_allows_placeholder_when_file_has_no_real_value(): + # Legit compilation: file album-artist empty → 'Various Artists' still writes. + assert guard_placeholder_overwrite('Various Artists', '') == 'Various Artists' + assert guard_placeholder_overwrite('Various Artists', 'Various Artists') == 'Various Artists' + + +# --------------------------------------------------------------------------- +# build_tag_diff — the preview (screenshot #2 scenario) +# --------------------------------------------------------------------------- + +def test_diff_protects_placeholder_over_real(): + file_tags = {'title': 'Counting Stars', 'artist': 'OneRepublic', + 'album': 'Native (Deluxe)', 'album_artist': 'OneRepublic'} + db_data = {'title': 'Counting Stars', 'artist_name': 'Various Artists', + 'album_title': '[Unknown Album]'} + diff = {d['field']: d for d in build_tag_diff(file_tags, db_data)} + + assert diff['Title']['changed'] is False # already equal + for f in ('Artist', 'Album', 'Album Artist'): + assert diff[f]['changed'] is False, f # held back, not a wrong overwrite + assert diff[f]['protected'] is True, f + # Nothing real to write → no changes overall. + assert not any(d['changed'] for d in build_tag_diff(file_tags, db_data)) + + +def test_diff_real_db_value_still_changes(): + file_tags = {'artist': 'Old Wrong'} + db_data = {'artist_name': 'OneRepublic'} + diff = {d['field']: d for d in build_tag_diff(file_tags, db_data)} + assert diff['Artist']['changed'] is True + assert diff['Artist']['protected'] is False + + +def test_diff_compilation_va_writes_when_file_empty(): + file_tags = {'album_artist': ''} # no real value to protect + db_data = {'artist_name': 'Various Artists'} + diff = {d['field']: d for d in build_tag_diff(file_tags, db_data)} + assert diff['Album Artist']['changed'] is True + assert diff['Album Artist']['protected'] is False + + +# --------------------------------------------------------------------------- +# write_tags_to_file — end-to-end on a real FLAC +# --------------------------------------------------------------------------- + +def _make_minimal_flac(path): + minimal = ( + b'fLaC' + b'\x80\x00\x00\x22' + b'\x00\x10\x00\x10' + + b'\x00\x00\x00\x00\x00\x00' + b'\x0a\xc4\x42\xf0\x00\x00\x00\x00' + b'\x00' * 16 + ) + with open(path, 'wb') as f: + f.write(minimal) + + +@pytest.fixture +def flac_path(): + fd, path = tempfile.mkstemp(suffix='.flac') + os.close(fd) + _make_minimal_flac(path) + yield path + try: + os.remove(path) + except OSError: + pass + + +def test_write_preserves_real_value_against_placeholder(flac_path): + # 1) lay down correct tags (the file is correctly tagged) + write_tags_to_file(flac_path, { + 'title': 'Counting Stars', 'artist_name': 'OneRepublic', + 'album_title': 'Native (Deluxe)', + }, embed_cover=False) + + # 2) attempt to write the mis-grouped DB junk over it + result = write_tags_to_file(flac_path, { + 'title': 'Counting Stars', 'artist_name': 'Various Artists', + 'album_title': '[Unknown Album]', + }, embed_cover=False) + assert result['success'] is True + + audio = FLAC(flac_path) + assert audio.get('artist') == ['OneRepublic'] # preserved, not clobbered + assert audio.get('album') == ['Native (Deluxe)'] # preserved + assert audio.get('albumartist') == ['OneRepublic'] # preserved + + +def test_write_real_value_still_overwrites(flac_path): + write_tags_to_file(flac_path, {'artist_name': 'OneRepublic'}, embed_cover=False) + # A real (non-placeholder) new value must still overwrite normally. + result = write_tags_to_file(flac_path, {'artist_name': 'Coldplay'}, embed_cover=False) + assert result['success'] is True + assert FLAC(flac_path).get('artist') == ['Coldplay']