diff --git a/swh/deposit/api/common.py b/swh/deposit/api/common.py --- a/swh/deposit/api/common.py +++ b/swh/deposit/api/common.py @@ -63,10 +63,7 @@ PARSING_ERROR, DepositError, ParserError, - make_error_dict, - make_error_response, - make_error_response_from_dict, - make_missing_slug_error, + raise_missing_slug_error, ) from ..models import DepositClient, DepositCollection, DepositRequest from ..parsers import parse_swh_reference, parse_xml @@ -290,9 +287,7 @@ try: deposit = Deposit.objects.get(pk=deposit_id) except Deposit.DoesNotExist: - return make_error_dict( - NOT_FOUND, f"The deposit {deposit_id} does not exist" - ) + raise DepositError(NOT_FOUND, f"The deposit {deposit_id} does not exist") DepositRequest.objects.filter(deposit=deposit, type=ARCHIVE_TYPE).delete() return {} @@ -312,9 +307,7 @@ try: deposit = Deposit.objects.get(pk=deposit_id) except Deposit.DoesNotExist: - return make_error_dict( - NOT_FOUND, f"The deposit {deposit_id} does not exist" - ) + raise DepositError(NOT_FOUND, f"The deposit {deposit_id} does not exist") if deposit.collection.name != collection_name: summary = "Cannot delete a deposit from another collection" @@ -322,7 +315,7 @@ deposit_id, collection_name, ) - return make_error_dict( + raise DepositError( BAD_REQUEST, summary=summary, verbose_description=description ) @@ -350,7 +343,7 @@ max_upload_size = self.config["max_upload_size"] if content_length: if content_length > max_upload_size: - return make_error_dict( + raise DepositError( MAX_UPLOAD_SIZE_EXCEEDED, f"Upload size limit exceeded (max {max_upload_size} bytes)." "Please consider sending the archive in multiple steps.", @@ -358,14 +351,12 @@ length = filehandler.size if length != content_length: - return make_error_dict( - status.HTTP_412_PRECONDITION_FAILED, "Wrong length" - ) + raise DepositError(status.HTTP_412_PRECONDITION_FAILED, "Wrong length") if md5sum: _md5sum = _compute_md5(filehandler) if _md5sum != md5sum: - return make_error_dict( + raise DepositError( CHECKSUM_MISMATCH, "Wrong md5 hash", f"The checksum sent {hashutil.hash_to_hex(md5sum)} and the actual " @@ -424,7 +415,7 @@ """ content_length = headers.content_length if not content_length: - return make_error_dict( + raise DepositError( BAD_REQUEST, "CONTENT_LENGTH header is mandatory", "For archive deposit, the CONTENT_LENGTH header must be sent.", @@ -432,7 +423,7 @@ content_disposition = headers.content_disposition if not content_disposition: - return make_error_dict( + raise DepositError( BAD_REQUEST, "CONTENT_DISPOSITION header is mandatory", "For archive deposit, the CONTENT_DISPOSITION header must be sent.", @@ -440,7 +431,7 @@ packaging = headers.packaging if packaging and packaging not in ACCEPT_PACKAGINGS: - return make_error_dict( + raise DepositError( BAD_REQUEST, f"Only packaging {ACCEPT_PACKAGINGS} is supported", f"The packaging provided {packaging} is not supported", @@ -457,7 +448,7 @@ slug = headers.slug if check_slug_is_present and not slug: - return make_missing_slug_error() + raise_missing_slug_error() # actual storage of data archive_metadata = filehandler @@ -542,7 +533,7 @@ """ slug = headers.slug if check_slug_is_present and not slug: - return make_missing_slug_error() + raise_missing_slug_error() content_types_present = set() @@ -555,7 +546,7 @@ fh = value content_type = fh.content_type if content_type in content_types_present: - return make_error_dict( + raise DepositError( ERROR_CONTENT, "Only 1 application/zip (or application/x-tar) archive " "and 1 atom+xml entry is supported (as per sword2.0 " @@ -570,7 +561,7 @@ data[content_type] = fh if len(content_types_present) != 2: - return make_error_dict( + raise DepositError( ERROR_CONTENT, "You must provide both 1 application/zip (or " "application/x-tar) and 1 atom+xml entry for multipart " @@ -594,7 +585,7 @@ try: raw_metadata, metadata = self._read_metadata(data["application/atom+xml"]) except ParserError: - return make_error_dict( + raise DepositError( PARSING_ERROR, "Malformed xml metadata", "The xml received is malformed. " @@ -765,7 +756,7 @@ try: raw_metadata, metadata = self._read_metadata(request.data) except ParserError: - return make_error_dict( + raise DepositError( BAD_REQUEST, "Malformed xml metadata", "The xml received is malformed. " @@ -773,7 +764,7 @@ ) if not metadata: - return make_error_dict( + raise DepositError( BAD_REQUEST, "Empty body request is not supported", "Atom entry deposit is supposed to send for metadata. " @@ -784,14 +775,16 @@ try: swhid = parse_swh_reference(metadata) except ValidationError as e: - return make_error_dict(PARSING_ERROR, "Invalid SWHID reference", str(e),) + raise DepositError( + PARSING_ERROR, "Invalid SWHID reference", str(e), + ) if swhid is not None: external_id = metadata.get("external_identifier", headers.slug) else: slug = headers.slug if check_slug_is_present and not slug: - return make_missing_slug_error() + raise_missing_slug_error() external_id = metadata.get("external_identifier", slug) @@ -803,12 +796,9 @@ ) if swhid is not None: - try: - swhid, swhid_ref, depo, depo_request = self._store_metadata_deposit( - deposit, swhid, metadata, raw_metadata - ) - except DepositError as deposit_error: - return deposit_error.to_dict() + swhid, swhid_ref, depo, depo_request = self._store_metadata_deposit( + deposit, swhid, metadata, raw_metadata + ) deposit.status = DEPOSIT_STATUS_LOAD_SUCCESS if isinstance(swhid_ref, SWHID): @@ -893,9 +883,7 @@ try: self._collection = DepositCollection.objects.get(name=collection_name) except DepositCollection.DoesNotExist: - return make_error_dict( - NOT_FOUND, f"Unknown collection name {collection_name}" - ) + raise DepositError(NOT_FOUND, f"Unknown collection name {collection_name}") assert self._collection is not None username = request.user.username @@ -905,13 +893,13 @@ username=username ) except DepositClient.DoesNotExist: - return make_error_dict(NOT_FOUND, f"Unknown client name {username}") + raise DepositError(NOT_FOUND, f"Unknown client name {username}") collection_id = self._collection.id collections = self._client.collections assert collections is not None if collection_id not in collections: - return make_error_dict( + raise DepositError( FORBIDDEN, f"Client {username} cannot access collection {collection_name}", ) @@ -922,27 +910,23 @@ try: deposit = Deposit.objects.get(pk=deposit_id) except Deposit.DoesNotExist: - return make_error_dict( + raise DepositError( NOT_FOUND, f"Deposit with id {deposit_id} does not exist" ) assert deposit is not None - checks = self.restrict_access(request, headers, deposit) - if checks: - return checks + self.restrict_access(request, headers, deposit) if headers.on_behalf_of: - return make_error_dict(MEDIATION_NOT_ALLOWED, "Mediation is not supported.") + raise DepositError(MEDIATION_NOT_ALLOWED, "Mediation is not supported.") - checks = self.additional_checks(request, headers, collection_name, deposit_id) - if "error" in checks: - return checks + self.additional_checks(request, headers, collection_name, deposit_id) return {"headers": headers} def restrict_access( self, request: Request, headers: ParsedRequestHeaders, deposit: Deposit - ) -> Dict[str, Any]: + ) -> None: """Allow modifications on deposit with status 'partial' only, reject the rest. """ @@ -951,14 +935,12 @@ DEPOSIT_STATUS_PARTIAL, ) description = f"This deposit has status '{deposit.status}'" - return make_error_dict( + raise DepositError( BAD_REQUEST, summary=summary, verbose_description=description ) - return {} def _basic_not_allowed_method(self, request: Request, method: str): - return make_error_response( - request, + raise DepositError( METHOD_NOT_ALLOWED, f"{method} method is not supported on this endpoint", ) @@ -1000,9 +982,7 @@ 404 if the deposit or the collection does not exist """ - checks = self.checks(request, collection_name, deposit_id) - if "error" in checks: - return make_error_response_from_dict(request, checks["error"]) + self.checks(request, collection_name, deposit_id) r = self.process_get(request, collection_name, deposit_id) @@ -1048,18 +1028,12 @@ """ checks = self.checks(request, collection_name, deposit_id) - if "error" in checks: - return make_error_response_from_dict(request, checks["error"]) headers = checks["headers"] status, iri_key, data = self.process_post( request, headers, collection_name, deposit_id ) - error = data.get("error") - if error: - return make_error_response_from_dict(request, error) - return self._make_deposit_receipt( request, collection_name, status, iri_key, data, ) @@ -1131,15 +1105,8 @@ """ checks = self.checks(request, collection_name, deposit_id) - if "error" in checks: - return make_error_response_from_dict(request, checks["error"]) - headers = checks["headers"] - data = self.process_put(request, headers, collection_name, deposit_id) - - error = data.get("error") - if error: - return make_error_response_from_dict(request, error) + self.process_put(request, headers, collection_name, deposit_id) return HttpResponse(status=status.HTTP_204_NO_CONTENT) @@ -1176,15 +1143,9 @@ 404 if the deposit or the collection does not exist """ - checks = self.checks(request, collection_name, deposit_id) - if "error" in checks: - return make_error_response_from_dict(request, checks["error"]) - + self.checks(request, collection_name, deposit_id) assert deposit_id is not None - data = self.process_delete(request, collection_name, deposit_id) - error = data.get("error") - if error: - return make_error_response_from_dict(request, error) + self.process_delete(request, collection_name, deposit_id) return HttpResponse(status=status.HTTP_204_NO_CONTENT) diff --git a/swh/deposit/api/content.py b/swh/deposit/api/content.py --- a/swh/deposit/api/content.py +++ b/swh/deposit/api/content.py @@ -7,7 +7,7 @@ from django.shortcuts import render from rest_framework import status -from ..errors import NOT_FOUND, make_error_response, make_error_response_from_dict +from ..errors import NOT_FOUND, make_error_response from ..models import DEPOSIT_STATUS_DETAIL, Deposit, DepositRequest from .common import APIBase @@ -22,9 +22,7 @@ """ def get(self, req, collection_name: str, deposit_id: int) -> HttpResponse: - checks = self.checks(req, collection_name, deposit_id) - if "error" in checks: - return make_error_response_from_dict(req, checks["error"]) + self.checks(req, collection_name, deposit_id) try: deposit = Deposit.objects.get(pk=deposit_id) diff --git a/swh/deposit/api/edit.py b/swh/deposit/api/edit.py --- a/swh/deposit/api/edit.py +++ b/swh/deposit/api/edit.py @@ -11,7 +11,7 @@ from swh.model.identifiers import parse_swhid from ..config import DEPOSIT_STATUS_LOAD_SUCCESS -from ..errors import BAD_REQUEST, DepositError, ParserError, make_error_dict +from ..errors import BAD_REQUEST, DepositError, ParserError from ..parsers import SWHAtomEntryParser, SWHMultiPartParser from .common import APIDelete, APIPut, ParsedRequestHeaders @@ -29,7 +29,7 @@ def restrict_access( self, request: Request, headers: ParsedRequestHeaders, deposit: Deposit - ) -> Dict[str, Any]: + ) -> None: """Relax restriction access to allow metadata update on deposit with status "done" when a swhid is provided. @@ -40,9 +40,9 @@ and deposit.status == DEPOSIT_STATUS_LOAD_SUCCESS ): # Allow metadata update on deposit with status "done" when swhid provided - return {} + return # otherwise, let the standard access restriction check occur - return super().restrict_access(request, headers, deposit) + super().restrict_access(request, headers, deposit) def process_put( self, @@ -104,7 +104,7 @@ assert deposit.status == DEPOSIT_STATUS_LOAD_SUCCESS if swhid != deposit.swhid: - return make_error_dict( + raise DepositError( BAD_REQUEST, f"Mismatched provided SWHID {swhid} with deposit's {deposit.swhid}.", "The provided SWHID does not match the deposit to update. " @@ -114,7 +114,7 @@ try: raw_metadata, metadata = self._read_metadata(request.data) except ParserError: - return make_error_dict( + raise DepositError( BAD_REQUEST, "Malformed xml metadata", "The xml received is malformed. " @@ -122,19 +122,16 @@ ) if not metadata: - return make_error_dict( + raise DepositError( BAD_REQUEST, "Empty body request is not supported", "Atom entry deposit is supposed to send for metadata. " "If the body is empty, there is no metadata.", ) - try: - _, _, deposit, deposit_request = self._store_metadata_deposit( - deposit, parse_swhid(swhid), metadata, raw_metadata, deposit.origin_url, - ) - except DepositError as deposit_error: - return deposit_error.to_dict() + _, _, deposit, deposit_request = self._store_metadata_deposit( + deposit, parse_swhid(swhid), metadata, raw_metadata, deposit.origin_url, + ) return { "deposit_id": deposit.id, diff --git a/swh/deposit/api/edit_media.py b/swh/deposit/api/edit_media.py --- a/swh/deposit/api/edit_media.py +++ b/swh/deposit/api/edit_media.py @@ -8,7 +8,7 @@ from rest_framework import status from ..config import CONT_FILE_IRI -from ..errors import BAD_REQUEST, make_error_dict +from ..errors import BAD_REQUEST, DepositError from ..parsers import SWHFileUploadTarParser, SWHFileUploadZipParser from .common import ( ACCEPT_ARCHIVE_CONTENT_TYPES, @@ -48,7 +48,7 @@ msg = "Packaging format supported is restricted to %s" % ( ", ".join(ACCEPT_ARCHIVE_CONTENT_TYPES) ) - return make_error_dict(BAD_REQUEST, msg) + raise DepositError(BAD_REQUEST, msg) return self._binary_upload( req, headers, collection_name, deposit_id=deposit_id, replace_archives=True @@ -76,8 +76,7 @@ msg = "Packaging format supported is restricted to %s" % ( ", ".join(ACCEPT_ARCHIVE_CONTENT_TYPES) ) - unused = 0 - return unused, "unused", make_error_dict(BAD_REQUEST, msg) + raise DepositError(BAD_REQUEST, msg) return ( status.HTTP_201_CREATED, diff --git a/swh/deposit/api/private/__init__.py b/swh/deposit/api/private/__init__.py --- a/swh/deposit/api/private/__init__.py +++ b/swh/deposit/api/private/__init__.py @@ -9,7 +9,7 @@ from swh.deposit import utils from swh.deposit.api.common import AuthenticatedAPIView -from swh.deposit.errors import NOT_FOUND, make_error_dict +from swh.deposit.errors import NOT_FOUND, DepositError from ...config import METADATA_TYPE, APIConfig from ...models import Deposit, DepositRequest @@ -81,14 +81,12 @@ try: Deposit.objects.get(pk=deposit_id) except Deposit.DoesNotExist: - return make_error_dict( + raise DepositError( NOT_FOUND, "Deposit with id %s does not exist" % deposit_id ) headers = self._read_headers(req) - checks = self.additional_checks(req, headers, collection_name, deposit_id) - if "error" in checks: - return checks + self.additional_checks(req, headers, collection_name, deposit_id) return {"headers": headers} diff --git a/swh/deposit/api/private/deposit_update_status.py b/swh/deposit/api/private/deposit_update_status.py --- a/swh/deposit/api/private/deposit_update_status.py +++ b/swh/deposit/api/private/deposit_update_status.py @@ -10,7 +10,7 @@ from swh.model.identifiers import DIRECTORY, REVISION, SNAPSHOT, swhid from . import APIPrivateView -from ...errors import BAD_REQUEST, make_error_dict +from ...errors import BAD_REQUEST, DepositError from ...models import DEPOSIT_STATUS_DETAIL, DEPOSIT_STATUS_LOAD_SUCCESS, Deposit from ..common import APIPut, ParsedRequestHeaders @@ -43,11 +43,11 @@ msg = "The status key is mandatory with possible values %s" % list( DEPOSIT_STATUS_DETAIL.keys() ) - return make_error_dict(BAD_REQUEST, msg) + raise DepositError(BAD_REQUEST, msg) if status not in DEPOSIT_STATUS_DETAIL: msg = "Possible status in %s" % list(DEPOSIT_STATUS_DETAIL.keys()) - return make_error_dict(BAD_REQUEST, msg) + raise DepositError(BAD_REQUEST, msg) if status == DEPOSIT_STATUS_LOAD_SUCCESS: missing_keys = [] @@ -61,7 +61,7 @@ f"Updating deposit status to {status}" f" requires information {','.join(missing_keys)}" ) - return make_error_dict(BAD_REQUEST, msg) + raise DepositError(BAD_REQUEST, msg) return {} diff --git a/swh/deposit/api/state.py b/swh/deposit/api/state.py --- a/swh/deposit/api/state.py +++ b/swh/deposit/api/state.py @@ -7,7 +7,7 @@ from django.shortcuts import render from rest_framework import status -from ..errors import NOT_FOUND, make_error_response, make_error_response_from_dict +from ..errors import NOT_FOUND, make_error_response from ..models import DEPOSIT_STATUS_DETAIL, Deposit from .common import APIBase from .converters import convert_status_detail @@ -23,9 +23,7 @@ """ def get(self, req, collection_name: str, deposit_id: int) -> HttpResponse: - checks = self.checks(req, collection_name, deposit_id) - if "error" in checks: - return make_error_response_from_dict(req, checks["error"]) + self.checks(req, collection_name, deposit_id) try: deposit = Deposit.objects.get(pk=deposit_id) diff --git a/swh/deposit/errors.py b/swh/deposit/errors.py --- a/swh/deposit/errors.py +++ b/swh/deposit/errors.py @@ -7,7 +7,7 @@ """ -from typing import Any, Dict +from typing import NoReturn from django.shortcuts import render from rest_framework import status @@ -152,11 +152,11 @@ return make_error_response_from_dict(req, error["error"]) -def make_missing_slug_error() -> Dict[str, Any]: +def raise_missing_slug_error() -> NoReturn: """Returns a missing slug header error dict """ - return make_error_dict( + raise DepositError( BAD_REQUEST, "Missing SLUG header", verbose_description=( @@ -171,10 +171,31 @@ """ - def __init__(self, key, summary, verbose_description): + def __init__(self, key, summary, verbose_description=None): self.key = key self.summary = summary self.verbose_description = verbose_description def to_dict(self): return make_error_dict(self.key, self.summary, self.verbose_description) + + +class DepositErrorMiddleware: + """A Django middleware that catches DepositError and returns a proper + error response.""" + + # __init__ and __call__ are boilerplate to make a pass-through Django + # middleware + + def __init__(self, get_response): + self.get_response = get_response + + def __call__(self, request): + response = self.get_response(request) + return response + + def process_exception(self, request, exception): + if isinstance(exception, DepositError): + return make_error_response_from_dict(request, exception.to_dict()["error"]) + else: + return None diff --git a/swh/deposit/settings/common.py b/swh/deposit/settings/common.py --- a/swh/deposit/settings/common.py +++ b/swh/deposit/settings/common.py @@ -48,6 +48,7 @@ "django.contrib.messages.middleware.MessageMiddleware", "django.middleware.clickjacking.XFrameOptionsMiddleware", "swh.deposit.auth.WrapBasicAuthenticationResponseMiddleware", + "swh.deposit.errors.DepositErrorMiddleware", ] ROOT_URLCONF = "swh.deposit.urls"