diff --git a/.github/workflows/cli-integration.yml b/.github/workflows/cli-integration.yml index 89bbc4a8c..fffd69947 100644 --- a/.github/workflows/cli-integration.yml +++ b/.github/workflows/cli-integration.yml @@ -75,7 +75,7 @@ jobs: - name: Run dandi-api tests in dandi-cli if: matrix.dandi-version == 'master' run: > - uvx --with "dandi[test] @ git+https://github.com/dandi/dandi-cli" + uvx --with "dandi[test] @ git+https://github.com/dandi/dandi-cli@use-new-asset-validation-errors" pytest --pyargs -v -s --dandi-api dandi env: DANDI_TESTS_PERSIST_DOCKER_COMPOSE: "1" diff --git a/dandiapi/api/migrations/0031_alter_asset_status.py b/dandiapi/api/migrations/0031_alter_asset_status.py new file mode 100644 index 000000000..ab7be1980 --- /dev/null +++ b/dandiapi/api/migrations/0031_alter_asset_status.py @@ -0,0 +1,22 @@ +# Generated by Django 5.2.7 on 2026-02-26 00:00 +from __future__ import annotations + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ('api', '0030_alter_asset_path'), + ] + + operations = [ + migrations.AlterField( + model_name='asset', + name='status', + field=models.CharField( + choices=[('Pending', 'Pending'), ('Valid', 'Valid'), ('Invalid', 'Invalid')], + default='Pending', + max_length=10, + ), + ), + ] diff --git a/dandiapi/api/models/asset.py b/dandiapi/api/models/asset.py index d8c2c3646..766fff296 100644 --- a/dandiapi/api/models/asset.py +++ b/dandiapi/api/models/asset.py @@ -45,7 +45,6 @@ class EmbargoedAssetWithinOpenDandisetError(Exception): class AssetStatus(models.TextChoices): PENDING = 'Pending' - VALIDATING = 'Validating' VALID = 'Valid' INVALID = 'Invalid' diff --git a/dandiapi/api/models/version.py b/dandiapi/api/models/version.py index 9363b8aa3..095b946ea 100644 --- a/dandiapi/api/models/version.py +++ b/dandiapi/api/models/version.py @@ -94,29 +94,43 @@ def publishable(self) -> bool: # Return False if any asset is not VALID return not self.assets.exclude(status=Asset.Status.VALID).exists() - @property - def asset_validation_errors(self) -> list[VersionAssetValidationError]: + def get_asset_validation_errors(self) -> list[VersionAssetValidationError]: # Import here to avoid dependency cycle + from dandiapi.zarr.models import ZarrArchiveStatus + from .asset import Asset # We want to display "Pending" assets in the validation errors list, - # despite them not being stored explicitly as errors in the database. - # Grab a random sample of 50 pending or currently validating assets - # and place them first in the list. - pending_assets: models.QuerySet[Asset] = ( - self.assets.filter(status__in=[Asset.Status.PENDING, Asset.Status.VALIDATING]) + # despite them not being stored explicitly as errors in the database. + # We must exclude pending zarr assets, as they are collected in the next step. + # For performance reasons, we only return the first 50 assets. + pending_assets: models.QuerySet[VersionAssetValidationError] = ( + self.assets.filter(status=Asset.Status.PENDING) + .exclude(zarr__status=ZarrArchiveStatus.PENDING) .annotate( field=models.Value(''), message=models.Value('asset is currently being validated, please wait.'), ) - .values('field', 'message', 'path')[:50] + .values('field', 'message', 'path') + ) + + # Next, get all zarr assets which have not been finalized. These also are not stored + # explicitly as errors in the database, so we need to construct them manually. + # For performance reasons, we only return the first 50 assets. + incomplete_zarr_assets: models.QuerySet[VersionAssetValidationError] = ( + self.assets.filter(zarr__status=ZarrArchiveStatus.PENDING) + .annotate( + field=models.Value(''), + message=models.Value('zarr asset is not yet finalized.'), + ) + .values('field', 'message', 'path') ) - # Next, get all INVALID assets. Each of these should have one or more - # validation errors stored in the database. + # Finally, get all INVALID assets. Each of these should have one or more + # validation errors stored in the database. # For performance reasons, we truncate the list of INVALID assets such - # that we only display errors for the 50 assets with the most errors. - invalid_assets: models.QuerySet[Asset] = ( + # that we only display errors for the 50 assets with the most errors. + invalid_assets: models.QuerySet[dict] = ( self.assets.filter(status=Asset.Status.INVALID) .alias( validation_error_count=models.Func( @@ -127,17 +141,21 @@ def asset_validation_errors(self) -> list[VersionAssetValidationError]: ) .order_by('-validation_error_count') .values('path', 'validation_errors') - )[:50] + ) - return list(pending_assets) + [ - { - 'field': error['field'], - 'message': error['message'], - 'path': asset['path'], - } - for asset in invalid_assets - for error in asset['validation_errors'] - ] + return ( + list(pending_assets) + + list(incomplete_zarr_assets) + + [ + { + 'field': error['field'], + 'message': error['message'], + 'path': asset['path'], + } + for asset in invalid_assets + for error in asset['validation_errors'] + ] + ) @staticmethod def datetime_to_version(time: datetime.datetime) -> str: diff --git a/dandiapi/api/tests/test_asset.py b/dandiapi/api/tests/test_asset.py index a23b7565a..c54ca016c 100644 --- a/dandiapi/api/tests/test_asset.py +++ b/dandiapi/api/tests/test_asset.py @@ -649,7 +649,6 @@ def test_asset_rest_info(api_client, version, asset): ('status', 'validation_error'), [ (Asset.Status.PENDING, ''), - (Asset.Status.VALIDATING, ''), (Asset.Status.VALID, ''), (Asset.Status.INVALID, 'error'), ], diff --git a/dandiapi/api/tests/test_version.py b/dandiapi/api/tests/test_version.py index 4252cbc42..5b009f313 100644 --- a/dandiapi/api/tests/test_version.py +++ b/dandiapi/api/tests/test_version.py @@ -10,6 +10,7 @@ from django.conf import settings from freezegun import freeze_time import pytest +from zarr_checksum.checksum import EMPTY_CHECKSUM from dandiapi.api.models.dandiset import Dandiset from dandiapi.api.services.metadata import version_aggregate_assets_summary @@ -20,6 +21,7 @@ PublishedVersionFactory, UserFactory, ) +from dandiapi.zarr.models import ZarrArchiveStatus if TYPE_CHECKING: from rest_framework.test import APIClient @@ -290,7 +292,6 @@ def test_version_invalid(version, status): 'status', [ Asset.Status.PENDING, - Asset.Status.VALIDATING, Asset.Status.INVALID, ], ) @@ -489,8 +490,7 @@ def test_version_rest_info(api_client, version): 'metadata': version.metadata, 'size': version.size, 'status': version.status, - 'asset_validation_errors': [], - 'version_validation_errors': [], + 'validation_errors': [], 'contact_person': version.metadata['contributor'][0]['name'], } @@ -498,9 +498,11 @@ def test_version_rest_info(api_client, version): @pytest.mark.django_db @pytest.mark.parametrize( 'asset_status', - [Asset.Status.PENDING, Asset.Status.VALIDATING, Asset.Status.VALID, Asset.Status.INVALID], + [Asset.Status.PENDING, Asset.Status.VALID, Asset.Status.INVALID], ) -def test_version_rest_info_with_asset(api_client, draft_asset_factory, asset_status: Asset.Status): +def test_version_asset_validation_errors( + api_client, draft_asset_factory, asset_status: Asset.Status +): version = DraftVersionFactory.create(status=Version.Status.VALID) asset = draft_asset_factory(status=asset_status) version.assets.add(asset) @@ -515,35 +517,54 @@ def test_version_rest_info_with_asset(api_client, draft_asset_factory, asset_sta 'path': asset.path, } ] - if asset_status in [Asset.Status.PENDING, Asset.Status.VALIDATING] + if asset_status == Asset.Status.PENDING else [] ) - assert api_client.get( - f'/api/dandisets/{version.dandiset.identifier}/versions/{version.version}/info/' - ).data == { - 'dandiset': { - 'identifier': version.dandiset.identifier, - 'created': TIMESTAMP_RE, - 'modified': TIMESTAMP_RE, - 'contact_person': version.metadata['contributor'][0]['name'], - 'embargo_status': 'OPEN', - 'star_count': 0, - 'is_starred': False, - }, - 'version': version.version, - 'name': version.name, - 'created': TIMESTAMP_RE, - 'modified': TIMESTAMP_RE, - 'asset_count': 1, - 'active_uploads': 0, - 'metadata': version.metadata, - 'size': version.size, - 'status': Asset.Status.VALID, - 'asset_validation_errors': expected_validation_errors, - 'version_validation_errors': [], - 'contact_person': version.metadata['contributor'][0]['name'], - } + resp = api_client.get( + f'/api/dandisets/{version.dandiset.identifier}/versions/{version.version}/asset_validation_errors/' + ) + assert resp.json() == expected_validation_errors + + +@pytest.mark.django_db +@pytest.mark.parametrize('zarr_status', [c[0] for c in ZarrArchiveStatus.choices]) +def test_version_zarr_asset_validation_errors( + api_client, draft_asset_factory, zarr_archive_factory, zarr_status: ZarrArchiveStatus +): + version = DraftVersionFactory.create(status=Version.Status.VALID) + zarr_archive = zarr_archive_factory( + status=zarr_status, + # Zarr save fails if status is Complete with a null checksum + checksum=EMPTY_CHECKSUM if zarr_status == ZarrArchiveStatus.COMPLETE else None, + ) + asset = draft_asset_factory(status=Asset.Status.PENDING, blob=None, zarr=zarr_archive) + version.assets.add(asset) + add_version_asset_paths(version=version) + + # Create expected validation errors for pending/validating assets + expected_validation_errors = ( + [ + { + 'field': '', + 'message': 'zarr asset is not yet finalized.', + 'path': asset.path, + } + ] + if zarr_status == ZarrArchiveStatus.PENDING + else [ + { + 'field': '', + 'message': 'asset is currently being validated, please wait.', + 'path': asset.path, + } + ] + ) + + resp = api_client.get( + f'/api/dandisets/{version.dandiset.identifier}/versions/{version.version}/asset_validation_errors/' + ) + assert resp.json() == expected_validation_errors @pytest.mark.django_db @@ -619,8 +640,7 @@ def test_version_rest_update(api_client): 'metadata': saved_metadata, 'size': draft_version.size, 'status': 'Pending', - 'asset_validation_errors': [], - 'version_validation_errors': [], + 'validation_errors': [], 'contact_person': 'Vargas, GetĂșlio', } diff --git a/dandiapi/api/views/serializers.py b/dandiapi/api/views/serializers.py index 0132735d1..11f7e68fe 100644 --- a/dandiapi/api/views/serializers.py +++ b/dandiapi/api/views/serializers.py @@ -373,15 +373,11 @@ class VersionDetailSerializer(VersionSerializer): class Meta(VersionSerializer.Meta): fields = [ *VersionSerializer.Meta.fields, - 'asset_validation_errors', - 'version_validation_errors', + 'validation_errors', 'metadata', 'contact_person', ] - # rename this field in the serializer to differentiate from asset_validation_errors - version_validation_errors = serializers.JSONField(source='validation_errors') - def get_contact_person(self, obj): return extract_contact_person(obj) diff --git a/dandiapi/api/views/version.py b/dandiapi/api/views/version.py index bcd1f9d4d..de9c3684a 100644 --- a/dandiapi/api/views/version.py +++ b/dandiapi/api/views/version.py @@ -86,6 +86,16 @@ def info(self, request, **kwargs): ) return Response(serializer.data, status=status.HTTP_200_OK) + @swagger_auto_schema( + manual_parameters=[DANDISET_PK_PARAM, VERSION_PARAM], + responses={200: 'A list of asset validation errors.'}, + ) + @action(detail=True, methods=['GET']) + def asset_validation_errors(self, request, **kwargs): + """Get the asset validation errors on a version.""" + version: Version = self.get_object() + return Response(version.get_asset_validation_errors(), status=status.HTTP_200_OK) + @swagger_auto_schema( request_body=VersionMetadataSerializer, responses={200: VersionDetailSerializer}, diff --git a/web/src/components/DLP/OverviewTab.vue b/web/src/components/DLP/OverviewTab.vue index 3f48f1314..5a4480c08 100644 --- a/web/src/components/DLP/OverviewTab.vue +++ b/web/src/components/DLP/OverviewTab.vue @@ -158,7 +158,7 @@