Permissions and db state fixes with new index field

api/funkwhale_api/playlists/models.py

@@ -67,13 +67,18 @@ class Playlist(models.Model):
         plt.save(update_fields=['index'])
         return index
 
+    def remove(self, index):
+        existing = self.playlist_tracks.select_for_update()
+        to_update = existing.filter(index__gt=index)
+        return to_update.update(index=models.F('index') - 1)
+
 
 class PlaylistTrack(models.Model):
     track = models.ForeignKey(
         'music.Track',
         related_name='playlist_tracks',
         on_delete=models.CASCADE)
-    index = models.PositiveIntegerField(null=True)
+    index = models.PositiveIntegerField(null=True, blank=True)
     playlist = models.ForeignKey(
         Playlist, related_name='playlist_tracks', on_delete=models.CASCADE)
     creation_date = models.DateTimeField(default=timezone.now)
@@ -81,3 +86,12 @@ class PlaylistTrack(models.Model):
     class Meta:
         ordering = ('-playlist', 'index')
         unique_together = ('playlist', 'index')
+
+    def delete(self, *args, **kwargs):
+        playlist = self.playlist
+        index = self.index
+        update_indexes = kwargs.pop('update_indexes', False)
+        r = super().delete(*args, **kwargs)
+        if index is not None and update_indexes:
+            playlist.remove(index)
+        return r

api/funkwhale_api/playlists/serializers.py

@@ -1,4 +1,5 @@
 from django.conf import settings
+from django.db import transaction
 from rest_framework import serializers
 from taggit.models import Tag
 
@@ -12,16 +13,23 @@ class PlaylistTrackSerializer(serializers.ModelSerializer):
 
     class Meta:
         model = models.PlaylistTrack
-        fields = ('id', 'track', 'playlist', 'position')
+        fields = ('id', 'track', 'playlist', 'index', 'creation_date')
 
 
-class PlaylistTrackCreateSerializer(serializers.ModelSerializer):
+class PlaylistTrackWriteSerializer(serializers.ModelSerializer):
+    index = serializers.IntegerField(
+        required=False, min_value=0, allow_null=True)
 
     class Meta:
         model = models.PlaylistTrack
         fields = ('id', 'track', 'playlist', 'index')
 
     def validate_playlist(self, value):
+        if self.context.get('request'):
+            # validate proper ownership on the playlist
+            if self.context['request'].user != value.user:
+                raise serializers.ValidationError(
+                    'You do not have the permission to edit this playlist')
         existing = value.playlist_tracks.count()
         if existing >= settings.PLAYLISTS_MAX_TRACKS:
             raise serializers.ValidationError(
@@ -29,6 +37,29 @@ class PlaylistTrackCreateSerializer(serializers.ModelSerializer):
                     settings.PLAYLISTS_MAX_TRACKS))
         return value
 
+    @transaction.atomic
+    def create(self, validated_data):
+        index = validated_data.pop('index', None)
+        instance = super().create(validated_data)
+        instance.playlist.insert(instance, index)
+        return instance
+
+    @transaction.atomic
+    def update(self, instance, validated_data):
+        update_index = 'index' in validated_data
+        index = validated_data.pop('index', None)
+        super().update(instance, validated_data)
+        if update_index:
+            instance.playlist.insert(instance, index)
+        return instance
+
+    def get_unique_together_validators(self):
+        """
+        We explicitely disable unique together validation here
+        because it collides with our internal logic
+        """
+        return []
+
 
 class PlaylistSerializer(serializers.ModelSerializer):
 

api/funkwhale_api/playlists/views.py

@@ -27,6 +27,7 @@ class PlaylistViewSet(
         permissions.OwnerPermission,
         IsAuthenticatedOrReadOnly,
     ]
+    owner_checks = ['write']
 
     @detail_route(methods=['get'])
     def tracks(self, request, *args, **kwargs):
@@ -66,25 +67,19 @@ class PlaylistTrackViewSet(
         permissions.OwnerPermission,
         IsAuthenticatedOrReadOnly,
     ]
+    owner_field = 'playlist.user'
+    owner_checks = ['write']
 
-    def create(self, request, *args, **kwargs):
-        serializer = serializers.PlaylistTrackCreateSerializer(
-            data=request.data)
-        serializer.is_valid(raise_exception=True)
-        if serializer.validated_data['playlist'].user != request.user:
-            return Response(
-                {'playlist': [
-                    'This playlist does not exists or you do not have the'
-                    'permission to edit it']
-                },
-                status=400)
-        instance = self.perform_create(serializer)
-        serializer = self.get_serializer(instance=instance)
-        headers = self.get_success_headers(serializer.data)
-        return Response(serializer.data, status=status.HTTP_201_CREATED, headers=headers)
+    def get_serializer_class(self):
+        if self.request.method in ['PUT', 'PATCH', 'DELETE', 'POST']:
+            return serializers.PlaylistTrackWriteSerializer
+        return self.serializer_class
 
     def get_queryset(self):
         return self.queryset.filter(
             fields.privacy_level_query(
                 self.request.user,
                 lookup_field='playlist__privacy_level'))
