From 3279d5bd5b10c28815f10592fe1e0fa6c4e1e36e Mon Sep 17 00:00:00 2001 From: jm Date: Wed, 5 Feb 2025 18:29:59 +0100 Subject: [PATCH 01/28] removed duplicate code --- dspace_rest_client/client.py | 33 ++++++++------------------------- 1 file changed, 8 insertions(+), 25 deletions(-) diff --git a/dspace_rest_client/client.py b/dspace_rest_client/client.py index 2633b19..3ee69bb 100644 --- a/dspace_rest_client/client.py +++ b/dspace_rest_client/client.py @@ -833,21 +833,6 @@ def create_collection(self, parent, data): params = {'parent': parent} return Collection(api_resource=parse_json(self.create_dso(url, params, data))) - def get_items(self): - """ - Get all items - @return: list of Item objects - """ - url = f'{self.API_ENDPOINT}/core/items' - items = list() - r = self.api_get(url) - r_json = parse_json(r) - if '_embedded' in r_json: - if 'items' in r_json['_embedded']: - for item_resource in r_json['_embedded']['items']: - items.append(Item(item_resource)) - return items - def get_item(self, uuid): """ Get an item, given its UUID @@ -864,29 +849,27 @@ def get_item(self, uuid): _logger.error(f'Invalid item UUID: {uuid}') return None - def get_items(self): + def get_items(self, page=0, size=20): """ Get all archived items for a logged-in administrator. Admin only! Usually you will want to use search or browse methods instead of this method @return: A list of items, or an error """ url = f'{self.API_ENDPOINT}/core/items' - # Empty item list - items = list() - # Perform the actual request - r_json = self.fetch_resource(url) - # Empty list items = list() + params = {} + if size is not None: + params['size'] = size + if page is not None: + params['page'] = page + r = self.api_get(url, params=params) + r_json = parse_json(response=r) if '_embedded' in r_json: - # This is a list of items if 'collections' in r_json['_embedded']: for item_resource in r_json['_embedded']['items']: items.append(Item(item_resource)) elif 'uuid' in r_json: - # This is a single item items.append(Item(r_json)) - - # Return list (populated or empty) return items def create_item(self, parent, item): From a906e75a301188410a0d9ddb98d7ec3908e755c9 Mon Sep 17 00:00:00 2001 From: jm Date: Sun, 16 Mar 2025 20:56:58 +0100 Subject: [PATCH 02/28] added remove_metadata and fixed get_item --- dspace_rest_client/client.py | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/dspace_rest_client/client.py b/dspace_rest_client/client.py index 3ee69bb..bab4bde 100644 --- a/dspace_rest_client/client.py +++ b/dspace_rest_client/client.py @@ -462,7 +462,7 @@ def create_dso(self, url, params, data): if r.status_code == 201: # 201 Created - success! new_dso = parse_json(r) - _logger.info(f'{new_dso["type"]} {new_dso["uuid"]} created successfully!') + _logger.info(f'Object type[{new_dso["type"]}] uuid:[{new_dso["uuid"]}] created successfully!') else: _logger.error(f'create operation failed: {r.status_code}: {r.text} ({url})') return r @@ -839,12 +839,13 @@ def get_item(self, uuid): @param uuid: the UUID of the item @return: the raw API response """ - # TODO - return constructed Item object instead, handling errors here? url = f'{self.API_ENDPOINT}/core/items' try: id = UUID(uuid).version url = f'{url}/{uuid}' - return self.api_get(url, None, None) + r = self.api_get(url, None, None) + r_json = parse_json(response=r) + return Item(r_json) except ValueError: _logger.error(f'Invalid item UUID: {uuid}') return None @@ -936,6 +937,25 @@ def add_metadata(self, dso, field, value, language=None, authority=None, confide return dso_type(api_resource=parse_json(r)) + def remove_metadata(self, dso, field): + """ + Remove metadata + """ + if dso is None or field is None or not isinstance(dso, DSpaceObject): + # TODO: separate these tests, and add better error handling + _logger.error('Invalid or missing DSpace object, field or value string') + return self + + dso_type = type(dso) + + # Place can be 0+ integer, or a hyphen - meaning "last" + path = f'/metadata/{field}' + url = dso.links['self']['href'] + + r = self.api_patch(url=url, operation=self.PatchOperation.REMOVE, path=path, value=None) + return dso_type(api_resource=parse_json(r)) + + def create_user(self, user, token=None): """ Create a user From 1415456cf4e464a3ced77e70e61a3af79507dccd Mon Sep 17 00:00:00 2001 From: jm Date: Mon, 17 Mar 2025 16:09:47 +0100 Subject: [PATCH 03/28] added owningCollection and request validation --- dspace_rest_client/client.py | 45 +++++++++++++++++++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-) diff --git a/dspace_rest_client/client.py b/dspace_rest_client/client.py index bab4bde..f421b6c 100644 --- a/dspace_rest_client/client.py +++ b/dspace_rest_client/client.py @@ -96,6 +96,7 @@ def __init__(self, api_endpoint=API_ENDPOINT, username=USERNAME, password=PASSWO self.PASSWORD = password self.SOLR_ENDPOINT = solr_endpoint self.solr = None + self._last_err = None try: import pysolr self.solr = pysolr.Solr(url=solr_endpoint, always_commit=True, timeout=300, auth=solr_auth) @@ -114,6 +115,10 @@ def __init__(self, api_endpoint=API_ENDPOINT, username=USERNAME, password=PASSWO self.request_headers = {'Content-type': 'application/json', 'User-Agent': self.USER_AGENT} self.list_request_headers = {'Content-type': 'text/uri-list', 'User-Agent': self.USER_AGENT} + @property + def last_err(self): + return self._last_err + def authenticate(self, retry=False): """ Authenticate with the DSpace REST API. As with other operations, perform XSRF refreshes when necessary. @@ -159,6 +164,25 @@ def authenticate(self, retry=False): # Default, return false return False + def verify_response(self, r, id_str: str, as_json: bool = False): + """ + Verify response from API. If response is not 200, log error and return False. + """ + if r.status_code != 200: + _logger.error(f'Error response [{id_str}]: {r.status_code}: {r.text} ... [ {r.url} ]') + self._last_err = r + return False + + if as_json: + try: + r.json() + except ValueError: + _logger.error(f'Error parsing JSON response [{id_str}]: {r.text} ... [ {r.url} ]') + return False + + return True + + def refresh_token(self): """ If the DSPACE-XSRF-TOKEN appears, we need to update our local stored token and re-send our API request @@ -176,6 +200,7 @@ def api_get(self, url, params=None, data=None, headers=None): @param headers: any override headers (eg. with short-lived token for download) @return: Response from API """ + self._last_err = None if headers is None: headers = self.request_headers r = self.session.get(url, params=params, data=data, headers=headers) @@ -192,6 +217,7 @@ def api_post(self, url, params, json, retry=False): @param retry: Has this method already been retried? Used if we need to refresh XSRF. @return: Response from API """ + self._last_err = None r = self.session.post(url, json=json, params=params, headers=self.request_headers) self.update_token(r) @@ -235,6 +261,7 @@ def api_post_uri(self, url, params, uri_list, retry=False): @param retry: Has this method already been retried? Used if we need to refresh XSRF. @return: Response from API """ + self._last_err = None r = self.session.post(url, data=uri_list, params=params, headers=self.list_request_headers) self.update_token(r) @@ -263,6 +290,7 @@ def api_put(self, url, params, json, retry=False): @param retry: Has this method already been retried? Used if we need to refresh XSRF. @return: Response from API """ + self._last_err = None r = self.session.put(url, params=params, json=json, headers=self.request_headers) self.update_token(r) @@ -292,6 +320,7 @@ def api_delete(self, url, params, retry=False): @param retry: Has this method already been retried? Used if we need to refresh XSRF. @return: Response from API """ + self._last_err = None r = self.session.delete(url, params=params, headers=self.request_headers) self.update_token(r) @@ -322,6 +351,7 @@ def api_patch(self, url, operation, path, value, retry=False): @return: @see https://github.com/DSpace/RestContract/blob/main/metadata-patch.md """ + self._last_err = None if url is None: logging.error('Missing required URL argument') return None @@ -873,6 +903,20 @@ def get_items(self, page=0, size=20): items.append(Item(r_json)) return items + def get_owningCollection(self, item_uuid): + """ + Get owningCollection + """ + url = f'{self.API_ENDPOINT}/core/items/{item_uuid}/owningCollection' + try: + r = self.api_get(url, None, None) + self.verify_response(r, f"item:{item_uuid}", True) + r_json = parse_json(response=r) + return Collection(r_json) + except ValueError: + _logger.error(f'Invalid owningCollection for UUID: {item_uuid}') + return None + def create_item(self, parent, item): """ Create an item beneath the given parent collection @@ -942,7 +986,6 @@ def remove_metadata(self, dso, field): Remove metadata """ if dso is None or field is None or not isinstance(dso, DSpaceObject): - # TODO: separate these tests, and add better error handling _logger.error('Invalid or missing DSpace object, field or value string') return self From bbc348310e5ff1f8c47e2559da885a23ca260a41 Mon Sep 17 00:00:00 2001 From: jm Date: Tue, 18 Mar 2025 13:36:08 +0100 Subject: [PATCH 04/28] enable result count, otherwise non authenticated might get into problems --- dspace_rest_client/client.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/dspace_rest_client/client.py b/dspace_rest_client/client.py index f421b6c..a37caeb 100644 --- a/dspace_rest_client/client.py +++ b/dspace_rest_client/client.py @@ -401,7 +401,7 @@ def api_patch(self, url, operation, path, value, retry=False): return r # PAGINATION - def search_objects(self, query=None, scope=None, filters=None, page=0, size=20, sort=None, dso_type=None): + def search_objects(self, query=None, scope=None, filters=None, page=0, size=20, sort=None, dso_type=None, details=None): """ Do a basic search with optional query, filters and dsoType params. @param query: query string @@ -436,6 +436,8 @@ def search_objects(self, query=None, scope=None, filters=None, page=0, size=20, # instead lots of 'does this key exist, etc etc' checks, just go for it and wrap in a try? try: + if details is not None: + details["page"] = r_json['_embedded']['searchResult']['page'] results = r_json['_embedded']['searchResult']['_embedded']['objects'] for result in results: resource = result['_embedded']['indexableObject'] @@ -653,7 +655,7 @@ def get_bitstreams(self, uuid=None, bundle=None, page=0, size=20, sort=None): url = bundle.links['bitstreams']['href'] else: url = f'{self.API_ENDPOINT}/core/bundles/{bundle.uuid}/bitstreams' - _logger.warning(f'Cannot find bundle bitstream links, will try to construct manually: {url}') + _logger.info(f'Cannot find bundle bitstream links, will try to construct manually: {url}') # Perform the actual request. By now, our URL and parameter should be properly set params = {} if size is not None: From 89b6757d24283f8580d1dc8780dbbd27df6a152a Mon Sep 17 00:00:00 2001 From: jm Date: Wed, 5 Nov 2025 23:18:01 +0100 Subject: [PATCH 05/28] docs: fix typos across repo - Fix 'bistreams' -> 'bitstreams' in docstring - Fix 'sucessfully' -> 'successfully' in log messages (2 instances) These spelling errors were found in comments and log messages and do not affect code behavior. --- dspace_rest_client/client.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/dspace_rest_client/client.py b/dspace_rest_client/client.py index a37caeb..cb9c510 100644 --- a/dspace_rest_client/client.py +++ b/dspace_rest_client/client.py @@ -6,7 +6,7 @@ DSpace REST API client library. Intended to make interacting with DSpace in Python 3 easier, particularly when creating, updating, retrieving and deleting DSpace Objects. This client library is a work in progress and currently only implements the most basic functionality. -It was originally created to assist with a migration of container structure, items and bistreams from a non-DSpace +It was originally created to assist with a migration of container structure, items and bitstreams from a non-DSpace system to a new DSpace 7 repository. It needs a lot of expansion: resource policies and permissions, validation of prepared objects and responses, @@ -537,7 +537,7 @@ def update_dso(self, dso, params=None): if r.status_code == 200: # 200 OK - success! updated_dso = dso_type(parse_json(r)) - _logger.debug(f'{updated_dso.type} {updated_dso.uuid} updated sucessfully!') + _logger.debug(f'{updated_dso.type} {updated_dso.uuid} updated successfully!') return updated_dso else: _logger.error(f'update operation failed: {r.status_code}: {r.text} ({url})') @@ -573,7 +573,7 @@ def delete_dso(self, dso=None, url=None, params=None): r = self.api_delete(url, params=params) if r.status_code == 204: # 204 No Content - success! - _logger.info(f'{url} was deleted sucessfully!') + _logger.info(f'{url} was deleted successfully!') return r else: _logger.error(f'update operation failed: {r.status_code}: {r.text} ({url})') From 9c332d27d0ccaee2a04718153f6556479b88c749 Mon Sep 17 00:00:00 2001 From: jm Date: Tue, 20 Jan 2026 10:05:01 +0100 Subject: [PATCH 06/28] added resourcepolicy specific for dtq --- dspace_rest_client/client.py | 21 +++++++++++++++++++++ dspace_rest_client/models.py | 28 +++++++++++++++++++++++++++- 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/dspace_rest_client/client.py b/dspace_rest_client/client.py index cb9c510..3540d7b 100644 --- a/dspace_rest_client/client.py +++ b/dspace_rest_client/client.py @@ -463,6 +463,27 @@ def fetch_resource(self, url, params=None): # ValueError / JSON handling moved to static method return parse_json(r) + def get_resourcepolicy(self, uuid, action='READ'): + """ + Fetch resource policies for a given resource UUID and action. + @param uuid: resource UUID to search for + @param action: action name to filter by (default: READ) + @return: Parsed JSON response from fetch_resource or None if error + """ + try: + # Validate UUID + id = UUID(uuid).version + url = f'{self.API_ENDPOINT}/authz/resourcepolicies/search/resource' + params = {'uuid': uuid} + if action is not None: + params['action'] = action + r_json = self.fetch_resource(url, params=params) + arr = r_json['_embedded'].get('resourcepolicies') or [] + return [ResourcePolicy(x) for x in arr] + except ValueError: + _logger.error(f'Invalid resource UUID: {uuid}') + return None + def get_dso(self, url, uuid): """ Base 'get DSpace Object' function. diff --git a/dspace_rest_client/models.py b/dspace_rest_client/models.py index 21e3a3c..4f0530b 100644 --- a/dspace_rest_client/models.py +++ b/dspace_rest_client/models.py @@ -13,7 +13,7 @@ __all__ = ['DSpaceObject', 'HALResource', 'ExternalDataObject', 'SimpleDSpaceObject', 'Community', - 'Collection', 'Item', 'Bundle', 'Bitstream', 'User', 'Group'] + 'Collection', 'Item', 'Bundle', 'Bitstream', 'User', 'Group', 'ResourcePolicy'] class HALResource: @@ -512,3 +512,29 @@ def __init__(self, api_resource): super(RelationshipType, self).__init__(api_resource) +class ResourcePolicy(AddressableHALResource): + """ + DQ specific. Extends Addressable HAL Resource to model a resource policy. + """ + def __init__(self, api_resource: dict): + super(ResourcePolicy, self).__init__(api_resource) + api_resource = api_resource or {} + self.name = api_resource.get('name') + self.description = api_resource.get('description') + self.startDate = api_resource.get('startDate') + self.endDate = api_resource.get('endDate') + self.type = api_resource.get('type') + self.action = api_resource.get('action') + self.policyType = api_resource.get('policyType') + + def as_dict(self): + return { + 'id': self.id, + 'name': self.name, + 'type': self.type, + 'description': self.description, + 'startDate': self.startDate, + 'endDate': self.endDate, + 'action': self.action, + 'policyType': self.policyType, + } \ No newline at end of file From e46b57ae7509f49415d1825c58534117a67102f4 Mon Sep 17 00:00:00 2001 From: jm Date: Wed, 21 Jan 2026 14:12:10 +0100 Subject: [PATCH 07/28] add None check --- dspace_rest_client/client.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/dspace_rest_client/client.py b/dspace_rest_client/client.py index 3540d7b..9e3196d 100644 --- a/dspace_rest_client/client.py +++ b/dspace_rest_client/client.py @@ -478,6 +478,9 @@ def get_resourcepolicy(self, uuid, action='READ'): if action is not None: params['action'] = action r_json = self.fetch_resource(url, params=params) + if '_embedded' not in (r_json or {}): + _logger.debug(f"No resource policies found for resource UUID: {uuid} [{url}]") + return None arr = r_json['_embedded'].get('resourcepolicies') or [] return [ResourcePolicy(x) for x in arr] except ValueError: From 5c4e9f0ec7a452202d911c5b45af5fd65c590670 Mon Sep 17 00:00:00 2001 From: jm Date: Thu, 22 Jan 2026 14:44:32 +0100 Subject: [PATCH 08/28] add resourcePolicy from d --- dspace_rest_client/models.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/dspace_rest_client/models.py b/dspace_rest_client/models.py index 4f0530b..07e8949 100644 --- a/dspace_rest_client/models.py +++ b/dspace_rest_client/models.py @@ -28,7 +28,9 @@ def __init__(self, api_resource=None): Default constructor @param api_resource: optional API resource (JSON) from a GET response or successful POST can populate instance """ + self._from_d = None if api_resource is not None: + self._from_d = api_resource if 'type' in api_resource: self.type = api_resource['type'] if '_links' in api_resource: @@ -135,6 +137,10 @@ def __init__(self, api_resource=None, dso=None): if '_links' in api_resource: self.links = api_resource['_links'].copy() + @property + def resourcePolicies(self): + return (self._from_d or {}).get('resourcePolicies') + def add_metadata(self, field, value, language=None, authority=None, confidence=-1, place=None): """ Add metadata to a DSO. This is performed on the local object only, it is not an API operation (see patch) From 259a9757764884a1c71c3dee980c2b63a9155706 Mon Sep 17 00:00:00 2001 From: Juraj Roka <95219754+jr-rk@users.noreply.github.com> Date: Thu, 12 Feb 2026 16:33:46 +0100 Subject: [PATCH 09/28] add group info to resourcePolicy --- dspace_rest_client/models.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/dspace_rest_client/models.py b/dspace_rest_client/models.py index 07e8949..26cbb45 100644 --- a/dspace_rest_client/models.py +++ b/dspace_rest_client/models.py @@ -532,7 +532,12 @@ def __init__(self, api_resource: dict): self.type = api_resource.get('type') self.action = api_resource.get('action') self.policyType = api_resource.get('policyType') - + self.groupName = None + self.groupUUID = None + if '_embedded' in api_resource: + if 'group' in api_resource['_embedded']: + self.groupName = api_resource['_embedded']['group'].get('name') + self.groupUUID = api_resource['_embedded']['group'].get('uuid') def as_dict(self): return { 'id': self.id, @@ -543,4 +548,6 @@ def as_dict(self): 'endDate': self.endDate, 'action': self.action, 'policyType': self.policyType, + 'groupName': self.groupName, + 'groupUUID': self.groupUUID, } \ No newline at end of file From 283e39c856e3cdc65cf3be55a98a0ec697020956 Mon Sep 17 00:00:00 2001 From: Juraj Roka <95219754+jr-rk@users.noreply.github.com> Date: Fri, 13 Feb 2026 14:25:32 +0100 Subject: [PATCH 10/28] fix group properties adding --- dspace_rest_client/models.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/dspace_rest_client/models.py b/dspace_rest_client/models.py index 26cbb45..903dd8a 100644 --- a/dspace_rest_client/models.py +++ b/dspace_rest_client/models.py @@ -532,9 +532,11 @@ def __init__(self, api_resource: dict): self.type = api_resource.get('type') self.action = api_resource.get('action') self.policyType = api_resource.get('policyType') - self.groupName = None - self.groupUUID = None - if '_embedded' in api_resource: + # Check for direct groupName/groupUUID (cached format from as_dict()) + self.groupName = api_resource.get('groupName') + self.groupUUID = api_resource.get('groupUUID') + # If not found, try extracting from _embedded structure (live API format) + if self.groupName is None and '_embedded' in api_resource: if 'group' in api_resource['_embedded']: self.groupName = api_resource['_embedded']['group'].get('name') self.groupUUID = api_resource['_embedded']['group'].get('uuid') From 59402c0d2ec83d3a54d7087a027b57f5eaadf0bd Mon Sep 17 00:00:00 2001 From: jm Date: Fri, 13 Feb 2026 23:39:19 +0100 Subject: [PATCH 11/28] fix: upstream bugfixes and proxy support Backports critical bugfixes and proxy support from upstream the-library-code/dspace-rest-python. Bugfixes: - Fix User model trailing commas that turned fields into tuples - Fix get_items() using wrong embedded key ('collections' -> 'items') - Fix InProgressSubmission step assigned from lastModified instead of step - Fix InProgressSubmission type assigned from lastModified instead of type - Fix EntityType type field overwriting label - Fix parse_json to handle None response safely Improvements: - Add proxy support via PROXY_URL env var and proxies constructor param - Add proxies to all HTTP methods (GET, POST, PUT, DELETE, PATCH, send) - Add proxies to authenticate status check GET - Add params parameter to api_patch method - Add embedded attribute to HALResource base class - Add ITER_PAGE_SIZE class variable (preparation for pagination) - Add upstream_ref/ to .gitignore --- .gitignore | 1 + dspace_rest_client/client.py | 51 ++++++++++++++++++++++++------------ dspace_rest_client/models.py | 22 +++++++++------- 3 files changed, 48 insertions(+), 26 deletions(-) diff --git a/.gitignore b/.gitignore index 0dc7c57..f0de7dc 100644 --- a/.gitignore +++ b/.gitignore @@ -9,3 +9,4 @@ __pypackages__/ env/ venv/ .idea/ +upstream_ref/ diff --git a/dspace_rest_client/client.py b/dspace_rest_client/client.py index 9e3196d..80408b7 100644 --- a/dspace_rest_client/client.py +++ b/dspace_rest_client/client.py @@ -16,11 +16,12 @@ """ import json import logging +import os +from uuid import UUID import requests from requests import Request -import os -from uuid import UUID + from .models import * __all__ = ['DSpaceClient'] @@ -37,9 +38,13 @@ def parse_json(response): """ response_json = None try: - response_json = response.json() + if response is not None: + response_json = response.json() except ValueError as err: - _logger.error(f'Error parsing response JSON: {err}. Body text: {response.text}') + if response is not None: + _logger.error(f'Error parsing response JSON: {err}. Body text: {response.text}') + else: + _logger.error(f'Error parsing response JSON: {err}. Response is None') return response_json @@ -73,6 +78,8 @@ class DSpaceClient: if 'USER_AGENT' in os.environ: USER_AGENT = os.environ['USER_AGENT'] verbose = False + ITER_PAGE_SIZE = 20 + PROXY_DICT = dict(http=os.environ["PROXY_URL"],https=os.environ["PROXY_URL"]) if "PROXY_URL" in os.environ else dict() # Simple enum for patch operation types class PatchOperation: @@ -82,7 +89,7 @@ class PatchOperation: MOVE = 'move' def __init__(self, api_endpoint=API_ENDPOINT, username=USERNAME, password=PASSWORD, solr_endpoint=SOLR_ENDPOINT, - solr_auth=SOLR_AUTH, fake_user_agent=False): + solr_auth=SOLR_AUTH, fake_user_agent=False, proxies=PROXY_DICT): """ Accept optional API endpoint, username, password arguments using the OS environment variables as defaults :param api_endpoint: base path to DSpace REST API, eg. http://localhost:8080/server/api @@ -95,6 +102,7 @@ def __init__(self, api_endpoint=API_ENDPOINT, username=USERNAME, password=PASSWO self.USERNAME = username self.PASSWORD = password self.SOLR_ENDPOINT = solr_endpoint + self.proxies = proxies self.solr = None self._last_err = None try: @@ -128,7 +136,8 @@ def authenticate(self, retry=False): # Set headers for requests made during authentication # Get and update CSRF token r = self.session.post(self.LOGIN_URL, data={'user': self.USERNAME, 'password': self.PASSWORD}, - headers=self.auth_request_headers) + headers=self.auth_request_headers, + proxies=self.proxies) self.update_token(r) if r.status_code == 403: @@ -154,7 +163,8 @@ def authenticate(self, retry=False): self.session.headers.update({'Authorization': r.headers.get('Authorization')}) # Get and check authentication status - r = self.session.get(f'{self.API_ENDPOINT}/authn/status', headers=self.request_headers) + r = self.session.get(f'{self.API_ENDPOINT}/authn/status', headers=self.request_headers, + proxies=self.proxies) if r.status_code == 200: r_json = parse_json(r) if 'authenticated' in r_json and r_json['authenticated'] is True: @@ -203,7 +213,8 @@ def api_get(self, url, params=None, data=None, headers=None): self._last_err = None if headers is None: headers = self.request_headers - r = self.session.get(url, params=params, data=data, headers=headers) + r = self.session.get(url, params=params, data=data, headers=headers, + proxies=self.proxies) self.update_token(r) return r @@ -218,7 +229,8 @@ def api_post(self, url, params, json, retry=False): @return: Response from API """ self._last_err = None - r = self.session.post(url, json=json, params=params, headers=self.request_headers) + r = self.session.post(url, json=json, params=params, headers=self.request_headers, + proxies=self.proxies) self.update_token(r) if r.status_code == 403: @@ -262,7 +274,8 @@ def api_post_uri(self, url, params, uri_list, retry=False): @return: Response from API """ self._last_err = None - r = self.session.post(url, data=uri_list, params=params, headers=self.list_request_headers) + r = self.session.post(url, data=uri_list, params=params, headers=self.list_request_headers, + proxies=self.proxies) self.update_token(r) if r.status_code == 403: @@ -291,7 +304,8 @@ def api_put(self, url, params, json, retry=False): @return: Response from API """ self._last_err = None - r = self.session.put(url, params=params, json=json, headers=self.request_headers) + r = self.session.put(url, params=params, json=json, headers=self.request_headers, + proxies=self.proxies) self.update_token(r) if r.status_code == 403: @@ -321,7 +335,8 @@ def api_delete(self, url, params, retry=False): @return: Response from API """ self._last_err = None - r = self.session.delete(url, params=params, headers=self.request_headers) + r = self.session.delete(url, params=params, headers=self.request_headers, + proxies=self.proxies) self.update_token(r) if r.status_code == 403: @@ -341,12 +356,13 @@ def api_delete(self, url, params, retry=False): return r - def api_patch(self, url, operation, path, value, retry=False): + def api_patch(self, url, operation, path, value, params=None, retry=False): """ @param url: DSpace REST API URL @param operation: 'add', 'remove', 'replace', or 'move' (see PatchOperation enumeration) @param path: path to perform operation - eg, metadata, withdrawn, etc. @param value: new value for add or replace operations, or 'original' path for move operations + @param params: Optional parameters @param retry: Has this method already been retried? Used if we need to refresh XSRF. @return: @see https://github.com/DSpace/RestContract/blob/main/metadata-patch.md @@ -377,7 +393,8 @@ def api_patch(self, url, operation, path, value, retry=False): # set headers # perform patch request - r = self.session.patch(url, json=[data], headers=self.request_headers) + r = self.session.patch(url, json=[data], params=params, headers=self.request_headers, + proxies=self.proxies) self.update_token(r) if r.status_code == 403: @@ -392,7 +409,7 @@ def api_patch(self, url, operation, path, value, retry=False): _logger.warning(f'Too many retries updating token: {r.status_code}: {r.text}') else: _logger.debug("Retrying request with updated CSRF token") - return self.api_patch(url, operation, path, value, True) + return self.api_patch(url, operation, path, value, params, True) elif r.status_code == 200: # 200 Success _logger.info(f'successful patch update to {r.json()["type"]} {r.json()["id"]}') @@ -727,7 +744,7 @@ def create_bitstream(self, bundle=None, name=None, path=None, mime=None, metadat h.update({'Content-Encoding': 'gzip', 'User-Agent': self.USER_AGENT}) req = Request('POST', url, data=payload, headers=h, files=files) prepared_req = self.session.prepare_request(req) - r = self.session.send(prepared_req) + r = self.session.send(prepared_req, proxies=self.proxies) if 'DSPACE-XSRF-TOKEN' in r.headers: t = r.headers['DSPACE-XSRF-TOKEN'] _logger.debug('Updating token to ' + t) @@ -922,7 +939,7 @@ def get_items(self, page=0, size=20): r = self.api_get(url, params=params) r_json = parse_json(response=r) if '_embedded' in r_json: - if 'collections' in r_json['_embedded']: + if 'items' in r_json['_embedded']: for item_resource in r_json['_embedded']['items']: items.append(Item(item_resource)) elif 'uuid' in r_json: diff --git a/dspace_rest_client/models.py b/dspace_rest_client/models.py index 903dd8a..f1c8550 100644 --- a/dspace_rest_client/models.py +++ b/dspace_rest_client/models.py @@ -37,6 +37,10 @@ def __init__(self, api_resource=None): self.links = api_resource['_links'].copy() else: self.links = {'self': {'href': None}} + if '_embedded' in api_resource: + self.embedded = api_resource['_embedded'].copy() + else: + self.embedded = {} class AddressableHALResource(HALResource): id = None @@ -421,12 +425,12 @@ class User(SimpleDSpaceObject): Extends DSpaceObject to implement specific attributes and methods for users (aka. EPersons) """ type = 'user' - name = None, - netid = None, - lastActive = None, - canLogIn = False, - email = None, - requireCertificate = False, + name = None + netid = None + lastActive = None + canLogIn = False + email = None + requireCertificate = False selfRegistered = False def __init__(self, api_resource=None): @@ -473,11 +477,11 @@ def __init__(self, api_resource): if 'lastModified' in api_resource: self.lastModified = api_resource['lastModified'] if 'step' in api_resource: - self.step = api_resource['lastModified'] + self.step = api_resource['step'] if 'sections' in api_resource: self.sections = api_resource['sections'].copy() if 'type' in api_resource: - self.lastModified = api_resource['lastModified'] + self.type = api_resource['type'] def as_dict(self): parent_dict = super(InProgressSubmission, self).as_dict() @@ -508,7 +512,7 @@ def __init__(self, api_resource): if 'label' in api_resource: self.label = api_resource['label'] if 'type' in api_resource: - self.label = api_resource['type'] + self.type = api_resource['type'] class RelationshipType(AddressableHALResource): """ From c7ca664cc9d748188d1550a6943380a4f756a234 Mon Sep 17 00:00:00 2001 From: Jozef Misutka <332350+vidiecan@users.noreply.github.com> Date: Fri, 13 Feb 2026 23:57:18 +0100 Subject: [PATCH 12/28] Remove upstream_ref from .gitignore --- .gitignore | 1 - 1 file changed, 1 deletion(-) diff --git a/.gitignore b/.gitignore index f0de7dc..0dc7c57 100644 --- a/.gitignore +++ b/.gitignore @@ -9,4 +9,3 @@ __pypackages__/ env/ venv/ .idea/ -upstream_ref/ From 291bc468f12c2fda1edbc220ea6361e45a27751a Mon Sep 17 00:00:00 2001 From: jm Date: Tue, 17 Feb 2026 00:38:04 +0100 Subject: [PATCH 13/28] add rp by ai --- dspace_rest_client/client.py | 46 ++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/dspace_rest_client/client.py b/dspace_rest_client/client.py index 9e3196d..1810074 100644 --- a/dspace_rest_client/client.py +++ b/dspace_rest_client/client.py @@ -487,6 +487,52 @@ def get_resourcepolicy(self, uuid, action='READ'): _logger.error(f'Invalid resource UUID: {uuid}') return None + def create_resourcepolicy( + self, resource_uuid, group_uuid, action='READ', + policy_name=None, start_date=None, end_date=None, + ): + """ + Create a new resource policy for a given DSpace resource. + Uses POST /authz/resourcepolicies?resource=&resource-type=bitstream + @param resource_uuid: UUID of the target bitstream (or other resource) + @param group_uuid: UUID of the group to grant access to + @param action: action name (default: READ) + @param policy_name: optional policy name + @param start_date: optional start date string (ISO 8601) + @param end_date: optional end date string (ISO 8601) + @return: ResourcePolicy on success, None on failure + """ + try: + UUID(resource_uuid) + UUID(group_uuid) + except ValueError: + _logger.error(f'Invalid UUID: resource={resource_uuid}, group={group_uuid}') + return None + + url = f'{self.API_ENDPOINT}/authz/resourcepolicies' + params = {'resource': resource_uuid, 'resource-type': 'bitstream'} + data = { + 'action': action, + 'name': policy_name, + 'startDate': start_date, + 'endDate': end_date, + } + # Link to the group via the eperson-group URI + group_uri = f'{self.API_ENDPOINT}/eperson/groups/{group_uuid}' + + r = self.api_post(url, params=params, json=data) + if r.status_code == 201: + rp = ResourcePolicy(parse_json(r)) + _logger.info(f'Created resource policy id={rp.id} for resource {resource_uuid}') + # Now link the group to the newly created policy + rp_group_url = f'{self.API_ENDPOINT}/authz/resourcepolicies/{rp.id}/group' + self.api_post_uri(rp_group_url, params=None, uri_list=group_uri) + return rp + + _logger.error( + f'Failed to create resource policy: {r.status_code}: {r.text}') + return None + def get_dso(self, url, uuid): """ Base 'get DSpace Object' function. From 3b3063a50633be122f6667284ecdfd637c41086c Mon Sep 17 00:00:00 2001 From: Juraj Roka <95219754+jr-rk@users.noreply.github.com> Date: Tue, 17 Feb 2026 17:30:07 +0100 Subject: [PATCH 14/28] fix rp create/delete --- dspace_rest_client/client.py | 36 +++++++++++++++++++----------------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/dspace_rest_client/client.py b/dspace_rest_client/client.py index 1810074..4719a2c 100644 --- a/dspace_rest_client/client.py +++ b/dspace_rest_client/client.py @@ -478,9 +478,12 @@ def get_resourcepolicy(self, uuid, action='READ'): if action is not None: params['action'] = action r_json = self.fetch_resource(url, params=params) - if '_embedded' not in (r_json or {}): - _logger.debug(f"No resource policies found for resource UUID: {uuid} [{url}]") + if r_json is None: + _logger.error(f"API call failed for resource UUID: {uuid}") return None + if '_embedded' not in r_json: + _logger.debug(f"No resource policies found for resource UUID: {uuid} [{url}]") + return [] arr = r_json['_embedded'].get('resourcepolicies') or [] return [ResourcePolicy(x) for x in arr] except ValueError: @@ -489,17 +492,16 @@ def get_resourcepolicy(self, uuid, action='READ'): def create_resourcepolicy( self, resource_uuid, group_uuid, action='READ', - policy_name=None, start_date=None, end_date=None, + start_date=None, end_date=None, ): """ Create a new resource policy for a given DSpace resource. - Uses POST /authz/resourcepolicies?resource=&resource-type=bitstream + Uses POST /api/authz/resourcepolicies?resource=&group= @param resource_uuid: UUID of the target bitstream (or other resource) @param group_uuid: UUID of the group to grant access to @param action: action name (default: READ) - @param policy_name: optional policy name - @param start_date: optional start date string (ISO 8601) - @param end_date: optional end date string (ISO 8601) + @param start_date: optional start date string (ISO 8601, YYYY-MM-DD) + @param end_date: optional end date string (ISO 8601, YYYY-MM-DD) @return: ResourcePolicy on success, None on failure """ try: @@ -510,23 +512,23 @@ def create_resourcepolicy( return None url = f'{self.API_ENDPOINT}/authz/resourcepolicies' - params = {'resource': resource_uuid, 'resource-type': 'bitstream'} + params = { + 'resource': resource_uuid, + 'group': group_uuid, + } data = { 'action': action, - 'name': policy_name, - 'startDate': start_date, - 'endDate': end_date, + 'type': 'resourcepolicy', } - # Link to the group via the eperson-group URI - group_uri = f'{self.API_ENDPOINT}/eperson/groups/{group_uuid}' + if start_date is not None: + data['startDate'] = start_date + if end_date is not None: + data['endDate'] = end_date r = self.api_post(url, params=params, json=data) - if r.status_code == 201: + if r.status_code in (200, 201): rp = ResourcePolicy(parse_json(r)) _logger.info(f'Created resource policy id={rp.id} for resource {resource_uuid}') - # Now link the group to the newly created policy - rp_group_url = f'{self.API_ENDPOINT}/authz/resourcepolicies/{rp.id}/group' - self.api_post_uri(rp_group_url, params=None, uri_list=group_uri) return rp _logger.error( From 1121bc7e28611db577fa3dfeb8709091c1f0971e Mon Sep 17 00:00:00 2001 From: jm Date: Wed, 18 Feb 2026 10:02:35 +0100 Subject: [PATCH 15/28] added repr to ResourcePolicy --- dspace_rest_client/models.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/dspace_rest_client/models.py b/dspace_rest_client/models.py index 903dd8a..e57a339 100644 --- a/dspace_rest_client/models.py +++ b/dspace_rest_client/models.py @@ -552,4 +552,7 @@ def as_dict(self): 'policyType': self.policyType, 'groupName': self.groupName, 'groupUUID': self.groupUUID, - } \ No newline at end of file + } + + def __repr__(self): + return f"ResourcePolicy: {self.name} [{self.groupName}] [action: {self.action}] [type: {self.type}]" \ No newline at end of file From 9f3cd618249fafce8cfa8f20fbeec3f997d9867b Mon Sep 17 00:00:00 2001 From: jm Date: Thu, 19 Feb 2026 15:47:16 +0100 Subject: [PATCH 16/28] reauth --- dspace_rest_client/client.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/dspace_rest_client/client.py b/dspace_rest_client/client.py index 4719a2c..fe4684a 100644 --- a/dspace_rest_client/client.py +++ b/dspace_rest_client/client.py @@ -781,14 +781,13 @@ def create_bitstream(self, bundle=None, name=None, path=None, mime=None, metadat _logger.debug('Updating token to ' + t) self.session.headers.update({'X-XSRF-Token': t}) self.session.cookies.update({'X-XSRF-Token': t}) - if r.status_code == 403: + if not retry and r.status_code in (401, 403): r_json = parse_json(r) if 'message' in r_json and 'CSRF token' in r_json['message']: - if retry: - _logger.error('Already retried... something must be wrong') - else: - _logger.debug("Retrying request with updated CSRF token") - return self.create_bitstream(bundle, name, path, mime, metadata, True) + _logger.debug("Retrying request with updated CSRF token") + else: + self.authenticate() + return self.create_bitstream(bundle, name, path, mime, metadata, True) if r.status_code == 201 or r.status_code == 200: # Success From e1c05a99bb598740ea4d072a58d576b562408766 Mon Sep 17 00:00:00 2001 From: jm Date: Mon, 23 Feb 2026 11:29:02 +0100 Subject: [PATCH 17/28] better logs --- dspace_rest_client/client.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/dspace_rest_client/client.py b/dspace_rest_client/client.py index ff9f0dd..8e238f2 100644 --- a/dspace_rest_client/client.py +++ b/dspace_rest_client/client.py @@ -496,15 +496,14 @@ def get_resourcepolicy(self, uuid, action='READ'): params['action'] = action r_json = self.fetch_resource(url, params=params) if r_json is None: - _logger.error(f"API call failed for resource UUID: {uuid}") return None if '_embedded' not in r_json: _logger.debug(f"No resource policies found for resource UUID: {uuid} [{url}]") return [] arr = r_json['_embedded'].get('resourcepolicies') or [] return [ResourcePolicy(x) for x in arr] - except ValueError: - _logger.error(f'Invalid resource UUID: {uuid}') + except ValueError as e: + _logger.error(f'Invalid resource UUID: {uuid} - {e}') return None def create_resourcepolicy( From 51b65fe7970348b6b6e78a1adf90b1f2601264ed Mon Sep 17 00:00:00 2001 From: jm Date: Tue, 24 Feb 2026 12:45:03 +0100 Subject: [PATCH 18/28] add timeout --- dspace_rest_client/client.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/dspace_rest_client/client.py b/dspace_rest_client/client.py index 8e238f2..b2d3a89 100644 --- a/dspace_rest_client/client.py +++ b/dspace_rest_client/client.py @@ -218,7 +218,7 @@ def api_get(self, url, params=None, data=None, headers=None): self.update_token(r) return r - def api_post(self, url, params, json, retry=False): + def api_post(self, url, params, json, retry=False, timeout=None): """ Perform a POST request. Refresh XSRF token if necessary. POSTs are typically used to create objects. @@ -230,7 +230,7 @@ def api_post(self, url, params, json, retry=False): """ self._last_err = None r = self.session.post(url, json=json, params=params, headers=self.request_headers, - proxies=self.proxies) + proxies=self.proxies, timeout=timeout) self.update_token(r) if r.status_code == 403: @@ -244,7 +244,7 @@ def api_post(self, url, params, json, retry=False): _logger.warning(f'Too many retries updating token: {r.status_code}: {r.text}') else: _logger.debug("Retrying request with updated CSRF token") - return self.api_post(url, params=params, json=json, retry=True) + return self.api_post(url, params=params, json=json, retry=True, timeout=timeout) # we need to log in again, if there is login error. This is a bad # solution copied from the past @@ -260,7 +260,7 @@ def api_post(self, url, params, json, retry=False): self.authenticate() # Try to authenticate and repeat the request 3 times - # if it won't happen log error - return self.api_post(url, params=params, json=json, retry=False) + return self.api_post(url, params=params, json=json, retry=False, timeout=timeout) return r def api_post_uri(self, url, params, uri_list, retry=False): From db03111fd9ac629dce0a22ee76af1ba68b06de6d Mon Sep 17 00:00:00 2001 From: jm Date: Wed, 25 Feb 2026 10:46:21 +0100 Subject: [PATCH 19/28] more robust check --- dspace_rest_client/client.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/dspace_rest_client/client.py b/dspace_rest_client/client.py index b2d3a89..0115557 100644 --- a/dspace_rest_client/client.py +++ b/dspace_rest_client/client.py @@ -239,7 +239,7 @@ def api_post(self, url, params, json, retry=False, timeout=None): # After speaking in #dev it seems that these do need occasional refreshes but I suspect # it's happening too often for me, so check for accidentally triggering it r_json = parse_json(r) - if 'message' in r_json and 'CSRF token' in r_json['message']: + if 'message' in (r_json or {}) and 'CSRF token' in r_json['message']: if retry: _logger.warning(f'Too many retries updating token: {r.status_code}: {r.text}') else: @@ -250,7 +250,7 @@ def api_post(self, url, params, json, retry=False, timeout=None): # solution copied from the past elif r.status_code == 401: r_json = parse_json(r) - if 'message' in r_json and 'Authentication is required' in r_json['message']: + if 'message' in (r_json or {}) and 'Authentication is required' in r_json['message']: if retry: logging.error( 'API Post: Already retried... something must be wrong') @@ -260,7 +260,7 @@ def api_post(self, url, params, json, retry=False, timeout=None): self.authenticate() # Try to authenticate and repeat the request 3 times - # if it won't happen log error - return self.api_post(url, params=params, json=json, retry=False, timeout=timeout) + return self.api_post(url, params=params, json=json, retry=True, timeout=timeout) return r def api_post_uri(self, url, params, uri_list, retry=False): From f9ca942921ec01396556fc85a17ab6b10ace7437 Mon Sep 17 00:00:00 2001 From: Jozef Misutka <332350+vidiecan@users.noreply.github.com> Date: Mon, 17 Aug 2026 09:01:53 +0200 Subject: [PATCH 20/28] fix(client): return empty bundles on 404 instead of crashing (#16) get_bundles() subscripted the None that fetch_resource returns on any non-200 response, so an item deleted since the cache was built (404) raised "'NoneType' object is not subscriptable" during a bitstream export - a scary CRITICAL line for what is really just "this item is gone". Record the failing response as _last_err in fetch_resource so callers can tell a gone resource (404) from a transient 5xx, then in get_bundles treat a 404 as a clean empty result. Any other failure still falls through and surfaces to the caller, so it keeps its retry and failure counting. Co-authored-by: jm Co-authored-by: Claude Opus 4.8 --- dspace_rest_client/client.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/dspace_rest_client/client.py b/dspace_rest_client/client.py index 0115557..e2058b9 100644 --- a/dspace_rest_client/client.py +++ b/dspace_rest_client/client.py @@ -475,6 +475,9 @@ def fetch_resource(self, url, params=None): """ r = self.api_get(url, params, None) if r.status_code != 200: + # record the failing response so callers can tell a 404 (the + # resource is gone) from a transient 5xx before we drop the body + self._last_err = r _logger.error(f'Error encountered fetching resource: {r.text}') return None # ValueError / JSON handling moved to static method @@ -698,6 +701,12 @@ def get_bundles(self, parent=None, uuid=None, page=0, size=20, sort=None): if sort is not None: params['sort'] = sort r_json = self.fetch_resource(url, params=params) + if r_json is None and getattr(self._last_err, 'status_code', None) == 404: + # the item (or bundle) no longer exists - a deleted item simply has + # no bundles, which is a clean empty result, not a crash. any other + # failure falls through and still surfaces to the caller. + _logger.info(f'No bundles: resource not found (404) [{url}]') + return bundles try: if single_result: bundles.append(Bundle(r_json)) From 7937db17ebd52eca44ee12e8a2bab7e465921b37 Mon Sep 17 00:00:00 2001 From: jm Date: Mon, 17 Aug 2026 18:07:57 +0200 Subject: [PATCH 21/28] test: add unit + integration suite for downstream usage, with CI DSpace-ISstag-integration drives this client from src/{ingest,export,reposync} and mcp/, but nothing here was under test - a library change could silently break that consumer. Add a pytest suite that mocks only the HTTP transport (requests_mock) and exercises the real URL-building and response-parsing for every DSpaceClient method and model the consumer relies on: - test_models: Item/Community/Collection/Bundle/Bitstream/ResourcePolicy construction + accessors (incl. the live _embedded.group -> groupUUID lift) - test_client_auth: constructor + authenticate() True/False semantics - test_client_read: search_objects, get_items, get_item, get_bundles (incl. the 404 -> [] contract from #16), get_bitstreams, get_collections, get_communities, get_resourcepolicy, fetch_resource (last_err on 404) - test_client_write: create_resourcepolicy, api_delete, create_bundle, create_item, create_bitstream (multipart upload) - test_repo_usage_contract: the exact multi-call chains the consumer runs (bitstream export, resource-policy replacement, MCP bundle walk, group-uuid resolution) Adds .github/workflows/tests.yml (pytest on Python 3.10 + 3.12) and requirements-test.txt. 56 tests, no network. Co-Authored-By: Claude Opus 4.8 --- .github/workflows/tests.yml | 40 +++++ .gitignore | 4 + requirements-test.txt | 5 + tests/_helpers.py | 94 +++++++++++ tests/conftest.py | 15 ++ tests/test_client_auth.py | 70 ++++++++ tests/test_client_read.py | 265 ++++++++++++++++++++++++++++++ tests/test_client_write.py | 164 ++++++++++++++++++ tests/test_models.py | 138 ++++++++++++++++ tests/test_repo_usage_contract.py | 154 +++++++++++++++++ 10 files changed, 949 insertions(+) create mode 100644 .github/workflows/tests.yml create mode 100644 requirements-test.txt create mode 100644 tests/_helpers.py create mode 100644 tests/conftest.py create mode 100644 tests/test_client_auth.py create mode 100644 tests/test_client_read.py create mode 100644 tests/test_client_write.py create mode 100644 tests/test_models.py create mode 100644 tests/test_repo_usage_contract.py diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml new file mode 100644 index 0000000..0179525 --- /dev/null +++ b/.github/workflows/tests.yml @@ -0,0 +1,40 @@ +name: Tests + +on: + push: + branches: [ dtq, dtq-dev, main ] + pull_request: + +permissions: + contents: read + +jobs: + test: + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + # Matches the DSpace-ISstag-integration consumer CI matrix; the library + # itself declares support for >=3.8 (see setup.py). + python-version: ["3.10", "3.12"] + + steps: + - uses: actions/checkout@v6 + + - name: Set up Python ${{ matrix.python-version }} + uses: actions/setup-python@v6 + with: + python-version: ${{ matrix.python-version }} + cache: pip + cache-dependency-path: | + setup.py + requirements-test.txt + + - name: Install package + test deps + run: | + python -m pip install --upgrade pip + pip install . + pip install -r requirements-test.txt + + - name: Run tests + run: python -m pytest tests/ -v diff --git a/.gitignore b/.gitignore index 0dc7c57..deba30b 100644 --- a/.gitignore +++ b/.gitignore @@ -1,6 +1,10 @@ __pycache__/ *.py[cod] *$py.class +*.egg-info/ +build/ +dist/ +.pytest_cache/ .python-version Pipfile.lock __pypackages__/ diff --git a/requirements-test.txt b/requirements-test.txt new file mode 100644 index 0000000..f061c74 --- /dev/null +++ b/requirements-test.txt @@ -0,0 +1,5 @@ +# Test-only dependencies for the dspace_rest_client suite. +# The library itself only needs `requests` (see setup.py); these add the test +# runner and an HTTP transport mock so tests never touch a real DSpace server. +pytest>=7.0 +requests-mock>=1.11 diff --git a/tests/_helpers.py b/tests/_helpers.py new file mode 100644 index 0000000..1b39ed7 --- /dev/null +++ b/tests/_helpers.py @@ -0,0 +1,94 @@ +""" +Shared helpers for the client test-suite. + +The tests mock *only* the HTTP transport (via ``requests_mock``) and let the +real ``DSpaceClient`` build URLs, send params and parse responses into model +objects. That way a change to the library that breaks URL construction or +response parsing - the two things downstream code (this repo) depends on - +fails a test instead of silently shipping. +""" +import os +import sys +from urllib.parse import urlparse, parse_qs + +# Make ``dspace_rest_client`` importable when a test module is run directly +# (``python tests/test_x.py``), not just under pytest (see conftest.py). +_ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) +if _ROOT not in sys.path: + sys.path.insert(0, _ROOT) + +from dspace_rest_client.client import DSpaceClient # noqa: E402 + +# Canonical test endpoint. All mocked URLs are built off this so a typo shows +# up as an unmatched request rather than a false pass. +API = "http://dspace.test/server/api" + +# Real, syntactically-valid UUIDs - several client methods validate their UUID +# arguments with ``uuid.UUID(...)`` and short-circuit on a bad one, so tests +# that expect a request to actually go out must use valid values. +ITEM_UUID = "11111111-1111-1111-1111-111111111111" +COLLECTION_UUID = "22222222-2222-2222-2222-222222222222" +BITSTREAM_UUID = "9f54ef33-c454-4d8e-a5fe-79d8291045ba" +ANON_GROUP_UUID = "6ecfd145-3b7d-429e-ab31-ef6905a05763" + + +def make_client(api_endpoint: str = API) -> DSpaceClient: + """A real client with no network touched. + + ``DSpaceClient.__init__`` performs no HTTP (it only creates a + ``requests.Session`` and, optionally, a pysolr handle), so a plain + construction is safe and gives us the genuine object under test. + """ + return DSpaceClient(api_endpoint, "tester@dspace.test", "secret") + + +def sent_params(request) -> dict: + """Case-preserving query params of a captured request. + + ``requests_mock``'s ``request.qs`` lowercases the whole query string, which + would mangle case-sensitive values (eg. ``action=READ``). Parsing the + original ``request.url`` keeps the real casing. + """ + return parse_qs(urlparse(request.url).query) + + +# --- response-body builders (shape mirrors the DSpace 7 REST API) --------- # + +def embedded(key: str, resources: list) -> dict: + """A HAL ``_embedded`` list envelope, eg. ``{"_embedded": {"bundles": [...]}}``.""" + return {"_embedded": {key: resources}} + + +def item_json(uuid: str = ITEM_UUID, name: str = "Thesis", **extra) -> dict: + d = {"uuid": uuid, "name": name, "type": "item", "metadata": {}, + "inArchive": True, "discoverable": True, "withdrawn": False} + d.update(extra) + return d + + +def bundle_json(uuid: str = "bnd", name: str = "ORIGINAL", + bitstreams_href: str = None, **extra) -> dict: + d = {"uuid": uuid, "name": name, "type": "bundle", "metadata": {}} + if bitstreams_href is not None: + d["_links"] = {"bitstreams": {"href": bitstreams_href}} + d.update(extra) + return d + + +def bitstream_json(uuid: str = "bs1", name: str = "thesis.pdf", size: int = 123, + seq: int = 1, checksum: str = "abc", **extra) -> dict: + d = {"uuid": uuid, "name": name, "type": "bitstream", "metadata": {}, + "sizeBytes": size, "sequenceId": seq, + "checkSum": {"checkSumAlgorithm": "MD5", "value": checksum}} + d.update(extra) + return d + + +def policy_json(pid: int = 1, action: str = "READ", group_name: str = "Anonymous", + group_uuid: str = ANON_GROUP_UUID, start_date: str = None) -> dict: + """A resource policy in the *live* API shape (group under ``_embedded``).""" + d = {"id": pid, "action": action, + "_embedded": {"group": {"name": group_name, "uuid": group_uuid}}} + if start_date is not None: + d["startDate"] = start_date + return d diff --git a/tests/conftest.py b/tests/conftest.py new file mode 100644 index 0000000..e1f4654 --- /dev/null +++ b/tests/conftest.py @@ -0,0 +1,15 @@ +""" +Pytest bootstrap for the dspace_rest_client test-suite. + +Ensures the in-tree ``dspace_rest_client`` package is importable when the tests +are run straight from a checkout (``pytest tests/``) without a prior +``pip install``. When the package *is* installed, inserting the source root at +the front of ``sys.path`` means the tests still exercise the working-tree copy, +which is the one we ship and vendor as a submodule. +""" +import os +import sys + +_ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) +if _ROOT not in sys.path: + sys.path.insert(0, _ROOT) diff --git a/tests/test_client_auth.py b/tests/test_client_auth.py new file mode 100644 index 0000000..dae22da --- /dev/null +++ b/tests/test_client_auth.py @@ -0,0 +1,70 @@ +""" +Construction and authentication contract. + +``ingest.dspace_be`` constructs the client from an endpoint/user/password and +calls ``authenticate()``; a False return is turned into a hard ConnectionError, +so the True/False semantics here matter. +""" +import unittest + +import requests_mock + +import _helpers # noqa: F401 +from _helpers import make_client, API + + +class TestConstructor(unittest.TestCase): + + def test_endpoints_derived_from_api_endpoint(self): + c = make_client("http://host:8080/server/api") + self.assertEqual(c.API_ENDPOINT, "http://host:8080/server/api") + self.assertEqual(c.LOGIN_URL, "http://host:8080/server/api/authn/login") + self.assertIsNotNone(c.session) + + def test_default_last_err_is_none(self): + self.assertIsNone(make_client().last_err) + + +class TestAuthenticate(unittest.TestCase): + + def test_success_returns_true_and_propagates_bearer_token(self): + c = make_client() + with requests_mock.Mocker() as m: + m.post(f"{API}/authn/login", status_code=200, + headers={"Authorization": "Bearer tok123"}) + m.get(f"{API}/authn/status", status_code=200, + json={"authenticated": True}) + self.assertTrue(c.authenticate()) + # the bearer token must land on the session for later calls + self.assertEqual(c.session.headers.get("Authorization"), "Bearer tok123") + + def test_invalid_credentials_401_returns_false(self): + c = make_client() + with requests_mock.Mocker() as m: + m.post(f"{API}/authn/login", status_code=401, + json={"message": "invalid"}) + self.assertFalse(c.authenticate()) + + def test_status_not_authenticated_returns_false(self): + c = make_client() + with requests_mock.Mocker() as m: + m.post(f"{API}/authn/login", status_code=200, + headers={"Authorization": "Bearer t"}) + m.get(f"{API}/authn/status", status_code=200, + json={"authenticated": False}) + self.assertFalse(c.authenticate()) + + def test_csrf_403_retries_once_then_gives_up(self): + c = make_client() + with requests_mock.Mocker() as m: + m.post(f"{API}/authn/login", status_code=403, + json={"message": "CSRF token required"}) + self.assertFalse(c.authenticate()) + login_calls = [r for r in m.request_history + if r.path == "/server/api/authn/login"] + # initial attempt + exactly one retry with the refreshed token + self.assertEqual(len(login_calls), 2) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_client_read.py b/tests/test_client_read.py new file mode 100644 index 0000000..f3a8bb7 --- /dev/null +++ b/tests/test_client_read.py @@ -0,0 +1,265 @@ +""" +Read-path contract: every GET method this repo calls. + +Each test stubs only the HTTP response and asserts the real client (a) hits the +right URL with the right query params and (b) parses the response into the model +objects / shapes the callers rely on. +""" +import unittest + +import requests_mock + +import _helpers # noqa: F401 +from _helpers import ( + make_client, sent_params, embedded, item_json, bundle_json, + bitstream_json, policy_json, API, ITEM_UUID, BITSTREAM_UUID) +from dspace_rest_client.models import Item, Bundle, Collection, Community + + +class TestSearchObjects(unittest.TestCase): + + def test_builds_url_params_and_parses_objects(self): + c = make_client() + body = {"_embedded": {"searchResult": { + "page": {"totalElements": 2, "size": 100}, + "_embedded": {"objects": [ + {"_embedded": {"indexableObject": item_json("u1", "A")}}, + {"_embedded": {"indexableObject": item_json("u2", "B")}}, + ]}}}} + with requests_mock.Mocker() as m: + m.get(f"{API}/discover/search/objects", json=body) + details = {} + res = c.search_objects(query="dc.identifier:123", size=100, + page=0, details=details) + self.assertEqual([d.uuid for d in res], ["u1", "u2"]) + p = sent_params(m.last_request) + self.assertEqual(p["query"], ["dc.identifier:123"]) + self.assertEqual(p["size"], ["100"]) + self.assertEqual(p["page"], ["0"]) + # details["page"] is what repo.search.export_iter reads as export_len + self.assertEqual(details["page"]["totalElements"], 2) + + def test_backend_error_returns_empty_list(self): + # fetch_resource returns None on a non-200; search_objects swallows the + # resulting TypeError and yields [] rather than crashing the crawl. + c = make_client() + with requests_mock.Mocker() as m: + m.get(f"{API}/discover/search/objects", status_code=500, text="boom") + self.assertEqual(c.search_objects(query="x"), []) + + def test_empty_result_set_returns_empty_list(self): + c = make_client() + body = {"_embedded": {"searchResult": { + "page": {"totalElements": 0}, + "_embedded": {"objects": []}}}} + with requests_mock.Mocker() as m: + m.get(f"{API}/discover/search/objects", json=body) + self.assertEqual(c.search_objects(query="x"), []) + + +class TestGetItems(unittest.TestCase): + + def test_parses_embedded_items_with_paging_params(self): + c = make_client() + body = embedded("items", [item_json("i1", "one"), item_json("i2", "two")]) + with requests_mock.Mocker() as m: + m.get(f"{API}/core/items", json=body) + items = c.get_items(page=2, size=50) + self.assertEqual([i.uuid for i in items], ["i1", "i2"]) + self.assertTrue(all(isinstance(i, Item) for i in items)) + p = sent_params(m.last_request) + self.assertEqual(p["page"], ["2"]) + self.assertEqual(p["size"], ["50"]) + + +class TestGetItem(unittest.TestCase): + + def test_returns_typed_item(self): + c = make_client() + with requests_mock.Mocker() as m: + m.get(f"{API}/core/items/{ITEM_UUID}", json=item_json(ITEM_UUID, "T")) + it = c.get_item(ITEM_UUID) + self.assertIsInstance(it, Item) + self.assertEqual(it.uuid, ITEM_UUID) + self.assertEqual(it.name, "T") + + def test_invalid_uuid_returns_none_without_request(self): + c = make_client() + with requests_mock.Mocker() as m: + self.assertIsNone(c.get_item("not-a-uuid")) + self.assertEqual(m.call_count, 0) + + +class TestGetBundles(unittest.TestCase): + + def test_by_parent_item_lists_bundles(self): + c = make_client() + parent = Item(item_json(ITEM_UUID)) + body = embedded("bundles", [ + bundle_json("b1", "ORIGINAL"), bundle_json("b2", "THUMBNAIL")]) + with requests_mock.Mocker() as m: + m.get(f"{API}/core/items/{ITEM_UUID}/bundles", json=body) + bundles = c.get_bundles(parent=parent, size=1000) + self.assertEqual([b.name for b in bundles], ["ORIGINAL", "THUMBNAIL"]) + self.assertTrue(all(isinstance(b, Bundle) for b in bundles)) + self.assertEqual(sent_params(m.last_request)["size"], ["1000"]) + + def test_by_uuid_returns_single_wrapped_in_list(self): + c = make_client() + with requests_mock.Mocker() as m: + m.get(f"{API}/core/bundles/b9", json=bundle_json("b9", "ORIGINAL")) + bundles = c.get_bundles(uuid="b9") + self.assertEqual(len(bundles), 1) + self.assertEqual(bundles[0].uuid, "b9") + + def test_deleted_item_404_returns_empty_list(self): + # PR #16 contract: a gone item is a clean empty result, not a crash. + c = make_client() + parent = Item(item_json(ITEM_UUID)) + with requests_mock.Mocker() as m: + m.get(f"{API}/core/items/{ITEM_UUID}/bundles", status_code=404, + json={"timestamp": "2026-01-01"}) + self.assertEqual(c.get_bundles(parent=parent), []) + + def test_no_args_returns_empty_without_request(self): + c = make_client() + with requests_mock.Mocker() as m: + self.assertEqual(c.get_bundles(), []) + self.assertEqual(m.call_count, 0) + + +class TestGetBitstreams(unittest.TestCase): + + def test_by_bundle_uses_embedded_link(self): + c = make_client() + href = f"{API}/core/bundles/bnd/bitstreams" + bundle = Bundle(bundle_json("bnd", bitstreams_href=href)) + body = embedded("bitstreams", [bitstream_json("s1", "a.pdf", size=10)]) + with requests_mock.Mocker() as m: + m.get(href, json=body) + bs = c.get_bitstreams(bundle=bundle, size=500) + self.assertEqual([b.uuid for b in bs], ["s1"]) + self.assertEqual(bs[0].sizeBytes, 10) + self.assertEqual(sent_params(m.last_request)["size"], ["500"]) + + def test_by_bundle_without_link_constructs_url(self): + c = make_client() + bundle = Bundle(bundle_json("bnd2")) # no _links -> manual URL + with requests_mock.Mocker() as m: + m.get(f"{API}/core/bundles/bnd2/bitstreams", + json=embedded("bitstreams", [])) + self.assertEqual(c.get_bitstreams(bundle=bundle), []) + + def test_no_args_returns_empty_list(self): + c = make_client() + with requests_mock.Mocker() as m: + self.assertEqual(c.get_bitstreams(), []) + self.assertEqual(m.call_count, 0) + + +class TestGetCollections(unittest.TestCase): + + def test_for_community_uses_collections_link(self): + c = make_client() + href = f"{API}/core/communities/c1/collections" + com = Community({"uuid": "c1", "name": "Com", "type": "community", + "_links": {"collections": {"href": href}}}) + body = embedded("collections", [ + {"uuid": "col1", "name": "Theses", "handle": "123/1", + "type": "collection"}]) + with requests_mock.Mocker() as m: + m.get(href, json=body) + cols = c.get_collections(community=com) + self.assertEqual([x.name for x in cols], ["Theses"]) + self.assertEqual(cols[0].handle, "123/1") + self.assertTrue(all(isinstance(x, Collection) for x in cols)) + + def test_plain_list(self): + c = make_client() + body = embedded("collections", [ + {"uuid": "col2", "name": "C2", "type": "collection"}]) + with requests_mock.Mocker() as m: + m.get(f"{API}/core/collections", json=body) + cols = c.get_collections() + self.assertEqual([x.uuid for x in cols], ["col2"]) + + +class TestGetCommunities(unittest.TestCase): + + def test_top_uses_search_top_endpoint(self): + c = make_client() + body = embedded("communities", [ + {"uuid": "c1", "name": "Top", "type": "community"}]) + with requests_mock.Mocker() as m: + m.get(f"{API}/core/communities/search/top", json=body) + coms = c.get_communities(top=True) + self.assertEqual([x.name for x in coms], ["Top"]) + self.assertTrue(all(isinstance(x, Community) for x in coms)) + + def test_plain_list(self): + c = make_client() + body = embedded("communities", [ + {"uuid": "c2", "name": "Other", "type": "community"}]) + with requests_mock.Mocker() as m: + m.get(f"{API}/core/communities", json=body) + coms = c.get_communities() + self.assertEqual([x.uuid for x in coms], ["c2"]) + + +class TestGetResourcePolicy(unittest.TestCase): + + def test_parses_live_policies_and_sends_uuid_action(self): + c = make_client() + body = embedded("resourcepolicies", [policy_json(pid=1)]) + with requests_mock.Mocker() as m: + m.get(f"{API}/authz/resourcepolicies/search/resource", json=body) + rps = c.get_resourcepolicy(BITSTREAM_UUID, action="READ") + self.assertEqual(len(rps), 1) + self.assertEqual(rps[0].groupName, "Anonymous") + p = sent_params(m.last_request) + self.assertEqual(p["uuid"], [BITSTREAM_UUID]) + self.assertEqual(p["action"], ["READ"]) + + def test_no_embedded_returns_empty_list(self): + c = make_client() + with requests_mock.Mocker() as m: + m.get(f"{API}/authz/resourcepolicies/search/resource", + json={"page": {"totalElements": 0}}) + self.assertEqual(c.get_resourcepolicy(BITSTREAM_UUID), []) + + def test_invalid_uuid_returns_none_without_request(self): + c = make_client() + with requests_mock.Mocker() as m: + self.assertIsNone(c.get_resourcepolicy("not-a-uuid")) + self.assertEqual(m.call_count, 0) + + def test_fetch_failure_returns_none(self): + c = make_client() + with requests_mock.Mocker() as m: + m.get(f"{API}/authz/resourcepolicies/search/resource", + status_code=500, text="boom") + self.assertIsNone(c.get_resourcepolicy(BITSTREAM_UUID)) + + +class TestFetchResource(unittest.TestCase): + + def test_200_returns_parsed_json(self): + c = make_client() + url = f"{API}/eperson/groups/search/byMetadata" + with requests_mock.Mocker() as m: + m.get(url, json=embedded("groups", [])) + self.assertEqual(c.fetch_resource(url, params={"query": "Anonymous"}), + {"_embedded": {"groups": []}}) + + def test_404_returns_none_and_records_last_err(self): + # group_uuid() and get_bundles() both branch on last_err.status_code. + c = make_client() + url = f"{API}/core/items/{ITEM_UUID}/bundles" + with requests_mock.Mocker() as m: + m.get(url, status_code=404, text="gone") + self.assertIsNone(c.fetch_resource(url)) + self.assertEqual(c.last_err.status_code, 404) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_client_write.py b/tests/test_client_write.py new file mode 100644 index 0000000..0f01e8d --- /dev/null +++ b/tests/test_client_write.py @@ -0,0 +1,164 @@ +""" +Write-path contract: the POST / DELETE methods this repo calls. + +The sync tooling only ever inspects a handful of things off these calls +(``ResourcePolicy.id`` after a create, the response ``status_code`` after a +delete, ``Bitstream.uuid`` after an upload); the tests pin the request that is +sent *and* the object that comes back. +""" +import os +import tempfile +import unittest + +import requests_mock + +import _helpers # noqa: F401 +from _helpers import ( + make_client, sent_params, bundle_json, bitstream_json, item_json, + API, ITEM_UUID, COLLECTION_UUID, BITSTREAM_UUID, ANON_GROUP_UUID) +from dspace_rest_client.models import Item, Bundle, Bitstream + + +class TestCreateResourcePolicy(unittest.TestCase): + + def test_success_sends_params_body_and_returns_policy(self): + c = make_client() + with requests_mock.Mocker() as m: + m.post(f"{API}/authz/resourcepolicies", status_code=201, + json={"id": 42, "action": "READ", "startDate": "2028-05-19", + "_embedded": {"group": {"name": "Anonymous", + "uuid": ANON_GROUP_UUID}}}) + rp = c.create_resourcepolicy( + resource_uuid=BITSTREAM_UUID, group_uuid=ANON_GROUP_UUID, + action="READ", start_date="2028-05-19") + self.assertIsNotNone(rp) + self.assertEqual(rp.id, 42) + p = sent_params(m.last_request) + self.assertEqual(p["resource"], [BITSTREAM_UUID]) + self.assertEqual(p["group"], [ANON_GROUP_UUID]) + body = m.last_request.json() + self.assertEqual(body["action"], "READ") + self.assertEqual(body["type"], "resourcepolicy") + self.assertEqual(body["startDate"], "2028-05-19") + + def test_without_start_date_omits_it_from_body(self): + c = make_client() + with requests_mock.Mocker() as m: + m.post(f"{API}/authz/resourcepolicies", status_code=201, + json={"id": 1, "action": "READ"}) + c.create_resourcepolicy(resource_uuid=BITSTREAM_UUID, + group_uuid=ANON_GROUP_UUID) + self.assertNotIn("startDate", m.last_request.json()) + + def test_invalid_uuid_returns_none_without_request(self): + c = make_client() + with requests_mock.Mocker() as m: + self.assertIsNone(c.create_resourcepolicy( + resource_uuid="bad", group_uuid=ANON_GROUP_UUID)) + self.assertEqual(m.call_count, 0) + + def test_server_error_returns_none(self): + c = make_client() + with requests_mock.Mocker() as m: + m.post(f"{API}/authz/resourcepolicies", status_code=500, text="boom") + self.assertIsNone(c.create_resourcepolicy( + resource_uuid=BITSTREAM_UUID, group_uuid=ANON_GROUP_UUID)) + + +class TestApiDelete(unittest.TestCase): + + def test_returns_response_with_status_code(self): + c = make_client() + url = f"{API}/authz/resourcepolicies/42" + with requests_mock.Mocker() as m: + m.delete(url, status_code=204) + self.assertEqual(c.api_delete(url, params=None).status_code, 204) + + def test_404_surfaces_as_response(self): + # files_access treats a 404 on delete as "already gone" == success. + c = make_client() + url = f"{API}/authz/resourcepolicies/7" + with requests_mock.Mocker() as m: + m.delete(url, status_code=404) + self.assertEqual(c.api_delete(url, params=None).status_code, 404) + + +class TestCreateBundle(unittest.TestCase): + + def test_posts_to_item_bundles_and_returns_bundle(self): + c = make_client() + parent = Item(item_json(ITEM_UUID)) + with requests_mock.Mocker() as m: + m.post(f"{API}/core/items/{ITEM_UUID}/bundles", status_code=201, + json=bundle_json("nb", "ORIGINAL")) + b = c.create_bundle(parent=parent) + self.assertIsInstance(b, Bundle) + self.assertEqual(b.uuid, "nb") + self.assertEqual(m.last_request.json(), + {"name": "ORIGINAL", "metadata": {}}) + + def test_none_parent_returns_none(self): + self.assertIsNone(make_client().create_bundle(parent=None)) + + +class TestCreateItem(unittest.TestCase): + + def test_posts_with_owning_collection_param_and_returns_item(self): + c = make_client() + item = Item({"name": "New thesis", "metadata": {}}) + with requests_mock.Mocker() as m: + m.post(f"{API}/core/items", status_code=201, + json=item_json("newu", "New thesis")) + out = c.create_item(parent=COLLECTION_UUID, item=item) + self.assertIsInstance(out, Item) + self.assertEqual(out.uuid, "newu") + self.assertEqual(sent_params(m.last_request)["owningCollection"], + [COLLECTION_UUID]) + + def test_non_item_returns_none(self): + self.assertIsNone(make_client().create_item( + parent=COLLECTION_UUID, item={"not": "an item"})) + + def test_none_parent_returns_none(self): + item = Item({"name": "x", "metadata": {}}) + self.assertIsNone(make_client().create_item(parent=None, item=item)) + + +class TestCreateBitstream(unittest.TestCase): + + def setUp(self): + fd, self.path = tempfile.mkstemp(suffix=".pdf") + with os.fdopen(fd, "wb") as fh: + fh.write(b"%PDF-1.4 hello world") + self.addCleanup(lambda: os.path.exists(self.path) and os.unlink(self.path)) + + def test_success_multipart_upload_returns_bitstream(self): + c = make_client() + bundle = Bundle(bundle_json("bnd")) + with requests_mock.Mocker() as m: + m.post(f"{API}/core/bundles/bnd/bitstreams", status_code=201, + json=bitstream_json("bsnew", "a.pdf", size=20)) + bs = c.create_bitstream( + bundle=bundle, name="a.pdf", path=self.path, + mime="application/pdf", + metadata={"dc.title": [{"value": "a.pdf"}]}) + self.assertIsInstance(bs, Bitstream) + self.assertEqual(bs.uuid, "bsnew") + self.assertEqual(bs.sizeBytes, 20) + # the request really was a multipart file upload + self.assertIn("multipart/form-data", + m.last_request.headers["Content-Type"]) + + def test_server_error_returns_none(self): + c = make_client() + bundle = Bundle(bundle_json("bnd")) + with requests_mock.Mocker() as m: + m.post(f"{API}/core/bundles/bnd/bitstreams", status_code=500, + text="boom") + self.assertIsNone(c.create_bitstream( + bundle=bundle, name="a.pdf", path=self.path, + mime="application/pdf")) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_models.py b/tests/test_models.py new file mode 100644 index 0000000..d18546d --- /dev/null +++ b/tests/test_models.py @@ -0,0 +1,138 @@ +""" +Model construction / accessor contract. + +This repo builds these model objects straight from cached or live API JSON and +reads a fixed set of attributes off them (``.uuid``, ``.name``, ``.metadata``, +``.sizeBytes``, ``.checkSum``, ``ResourcePolicy.groupUUID`` ...). If a rename or +a parsing change in the library dropped one of those, the audit/export/sync +tooling would break - these tests pin the shape. +""" +import unittest + +import _helpers # noqa: F401 (bootstraps sys.path for direct runs) +from dspace_rest_client.models import ( + Item, Community, Collection, Bundle, Bitstream, ResourcePolicy) + + +class TestItem(unittest.TestCase): + + def test_core_fields_from_api_resource(self): + it = Item({ + "uuid": "u1", "name": "Thesis", "type": "item", + "metadata": {"dc.title": [{"value": "Thesis"}]}, + "_links": {"self": {"href": "http://x/items/u1"}}, + }) + self.assertEqual(it.uuid, "u1") + self.assertEqual(it.name, "Thesis") + self.assertEqual(it.type, "item") + self.assertEqual(it.metadata["dc.title"][0]["value"], "Thesis") + self.assertEqual(it.links["self"]["href"], "http://x/items/u1") + + def test_type_is_item_even_without_explicit_type(self): + # ingest.dspace_be.create_item builds Item(dict) from a hand-made dict + # that has no "type" key; the class must still self-identify as an item. + it = Item({"name": "n", "metadata": {}}) + self.assertEqual(it.type, "item") + + def test_as_dict_carries_item_flags(self): + it = Item({"uuid": "u1", "name": "N", "metadata": {}, + "inArchive": True, "discoverable": True, "withdrawn": False}) + d = it.as_dict() + self.assertEqual(d["uuid"], "u1") + self.assertEqual(d["type"], "item") + self.assertEqual( + (d["inArchive"], d["discoverable"], d["withdrawn"]), + (True, True, False)) + + +class TestCommunityCollection(unittest.TestCase): + + def test_community_fields_and_links(self): + com = Community({ + "uuid": "c1", "name": "Faculty", "type": "community", + "_links": {"collections": {"href": "http://x/communities/c1/collections"}}, + }) + self.assertEqual(com.uuid, "c1") + self.assertEqual(com.name, "Faculty") + self.assertEqual(com.type, "community") + self.assertEqual(com.links["collections"]["href"], + "http://x/communities/c1/collections") + + def test_collection_fields_including_handle(self): + col = Collection({"uuid": "col1", "name": "Theses", + "handle": "123456789/1", "type": "collection"}) + self.assertEqual(col.uuid, "col1") + self.assertEqual(col.name, "Theses") + self.assertEqual(col.handle, "123456789/1") + self.assertEqual(col.type, "collection") + + +class TestBundleBitstream(unittest.TestCase): + + def test_bundle_fields_and_bitstreams_link(self): + # get_bitstreams(bundle=...) prefers this embedded link over a manually + # constructed URL, so it is part of the contract. + b = Bundle({"uuid": "b1", "name": "ORIGINAL", "type": "bundle", + "metadata": {}, + "_links": {"bitstreams": {"href": "http://x/bundles/b1/bitstreams"}}}) + self.assertEqual((b.uuid, b.name, b.type), ("b1", "ORIGINAL", "bundle")) + self.assertEqual(b.links["bitstreams"]["href"], + "http://x/bundles/b1/bitstreams") + + def test_bitstream_file_fields(self): + # export/_dspace serialises exactly these attributes per bitstream. + b = Bitstream({"uuid": "s1", "name": "f.pdf", "type": "bitstream", + "metadata": {"dc.title": [{"value": "f.pdf"}]}, + "sizeBytes": 2048, "sequenceId": 3, + "checkSum": {"checkSumAlgorithm": "MD5", "value": "deadbeef"}}) + self.assertEqual(b.uuid, "s1") + self.assertEqual(b.name, "f.pdf") + self.assertEqual(b.sizeBytes, 2048) + self.assertEqual(b.sequenceId, 3) + self.assertEqual(b.checkSum["value"], "deadbeef") + d = b.as_dict() + self.assertEqual(d["sizeBytes"], 2048) + self.assertEqual(d["checkSum"]["value"], "deadbeef") + self.assertEqual(d["sequenceId"], 3) + + +class TestResourcePolicy(unittest.TestCase): + + def test_direct_cached_format(self): + # the shape produced by ResourcePolicy.as_dict() and re-read from cache + rp = ResourcePolicy({"id": 5, "action": "READ", "groupName": "Anonymous", + "groupUUID": "g1", "startDate": "2028-01-01", + "endDate": None}) + self.assertEqual(rp.id, 5) + self.assertEqual(rp.action, "READ") + self.assertEqual(rp.groupName, "Anonymous") + self.assertEqual(rp.groupUUID, "g1") + self.assertEqual(rp.startDate, "2028-01-01") + self.assertIsNone(rp.endDate) + + def test_live_api_embedded_group_format(self): + # This is what /authz/resourcepolicies actually returns; files_access + # relies on groupName/groupUUID being lifted out of _embedded.group. + rp = ResourcePolicy({"id": 9, "action": "READ", "startDate": "2028-05-19", + "_embedded": {"group": {"name": "Anonymous", + "uuid": "anon-uuid"}}}) + self.assertEqual(rp.groupName, "Anonymous") + self.assertEqual(rp.groupUUID, "anon-uuid") + + def test_as_dict_roundtrip_keeps_group_and_action(self): + rp = ResourcePolicy({"id": 7, "action": "READ", + "_embedded": {"group": {"name": "Anonymous", + "uuid": "anon"}}}) + d = rp.as_dict() + self.assertEqual(d["id"], 7) + self.assertEqual(d["action"], "READ") + self.assertEqual(d["groupName"], "Anonymous") + self.assertEqual(d["groupUUID"], "anon") + # a re-parse of the cached dict must survive the round-trip + rp2 = ResourcePolicy(d) + self.assertEqual(rp2.groupUUID, "anon") + self.assertEqual(rp2.action, "READ") + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_repo_usage_contract.py b/tests/test_repo_usage_contract.py new file mode 100644 index 0000000..460d708 --- /dev/null +++ b/tests/test_repo_usage_contract.py @@ -0,0 +1,154 @@ +""" +Integration contract: the exact multi-call sequences DSpace-ISstag-integration +runs against this library. + +Where the per-method tests pin one call, these replay a whole flow end-to-end +(only the HTTP transport mocked) so that a library change which individually +looks harmless but breaks a *chain* our tooling depends on still fails here. + +Each test names the source it mirrors. +""" +import unittest +from types import SimpleNamespace + +import requests_mock + +import _helpers # noqa: F401 +from _helpers import ( + make_client, embedded, item_json, bundle_json, bitstream_json, + policy_json, API, ITEM_UUID, BITSTREAM_UUID, ANON_GROUP_UUID) +from dspace_rest_client.models import Item + + +class TestBitstreamExportChain(unittest.TestCase): + """Mirrors src/export/_dspace.py :: exporter.export_bitstreams - + get_bundles(parent) -> get_resourcepolicy(bundle) -> get_bitstreams(bundle) + -> get_resourcepolicy(bitstream), then serialises a fixed set of attrs.""" + + # real UUIDs: the export chain calls get_resourcepolicy() on the bundle and + # bitstream, and that method validates (and short-circuits on) its UUID arg. + BUNDLE_UUID = "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa" + + def test_full_chain_yields_serialisable_attributes(self): + c = make_client() + item = Item(item_json(ITEM_UUID)) + bits_href = f"{API}/core/bundles/{self.BUNDLE_UUID}/bitstreams" + with requests_mock.Mocker() as m: + m.get(f"{API}/core/items/{ITEM_UUID}/bundles", + json=embedded("bundles", + [bundle_json(self.BUNDLE_UUID, "ORIGINAL", + bitstreams_href=bits_href)])) + m.get(bits_href, + json=embedded("bitstreams", + [bitstream_json(BITSTREAM_UUID, "thesis.pdf", + size=123, seq=1, checksum="abc")])) + m.get(f"{API}/authz/resourcepolicies/search/resource", + json=embedded("resourcepolicies", [policy_json(pid=1)])) + + bundles = c.get_bundles(parent=item, size=1000) + self.assertEqual(len(bundles), 1) + bundle = bundles[0] + self.assertEqual((bundle.name, bundle.uuid, bundle.type), + ("ORIGINAL", self.BUNDLE_UUID, "bundle")) + + bundle_rp = c.get_resourcepolicy(bundle.uuid) + self.assertEqual([rp.as_dict()["groupName"] for rp in bundle_rp], + ["Anonymous"]) + + bitstreams = c.get_bitstreams(bundle=bundle, size=1000) + self.assertEqual(len(bitstreams), 1) + b = bitstreams[0] + # the exporter reads exactly these off each bitstream + self.assertEqual((b.name, b.uuid, b.sizeBytes, b.sequenceId), + ("thesis.pdf", BITSTREAM_UUID, 123, 1)) + self.assertEqual(b.checkSum["value"], "abc") + + bs_rp = c.get_resourcepolicy(b.uuid) + self.assertEqual(bs_rp[0].as_dict()["groupUUID"], ANON_GROUP_UUID) + + +class TestPolicyReplacementChain(unittest.TestCase): + """Mirrors src/reposync/_files.py :: files_access._set_read_policy - + read live policies, create a fresh Anonymous READ policy, delete the old.""" + + def test_read_create_delete(self): + c = make_client() + with requests_mock.Mocker() as m: + m.get(f"{API}/authz/resourcepolicies/search/resource", + json=embedded("resourcepolicies", [policy_json(pid=11)])) + m.post(f"{API}/authz/resourcepolicies", status_code=201, + json={"id": 99, "action": "READ", "startDate": "2028-05-19", + "_embedded": {"group": {"name": "Anonymous", + "uuid": ANON_GROUP_UUID}}}) + m.delete(f"{API}/authz/resourcepolicies/11", status_code=204) + + live = c.get_resourcepolicy(BITSTREAM_UUID, action="READ") + # _set_read_policy reads id / action / groupName off each live policy + self.assertEqual(live[0].id, 11) + self.assertEqual(live[0].action, "READ") + self.assertEqual(live[0].groupName, "Anonymous") + + new = c.create_resourcepolicy( + resource_uuid=BITSTREAM_UUID, group_uuid=ANON_GROUP_UUID, + action="READ", start_date="2028-05-19") + self.assertEqual(new.id, 99) + + r = c.api_delete( + f"{API}/authz/resourcepolicies/{live[0].id}", params=None) + self.assertEqual(r.status_code, 204) + + +class TestMcpBundleWalk(unittest.TestCase): + """Mirrors mcp/core.py :: make_service_from_env - lookup via search_objects, + then get_item -> get_bundles(SimpleNamespace parent) -> get_bitstreams.""" + + def test_lookup_then_item_bundles_bitstreams(self): + c = make_client() + bits_href = f"{API}/core/bundles/bnd/bitstreams" + with requests_mock.Mocker() as m: + m.get(f"{API}/discover/search/objects", json={"_embedded": { + "searchResult": {"page": {"totalElements": 1}, + "_embedded": {"objects": [ + {"_embedded": {"indexableObject": + item_json(ITEM_UUID, "T")}}]}}}}) + m.get(f"{API}/core/items/{ITEM_UUID}", json=item_json(ITEM_UUID, "T")) + m.get(f"{API}/core/items/{ITEM_UUID}/bundles", + json=embedded("bundles", + [bundle_json("bnd", "ORIGINAL", + bitstreams_href=bits_href)])) + m.get(bits_href, + json=embedded("bitstreams", [bitstream_json("bs1", "a.pdf")])) + + matches = c.search_objects(query="dc.identifier:42") + self.assertEqual(matches[0].uuid, ITEM_UUID) + + item = c.get_item(ITEM_UUID) + self.assertEqual(item.uuid, ITEM_UUID) + + # mcp passes a duck-typed parent (only .uuid), not a real Item + parent = SimpleNamespace(uuid=item.uuid) + bundles = c.get_bundles(parent=parent, size=200) + self.assertEqual(bundles[0].name, "ORIGINAL") + + bitstreams = c.get_bitstreams(bundle=bundles[0], size=500) + self.assertEqual(bitstreams[0].uuid, "bs1") + + +class TestGroupSearchForUuidResolution(unittest.TestCase): + """Mirrors src/ingest/_dspace.py :: dspace_be.group_uuid - a raw + fetch_resource on the group-search endpoint, reading _embedded.groups.""" + + def test_group_search_shape(self): + c = make_client() + url = f"{API}/eperson/groups/search/byMetadata" + with requests_mock.Mocker() as m: + m.get(url, json=embedded("groups", [ + {"name": "Anonymous", "uuid": "anon-uuid"}])) + r = c.fetch_resource(url, params={"query": "Anonymous", "size": 100}) + groups = (r.get("_embedded") or {}).get("groups") or [] + self.assertEqual(groups[0]["name"], "Anonymous") + self.assertEqual(groups[0]["uuid"], "anon-uuid") + + +if __name__ == "__main__": + unittest.main() From 2687c6c4d89f2cb6eec17daff69b130f3f7cd85c Mon Sep 17 00:00:00 2001 From: jm Date: Mon, 17 Aug 2026 18:28:49 +0200 Subject: [PATCH 22/28] test: harden suite after adversarial review (faithfulness + gaps) Acting on a two-reviewer audit (workflow + Opus-5 advisor): Faithfulness fixes (tests that could pass against a broken library): - get_bitstreams embedded-link test used an href byte-identical to the fallback URL, so it could not distinguish the branches; give it a distinct href that the fallback could not produce. - MCP chain fed a link-bearing Bundle to get_bitstreams; the real consumer round-trips through as_dict() (drops _links) and hits the fallback URL - rebuild the Bundle from as_dict() and assert the fallback is used. - export chain fetched policies with action='READ' (raw-client default) but the real exporter goes through a wrapper defaulting to action=None (no filter); call with action=None and assert no action param is sent. - get_resourcepolicy empty test used a no-_embedded body (defensive branch); the live API returns an _embedded envelope even when empty - use that. Coverage / stronger assertions: - get_resourcepolicy action=None omits the filter and returns all actions. - create_item now asserts the POST body (name/metadata/type/flags), not just the uuid; create_bitstream asserts the multipart 'properties' payload (name/bundleName/metadata). - search_objects result now asserts .as_dict() and links['self']['href'], the two accessors every consumer reads. - model tests assert parsed .metadata and checkSum.checkSumAlgorithm (dropped the tautological hard-set .type assertions' reliance). - get_bitstreams non-200, and create_item/create_bundle server-error: characterization tests pinning the current (non-fail-safe) behavior the consumers depend on, flagged in-comment for a future library hardening. 60 tests, still no network. Co-Authored-By: Claude Opus 4.8 --- requirements-test.txt | 6 ++-- tests/_helpers.py | 17 +++++++++ tests/test_client_read.py | 59 +++++++++++++++++++++++++++---- tests/test_client_write.py | 49 +++++++++++++++++++++++-- tests/test_models.py | 9 ++++- tests/test_repo_usage_contract.py | 41 +++++++++++++++------ 6 files changed, 158 insertions(+), 23 deletions(-) diff --git a/requirements-test.txt b/requirements-test.txt index f061c74..575ad6c 100644 --- a/requirements-test.txt +++ b/requirements-test.txt @@ -1,5 +1,7 @@ # Test-only dependencies for the dspace_rest_client suite. -# The library itself only needs `requests` (see setup.py); these add the test -# runner and an HTTP transport mock so tests never touch a real DSpace server. +# The test runner and an HTTP transport mock so tests never touch a real DSpace +# server. `requests` is listed explicitly so `pip install -r requirements-test.txt` +# alone is enough to import the package (CI also `pip install .`s it via setup.py). pytest>=7.0 requests-mock>=1.11 +requests diff --git a/tests/_helpers.py b/tests/_helpers.py index 1b39ed7..6fdbecf 100644 --- a/tests/_helpers.py +++ b/tests/_helpers.py @@ -7,7 +7,9 @@ response parsing - the two things downstream code (this repo) depends on - fails a test instead of silently shipping. """ +import json import os +import re import sys from urllib.parse import urlparse, parse_qs @@ -52,6 +54,21 @@ def sent_params(request) -> dict: return parse_qs(urlparse(request.url).query) +def multipart_properties(request) -> dict: + """Parse the JSON ``properties`` part of a create_bitstream multipart body. + + ``create_bitstream`` sends ``properties = json.dumps({name, metadata, + bundleName}) + ';application/json'`` as a form field. This is what actually + carries the bitstream's metadata to DSpace, so tests assert on it. + """ + body = request.body + if isinstance(body, bytes): + body = body.decode("utf-8", "replace") + m = re.search(r'name="properties"\r?\n\r?\n(.*?);application/json', + body, re.DOTALL) + return json.loads(m.group(1)) if m else None + + # --- response-body builders (shape mirrors the DSpace 7 REST API) --------- # def embedded(key: str, resources: list) -> dict: diff --git a/tests/test_client_read.py b/tests/test_client_read.py index f3a8bb7..ae08c4c 100644 --- a/tests/test_client_read.py +++ b/tests/test_client_read.py @@ -20,10 +20,14 @@ class TestSearchObjects(unittest.TestCase): def test_builds_url_params_and_parses_objects(self): c = make_client() + # every caller's next step is dso.as_dict() + dso.links['self']['href'] + # (repo._search.dso2dict, mcp._dso_to_dict), so the search result must + # carry both - include a self link to prove it survives parsing. + obj1 = item_json("u1", "A", _links={"self": {"href": f"{API}/items/u1"}}) body = {"_embedded": {"searchResult": { "page": {"totalElements": 2, "size": 100}, "_embedded": {"objects": [ - {"_embedded": {"indexableObject": item_json("u1", "A")}}, + {"_embedded": {"indexableObject": obj1}}, {"_embedded": {"indexableObject": item_json("u2", "B")}}, ]}}}} with requests_mock.Mocker() as m: @@ -32,6 +36,9 @@ def test_builds_url_params_and_parses_objects(self): res = c.search_objects(query="dc.identifier:123", size=100, page=0, details=details) self.assertEqual([d.uuid for d in res], ["u1", "u2"]) + # the two accessors every consumer reads off a search hit + self.assertEqual(res[0].links["self"]["href"], f"{API}/items/u1") + self.assertEqual(res[0].as_dict()["uuid"], "u1") p = sent_params(m.last_request) self.assertEqual(p["query"], ["dc.identifier:123"]) self.assertEqual(p["size"], ["100"]) @@ -132,7 +139,10 @@ class TestGetBitstreams(unittest.TestCase): def test_by_bundle_uses_embedded_link(self): c = make_client() - href = f"{API}/core/bundles/bnd/bitstreams" + # href deliberately NOT equal to the fallback URL (.../bundles/bnd/ + # bitstreams): if the embedded-link branch were removed the client would + # build the fallback, which is unmocked, and this test would fail. + href = f"{API}/core/bundles/HREF-ONLY-PATH/bitstreams" bundle = Bundle(bundle_json("bnd", bitstreams_href=href)) body = embedded("bitstreams", [bitstream_json("s1", "a.pdf", size=10)]) with requests_mock.Mocker() as m: @@ -140,6 +150,7 @@ def test_by_bundle_uses_embedded_link(self): bs = c.get_bitstreams(bundle=bundle, size=500) self.assertEqual([b.uuid for b in bs], ["s1"]) self.assertEqual(bs[0].sizeBytes, 10) + self.assertEqual(m.last_request.url.split("?")[0], href) self.assertEqual(sent_params(m.last_request)["size"], ["500"]) def test_by_bundle_without_link_constructs_url(self): @@ -147,8 +158,12 @@ def test_by_bundle_without_link_constructs_url(self): bundle = Bundle(bundle_json("bnd2")) # no _links -> manual URL with requests_mock.Mocker() as m: m.get(f"{API}/core/bundles/bnd2/bitstreams", - json=embedded("bitstreams", [])) - self.assertEqual(c.get_bitstreams(bundle=bundle), []) + json=embedded("bitstreams", + [bitstream_json("s9", "x.pdf", size=7)])) + bs = c.get_bitstreams(bundle=bundle) + # proves both the constructed URL AND parsing on the fallback path + self.assertEqual([b.uuid for b in bs], ["s9"]) + self.assertEqual(bs[0].sizeBytes, 7) def test_no_args_returns_empty_list(self): c = make_client() @@ -156,6 +171,21 @@ def test_no_args_returns_empty_list(self): self.assertEqual(c.get_bitstreams(), []) self.assertEqual(m.call_count, 0) + def test_non_200_currently_raises_no_failsafe(self): + # CHARACTERIZATION of a known sharp edge: unlike get_bundles (which since + # PR #16 returns [] on a 404), get_bitstreams has no fail-safe - a non-200 + # makes fetch_resource return None which this method then subscripts, so + # it raises. The consumers (export/_dspace, reposync/_files) iterate the + # result unguarded, so this is a real crash risk. Pinned deliberately: if + # the library is hardened to return [], update this test to assert that. + c = make_client() + bundle = Bundle(bundle_json("bnd2")) + with requests_mock.Mocker() as m: + m.get(f"{API}/core/bundles/bnd2/bitstreams", + status_code=500, text="boom") + with self.assertRaises(Exception): + c.get_bitstreams(bundle=bundle) + class TestGetCollections(unittest.TestCase): @@ -220,11 +250,28 @@ def test_parses_live_policies_and_sends_uuid_action(self): self.assertEqual(p["uuid"], [BITSTREAM_UUID]) self.assertEqual(p["action"], ["READ"]) - def test_no_embedded_returns_empty_list(self): + def test_action_none_omits_the_action_filter(self): + # The bitstream export path calls this via the ingest wrapper whose + # default is action=None (ingest/_dspace.py get_resourcepolicy), which + # must fetch policies of ALL actions - so no `action` param is sent. + c = make_client() + body = embedded("resourcepolicies", [ + policy_json(pid=1, action="READ"), + policy_json(pid=2, action="WRITE")]) + with requests_mock.Mocker() as m: + m.get(f"{API}/authz/resourcepolicies/search/resource", json=body) + rps = c.get_resourcepolicy(BITSTREAM_UUID, action=None) + self.assertEqual([rp.action for rp in rps], ["READ", "WRITE"]) + p = sent_params(m.last_request) + self.assertEqual(p["uuid"], [BITSTREAM_UUID]) + self.assertNotIn("action", p) + + def test_empty_result_set_returns_empty_list(self): + # The live endpoint returns an _embedded envelope even when empty. c = make_client() with requests_mock.Mocker() as m: m.get(f"{API}/authz/resourcepolicies/search/resource", - json={"page": {"totalElements": 0}}) + json=embedded("resourcepolicies", [])) self.assertEqual(c.get_resourcepolicy(BITSTREAM_UUID), []) def test_invalid_uuid_returns_none_without_request(self): diff --git a/tests/test_client_write.py b/tests/test_client_write.py index 0f01e8d..ee3922e 100644 --- a/tests/test_client_write.py +++ b/tests/test_client_write.py @@ -14,8 +14,9 @@ import _helpers # noqa: F401 from _helpers import ( - make_client, sent_params, bundle_json, bitstream_json, item_json, - API, ITEM_UUID, COLLECTION_UUID, BITSTREAM_UUID, ANON_GROUP_UUID) + make_client, sent_params, multipart_properties, bundle_json, + bitstream_json, item_json, API, ITEM_UUID, COLLECTION_UUID, + BITSTREAM_UUID, ANON_GROUP_UUID) from dspace_rest_client.models import Item, Bundle, Bitstream @@ -100,6 +101,21 @@ def test_posts_to_item_bundles_and_returns_bundle(self): def test_none_parent_returns_none(self): self.assertIsNone(make_client().create_bundle(parent=None)) + def test_server_error_returns_truthy_uuidless_bundle(self): + # CHARACTERIZATION: like create_item, create_bundle wraps the response + # unconditionally -> a truthy Bundle with uuid=None on failure, not None. + # The importer's `if not bundle` guard (reposync/_importer.py:118-120) + # never fires because a Bundle instance is always truthy. Pinned. + c = make_client() + parent = Item(item_json(ITEM_UUID)) + with requests_mock.Mocker() as m: + m.post(f"{API}/core/items/{ITEM_UUID}/bundles", + status_code=500, text="boom") + out = c.create_bundle(parent=parent) + self.assertIsInstance(out, Bundle) + self.assertIsNone(out.uuid) + self.assertTrue(out) + class TestCreateItem(unittest.TestCase): @@ -114,6 +130,27 @@ def test_posts_with_owning_collection_param_and_returns_item(self): self.assertEqual(out.uuid, "newu") self.assertEqual(sent_params(m.last_request)["owningCollection"], [COLLECTION_UUID]) + # the POST body is item.as_dict() - this is how the importer's built + # metadata actually reaches DSpace, so pin it, not just the uuid. + body = m.last_request.json() + self.assertEqual(body["name"], "New thesis") + self.assertEqual(body["type"], "item") + self.assertEqual(body["metadata"], {}) + self.assertIs(body["inArchive"], True) + + def test_server_error_returns_truthy_uuidless_item(self): + # CHARACTERIZATION: create_item wraps the response unconditionally, so a + # failed create yields a truthy Item with uuid=None, NOT None. The + # importer guards with `if dso is None` (reposync/_importer.py:127-129), + # which therefore never fires on failure. Pinned; see the fail-safe note. + c = make_client() + item = Item({"name": "x", "metadata": {}}) + with requests_mock.Mocker() as m: + m.post(f"{API}/core/items", status_code=500, text="boom") + out = c.create_item(parent=COLLECTION_UUID, item=item) + self.assertIsInstance(out, Item) + self.assertIsNone(out.uuid) + self.assertTrue(out) # truthy despite the failure def test_non_item_returns_none(self): self.assertIsNone(make_client().create_item( @@ -145,9 +182,15 @@ def test_success_multipart_upload_returns_bitstream(self): self.assertIsInstance(bs, Bitstream) self.assertEqual(bs.uuid, "bsnew") self.assertEqual(bs.sizeBytes, 20) - # the request really was a multipart file upload + # the request really was a multipart file upload... self.assertIn("multipart/form-data", m.last_request.headers["Content-Type"]) + # ...carrying the name/bundleName/metadata that actually attach the + # bitstream's metadata in DSpace (reposync/_utils.create_new_bitstream) + props = multipart_properties(m.last_request) + self.assertEqual(props["name"], "a.pdf") + self.assertEqual(props["bundleName"], "ORIGINAL") # == bundle.name + self.assertEqual(props["metadata"], {"dc.title": [{"value": "a.pdf"}]}) def test_server_error_returns_none(self): c = make_client() diff --git a/tests/test_models.py b/tests/test_models.py index d18546d..2e06059 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -73,9 +73,12 @@ def test_bundle_fields_and_bitstreams_link(self): # get_bitstreams(bundle=...) prefers this embedded link over a manually # constructed URL, so it is part of the contract. b = Bundle({"uuid": "b1", "name": "ORIGINAL", "type": "bundle", - "metadata": {}, + "metadata": {"dc.title": [{"value": "ORIGINAL"}]}, "_links": {"bitstreams": {"href": "http://x/bundles/b1/bitstreams"}}}) self.assertEqual((b.uuid, b.name, b.type), ("b1", "ORIGINAL", "bundle")) + # .metadata is parsed from the response (unlike .type, a class constant) + # and is serialised by export/_dspace.py:323, so pin it. + self.assertEqual(b.metadata, {"dc.title": [{"value": "ORIGINAL"}]}) self.assertEqual(b.links["bitstreams"]["href"], "http://x/bundles/b1/bitstreams") @@ -90,6 +93,10 @@ def test_bitstream_file_fields(self): self.assertEqual(b.sizeBytes, 2048) self.assertEqual(b.sequenceId, 3) self.assertEqual(b.checkSum["value"], "deadbeef") + # the checksum verifier compares checkSumAlgorithm == "MD5" + # (reposync/_files.py:187-189); .metadata is serialised by the exporter. + self.assertEqual(b.checkSum["checkSumAlgorithm"], "MD5") + self.assertEqual(b.metadata, {"dc.title": [{"value": "f.pdf"}]}) d = b.as_dict() self.assertEqual(d["sizeBytes"], 2048) self.assertEqual(d["checkSum"]["value"], "deadbeef") diff --git a/tests/test_repo_usage_contract.py b/tests/test_repo_usage_contract.py index 460d708..55b57f6 100644 --- a/tests/test_repo_usage_contract.py +++ b/tests/test_repo_usage_contract.py @@ -15,9 +15,9 @@ import _helpers # noqa: F401 from _helpers import ( - make_client, embedded, item_json, bundle_json, bitstream_json, + make_client, sent_params, embedded, item_json, bundle_json, bitstream_json, policy_json, API, ITEM_UUID, BITSTREAM_UUID, ANON_GROUP_UUID) -from dspace_rest_client.models import Item +from dspace_rest_client.models import Item, Bundle class TestBitstreamExportChain(unittest.TestCase): @@ -33,15 +33,19 @@ def test_full_chain_yields_serialisable_attributes(self): c = make_client() item = Item(item_json(ITEM_UUID)) bits_href = f"{API}/core/bundles/{self.BUNDLE_UUID}/bitstreams" + bundle_md = {"dc.title": [{"value": "ORIGINAL"}]} + bs_md = {"dc.description": [{"value": "VŠKP"}]} with requests_mock.Mocker() as m: m.get(f"{API}/core/items/{ITEM_UUID}/bundles", json=embedded("bundles", [bundle_json(self.BUNDLE_UUID, "ORIGINAL", - bitstreams_href=bits_href)])) + bitstreams_href=bits_href, + metadata=bundle_md)])) m.get(bits_href, json=embedded("bitstreams", [bitstream_json(BITSTREAM_UUID, "thesis.pdf", - size=123, seq=1, checksum="abc")])) + size=123, seq=1, checksum="abc", + metadata=bs_md)])) m.get(f"{API}/authz/resourcepolicies/search/resource", json=embedded("resourcepolicies", [policy_json(pid=1)])) @@ -50,20 +54,26 @@ def test_full_chain_yields_serialisable_attributes(self): bundle = bundles[0] self.assertEqual((bundle.name, bundle.uuid, bundle.type), ("ORIGINAL", self.BUNDLE_UUID, "bundle")) + self.assertEqual(bundle.metadata, bundle_md) - bundle_rp = c.get_resourcepolicy(bundle.uuid) + # the exporter calls get_resourcepolicy via a wrapper defaulting to + # action=None (ingest/_dspace.py), so NO action filter is sent. + bundle_rp = c.get_resourcepolicy(bundle.uuid, action=None) self.assertEqual([rp.as_dict()["groupName"] for rp in bundle_rp], ["Anonymous"]) + self.assertNotIn("action", sent_params(m.last_request)) bitstreams = c.get_bitstreams(bundle=bundle, size=1000) self.assertEqual(len(bitstreams), 1) b = bitstreams[0] - # the exporter reads exactly these off each bitstream + # a representative set of the attributes the exporter serialises + # (src/export/_dspace.py:333-340) - name/uuid/size/seq/checksum/meta self.assertEqual((b.name, b.uuid, b.sizeBytes, b.sequenceId), ("thesis.pdf", BITSTREAM_UUID, 123, 1)) self.assertEqual(b.checkSum["value"], "abc") + self.assertEqual(b.metadata, bs_md) - bs_rp = c.get_resourcepolicy(b.uuid) + bs_rp = c.get_resourcepolicy(b.uuid, action=None) self.assertEqual(bs_rp[0].as_dict()["groupUUID"], ANON_GROUP_UUID) @@ -104,7 +114,11 @@ class TestMcpBundleWalk(unittest.TestCase): def test_lookup_then_item_bundles_bitstreams(self): c = make_client() - bits_href = f"{API}/core/bundles/bnd/bitstreams" + # mcp/core.py drops _links (it round-trips bundles through as_dict), so + # get_bitstreams must use the manually-constructed fallback URL, not the + # embedded link. The link below is a decoy that must NOT be requested. + decoy_href = f"{API}/core/bundles/DECOY-LINK/bitstreams" + fallback = f"{API}/core/bundles/bnd/bitstreams" with requests_mock.Mocker() as m: m.get(f"{API}/discover/search/objects", json={"_embedded": { "searchResult": {"page": {"totalElements": 1}, @@ -115,8 +129,8 @@ def test_lookup_then_item_bundles_bitstreams(self): m.get(f"{API}/core/items/{ITEM_UUID}/bundles", json=embedded("bundles", [bundle_json("bnd", "ORIGINAL", - bitstreams_href=bits_href)])) - m.get(bits_href, + bitstreams_href=decoy_href)])) + m.get(fallback, json=embedded("bitstreams", [bitstream_json("bs1", "a.pdf")])) matches = c.search_objects(query="dc.identifier:42") @@ -130,8 +144,13 @@ def test_lookup_then_item_bundles_bitstreams(self): bundles = c.get_bundles(parent=parent, size=200) self.assertEqual(bundles[0].name, "ORIGINAL") - bitstreams = c.get_bitstreams(bundle=bundles[0], size=500) + # mirror mcp: rebuild the Bundle from as_dict() (which strips _links) + # so get_bitstreams takes the fallback-URL branch the consumer hits. + bstub = Bundle(bundles[0].as_dict()) + self.assertNotIn("bitstreams", bstub.links) + bitstreams = c.get_bitstreams(bundle=bstub, size=500) self.assertEqual(bitstreams[0].uuid, "bs1") + self.assertEqual(m.last_request.url.split("?")[0], fallback) class TestGroupSearchForUuidResolution(unittest.TestCase): From ea3e670df354ff07049a5e84749636378a3be2c8 Mon Sep 17 00:00:00 2001 From: jm Date: Mon, 17 Aug 2026 18:45:16 +0200 Subject: [PATCH 23/28] ci: run push builds only on dtq dtq-dev no longer exists, and we only care about dtq for now. PRs still trigger via the pull_request event. Co-Authored-By: Claude Opus 4.8 --- .github/workflows/tests.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 0179525..affd373 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -2,7 +2,7 @@ name: Tests on: push: - branches: [ dtq, dtq-dev, main ] + branches: [ dtq ] pull_request: permissions: From a2b714079c932cd78dd4b2ca94f9094f4eb7081a Mon Sep 17 00:00:00 2001 From: jm Date: Mon, 17 Aug 2026 18:54:48 +0200 Subject: [PATCH 24/28] fix(client): fail-safe get_bitstreams + None on failed create_item/bundle Review follow-ups, each confirmed by the tests: - get_bitstreams: a 404 now returns [] (a gone bundle has no bitstreams), mirroring the get_bundles #16 fix; other errors still surface so a transient 5xx is retried, not swallowed; a 200 with no bitstreams returns [] not None. The consumers (export/_dspace, reposync/_files) iterate the result unguarded. - create_item / create_bundle: return None on a non-2xx response instead of a truthy uuid-less object, so the importer's `if dso is None` / `if not bundle` guards actually fire. Also addresses the Copilot review: the multipart_properties test helper now asserts the 'properties' part exists (fails loudly) instead of returning None. Tests updated to assert the new behavior. 62 tests, no network. Co-Authored-By: Claude Opus 4.8 --- dspace_rest_client/client.py | 33 +++++++++++++++++++++++++-------- tests/_helpers.py | 5 ++++- tests/test_client_read.py | 30 +++++++++++++++++++++++------- tests/test_client_write.py | 26 ++++++++------------------ 4 files changed, 60 insertions(+), 34 deletions(-) diff --git a/dspace_rest_client/client.py b/dspace_rest_client/client.py index 31166cc..7049228 100644 --- a/dspace_rest_client/client.py +++ b/dspace_rest_client/client.py @@ -764,7 +764,13 @@ def create_bundle(self, parent=None, name='ORIGINAL'): if parent is None: return None url = f'{self.API_ENDPOINT}/core/items/{parent.uuid}/bundles' - return Bundle(api_resource=parse_json(self.api_post(url, params=None, json={'name': name, 'metadata': {}}))) + r = self.api_post(url, params=None, json={'name': name, 'metadata': {}}) + if r.status_code not in (200, 201): + # return None on failure (not a uuid-less Bundle) so callers' + # `if not bundle` guards actually fire + _logger.error(f'Failed to create bundle: {r.status_code}: {r.text}') + return None + return Bundle(api_resource=parse_json(r)) # PAGINATION def get_bitstreams(self, uuid=None, bundle=None, page=0, size=20, sort=None): @@ -794,12 +800,18 @@ def get_bitstreams(self, uuid=None, bundle=None, page=0, size=20, sort=None): if sort is not None: params['sort'] = sort r_json = self.fetch_resource(url, params=params) - if '_embedded' in r_json: - if 'bitstreams' in r_json['_embedded']: - bitstreams = list() - for bitstream_resource in r_json['_embedded']['bitstreams']: - bitstreams.append(Bitstream(bitstream_resource)) - return bitstreams + if r_json is None and getattr(self._last_err, 'status_code', None) == 404: + # the bundle (or item) is gone - no bitstreams, a clean empty result + # rather than a crash. Mirrors get_bundles (#16). Any other failure + # (a transient 5xx, say) falls through and still surfaces to the + # caller so it is retried, not silently recorded as "no bitstreams". + _logger.info(f'No bitstreams: resource not found (404) [{url}]') + return list() + bitstreams = list() + if '_embedded' in r_json and 'bitstreams' in r_json['_embedded']: + for bitstream_resource in r_json['_embedded']['bitstreams']: + bitstreams.append(Bitstream(bitstream_resource)) + return bitstreams def create_bitstream(self, bundle=None, name=None, path=None, mime=None, metadata=None, retry=False): """ @@ -1085,7 +1097,12 @@ def create_item(self, parent, item): if not isinstance(item, Item): _logger.error('Need a valid item') return None - return Item(api_resource=parse_json(self.create_dso(url, params=params, data=item.as_dict()))) + r = self.create_dso(url, params=params, data=item.as_dict()) + if r is None or r.status_code != 201: + # return None on failure (not a uuid-less Item) so callers' + # `if dso is None` guards actually fire + return None + return Item(api_resource=parse_json(r)) def update_item(self, item): """ diff --git a/tests/_helpers.py b/tests/_helpers.py index 6fdbecf..d4183cc 100644 --- a/tests/_helpers.py +++ b/tests/_helpers.py @@ -66,7 +66,10 @@ def multipart_properties(request) -> dict: body = body.decode("utf-8", "replace") m = re.search(r'name="properties"\r?\n\r?\n(.*?);application/json', body, re.DOTALL) - return json.loads(m.group(1)) if m else None + # fail the test loudly rather than returning None and deferring the error + assert m is not None, \ + "create_bitstream multipart body has no JSON 'properties' part" + return json.loads(m.group(1)) # --- response-body builders (shape mirrors the DSpace 7 REST API) --------- # diff --git a/tests/test_client_read.py b/tests/test_client_read.py index ae08c4c..2ddceb7 100644 --- a/tests/test_client_read.py +++ b/tests/test_client_read.py @@ -171,13 +171,19 @@ def test_no_args_returns_empty_list(self): self.assertEqual(c.get_bitstreams(), []) self.assertEqual(m.call_count, 0) - def test_non_200_currently_raises_no_failsafe(self): - # CHARACTERIZATION of a known sharp edge: unlike get_bundles (which since - # PR #16 returns [] on a 404), get_bitstreams has no fail-safe - a non-200 - # makes fetch_resource return None which this method then subscripts, so - # it raises. The consumers (export/_dspace, reposync/_files) iterate the - # result unguarded, so this is a real crash risk. Pinned deliberately: if - # the library is hardened to return [], update this test to assert that. + def test_deleted_bundle_404_returns_empty_list(self): + # 404 -> [] fail-safe, mirroring get_bundles (#16): a gone bundle simply + # has no bitstreams, which is a clean empty result, not a crash. + c = make_client() + bundle = Bundle(bundle_json("bnd2")) + with requests_mock.Mocker() as m: + m.get(f"{API}/core/bundles/bnd2/bitstreams", + status_code=404, json={"timestamp": "2026-01-01"}) + self.assertEqual(c.get_bitstreams(bundle=bundle), []) + + def test_non_404_error_still_surfaces(self): + # a transient 5xx must NOT masquerade as "no bitstreams"; it surfaces so + # the caller can retry, exactly as get_bundles does for non-404 errors. c = make_client() bundle = Bundle(bundle_json("bnd2")) with requests_mock.Mocker() as m: @@ -186,6 +192,16 @@ def test_non_200_currently_raises_no_failsafe(self): with self.assertRaises(Exception): c.get_bitstreams(bundle=bundle) + def test_200_without_bitstreams_returns_empty_list(self): + # a well-formed response with no bitstreams -> [] (not None), so callers + # can iterate the result unconditionally. + c = make_client() + bundle = Bundle(bundle_json("bnd2")) + with requests_mock.Mocker() as m: + m.get(f"{API}/core/bundles/bnd2/bitstreams", + json={"page": {"totalElements": 0}}) + self.assertEqual(c.get_bitstreams(bundle=bundle), []) + class TestGetCollections(unittest.TestCase): diff --git a/tests/test_client_write.py b/tests/test_client_write.py index ee3922e..28231e6 100644 --- a/tests/test_client_write.py +++ b/tests/test_client_write.py @@ -101,20 +101,15 @@ def test_posts_to_item_bundles_and_returns_bundle(self): def test_none_parent_returns_none(self): self.assertIsNone(make_client().create_bundle(parent=None)) - def test_server_error_returns_truthy_uuidless_bundle(self): - # CHARACTERIZATION: like create_item, create_bundle wraps the response - # unconditionally -> a truthy Bundle with uuid=None on failure, not None. - # The importer's `if not bundle` guard (reposync/_importer.py:118-120) - # never fires because a Bundle instance is always truthy. Pinned. + def test_server_error_returns_none(self): + # a failed create returns None (not a uuid-less Bundle), so the + # importer's `if not bundle` guard fires correctly. c = make_client() parent = Item(item_json(ITEM_UUID)) with requests_mock.Mocker() as m: m.post(f"{API}/core/items/{ITEM_UUID}/bundles", status_code=500, text="boom") - out = c.create_bundle(parent=parent) - self.assertIsInstance(out, Bundle) - self.assertIsNone(out.uuid) - self.assertTrue(out) + self.assertIsNone(c.create_bundle(parent=parent)) class TestCreateItem(unittest.TestCase): @@ -138,19 +133,14 @@ def test_posts_with_owning_collection_param_and_returns_item(self): self.assertEqual(body["metadata"], {}) self.assertIs(body["inArchive"], True) - def test_server_error_returns_truthy_uuidless_item(self): - # CHARACTERIZATION: create_item wraps the response unconditionally, so a - # failed create yields a truthy Item with uuid=None, NOT None. The - # importer guards with `if dso is None` (reposync/_importer.py:127-129), - # which therefore never fires on failure. Pinned; see the fail-safe note. + def test_server_error_returns_none(self): + # a failed create returns None (not a uuid-less Item), so the importer's + # `if dso is None` guard (reposync/_importer.py:127-129) fires correctly. c = make_client() item = Item({"name": "x", "metadata": {}}) with requests_mock.Mocker() as m: m.post(f"{API}/core/items", status_code=500, text="boom") - out = c.create_item(parent=COLLECTION_UUID, item=item) - self.assertIsInstance(out, Item) - self.assertIsNone(out.uuid) - self.assertTrue(out) # truthy despite the failure + self.assertIsNone(c.create_item(parent=COLLECTION_UUID, item=item)) def test_non_item_returns_none(self): self.assertIsNone(make_client().create_item( From 4049e9466deacff37afcf958275e478b5038c794 Mon Sep 17 00:00:00 2001 From: jm Date: Mon, 17 Aug 2026 21:11:01 +0200 Subject: [PATCH 25/28] fix(client): raise informative error on non-404 fetch failure get_bundles and get_bitstreams already return [] on a 404 (deleted item or bundle). On any other failure fetch_resource returns None and the code then subscripted it, surfacing an opaque 'NoneType is not subscriptable' TypeError with no status or url. Raise an explicit RuntimeError carrying the HTTP status and url instead, so a transient 5xx is a legible, retryable error. Addresses a Copilot review note; tests assert the status/url is in the message. Co-Authored-By: Claude Opus 4.8 --- dspace_rest_client/client.py | 34 +++++++++++++++++++++------------- tests/test_client_read.py | 22 ++++++++++++++++++---- 2 files changed, 39 insertions(+), 17 deletions(-) diff --git a/dspace_rest_client/client.py b/dspace_rest_client/client.py index 7049228..c298378 100644 --- a/dspace_rest_client/client.py +++ b/dspace_rest_client/client.py @@ -733,12 +733,17 @@ def get_bundles(self, parent=None, uuid=None, page=0, size=20, sort=None): if sort is not None: params['sort'] = sort r_json = self.fetch_resource(url, params=params) - if r_json is None and getattr(self._last_err, 'status_code', None) == 404: - # the item (or bundle) no longer exists - a deleted item simply has - # no bundles, which is a clean empty result, not a crash. any other - # failure falls through and still surfaces to the caller. - _logger.info(f'No bundles: resource not found (404) [{url}]') - return bundles + if r_json is None: + status = getattr(self._last_err, 'status_code', None) + if status == 404: + # a deleted item (or bundle) simply has no bundles, which is a + # clean empty result, not a crash. + _logger.info(f'No bundles: resource not found (404) [{url}]') + return bundles + # any other failure surfaces with its status + url, not as an opaque + # 'NoneType is not subscriptable' further down, so the caller can + # see what failed and retry. + raise RuntimeError(f'Failed to fetch bundles: HTTP {status} [{url}]') try: if single_result: bundles.append(Bundle(r_json)) @@ -800,13 +805,16 @@ def get_bitstreams(self, uuid=None, bundle=None, page=0, size=20, sort=None): if sort is not None: params['sort'] = sort r_json = self.fetch_resource(url, params=params) - if r_json is None and getattr(self._last_err, 'status_code', None) == 404: - # the bundle (or item) is gone - no bitstreams, a clean empty result - # rather than a crash. Mirrors get_bundles (#16). Any other failure - # (a transient 5xx, say) falls through and still surfaces to the - # caller so it is retried, not silently recorded as "no bitstreams". - _logger.info(f'No bitstreams: resource not found (404) [{url}]') - return list() + if r_json is None: + status = getattr(self._last_err, 'status_code', None) + if status == 404: + # the bundle (or item) is gone - no bitstreams, a clean empty + # result rather than a crash. Mirrors get_bundles. + _logger.info(f'No bitstreams: resource not found (404) [{url}]') + return list() + # a transient 5xx must NOT masquerade as "no bitstreams"; surface it + # with status + url so the caller can retry, not an opaque TypeError. + raise RuntimeError(f'Failed to fetch bitstreams: HTTP {status} [{url}]') bitstreams = list() if '_embedded' in r_json and 'bitstreams' in r_json['_embedded']: for bitstream_resource in r_json['_embedded']['bitstreams']: diff --git a/tests/test_client_read.py b/tests/test_client_read.py index 2ddceb7..f48b04a 100644 --- a/tests/test_client_read.py +++ b/tests/test_client_read.py @@ -128,6 +128,18 @@ def test_deleted_item_404_returns_empty_list(self): json={"timestamp": "2026-01-01"}) self.assertEqual(c.get_bundles(parent=parent), []) + def test_non_404_error_raises_informative_error(self): + # a non-404 fetch failure surfaces with its status + url so the caller + # can retry, rather than an opaque 'NoneType is not subscriptable'. + c = make_client() + parent = Item(item_json(ITEM_UUID)) + with requests_mock.Mocker() as m: + m.get(f"{API}/core/items/{ITEM_UUID}/bundles", + status_code=500, text="boom") + with self.assertRaises(RuntimeError) as ctx: + c.get_bundles(parent=parent) + self.assertIn("500", str(ctx.exception)) + def test_no_args_returns_empty_without_request(self): c = make_client() with requests_mock.Mocker() as m: @@ -181,16 +193,18 @@ def test_deleted_bundle_404_returns_empty_list(self): status_code=404, json={"timestamp": "2026-01-01"}) self.assertEqual(c.get_bitstreams(bundle=bundle), []) - def test_non_404_error_still_surfaces(self): - # a transient 5xx must NOT masquerade as "no bitstreams"; it surfaces so - # the caller can retry, exactly as get_bundles does for non-404 errors. + def test_non_404_error_raises_informative_error(self): + # a transient 5xx must NOT masquerade as "no bitstreams"; it surfaces + # with its status + url (not a bare TypeError) so the caller can retry. c = make_client() bundle = Bundle(bundle_json("bnd2")) with requests_mock.Mocker() as m: m.get(f"{API}/core/bundles/bnd2/bitstreams", status_code=500, text="boom") - with self.assertRaises(Exception): + with self.assertRaises(RuntimeError) as ctx: c.get_bitstreams(bundle=bundle) + self.assertIn("500", str(ctx.exception)) + self.assertIn("/core/bundles/bnd2/bitstreams", str(ctx.exception)) def test_200_without_bitstreams_returns_empty_list(self): # a well-formed response with no bitstreams -> [] (not None), so callers From feeb3997e7a4ae0ae33962cafa79447f49ff5729 Mon Sep 17 00:00:00 2001 From: jm Date: Mon, 17 Aug 2026 22:21:38 +0200 Subject: [PATCH 26/28] fix(client): correctness sweep - logging, timeouts, file handle, None guards Python-hygiene fixes (no behaviour change on the happy path): - Remove logging.basicConfig() at import - a library must not configure the root logger. Attach a NullHandler and route everything through the module _logger (dropped the stray root logging.* calls). - Add a configurable per-request timeout (DEFAULT_TIMEOUT=60, `timeout=` constructor arg) on every session call, so a stalled server cannot hang the client forever. - create_bitstream: open the upload file in a `with` block so the handle is always closed instead of leaked to the GC. - Bitstream(None) no longer raises TypeError on the __init__ membership checks. - create_clarinlruallowances: parameterise metadata_payload; drop the leftover hardcoded {"metadataValue":"Test"} debug data (refuses with no payload). - models: modernise super(Cls, self) -> super() throughout. Mutable class-attribute defaults were intentionally left untouched. 68 tests (5 new), no network. Co-Authored-By: Claude Opus 4.8 --- dspace_rest_client/client.py | 84 +++++++++++++++++++++--------------- dspace_rest_client/models.py | 51 +++++++++++----------- tests/test_client_auth.py | 15 +++++++ tests/test_client_write.py | 22 ++++++++++ tests/test_models.py | 7 +++ 5 files changed, 120 insertions(+), 59 deletions(-) diff --git a/dspace_rest_client/client.py b/dspace_rest_client/client.py index c298378..6b88223 100644 --- a/dspace_rest_client/client.py +++ b/dspace_rest_client/client.py @@ -26,8 +26,11 @@ __all__ = ['DSpaceClient'] -logging.basicConfig(format='%(asctime)s - %(message)s', level=logging.INFO) _logger = logging.getLogger("dspace.client") +# A library must not configure the root logger - that is the consuming +# application's job. Attach a NullHandler so records are dropped unless the +# application opts in to logging. +_logger.addHandler(logging.NullHandler()) def parse_json(response): @@ -79,6 +82,9 @@ class DSpaceClient: USER_AGENT = os.environ['USER_AGENT'] verbose = False ITER_PAGE_SIZE = 20 + # Default per-request timeout in seconds so a stalled server cannot hang the + # client forever; override via the `timeout` constructor argument. + DEFAULT_TIMEOUT = 60 PROXY_DICT = dict(http=os.environ["PROXY_URL"],https=os.environ["PROXY_URL"]) if "PROXY_URL" in os.environ else dict() # Simple enum for patch operation types @@ -89,7 +95,7 @@ class PatchOperation: MOVE = 'move' def __init__(self, api_endpoint=API_ENDPOINT, username=USERNAME, password=PASSWORD, solr_endpoint=SOLR_ENDPOINT, - solr_auth=SOLR_AUTH, fake_user_agent=False, proxies=PROXY_DICT): + solr_auth=SOLR_AUTH, fake_user_agent=False, proxies=PROXY_DICT, timeout=None): """ Accept optional API endpoint, username, password arguments using the OS environment variables as defaults :param api_endpoint: base path to DSpace REST API, eg. http://localhost:8080/server/api @@ -105,6 +111,7 @@ def __init__(self, api_endpoint=API_ENDPOINT, username=USERNAME, password=PASSWO self.proxies = proxies self.solr = None self._last_err = None + self.timeout = timeout if timeout is not None else self.DEFAULT_TIMEOUT try: import pysolr self.solr = pysolr.Solr(url=solr_endpoint, always_commit=True, timeout=300, auth=solr_auth) @@ -137,7 +144,7 @@ def authenticate(self, retry=False): # Get and update CSRF token r = self.session.post(self.LOGIN_URL, data={'user': self.USERNAME, 'password': self.PASSWORD}, headers=self.auth_request_headers, - proxies=self.proxies) + proxies=self.proxies, timeout=self.timeout) self.update_token(r) if r.status_code == 403: @@ -164,7 +171,7 @@ def authenticate(self, retry=False): # Get and check authentication status r = self.session.get(f'{self.API_ENDPOINT}/authn/status', headers=self.request_headers, - proxies=self.proxies) + proxies=self.proxies, timeout=self.timeout) if r.status_code == 200: r_json = parse_json(r) if 'authenticated' in r_json and r_json['authenticated'] is True: @@ -214,7 +221,7 @@ def api_get(self, url, params=None, data=None, headers=None): if headers is None: headers = self.request_headers r = self.session.get(url, params=params, data=data, headers=headers, - proxies=self.proxies) + proxies=self.proxies, timeout=self.timeout) self.update_token(r) return r @@ -230,7 +237,7 @@ def api_post(self, url, params, json, retry=False, timeout=None): """ self._last_err = None r = self.session.post(url, json=json, params=params, headers=self.request_headers, - proxies=self.proxies, timeout=timeout) + proxies=self.proxies, timeout=timeout if timeout is not None else self.timeout) self.update_token(r) if r.status_code == 403: @@ -252,10 +259,10 @@ def api_post(self, url, params, json, retry=False, timeout=None): r_json = parse_json(r) if 'message' in (r_json or {}) and 'Authentication is required' in r_json['message']: if retry: - logging.error( + _logger.error( 'API Post: Already retried... something must be wrong') else: - logging.debug("API Post: Retrying request with updated CSRF token") + _logger.debug("API Post: Retrying request with updated CSRF token") # try to authenticate self.authenticate() # Try to authenticate and repeat the request 3 times - @@ -275,7 +282,7 @@ def api_post_uri(self, url, params, uri_list, retry=False): """ self._last_err = None r = self.session.post(url, data=uri_list, params=params, headers=self.list_request_headers, - proxies=self.proxies) + proxies=self.proxies, timeout=self.timeout) self.update_token(r) if r.status_code == 403: @@ -305,7 +312,7 @@ def api_put(self, url, params, json, retry=False): """ self._last_err = None r = self.session.put(url, params=params, json=json, headers=self.request_headers, - proxies=self.proxies) + proxies=self.proxies, timeout=self.timeout) self.update_token(r) if r.status_code == 403: @@ -337,7 +344,7 @@ def api_put_uri(self, url, params, uri_list, retry=False): """ self._last_err = None r = self.session.put(url, params=params, data=uri_list, headers=self.list_request_headers, - proxies=self.proxies) + proxies=self.proxies, timeout=self.timeout) self.update_token(r) if r.status_code == 403: @@ -368,7 +375,7 @@ def api_delete(self, url, params, retry=False): """ self._last_err = None r = self.session.delete(url, params=params, headers=self.request_headers, - proxies=self.proxies) + proxies=self.proxies, timeout=self.timeout) self.update_token(r) if r.status_code == 403: @@ -401,15 +408,15 @@ def api_patch(self, url, operation, path, value, params=None, retry=False): """ self._last_err = None if url is None: - logging.error('Missing required URL argument') + _logger.error('Missing required URL argument') return None if path is None: - logging.error('Need valid path eg. /withdrawn or /metadata/dc.title/0/language') + _logger.error('Need valid path eg. /withdrawn or /metadata/dc.title/0/language') return None if (operation == self.PatchOperation.ADD or operation == self.PatchOperation.REPLACE or operation == self.PatchOperation.MOVE) and value is None: # missing value required for add/replace/move operations - logging.error('Missing required "value" argument for add/replace/move operations') + _logger.error('Missing required "value" argument for add/replace/move operations') return None # compile patch data @@ -426,7 +433,7 @@ def api_patch(self, url, operation, path, value, params=None, retry=False): # set headers # perform patch request r = self.session.patch(url, json=[data], params=params, headers=self.request_headers, - proxies=self.proxies) + proxies=self.proxies, timeout=self.timeout) self.update_token(r) if r.status_code == 403: @@ -635,7 +642,7 @@ def update_dso(self, dso, params=None): return None dso_type = type(dso) if not isinstance(dso, SimpleDSpaceObject): - logging.error('Only SimpleDSpaceObject types (eg Item, Collection, Community) ' + _logger.error('Only SimpleDSpaceObject types (eg Item, Collection, Community) ' 'are supported by generic update_dso PUT.') return dso try: @@ -682,11 +689,11 @@ def delete_dso(self, dso=None, url=None, params=None): """ if dso is None: if url is None: - logging.error('Need a DSO or a URL to delete') + _logger.error('Need a DSO or a URL to delete') return None else: if not isinstance(dso, SimpleDSpaceObject): - logging.error('Only SimpleDSpaceObject types (eg Item, Collection, Community, EPerson) ' + _logger.error('Only SimpleDSpaceObject types (eg Item, Collection, Community, EPerson) ' 'are supported by generic update_dso PUT.') return dso # Get self URI from HAL links @@ -844,15 +851,17 @@ def create_bitstream(self, bundle=None, name=None, path=None, mime=None, metadat if metadata is None: metadata = {} url = f'{self.API_ENDPOINT}/core/bundles/{bundle.uuid}/bitstreams' - file = (name, open(path, 'rb'), mime) - files = {'file': file} - properties = {'name': name, 'metadata': metadata, 'bundleName': bundle.name} - payload = {'properties': json.dumps(properties) + ';application/json'} - h = self.session.headers - h.update({'Content-Encoding': 'gzip', 'User-Agent': self.USER_AGENT}) - req = Request('POST', url, data=payload, headers=h, files=files) - prepared_req = self.session.prepare_request(req) - r = self.session.send(prepared_req, proxies=self.proxies) + # open the file in a context manager so the handle is always closed, + # even if prepare/send raises (it was previously leaked to the GC). + with open(path, 'rb') as fh: + files = {'file': (name, fh, mime)} + properties = {'name': name, 'metadata': metadata, 'bundleName': bundle.name} + payload = {'properties': json.dumps(properties) + ';application/json'} + h = self.session.headers + h.update({'Content-Encoding': 'gzip', 'User-Agent': self.USER_AGENT}) + req = Request('POST', url, data=payload, headers=h, files=files) + prepared_req = self.session.prepare_request(req) + r = self.session.send(prepared_req, proxies=self.proxies, timeout=self.timeout) if 'DSPACE-XSRF-TOKEN' in r.headers: t = r.headers['DSPACE-XSRF-TOKEN'] _logger.debug('Updating token to ' + t) @@ -1200,7 +1209,7 @@ def create_user(self, user, token=None): def delete_user(self, user): if not isinstance(user, User): - logging.error('Must be a valid user') + _logger.error('Must be a valid user') return None return self.delete_dso(user) @@ -1430,16 +1439,21 @@ def get_clarinlruallowances_by_bitstream_and_user(self, bitstream_uuid, user_uui return None - def create_clarinlruallowances(self, bitstream_uuid): + def create_clarinlruallowances(self, bitstream_uuid, metadata_payload=None): """ - Create clarinlruallowances for a bitstream for logged user - by managing user metadata of bitstream. + Create clarinlruallowances for a bitstream for the logged-in user by + managing the bitstream's user metadata. + @param bitstream_uuid: target bitstream UUID + @param metadata_payload: list of {"metadataKey", "metadataValue"} dicts. + Required - there is no meaningful default (the + previous hardcoded "Test" value was leftover + debug data, not usable for real callers). """ + if not metadata_payload: + _logger.error('create_clarinlruallowances requires a metadata_payload') + return False url = f'{self.API_ENDPOINT}/core/clarinusermetadata/manage' params = {'bitstreamUUID': bitstream_uuid} - metadata_payload = [ - {"metadataKey": "NAME", "metadataValue": "Test"} - ] try: response = self.api_post(url, json=metadata_payload, params=params) if response.status_code == 200: diff --git a/dspace_rest_client/models.py b/dspace_rest_client/models.py index f09aa2c..a843bc1 100644 --- a/dspace_rest_client/models.py +++ b/dspace_rest_client/models.py @@ -237,9 +237,9 @@ def __init__(self, api_resource=None, dso=None): """ if dso is not None: api_resource = dso.as_dict() - super(Item, self).__init__(dso=dso) + super().__init__(dso=dso) else: - super(Item, self).__init__(api_resource) + super().__init__(api_resource) if api_resource is not None: self.type = 'item' @@ -263,7 +263,7 @@ def as_dict(self): Return a dict representation of this Item, based on super with item-specific attributes added @return: dict of Item for API use """ - dso_dict = super(Item, self).as_dict() + dso_dict = super().as_dict() item_dict = {'inArchive': self.inArchive, 'discoverable': self.discoverable, 'withdrawn': self.withdrawn} return {**dso_dict, **item_dict} @@ -287,7 +287,7 @@ def __init__(self, api_resource=None): Default constructor. Call DSpaceObject init then set item-specific attributes @param api_resource: API result object to use as initial data """ - super(Community, self).__init__(api_resource) + super().__init__(api_resource) self.type = 'community' def as_dict(self): @@ -295,7 +295,7 @@ def as_dict(self): Return a dict representation of this Community, based on super with community-specific attributes added @return: dict of Item for API use """ - dso_dict = super(Community, self).as_dict() + dso_dict = super().as_dict() # TODO: More community-specific stuff community_dict = {} return {**dso_dict, **community_dict} @@ -312,11 +312,11 @@ def __init__(self, api_resource=None): Default constructor. Call DSpaceObject init then set collection-specific attributes @param api_resource: API result object to use as initial data """ - super(Collection, self).__init__(api_resource) + super().__init__(api_resource) self.type = 'collection' def as_dict(self): - dso_dict = super(Collection, self).as_dict() + dso_dict = super().as_dict() """ Return a dict representation of this Collection, based on super with collection-specific attributes added @return: dict of Item for API use @@ -336,7 +336,7 @@ def __init__(self, api_resource=None): Default constructor. Call DSpaceObject init then set bundle-specific attributes @param api_resource: API result object to use as initial data """ - super(Bundle, self).__init__(api_resource) + super().__init__(api_resource) self.type = 'bundle' def as_dict(self): @@ -344,7 +344,7 @@ def as_dict(self): Return a dict representation of this Bundle, based on super with bundle-specific attributes added @return: dict of Bundle for API use """ - dso_dict = super(Bundle, self).as_dict() + dso_dict = super().as_dict() bundle_dict = {} return {**dso_dict, **bundle_dict} @@ -368,8 +368,11 @@ def __init__(self, api_resource=None): Default constructor. Call DSpaceObject init then set bitstream-specific attributes @param api_resource: API result object to use as initial data """ - super(Bitstream, self).__init__(api_resource) + super().__init__(api_resource) self.type = 'bitstream' + # tolerate Bitstream(None): other models guard this, and without it the + # membership tests below raise TypeError on a None api_resource. + api_resource = api_resource or {} if 'bundleName' in api_resource: self.bundleName = api_resource['bundleName'] if 'sizeBytes' in api_resource: @@ -384,7 +387,7 @@ def as_dict(self): Return a dict representation of this Bitstream, based on super with bitstream-specific attributes added @return: dict of Bitstream for API use """ - dso_dict = super(Bitstream, self).as_dict() + dso_dict = super().as_dict() bitstream_dict = {'bundleName': self.bundleName, 'sizeBytes': self.sizeBytes, 'checkSum': self.checkSum, 'sequenceId': self.sequenceId} return {**dso_dict, **bitstream_dict} @@ -403,7 +406,7 @@ def __init__(self, api_resource=None): Default constructor. Call DSpaceObject init then set group-specific attributes @param api_resource: API result object to use as initial data """ - super(Group, self).__init__(api_resource) + super().__init__(api_resource) self.type = 'group' if 'name' in api_resource: self.name = api_resource['name'] @@ -415,7 +418,7 @@ def as_dict(self): Return a dict representation of this Group, based on super with group-specific attributes added @return: dict of Group for API use """ - dso_dict = super(Group, self).as_dict() + dso_dict = super().as_dict() group_dict = {'name': self.name, 'permanent': self.permanent} return {**dso_dict, **group_dict} @@ -438,7 +441,7 @@ def __init__(self, api_resource=None): Default constructor. Call DSpaceObject init then set user-specific attributes @param api_resource: API result object to use as initial data """ - super(User, self).__init__(api_resource) + super().__init__(api_resource) self.type = 'user' if 'name' in api_resource: self.name = api_resource['name'] @@ -460,7 +463,7 @@ def as_dict(self): Return a dict representation of this User, based on super with user-specific attributes added @return: dict of User for API use """ - dso_dict = super(User, self).as_dict() + dso_dict = super().as_dict() user_dict = {'name': self.name, 'netid': self.netid, 'lastActive': self.lastActive, 'canLogIn': self.canLogIn, 'email': self.email, 'requireCertificate': self.requireCertificate, 'selfRegistered': self.selfRegistered} @@ -473,7 +476,7 @@ class InProgressSubmission(AddressableHALResource): type = None def __init__(self, api_resource): - super(InProgressSubmission, self).__init__(api_resource) + super().__init__(api_resource) if 'lastModified' in api_resource: self.lastModified = api_resource['lastModified'] if 'step' in api_resource: @@ -484,7 +487,7 @@ def __init__(self, api_resource): self.type = api_resource['type'] def as_dict(self): - parent_dict = super(InProgressSubmission, self).as_dict() + parent_dict = super().as_dict() dict = { 'lastModified': self.lastModified, 'step': self.step, @@ -496,10 +499,10 @@ def as_dict(self): class WorkspaceItem(InProgressSubmission): def __init__(self, api_resource): - super(WorkspaceItem, self).__init__(api_resource) + super().__init__(api_resource) def as_dict(self): - return super(WorkspaceItem, self).as_dict() + return super().as_dict() class EntityType(AddressableHALResource): """ @@ -508,7 +511,7 @@ class EntityType(AddressableHALResource): are all common entity types used in DSpace 7+ """ def __init__(self, api_resource): - super(EntityType, self).__init__(api_resource) + super().__init__(api_resource) if 'label' in api_resource: self.label = api_resource['label'] if 'type' in api_resource: @@ -519,14 +522,14 @@ class RelationshipType(AddressableHALResource): TODO: RelationshipType """ def __init__(self, api_resource): - super(RelationshipType, self).__init__(api_resource) + super().__init__(api_resource) class License(AddressableHALResource): """ Specific attributes and functions for licenses """ def __init__(self, api_resource=None): - super(License, self).__init__(api_resource) + super().__init__(api_resource) api_resource = api_resource or {} self.type = 'clarinlicense' self.name = api_resource.get('name') @@ -559,7 +562,7 @@ def __init__(self, api_resource=None): Default constructor. Call DSpaceObject init then set label-specific attributes @param api_resource: API result object to use as initial data """ - super(Label, self).__init__(api_resource) + super().__init__(api_resource) api_resource = api_resource or {} self.type = 'clarinlicenselabel' self.label = api_resource.get('label') @@ -582,7 +585,7 @@ class ResourcePolicy(AddressableHALResource): DQ specific. Extends Addressable HAL Resource to model a resource policy. """ def __init__(self, api_resource: dict): - super(ResourcePolicy, self).__init__(api_resource) + super().__init__(api_resource) api_resource = api_resource or {} self.name = api_resource.get('name') self.description = api_resource.get('description') diff --git a/tests/test_client_auth.py b/tests/test_client_auth.py index dae22da..08a1556 100644 --- a/tests/test_client_auth.py +++ b/tests/test_client_auth.py @@ -9,8 +9,11 @@ import requests_mock +import logging + import _helpers # noqa: F401 from _helpers import make_client, API +from dspace_rest_client.client import DSpaceClient class TestConstructor(unittest.TestCase): @@ -24,6 +27,18 @@ def test_endpoints_derived_from_api_endpoint(self): def test_default_last_err_is_none(self): self.assertIsNone(make_client().last_err) + def test_timeout_default_and_override(self): + # every request must be bounded so a stalled server can't hang forever + self.assertEqual(make_client().timeout, DSpaceClient.DEFAULT_TIMEOUT) + self.assertEqual(DSpaceClient(API, "u", "p", timeout=5).timeout, 5) + + def test_library_does_not_hijack_root_logger(self): + # importing the client must not call logging.basicConfig; the module + # logger carries a NullHandler so records drop unless the app opts in. + lg = logging.getLogger("dspace.client") + self.assertTrue( + any(isinstance(h, logging.NullHandler) for h in lg.handlers)) + class TestAuthenticate(unittest.TestCase): diff --git a/tests/test_client_write.py b/tests/test_client_write.py index 28231e6..9e4815c 100644 --- a/tests/test_client_write.py +++ b/tests/test_client_write.py @@ -193,5 +193,27 @@ def test_server_error_returns_none(self): mime="application/pdf")) +class TestCreateClarinAllowances(unittest.TestCase): + + def test_requires_metadata_payload(self): + # the previous hardcoded {"metadataValue":"Test"} is gone; with no + # payload the call refuses and makes no request. + c = make_client() + with requests_mock.Mocker() as m: + self.assertFalse(c.create_clarinlruallowances(BITSTREAM_UUID)) + self.assertEqual(m.call_count, 0) + + def test_posts_supplied_payload(self): + c = make_client() + payload = [{"metadataKey": "NAME", "metadataValue": "real value"}] + with requests_mock.Mocker() as m: + m.post(f"{API}/core/clarinusermetadata/manage", status_code=200, + json={}) + self.assertTrue(c.create_clarinlruallowances(BITSTREAM_UUID, payload)) + self.assertEqual(m.last_request.json(), payload) + self.assertEqual(sent_params(m.last_request)["bitstreamUUID"], + [BITSTREAM_UUID]) + + if __name__ == "__main__": unittest.main() diff --git a/tests/test_models.py b/tests/test_models.py index 2e06059..8167548 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -102,6 +102,13 @@ def test_bitstream_file_fields(self): self.assertEqual(d["checkSum"]["value"], "deadbeef") self.assertEqual(d["sequenceId"], 3) + def test_bitstream_from_none_does_not_crash(self): + # some cache/None paths construct Bitstream(None); it must not raise the + # way it used to on the membership checks in __init__. + b = Bitstream(None) + self.assertEqual(b.type, "bitstream") + self.assertIsNone(b.uuid) + class TestResourcePolicy(unittest.TestCase): From e9eef068414d5ba71a2a44ad2d62356be9c021b1 Mon Sep 17 00:00:00 2001 From: jm Date: Tue, 18 Aug 2026 00:29:01 +0200 Subject: [PATCH 27/28] fix: address Copilot review (handler guard, header copy, docstrings) - guard the NullHandler registration so reloads/re-imports don't accumulate duplicate handlers - create_bitstream: copy the session headers before adding Content-Encoding so it doesn't leak onto every subsequent request / across threads - move Collection.as_dict's docstring to the first statement (it was a no-op string literal after code); rename the `dict` builtin-shadow in InProgressSubmission.as_dict Co-Authored-By: Claude Opus 4.8 --- dspace_rest_client/client.py | 12 ++++++++---- dspace_rest_client/models.py | 6 +++--- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/dspace_rest_client/client.py b/dspace_rest_client/client.py index 6b88223..9cacb08 100644 --- a/dspace_rest_client/client.py +++ b/dspace_rest_client/client.py @@ -28,9 +28,11 @@ _logger = logging.getLogger("dspace.client") # A library must not configure the root logger - that is the consuming -# application's job. Attach a NullHandler so records are dropped unless the -# application opts in to logging. -_logger.addHandler(logging.NullHandler()) +# application's job. Attach a NullHandler (once) so records are dropped unless +# the application opts in to logging - guarded so reloads/re-imports don't +# accumulate duplicate handlers. +if not any(isinstance(h, logging.NullHandler) for h in _logger.handlers): + _logger.addHandler(logging.NullHandler()) def parse_json(response): @@ -857,7 +859,9 @@ def create_bitstream(self, bundle=None, name=None, path=None, mime=None, metadat files = {'file': (name, fh, mime)} properties = {'name': name, 'metadata': metadata, 'bundleName': bundle.name} payload = {'properties': json.dumps(properties) + ';application/json'} - h = self.session.headers + # copy the session headers so this request's Content-Encoding does + # not leak onto every subsequent request (and across threads) + h = dict(self.session.headers) h.update({'Content-Encoding': 'gzip', 'User-Agent': self.USER_AGENT}) req = Request('POST', url, data=payload, headers=h, files=files) prepared_req = self.session.prepare_request(req) diff --git a/dspace_rest_client/models.py b/dspace_rest_client/models.py index a843bc1..4427512 100644 --- a/dspace_rest_client/models.py +++ b/dspace_rest_client/models.py @@ -316,11 +316,11 @@ def __init__(self, api_resource=None): self.type = 'collection' def as_dict(self): - dso_dict = super().as_dict() """ Return a dict representation of this Collection, based on super with collection-specific attributes added @return: dict of Item for API use """ + dso_dict = super().as_dict() collection_dict = {} return {**dso_dict, **collection_dict} @@ -488,13 +488,13 @@ def __init__(self, api_resource): def as_dict(self): parent_dict = super().as_dict() - dict = { + submission_dict = { 'lastModified': self.lastModified, 'step': self.step, 'sections': self.sections, 'type': self.type } - return {**parent_dict, **dict} + return {**parent_dict, **submission_dict} class WorkspaceItem(InProgressSubmission): From 0c7411b9602d1469ab6a9785076f362bba9e451b Mon Sep 17 00:00:00 2001 From: Juraj Roka <95219754+jr-rk@users.noreply.github.com> Date: Thu, 20 Aug 2026 10:33:28 +0200 Subject: [PATCH 28/28] test: CLARIN consumer-contract suite + main-tailored differential CI Add characterization tests that replay how the three main-lineage consumers (dspace-import-clarin, dspace-rest-test, dspace-item-importer) actually call this library, plus a branch-differential CI job that runs the shared CLARIN contract against BOTH the dtq and main implementations. Only the HTTP transport is mocked; the real client builds URLs and parses responses. This PR is test + CI infrastructure only - no dspace_rest_client/ changes. The shared-surface fixes and dtq behaviour deltas that the tests would otherwise pin live in a separate, stacked PR, so this one stays scoped to validating main's usage surface and proposes nothing for main. Markers (pytest.ini): clarin - shared contract, must hold on both main and dtq dtq_only - existing dtq-only surface/behaviour, deselected on the main leg Co-Authored-By: Claude Opus 4.8 --- .github/workflows/tests.yml | 128 ++++++++++++++++- .gitignore | 2 + pytest.ini | 9 ++ requirements-test.txt | 2 + tests/_helpers.py | 66 +++++++++ tests/test_clarin_read.py | 202 ++++++++++++++++++++++++++ tests/test_clarin_usage_contract.py | 192 +++++++++++++++++++++++++ tests/test_clarin_write.py | 214 ++++++++++++++++++++++++++++ tests/test_client_read.py | 17 +++ tests/test_models_clarin.py | 94 ++++++++++++ tests/test_transport_hardening.py | 126 ++++++++++++++++ 11 files changed, 1050 insertions(+), 2 deletions(-) create mode 100644 pytest.ini create mode 100644 tests/test_clarin_read.py create mode 100644 tests/test_clarin_usage_contract.py create mode 100644 tests/test_clarin_write.py create mode 100644 tests/test_models_clarin.py create mode 100644 tests/test_transport_hardening.py diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index affd373..87d8625 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -2,8 +2,11 @@ name: Tests on: push: - branches: [ dtq ] + # dtq is the mainline; main is the CLARIN branch we are validating a merge + # into. feat/** and fix/** get CI before they open a PR. + branches: [ dtq, main, 'feat/**', 'fix/**' ] pull_request: + workflow_dispatch: # manual validation of a merge candidate permissions: contents: read @@ -37,4 +40,125 @@ jobs: pip install -r requirements-test.txt - name: Run tests - run: python -m pytest tests/ -v + # Coverage floor guards against the CLARIN surface silently sliding back + # toward the zero it had before test/clarin-usage-coverage landed. + run: > + python -m pytest tests/ -v + --cov=dspace_rest_client --cov-report=term-missing + --cov-fail-under=70 + + differential-contract: + # THE MERGE GATE. Run the CLARIN consumer-contract suite against BOTH the + # dtq implementation (this checkout) and the main implementation (swapped in + # from origin/main). A test green on both proves the merge preserves that + # behaviour. Tests that deliberately encode a dtq fix or behaviour change + # are marked @pytest.mark.dtq_only and are skipped on the main leg. + # + # Leg selection targets the four CLARIN test files explicitly: the DQ test + # modules import dtq-only symbols (ResourcePolicy, ...) at module scope, so + # collecting them against the main implementation would be an import error. + name: CLARIN contract vs ${{ matrix.impl }} impl + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + impl: [dtq, main] + + steps: + - uses: actions/checkout@v6 + with: + fetch-depth: 0 # need origin/main to swap the implementation in + + - name: Set up Python + uses: actions/setup-python@v6 + with: + python-version: "3.10" + cache: pip + cache-dependency-path: requirements-test.txt + + - name: Install test deps + run: | + python -m pip install --upgrade pip + pip install -r requirements-test.txt + + - name: Swap in the ${{ matrix.impl }} implementation + if: matrix.impl != 'dtq' + run: git checkout "origin/${{ matrix.impl }}" -- dspace_rest_client/ + + - name: Run CLARIN contract suite + run: | + FILES="tests/test_clarin_read.py tests/test_clarin_write.py \ + tests/test_models_clarin.py tests/test_clarin_usage_contract.py" + if [ "${{ matrix.impl }}" = "dtq" ]; then + python -m pytest $FILES -v # full CLARIN surface, incl. dtq_only + else + python -m pytest $FILES -v -m "not dtq_only" # shared contract only + fi + + # ---- Consumer smoke jobs ------------------------------------------------- + # Turn "the API surface is a superset" into "the consumers still import/run". + # Gated on CONSUMER_READ_TOKEN: until that read-scoped token for the private + # consumer repos exists, the gate job reports enabled=false and consumer-smoke + # is skipped (a clean green), per plan §6.3. + check-consumer-token: + runs-on: ubuntu-latest + outputs: + enabled: ${{ steps.probe.outputs.enabled }} + steps: + - id: probe + env: + TOKEN: ${{ secrets.CONSUMER_READ_TOKEN }} + run: echo "enabled=${{ env.TOKEN != '' }}" >> "$GITHUB_OUTPUT" + + consumer-smoke: + needs: check-consumer-token + if: needs.check-consumer-token.outputs.enabled == 'true' + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + include: + - repo: DSpace-ISstag-integration + ref: main + smoke: python -m pytest tests/ mcp/tests/ -q + - repo: dspace-rest-test + ref: master + smoke: python -c "import dspace_rest_client.client" + - repo: dspace-import-clarin + ref: main + smoke: python -c "import dspace_rest_client.client" + # dspace-item-importer is intentionally omitted until its .gitmodules + # is repointed off the deleted `dtq-dev` branch (plan §6.3 / brief §5). + steps: + - uses: actions/checkout@v6 + with: + path: candidate + + - uses: actions/checkout@v6 + with: + repository: dataquest-dev/${{ matrix.repo }} + ref: ${{ matrix.ref }} + token: ${{ secrets.CONSUMER_READ_TOKEN }} + submodules: recursive + path: consumer + + - name: Set up Python + uses: actions/setup-python@v6 + with: + python-version: "3.10" + + - name: Point the consumer submodule at this candidate commit + run: | + rm -rf consumer/libs/dspace-rest-python + cp -r candidate consumer/libs/dspace-rest-python + + - name: Install and smoke + working-directory: consumer + run: | + python -m pip install --upgrade pip + pip install ./libs/dspace-rest-python + if [ -f requirements.lock ]; then pip install -r requirements.lock; fi + if [ -f libs/dspace-rest-python/requirements-test.txt ]; then + pip install -r libs/dspace-rest-python/requirements-test.txt + fi + ${{ matrix.smoke }} diff --git a/.gitignore b/.gitignore index deba30b..eb1f1d5 100644 --- a/.gitignore +++ b/.gitignore @@ -13,3 +13,5 @@ __pypackages__/ env/ venv/ .idea/ +.coverage +coverage.xml diff --git a/pytest.ini b/pytest.ini new file mode 100644 index 0000000..1f08f44 --- /dev/null +++ b/pytest.ini @@ -0,0 +1,9 @@ +[pytest] +# Markers used to slice the suite for the branch-differential CI run +# (see .github/workflows/tests.yml :: differential-contract). A pytest.ini +# takes precedence over any [tool.pytest.ini_options] a future pyproject.toml +# might add, so the marker registry stays in one place regardless of packaging. +markers = + dtq_only: behaviour introduced on dtq; not expected to hold on the main implementation + clarin: exercises the CLARIN/UFAL surface (main-lineage consumers) + dq: exercises the DQ integration surface (DSpace-ISstag-integration) diff --git a/requirements-test.txt b/requirements-test.txt index 575ad6c..f8ec9b4 100644 --- a/requirements-test.txt +++ b/requirements-test.txt @@ -5,3 +5,5 @@ pytest>=7.0 requests-mock>=1.11 requests +# coverage floor is enforced in CI (tests.yml :: test job, --cov-fail-under) +pytest-cov>=4.0 diff --git a/tests/_helpers.py b/tests/_helpers.py index d4183cc..ab8a11d 100644 --- a/tests/_helpers.py +++ b/tests/_helpers.py @@ -32,6 +32,10 @@ COLLECTION_UUID = "22222222-2222-2222-2222-222222222222" BITSTREAM_UUID = "9f54ef33-c454-4d8e-a5fe-79d8291045ba" ANON_GROUP_UUID = "6ecfd145-3b7d-429e-ab31-ef6905a05763" +# Used by the CLARIN-side suites (eperson/group lookups, submit groups). +EPERSON_UUID = "33333333-3333-3333-3333-333333333333" +GROUP_UUID = "44444444-4444-4444-4444-444444444444" +BUNDLE_UUID = "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa" def make_client(api_endpoint: str = API) -> DSpaceClient: @@ -112,3 +116,65 @@ def policy_json(pid: int = 1, action: str = "READ", group_name: str = "Anonymous if start_date is not None: d["startDate"] = start_date return d + + +def raw_policy_json(pid: int = 1, action: str = "READ", **extra) -> dict: + """A resource policy in the *raw* shape the CLARIN ``get_resource_policy`` + returns (a plain dict the caller subscripts as ``["id"]``), not a model.""" + d = {"id": pid, "action": action, "type": "resourcepolicy"} + d.update(extra) + return d + + +def group_json(uuid: str = GROUP_UUID, name: str = "Anonymous", + permanent: bool = False, **extra) -> dict: + d = {"uuid": uuid, "name": name, "type": "group", "permanent": permanent} + d.update(extra) + return d + + +def user_json(uuid: str = EPERSON_UUID, email: str = "tester@dspace.test", + name: str = "Tester", netid: str = None, can_login: bool = True, + **extra) -> dict: + d = {"uuid": uuid, "type": "eperson", "name": name, "email": email, + "canLogIn": can_login} + if netid is not None: + d["netid"] = netid + d.update(extra) + return d + + +def label_json(lid: int = 10, label: str = "PUB", title: str = "Publicly available", + icon: str = "pub.png", extended: bool = False) -> dict: + return {"id": lid, "label": label, "title": title, "icon": icon, + "extended": extended} + + +def license_json(lid: int = 1, name: str = "CC-BY", + definition: str = "https://creativecommons.org/licenses/by/4.0/", + confirmation: int = 1, required_info: str = "SEND_TOKEN", + label: dict = None, extended: list = None) -> dict: + d = {"id": lid, "name": name, "definition": definition, + "confirmation": confirmation, "requiredInfo": required_info} + if label is not None: + d["clarinLicenseLabel"] = label + if extended is not None: + d["extendedClarinLicenseLabels"] = extended + return d + + +def clarin_allowance_json(aid: int = 1, **extra) -> dict: + d = {"id": aid, "type": "clarinlruallowance"} + d.update(extra) + return d + + +def search_envelope(items: list) -> dict: + """The ``discover/search/objects`` HAL envelope, wrapping each item as an + ``indexableObject``. Used by ``get_items_from_collection`` and + ``search_objects``. + """ + return {"_embedded": {"searchResult": { + "page": {"totalElements": len(items)}, + "_embedded": {"objects": [ + {"_embedded": {"indexableObject": it}} for it in items]}}}} diff --git a/tests/test_clarin_read.py b/tests/test_clarin_read.py new file mode 100644 index 0000000..78663c4 --- /dev/null +++ b/tests/test_clarin_read.py @@ -0,0 +1,202 @@ +""" +CLARIN/UFAL read surface - the methods the three main-lineage consumers +(dspace-rest-test, dspace-import-clarin, dspace-item-importer) call but which +had zero coverage after `main` was merged into `dtq`. + +Same discipline as the DQ suite: mock only the HTTP transport, let the real +client build URLs / parse responses. Each test names the consumer it mirrors. + +Marks: + clarin - shared surface, must hold on both `main` and `dtq` + dtq_only - asserts a fix or a method that exists only on `dtq`; deselected + when the differential-contract CI job runs against `main`. +""" +import unittest + +import pytest +import requests_mock + +import _helpers # noqa: F401 +from _helpers import ( + make_client, sent_params, embedded, item_json, bundle_json, raw_policy_json, + user_json, clarin_allowance_json, search_envelope, + API, ITEM_UUID, COLLECTION_UUID, BUNDLE_UUID, EPERSON_UUID) +from dspace_rest_client.models import Bundle, Item, Collection, User + +pytestmark = pytest.mark.clarin + + +class TestGetResourcePolicyDict(unittest.TestCase): + """Mirrors dspace-import-clarin - get_resource_policy(uuid)["id"]. + The dict-subscript contract that blocks the resource-policy API unification; + it must keep returning a raw dict, not a model.""" + + URL = f"{API}/authz/resourcepolicies/search/resource" + + def test_returns_first_raw_dict_with_id_subscript(self): + c = make_client() + with requests_mock.Mocker() as m: + m.get(self.URL, json=embedded("resourcepolicies", + [raw_policy_json(pid=7), + raw_policy_json(pid=8)])) + rp = c.get_resource_policy(BUNDLE_UUID) + self.assertIsInstance(rp, dict) + self.assertEqual(rp["id"], 7) # dict subscript, first policy + + def test_sends_uuid_and_both_embeds(self): + c = make_client() + with requests_mock.Mocker() as m: + m.get(self.URL, json=embedded("resourcepolicies", [raw_policy_json()])) + c.get_resource_policy(BUNDLE_UUID) + qs = sent_params(m.last_request) + self.assertEqual(qs["uuid"], [BUNDLE_UUID]) + self.assertEqual(sorted(qs["embed"]), ["eperson", "group"]) + + +class TestGetBundleByName(unittest.TestCase): + """Mirrors dspace-import-clarin - get_bundle_by_name('ORIGINAL', item).""" + + URL = f"{API}/core/items/{ITEM_UUID}/bundles" + + def test_matches_named_bundle(self): + c = make_client() + with requests_mock.Mocker() as m: + m.get(self.URL, json=embedded("bundles", [ + bundle_json("b1", "LICENSE"), + bundle_json("b2", "ORIGINAL")])) + b = c.get_bundle_by_name("ORIGINAL", ITEM_UUID) + self.assertIsInstance(b, Bundle) + self.assertEqual((b.uuid, b.name), ("b2", "ORIGINAL")) + + def test_no_match_returns_none(self): + c = make_client() + with requests_mock.Mocker() as m: + m.get(self.URL, json=embedded("bundles", [bundle_json("b1", "LICENSE")])) + self.assertIsNone(c.get_bundle_by_name("ORIGINAL", ITEM_UUID)) + + +class TestGetItemsFromCollection(unittest.TestCase): + """Mirrors dspace-import-clarin - get_items_from_collection(collection).""" + + URL = f"{API}/discover/search/objects" + + def test_parses_search_envelope(self): + c = make_client() + other = "22222222-2222-2222-2222-222222222222" + with requests_mock.Mocker() as m: + m.get(self.URL, json=search_envelope([ + item_json(ITEM_UUID, "A"), item_json(other, "B")])) + items = c.get_items_from_collection(COLLECTION_UUID) + self.assertEqual(len(items), 2) + self.assertTrue(all(isinstance(i, Item) for i in items)) + self.assertEqual([i.uuid for i in items], [ITEM_UUID, other]) + + def test_sends_scope_dsotype_sort_embed(self): + c = make_client() + with requests_mock.Mocker() as m: + m.get(self.URL, json=search_envelope([])) + c.get_items_from_collection(COLLECTION_UUID) + qs = sent_params(m.last_request) + self.assertEqual(qs["scope"], [COLLECTION_UUID]) + self.assertEqual(qs["dsoType"], ["ITEM"]) + self.assertEqual(qs["sort"], ["dc.date.accessioned,DESC"]) + self.assertEqual(qs["embed"], ["thumbnail"]) + + +class TestGetItemByHandle(unittest.TestCase): + """Mirrors dspace-rest-test - get_item_by_handle(handle).""" + + URL = f"{API}/core/items/search/byHandle" + + def test_returns_first_item(self): + c = make_client() + with requests_mock.Mocker() as m: + m.get(self.URL, json=embedded("items", [item_json(ITEM_UUID, "T")])) + item = c.get_item_by_handle("123456789/42") + self.assertIsInstance(item, Item) + self.assertEqual(item.uuid, ITEM_UUID) + self.assertEqual(sent_params(m.last_request)["handle"], ["123456789/42"]) + + def test_none_handle_short_circuits(self): + c = make_client() + with requests_mock.Mocker() as m: + self.assertIsNone(c.get_item_by_handle(None)) + self.assertFalse(m.called) + + def test_no_match_returns_none(self): + c = make_client() + with requests_mock.Mocker() as m: + m.get(self.URL, json=embedded("items", [])) + self.assertIsNone(c.get_item_by_handle("123456789/0")) + + def test_non_json_body_returns_none(self): + c = make_client() + with requests_mock.Mocker() as m: + m.get(self.URL, status_code=500, text="error") + self.assertIsNone(c.get_item_by_handle("123456789/42")) + + +class TestGetUserByEmail(unittest.TestCase): + """Mirrors dspace-rest-test - get_user_by_email(email).""" + + URL = f"{API}/eperson/epersons/search/byEmail" + + def test_returns_user_with_uuid_and_email(self): + c = make_client() + with requests_mock.Mocker() as m: + m.get(self.URL, json=user_json(email="a@b.c", netid="n1")) + u = c.get_user_by_email("a@b.c") + self.assertIsInstance(u, User) + self.assertEqual((u.uuid, u.email), (EPERSON_UUID, "a@b.c")) + self.assertEqual(sent_params(m.last_request)["email"], ["a@b.c"]) + + +class TestGetClarinAllowances(unittest.TestCase): + """Mirrors dspace-rest-test - get_clarinlruallowances[_by_bitstream_and_user].""" + + def test_returns_embedded_list(self): + c = make_client() + with requests_mock.Mocker() as m: + m.get(f"{API}/core/clarinlruallowances", + json=embedded("clarinlruallowances", [clarin_allowance_json(1)])) + allowances = c.get_clarinlruallowances() + self.assertEqual(len(allowances), 1) + self.assertEqual(allowances[0]["id"], 1) + + def test_error_returns_none(self): + c = make_client() + with requests_mock.Mocker() as m: + m.get(f"{API}/core/clarinlruallowances", status_code=500, text="boom") + self.assertIsNone(c.get_clarinlruallowances()) + + def test_by_bitstream_and_user_sends_both_params(self): + c = make_client() + with requests_mock.Mocker() as m: + m.get(f"{API}/core/clarinlruallowances/search/byBitstreamAndUser", + json=embedded("clarinlruallowances", [clarin_allowance_json(9)])) + out = c.get_clarinlruallowances_by_bitstream_and_user("bs-1", "usr-1") + self.assertEqual(out[0]["id"], 9) + qs = sent_params(m.last_request) + self.assertEqual(qs["bitstreamUUID"], ["bs-1"]) + self.assertEqual(qs["userUUID"], ["usr-1"]) + + +@pytest.mark.dtq_only +class TestGetOwningCollection(unittest.TestCase): + """dtq-only method. Mirrors src/repo/_audit.py:105-111, which relies on a + None return + last_err.status_code to drive its 401 reauth retry.""" + + URL = f"{API}/core/items/{ITEM_UUID}/owningCollection" + + def test_returns_typed_collection(self): + c = make_client() + with requests_mock.Mocker() as m: + m.get(self.URL, json={"uuid": COLLECTION_UUID, "name": "Coll", + "type": "collection"}) + col = c.get_owningCollection(ITEM_UUID) + self.assertIsInstance(col, Collection) + self.assertEqual(col.uuid, COLLECTION_UUID) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_clarin_usage_contract.py b/tests/test_clarin_usage_contract.py new file mode 100644 index 0000000..7dbd04e --- /dev/null +++ b/tests/test_clarin_usage_contract.py @@ -0,0 +1,192 @@ +""" +CLARIN-side integration contracts - the whole multi-call sequences the three +main-lineage consumers run, the counterpart to test_repo_usage_contract.py. + +Where the per-method tests pin one call, these replay a flow end-to-end (only +the HTTP transport mocked) so a library change that individually looks harmless +but breaks a *chain* a consumer depends on still fails here. + +Each class names the consumer repo it mirrors. clarin = must hold on both +main and dtq; dtq_only = relies on a dtq fix/behaviour, deselected on the main +leg of the differential-contract CI job. +""" +import unittest + +import pytest +import requests_mock + +import _helpers # noqa: F401 +from _helpers import ( + make_client, sent_params, embedded, item_json, bundle_json, raw_policy_json, + user_json, group_json, license_json, label_json, clarin_allowance_json, + search_envelope, API, ITEM_UUID, COLLECTION_UUID, BUNDLE_UUID, BITSTREAM_UUID, + EPERSON_UUID, GROUP_UUID) +from dspace_rest_client.models import Item, Bundle, Group, User, License + +pytestmark = pytest.mark.clarin + + +class TestImportClarinPolicyChain(unittest.TestCase): + """Mirrors dspace-import-clarin - locate the ORIGINAL bundle, read its + resource policy as a raw dict, move the policy to a new group.""" + + def test_bundle_then_policy_dict_then_group_update(self): + c = make_client() + with requests_mock.Mocker() as m: + m.get(f"{API}/core/items/{ITEM_UUID}/bundles", + json=embedded("bundles", [ + bundle_json("lic", "LICENSE"), + bundle_json(BUNDLE_UUID, "ORIGINAL")])) + m.get(f"{API}/authz/resourcepolicies/search/resource", + json=embedded("resourcepolicies", [raw_policy_json(pid=55)])) + m.put(f"{API}/authz/resourcepolicies/55/group", + status_code=200, json={}) + + bundle = c.get_bundle_by_name("ORIGINAL", ITEM_UUID) + self.assertEqual(bundle.uuid, BUNDLE_UUID) + + policy = c.get_resource_policy(bundle.uuid) + pid = policy["id"] # dict subscript, not a model + self.assertEqual(pid, 55) + + r = c.update_resource_policy_group(pid, GROUP_UUID) + self.assertEqual(r.status_code, 200) + self.assertEqual(m.last_request.method, "PUT") + + +class TestImportClarinLicenseIngest(unittest.TestCase): + """Mirrors dspace-import-clarin - read a collection's items, then build the + License dicts the importer writes out.""" + + def test_collection_items_then_license_to_dict(self): + c = make_client() + with requests_mock.Mocker() as m: + m.get(f"{API}/discover/search/objects", + json=search_envelope([item_json(ITEM_UUID, "A"), + item_json(COLLECTION_UUID, "B")])) + items = c.get_items_from_collection(COLLECTION_UUID) + self.assertEqual(len(items), 2) + self.assertTrue(all(isinstance(i, Item) for i in items)) + + lic = License(license_json(lid=1, name="CC-BY", + label=label_json(lid=9, label="PUB"))) + out = lic.to_dict() + self.assertEqual(out["license_id"], 1) + self.assertEqual(out["label_id"], 9) + + +class TestImportClarinMetadataRemoval(unittest.TestCase): + """Mirrors dspace-import-clarin - read items, remove an indexed metadata + value from one (the 3-arg remove_metadata(item, field, place) form).""" + + def test_items_then_indexed_remove(self): + c = make_client() + self_href = f"{API}/core/items/{ITEM_UUID}" + with requests_mock.Mocker() as m: + m.get(f"{API}/discover/search/objects", + json=search_envelope([item_json( + ITEM_UUID, "A", _links={"self": {"href": self_href}})])) + m.patch(self_href, status_code=200, + json=item_json(ITEM_UUID, "A", id=ITEM_UUID)) + + items = c.get_items_from_collection(COLLECTION_UUID) + c.remove_metadata(items[0], "dc.title", 0) + + body = m.last_request.json() + self.assertEqual(body[0]["op"], "remove") + self.assertEqual(body[0]["path"], "/metadata/dc.title/0") + + +class TestRestTestSubmitterSetup(unittest.TestCase): + """Mirrors dspace-rest-test - resolve a user by email, create a collection + submitter group, add the user to it.""" + + def test_email_group_member(self): + c = make_client() + with requests_mock.Mocker() as m: + m.get(f"{API}/eperson/epersons/search/byEmail", + json=user_json(uuid=EPERSON_UUID, email="sub@dq.sk")) + m.post(f"{API}/core/collections/{COLLECTION_UUID}/submittersGroup", + status_code=201, json=group_json(GROUP_UUID, "submitters")) + m.post(f"{API}/eperson/groups/{GROUP_UUID}/epersons", status_code=204) + + user = c.get_user_by_email("sub@dq.sk") + self.assertIsInstance(user, User) + + group = c.create_submit_group( + type("C", (), {"uuid": COLLECTION_UUID})()) + self.assertIsInstance(group, Group) + + self.assertTrue(c.add_member(group, user)) + + +class TestRestTestBitstreamPolicyFlow(unittest.TestCase): + """Mirrors dspace-rest-test - resolve an item by handle, find its ORIGINAL + bundle, grant a resource policy, then read user allowances.""" + + def test_handle_bundle_policy_allowance(self): + c = make_client() + with requests_mock.Mocker() as m: + m.get(f"{API}/core/items/search/byHandle", + json=embedded("items", [item_json(ITEM_UUID, "T")])) + m.get(f"{API}/core/items/{ITEM_UUID}/bundles", + json=embedded("bundles", [bundle_json(BUNDLE_UUID, "ORIGINAL")])) + m.post(f"{API}/authz/resourcepolicies", status_code=201, json={"id": 1}) + m.get(f"{API}/core/clarinlruallowances", + json=embedded("clarinlruallowances", [clarin_allowance_json(1)])) + + item = c.get_item_by_handle("123456789/42") + self.assertEqual(item.uuid, ITEM_UUID) + + bundle = c.get_bundle_by_name("ORIGINAL", item.uuid) + self.assertEqual(bundle.name, "ORIGINAL") + + ok = c.create_resource_policy( + BITSTREAM_UUID, data={"action": "READ"}, group_uuid=GROUP_UUID) + self.assertTrue(ok) + + allowances = c.get_clarinlruallowances() + self.assertEqual(len(allowances), 1) + + +class TestItemImporterPolicyRead(unittest.TestCase): + """Mirrors dspace-item-importer - create a policy, then read it back using + the `.get('id')` access form (not the `["id"]` subscript import-clarin uses).""" + + def test_create_then_get_with_dict_get(self): + c = make_client() + with requests_mock.Mocker() as m: + m.post(f"{API}/authz/resourcepolicies", status_code=201, json={"id": 88}) + m.get(f"{API}/authz/resourcepolicies/search/resource", + json=embedded("resourcepolicies", [raw_policy_json(pid=88)])) + + self.assertTrue( + c.create_resource_policy(BITSTREAM_UUID, data={"action": "READ"})) + + policy = c.get_resource_policy(BUNDLE_UUID) + self.assertEqual(policy.get("id"), 88) # .get(), not [...] + + +@pytest.mark.dtq_only +class TestRestTestNoArgGetItems(unittest.TestCase): + """Mirrors dspace-rest-test/tests/integration/create_bitstreams.py:145 - + the no-arg get_items() call. B1: on main the shadowed second def returned [] + (its `if len(all_items) < 3:` branch flipped); on dtq it paginates and + returns real items with page=0&size=20.""" + + def test_no_arg_get_items_paginates_and_returns_items(self): + c = make_client() + with requests_mock.Mocker() as m: + m.get(f"{API}/core/items", json=embedded("items", [ + item_json("11111111-1111-1111-1111-111111111111", "A"), + item_json("22222222-2222-2222-2222-222222222222", "B"), + item_json("33333333-3333-3333-3333-333333333333", "C")])) + items = c.get_items() + self.assertEqual(len(items), 3) + qs = sent_params(m.last_request) + self.assertEqual(qs["page"], ["0"]) + self.assertEqual(qs["size"], ["20"]) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_clarin_write.py b/tests/test_clarin_write.py new file mode 100644 index 0000000..a4d4853 --- /dev/null +++ b/tests/test_clarin_write.py @@ -0,0 +1,214 @@ +""" +CLARIN/UFAL write surface - resource-policy creation, submitter-group setup, +group membership and metadata removal. Called by dspace-rest-test and +dspace-import-clarin; zero coverage before this suite. + +Mock only the HTTP transport. Each test names the consumer it mirrors. +See test_clarin_read.py for the clarin / dtq_only marker meaning. +""" +import unittest + +import pytest +import requests_mock + +import _helpers # noqa: F401 +from _helpers import ( + make_client, sent_params, item_json, embedded, + API, ITEM_UUID, BITSTREAM_UUID, COLLECTION_UUID, EPERSON_UUID, GROUP_UUID) +from dspace_rest_client.models import Group, User, Collection, Item + +pytestmark = pytest.mark.clarin + +LOGIN_URL = f"{API}/authn/login" +STATUS_URL = f"{API}/authn/status" + + +class TestCreateResourcePolicy(unittest.TestCase): + """Mirrors dspace-rest-test / dspace-item-importer - + create_resource_policy(resource, data, group_uuid=...) truthiness.""" + + URL = f"{API}/authz/resourcepolicies" + + def test_sends_resource_and_group_params_and_body(self): + c = make_client() + with requests_mock.Mocker() as m: + m.post(self.URL, status_code=201, json={"id": 5}) + ok = c.create_resource_policy( + BITSTREAM_UUID, data={"action": "READ"}, group_uuid=GROUP_UUID) + self.assertTrue(ok) + qs = sent_params(m.last_request) + self.assertEqual(qs["resource"], [BITSTREAM_UUID]) + self.assertEqual(qs["group"], [GROUP_UUID]) + self.assertNotIn("eperson", qs) + self.assertEqual(m.last_request.json(), {"action": "READ"}) + + def test_sends_eperson_param_when_given(self): + c = make_client() + with requests_mock.Mocker() as m: + m.post(self.URL, status_code=201, json={"id": 5}) + c.create_resource_policy( + BITSTREAM_UUID, data={"action": "READ"}, eperson_uuid=EPERSON_UUID) + qs = sent_params(m.last_request) + self.assertEqual(qs["eperson"], [EPERSON_UUID]) + self.assertNotIn("group", qs) + + def test_omits_absent_optional_params(self): + c = make_client() + with requests_mock.Mocker() as m: + m.post(self.URL, status_code=201, json={"id": 5}) + c.create_resource_policy(BITSTREAM_UUID, data={"action": "READ"}) + qs = sent_params(m.last_request) + self.assertEqual(qs["resource"], [BITSTREAM_UUID]) + self.assertNotIn("group", qs) + self.assertNotIn("eperson", qs) + + def test_returns_true_only_on_201(self): + c = make_client() + with requests_mock.Mocker() as m: + m.post(self.URL, status_code=201, json={"id": 5}) + self.assertIs(c.create_resource_policy(BITSTREAM_UUID, data={}), True) + + def test_non_201_returns_false(self): + c = make_client() + with requests_mock.Mocker() as m: + m.post(self.URL, status_code=422, json={"message": "bad"}) + self.assertIs(c.create_resource_policy(BITSTREAM_UUID, data={}), False) + + def test_401_reauthenticates_and_retries(self): + """A 401 triggers authenticate()+retry, so a policy create that hit an + expired session still succeeds. Shared: both main and dtq's api_post + carry this reauth block (verified against the main impl), so it is a + contract both must keep - not a dtq-only delta.""" + c = make_client() + with requests_mock.Mocker() as m: + m.post(self.URL, [ + {"status_code": 401, + "json": {"message": "Authentication is required"}}, + {"status_code": 201, "json": {"id": 5}}]) + m.post(LOGIN_URL, status_code=200) + m.get(STATUS_URL, status_code=200, json={"authenticated": True}) + ok = c.create_resource_policy(BITSTREAM_UUID, data={"action": "READ"}) + self.assertTrue(ok) + posts = [h for h in m.request_history + if h.method == "POST" and h.url.startswith(self.URL)] + self.assertEqual(len(posts), 2) # first 401, retry 201 + + +class TestUpdateResourcePolicyGroup(unittest.TestCase): + """Mirrors dspace-import-clarin - update_resource_policy_group(id, group).""" + + def test_puts_uri_list_and_returns_response(self): + c = make_client() + url = f"{API}/authz/resourcepolicies/77/group" + with requests_mock.Mocker() as m: + m.put(url, status_code=200, json={}) + r = c.update_resource_policy_group(77, GROUP_UUID) + self.assertEqual(r.status_code, 200) # returns raw Response + self.assertEqual(m.last_request.text, + f"{API}/eperson/groups/{GROUP_UUID}") + self.assertEqual( + m.last_request.headers["Content-type"], "text/uri-list") + + def test_403_csrf_retries_once(self): + c = make_client() + url = f"{API}/authz/resourcepolicies/77/group" + with requests_mock.Mocker() as m: + m.put(url, [ + {"status_code": 403, "json": {"message": "Invalid CSRF token"}}, + {"status_code": 200, "json": {}}]) + r = c.update_resource_policy_group(77, GROUP_UUID) + self.assertEqual(r.status_code, 200) + self.assertEqual( + len([h for h in m.request_history if h.method == "PUT"]), 2) + + +class TestCreateSubmitGroup(unittest.TestCase): + """Mirrors dspace-rest-test - create_submit_group(collection).""" + + def _collection(self): + return Collection({"uuid": COLLECTION_UUID, "type": "collection"}) + + def test_posts_to_submitters_group_url_and_returns_group(self): + c = make_client() + url = f"{API}/core/collections/{COLLECTION_UUID}/submittersGroup" + with requests_mock.Mocker() as m: + m.post(url, status_code=201, + json={"uuid": GROUP_UUID, "name": "submitters", "type": "group"}) + g = c.create_submit_group(self._collection()) + self.assertIsInstance(g, Group) + self.assertEqual(g.uuid, GROUP_UUID) + + def test_non_201_returns_none(self): + c = make_client() + url = f"{API}/core/collections/{COLLECTION_UUID}/submittersGroup" + with requests_mock.Mocker() as m: + m.post(url, status_code=500, text="boom") + self.assertIsNone(c.create_submit_group(self._collection())) + + +class TestAddMember(unittest.TestCase): + """Mirrors dspace-rest-test - add_member(group, eperson).""" + + def _group(self): + return Group({"uuid": GROUP_UUID, "name": "submitters"}) + + def _user(self): + return User({"uuid": EPERSON_UUID, "email": "a@b.c"}) + + def test_non_204_returns_false(self): + c = make_client() + url = f"{API}/eperson/groups/{GROUP_UUID}/epersons" + with requests_mock.Mocker() as m: + m.post(url, status_code=422, json={"message": "nope"}) + self.assertFalse(c.add_member(self._group(), self._user())) + + def test_rejects_non_group_and_non_user_without_request(self): + c = make_client() + with requests_mock.Mocker() as m: + self.assertFalse(c.add_member("not-a-group", self._user())) + self.assertFalse(c.add_member(self._group(), "not-a-user")) + self.assertFalse(m.called) + + +class TestRemoveMetadata(unittest.TestCase): + """Mirrors dspace-import-clarin - remove_metadata(item, field, place).""" + + def _item(self): + return Item(item_json( + ITEM_UUID, "T", + _links={"self": {"href": f"{API}/core/items/{ITEM_UUID}"}})) + + def test_with_place_patches_indexed_path(self): + c = make_client() + url = f"{API}/core/items/{ITEM_UUID}" + with requests_mock.Mocker() as m: + m.patch(url, status_code=200, + json=item_json(ITEM_UUID, "T", id=ITEM_UUID)) + c.remove_metadata(self._item(), "dc.title", 0) + body = m.last_request.json() + self.assertEqual(body[0]["op"], "remove") + self.assertEqual(body[0]["path"], "/metadata/dc.title/0") + + @pytest.mark.dtq_only + def test_place_none_removes_whole_field(self): + """B2: BEHAVIOUR CHANGE vs main. On main `place` was mandatory and a + None place was a no-op; on dtq place=None removes EVERY value of the + field (path has no index).""" + c = make_client() + url = f"{API}/core/items/{ITEM_UUID}" + with requests_mock.Mocker() as m: + m.patch(url, status_code=200, + json=item_json(ITEM_UUID, "T", id=ITEM_UUID)) + c.remove_metadata(self._item(), "dc.title") # place defaults None + body = m.last_request.json() + self.assertEqual(body[0]["path"], "/metadata/dc.title") + + def test_invalid_dso_returns_self_without_request(self): + c = make_client() + with requests_mock.Mocker() as m: + self.assertIs(c.remove_metadata(None, "dc.title", 0), c) + self.assertFalse(m.called) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_client_read.py b/tests/test_client_read.py index f48b04a..9b67890 100644 --- a/tests/test_client_read.py +++ b/tests/test_client_read.py @@ -7,6 +7,7 @@ """ import unittest +import pytest import requests_mock import _helpers # noqa: F401 @@ -78,6 +79,22 @@ def test_parses_embedded_items_with_paging_params(self): self.assertEqual(p["page"], ["2"]) self.assertEqual(p["size"], ["50"]) + @pytest.mark.dtq_only + def test_no_arg_defaults_to_first_page(self): + """B1: the no-arg get_items() form (dspace-rest-test .../create_bitstreams.py + :145) sends page=0&size=20 and returns items. On main the *second*, + shadowing `def get_items(self)` gated on 'collections' and always + returned [] - this is the behaviour that changes on merge.""" + c = make_client() + with requests_mock.Mocker() as m: + m.get(f"{API}/core/items", + json=embedded("items", [item_json("i1", "one")])) + items = c.get_items() + self.assertEqual(len(items), 1) + p = sent_params(m.last_request) + self.assertEqual(p["page"], ["0"]) + self.assertEqual(p["size"], ["20"]) + class TestGetItem(unittest.TestCase): diff --git a/tests/test_models_clarin.py b/tests/test_models_clarin.py new file mode 100644 index 0000000..89f3d4c --- /dev/null +++ b/tests/test_models_clarin.py @@ -0,0 +1,94 @@ +""" +CLARIN/UFAL model classes - License, Label (dspace-import-clarin) and the +Group / User objects the submitter-setup flow builds. No coverage before this. + +See test_clarin_read.py for the clarin / dtq_only marker meaning. +""" +import unittest + +import pytest + +import _helpers # noqa: F401 +from _helpers import ( + group_json, user_json, license_json, label_json, EPERSON_UUID, GROUP_UUID) +from dspace_rest_client.models import License, Label, Group, User + +pytestmark = pytest.mark.clarin + + +class TestLicense(unittest.TestCase): + """Mirrors dspace-import-clarin - License(...) / .to_dict().""" + + def test_core_fields(self): + lic = License(license_json(lid=3, name="CC-BY", confirmation=1, + required_info="SEND_TOKEN")) + self.assertEqual(lic.id, 3) + self.assertEqual(lic.name, "CC-BY") + self.assertEqual(lic.confirmation, 1) + self.assertEqual(lic.requiredInfo, "SEND_TOKEN") + self.assertTrue(lic.definition) + + def test_nested_clarin_license_label_becomes_label(self): + lic = License(license_json(label=label_json(lid=10, label="PUB"))) + self.assertIsInstance(lic.licenseLabel, Label) + self.assertEqual(lic.licenseLabel.label, "PUB") + + def test_extended_labels_list(self): + lic = License(license_json(extended=[label_json(11, "A"), + label_json(12, "B")])) + self.assertEqual(len(lic.extendedLicenseLabel), 2) + self.assertTrue(all(isinstance(x, Label) for x in lic.extendedLicenseLabel)) + + def test_to_dict_keys(self): + lic = License(license_json(lid=3, label=label_json(lid=10))) + self.assertEqual( + set(lic.to_dict()), + {"name", "license_id", "definition", "confirmation", + "required_info", "label_id"}) + self.assertEqual(lic.to_dict()["license_id"], 3) + self.assertEqual(lic.to_dict()["label_id"], 10) + + def test_to_dict_label_id_none_when_no_label(self): + lic = License(license_json()) + self.assertIsNone(lic.to_dict()["label_id"]) + + def test_from_empty_resource_does_not_crash(self): + self.assertIsNone(License({}).name) + + +class TestLabel(unittest.TestCase): + """Mirrors dspace-import-clarin - Label(...) / .to_dict().""" + + def test_core_fields_and_to_dict(self): + lab = Label(label_json(lid=7, label="PUB", title="Public", icon="p.png", + extended=True)) + self.assertEqual((lab.label, lab.title, lab.icon), ("PUB", "Public", "p.png")) + d = lab.to_dict() + self.assertEqual(d["label_id"], 7) + self.assertTrue(d["is_extended"]) + + def test_extended_defaults_false(self): + self.assertFalse(Label({"label": "x"}).extended) + + +class TestGroup(unittest.TestCase): + """Group is built by create_submit_group / consumed by add_member.""" + + def test_fields_and_as_dict(self): + g = Group(group_json(uuid=GROUP_UUID, name="submitters", permanent=True)) + self.assertEqual((g.uuid, g.name, g.permanent), (GROUP_UUID, "submitters", True)) + self.assertEqual(g.as_dict()["name"], "submitters") + + +class TestUser(unittest.TestCase): + """User is built by get_user_by_email / consumed by add_member.""" + + def test_fields_and_as_dict(self): + u = User(user_json(uuid=EPERSON_UUID, email="a@b.c", netid="n1")) + self.assertEqual((u.uuid, u.email, u.netid), (EPERSON_UUID, "a@b.c", "n1")) + self.assertTrue(u.canLogIn) + self.assertEqual(u.as_dict()["email"], "a@b.c") + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_transport_hardening.py b/tests/test_transport_hardening.py new file mode 100644 index 0000000..643a7f7 --- /dev/null +++ b/tests/test_transport_hardening.py @@ -0,0 +1,126 @@ +""" +Transport hardening introduced on dtq - timeout / proxies on every verb, the +verify_response helper, and the last_err bookkeeping. These are the changes +that make the newly merged CLARIN methods actually surface transient failures +(instead of hanging) - so they are proven here against the low-level api_*. + +All dtq_only: main's client takes no timeout/proxies and has no +verify_response / last_err. +""" +import unittest + +import pytest + +import _helpers # noqa: F401 +from _helpers import make_client, API + +pytestmark = [pytest.mark.dtq_only] + + +class FakeResp: + """Minimal stand-in for requests.Response - enough for update_token / + parse_json / verify_response to run without a real transport.""" + + def __init__(self, status_code=200, body=None, bad_json=False): + self.status_code = status_code + self.headers = {} + self.url = "http://dspace.test" + self.text = "" if body is None else str(body) + self._body = {} if body is None else body + self._bad_json = bad_json + + def json(self): + if self._bad_json: + raise ValueError("not json") + return self._body + + +class RecordingSession: + """Captures the kwargs each verb is called with.""" + + def __init__(self, resp): + self._resp = resp + self.calls = {} + + def _verb(self, name): + def f(url, **kw): + self.calls[name] = kw + return self._resp + return f + + def __getattr__(self, name): + if name in ("get", "post", "put", "delete", "patch"): + return self._verb(name) + raise AttributeError(name) + + +def _client_with_session(timeout=42, proxies=None): + c = make_client() + c.timeout = timeout + c.proxies = proxies if proxies is not None else {"http": "http://proxy:3128"} + sess = RecordingSession(FakeResp(200, {})) + c.session = sess + return c, sess + + +class TestTimeoutAndProxiesOnEveryVerb(unittest.TestCase): + + def test_timeout_is_passed_on_every_verb(self): + c, sess = _client_with_session(timeout=7) + c.api_get(f"{API}/x") + c.api_post(f"{API}/x", params=None, json={}) + c.api_put(f"{API}/x", params=None, json={}) + c.api_delete(f"{API}/x", params=None) + for verb in ("get", "post", "put", "delete"): + self.assertEqual(sess.calls[verb].get("timeout"), 7, + f"{verb} did not pass timeout to the transport") + + def test_proxies_are_passed_on_every_verb(self): + proxies = {"http": "http://proxy:3128", "https": "http://proxy:3128"} + c, sess = _client_with_session(proxies=proxies) + c.api_get(f"{API}/x") + c.api_post(f"{API}/x", params=None, json={}) + c.api_put(f"{API}/x", params=None, json={}) + c.api_delete(f"{API}/x", params=None) + for verb in ("get", "post", "put", "delete"): + self.assertEqual(sess.calls[verb].get("proxies"), proxies, + f"{verb} did not pass proxies to the transport") + + def test_clarin_get_honours_custom_timeout(self): + """A CLARIN read (get_clarinlruallowances) must ride the same timeout.""" + c, sess = _client_with_session(timeout=3) + c.get_clarinlruallowances() + self.assertEqual(sess.calls["get"].get("timeout"), 3) + + +class TestVerifyResponse(unittest.TestCase): + + def test_non_200_records_last_err_and_returns_false(self): + c = make_client() + r = FakeResp(503, "unavailable") + self.assertFalse(c.verify_response(r, "id:1")) + self.assertIs(c.last_err, r) + + def test_200_ok_returns_true(self): + c = make_client() + self.assertTrue(c.verify_response(FakeResp(200, {"ok": True}), "id:1")) + + def test_as_json_on_invalid_body_returns_false(self): + c = make_client() + self.assertFalse( + c.verify_response(FakeResp(200, bad_json=True), "id:1", as_json=True)) + + +class TestLastErrReset(unittest.TestCase): + + def test_last_err_is_reset_at_the_start_of_each_request(self): + """A stale error from a previous call must not be read by the next one: + api_* clears _last_err on entry.""" + c, _sess = _client_with_session() + c._last_err = FakeResp(500, "stale") + c.api_get(f"{API}/x") + self.assertIsNone(c.last_err) + + +if __name__ == "__main__": + unittest.main()