#915: post-processing hydrates lean album context from the PRIMARY source (parity with Reorganize)
Root cause: the only album-context backfill in the download path (hydrate_download_metadata) goes through spotify_client.get_track_details — Spotify-only. An iTunes/Deezer-primary user's download kept a lean context (no release_date), so the path dropped $year and the date defaulted to YYYY-01-01 — until they ran a Reorganize, which reads the full album from the PRIMARY source. That asymmetry IS the bug. Fix: when the context is lean and the primary source isn't Spotify, hydrate it from that source via get_album_for_source — the exact path Reorganize/Enrich use. Verified the primary source returns the real data (live iTunes get_album for the reporter's album: release_date 2024-04-17, not 2024-01-01). backfill_album_context_from_source is a pure, injected-fn seam: 6 tests (hydrate, no-op when complete / spotify-primary / sentinel-id, stays-lean on None, swallows source errors). 552 downloads tests green.
This commit is contained in:
parent
cce7df4f3d
commit
2b17ed8451
3 changed files with 134 additions and 0 deletions
|
|
@ -205,6 +205,21 @@ def attempt_download_with_candidates(task_id, candidates, track, batch_id=None,
|
||||||
'artists': _fallback_album_artists
|
'artists': _fallback_album_artists
|
||||||
}
|
}
|
||||||
|
|
||||||
|
# #915: parity with Reorganize / manual Enrich. If the album context is lean
|
||||||
|
# (no release_date) and the user's PRIMARY metadata source isn't Spotify, hydrate
|
||||||
|
# it from that source — the same place a reorganize reads — so the download's
|
||||||
|
# $year folder, release_date and album_type match instead of dropping the year /
|
||||||
|
# defaulting to YYYY-01-01 and forcing a manual reorganize afterwards.
|
||||||
|
try:
|
||||||
|
from core.downloads.track_metadata_backfill import backfill_album_context_from_source
|
||||||
|
from core.metadata import registry as _meta_registry
|
||||||
|
from core.metadata.album_tracks import get_album_for_source as _get_album_for_source
|
||||||
|
backfill_album_context_from_source(
|
||||||
|
spotify_album_context, _meta_registry.get_primary_source(), _get_album_for_source,
|
||||||
|
)
|
||||||
|
except Exception as _bf_err: # noqa: BLE001 — never let backfill break a download
|
||||||
|
logger.debug("[Context] primary-source album backfill skipped: %s", _bf_err)
|
||||||
|
|
||||||
download_payload = candidate.__dict__
|
download_payload = candidate.__dict__
|
||||||
|
|
||||||
username = download_payload.get('username')
|
username = download_payload.get('username')
|
||||||
|
|
|
||||||
|
|
@ -93,6 +93,56 @@ def _backfill_album_context(
|
||||||
album_context['image_url'] = first['url']
|
album_context['image_url'] = first['url']
|
||||||
|
|
||||||
|
|
||||||
|
# Placeholder album ids used when no real source album id is known — never queryable.
|
||||||
|
_SENTINEL_ALBUM_IDS = {'explicit_album', 'from_sync_modal', ''}
|
||||||
|
|
||||||
|
|
||||||
|
def backfill_album_context_from_source(
|
||||||
|
album_context: Dict[str, Any],
|
||||||
|
primary_source: Optional[str],
|
||||||
|
get_album_for_source_fn: Any,
|
||||||
|
) -> bool:
|
||||||
|
"""Hydrate a lean album context from the user's PRIMARY metadata source (#915).
|
||||||
|
|
||||||
|
Post-processing's only album backfill (:func:`hydrate_download_metadata`) goes through
|
||||||
|
``spotify_client.get_track_details`` — Spotify-only. An iTunes/Deezer-primary user's
|
||||||
|
download therefore kept a lean context (no ``release_date``), so the path dropped the
|
||||||
|
``$year`` and the date defaulted to ``YYYY-01-01`` — until they ran a Reorganize, which
|
||||||
|
reads the full album from the PRIMARY source. This closes that gap by doing the same:
|
||||||
|
fetch the full album from the primary source and backfill, so a download's pathing/tags
|
||||||
|
match what a later reorganize would produce.
|
||||||
|
|
||||||
|
``get_album_for_source_fn(source, album_id)`` is injected (the real one is
|
||||||
|
``core.metadata.album_tracks.get_album_for_source``) so this stays pure + testable.
|
||||||
|
No-op when: the context is already complete; the primary source is spotify (the existing
|
||||||
|
track-details path covers it); or no real source album id is present. Returns True when
|
||||||
|
it filled anything. Never raises — a backfill failure must not break a download.
|
||||||
|
"""
|
||||||
|
if not isinstance(album_context, dict) or not _album_is_lean(album_context):
|
||||||
|
return False
|
||||||
|
if not primary_source or primary_source == 'spotify':
|
||||||
|
return False
|
||||||
|
album_id = album_context.get('id')
|
||||||
|
if not album_id or str(album_id) in _SENTINEL_ALBUM_IDS:
|
||||||
|
return False
|
||||||
|
try:
|
||||||
|
album = get_album_for_source_fn(primary_source, str(album_id))
|
||||||
|
except Exception as e: # noqa: BLE001 — defensive: never let backfill break a download
|
||||||
|
logger.warning("[Context] primary-source (%s) album backfill failed: %s", primary_source, e)
|
||||||
|
return False
|
||||||
|
if not isinstance(album, dict):
|
||||||
|
return False
|
||||||
|
before = album_context.get('release_date')
|
||||||
|
_backfill_album_context(album_context, {'album': album})
|
||||||
|
if album_context.get('release_date') and album_context.get('release_date') != before:
|
||||||
|
logger.info(
|
||||||
|
"[Context] Hydrated lean album context from primary source %s "
|
||||||
|
"(release_date=%r, total_tracks=%r)",
|
||||||
|
primary_source, album_context.get('release_date'), album_context.get('total_tracks'),
|
||||||
|
)
|
||||||
|
return True
|
||||||
|
|
||||||
|
|
||||||
def hydrate_download_metadata(
|
def hydrate_download_metadata(
|
||||||
track: Any,
|
track: Any,
|
||||||
track_info: Any,
|
track_info: Any,
|
||||||
|
|
@ -172,4 +222,5 @@ def hydrate_download_metadata(
|
||||||
__all__ = [
|
__all__ = [
|
||||||
'ResolvedTrackMetadata',
|
'ResolvedTrackMetadata',
|
||||||
'hydrate_download_metadata',
|
'hydrate_download_metadata',
|
||||||
|
'backfill_album_context_from_source',
|
||||||
]
|
]
|
||||||
|
|
|
||||||
|
|
@ -507,5 +507,73 @@ def test_api_called_at_most_once_per_invocation():
|
||||||
assert client.get_track_details.call_count == 1
|
assert client.get_track_details.call_count == 1
|
||||||
|
|
||||||
|
|
||||||
|
# ── #915: source-aware album-context backfill (parity with Reorganize/Enrich) ──
|
||||||
|
from core.downloads.track_metadata_backfill import backfill_album_context_from_source # noqa: E402
|
||||||
|
|
||||||
|
|
||||||
|
def _lean_ctx(album_id="itunes-123"):
|
||||||
|
return {"id": album_id, "name": "Big OST", "release_date": "", "total_tracks": 0, "album_type": "album"}
|
||||||
|
|
||||||
|
|
||||||
|
def _itunes_album(**over):
|
||||||
|
base = {"id": "itunes-123", "name": "Big OST", "release_date": "2024-04-17",
|
||||||
|
"total_tracks": 70, "album_type": "album"}
|
||||||
|
base.update(over)
|
||||||
|
return base
|
||||||
|
|
||||||
|
|
||||||
|
def test_backfill_hydrates_lean_context_from_primary_source():
|
||||||
|
ctx = _lean_ctx()
|
||||||
|
calls = []
|
||||||
|
|
||||||
|
def get_album(source, album_id):
|
||||||
|
calls.append((source, album_id))
|
||||||
|
return _itunes_album()
|
||||||
|
|
||||||
|
assert backfill_album_context_from_source(ctx, "itunes", get_album) is True
|
||||||
|
assert ctx["release_date"] == "2024-04-17" # real date, not YYYY-01-01
|
||||||
|
assert ctx["total_tracks"] == 70
|
||||||
|
assert calls == [("itunes", "itunes-123")] # queried the PRIMARY source with the album id
|
||||||
|
|
||||||
|
|
||||||
|
def test_backfill_noop_when_context_already_complete():
|
||||||
|
ctx = {"id": "itunes-123", "release_date": "2024-04-17", "total_tracks": 70, "album_type": "album"}
|
||||||
|
called = []
|
||||||
|
backfill_album_context_from_source(ctx, "itunes", lambda *a: called.append(a))
|
||||||
|
assert called == [] # complete -> no fetch
|
||||||
|
assert ctx["release_date"] == "2024-04-17"
|
||||||
|
|
||||||
|
|
||||||
|
def test_backfill_noop_for_spotify_primary():
|
||||||
|
# Spotify is covered by hydrate_download_metadata's get_track_details path.
|
||||||
|
called = []
|
||||||
|
assert backfill_album_context_from_source(_lean_ctx(), "spotify", lambda *a: called.append(a)) is False
|
||||||
|
assert called == []
|
||||||
|
|
||||||
|
|
||||||
|
def test_backfill_noop_for_sentinel_album_id():
|
||||||
|
called = []
|
||||||
|
for sentinel in ("explicit_album", "from_sync_modal", ""):
|
||||||
|
backfill_album_context_from_source(_lean_ctx(sentinel), "itunes", lambda *a: called.append(a))
|
||||||
|
assert called == [] # no real id -> never queries
|
||||||
|
|
||||||
|
|
||||||
|
def test_backfill_leaves_context_lean_when_source_returns_nothing():
|
||||||
|
ctx = _lean_ctx()
|
||||||
|
backfill_album_context_from_source(ctx, "itunes", lambda *a: None)
|
||||||
|
assert ctx["release_date"] == "" # still lean, but no crash
|
||||||
|
|
||||||
|
|
||||||
|
def test_backfill_swallows_source_errors():
|
||||||
|
ctx = _lean_ctx()
|
||||||
|
|
||||||
|
def boom(*_a):
|
||||||
|
raise RuntimeError("itunes down")
|
||||||
|
|
||||||
|
# Must not raise — a backfill failure cannot break a download.
|
||||||
|
assert backfill_album_context_from_source(ctx, "itunes", boom) is False
|
||||||
|
assert ctx["release_date"] == ""
|
||||||
|
|
||||||
|
|
||||||
if __name__ == '__main__':
|
if __name__ == '__main__':
|
||||||
pytest.main([__file__, '-v'])
|
pytest.main([__file__, '-v'])
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue