From a6364ac2836e8d59d7365cd1c95445cfcdd4e911 Mon Sep 17 00:00:00 2001 From: ragnarlotus Date: Fri, 26 Jun 2026 06:22:07 +0200 Subject: [PATCH] fix album completeness canonical edition matching --- core/repair_jobs/album_completeness.py | 154 ++++++++++++++----- tests/test_album_completeness_job.py | 204 ++++++++++++++++++++++++- 2 files changed, 319 insertions(+), 39 deletions(-) diff --git a/core/repair_jobs/album_completeness.py b/core/repair_jobs/album_completeness.py index 24b1260f..5744a5bf 100644 --- a/core/repair_jobs/album_completeness.py +++ b/core/repair_jobs/album_completeness.py @@ -21,9 +21,10 @@ class AlbumCompletenessJob(RepairJob): help_text = ( 'Compares the number of tracks you have for each album against the expected total ' 'from your configured metadata sources. Counts cached during normal enrichment are ' - 'used when available; otherwise the job queries a metadata source directly. Albums ' - 'where tracks are missing get flagged as findings with details about which tracks ' - 'are absent.\n\n' + 'used when no canonical edition is pinned; otherwise the exact canonical source and ' + 'album ID are queried directly. The same canonical tracklist is used for both the ' + 'expected total and the missing-track calculation. Albums where tracks are missing ' + 'get flagged as findings with details about which tracks are absent.\n\n' 'Useful for catching partial downloads or albums where some tracks failed to download. ' 'You can use the Download Missing feature from the album page to fill gaps.\n\n' 'Settings:\n' @@ -56,6 +57,7 @@ class AlbumCompletenessJob(RepairJob): has_itunes = False has_deezer = False has_api_track_count = False + has_canonical = False try: conn = context.db._get_connection() cursor = conn.cursor() @@ -67,6 +69,10 @@ class AlbumCompletenessJob(RepairJob): has_deezer = 'deezer_id' in columns has_discogs = 'discogs_id' in columns has_hydrabase = 'soul_id' in columns + has_canonical = ( + 'canonical_source' in columns + and 'canonical_album_id' in columns + ) # Detect the `api_track_count` column — older DBs may not have it # yet (migration runs on app start, but repair-job code mustn't @@ -101,8 +107,13 @@ class AlbumCompletenessJob(RepairJob): select_cols.append(('al.discogs_id', 'discogs_album_id')) if has_hydrabase: select_cols.append(('al.soul_id', 'hydrabase_album_id')) + if has_canonical: + select_cols.extend([ + ('al.canonical_source', 'canonical_source'), + ('al.canonical_album_id', 'canonical_album_id'), + ]) - # WHERE: album has at least one source ID + # WHERE: album has at least one source ID or a complete canonical pair where_parts = ["(al.spotify_album_id IS NOT NULL AND al.spotify_album_id != '')"] if has_itunes: where_parts.append("(al.itunes_album_id IS NOT NULL AND al.itunes_album_id != '')") @@ -112,6 +123,11 @@ class AlbumCompletenessJob(RepairJob): where_parts.append("(al.discogs_id IS NOT NULL AND al.discogs_id != '')") if has_hydrabase: where_parts.append("(al.soul_id IS NOT NULL AND al.soul_id != '')") + if has_canonical: + where_parts.append( + "(al.canonical_source IS NOT NULL AND al.canonical_source != '' " + "AND al.canonical_album_id IS NOT NULL AND al.canonical_album_id != '')" + ) where_clause = ' OR '.join(where_parts) select_sql = ', '.join(f'{expr} AS {alias}' for expr, alias in select_cols) @@ -159,6 +175,8 @@ class AlbumCompletenessJob(RepairJob): deezer_album_id = row[column_index['deezer_album_id']] if 'deezer_album_id' in column_index else None discogs_album_id = row[column_index['discogs_album_id']] if 'discogs_album_id' in column_index else None hydrabase_album_id = row[column_index['hydrabase_album_id']] if 'hydrabase_album_id' in column_index else None + canonical_source = row[column_index['canonical_source']] if 'canonical_source' in column_index else None + canonical_album_id = row[column_index['canonical_album_id']] if 'canonical_album_id' in column_index else None # Cached authoritative track count from a prior API lookup (NULL # on unscanned albums and on DBs predating the column migration). cached_api_count = row[column_index['api_track_count']] if 'api_track_count' in column_index else None @@ -181,20 +199,41 @@ class AlbumCompletenessJob(RepairJob): 'hydrabase': hydrabase_album_id or '', } - # Expected total comes from the metadata provider, NOT from - # al.track_count — that column holds the observed count from - # server syncs (Plex leafCount, SoulSync standalone len(tracks)) - # which by definition always equals actual_count and made the - # job skip every album. Use the cached api_track_count if a - # prior scan already looked it up; otherwise hit the API and - # persist the answer for next time. - expected_total = cached_api_count - if not expected_total: - expected_total = self._get_expected_total(context, primary_source, album_ids) - # Only persist positive results. Zero/None would keep - # re-triggering the lookup on every scan. - if expected_total and expected_total > 0 and has_api_track_count: - self._save_api_track_count(context, album_id, expected_total) + # A complete canonical pair identifies one exact edition, + # independently of provider. Its tracklist must drive both the + # expected total and the missing-track calculation; neither a + # cached count nor another source may silently replace it. + canonical_items = None + resolved_source = primary_source + resolved_album_id = self._get_album_id_for_source(primary_source, album_ids) or '' + + if canonical_source and canonical_album_id: + api_tracks = self._get_album_tracks( + str(canonical_source), + str(canonical_album_id), + ) + canonical_items = self._extract_track_items(api_tracks) + expected_total = len(canonical_items) + resolved_source = str(canonical_source) + resolved_album_id = str(canonical_album_id) + else: + # Preserve the existing behavior for albums that have not + # resolved a canonical edition yet. + expected_total = cached_api_count + if not expected_total: + expected_total = self._get_expected_total( + context, + primary_source, + album_ids, + ) + # Only persist positive results. Zero/None would keep + # re-triggering the lookup on every scan. + if expected_total and expected_total > 0 and has_api_track_count: + self._save_api_track_count( + context, + album_id, + expected_total, + ) # Skip singles/EPs based on expected track count (not local count) if expected_total and expected_total < min_tracks: @@ -224,8 +263,20 @@ class AlbumCompletenessJob(RepairJob): context.update_progress(i + 1, total) continue - # Album is incomplete — try to find which tracks are missing - missing_tracks = self._find_missing_tracks(context, primary_source, album_id, album_ids) + # Album is incomplete — identify missing tracks from the same + # canonical edition used for the expected total when one is pinned. + missing_tracks = self._find_missing_tracks( + context, + primary_source, + album_id, + album_ids, + resolved_source=( + resolved_source + if canonical_items is not None + else None + ), + resolved_items=canonical_items, + ) if context.report_progress: context.report_progress( @@ -250,8 +301,10 @@ class AlbumCompletenessJob(RepairJob): 'album_id': album_id, 'album_title': title, 'artist': artist_name, - 'primary_source': primary_source, - 'primary_album_id': self._get_album_id_for_source(primary_source, album_ids) or '', + 'primary_source': resolved_source, + 'primary_album_id': resolved_album_id, + 'canonical_source': canonical_source or '', + 'canonical_album_id': canonical_album_id or '', 'spotify_album_id': spotify_album_id or '', 'itunes_album_id': itunes_album_id or '', 'deezer_album_id': deezer_album_id or '', @@ -316,8 +369,19 @@ class AlbumCompletenessJob(RepairJob): return 0 - def _find_missing_tracks(self, context, primary_source, album_id, album_ids): - """Identify which specific tracks are missing using the active metadata provider first.""" + def _find_missing_tracks( + self, + context, + primary_source, + album_id, + album_ids, + resolved_source=None, + resolved_items=None, + ): + """Identify missing tracks from one resolved metadata edition. + + Without a supplied edition, preserve the existing source-priority lookup. + """ # Get track numbers we already have owned_numbers = set() conn = None @@ -336,19 +400,33 @@ class AlbumCompletenessJob(RepairJob): if conn: conn.close() - api_tracks = None - for source in get_source_priority(primary_source): - source_album_id = self._get_album_id_for_source(source, album_ids) - if not source_album_id: - continue - api_tracks = self._get_album_tracks(source, source_album_id) - if self._extract_track_items(api_tracks): - break + if resolved_items is None: + api_tracks = None + for source in get_source_priority(primary_source): + source_album_id = self._get_album_id_for_source( + source, + album_ids, + ) + if not source_album_id: + continue + + api_tracks = self._get_album_tracks( + source, + source_album_id, + ) + if self._extract_track_items(api_tracks): + resolved_source = source + break + + items = self._extract_track_items(api_tracks) + else: + items = resolved_items - items = self._extract_track_items(api_tracks) if not items: return [] + track_source = resolved_source or primary_source + # All supported provider responses expose the same core fields once normalized. # items[].track_number, items[].name, items[].disc_number, items[].id, items[].artists missing_tracks = [] @@ -365,7 +443,7 @@ class AlbumCompletenessJob(RepairJob): 'track_number': tn, 'name': item.get('name', ''), 'disc_number': item.get('disc_number', 1), - 'source': item.get('_source', primary_source), + 'source': item.get('_source', track_source), 'source_track_id': item.get('id', ''), 'track_id': item.get('id', ''), 'spotify_track_id': item.get('id', ''), @@ -430,6 +508,14 @@ class AlbumCompletenessJob(RepairJob): where_parts.append("(discogs_id IS NOT NULL AND discogs_id != '')") if 'soul_id' in columns: where_parts.append("(soul_id IS NOT NULL AND soul_id != '')") + if ( + 'canonical_source' in columns + and 'canonical_album_id' in columns + ): + where_parts.append( + "(canonical_source IS NOT NULL AND canonical_source != '' " + "AND canonical_album_id IS NOT NULL AND canonical_album_id != '')" + ) cursor.execute(f""" SELECT COUNT(*) FROM albums diff --git a/tests/test_album_completeness_job.py b/tests/test_album_completeness_job.py index 8d58a03c..bd0885cd 100644 --- a/tests/test_album_completeness_job.py +++ b/tests/test_album_completeness_job.py @@ -420,6 +420,8 @@ class _SharedMemoryDB: deezer_id TEXT, discogs_id TEXT, soul_id TEXT, + canonical_source TEXT, + canonical_album_id TEXT, track_count INTEGER, api_track_count INTEGER ); @@ -442,13 +444,41 @@ class _SharedMemoryDB: ) self._keepalive.commit() - def insert_album(self, album_id, artist_id, title, *, spotify_id=None, - track_count=None, api_track_count=None): + def insert_album( + self, + album_id, + artist_id, + title, + *, + spotify_id=None, + canonical_source=None, + canonical_album_id=None, + track_count=None, + api_track_count=None, + ): self._keepalive.execute( """INSERT INTO albums - (id, artist_id, title, spotify_album_id, track_count, api_track_count) - VALUES (?, ?, ?, ?, ?, ?)""", - (album_id, artist_id, title, spotify_id, track_count, api_track_count), + ( + id, + artist_id, + title, + spotify_album_id, + canonical_source, + canonical_album_id, + track_count, + api_track_count + ) + VALUES (?, ?, ?, ?, ?, ?, ?, ?)""", + ( + album_id, + artist_id, + title, + spotify_id, + canonical_source, + canonical_album_id, + track_count, + api_track_count, + ), ) self._keepalive.commit() @@ -543,6 +573,170 @@ def test_scan_uses_cached_api_track_count_without_expected_total_lookup(monkeypa assert finding['details']['actual_tracks'] == 10 +def test_scan_uses_exact_canonical_edition_for_count_and_missing_tracks( + monkeypatch, +): + db = _SharedMemoryDB() + db.insert_artist('a1', 'Test Artist') + db.insert_album( + 'alb-canonical', + 'a1', + 'Canonical Album', + spotify_id='sp-other-edition', + canonical_source='deezer', + canonical_album_id='dz-canonical', + track_count=2, + api_track_count=99, + ) + db.insert_tracks('alb-canonical', 2) + + calls = [] + + def get_tracks(source, album_id): + calls.append((source, album_id)) + + if source == 'deezer' and album_id == 'dz-canonical': + return { + 'items': [ + { + 'id': f'dz-{number}', + 'name': f'Canonical Track {number}', + 'track_number': number, + 'disc_number': 1, + 'artists': [], + } + for number in range(1, 5) + ], + } + + if source == 'spotify' and album_id == 'sp-other-edition': + return { + 'items': [ + { + 'id': f'sp-{number}', + 'name': f'Other Edition Track {number}', + 'track_number': number, + 'disc_number': 1, + 'artists': [], + } + for number in range(1, 13) + ], + } + + return None + + monkeypatch.setattr( + album_completeness_module, + 'get_album_tracks_for_source', + get_tracks, + ) + monkeypatch.setattr( + album_completeness_module, + 'get_primary_source', + lambda: 'spotify', + ) + monkeypatch.setattr( + album_completeness_module, + 'get_source_priority', + lambda primary: ['spotify', 'deezer'], + ) + + findings = [] + context = _make_job_context( + db, + create_finding=lambda **kwargs: (findings.append(kwargs) or True), + ) + + result = AlbumCompletenessJob().scan(context) + + assert result.findings_created == 1 + assert calls == [('deezer', 'dz-canonical')] + + details = findings[0]['details'] + + assert details['primary_source'] == 'deezer' + assert details['primary_album_id'] == 'dz-canonical' + assert details['canonical_source'] == 'deezer' + assert details['canonical_album_id'] == 'dz-canonical' + assert details['expected_tracks'] == 4 + assert details['actual_tracks'] == 2 + assert [ + track['track_number'] + for track in details['missing_tracks'] + ] == [3, 4] + assert all( + track['source'] == 'deezer' + for track in details['missing_tracks'] + ) + + +def test_scan_does_not_fallback_when_canonical_edition_is_unavailable( + monkeypatch, +): + db = _SharedMemoryDB() + db.insert_artist('a1', 'Test Artist') + db.insert_album( + 'alb-unavailable-canonical', + 'a1', + 'Unavailable Canonical Album', + spotify_id='sp-other-edition', + canonical_source='deezer', + canonical_album_id='dz-unavailable', + track_count=2, + api_track_count=99, + ) + db.insert_tracks('alb-unavailable-canonical', 2) + + calls = [] + + def get_tracks(source, album_id): + calls.append((source, album_id)) + + if source == 'deezer' and album_id == 'dz-unavailable': + return {'items': []} + + return { + 'items': [ + { + 'id': f'sp-{number}', + 'name': f'Other Edition Track {number}', + 'track_number': number, + 'disc_number': 1, + 'artists': [], + } + for number in range(1, 11) + ], + } + + monkeypatch.setattr( + album_completeness_module, + 'get_album_tracks_for_source', + get_tracks, + ) + monkeypatch.setattr( + album_completeness_module, + 'get_primary_source', + lambda: 'spotify', + ) + monkeypatch.setattr( + album_completeness_module, + 'get_source_priority', + lambda primary: ['spotify', 'deezer'], + ) + + findings = [] + context = _make_job_context( + db, + create_finding=lambda **kwargs: (findings.append(kwargs) or True), + ) + + result = AlbumCompletenessJob().scan(context) + + assert result.findings_created == 0 + assert findings == [] + assert calls == [('deezer', 'dz-unavailable')] + + def test_scan_falls_back_to_api_and_persists_count_on_cache_miss(monkeypatch): """Integration: when api_track_count is NULL, scan calls the API, gets the expected total, caches it, and creates the finding."""