From 55f4fde0f4edda981c18dac1e6e759cb26f39e47 Mon Sep 17 00:00:00 2001
From: Agate <me@agate.blue>
Date: Mon, 6 Jul 2020 10:16:45 +0200
Subject: [PATCH] Fix #1102: Do not include tracks in album API representation
---
api/funkwhale_api/manage/serializers.py | 7 ++--
api/funkwhale_api/manage/views.py | 2 +-
api/funkwhale_api/music/models.py | 4 ---
api/funkwhale_api/music/serializers.py | 30 ++--------------
api/funkwhale_api/music/views.py | 10 ++----
api/tests/manage/test_serializers.py | 4 +--
api/tests/music/test_serializers.py | 36 +++----------------
api/tests/music/test_views.py | 2 +-
changes/changelog.d/1102.enhancement | 1 +
changes/notes.rst | 8 +++++
docs/api/definitions.yml | 7 ++--
.../src/components/audio/ChannelSerieCard.vue | 4 +--
front/src/components/audio/album/Card.vue | 2 +-
front/src/components/channels/AlbumSelect.vue | 2 +-
front/src/components/library/AlbumBase.vue | 2 +-
.../components/manage/library/AlbumsTable.vue | 2 +-
16 files changed, 37 insertions(+), 86 deletions(-)
create mode 100644 changes/changelog.d/1102.enhancement
diff --git a/api/funkwhale_api/manage/serializers.py b/api/funkwhale_api/manage/serializers.py
index d29433e566..d8cab97312 100644
--- a/api/funkwhale_api/manage/serializers.py
+++ b/api/funkwhale_api/manage/serializers.py
@@ -336,6 +336,7 @@ class ManageBaseArtistSerializer(serializers.ModelSerializer):
class ManageBaseAlbumSerializer(serializers.ModelSerializer):
cover = music_serializers.cover_field
domain = serializers.CharField(source="domain_name")
+ tracks_count = serializers.SerializerMethodField()
class Meta:
model = music_models.Album
@@ -349,8 +350,12 @@ class ManageBaseAlbumSerializer(serializers.ModelSerializer):
"cover",
"domain",
"is_local",
+ "tracks_count",
]
+ def get_tracks_count(self, o):
+ return getattr(o, "_tracks_count", None)
+
class ManageNestedTrackSerializer(serializers.ModelSerializer):
domain = serializers.CharField(source="domain_name")
@@ -428,7 +433,6 @@ class ManageNestedArtistSerializer(ManageBaseArtistSerializer):
class ManageAlbumSerializer(
music_serializers.OptionalDescriptionMixin, ManageBaseAlbumSerializer
):
- tracks = ManageNestedTrackSerializer(many=True)
attributed_to = ManageBaseActorSerializer()
artist = ManageNestedArtistSerializer()
tags = serializers.SerializerMethodField()
@@ -437,7 +441,6 @@ class ManageAlbumSerializer(
model = music_models.Album
fields = ManageBaseAlbumSerializer.Meta.fields + [
"artist",
- "tracks",
"attributed_to",
"tags",
]
diff --git a/api/funkwhale_api/manage/views.py b/api/funkwhale_api/manage/views.py
index adb7128e26..bc3908daee 100644
--- a/api/funkwhale_api/manage/views.py
+++ b/api/funkwhale_api/manage/views.py
@@ -128,7 +128,7 @@ class ManageAlbumViewSet(
music_models.Album.objects.all()
.order_by("-id")
.select_related("attributed_to", "artist", "attachment_cover")
- .prefetch_related("tracks", music_views.TAG_PREFETCH)
+ .with_tracks_count()
)
serializer_class = serializers.ManageAlbumSerializer
filterset_class = filters.ManageAlbumFilterSet
diff --git a/api/funkwhale_api/music/models.py b/api/funkwhale_api/music/models.py
index e020a619b1..1d32a01bd9 100644
--- a/api/funkwhale_api/music/models.py
+++ b/api/funkwhale_api/music/models.py
@@ -319,10 +319,6 @@ class AlbumQuerySet(common_models.LocalFromFidQuerySet, models.QuerySet):
else:
return self.exclude(pk__in=matches)
- def with_prefetched_tracks_and_playable_uploads(self, actor):
- tracks = Track.objects.with_playable_uploads(actor)
- return self.prefetch_related(models.Prefetch("tracks", queryset=tracks))
-
class Album(APIModelMixin):
title = models.CharField(max_length=MAX_LENGTHS["ALBUM_TITLE"])
diff --git a/api/funkwhale_api/music/serializers.py b/api/funkwhale_api/music/serializers.py
index 6abd6e2626..7dfef3bcac 100644
--- a/api/funkwhale_api/music/serializers.py
+++ b/api/funkwhale_api/music/serializers.py
@@ -187,35 +187,12 @@ def serialize_artist_simple(artist):
return data
-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": DATETIME_FIELD.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(OptionalDescriptionMixin, serializers.Serializer):
- # XXX: remove in 1.0, it's expensive and can work with a filter/api call
- tracks = serializers.SerializerMethodField()
artist = serializers.SerializerMethodField()
cover = cover_field
is_playable = serializers.SerializerMethodField()
tags = serializers.SerializerMethodField()
+ tracks_count = serializers.SerializerMethodField()
attributed_to = serializers.SerializerMethodField()
id = serializers.IntegerField()
fid = serializers.URLField()
@@ -232,9 +209,8 @@ class AlbumSerializer(OptionalDescriptionMixin, serializers.Serializer):
def get_artist(self, o):
return serialize_artist_simple(o.artist)
- def get_tracks(self, o):
- ordered_tracks = o.tracks.all()
- return [serialize_album_track(track) for track in ordered_tracks]
+ def get_tracks_count(self, o):
+ return getattr(o, "_tracks_count", None)
def get_is_playable(self, obj):
try:
diff --git a/api/funkwhale_api/music/views.py b/api/funkwhale_api/music/views.py
index c34d9b175e..5a4e81da4b 100644
--- a/api/funkwhale_api/music/views.py
+++ b/api/funkwhale_api/music/views.py
@@ -181,6 +181,7 @@ class AlbumViewSet(
queryset = (
models.Album.objects.all()
.order_by("-creation_date")
+ .with_tracks_count()
.prefetch_related("artist", "attributed_to", "attachment_cover")
)
serializer_class = serializers.AlbumSerializer
@@ -217,14 +218,7 @@ class AlbumViewSet(
queryset = queryset.exclude(artist__channel=None).filter(
artist__attributed_to=self.request.user.actor
)
- tracks = (
- models.Track.objects.prefetch_related("artist")
- .with_playable_uploads(utils.get_actor_from_request(self.request))
- .order_for_album()
- )
- qs = queryset.prefetch_related(
- Prefetch("tracks", queryset=tracks), TAG_PREFETCH
- )
+ qs = queryset.prefetch_related(TAG_PREFETCH)
return qs
libraries = action(methods=["get"], detail=True)(
diff --git a/api/tests/manage/test_serializers.py b/api/tests/manage/test_serializers.py
index c4dbaa45ed..d6de65a47e 100644
--- a/api/tests/manage/test_serializers.py
+++ b/api/tests/manage/test_serializers.py
@@ -373,7 +373,7 @@ def test_manage_nested_artist_serializer(factories, now, to_api_date):
def test_manage_album_serializer(factories, now, to_api_date):
album = factories["music.Album"](attributed=True, with_cover=True)
- track = factories["music.Track"](album=album)
+ setattr(album, "_tracks_count", 42)
expected = {
"id": album.id,
"domain": album.domain_name,
@@ -385,11 +385,11 @@ def test_manage_album_serializer(factories, now, to_api_date):
"release_date": album.release_date.isoformat(),
"cover": common_serializers.AttachmentSerializer(album.attachment_cover).data,
"artist": serializers.ManageNestedArtistSerializer(album.artist).data,
- "tracks": [serializers.ManageNestedTrackSerializer(track).data],
"attributed_to": serializers.ManageBaseActorSerializer(
album.attributed_to
).data,
"tags": [],
+ "tracks_count": 42,
}
s = serializers.ManageAlbumSerializer(album)
diff --git a/api/tests/music/test_serializers.py b/api/tests/music/test_serializers.py
index 5361950e1b..9500a2c6a2 100644
--- a/api/tests/music/test_serializers.py
+++ b/api/tests/music/test_serializers.py
@@ -115,34 +115,6 @@ def test_artist_with_albums_serializer_channel(factories, to_api_date):
assert serializer.data == expected
-def test_album_track_serializer(factories, to_api_date):
- upload = factories["music.Upload"](
- track__license="cc-by-4.0", track__copyright="test", track__disc_number=2
- )
- track = upload.track
- setattr(track, "playable_uploads", [upload])
-
- expected = {
- "id": track.id,
- "fid": track.fid,
- "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.serialize_upload(upload)],
- "creation_date": to_api_date(track.creation_date),
- "listen_url": track.listen_url,
- "duration": None,
- "license": track.license.code,
- "copyright": track.copyright,
- "is_local": track.is_local,
- }
- data = serializers.serialize_album_track(track)
- assert data == expected
-
-
def test_upload_serializer(factories, to_api_date):
upload = factories["music.Upload"]()
@@ -200,7 +172,7 @@ def test_album_serializer(factories, to_api_date):
track1 = factories["music.Track"](
position=2, album__attributed_to=actor, album__with_cover=True
)
- track2 = factories["music.Track"](position=1, album=track1.album)
+ factories["music.Track"](position=1, album=track1.album)
album = track1.album
expected = {
"id": album.id,
@@ -212,12 +184,14 @@ def test_album_serializer(factories, to_api_date):
"is_playable": False,
"cover": common_serializers.AttachmentSerializer(album.attachment_cover).data,
"release_date": to_api_date(album.release_date),
- "tracks": [serializers.serialize_album_track(t) for t in [track2, track1]],
+ "tracks_count": 2,
"is_local": album.is_local,
"tags": [],
"attributed_to": federation_serializers.APIActorSerializer(actor).data,
}
- serializer = serializers.AlbumSerializer(album)
+ serializer = serializers.AlbumSerializer(
+ album.__class__.objects.with_tracks_count().get(pk=album.pk)
+ )
assert serializer.data == expected
diff --git a/api/tests/music/test_views.py b/api/tests/music/test_views.py
index 5e3c9c953b..2fa20aa1da 100644
--- a/api/tests/music/test_views.py
+++ b/api/tests/music/test_views.py
@@ -57,7 +57,7 @@ def test_album_list_serializer(api_request, factories, logged_in_api_client):
).track
album = track.album
request = api_request.get("/")
- qs = album.__class__.objects.with_prefetched_tracks_and_playable_uploads(None)
+ qs = album.__class__.objects.with_tracks_count()
serializer = serializers.AlbumSerializer(
qs, many=True, context={"request": request}
)
diff --git a/changes/changelog.d/1102.enhancement b/changes/changelog.d/1102.enhancement
new file mode 100644
index 0000000000..6c80b79381
--- /dev/null
+++ b/changes/changelog.d/1102.enhancement
@@ -0,0 +1 @@
+Do not include tracks in album API representation (#1102)
\ No newline at end of file
diff --git a/changes/notes.rst b/changes/notes.rst
index 8d84dc0262..a95ffd69af 100644
--- a/changes/notes.rst
+++ b/changes/notes.rst
@@ -31,3 +31,11 @@ Now, it returns all the accessible libraries (including ones from other users an
If you are consuming the API via a third-party client and need to retrieve your libraries,
use the ``scope`` parameter, like this: ``GET /api/v1/libraries?scope=me``
+
+API breaking change in ``/api/v1/albums``
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+To increase performance, querying ``/api/v1/albums`` doesn't return album tracks anymore. This caused
+some performance issues, especially as some albums and series have dozens or even hundreds of tracks.
+
+If you want to retrieve tracks for an album, you can query ``/api/v1/tracks/?album=<albumid>``.
\ No newline at end of file
diff --git a/docs/api/definitions.yml b/docs/api/definitions.yml
index 4903b2c390..6cc41dccba 100644
--- a/docs/api/definitions.yml
+++ b/docs/api/definitions.yml
@@ -198,10 +198,9 @@ Album:
- $ref: "#/BaseAlbum"
- type: "object"
properties:
- tracks:
- type: "array"
- items:
- $ref: "#/AlbumTrack"
+ tracks_count:
+ type: "integer"
+ format: "int64"
ArtistAlbum:
type: "object"
diff --git a/front/src/components/audio/ChannelSerieCard.vue b/front/src/components/audio/ChannelSerieCard.vue
index 5a43e02dcc..2ce1764513 100644
--- a/front/src/components/audio/ChannelSerieCard.vue
+++ b/front/src/components/audio/ChannelSerieCard.vue
@@ -15,8 +15,8 @@
<div class="description">
<translate translate-context="Content/Channel/Paragraph"
translate-plural="%{ count } episodes"
- :translate-n="serie.tracks.length"
- :translate-params="{count: serie.tracks.length}">
+ :translate-n="serie.tracks_count"
+ :translate-params="{count: serie.tracks_count}">
%{ count } episode
</translate>
</div>
diff --git a/front/src/components/audio/album/Card.vue b/front/src/components/audio/album/Card.vue
index c619a90537..63f418c0f6 100644
--- a/front/src/components/audio/album/Card.vue
+++ b/front/src/components/audio/album/Card.vue
@@ -20,7 +20,7 @@
</div>
</div>
<div class="extra content">
- <translate translate-context="*/*/*" :translate-params="{count: album.tracks.length}" :translate-n="album.tracks.length" translate-plural="%{ count } tracks">%{ count } track</translate>
+ <translate translate-context="*/*/*" :translate-params="{count: album.tracks_count}" :translate-n="album.tracks_count" translate-plural="%{ count } tracks">%{ count } track</translate>
<play-button class="right floated basic icon" :dropdown-only="true" :is-playable="album.is_playable" :dropdown-icon-classes="['ellipsis', 'horizontal', 'large really discrete']" :album="album"></play-button>
</div>
</div>
diff --git a/front/src/components/channels/AlbumSelect.vue b/front/src/components/channels/AlbumSelect.vue
index ed4b493e88..0beafbbc72 100644
--- a/front/src/components/channels/AlbumSelect.vue
+++ b/front/src/components/channels/AlbumSelect.vue
@@ -9,7 +9,7 @@
<translate translate-context="*/*/*">None</translate>
</option>
<option v-for="album in albums" :key="album.id" :value="album.id">
- {{ album.title }} (<translate translate-context="*/*/*" :translate-params="{count: album.tracks.length}" :translate-n="album.tracks.length" translate-plural="%{ count } tracks">%{ count } track</translate>)
+ {{ album.title }} (<translate translate-context="*/*/*" :translate-params="{count: album.tracks_count}" :translate-n="album.tracks_count" translate-plural="%{ count } tracks">%{ count } track</translate>)
</option>
</select>
</div>
diff --git a/front/src/components/library/AlbumBase.vue b/front/src/components/library/AlbumBase.vue
index 104280a76f..071b133574 100644
--- a/front/src/components/library/AlbumBase.vue
+++ b/front/src/components/library/AlbumBase.vue
@@ -195,7 +195,7 @@ export default {
},
computed: {
totalTracks () {
- return this.object.tracks.length
+ return this.object.tracks_count
},
isChannel () {
return this.object.artist.channel
diff --git a/front/src/components/manage/library/AlbumsTable.vue b/front/src/components/manage/library/AlbumsTable.vue
index af7c82d1af..72f5aee4c5 100644
--- a/front/src/components/manage/library/AlbumsTable.vue
+++ b/front/src/components/manage/library/AlbumsTable.vue
@@ -67,7 +67,7 @@
</span>
</td>
<td>
- {{ scope.obj.tracks.length }}
+ {{ scope.obj.tracks_count }}
</td>
<td>
<human-date v-if="scope.obj.release_date" :date="scope.obj.release_date"></human-date>
--
GitLab