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

2020-06-23 11:59:30 +02:00 · 2020-06-23 11:59:30 +02:00 · 0c05ac6512
commit 0c05ac6512
--- a/api/funkwhale_api/music/metadata.py
+++ b/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"]]
--- a/api/funkwhale_api/music/tasks.py
+++ b/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()
--- a/api/tests/music/test.m4a
+++ b/api/tests/music/test.m4a
--- a/api/tests/music/test.mp3
+++ b/api/tests/music/test.mp3
--- a/api/tests/music/test.ogg
+++ b/api/tests/music/test.ogg
--- a/api/tests/music/test.opus
+++ b/api/tests/music/test.opus
--- a/api/tests/music/test_metadata.py
+++ b/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",
--- a/api/tests/music/test_tasks.py
+++ b/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",
--- a/changes/changelog.d/1104.bugfix
+++ b/changes/changelog.d/1104.bugfix
@ -0,0 +1 @@
+Fixed invalid metadata when importing multi-artists tracks/albums (#1104)
				`@ -0,0 +1 @@`
				`Fixed invalid metadata when importing multi-artists tracks/albums (#1104)`