Merge branch '1104-featuring' into 'develop'

Fix #1104: invalid metadata when importing multi-artists tracks/albums

Closes #1104

See merge request funkwhale/funkwhale!1152

api/funkwhale_api/music/metadata.py

@@ -189,6 +189,7 @@ CONF = {
             "disc_number": {"field": "DISCNUMBER"},
             "title": {},
             "artist": {},
+            "artists": {},
             "album_artist": {"field": "albumartist"},
             "album": {},
             "date": {"field": "date"},
@@ -213,6 +214,7 @@ CONF = {
             "disc_number": {"field": "DISCNUMBER"},
             "title": {},
             "artist": {},
+            "artists": {},
             "album_artist": {"field": "albumartist"},
             "album": {},
             "date": {"field": "date"},
@@ -237,6 +239,7 @@ CONF = {
             "disc_number": {"field": "DISCNUMBER"},
             "title": {},
             "artist": {},
+            "artists": {},
             "album_artist": {"field": "albumartist"},
             "album": {},
             "date": {"field": "date"},
@@ -258,6 +261,7 @@ CONF = {
             "disc_number": {"field": "TPOS"},
             "title": {"field": "TIT2"},
             "artist": {"field": "TPE1"},
+            "artists": {"field": "ARTISTS"},
             "album_artist": {"field": "TPE2"},
             "album": {"field": "TALB"},
             "date": {"field": "TDRC"},
@@ -280,6 +284,7 @@ CONF = {
             "disc_number": {"field": "disk", "to_application": get_mp4_position},
             "title": {"field": "©nam"},
             "artist": {"field": "©ART"},
+            "artists": {"field": "----:com.apple.iTunes:ARTISTS"},
             "album_artist": {"field": "aART"},
             "album": {"field": "©alb"},
             "date": {"field": "©day"},
@@ -308,6 +313,7 @@ CONF = {
             "disc_number": {"field": "discnumber"},
             "title": {},
             "artist": {},
+            "artists": {},
             "album_artist": {"field": "albumartist"},
             "album": {},
             "date": {"field": "date"},
@@ -468,9 +474,17 @@ class ArtistField(serializers.Field):
 
     def get_value(self, data):
         if self.for_album:
-            keys = [("names", "album_artist"), ("mbids", "musicbrainz_albumartistid")]
+            keys = [
+                ("artists", "artists"),
+                ("names", "album_artist"),
+                ("mbids", "musicbrainz_albumartistid"),
+            ]
         else:
-            keys = [("names", "artist"), ("mbids", "musicbrainz_artistid")]
+            keys = [
+                ("artists", "artists"),
+                ("names", "artist"),
+                ("mbids", "musicbrainz_artistid"),
+            ]
 
         final = {}
         for field, key in keys:
@@ -499,7 +513,16 @@ class ArtistField(serializers.Field):
 
         # now, we split on artist names, using the same separator as the one used
         # by mbids, if any
-        if used_separator and mbids:
+        names = []
+
+        if data.get("artists", None):
+            for separator in separators:
+                if separator in data["artists"]:
+                    names = [n.strip() for n in data["artists"].split(separator)]
+                    break
+            if not names:
+                names = [data["artists"]]
+        elif used_separator and mbids:
             names = [n.strip() for n in data["names"].split(used_separator)]
         else:
             names = [data["names"]]

api/funkwhale_api/music/tasks.py

@@ -517,43 +517,15 @@ def _get_track(data, attributed_to=None, **forced_values):
             pass
 
     # get / create artist and album artist
+    artists = getter(data, "artists", default=[])
     if "artist" in forced_values:
         artist = forced_values["artist"]
     else:
-        artists = getter(data, "artists", default=[])
         artist_data = artists[0]
-        artist_mbid = artist_data.get("mbid", None)
-        artist_fid = artist_data.get("fid", None)
-        artist_name = truncate(artist_data["name"], models.MAX_LENGTHS["ARTIST_NAME"])
-
-        if artist_mbid:
-            query = Q(mbid=artist_mbid)
-        else:
-            query = Q(name__iexact=artist_name)
-        if artist_fid:
-            query |= Q(fid=artist_fid)
-        defaults = {
-            "name": artist_name,
-            "mbid": artist_mbid,
-            "fid": artist_fid,
-            "from_activity_id": from_activity_id,
-            "attributed_to": artist_data.get("attributed_to", attributed_to),
-        }
-        if artist_data.get("fdate"):
-            defaults["creation_date"] = artist_data.get("fdate")
-
-        artist, created = get_best_candidate_or_create(
-            models.Artist, query, defaults=defaults, sort_fields=["mbid", "fid"]
+        artist = get_artist(
+            artist_data, attributed_to=attributed_to, from_activity_id=from_activity_id
         )
-        if created:
-            tags_models.add_tags(artist, *artist_data.get("tags", []))
-            common_utils.attach_content(
-                artist, "description", artist_data.get("description")
-            )
-            common_utils.attach_file(
-                artist, "attachment_cover", artist_data.get("cover_data")
-            )
-
+        artist_name = artist.name
     if "album" in forced_values:
         album = forced_values["album"]
     else:
@@ -695,6 +667,12 @@ def _get_track(data, attributed_to=None, **forced_values):
     if track_fid:
         query |= Q(fid=track_fid)
 
+    if album and len(artists) > 1:
+        # we use the second artist to preserve featuring information
+        artist = artist = get_artist(
+            artists[1], attributed_to=attributed_to, from_activity_id=from_activity_id
+        )
+
     defaults = {
         "title": track_title,
         "album": album,
@@ -726,6 +704,41 @@ def _get_track(data, attributed_to=None, **forced_values):
     return track
 
 
+def get_artist(artist_data, attributed_to, from_activity_id):
+    artist_mbid = artist_data.get("mbid", None)
+    artist_fid = artist_data.get("fid", None)
+    artist_name = truncate(artist_data["name"], models.MAX_LENGTHS["ARTIST_NAME"])
+
+    if artist_mbid:
+        query = Q(mbid=artist_mbid)
+    else:
+        query = Q(name__iexact=artist_name)
+    if artist_fid:
+        query |= Q(fid=artist_fid)
+    defaults = {
+        "name": artist_name,
+        "mbid": artist_mbid,
+        "fid": artist_fid,
+        "from_activity_id": from_activity_id,
+        "attributed_to": artist_data.get("attributed_to", attributed_to),
+    }
+    if artist_data.get("fdate"):
+        defaults["creation_date"] = artist_data.get("fdate")
+
+    artist, created = get_best_candidate_or_create(
+        models.Artist, query, defaults=defaults, sort_fields=["mbid", "fid"]
+    )
+    if created:
+        tags_models.add_tags(artist, *artist_data.get("tags", []))
+        common_utils.attach_content(
+            artist, "description", artist_data.get("description")
+        )
+        common_utils.attach_file(
+            artist, "attachment_cover", artist_data.get("cover_data")
+        )
+    return artist
+
+
 @receiver(signals.upload_import_status_updated)
 def broadcast_import_status_update_to_owner(old_status, new_status, upload, **kwargs):
     user = upload.library.actor.get_user()

api/tests/music/test_metadata.py

@@ -17,6 +17,7 @@ DATA_DIR = os.path.dirname(os.path.abspath(__file__))
         ("title", "Peer Gynt Suite no. 1, op. 46: I. Morning"),
         ("artist", "Edvard Grieg"),
         ("album_artist", "Edvard Grieg; Musopen Symphony Orchestra"),
+        ("artists", "Edvard Grieg; Musopen Symphony Orchestra"),
         ("album", "Peer Gynt Suite no. 1, op. 46"),
         ("date", "2012-08-15"),
         ("position", "1"),
@@ -48,6 +49,7 @@ def test_can_get_metadata_all():
         "title": "Peer Gynt Suite no. 1, op. 46: I. Morning",
         "artist": "Edvard Grieg",
         "album_artist": "Edvard Grieg; Musopen Symphony Orchestra",
+        "artists": "Edvard Grieg; Musopen Symphony Orchestra",
         "album": "Peer Gynt Suite no. 1, op. 46",
         "date": "2012-08-15",
         "position": "1",
@@ -70,6 +72,7 @@ def test_can_get_metadata_all():
         ("title", "Peer Gynt Suite no. 1, op. 46: I. Morning"),
         ("artist", "Edvard Grieg"),
         ("album_artist", "Edvard Grieg; Musopen Symphony Orchestra"),
+        ("artists", "Edvard Grieg; Musopen Symphony Orchestra"),
         ("album", "Peer Gynt Suite no. 1, op. 46"),
         ("date", "2012-08-15"),
         ("position", "1"),
@@ -126,6 +129,7 @@ def test_can_get_metadata_from_ogg_theora_file(field, value):
         ("title", "Bend"),
         ("artist", "Binärpilot"),
         ("album_artist", "Binärpilot"),
+        ("artists", "Binärpilot; Another artist"),
         ("album", "You Can't Stop Da Funk"),
         ("date", "2006-02-07"),
         ("position", "2/4"),
@@ -202,6 +206,7 @@ def test_can_get_metadata_from_flac_file(field, value):
         ("title", "Peer Gynt Suite no. 1, op. 46: I. Morning"),
         ("artist", "Edvard Grieg"),
         ("album_artist", "Edvard Grieg; Musopen Symphony Orchestra"),
+        ("artists", "Edvard Grieg; Musopen Symphony Orchestra"),
         ("album", "Peer Gynt Suite no. 1, op. 46"),
         ("date", "2012-08-15"),
         ("position", 1),
@@ -283,7 +288,8 @@ def test_metadata_fallback_ogg_theora(mocker):
                     {
                         "name": "Binärpilot",
                         "mbid": uuid.UUID("9c6bddde-6228-4d9f-ad0d-03f6fcb19e13"),
-                    }
+                    },
+                    {"name": "Another artist", "mbid": None},
                 ],
                 "album": {
                     "title": "You Can't Stop Da Funk",
@@ -293,7 +299,8 @@ def test_metadata_fallback_ogg_theora(mocker):
                         {
                             "name": "Binärpilot",
                             "mbid": uuid.UUID("9c6bddde-6228-4d9f-ad0d-03f6fcb19e13"),
-                        }
+                        },
+                        {"name": "Another artist", "mbid": None},
                     ],
                 },
                 "position": 2,
@@ -313,7 +320,8 @@ def test_metadata_fallback_ogg_theora(mocker):
                     {
                         "name": "Edvard Grieg",
                         "mbid": uuid.UUID("013c8e5b-d72a-4cd3-8dee-6c64d6125823"),
-                    }
+                    },
+                    {"name": "Musopen Symphony Orchestra", "mbid": None},
                 ],
                 "album": {
                     "title": "Peer Gynt Suite no. 1, op. 46",
@@ -347,7 +355,8 @@ def test_metadata_fallback_ogg_theora(mocker):
                     {
                         "name": "Edvard Grieg",
                         "mbid": uuid.UUID("013c8e5b-d72a-4cd3-8dee-6c64d6125823"),
-                    }
+                    },
+                    {"name": "Musopen Symphony Orchestra", "mbid": None},
                 ],
                 "album": {
                     "title": "Peer Gynt Suite no. 1, op. 46",

api/tests/music/test_tasks.py

@@ -143,6 +143,20 @@ def test_can_create_track_from_file_metadata_description(factories):
     assert track.description.content_type == "text/plain"
 
 
+def test_can_create_track_from_file_metadata_use_featuring(factories):
+    metadata = {
+        "title": "Whole Lotta Love",
+        "position": 1,
+        "disc_number": 1,
+        "description": {"text": "hello there", "content_type": "text/plain"},
+        "album": {"title": "Test album"},
+        "artists": [{"name": "Santana"}, {"name": "Anatnas"}],
+    }
+    track = tasks.get_track_from_import_metadata(metadata)
+
+    assert track.artist.name == "Anatnas"
+
+
 def test_can_create_track_from_file_metadata_mbid(factories, mocker):
     metadata = {
         "title": "Test track",

changes/changelog.d/1104.bugfix

@@ -0,0 +1 @@
+Fixed invalid metadata when importing multi-artists tracks/albums (#1104)