D4431.diff
No OneTemporary
Actions

Size

18 KB

Subscribers

None

D4431.diff
View Options

	diff --git a/swh/deposit/cli/client.py b/swh/deposit/cli/client.py
	--- a/swh/deposit/cli/client.py
	+++ b/swh/deposit/cli/client.py
	@@ -12,7 +12,7 @@
	# control
	import os
	import sys
	-from typing import TYPE_CHECKING, Any, Dict, List, Optional, Tuple
	+from typing import TYPE_CHECKING, Any, Collection, Dict, List, Optional

	import click

	@@ -111,22 +111,6 @@
	return xmltodict.unparse(codemetadata, pretty=True)


	-def _client(url: str, username: str, password: str) -> PublicApiDepositClient:
	- """Instantiate a client to access the deposit api server
	-
	- Args:
	- url (str): Deposit api server
	- username (str): User
	- password (str): User's password
	-
	- """
	- from swh.deposit.client import PublicApiDepositClient
	-
	- return PublicApiDepositClient(
	- {"url": url, "auth": {"username": username, "password": password},}
	- )
	-
	-
	def _collection(client: PublicApiDepositClient) -> str:
	"""Retrieve the client's collection

	@@ -140,13 +124,13 @@


	def client_command_parse_input(
	+ client,
	username: str,
	- password: str,
	archive: Optional[str],
	metadata: Optional[str],
	+ collection: Optional[str],
	archive_deposit: bool,
	metadata_deposit: bool,
	- collection: Optional[str],
	slug: Optional[str],
	partial: bool,
	deposit_id: Optional[int],
	@@ -189,9 +173,8 @@

	'archive': the software archive to deposit
	'username': username
	- 'password': associated password
	'metadata': the metadata file to deposit
	- 'collection': the username's associated client
	+ 'collection: the user's collection under which to put the deposit
	'slug': the slug or external id identifying the deposit to make
	'partial': if the deposit is partial or not
	'client': instantiated class
	@@ -265,20 +248,16 @@
	if replace and not deposit_id:
	raise InputError("To update an existing deposit, you must provide its id")

	- client = _client(url, username, password)
	-
	if not collection:
	collection = _collection(client)

	return {
	"archive": archive,
	"username": username,
	- "password": password,
	"metadata": metadata,
	"collection": collection,
	"slug": slug,
	"in_progress": partial,
	- "client": client,
	"url": url,
	"deposit_id": deposit_id,
	"swhid": swhid,
	@@ -286,42 +265,11 @@
	}


	-def _subdict(d: Dict[str, Any], keys: Tuple[str, ...]) -> Dict[str, Any]:
	+def _subdict(d: Dict[str, Any], keys: Collection[str]) -> Dict[str, Any]:
	"return a dict from d with only given keys"
	return {k: v for k, v in d.items() if k in keys}


	-def deposit_create(config: Dict[str, Any]) -> Dict[str, Any]:
	- """Delegate the actual deposit to the deposit client.
	-
	- """
	- logger.debug("Create deposit")
	-
	- client = config["client"]
	- keys = ("collection", "archive", "metadata", "slug", "in_progress")
	- return client.deposit_create(**_subdict(config, keys))
	-
	-
	-def deposit_update(config: Dict[str, Any]) -> Dict[str, Any]:
	- """Delegate the actual deposit to the deposit client.
	-
	- """
	- logger.debug("Update deposit")
	-
	- client = config["client"]
	- keys = (
	- "collection",
	- "deposit_id",
	- "archive",
	- "metadata",
	- "slug",
	- "in_progress",
	- "replace",
	- "swhid",
	- )
	- return client.deposit_update(**_subdict(config, keys))
	-
	-
	@deposit.command()
	@click.option("--username", required=True, help="(Mandatory) User's name")
	@click.option(
	@@ -439,22 +387,22 @@
	"""
	import tempfile

	- from swh.deposit.client import MaintenanceError
	+ from swh.deposit.client import MaintenanceError, PublicApiDepositClient

	url = _url(url)
	- config = {}

	+ client = PublicApiDepositClient(url=url, auth=(username, password))
	with tempfile.TemporaryDirectory() as temp_dir:
	try:
	logger.debug("Parsing cli options")
	config = client_command_parse_input(
	+ client,
	username,
	- password,
	archive,
	metadata,
	+ collection,
	archive_deposit,
	metadata_deposit,
	- collection,
	slug,
	partial,
	deposit_id,
	@@ -475,12 +423,13 @@
	if verbose:
	logger.info("Parsed configuration: %s", config)

	- deposit_id = config["deposit_id"]
	-
	- if deposit_id:
	- data = deposit_update(config)
	+ keys = ["archive", "collection", "in_progress", "metadata", "slug"]
	+ if config["deposit_id"]:
	+ keys += ["deposit_id", "replace", "swhid"]
	+ data = client.deposit_update(**_subdict(config, keys))
	else:
	- data = deposit_create(config)
	+ data = client.deposit_create(**_subdict(config, keys))
	+
	print_result(data, output_format)


	@@ -509,12 +458,12 @@
	"""Deposit's status

	"""
	- from swh.deposit.client import MaintenanceError
	+ from swh.deposit.client import MaintenanceError, PublicApiDepositClient

	url = _url(url)
	logger.debug("Status deposit")
	try:
	- client = _client(url, username, password)
	+ client = PublicApiDepositClient(url=url, auth=(username, password))
	collection = _collection(client)
	except InputError as e:
	logger.error("Problem during parsing options: %s", e)
	diff --git a/swh/deposit/client.py b/swh/deposit/client.py
	--- a/swh/deposit/client.py
	+++ b/swh/deposit/client.py
	@@ -11,8 +11,9 @@
	import hashlib
	import logging
	import os
	-from typing import Any, Dict, Optional
	+from typing import Any, Dict, Optional, Tuple
	from urllib.parse import urljoin
	+import warnings

	import requests
	import xmltodict
	@@ -131,20 +132,43 @@
	return m


	+def handle_deprecated_config(config: Dict) -> Tuple[str, Optional[Tuple[str, str]]]:
	+ warnings.warn(
	+ '"config" argument is deprecated, please '
	+ 'use "url" and "auth" arguments instead; note that "auth" '
	+ "expects now a couple (username, password) and not a dict.",
	+ DeprecationWarning,
	+ )
	+ url: str = config["url"]
	+ auth: Optional[Tuple[str, str]] = None
	+
	+ if config.get("auth"):
	+ auth = (config["auth"]["username"], config["auth"]["password"])
	+
	+ return (url, auth)
	+
	+
	class BaseApiDepositClient:
	"""Deposit client base class

	"""

	- def __init__(self, config=None, _client=requests):
	- self.config: Dict[str, Any] = config or load_from_envvar()
	- self._client = _client
	- self.base_url = self.config["url"].strip("/") + "/"
	- auth = self.config["auth"]
	- if auth == {}:
	- self.auth = None
	- else:
	- self.auth = (auth["username"], auth["password"])
	+ def __init__(
	+ self,
	+ config: Optional[Dict] = None,
	+ url: Optional[str] = None,
	+ auth: Optional[Tuple[str, str]] = None,
	+ ):
	+ if not url and not config:
	+ config = load_from_envvar()
	+ if config:
	+ url, auth = handle_deprecated_config(config)
	+
	+ # needed to help mypy not be fooled by the Optional nature of url
	+ assert url is not None
	+
	+ self.base_url = url.strip("/") + "/"
	+ self.auth = auth

	def do(self, method, url, args, *kwargs):
	"""Internal method to deal with requests, possibly with basic http
	@@ -157,10 +181,7 @@
	The request's execution

	"""
	- if hasattr(self._client, method):
	- method_fn = getattr(self._client, method)
	- else:
	- raise ValueError("Development error, unsupported method %s" % (method))
	+ method_fn = getattr(requests, method)

	if self.auth:
	kwargs["auth"] = self.auth
	@@ -278,8 +299,10 @@

	"""

	- def __init__(self, config, error_msg=None, empty_result={}):
	- super().__init__(config)
	+ def __init__(
	+ self, config=None, url=None, auth=None, error_msg=None, empty_result={}
	+ ):
	+ super().__init__(url=url, auth=auth, config=config)
	self.error_msg = error_msg
	self.empty_result = empty_result

	@@ -382,9 +405,11 @@

	"""

	- def __init__(self, config):
	+ def __init__(self, config=None, url=None, auth=None):
	super().__init__(
	- config,
	+ url=url,
	+ auth=auth,
	+ config=config,
	error_msg="Service document failure at %s: %s",
	empty_result={"collection": None},
	)
	@@ -407,9 +432,11 @@

	"""

	- def __init__(self, config):
	+ def __init__(self, config=None, url=None, auth=None):
	super().__init__(
	- config,
	+ url=url,
	+ auth=auth,
	+ config=config,
	error_msg="Status check failure at %s: %s",
	empty_result={
	"deposit_status": None,
	@@ -446,9 +473,11 @@

	"""

	- def __init__(self, config):
	+ def __init__(self, config=None, url=None, auth=None):
	super().__init__(
	- config,
	+ url=url,
	+ auth=auth,
	+ config=config,
	error_msg="Post Deposit failure at %s: %s",
	empty_result={"deposit_id": None, "deposit_status": None,},
	)
	@@ -606,11 +635,13 @@

	def service_document(self):
	"""Retrieve service document endpoint's information."""
	- return ServiceDocumentDepositClient(self.config).execute()
	+ return ServiceDocumentDepositClient(url=self.base_url, auth=self.auth).execute()

	def deposit_status(self, collection: str, deposit_id: int):
	"""Retrieve status information on a deposit."""
	- return StatusDepositClient(self.config).execute(collection, deposit_id)
	+ return StatusDepositClient(url=self.base_url, auth=self.auth).execute(
	+ collection, deposit_id
	+ )

	def deposit_create(
	self,
	@@ -622,15 +653,17 @@
	):
	"""Create a new deposit (archive, metadata, both as multipart)."""
	if archive and not metadata:
	- return CreateArchiveDepositClient(self.config).execute(
	- collection, in_progress, slug, archive_path=archive
	- )
	+ return CreateArchiveDepositClient(
	+ url=self.base_url, auth=self.auth
	+ ).execute(collection, in_progress, slug, archive_path=archive)
	elif not archive and metadata:
	- return CreateMetadataDepositClient(self.config).execute(
	- collection, in_progress, slug, metadata_path=metadata
	- )
	+ return CreateMetadataDepositClient(
	+ url=self.base_url, auth=self.auth
	+ ).execute(collection, in_progress, slug, metadata_path=metadata)
	else:
	- return CreateMultipartDepositClient(self.config).execute(
	+ return CreateMultipartDepositClient(
	+ url=self.base_url, auth=self.auth
	+ ).execute(
	collection,
	in_progress,
	slug,
	@@ -670,7 +703,7 @@
	"deposit_id": deposit_id,
	}
	if archive and not metadata:
	- r = UpdateArchiveDepositClient(self.config).execute(
	+ r = UpdateArchiveDepositClient(url=self.base_url, auth=self.auth).execute(
	collection,
	in_progress,
	slug,
	@@ -679,7 +712,9 @@
	replace=replace,
	)
	elif not archive and metadata and swhid is None:
	- r = UpdateMetadataOnPartialDepositClient(self.config).execute(
	+ r = UpdateMetadataOnPartialDepositClient(
	+ url=self.base_url, auth=self.auth
	+ ).execute(
	collection,
	in_progress,
	slug,
	@@ -688,7 +723,9 @@
	replace=replace,
	)
	elif not archive and metadata and swhid is not None:
	- r = UpdateMetadataOnDoneDepositClient(self.config).execute(
	+ r = UpdateMetadataOnDoneDepositClient(
	+ url=self.base_url, auth=self.auth
	+ ).execute(
	collection,
	in_progress,
	slug,
	@@ -697,7 +734,7 @@
	swhid=swhid,
	)
	else:
	- r = UpdateMultipartDepositClient(self.config).execute(
	+ r = UpdateMultipartDepositClient(url=self.base_url, auth=self.auth).execute(
	collection,
	in_progress,
	slug,
	diff --git a/swh/deposit/tests/cli/test_client.py b/swh/deposit/tests/cli/test_client.py
	--- a/swh/deposit/tests/cli/test_client.py
	+++ b/swh/deposit/tests/cli/test_client.py
	@@ -18,7 +18,6 @@
	from swh.deposit.cli import deposit as cli
	from swh.deposit.cli.client import (
	InputError,
	- _client,
	_collection,
	_url,
	generate_metadata,
	@@ -30,14 +29,6 @@
	from ..conftest import TEST_USER


	-@pytest.fixture
	-def deposit_config():
	- return {
	- "url": "https://deposit.swh.test/1",
	- "auth": {"username": "test", "password": "test",},
	- }
	-
	-
	@pytest.fixture
	def datadir(request):
	"""Override default datadir to target main test datadir"""
	@@ -64,7 +55,7 @@

	"""
	mock_client = MagicMock()
	- mocker.patch("swh.deposit.cli.client._client", return_value=mock_client)
	+ mocker.patch("swh.deposit.client.PublicApiDepositClient", return_value=mock_client)
	mock_client.service_document.side_effect = MaintenanceError(
	"Database backend maintenance: Temporarily unavailable, try again later."
	)
	@@ -76,11 +67,6 @@
	assert _url("https://other/1") == "https://other/1"


	-def test_cli_client():
	- client = _client("http://deposit", "user", "pass")
	- assert isinstance(client, PublicApiDepositClient)
	-
	-
	def test_cli_collection_error():
	mock_client = MagicMock()
	mock_client.service_document.return_value = {"error": "something went wrong"}
	@@ -91,8 +77,10 @@
	assert "Service document retrieval: something went wrong" == str(e.value)


	-def test_cli_collection_ok(deposit_config, requests_mock_datadir):
	- client = PublicApiDepositClient(deposit_config)
	+def test_cli_collection_ok(requests_mock_datadir):
	+ client = PublicApiDepositClient(
	+ url="https://deposit.swh.test/1", auth=("test", "test")
	+ )
	collection_name = _collection(client)
	assert collection_name == "test"

	diff --git a/swh/deposit/tests/loader/test_client.py b/swh/deposit/tests/loader/test_client.py
	--- a/swh/deposit/tests/loader/test_client.py
	+++ b/swh/deposit/tests/loader/test_client.py
	@@ -6,7 +6,6 @@
	import json
	import os
	from typing import Any, Callable, Optional
	-import unittest
	from urllib.parse import urlparse

	import pytest
	@@ -14,7 +13,6 @@
	from swh.deposit.client import PrivateApiDepositClient
	from swh.deposit.config import DEPOSIT_STATUS_LOAD_FAILURE, DEPOSIT_STATUS_LOAD_SUCCESS

	-
	CLIENT_TEST_CONFIG = {
	"url": "https://nowhere.org/",
	"auth": {}, # no authentication in test scenario
	@@ -28,12 +26,13 @@

	def test_client_config(deposit_config_path):
	for client in [
	- # config passed as constructor parameter
	- PrivateApiDepositClient(config=CLIENT_TEST_CONFIG),
	- # config loaded from environment
	- PrivateApiDepositClient()
	+ # config passed as constructor parameter
	+ PrivateApiDepositClient(config=CLIENT_TEST_CONFIG),
	+ # config loaded from environment
	+ PrivateApiDepositClient(),
	]:
	- assert client.config == CLIENT_TEST_CONFIG
	+ assert client.base_url == CLIENT_TEST_CONFIG["url"]
	+ assert client.auth is None


	def build_expected_path(datadir, base_url: str, api_url: str) -> str:
	@@ -205,58 +204,36 @@
	# private api update status


	-class FakeRequestClientPut:
	- """Fake Request client dedicated to put request method calls.
	+def test_status_update(mocker):
	+ """Update status
	+
	+ """
	+ mocked_put = mocker.patch("swh.deposit.client.requests.put")
	+
	+ deposit_client = PrivateApiDepositClient(config=CLIENT_TEST_CONFIG)
	+ deposit_client.status_update(
	+ "/update/status", DEPOSIT_STATUS_LOAD_SUCCESS, revision_id="some-revision-id",
	+ )
	+
	+ mocked_put.assert_called_once_with(
	+ "https://nowhere.org/update/status",
	+ json={
	+ "status": DEPOSIT_STATUS_LOAD_SUCCESS,
	+ "revision_id": "some-revision-id",
	+ },
	+ )
	+
	+
	+def test_status_update_with_no_revision_id(mocker):
	+ """Reading metadata can fail for some reasons

	"""
	+ mocked_put = mocker.patch("swh.deposit.client.requests.put")
	+
	+ deposit_client = PrivateApiDepositClient(config=CLIENT_TEST_CONFIG)
	+ deposit_client.status_update("/update/status/fail", DEPOSIT_STATUS_LOAD_FAILURE)

	- args = None
	- kwargs = None
	-
	- def put(self, args, *kwargs):
	- self.args = args
	- self.kwargs = kwargs
	-
	-
	-class PrivateApiDepositClientStatusUpdateTest(unittest.TestCase):
	- def test_status_update(self):
	- """Update status
	-
	- """
	- _client = FakeRequestClientPut()
	- deposit_client = PrivateApiDepositClient(
	- config=CLIENT_TEST_CONFIG, _client=_client
	- )
	-
	- deposit_client.status_update(
	- "/update/status",
	- DEPOSIT_STATUS_LOAD_SUCCESS,
	- revision_id="some-revision-id",
	- )
	-
	- self.assertEqual(_client.args, ("https://nowhere.org/update/status",))
	- self.assertEqual(
	- _client.kwargs,
	- {
	- "json": {
	- "status": DEPOSIT_STATUS_LOAD_SUCCESS,
	- "revision_id": "some-revision-id",
	- }
	- },
	- )
	-
	- def test_status_update_with_no_revision_id(self):
	- """Reading metadata can fail for some reasons
	-
	- """
	- _client = FakeRequestClientPut()
	- deposit_client = PrivateApiDepositClient(
	- config=CLIENT_TEST_CONFIG, _client=_client
	- )
	-
	- deposit_client.status_update("/update/status/fail", DEPOSIT_STATUS_LOAD_FAILURE)
	-
	- self.assertEqual(_client.args, ("https://nowhere.org/update/status/fail",))
	- self.assertEqual(
	- _client.kwargs, {"json": {"status": DEPOSIT_STATUS_LOAD_FAILURE,}}
	- )
	+ mocked_put.assert_called_once_with(
	+ "https://nowhere.org/update/status/fail",
	+ json={"status": DEPOSIT_STATUS_LOAD_FAILURE,},
	+ )

File Metadata

Mime Type: text/plain
Expires: Tue, Jun 3, 7:26 PM (1 w, 1 d ago)
Storage Engine: blob
Storage Format: Raw Data
Storage Handle: 3229514

D4431.diffNo OneTemporaryActions

D4431.diffView Options

File Metadata

Event Timeline

D4431.diff
No OneTemporary
Actions

D4431.diff
View Options