From 2e8077f7e8a61b5cc3a091629044be7fa0b597f3 Mon Sep 17 00:00:00 2001 From: Manuel Raynaud Date: Mon, 23 Jun 2025 11:40:34 +0200 Subject: [PATCH] =?UTF-8?q?=E2=99=BB=EF=B8=8F(back)=20move=20check=20const?= =?UTF-8?q?raint=20on=20filename=20in=20save=20method?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The check constraint on the filename can be moved in the save method. A check constraint will execute a sql query and in this case it is not needed, we have all the element needed to do this check in the python code and not in the database. This will reduce the number queries on every save. --- .../0008_alter_item_options_and_more.py | 21 ++++++++++++ src/backend/core/models.py | 32 +++++++++++++------ .../tests/authentication/test_backends.py | 2 +- .../core/tests/test_models_invitations.py | 2 +- src/backend/core/tests/test_models_items.py | 4 +-- 5 files changed, 47 insertions(+), 14 deletions(-) create mode 100644 src/backend/core/migrations/0008_alter_item_options_and_more.py diff --git a/src/backend/core/migrations/0008_alter_item_options_and_more.py b/src/backend/core/migrations/0008_alter_item_options_and_more.py new file mode 100644 index 00000000..9962e9a9 --- /dev/null +++ b/src/backend/core/migrations/0008_alter_item_options_and_more.py @@ -0,0 +1,21 @@ +# Generated by Django 5.2.3 on 2025-06-23 09:22 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('core', '0007_item_hard_deleted_at'), + ] + + operations = [ + migrations.AlterModelOptions( + name='item', + options={'ordering': ('created_at',), 'verbose_name': 'Item', 'verbose_name_plural': 'Items'}, + ), + migrations.RemoveConstraint( + model_name='item', + name='check_filename_set_for_files', + ), + ] diff --git a/src/backend/core/models.py b/src/backend/core/models.py index 6aedd60a..ea7766d9 100644 --- a/src/backend/core/models.py +++ b/src/backend/core/models.py @@ -556,16 +556,7 @@ class Item(TreeModel, BaseModel): | models.Q(deleted_at=models.F("ancestors_deleted_at")) ), name="check_deleted_at_matches_ancestors_deleted_at_when_set", - ), - models.CheckConstraint( - condition=models.Q( - models.Q(type=ItemTypeChoices.FILE, filename__isnull=False) - | models.Q( - ~models.Q(type=ItemTypeChoices.FILE), filename__isnull=True - ) - ), - name="check_filename_set_for_files", - ), + ) ] indexes = [ GistIndex(fields=["path"]), @@ -576,6 +567,27 @@ class Item(TreeModel, BaseModel): def save(self, *args, **kwargs): """Set the upload state to pending if it's the first save and it's a file""" + # Validate filename requirements based on item type + if self.type == ItemTypeChoices.FILE: + if self.filename is None: + raise ValidationError( + { + "filename": ValidationError( + _("Filename is required for files."), + code="item_filename_required_for_files", + ) + } + ) + elif self.filename is not None: + raise ValidationError( + { + "filename": ValidationError( + _("Filename is only allowed for files."), + code="item_filename_only_allowed_for_files", + ) + } + ) + if self.created_at is None and self.type == ItemTypeChoices.FILE: self.upload_state = ItemUploadStateChoices.PENDING diff --git a/src/backend/core/tests/authentication/test_backends.py b/src/backend/core/tests/authentication/test_backends.py index 935db218..2d472edb 100644 --- a/src/backend/core/tests/authentication/test_backends.py +++ b/src/backend/core/tests/authentication/test_backends.py @@ -500,7 +500,7 @@ def test_authentication_session_tokens( status=200, ) - with django_assert_num_queries(30): + with django_assert_num_queries(27): user = klass.authenticate( request, code="test-code", diff --git a/src/backend/core/tests/test_models_invitations.py b/src/backend/core/tests/test_models_invitations.py index 16112890..2a406713 100644 --- a/src/backend/core/tests/test_models_invitations.py +++ b/src/backend/core/tests/test_models_invitations.py @@ -143,7 +143,7 @@ def test_models_invitationd_new_user_filter_expired_invitations(): ).exists() -@pytest.mark.parametrize("num_invitations, num_queries", [(0, 27), (1, 31), (20, 31)]) +@pytest.mark.parametrize("num_invitations, num_queries", [(0, 24), (1, 28), (20, 28)]) def test_models_invitations_new_userd_user_creation_constant_num_queries( django_assert_num_queries, num_invitations, num_queries ): diff --git a/src/backend/core/tests/test_models_items.py b/src/backend/core/tests/test_models_items.py index cb419c90..c405bf8c 100644 --- a/src/backend/core/tests/test_models_items.py +++ b/src/backend/core/tests/test_models_items.py @@ -699,7 +699,7 @@ def test_models_items_creating_file_without_filename_should_fail(): factories.ItemFactory(type=models.ItemTypeChoices.FILE, filename=None) assert exc_info.value.message_dict == { - "__all__": ["Constraint “check_filename_set_for_files” is violated."] + "filename": ["Filename is required for files."] } @@ -717,7 +717,7 @@ def test_models_items_creating_non_file_with_filename_should_fail(item_type): factories.ItemFactory(type=item_type, filename="file.txt") assert exc_info.value.message_dict == { - "__all__": ["Constraint “check_filename_set_for_files” is violated."] + "filename": ["Filename is only allowed for files."] }