From 43b8a2e3f0a2fc00a9fb1e1c5417a5f5fbbec77a Mon Sep 17 00:00:00 2001 From: Manuel Raynaud Date: Tue, 7 Jun 2022 17:43:47 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B(back)=20change=20thumbnail=20video?= =?UTF-8?q?=20relation=20to=20ForeignKey=20field?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 https://github.com/makinacorpus/django-safedelete/issues/211 --- CHANGELOG.md | 4 ++ .../0048_modify_thumbnail_video_relation.py | 36 ++++++++++++++++++ src/backend/marsha/core/models/video.py | 16 +++++++- .../marsha/core/serializers/thumbnail.py | 25 ++++++++++++- src/backend/marsha/core/serializers/video.py | 37 ++++++++++++++----- .../marsha/core/tests/test_api_thumbnail.py | 18 +++++++++ 6 files changed, 124 insertions(+), 12 deletions(-) create mode 100644 src/backend/marsha/core/migrations/0048_modify_thumbnail_video_relation.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 9a5822fc7b..4f4683e21b 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/backend/marsha/core/migrations/0048_modify_thumbnail_video_relation.py b/src/backend/marsha/core/migrations/0048_modify_thumbnail_video_relation.py new file mode 100644 index 0000000000..d3b49f5058 --- /dev/null +++ b/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", + ), + ), + ] diff --git a/src/backend/marsha/core/models/video.py b/src/backend/marsha/core/models/video.py index fcc585c45e..d498edb12d 100644 --- a/src/backend/marsha/core/models/video.py +++ b/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. diff --git a/src/backend/marsha/core/serializers/thumbnail.py b/src/backend/marsha/core/serializers/thumbnail.py index 5758840cef..daaf94854f 100644 --- a/src/backend/marsha/core/serializers/thumbnail.py +++ b/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. diff --git a/src/backend/marsha/core/serializers/video.py b/src/backend/marsha/core/serializers/video.py index 3da9f9181f..0fd2def3a0 100644 --- a/src/backend/marsha/core/serializers/video.py +++ b/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": {}} diff --git a/src/backend/marsha/core/tests/test_api_thumbnail.py b/src/backend/marsha/core/tests/test_api_thumbnail.py index f7f487663f..bbe8f9ed52 100644 --- a/src/backend/marsha/core/tests/test_api_thumbnail.py +++ b/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."""