🐛(back) change thumbnail video relation to ForeignKey field

The relation between thumbnail and video was a OneToOneField and it is
exactly the behavior we want. Unfortunately the app djang-safedelete
does not manage it and it is then impossible for us to delete a
thumbnail belonging to a video. The thumbnail is always return. To fix
this issue we decided to change the relation with a ForeignKey field and
to mimic the behavior of a OneToOneField by adding a UniqueConstraint
and managing the uniqueness of the resource in the VideoSerializer.
Moreover, we must keep the thumbnail instance in the VideoSerializer to
avoid to query the database again and again to fetch the instance every
time we need it. Without this, the number of queries is growing.
An issue is open to track this bug
makinacorpus/django-safedelete#211

CHANGELOG.md

@@ -8,6 +8,10 @@ Versioning](https://semver.org/spec/v2.0.0.html).
 
 ## [Unreleased]
 
+### Fixed
+
+- A thumbnail resource can be deleted and recreated.
+
 ## [4.0.0-beta.5] - 2022-06-07
 
 ### Added

src/backend/marsha/core/migrations/0048_modify_thumbnail_video_relation.py

@@ -0,0 +1,36 @@
+# Generated by Django 4.0.5 on 2022-06-07 13:00
+
+from django.conf import settings
+import django.contrib.postgres.fields
+from django.db import migrations, models
+import django.db.models.deletion
+import django.utils.timezone
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ("core", "0047_add_video_join_mode"),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name="thumbnail",
+            name="video",
+            field=models.ForeignKey(
+                help_text="video for which this thumbnail is",
+                on_delete=django.db.models.deletion.CASCADE,
+                related_name="thumbnail",
+                to="core.video",
+                verbose_name="video",
+            ),
+        ),
+        migrations.AddConstraint(
+            model_name="thumbnail",
+            constraint=models.UniqueConstraint(
+                condition=models.Q(("deleted", None)),
+                fields=("video",),
+                name="thumbnail_video_unique_idx",
+            ),
+        ),
+    ]

src/backend/marsha/core/models/video.py

@@ -494,8 +494,8 @@ class Thumbnail(AbstractImage):
 
     RESOURCE_NAME = "thumbnails"
 
