From 70e015b02e725f764572475ffd452a957d76812e Mon Sep 17 00:00:00 2001 From: Brian Helba Date: Mon, 6 Nov 2023 18:34:24 -0500 Subject: [PATCH] Fix N818 Exception name should be named with an Error suffix --- dandiapi/api/asset_paths.py | 4 +-- dandiapi/api/services/asset/__init__.py | 30 +++++++++--------- dandiapi/api/services/asset/exceptions.py | 12 ++++---- dandiapi/api/services/dandiset/__init__.py | 14 ++++----- dandiapi/api/services/dandiset/exceptions.py | 4 +-- dandiapi/api/services/embargo/__init__.py | 10 +++--- dandiapi/api/services/embargo/exceptions.py | 6 ++-- dandiapi/api/services/exceptions.py | 6 ++-- dandiapi/api/services/metadata/__init__.py | 11 ++++--- dandiapi/api/services/metadata/exceptions.py | 6 ++-- dandiapi/api/services/publish/__init__.py | 32 ++++++++++---------- dandiapi/api/services/publish/exceptions.py | 14 ++++----- dandiapi/api/tests/test_asset.py | 6 ++-- dandiapi/api/tests/test_asset_paths.py | 4 +-- dandiapi/api/views/asset.py | 4 +-- dandiapi/drf_utils.py | 4 +-- 16 files changed, 85 insertions(+), 82 deletions(-) diff --git a/dandiapi/api/asset_paths.py b/dandiapi/api/asset_paths.py index 97597d024..a24323043 100644 --- a/dandiapi/api/asset_paths.py +++ b/dandiapi/api/asset_paths.py @@ -125,12 +125,12 @@ def insert_asset_paths(asset: Asset, version: Version): path=asset.path, asset=asset, version=version ) except IntegrityError as e: - from dandiapi.api.services.asset.exceptions import AssetAlreadyExists + from dandiapi.api.services.asset.exceptions import AssetAlreadyExistsError # If there are simultaneous requests to create the same asset, this check constraint can # fail, and should be handled directly, rather than be allowed to bubble up if 'unique-version-path' in str(e): - raise AssetAlreadyExists + raise AssetAlreadyExistsError # Re-raise original exception otherwise raise diff --git a/dandiapi/api/services/asset/__init__.py b/dandiapi/api/services/asset/__init__.py index 135798d49..eb88c64db 100644 --- a/dandiapi/api/services/asset/__init__.py +++ b/dandiapi/api/services/asset/__init__.py @@ -4,11 +4,11 @@ from dandiapi.api.models.asset import Asset, AssetBlob, EmbargoedAssetBlob from dandiapi.api.models.version import Version from dandiapi.api.services.asset.exceptions import ( - AssetAlreadyExists, - AssetPathConflict, - DandisetOwnerRequired, - DraftDandisetNotModifiable, - ZarrArchiveBelongsToDifferentDandiset, + AssetAlreadyExistsError, + AssetPathConflictError, + DandisetOwnerRequiredError, + DraftDandisetNotModifiableError, + ZarrArchiveBelongsToDifferentDandisetError, ) from dandiapi.zarr.models import ZarrArchive @@ -58,9 +58,9 @@ def change_asset( assert 'path' in new_metadata, 'Path must be present in new_metadata' if not user.has_perm('owner', version.dandiset): - raise DandisetOwnerRequired + raise DandisetOwnerRequiredError elif version.version != 'draft': - raise DraftDandisetNotModifiable + raise DraftDandisetNotModifiableError path = new_metadata['path'] new_metadata_stripped = Asset.strip_metadata(new_metadata) @@ -75,7 +75,7 @@ def change_asset( # Verify we aren't changing path to the same value as an existing asset if version.assets.filter(path=path).exclude(asset_id=asset.asset_id).exists(): - raise AssetAlreadyExists + raise AssetAlreadyExistsError with transaction.atomic(): remove_asset_from_version(user=user, asset=asset, version=version) @@ -109,23 +109,23 @@ def add_asset_to_version( assert 'path' in metadata, 'Path must be present in metadata' if not user.has_perm('owner', version.dandiset): - raise DandisetOwnerRequired + raise DandisetOwnerRequiredError elif version.version != 'draft': - raise DraftDandisetNotModifiable + raise DraftDandisetNotModifiableError # Check if there are already any assets with the same path path = metadata['path'] if version.assets.filter(path=path).exists(): - raise AssetAlreadyExists + raise AssetAlreadyExistsError # Check if there are any assets that conflict with this path conflicts = get_conflicting_paths(path, version) if conflicts: - raise AssetPathConflict(new_path=path, existing_paths=conflicts) + raise AssetPathConflictError(new_path=path, existing_paths=conflicts) # Ensure zarr archive doesn't already belong to a dandiset if zarr_archive and zarr_archive.dandiset != version.dandiset: - raise ZarrArchiveBelongsToDifferentDandiset + raise ZarrArchiveBelongsToDifferentDandisetError if isinstance(asset_blob, EmbargoedAssetBlob): embargoed_asset_blob = asset_blob @@ -155,9 +155,9 @@ def add_asset_to_version( def remove_asset_from_version(*, user, asset: Asset, version: Version) -> Version: if not user.has_perm('owner', version.dandiset): - raise DandisetOwnerRequired + raise DandisetOwnerRequiredError elif version.version != 'draft': - raise DraftDandisetNotModifiable + raise DraftDandisetNotModifiableError with transaction.atomic(): # Remove asset paths and asset itself from version diff --git a/dandiapi/api/services/asset/exceptions.py b/dandiapi/api/services/asset/exceptions.py index 80dfffa9b..0bd06d91d 100644 --- a/dandiapi/api/services/asset/exceptions.py +++ b/dandiapi/api/services/asset/exceptions.py @@ -1,23 +1,23 @@ from rest_framework import status -from dandiapi.api.services.exceptions import DandiException +from dandiapi.api.services.exceptions import DandiError -class DandisetOwnerRequired(DandiException): +class DandisetOwnerRequiredError(DandiError): http_status_code = status.HTTP_403_FORBIDDEN -class DraftDandisetNotModifiable(DandiException): +class DraftDandisetNotModifiableError(DandiError): http_status_code = status.HTTP_405_METHOD_NOT_ALLOWED message = 'Only draft versions can be modified.' -class AssetAlreadyExists(DandiException): +class AssetAlreadyExistsError(DandiError): http_status_code = status.HTTP_409_CONFLICT message = 'An asset with that path already exists' -class AssetPathConflict(DandiException): +class AssetPathConflictError(DandiError): http_status_code = status.HTTP_409_CONFLICT def __init__(self, new_path: str, existing_paths: list[str]) -> None: @@ -25,6 +25,6 @@ def __init__(self, new_path: str, existing_paths: list[str]) -> None: super().__init__(message) -class ZarrArchiveBelongsToDifferentDandiset(DandiException): +class ZarrArchiveBelongsToDifferentDandisetError(DandiError): http_status_code = status.HTTP_400_BAD_REQUEST message = 'The zarr archive belongs to a different dandiset' diff --git a/dandiapi/api/services/dandiset/__init__.py b/dandiapi/api/services/dandiset/__init__.py index 2c125ba12..83497e889 100644 --- a/dandiapi/api/services/dandiset/__init__.py +++ b/dandiapi/api/services/dandiset/__init__.py @@ -2,8 +2,8 @@ from dandiapi.api.models.dandiset import Dandiset from dandiapi.api.models.version import Version -from dandiapi.api.services.dandiset.exceptions import DandisetAlreadyExists -from dandiapi.api.services.exceptions import AdminOnlyOperation, NotAllowed +from dandiapi.api.services.dandiset.exceptions import DandisetAlreadyExistsError +from dandiapi.api.services.exceptions import AdminOnlyOperationError, NotAllowedError from dandiapi.api.services.version.metadata import _normalize_version_metadata @@ -16,13 +16,13 @@ def create_dandiset( version_metadata: dict, ) -> tuple[Dandiset, Version]: if identifier and not user.is_superuser: - raise AdminOnlyOperation( + raise AdminOnlyOperationError( 'Creating a dandiset for a given identifier is an admin only operation.' ) existing_dandiset = Dandiset.objects.filter(id=identifier).first() if existing_dandiset: - raise DandisetAlreadyExists(f'Dandiset {existing_dandiset.identifier} already exists') + raise DandisetAlreadyExistsError(f'Dandiset {existing_dandiset.identifier} already exists') embargo_status = Dandiset.EmbargoStatus.EMBARGOED if embargo else Dandiset.EmbargoStatus.OPEN version_metadata = _normalize_version_metadata( @@ -48,11 +48,11 @@ def create_dandiset( def delete_dandiset(*, user, dandiset: Dandiset) -> None: if not user.has_perm('owner', dandiset): - raise NotAllowed('Cannot delete dandisets which you do not own.') + raise NotAllowedError('Cannot delete dandisets which you do not own.') if dandiset.versions.exclude(version='draft').exists(): - raise NotAllowed('Cannot delete dandisets with published versions.') + raise NotAllowedError('Cannot delete dandisets with published versions.') if dandiset.versions.filter(status=Version.Status.PUBLISHING).exists(): - raise NotAllowed('Cannot delete dandisets that are currently being published.') + raise NotAllowedError('Cannot delete dandisets that are currently being published.') # Delete all versions first, so that AssetPath deletion is cascaded # through versions, rather than through zarrs directly diff --git a/dandiapi/api/services/dandiset/exceptions.py b/dandiapi/api/services/dandiset/exceptions.py index bfd02b70d..40a886332 100644 --- a/dandiapi/api/services/dandiset/exceptions.py +++ b/dandiapi/api/services/dandiset/exceptions.py @@ -1,7 +1,7 @@ from rest_framework import status -from dandiapi.api.services.exceptions import DandiException +from dandiapi.api.services.exceptions import DandiError -class DandisetAlreadyExists(DandiException): +class DandisetAlreadyExistsError(DandiError): http_status_code = status.HTTP_400_BAD_REQUEST diff --git a/dandiapi/api/services/embargo/__init__.py b/dandiapi/api/services/embargo/__init__.py index d3466cfbc..4b0b2c48c 100644 --- a/dandiapi/api/services/embargo/__init__.py +++ b/dandiapi/api/services/embargo/__init__.py @@ -4,16 +4,16 @@ from dandiapi.api.copy import copy_object_multipart from dandiapi.api.models import Asset, AssetBlob, Dandiset, Upload, Version -from dandiapi.api.services.asset.exceptions import DandisetOwnerRequired +from dandiapi.api.services.asset.exceptions import DandisetOwnerRequiredError from dandiapi.api.tasks import unembargo_dandiset_task -from .exceptions import AssetNotEmbargoed, DandisetNotEmbargoed +from .exceptions import AssetNotEmbargoedError, DandisetNotEmbargoedError def _unembargo_asset(asset: Asset): """Unembargo an asset by copying its blob to the public bucket.""" if asset.embargoed_blob is None: - raise AssetNotEmbargoed + raise AssetNotEmbargoedError # Use existing AssetBlob if possible etag = asset.embargoed_blob.etag @@ -72,9 +72,9 @@ def _unembargo_dandiset(dandiset: Dandiset): def unembargo_dandiset(*, user: User, dandiset: Dandiset): """Unembargo a dandiset by copying all embargoed asset blobs to the public bucket.""" if dandiset.embargo_status != Dandiset.EmbargoStatus.EMBARGOED: - raise DandisetNotEmbargoed + raise DandisetNotEmbargoedError if not user.has_perm('owner', dandiset): - raise DandisetOwnerRequired + raise DandisetOwnerRequiredError unembargo_dandiset_task.delay(dandiset.id) diff --git a/dandiapi/api/services/embargo/exceptions.py b/dandiapi/api/services/embargo/exceptions.py index 79934d695..1955b49d6 100644 --- a/dandiapi/api/services/embargo/exceptions.py +++ b/dandiapi/api/services/embargo/exceptions.py @@ -1,13 +1,13 @@ from rest_framework import status -from dandiapi.api.services.exceptions import DandiException +from dandiapi.api.services.exceptions import DandiError -class AssetNotEmbargoed(DandiException): +class AssetNotEmbargoedError(DandiError): http_status_code = status.HTTP_400_BAD_REQUEST message = 'Only embargoed assets can be unembargoed.' -class DandisetNotEmbargoed(DandiException): +class DandisetNotEmbargoedError(DandiError): http_status_code = status.HTTP_400_BAD_REQUEST message = 'Dandiset not embargoed' diff --git a/dandiapi/api/services/exceptions.py b/dandiapi/api/services/exceptions.py index 109f5566a..777dc432e 100644 --- a/dandiapi/api/services/exceptions.py +++ b/dandiapi/api/services/exceptions.py @@ -1,7 +1,7 @@ from rest_framework import status -class DandiException(Exception): +class DandiError(Exception): message: str | None http_status_code: int | None @@ -14,10 +14,10 @@ def __init__( super().__init__(*args) -class NotAllowed(DandiException): +class NotAllowedError(DandiError): message = 'Action not allowed' http_status_code = status.HTTP_403_FORBIDDEN -class AdminOnlyOperation(DandiException): +class AdminOnlyOperationError(DandiError): pass diff --git a/dandiapi/api/services/metadata/__init__.py b/dandiapi/api/services/metadata/__init__.py index 1ca65ab9d..05b9a4d55 100644 --- a/dandiapi/api/services/metadata/__init__.py +++ b/dandiapi/api/services/metadata/__init__.py @@ -7,7 +7,10 @@ import jsonschema.exceptions from dandiapi.api.models import Asset, Version -from dandiapi.api.services.metadata.exceptions import AssetHasBeenPublished, VersionHasBeenPublished +from dandiapi.api.services.metadata.exceptions import ( + AssetHasBeenPublishedError, + VersionHasBeenPublishedError, +) from dandiapi.api.services.publish import _build_publishable_version_from_draft logger = get_task_logger(__name__) @@ -38,7 +41,7 @@ def validate_asset_metadata(*, asset: Asset) -> bool: # Published assets are immutable if asset.published: - raise AssetHasBeenPublished + raise AssetHasBeenPublishedError # track the state of the asset before to use optimistic locking asset_state = asset.status @@ -76,7 +79,7 @@ def validate_asset_metadata(*, asset: Asset) -> bool: def version_aggregate_assets_summary(version: Version) -> None: if version.version != 'draft': - raise VersionHasBeenPublished + raise VersionHasBeenPublishedError version.metadata['assetsSummary'] = aggregate_assets_summary( asset.full_metadata @@ -117,7 +120,7 @@ def _build_validatable_version_metadata(version: Version) -> dict: # Published versions are immutable if version.version != 'draft': - raise VersionHasBeenPublished + raise VersionHasBeenPublishedError with transaction.atomic(): # validating version metadata needs to lock the version to avoid racing with diff --git a/dandiapi/api/services/metadata/exceptions.py b/dandiapi/api/services/metadata/exceptions.py index a4421df2c..564c8257d 100644 --- a/dandiapi/api/services/metadata/exceptions.py +++ b/dandiapi/api/services/metadata/exceptions.py @@ -1,13 +1,13 @@ from rest_framework import status -from dandiapi.api.services.exceptions import DandiException +from dandiapi.api.services.exceptions import DandiError -class AssetHasBeenPublished(DandiException): +class AssetHasBeenPublishedError(DandiError): http_status_code = status.HTTP_500_INTERNAL_SERVER_ERROR message = 'This asset has been published and cannot be modified.' -class VersionHasBeenPublished(DandiException): +class VersionHasBeenPublishedError(DandiError): http_status_code = status.HTTP_500_INTERNAL_SERVER_ERROR message = 'This version has been published and cannot be modified.' diff --git a/dandiapi/api/services/publish/__init__.py b/dandiapi/api/services/publish/__init__.py index 61b12f339..1c5402b02 100644 --- a/dandiapi/api/services/publish/__init__.py +++ b/dandiapi/api/services/publish/__init__.py @@ -9,14 +9,14 @@ from dandiapi.api import doi from dandiapi.api.asset_paths import add_version_asset_paths from dandiapi.api.models import Asset, Dandiset, Version -from dandiapi.api.services.exceptions import NotAllowed +from dandiapi.api.services.exceptions import NotAllowedError from dandiapi.api.services.publish.exceptions import ( - DandisetAlreadyPublished, - DandisetAlreadyPublishing, - DandisetBeingValidated, - DandisetInvalidMetadata, - DandisetNotLocked, - DandisetValidationPending, + DandisetAlreadyPublishedError, + DandisetAlreadyPublishingError, + DandisetBeingValidatedError, + DandisetInvalidMetadataError, + DandisetNotLockedError, + DandisetValidationPendingError, ) from dandiapi.api.tasks import write_manifest_files @@ -45,27 +45,27 @@ def _lock_dandiset_for_publishing(*, user: User, dandiset: Dandiset) -> None: not user.has_perm('owner', dandiset) or dandiset.embargo_status != Dandiset.EmbargoStatus.OPEN ): - raise NotAllowed + raise NotAllowedError if dandiset.zarr_archives.exists() or dandiset.embargoed_zarr_archives.exists(): # TODO: return a string instead of a list here - raise NotAllowed(['Cannot publish dandisets which contain zarrs'], 400) # type: ignore + raise NotAllowedError(['Cannot publish dandisets which contain zarrs'], 400) # type: ignore with transaction.atomic(): draft_version: Version = dandiset.versions.select_for_update().get(version='draft') if not draft_version.publishable: match draft_version.status: case Version.Status.PUBLISHED: - raise DandisetAlreadyPublished + raise DandisetAlreadyPublishedError case Version.Status.PUBLISHING: - raise DandisetAlreadyPublishing + raise DandisetAlreadyPublishingError case Version.Status.VALIDATING: - raise DandisetBeingValidated + raise DandisetBeingValidatedError case Version.Status.INVALID: - raise DandisetInvalidMetadata + raise DandisetInvalidMetadataError case Version.Status.PENDING: - raise DandisetValidationPending + raise DandisetValidationPendingError case Version.Status.VALID: - raise DandisetInvalidMetadata + raise DandisetInvalidMetadataError case other: raise NotImplementedError( f'Draft version of dandiset {dandiset.identifier} ' @@ -110,7 +110,7 @@ def _publish_dandiset(dandiset_id: int) -> None: ) if old_version.status != Version.Status.PUBLISHING: - raise DandisetNotLocked( + raise DandisetNotLockedError( 'Dandiset must be in PUBLISHING state. Call `_lock_dandiset_for_publishing()` ' 'before this function.' ) diff --git a/dandiapi/api/services/publish/exceptions.py b/dandiapi/api/services/publish/exceptions.py index de9063565..a0a3cb95f 100644 --- a/dandiapi/api/services/publish/exceptions.py +++ b/dandiapi/api/services/publish/exceptions.py @@ -1,32 +1,32 @@ from rest_framework import status -from dandiapi.api.services.exceptions import DandiException +from dandiapi.api.services.exceptions import DandiError -class DandisetAlreadyPublished(DandiException): +class DandisetAlreadyPublishedError(DandiError): http_status_code = status.HTTP_400_BAD_REQUEST message = 'There have been no changes to the draft version since the last publish.' -class DandisetAlreadyPublishing(DandiException): +class DandisetAlreadyPublishingError(DandiError): http_status_code = status.HTTP_423_LOCKED message = 'Dandiset is currently being published' -class DandisetBeingValidated(DandiException): +class DandisetBeingValidatedError(DandiError): http_status_code = status.HTTP_409_CONFLICT message = 'Dandiset is currently being validated' -class DandisetInvalidMetadata(DandiException): +class DandisetInvalidMetadataError(DandiError): http_status_code = status.HTTP_400_BAD_REQUEST message = 'Dandiset metadata or asset metadata is not valid' -class DandisetValidationPending(DandiException): +class DandisetValidationPendingError(DandiError): http_status_code = status.HTTP_409_CONFLICT message = 'Metadata validation is pending for this dandiset, please try again later.' -class DandisetNotLocked(DandiException): +class DandisetNotLockedError(DandiError): http_status_code = status.HTTP_409_CONFLICT diff --git a/dandiapi/api/tests/test_asset.py b/dandiapi/api/tests/test_asset.py index 18c44ca1e..f9cae4e0f 100644 --- a/dandiapi/api/tests/test_asset.py +++ b/dandiapi/api/tests/test_asset.py @@ -14,7 +14,7 @@ from dandiapi.api.models.asset_paths import AssetPath from dandiapi.api.models.dandiset import Dandiset from dandiapi.api.services.asset import add_asset_to_version -from dandiapi.api.services.asset.exceptions import AssetPathConflict +from dandiapi.api.services.asset.exceptions import AssetPathConflictError from dandiapi.api.services.publish import publish_asset from dandiapi.api.tasks.scheduled import validate_pending_asset_metadata from dandiapi.zarr.models import ZarrArchive, ZarrArchiveStatus @@ -557,7 +557,7 @@ def test_asset_create_conflicting_path(api_client, user, draft_version, asset_bl ) # Add an asset that has a path which fully contains that of the first asset - with pytest.raises(AssetPathConflict): + with pytest.raises(AssetPathConflictError): add_asset_to_version( user=user, version=draft_version, @@ -569,7 +569,7 @@ def test_asset_create_conflicting_path(api_client, user, draft_version, asset_bl ) # Add an asset that's path is fully contained by the first asset - with pytest.raises(AssetPathConflict): + with pytest.raises(AssetPathConflictError): add_asset_to_version( user=user, version=draft_version, diff --git a/dandiapi/api/tests/test_asset_paths.py b/dandiapi/api/tests/test_asset_paths.py index b53b8ea8a..00fb41ea2 100644 --- a/dandiapi/api/tests/test_asset_paths.py +++ b/dandiapi/api/tests/test_asset_paths.py @@ -13,7 +13,7 @@ ) from dandiapi.api.models import Asset, AssetPath, Version from dandiapi.api.models.asset_paths import AssetPathRelation -from dandiapi.api.services.asset.exceptions import AssetAlreadyExists +from dandiapi.api.services.asset.exceptions import AssetAlreadyExistsError from dandiapi.api.tasks import publish_dandiset_task @@ -113,7 +113,7 @@ def test_asset_path_add_asset_conflicting_path(draft_version_factory, asset_fact assert version.asset_paths.filter(asset__isnull=False).count() == 1 # Ensure that adding asset2 raises the correct exception - with pytest.raises(AssetAlreadyExists): + with pytest.raises(AssetAlreadyExistsError): add_asset_paths(asset2, version) # Ensure that there no new asset paths created diff --git a/dandiapi/api/views/asset.py b/dandiapi/api/views/asset.py index 79de2e0ba..7e986372b 100644 --- a/dandiapi/api/views/asset.py +++ b/dandiapi/api/views/asset.py @@ -8,7 +8,7 @@ change_asset, remove_asset_from_version, ) -from dandiapi.api.services.asset.exceptions import DraftDandisetNotModifiable +from dandiapi.api.services.asset.exceptions import DraftDandisetNotModifiableError from dandiapi.zarr.models import ZarrArchive try: @@ -337,7 +337,7 @@ def update(self, request, versions__dandiset__pk, versions__version, **kwargs): version=versions__version, ) if version.version != 'draft': - raise DraftDandisetNotModifiable + raise DraftDandisetNotModifiableError serializer = AssetRequestSerializer(data=self.request.data) serializer.is_valid(raise_exception=True) diff --git a/dandiapi/drf_utils.py b/dandiapi/drf_utils.py index f39e5a9fd..33d11a88d 100644 --- a/dandiapi/drf_utils.py +++ b/dandiapi/drf_utils.py @@ -8,7 +8,7 @@ from rest_framework.serializers import as_serializer_error from rest_framework.views import exception_handler -from dandiapi.api.services.exceptions import DandiException +from dandiapi.api.services.exceptions import DandiError def rewrap_django_core_exceptions(exc: Exception, ctx: dict) -> Response | None: @@ -25,7 +25,7 @@ def rewrap_django_core_exceptions(exc: Exception, ctx: dict) -> Response | None: return Response(exc.error_list[0].message, status=status.HTTP_400_BAD_REQUEST) else: exc = drf_exceptions.ValidationError(as_serializer_error(exc)) - elif isinstance(exc, DandiException): + elif isinstance(exc, DandiError): return Response(exc.message, status=exc.http_status_code or status.HTTP_400_BAD_REQUEST) if isinstance(exc, Http404):