Rework FolderSerializer to use native DRF validation

dkc/core/models/folder.py

@@ -116,9 +116,7 @@ def increment_size(self, amount: int) -> None:
 
     def clean(self) -> None:
         if self.parent and self.parent.files.filter(name=self.name).exists():
-            raise ValidationError(
-                {'name': f'There is already a file here with the name "{self.name}".'}
-            )
+            raise ValidationError({'name': 'A file with that name already exists here.'})
         super().clean()
 
     @classmethod

dkc/core/rest/folder.py

@@ -12,7 +12,7 @@
 from rest_framework.response import Response
 from rest_framework.viewsets import ModelViewSet
 
-from dkc.core.models import Folder, Terms, TermsAgreement, Tree
+from dkc.core.models import File, Folder, Terms, TermsAgreement, Tree
 from dkc.core.permissions import (
     HasAccess,
     IsAdmin,
@@ -23,10 +23,10 @@
 )
 
 from .filtering import ActionSpecificFilterBackend, IntegerOrNullFilter
-from .utils import FullCleanModelSerializer
+from .utils import FormattableDict
 
 
-class FolderSerializer(FullCleanModelSerializer):
+class FolderSerializer(serializers.ModelSerializer):
     public: bool = serializers.BooleanField(read_only=True)
     access: Dict[str, bool] = serializers.SerializerMethodField()
 
@@ -43,14 +43,51 @@ class Meta:
             'public',
             'access',
         ]
+        # ModelSerializer cannot auto-generate validators for model-level constraints
+        validators = [
+            serializers.UniqueTogetherValidator(
+                queryset=Folder.objects.all(),
+                fields=['parent', 'name'],
+                message=FormattableDict({'name': 'A folder with that name already exists here.'}),
+            ),
+            # This could also be implemented as a UniqueValidator on 'name',
+            # but its easier to not explicitly redefine the whole serializer field
+            serializers.UniqueTogetherValidator(
+                queryset=Folder.objects.filter(parent=None),
+                fields=['name'],
+                message=FormattableDict({'name': 'A root folder with that name already exists.'}),
+            ),
+            # folder_max_depth and unique_root_folder_per_tree are internal sanity constraints,
+            # and do not need to be enforced as validators
+        ]
 
     def get_access(self, folder: Folder) -> Dict[str, bool]:
         return folder.tree.get_access(self.context.get('user'))
 
+    def validate(self, attrs):
+        self._validate_unique_file_siblings(attrs)
+        return attrs
+
+    def _validate_unique_file_siblings(self, attrs):
+        if self.instance is None:
+            # Create
+            # By this point, other validators will have run, ensuring that 'name' and 'parent' exist
+            name = attrs['name']
+            parent_id = attrs['parent']
+        else:
+            # Update
+            # On a partial update, 'name' and 'parent' might be absent, so use the existing instance
+            name = attrs['name'] if 'name' in attrs else self.instance.name
+            parent_id = attrs['parent'] if 'parent' in attrs else self.instance.parent_id
+        if parent_id is not None and File.objects.filter(name=name, folder_id=parent_id).exists():
+            raise serializers.ValidationError(
+                {'name': 'A file with that name already exists here.'}, code='unique'
+            )
+
 
 class FolderUpdateSerializer(FolderSerializer):
     class Meta(FolderSerializer.Meta):
-        fields = ['id', 'name', 'description']
+        read_only_fields = ['parent']
 
 
 class TermsSerializer(serializers.ModelSerializer):
@@ -134,18 +171,15 @@ def get_serializer_context(self):
     # Atomically roll back the tree creation if folder creation fails
     @transaction.atomic
     def perform_create(self, serializer: serializers.ModelSerializer):
-        parent: Folder = serializer.validated_data.get('parent')
+        parent: Folder = serializer.validated_data['parent']
+        user: User = self.request.user
         if parent:
             tree = parent.tree
-            if not tree.has_permission(serializer.context['user'], permission=Permission.write):
+            if not tree.has_permission(user, permission=Permission.write):
                 raise PermissionDenied()
         else:
             tree = Tree.objects.create()
-            tree.grant_permission(
-                PermissionGrant(
-                    user_or_group=serializer.context['user'], permission=Permission.admin
-                )
-            )
+            tree.grant_permission(PermissionGrant(user_or_group=user, permission=Permission.admin))
         serializer.save(tree=tree)
 
     @swagger_auto_schema(

dkc/core/rest/utils.py

@@ -20,3 +20,14 @@ def validate(self, data):
             except DjangoValidationError as exc:
                 raise serializers.ValidationError(serializers.as_serializer_error(exc))
         return data
+
+
+class FormattableDict(dict):
+    """
+    A dict with a no-op .format method.
+
+    This is useful to pass into DRF, as the `message` of an eventual `ValidationError`.
+    """
+
+    def format(self, *args, **kwargs):
+        return self

dkc/core/tests/test_folder.py

@@ -1,5 +1,3 @@
-import re
-
 from django.core.exceptions import ValidationError
 from django.db.utils import IntegrityError
 import pytest
@@ -73,11 +71,8 @@ def test_folder_sibling_names_unique(folder, folder_factory):
 
 @pytest.mark.django_db
 def test_folder_sibling_names_unique_files(file, folder_factory):
-    escaped = re.escape(file.name)
     sibling_folder = folder_factory.build(parent=file.folder, name=file.name)
-    with pytest.raises(
-        ValidationError, match=fr'There is already a file here with the name "{escaped}"\.'
-    ):
+    with pytest.raises(ValidationError, match='A file with that name already exists here.'):
         sibling_folder.full_clean()
 
 

dkc/core/tests/test_folder_rest.py

@@ -24,9 +24,77 @@ def test_folder_rest_path(admin_api_client, folder, folder_factory):
     assert [f['name'] for f in resp.data] == [folder.name, child.name, grandchild.name]
 
 
+@pytest.mark.django_db
+@pytest.mark.parametrize(
+    'name', ['', 'ten_chars_' * 30, 'foo/bar'], ids=['empty', 'too_long', 'forward_slash']
+)
+def test_folder_rest_create_invalid_name(admin_api_client, name):
+    resp = admin_api_client.post('/api/v2/folders', data={'name': name})
+    assert resp.status_code == 400
+    assert 'name' in resp.data
+
+
+@pytest.mark.django_db
+@pytest.mark.parametrize('description', ['ten_chars_' * 301], ids=['too_long'])
+def test_folder_rest_create_invalid_description(admin_api_client, description):
+    resp = admin_api_client.post(
+        '/api/v2/folders', data={'name': 'test folder', 'description': description}
+    )
+    assert resp.status_code == 400
+    assert 'description' in resp.data
+
+
+@pytest.mark.django_db
+@pytest.mark.parametrize('parent', ['foo', -1, 9000], ids=['non_int', 'negative', 'nonexistent'])
+def test_folder_rest_create_invalid_parent(admin_api_client, parent):
+    resp = admin_api_client.post('/api/v2/folders', data={'name': 'test folder', 'parent': parent})
+    assert resp.status_code == 400
+    assert 'parent' in resp.data
+
+
+@pytest.mark.django_db
+def test_folder_rest_create_invalid_duplicate_root(admin_api_client, folder):
+    resp = admin_api_client.post('/api/v2/folders', data={'name': folder.name, 'parent': None})
+    assert resp.status_code == 400
+    assert 'name' in resp.data
+
+
+@pytest.mark.django_db
+def test_folder_rest_create_invalid_duplicate_sibling_folder(admin_api_client, child_folder):
+    resp = admin_api_client.post(
+        '/api/v2/folders', data={'name': child_folder.name, 'parent': child_folder.parent.id}
+    )
+    assert resp.status_code == 400
+    assert 'name' in resp.data
+
+
+@pytest.mark.django_db
+def test_folder_rest_create_invalid_duplicate_sibling_file(admin_api_client, folder, file_factory):
+    child_file = file_factory(folder=folder)
+    resp = admin_api_client.post(
+        '/api/v2/folders', data={'name': child_file.name, 'parent': folder.id}
+    )
+    assert resp.status_code == 400
+    assert 'name' in resp.data
+
+
+@pytest.mark.django_db
+def test_folder_rest_create_invalid_duplicate_sibling_file_update(
+    admin_api_client, folder, file_factory, folder_factory
+):
+    # Since this is implemented with an explicitly separate code path, it deserves its own test
+    child_file = file_factory(folder=folder)
+    child_folder = folder_factory(parent=folder)
+    resp = admin_api_client.patch(
+        f'/api/v2/folders/{child_folder.id}', data={'name': child_file.name}
+    )
+    assert resp.status_code == 400
+    assert 'name' in resp.data
+
+
 @pytest.mark.django_db
 def test_folder_rest_create_root(admin_api_client):
-    resp = admin_api_client.post('/api/v2/folders', data={'name': 'test folder'})
+    resp = admin_api_client.post('/api/v2/folders', data={'name': 'test folder', 'parent': None})
     assert resp.status_code == 201
     assert Folder.objects.count() == 1
 
@@ -51,7 +119,7 @@ def test_folder_rest_retrieve(admin_api_client, folder):
 
 @pytest.mark.django_db
 def test_folder_rest_update(admin_api_client, folder):
-    resp = admin_api_client.put(
+    resp = admin_api_client.patch(
         f'/api/v2/folders/{folder.id}', data={'name': 'New name', 'description': 'New description'}
     )
     assert resp.status_code == 200

dkc/core/tests/test_permissions.py

@@ -278,7 +278,7 @@ def test_delete_permissions(api_client, user, user_factory, admin_folder):
 @pytest.mark.django_db
 def test_root_folder_create_sets_permissions(api_client, user):
     api_client.force_authenticate(user=user)
-    resp = api_client.post('/api/v2/folders', data={'name': 'test'})
+    resp = api_client.post('/api/v2/folders', data={'name': 'test', 'parent': None})
     assert resp.status_code == 201
     assert resp.data['public'] is False
     assert resp.data['access'] == {'read': True, 'write': True, 'admin': True}