diff --git a/api/funkwhale_api/music/metadata.py b/api/funkwhale_api/music/metadata.py index 7a105e432..3347dfb63 100644 --- a/api/funkwhale_api/music/metadata.py +++ b/api/funkwhale_api/music/metadata.py @@ -8,7 +8,8 @@ import mutagen.oggtheora import mutagen.oggvorbis import mutagen.flac -from django import forms +from rest_framework import serializers +from rest_framework.compat import Mapping logger = logging.getLogger(__name__) NODEFAULT = object() @@ -122,85 +123,23 @@ def get_mp3_recording_id(f, k): raise TagNotFound(k) -def convert_position(v): - try: - return int(v) - except ValueError: - # maybe the position is of the form "1/4" - pass - - try: - return int(v.split("/")[0]) - except (ValueError, AttributeError, IndexError): - pass - - -class FirstUUIDField(forms.UUIDField): - def to_python(self, value): - try: - # sometimes, Picard leaves two uuids in the field, separated - # by a slash or a ; - value = value.split(";")[0].split("/")[0].strip() - except (AttributeError, IndexError, TypeError): - pass - - return super().to_python(value) - - -def get_date(value): - ADDITIONAL_FORMATS = ["%Y-%d-%m %H:%M"] # deezer date format - try: - parsed = pendulum.parse(str(value)) - return datetime.date(parsed.year, parsed.month, parsed.day) - except pendulum.exceptions.ParserError: - pass - - for date_format in ADDITIONAL_FORMATS: - try: - parsed = datetime.datetime.strptime(value, date_format) - except ValueError: - continue - else: - return datetime.date(parsed.year, parsed.month, parsed.day) - - raise ParseError("{} cannot be parsed as a date".format(value)) - - -def split_and_return_first(separator): - def inner(v): - return v.split(separator)[0].strip() - - return inner - - -VALIDATION = { - "musicbrainz_artistid": FirstUUIDField(), - "musicbrainz_albumid": FirstUUIDField(), - "musicbrainz_recordingid": FirstUUIDField(), - "musicbrainz_albumartistid": FirstUUIDField(), -} +VALIDATION = {} CONF = { "OggOpus": { "getter": lambda f, k: f[k][0], "fields": { - "track_number": { - "field": "TRACKNUMBER", - "to_application": convert_position, - }, - "disc_number": {"field": "DISCNUMBER", "to_application": convert_position}, + "position": {"field": "TRACKNUMBER"}, + "disc_number": {"field": "DISCNUMBER"}, "title": {}, "artist": {}, - "album_artist": { - "field": "albumartist", - "to_application": split_and_return_first(";"), - }, + "album_artist": {"field": "albumartist"}, "album": {}, - "date": {"field": "date", "to_application": get_date}, + "date": {"field": "date"}, "musicbrainz_albumid": {}, "musicbrainz_artistid": {}, "musicbrainz_albumartistid": {}, - "musicbrainz_recordingid": {"field": "musicbrainz_trackid"}, + "mbid": {"field": "musicbrainz_trackid"}, "license": {}, "copyright": {}, }, @@ -208,23 +147,17 @@ CONF = { "OggVorbis": { "getter": lambda f, k: f[k][0], "fields": { - "track_number": { - "field": "TRACKNUMBER", - "to_application": convert_position, - }, - "disc_number": {"field": "DISCNUMBER", "to_application": convert_position}, + "position": {"field": "TRACKNUMBER"}, + "disc_number": {"field": "DISCNUMBER"}, "title": {}, "artist": {}, - "album_artist": { - "field": "albumartist", - "to_application": split_and_return_first(";"), - }, + "album_artist": {"field": "albumartist"}, "album": {}, - "date": {"field": "date", "to_application": get_date}, + "date": {"field": "date"}, "musicbrainz_albumid": {}, "musicbrainz_artistid": {}, "musicbrainz_albumartistid": {}, - "musicbrainz_recordingid": {"field": "musicbrainz_trackid"}, + "mbid": {"field": "musicbrainz_trackid"}, "license": {}, "copyright": {}, "pictures": { @@ -236,20 +169,17 @@ CONF = { "OggTheora": { "getter": lambda f, k: f[k][0], "fields": { - "track_number": { - "field": "TRACKNUMBER", - "to_application": convert_position, - }, - "disc_number": {"field": "DISCNUMBER", "to_application": convert_position}, + "position": {"field": "TRACKNUMBER"}, + "disc_number": {"field": "DISCNUMBER"}, "title": {}, "artist": {}, "album_artist": {"field": "albumartist"}, "album": {}, - "date": {"field": "date", "to_application": get_date}, + "date": {"field": "date"}, "musicbrainz_albumid": {"field": "MusicBrainz Album Id"}, "musicbrainz_artistid": {"field": "MusicBrainz Artist Id"}, "musicbrainz_albumartistid": {"field": "MusicBrainz Album Artist Id"}, - "musicbrainz_recordingid": {"field": "MusicBrainz Track Id"}, + "mbid": {"field": "MusicBrainz Track Id"}, "license": {}, "copyright": {}, }, @@ -258,20 +188,17 @@ CONF = { "getter": get_id3_tag, "clean_pictures": clean_id3_pictures, "fields": { - "track_number": {"field": "TRCK", "to_application": convert_position}, - "disc_number": {"field": "TPOS", "to_application": convert_position}, + "position": {"field": "TRCK"}, + "disc_number": {"field": "TPOS"}, "title": {"field": "TIT2"}, "artist": {"field": "TPE1"}, "album_artist": {"field": "TPE2"}, "album": {"field": "TALB"}, - "date": {"field": "TDRC", "to_application": get_date}, + "date": {"field": "TDRC"}, "musicbrainz_albumid": {"field": "MusicBrainz Album Id"}, "musicbrainz_artistid": {"field": "MusicBrainz Artist Id"}, "musicbrainz_albumartistid": {"field": "MusicBrainz Album Artist Id"}, - "musicbrainz_recordingid": { - "field": "UFID", - "getter": get_mp3_recording_id, - }, + "mbid": {"field": "UFID", "getter": get_mp3_recording_id}, "pictures": {}, "license": {"field": "WCOP"}, "copyright": {"field": "TCOP"}, @@ -281,20 +208,17 @@ CONF = { "getter": get_flac_tag, "clean_pictures": clean_flac_pictures, "fields": { - "track_number": { - "field": "tracknumber", - "to_application": convert_position, - }, - "disc_number": {"field": "discnumber", "to_application": convert_position}, + "position": {"field": "tracknumber"}, + "disc_number": {"field": "discnumber"}, "title": {}, "artist": {}, "album_artist": {"field": "albumartist"}, "album": {}, - "date": {"field": "date", "to_application": get_date}, + "date": {"field": "date"}, "musicbrainz_albumid": {}, "musicbrainz_artistid": {}, "musicbrainz_albumartistid": {}, - "musicbrainz_recordingid": {"field": "musicbrainz_trackid"}, + "mbid": {"field": "musicbrainz_trackid"}, "test": {}, "pictures": {}, "license": {}, @@ -304,7 +228,7 @@ CONF = { } ALL_FIELDS = [ - "track_number", + "position", "disc_number", "title", "artist", @@ -314,13 +238,13 @@ ALL_FIELDS = [ "musicbrainz_albumid", "musicbrainz_artistid", "musicbrainz_albumartistid", - "musicbrainz_recordingid", + "mbid", "license", "copyright", ] -class Metadata(object): +class Metadata(Mapping): def __init__(self, filething, kind=mutagen.File): self._file = kind(filething) if self._file is None: @@ -368,6 +292,21 @@ class Metadata(object): else: return self.fallback.get(key, default=default) + def all(self): + """ + Return a dict with all support metadata fields, if they are available + """ + final = {} + for field in self._conf["fields"]: + if field in ["pictures"]: + continue + value = self.get(field, None) + if value is None: + continue + final[field] = str(value) + + return final + def _get_from_self(self, key, default=NODEFAULT): try: field_conf = self._conf["fields"][key] @@ -390,25 +329,6 @@ class Metadata(object): v = field.to_python(v) return v - def all(self, ignore_parse_errors=True): - """ - Return a dict containing all metadata of the file - """ - - data = {} - for field in ALL_FIELDS: - try: - data[field] = self.get(field, None) - except (TagNotFound, forms.ValidationError): - data[field] = None - except ParseError as e: - if not ignore_parse_errors: - raise - logger.warning("Unparsable field {}: {}".format(field, str(e))) - data[field] = None - - return data - def get_picture(self, *picture_types): if not picture_types: raise ValueError("You need to request at least one picture type") @@ -430,3 +350,166 @@ class Metadata(object): for p in pictures: if p["type"] == ptype: return p + + def __getitem__(self, key): + return self.get(key) + + def __len__(self): + return 1 + + def __iter__(self): + for field in self._conf["fields"]: + yield field + + +class ArtistField(serializers.Field): + def __init__(self, *args, **kwargs): + self.for_album = kwargs.pop("for_album", False) + super().__init__(*args, **kwargs) + + def get_value(self, data): + if self.for_album: + keys = [("names", "album_artist"), ("mbids", "musicbrainz_albumartistid")] + else: + keys = [("names", "artist"), ("mbids", "musicbrainz_artistid")] + + final = {} + for field, key in keys: + final[field] = data.get(key, None) + + return final + + def to_internal_value(self, data): + # we have multiple values that can be separated by various separators + separators = [";", "/"] + # we get a list like that if tagged via musicbrainz + # ae29aae4-abfb-4609-8f54-417b1f4d64cc; 3237b5a8-ae44-400c-aa6d-cea51f0b9074; + raw_mbids = data["mbids"] + used_separator = None + mbids = [raw_mbids] + if raw_mbids: + for separator in separators: + if separator in raw_mbids: + used_separator = separator + mbids = [m.strip() for m in raw_mbids.split(separator)] + break + + # now, we split on artist names, using the same separator as the one used + # by mbids, if any + if used_separator and mbids: + names = [n.strip() for n in data["names"].split(used_separator)] + else: + names = [data["names"]] + + final = [] + for i, name in enumerate(names): + try: + mbid = mbids[i] + except IndexError: + mbid = None + artist = {"name": name, "mbid": mbid} + final.append(artist) + + field = serializers.ListField(child=ArtistSerializer(), min_length=1) + + return field.to_internal_value(final) + + +class AlbumField(serializers.Field): + def get_value(self, data): + return data + + def to_internal_value(self, data): + try: + title = data.get("album") + except TagNotFound: + raise serializers.ValidationError("Missing album tag") + final = { + "title": title, + "release_date": data.get("date", None), + "mbid": data.get("musicbrainz_albumid", None), + } + artists_field = ArtistField(for_album=True) + payload = artists_field.get_value(data) + artists = artists_field.to_internal_value(payload) + album_serializer = AlbumSerializer(data=final) + album_serializer.is_valid(raise_exception=True) + album_serializer.validated_data["artists"] = artists + return album_serializer.validated_data + + +class CoverDataField(serializers.Field): + def get_value(self, data): + return data + + def to_internal_value(self, data): + return data.get_picture("cover_front", "other") + + +class PermissiveDateField(serializers.CharField): + def to_internal_value(self, value): + if not value: + return None + value = super().to_internal_value(str(value)) + ADDITIONAL_FORMATS = [ + "%Y-%d-%m %H:%M", # deezer date format + "%Y-%W", # weird date format based on week number, see #718 + ] + + for date_format in ADDITIONAL_FORMATS: + try: + parsed = datetime.datetime.strptime(value, date_format) + except ValueError: + continue + else: + return datetime.date(parsed.year, parsed.month, parsed.day) + + try: + parsed = pendulum.parse(str(value)) + return datetime.date(parsed.year, parsed.month, parsed.day) + except pendulum.exceptions.ParserError: + pass + + return None + + +class ArtistSerializer(serializers.Serializer): + name = serializers.CharField() + mbid = serializers.UUIDField(required=False, allow_null=True) + + +class AlbumSerializer(serializers.Serializer): + title = serializers.CharField() + mbid = serializers.UUIDField(required=False, allow_null=True) + release_date = PermissiveDateField(required=False, allow_null=True) + + +class PositionField(serializers.CharField): + def to_internal_value(self, v): + v = super().to_internal_value(v) + if not v: + return v + + try: + return int(v) + except ValueError: + # maybe the position is of the form "1/4" + pass + + try: + return int(v.split("/")[0]) + except (ValueError, AttributeError, IndexError): + pass + + +class TrackMetadataSerializer(serializers.Serializer): + title = serializers.CharField() + position = PositionField(allow_null=True, required=False) + disc_number = PositionField(allow_null=True, required=False) + copyright = serializers.CharField(allow_null=True, required=False) + license = serializers.CharField(allow_null=True, required=False) + mbid = serializers.UUIDField(allow_null=True, required=False) + + album = AlbumField() + artists = ArtistField() + cover_data = CoverDataField() diff --git a/api/funkwhale_api/music/tasks.py b/api/funkwhale_api/music/tasks.py index 47cb4eb38..f64b7eed2 100644 --- a/api/funkwhale_api/music/tasks.py +++ b/api/funkwhale_api/music/tasks.py @@ -151,10 +151,11 @@ class UploadImportError(ValueError): super().__init__(code) -def fail_import(upload, error_code): +def fail_import(upload, error_code, detail=None, **fields): old_status = upload.import_status upload.import_status = "errored" - upload.import_details = {"error_code": error_code} + upload.import_details = {"error_code": error_code, "detail": detail} + upload.import_details.update(fields) upload.import_date = timezone.now() upload.save(update_fields=["import_details", "import_status", "import_date"]) @@ -182,19 +183,29 @@ def process_upload(upload): old_status = upload.import_status audio_file = upload.get_audio_file() additional_data = {} + + m = metadata.Metadata(audio_file) + try: + serializer = metadata.TrackMetadataSerializer(data=m) + serializer.is_valid() + except Exception: + fail_import(upload, "unknown_error") + raise + if not serializer.is_valid(): + detail = serializer.errors + try: + metadata_dump = m.all() + except Exception as e: + logger.warn("Cannot dump metadata for file %s: %s", audio_file, str(e)) + return fail_import( + upload, "invalid_metadata", detail=detail, file_metadata=metadata_dump + ) + + final_metadata = collections.ChainMap( + additional_data, serializer.validated_data, import_metadata + ) + additional_data["upload_source"] = upload.source try: - if not audio_file: - # we can only rely on user proveded data - final_metadata = import_metadata - else: - # we use user provided data and data from the file itself - m = metadata.Metadata(audio_file) - file_metadata = m.all() - final_metadata = collections.ChainMap( - additional_data, import_metadata, file_metadata - ) - additional_data["cover_data"] = m.get_picture("cover_front", "other") - additional_data["upload_source"] = upload.source track = get_track_from_import_metadata(final_metadata) except UploadImportError as e: return fail_import(upload, e.code) @@ -276,43 +287,45 @@ def federation_audio_track_to_metadata(payload): Given a valid payload as returned by federation.serializers.TrackSerializer.validated_data, returns a correct metadata payload for use with get_track_from_import_metadata. """ - musicbrainz_recordingid = payload.get("musicbrainzId") - musicbrainz_artistid = payload["artists"][0].get("musicbrainzId") - musicbrainz_albumartistid = payload["album"]["artists"][0].get("musicbrainzId") - musicbrainz_albumid = payload["album"].get("musicbrainzId") - new_data = { "title": payload["name"], - "album": payload["album"]["name"], - "track_number": payload.get("position") or 1, + "position": payload.get("position") or 1, "disc_number": payload.get("disc"), - "artist": payload["artists"][0]["name"], - "album_artist": payload["album"]["artists"][0]["name"], - "date": payload["album"].get("released"), "license": payload.get("license"), "copyright": payload.get("copyright"), - # musicbrainz - "musicbrainz_recordingid": str(musicbrainz_recordingid) - if musicbrainz_recordingid - else None, - "musicbrainz_artistid": str(musicbrainz_artistid) - if musicbrainz_artistid - else None, - "musicbrainz_albumartistid": str(musicbrainz_albumartistid) - if musicbrainz_albumartistid - else None, - "musicbrainz_albumid": str(musicbrainz_albumid) - if musicbrainz_albumid + "mbid": str(payload.get("musicbrainzId")) + if payload.get("musicbrainzId") else None, + "album": { + "title": payload["album"]["name"], + "fdate": payload["album"]["published"], + "fid": payload["album"]["id"], + "mbid": str(payload["album"]["musicbrainzId"]) + if payload["album"].get("musicbrainzId") + else None, + "release_date": payload["album"].get("released"), + "artists": [ + { + "fid": a["id"], + "name": a["name"], + "fdate": a["published"], + "mbid": str(a["musicbrainzId"]) if a.get("musicbrainzId") else None, + } + for a in payload["album"]["artists"] + ], + }, + "artists": [ + { + "fid": a["id"], + "name": a["name"], + "fdate": a["published"], + "mbid": str(a["musicbrainzId"]) if a.get("musicbrainzId") else None, + } + for a in payload["artists"] + ], # federation "fid": payload["id"], - "artist_fid": payload["artists"][0]["id"], - "album_artist_fid": payload["album"]["artists"][0]["id"], - "album_fid": payload["album"]["id"], "fdate": payload["published"], - "album_fdate": payload["album"]["published"], - "album_artist_fdate": payload["album"]["artists"][0]["published"], - "artist_fdate": payload["artists"][0]["published"], } cover = payload["album"].get("cover") if cover: @@ -405,8 +418,8 @@ def _get_track(data): return track from_activity_id = data.get("from_activity_id", None) - track_mbid = data.get("musicbrainz_recordingid", None) - album_mbid = data.get("musicbrainz_albumid", None) + track_mbid = data.get("mbid", None) + album_mbid = getter(data, "album", "mbid") track_fid = getter(data, "fid") query = None @@ -428,9 +441,12 @@ def _get_track(data): pass # get / create artist and album artist - artist_mbid = data.get("musicbrainz_artistid", None) - artist_fid = data.get("artist_fid", None) - artist_name = data["artist"] + artists = getter(data, "artists", default=[]) + artist = artists[0] + artist_mbid = artist.get("mbid", None) + artist_fid = artist.get("fid", None) + artist_name = artist["name"] + if artist_mbid: query = Q(mbid=artist_mbid) else: @@ -443,20 +459,22 @@ def _get_track(data): "fid": artist_fid, "from_activity_id": from_activity_id, } - if data.get("artist_fdate"): - defaults["creation_date"] = data.get("artist_fdate") + if artist.get("fdate"): + defaults["creation_date"] = artist.get("fdate") artist = get_best_candidate_or_create( models.Artist, query, defaults=defaults, sort_fields=["mbid", "fid"] )[0] - album_artist_name = data.get("album_artist") or artist_name + album_artists = getter(data, "album", "artists", default=artists) + album_artist = album_artists[0] + album_artist_name = album_artist.get("name") if album_artist_name == artist_name: album_artist = artist else: query = Q(name__iexact=album_artist_name) - album_artist_mbid = data.get("musicbrainz_albumartistid", None) - album_artist_fid = data.get("album_artist_fid", None) + album_artist_mbid = album_artist.get("mbid", None) + album_artist_fid = album_artist.get("fid", None) if album_artist_mbid: query |= Q(mbid=album_artist_mbid) if album_artist_fid: @@ -467,16 +485,17 @@ def _get_track(data): "fid": album_artist_fid, "from_activity_id": from_activity_id, } - if data.get("album_artist_fdate"): - defaults["creation_date"] = data.get("album_artist_fdate") + if album_artist.get("fdate"): + defaults["creation_date"] = album_artist.get("fdate") album_artist = get_best_candidate_or_create( models.Artist, query, defaults=defaults, sort_fields=["mbid", "fid"] )[0] # get / create album - album_title = data["album"] - album_fid = data.get("album_fid", None) + album = data["album"] + album_title = album["title"] + album_fid = album.get("fid", None) if album_mbid: query = Q(mbid=album_mbid) @@ -489,12 +508,12 @@ def _get_track(data): "title": album_title, "artist": album_artist, "mbid": album_mbid, - "release_date": data.get("date"), + "release_date": album.get("release_date"), "fid": album_fid, "from_activity_id": from_activity_id, } - if data.get("album_fdate"): - defaults["creation_date"] = data.get("album_fdate") + if album.get("fdate"): + defaults["creation_date"] = album.get("fdate") album = get_best_candidate_or_create( models.Album, query, defaults=defaults, sort_fields=["mbid", "fid"] @@ -502,10 +521,8 @@ def _get_track(data): # get / create track track_title = data["title"] - track_number = data.get("track_number", 1) - query = Q( - title__iexact=track_title, artist=artist, album=album, position=track_number - ) + position = data.get("position", 1) + query = Q(title__iexact=track_title, artist=artist, album=album, position=position) if track_mbid: query |= Q(mbid=track_mbid) if track_fid: @@ -515,7 +532,7 @@ def _get_track(data): "album": album, "mbid": track_mbid, "artist": artist, - "position": track_number, + "position": position, "disc_number": data.get("disc_number"), "fid": track_fid, "from_activity_id": from_activity_id, diff --git a/api/tests/music/test_metadata.py b/api/tests/music/test_metadata.py index f91f7545b..e2c180a63 100644 --- a/api/tests/music/test_metadata.py +++ b/api/tests/music/test_metadata.py @@ -11,45 +11,22 @@ from funkwhale_api.music import metadata DATA_DIR = os.path.dirname(os.path.abspath(__file__)) -def test_get_all_metadata_at_once(): - path = os.path.join(DATA_DIR, "test.ogg") - data = metadata.Metadata(path) - - expected = { - "title": "Peer Gynt Suite no. 1, op. 46: I. Morning", - "artist": "Edvard Grieg", - "album_artist": "Edvard Grieg", - "album": "Peer Gynt Suite no. 1, op. 46", - "date": datetime.date(2012, 8, 15), - "track_number": 1, - "disc_number": 1, - "musicbrainz_albumid": uuid.UUID("a766da8b-8336-47aa-a3ee-371cc41ccc75"), - "musicbrainz_recordingid": uuid.UUID("bd21ac48-46d8-4e78-925f-d9cc2a294656"), - "musicbrainz_artistid": uuid.UUID("013c8e5b-d72a-4cd3-8dee-6c64d6125823"), - "musicbrainz_albumartistid": uuid.UUID("013c8e5b-d72a-4cd3-8dee-6c64d6125823"), - "license": "Dummy license: http://creativecommons.org/licenses/by-sa/4.0/", - "copyright": "Someone", - } - - assert data.all() == expected - - @pytest.mark.parametrize( "field,value", [ ("title", "Peer Gynt Suite no. 1, op. 46: I. Morning"), ("artist", "Edvard Grieg"), - ("album_artist", "Edvard Grieg"), + ("album_artist", "Edvard Grieg; Musopen Symphony Orchestra"), ("album", "Peer Gynt Suite no. 1, op. 46"), - ("date", datetime.date(2012, 8, 15)), - ("track_number", 1), - ("disc_number", 1), - ("musicbrainz_albumid", uuid.UUID("a766da8b-8336-47aa-a3ee-371cc41ccc75")), - ("musicbrainz_recordingid", uuid.UUID("bd21ac48-46d8-4e78-925f-d9cc2a294656")), - ("musicbrainz_artistid", uuid.UUID("013c8e5b-d72a-4cd3-8dee-6c64d6125823")), + ("date", "2012-08-15"), + ("position", "1"), + ("disc_number", "1"), + ("musicbrainz_albumid", "a766da8b-8336-47aa-a3ee-371cc41ccc75"), + ("mbid", "bd21ac48-46d8-4e78-925f-d9cc2a294656"), + ("musicbrainz_artistid", "013c8e5b-d72a-4cd3-8dee-6c64d6125823"), ( "musicbrainz_albumartistid", - uuid.UUID("013c8e5b-d72a-4cd3-8dee-6c64d6125823"), + "013c8e5b-d72a-4cd3-8dee-6c64d6125823;5b4d7d2d-36df-4b38-95e3-a964234f520f", ), ("license", "Dummy license: http://creativecommons.org/licenses/by-sa/4.0/"), ("copyright", "Someone"), @@ -62,22 +39,44 @@ def test_can_get_metadata_from_ogg_file(field, value): assert data.get(field) == value +def test_can_get_metadata_all(): + path = os.path.join(DATA_DIR, "test.ogg") + data = metadata.Metadata(path) + + expected = { + "title": "Peer Gynt Suite no. 1, op. 46: I. Morning", + "artist": "Edvard Grieg", + "album_artist": "Edvard Grieg; Musopen Symphony Orchestra", + "album": "Peer Gynt Suite no. 1, op. 46", + "date": "2012-08-15", + "position": "1", + "disc_number": "1", + "musicbrainz_albumid": "a766da8b-8336-47aa-a3ee-371cc41ccc75", + "mbid": "bd21ac48-46d8-4e78-925f-d9cc2a294656", + "musicbrainz_artistid": "013c8e5b-d72a-4cd3-8dee-6c64d6125823", + "musicbrainz_albumartistid": "013c8e5b-d72a-4cd3-8dee-6c64d6125823;5b4d7d2d-36df-4b38-95e3-a964234f520f", + "license": "Dummy license: http://creativecommons.org/licenses/by-sa/4.0/", + "copyright": "Someone", + } + assert data.all() == expected + + @pytest.mark.parametrize( "field,value", [ ("title", "Peer Gynt Suite no. 1, op. 46: I. Morning"), ("artist", "Edvard Grieg"), - ("album_artist", "Edvard Grieg"), + ("album_artist", "Edvard Grieg; Musopen Symphony Orchestra"), ("album", "Peer Gynt Suite no. 1, op. 46"), - ("date", datetime.date(2012, 8, 15)), - ("track_number", 1), - ("disc_number", 1), - ("musicbrainz_albumid", uuid.UUID("a766da8b-8336-47aa-a3ee-371cc41ccc75")), - ("musicbrainz_recordingid", uuid.UUID("bd21ac48-46d8-4e78-925f-d9cc2a294656")), - ("musicbrainz_artistid", uuid.UUID("013c8e5b-d72a-4cd3-8dee-6c64d6125823")), + ("date", "2012-08-15"), + ("position", "1"), + ("disc_number", "1"), + ("musicbrainz_albumid", "a766da8b-8336-47aa-a3ee-371cc41ccc75"), + ("mbid", "bd21ac48-46d8-4e78-925f-d9cc2a294656"), + ("musicbrainz_artistid", "013c8e5b-d72a-4cd3-8dee-6c64d6125823"), ( "musicbrainz_albumartistid", - uuid.UUID("013c8e5b-d72a-4cd3-8dee-6c64d6125823"), + "013c8e5b-d72a-4cd3-8dee-6c64d6125823;5b4d7d2d-36df-4b38-95e3-a964234f520f", ), ("license", "Dummy license: http://creativecommons.org/licenses/by-sa/4.0/"), ("copyright", "Someone"), @@ -97,16 +96,13 @@ def test_can_get_metadata_from_opus_file(field, value): ("artist", "Die Toten Hosen"), ("album_artist", "Die Toten Hosen"), ("album", "Ballast der Republik"), - ("date", datetime.date(2012, 5, 4)), - ("track_number", 1), - ("disc_number", 1), - ("musicbrainz_albumid", uuid.UUID("1f0441ad-e609-446d-b355-809c445773cf")), - ("musicbrainz_recordingid", uuid.UUID("124d0150-8627-46bc-bc14-789a3bc960c8")), - ("musicbrainz_artistid", uuid.UUID("c3bc80a6-1f4a-4e17-8cf0-6b1efe8302f1")), - ( - "musicbrainz_albumartistid", - uuid.UUID("c3bc80a6-1f4a-4e17-8cf0-6b1efe8302f1"), - ), + ("date", "2012-05-04"), + ("position", "1/16"), + ("disc_number", "1/2"), + ("musicbrainz_albumid", "1f0441ad-e609-446d-b355-809c445773cf"), + ("mbid", "124d0150-8627-46bc-bc14-789a3bc960c8"), + ("musicbrainz_artistid", "c3bc80a6-1f4a-4e17-8cf0-6b1efe8302f1"), + ("musicbrainz_albumartistid", "c3bc80a6-1f4a-4e17-8cf0-6b1efe8302f1"), # somehow, I cannot successfully create an ogg theora file # with the proper license field # ("license", "Dummy license: http://creativecommons.org/licenses/by-sa/4.0/"), @@ -126,16 +122,13 @@ def test_can_get_metadata_from_ogg_theora_file(field, value): ("artist", "Binärpilot"), ("album_artist", "Binärpilot"), ("album", "You Can't Stop Da Funk"), - ("date", datetime.date(2006, 2, 7)), - ("track_number", 2), - ("disc_number", 1), - ("musicbrainz_albumid", uuid.UUID("ce40cdb1-a562-4fd8-a269-9269f98d4124")), - ("musicbrainz_recordingid", uuid.UUID("f269d497-1cc0-4ae4-a0c4-157ec7d73fcb")), - ("musicbrainz_artistid", uuid.UUID("9c6bddde-6228-4d9f-ad0d-03f6fcb19e13")), - ( - "musicbrainz_albumartistid", - uuid.UUID("9c6bddde-6228-4d9f-ad0d-03f6fcb19e13"), - ), + ("date", "2006-02-07"), + ("position", "2/4"), + ("disc_number", "1/1"), + ("musicbrainz_albumid", "ce40cdb1-a562-4fd8-a269-9269f98d4124"), + ("mbid", "f269d497-1cc0-4ae4-a0c4-157ec7d73fcb"), + ("musicbrainz_artistid", "9c6bddde-6228-4d9f-ad0d-03f6fcb19e13"), + ("musicbrainz_albumartistid", "9c6bddde-6228-4d9f-ad0d-03f6fcb19e13"), ("license", "https://creativecommons.org/licenses/by-nc-nd/2.5/"), ("copyright", "Someone"), ], @@ -144,7 +137,7 @@ def test_can_get_metadata_from_id3_mp3_file(field, value): path = os.path.join(DATA_DIR, "test.mp3") data = metadata.Metadata(path) - assert data.get(field) == value + assert str(data.get(field)) == value @pytest.mark.parametrize( @@ -170,16 +163,13 @@ def test_can_get_pictures(name): ("artist", "Nine Inch Nails"), ("album_artist", "Nine Inch Nails"), ("album", "The Slip"), - ("date", datetime.date(2008, 5, 5)), - ("track_number", 1), - ("disc_number", 1), - ("musicbrainz_albumid", uuid.UUID("12b57d46-a192-499e-a91f-7da66790a1c1")), - ("musicbrainz_recordingid", uuid.UUID("30f3f33e-8d0c-4e69-8539-cbd701d18f28")), - ("musicbrainz_artistid", uuid.UUID("b7ffd2af-418f-4be2-bdd1-22f8b48613da")), - ( - "musicbrainz_albumartistid", - uuid.UUID("b7ffd2af-418f-4be2-bdd1-22f8b48613da"), - ), + ("date", "2008-05-05"), + ("position", "1"), + ("disc_number", "1"), + ("musicbrainz_albumid", "12b57d46-a192-499e-a91f-7da66790a1c1"), + ("mbid", "30f3f33e-8d0c-4e69-8539-cbd701d18f28"), + ("musicbrainz_artistid", "b7ffd2af-418f-4be2-bdd1-22f8b48613da"), + ("musicbrainz_albumartistid", "b7ffd2af-418f-4be2-bdd1-22f8b48613da"), ("license", "http://creativecommons.org/licenses/by-nc-sa/3.0/us/"), ("copyright", "2008 nin"), ], @@ -199,54 +189,18 @@ def test_can_get_metadata_from_flac_file_not_crash_if_empty(): data.get("test") -@pytest.mark.parametrize( - "field_name", - [ - "musicbrainz_artistid", - "musicbrainz_albumid", - "musicbrainz_recordingid", - "musicbrainz_albumartistid", - ], -) -def test_mbid_clean_keeps_only_first(field_name): - u1 = str(uuid.uuid4()) - u2 = str(uuid.uuid4()) - field = metadata.VALIDATION[field_name] - result = field.to_python("/".join([u1, u2])) - - assert str(result) == u1 - - @pytest.mark.parametrize( "raw,expected", [ ("2017", datetime.date(2017, 1, 1)), ("2017-12-31", datetime.date(2017, 12, 31)), ("2017-14-01 01:32", datetime.date(2017, 1, 14)), # deezer format + ("2017-02", datetime.date(2017, 1, 1)), # weird format that exists + ("nonsense", None), ], ) def test_date_parsing(raw, expected): - assert metadata.get_date(raw) == expected - - -def test_date_parsing_failure(): - with pytest.raises(metadata.ParseError): - metadata.get_date("noop") - - -def test_metadata_all_ignore_parse_errors_true(mocker): - path = os.path.join(DATA_DIR, "sample.flac") - data = metadata.Metadata(path) - mocker.patch.object(data, "get", side_effect=metadata.ParseError("Failure")) - assert data.all()["date"] is None - - -def test_metadata_all_ignore_parse_errors_false(mocker): - path = os.path.join(DATA_DIR, "sample.flac") - data = metadata.Metadata(path) - mocker.patch.object(data, "get", side_effect=metadata.ParseError("Failure")) - with pytest.raises(metadata.ParseError): - data.all(ignore_parse_errors=False) + assert metadata.PermissiveDateField().to_internal_value(raw) == expected def test_metadata_fallback_ogg_theora(mocker): @@ -264,3 +218,247 @@ def test_metadata_fallback_ogg_theora(mocker): assert data.get("pictures", "default") == expected_result fallback_get.assert_called_once_with("pictures", "default") + + +@pytest.mark.parametrize( + "path, expected", + [ + ( + "test.mp3", + { + "title": "Bend", + "artists": [ + { + "name": "Binärpilot", + "mbid": uuid.UUID("9c6bddde-6228-4d9f-ad0d-03f6fcb19e13"), + } + ], + "album": { + "title": "You Can't Stop Da Funk", + "mbid": uuid.UUID("ce40cdb1-a562-4fd8-a269-9269f98d4124"), + "release_date": datetime.date(2006, 2, 7), + "artists": [ + { + "name": "Binärpilot", + "mbid": uuid.UUID("9c6bddde-6228-4d9f-ad0d-03f6fcb19e13"), + } + ], + }, + "position": 2, + "disc_number": 1, + "mbid": uuid.UUID("f269d497-1cc0-4ae4-a0c4-157ec7d73fcb"), + "license": "https://creativecommons.org/licenses/by-nc-nd/2.5/", + "copyright": "Someone", + }, + ), + ( + "test.ogg", + { + "title": "Peer Gynt Suite no. 1, op. 46: I. Morning", + "artists": [ + { + "name": "Edvard Grieg", + "mbid": uuid.UUID("013c8e5b-d72a-4cd3-8dee-6c64d6125823"), + } + ], + "album": { + "title": "Peer Gynt Suite no. 1, op. 46", + "mbid": uuid.UUID("a766da8b-8336-47aa-a3ee-371cc41ccc75"), + "release_date": datetime.date(2012, 8, 15), + "artists": [ + { + "name": "Edvard Grieg", + "mbid": uuid.UUID("013c8e5b-d72a-4cd3-8dee-6c64d6125823"), + }, + { + "name": "Musopen Symphony Orchestra", + "mbid": uuid.UUID("5b4d7d2d-36df-4b38-95e3-a964234f520f"), + }, + ], + }, + "position": 1, + "disc_number": 1, + "mbid": uuid.UUID("bd21ac48-46d8-4e78-925f-d9cc2a294656"), + "license": "Dummy license: http://creativecommons.org/licenses/by-sa/4.0/", + "copyright": "Someone", + }, + ), + ( + "test.opus", + { + "title": "Peer Gynt Suite no. 1, op. 46: I. Morning", + "artists": [ + { + "name": "Edvard Grieg", + "mbid": uuid.UUID("013c8e5b-d72a-4cd3-8dee-6c64d6125823"), + } + ], + "album": { + "title": "Peer Gynt Suite no. 1, op. 46", + "mbid": uuid.UUID("a766da8b-8336-47aa-a3ee-371cc41ccc75"), + "release_date": datetime.date(2012, 8, 15), + "artists": [ + { + "name": "Edvard Grieg", + "mbid": uuid.UUID("013c8e5b-d72a-4cd3-8dee-6c64d6125823"), + }, + { + "name": "Musopen Symphony Orchestra", + "mbid": uuid.UUID("5b4d7d2d-36df-4b38-95e3-a964234f520f"), + }, + ], + }, + "position": 1, + "disc_number": 1, + "mbid": uuid.UUID("bd21ac48-46d8-4e78-925f-d9cc2a294656"), + "license": "Dummy license: http://creativecommons.org/licenses/by-sa/4.0/", + "copyright": "Someone", + }, + ), + ( + "test_theora.ogg", + { + "title": "Drei Kreuze (dass wir hier sind)", + "artists": [ + { + "name": "Die Toten Hosen", + "mbid": uuid.UUID("c3bc80a6-1f4a-4e17-8cf0-6b1efe8302f1"), + } + ], + "album": { + "title": "Ballast der Republik", + "mbid": uuid.UUID("1f0441ad-e609-446d-b355-809c445773cf"), + "release_date": datetime.date(2012, 5, 4), + "artists": [ + { + "name": "Die Toten Hosen", + "mbid": uuid.UUID("c3bc80a6-1f4a-4e17-8cf0-6b1efe8302f1"), + } + ], + }, + "position": 1, + "disc_number": 1, + "mbid": uuid.UUID("124d0150-8627-46bc-bc14-789a3bc960c8"), + # somehow, I cannot successfully create an ogg theora file + # with the proper license field + # ("license", "Dummy license: http://creativecommons.org/licenses/by-sa/4.0/"), + "copyright": "℗ 2012 JKP GmbH & Co. KG", + }, + ), + ( + "sample.flac", + { + "title": "999,999", + "artists": [ + { + "name": "Nine Inch Nails", + "mbid": uuid.UUID("b7ffd2af-418f-4be2-bdd1-22f8b48613da"), + } + ], + "album": { + "title": "The Slip", + "mbid": uuid.UUID("12b57d46-a192-499e-a91f-7da66790a1c1"), + "release_date": datetime.date(2008, 5, 5), + "artists": [ + { + "name": "Nine Inch Nails", + "mbid": uuid.UUID("b7ffd2af-418f-4be2-bdd1-22f8b48613da"), + } + ], + }, + "position": 1, + "disc_number": 1, + "mbid": uuid.UUID("30f3f33e-8d0c-4e69-8539-cbd701d18f28"), + "license": "http://creativecommons.org/licenses/by-nc-sa/3.0/us/", + "copyright": "2008 nin", + }, + ), + ], +) +def test_track_metadata_serializer(path, expected, mocker): + path = os.path.join(DATA_DIR, path) + data = metadata.Metadata(path) + get_picture = mocker.patch.object(data, "get_picture") + expected["cover_data"] = get_picture.return_value + + serializer = metadata.TrackMetadataSerializer(data=data) + assert serializer.is_valid(raise_exception=True) is True + assert serializer.validated_data == expected + + get_picture.assert_called_once_with("cover_front", "other") + + +@pytest.mark.parametrize( + "raw, expected", + [ + ( + { + "names": "Hello; World", + "mbids": "f269d497-1cc0-4ae4-a0c4-157ec7d73fcb; f269d497-1cc0-4ae4-a0c4-157ec7d73fcd ", + }, + [ + { + "name": "Hello", + "mbid": uuid.UUID("f269d497-1cc0-4ae4-a0c4-157ec7d73fcb"), + }, + { + "name": "World", + "mbid": uuid.UUID("f269d497-1cc0-4ae4-a0c4-157ec7d73fcd"), + }, + ], + ), + ( + { + "names": "Hello; World; Foo", + "mbids": "f269d497-1cc0-4ae4-a0c4-157ec7d73fcb; f269d497-1cc0-4ae4-a0c4-157ec7d73fcd ", + }, + [ + { + "name": "Hello", + "mbid": uuid.UUID("f269d497-1cc0-4ae4-a0c4-157ec7d73fcb"), + }, + { + "name": "World", + "mbid": uuid.UUID("f269d497-1cc0-4ae4-a0c4-157ec7d73fcd"), + }, + {"name": "Foo", "mbid": None}, + ], + ), + ], +) +def test_artists_cleaning(raw, expected): + field = metadata.ArtistField() + assert field.to_internal_value(raw) == expected + + +@pytest.mark.parametrize( + "data, errored_field", + [ + ({"name": "Hello", "mbid": "wrong-uuid"}, "mbid"), # wrong uuid + ({"name": "", "mbid": "f269d497-1cc0-4ae4-a0c4-157ec7d73fcd"}, "name"), + ], +) +def test_artist_serializer_validation(data, errored_field): + serializer = metadata.ArtistSerializer(data=data) + assert serializer.is_valid() is False + + assert len(serializer.errors) == 1 + assert errored_field in serializer.errors + + +@pytest.mark.parametrize( + "data, errored_field", + [ + ({"title": "Hello", "mbid": "wrong"}, "mbid"), # wrong uuid + ( + {"title": "", "mbid": "f269d497-1cc0-4ae4-a0c4-157ec7d73fcd"}, + "title", + ), # empty title + ], +) +def test_album_serializer_validation(data, errored_field): + serializer = metadata.AlbumSerializer(data=data) + assert serializer.is_valid() is False + + assert len(serializer.errors) == 1 + assert errored_field in serializer.errors diff --git a/api/tests/music/test_tasks.py b/api/tests/music/test_tasks.py index 2ba95209a..d2bb3a903 100644 --- a/api/tests/music/test_tasks.py +++ b/api/tests/music/test_tasks.py @@ -20,15 +20,13 @@ DATA_DIR = os.path.dirname(os.path.abspath(__file__)) def test_can_create_track_from_file_metadata_no_mbid(db, mocker): metadata = { "title": "Test track", - "artist": "Test artist", - "album": "Test album", - "date": datetime.date(2012, 8, 15), - "track_number": 4, + "artists": [{"name": "Test artist"}], + "album": {"title": "Test album", "release_date": datetime.date(2012, 8, 15)}, + "position": 4, "disc_number": 2, "license": "Hello world: http://creativecommons.org/licenses/by-sa/4.0/", "copyright": "2018 Someone", } - mocker.patch("funkwhale_api.music.metadata.Metadata.all", return_value=metadata) match_license = mocker.spy(licenses, "match") track = tasks.get_track_from_import_metadata(metadata) @@ -39,10 +37,10 @@ def test_can_create_track_from_file_metadata_no_mbid(db, mocker): assert track.disc_number == 2 assert track.license.code == "cc-by-sa-4.0" assert track.copyright == metadata["copyright"] - assert track.album.title == metadata["album"] + assert track.album.title == metadata["album"]["title"] assert track.album.mbid is None assert track.album.release_date == datetime.date(2012, 8, 15) - assert track.artist.name == metadata["artist"] + assert track.artist.name == metadata["artists"][0]["name"] assert track.artist.mbid is None match_license.assert_called_once_with(metadata["license"], metadata["copyright"]) @@ -50,33 +48,38 @@ def test_can_create_track_from_file_metadata_no_mbid(db, mocker): def test_can_create_track_from_file_metadata_mbid(factories, mocker): metadata = { "title": "Test track", - "artist": "Test artist", - "album_artist": "Test album artist", - "album": "Test album", - "date": datetime.date(2012, 8, 15), - "track_number": 4, - "musicbrainz_albumid": "ce40cdb1-a562-4fd8-a269-9269f98d4124", - "musicbrainz_recordingid": "f269d497-1cc0-4ae4-a0c4-157ec7d73fcb", - "musicbrainz_artistid": "9c6bddde-6228-4d9f-ad0d-03f6fcb19e13", - "musicbrainz_albumartistid": "9c6bddde-6478-4d9f-ad0d-03f6fcb19e13", + "artists": [ + {"name": "Test artist", "mbid": "9c6bddde-6228-4d9f-ad0d-03f6fcb19e13"} + ], + "album": { + "title": "Test album", + "release_date": datetime.date(2012, 8, 15), + "mbid": "9c6bddde-6478-4d9f-ad0d-03f6fcb19e15", + "artists": [ + { + "name": "Test album artist", + "mbid": "9c6bddde-6478-4d9f-ad0d-03f6fcb19e13", + } + ], + }, + "position": 4, + "mbid": "f269d497-1cc0-4ae4-a0c4-157ec7d73fcb", "cover_data": {"content": b"image_content", "mimetype": "image/png"}, } - mocker.patch("funkwhale_api.music.metadata.Metadata.all", return_value=metadata) - track = tasks.get_track_from_import_metadata(metadata) assert track.title == metadata["title"] - assert track.mbid == metadata["musicbrainz_recordingid"] + assert track.mbid == metadata["mbid"] assert track.position == 4 assert track.disc_number is None - assert track.album.title == metadata["album"] - assert track.album.mbid == metadata["musicbrainz_albumid"] - assert track.album.artist.mbid == metadata["musicbrainz_albumartistid"] - assert track.album.artist.name == metadata["album_artist"] + assert track.album.title == metadata["album"]["title"] + assert track.album.mbid == metadata["album"]["mbid"] + assert track.album.artist.mbid == metadata["album"]["artists"][0]["mbid"] + assert track.album.artist.name == metadata["album"]["artists"][0]["name"] assert track.album.release_date == datetime.date(2012, 8, 15) - assert track.artist.name == metadata["artist"] - assert track.artist.mbid == metadata["musicbrainz_artistid"] + assert track.artist.name == metadata["artists"][0]["name"] + assert track.artist.mbid == metadata["artists"][0]["mbid"] def test_can_create_track_from_file_metadata_mbid_existing_album_artist( @@ -85,22 +88,21 @@ def test_can_create_track_from_file_metadata_mbid_existing_album_artist( artist = factories["music.Artist"]() album = factories["music.Album"]() metadata = { - "artist": "", - "album": "", + "album": { + "mbid": album.mbid, + "title": "", + "artists": [{"name": "", "mbid": album.artist.mbid}], + }, "title": "Hello", - "track_number": 4, - "musicbrainz_albumid": album.mbid, - "musicbrainz_recordingid": "f269d497-1cc0-4ae4-a0c4-157ec7d73fcb", - "musicbrainz_artistid": artist.mbid, - "musicbrainz_albumartistid": album.artist.mbid, + "position": 4, + "artists": [{"mbid": artist.mbid, "name": ""}], + "mbid": "f269d497-1cc0-4ae4-a0c4-157ec7d73fcb", } - mocker.patch("funkwhale_api.music.metadata.Metadata.all", return_value=metadata) - track = tasks.get_track_from_import_metadata(metadata) assert track.title == metadata["title"] - assert track.mbid == metadata["musicbrainz_recordingid"] + assert track.mbid == metadata["mbid"] assert track.position == 4 assert track.album == album assert track.artist == artist @@ -112,18 +114,17 @@ def test_can_create_track_from_file_metadata_fid_existing_album_artist( artist = factories["music.Artist"]() album = factories["music.Album"]() metadata = { - "artist": "", - "album": "", + "artists": [{"name": "", "fid": artist.fid}], + "album": { + "title": "", + "fid": album.fid, + "artists": [{"name": "", "fid": album.artist.fid}], + }, "title": "Hello", - "track_number": 4, + "position": 4, "fid": "https://hello", - "album_fid": album.fid, - "artist_fid": artist.fid, - "album_artist_fid": album.artist.fid, } - mocker.patch("funkwhale_api.music.metadata.Metadata.all", return_value=metadata) - track = tasks.get_track_from_import_metadata(metadata) assert track.title == metadata["title"] @@ -139,13 +140,11 @@ def test_can_create_track_from_file_metadata_distinct_release_mbid(factories): album = factories["music.Album"](artist=artist) track = factories["music.Track"](album=album, artist=artist) metadata = { - "artist": artist.name, - "album": album.title, + "artists": [{"name": artist.name, "mbid": artist.mbid}], + "album": {"title": album.title, "mbid": str(uuid.uuid4())}, "title": track.title, - "track_number": 4, + "position": 4, "fid": "https://hello", - "musicbrainz_artistid": artist.mbid, - "musicbrainz_albumid": str(uuid.uuid4()), } new_track = tasks.get_track_from_import_metadata(metadata) @@ -162,12 +161,10 @@ def test_can_create_track_from_file_metadata_distinct_position(factories): album = factories["music.Album"](artist=artist) track = factories["music.Track"](album=album, artist=artist) metadata = { - "artist": artist.name, - "album": album.title, + "artists": [{"name": artist.name, "mbid": artist.mbid}], + "album": {"title": album.title, "mbid": album.mbid}, "title": track.title, - "track_number": track.position + 1, - "musicbrainz_artistid": artist.mbid, - "musicbrainz_albumid": album.mbid, + "position": track.position + 1, } new_track = tasks.get_track_from_import_metadata(metadata) @@ -177,23 +174,28 @@ def test_can_create_track_from_file_metadata_distinct_position(factories): def test_can_create_track_from_file_metadata_federation(factories, mocker, r_mock): metadata = { - "artist": "Artist", - "album": "Album", - "album_artist": "Album artist", + "artists": [ + {"name": "Artist", "fid": "https://artist.fid", "fdate": timezone.now()} + ], + "album": { + "title": "Album", + "fid": "https://album.fid", + "fdate": timezone.now(), + "artists": [ + { + "name": "Album artist", + "fid": "https://album.artist.fid", + "fdate": timezone.now(), + } + ], + }, "title": "Hello", - "track_number": 4, + "position": 4, "fid": "https://hello", - "album_fid": "https://album.fid", - "artist_fid": "https://artist.fid", - "album_artist_fid": "https://album.artist.fid", "fdate": timezone.now(), - "album_fdate": timezone.now(), - "album_artist_fdate": timezone.now(), - "artist_fdate": timezone.now(), "cover_data": {"url": "https://cover/hello.png", "mimetype": "image/png"}, } r_mock.get(metadata["cover_data"]["url"], body=io.BytesIO(b"coucou")) - mocker.patch("funkwhale_api.music.metadata.Metadata.all", return_value=metadata) track = tasks.get_track_from_import_metadata(metadata, update_cover=True) @@ -203,15 +205,15 @@ def test_can_create_track_from_file_metadata_federation(factories, mocker, r_moc assert track.position == 4 assert track.album.cover.read() == b"coucou" assert track.album.cover.path.endswith(".png") - assert track.album.fid == metadata["album_fid"] - assert track.album.title == metadata["album"] - assert track.album.creation_date == metadata["album_fdate"] - assert track.album.artist.fid == metadata["album_artist_fid"] - assert track.album.artist.name == metadata["album_artist"] - assert track.album.artist.creation_date == metadata["album_artist_fdate"] - assert track.artist.fid == metadata["artist_fid"] - assert track.artist.name == metadata["artist"] - assert track.artist.creation_date == metadata["artist_fdate"] + assert track.album.fid == metadata["album"]["fid"] + assert track.album.title == metadata["album"]["title"] + assert track.album.creation_date == metadata["album"]["fdate"] + assert track.album.artist.fid == metadata["album"]["artists"][0]["fid"] + assert track.album.artist.name == metadata["album"]["artists"][0]["name"] + assert track.album.artist.creation_date == metadata["album"]["artists"][0]["fdate"] + assert track.artist.fid == metadata["artists"][0]["fid"] + assert track.artist.name == metadata["artists"][0]["name"] + assert track.artist.creation_date == metadata["artists"][0]["fdate"] def test_sort_candidates(factories): @@ -391,7 +393,38 @@ def test_upload_import_error(factories, now, temp_signal): assert upload.import_status == "errored" assert upload.import_date == now - assert upload.import_details == {"error_code": "track_uuid_not_found"} + assert upload.import_details == { + "error_code": "track_uuid_not_found", + "detail": None, + } + handler.assert_called_once_with( + upload=upload, + old_status="pending", + new_status="errored", + sender=None, + signal=signals.upload_import_status_updated, + ) + + +def test_upload_import_error_metadata(factories, now, temp_signal, mocker): + path = os.path.join(DATA_DIR, "test.ogg") + upload = factories["music.Upload"](audio_file__frompath=path) + mocker.patch.object( + metadata.AlbumField, + "to_internal_value", + side_effect=metadata.serializers.ValidationError("Hello"), + ) + with temp_signal(signals.upload_import_status_updated) as handler: + tasks.process_upload(upload_id=upload.pk) + upload.refresh_from_db() + + assert upload.import_status == "errored" + assert upload.import_date == now + assert upload.import_details == { + "error_code": "invalid_metadata", + "detail": {"album": ["Hello"]}, + "file_metadata": metadata.Metadata(path).all(), + } handler.assert_called_once_with( upload=upload, old_status="pending", @@ -494,31 +527,43 @@ def test_federation_audio_track_to_metadata(now): serializer = federation_serializers.TrackSerializer(data=payload) serializer.is_valid(raise_exception=True) expected = { - "artist": payload["artists"][0]["name"], - "album": payload["album"]["name"], - "album_artist": payload["album"]["artists"][0]["name"], "title": payload["name"], - "date": released, - "track_number": payload["position"], + "position": payload["position"], "disc_number": payload["disc"], "license": "http://creativecommons.org/licenses/by-sa/4.0/", "copyright": "2018 Someone", - # musicbrainz - "musicbrainz_albumid": payload["album"]["musicbrainzId"], - "musicbrainz_recordingid": payload["musicbrainzId"], - "musicbrainz_artistid": payload["artists"][0]["musicbrainzId"], - "musicbrainz_albumartistid": payload["album"]["artists"][0]["musicbrainzId"], - # federation - "fid": payload["id"], - "album_fid": payload["album"]["id"], - "artist_fid": payload["artists"][0]["id"], - "album_artist_fid": payload["album"]["artists"][0]["id"], + "mbid": payload["musicbrainzId"], "fdate": serializer.validated_data["published"], - "artist_fdate": serializer.validated_data["artists"][0]["published"], - "album_artist_fdate": serializer.validated_data["album"]["artists"][0][ - "published" + "fid": payload["id"], + "album": { + "title": payload["album"]["name"], + "release_date": released, + "mbid": payload["album"]["musicbrainzId"], + "fid": payload["album"]["id"], + "fdate": serializer.validated_data["album"]["published"], + "artists": [ + { + "name": a["name"], + "mbid": a["musicbrainzId"], + "fid": a["id"], + "fdate": serializer.validated_data["album"]["artists"][i][ + "published" + ], + } + for i, a in enumerate(payload["album"]["artists"]) + ], + }, + # musicbrainz + # federation + "artists": [ + { + "name": a["name"], + "mbid": a["musicbrainzId"], + "fid": a["id"], + "fdate": serializer.validated_data["artists"][i]["published"], + } + for i, a in enumerate(payload["artists"]) ], - "album_fdate": serializer.validated_data["album"]["published"], "cover_data": { "mimetype": serializer.validated_data["album"]["cover"]["mediaType"], "url": serializer.validated_data["album"]["cover"]["href"], @@ -528,10 +573,6 @@ def test_federation_audio_track_to_metadata(now): result = tasks.federation_audio_track_to_metadata(serializer.validated_data) assert result == expected - # ensure we never forget to test a mandatory field - for k in metadata.ALL_FIELDS: - assert k in result - def test_scan_library_fetches_page_and_calls_scan_page(now, mocker, factories, r_mock): scan = factories["music.LibraryScan"]() diff --git a/changes/changelog.d/252.feature b/changes/changelog.d/252.feature new file mode 100644 index 000000000..4bbb2da1e --- /dev/null +++ b/changes/changelog.d/252.feature @@ -0,0 +1 @@ +Improved error handling and display during import (#252, #718, #583, #501, #544) diff --git a/changes/changelog.d/718.bugfix b/changes/changelog.d/718.bugfix new file mode 100644 index 000000000..4dd1e13fe --- /dev/null +++ b/changes/changelog.d/718.bugfix @@ -0,0 +1 @@ +Fixed crashing upload processing on invalid date format (#718) diff --git a/changes/notes.rst b/changes/notes.rst index 8f5ee15f7..858ed5eeb 100644 --- a/changes/notes.rst +++ b/changes/notes.rst @@ -35,6 +35,15 @@ enabled in a future release). If you want to start building an app on top of Funkwhale's API, please check-out `https://docs.funkwhale.audio/api.html`_ and `https://docs.funkwhale.audio/developers/authentication.html`_. +Better error handling and display during import +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +Funkwhale should now be more resilient to missing tags in imported files, and give +you more insights when something goes wrong, including the specific tags that were missing +or invalid, and additional debug information to share in your support requests. + +This information is available in all pages that list uploads, when clicking on the button next to the upload status. + Prune library command ^^^^^^^^^^^^^^^^^^^^^ diff --git a/docs/users/upload.rst b/docs/users/upload.rst index 1dfa4d76f..169c72710 100644 --- a/docs/users/upload.rst +++ b/docs/users/upload.rst @@ -154,3 +154,30 @@ Then select the files you want to delete using the checkboxes on the left ; you Finally, select "Delete" in the "Action" menu and click "Go". This operation does *not* remove metadata, meaning that deleted tracks will remain visible in your library. They just won't be playable anymore. + + +Common errors during import +--------------------------- + +.. _invalid_metadata: + +Invalid metadata +:::::::::::::::: + +This error occurs when the uploaded file miss some mandatory tags, or when some tags have +incorrect values (e.g an empty title or artist name). + +To fix this issue, please retag the file properly as described in :ref:`upload-tagging` +and reupload it. + + +.. _unknown_error: + +Unkwown error +::::::::::::: + +This error can happen for multiple reasons and likely indicates an issue with the Funkwhale +server (e.g. misconfiguration) or with Funkwhale itself. + +If the issue persists when you relaunch the import, get in touch with our instance admin +or open a support thread on our forum. diff --git a/front/src/views/content/libraries/FilesTable.vue b/front/src/views/content/libraries/FilesTable.vue index e630373a2..f6c4e69ef 100644 --- a/front/src/views/content/libraries/FilesTable.vue +++ b/front/src/views/content/libraries/FilesTable.vue @@ -35,6 +35,88 @@ + +
+ Import detail +
+
+
+
+ Upload is still pending and will soon be processed by the server. +
+
+ Upload was successfully processed by the server. +
+
+ Upload was skipped because a similar one is already available in one of your libraries. +
+
+ An error occured during upload processing. You will find more information below. +
+ +
+
+
+
+ Close +
+
+
@@ -80,11 +162,13 @@ - - + + {{ labels.importStatuses[scope.obj.import_status].label }} - + {{ time.parse(scope.obj.duration) }} @@ -132,7 +216,33 @@ import ActionTable from '@/components/common/ActionTable' import OrderingMixin from '@/components/mixins/Ordering' import TranslationsMixin from '@/components/mixins/Translations' import SmartSearchMixin from '@/components/mixins/SmartSearch' +import Modal from '@/components/semantic/Modal' +function getErrors(payload) { + let errors = [] + for (var k in payload) { + if (payload.hasOwnProperty(k)) { + let value = payload[k] + if (Array.isArray(value)) { + errors.push({ + key: k, + value: value.join(', ') + }) + } else { + // possibly artists, so nested errors + if (typeof value === 'object') { + getErrors(value).forEach((e) => { + errors.push({ + key: `${k} / ${e.key}`, + value: e.value + }) + }) + } + } + } + } + return errors +} export default { mixins: [OrderingMixin, TranslationsMixin, SmartSearchMixin], props: { @@ -142,11 +252,14 @@ export default { }, components: { Pagination, - ActionTable + ActionTable, + Modal }, data () { return { time, + detailedUpload: null, + showUploadDetailModal: false, isLoading: false, result: null, page: 1, @@ -193,12 +306,41 @@ export default { }, selectPage: function (page) { this.page = page + }, + getErrorData (upload) { + let payload = upload.import_details || {} + let d = { + supportUrl: 'https://governance.funkwhale.audio/g/246YOJ1m/funkwhale-support', + errorRows: [] + } + if (!payload.error_code) { + d.errorCode = 'unknown_error' + } else { + d.errorCode = payload.error_code + } + d.documentationUrl = `https://docs.funkwhale.audio/users/upload.html#${d.errorCode}` + if (d.errorCode === 'invalid_metadata') { + d.label = this.$pgettext('Popup/Import/Error.Label', 'Invalid metadata') + d.detail = this.$pgettext('Popup/Import/Error.Label', 'The metadata included in the file is invalid or some mandatory fields are missing.') + let detail = payload.detail || {} + d.errorRows = getErrors(detail) + } else { + d.label = this.$pgettext('Popup/Import/Error.Label', 'Unkwown error') + d.detail = this.$pgettext('Popup/Import/Error.Label', 'An unkwown error occured') + } + let debugInfo = { + source: upload.source, + ...payload, + } + d.debugInfo = JSON.stringify(debugInfo, null, 4) + return d } }, computed: { labels () { return { searchPlaceholder: this.$pgettext('Content/Library/Input.Placeholder', 'Search by title, artist, album…'), + statusDetailTitle: this.$pgettext('Content/Library/Link.Title', 'Click to display more information about the import process for this upload'), importStatuses: { skipped: { label: this.$pgettext('Content/Library/*', 'Skipped'),