Differential D4475 Diff 15910 swh/deposit/api/common.py

Changeset View

Standalone View

swh/deposit/api/common.py

# Copyright (C) 2017-2020 The Software Heritage developers		# Copyright (C) 2017-2020 The Software Heritage developers
# See the AUTHORS file at the top-level directory of this distribution		# See the AUTHORS file at the top-level directory of this distribution
# License: GNU General Public License version 3, or any later version		# License: GNU General Public License version 3, or any later version
# See top-level LICENSE file for more information		# See top-level LICENSE file for more information

		import attr

from abc import ABCMeta, abstractmethod		from abc import ABCMeta, abstractmethod
import datetime		import datetime
import hashlib		import hashlib
import json		import json
from typing import Any, Dict, Optional, Sequence, Tuple, Type, Union		from typing import Any, Dict, Optional, Sequence, Tuple, Type, Union

from django.http import FileResponse, HttpResponse		from django.http import FileResponse, HttpResponse
from django.shortcuts import render		from django.shortcuts import render
from django.urls import reverse		from django.urls import reverse
from django.utils import timezone		from django.utils import timezone
from rest_framework import status		from rest_framework import status
from rest_framework.authentication import BaseAuthentication, BasicAuthentication		from rest_framework.authentication import BaseAuthentication, BasicAuthentication
from rest_framework.permissions import BasePermission, IsAuthenticated		from rest_framework.permissions import BasePermission, IsAuthenticated
from rest_framework.request import Request		from rest_framework.request import Request
from rest_framework.views import APIView		from rest_framework.views import APIView

		from swh.deposit.api.checks import check_metadata
		from swh.deposit.api.converters import convert_status_detail
		from swh.deposit.models import Deposit
from swh.model import hashutil		from swh.model import hashutil
		from swh.model.identifiers import SWHID, ValidationError
		from swh.model.model import (
		MetadataAuthority,
		MetadataAuthorityType,
		MetadataFetcher,
		MetadataTargetType,
		RawExtrinsicMetadata,
		)
from swh.scheduler.utils import create_oneshot_task_dict		from swh.scheduler.utils import create_oneshot_task_dict

