From a4009b06c1771b2f7197d4f8273e489c418032dc Mon Sep 17 00:00:00 2001 From: Brian Helba Date: Tue, 7 Oct 2025 23:12:18 -0400 Subject: [PATCH 1/3] Upgrade `djangorestframework` --- pyproject.toml | 4 +--- uv.lock | 9 ++++----- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 0121d7fa1..d5f4cad69 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -28,9 +28,7 @@ dependencies = [ "django-resonant-settings[allauth,celery]", "django-resonant-utils", "django-stubs-ext", - # TODO: pin djangorestframework until we figure out what the cause of - # https://github.com/dandi/dandi-archive/issues/1896 is. - "djangorestframework<3.15.0", + "djangorestframework", "drf-extensions", "drf-yasg", "fsspec[http]", diff --git a/uv.lock b/uv.lock index 0f256a552..c80c1ead8 100644 --- a/uv.lock +++ b/uv.lock @@ -827,7 +827,7 @@ requires-dist = [ { name = "django-s3-file-field", extras = ["s3"] }, { name = "django-storages", extras = ["s3"] }, { name = "django-stubs-ext" }, - { name = "djangorestframework", specifier = "<3.15.0" }, + { name = "djangorestframework" }, { name = "drf-extensions" }, { name = "drf-yasg" }, { name = "fsspec", extras = ["http"] }, @@ -1179,15 +1179,14 @@ wheels = [ [[package]] name = "djangorestframework" -version = "3.14.0" +version = "3.16.1" source = { registry = "https://pypi.org/simple" } dependencies = [ { name = "django" }, - { name = "pytz" }, ] -sdist = { url = "https://files.pythonhosted.org/packages/8e/53/5b2a002c5ebafd60dff1e1945a7d63dee40155830997439a9ba324f0fd50/djangorestframework-3.14.0.tar.gz", hash = "sha256:579a333e6256b09489cbe0a067e66abe55c6595d8926be6b99423786334350c8", size = 1055343, upload-time = "2022-09-22T11:38:44.245Z" } +sdist = { url = "https://files.pythonhosted.org/packages/8a/95/5376fe618646fde6899b3cdc85fd959716bb67542e273a76a80d9f326f27/djangorestframework-3.16.1.tar.gz", hash = "sha256:166809528b1aced0a17dc66c24492af18049f2c9420dbd0be29422029cfc3ff7", size = 1089735, upload-time = "2025-08-06T17:50:53.251Z" } wheels = [ - { url = "https://files.pythonhosted.org/packages/ff/4b/3b46c0914ba4b7546a758c35fdfa8e7f017fcbe7f23c878239e93623337a/djangorestframework-3.14.0-py3-none-any.whl", hash = "sha256:eb63f58c9f218e1a7d064d17a70751f528ed4e1d35547fdade9aaf4cd103fd08", size = 1062761, upload-time = "2022-09-22T11:38:41.825Z" }, + { url = "https://files.pythonhosted.org/packages/b0/ce/bf8b9d3f415be4ac5588545b5fcdbbb841977db1c1d923f7568eeabe1689/djangorestframework-3.16.1-py3-none-any.whl", hash = "sha256:33a59f47fb9c85ede792cbf88bde71893bcda0667bc573f784649521f1102cec", size = 1080442, upload-time = "2025-08-06T17:50:50.667Z" }, ] [[package]] From d4030124ee19708c3fcd8e5b34d76b77a1a94175 Mon Sep 17 00:00:00 2001 From: Brian Helba Date: Fri, 24 Oct 2025 11:44:16 -0400 Subject: [PATCH 2/3] Rename the `NestedAssetViewSet` URL basename to avoid a conflict Also rename the `VersionViewSet` for consistency. --- dandiapi/urls.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dandiapi/urls.py b/dandiapi/urls.py index b9b723fca..653f0a080 100644 --- a/dandiapi/urls.py +++ b/dandiapi/urls.py @@ -45,13 +45,13 @@ .register( r'versions', VersionViewSet, - basename='version', + basename='dandiset-version', parents_query_lookups=[f'dandiset__{DandisetViewSet.lookup_field}'], ) .register( r'assets', NestedAssetViewSet, - basename='asset', + basename='dandiset-version-asset', parents_query_lookups=[ f'versions__dandiset__{DandisetViewSet.lookup_field}', f'versions__{VersionViewSet.lookup_field}', From 24e2ca1355fc186a23cd1349e583ab37b470c16f Mon Sep 17 00:00:00 2001 From: Brian Helba Date: Fri, 24 Oct 2025 11:55:56 -0400 Subject: [PATCH 3/3] 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. --- dandiapi/api/views/serializers.py | 21 ++++++++++ dandiapi/zarr/tests/test_zarr.py | 14 ++++--- dandiapi/zarr/views/__init__.py | 70 +++++++++++++++++++------------ 3 files changed, 73 insertions(+), 32 deletions(-) diff --git a/dandiapi/api/views/serializers.py b/dandiapi/api/views/serializers.py index 240e58a5f..5f4951013 100644 --- a/dandiapi/api/views/serializers.py +++ b/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() diff --git a/dandiapi/zarr/tests/test_zarr.py b/dandiapi/zarr/tests/test_zarr.py index 379534081..a6a7cbab2 100644 --- a/dandiapi/zarr/tests/test_zarr.py +++ b/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 diff --git a/dandiapi/zarr/views/__init__.py b/dandiapi/zarr/views/__init__.py index 82a8a19cb..64a8e264f 100644 --- a/dandiapi/zarr/views/__init__.py +++ b/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.',