From 857fab526d383e6f355f7dad287fd07187a9c91e Mon Sep 17 00:00:00 2001 From: Eliot Berriot <contact@eliotberriot.com> Date: Tue, 22 May 2018 21:10:48 +0200 Subject: [PATCH] See #224: less clutered music API, better serializers --- api/funkwhale_api/favorites/serializers.py | 2 - api/funkwhale_api/history/serializers.py | 1 - api/funkwhale_api/history/views.py | 1 - api/funkwhale_api/music/models.py | 6 + api/funkwhale_api/music/serializers.py | 140 ++++++++++++++------- api/funkwhale_api/music/views.py | 41 ++---- api/funkwhale_api/playlists/serializers.py | 4 +- api/funkwhale_api/radios/serializers.py | 4 +- api/funkwhale_api/radios/views.py | 6 +- api/tests/conftest.py | 15 +++ api/tests/music/test_api.py | 35 ------ api/tests/music/test_serializers.py | 121 ++++++++++++++++++ api/tests/music/test_views.py | 60 +++++++++ api/tests/radios/test_api.py | 4 +- 14 files changed, 318 insertions(+), 122 deletions(-) create mode 100644 api/tests/music/test_serializers.py diff --git a/api/funkwhale_api/favorites/serializers.py b/api/funkwhale_api/favorites/serializers.py index 276b0f6b..bb4538b2 100644 --- a/api/funkwhale_api/favorites/serializers.py +++ b/api/funkwhale_api/favorites/serializers.py @@ -3,7 +3,6 @@ from django.conf import settings from rest_framework import serializers from funkwhale_api.activity import serializers as activity_serializers -from funkwhale_api.music.serializers import TrackSerializerNested from funkwhale_api.music.serializers import TrackActivitySerializer from funkwhale_api.users.serializers import UserActivitySerializer @@ -35,7 +34,6 @@ class TrackFavoriteActivitySerializer(activity_serializers.ModelSerializer): class UserTrackFavoriteSerializer(serializers.ModelSerializer): - # track = TrackSerializerNested(read_only=True) class Meta: model = models.TrackFavorite fields = ('id', 'track', 'creation_date') diff --git a/api/funkwhale_api/history/serializers.py b/api/funkwhale_api/history/serializers.py index f7333f24..572787ae 100644 --- a/api/funkwhale_api/history/serializers.py +++ b/api/funkwhale_api/history/serializers.py @@ -1,7 +1,6 @@ from rest_framework import serializers from funkwhale_api.activity import serializers as activity_serializers -from funkwhale_api.music.serializers import TrackSerializerNested from funkwhale_api.music.serializers import TrackActivitySerializer from funkwhale_api.users.serializers import UserActivitySerializer diff --git a/api/funkwhale_api/history/views.py b/api/funkwhale_api/history/views.py index bea96a41..3da8b2a3 100644 --- a/api/funkwhale_api/history/views.py +++ b/api/funkwhale_api/history/views.py @@ -6,7 +6,6 @@ from rest_framework.decorators import detail_route from funkwhale_api.activity import record from funkwhale_api.common.permissions import ConditionalAuthentication -from funkwhale_api.music.serializers import TrackSerializerNested from . import models from . import serializers diff --git a/api/funkwhale_api/music/models.py b/api/funkwhale_api/music/models.py index 1259cc3c..51ccec90 100644 --- a/api/funkwhale_api/music/models.py +++ b/api/funkwhale_api/music/models.py @@ -80,6 +80,12 @@ class ArtistQuerySet(models.QuerySet): def with_albums_count(self): return self.annotate(_albums_count=models.Count('albums')) + def with_albums(self): + return self.prefetch_related( + models.Prefetch( + 'albums', queryset=Album.objects.with_tracks_count()) + ) + class Artist(APIModelMixin): name = models.CharField(max_length=255) diff --git a/api/funkwhale_api/music/serializers.py b/api/funkwhale_api/music/serializers.py index d9d48496..04264692 100644 --- a/api/funkwhale_api/music/serializers.py +++ b/api/funkwhale_api/music/serializers.py @@ -13,24 +13,38 @@ from . import models from . import tasks -class TagSerializer(serializers.ModelSerializer): - class Meta: - model = Tag - fields = ('id', 'name', 'slug') - +class ArtistAlbumSerializer(serializers.ModelSerializer): + tracks_count = serializers.SerializerMethodField() -class SimpleArtistSerializer(serializers.ModelSerializer): class Meta: - model = models.Artist - fields = ('id', 'mbid', 'name', 'creation_date') + model = models.Album + fields = ( + 'id', + 'mbid', + 'title', + 'artist', + 'release_date', + 'cover', + 'creation_date', + 'tracks_count', + ) + def get_tracks_count(self, o): + return o._tracks_count -class ArtistSerializer(serializers.ModelSerializer): - tags = TagSerializer(many=True, read_only=True) + +class ArtistWithAlbumsSerializer(serializers.ModelSerializer): + albums = ArtistAlbumSerializer(many=True, read_only=True) class Meta: model = models.Artist - fields = ('id', 'mbid', 'name', 'tags', 'creation_date') + fields = ( + 'id', + 'mbid', + 'name', + 'creation_date', + 'albums', + ) class TrackFileSerializer(serializers.ModelSerializer): @@ -62,71 +76,107 @@ class TrackFileSerializer(serializers.ModelSerializer): return url -class SimpleAlbumSerializer(serializers.ModelSerializer): +class AlbumTrackSerializer(serializers.ModelSerializer): + files = TrackFileSerializer(many=True, read_only=True) class Meta: - model = models.Album - fields = ('id', 'mbid', 'title', 'release_date', 'cover') + model = models.Track + fields = ( + 'id', + 'mbid', + 'title', + 'album', + 'artist', + 'creation_date', + 'files', + 'position', + ) + + +class ArtistSimpleSerializer(serializers.ModelSerializer): + class Meta: + model = models.Artist + fields = ( + 'id', + 'mbid', + 'name', + 'creation_date', + ) class AlbumSerializer(serializers.ModelSerializer): - tags = TagSerializer(many=True, read_only=True) + tracks = serializers.SerializerMethodField() + artist = ArtistSimpleSerializer(read_only=True) + class Meta: model = models.Album - fields = ('id', 'mbid', 'title', 'cover', 'release_date', 'tags') - + fields = ( + 'id', + 'mbid', + 'title', + 'artist', + 'tracks', + 'release_date', + 'cover', + 'creation_date', + ) -class LyricsMixin(serializers.ModelSerializer): - lyrics = serializers.SerializerMethodField() + def get_tracks(self, o): + ordered_tracks = sorted(o.tracks.all(), key=lambda v: v.position) + return AlbumTrackSerializer(ordered_tracks, many=True).data - def get_lyrics(self, obj): - return obj.get_lyrics_url() +class TrackAlbumSerializer(serializers.ModelSerializer): + artist = ArtistSimpleSerializer(read_only=True) -class TrackSerializer(LyricsMixin): - files = TrackFileSerializer(many=True, read_only=True) - tags = TagSerializer(many=True, read_only=True) class Meta: - model = models.Track + model = models.Album fields = ( 'id', 'mbid', 'title', 'artist', - 'files', - 'tags', - 'position', - 'lyrics') + 'release_date', + 'cover', + 'creation_date', + ) -class TrackSerializerNested(LyricsMixin): - artist = ArtistSerializer() +class TrackSerializer(serializers.ModelSerializer): files = TrackFileSerializer(many=True, read_only=True) - album = SimpleAlbumSerializer(read_only=True) - tags = TagSerializer(many=True, read_only=True) + artist = ArtistSimpleSerializer(read_only=True) + album = TrackAlbumSerializer(read_only=True) + lyrics = serializers.SerializerMethodField() class Meta: model = models.Track - fields = ('id', 'mbid', 'title', 'artist', 'files', 'album', 'tags', 'lyrics') + fields = ( + 'id', + 'mbid', + 'title', + 'album', + 'artist', + 'creation_date', + 'files', + 'position', + 'lyrics', + ) + def get_lyrics(self, obj): + return obj.get_lyrics_url() -class AlbumSerializerNested(serializers.ModelSerializer): - tracks = TrackSerializer(many=True, read_only=True) - artist = SimpleArtistSerializer() - tags = TagSerializer(many=True, read_only=True) +class TagSerializer(serializers.ModelSerializer): class Meta: - model = models.Album - fields = ('id', 'mbid', 'title', 'cover', 'artist', 'release_date', 'tracks', 'tags') + model = Tag + fields = ('id', 'name', 'slug') -class ArtistSerializerNested(serializers.ModelSerializer): - albums = AlbumSerializerNested(many=True, read_only=True) - tags = TagSerializer(many=True, read_only=True) +class SimpleAlbumSerializer(serializers.ModelSerializer): class Meta: - model = models.Artist - fields = ('id', 'mbid', 'name', 'albums', 'tags') + model = models.Album + fields = ('id', 'mbid', 'title', 'release_date', 'cover') class LyricsSerializer(serializers.ModelSerializer): diff --git a/api/funkwhale_api/music/views.py b/api/funkwhale_api/music/views.py index e71d3555..3a03a922 100644 --- a/api/funkwhale_api/music/views.py +++ b/api/funkwhale_api/music/views.py @@ -68,13 +68,8 @@ class TagViewSetMixin(object): class ArtistViewSet(SearchMixin, viewsets.ReadOnlyModelViewSet): - queryset = ( - models.Artist.objects.all() - .prefetch_related( - 'albums__tracks__files', - 'albums__tracks__artist', - 'albums__tracks__tags')) - serializer_class = serializers.ArtistSerializerNested + queryset = models.Artist.objects.with_albums() + serializer_class = serializers.ArtistWithAlbumsSerializer permission_classes = [ConditionalAuthentication] search_fields = ['name__unaccent'] filter_class = filters.ArtistFilter @@ -88,7 +83,7 @@ class AlbumViewSet(SearchMixin, viewsets.ReadOnlyModelViewSet): .select_related() .prefetch_related('tracks__tags', 'tracks__files')) - serializer_class = serializers.AlbumSerializerNested + serializer_class = serializers.AlbumSerializer permission_classes = [ConditionalAuthentication] search_fields = ['title__unaccent'] ordering_fields = ('creation_date',) @@ -166,7 +161,7 @@ class TrackViewSet( A simple ViewSet for viewing and editing accounts. """ queryset = (models.Track.objects.all().for_nested_serialization()) - serializer_class = serializers.TrackSerializerNested + serializer_class = serializers.TrackSerializer permission_classes = [ConditionalAuthentication] search_fields = ['title', 'artist__name'] ordering_fields = ( @@ -370,10 +365,10 @@ class Search(views.APIView): def get(self, request, *args, **kwargs): query = request.GET['query'] results = { - 'tags': serializers.TagSerializer(self.get_tags(query), many=True).data, - 'artists': serializers.ArtistSerializerNested(self.get_artists(query), many=True).data, - 'tracks': serializers.TrackSerializerNested(self.get_tracks(query), many=True).data, - 'albums': serializers.AlbumSerializerNested(self.get_albums(query), many=True).data, + # 'tags': serializers.TagSerializer(self.get_tags(query), many=True).data, + 'artists': serializers.ArtistWithAlbumsSerializer(self.get_artists(query), many=True).data, + 'tracks': serializers.TrackSerializer(self.get_tracks(query), many=True).data, + 'albums': serializers.AlbumSerializer(self.get_albums(query), many=True).data, } return Response(results, status=200) @@ -387,14 +382,10 @@ class Search(views.APIView): return ( models.Track.objects.all() .filter(query_obj) - .select_related('album__artist') - .prefetch_related( - 'tags', - 'artist__albums__tracks__tags', - 'files') + .select_related('artist', 'album__artist') + .prefetch_related('files') )[:self.max_results] - def get_albums(self, query): search_fields = [ 'mbid', @@ -406,27 +397,19 @@ class Search(views.APIView): .filter(query_obj) .select_related() .prefetch_related( - 'tracks__tags', 'tracks__files', - ) + ) )[:self.max_results] - def get_artists(self, query): search_fields = ['mbid', 'name__unaccent'] query_obj = utils.get_query(query, search_fields) return ( models.Artist.objects.all() .filter(query_obj) - .select_related() - .prefetch_related( - 'albums__tracks__tags', - 'albums__tracks__files', - ) - + .with_albums() )[:self.max_results] - def get_tags(self, query): search_fields = ['slug', 'name__unaccent'] query_obj = utils.get_query(query, search_fields) diff --git a/api/funkwhale_api/playlists/serializers.py b/api/funkwhale_api/playlists/serializers.py index fcb2a412..3f01fd68 100644 --- a/api/funkwhale_api/playlists/serializers.py +++ b/api/funkwhale_api/playlists/serializers.py @@ -5,13 +5,13 @@ from taggit.models import Tag from funkwhale_api.common import preferences from funkwhale_api.music.models import Track -from funkwhale_api.music.serializers import TrackSerializerNested +from funkwhale_api.music.serializers import TrackSerializer from funkwhale_api.users.serializers import UserBasicSerializer from . import models class PlaylistTrackSerializer(serializers.ModelSerializer): - track = TrackSerializerNested() + track = TrackSerializer() class Meta: model = models.PlaylistTrack diff --git a/api/funkwhale_api/radios/serializers.py b/api/funkwhale_api/radios/serializers.py index 195b382c..8c59f871 100644 --- a/api/funkwhale_api/radios/serializers.py +++ b/api/funkwhale_api/radios/serializers.py @@ -1,6 +1,6 @@ from rest_framework import serializers -from funkwhale_api.music.serializers import TrackSerializerNested +from funkwhale_api.music.serializers import TrackSerializer from funkwhale_api.users.serializers import UserBasicSerializer from . import filters @@ -46,7 +46,7 @@ class RadioSessionTrackSerializerCreate(serializers.ModelSerializer): class RadioSessionTrackSerializer(serializers.ModelSerializer): - track = TrackSerializerNested() + track = TrackSerializer() class Meta: model = models.RadioSessionTrack diff --git a/api/funkwhale_api/radios/views.py b/api/funkwhale_api/radios/views.py index 37c07c5e..ca510b82 100644 --- a/api/funkwhale_api/radios/views.py +++ b/api/funkwhale_api/radios/views.py @@ -7,7 +7,7 @@ from rest_framework import status from rest_framework.response import Response from rest_framework.decorators import detail_route, list_route -from funkwhale_api.music.serializers import TrackSerializerNested +from funkwhale_api.music.serializers import TrackSerializer from funkwhale_api.common.permissions import ConditionalAuthentication from . import models @@ -49,7 +49,7 @@ class RadioViewSet( page = self.paginate_queryset(tracks) if page is not None: - serializer = TrackSerializerNested(page, many=True) + serializer = TrackSerializer(page, many=True) return self.get_paginated_response(serializer.data) @list_route(methods=['get']) @@ -72,7 +72,7 @@ class RadioViewSet( results = filters.test(f) if results['candidates']['sample']: qs = results['candidates']['sample'].for_nested_serialization() - results['candidates']['sample'] = TrackSerializerNested( + results['candidates']['sample'] = TrackSerializer( qs, many=True).data data['filters'].append(results) diff --git a/api/tests/conftest.py b/api/tests/conftest.py index b7a7d071..d6b1c020 100644 --- a/api/tests/conftest.py +++ b/api/tests/conftest.py @@ -1,3 +1,4 @@ +import datetime import factory import pytest import requests_mock @@ -10,6 +11,7 @@ from django.test import client from dynamic_preferences.registries import global_preferences_registry +from rest_framework import fields as rest_fields from rest_framework.test import APIClient from rest_framework.test import APIRequestFactory @@ -233,3 +235,16 @@ def assert_user_permission(): assert HasUserPermission in view.permission_classes assert set(view.required_permissions) == set(permissions) return inner + + +@pytest.fixture +def to_api_date(): + def inner(value): + if isinstance(value, datetime.datetime): + f = rest_fields.DateTimeField() + return f.to_representation(value) + if isinstance(value, datetime.date): + f = rest_fields.DateField() + return f.to_representation(value) + raise ValueError('Invalid value: {}'.format(value)) + return inner diff --git a/api/tests/music/test_api.py b/api/tests/music/test_api.py index 53ee29f3..7aa20e62 100644 --- a/api/tests/music/test_api.py +++ b/api/tests/music/test_api.py @@ -223,41 +223,6 @@ def test_user_can_create_import_job_with_file( import_job_id=job.pk) -def test_can_search_artist(factories, logged_in_client): - artist1 = factories['music.Artist']() - artist2 = factories['music.Artist']() - expected = [serializers.ArtistSerializerNested(artist1).data] - url = reverse('api:v1:artists-search') - response = logged_in_client.get(url, {'query': artist1.name}) - assert response.data == expected - - -def test_can_search_artist_by_name_start(factories, logged_in_client): - artist1 = factories['music.Artist'](name='alpha') - artist2 = factories['music.Artist'](name='beta') - expected = { - 'next': None, - 'previous': None, - 'count': 1, - 'results': [serializers.ArtistSerializerNested(artist1).data] - } - url = reverse('api:v1:artists-list') - response = logged_in_client.get(url, {'name__startswith': 'a'}) - - assert expected == response.data - - -def test_can_search_tracks(factories, logged_in_client): - track1 = factories['music.Track'](title="test track 1") - track2 = factories['music.Track']() - query = 'test track 1' - expected = [serializers.TrackSerializerNested(track1).data] - url = reverse('api:v1:tracks-search') - response = logged_in_client.get(url, {'query': query}) - - assert expected == response.data - - @pytest.mark.parametrize('route,method', [ ('api:v1:tags-list', 'get'), ('api:v1:tracks-list', 'get'), diff --git a/api/tests/music/test_serializers.py b/api/tests/music/test_serializers.py new file mode 100644 index 00000000..fa22cece --- /dev/null +++ b/api/tests/music/test_serializers.py @@ -0,0 +1,121 @@ +from funkwhale_api.music import serializers + + +def test_artist_album_serializer(factories, to_api_date): + track = factories['music.Track']() + album = track.album + album = album.__class__.objects.with_tracks_count().get(pk=album.pk) + expected = { + 'id': album.id, + 'mbid': str(album.mbid), + 'title': album.title, + 'artist': album.artist.id, + 'creation_date': to_api_date(album.creation_date), + 'tracks_count': 1, + 'cover': album.cover.url, + 'release_date': to_api_date(album.release_date), + } + serializer = serializers.ArtistAlbumSerializer(album) + + assert serializer.data == expected + + +def test_artist_with_albums_serializer(factories, to_api_date): + track = factories['music.Track']() + artist = track.artist + artist = artist.__class__.objects.with_albums().get(pk=artist.pk) + album = list(artist.albums.all())[0] + + expected = { + 'id': artist.id, + 'mbid': str(artist.mbid), + 'name': artist.name, + 'creation_date': to_api_date(artist.creation_date), + 'albums': [ + serializers.ArtistAlbumSerializer(album).data + ] + } + serializer = serializers.ArtistWithAlbumsSerializer(artist) + assert serializer.data == expected + + +def test_album_track_serializer(factories, to_api_date): + tf = factories['music.TrackFile']() + track = tf.track + + expected = { + 'id': track.id, + 'artist': track.artist.id, + 'album': track.album.id, + 'mbid': str(track.mbid), + 'title': track.title, + 'position': track.position, + 'creation_date': to_api_date(track.creation_date), + 'files': [ + serializers.TrackFileSerializer(tf).data + ] + } + serializer = serializers.AlbumTrackSerializer(track) + assert serializer.data == expected + + +def test_track_file_serializer(factories, to_api_date): + tf = factories['music.TrackFile']() + + expected = { + 'id': tf.id, + 'path': tf.path, + 'source': tf.source, + 'filename': tf.filename, + 'mimetype': tf.mimetype, + 'track': tf.track.pk, + 'duration': tf.duration, + 'mimetype': tf.mimetype, + 'bitrate': tf.bitrate, + 'size': tf.size, + } + serializer = serializers.TrackFileSerializer(tf) + assert serializer.data == expected + + +def test_album_serializer(factories, to_api_date): + track1 = factories['music.Track'](position=2) + track2 = factories['music.Track'](position=1, album=track1.album) + album = track1.album + expected = { + 'id': album.id, + 'mbid': str(album.mbid), + 'title': album.title, + 'artist': serializers.ArtistSimpleSerializer(album.artist).data, + 'creation_date': to_api_date(album.creation_date), + 'cover': album.cover.url, + 'release_date': to_api_date(album.release_date), + 'tracks': serializers.AlbumTrackSerializer( + [track2, track1], + many=True + ).data + } + serializer = serializers.AlbumSerializer(album) + + assert serializer.data == expected + + +def test_track_serializer(factories, to_api_date): + tf = factories['music.TrackFile']() + track = tf.track + + expected = { + 'id': track.id, + 'artist': serializers.ArtistSimpleSerializer(track.artist).data, + 'album': serializers.TrackAlbumSerializer(track.album).data, + 'mbid': str(track.mbid), + 'title': track.title, + 'position': track.position, + 'creation_date': to_api_date(track.creation_date), + 'lyrics': track.get_lyrics_url(), + 'files': [ + serializers.TrackFileSerializer(tf).data + ] + } + serializer = serializers.TrackSerializer(track) + assert serializer.data == expected diff --git a/api/tests/music/test_views.py b/api/tests/music/test_views.py index 030fc3a7..38366442 100644 --- a/api/tests/music/test_views.py +++ b/api/tests/music/test_views.py @@ -4,6 +4,7 @@ import pytest from django.urls import reverse from django.utils import timezone +from funkwhale_api.music import serializers from funkwhale_api.music import views from funkwhale_api.federation import actors @@ -16,6 +17,65 @@ def test_permissions(assert_user_permission, view, permissions): assert_user_permission(view, permissions) +def test_artist_list_serializer(api_request, factories, logged_in_api_client): + track = factories['music.Track']() + artist = track.artist + request = api_request.get('/') + qs = artist.__class__.objects.with_albums() + serializer = serializers.ArtistWithAlbumsSerializer( + qs, many=True, context={'request': request}) + expected = { + 'count': 1, + 'next': None, + 'previous': None, + 'results': serializer.data + } + url = reverse('api:v1:artists-list') + response = logged_in_api_client.get(url) + + assert response.status_code == 200 + assert response.data == expected + + +def test_album_list_serializer(api_request, factories, logged_in_api_client): + track = factories['music.Track']() + album = track.album + request = api_request.get('/') + qs = album.__class__.objects.all() + serializer = serializers.AlbumSerializer( + qs, many=True, context={'request': request}) + expected = { + 'count': 1, + 'next': None, + 'previous': None, + 'results': serializer.data + } + url = reverse('api:v1:albums-list') + response = logged_in_api_client.get(url) + + assert response.status_code == 200 + assert response.data == expected + + +def test_track_list_serializer(api_request, factories, logged_in_api_client): + track = factories['music.Track']() + request = api_request.get('/') + qs = track.__class__.objects.all() + serializer = serializers.TrackSerializer( + qs, many=True, context={'request': request}) + expected = { + 'count': 1, + 'next': None, + 'previous': None, + 'results': serializer.data + } + url = reverse('api:v1:tracks-list') + response = logged_in_api_client.get(url) + + assert response.status_code == 200 + assert response.data == expected + + @pytest.mark.parametrize('param,expected', [ ('true', 'full'), ('false', 'empty'), diff --git a/api/tests/radios/test_api.py b/api/tests/radios/test_api.py index 25c09901..66bf6052 100644 --- a/api/tests/radios/test_api.py +++ b/api/tests/radios/test_api.py @@ -3,7 +3,7 @@ import pytest from django.urls import reverse -from funkwhale_api.music.serializers import TrackSerializerNested +from funkwhale_api.music.serializers import TrackSerializer from funkwhale_api.radios import filters from funkwhale_api.radios import serializers @@ -43,7 +43,7 @@ def test_can_validate_config(logged_in_client, factories): expected = { 'count': candidates.count(), - 'sample': TrackSerializerNested(candidates, many=True).data + 'sample': TrackSerializer(candidates, many=True).data } assert payload['filters'][0]['candidates'] == expected assert payload['filters'][0]['errors'] == [] -- GitLab