diff --git a/swh/deposit/api/collection.py b/swh/deposit/api/collection.py --- a/swh/deposit/api/collection.py +++ b/swh/deposit/api/collection.py @@ -7,14 +7,21 @@ from rest_framework import status -from ..config import EDIT_IRI +from ..config import DEPOSIT_STATUS_LOAD_SUCCESS, EDIT_IRI +from ..models import Deposit from ..parsers import ( SWHAtomEntryParser, SWHFileUploadTarParser, SWHFileUploadZipParser, SWHMultiPartParser, ) -from .common import ACCEPT_ARCHIVE_CONTENT_TYPES, APIPost, ParsedRequestHeaders, Receipt +from .common import ( + ACCEPT_ARCHIVE_CONTENT_TYPES, + APIPost, + ParsedRequestHeaders, + Receipt, + get_collection_by_name, +) class CollectionAPI(APIPost): @@ -38,7 +45,7 @@ req, headers: ParsedRequestHeaders, collection_name: str, - deposit_id: Optional[int] = None, + deposit: Optional[Deposit] = None, ) -> Tuple[int, str, Receipt]: """Create a first deposit as: - archive deposit (1 zip) @@ -83,18 +90,51 @@ provided """ - assert deposit_id is None + assert deposit is None + + deposit = self._deposit_create(req, collection_name, external_id=headers.slug) + if req.content_type in ACCEPT_ARCHIVE_CONTENT_TYPES: receipt = self._binary_upload( - req, headers, collection_name, check_slug_is_present=True + req, headers, collection_name, deposit, check_slug_is_present=True ) elif req.content_type.startswith("multipart/"): receipt = self._multipart_upload( - req, headers, collection_name, check_slug_is_present=True + req, headers, collection_name, deposit, check_slug_is_present=True ) else: receipt = self._atom_entry( - req, headers, collection_name, check_slug_is_present=True + req, headers, collection_name, deposit, check_slug_is_present=True ) return status.HTTP_201_CREATED, EDIT_IRI, receipt + + def _deposit_create( + self, request, collection_name: str, external_id: Optional[str] + ) -> Deposit: + collection = get_collection_by_name(collection_name) + client = self.get_client(request) + deposit_parent: Optional[Deposit] = None + + if external_id: + try: + # find a deposit parent (same external id, status load to success) + deposit_parent = ( + Deposit.objects.filter( + client=client, + external_id=external_id, + status=DEPOSIT_STATUS_LOAD_SUCCESS, + ) + .order_by("-id")[0:1] + .get() + ) + except Deposit.DoesNotExist: + # then no parent for that deposit, deposit_parent already None + pass + + return Deposit( + collection=collection, + external_id=external_id or "", + client=client, + parent=deposit_parent, + ) 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 @@ -115,6 +115,8 @@ raise DepositError(NOT_FOUND, f"Deposit {deposit_id} does not exist") if collection_name and deposit.collection.name != collection_name: + get_collection_by_name(collection_name) # raises if does not exist + raise DepositError( NOT_FOUND, f"Deposit {deposit_id} does not belong to collection {collection_name}", @@ -123,6 +125,18 @@ return deposit +def get_collection_by_name(collection_name: str): + """Gets an existing Deposit object if it exists, or raises `DepositError`.""" + try: + collection = DepositCollection.objects.get(name=collection_name) + except DepositCollection.DoesNotExist: + raise DepositError(NOT_FOUND, f"Unknown collection name {collection_name}") + + assert collection is not None + + return collection + + class AuthenticatedAPIView(APIView): """Mixin intended as a based API view to enforce the basic authentication check @@ -138,6 +152,8 @@ """ + _client: Optional[DepositClient] = None + def _read_headers(self, request: Request) -> ParsedRequestHeaders: """Read and unify the necessary headers from the request (those are not stored in the same location or not properly formatted). @@ -188,7 +204,7 @@ def _deposit_put( self, request: Request, - deposit_id: Optional[int] = None, + deposit: Deposit, in_progress: bool = False, external_id: Optional[str] = None, ) -> Deposit: @@ -196,7 +212,7 @@ Args: request: request data - deposit_id: deposit identifier + deposit: deposit being updated/created in_progress: deposit status external_id: external identifier to associate to the deposit @@ -204,48 +220,22 @@ The Deposit instance saved or updated. """ - complete_date: Optional[datetime.datetime] = None - deposit_parent: Optional[Deposit] = None - if in_progress is False: - complete_date = timezone.now() - status_type = DEPOSIT_STATUS_DEPOSITED + self._complete_deposit(deposit) else: - status_type = DEPOSIT_STATUS_PARTIAL + deposit.status = DEPOSIT_STATUS_PARTIAL + deposit.save() - if not deposit_id: - try: - # find a deposit parent (same external id, status load to success) - deposit_parent = ( - Deposit.objects.filter( - client=self._client, - external_id=external_id, - status=DEPOSIT_STATUS_LOAD_SUCCESS, - ) - .order_by("-id")[0:1] - .get() - ) - except Deposit.DoesNotExist: - # then no parent for that deposit, deposit_parent already None - pass - - deposit = Deposit( - collection=self._collection, - external_id=external_id or "", - complete_date=complete_date, - status=status_type, - client=self._client, - parent=deposit_parent, - ) - else: - deposit = get_deposit_by_id(deposit_id) + return deposit - # update metadata - deposit.complete_date = complete_date - deposit.status = status_type + def _complete_deposit(self, deposit: Deposit) -> None: + """Marks the deposit as 'deposited', then schedule a check task if configured + to do so.""" + deposit.complete_date = timezone.now() + deposit.status = DEPOSIT_STATUS_DEPOSITED + deposit.save() if self.config["checks"]: - deposit.save() # needed to have a deposit id scheduler = self.scheduler if deposit.status == DEPOSIT_STATUS_DEPOSITED and not deposit.check_task_id: task = create_oneshot_task_dict( @@ -257,9 +247,7 @@ check_task_id = scheduler.create_tasks([task])[0]["id"] deposit.check_task_id = check_task_id - deposit.save() - - return deposit + deposit.save() def _deposit_request_put( self, @@ -312,33 +300,30 @@ assert deposit_request is not None return deposit_request - def _delete_archives(self, collection_name: str, deposit_id: int) -> Dict: + def _delete_archives(self, collection_name: str, deposit: Deposit) -> Dict: """Delete archive references from the deposit id. """ - deposit = get_deposit_by_id(deposit_id) DepositRequest.objects.filter(deposit=deposit, type=ARCHIVE_TYPE).delete() return {} - def _delete_deposit(self, collection_name: str, deposit_id: int) -> Dict: + def _delete_deposit(self, collection_name: str, deposit: Deposit) -> Dict: """Delete deposit reference. Args: collection_name: Client's collection - deposit_id: The deposit to delete + deposit: The deposit to delete Returns Empty dict when ok. Dict with error key to describe the failure. """ - deposit = get_deposit_by_id(deposit_id) - if deposit.collection.name != collection_name: summary = "Cannot delete a deposit from another collection" description = "Deposit %s does not belong to the collection %s" % ( - deposit_id, + deposit.id, collection_name, ) raise DepositError( @@ -404,7 +389,7 @@ request: Request, headers: ParsedRequestHeaders, collection_name: str, - deposit_id: Optional[int] = None, + deposit: Deposit, replace_metadata: bool = False, replace_archives: bool = False, check_slug_is_present: bool = False, @@ -418,7 +403,7 @@ and inject in db headers: parsed request headers collection_name: the associated client - deposit_id: deposit identifier if provided + deposit: deposit to be updated replace_metadata: 'Update or add' request to existing deposit. If False (default), this adds new metadata request to existing ones. Otherwise, this will replace existing metadata. @@ -476,10 +461,7 @@ # actual storage of data archive_metadata = filehandler deposit = self._deposit_put( - request, - deposit_id=deposit_id, - in_progress=headers.in_progress, - external_id=slug, + request, deposit=deposit, in_progress=headers.in_progress, external_id=slug, ) self._deposit_request_put( deposit, @@ -509,7 +491,7 @@ request: Request, headers: ParsedRequestHeaders, collection_name: str, - deposit_id: Optional[int] = None, + deposit: Deposit, replace_metadata: bool = False, replace_archives: bool = False, check_slug_is_present: bool = False, @@ -525,7 +507,7 @@ and inject in db headers: parsed request headers collection_name: the associated client - deposit_id: deposit identifier if provided + deposit: deposit to be updated replace_metadata: 'Update or add' request to existing deposit. If False (default), this adds new metadata request to existing ones. Otherwise, this will replace existing metadata. @@ -607,10 +589,7 @@ # actual storage of data deposit = self._deposit_put( - request, - deposit_id=deposit_id, - in_progress=headers.in_progress, - external_id=slug, + request, deposit=deposit, in_progress=headers.in_progress, external_id=slug, ) deposit_request_data = { ARCHIVE_KEY: filehandler, @@ -727,7 +706,7 @@ request: Request, headers: ParsedRequestHeaders, collection_name: str, - deposit_id: Optional[int] = None, + deposit: Deposit, replace_metadata: bool = False, replace_archives: bool = False, check_slug_is_present: bool = False, @@ -739,7 +718,7 @@ and inject in db headers: parsed request headers collection_name: the associated client - deposit_id: deposit identifier if provided + deposit: deposit to be updated replace_metadata: 'Update or add' request to existing deposit. If False (default), this adds new metadata request to existing ones. Otherwise, this will replace existing metadata. @@ -800,7 +779,7 @@ deposit = self._deposit_put( request, - deposit_id=deposit_id, + deposit=deposit, in_progress=headers.in_progress, external_id=headers.slug, ) @@ -844,24 +823,23 @@ request: Request, headers: ParsedRequestHeaders, collection_name: str, - deposit_id: int, + deposit: Deposit, ) -> Receipt: - """Empty post to finalize an empty deposit. + """Empty post to finalize a deposit. Args: request: the request holding information to parse and inject in db headers: parsed request headers collection_name: the associated client - deposit_id: deposit identifier + deposit: deposit to be finalized """ - deposit = get_deposit_by_id(deposit_id) - deposit.complete_date = timezone.now() - deposit.status = DEPOSIT_STATUS_DEPOSITED - deposit.save() + self._complete_deposit(deposit) + + assert deposit.complete_date is not None return Receipt( - deposit_id=deposit_id, + deposit_id=deposit.id, deposit_date=deposit.complete_date, status=deposit.status, archive=None, @@ -872,7 +850,7 @@ request: Request, headers: ParsedRequestHeaders, collection_name: str, - deposit_id: Optional[int] = None, + deposit: Optional[Deposit], ) -> Dict[str, Any]: """Permit the child class to enrich additional checks. @@ -882,45 +860,52 @@ """ return {} - def checks( - self, request: Request, collection_name: str, deposit_id: Optional[int] = None - ) -> ParsedRequestHeaders: - try: - self._collection = DepositCollection.objects.get(name=collection_name) - except DepositCollection.DoesNotExist: - raise DepositError(NOT_FOUND, f"Unknown collection name {collection_name}") - assert self._collection is not None - + def get_client(self, request) -> DepositClient: + # This class depends on AuthenticatedAPIView, so request.user.username + # is always set username = request.user.username - if username: # unauthenticated request can have the username empty + assert username is not None + + if self._client is None: try: - self._client: DepositClient = DepositClient.objects.get( # type: ignore + self._client = DepositClient.objects.get( # type: ignore username=username ) except DepositClient.DoesNotExist: 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: - raise DepositError( - FORBIDDEN, - f"Client {username} cannot access collection {collection_name}", - ) + assert self._client.username == username - headers = self._read_headers(request) + return self._client + + def checks( + self, request: Request, collection_name: str, deposit: Optional[Deposit] = None + ) -> ParsedRequestHeaders: + if deposit is None: + collection = get_collection_by_name(collection_name) + else: + assert collection_name == deposit.collection.name + collection = deposit.collection + + client = self.get_client(request) + collection_id = collection.id + collections = client.collections + assert collections is not None + if collection_id not in collections: + raise DepositError( + FORBIDDEN, + f"Client {client.username} cannot access collection {collection_name}", + ) - if deposit_id: - deposit = get_deposit_by_id(deposit_id) + headers = self._read_headers(request) - assert deposit is not None + if deposit is not None: self.restrict_access(request, headers, deposit) if headers.on_behalf_of: raise DepositError(MEDIATION_NOT_ALLOWED, "Mediation is not supported.") - self.additional_checks(request, headers, collection_name, deposit_id) + self.additional_checks(request, headers, collection_name, deposit) return headers @@ -981,9 +966,10 @@ 404 if the deposit or the collection does not exist """ - self.checks(request, collection_name, deposit_id) + deposit = get_deposit_by_id(deposit_id, collection_name) + self.checks(request, collection_name, deposit) - r = self.process_get(request, collection_name, deposit_id) + r = self.process_get(request, collection_name, deposit) status, content, content_type = r if content_type == "swh/generator": @@ -999,7 +985,7 @@ @abstractmethod def process_get( - self, request: Request, collection_name: str, deposit_id: int + self, request: Request, collection_name: str, deposit: Deposit ) -> Tuple[int, Any, str]: """Routine to deal with the deposit's get processing. @@ -1026,10 +1012,14 @@ 404 if the deposit or the collection does not exist """ - headers = self.checks(request, collection_name, deposit_id) + if deposit_id is None: + deposit = None + else: + deposit = get_deposit_by_id(deposit_id, collection_name) + headers = self.checks(request, collection_name, deposit) status, iri_key, receipt = self.process_post( - request, headers, collection_name, deposit_id + request, headers, collection_name, deposit ) return self._make_deposit_receipt( @@ -1075,7 +1065,7 @@ request, headers: ParsedRequestHeaders, collection_name: str, - deposit_id: Optional[int] = None, + deposit: Optional[Deposit] = None, ) -> Tuple[int, str, Receipt]: """Routine to deal with the deposit's processing. @@ -1105,8 +1095,12 @@ 404 if the deposit or the collection does not exist """ - headers = self.checks(request, collection_name, deposit_id) - self.process_put(request, headers, collection_name, deposit_id) + if deposit_id is None: + deposit = None + else: + deposit = get_deposit_by_id(deposit_id, collection_name) + headers = self.checks(request, collection_name, deposit) + self.process_put(request, headers, collection_name, deposit) return HttpResponse(status=status.HTTP_204_NO_CONTENT) @@ -1116,7 +1110,7 @@ request: Request, headers: ParsedRequestHeaders, collection_name: str, - deposit_id: int, + deposit: Deposit, ) -> None: """Routine to deal with updating a deposit in some way. @@ -1143,15 +1137,16 @@ 404 if the deposit or the collection does not exist """ - self.checks(request, collection_name, deposit_id) assert deposit_id is not None - self.process_delete(request, collection_name, deposit_id) + deposit = get_deposit_by_id(deposit_id, collection_name) + self.checks(request, collection_name, deposit) + self.process_delete(request, collection_name, deposit) return HttpResponse(status=status.HTTP_204_NO_CONTENT) @abstractmethod def process_delete( - self, request: Request, collection_name: str, deposit_id: int + self, request: Request, collection_name: str, deposit: Deposit ) -> None: """Routine to delete a resource. 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 @@ -21,10 +21,10 @@ """ def get(self, req, collection_name: str, deposit_id: int) -> HttpResponse: - self.checks(req, collection_name, deposit_id) - deposit = get_deposit_by_id(deposit_id, collection_name) + self.checks(req, collection_name, deposit) + requests = DepositRequest.objects.filter(deposit=deposit) context = { "deposit_id": 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 ..config import DEPOSIT_STATUS_LOAD_SUCCESS from ..errors import BAD_REQUEST, DepositError, ParserError from ..parsers import SWHAtomEntryParser, SWHMultiPartParser -from .common import APIDelete, APIPut, ParsedRequestHeaders, get_deposit_by_id +from .common import APIDelete, APIPut, ParsedRequestHeaders class EditAPI(APIPut, APIDelete): @@ -47,7 +47,7 @@ request, headers: ParsedRequestHeaders, collection_name: str, - deposit_id: int, + deposit: Deposit, ) -> None: """This allows the following scenarios: @@ -77,7 +77,7 @@ request, headers, collection_name, - deposit_id=deposit_id, + deposit=deposit, replace_archives=True, replace_metadata=True, ) @@ -88,7 +88,7 @@ request, headers, collection_name, - deposit_id=deposit_id, + deposit=deposit, replace_metadata=True, ) return @@ -97,7 +97,6 @@ # Write to the metadata storage (and the deposit backend) # no ingestion triggered - deposit = get_deposit_by_id(deposit_id, collection_name) assert deposit.status == DEPOSIT_STATUS_LOAD_SUCCESS if swhid != deposit.swhid: @@ -130,10 +129,10 @@ deposit, parse_swhid(swhid), metadata, raw_metadata, deposit.origin_url, ) - def process_delete(self, req, collection_name: str, deposit_id: int) -> None: + def process_delete(self, req, collection_name: str, deposit: Deposit) -> None: """Delete the container (deposit). source: http://swordapp.github.io/SWORDv2-Profile/SWORDProfile.html#protocoloperations_deleteconteiner # noqa """ - self._delete_deposit(collection_name, deposit_id) + self._delete_deposit(collection_name, deposit) 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 @@ -9,6 +9,7 @@ from ..config import CONT_FILE_IRI from ..errors import BAD_REQUEST, DepositError +from ..models import Deposit from ..parsers import SWHFileUploadTarParser, SWHFileUploadZipParser from .common import ( ACCEPT_ARCHIVE_CONTENT_TYPES, @@ -35,7 +36,7 @@ ) def process_put( - self, req, headers: ParsedRequestHeaders, collection_name: str, deposit_id: int + self, req, headers: ParsedRequestHeaders, collection_name: str, deposit: Deposit ) -> None: """Replace existing content for the existing deposit. @@ -52,7 +53,7 @@ raise DepositError(BAD_REQUEST, msg) self._binary_upload( - req, headers, collection_name, deposit_id=deposit_id, replace_archives=True + req, headers, collection_name, deposit=deposit, replace_archives=True ) def process_post( @@ -60,7 +61,7 @@ req, headers: ParsedRequestHeaders, collection_name: str, - deposit_id: Optional[int] = None, + deposit: Optional[Deposit] = None, ) -> Tuple[int, str, Receipt]: """Add new content to the existing deposit. @@ -73,6 +74,8 @@ Body: [optional Deposit Receipt] """ + assert deposit is not None + if req.content_type not in ACCEPT_ARCHIVE_CONTENT_TYPES: msg = "Packaging format supported is restricted to %s" % ( ", ".join(ACCEPT_ARCHIVE_CONTENT_TYPES) @@ -82,10 +85,10 @@ return ( status.HTTP_201_CREATED, CONT_FILE_IRI, - self._binary_upload(req, headers, collection_name, deposit_id), + self._binary_upload(req, headers, collection_name, deposit), ) - def process_delete(self, req, collection_name: str, deposit_id: int) -> None: + def process_delete(self, req, collection_name: str, deposit: Deposit) -> None: """Delete content (archives) from existing deposit. source: http://swordapp.github.io/SWORDv2-Profile/SWORDProfile.html#protocoloperations_deletingcontent # noqa @@ -94,4 +97,4 @@ 204 Created """ - self._delete_archives(collection_name, deposit_id) + self._delete_archives(collection_name, deposit) 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 @@ -12,7 +12,6 @@ from ...config import METADATA_TYPE, APIConfig from ...models import Deposit, DepositRequest -from ..common import get_deposit_by_id class DepositReadMixin: @@ -20,20 +19,17 @@ """ - def _deposit_requests(self, deposit, request_type): + def _deposit_requests(self, deposit: Deposit, request_type: str): """Given a deposit, yields its associated deposit_request Args: - deposit (Deposit): Deposit to list requests for - request_type (str): 'archive' or 'metadata' + deposit: Deposit to list requests for + request_type: 'archive' or 'metadata' Yields: deposit requests of type request_type associated to the deposit """ - if isinstance(deposit, int): - deposit = get_deposit_by_id(deposit) - deposit_requests = DepositRequest.objects.filter( type=request_type, deposit=deposit ).order_by("id") @@ -73,15 +69,12 @@ authentication_classes = () permission_classes = (AllowAny,) - def checks(self, req, collection_name, deposit_id=None): + def checks(self, req, collection_name, deposit=None): """Override default checks implementation to allow empty collection. """ - if deposit_id: - get_deposit_by_id(deposit_id) - headers = self._read_headers(req) - self.additional_checks(req, headers, collection_name, deposit_id) + self.additional_checks(req, headers, collection_name, deposit) return {"headers": headers} diff --git a/swh/deposit/api/private/deposit_check.py b/swh/deposit/api/private/deposit_check.py --- a/swh/deposit/api/private/deposit_check.py +++ b/swh/deposit/api/private/deposit_check.py @@ -19,7 +19,7 @@ from ...config import ARCHIVE_TYPE, DEPOSIT_STATUS_REJECTED, DEPOSIT_STATUS_VERIFIED from ...models import Deposit, DepositRequest from ..checks import check_metadata -from ..common import APIGet, get_deposit_by_id +from ..common import APIGet MANDATORY_ARCHIVE_UNREADABLE = ( "At least one of its associated archives is not readable" # noqa @@ -131,7 +131,7 @@ return True, None def process_get( - self, req: Request, collection_name: str, deposit_id: int + self, req: Request, collection_name: str, deposit: Deposit ) -> Tuple[int, Dict, str]: """Build a unique tarball from the multiple received and stream that content to the client. @@ -139,13 +139,12 @@ Args: req: Client request collection_name: Collection owning the deposit - deposit_id: Deposit concerned by the reading + deposit: Deposit concerned by the reading Returns: Tuple status, stream of content, content-type """ - deposit = get_deposit_by_id(deposit_id) metadata, _ = self._metadata_get(deposit) problems: Dict = {} # will check each deposit's associated request (both of type diff --git a/swh/deposit/api/private/deposit_read.py b/swh/deposit/api/private/deposit_read.py --- a/swh/deposit/api/private/deposit_read.py +++ b/swh/deposit/api/private/deposit_read.py @@ -19,7 +19,7 @@ from . import APIPrivateView, DepositReadMixin from ...config import ARCHIVE_TYPE, SWH_PERSON from ...models import Deposit -from ..common import APIGet, get_deposit_by_id +from ..common import APIGet @contextmanager @@ -74,7 +74,7 @@ os.makedirs(self.extraction_dir) def process_get( - self, request, collection_name: str, deposit_id: int + self, request, collection_name: str, deposit: Deposit ) -> Tuple[int, Any, str]: """Build a unique tarball from the multiple received and stream that content to the client. @@ -82,7 +82,7 @@ Args: request (Request): collection_name: Collection owning the deposit - deposit_id: Deposit concerned by the reading + deposit: Deposit concerned by the reading Returns: Tuple status, stream of content, content-type @@ -90,7 +90,7 @@ """ archive_paths = [ r.archive.path - for r in self._deposit_requests(deposit_id, request_type=ARCHIVE_TYPE) + for r in self._deposit_requests(deposit, request_type=ARCHIVE_TYPE) ] return ( status.HTTP_200_OK, @@ -193,8 +193,7 @@ } def process_get( - self, request, collection_name: str, deposit_id: int + self, request, collection_name: str, deposit: Deposit ) -> Tuple[int, Dict, str]: - deposit = get_deposit_by_id(deposit_id) data = self.metadata_read(deposit) return status.HTTP_200_OK, data if data else {}, "application/json" 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 @@ -9,8 +9,8 @@ from . import APIPrivateView from ...errors import BAD_REQUEST, DepositError -from ...models import DEPOSIT_STATUS_DETAIL, DEPOSIT_STATUS_LOAD_SUCCESS -from ..common import APIPut, ParsedRequestHeaders, get_deposit_by_id +from ...models import DEPOSIT_STATUS_DETAIL, DEPOSIT_STATUS_LOAD_SUCCESS, Deposit +from ..common import APIPut, ParsedRequestHeaders MANDATORY_KEYS = ["origin_url", "revision_id", "directory_id", "snapshot_id"] @@ -25,7 +25,7 @@ parser_classes = (JSONParser,) def additional_checks( - self, request, headers: ParsedRequestHeaders, collection_name, deposit_id=None + self, request, headers: ParsedRequestHeaders, collection_name, deposit=None ): """Enrich existing checks to the default ones. @@ -68,7 +68,7 @@ request, headers: ParsedRequestHeaders, collection_name: str, - deposit_id: int, + deposit: Deposit, ) -> None: """Update the deposit with status and SWHIDs @@ -79,8 +79,6 @@ """ data = request.data - deposit = get_deposit_by_id(deposit_id) - status = data["status"] deposit.status = status if status == DEPOSIT_STATUS_LOAD_SUCCESS: 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 @@ -22,10 +22,10 @@ """ def get(self, req, collection_name: str, deposit_id: int) -> HttpResponse: - self.checks(req, collection_name, deposit_id) - deposit = get_deposit_by_id(deposit_id, collection_name) + self.checks(req, collection_name, deposit) + status_detail = convert_status_detail(deposit.status_detail) if not status_detail: status_detail = DEPOSIT_STATUS_DETAIL[deposit.status] diff --git a/swh/deposit/api/sword_edit.py b/swh/deposit/api/sword_edit.py --- a/swh/deposit/api/sword_edit.py +++ b/swh/deposit/api/sword_edit.py @@ -11,6 +11,7 @@ from swh.storage.interface import StorageInterface from ..config import EDIT_IRI, EM_IRI +from ..models import Deposit from ..parsers import SWHAtomEntryParser, SWHMultiPartParser from .common import APIPost, ParsedRequestHeaders, Receipt @@ -37,7 +38,7 @@ request, headers: ParsedRequestHeaders, collection_name: str, - deposit_id: Optional[int] = None, + deposit: Optional[Deposit] = None, ) -> Tuple[int, str, Receipt]: """Add new metadata/archive to existing deposit. @@ -63,20 +64,18 @@ For the empty post case, this returns a 200. """ # noqa - assert deposit_id is not None + assert deposit is not None if request.content_type.startswith("multipart/"): receipt = self._multipart_upload( - request, headers, collection_name, deposit_id=deposit_id + request, headers, collection_name, deposit=deposit ) return (status.HTTP_201_CREATED, EM_IRI, receipt) content_length = headers.content_length or 0 if content_length == 0 and headers.in_progress is False: # check for final empty post - receipt = self._empty_post(request, headers, collection_name, deposit_id) + receipt = self._empty_post(request, headers, collection_name, deposit) return (status.HTTP_200_OK, EDIT_IRI, receipt) - receipt = self._atom_entry( - request, headers, collection_name, deposit_id=deposit_id - ) + receipt = self._atom_entry(request, headers, collection_name, deposit=deposit) return (status.HTTP_201_CREATED, EM_IRI, receipt) diff --git a/swh/deposit/tests/api/test_deposit_schedule.py b/swh/deposit/tests/api/test_deposit_schedule.py --- a/swh/deposit/tests/api/test_deposit_schedule.py +++ b/swh/deposit/tests/api/test_deposit_schedule.py @@ -11,7 +11,12 @@ import pytest from rest_framework import status -from swh.deposit.config import COL_IRI, DEPOSIT_STATUS_DEPOSITED +from swh.deposit.config import ( + COL_IRI, + DEPOSIT_STATUS_DEPOSITED, + DEPOSIT_STATUS_PARTIAL, + SE_IRI, +) from swh.deposit.parsers import parse_xml @@ -28,12 +33,32 @@ return datetime.datetime.now(tz=datetime.timezone.utc) +def assert_task_for_deposit( + swh_scheduler, deposit_id, timestamp_before_call, timestamp_after_call +): + tasks = swh_scheduler.grab_ready_tasks("check-deposit") + assert len(tasks) == 1 + task = tasks[0] + + assert timestamp_before_call <= task.pop("next_run") <= timestamp_after_call + assert task["arguments"] == { + "args": [], + "kwargs": {"collection": "test", "deposit_id": deposit_id,}, + } + assert task["policy"] == "oneshot" + assert task["type"] == "check-deposit" + assert task["retries_left"] == 3 + + def test_add_deposit_schedules_check( authenticated_client, deposit_collection, sample_archive, swh_scheduler ): - """Posting deposit on collection creates a checker task + """Posting deposit by POST Col-IRI creates a checker task """ + tasks = swh_scheduler.grab_ready_tasks("check-deposit") + assert len(tasks) == 0 + external_id = "external-id-schedules-check" url = reverse(COL_IRI, args=[deposit_collection.name]) @@ -59,17 +84,50 @@ response_content = parse_xml(BytesIO(response.content)) actual_state = response_content["swh:deposit_status"] assert actual_state == DEPOSIT_STATUS_DEPOSITED - deposit_id = response_content["swh:deposit_id"] + deposit_id = int(response_content["swh:deposit_id"]) + + assert_task_for_deposit( + swh_scheduler, deposit_id, timestamp_before_call, timestamp_after_call + ) + + +def test_update_deposit_schedules_check( + authenticated_client, + deposit_collection, + partial_deposit_with_metadata, + atom_dataset, + swh_scheduler, +): + """Updating deposit by POST SE-IRI creates a checker task + + """ + deposit = partial_deposit_with_metadata + assert deposit.status == DEPOSIT_STATUS_PARTIAL tasks = swh_scheduler.grab_ready_tasks("check-deposit") - assert len(tasks) == 1 - task = tasks[0] + assert len(tasks) == 0 - assert timestamp_before_call <= task.pop("next_run") <= timestamp_after_call - assert task["arguments"] == { - "args": [], - "kwargs": {"collection": "test", "deposit_id": int(deposit_id),}, - } - assert task["policy"] == "oneshot" - assert task["type"] == "check-deposit" - assert task["retries_left"] == 3 + update_uri = reverse(SE_IRI, args=[deposit_collection.name, deposit.id]) + + timestamp_before_call = now() + + response = authenticated_client.post( + update_uri, + content_type="application/atom+xml;type=entry", + data="", + size=0, + HTTP_IN_PROGRESS=False, + ) + + timestamp_after_call = now() + + assert response.status_code == status.HTTP_200_OK + + response_content = parse_xml(BytesIO(response.content)) + actual_state = response_content["swh:deposit_status"] + assert actual_state == DEPOSIT_STATUS_DEPOSITED + assert deposit.id == int(response_content["swh:deposit_id"]) + + assert_task_for_deposit( + swh_scheduler, deposit.id, timestamp_before_call, timestamp_after_call + ) diff --git a/swh/deposit/tests/api/test_deposit_update.py b/swh/deposit/tests/api/test_deposit_update.py --- a/swh/deposit/tests/api/test_deposit_update.py +++ b/swh/deposit/tests/api/test_deposit_update.py @@ -358,7 +358,9 @@ ) assert response.status_code == status.HTTP_404_NOT_FOUND response_content = parse_xml(response.content) - assert "Unknown collection name" in response_content["sword:error"]["atom:summary"] + assert ( + "Deposit 1000 does not exist" in response_content["sword:error"]["atom:summary"] + ) def test_add_metadata_to_unknown_collection( diff --git a/swh/deposit/tests/conftest.py b/swh/deposit/tests/conftest.py --- a/swh/deposit/tests/conftest.py +++ b/swh/deposit/tests/conftest.py @@ -294,6 +294,7 @@ sample_archive, external_id: str, deposit_status=DEPOSIT_STATUS_DEPOSITED, + in_progress=False, ): """Create a skeleton shell deposit @@ -309,7 +310,7 @@ HTTP_SLUG=external_id, HTTP_CONTENT_MD5=sample_archive["md5sum"], HTTP_PACKAGING="http://purl.org/net/sword/package/SimpleZip", - HTTP_IN_PROGRESS="false", + HTTP_IN_PROGRESS=str(in_progress).lower(), HTTP_CONTENT_DISPOSITION="attachment; filename=%s" % (sample_archive["name"]), ) @@ -331,10 +332,9 @@ def create_binary_deposit( authenticated_client, collection_name: str, - sample_archive, - external_id: str, deposit_status: str = DEPOSIT_STATUS_DEPOSITED, atom_dataset: Mapping[str, bytes] = {}, + **kwargs, ): """Create a deposit with both metadata and archive set. Then alters its status to `deposit_status`. @@ -343,9 +343,8 @@ deposit = create_deposit( authenticated_client, collection_name, - sample_archive, - external_id=external_id, deposit_status=DEPOSIT_STATUS_PARTIAL, + **kwargs, ) response = authenticated_client.post( @@ -362,15 +361,12 @@ from swh.deposit.models import Deposit deposit = Deposit._default_manager.get(pk=deposit.id) - if deposit.status != deposit_status: - deposit.status = deposit_status - deposit.save() assert deposit.status == deposit_status return deposit -def deposit_factory(deposit_status=DEPOSIT_STATUS_DEPOSITED): +def deposit_factory(deposit_status=DEPOSIT_STATUS_DEPOSITED, in_progress=False): """Build deposit with a specific status """ @@ -389,6 +385,7 @@ sample_archive, external_id=external_id, deposit_status=deposit_status, + in_progress=in_progress, ) return _deposit @@ -396,7 +393,9 @@ deposited_deposit = deposit_factory() rejected_deposit = deposit_factory(deposit_status=DEPOSIT_STATUS_REJECTED) -partial_deposit = deposit_factory(deposit_status=DEPOSIT_STATUS_PARTIAL) +partial_deposit = deposit_factory( + deposit_status=DEPOSIT_STATUS_PARTIAL, in_progress=True +) verified_deposit = deposit_factory(deposit_status=DEPOSIT_STATUS_VERIFIED) completed_deposit = deposit_factory(deposit_status=DEPOSIT_STATUS_LOAD_SUCCESS) failed_deposit = deposit_factory(deposit_status=DEPOSIT_STATUS_LOAD_FAILURE) @@ -412,8 +411,9 @@ return create_binary_deposit( authenticated_client, deposit_collection.name, - sample_archive, + sample_archive=sample_archive, external_id="external-id-partial", + in_progress=True, deposit_status=DEPOSIT_STATUS_PARTIAL, atom_dataset=atom_dataset, )