[music] Resolve artist identity before splitting (fd86a11a)

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

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",
+    ]

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}"
+        )

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_artist_names = [
-                track.artists.all().values_list("name", flat=True)
+                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(
+            self.style.SUCCESS(f"Wrote {len(rows)} mismatched track(s) to {file_path}")
-                f"Wrote {len(rows)} mismatched track(s) to {file_path}"
-            )
         )

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 = ""
+        artist_name_str = str(self.artist) if self.artist else ""
-        if self.artist:
-            artist_name_str = " & ".join(parse_artist_names(str(self.artist)))
         album_name = ""
         if self.primary_album:
             album_name = self.primary_album.name

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)