From 5702bf56407007c20b50b540e0c9d3f45711b314 Mon Sep 17 00:00:00 2001 From: nebay-abraha Date: Mon, 13 Jul 2026 07:36:20 +0100 Subject: [PATCH 1/5] fix: improve response error handling and add public exception hierarchy --- .gitignore | 2 + rspace_client/__init__.py | 14 ++- rspace_client/client_base.py | 83 +++++++++------- rspace_client/exceptions.py | 50 ++++++++++ rspace_client/tests/exceptions_test.py | 127 +++++++++++++++++++++++++ 5 files changed, 239 insertions(+), 37 deletions(-) create mode 100644 rspace_client/exceptions.py create mode 100644 rspace_client/tests/exceptions_test.py diff --git a/.gitignore b/.gitignore index 7ffaf03..af469f8 100644 --- a/.gitignore +++ b/.gitignore @@ -103,3 +103,5 @@ rspace_client/.spyproject/config/workspace.ini # mac os .DS_Store + +.claude/ diff --git a/rspace_client/__init__.py b/rspace_client/__init__.py index 89418c9..03b8c98 100644 --- a/rspace_client/__init__.py +++ b/rspace_client/__init__.py @@ -6,6 +6,13 @@ from .eln.advanced_query_builder import AdvancedQueryBuilder from .utils import createELNClient from .eln.field_content import FieldContent +from .exceptions import ( + ApiError, + AuthenticationError, + NoSuchLinkRel, + RSpaceConnectionError, + RSpaceError, +) __all__ = [ "ELNClient", @@ -13,5 +20,10 @@ "AdvancedQueryBuilder", "createELNClient", "FieldContent", - "notebook_sync" + "notebook_sync", + "RSpaceError", + "RSpaceConnectionError", + "AuthenticationError", + "ApiError", + "NoSuchLinkRel", ] diff --git a/rspace_client/client_base.py b/rspace_client/client_base.py index e4236fe..5fc5e22 100644 --- a/rspace_client/client_base.py +++ b/rspace_client/client_base.py @@ -2,6 +2,14 @@ import requests import sys +from rspace_client.exceptions import ( + ApiError, + AuthenticationError, + NoSuchLinkRel, + RSpaceConnectionError, + RSpaceError, +) + class Pagination: """ @@ -65,33 +73,43 @@ def _responseContainsJson(response): and "application/json" in response.headers["Content-Type"] ) + @staticmethod + def _get_error_detail(response): + """ + Builds an error description from a response body, tolerating bodies + whose Content-Type claims JSON but do not parse. + """ + if ClientBase._responseContainsJson(response): + try: + return ClientBase._get_formated_error_message(response.json()) + except ValueError: + pass + return "error message: {}".format(response.text) + @staticmethod def _handle_response(response): - # Check whether response includes UNAUTHORIZED response code - # print("status: {}, header: {}".format(response.headers, response.status_code)) if response.status_code == 401: - raise ClientBase.AuthenticationError(response.json()["message"]) + raise AuthenticationError( + "Error code: 401, {}".format(ClientBase._get_error_detail(response)) + ) try: response.raise_for_status() + except requests.HTTPError: + raise ApiError( + "Error code: {}, {}".format( + response.status_code, ClientBase._get_error_detail(response) + ), + response_status_code=response.status_code, + response=response, + ) - if ClientBase._responseContainsJson(response): - return response.json() - elif response.text: - return response.text - else: - return response - except: - if "application/json" in response.headers["Content-Type"]: - error = "Error code: {}, {}".format( - response.status_code, - ClientBase._get_formated_error_message(response.json()), - ) - else: - error = "Error code: {}, error message: {}".format( - response.status_code, response.text - ) - raise ClientBase.ApiError(error, response_status_code=response.status_code) + if ClientBase._responseContainsJson(response): + return response.json() + elif response.text: + return response.text + else: + return response def doDelete(self, path, resource_id): """ @@ -141,7 +159,7 @@ def retrieve_api_results( return self._handle_response(response) except requests.exceptions.ConnectionError as e: - raise ClientBase.ConnectionError(e) + raise RSpaceConnectionError(e) @staticmethod def _get_links(response): @@ -154,7 +172,7 @@ def _get_links(response): try: return response["_links"] except KeyError: - raise ClientBase.NoSuchLinkRel("There are no links!") + raise NoSuchLinkRel("There are no links!") def get_link_contents(self, response, link_rel): """ @@ -175,7 +193,7 @@ def get_link(self, response, link_rel): for link in self._get_links(response): if link["rel"] == link_rel: return link["link"] - raise ClientBase.NoSuchLinkRel( + raise NoSuchLinkRel( 'Requested link rel "{}", available rel(s): {}'.format( link_rel, (", ".join(x["rel"] for x in self._get_links(response))) ) @@ -251,16 +269,9 @@ def _stream( else: break - class ConnectionError(Exception): - pass - - class AuthenticationError(Exception): - pass - - class NoSuchLinkRel(Exception): - pass - - class ApiError(Exception): - def __init__(self, error_message, response_status_code=None): - Exception.__init__(self, error_message) - self.response_status_code = response_status_code + # Deprecated aliases of the module-level classes in rspace_client.exceptions, + # kept so existing `except ClientBase.ApiError:` code works. Remove in 3.0. + ConnectionError = RSpaceConnectionError + AuthenticationError = AuthenticationError + NoSuchLinkRel = NoSuchLinkRel + ApiError = ApiError diff --git a/rspace_client/exceptions.py b/rspace_client/exceptions.py new file mode 100644 index 0000000..9c86659 --- /dev/null +++ b/rspace_client/exceptions.py @@ -0,0 +1,50 @@ +""" +Exception hierarchy for rspace-client. + +All exceptions raised by this library inherit from RSpaceError, so user code +can catch anything the client raises with ``except rspace_client.RSpaceError``. + +Before version 2.8.0 these lived as nested classes on ClientBase +(e.g. ``ClientBase.ApiError``). Those names remain as aliases of the classes +defined here and will be removed in 3.0. +""" +from typing import Optional + +import requests + + +class RSpaceError(Exception): + """Base class for all errors raised by rspace-client.""" + + +class RSpaceConnectionError(RSpaceError): + """Raised when the client cannot connect to the RSpace server.""" + + +class AuthenticationError(RSpaceError): + """Raised when the server rejects the API key (HTTP 401).""" + + +class NoSuchLinkRel(RSpaceError): + """Raised when a requested link rel is not present in a response.""" + + +class ApiError(RSpaceError): + """ + Raised when the server returns an error response. + + :param error_message: human-readable description of the failure + :param response_status_code: HTTP status code of the error response + :param response: the raw requests.Response, when available, so callers + can inspect headers and body + """ + + def __init__( + self, + error_message: str, + response_status_code: Optional[int] = None, + response: Optional[requests.Response] = None, + ): + super().__init__(error_message) + self.response_status_code = response_status_code + self.response = response diff --git a/rspace_client/tests/exceptions_test.py b/rspace_client/tests/exceptions_test.py new file mode 100644 index 0000000..ea3d8cd --- /dev/null +++ b/rspace_client/tests/exceptions_test.py @@ -0,0 +1,127 @@ +#!/usr/bin/env python3 +# -*- coding: utf-8 -*- +""" +Unit tests for the exception hierarchy and ClientBase._handle_response. +""" +import json +import unittest + +import requests + +import rspace_client +from rspace_client.client_base import ClientBase +from rspace_client.exceptions import ( + ApiError, + AuthenticationError, + NoSuchLinkRel, + RSpaceConnectionError, + RSpaceError, +) + + +def make_response(status_code, json_body=None, text_body=None, content_type=None): + """ + Builds a real requests.Response without a network call. + """ + response = requests.models.Response() + response.status_code = status_code + if json_body is not None: + response._content = json.dumps(json_body).encode("utf-8") + response.headers["Content-Type"] = "application/json" + elif text_body is not None: + response._content = text_body.encode("utf-8") + else: + response._content = b"" + if content_type is not None: + response.headers["Content-Type"] = content_type + return response + + +class HandleResponseTest(unittest.TestCase): + def test_success_json_returns_dict(self): + response = make_response(200, json_body={"id": 1}) + self.assertEqual({"id": 1}, ClientBase._handle_response(response)) + + def test_success_text_returns_text(self): + response = make_response(200, text_body="plain text", content_type="text/plain") + self.assertEqual("plain text", ClientBase._handle_response(response)) + + def test_success_empty_returns_response(self): + response = make_response(204) + self.assertIs(response, ClientBase._handle_response(response)) + + def test_json_error_raises_api_error_with_details(self): + response = make_response( + 400, json_body={"message": "bad request", "errors": ["name is required"]} + ) + with self.assertRaises(ApiError) as cm: + ClientBase._handle_response(response) + self.assertEqual(400, cm.exception.response_status_code) + self.assertIs(response, cm.exception.response) + self.assertIn("bad request", str(cm.exception)) + self.assertIn("name is required", str(cm.exception)) + + def test_error_without_content_type_header_raises_api_error(self): + # previously raised KeyError when the Content-Type header was missing + response = make_response(500, text_body="Internal Server Error") + with self.assertRaises(ApiError) as cm: + ClientBase._handle_response(response) + self.assertEqual(500, cm.exception.response_status_code) + + def test_error_with_malformed_json_body_raises_api_error(self): + # Content-Type says JSON but the body does not parse + response = make_response( + 502, text_body="Bad Gateway", content_type="application/json" + ) + with self.assertRaises(ApiError) as cm: + ClientBase._handle_response(response) + self.assertIn("Bad Gateway", str(cm.exception)) + + def test_401_json_raises_authentication_error(self): + response = make_response(401, json_body={"message": "invalid key"}) + with self.assertRaises(AuthenticationError) as cm: + ClientBase._handle_response(response) + self.assertIn("invalid key", str(cm.exception)) + + def test_401_non_json_raises_authentication_error(self): + # previously crashed with a JSON decode error, e.g. a 401 from a proxy + response = make_response( + 401, text_body="Unauthorized", content_type="text/html" + ) + with self.assertRaises(AuthenticationError): + ClientBase._handle_response(response) + + +class ExceptionHierarchyTest(unittest.TestCase): + def test_all_inherit_from_rspace_error(self): + for exception_class in ( + ApiError, + AuthenticationError, + NoSuchLinkRel, + RSpaceConnectionError, + ): + self.assertTrue(issubclass(exception_class, RSpaceError)) + + def test_deprecated_nested_aliases_are_same_classes(self): + self.assertIs(ClientBase.ApiError, ApiError) + self.assertIs(ClientBase.AuthenticationError, AuthenticationError) + self.assertIs(ClientBase.NoSuchLinkRel, NoSuchLinkRel) + self.assertIs(ClientBase.ConnectionError, RSpaceConnectionError) + + def test_catching_via_deprecated_alias_still_works(self): + with self.assertRaises(ClientBase.ApiError): + raise ApiError("boom", response_status_code=418) + + def test_exceptions_exported_from_package_root(self): + self.assertIs(rspace_client.ApiError, ApiError) + self.assertIs(rspace_client.RSpaceError, RSpaceError) + + def test_api_error_carries_response_object(self): + response = make_response(503, text_body="unavailable") + error = ApiError("failed", response_status_code=503, response=response) + self.assertIs(response, error.response) + self.assertEqual(503, error.response_status_code) + + +if __name__ == "__main__": + unittest.main() From 1a90c2dfdaf845a908a47c8c04026084ad91983c Mon Sep 17 00:00:00 2001 From: nebay-abraha Date: Mon, 13 Jul 2026 07:43:13 +0100 Subject: [PATCH 2/5] refactor http client to use shared session and retry policy --- pyproject.toml | 1 + rspace_client/client_base.py | 81 ++++++++- rspace_client/tests/eln_fs_test.py | 24 +-- rspace_client/tests/http_layer_test.py | 160 ++++++++++++++++++ rspace_client/tests/inv_attachment_fs_test.py | 16 +- 5 files changed, 258 insertions(+), 24 deletions(-) create mode 100644 rspace_client/tests/http_layer_test.py diff --git a/pyproject.toml b/pyproject.toml index ed05d22..a808117 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -31,6 +31,7 @@ setuptools = "<82" python-dotenv = "^1.1.1" black = "^21.6b0" pytest = "^8.0.0" +responses = "^0.26.2" [build-system] requires = ["poetry-core>=1.0.0"] diff --git a/rspace_client/client_base.py b/rspace_client/client_base.py index 5fc5e22..d0efc50 100644 --- a/rspace_client/client_base.py +++ b/rspace_client/client_base.py @@ -2,6 +2,9 @@ import requests import sys +from requests.adapters import HTTPAdapter +from urllib3.util.retry import Retry + from rspace_client.exceptions import ( ApiError, AuthenticationError, @@ -10,6 +13,26 @@ RSpaceError, ) +DEFAULT_TIMEOUT = (3.05, 30) # (connect, read) seconds +DEFAULT_MAX_RETRIES = 3 +DEFAULT_BACKOFF_FACTOR = 0.5 +RETRY_STATUSES = (429, 500, 502, 503, 504) + + +class _RSpaceRetry(Retry): + """ + Retry policy that additionally retries POST requests, but only on + statuses where the server rejected the request before processing it + (rate limiting / unavailable), so a replay cannot create duplicates. + """ + + POST_RETRYABLE_STATUSES = frozenset({429, 503}) + + def is_retry(self, method, status_code, has_retry_after=False): + if method and method.upper() == "POST": + return status_code in self.POST_RETRYABLE_STATUSES + return super().is_retry(method, status_code, has_retry_after) + class Pagination: """ @@ -34,13 +57,59 @@ def __init__( class ClientBase: """Base class of common methods for all API clients""" - def __init__(self, rspace_url, api_key): + def __init__( + self, + rspace_url, + api_key, + timeout=DEFAULT_TIMEOUT, + max_retries=DEFAULT_MAX_RETRIES, + backoff_factor=DEFAULT_BACKOFF_FACTOR, + ): """ Initializes RSpace client. :param api_key: RSpace API key of a user can be found on 'My Profile' page + :param timeout: seconds to wait for the server, either a single number + or a (connect, read) tuple, applied to every request + :param max_retries: how many times to retry a request that failed with + a retryable status (429/5xx) or a connection error. Set to 0 to + disable retries. + :param backoff_factor: multiplier for exponential backoff between + retries; the Retry-After header is honored when the server sends it """ self.rspace_url = rspace_url.rstrip('/') self.api_key = api_key + self.timeout = timeout + self._session = self._build_session(max_retries, backoff_factor) + + def _build_session(self, max_retries, backoff_factor): + session = requests.Session() + retry = _RSpaceRetry( + total=max_retries, + backoff_factor=backoff_factor, + status_forcelist=RETRY_STATUSES, + allowed_methods=("GET", "PUT", "DELETE"), + respect_retry_after_header=True, + # once retries are exhausted, return the last response so it is + # mapped to ApiError as usual instead of raising RetryError + raise_on_status=False, + ) + adapter = HTTPAdapter(max_retries=retry) + session.mount("https://", adapter) + session.mount("http://", adapter) + session.headers["apiKey"] = self.api_key + return session + + def close(self): + """ + Closes the underlying HTTP session and its pooled connections. + """ + self._session.close() + + def __enter__(self): + return self + + def __exit__(self, exc_type, exc_value, traceback): + self.close() def _get_headers(self, content_type="application/json"): return {"apiKey": self.api_key, "Accept": content_type} @@ -141,14 +210,16 @@ def retrieve_api_results( headers = self._get_headers(content_type) try: if request_type == "GET": - response = requests.get(url, params=params, headers=headers) + response = self._session.get( + url, params=params, headers=headers, timeout=self.timeout + ) elif ( request_type == "PUT" or request_type == "POST" or request_type == "DELETE" ): - response = requests.request( - request_type, url, json=params, headers=headers + response = self._session.request( + request_type, url, json=params, headers=headers, timeout=self.timeout ) else: raise ValueError( @@ -158,7 +229,7 @@ def retrieve_api_results( ) return self._handle_response(response) - except requests.exceptions.ConnectionError as e: + except (requests.exceptions.ConnectionError, requests.exceptions.Timeout) as e: raise RSpaceConnectionError(e) @staticmethod diff --git a/rspace_client/tests/eln_fs_test.py b/rspace_client/tests/eln_fs_test.py index 4c2f17e..95d7b07 100644 --- a/rspace_client/tests/eln_fs_test.py +++ b/rspace_client/tests/eln_fs_test.py @@ -93,7 +93,7 @@ def mock_requests_post(url, *args, **kwargs): class ElnFilesystemTest(unittest.TestCase): - @patch('requests.get', side_effect=mock_requests_get) + @patch('requests.Session.get', side_effect=mock_requests_get) def setUp(self, mock_get) -> None: super().setUp() self.fs = GalleryFilesystem("https://example.com", "api_key") @@ -104,52 +104,54 @@ def test_path_to_id(self): self.assertEqual("456", path_to_id("GF123/GF456")) self.assertEqual("456", path_to_id("/GF123/GF456")) - @patch('requests.get', side_effect=mock_requests_get) + @patch('requests.Session.get', side_effect=mock_requests_get) def test_get_info_folder(self, mock_get): folder_info = self.fs.getinfo("GF123") self.assertEqual("GF123", folder_info.raw["basic"]["name"]) self.assertTrue(folder_info.raw["basic"]["is_dir"]) self.assertEqual(0, folder_info.raw["details"]["size"]) - @patch('requests.get', side_effect=mock_requests_get) + @patch('requests.Session.get', side_effect=mock_requests_get) def test_get_info_file(self, mock_get): file_info = self.fs.getinfo("GL123") self.assertEqual("GL123", file_info.raw["basic"]["name"]) self.assertFalse(file_info.raw["basic"]["is_dir"]) self.assertEqual(1024, file_info.raw["details"]["size"]) - @patch('requests.get', side_effect=mock_requests_get) + @patch('requests.Session.get', side_effect=mock_requests_get) def test_listdir_root(self, mock_list_folder_tree): result = self.fs.listdir('/') expected = ['GF123', 'GL456', 'GF789', 'GL012'] self.assertEqual(result, expected) - @patch('requests.get', side_effect=mock_requests_get) + @patch('requests.Session.get', side_effect=mock_requests_get) def test_listdir_root_specific_folder(self, mock_list_folder_tree): expected = ['GF123', 'GL456', 'GF789', 'GL012'] result = self.fs.listdir('GF123') self.assertEqual(result, expected) - @patch('requests.request', side_effect=mock_requests_post) - @patch('requests.get', side_effect=mock_requests_get) + @patch('requests.Session.request', side_effect=mock_requests_post) + @patch('requests.Session.get', side_effect=mock_requests_get) def test_makedir(self, mock_get, mock_post): self.fs.makedir('GF123/newFolder') mock_post.assert_called_once_with( 'POST', 'https://example.com/api/v1/folders', json={'name': 'newFolder', 'parentFolderId': 123, 'notebook': False}, - headers=ANY + headers=ANY, + timeout=ANY ) - @patch('requests.request', side_effect=mock_requests_post) - @patch('requests.get', side_effect=mock_requests_get) + @patch('requests.Session.request', side_effect=mock_requests_post) + @patch('requests.Session.get', side_effect=mock_requests_get) def test_removedir(self, mock_get, mock_post): self.fs.removedir('GF456') mock_post.assert_called_once_with( 'DELETE', 'https://example.com/api/v1/folders/456', json=ANY, - headers=ANY + headers=ANY, + timeout=ANY ) @patch('requests.get') diff --git a/rspace_client/tests/http_layer_test.py b/rspace_client/tests/http_layer_test.py new file mode 100644 index 0000000..6cdb0fb --- /dev/null +++ b/rspace_client/tests/http_layer_test.py @@ -0,0 +1,160 @@ +#!/usr/bin/env python3 +# -*- coding: utf-8 -*- +""" +Unit tests for the HTTP layer of ClientBase: session construction, retry +policy, timeout propagation and exception mapping. + +The retry loop itself runs inside urllib3 (below the adapter layer that the +responses library mocks), so the retry *policy* is tested directly on +_RSpaceRetry while request/response behavior is tested through responses. +""" +import unittest +from unittest import mock + +import requests +import responses + +from rspace_client.client_base import ( + ClientBase, + DEFAULT_MAX_RETRIES, + DEFAULT_TIMEOUT, + RETRY_STATUSES, + _RSpaceRetry, +) +from rspace_client.eln.eln import ELNClient +from rspace_client.exceptions import ApiError, RSpaceConnectionError +from rspace_client.tests.exceptions_test import make_response + +RSPACE_URL = "https://example.com" +API_URL = RSPACE_URL + "/api/v1" + + +def make_retry(**kwargs): + defaults = dict( + total=DEFAULT_MAX_RETRIES, + status_forcelist=RETRY_STATUSES, + allowed_methods=("GET", "PUT", "DELETE"), + raise_on_status=False, + ) + defaults.update(kwargs) + return _RSpaceRetry(**defaults) + + +class RetryPolicyTest(unittest.TestCase): + def test_idempotent_methods_retry_on_all_listed_statuses(self): + retry = make_retry() + for method in ("GET", "PUT", "DELETE"): + for status in RETRY_STATUSES: + self.assertTrue( + retry.is_retry(method, status), + "{} {} should be retried".format(method, status), + ) + + def test_post_retries_only_when_server_rejected_before_processing(self): + retry = make_retry() + self.assertTrue(retry.is_retry("POST", 429)) + self.assertTrue(retry.is_retry("POST", 503)) + for status in (500, 502, 504): + self.assertFalse( + retry.is_retry("POST", status), + "POST {} must not be retried: the server may have processed " + "the request, so a replay could create duplicates".format(status), + ) + + def test_success_statuses_are_not_retried(self): + retry = make_retry() + for method in ("GET", "POST", "PUT", "DELETE"): + self.assertFalse(retry.is_retry(method, 200)) + + +class SessionConfigTest(unittest.TestCase): + def setUp(self): + self.client = ELNClient(RSPACE_URL, "fake-api-key") + + def tearDown(self): + self.client.close() + + def test_session_has_api_key_header(self): + self.assertEqual("fake-api-key", self.client._session.headers["apiKey"]) + + def test_retry_adapter_mounted_for_both_schemes(self): + for scheme in ("https://", "http://"): + adapter = self.client._session.get_adapter(scheme + "example.com") + retry = adapter.max_retries + self.assertIsInstance(retry, _RSpaceRetry) + self.assertEqual(DEFAULT_MAX_RETRIES, retry.total) + self.assertFalse(retry.raise_on_status) + self.assertTrue(retry.respect_retry_after_header) + + def test_client_is_a_context_manager(self): + with ELNClient(RSPACE_URL, "fake-api-key") as client: + self.assertIsNotNone(client._session) + + +class TimeoutPropagationTest(unittest.TestCase): + def test_default_timeout_passed_to_get(self): + client = ELNClient(RSPACE_URL, "fake-api-key") + with mock.patch.object( + client._session, "get", return_value=make_response(200, json_body={}) + ) as mocked_get: + client.retrieve_api_results("/status") + self.assertEqual(DEFAULT_TIMEOUT, mocked_get.call_args.kwargs["timeout"]) + + def test_constructor_timeout_passed_to_non_get(self): + client = ELNClient(RSPACE_URL, "fake-api-key", timeout=(5, 120)) + with mock.patch.object( + client._session, "request", return_value=make_response(200, json_body={}) + ) as mocked_request: + client.retrieve_api_results("/forms", request_type="POST") + self.assertEqual((5, 120), mocked_request.call_args.kwargs["timeout"]) + + +class ExceptionMappingTest(unittest.TestCase): + def setUp(self): + self.client = ELNClient(RSPACE_URL, "fake-api-key") + + def tearDown(self): + self.client.close() + + @responses.activate + def test_http_error_maps_to_api_error(self): + responses.add( + responses.GET, + API_URL + "/documents", + json={"message": "not found", "errors": []}, + status=404, + ) + with self.assertRaises(ApiError) as cm: + self.client.retrieve_api_results("/documents") + self.assertEqual(404, cm.exception.response_status_code) + + @responses.activate + def test_connection_error_maps_to_rspace_connection_error(self): + responses.add( + responses.GET, + API_URL + "/status", + body=requests.exceptions.ConnectionError("refused"), + ) + with self.assertRaises(RSpaceConnectionError): + self.client.retrieve_api_results("/status") + + @responses.activate + def test_read_timeout_maps_to_rspace_connection_error(self): + responses.add( + responses.GET, + API_URL + "/status", + body=requests.exceptions.ReadTimeout("timed out"), + ) + with self.assertRaises(RSpaceConnectionError): + self.client.retrieve_api_results("/status") + + @responses.activate + def test_successful_json_call_returns_dict(self): + responses.add( + responses.GET, API_URL + "/status", json={"message": "OK"}, status=200 + ) + self.assertEqual({"message": "OK"}, self.client.retrieve_api_results("/status")) + + +if __name__ == "__main__": + unittest.main() diff --git a/rspace_client/tests/inv_attachment_fs_test.py b/rspace_client/tests/inv_attachment_fs_test.py index b730b0a..a2b09c9 100644 --- a/rspace_client/tests/inv_attachment_fs_test.py +++ b/rspace_client/tests/inv_attachment_fs_test.py @@ -57,22 +57,22 @@ def mock_requests(url, *args, **kwargs): class InvAttachmentFilesystemTest(unittest.TestCase): - @patch('requests.get', side_effect=mock_requests_get) + @patch('requests.Session.get', side_effect=mock_requests_get) def setUp(self, mock_get) -> None: super().setUp() self.fs = InventoryAttachmentFilesystem("https://example.com", "api_key") - @patch('requests.get', side_effect=mock_requests_get) + @patch('requests.Session.get', side_effect=mock_requests_get) def test_get_info_folder(self, mock_get): folder_info = self.fs.getinfo("IF123") self.assertEqual("IF123", folder_info.raw["basic"]["name"]) self.assertFalse(folder_info.raw["basic"]["is_dir"]) self.assertEqual(40721, folder_info.raw["details"]["size"]) - @patch('requests.request', side_effect=mock_requests) + @patch('requests.Session.request', side_effect=mock_requests) def test_remove(self, mock_request): self.fs.remove("IF123") - mock_request.assert_called_with('DELETE', 'https://example.com/api/inventory/v1/files/123', json=None, headers=ANY) + mock_request.assert_called_with('DELETE', 'https://example.com/api/inventory/v1/files/123', json=None, headers=ANY, timeout=ANY) @patch('requests.get') def test_download(self, mock_get): @@ -102,22 +102,22 @@ def test_upload(self, mock_post): headers=ANY ) - @patch('requests.get', side_effect=mock_requests_get) + @patch('requests.Session.get', side_effect=mock_requests_get) def test_listdir_container(self, mock_get): result = self.fs.listdir('/IC123') self.assertEqual(result, ['IF32772']) - @patch('requests.get', side_effect=mock_requests_get) + @patch('requests.Session.get', side_effect=mock_requests_get) def test_listdir_sample(self, mock_get): result = self.fs.listdir('/SA123') self.assertEqual(result, ['IF32772']) - @patch('requests.get', side_effect=mock_requests_get) + @patch('requests.Session.get', side_effect=mock_requests_get) def test_listdir_subsample(self, mock_get): result = self.fs.listdir('/SS123') self.assertEqual(result, ['IF32772']) - @patch('requests.get', side_effect=mock_requests_get) + @patch('requests.Session.get', side_effect=mock_requests_get) def test_listdir_sample_template(self, mock_get): result = self.fs.listdir('/IT123') self.assertEqual(result, ['IF32772']) From 5355caced9993225b78816d1d743d1924dc73910 Mon Sep 17 00:00:00 2001 From: nebay-abraha Date: Mon, 13 Jul 2026 07:52:04 +0100 Subject: [PATCH 3/5] Unify upload and download request handling --- rspace_client/client_base.py | 53 ++++++++++++++- rspace_client/eln/eln.py | 24 ++----- rspace_client/inv/inv.py | 39 +++-------- rspace_client/tests/eln_fs_test.py | 5 +- rspace_client/tests/http_layer_test.py | 68 +++++++++++++++++++ rspace_client/tests/inv_attachment_fs_test.py | 5 +- 6 files changed, 141 insertions(+), 53 deletions(-) diff --git a/rspace_client/client_base.py b/rspace_client/client_base.py index d0efc50..65256ec 100644 --- a/rspace_client/client_base.py +++ b/rspace_client/client_base.py @@ -156,7 +156,11 @@ def _get_error_detail(response): return "error message: {}".format(response.text) @staticmethod - def _handle_response(response): + def _check_status(response): + """ + Raises AuthenticationError on 401 and ApiError on any other HTTP + error status; returns None for successful responses. + """ if response.status_code == 401: raise AuthenticationError( "Error code: 401, {}".format(ClientBase._get_error_detail(response)) @@ -173,6 +177,10 @@ def _handle_response(response): response=response, ) + @staticmethod + def _handle_response(response): + ClientBase._check_status(response) + if ClientBase._responseContainsJson(response): return response.json() elif response.text: @@ -232,6 +240,49 @@ def retrieve_api_results( except (requests.exceptions.ConnectionError, requests.exceptions.Timeout) as e: raise RSpaceConnectionError(e) + def _post_multipart(self, endpoint, files=None, data=None): + """ + POSTs a multipart/form-data request (typically a file upload) through + the shared session, so timeout, retries and error handling behave the + same as for JSON API calls. + :param endpoint: full URL or path relative to the API root + :param files: dict of multipart file parts, passed to requests + :param data: dict of additional form fields + :return: parsed response, as for retrieve_api_results + """ + url = endpoint + if not endpoint.startswith(self._get_api_url()): + url = self._get_api_url() + endpoint + try: + response = self._session.post( + url, + files=files, + data=data, + headers=self._get_headers(), + timeout=self.timeout, + ) + return self._handle_response(response) + except (requests.exceptions.ConnectionError, requests.exceptions.Timeout) as e: + raise RSpaceConnectionError(e) + + def _get_raw_response(self, url, params=None, accept="application/octet-stream"): + """ + GETs a URL through the shared session and returns the raw response, + for binary content such as images. Raises AuthenticationError or + ApiError on HTTP error statuses, like retrieve_api_results. + """ + try: + response = self._session.get( + url, + params=params, + headers={"apiKey": self.api_key, "Accept": accept}, + timeout=self.timeout, + ) + except (requests.exceptions.ConnectionError, requests.exceptions.Timeout) as e: + raise RSpaceConnectionError(e) + self._check_status(response) + return response + @staticmethod def _get_links(response): """ diff --git a/rspace_client/eln/eln.py b/rspace_client/eln/eln.py index a9b3b0e..d969158 100644 --- a/rspace_client/eln/eln.py +++ b/rspace_client/eln/eln.py @@ -1,7 +1,6 @@ import datetime import time import os -import requests import rspace_client.eln.filetree_importer as importer from rspace_client.eln.dcs import DocumentCreationStrategy @@ -366,13 +365,7 @@ def upload_file(self, file, folder_id=None, caption=None): if caption is not None: data["caption"] = caption - response = requests.post( - self._get_api_url() + "/files", - files={"file": file}, - data=data, - headers=self._get_headers(), - ) - return self._handle_response(response) + return self._post_multipart("/files", files={"file": file}, data=data) def update_file(self, file, fileId): """ @@ -382,12 +375,9 @@ def update_file(self, file, fileId): :param fileId: Id of the file to replace :return: updated File response as a dictionary """ - response = requests.post( - self._get_api_url() + "/files/{}/file".format(fileId), - files={"file": file}, - headers=self._get_headers(), + return self._post_multipart( + "/files/{}/file".format(fileId), files={"file": file} ) - return self._handle_response(response) # Activity methods def get_activity( @@ -856,13 +846,7 @@ def import_word(self, file, folder_id=None, image_folder_id=None): numeric_imagefolder_id = self._get_numeric_record_id(image_folder_id) data["imageFolderId"] = numeric_imagefolder_id - response = requests.post( - self._get_api_url() + "/import/word", - files={"file": file}, - data=data, - headers=self._get_headers(), - ) - return self._handle_response(response) + return self._post_multipart("/import/word", files={"file": file}, data=data) # Miscellaneous methods def get_status(self): diff --git a/rspace_client/inv/inv.py b/rspace_client/inv/inv.py index 5ea0b9d..8501d53 100644 --- a/rspace_client/inv/inv.py +++ b/rspace_client/inv/inv.py @@ -7,7 +7,6 @@ import sys, io, base64 import requests import pprint -import requests from typing import Optional, Sequence, Union, List, TypedDict, BinaryIO from rspace_client.client_base import ClientBase, Pagination @@ -1781,13 +1780,10 @@ def upload_attachment(self, inventory_item: Union[str, dict], file) -> dict: global_id = Id(inventory_item) fs = {"parentGlobalId": global_id.as_global_id()} fsStr = json.dumps(fs) - headers = self._get_headers() - response = requests.post( - self._get_api_url() + "/files", + return self._post_multipart( + "/files", files={"file": file, "fileSettings": (None, fsStr, "application/json")}, - headers=headers, ) - return self._handle_response(response) def delete_attachment_by_id(self, attachment_id: Union[str, int]) -> None: """ @@ -1809,14 +1805,11 @@ def download_attachment_by_id(self, attachment_id: Union[str, int], file_path: U ) def upload_attachment_by_global_id(self, record_global_id: str, file: BinaryIO) -> None: - print(record_global_id, json.dumps({"parentGlobalId": record_global_id})) - response = requests.post( - self._get_api_url() + "/files", + return self._post_multipart( + "/files", + files={"file": file}, data={"fileSettings": json.dumps({"parentGlobalId": record_global_id})}, - files={"file": file }, - headers=self._get_headers(), ) - return self._handle_response(response) def split_subsample( self, @@ -2509,13 +2502,9 @@ def set_sample_template_icon(self, sample_template_id: Union[int, str], file): """ st_id = Id(sample_template_id) - headers = self._get_headers() - response = requests.post( - f"{self._get_api_url()}/sampleTemplates/{st_id.as_id()}/icon", - files={"file": file}, - headers=headers, + return self._post_multipart( + f"/sampleTemplates/{st_id.as_id()}/icon", files={"file": file} ) - return self._handle_response(response) def get_sample_template_icon( self, sample_template_id: Union[int, str], icon_id: int, outfile @@ -2673,13 +2662,9 @@ def set_instrument_template_icon( """ it_id = Id(instrument_template_id) - headers = self._get_headers() - response = requests.post( - f"{self._get_api_url()}/instrumentTemplates/{it_id.as_id()}/icon", - files={"file": file}, - headers=headers, + return self._post_multipart( + f"/instrumentTemplates/{it_id.as_id()}/icon", files={"file": file} ) - return self._handle_response(response) def get_instrument_template_icon( self, instrument_template_id: Union[int, str], icon_id: int, outfile @@ -2851,10 +2836,8 @@ def barcode( Id(global_id) ## validate is identifier data = {"content": global_id, "barcodeType": barcode_type.name} url = f"{self._get_api_url()}/barcodes" - headers = {"apiKey": self.api_key, "Accept": "image/png"} - resp = requests.get(url, headers=headers, params=data) - resp.raise_for_status() + resp = self._get_raw_response(url, params=data, accept="image/png") content = resp.content if outfile is not None: with open(outfile, "wb") as fd: @@ -2957,7 +2940,7 @@ def test_datacite_connection(self, provider: str = "IGSN_DATACITE") -> bool: headers = self._get_headers("application/json") try: - response = requests.get(url, headers=headers) + response = self._session.get(url, headers=headers, timeout=self.timeout) if response.status_code != 200: return False return bool(response.json()) diff --git a/rspace_client/tests/eln_fs_test.py b/rspace_client/tests/eln_fs_test.py index 95d7b07..9e0ca19 100644 --- a/rspace_client/tests/eln_fs_test.py +++ b/rspace_client/tests/eln_fs_test.py @@ -168,7 +168,7 @@ def test_download(self, mock_get): headers=ANY ) - @patch('requests.post') + @patch('requests.Session.post') def test_upload(self, mock_post): mock_response = MagicMock() mock_response.json.return_value = {'id': '456'} @@ -179,7 +179,8 @@ def test_upload(self, mock_post): 'https://example.com/api/v1/files', files={'file': file_obj}, data={'folderId': 123}, - headers=ANY + headers=ANY, + timeout=ANY ) if __name__ == '__main__': diff --git a/rspace_client/tests/http_layer_test.py b/rspace_client/tests/http_layer_test.py index 6cdb0fb..3e73780 100644 --- a/rspace_client/tests/http_layer_test.py +++ b/rspace_client/tests/http_layer_test.py @@ -156,5 +156,73 @@ def test_successful_json_call_returns_dict(self): self.assertEqual({"message": "OK"}, self.client.retrieve_api_results("/status")) +class MultipartHelperTest(unittest.TestCase): + def setUp(self): + self.client = ELNClient(RSPACE_URL, "fake-api-key") + + def tearDown(self): + self.client.close() + + @responses.activate + def test_upload_success_returns_parsed_json(self): + responses.add( + responses.POST, API_URL + "/files", json={"id": 123}, status=200 + ) + result = self.client._post_multipart( + "/files", files={"file": ("f.txt", b"content")}, data={"folderId": 1} + ) + self.assertEqual({"id": 123}, result) + + @responses.activate + def test_upload_error_maps_to_api_error_like_json_calls(self): + responses.add( + responses.POST, + API_URL + "/files", + json={"message": "file too large", "errors": []}, + status=413, + ) + with self.assertRaises(ApiError) as cm: + self.client._post_multipart("/files", files={"file": ("f.txt", b"x")}) + self.assertEqual(413, cm.exception.response_status_code) + + def test_upload_passes_timeout(self): + with mock.patch.object( + self.client._session, "post", return_value=make_response(200, json_body={}) + ) as mocked_post: + self.client._post_multipart("/files", files={"file": ("f.txt", b"x")}) + self.assertEqual(DEFAULT_TIMEOUT, mocked_post.call_args.kwargs["timeout"]) + + +class RawResponseHelperTest(unittest.TestCase): + def setUp(self): + self.client = ELNClient(RSPACE_URL, "fake-api-key") + + def tearDown(self): + self.client.close() + + @responses.activate + def test_binary_get_returns_raw_response(self): + responses.add( + responses.GET, + API_URL + "/barcodes", + body=b"\x89PNG binary", + status=200, + content_type="image/png", + ) + response = self.client._get_raw_response( + API_URL + "/barcodes", accept="image/png" + ) + self.assertEqual(b"\x89PNG binary", response.content) + + @responses.activate + def test_binary_get_error_maps_to_api_error(self): + responses.add( + responses.GET, API_URL + "/barcodes", body="nope", status=404 + ) + with self.assertRaises(ApiError) as cm: + self.client._get_raw_response(API_URL + "/barcodes") + self.assertEqual(404, cm.exception.response_status_code) + + if __name__ == "__main__": unittest.main() diff --git a/rspace_client/tests/inv_attachment_fs_test.py b/rspace_client/tests/inv_attachment_fs_test.py index a2b09c9..893b87c 100644 --- a/rspace_client/tests/inv_attachment_fs_test.py +++ b/rspace_client/tests/inv_attachment_fs_test.py @@ -88,7 +88,7 @@ def test_download(self, mock_get): headers=ANY ) - @patch('requests.post') + @patch('requests.Session.post') def test_upload(self, mock_post): mock_response = MagicMock() mock_response.json.return_value = {'id': '456'} @@ -99,7 +99,8 @@ def test_upload(self, mock_post): 'https://example.com/api/inventory/v1/files', data={'fileSettings': '{"parentGlobalId": "SS123"}'}, files={'file': file_obj}, - headers=ANY + headers=ANY, + timeout=ANY ) @patch('requests.Session.get', side_effect=mock_requests_get) From 6f53a4d2381219d40ab5cc3a4ccff50547c61f4a Mon Sep 17 00:00:00 2001 From: nebay-abraha Date: Mon, 13 Jul 2026 07:59:58 +0100 Subject: [PATCH 4/5] improve download streaming and library metadata handling --- rspace_client/__init__.py | 7 ++ rspace_client/client_base.py | 67 +++++++++------ rspace_client/tests/eln_fs_test.py | 8 +- rspace_client/tests/http_layer_test.py | 82 +++++++++++++++++++ rspace_client/tests/inv_attachment_fs_test.py | 8 +- 5 files changed, 141 insertions(+), 31 deletions(-) diff --git a/rspace_client/__init__.py b/rspace_client/__init__.py index 03b8c98..3dc25c7 100644 --- a/rspace_client/__init__.py +++ b/rspace_client/__init__.py @@ -1,6 +1,13 @@ """ RSpace API client for interacting with RSpace ELN and Inventory """ +from importlib.metadata import PackageNotFoundError, version + +try: + __version__ = version("rspace-client") +except PackageNotFoundError: + __version__ = "unknown" + from .eln.eln import ELNClient from .inv.inv import InventoryClient from .eln.advanced_query_builder import AdvancedQueryBuilder diff --git a/rspace_client/client_base.py b/rspace_client/client_base.py index 65256ec..d939e7a 100644 --- a/rspace_client/client_base.py +++ b/rspace_client/client_base.py @@ -1,6 +1,8 @@ +import logging import re import requests -import sys + +from typing import Optional from requests.adapters import HTTPAdapter from urllib3.util.retry import Retry @@ -13,6 +15,8 @@ RSpaceError, ) +logger = logging.getLogger("rspace_client") + DEFAULT_TIMEOUT = (3.05, 30) # (connect, read) seconds DEFAULT_MAX_RETRIES = 3 DEFAULT_BACKOFF_FACTOR = 0.5 @@ -321,25 +325,29 @@ def get_link(self, response, link_rel): ) ) - def download_link_to_file(self, url, filename, chunk_size=128): + def download_link_to_file(self, url, filename, chunk_size=8192): """ - Downloads a file from the API server. + Downloads a file from the API server, streaming it to disk in chunks + so large files are not buffered in memory. :param url: URL of the file to be downloaded :param filename: file path to save the file to or an already opened file object - :param chunk_size: size of the chunks to download at a time, default is 128 + :param chunk_size: size of the chunks to download at a time, default is 8192 """ headers = {"apiKey": self.api_key, "Accept": "application/octet-stream"} - if isinstance(filename, str): - with open(filename, "wb") as fd: - for chunk in requests.get(url, headers=headers).iter_content( - chunk_size=chunk_size - ): - fd.write(chunk) - else: - for chunk in requests.get(url, headers=headers).iter_content( - chunk_size=chunk_size - ): - filename.write(chunk) + try: + with self._session.get( + url, headers=headers, stream=True, timeout=self.timeout + ) as response: + self._check_status(response) + if isinstance(filename, str): + with open(filename, "wb") as fd: + for chunk in response.iter_content(chunk_size=chunk_size): + fd.write(chunk) + else: + for chunk in response.iter_content(chunk_size=chunk_size): + filename.write(chunk) + except (requests.exceptions.ConnectionError, requests.exceptions.Timeout) as e: + raise RSpaceConnectionError(e) def link_exists(self, response, link_rel): """ @@ -351,12 +359,17 @@ def link_exists(self, response, link_rel): return link_rel in [x["rel"] for x in self._get_links(response)] def serr(self, msg: str): - print(msg, file=sys.stderr) + """ + Deprecated: logs a warning through the 'rspace_client' logger instead + of printing to stderr. Use the logging module directly; this method + will be removed in 3.0. + """ + logger.warning(msg) def _stream( self, endpoint: str, - pagination: Pagination = Pagination(), + pagination: Optional[Pagination] = None, ): """ Yields items, making paginated requests to the server as each page @@ -371,25 +384,25 @@ def _stream( An endpoint with a GET request that makes paginated listings pagination : Pagination, optional The pagination control. The default is Pagination(). - : Pagination Yields ------ item : A stream of items, depending on the endpoint called """ + if pagination is None: + pagination = Pagination() urlStr = f"{self._get_api_url()}/{endpoint}" next_link = requests.Request(url=urlStr, params=pagination.data).prepare().url - while True: - if next_link is not None: - items = self.retrieve_api_results(next_link) - for item in items[endpoint]: - yield item - if self.link_exists(items, "next"): - next_link = self.get_link(items, "next") - else: - break + while next_link is not None: + items = self.retrieve_api_results(next_link) + for item in items[endpoint]: + yield item + if self.link_exists(items, "next"): + next_link = self.get_link(items, "next") + else: + next_link = None # Deprecated aliases of the module-level classes in rspace_client.exceptions, # kept so existing `except ClientBase.ApiError:` code works. Remove in 3.0. diff --git a/rspace_client/tests/eln_fs_test.py b/rspace_client/tests/eln_fs_test.py index 9e0ca19..30c6750 100644 --- a/rspace_client/tests/eln_fs_test.py +++ b/rspace_client/tests/eln_fs_test.py @@ -154,10 +154,12 @@ def test_removedir(self, mock_get, mock_post): timeout=ANY ) - @patch('requests.get') + @patch('requests.Session.get') def test_download(self, mock_get): mock_response = MagicMock() + mock_response.status_code = 200 mock_response.iter_content = MagicMock(return_value=[b'chunk1', b'chunk2', b'chunk3']) + mock_response.__enter__ = MagicMock(return_value=mock_response) mock_get.return_value = mock_response file_obj = BytesIO() self.fs.download('/GL123', file_obj) @@ -165,7 +167,9 @@ def test_download(self, mock_get): self.assertEqual(file_obj.read(), b'chunk1chunk2chunk3') mock_get.assert_called_once_with( 'https://example.com/api/v1/files/123/file', - headers=ANY + headers=ANY, + stream=True, + timeout=ANY ) @patch('requests.Session.post') diff --git a/rspace_client/tests/http_layer_test.py b/rspace_client/tests/http_layer_test.py index 3e73780..ca6b712 100644 --- a/rspace_client/tests/http_layer_test.py +++ b/rspace_client/tests/http_layer_test.py @@ -9,11 +9,14 @@ _RSpaceRetry while request/response behavior is tested through responses. """ import unittest +from io import BytesIO from unittest import mock import requests import responses +import rspace_client + from rspace_client.client_base import ( ClientBase, DEFAULT_MAX_RETRIES, @@ -224,5 +227,84 @@ def test_binary_get_error_maps_to_api_error(self): self.assertEqual(404, cm.exception.response_status_code) +class DownloadTest(unittest.TestCase): + def setUp(self): + self.client = ELNClient(RSPACE_URL, "fake-api-key") + + def tearDown(self): + self.client.close() + + @responses.activate + def test_download_streams_to_file_object(self): + responses.add( + responses.GET, API_URL + "/files/1/file", body=b"file content", status=200 + ) + buffer = BytesIO() + self.client.download_link_to_file(API_URL + "/files/1/file", buffer) + self.assertEqual(b"file content", buffer.getvalue()) + + @responses.activate + def test_download_error_raises_api_error_and_writes_nothing(self): + # previously the error body was silently written into the target file + responses.add( + responses.GET, + API_URL + "/files/1/file", + json={"message": "not found", "errors": []}, + status=404, + ) + buffer = BytesIO() + with self.assertRaises(ApiError): + self.client.download_link_to_file(API_URL + "/files/1/file", buffer) + self.assertEqual(b"", buffer.getvalue()) + + def test_download_requests_streaming(self): + mock_response = mock.MagicMock() + mock_response.status_code = 200 + mock_response.__enter__ = mock.MagicMock(return_value=mock_response) + mock_response.iter_content.return_value = [b"x"] + with mock.patch.object( + self.client._session, "get", return_value=mock_response + ) as mocked_get: + self.client.download_link_to_file(API_URL + "/files/1/file", BytesIO()) + self.assertTrue(mocked_get.call_args.kwargs["stream"]) + self.assertEqual(DEFAULT_TIMEOUT, mocked_get.call_args.kwargs["timeout"]) + + +class StreamPaginationTest(unittest.TestCase): + def setUp(self): + self.client = ELNClient(RSPACE_URL, "fake-api-key") + + def tearDown(self): + self.client.close() + + def test_stream_pages_until_no_next_link(self): + page1 = { + "samples": [{"id": 1}, {"id": 2}], + "_links": [{"rel": "next", "link": API_URL + "/samples?pageNumber=1"}], + } + page2 = {"samples": [{"id": 3}], "_links": []} + with mock.patch.object( + self.client, "retrieve_api_results", side_effect=[page1, page2] + ) as mocked: + items = list(self.client._stream("samples")) + self.assertEqual([1, 2, 3], [item["id"] for item in items]) + self.assertEqual(2, mocked.call_count) + + +class MiscTest(unittest.TestCase): + def test_package_exposes_version(self): + self.assertTrue(hasattr(rspace_client, "__version__")) + self.assertNotEqual("", rspace_client.__version__) + + def test_serr_logs_warning_instead_of_printing(self): + client = ELNClient(RSPACE_URL, "fake-api-key") + try: + with self.assertLogs("rspace_client", level="WARNING") as logs: + client.serr("something went wrong") + self.assertIn("something went wrong", logs.output[0]) + finally: + client.close() + + if __name__ == "__main__": unittest.main() diff --git a/rspace_client/tests/inv_attachment_fs_test.py b/rspace_client/tests/inv_attachment_fs_test.py index 893b87c..0b22707 100644 --- a/rspace_client/tests/inv_attachment_fs_test.py +++ b/rspace_client/tests/inv_attachment_fs_test.py @@ -74,10 +74,12 @@ def test_remove(self, mock_request): self.fs.remove("IF123") mock_request.assert_called_with('DELETE', 'https://example.com/api/inventory/v1/files/123', json=None, headers=ANY, timeout=ANY) - @patch('requests.get') + @patch('requests.Session.get') def test_download(self, mock_get): mock_response = MagicMock() + mock_response.status_code = 200 mock_response.iter_content = MagicMock(return_value=[b'chunk1', b'chunk2', b'chunk3']) + mock_response.__enter__ = MagicMock(return_value=mock_response) mock_get.return_value = mock_response file_obj = BytesIO() self.fs.download('/IF123', file_obj) @@ -85,7 +87,9 @@ def test_download(self, mock_get): self.assertEqual(file_obj.read(), b'chunk1chunk2chunk3') mock_get.assert_called_once_with( 'https://example.com/api/inventory/v1/files/123/file', - headers=ANY + headers=ANY, + stream=True, + timeout=ANY ) @patch('requests.Session.post') From 07291556eeafb6e49ed6b2a1d9fbd811e3a374ce Mon Sep 17 00:00:00 2001 From: nebay-abraha Date: Mon, 13 Jul 2026 17:45:47 +0100 Subject: [PATCH 5/5] address copilot PR comments --- rspace_client/client_base.py | 13 ++++++++++--- rspace_client/exceptions.py | 2 +- rspace_client/inv/inv.py | 2 +- rspace_client/tests/http_layer_test.py | 24 ++++++++++++------------ 4 files changed, 24 insertions(+), 17 deletions(-) diff --git a/rspace_client/client_base.py b/rspace_client/client_base.py index d939e7a..695f60c 100644 --- a/rspace_client/client_base.py +++ b/rspace_client/client_base.py @@ -115,8 +115,15 @@ def __enter__(self): def __exit__(self, exc_type, exc_value, traceback): self.close() + @staticmethod + def _is_absolute_url(url): + return url.startswith("http://") or url.startswith("https://") + def _get_headers(self, content_type="application/json"): - return {"apiKey": self.api_key, "Accept": content_type} + headers = {"apiKey": self.api_key} + if content_type is not None: + headers["Accept"] = content_type + return headers @staticmethod def _get_numeric_record_id(global_id): @@ -216,7 +223,7 @@ def retrieve_api_results( :return: parsed JSON response as a dictionary """ url = endpoint - if not endpoint.startswith(self._get_api_url()): + if not self._is_absolute_url(endpoint): url = self._get_api_url() + endpoint headers = self._get_headers(content_type) @@ -255,7 +262,7 @@ def _post_multipart(self, endpoint, files=None, data=None): :return: parsed response, as for retrieve_api_results """ url = endpoint - if not endpoint.startswith(self._get_api_url()): + if not self._is_absolute_url(endpoint): url = self._get_api_url() + endpoint try: response = self._session.post( diff --git a/rspace_client/exceptions.py b/rspace_client/exceptions.py index 9c86659..833ad73 100644 --- a/rspace_client/exceptions.py +++ b/rspace_client/exceptions.py @@ -4,7 +4,7 @@ All exceptions raised by this library inherit from RSpaceError, so user code can catch anything the client raises with ``except rspace_client.RSpaceError``. -Before version 2.8.0 these lived as nested classes on ClientBase +In earlier releases these lived as nested classes on ClientBase (e.g. ``ClientBase.ApiError``). Those names remain as aliases of the classes defined here and will be removed in 3.0. """ diff --git a/rspace_client/inv/inv.py b/rspace_client/inv/inv.py index 8501d53..d763f45 100644 --- a/rspace_client/inv/inv.py +++ b/rspace_client/inv/inv.py @@ -1804,7 +1804,7 @@ def download_attachment_by_id(self, attachment_id: Union[str, int], file_path: U f"{url_base}/files/{attachment_id}/file", file_path, chunk_size ) - def upload_attachment_by_global_id(self, record_global_id: str, file: BinaryIO) -> None: + def upload_attachment_by_global_id(self, record_global_id: str, file: BinaryIO) -> dict: return self._post_multipart( "/files", files={"file": file}, diff --git a/rspace_client/tests/http_layer_test.py b/rspace_client/tests/http_layer_test.py index ca6b712..cc2f53e 100644 --- a/rspace_client/tests/http_layer_test.py +++ b/rspace_client/tests/http_layer_test.py @@ -96,20 +96,20 @@ def test_client_is_a_context_manager(self): class TimeoutPropagationTest(unittest.TestCase): def test_default_timeout_passed_to_get(self): - client = ELNClient(RSPACE_URL, "fake-api-key") - with mock.patch.object( - client._session, "get", return_value=make_response(200, json_body={}) - ) as mocked_get: - client.retrieve_api_results("/status") - self.assertEqual(DEFAULT_TIMEOUT, mocked_get.call_args.kwargs["timeout"]) + with ELNClient(RSPACE_URL, "fake-api-key") as client: + with mock.patch.object( + client._session, "get", return_value=make_response(200, json_body={}) + ) as mocked_get: + client.retrieve_api_results("/status") + self.assertEqual(DEFAULT_TIMEOUT, mocked_get.call_args.kwargs["timeout"]) def test_constructor_timeout_passed_to_non_get(self): - client = ELNClient(RSPACE_URL, "fake-api-key", timeout=(5, 120)) - with mock.patch.object( - client._session, "request", return_value=make_response(200, json_body={}) - ) as mocked_request: - client.retrieve_api_results("/forms", request_type="POST") - self.assertEqual((5, 120), mocked_request.call_args.kwargs["timeout"]) + with ELNClient(RSPACE_URL, "fake-api-key", timeout=(5, 120)) as client: + with mock.patch.object( + client._session, "request", return_value=make_response(200, json_body={}) + ) as mocked_request: + client.retrieve_api_results("/forms", request_type="POST") + self.assertEqual((5, 120), mocked_request.call_args.kwargs["timeout"]) class ExceptionMappingTest(unittest.TestCase):