From 0c05ac65129efa7091676b3c52b253269f49646b Mon Sep 17 00:00:00 2001 From: Agate Date: Tue, 23 Jun 2020 11:59:30 +0200 Subject: [PATCH] Fix #1104: invalid metadata when importing multi-artists tracks/albums --- api/funkwhale_api/music/metadata.py | 29 +++++++++-- api/funkwhale_api/music/tasks.py | 77 ++++++++++++++++------------ api/tests/music/test.m4a | Bin 51512 -> 51512 bytes api/tests/music/test.mp3 | Bin 297745 -> 297745 bytes api/tests/music/test.ogg | Bin 15918 -> 15918 bytes api/tests/music/test.opus | Bin 15643 -> 15643 bytes api/tests/music/test_metadata.py | 17 ++++-- api/tests/music/test_tasks.py | 14 +++++ changes/changelog.d/1104.bugfix | 1 + 9 files changed, 99 insertions(+), 39 deletions(-) create mode 100644 changes/changelog.d/1104.bugfix diff --git a/api/funkwhale_api/music/metadata.py b/api/funkwhale_api/music/metadata.py index 105bb6fe6..6a9847e44 100644 --- 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"]] diff --git a/api/funkwhale_api/music/tasks.py b/api/funkwhale_api/music/tasks.py index de2d00315..e44888a1f 100644 --- 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() diff --git a/api/tests/music/test.m4a b/api/tests/music/test.m4a index 24c49c2db1b09724aff7608663d6abd8f7b0748f..4b8f919b737634d8b47ac9c0bd22ac6a064a64b3 100644 GIT binary patch delta 66 zcmdlniFwB)<_%WvjNO~9-PbWO=1*?)P!4E{*x2gWG6c?^K4dUIh^0jRnpE?!U)7nK+L?ItAvGJ900^x7!Lpd delta 58 zcmbO@O=#jYp$&~plg}}$Z7yO8V`TK7+{h+7nU{@cvpdV-{K*A{3e8+4?OY{{K+FWh L%-gw2SlGn@Npunz diff --git a/api/tests/music/test.ogg b/api/tests/music/test.ogg index 7d1f523dc4e4c44c011c2aa8d8ee2794f6d51f5f..70547f63e58e27a736d11e598e3332d84b92f1a9 100644 GIT binary patch delta 192 zcmZ2iv#w@B01M+KiNc8?rzZ2UicgkfJTf_(>5ix%0|P^ltBq-{j_ zo|y fh-Yv}@Z^U~nn1USPoB$CxLK9;7xQLo&d(+Qjz>0Z delta 194 zcmZ2iv#w@B0E-a+1(S&(rzV>+9+~`#$!4-Sv$~of0|P^ltBq-{hv zFHq3O(aF`v*0;1czaTYl@=wM)d^|wW5dRQI9~aMH=ite`%#w;AArQ|m)Yr*1$kq^~ z5v(L6$kEw5*w$q7GG?2}qAUf9B0w3(pb*dCkYHQal(NL46b1L9%+z#!kPgofAJ@qa h%yN@CS$HPzWa*eJ$0)wpk@Xkz1z`)?_@9XR87h>y?nv;{SP?C{alscJ-@rVj9P|nBE z$<@c!x3oCFAT^I4DB$84;%aMTU}&UkV4-VhI@zD;&g9RG-hpC3xgb{`SI1!2;1I`< z&|usAw6x6R%)}fXpiGE=h@+2-XRvdyts#gHV)}*pI=Kef8iJhQ>+0g^XzP-ho>`KZ oqu`sGl9@O;iFq%h!DIy%@yVB%WG0)l?B2}D`jdIH9OqwS04Z-eH~;_u delta 201 zcmbPTHM?qpE6cZcHj^iM?wmZAMSQXW<62!Y1_p*8S07i$VAtRf$B@uq+x)b&%;e0( z9DbmTi(`nZt&xGDk*+{DRcH$&5^Q_;`S#A^stb zJ}#cY&cTxtm?Z^3LLi=BsIQZ2kgeh5O6I*1B0xdMpb*dCkYHQal(NL46b1L9%+&PB tSuDH7g@KaJ{=UAhej&CVsX00M3MCn-MX8%rS${H5PGFVUEXVoR7yxS9I)wlL diff --git a/api/tests/music/test_metadata.py b/api/tests/music/test_metadata.py index d46853ea0..28c39dd59 100644 --- 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", diff --git a/api/tests/music/test_tasks.py b/api/tests/music/test_tasks.py index 989382d79..6aecd35fe 100644 --- 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", diff --git a/changes/changelog.d/1104.bugfix b/changes/changelog.d/1104.bugfix new file mode 100644 index 000000000..23e765a75 --- /dev/null +++ b/changes/changelog.d/1104.bugfix @@ -0,0 +1 @@ +Fixed invalid metadata when importing multi-artists tracks/albums (#1104)