+
+    def perform_destroy(self, instance):
+        instance.delete(update_indexes=True)

api/tests/playlists/test_models.py

@@ -72,3 +72,18 @@ def test_cannot_insert_at_negative_index(factories):
     new = factories['playlists.PlaylistTrack'](playlist=plt.playlist)
     with pytest.raises(forms.ValidationError):
         plt.playlist.insert(new, -1)
+
+
+def test_remove_update_indexes(factories):
+    playlist = factories['playlists.Playlist']()
+    first = factories['playlists.PlaylistTrack'](playlist=playlist, index=0)
+    second = factories['playlists.PlaylistTrack'](playlist=playlist, index=1)
+    third = factories['playlists.PlaylistTrack'](playlist=playlist, index=2)
+
+    second.delete(update_indexes=True)
+
+    first.refresh_from_db()
+    third.refresh_from_db()
+
+    assert first.index == 0
+    assert third.index == 1

api/tests/playlists/test_serializers.py

@@ -1,16 +1,74 @@
+from funkwhale_api.playlists import models
 from funkwhale_api.playlists import serializers
 
 
-def test_cannot_max_500_tracks_per_playlist(mocker, factories, settings):
+def test_cannot_max_500_tracks_per_playlist(factories, settings):
     settings.PLAYLISTS_MAX_TRACKS = 2
     playlist = factories['playlists.Playlist']()
     plts = factories['playlists.PlaylistTrack'].create_batch(
         size=2, playlist=playlist)
     track = factories['music.Track']()
-    serializer = serializers.PlaylistTrackCreateSerializer(data={
+    serializer = serializers.PlaylistTrackWriteSerializer(data={
         'playlist': playlist.pk,
         'track': track.pk,
     })
 
     assert serializer.is_valid() is False
     assert 'playlist' in serializer.errors
+
+
+def test_create_insert_is_called_when_index_is_None(factories, mocker):
+    insert = mocker.spy(models.Playlist, 'insert')
+    playlist = factories['playlists.Playlist']()
+    track = factories['music.Track']()
+    serializer = serializers.PlaylistTrackWriteSerializer(data={
+        'playlist': playlist.pk,
+        'track': track.pk,
+        'index': None,
+    })
+    assert serializer.is_valid() is True
+
+    plt = serializer.save()
+    insert.assert_called_once_with(playlist, plt, None)
+    assert plt.index == 0
+
+
+def test_create_insert_is_called_when_index_is_provided(factories, mocker):
+    playlist = factories['playlists.Playlist']()
+    first = factories['playlists.PlaylistTrack'](playlist=playlist, index=0)
+    insert = mocker.spy(models.Playlist, 'insert')
+    factories['playlists.Playlist']()
+    track = factories['music.Track']()
+    serializer = serializers.PlaylistTrackWriteSerializer(data={
+        'playlist': playlist.pk,
+        'track': track.pk,
+        'index': 0,
+    })
+    assert serializer.is_valid() is True
+
+    plt = serializer.save()
+    first.refresh_from_db()
+    insert.assert_called_once_with(playlist, plt, 0)
+    assert plt.index == 0
+    assert first.index == 1
+
+
+def test_update_insert_is_called_when_index_is_provided(factories, mocker):
+    playlist = factories['playlists.Playlist']()
+    first = factories['playlists.PlaylistTrack'](playlist=playlist, index=0)
+    second = factories['playlists.PlaylistTrack'](playlist=playlist, index=1)
+    insert = mocker.spy(models.Playlist, 'insert')
+    factories['playlists.Playlist']()
+    track = factories['music.Track']()
+    serializer = serializers.PlaylistTrackWriteSerializer(second, data={
+        'playlist': playlist.pk,
+        'track': second.track.pk,
+        'index': 0,
+    })
+    assert serializer.is_valid() is True
+
+    plt = serializer.save()
+    first.refresh_from_db()
+    insert.assert_called_once_with(playlist, plt, 0)
+    assert plt.index == 0
+    assert first.index == 1

api/tests/playlists/test_views.py

@@ -80,6 +80,21 @@ def test_only_can_add_track_on_own_playlist_via_api(
     assert playlist.playlist_tracks.count() == 0
 
 
+def test_deleting_plt_updates_indexes(
+        mocker, factories, logged_in_api_client):
+    remove = mocker.spy(models.Playlist, 'remove')
+    track = factories['music.Track']()
+    plt = factories['playlists.PlaylistTrack'](
+        index=0,
+        playlist__user=logged_in_api_client.user)
+    url = reverse('api:v1:playlist-tracks-detail', kwargs={'pk': plt.pk})
+
+    response = logged_in_api_client.delete(url)
+
+    assert response.status_code == 204
+    remove.assert_called_once_with(plt.playlist, 0)
+
+
 @pytest.mark.parametrize('level', ['instance', 'me', 'followers'])
 def test_playlist_privacy_respected_in_list_anon(level, factories, api_client):
     factories['playlists.Playlist'](privacy_level=level)