diff --git a/src/documents/models.py b/src/documents/models.py index f74dd8686..c8778b4f5 100644 --- a/src/documents/models.py +++ b/src/documents/models.py @@ -99,6 +99,8 @@ class Correspondent(MatchingModel): class Tag(MatchingModel): color = models.CharField(_("color"), max_length=7, default="#a6cee3") + # Maximum allowed nesting depth for tags (root = 1, max depth = 5) + MAX_NESTING_DEPTH: Final[int] = 5 is_inbox_tag = models.BooleanField( _("is inbox tag"), @@ -136,9 +138,43 @@ class Tag(MatchingModel): ancestors.extend(self.parent.get_all_ancestors()) return ancestors + def subtree_height(self, node) -> int: + children = list(node.children.all()) + if not children: + return 0 + return 1 + max(self.subtree_height(child) for child in children) + def clean(self): + # Prevent self-parenting if self.parent == self: - raise ValidationError("Cannot set itself as parent.") + raise ValidationError(_("Cannot set itself as parent.")) + + # Prevent assigning a descendant as parent + if ( + self.parent + and self.pk is not None + and any( + ancestor.pk == self.pk for ancestor in self.parent.get_all_ancestors() + ) + ): + raise ValidationError(_("Cannot set parent to a descendant.")) + + # Enforce maximum nesting depth + new_parent_depth = 0 + if self.parent: + new_parent_depth = len(self.parent.get_all_ancestors()) + 1 + if self.pk is None: + # Unsaved tag cannot have children; treat as leaf + height = 0 + else: + try: + height = self.subtree_height(self) + except RecursionError: + raise ValidationError(_("Invalid tag hierarchy.")) + deepest_new_depth = (new_parent_depth + 1) + height + if deepest_new_depth > self.MAX_NESTING_DEPTH: + raise ValidationError(_("Maximum nesting depth exceeded.")) + return super().clean() diff --git a/src/documents/serialisers.py b/src/documents/serialisers.py index bea37126c..83d250eaa 100644 --- a/src/documents/serialisers.py +++ b/src/documents/serialisers.py @@ -13,6 +13,7 @@ from django.conf import settings from django.contrib.auth.models import Group from django.contrib.auth.models import User from django.contrib.contenttypes.models import ContentType +from django.core.exceptions import ValidationError from django.core.validators import DecimalValidator from django.core.validators import MaxLengthValidator from django.core.validators import RegexValidator @@ -579,6 +580,30 @@ class TagSerializer(MatchingModelSerializer, OwnedObjectSerializer): raise serializers.ValidationError(_("Invalid color.")) return color + def validate(self, attrs): + # Validate when changing parent + parent = attrs.get("parent", self.instance.parent if self.instance else None) + + if self.instance: + # Temporarily set parent on the instance if updating and use model clean() + original_parent = self.instance.parent + try: + self.instance.parent = parent + self.instance.clean() + except ValidationError as e: + raise serializers.ValidationError({"parent": list(e)}) + finally: + self.instance.parent = original_parent + else: + # For new instances, create a transient Tag and validate + temp = Tag(parent=parent) + try: + temp.clean() + except ValidationError as e: + raise serializers.ValidationError({"parent": list(e)}) + + return super().validate(attrs) + class CorrespondentField(serializers.PrimaryKeyRelatedField): def get_queryset(self): diff --git a/src/documents/tests/test_tag_hierarchy.py b/src/documents/tests/test_tag_hierarchy.py index b0c85ab48..14ba3cce4 100644 --- a/src/documents/tests/test_tag_hierarchy.py +++ b/src/documents/tests/test_tag_hierarchy.py @@ -110,3 +110,84 @@ class TestTagHierarchy(APITestCase): self.document.refresh_from_db() tags = set(self.document.tags.values_list("pk", flat=True)) assert tags == {self.parent.pk, orphan.pk} + + def test_cannot_set_parent_to_self(self): + tag = Tag.objects.create(name="Selfie") + resp = self.client.patch( + f"/api/tags/{tag.pk}/", + {"parent": tag.pk}, + format="json", + ) + assert resp.status_code == 400 + assert "parent" in resp.data + + def test_cannot_set_parent_to_descendant(self): + a = Tag.objects.create(name="A") + b = Tag.objects.create(name="B", parent=a) + c = Tag.objects.create(name="C", parent=b) + + # Attempt to set A's parent to C (descendant) should fail + resp = self.client.patch( + f"/api/tags/{a.pk}/", + {"parent": c.pk}, + format="json", + ) + assert resp.status_code == 400 + assert "parent" in resp.data + + def test_max_depth_on_create(self): + a = Tag.objects.create(name="A1") + b = Tag.objects.create(name="B1", parent=a) + c = Tag.objects.create(name="C1", parent=b) + d = Tag.objects.create(name="D1", parent=c) + + # Creating E under D yields depth 5: allowed + resp_ok = self.client.post( + "/api/tags/", + {"name": "E1", "parent": d.pk}, + format="json", + ) + assert resp_ok.status_code in (200, 201) + e_id = ( + resp_ok.data["id"] if resp_ok.status_code == 201 else resp_ok.data.get("id") + ) + assert e_id is not None + + # Creating F under E would yield depth 6: rejected + resp_fail = self.client.post( + "/api/tags/", + {"name": "F1", "parent": e_id}, + format="json", + ) + assert resp_fail.status_code == 400 + assert "parent" in resp_fail.data + assert any("Maximum" in str(msg) for msg in resp_fail.data["parent"]) + + def test_max_depth_on_move_subtree(self): + a = Tag.objects.create(name="A2") + b = Tag.objects.create(name="B2", parent=a) + c = Tag.objects.create(name="C2", parent=b) + d = Tag.objects.create(name="D2", parent=c) + + x = Tag.objects.create(name="X2") + y = Tag.objects.create(name="Y2", parent=x) + assert y.parent_id == x.id + + # Moving X under D would make deepest node Y exceed depth 5 -> reject + resp_fail = self.client.patch( + f"/api/tags/{x.pk}/", + {"parent": d.pk}, + format="json", + ) + assert resp_fail.status_code == 400 + assert "parent" in resp_fail.data + + # Moving X under C (depth 3) should be allowed (deepest becomes 5) + resp_ok = self.client.patch( + f"/api/tags/{x.pk}/", + {"parent": c.pk}, + format="json", + ) + assert resp_ok.status_code in (200, 202) + x.refresh_from_db() + assert x.parent_id == c.id