Merge branch 'import-small-issues' into 'develop'

Import small issues

See merge request funkwhale/funkwhale!908

api/funkwhale_api/music/models.py

@@ -34,6 +34,13 @@ from . import importers, metadata, utils
 
 logger = logging.getLogger(__name__)
 
+MAX_LENGTHS = {
+    "ARTIST_NAME": 255,
+    "ALBUM_TITLE": 255,
+    "TRACK_TITLE": 255,
+    "COPYRIGHT": 500,
+}
+
 
 def empty_dict():
     return {}
@@ -187,7 +194,7 @@ class ArtistQuerySet(common_models.LocalFromFidQuerySet, models.QuerySet):
 
 
 class Artist(APIModelMixin):
-    name = models.CharField(max_length=255)
+    name = models.CharField(max_length=MAX_LENGTHS["ARTIST_NAME"])
     federation_namespace = "artists"
     musicbrainz_model = "artist"
     musicbrainz_mapping = {
@@ -275,7 +282,7 @@ class AlbumQuerySet(common_models.LocalFromFidQuerySet, models.QuerySet):
 
 
 class Album(APIModelMixin):
-    title = models.CharField(max_length=255)
+    title = models.CharField(max_length=MAX_LENGTHS["ALBUM_TITLE"])
     artist = models.ForeignKey(Artist, related_name="albums", on_delete=models.CASCADE)
     release_date = models.DateField(null=True, blank=True)
     release_group_id = models.UUIDField(null=True, blank=True)
@@ -330,6 +337,7 @@ class Album(APIModelMixin):
         if data:
             extensions = {"image/jpeg": "jpg", "image/png": "png", "image/gif": "gif"}
             extension = extensions.get(data["mimetype"], "jpg")
+            f = None
             if data.get("content"):
                 # we have to cover itself
                 f = ContentFile(data["content"])
@@ -349,15 +357,17 @@ class Album(APIModelMixin):
                     return
                 else:
                     f = ContentFile(response.content)
-            self.cover.save("{}.{}".format(self.uuid, extension), f, save=False)
+            if f:
-            self.save(update_fields=["cover"])
+                self.cover.save("{}.{}".format(self.uuid, extension), f, save=False)
-            return self.cover.file
+                self.save(update_fields=["cover"])
+                return self.cover.file
         if self.mbid:
             image_data = musicbrainz.api.images.get_front(str(self.mbid))
             f = ContentFile(image_data)
             self.cover.save("{0}.jpg".format(self.mbid), f, save=False)
             self.save(update_fields=["cover"])
-        return self.cover.file
+        if self.cover:
+            return self.cover.file
 
     def __str__(self):
         return self.title
@@ -445,7 +455,7 @@ def get_artist(release_list):
 
 
 class Track(APIModelMixin):
-    title = models.CharField(max_length=255)
+    title = models.CharField(max_length=MAX_LENGTHS["TRACK_TITLE"])
     artist = models.ForeignKey(Artist, related_name="tracks", on_delete=models.CASCADE)
     disc_number = models.PositiveIntegerField(null=True, blank=True)
     position = models.PositiveIntegerField(null=True, blank=True)
@@ -469,7 +479,9 @@ class Track(APIModelMixin):
         on_delete=models.SET_NULL,
         related_name="attributed_tracks",
     )
-    copyright = models.CharField(max_length=500, null=True, blank=True)
+    copyright = models.CharField(
+        max_length=MAX_LENGTHS["COPYRIGHT"], null=True, blank=True
+    )
     federation_namespace = "tracks"
     musicbrainz_model = "recording"
     api = musicbrainz.api.recordings

api/funkwhale_api/music/tasks.py

@@ -406,6 +406,12 @@ def get_track_from_import_metadata(data, update_cover=False, attributed_to=None)
     return track
 
 
+def truncate(v, length):
+    if v is None:
+        return v
+    return v[:length]
+
+
 def _get_track(data, attributed_to=None):
     track_uuid = getter(data, "funkwhale", "track", "uuid")
 
@@ -447,7 +453,7 @@ def _get_track(data, attributed_to=None):
     artist_data = artists[0]
     artist_mbid = artist_data.get("mbid", None)
     artist_fid = artist_data.get("fid", None)
