Merge branch 'master' into develop

api/funkwhale_api/common/utils.py

@@ -113,6 +113,9 @@ def chunk_queryset(source_qs, chunk_size):
 
 
 def join_url(start, end):
+    if end.startswith("http://") or end.startswith("https://"):
+        # alread a full URL, joining makes no sense
+        return end
     if start.endswith("/") and end.startswith("/"):
         return start + end[1:]
 

api/funkwhale_api/music/metadata.py

@@ -481,14 +481,26 @@ class PermissiveDateField(serializers.CharField):
         return None
 
 
+class MBIDField(serializers.UUIDField):
+    def __init__(self, *args, **kwargs):
+        kwargs.setdefault("allow_null", True)
+        kwargs.setdefault("required", False)
+        super().__init__(*args, **kwargs)
+
+    def to_internal_value(self, v):
+        if v in ["", None]:
+            return None
+        return super().to_internal_value(v)
+
+
 class ArtistSerializer(serializers.Serializer):
     name = serializers.CharField()
-    mbid = serializers.UUIDField(required=False, allow_null=True)
+    mbid = MBIDField()
 
 
 class AlbumSerializer(serializers.Serializer):
     title = serializers.CharField()
-    mbid = serializers.UUIDField(required=False, allow_null=True)
+    mbid = MBIDField()
     release_date = PermissiveDateField(required=False, allow_null=True)
 
 
@@ -512,16 +524,35 @@ class PositionField(serializers.CharField):
 
 class TrackMetadataSerializer(serializers.Serializer):
     title = serializers.CharField()
-    position = PositionField(allow_null=True, required=False)
+    position = PositionField(allow_blank=True, allow_null=True, required=False)
-    disc_number = PositionField(allow_null=True, required=False)
+    disc_number = PositionField(allow_blank=True, allow_null=True, required=False)
-    copyright = serializers.CharField(allow_null=True, required=False)
+    copyright = serializers.CharField(allow_blank=True, allow_null=True, required=False)
-    license = serializers.CharField(allow_null=True, required=False)
+    license = serializers.CharField(allow_blank=True, allow_null=True, required=False)
-    mbid = serializers.UUIDField(allow_null=True, required=False)
+    mbid = MBIDField()
 
     album = AlbumField()
     artists = ArtistField()
     cover_data = CoverDataField()
 
+    remove_blank_null_fields = [
+        "copyright",
+        "license",
+        "position",
+        "disc_number",
+        "mbid",
+    ]
+
+    def validate(self, validated_data):
+        validated_data = super().validate(validated_data)
+        for field in self.remove_blank_null_fields:
+            try:
+                v = validated_data[field]
+            except KeyError:
+                continue
+            if v in ["", None]:
+                validated_data.pop(field)
+        return validated_data
+
 
 class FakeMetadata(Mapping):
     def __init__(self, data, picture=None):

api/funkwhale_api/music/models.py

@@ -870,6 +870,13 @@ class UploadVersion(models.Model):
 
     @property
     def filename(self):
+        try:
+            return (
+                self.upload.track.full_name
+                + "."
+                + utils.MIMETYPE_TO_EXTENSION[self.mimetype]
+            )
+        except KeyError:
             return self.upload.filename
 
     @property

api/funkwhale_api/music/views.py

@@ -285,6 +285,11 @@ def should_transcode(upload, format, max_bitrate=None):
     return format_need_transcoding or bitrate_need_transcoding
 
 
+def get_content_disposition(filename):
+    filename = "filename*=UTF-8''{}".format(urllib.parse.quote(filename))
+    return "attachment; {}".format(filename)
+
+
 def handle_serve(upload, user, format=None, max_bitrate=None, proxy_media=True):
     f = upload
     # we update the accessed_date
@@ -342,8 +347,7 @@ def handle_serve(upload, user, format=None, max_bitrate=None, proxy_media=True):
     mapping = {"nginx": "X-Accel-Redirect", "apache2": "X-Sendfile"}
     file_header = mapping[settings.REVERSE_PROXY_TYPE]
     response[file_header] = file_path
-    filename = "filename*=UTF-8''{}".format(urllib.parse.quote(filename))
+    response["Content-Disposition"] = get_content_disposition(filename)
-    response["Content-Disposition"] = "attachment; {}".format(filename)
     if mt:
         response["Content-Type"] = mt
 

api/tests/common/test_utils.py

@@ -85,3 +85,17 @@ def test_get_updated_fields(conf, mock_args, data, expected, mocker):
     obj = mocker.Mock(**mock_args)
 
     assert utils.get_updated_fields(conf, data, obj) == expected
+
+
+@pytest.mark.parametrize(
+    "start, end, expected",
+    [
+        ("https://domain", "/api", "https://domain/api"),
+        ("https://domain/", "/api", "https://domain/api"),
+        ("https://domain", "api", "https://domain/api"),
+        ("https://domain", "https://api", "https://api"),
+        ("https://domain", "http://api", "http://api"),
+    ],
+)
+def test_join_url(start, end, expected):
+    assert utils.join_url(start, end) == expected

api/tests/music/test_metadata.py

@@ -539,6 +539,33 @@ def test_serializer_album_artist_missing():
     assert serializer.validated_data == expected
 
 
+@pytest.mark.parametrize(
+    "field_name", ["copyright", "license", "mbid", "position", "disc_number"]
+)
+def test_serializer_empty_fields(field_name):
+    data = {
+        "title": "Track Title",
+        "artist": "Track Artist",
+        "album": "Track Album",
+        # empty copyright/license field shouldn't fail, cf #850
+        field_name: "",
+    }
+    expected = {
+        "title": "Track Title",
+        "artists": [{"name": "Track Artist", "mbid": None}],
+        "album": {
+            "title": "Track Album",
+            "mbid": None,
+            "release_date": None,
+            "artists": [],
+        },
+        "cover_data": None,
+    }
+    serializer = metadata.TrackMetadataSerializer(data=metadata.FakeMetadata(data))
+    assert serializer.is_valid(raise_exception=True) is True
+    assert serializer.validated_data == expected
+
+
 def test_artist_field_featuring():
     data = {
         "artist": "Santana feat. Chris Cornell",

api/tests/music/test_views.py

@@ -1,6 +1,7 @@
 import io
 import magic
 import os
+import urllib.parse
 
 import pytest
 from django.urls import reverse
@@ -412,7 +413,7 @@ def test_handle_serve_create_mp3_version(factories, now):
     user = factories["users.User"]()
     upload = factories["music.Upload"](bitrate=42)
     response = views.handle_serve(upload, user, format="mp3")
-
+    expected_filename = upload.track.full_name + ".mp3"
     version = upload.versions.latest("id")
 
     assert version.mimetype == "audio/mpeg"
@@ -421,7 +422,9 @@ def test_handle_serve_create_mp3_version(factories, now):
     assert version.audio_file_path.endswith(".mp3")
     assert version.size == version.audio_file.size
     assert magic.from_buffer(version.audio_file.read(), mime=True) == "audio/mpeg"
-
+    assert response["Content-Disposition"] == "attachment; filename*=UTF-8''{}".format(
+        urllib.parse.quote(expected_filename)
+    )
     assert response.status_code == 200
 
 

changes/changelog.d/848.bugfix

@@ -0,0 +1 @@
+Fixed invalid file extension for transcoded tracks (#848)

changes/changelog.d/850.bugfix

@@ -0,0 +1 @@
+Ensure empty but optional fields in file metadata don't error during import (#850)

changes/changelog.d/851.bugfix

@@ -0,0 +1 @@
+Fixed wrong og:image url when using S3 storage (#851)