Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix N818 Exception name should be named with an Error suffix #1749

Merged
merged 1 commit into from
Nov 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions dandiapi/api/asset_paths.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
30 changes: 15 additions & 15 deletions dandiapi/api/services/asset/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
12 changes: 6 additions & 6 deletions dandiapi/api/services/asset/exceptions.py
Original file line number Diff line number Diff line change
@@ -1,30 +1,30 @@
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:
message = f'Path of new asset "{new_path}" conflicts with existing assets: {existing_paths}'
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'
14 changes: 7 additions & 7 deletions dandiapi/api/services/dandiset/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand All @@ -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(
Expand All @@ -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
Expand Down
4 changes: 2 additions & 2 deletions dandiapi/api/services/dandiset/exceptions.py
Original file line number Diff line number Diff line change
@@ -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
10 changes: 5 additions & 5 deletions dandiapi/api/services/embargo/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
6 changes: 3 additions & 3 deletions dandiapi/api/services/embargo/exceptions.py
Original file line number Diff line number Diff line change
@@ -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'
6 changes: 3 additions & 3 deletions dandiapi/api/services/exceptions.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
from rest_framework import status


class DandiException(Exception):
class DandiError(Exception):
message: str | None
http_status_code: int | None

Expand All @@ -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
11 changes: 7 additions & 4 deletions dandiapi/api/services/metadata/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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__)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
6 changes: 3 additions & 3 deletions dandiapi/api/services/metadata/exceptions.py
Original file line number Diff line number Diff line change
@@ -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.'
32 changes: 16 additions & 16 deletions dandiapi/api/services/publish/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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} '
Expand Down Expand Up @@ -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.'
)
Expand Down
Loading