From b382f1412a092d2001b3609baf60c55b59804788 Mon Sep 17 00:00:00 2001 From: shamoon <4887959+shamoon@users.noreply.github.com> Date: Fri, 3 Mar 2023 11:43:34 -0800 Subject: [PATCH 1/4] Change model uniqueness from name to name+owner --- ...type_options_alter_tag_options_and_more.py | 107 ++++++++++++++++++ src/documents/models.py | 59 ++++++---- src/documents/serialisers.py | 12 ++ 3 files changed, 153 insertions(+), 25 deletions(-) create mode 100644 src/documents/migrations/1033_alter_documenttype_options_alter_tag_options_and_more.py diff --git a/src/documents/migrations/1033_alter_documenttype_options_alter_tag_options_and_more.py b/src/documents/migrations/1033_alter_documenttype_options_alter_tag_options_and_more.py new file mode 100644 index 000000000..543f3a492 --- /dev/null +++ b/src/documents/migrations/1033_alter_documenttype_options_alter_tag_options_and_more.py @@ -0,0 +1,107 @@ +# Generated by Django 4.1.5 on 2023-03-04 22:33 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("documents", "1032_alter_correspondent_matching_algorithm_and_more"), + ] + + operations = [ + migrations.AlterModelOptions( + name="documenttype", + options={ + "ordering": ("name",), + "verbose_name": "document type", + "verbose_name_plural": "document types", + }, + ), + migrations.AlterModelOptions( + name="tag", + options={ + "ordering": ("name",), + "verbose_name": "tag", + "verbose_name_plural": "tags", + }, + ), + migrations.AlterField( + model_name="correspondent", + name="name", + field=models.CharField(max_length=128, verbose_name="name"), + ), + migrations.AlterField( + model_name="documenttype", + name="name", + field=models.CharField(max_length=128, verbose_name="name"), + ), + migrations.AlterField( + model_name="storagepath", + name="name", + field=models.CharField(max_length=128, verbose_name="name"), + ), + migrations.AlterField( + model_name="tag", + name="name", + field=models.CharField(max_length=128, verbose_name="name"), + ), + migrations.AddConstraint( + model_name="correspondent", + constraint=models.UniqueConstraint( + fields=("name", "owner"), + name="documents_correspondent_unique_name_owner", + ), + ), + migrations.AddConstraint( + model_name="correspondent", + constraint=models.UniqueConstraint( + condition=models.Q(("owner__isnull", True)), + fields=("name",), + name="documents_correspondent_name_uniq", + ), + ), + migrations.AddConstraint( + model_name="documenttype", + constraint=models.UniqueConstraint( + fields=("name", "owner"), + name="documents_documenttype_unique_name_owner", + ), + ), + migrations.AddConstraint( + model_name="documenttype", + constraint=models.UniqueConstraint( + condition=models.Q(("owner__isnull", True)), + fields=("name",), + name="documents_documenttype_name_uniq", + ), + ), + migrations.AddConstraint( + model_name="storagepath", + constraint=models.UniqueConstraint( + fields=("name", "owner"), name="documents_storagepath_unique_name_owner" + ), + ), + migrations.AddConstraint( + model_name="storagepath", + constraint=models.UniqueConstraint( + condition=models.Q(("owner__isnull", True)), + fields=("name",), + name="documents_storagepath_name_uniq", + ), + ), + migrations.AddConstraint( + model_name="tag", + constraint=models.UniqueConstraint( + fields=("name", "owner"), name="documents_tag_unique_name_owner" + ), + ), + migrations.AddConstraint( + model_name="tag", + constraint=models.UniqueConstraint( + condition=models.Q(("owner__isnull", True)), + fields=("name",), + name="documents_tag_name_uniq", + ), + ), + ] diff --git a/src/documents/models.py b/src/documents/models.py index 68fba37b9..303abe730 100644 --- a/src/documents/models.py +++ b/src/documents/models.py @@ -23,7 +23,20 @@ ALL_STATES = sorted(states.ALL_STATES) TASK_STATE_CHOICES = sorted(zip(ALL_STATES, ALL_STATES)) -class MatchingModel(models.Model): +class ModelWithOwner(models.Model): + owner = models.ForeignKey( + User, + blank=True, + null=True, + on_delete=models.SET_NULL, + verbose_name=_("owner"), + ) + + class Meta: + abstract = True + + +class MatchingModel(ModelWithOwner): MATCH_NONE = 0 MATCH_ANY = 1 @@ -43,7 +56,7 @@ class MatchingModel(models.Model): (MATCH_AUTO, _("Automatic")), ) - name = models.CharField(_("name"), max_length=128, unique=True) + name = models.CharField(_("name"), max_length=128) match = models.CharField(_("match"), max_length=256, blank=True) @@ -58,32 +71,29 @@ class MatchingModel(models.Model): class Meta: abstract = True ordering = ("name",) + constraints = [ + models.UniqueConstraint( + fields=["name", "owner"], + name="%(app_label)s_%(class)s_unique_name_owner", + ), + models.UniqueConstraint( + name="%(app_label)s_%(class)s_name_uniq", + fields=["name"], + condition=models.Q(owner__isnull=True), + ), + ] def __str__(self): return self.name -class ModelWithOwner(models.Model): - owner = models.ForeignKey( - User, - blank=True, - null=True, - on_delete=models.SET_NULL, - verbose_name=_("owner"), - ) - - class Meta: - abstract = True - - -class Correspondent(MatchingModel, ModelWithOwner): - class Meta: - ordering = ("name",) +class Correspondent(MatchingModel): + class Meta(MatchingModel.Meta): verbose_name = _("correspondent") verbose_name_plural = _("correspondents") -class Tag(MatchingModel, ModelWithOwner): +class Tag(MatchingModel): color = models.CharField(_("color"), max_length=7, default="#a6cee3") @@ -96,25 +106,24 @@ class Tag(MatchingModel, ModelWithOwner): ), ) - class Meta: + class Meta(MatchingModel.Meta): verbose_name = _("tag") verbose_name_plural = _("tags") -class DocumentType(MatchingModel, ModelWithOwner): - class Meta: +class DocumentType(MatchingModel): + class Meta(MatchingModel.Meta): verbose_name = _("document type") verbose_name_plural = _("document types") -class StoragePath(MatchingModel, ModelWithOwner): +class StoragePath(MatchingModel): path = models.CharField( _("path"), max_length=512, ) - class Meta: - ordering = ("name",) + class Meta(MatchingModel.Meta): verbose_name = _("storage path") verbose_name_plural = _("storage paths") diff --git a/src/documents/serialisers.py b/src/documents/serialisers.py index e61c0bd97..6fa4cd065 100644 --- a/src/documents/serialisers.py +++ b/src/documents/serialisers.py @@ -186,6 +186,18 @@ class OwnedObjectSerializer(serializers.ModelSerializer, SetPermissionsMixin): def update(self, instance, validated_data): if "set_permissions" in validated_data: self._set_permissions(validated_data["set_permissions"], instance) + if "owner" in validated_data and "name" in self.Meta.fields: + name = validated_data["name"] if "name" in validated_data else instance.name + print(name) + not_unique = ( + self.Meta.model.objects.exclude(pk=instance.pk) + .filter(owner=validated_data["owner"], name=name) + .exists() + ) + if not_unique: + raise serializers.ValidationError( + "Object violates owner / name unique constraint", + ) return super().update(instance, validated_data) From 29251b6e3864d4f75fb7b4c611268fe87c5ff717 Mon Sep 17 00:00:00 2001 From: shamoon <4887959+shamoon@users.noreply.github.com> Date: Sat, 4 Mar 2023 15:00:41 -0800 Subject: [PATCH 2/4] Add test coverage for owner-aware unique constraints --- src/documents/tests/test_api.py | 41 +++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/src/documents/tests/test_api.py b/src/documents/tests/test_api.py index a38651984..6624bab05 100644 --- a/src/documents/tests/test_api.py +++ b/src/documents/tests/test_api.py @@ -20,6 +20,8 @@ except ImportError: import backports.zoneinfo as zoneinfo import pytest +from django.db import transaction +from django.db.utils import IntegrityError from django.conf import settings from django.contrib.auth.models import Group from django.contrib.auth.models import Permission @@ -1844,6 +1846,45 @@ class TestDocumentApi(DirectoriesMixin, APITestCase): ) self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) + def test_tag_unique_name_and_owner(self): + user1 = User.objects.create_user(username="test1") + user2 = User.objects.create_user(username="test2") + + self.client.post("/api/tags/", {"name": "tag 1"}, format="json") + + with transaction.atomic(): + with self.assertRaises(IntegrityError): + self.client.post("/api/tags/", {"name": "tag 1"}, format="json") + + self.client.post( + "/api/tags/", + {"name": "tag 2", "owner": user1.pk}, + format="json", + ) + + try: + self.client.post( + "/api/tags/", + {"name": "tag 2", "owner": user2.pk}, + format="json", + ) + except IntegrityError as e: + assert False, f"Exception {e}" + + def test_tag_unique_name_and_owner_enforced_on_update(self): + user1 = User.objects.create_user(username="test1") + user2 = User.objects.create_user(username="test2") + + Tag.objects.create(name="tag 1", owner=user1) + tag2 = Tag.objects.create(name="tag 1", owner=user2) + + response = self.client.patch( + f"/api/tags/{tag2.id}/", + {"owner": user1.pk}, + format="json", + ) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + class TestDocumentApiV2(DirectoriesMixin, APITestCase): def setUp(self): From c4ac35164bcc5239ba3c321e84b034e8b02d4c7f Mon Sep 17 00:00:00 2001 From: shamoon <4887959+shamoon@users.noreply.github.com> Date: Wed, 8 Mar 2023 14:35:10 -0800 Subject: [PATCH 3/4] API should 400 on unique violations --- src/documents/serialisers.py | 22 ++++++++++++++++++-- src/documents/tests/test_api.py | 36 ++++++++++++++++++++++----------- 2 files changed, 44 insertions(+), 14 deletions(-) diff --git a/src/documents/serialisers.py b/src/documents/serialisers.py index 6fa4cd065..d5fc0f079 100644 --- a/src/documents/serialisers.py +++ b/src/documents/serialisers.py @@ -68,6 +68,25 @@ class MatchingModelSerializer(serializers.ModelSerializer): slug = SerializerMethodField() + def validate(self, data): + # see https://github.com/encode/django-rest-framework/issues/7173 + name = data["name"] if "name" in data else self.instance.name + owner = ( + data["owner"] + if "owner" in data + else self.user + if hasattr(self, "user") + else None + ) + if ("name" in data or "owner" in data) and self.Meta.model.objects.filter( + name=name, + owner=owner, + ).exists(): + raise serializers.ValidationError( + {"error": "Object violates owner / name unique constraint"}, + ) + return data + def validate_match(self, match): if ( "matching_algorithm" in self.initial_data @@ -188,7 +207,6 @@ class OwnedObjectSerializer(serializers.ModelSerializer, SetPermissionsMixin): self._set_permissions(validated_data["set_permissions"], instance) if "owner" in validated_data and "name" in self.Meta.fields: name = validated_data["name"] if "name" in validated_data else instance.name - print(name) not_unique = ( self.Meta.model.objects.exclude(pk=instance.pk) .filter(owner=validated_data["owner"], name=name) @@ -196,7 +214,7 @@ class OwnedObjectSerializer(serializers.ModelSerializer, SetPermissionsMixin): ) if not_unique: raise serializers.ValidationError( - "Object violates owner / name unique constraint", + {"error": "Object violates owner / name unique constraint"}, ) return super().update(instance, validated_data) diff --git a/src/documents/tests/test_api.py b/src/documents/tests/test_api.py index 6624bab05..419c7ffe9 100644 --- a/src/documents/tests/test_api.py +++ b/src/documents/tests/test_api.py @@ -1850,11 +1850,11 @@ class TestDocumentApi(DirectoriesMixin, APITestCase): user1 = User.objects.create_user(username="test1") user2 = User.objects.create_user(username="test2") - self.client.post("/api/tags/", {"name": "tag 1"}, format="json") + response = self.client.post("/api/tags/", {"name": "tag 1"}, format="json") + self.assertEqual(response.status_code, status.HTTP_201_CREATED) - with transaction.atomic(): - with self.assertRaises(IntegrityError): - self.client.post("/api/tags/", {"name": "tag 1"}, format="json") + response = self.client.post("/api/tags/", {"name": "tag 1"}, format="json") + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) self.client.post( "/api/tags/", @@ -1862,14 +1862,26 @@ class TestDocumentApi(DirectoriesMixin, APITestCase): format="json", ) - try: - self.client.post( - "/api/tags/", - {"name": "tag 2", "owner": user2.pk}, - format="json", - ) - except IntegrityError as e: - assert False, f"Exception {e}" + response = self.client.post( + "/api/tags/", + {"name": "tag 2", "owner": user1.pk}, + format="json", + ) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + + self.client.post( + "/api/tags/", + {"name": "tag 3", "owner": user1.pk}, + format="json", + ) + + response = self.client.post( + "/api/tags/", + {"name": "tag 3", "owner": user2.pk}, + format="json", + ) + + self.assertEqual(response.status_code, status.HTTP_201_CREATED) def test_tag_unique_name_and_owner_enforced_on_update(self): user1 = User.objects.create_user(username="test1") From 5e7b93d153be5aa8de1067666051a9c1fa63f500 Mon Sep 17 00:00:00 2001 From: Trenton H <797416+stumpylog@users.noreply.github.com> Date: Thu, 9 Mar 2023 08:43:31 -0800 Subject: [PATCH 4/4] Comment up the testing a bit more --- src/documents/tests/test_api.py | 77 +++++++++++++++++++++++++-------- 1 file changed, 60 insertions(+), 17 deletions(-) diff --git a/src/documents/tests/test_api.py b/src/documents/tests/test_api.py index 419c7ffe9..96f82dea3 100644 --- a/src/documents/tests/test_api.py +++ b/src/documents/tests/test_api.py @@ -1847,21 +1847,58 @@ class TestDocumentApi(DirectoriesMixin, APITestCase): self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) def test_tag_unique_name_and_owner(self): + """ + GIVEN: + - Multiple users + - Tags owned by particular users + WHEN: + - API request for creating items which are unique by name and owner + THEN: + - Unique items are created + - Non-unique items are not allowed + """ user1 = User.objects.create_user(username="test1") + user1.user_permissions.add(*Permission.objects.filter(codename="add_tag")) + user1.save() + user2 = User.objects.create_user(username="test2") + user2.user_permissions.add(*Permission.objects.filter(codename="add_tag")) + user2.save() + # User 1 creates tag 1 owned by user 1 by default + # No issue + self.client.force_authenticate(user1) response = self.client.post("/api/tags/", {"name": "tag 1"}, format="json") self.assertEqual(response.status_code, status.HTTP_201_CREATED) + # User 2 creates tag 1 owned by user 2 by default + # No issue + self.client.force_authenticate(user2) response = self.client.post("/api/tags/", {"name": "tag 1"}, format="json") - self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + self.assertEqual(response.status_code, status.HTTP_201_CREATED) - self.client.post( + # User 2 creates tag 2 owned by user 1 + # No issue + self.client.force_authenticate(user2) + response = self.client.post( "/api/tags/", {"name": "tag 2", "owner": user1.pk}, format="json", ) + self.assertEqual(response.status_code, status.HTTP_201_CREATED) + # User 1 creates tag 2 owned by user 1 by default + # Not allowed, would create tag2/user1 which already exists + self.client.force_authenticate(user1) + response = self.client.post( + "/api/tags/", + {"name": "tag 2"}, + format="json", + ) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + + # User 1 creates tag 2 owned by user 1 + # Not allowed, would create tag2/user1 which already exists response = self.client.post( "/api/tags/", {"name": "tag 2", "owner": user1.pk}, @@ -1869,27 +1906,33 @@ class TestDocumentApi(DirectoriesMixin, APITestCase): ) self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) - self.client.post( - "/api/tags/", - {"name": "tag 3", "owner": user1.pk}, - format="json", - ) - - response = self.client.post( - "/api/tags/", - {"name": "tag 3", "owner": user2.pk}, - format="json", - ) - - self.assertEqual(response.status_code, status.HTTP_201_CREATED) - def test_tag_unique_name_and_owner_enforced_on_update(self): + """ + GIVEN: + - Multiple users + - Tags owned by particular users + WHEN: + - API request for to update tag in such as way as makes it non-unqiue + THEN: + - Unique items are created + - Non-unique items are not allowed on update + """ user1 = User.objects.create_user(username="test1") - user2 = User.objects.create_user(username="test2") + user1.user_permissions.add(*Permission.objects.filter(codename="change_tag")) + user1.save() + user2 = User.objects.create_user(username="test2") + user2.user_permissions.add(*Permission.objects.filter(codename="change_tag")) + user2.save() + + # Create name tag 1 owned by user 1 + # Create name tag 1 owned by user 2 Tag.objects.create(name="tag 1", owner=user1) tag2 = Tag.objects.create(name="tag 1", owner=user2) + # User 2 attempts to change the owner of tag to user 1 + # Not allowed, would change to tag1/user1 which already exists + self.client.force_authenticate(user2) response = self.client.patch( f"/api/tags/{tag2.id}/", {"owner": user1.pk},