-    artist_name = artist_data["name"]
+    artist_name = truncate(artist_data["name"], models.MAX_LENGTHS["ARTIST_NAME"])
 
     if artist_mbid:
         query = Q(mbid=artist_mbid)
@@ -473,7 +479,9 @@ def _get_track(data, attributed_to=None):
 
     album_artists = getter(data, "album", "artists", default=artists) or artists
     album_artist_data = album_artists[0]
-    album_artist_name = album_artist_data.get("name")
+    album_artist_name = truncate(
+        album_artist_data.get("name"), models.MAX_LENGTHS["ARTIST_NAME"]
+    )
     if album_artist_name == artist_name:
         album_artist = artist
     else:
@@ -502,7 +510,7 @@ def _get_track(data, attributed_to=None):
 
     # get / create album
     album_data = data["album"]
-    album_title = album_data["title"]
+    album_title = truncate(album_data["title"], models.MAX_LENGTHS["ALBUM_TITLE"])
     album_fid = album_data.get("fid", None)
 
     if album_mbid:
@@ -531,7 +539,7 @@ def _get_track(data, attributed_to=None):
         tags_models.add_tags(album, *album_data.get("tags", []))
 
     # get / create track
-    track_title = data["title"]
+    track_title = truncate(data["title"], models.MAX_LENGTHS["TRACK_TITLE"])
     position = data.get("position", 1)
     query = Q(title__iexact=track_title, artist=artist, album=album, position=position)
     if track_mbid:
@@ -549,7 +557,7 @@ def _get_track(data, attributed_to=None):
         "from_activity_id": from_activity_id,
         "attributed_to": data.get("attributed_to", attributed_to),
         "license": licenses.match(data.get("license"), data.get("copyright")),
-        "copyright": data.get("copyright"),
+        "copyright": truncate(data.get("copyright"), models.MAX_LENGTHS["COPYRIGHT"]),
     }
     if data.get("fdate"):
         defaults["creation_date"] = data.get("fdate")

api/tests/music/test_music.py

@@ -133,3 +133,11 @@ def test_can_download_image_file_for_album(binary_cover, mocker, factories):
     album.save()
 
     assert album.cover.file.read() == binary_cover
+
+
+def test_album_get_image_doesnt_crash_with_empty_data(mocker, factories):
+    album = factories["music.Album"](mbid=None, cover=None)
+    assert (
+        album.get_image(data={"content": "", "url": "", "mimetype": "image/png"})
+        is None
+    )

api/tests/music/test_tasks.py

@@ -9,7 +9,7 @@ from django.utils import timezone
 
 from funkwhale_api.federation import serializers as federation_serializers
 from funkwhale_api.federation import jsonld
-from funkwhale_api.music import licenses, metadata, signals, tasks
+from funkwhale_api.music import licenses, metadata, models, signals, tasks
 
 DATA_DIR = os.path.dirname(os.path.abspath(__file__))
 
@@ -79,6 +79,32 @@ def test_can_create_track_from_file_metadata_attributed_to(factories, mocker):
     assert track.artist.attributed_to == actor
 
 
+def test_can_create_track_from_file_metadata_truncates_too_long_values(
+    factories, mocker
+):
+    metadata = {
+        "title": "a" * 5000,
+        "artists": [{"name": "b" * 5000}],
+        "album": {"title": "c" * 5000, "release_date": datetime.date(2012, 8, 15)},
+        "position": 4,
+        "disc_number": 2,
+        "copyright": "d" * 5000,
+    }
+
+    track = tasks.get_track_from_import_metadata(metadata)
+
+    assert track.title == metadata["title"][: models.MAX_LENGTHS["TRACK_TITLE"]]
+    assert track.copyright == metadata["copyright"][: models.MAX_LENGTHS["COPYRIGHT"]]
+    assert (
+        track.album.title
+        == metadata["album"]["title"][: models.MAX_LENGTHS["ALBUM_TITLE"]]
+    )
+    assert (
+        track.artist.name
+        == metadata["artists"][0]["name"][: models.MAX_LENGTHS["ARTIST_NAME"]]
+    )
+
+
 def test_can_create_track_from_file_metadata_featuring(factories):
     metadata = {
         "title": "Whole Lotta Love",

changes/changelog.d/imports-small.bugfix

@@ -0,0 +1 @@
+Fixed import crashing with empty cover file or too long values on some fields