diff --git a/core/discogs_worker.py b/core/discogs_worker.py index 1b124a22..84270de9 100644 --- a/core/discogs_worker.py +++ b/core/discogs_worker.py @@ -23,6 +23,29 @@ from core.worker_utils import interruptible_sleep, set_album_api_track_count logger = get_logger("discogs_worker") +def count_discogs_real_tracks(tracklist) -> int: + """Count actual songs in a Discogs tracklist response. + + Discogs tracklists interleave real tracks with section headings + (``type_=='heading'``), index markers (``type_=='index'``), + and sub-tracks (``type_=='sub_track'``) that aren't themselves + songs. We count anything that's explicitly typed as ``'track'`` OR + has an empty/missing ``type_`` field — matching exactly what + :meth:`core.discogs_client.DiscogsClient.get_album_tracks` itself + treats as a real track (`type_ in ('track', '')`). Counting any + narrower set silently disagrees with the repair job's fallback + `_get_expected_total` path, which calls `get_album_tracks_for_source` + under the hood and therefore uses the client's count. + + Reported by kettui on PR #374 — original filter only counted + ``type_=='track'`` and undercounted releases where the discogs + response left ``type_`` empty for some real tracks. + """ + if not tracklist: + return 0 + return sum(1 for t in tracklist if (t.get('type_') or '') in ('track', '')) + + class DiscogsWorker: """Background worker for enriching library artists and albums with Discogs metadata.""" @@ -455,15 +478,13 @@ class DiscogsWorker: """, (image_url, album_id)) # Cache the authoritative expected track count for the Album - # Completeness repair job. Discogs tracklists mix actual tracks - # with section headings and index entries — only type_=='track' - # rows are real songs, so filter before counting. (Helper can't - # do this filter for us; it's Discogs-specific shape.) - tracklist = data.get('tracklist') or [] + # Completeness repair job. See `count_discogs_real_tracks` + # for why we accept both `type_ == 'track'` and empty `type_` + # (kettui's PR #374 review — narrower filter undercounted). set_album_api_track_count( cursor, album_id, - sum(1 for t in tracklist if t.get('type_') == 'track'), + count_discogs_real_tracks(data.get('tracklist')), ) conn.commit() diff --git a/tests/test_discogs_track_count.py b/tests/test_discogs_track_count.py new file mode 100644 index 00000000..0ad04986 --- /dev/null +++ b/tests/test_discogs_track_count.py @@ -0,0 +1,129 @@ +"""Tests for `discogs_worker.count_discogs_real_tracks` — the filter +that decides which entries in a Discogs tracklist count as real songs +when caching the authoritative track count for the Album Completeness +repair job. + +Reported by kettui on PR #374: the original inline filter only kept +``type_ == 'track'`` rows, but `discogs_client.get_album_tracks` itself +keeps both ``type_ == 'track'`` AND rows with an empty/missing +``type_``. The narrower filter would undercount releases whose Discogs +response left ``type_`` blank for some real tracks — and the repair +job's fallback path (`_get_expected_total`) would silently disagree +with the cached count. +""" + +from core.discogs_worker import count_discogs_real_tracks + + +# --------------------------------------------------------------------------- +# The kettui case: empty type_ counts as a real track +# --------------------------------------------------------------------------- + +def test_empty_type_counts_as_track(): + tracklist = [ + {'title': 'Track 1', 'type_': 'track'}, + {'title': 'Track 2', 'type_': ''}, # <-- the bug + {'title': 'Track 3', 'type_': 'track'}, + ] + assert count_discogs_real_tracks(tracklist) == 3 + + +def test_missing_type_field_counts_as_track(): + """Discogs sometimes omits the field entirely rather than sending + an empty string. Both shapes mean 'real track'.""" + tracklist = [ + {'title': 'Track 1'}, # no type_ key at all + {'title': 'Track 2', 'type_': 'track'}, + ] + assert count_discogs_real_tracks(tracklist) == 2 + + +def test_none_type_counts_as_track(): + """And the field may be present-but-None on some clients/versions.""" + tracklist = [ + {'title': 'Track 1', 'type_': None}, + {'title': 'Track 2', 'type_': 'track'}, + ] + assert count_discogs_real_tracks(tracklist) == 2 + + +# --------------------------------------------------------------------------- +# Non-track rows are excluded (Discogs's structural markers) +# --------------------------------------------------------------------------- + +def test_headings_excluded(): + tracklist = [ + {'title': 'Disc 1', 'type_': 'heading'}, + {'title': 'Track 1', 'type_': 'track'}, + {'title': 'Track 2', 'type_': 'track'}, + {'title': 'Disc 2', 'type_': 'heading'}, + {'title': 'Track 3', 'type_': 'track'}, + ] + assert count_discogs_real_tracks(tracklist) == 3 + + +def test_indices_excluded(): + tracklist = [ + {'title': 'Side A', 'type_': 'index'}, + {'title': 'Track 1', 'type_': 'track'}, + {'title': 'Side B', 'type_': 'index'}, + {'title': 'Track 2', 'type_': 'track'}, + ] + assert count_discogs_real_tracks(tracklist) == 2 + + +def test_sub_tracks_excluded(): + """Sub-tracks (parts of a medley) shouldn't double-count against + the parent track.""" + tracklist = [ + {'title': 'Medley', 'type_': 'track'}, + {'title': 'Part A', 'type_': 'sub_track'}, + {'title': 'Part B', 'type_': 'sub_track'}, + {'title': 'Encore', 'type_': 'track'}, + ] + assert count_discogs_real_tracks(tracklist) == 2 + + +def test_unknown_type_excluded(): + """Conservative — if Discogs adds a new structural marker we + haven't seen, don't count it as a track until we explicitly add + it to the allowlist.""" + tracklist = [ + {'title': 'Track 1', 'type_': 'track'}, + {'title': 'Some Future Marker', 'type_': 'experimental_thing'}, + ] + assert count_discogs_real_tracks(tracklist) == 1 + + +# --------------------------------------------------------------------------- +# Defensive — bad input shouldn't raise +# --------------------------------------------------------------------------- + +def test_empty_tracklist_returns_zero(): + assert count_discogs_real_tracks([]) == 0 + + +def test_none_tracklist_returns_zero(): + assert count_discogs_real_tracks(None) == 0 + + +# --------------------------------------------------------------------------- +# Realistic mixed tracklist (the kettui case in context) +# --------------------------------------------------------------------------- + +def test_realistic_multi_disc_tracklist(): + """A 2-disc release with headings, real tracks, AND a few rows + with empty type_ — should count all real tracks but no markers.""" + tracklist = [ + {'title': 'Disc 1', 'type_': 'heading'}, + {'title': 'Track 1', 'type_': 'track'}, + {'title': 'Track 2', 'type_': 'track'}, + {'title': 'Track 3', 'type_': ''}, # the kettui case + {'title': 'Track 4', 'type_': 'track'}, + {'title': 'Disc 2', 'type_': 'heading'}, + {'title': 'Track 5', 'type_': 'track'}, + {'title': 'Track 6'}, # missing type_ + {'title': 'Bonus Index', 'type_': 'index'}, + {'title': 'Track 7', 'type_': 'track'}, + ] + assert count_discogs_real_tracks(tracklist) == 7