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,}, + )