Track "01" bug: recover real track position instead of fabricating 1
Single tracks (esp. Deezer-sourced) imported as "01 - Title" regardless of their real album position — e.g. Fly Away (track 2 of Greatest Hits) landed as 01, littering album folders with duplicate "01" files. Root cause: a Deezer single track is matched via /search/track, which omits track_position, so the context never carried the real number; then service.py + context.py fabricated a confident track_number=1 from that gap. Because the resolver puts that first, the fake 1 beat the source. It is source-agnostic (slskd-with-Deezer-metadata hits it too) — albums work because /album/<id>/tracks DOES include positions. Fix (at the shared import funnel, strictly additive): - track_number.py: new read_embedded_track_number() (mutagen, local, no network) + an optional embedded_track_number arg on resolve_track_number. The downloaded file already carries the source-written position (deemix wrote it); consult it LAST — only when metadata AND the "NN - Title" filename both come up empty — so it can only fill the gap that would otherwise hit the default-1 floor. Never overrides a value the pre-fix resolver produced (no regression for correctly-named/mistagged files). - pipeline.py: read the file tag at the resolve step and pass it in. - De-poison: service.py:217 + context.py default to 0 (the existing "unknown" sentinel, like total_tracks), NOT 1 — so the fake 1 no longer blocks recovery. Frontend already treats falsy track_number as unknown (omits it), so this also drops the bogus "1." in the UI. 13 new resolver tests incl. the no-regression precedence guards; full imports + wishlist suites green (583), no behavior change for albums.
This commit is contained in:
parent
48e86a1a58
commit
d15b3a185d
5 changed files with 196 additions and 14 deletions
|
|
@ -319,7 +319,11 @@ def build_import_album_info(
|
||||||
(album_info or {}).get("track_number")
|
(album_info or {}).get("track_number")
|
||||||
or track_info.get("track_number")
|
or track_info.get("track_number")
|
||||||
or original_search.get("track_number")
|
or original_search.get("track_number")
|
||||||
or 1
|
# "Track 01" bug: default to 0 (the codebase's "unknown" sentinel,
|
||||||
|
# same as total_tracks below), NOT 1. A fabricated 1 looks
|
||||||
|
# authoritative and blocks the pipeline's downstream recovery
|
||||||
|
# (embedded file tag / resolve chain); 0 lets it fall through.
|
||||||
|
or 0
|
||||||
)
|
)
|
||||||
disc_number = (
|
disc_number = (
|
||||||
(album_info or {}).get("disc_number")
|
(album_info or {}).get("disc_number")
|
||||||
|
|
|
||||||
|
|
@ -629,9 +629,17 @@ def post_process_matched_download(context_key, context, file_path, runtime, meta
|
||||||
# See ``core/imports/track_number.py`` for the resolution
|
# See ``core/imports/track_number.py`` for the resolution
|
||||||
# chain — pure function, unit-tested in isolation, single
|
# chain — pure function, unit-tested in isolation, single
|
||||||
# place to fix the rule.
|
# place to fix the rule.
|
||||||
from core.imports.track_number import resolve_track_number
|
from core.imports.track_number import resolve_track_number, read_embedded_track_number
|
||||||
track_info_for_resolve = context.get('track_info') if isinstance(context, dict) else None
|
track_info_for_resolve = context.get('track_info') if isinstance(context, dict) else None
|
||||||
track_number = resolve_track_number(album_info, track_info_for_resolve, file_path)
|
# "Track 01" bug: a single Deezer track is matched via an endpoint
|
||||||
|
# that omits track_position, so the context never carried the real
|
||||||
|
# number. The downloaded file itself does (deemix/source wrote it),
|
||||||
|
# so read it as a source between metadata and the filename guess.
|
||||||
|
embedded_track_number = read_embedded_track_number(file_path)
|
||||||
|
track_number = resolve_track_number(
|
||||||
|
album_info, track_info_for_resolve, file_path,
|
||||||
|
embedded_track_number=embedded_track_number,
|
||||||
|
)
|
||||||
logger.debug(
|
logger.debug(
|
||||||
"Final track_number processing: source=%s album_info=%s resolved=%s",
|
"Final track_number processing: source=%s album_info=%s resolved=%s",
|
||||||
album_info.get('source', 'unknown'),
|
album_info.get('source', 'unknown'),
|
||||||
|
|
|
||||||
|
|
@ -65,17 +65,64 @@ def _coerce_spotify_data(track_info: Any) -> dict:
|
||||||
return {}
|
return {}
|
||||||
|
|
||||||
|
|
||||||
|
def read_embedded_track_number(file_path: str) -> Optional[int]:
|
||||||
|
"""Read the track position from a downloaded audio file's own tags.
|
||||||
|
|
||||||
|
Streaming sources (Deezer/deemix, Qobuz, Tidal) and most Soulseek
|
||||||
|
uploads write the correct album position into the file itself. That
|
||||||
|
tag is authoritative for the *source's* idea of the track's place on
|
||||||
|
its album — more reliable than a filename guess — so the resolver
|
||||||
|
consults it before falling back to the filename / default-1 floor.
|
||||||
|
|
||||||
|
Issue #874-adjacent / "Track 01" bug: a single Deezer track is matched
|
||||||
|
via Deezer's ``/search/track`` endpoint, which omits ``track_position``
|
||||||
|
(core/deezer_client.py), so the metadata context never carried the
|
||||||
|
real number — but the downloaded file *does* (deemix wrote it). This
|
||||||
|
recovers it with no network call.
|
||||||
|
|
||||||
|
Returns a positive int, or None when the file has no usable
|
||||||
|
tracknumber tag / can't be read. Never raises. Handles the common
|
||||||
|
``"2/15"`` (number/total) form by taking the leading number.
|
||||||
|
"""
|
||||||
|
if not file_path:
|
||||||
|
return None
|
||||||
|
try:
|
||||||
|
from mutagen import File as MutagenFile
|
||||||
|
audio = MutagenFile(file_path, easy=True)
|
||||||
|
if audio is None:
|
||||||
|
return None
|
||||||
|
raw = audio.get('tracknumber')
|
||||||
|
if isinstance(raw, list):
|
||||||
|
raw = raw[0] if raw else None
|
||||||
|
if raw is None:
|
||||||
|
return None
|
||||||
|
# "2/15" -> "2"; bare "2" -> "2".
|
||||||
|
text = str(raw).split('/', 1)[0].strip()
|
||||||
|
return _coerce_positive(text)
|
||||||
|
except Exception:
|
||||||
|
return None
|
||||||
|
|
||||||
|
|
||||||
def resolve_track_number(
|
def resolve_track_number(
|
||||||
album_info: Any,
|
album_info: Any,
|
||||||
track_info: Any,
|
track_info: Any,
|
||||||
file_path: str,
|
file_path: str,
|
||||||
|
embedded_track_number: Any = None,
|
||||||
) -> Optional[int]:
|
) -> Optional[int]:
|
||||||
"""Walk the resolution chain and return the first valid positive
|
"""Walk the resolution chain and return the first valid positive
|
||||||
int found, or None when every source is missing / unusable.
|
int found, or None when every source is missing / unusable.
|
||||||
|
|
||||||
Caller is responsible for the final default-1 floor — leaving
|
Order: album_info -> track_info -> nested spotify_data -> filename ->
|
||||||
that out of this function so tests can pin "everything missing
|
``embedded_track_number`` (the source-written file tag, when the caller
|
||||||
|
supplies it). Caller is responsible for the final default-1 floor —
|
||||||
|
leaving that out of this function so tests can pin "everything missing
|
||||||
returns None" separate from the floor behaviour.
|
returns None" separate from the floor behaviour.
|
||||||
|
|
||||||
|
``embedded_track_number`` is passed in (not read here) so this stays a
|
||||||
|
pure function — the file I/O lives in :func:`read_embedded_track_number`.
|
||||||
|
It is consulted **last**, only when every other source came up empty, so
|
||||||
|
it can never override a value the pre-fix resolver already produced — it
|
||||||
|
only fills the gap that would otherwise hit the default-1 floor.
|
||||||
"""
|
"""
|
||||||
album_info = album_info if isinstance(album_info, dict) else {}
|
album_info = album_info if isinstance(album_info, dict) else {}
|
||||||
track_info = track_info if isinstance(track_info, dict) else {}
|
track_info = track_info if isinstance(track_info, dict) else {}
|
||||||
|
|
@ -96,10 +143,19 @@ def resolve_track_number(
|
||||||
# default-1 floor is the single source of that fallback —
|
# default-1 floor is the single source of that fallback —
|
||||||
# otherwise this resolver would silently fill 1 and the
|
# otherwise this resolver would silently fill 1 and the
|
||||||
# downstream floor logic would have no effect.
|
# downstream floor logic would have no effect.
|
||||||
if not file_path:
|
if file_path:
|
||||||
return None
|
try:
|
||||||
try:
|
from_filename = extract_explicit_track_number(file_path)
|
||||||
from_filename = extract_explicit_track_number(file_path)
|
except Exception:
|
||||||
except Exception:
|
from_filename = None
|
||||||
from_filename = None
|
ff = _coerce_positive(from_filename)
|
||||||
return _coerce_positive(from_filename)
|
if ff is not None:
|
||||||
|
return ff
|
||||||
|
|
||||||
|
# Embedded source-written file tag is consulted LAST — only when every
|
||||||
|
# other source (metadata + the ripped-album "NN - Title" filename) came
|
||||||
|
# up empty. This is deliberate: it can ONLY fill the gap that would
|
||||||
|
# otherwise hit the caller's default-1 floor, so it never overrides a
|
||||||
|
# value the pre-fix resolver would have used. A correctly-named file
|
||||||
|
# with a stale/wrong embedded tag is therefore never regressed.
|
||||||
|
return _coerce_positive(embedded_track_number)
|
||||||
|
|
|
||||||
|
|
@ -214,7 +214,11 @@ class WishlistService:
|
||||||
"preview_url": track_data.get("preview_url") if isinstance(track_data, dict) else None,
|
"preview_url": track_data.get("preview_url") if isinstance(track_data, dict) else None,
|
||||||
"external_urls": track_data.get("external_urls", {}) if isinstance(track_data, dict) else {},
|
"external_urls": track_data.get("external_urls", {}) if isinstance(track_data, dict) else {},
|
||||||
"popularity": track_data.get("popularity", 0) if isinstance(track_data, dict) else 0,
|
"popularity": track_data.get("popularity", 0) if isinstance(track_data, dict) else 0,
|
||||||
"track_number": track_data.get("track_number", 1) if isinstance(track_data, dict) else 1,
|
# "Track 01" bug: 0 = "unknown position", NOT a fabricated 1.
|
||||||
|
# A fake 1 looks authoritative and blocks the import
|
||||||
|
# pipeline's track-number recovery; 0 lets it recover the
|
||||||
|
# real position (file tag / source lookup) before the floor.
|
||||||
|
"track_number": track_data.get("track_number", 0) if isinstance(track_data, dict) else 0,
|
||||||
"disc_number": track_data.get("disc_number", 1) if isinstance(track_data, dict) else 1,
|
"disc_number": track_data.get("disc_number", 1) if isinstance(track_data, dict) else 1,
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -14,7 +14,7 @@ from __future__ import annotations
|
||||||
|
|
||||||
from unittest.mock import patch
|
from unittest.mock import patch
|
||||||
|
|
||||||
from core.imports.track_number import resolve_track_number
|
from core.imports.track_number import read_embedded_track_number, resolve_track_number
|
||||||
|
|
||||||
|
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
|
|
@ -226,3 +226,113 @@ def test_non_dict_track_info_treated_as_empty():
|
||||||
"""Defensive — non-dict track_info won't crash the resolver."""
|
"""Defensive — non-dict track_info won't crash the resolver."""
|
||||||
result = resolve_track_number({}, 'not a dict', '/dir/file.flac')
|
result = resolve_track_number({}, 'not a dict', '/dir/file.flac')
|
||||||
assert result is None
|
assert result is None
|
||||||
|
|
||||||
|
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# "Track 01" bug (Deezer single tracks): embedded file-tag source.
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
|
||||||
|
|
||||||
|
def test_embedded_tag_used_when_metadata_missing():
|
||||||
|
"""The Deezer single-track case: context carried no position (search
|
||||||
|
endpoint omits track_position), but the downloaded file did. With the
|
||||||
|
metadata-context de-poisoned to 0/None, the embedded tag is consulted
|
||||||
|
BEFORE the filename guess."""
|
||||||
|
result = resolve_track_number(
|
||||||
|
album_info={'track_number': 0}, # de-poisoned "unknown"
|
||||||
|
track_info={'track_number': 0},
|
||||||
|
file_path='/dl/Lenny Kravitz - Fly Away.flac', # no number prefix
|
||||||
|
embedded_track_number=2,
|
||||||
|
)
|
||||||
|
assert result == 2
|
||||||
|
|
||||||
|
|
||||||
|
def test_real_metadata_still_beats_embedded_tag():
|
||||||
|
"""No regression for album downloads: an authoritative album_info
|
||||||
|
position wins over the file tag (they normally agree, but the
|
||||||
|
context is the source of truth when present)."""
|
||||||
|
result = resolve_track_number(
|
||||||
|
album_info={'track_number': 5},
|
||||||
|
track_info={},
|
||||||
|
file_path='/dl/file.flac',
|
||||||
|
embedded_track_number=2,
|
||||||
|
)
|
||||||
|
assert result == 5
|
||||||
|
|
||||||
|
|
||||||
|
def test_filename_beats_embedded_tag_no_regression():
|
||||||
|
"""SAFETY: embedded tag is consulted LAST, so a correctly-named ripped
|
||||||
|
file ('05 - Song') with a stale/wrong embedded tag is NOT regressed —
|
||||||
|
the filename still wins, exactly as before the fix."""
|
||||||
|
result = resolve_track_number(
|
||||||
|
album_info={},
|
||||||
|
track_info={},
|
||||||
|
file_path='/dl/09 - Mislabelled.flac',
|
||||||
|
embedded_track_number=2, # wrong/stale tag must NOT win
|
||||||
|
)
|
||||||
|
assert result == 9
|
||||||
|
|
||||||
|
|
||||||
|
def test_embedded_only_fills_the_floor_gap():
|
||||||
|
"""When metadata AND filename are both empty (the Deezer case), the
|
||||||
|
embedded tag fills what would otherwise be the blind default-1 floor."""
|
||||||
|
result = resolve_track_number(
|
||||||
|
album_info={}, track_info={},
|
||||||
|
file_path='/dl/Lenny Kravitz - Fly Away.flac', # no number prefix
|
||||||
|
embedded_track_number=2,
|
||||||
|
)
|
||||||
|
assert result == 2
|
||||||
|
|
||||||
|
|
||||||
|
def test_resolver_without_embedded_arg_is_backwards_compatible():
|
||||||
|
"""The new parameter is optional — existing 3-arg callers unaffected."""
|
||||||
|
assert resolve_track_number({}, {'track_number': 4}, '/x.flac') == 4
|
||||||
|
|
||||||
|
|
||||||
|
# read_embedded_track_number — mutagen-backed file tag reader.
|
||||||
|
|
||||||
|
def _fake_mutagen(tag_value):
|
||||||
|
"""Patch target factory: returns a callable standing in for
|
||||||
|
``mutagen.File`` that yields an object whose .get('tracknumber')
|
||||||
|
returns tag_value (mimicking easy=True list-valued tags)."""
|
||||||
|
class _Audio(dict):
|
||||||
|
pass
|
||||||
|
def _factory(path, easy=False):
|
||||||
|
a = _Audio()
|
||||||
|
if tag_value is not None:
|
||||||
|
a['tracknumber'] = tag_value
|
||||||
|
return a
|
||||||
|
return _factory
|
||||||
|
|
||||||
|
|
||||||
|
def test_read_embedded_handles_number_slash_total():
|
||||||
|
with patch('mutagen.File', _fake_mutagen(['2/15'])):
|
||||||
|
assert read_embedded_track_number('/x.flac') == 2
|
||||||
|
|
||||||
|
|
||||||
|
def test_read_embedded_handles_bare_number():
|
||||||
|
with patch('mutagen.File', _fake_mutagen(['7'])):
|
||||||
|
assert read_embedded_track_number('/x.flac') == 7
|
||||||
|
|
||||||
|
|
||||||
|
def test_read_embedded_none_when_tag_absent():
|
||||||
|
with patch('mutagen.File', _fake_mutagen(None)):
|
||||||
|
assert read_embedded_track_number('/x.flac') is None
|
||||||
|
|
||||||
|
|
||||||
|
def test_read_embedded_none_when_file_unreadable():
|
||||||
|
def _factory(path, easy=False):
|
||||||
|
return None
|
||||||
|
with patch('mutagen.File', _factory):
|
||||||
|
assert read_embedded_track_number('/x.flac') is None
|
||||||
|
|
||||||
|
|
||||||
|
def test_read_embedded_never_raises():
|
||||||
|
def _boom(path, easy=False):
|
||||||
|
raise RuntimeError('corrupt')
|
||||||
|
with patch('mutagen.File', _boom):
|
||||||
|
assert read_embedded_track_number('/x.flac') is None
|
||||||
|
|
||||||
|
|
||||||
|
def test_read_embedded_empty_path_returns_none():
|
||||||
|
assert read_embedded_track_number('') is None
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue