Move validation logic into ZarrArchiveSerializer

The latest DRF now enforces uniqueness constraints in serializer validation,
so all other validation should happen at the same time to avoid incorrect
error codes being returned.

Author: brianhelba

dandiapi/api/views/serializers.py

@@ -1,6 +1,7 @@
 from __future__ import annotations
 
 from datetime import date, timedelta
+import re
 from typing import TYPE_CHECKING, Any
 
 from django.conf import settings
@@ -43,7 +44,27 @@ class UserDetailSerializer(serializers.Serializer):
     status = serializers.CharField()
 
 
+class DandisetIdentifierField(serializers.Field[int, str | int, str, Any]):
+    default_error_messages = {'invalid': 'A valid Dandiset identifier is required.'}
+
+    def to_internal_value(self, data):
+        if not isinstance(data, str | int):
+            self.fail('invalid')
+        if isinstance(data, str):
+            if not re.fullmatch(Dandiset.IDENTIFIER_REGEX, data):
+                self.fail('invalid')
+            try:
+                return int(data)
+            except (ValueError, TypeError):
+                self.fail('invalid')
+        return data
+
+    def to_representation(self, value):
+        return f'{value:06}'
+
+
 class DandisetSerializer(serializers.ModelSerializer):
+    identifier = DandisetIdentifierField()
     contact_person = serializers.SerializerMethodField(method_name='get_contact_person')
     star_count = serializers.SerializerMethodField()
     is_starred = serializers.SerializerMethodField()

dandiapi/zarr/tests/test_zarr.py

@@ -57,18 +57,20 @@ def test_zarr_rest_dandiset_malformed(api_client):
         },
     )
     assert resp.status_code == 400
-    assert resp.json() == {'dandiset': ['This value does not match the required pattern.']}
+    assert resp.json() == {'dandiset': ['A valid Dandiset identifier is required.']}
 
 
 @pytest.mark.django_db
-def test_zarr_rest_create_not_an_owner(api_client, zarr_archive):
+def test_zarr_rest_create_not_an_owner(api_client):
     user = UserFactory.create()
     api_client.force_authenticate(user=user)
+    # Don't make the user an owner
+    dandiset = DandisetFactory.create()
     resp = api_client.post(
         '/api/zarr/',
         {
-            'name': zarr_archive.name,
-            'dandiset': zarr_archive.dandiset.identifier,
+            'name': 'New Zarr',
+            'dandiset': dandiset.identifier,
         },
     )
     assert resp.status_code == 403
@@ -87,7 +89,9 @@ def test_zarr_rest_create_duplicate(api_client, zarr_archive):
         },
     )
     assert resp.status_code == 400
-    assert resp.json() == ['Zarr already exists']
+    assert resp.json() == {
+        'non_field_errors': ['The fields dandiset, name must make a unique set.']
+    }
 
 
 @pytest.mark.django_db

dandiapi/zarr/views/__init__.py

@@ -19,10 +19,12 @@
 from dandiapi.api.services.exceptions import DandiError
 from dandiapi.api.services.permissions.dandiset import get_visible_dandisets, is_dandiset_owner
 from dandiapi.api.views.pagination import DandiPagination
+from dandiapi.api.views.serializers import DandisetIdentifierField
 from dandiapi.zarr.models import ZarrArchive, ZarrArchiveStatus
 from dandiapi.zarr.tasks import ingest_zarr_archive
 
 if TYPE_CHECKING:
+    from django.contrib.auth.models import AnonymousUser, User
     from django.db.models.query import QuerySet
 
 logger = logging.getLogger(__name__)
@@ -37,7 +39,7 @@ class ZarrDeleteFileRequestSerializer(serializers.Serializer):
     path = serializers.CharField()
 
 
-class ZarrSerializer(serializers.ModelSerializer):
+class ZarrArchiveSerializer(serializers.ModelSerializer):
     class Meta:
         model = ZarrArchive
         read_only_fields = [
@@ -49,7 +51,35 @@ class Meta:
         ]
         fields = ['name', 'dandiset', *read_only_fields]
 
-    dandiset = serializers.RegexField(f'^{Dandiset.IDENTIFIER_REGEX}$')
+    dandiset = serializers.PrimaryKeyRelatedField(
+        queryset=Dandiset.objects.all(),
+        pk_field=DandisetIdentifierField(),
+    )
+
+    def __init__(self, instance=None, data=serializers.empty, **kwargs):
+        super().__init__(instance=instance, data=data, **kwargs)
+
+        # If this was loaded via ".get_serializer", then the request is available in context
+        self.user: User | AnonymousUser | None = (
+            self._context['request'].user if 'request' in self._context else None
+        )
+
+        # A user is only necessary for validating input, but not to serialize output
+        if self.user is not None:
+            # Set the queryset here, before it's actually evaluated
+            self.fields['dandiset'].queryset = get_visible_dandisets(self.user)
+
+    def validate_dandiset(self, dandiset: Dandiset) -> Dandiset:
+        if self.user is None:
+            raise ValueError('Serializer user not set')
+        if not is_dandiset_owner(dandiset, self.user):
+            raise PermissionDenied
+        if dandiset.unembargo_in_progress:
+            raise DandiError(
+                message='Cannot add zarr to dandiset during unembargo',
+                http_status_code=status.HTTP_400_BAD_REQUEST,
+            )
+        return dandiset
 
 
 class ZarrListSerializer(serializers.ModelSerializer):
@@ -91,7 +121,7 @@ class ZarrListQuerySerializer(serializers.Serializer):
 
 
 class ZarrViewSet(ReadOnlyModelViewSet):
-    serializer_class = ZarrSerializer
+    serializer_class = ZarrArchiveSerializer
     pagination_class = DandiPagination
 
     queryset = ZarrArchive.objects.select_related('dandiset').order_by('created').all()
@@ -127,42 +157,28 @@ def list(self, request, *args, **kwargs):
         return self.get_paginated_response(serializer.data)
 
     @swagger_auto_schema(
-        request_body=ZarrSerializer,
-        responses={200: ZarrSerializer},
+        request_body=ZarrArchiveSerializer,
+        responses={200: ZarrArchiveSerializer},
         operation_summary='Create a new zarr archive.',
         operation_description='',
     )
     def create(self, request):
         """Create a new zarr archive."""
-        serializer = ZarrSerializer(data=request.data)
+        serializer = self.get_serializer(data=request.data)
         serializer.is_valid(raise_exception=True)
 
-        name = serializer.validated_data['name']
-        dandiset = get_object_or_404(
-            get_visible_dandisets(request.user), id=serializer.validated_data['dandiset']
-        )
-        if not is_dandiset_owner(dandiset, request.user):
-            raise PermissionDenied
-
-        # Prevent addition to dandiset during unembargo
-        if dandiset.unembargo_in_progress:
-            raise DandiError(
-                message='Cannot add zarr to dandiset during unembargo',
-                http_status_code=status.HTTP_400_BAD_REQUEST,
-            )
-
-        zarr_archive: ZarrArchive = ZarrArchive(name=name, dandiset=dandiset)
         with transaction.atomic():
-            # Use nested transaction block to prevent zarr creation race condition
             try:
-                with transaction.atomic():
-                    zarr_archive.save()
+                zarr_archive: ZarrArchive = serializer.save()
             except IntegrityError as e:
                 raise ValidationError('Zarr already exists') from e
 
-            audit.create_zarr(dandiset=dandiset, user=request.user, zarr_archive=zarr_archive)
+            audit.create_zarr(
+                dandiset=serializer.validated_data['dandiset'],
+                user=request.user,
+                zarr_archive=zarr_archive,
+            )
 
-        serializer = ZarrSerializer(instance=zarr_archive)
         return Response(serializer.data, status=status.HTTP_200_OK)
 
     @swagger_auto_schema(
@@ -328,7 +344,7 @@ def create_files(self, request, zarr_id):
     @swagger_auto_schema(
         request_body=ZarrDeleteFileRequestSerializer(many=True),
         responses={
-            200: ZarrSerializer(many=True),
+            200: ZarrArchiveSerializer(many=True),
             400: ZarrArchive.INGEST_ERROR_MSG,
         },
         operation_summary='Delete files from a zarr archive.',