-    video = models.OneToOneField(
-        to="Video",
+    video = models.ForeignKey(
+        Video,
         related_name="thumbnail",
         verbose_name=_("video"),
         help_text=_("video for which this thumbnail is"),
@@ -509,6 +509,18 @@ class Meta:
         db_table = "video_thumbnail"
         verbose_name = _("thumbnail")
         ordering = ["-created_on", "id"]
+        # The relation with the video is a foreign key
+        # but we want the behavior of a OneToOneField. Unfortunately,
+        # this field doesn't work with django-safedelete so we have to manage
+        # the unicity with a unique constraint.
+        # see https://github.com/makinacorpus/django-safedelete/issues/211
+        constraints = [
+            models.UniqueConstraint(
+                fields=["video"],
+                condition=models.Q(deleted=None),
+                name="thumbnail_video_unique_idx",
+            )
+        ]
 
     def get_source_s3_key(self, stamp=None):
         """Compute the S3 key in the source bucket.

src/backend/marsha/core/serializers/thumbnail.py

@@ -1,5 +1,6 @@
 """Structure of Thumbnail related models API responses with Django Rest Framework serializers."""
 from django.conf import settings
+from django.db.utils import IntegrityError
 
 from rest_framework import serializers
 from rest_framework_simplejwt.models import TokenUser
@@ -59,7 +60,29 @@ def create(self, validated_data):
         user = self.context["request"].user
         if not validated_data.get("video_id") and isinstance(user, TokenUser):
             validated_data["video_id"] = user.id
-        return super().create(validated_data)
+        try:
+            return super().create(validated_data)
+        except IntegrityError as exc:
+            raise serializers.ValidationError(
+                {"video": ["Thumbnail with this Video already exists."]}
+            ) from exc
+
+    def to_representation(self, instance):
+        """
+        Object instance -> Dict of primitive datatypes.
+        Depending if the serializer was instancianted with a Thumbnail
+        model instance or not, we have to fetch it in the database to avoid error
+        trying to work with a None instance in all the serializerMethodField
+        of this serializer.
+        """
+        if isinstance(instance, Thumbnail):
+            return super().to_representation(instance)
+
+        try:
+            thumbnail = instance.get()
+            return super().to_representation(thumbnail)
+        except Thumbnail.DoesNotExist:
+            return None
 
     def get_urls(self, obj):
         """Urls of the thumbnail.

src/backend/marsha/core/serializers/video.py

@@ -49,6 +49,8 @@ class InitLiveStateSerializer(serializers.Serializer):
 class VideoBaseSerializer(serializers.ModelSerializer):
     """Base Serializer to factorize common Video attributes."""
 
+    thumbnail_instance = None
+
     class Meta:  # noqa
         model = Video
         fields = (
@@ -62,9 +64,31 @@ class Meta:  # noqa
         )
 
     urls = serializers.SerializerMethodField()
-    thumbnail = ThumbnailSerializer(read_only=True, allow_null=True)
+    thumbnail = serializers.SerializerMethodField()
     is_ready_to_show = serializers.BooleanField(read_only=True)
 
+    def to_representation(self, instance):
+        """
+        Object instance -> Dict of primitive datatypes.
+        Try to fetch existing thumbnail related to the current video.
+        If a thumbnail exists, we keep it in the serializer instance
+        to use it several times without having to fetch it again and again
+        in the database.
+        """
+        try:
+            self.thumbnail_instance = instance.thumbnail.get()
+        except Thumbnail.DoesNotExist:
+            pass
+
+        return super().to_representation(instance)
+
+    def get_thumbnail(self, _):
+        """Return a serialized thumbnail if it exists."""
+        if self.thumbnail_instance:
+            return ThumbnailSerializer(self.thumbnail_instance).data
+
+        return None
+
     def get_urls(self, obj):
         """Urls of the video for each type of encoding.
 
@@ -101,14 +125,9 @@ def get_urls(self, obj):
             return None
 
         thumbnail_urls = {}
-        try:
-            thumbnail = obj.thumbnail
-        except Thumbnail.DoesNotExist:
-            pass
-        else:
-            if thumbnail.uploaded_on is not None:
-                thumbnail_serialized = ThumbnailSerializer(thumbnail)
-                thumbnail_urls.update(thumbnail_serialized.data.get("urls"))
+        if self.thumbnail_instance and self.thumbnail_instance.uploaded_on is not None:
+            thumbnail_serialized = ThumbnailSerializer(self.thumbnail_instance)
+            thumbnail_urls.update(thumbnail_serialized.data.get("urls"))
 
         urls = {"mp4": {}, "thumbnails": {}}
 

src/backend/marsha/core/tests/test_api_thumbnail.py

@@ -314,12 +314,30 @@ def test_api_thumbnail_delete_instructor(self):
         jwt_token.payload["roles"] = [random.choice(["instructor", "administrator"])]
         jwt_token.payload["permissions"] = {"can_update": True}
 
+        self.assertEqual(Thumbnail.objects.count(), 1)
+
         response = self.client.delete(
             f"/api/thumbnails/{thumbnail.id}/",
             HTTP_AUTHORIZATION=f"Bearer {jwt_token}",
         )
         self.assertEqual(response.status_code, 204)
         self.assertFalse(Thumbnail.objects.filter(id=thumbnail.id).exists())
+        self.assertEqual(Thumbnail.objects.count(), 0)
+
+        response = self.client.get(
+            f"/api/videos/{video.id}/",
+            HTTP_AUTHORIZATION=f"Bearer {jwt_token}",
+        )
+
+        content = response.json()
+        self.assertIsNone(content["thumbnail"])
+
+        # Creating a new thumbnail should be allowed.
+        response = self.client.post(
+            "/api/thumbnails/", HTTP_AUTHORIZATION=f"Bearer {jwt_token}"
+        )
+
+        self.assertEqual(response.status_code, 201)
 
     def test_api_thumbnail_delete_instructor_in_read_only(self):
         """Instructor should not be able to delete thumbnails in a read_only mode."""