from ..config import (		from ..config import (
ARCHIVE_KEY,		ARCHIVE_KEY,
ARCHIVE_TYPE,		ARCHIVE_TYPE,
CONT_FILE_IRI,		CONT_FILE_IRI,
DEPOSIT_STATUS_DEPOSITED,		DEPOSIT_STATUS_DEPOSITED,
DEPOSIT_STATUS_LOAD_SUCCESS,		DEPOSIT_STATUS_LOAD_SUCCESS,
Show All 16 Lines	from ..errors import (
METHOD_NOT_ALLOWED,		METHOD_NOT_ALLOWED,
NOT_FOUND,		NOT_FOUND,
PARSING_ERROR,		PARSING_ERROR,
ParserError,		ParserError,
make_error_dict,		make_error_dict,
make_error_response,		make_error_response,
make_error_response_from_dict,		make_error_response_from_dict,
)		)
from ..models import Deposit, DepositClient, DepositCollection, DepositRequest		from ..models import DepositClient, DepositCollection, DepositRequest
from ..parsers import parse_xml		from ..parsers import parse_swh_reference, parse_xml

ACCEPT_PACKAGINGS = ["http://purl.org/net/sword/package/SimpleZip"]		ACCEPT_PACKAGINGS = ["http://purl.org/net/sword/package/SimpleZip"]
ACCEPT_ARCHIVE_CONTENT_TYPES = ["application/zip", "application/x-tar"]		ACCEPT_ARCHIVE_CONTENT_TYPES = ["application/zip", "application/x-tar"]


class AuthenticatedAPIView(APIView):		class AuthenticatedAPIView(APIView):
"""Mixin intended as a based API view to enforce the basic		"""Mixin intended as a based API view to enforce the basic
authentication check		authentication check
▲ Show 20 Lines • Show All 533 Lines • ▼ Show 20 Lines	) -> Dict:
assert filehandler is not None		assert filehandler is not None
return {		return {
"deposit_id": deposit.id,		"deposit_id": deposit.id,
"deposit_date": deposit.reception_date,		"deposit_date": deposit.reception_date,
"archive": filehandler.name,		"archive": filehandler.name,
"status": deposit.status,		"status": deposit.status,
}		}

		def _store_metadata_deposit(
		self,
		deposit: Deposit,
		swhid_reference: Union[str, SWHID],
		metadata: Dict,
		raw_metadata: bytes,
		with_deposit_origin: bool = False,
		) -> Dict:
		"""Metadata-only deposit

		Checks:
		vlorentzUnsubmitted Done Inline Actions What does "In the nominal scenario" mean? vlorentz: What does "In the nominal scenario" mean?
		ardumontAuthorUnsubmitted Done Inline Actions well, no bump in the road, all user inputs are correct so we can do the actual job properly. ardumont: well, no bump in the road, all user inputs are correct so we can do the actual job properly.
		- The metadata received should pass the functional metadata checks.
		- The SWHID is technically valid

		"""
		moraneggUnsubmitted Not Done Inline Actions object referenced (SWhiD or origin url) moranegg: object referenced (SWhiD or origin url)
		if not metadata:
		return make_error_dict(
		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.",
		)

		moraneggUnsubmitted Not Done Inline Actions the word origin here doesn't mean a url origin, right? maybe use `with_original_deposit`? moranegg: the word origin here doesn't mean a url origin, right? maybe use `with_original_deposit`?
		ardumontAuthorUnsubmitted Done Inline Actions yes, it does mean an origin url as per the deposit update scenario. and lol, I initially made that a bool and called this `with_origin_deposit`... pretty much similar. ardumont: yes, it does mean an origin url as per the deposit update scenario. ---- and lol, I initially…
		metadata_ok, error_details = check_metadata(metadata)
		if not metadata_ok:
		assert error_details, "Details should be set when a failure occurs"
		return make_error_dict(
		BAD_REQUEST,
		"Functional metadata checks failure",
		convert_status_detail(error_details),
		vlorentzUnsubmitted Done Inline Actions `if metadata is not None` vlorentz: `if metadata is not None`
		ardumontAuthorUnsubmitted Done Inline Actions nah, i recall it's not the same here. (plus that code is just moving from the deposit_update part). And indeed, existing tests are complaining about it. ardumont: nah, i recall it's not the same here. (plus that code is just moving from the deposit_update…
		)

		metadata_authority = MetadataAuthority(
		type=MetadataAuthorityType.DEPOSIT_CLIENT,
		ardumontAuthorUnsubmitted Done Inline Actions heh, that's not a possible situation... (if we are here, that means we have extracted the swhid or origin reference so it's not an empty body request ¯\_(ツ)_/¯). ardumont: heh, that's not a possible situation... (if we are here, that means we have extracted the swhid…
		url=deposit.client.provider_url,
		metadata={"name": deposit.client.last_name},
		)

		metadata_fetcher = MetadataFetcher(
		name=self.tool["name"],
		version=self.tool["version"],
		metadata=self.tool["configuration"],
		)

		# replace metadata within the deposit backend
		moraneggUnsubmitted Not Done Inline Actions there is a notion of first_name and last_name for a client? moranegg: there is a notion of first_name and last_name for a client?
		deposit_request_data = {
		METADATA_KEY: metadata,
		RAW_METADATA_KEY: raw_metadata,
		}

		# actually add the metadata to the completed deposit
		deposit_request = self._deposit_request_put(deposit, deposit_request_data)

		map_type = {
		"content": MetadataTargetType.CONTENT,
		"directory": MetadataTargetType.DIRECTORY,
		vlorentzUnsubmitted Done Inline Actions `metadata={}` vlorentz: `metadata={}`
		ardumontAuthorUnsubmitted Done Inline Actions why should this be empty? (also if that's ending up not being used, i can make that be dropped completely from the configuration then) ardumont: why should this be empty? (also if that's ending up not being used, i can make that be dropped…
		vlorentzUnsubmitted Not Done Inline Actions Hmm actually, nevermind. I just re-read the metadata spec. It would be an issue currently because the config is not part of the unique key, but olasd is working on that. vlorentz: Hmm actually, nevermind. I just re-read the metadata spec. It would be an issue currently…
		"revision": MetadataTargetType.REVISION,
		"release": MetadataTargetType.RELEASE,
		"snapshot": MetadataTargetType.SNAPSHOT,
		"origin": MetadataTargetType.ORIGIN,
		}

		metadata_d = {"origin": None}
		if isinstance(swhid_reference, SWHID):
		object_type = map_type[swhid_reference.object_type]
		if swhid_reference.metadata:
		path = swhid_reference.metadata.get("path")
		metadata_d = {
		"origin": swhid_reference.metadata.get("origin"),
		# "visit": swhid_reference.metadata.get("visit"),
		"path": path.encode() if path else None,
		# "...": swhid_reference.metadata.get("anchor")
		}
		# Make the swhid a swhid core
		ardumontAuthorUnsubmitted Done Inline Actions @vlorentz i'm confused as how to actually map from a swhid like [1] to a raw extrinsic metadata. Especially in regards to the swhid metadata `visit` and `anchor` qualifiers which are swhid themselves... Do you have any clues? [1] swh:1:dir:c4993c872593e960dc84e4430dbbfbc34fd706d0;origin=https://inria.halpreprod.archives-ouvertes.fr/hal-01243573;visit=swh:1:snp:0175049fc45055a3824a1675ac06e3711619a55a;anchor=swh:1:rev:b5f505b005435fa5c4fa4c279792bd7b17167c04;path=/ ( ^ that's existing swhid from the deposit itself) ardumont: @vlorentz i'm confused as how to actually map from a swhid like [1] to a raw extrinsic metadata.
		ardumontAuthorUnsubmitted Done Inline Actions got my answers at the docs [1] "visit" is a snapshot core swhid (if any) "anchor" is a core swhid of leaf node (either directory, a revision, a release or a snapshot) [1] https://docs.softwareheritage.org/devel/swh-model/persistent-identifiers.html#qualifiers ardumont: got my answers at the docs [1] - "visit" is a snapshot core swhid (if any) - "anchor" is a…
		swhid_reference = attr.evolve(swhid_reference, metadata={})
		else:
		vlorentzUnsubmitted Done Inline Actions Simpler IMO: swhid_core: Union[str, SWHID] if isinstance(swhid_reference, str): swhid_core = swhid_reference else: swhid_core = attr.evolve(swhid_reference, metadata={}) vlorentz: Simpler IMO: ``` swhid_core: Union[str, SWHID] if isinstance(swhid_reference, str)…
		moraneggUnsubmitted Not Done Inline Actions naming here is difficult to follow. A suggestion: use `object_reference` which can be an `origin` or a `swhid_core` for the context without the swhid_core use `swhid_only_context` for the swhid wth context `swhid_with_context` moranegg: naming here is difficult to follow. A suggestion: use `object_reference` which can be an…
		object_type = MetadataTargetType.ORIGIN

		if with_deposit_origin: # metadata deposit update on completed deposit
		vlorentzUnsubmitted Done Inline Actions `object_type = MetadataTargetType(swhid_reference.object_type)` + assert it's not `MetadataTargetType.ORIGIN` vlorentz: `object_type = MetadataTargetType(swhid_reference.object_type)` + assert it's not…
		ardumontAuthorUnsubmitted Done Inline Actions neat! ardumont: neat!
		metadata_d["origin"] = deposit.origin_url

		# store that metadata to the metadata storage
		metadata_object = RawExtrinsicMetadata(
		type=object_type,
		vlorentzUnsubmitted Done Inline Actions encoding the path should be done in `parse_swhid` vlorentz: encoding the path should be done in `parse_swhid`
		ardumontAuthorUnsubmitted Done Inline Actions how so? Do you mean the path stored in the SWHID class (resulting from parse_swhid call) should be bytes already? If that's the case, let's keep it, and modify swh.model later with this. I'll do the required adaptations later. ardumont: how so? Do you mean the path stored in the SWHID class (resulting from parse_swhid call)…
		vlorentzUnsubmitted Done Inline Actions ok vlorentz: ok
		target=swhid_reference,
		discovery_date=deposit_request.date,
		authority=metadata_authority,
		fetcher=metadata_fetcher,
		ardumontAuthorUnsubmitted Done Inline Actions Those should not raise providing D4490 ardumont: Those should not raise providing D4490
		format="sword-v2-atom-codemeta",
		metadata=raw_metadata,
		**metadata_d
		)
		ardumontAuthorUnsubmitted Done Inline Actions same remark as above. ardumont: same remark as above.

		vlorentzUnsubmitted Done Inline Actions What if there is a `visit` qualifier and the `anchor` is also a snapshot? vlorentz: What if there is a `visit` qualifier and the `anchor` is also a snapshot?
		ardumontAuthorUnsubmitted Done Inline Actions Well, i thought of it as well but i don't know what to do for that case... So right now, it's left as is indeed (i should have said as much in the code). Note that I don't really know what that means when such case arise... I guess i could make the parsing in D4490 fail when such case arise... At least, with the right logging instructions, we'd have a way of knowing if people actually use both at the same time ¯\_(ツ)_/¯... Do you have a better proposal than failing the parsing (and log it)? ardumont: Well, i thought of it as well but i don't know what to do for that case... So right now, it's…
		vlorentzUnsubmitted Done Inline Actions but i don't know what to do for that case... return an HTTP error vlorentz: > but i don't know what to do for that case... return an HTTP error
		ardumontAuthorUnsubmitted Done Inline Actions Let's discuss that in D4493 which tries to do that ;) ardumont: Let's discuss that in D4493 which tries to do that ;)
		# write to metadata storage
		self.storage_metadata.metadata_authority_add([metadata_authority])
		self.storage_metadata.metadata_fetcher_add([metadata_fetcher])
		self.storage_metadata.raw_extrinsic_metadata_add([metadata_object])

		return {
		"deposit_id": deposit.id,
		vlorentzUnsubmitted Done Inline Actions I don't understand how this conditional is related to checking if it's a metadata-only deposit vlorentz: I don't understand how this conditional is related to checking if it's a metadata-only deposit
		ardumontAuthorUnsubmitted Done Inline Actions well, aside from specifically add a flag "with_metadata_only_deposit" (or something), how do you do detect it? For me we have currently 2 cases: metadata-update: in that case, the deposit status is done (it's a completed one) and its swhid and swhid context are set metadata-only deposit: in that case the deposit is new, so its status is something like 'partial' and swhid and swhid context are null. But i'm fine if you ask me to open a flag instead, it may be clearer that way. ardumont: well, aside from specifically add a flag "with_metadata_only_deposit" (or something), how do…
		"deposit_date": deposit_request.date,
		vlorentzUnsubmitted Done Inline Actions move this (the computation of `metadata_d`) to its own function. (We will probably also move it to swh-model at some point) vlorentz: move this (the computation of `metadata_d`) to its own function. (We will probably also move…
		ardumontAuthorUnsubmitted Done Inline Actions yep ardumont: yep
		"status": deposit.status,
		"archive": None,
		}

def _atom_entry(		def _atom_entry(
self,		self,
		vlorentzUnsubmitted Done Inline Actions Why does a function named `_store_metadata_deposit` update the deposit's status? Shouldn't this be done in the caller function? (and you might be able to drop the conditional that way) vlorentz: Why does a function named `_store_metadata_deposit` update the deposit's status? Shouldn't this…
		ardumontAuthorUnsubmitted Done Inline Actions oh yeah, indeed, it crossed my mind and then i forget to act on it. Thanks. ardumont: oh yeah, indeed, it crossed my mind and then i forget to act on it. Thanks.
		ardumontAuthorUnsubmitted Done Inline Actions Why does a function named _store_metadata_deposit update the deposit's status? to be fair, i mentioned it in the docstring... Shouldn't this be done in the caller function? yeah that'd be more clean. But, that's tedious with the current way of dealing with errors... that returns a dict... and not a tuple of stuff i'd like to have... The signature will become ugly... (and you might be able to drop the conditional that way) indeed, that's the advantage. ardumont: > Why does a function named _store_metadata_deposit update the deposit's status? to be fair…
request: Request,		request: Request,
headers: Dict[str, Any],		headers: Dict[str, Any],
collection_name: str,		collection_name: str,
deposit_id: Optional[int] = None,		deposit_id: Optional[int] = None,
replace_metadata: bool = False,		replace_metadata: bool = False,
replace_archives: bool = False,		replace_archives: bool = False,
) -> Dict[str, Any]:		) -> Dict[str, Any]:
"""Atom entry deposit.		"""Atom entry deposit.
▲ Show 20 Lines • Show All 41 Lines • ▼ Show 20 Lines	) -> Dict[str, Any]:
if not metadata:		if not metadata:
return make_error_dict(		return make_error_dict(
BAD_REQUEST,		BAD_REQUEST,
"Empty body request is not supported",		"Empty body request is not supported",
"Atom entry deposit is supposed to send for metadata. "		"Atom entry deposit is supposed to send for metadata. "
"If the body is empty, there is no metadata.",		"If the body is empty, there is no metadata.",
)		)

external_id = metadata.get("external_identifier", headers["slug"])		# Determine if we are in the metadata-only deposit case
		try:
		swhid = parse_swh_reference(metadata)
		except ValidationError as e:
		return make_error_dict(PARSING_ERROR, "Invalid SWHID reference", str(e),)

# TODO: Determine if we are in the metadata-only deposit case. If it is, then		external_id = metadata.get("external_identifier", headers["slug"])
# save deposit and deposit request typed 'metadata' and send metadata to the
# metadata storage. Otherwise, do as existing deposit.

deposit = self._deposit_put(		deposit = self._deposit_put(
request,		request,
deposit_id=deposit_id,		deposit_id=deposit_id,
in_progress=headers["in-progress"],		in_progress=headers["in-progress"],
external_id=external_id,		external_id=external_id,
)		)

		if swhid is not None:
		return self._store_metadata_deposit(deposit, swhid, metadata, raw_metadata)

self._deposit_request_put(		self._deposit_request_put(
deposit,		deposit,
		ardumontAuthorUnsubmitted Done Inline Actions and now i missed some cases, fixing as well. ardumont: and now i missed some cases, fixing as well.
{METADATA_KEY: metadata, RAW_METADATA_KEY: raw_metadata},		{METADATA_KEY: metadata, RAW_METADATA_KEY: raw_metadata},
		vlorentzUnsubmitted Done Inline Actions tuple = success, dict = error? meh what about an exception instead? vlorentz: tuple = success, dict = error? meh what about an exception instead?
		ardumontAuthorUnsubmitted Done Inline Actions tuple = success, dict = error? meh well, yeah... what about an exception instead? I don't really want to plunge into refactoring the current actual exception handling in the deposit right now. ardumont: > tuple = success, dict = error? meh well, yeah... > what about an exception instead? I…
		vlorentzUnsubmitted Done Inline Actions what about returning `(boolean, result)`, like the metadata validation function? vlorentz: what about returning `(boolean, result)`, like the metadata validation function?
		ardumontAuthorUnsubmitted Done Inline Actions I was ok with the last suggestion and i implemented it accordingly. But in the end, that just added another indirection... So in the end, i prefer adding some exception handling locally instead (as per your first proposal). (We will adapt the other part of the deposit similarly later if we agree upon that ;) ardumont: I was ok with the last suggestion and i implemented it accordingly. But in the end, that just…
		ardumontAuthorUnsubmitted Done Inline Actions T2792 to rework the exception handling. ardumont: T2792 to rework the exception handling.
replace_metadata,		replace_metadata,
replace_archives,		replace_archives,
)		)

return {		return {
"deposit_id": deposit.id,		"deposit_id": deposit.id,
"deposit_date": deposit.reception_date,		"deposit_date": deposit.reception_date,
		moraneggUnsubmitted Not Done Inline Actions looks like the same date, is that normal behavior? it's when there is only one deposit request without `in-progress` right? moranegg: looks like the same date, is that normal behavior? it's when there is only one deposit request…
"archive": None,		"archive": None,
"status": deposit.status,		"status": deposit.status,
}		}

def _empty_post(		def _empty_post(
self, request: Request, headers: Dict, collection_name: str, deposit_id: int		self, request: Request, headers: Dict, collection_name: str, deposit_id: int
) -> Dict[str, Any]:		) -> Dict[str, Any]:
"""Empty post to finalize an empty deposit.		"""Empty post to finalize an empty deposit.
▲ Show 20 Lines • Show All 347 Lines • Show Last 20 Lines