From 05f0129025bae36fc1b1101b03687ecd698e9500 Mon Sep 17 00:00:00 2001
From: Eliot Berriot <contact@eliotberriot.com>
Date: Thu, 4 Apr 2019 16:07:43 +0200
Subject: [PATCH] Better error handling on display for import errors (#718,
#583, #501, #252, #544)
---
api/funkwhale_api/music/metadata.py | 329 ++++++++------
api/funkwhale_api/music/tasks.py | 145 +++---
api/tests/music/test_metadata.py | 416 +++++++++++++-----
api/tests/music/test_tasks.py | 233 ++++++----
changes/changelog.d/252.feature | 1 +
changes/changelog.d/718.bugfix | 1 +
changes/notes.rst | 9 +
docs/users/upload.rst | 27 ++
.../views/content/libraries/FilesTable.vue | 150 ++++++-
9 files changed, 915 insertions(+), 396 deletions(-)
create mode 100644 changes/changelog.d/252.feature
create mode 100644 changes/changelog.d/718.bugfix
diff --git a/api/funkwhale_api/music/metadata.py b/api/funkwhale_api/music/metadata.py
index 7a105e432c..3347dfb632 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 47cb4eb38f..f64b7eed26 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 f91f7545b8..e2c180a639 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 2ba95209a4..d2bb3a9035 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",
+ "mbid": payload["musicbrainzId"],
+ "fdate": serializer.validated_data["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
- "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"],
- "fdate": serializer.validated_data["published"],
- "artist_fdate": serializer.validated_data["artists"][0]["published"],
- "album_artist_fdate": serializer.validated_data["album"]["artists"][0][
- "published"
+ "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 0000000000..4bbb2da1e5
--- /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 0000000000..4dd1e13fef
--- /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 8f5ee15f7b..858ed5eebc 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 1dfa4d76fb..169c72710f 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 e630373a2e..f6c4e69efa 100644
--- a/front/src/views/content/libraries/FilesTable.vue
+++ b/front/src/views/content/libraries/FilesTable.vue
@@ -35,6 +35,88 @@
</div>
</div>
</div>
+ <modal :show.sync="showUploadDetailModal">
+ <div class="header">
+ <translate translate-context="Popup/Import/Title">Import detail</translate>
+ </div>
+ <div class="content" v-if="detailedUpload">
+ <div class="description">
+ <div class="ui message" v-if="detailedUpload.import_status === 'pending'">
+ <translate translate-context="Popup/Import/Message">Upload is still pending and will soon be processed by the server.</translate>
+ </div>
+ <div class="ui success message" v-if="detailedUpload.import_status === 'finished'">
+ <translate translate-context="Popup/Import/Message">Upload was successfully processed by the server.</translate>
+ </div>
+ <div class="ui warning message" v-if="detailedUpload.import_status === 'skipped'">
+ <translate translate-context="Popup/Import/Message">Upload was skipped because a similar one is already available in one of your libraries.</translate>
+ </div>
+ <div class="ui error message" v-if="detailedUpload.import_status === 'errored'">
+ <translate translate-context="Popup/Import/Message">An error occured during upload processing. You will find more information below.</translate>
+ </div>
+ <template v-if="detailedUpload.import_status === 'errored'">
+ <table class="ui very basic collapsing celled table">
+ <tbody>
+ <tr>
+ <td>
+ <translate translate-context="Popup/Import/Table.Label/Noun">Error type</translate>
+ </td>
+ <td>
+ {{ getErrorData(detailedUpload).label }}
+ </td>
+ </tr>
+ <tr>
+ <td>
+ <translate translate-context="Popup/Import/Table.Label/Noun">Error detail</translate>
+ </td>
+ <td>
+ {{ getErrorData(detailedUpload).detail }}
+ <ul v-if="getErrorData(detailedUpload).errorRows.length > 0">
+ <li v-for="row in getErrorData(detailedUpload).errorRows">
+ {{ row.key}}: {{ row.value}}
+ </li>
+ </ul>
+ </td>
+ </tr>
+ <tr>
+ <td>
+ <translate translate-context="Popup/Import/Table.Label/Noun">Getting help</translate>
+ </td>
+ <td>
+ <ul>
+ <li>
+ <a :href="getErrorData(detailedUpload).documentationUrl" target="_blank">
+ <translate translate-context="Popup/Import/Table.Label/Value">Read our documentation for this error</translate>
+ </a>
+ </li>
+ <li>
+ <a :href="getErrorData(detailedUpload).supportUrl" target="_blank">
+ <translate translate-context="Popup/Import/Table.Label/Value">Open a support thread (include the debug information below in your message)</translate>
+ </a>
+ </li>
+ </ul>
+ </td>
+ </tr>
+ <tr>
+ <td>
+ <translate translate-context="Popup/Import/Table.Label/Noun">Debug information</translate>
+ </td>
+ <td>
+ <div class="ui form">
+ <textarea class="ui textarea" rows="10" :value="getErrorData(detailedUpload).debugInfo"></textarea>
+ </div>
+ </td>
+ </tr>
+ </tbody>
+ </table>
+ </template>
+ </div>
+ </div>
+ <div class="actions">
+ <div class="ui deny button">
+ <translate translate-context="*/*/Button.Label/Verb">Close</translate>
+ </div>
+ </div>
+ </modal>
<div class="dimmable">
<div v-if="isLoading" class="ui active inverted dimmer">
<div class="ui loader"></div>
@@ -80,11 +162,13 @@
<td>
<human-date :date="scope.obj.creation_date"></human-date>
</td>
- <td :title="labels.importStatuses[scope.obj.import_status].help">
- <span class="discrete link" @click="addSearchToken('status', scope.obj.import_status)">
+ <td>
+ <span class="discrete link" @click="addSearchToken('status', scope.obj.import_status)" :title="labels.importStatuses[scope.obj.import_status].help">
{{ labels.importStatuses[scope.obj.import_status].label }}
- <i class="question circle outline icon"></i>
</span>
+ <button class="ui tiny basic icon button" :title="labels.statusDetailTitle" @click="detailedUpload = scope.obj; showUploadDetailModal = true">
+ <i class="question circle outline icon"></i>
+ </button>
</td>
<td v-if="scope.obj.duration">
{{ 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'),
--
GitLab