From 5e3b0055dc20077f9071eed604e2ef8d0dea4faf Mon Sep 17 00:00:00 2001 From: Colin Powell Date: Sat, 8 Aug 2026 20:35:45 -0400 Subject: [PATCH] [music] Resolve artist identity before splitting (fd86a11a) --- PROJECT.org | 18 +- tests/music_tests/__init__.py | 0 tests/music_tests/test_artist_resolution.py | 173 ++++++++++++++++++ .../commands/reconcile_split_artists.py | 170 +++++++++++++++++ .../commands/report_mismatched_metadata.py | 60 ++---- vrobbler/apps/music/models.py | 10 +- vrobbler/apps/music/utils.py | 76 ++++++++ 7 files changed, 453 insertions(+), 54 deletions(-) create mode 100644 tests/music_tests/__init__.py create mode 100644 tests/music_tests/test_artist_resolution.py create mode 100644 vrobbler/apps/music/management/commands/reconcile_split_artists.py diff --git a/PROJECT.org b/PROJECT.org index feb4255..17b3dcc 100644 --- a/PROJECT.org +++ b/PROJECT.org @@ -612,7 +612,7 @@ favorited media objects. *** Description As an example https://comicbookroundup.com/comic-books/reviews/humanoids-publishing/the-history-of-science-fiction -** TODO [#A] Add an exception list of artists as a constant that are exempted from splitting :music:artists:metadata: +** DONE [#A] Add an exception list of artists as a constant that are exempted from splitting :music:artists:metadata: :PROPERTIES: :ID: fd86a11a-73ec-470d-b5e3-2d90ba9137c8 :END: @@ -623,6 +623,22 @@ Certain artists like "Simon & Garfunkel" are actually one artist. While we don't tracks into featured artists, we should have a "LITERAL_ARTIST_TITLES" constant that can have exceptions like this put into it and then we stop trying to pull the artist apart when we run into it. +*** Notes + +Instead of a hardcoded exception list, we resolve artist identity before +splitting on " & ": + + - `resolve_artist_names()` in music/utils.py keeps an artist string literal + when a MusicBrainz artist id is provided (webhooks), when the full string + already exists as an Artist, or when it matches a single MusicBrainz + artist (e.g. "Simon & Garfunkel" is a Group). Splitting on " & " is now a + last resort for genuine collaborations. + - `Track.find_or_create()` and `Track.fix_metadata()` use the resolved name + instead of always splitting. + - `reconcile_split_artists` management command repairs tracks already split + by the old logic, using the raw artist data stored in scrobble logs, and + deletes orphaned fragment artists. + ** TODO [#A] Update how board game scrobbles work :boardgames: *** Description diff --git a/tests/music_tests/__init__.py b/tests/music_tests/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/music_tests/test_artist_resolution.py b/tests/music_tests/test_artist_resolution.py new file mode 100644 index 0000000..4823841 --- /dev/null +++ b/tests/music_tests/test_artist_resolution.py @@ -0,0 +1,173 @@ +from unittest.mock import patch + +import pytest +from django.contrib.auth import get_user_model +from django.core.management import call_command +from music.models import Artist, Track +from music.utils import resolve_artist_names +from scrobbles.models import Scrobble + + +@pytest.mark.django_db +def test_resolve_with_mbid_keeps_literal_name(): + names = resolve_artist_names("Simon & Garfunkel", artist_mbid="grp-1") + assert names == ["Simon & Garfunkel"] + + +@pytest.mark.django_db +@patch("music.utils.lookup_artist_from_mb", return_value={"name": "Simon & Garfunkel"}) +def test_resolve_mb_exact_match_keeps_literal_name(mock_lookup): + names = resolve_artist_names("Simon & Garfunkel") + assert names == ["Simon & Garfunkel"] + mock_lookup.assert_called_once_with("Simon & Garfunkel") + + +@pytest.mark.django_db +@patch( + "music.utils.lookup_artist_from_mb", return_value={"name": "Simon and Garfunkel"} +) +def test_resolve_mb_inexact_match_falls_back_to_split(mock_lookup): + names = resolve_artist_names("Simon & Garfunkel") + assert names == ["Simon", "Garfunkel"] + + +@pytest.mark.django_db +@patch("music.utils.lookup_artist_from_mb", return_value={}) +def test_resolve_splits_collab_when_mb_has_no_single_artist(mock_lookup): + names = resolve_artist_names("Matt Sweeney & Bonnie Prince Billy") + assert names == ["Matt Sweeney", "Bonnie Prince Billy"] + + +@pytest.mark.django_db +@patch("music.utils.lookup_artist_from_mb") +def test_resolve_existing_db_artist_skips_mb_lookup(mock_lookup): + Artist.objects.create(name="Simon & Garfunkel") + names = resolve_artist_names("Simon & Garfunkel") + assert names == ["Simon & Garfunkel"] + mock_lookup.assert_not_called() + + +@pytest.mark.django_db +@patch("music.utils.lookup_artist_from_mb", return_value={}) +def test_resolve_strips_featured_names(mock_lookup): + names = resolve_artist_names("Ariana Grande feat. Zedd") + assert names == ["Ariana Grande"] + + +@pytest.mark.django_db +def test_find_or_create_trust_webhook_keeps_literal_artist(): + track = Track.find_or_create( + title="Bridge over Troubled Water", + artist_name="Simon & Garfunkel", + album_name="Bridge over Troubled Water", + artist_mbid="grp-1", + trust_webhook_data=True, + ) + + assert list(track.artists.all().values_list("name", flat=True)) == [ + "Simon & Garfunkel" + ] + assert not Artist.objects.filter(name="Simon").exists() + assert not Artist.objects.filter(name="Garfunkel").exists() + + +@pytest.mark.django_db +@patch("music.models.resolve_track", return_value=(None, "")) +def test_find_or_create_reuses_literal_artist(mock_resolve): + artist = Artist.objects.create(name="Simon & Garfunkel") + + track = Track.find_or_create( + title="Bridge over Troubled Water", + artist_name="Simon & Garfunkel", + ) + + assert list(track.artists.all().values_list("name", flat=True)) == [ + "Simon & Garfunkel" + ] + assert not Artist.objects.filter(name="Simon").exists() + assert not Artist.objects.filter(name="Garfunkel").exists() + + +@pytest.mark.django_db +@patch("music.models.get_track_metadata_with_artist", return_value=None) +@patch("music.models.lookup_artist_from_tadb", return_value=None) +def test_reconcile_command_merges_split_artists(mock_tadb, mock_track_meta): + user = get_user_model().objects.create(email="reconcile@example.com") + simon = Artist.objects.create(name="Simon") + garfunkel = Artist.objects.create(name="Garfunkel") + track = Track.objects.create(title="Bridge over Troubled Water", artist_fk=simon) + track.artists.add(simon, garfunkel) + Scrobble.objects.create( + track=track, + media_type="Track", + user=user, + log={ + "raw_data": { + "artist": "Simon & Garfunkel", + "musicbrainz_artist_id": "grp-1", + } + }, + ) + + call_command("reconcile_split_artists", "--commit") + + track.refresh_from_db() + assert [a.name for a in track.artists.all()] == ["Simon & Garfunkel"] + assert track.artist_fk.name == "Simon & Garfunkel" + assert "artist-reconciled" in track.tags.names() + assert not Artist.objects.filter(name="Simon").exists() + assert not Artist.objects.filter(name="Garfunkel").exists() + + +@pytest.mark.django_db +def test_reconcile_command_dry_run_makes_no_changes(): + user = get_user_model().objects.create(email="dry@example.com") + simon = Artist.objects.create(name="Simon") + garfunkel = Artist.objects.create(name="Garfunkel") + track = Track.objects.create(title="Bridge over Troubled Water") + track.artists.add(simon, garfunkel) + Scrobble.objects.create( + track=track, + media_type="Track", + user=user, + log={ + "raw_data": { + "artist": "Simon & Garfunkel", + "musicbrainz_artist_id": "grp-1", + } + }, + ) + + call_command("reconcile_split_artists") + + track.refresh_from_db() + assert sorted(track.artists.all().values_list("name", flat=True)) == [ + "Garfunkel", + "Simon", + ] + assert Artist.objects.filter(name="Simon").exists() + assert Artist.objects.filter(name="Garfunkel").exists() + + +@pytest.mark.django_db +@patch("music.utils.lookup_artist_from_mb", return_value={}) +def test_reconcile_command_leaves_genuine_collab_alone(mock_lookup): + user = get_user_model().objects.create(email="collab@example.com") + matt = Artist.objects.create(name="Matt Sweeney") + bonnie = Artist.objects.create(name="Bonnie Prince Billy") + track = Track.objects.create(title="My Morning Song") + track.artists.add(matt, bonnie) + Scrobble.objects.create( + track=track, + media_type="Track", + user=user, + log={"raw_data": {"artist": "Matt Sweeney & Bonnie Prince Billy"}}, + ) + + call_command("reconcile_split_artists", "--commit") + + track.refresh_from_db() + assert sorted(track.artists.all().values_list("name", flat=True)) == [ + "Bonnie Prince Billy", + "Matt Sweeney", + ] diff --git a/vrobbler/apps/music/management/commands/reconcile_split_artists.py b/vrobbler/apps/music/management/commands/reconcile_split_artists.py new file mode 100644 index 0000000..86a0763 --- /dev/null +++ b/vrobbler/apps/music/management/commands/reconcile_split_artists.py @@ -0,0 +1,170 @@ +import logging +import time + +from django.core.management.base import BaseCommand +from django.db import transaction +from music.models import Album, Artist, Track +from music.utils import ( + get_raw_artist_data, + normalize_name, + resolve_artist_names, +) + +logger = logging.getLogger(__name__) + + +class Command(BaseCommand): + help = ( + "Reconcile track artists against the raw artist data stored in " + "scrobble logs from Mopidy/Jellyfin webhooks. Repairs tracks whose " + "artists were wrongly split (e.g. 'Simon & Garfunkel' stored as two " + "artists) and deletes any artists left with no references." + ) + + def add_arguments(self, parser): + parser.add_argument( + "--commit", + action="store_true", + help="Commit changes to the database", + ) + parser.add_argument( + "--batch-size", + type=int, + default=100, + help="Number of scrobbles to process per batch (default: 100)", + ) + + def _resolve_names( + self, cache: dict, raw_artist: str, artist_mbid: str + ) -> list[str]: + key = (normalize_name(raw_artist), artist_mbid) + if key not in cache: + cache[key] = resolve_artist_names(raw_artist, artist_mbid) + return cache[key] + + def _resolve_artists( + self, cache: dict, expected_names: list[str], track + ) -> list[Artist]: + artists = [] + for name in expected_names: + if name not in cache: + cache[name] = Artist.objects.filter(name=name).first() + if not cache[name]: + cache[name] = Artist.find_or_create(name, track_name=track.title) + if cache[name]: + artists.append(cache[name]) + return artists + + def _artist_is_orphan(self, artist: Artist) -> bool: + if Track.objects.filter(artist_fk=artist).exists(): + return False + if Track.objects.filter(artists=artist).exists(): + return False + if Album.objects.filter(album_artist=artist).exists(): + return False + if Album.objects.filter(artists=artist).exists(): + return False + return True + + def handle(self, *args, **options): + from scrobbles.models import Scrobble + + commit = options["commit"] + batch_size = options["batch_size"] + + if not commit: + self.stdout.write( + "Dry run — no changes will be saved. Use --commit to apply." + ) + + qs = ( + Scrobble.objects.filter(media_type=Scrobble.MediaType.TRACK) + .exclude(log__isnull=True) + .exclude(log={}) + .order_by("id") + ) + + scrobble_ids = list(qs.values_list("pk", flat=True)) + self.stdout.write(f"Processing {len(scrobble_ids)} track scrobbles") + + resolution_cache = {} + artist_cache = {} + reconciled = 0 + orphaned_ids = set() + + for batch_num, offset in enumerate(range(0, len(scrobble_ids), batch_size)): + batch_pks = scrobble_ids[offset : offset + batch_size] + with transaction.atomic(): + batch = ( + Scrobble.objects.filter(pk__in=batch_pks) + .select_related("track") + .prefetch_related("track__artists") + ) + for scrobble in batch.iterator(chunk_size=batch_size): + track = scrobble.track + if not track: + continue + + raw_data = scrobble.log.get("raw_data") + if not raw_data: + continue + + raw_artist, _, raw_artist_mbid = get_raw_artist_data(raw_data) + if not raw_artist: + continue + + expected_names = self._resolve_names( + resolution_cache, raw_artist, raw_artist_mbid or "" + ) + stored_names = [ + n for n in track.artists.all().values_list("name", flat=True) + ] + if track.artist_fk and track.artist_fk.name not in stored_names: + stored_names.append(track.artist_fk.name) + + expected = {normalize_name(n) for n in expected_names} + stored = {normalize_name(n) for n in stored_names} + if expected == stored: + continue + + reconciled += 1 + self.stdout.write( + f" Track '{track.title}' artists {stored_names} -> " + f"{expected_names} (raw: '{raw_artist}')" + ) + + if commit: + artists = self._resolve_artists( + artist_cache, expected_names, track + ) + if not artists: + continue + old_artist_ids = set( + track.artists.all().values_list("id", flat=True) + ) + track.artists.set(artists) + track.artist_fk = artists[0] + track.save(update_fields=["artist_fk"]) + track.tags.add("artist-reconciled") + orphaned_ids.update(old_artist_ids - {a.id for a in artists}) + + self.stdout.write( + f" Batch {batch_num + 1}: {offset + len(batch_pks)}/{len(scrobble_ids)} processed, " + f"{reconciled} reconciled so far" + ) + time.sleep(1) + + deleted = 0 + if commit and orphaned_ids: + for artist_id in sorted(orphaned_ids): + artist = Artist.objects.filter(id=artist_id).first() + if artist and self._artist_is_orphan(artist): + artist.delete() + deleted += 1 + self.stdout.write(f" Deleted orphaned artist '{artist.name}'") + + self.stdout.write( + f"\nResults (commit={commit}):\n" + f" Tracks reconciled: {reconciled}\n" + f" Orphaned artists deleted: {deleted}" + ) diff --git a/vrobbler/apps/music/management/commands/report_mismatched_metadata.py b/vrobbler/apps/music/management/commands/report_mismatched_metadata.py index 8338560..cbccba1 100644 --- a/vrobbler/apps/music/management/commands/report_mismatched_metadata.py +++ b/vrobbler/apps/music/management/commands/report_mismatched_metadata.py @@ -2,46 +2,16 @@ import csv import logging from django.core.management.base import BaseCommand +from music.utils import ( + album_mismatch, + artist_mismatch, + get_artist_source, + get_raw_artist_data, +) logger = logging.getLogger(__name__) -def _get_source(raw_data): - if "Artist" in raw_data: - return "Jellyfin" - if "artist" in raw_data: - return "Mopidy" - return None - - -def _get_raw_values(raw_data, source): - if source == "Jellyfin": - return raw_data.get("Artist", ""), raw_data.get("Album", "") - return raw_data.get("artist", ""), raw_data.get("album", "") - - -def _normalize(name): - return name.strip().casefold() - - -def _artist_mismatch(raw_artist, track_artist_names): - if not raw_artist or not track_artist_names: - return False - track_names = [_normalize(n) for n in track_artist_names.split(" / ")] - raw = _normalize(raw_artist) - if raw in track_names: - return False - if raw == _normalize(track_artist_names): - return False - return True - - -def _album_mismatch(raw_album, track_album_name): - if not raw_album or not track_album_name: - return False - return _normalize(raw_album) != _normalize(track_album_name) - - class Command(BaseCommand): help = ( "Outputs a CSV of track IDs where raw metadata from scrobble logs " @@ -80,26 +50,26 @@ class Command(BaseCommand): if not raw_data: continue - source = _get_source(raw_data) + source = get_artist_source(raw_data) if not source: continue - raw_artist, raw_album = _get_raw_values(raw_data, source) + raw_artist, raw_album, _ = get_raw_artist_data(raw_data) if not raw_artist and not raw_album: continue - track_artist_names = " / ".join( - track.artists.all().values_list("name", flat=True) - ) + track_artist_names = [ + name for name in track.artists.all().values_list("name", flat=True) + ] track_album_name = track.album.name if track.album else "" - if _artist_mismatch(raw_artist, track_artist_names) or _album_mismatch( + if artist_mismatch(raw_artist, track_artist_names) or album_mismatch( raw_album, track_album_name ): rows.append( { "track_id": track.id, - "track_artist_name": track_artist_names, + "track_artist_name": " / ".join(track_artist_names), "track_album_name": track_album_name, "raw_artist": raw_artist, "raw_album": raw_album, @@ -121,7 +91,5 @@ class Command(BaseCommand): writer.writerows(rows) self.stdout.write( - self.style.SUCCESS( - f"Wrote {len(rows)} mismatched track(s) to {file_path}" - ) + self.style.SUCCESS(f"Wrote {len(rows)} mismatched track(s) to {file_path}") ) diff --git a/vrobbler/apps/music/models.py b/vrobbler/apps/music/models.py index c6677c9..9952aa5 100644 --- a/vrobbler/apps/music/models.py +++ b/vrobbler/apps/music/models.py @@ -654,10 +654,10 @@ class Track(ScrobblableMixin): further enrichment; otherwise the track is created and stamped with the provided mbids, and async enrichment can pick it up later via :meth:`fix_metadata`.""" - from music.utils import parse_artist_names + from music.utils import resolve_artist_names if artist_names is None and artist_name: - artist_names = parse_artist_names(artist_name) + artist_names = resolve_artist_names(artist_name, artist_mbid or "") if not artist_names: artist_names = [] @@ -757,8 +757,6 @@ class Track(ScrobblableMixin): This is safe to call off the request path (e.g. from the async webhook enrichment task), where it can afford slower lookups and subtler matching rules.""" - from music.utils import parse_artist_names - if "musicbrainz-enriched" in self.tags.names() and not force_update: logger.info( f"Track {self} already enriched, skipping", @@ -766,9 +764,7 @@ class Track(ScrobblableMixin): ) return True - artist_name_str = "" - if self.artist: - artist_name_str = " & ".join(parse_artist_names(str(self.artist))) + artist_name_str = str(self.artist) if self.artist else "" album_name = "" if self.primary_album: album_name = self.primary_album.name diff --git a/vrobbler/apps/music/utils.py b/vrobbler/apps/music/utils.py index 08fe2dc..81e3f39 100644 --- a/vrobbler/apps/music/utils.py +++ b/vrobbler/apps/music/utils.py @@ -3,6 +3,7 @@ import re from django.db import IntegrityError, models, transaction from music.constants import VARIOUS_ARTIST_DICT +from music.musicbrainz import lookup_artist_from_mb logger = logging.getLogger(__name__) @@ -35,6 +36,31 @@ def parse_artist_names(name: str) -> list[str]: return [name] +def resolve_artist_names(name: str, artist_mbid: str = "") -> list[str]: + """Resolve an artist string into one or more artist names. + + Prefers treating the full string as a single artist. An authoritative + MusicBrainz artist id (e.g. from a webhook) or an exact match against a + known single artist means we keep the name literal -- so acts like + "Simon & Garfunkel" are never pulled apart. Splitting on ' & ' (for + genuine collaborations) is only a last resort. + """ + name = clean_artist_name(name) + if artist_mbid: + return [name] + + from music.models import Artist + + if Artist.objects.filter(name=name).exists(): + return [name] + + result = lookup_artist_from_mb(name) + if result and result.get("name", "").strip().casefold() == name.casefold(): + return [name] + + return parse_artist_names(name) + + def get_or_create_various_artists() -> "Artist": from music.models import Artist @@ -121,3 +147,53 @@ def condense_albums(commit: bool = False): Track.objects.filter(scrobble__isnull=True).delete() return len(set(processed_ids)) + + +def get_artist_source(raw_data: dict) -> str: + """Return the provider that produced raw webhook data ('Jellyfin'/'Mopidy').""" + if "Artist" in raw_data: + return "Jellyfin" + if "artist" in raw_data: + return "Mopidy" + return "" + + +def get_raw_artist_data(raw_data: dict) -> tuple[str, str, str]: + """Extract (artist_name, album_name, artist_mbid) from raw webhook data.""" + source = get_artist_source(raw_data) + if source == "Jellyfin": + return ( + raw_data.get("Artist", ""), + raw_data.get("Album", ""), + raw_data.get("Provider_musicbrainzartist", ""), + ) + if source == "Mopidy": + return ( + raw_data.get("artist", ""), + raw_data.get("album", ""), + raw_data.get("musicbrainz_artist_id", ""), + ) + return "", "", "" + + +def normalize_name(name: str) -> str: + return name.strip().casefold() + + +def artist_mismatch(raw_artist: str, track_artist_names: list[str]) -> bool: + """True when a raw artist string doesn't match the stored track artists.""" + if not raw_artist or not track_artist_names: + return False + track_names = {normalize_name(n) for n in track_artist_names} + raw = normalize_name(raw_artist) + if raw in track_names: + return False + if raw == normalize_name(" / ".join(track_artist_names)): + return False + return True + + +def album_mismatch(raw_album: str, track_album_name: str) -> bool: + if not raw_album or not track_album_name: + return False + return normalize_name(raw_album) != normalize_name(track_album_name)