From 81349e2b57cd0e8c4a6d9bb4d8a2ed2fe3c7f100 Mon Sep 17 00:00:00 2001
From: Eliot Berriot <contact@eliotberriot.com>
Date: Tue, 24 Sep 2019 09:48:04 +0200
Subject: [PATCH] Fix #865: Performance optimization on /artists, /albums and
/tracks endpoints
---
api/funkwhale_api/music/serializers.py | 128 ++++++++++++-------------
api/funkwhale_api/music/views.py | 10 +-
api/tests/music/test_serializers.py | 24 ++---
api/tests/music/test_views.py | 6 +-
changes/changelog.d/865.bugfix | 1 +
5 files changed, 88 insertions(+), 81 deletions(-)
create mode 100644 changes/changelog.d/865.bugfix
diff --git a/api/funkwhale_api/music/serializers.py b/api/funkwhale_api/music/serializers.py
index 630f9f4e1..77854d437 100644
--- a/api/funkwhale_api/music/serializers.py
+++ b/api/funkwhale_api/music/serializers.py
@@ -104,58 +104,50 @@ class ArtistWithAlbumsSerializer(serializers.ModelSerializer):
get_attributed_to = serialize_attributed_to
def get_tracks_count(self, o):
- return getattr(o, "_tracks_count", None)
-
-
-class ArtistSimpleSerializer(serializers.ModelSerializer):
- class Meta:
- model = models.Artist
- fields = ("id", "fid", "mbid", "name", "creation_date", "is_local")
-
-
-class AlbumTrackSerializer(serializers.ModelSerializer):
- artist = ArtistSimpleSerializer(read_only=True)
- uploads = serializers.SerializerMethodField()
- listen_url = serializers.SerializerMethodField()
- duration = serializers.SerializerMethodField()
-
- class Meta:
- model = models.Track
- fields = (
- "id",
- "fid",
- "mbid",
- "title",
- "album",
- "artist",
- "creation_date",
- "position",
- "disc_number",
- "uploads",
- "listen_url",
- "duration",
- "copyright",
- "license",
- "is_local",
- )
-
- def get_uploads(self, obj):
- uploads = getattr(obj, "playable_uploads", [])
- return TrackUploadSerializer(uploads, many=True).data
-
- def get_listen_url(self, obj):
- return obj.listen_url
-
- def get_duration(self, obj):
- try:
- return obj.duration
- except AttributeError:
- return None
+ tracks = getattr(o, "_prefetched_tracks", None)
+ return len(tracks) if tracks else None
+
+
+def serialize_artist_simple(artist):
+ return {
+ "id": artist.id,
+ "fid": artist.fid,
+ "mbid": str(artist.mbid),
+ "name": artist.name,
+ "creation_date": serializers.DateTimeField().to_representation(
+ artist.creation_date
+ ),
+ "is_local": artist.is_local,
+ }
+
+
+def serialize_album_track(track):
+ return {
+ "id": track.id,
+ "fid": track.fid,
+ "mbid": str(track.mbid),
+ "title": track.title,
+ "artist": serialize_artist_simple(track.artist),
+ "album": track.album_id,
+ "creation_date": serializers.DateTimeField().to_representation(
+ track.creation_date
+ ),
+ "position": track.position,
+ "disc_number": track.disc_number,
+ "uploads": [
+ serialize_upload(u) for u in getattr(track, "playable_uploads", [])
+ ],
+ "listen_url": track.listen_url,
+ "duration": getattr(track, "duration", None),
+ "copyright": track.copyright,
+ "license": track.license_id,
+ "is_local": track.is_local,
+ }
class AlbumSerializer(serializers.ModelSerializer):
tracks = serializers.SerializerMethodField()
- artist = ArtistSimpleSerializer(read_only=True)
+ artist = serializers.SerializerMethodField()
cover = cover_field
is_playable = serializers.SerializerMethodField()
tags = serializers.SerializerMethodField()
@@ -181,9 +173,12 @@ class AlbumSerializer(serializers.ModelSerializer):
get_attributed_to = serialize_attributed_to
+ def get_artist(self, o):
+ return serialize_artist_simple(o.artist)
+
def get_tracks(self, o):
ordered_tracks = o.tracks.all()
- return AlbumTrackSerializer(ordered_tracks, many=True).data
+ return [serialize_album_track(track) for track in ordered_tracks]
def get_is_playable(self, obj):
try:
@@ -199,7 +194,7 @@ class AlbumSerializer(serializers.ModelSerializer):
class TrackAlbumSerializer(serializers.ModelSerializer):
- artist = ArtistSimpleSerializer(read_only=True)
+ artist = serializers.SerializerMethodField()
cover = cover_field
class Meta:
@@ -216,23 +211,24 @@ class TrackAlbumSerializer(serializers.ModelSerializer):
"is_local",
)
+ def get_artist(self, o):
+ return serialize_artist_simple(o.artist)
-class TrackUploadSerializer(serializers.ModelSerializer):
- class Meta:
- model = models.Upload
- fields = (
- "uuid",
- "listen_url",
- "size",
- "duration",
- "bitrate",
- "mimetype",
- "extension",
- )
+
+def serialize_upload(upload):
+ return {
+ "uuid": str(upload.uuid),
+ "listen_url": upload.listen_url,
+ "size": upload.size,
+ "duration": upload.duration,
+ "bitrate": upload.bitrate,
+ "mimetype": upload.mimetype,
+ "extension": upload.extension,
+ }
class TrackSerializer(serializers.ModelSerializer):
- artist = ArtistSimpleSerializer(read_only=True)
+ artist = serializers.SerializerMethodField()
album = TrackAlbumSerializer(read_only=True)
uploads = serializers.SerializerMethodField()
listen_url = serializers.SerializerMethodField()
@@ -262,12 +258,14 @@ class TrackSerializer(serializers.ModelSerializer):
get_attributed_to = serialize_attributed_to
+ def get_artist(self, o):
+ return serialize_artist_simple(o.artist)
+
def get_listen_url(self, obj):
return obj.listen_url
def get_uploads(self, obj):
- uploads = getattr(obj, "playable_uploads", [])
- return TrackUploadSerializer(uploads, many=True).data
+ return [serialize_upload(u) for u in getattr(obj, "playable_uploads", [])]
def get_tags(self, obj):
tagged_items = getattr(obj, "_prefetched_tagged_items", [])
diff --git a/api/funkwhale_api/music/views.py b/api/funkwhale_api/music/views.py
index 33d3663c1..4783a8d45 100644
--- a/api/funkwhale_api/music/views.py
+++ b/api/funkwhale_api/music/views.py
@@ -96,8 +96,14 @@ def refetch_obj(obj, queryset):
class ArtistViewSet(common_views.SkipFilterForGetObject, viewsets.ReadOnlyModelViewSet):
queryset = (
models.Artist.objects.all()
- .select_related("attributed_to")
- .annotate(_tracks_count=Count("tracks"))
+ .prefetch_related("attributed_to")
+ .prefetch_related(
+ Prefetch(
+ "tracks",
+ queryset=models.Track.objects.all(),
+ to_attr="_prefetched_tracks",
+ )
+ )
)
serializer_class = serializers.ArtistWithAlbumsSerializer
permission_classes = [oauth_permissions.ScopePermission]
diff --git a/api/tests/music/test_serializers.py b/api/tests/music/test_serializers.py
index 333881037..4eaf54ad5 100644
--- a/api/tests/music/test_serializers.py
+++ b/api/tests/music/test_serializers.py
@@ -62,7 +62,7 @@ def test_artist_with_albums_serializer(factories, to_api_date):
artist = track.artist
artist = artist.__class__.objects.with_albums().get(pk=artist.pk)
album = list(artist.albums.all())[0]
- setattr(artist, "_tracks_count", 42)
+ setattr(artist, "_prefetched_tracks", range(42))
expected = {
"id": artist.id,
"fid": artist.fid,
@@ -89,13 +89,13 @@ def test_album_track_serializer(factories, to_api_date):
expected = {
"id": track.id,
"fid": track.fid,
- "artist": serializers.ArtistSimpleSerializer(track.artist).data,
+ "artist": serializers.serialize_artist_simple(track.artist),
"album": track.album.id,
"mbid": str(track.mbid),
"title": track.title,
"position": track.position,
"disc_number": track.disc_number,
- "uploads": [serializers.TrackUploadSerializer(upload).data],
+ "uploads": [serializers.serialize_upload(upload)],
"creation_date": to_api_date(track.creation_date),
"listen_url": track.listen_url,
"duration": None,
@@ -103,8 +103,8 @@ def test_album_track_serializer(factories, to_api_date):
"copyright": track.copyright,
"is_local": track.is_local,
}
- serializer = serializers.AlbumTrackSerializer(track)
- assert serializer.data == expected
+ data = serializers.serialize_album_track(track)
+ assert data == expected
def test_upload_serializer(factories, to_api_date):
@@ -169,7 +169,7 @@ def test_album_serializer(factories, to_api_date):
"fid": album.fid,
"mbid": str(album.mbid),
"title": album.title,
- "artist": serializers.ArtistSimpleSerializer(album.artist).data,
+ "artist": serializers.serialize_artist_simple(album.artist),
"creation_date": to_api_date(album.creation_date),
"is_playable": False,
"cover": {
@@ -179,7 +179,7 @@ def test_album_serializer(factories, to_api_date):
"small_square_crop": album.cover.crop["50x50"].url,
},
"release_date": to_api_date(album.release_date),
- "tracks": serializers.AlbumTrackSerializer([track2, track1], many=True).data,
+ "tracks": [serializers.serialize_album_track(t) for t in [track2, track1]],
"is_local": album.is_local,
"tags": [],
"attributed_to": federation_serializers.APIActorSerializer(actor).data,
@@ -202,13 +202,13 @@ def test_track_serializer(factories, to_api_date):
expected = {
"id": track.id,
"fid": track.fid,
- "artist": serializers.ArtistSimpleSerializer(track.artist).data,
+ "artist": serializers.serialize_artist_simple(track.artist),
"album": serializers.TrackAlbumSerializer(track.album).data,
"mbid": str(track.mbid),
"title": track.title,
"position": track.position,
"disc_number": track.disc_number,
- "uploads": [serializers.TrackUploadSerializer(upload).data],
+ "uploads": [serializers.serialize_upload(upload)],
"creation_date": to_api_date(track.creation_date),
"listen_url": track.listen_url,
"license": upload.track.license.code,
@@ -317,7 +317,7 @@ def test_manage_upload_action_relaunch_import(factories, mocker):
assert m.call_count == 3
-def test_track_upload_serializer(factories):
+def test_serialize_upload(factories):
upload = factories["music.Upload"]()
expected = {
@@ -330,8 +330,8 @@ def test_track_upload_serializer(factories):
"duration": upload.duration,
}
- serializer = serializers.TrackUploadSerializer(upload)
- assert serializer.data == expected
+ data = serializers.serialize_upload(upload)
+ assert data == expected
@pytest.mark.parametrize(
diff --git a/api/tests/music/test_views.py b/api/tests/music/test_views.py
index b9064cb05..4a423ddb1 100644
--- a/api/tests/music/test_views.py
+++ b/api/tests/music/test_views.py
@@ -6,7 +6,7 @@ import urllib.parse
import uuid
import pytest
-from django.db.models import Count
+from django.db.models import Prefetch
from django.urls import reverse
from django.utils import timezone
@@ -28,7 +28,9 @@ def test_artist_list_serializer(api_request, factories, logged_in_api_client):
).track
artist = track.artist
request = api_request.get("/")
- qs = artist.__class__.objects.with_albums().annotate(_tracks_count=Count("tracks"))
+ qs = artist.__class__.objects.with_albums().prefetch_related(
+ Prefetch("tracks", to_attr="_prefetched_tracks")
+ )
serializer = serializers.ArtistWithAlbumsSerializer(
qs, many=True, context={"request": request}
)
diff --git a/changes/changelog.d/865.bugfix b/changes/changelog.d/865.bugfix
new file mode 100644
index 000000000..183530e0d
--- /dev/null
+++ b/changes/changelog.d/865.bugfix
@@ -0,0 +1 @@
+Improved performance of /artists, /albums and /tracks API endpoints by a factor 2 (#865)
--
GitLab