diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index d52185c..95c9d22 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -43,27 +43,23 @@ jobs: steps: - uses: actions/checkout@v6 - - name: Install uv - uses: astral-sh/setup-uv@v6 + - name: Set up Python + uses: actions/setup-python@v6 + with: + python-version: "3.12" + + - name: Install pre-commit + run: pip install pre-commit==4.2.0 - name: Ruff - run: uvx ruff@0.12.7 check dspace_rest_client tests + run: pre-commit run ruff --all-files - name: Pylint - run: >- - uvx --with requests --with pysolr pylint@3.3.7 - --rcfile=pyproject.toml dspace_rest_client - - typecheck: - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v6 - - - name: Install uv - uses: astral-sh/setup-uv@v6 + run: pre-commit run pylint --all-files + # Non-blocking: strict typing is being introduced incrementally, so a + # mypy finding is reported in the log but does not fail the job. The + # version is pinned by the mirrors-mypy rev in .pre-commit-config.yaml. - name: Mypy - # Non-blocking: strict typing is being introduced incrementally, so a - # mypy failure is reported but does not fail the (green) job. continue-on-error: true - run: uvx --with requests --with pysolr mypy dspace_rest_client + run: pre-commit run mypy --all-files diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index de7c633..d9a0225 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -12,16 +12,26 @@ repos: rev: v2.3.2 hooks: - id: autopep8 + files: ^(dspace_rest_client/|tests/) args: ['-i', '--max-line-length=90', '--ignore=E402'] - repo: https://github.com/astral-sh/ruff-pre-commit rev: v0.12.7 hooks: - id: ruff + files: ^(dspace_rest_client/|tests/) args: [--fix, --exit-non-zero-on-fix] - repo: https://github.com/pylint-dev/pylint rev: v3.3.7 hooks: - id: pylint - exclude: ^tests/ + files: ^dspace_rest_client/ args: ['-rn', '-sn', '--rcfile=pyproject.toml'] additional_dependencies: [requests, pysolr] + - repo: https://github.com/pre-commit/mirrors-mypy + rev: v2.3.1 + hooks: + - id: mypy + files: ^dspace_rest_client/ + args: [dspace_rest_client] + pass_filenames: false + additional_dependencies: [requests, pysolr] diff --git a/CHANGELOG.md b/CHANGELOG.md index 67defbf..7e38698 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,35 @@ # Changelog +### 0.2.0 + +Date: Unreleased + +**Changes** + +1. Migrated packaging and release builds from `setup.py` to `pyproject.toml`. +2. Raised the minimum supported Python version to 3.10 and added type checking. +3. Hardened UUID validation across every UUID-taking method (`get_dso`, `get_item`, + `get_resourcepolicy`, `create_resourcepolicy`, `get_owningCollection`): a `None` + or non-string argument is now logged and returns `None` instead of raising + `TypeError`. +4. A non-JSON `401`/`403` body no longer crashes the CSRF-refresh/retry path of + `api_post`, `api_post_uri`, `api_put`, `api_put_uri`, `api_delete`, `api_patch` + and `create_bitstream`. +5. `get_communities`, `get_collections` and `get_bundle_by_name` return `None` on a + failed or non-JSON response instead of raising `TypeError`. +6. `add_metadata` / `remove_metadata` return `None` (not the client) on invalid input. +7. Model attribute defaults moved from the class body into `__init__` as plain + instance attributes, so no instance can share (or mutate) a class-level + `links`, `embedded`, `metadata`, `checkSum` or `sections` dict. `Group()` / + `User()` and `EntityType()` accept a `None` API resource, and `Item.from_dso` / + `DSpaceObject(dso=...)` deep-copy metadata instead of aliasing it. Side effect: + `id` on a `DSpaceObject` (and its subclasses) and `label` on an `EntityType` + now default to `None` rather than raising `AttributeError` when the API + resource omits them. +8. `models.__all__` exports the full model surface re-exported by the package. +9. Moved direct Solr support to the documented `solr` optional dependency group; + `solr_query()` raises an actionable `RuntimeError` when the extra is missing. + ### 0.1.10 Date: 2024-04-04 diff --git a/MAINTAINING.md b/MAINTAINING.md index 0efeed4..c500b54 100644 --- a/MAINTAINING.md +++ b/MAINTAINING.md @@ -6,13 +6,12 @@ These notes are for maintenance of the Git / PyPI source and releases, rather th All the tasks we need to do, in order, when releasing a new version: -1. - [ ] **Check the main branch!** - we should have all the changes we want to include merged/picked and tested -2. - [ ] **Update setup.py** - this might include other dependency or project description changes, but usually will just be a case of incrementing the version number, e.g. `0.1.9` -> `0.1.10`. Note the new number. -3. - [ ] **Update publish.sh** - this simple publish script performs the publish to PyPI and will need the new version number -4. - [ ] **Update CHANGELOG.md** - new versions go at the top of the file. See previous release blocks for formatting. I include a 'thanks' or 'reported by' attribution for PRs contributed or issues reported. The new version number from `setup.py` is used for the heading and the (future) PyPI URL -4. - [ ] **Commit release preparation** - once you are happy with the steps above, commit with a message like 'Prepare release 0.1.10' -5. - [ ] **Push branch** - making sure github is up to date, (in future: CI) -6. - [ ] **Clear out build and dist directories**: OPTIONAL, but nice to start with a clean Python build environment before making this new version -7. - [ ] **Run publish script** - this will run `setup.py` to build a new version then upload to PyPI with twine - you will be prompted for credentials interactively +1. - [ ] **Check the main branch** — confirm all intended changes are merged and CI is green. +2. - [ ] **Update `project.version` in `pyproject.toml`** — note the new version number. +3. - [ ] **Update `CHANGELOG.md`** — move the new version to the top, add the release date and future PyPI URL, and summarize user-visible changes. +4. - [ ] **Install release tools** — run `python -m pip install ".[release]"`. +5. - [ ] **Run tests and checks** — run `python -m pytest tests/ -v` and `pre-commit run --all-files`. +6. - [ ] **Commit and push release preparation** — use a message such as `Prepare release 0.2.0`, then confirm CI remains green. +7. - [ ] **Run `./publish.sh`** — it clears old artifacts, builds the sdist and wheel from `pyproject.toml`, validates them with Twine, and uploads them to PyPI. Twine prompts for credentials when needed. -TODO: If we just keep a `version` file around some of these steps can be more easily automated or derived instead of updated by hand, but for now it's all pretty simple. \ No newline at end of file +The publish script derives artifact names from the build output, so it does not need a version-specific edit. \ No newline at end of file diff --git a/README.md b/README.md index 2841747..46eee50 100644 --- a/README.md +++ b/README.md @@ -1,5 +1,5 @@ # DSpace Python REST Client Library -This client library allows Python 3 scripts (Python 2 probably compatible but not officially supported) to interact with +This client library allows Python 3.10+ scripts to interact with DSpace 7+ repositories, using the DSpace REST API. This library is a work in progress and so far offers basic create, update, retrieve functionality for @@ -15,8 +15,8 @@ PyPI homepage: https://pypi.org/project/dspace-rest-client/ * Working DSpace 7 repository with an accessible REST API ## Installation -To install with pip: -`pip install dspace_rest_client` +To install with pip: +`pip install dspace-rest-client` (or `pip3` or `python -m pip` as appropriate to your environment) @@ -27,6 +27,17 @@ cd dspace-rest-python pip install . ``` +### Solr support + +Direct Solr queries require the optional `solr` dependency group: + +```commandline +pip install "dspace-rest-client[solr]" +``` + +Without that extra, REST API operations remain available, but `solr_query()` +raises an error explaining how to install Solr support. + ## Usage After installing dependencies, you're ready to run the script. diff --git a/dspace_rest_client/client.py b/dspace_rest_client/client.py index f58a688..444ca60 100644 --- a/dspace_rest_client/client.py +++ b/dspace_rest_client/client.py @@ -16,7 +16,7 @@ """ from __future__ import annotations -import json as _json +from json import dumps import logging import os from typing import Any, Optional @@ -61,13 +61,14 @@ def parse_json(response) -> Any: response_json = response.json() except ValueError as err: if response is not None: - _logger.error(f'Error parsing response JSON: {err}. Body text: {response.text}') + _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 -def _is_valid_uuid(value: str) -> bool: +def _is_valid_uuid(value: object) -> bool: """Return True if `value` is a well-formed UUID string, else False.""" try: UUID(str(value)) @@ -76,6 +77,13 @@ def _is_valid_uuid(value: str) -> bool: return False +def _response_message_contains(response: requests.Response, text: str) -> bool: + response_json = parse_json(response) + if not isinstance(response_json, dict): + return False + return text in str(response_json.get('message', '')) + + class DSpaceClient: """ Main class of the API client itself. This client uses request sessions to connect and authenticate to @@ -85,7 +93,7 @@ class DSpaceClient: Higher level get, create, update, partial_update (patch) functions are implemented for each DSO type """ # Set up basic environment, variables - session = None + session: requests.Session API_ENDPOINT = 'http://localhost:8080/server/api' SOLR_ENDPOINT = 'http://localhost:8983/solr' SOLR_AUTH = None @@ -142,13 +150,15 @@ def __init__(self, api_endpoint: str = API_ENDPOINT, username: str = USERNAME, # never leaks into other default-constructed clients. self.proxies = proxies if proxies is not None else dict(self.PROXY_DICT) self.solr = None - self._last_err = None + self._last_err: requests.Response | None = 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) - except Exception: - pass + except ImportError: + pysolr = None + if pysolr is not None: + self.solr = pysolr.Solr( + url=solr_endpoint, always_commit=True, timeout=300, auth=solr_auth) # If fake_user_agent was specified, use this string that is known (as of 2023-12-03) to succeed with # requests to Cloudfront-protected API endpoints (tested on demo.dspace.org) # Otherwise, the user agent will be the more helpful and accurate default of 'DSpace Python REST Client' @@ -159,8 +169,10 @@ def __init__(self, api_endpoint: str = API_ENDPOINT, username: str = USERNAME, 'Chrome/39.0.2171.95 Safari/537.36' # Set headers based on this self.auth_request_headers = {'User-Agent': self.USER_AGENT} - 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} + 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): @@ -185,7 +197,8 @@ def authenticate(self, retry: bool = False) -> bool: # 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 if retry: - _logger.error(f'Too many retries updating token: {r.status_code}: {r.text}') + _logger.error( + f'Too many retries updating token: {r.status_code}: {r.text}') return False _logger.debug("Retrying request with updated CSRF token") return self.authenticate(retry=True) @@ -193,12 +206,13 @@ def authenticate(self, retry: bool = False) -> bool: if r.status_code == 401: # 401 Unauthorized # If we get a 401, this means a general authentication failure - _logger.error(f'Authentication failure: invalid credentials for user {self.USERNAME}') + _logger.error( + f'Authentication failure: invalid credentials for user {self.USERNAME}') return False # Update headers with new bearer token if present if 'Authorization' in r.headers: - self.session.headers.update({'Authorization': r.headers.get('Authorization')}) + self.session.headers.update({'Authorization': r.headers['Authorization']}) # Get and check authentication status r = self.session.get(f'{self.API_ENDPOINT}/authn/status', headers=self.request_headers, @@ -217,7 +231,8 @@ def verify_response(self, r, id_str: str, as_json: bool = False) -> bool: 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} ]') + _logger.error( + f'Error response [{id_str}]: {r.status_code}: {r.text} ... [ {r.url} ]') self._last_err = r return False @@ -225,12 +240,12 @@ def verify_response(self, r, id_str: str, as_json: bool = False) -> bool: try: r.json() except ValueError: - _logger.error(f'Error parsing JSON response [{id_str}]: {r.text} ... [ {r.url} ]') + _logger.error( + f'Error parsing JSON response [{id_str}]: {r.text} ... [ {r.url} ]') return False return True - def refresh_token(self) -> None: """ If the DSPACE-XSRF-TOKEN appears, we need to update our local stored token and re-send our API request @@ -278,10 +293,10 @@ def api_post(self, url: str, params, json: Any, retry: bool = False, # If we had a CSRF failure, retry the request with the updated token # 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 or {}) and 'CSRF token' in r_json['message']: + if _response_message_contains(r, 'CSRF token'): if retry: - _logger.warning(f'Too many retries updating token: {r.status_code}: {r.text}') + _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, timeout=timeout) @@ -289,8 +304,7 @@ def api_post(self, url: str, params, json: Any, retry: bool = False, # we need to log in again, if there is login error. This is a bad # solution copied from the past elif r.status_code == 401: - r_json = parse_json(r) - if 'message' in (r_json or {}) and 'Authentication is required' in r_json['message']: + if _response_message_contains(r, 'Authentication is required'): if retry: _logger.error( 'API Post: Already retried... something must be wrong') @@ -324,10 +338,10 @@ def api_post_uri(self, url: str, params, uri_list, # If we had a CSRF failure, retry the request with the updated token # 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 = r.json() - if 'message' in r_json and 'CSRF token' in r_json['message']: + if _response_message_contains(r, 'CSRF token'): if retry: - _logger.warning(f'Too many retries updating token: {r.status_code}: {r.text}') + _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_uri(url, params=params, uri_list=uri_list, retry=True) @@ -357,10 +371,10 @@ def api_put(self, url: str, params, json: Any, # it's happening too often for me, so check for accidentally triggering it _logger.debug(r.text) # Parse response - r_json = parse_json(r) - if 'message' in r_json and 'CSRF token' in r_json['message']: + if _response_message_contains(r, 'CSRF token'): if retry: - _logger.warning(f'Too many retries updating token: {r.status_code}: {r.text}') + _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_put(url, params=params, json=json, retry=True) @@ -390,10 +404,10 @@ def api_put_uri(self, url: str, params, uri_list, # it's happening too often for me, so check for accidentally triggering it _logger.debug(r.text) # Parse response - r_json = parse_json(r) - if 'message' in r_json and 'CSRF token' in r_json['message']: + if _response_message_contains(r, 'CSRF token'): if retry: - _logger.warning(f'Too many retries updating token: {r.status_code}: {r.text}') + _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_put_uri(url, params=params, uri_list=uri_list, retry=True) @@ -421,10 +435,10 @@ def api_delete(self, url: str, params, retry: bool = False) -> requests.Response # it's happening too often for me, so check for accidentally triggering it _logger.debug(r.text) # Parse response - r_json = parse_json(r) - if 'message' in r_json and 'CSRF token' in r_json['message']: + if _response_message_contains(r, 'CSRF token'): if retry: - _logger.warning(f'Too many retries updating token: {r.status_code}: {r.text}') + _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_delete(url, params=params, retry=True) @@ -448,12 +462,14 @@ def api_patch(self, url: str, operation, path, value, params=None, _logger.error('Missing required URL argument') return None if path is None: - _logger.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 in (self.PatchOperation.ADD, self.PatchOperation.REPLACE, self.PatchOperation.MOVE) and value is None: # missing value required for add/replace/move operations - _logger.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 @@ -479,16 +495,17 @@ def api_patch(self, url: str, operation, path, value, params=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 _logger.debug(r.text) - r_json = parse_json(r) - if 'message' in r_json and 'CSRF token' in r_json['message']: + if _response_message_contains(r, 'CSRF token'): if retry: - _logger.warning(f'Too many retries updating token: {r.status_code}: {r.text}') + _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, params, True) elif r.status_code == 200: # 200 Success - _logger.info(f'successful patch update to {r.json()["type"]} {r.json()["id"]}') + _logger.info( + f'successful patch update to {r.json()["type"]} {r.json()["id"]}') # Return the raw API response return r @@ -568,24 +585,21 @@ def get_resourcepolicy(self, uuid: str, @param action: action name to filter by (default: READ) @return: Parsed JSON response from fetch_resource or None if error """ - try: - # Validate UUID - UUID(uuid) - 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) - if r_json is None: - 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 as e: - _logger.error(f'Invalid resource UUID: {uuid} - {e}') + if not _is_valid_uuid(uuid): + _logger.error(f'Invalid resource UUID: {uuid}') + return None + 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) + if r_json is None: 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] def create_resourcepolicy( self, resource_uuid: str, group_uuid: str, action: str = 'READ', @@ -601,10 +615,7 @@ def create_resourcepolicy( @param end_date: optional end date string (ISO 8601, YYYY-MM-DD) @return: ResourcePolicy on success, None on failure """ - try: - UUID(resource_uuid) - UUID(group_uuid) - except ValueError: + if not _is_valid_uuid(resource_uuid) or not _is_valid_uuid(group_uuid): _logger.error(f'Invalid UUID: resource={resource_uuid}, group={group_uuid}') return None @@ -625,7 +636,8 @@ def create_resourcepolicy( r = self.api_post(url, params=params, json=data) 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}') + _logger.info( + f'Created resource policy id={rp.id} for resource {resource_uuid}') return rp _logger.error( @@ -640,14 +652,11 @@ def get_dso(self, url: str, uuid: str) -> Optional[requests.Response]: @param uuid: UUID of object to retrieve @return: Parsed JSON response from fetch_resource """ - try: - # Try to get UUID version to test validity - UUID(uuid) - url = f'{url}/{uuid}' - return self.api_get(url, None, None) - except ValueError: + if not _is_valid_uuid(uuid): _logger.error(f'Invalid DSO UUID: {uuid}') return None + url = f'{url}/{uuid}' + return self.api_get(url, None, None) def create_dso(self, url: str, params, data) -> requests.Response: """ @@ -663,7 +672,8 @@ def create_dso(self, url: str, params, data) -> requests.Response: if r.status_code == 201: # 201 Created - success! new_dso = parse_json(r) - _logger.info(f'Object type[{new_dso["type"]}] uuid:[{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 @@ -682,7 +692,7 @@ def update_dso(self, dso, params=None) -> Optional[DSpaceObject]: dso_type = type(dso) if not isinstance(dso, SimpleDSpaceObject): _logger.error('Only SimpleDSpaceObject types (eg Item, Collection, Community) ' - 'are supported by generic update_dso PUT.') + 'are supported by generic update_dso PUT.') return dso try: # Get self URI from HAL links @@ -692,19 +702,12 @@ def update_dso(self, dso, params=None) -> Optional[DSpaceObject]: if 'lastModified' in data: data.pop('lastModified') - # if 'id' in data: - # data.pop('id') - # if 'handle' in data: - # data.pop('handle') - # if 'uuid' in data: - # data.pop('uuid') - # if 'type' in data: - # data.pop('type') r = self.api_put(url, params=params, json=data) 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 successfully!') + _logger.debug( + f'{updated_dso.type} {updated_dso.uuid} updated successfully!') return updated_dso _logger.error(f'update operation failed: {r.status_code}: {r.text} ({url})') return None @@ -730,7 +733,7 @@ def delete_dso(self, dso=None, url=None, params=None): else: if not isinstance(dso, SimpleDSpaceObject): _logger.error('Only SimpleDSpaceObject types (eg Item, Collection, Community, EPerson) ' - 'are supported by generic update_dso PUT.') + 'are supported by generic update_dso PUT.') return dso # Get self URI from HAL links url = dso.links['self']['href'] @@ -759,7 +762,7 @@ def get_bundles(self, parent=None, uuid=None, page: int = 0, size: int = 20, """ # TODO: It is probably wise to allow the parent UUID to be simply passed as an alternative to having the full # python object as constructed by this REST client, for more flexible usage. - bundles = [] + bundles: list[Bundle] = [] single_result = False if uuid is not None: url = f'{self.API_ENDPOINT}/core/bundles/{uuid}' @@ -768,7 +771,7 @@ def get_bundles(self, parent=None, uuid=None, page: int = 0, size: int = 20, url = f'{self.API_ENDPOINT}/core/items/{parent.uuid}/bundles' else: return [] - params = {} + params: dict[str, Any] = {} if size is not None: params['size'] = size if page is not None: @@ -839,9 +842,10 @@ def get_bitstreams(self, uuid=None, bundle=None, page: int = 0, size: int = 20, url = bundle.links['bitstreams']['href'] else: url = f'{self.API_ENDPOINT}/core/bundles/{bundle.uuid}/bitstreams' - _logger.info(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 = {} + params: dict[str, Any] = {} if size is not None: params['size'] = size if page is not None: @@ -894,22 +898,18 @@ def create_bitstream(self, bundle=None, name=None, path=None, mime=None, 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'} + payload = {'properties': dumps(properties) + ';application/json'} # 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) - 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(f'Updating token to {t}') - self.session.headers.update({'X-XSRF-Token': t}) - self.session.cookies.update({'X-XSRF-Token': t}) + r = self.session.send(prepared_req, proxies=self.proxies, + timeout=self.timeout) + self.update_token(r) 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 _response_message_contains(r, 'CSRF token'): _logger.debug("Retrying request with updated CSRF token") else: self.authenticate() @@ -946,20 +946,21 @@ def get_communities(self, uuid: Optional[str] = None, page: int = 0, size: int = @return: list of communities, or None if error """ url = f'{self.API_ENDPOINT}/core/communities' - params = {} + params: dict[str, Any] = {} if size is not None: params['size'] = size if page is not None: params['page'] = page if sort is not None: params['sort'] = sort + request_params: dict[str, Any] | None = params if uuid is not None: if not _is_valid_uuid(uuid): _logger.error(f'Invalid community UUID: {uuid}') return None # Set URL and parameters url = f'{url}/{uuid}' - params = None + request_params = None if top: # Set new URL @@ -967,7 +968,13 @@ def get_communities(self, uuid: Optional[str] = None, page: int = 0, size: int = _logger.debug(f'Performing get on {url}') # Perform actual get - r_json = self.fetch_resource(url, params) + r_json = self.fetch_resource(url, request_params) + if r_json is None: + # a failed / non-JSON response is an error, not "no communities" - + # returning None here matches the documented contract instead of + # raising an opaque TypeError on the membership test below. + _logger.error(f'Failed to fetch communities [{url}]') + return None # Empty list communities = [] if '_embedded' in r_json: @@ -1007,13 +1014,14 @@ def get_collections(self, uuid: Optional[str] = None, community=None, page: int for consistency of handling results, even the uuid search will be a list of one """ url = f'{self.API_ENDPOINT}/core/collections' - params = {} + params: dict[str, Any] = {} if size is not None: params['size'] = size if page is not None: params['page'] = page if sort is not None: params['sort'] = sort + request_params: dict[str, Any] | None = params # First, handle case of UUID. It overrides the other arguments as it is a request for a single collection if uuid is not None: if not _is_valid_uuid(uuid): @@ -1021,7 +1029,7 @@ def get_collections(self, uuid: Optional[str] = None, community=None, page: int return None # Update URL and parameters url = f'{url}/{uuid}' - params = None + request_params = None if community is not None: if 'collections' in community.links and 'href' in community.links['collections']: @@ -1029,7 +1037,12 @@ def get_collections(self, uuid: Optional[str] = None, community=None, page: int url = community.links['collections']['href'] # Perform the actual request. By now, our URL and parameter should be properly set - r_json = self.fetch_resource(url, params=params) + r_json = self.fetch_resource(url, params=request_params) + if r_json is None: + # see get_communities: an error is reported as None, not as an + # empty result or a TypeError. + _logger.error(f'Failed to fetch collections [{url}]') + return None # Empty list collections = [] if '_embedded' in r_json: @@ -1066,15 +1079,13 @@ def get_item(self, uuid: str) -> Optional[Item]: @return: the raw API response """ url = f'{self.API_ENDPOINT}/core/items' - try: - UUID(uuid) - url = f'{url}/{uuid}' - r = self.api_get(url, None, None) - r_json = parse_json(response=r) - return Item(r_json) - except ValueError: + if not _is_valid_uuid(uuid): _logger.error(f'Invalid item UUID: {uuid}') return None + url = f'{url}/{uuid}' + r = self.api_get(url, None, None) + r_json = parse_json(response=r) + return Item(r_json) def get_item_by_handle(self, handle) -> Optional[Item]: """ @@ -1126,10 +1137,13 @@ def get_owningCollection(self, item_uuid: str) -> Optional[Collection]: """ Get owningCollection """ + if not _is_valid_uuid(item_uuid): + _logger.error(f'Invalid item UUID: {item_uuid}') + return None 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) + self.verify_response(r, f"item:{item_uuid}", True) r_json = parse_json(response=r) return Collection(r_json) except ValueError: @@ -1186,7 +1200,7 @@ def add_metadata(self, dso, field, value, language=None, authority=None, if dso is None or field is None or value 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 + return None dso_type = type(dso) @@ -1217,14 +1231,15 @@ def remove_metadata(self, dso, field, place=None): """ if dso is None or field is None or not isinstance(dso, DSpaceObject): _logger.error('Invalid or missing DSpace object, field or value string') - return self + return None dso_type = type(dso) path = f'/metadata/{field}' if place is None else f'/metadata/{field}/{place}' url = dso.links['self']['href'] - r = self.api_patch(url=url, operation=self.PatchOperation.REMOVE, path=path, value=None) + 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) -> User: @@ -1319,10 +1334,9 @@ def add_member(self, group, eperson) -> bool: if r.status_code == 204: return True _logger.error(f"Failed to add user {eperson.uuid} to group {group.uuid}. " - f"Status code: {r.status_code}") + f"Status code: {r.status_code}") return False - def start_workflow(self, workspace_item) -> None: url = f'{self.API_ENDPOINT}/workflow/workflowitems' res = parse_json(self.api_post_uri(url, params=None, uri_list=workspace_item)) @@ -1367,6 +1381,10 @@ def get_short_lived_token(self) -> Optional[str]: def solr_query(self, query, filters=None, fields=None, start: int = 0, rows: int = 999999999): + if self.solr is None: + raise RuntimeError( + 'Solr support requires the optional dependency: ' + 'pip install "dspace-rest-client[solr]"') if fields is None: fields = [] if filters is None: @@ -1399,10 +1417,13 @@ def get_bundle_by_name(self, name, item_uuid: str) -> Optional[Bundle]: Get a bundle by name for a specific item @param name: Name of the bundle @param item_uuid: UUID of the item - @return: Bundle object + @return: Bundle object, or None if not found / on error """ url = f'{self.API_ENDPOINT}/core/items/{item_uuid}/bundles' r_json = self.fetch_resource(url, params=None) + if r_json is None: + _logger.error(f'Failed to fetch bundles [{url}]') + return None if '_embedded' in r_json: if 'bundles' in r_json['_embedded']: for bundle in r_json['_embedded']['bundles']: @@ -1439,7 +1460,6 @@ def create_resource_policy(self, resource_uuid: str, data, group_uuid=None, return True return False - def update_resource_policy_group(self, policy_id, group_uuid: str) -> requests.Response: """ Update a resource policy with a new group @@ -1460,7 +1480,7 @@ def get_clarinlruallowances(self) -> Optional[list]: allowances = data.get('_embedded', {}).get('clarinlruallowances') if allowances: return allowances - except Exception as e: + except Exception as e: # pylint: disable=broad-exception-caught _logger.error(f"Error fetching CLARIN LRU allowances [{url}]: {e}") return None @@ -1477,11 +1497,10 @@ def get_clarinlruallowances_by_bitstream_and_user( allowances = data.get('_embedded', {}).get('clarinlruallowances') if allowances: return allowances - except Exception as e: + except Exception as e: # pylint: disable=broad-exception-caught _logger.error(f"Error fetching user allowances: {e}") return None - def create_clarinlruallowances(self, bitstream_uuid: str, metadata_payload=None) -> bool: """ Create clarinlruallowances for a bitstream for the logged-in user by @@ -1501,11 +1520,10 @@ def create_clarinlruallowances(self, bitstream_uuid: str, metadata_payload=None) response = self.api_post(url, json=metadata_payload, params=params) if response.status_code == 200: return True - except Exception as e: + except Exception as e: # pylint: disable=broad-exception-caught _logger.error(f"Error managing user metadata: {e}") return False - def get_user_by_email(self, email: str) -> Optional[User]: """ Retrieve user details using their email address. @@ -1516,6 +1534,6 @@ def get_user_by_email(self, email: str) -> Optional[User]: response = self.api_get(url, params=params) user_data = parse_json(response) return User(user_data) - except Exception as e: + except Exception as e: # pylint: disable=broad-exception-caught _logger.error(f"Error retrieving user by email {email}: {e}") return None diff --git a/dspace_rest_client/models.py b/dspace_rest_client/models.py index b0db7f2..cbe919b 100644 --- a/dspace_rest_client/models.py +++ b/dspace_rest_client/models.py @@ -11,60 +11,111 @@ """ from __future__ import annotations +from collections.abc import Callable +from copy import deepcopy import json from typing import Any -__all__ = ['DSpaceObject', 'HALResource', 'ExternalDataObject', 'SimpleDSpaceObject', 'Community', - 'Collection', 'Item', 'Bundle', 'Bitstream', 'User', 'Group', 'ResourcePolicy'] +__all__ = [ + 'HALResource', 'AddressableHALResource', 'ExternalDataObject', 'DSpaceObject', + 'SimpleDSpaceObject', 'Item', 'Community', 'Collection', 'Bundle', 'Bitstream', + 'Group', 'User', 'InProgressSubmission', 'WorkspaceItem', 'EntityType', + 'RelationshipType', 'License', 'Label', 'ResourcePolicy', +] + + +def _fresh(default: Any) -> Any: + """ + Return a per-instance copy of a declared default. + + Mutable defaults must never be shared between instances - that is exactly + the class-attribute bug these constructors are structured to make impossible. + """ + return deepcopy(default) if isinstance(default, (dict, list, set)) else default + + +def _shallow(value: Any) -> Any: + """Shallow copy of a value taken from an API resource.""" + return value.copy() class HALResource: """ Base class to represent HAL+JSON API resources + + Attribute *types* are declared in the class body as bare annotations and the + *values* are assigned per instance in __init__, mostly through the shared + _init_fields() helper. Nothing is given a class-level value: that would be a + single object shared by every instance, so a mutable one (links, embedded, + metadata, checkSum, sections) leaks mutations between them. """ - links = {} - type = None + type: str | None + links: dict[str, Any] + embedded: dict[str, Any] def __init__(self, api_resource: dict[str, Any] | None = None) -> 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: - self.links = api_resource['_links'].copy() - else: - self.links = {'self': {'href': None}} - if '_embedded' in api_resource: - self.embedded = api_resource['_embedded'].copy() + self._from_d: dict[str, Any] | None = api_resource + self._init_fields(api_resource, type=None) + # _links / _embedded are HAL envelope keys rather than plain fields, and + # a resource that carries no _links still gets the self-href placeholder. + if api_resource is None: + self.links = {} + self.embedded = {} + else: + self.links = (deepcopy(api_resource['_links']) if '_links' in api_resource + else {'self': {'href': None}}) + self.embedded = (deepcopy(api_resource['_embedded']) + if '_embedded' in api_resource else {}) + + def _init_fields(self, api_resource: dict[str, Any] | None, + copy: Callable[[Any], Any] | None = None, /, + **defaults: Any) -> None: + """ + Assign each named attribute from `api_resource`, falling back to its default. + + Replaces the `self.x = ` + `if 'x' in api_resource: ...` pair + every model used to repeat per field: each field is now named once, + beside its default. + + @param api_resource: resource to read from; None or {} means "all defaults" + @param copy: copier for values taken from the resource; positional-only, + so it can never collide with a field name + @param defaults: field name -> default value + """ + resource = api_resource or {} + for attr, default in defaults.items(): + if attr in resource: + value = resource[attr] + setattr(self, attr, copy(value) if copy is not None else value) else: - self.embedded = {} + setattr(self, attr, _fresh(default)) + class AddressableHALResource(HALResource): - id = None + id: Any + def __init__(self, api_resource: dict[str, Any] | None = None) -> None: super().__init__(api_resource) - if api_resource is not None: - if 'id' in api_resource: - self.id = api_resource['id'] + self._init_fields(api_resource, id=None) def as_dict(self) -> dict[str, Any]: return {'id': self.id} + class ExternalDataObject(HALResource): """ Generic External Data Object as configured in DSpace's external data providers framework """ - id = None - display = None - value = None - externalSource = None - metadata = {} + id: Any + display: Any + value: Any + externalSource: Any + metadata: dict[str, Any] def __init__(self, api_resource: dict[str, Any] | None = None) -> None: """ @@ -72,20 +123,9 @@ def __init__(self, api_resource: dict[str, Any] | None = None) -> None: @param api_resource: optional API resource (JSON) from a GET response or successful POST can populate instance """ super().__init__(api_resource) - - self.metadata = {} - - if api_resource is not None: - if 'id' in api_resource: - self.id = api_resource['id'] - if 'display' in api_resource: - self.display = api_resource['display'] - if 'value' in api_resource: - self.value = api_resource['value'] - if 'externalSource' in api_resource: - self.externalSource = api_resource['externalSource'] - if 'metadata' in api_resource: - self.metadata = api_resource['metadata'].copy() + self._init_fields(api_resource, id=None, display=None, value=None, + externalSource=None) + self._init_fields(api_resource, deepcopy, metadata={}) def get_metadata_values(self, field: str) -> list: """ @@ -106,13 +146,13 @@ class DSpaceObject(HALResource): operations are included in the dict returned by asDict(). Implements toJSON() as well. This class can be used on its own but is generally expected to be extended by other types: Item, Bitstream, etc. """ - uuid = None - name = None - handle = None - metadata = {} - lastModified = None - type = None - parent = None + id: Any + uuid: str | None + name: str | None + handle: str | None + lastModified: Any + parent: Any + metadata: dict[str, Any] def __init__( self, @@ -124,29 +164,18 @@ def __init__( @param api_resource: optional API resource (JSON) from a GET response or successful POST can populate instance """ super().__init__(api_resource) - self.type = None - self.metadata = {} - if dso is not None: + # copying another DSO: its as_dict() becomes the resource, and its + # HAL links come across directly (as_dict() does not carry _links) api_resource = dso.as_dict() - self.links = dso.links.copy() - if api_resource is not None: - if 'id' in api_resource: - self.id = api_resource['id'] - if 'uuid' in api_resource: - self.uuid = api_resource['uuid'] - if 'type' in api_resource: - self.type = api_resource['type'] - if 'name' in api_resource: - self.name = api_resource['name'] - if 'handle' in api_resource: - self.handle = api_resource['handle'] - if 'metadata' in api_resource: - self.metadata = api_resource['metadata'].copy() - # Python interprets _ prefix as private so for now, renaming this and handling it separately - # alternatively - each item could implement getters, or a public method to return links - if '_links' in api_resource: - self.links = api_resource['_links'].copy() + self.links = deepcopy(dso.links) + # lastModified and parent are local-only: as_dict() emits lastModified, + # but no constructor has ever read either one back off an API resource. + self.lastModified = None + self.parent = None + self._init_fields(api_resource, id=None, uuid=None, type=None, name=None, + handle=None) + self._init_fields(api_resource, deepcopy, metadata={}) @property def resourcePolicies(self) -> Any: @@ -239,11 +268,9 @@ class Item(SimpleDSpaceObject): """ Extends DSpaceObject to implement specific attributes and functions for items """ - type = 'item' - inArchive = False - discoverable = False - withdrawn = False - metadata = {} + inArchive: bool + discoverable: bool + withdrawn: bool def __init__( self, @@ -260,11 +287,16 @@ def __init__( else: super().__init__(api_resource) + self._init_fields(api_resource, discoverable=False, withdrawn=False) + # inArchive is the one field whose default depends on *how* the Item was + # built: an API resource that omits it describes an archived item, while + # a bare Item() is not archived yet. Item is also the only subclass that + # stamps `type` solely when built from a resource - DSpaceObject.__init__ + # has already assigned self.type, so a bare Item() keeps type None. + self.inArchive = False if api_resource is not None: self.type = 'item' - self.inArchive = api_resource['inArchive'] if 'inArchive' in api_resource else True - self.discoverable = api_resource['discoverable'] if 'discoverable' in api_resource else False - self.withdrawn = api_resource['withdrawn'] if 'withdrawn' in api_resource else False + self.inArchive = api_resource.get('inArchive', True) def get_metadata_values(self, field: str) -> list: """ @@ -283,15 +315,15 @@ def as_dict(self) -> dict[str, Any]: @return: dict of Item for API use """ dso_dict = super().as_dict() - item_dict = {'inArchive': self.inArchive, 'discoverable': self.discoverable, 'withdrawn': self.withdrawn} + item_dict = {'inArchive': self.inArchive, + 'discoverable': self.discoverable, 'withdrawn': self.withdrawn} return {**dso_dict, **item_dict} @classmethod def from_dso(cls, dso: DSpaceObject) -> Item: # Create new Item and copy everything over from this dso item = cls() - for key, value in dso.__dict__.items(): - item.__dict__[key] = value + item.__dict__.update(deepcopy(dso.__dict__)) return item @@ -299,7 +331,6 @@ class Community(SimpleDSpaceObject): """ Extends DSpaceObject to implement specific attributes and functions for communities """ - type = 'community' def __init__(self, api_resource: dict[str, Any] | None = None) -> None: """ @@ -309,22 +340,11 @@ def __init__(self, api_resource: dict[str, Any] | None = None) -> None: super().__init__(api_resource) self.type = 'community' - def as_dict(self) -> dict[str, Any]: - """ - 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().as_dict() - # TODO: More community-specific stuff - community_dict = {} - return {**dso_dict, **community_dict} - class Collection(SimpleDSpaceObject): """ Extends DSpaceObject to implement specific attributes and functions for collections """ - type = 'collection' def __init__(self, api_resource: dict[str, Any] | None = None) -> None: """ @@ -334,21 +354,11 @@ def __init__(self, api_resource: dict[str, Any] | None = None) -> None: super().__init__(api_resource) self.type = 'collection' - def as_dict(self) -> dict[str, Any]: - """ - 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} - class Bundle(DSpaceObject): """ Extends DSpaceObject to implement specific attributes and functions for bundles """ - type = 'bundle' def __init__(self, api_resource: dict[str, Any] | None = None) -> None: """ @@ -358,29 +368,16 @@ def __init__(self, api_resource: dict[str, Any] | None = None) -> None: super().__init__(api_resource) self.type = 'bundle' - def as_dict(self) -> dict[str, Any]: - """ - 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().as_dict() - bundle_dict = {} - return {**dso_dict, **bundle_dict} - class Bitstream(DSpaceObject): """ Extends DSpaceObject to implement specific attributes and functions for bundles """ - type = 'bitstream' # Bitstream has a few extra fields specific to file storage - bundleName = None - sizeBytes = None - checkSum = { - 'checkSumAlgorithm': 'MD5', - 'value': None - } - sequenceId = None + bundleName: str | None + sizeBytes: int | None + checkSum: dict[str, Any] + sequenceId: int | None def __init__(self, api_resource: dict[str, Any] | None = None) -> None: """ @@ -389,17 +386,10 @@ def __init__(self, api_resource: dict[str, Any] | None = None) -> None: """ 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: - self.sizeBytes = api_resource['sizeBytes'] - if 'checkSum' in api_resource: - self.checkSum = api_resource['checkSum'] - if 'sequenceId' in api_resource: - self.sequenceId = api_resource['sequenceId'] + self._init_fields(api_resource, bundleName=None, sizeBytes=None, + sequenceId=None) + self._init_fields(api_resource, _shallow, + checkSum={'checkSumAlgorithm': 'MD5', 'value': None}) def as_dict(self) -> dict[str, Any]: """ @@ -416,9 +406,7 @@ class Group(DSpaceObject): """ Extends DSpaceObject to implement specific attributes and methods for groups (aka. EPersonGroups) """ - type = 'group' - name = None - permanent = False + permanent: bool def __init__(self, api_resource: dict[str, Any] | None = None) -> None: """ @@ -427,10 +415,7 @@ def __init__(self, api_resource: dict[str, Any] | None = None) -> None: """ super().__init__(api_resource) self.type = 'group' - if 'name' in api_resource: - self.name = api_resource['name'] - if 'permanent' in api_resource: - self.permanent = api_resource['permanent'] + self._init_fields(api_resource, name=None, permanent=False) def as_dict(self) -> dict[str, Any]: """ @@ -446,14 +431,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 - selfRegistered = False + netid: str | None + lastActive: Any + canLogIn: bool + email: str | None + requireCertificate: bool + selfRegistered: bool def __init__(self, api_resource: dict[str, Any] | None = None) -> None: """ @@ -462,20 +445,9 @@ def __init__(self, api_resource: dict[str, Any] | None = None) -> None: """ super().__init__(api_resource) self.type = 'user' - if 'name' in api_resource: - self.name = api_resource['name'] - if 'netid' in api_resource: - self.netid = api_resource['netid'] - if 'lastActive' in api_resource: - self.lastActive = api_resource['lastActive'] - if 'canLogIn' in api_resource: - self.canLogIn = api_resource['canLogIn'] - if 'email' in api_resource: - self.email = api_resource['email'] - if 'requireCertificate' in api_resource: - self.requireCertificate = api_resource['requireCertificate'] - if 'selfRegistered' in api_resource: - self.selfRegistered = api_resource['selfRegistered'] + self._init_fields(api_resource, name=None, netid=None, lastActive=None, + canLogIn=False, email=None, requireCertificate=False, + selfRegistered=False) def as_dict(self) -> dict[str, Any]: """ @@ -488,22 +460,16 @@ def as_dict(self) -> dict[str, Any]: 'selfRegistered': self.selfRegistered} return {**dso_dict, **user_dict} + class InProgressSubmission(AddressableHALResource): - lastModified = None - step = None - sections = {} - type = None + lastModified: Any + step: Any + sections: dict[str, Any] - def __init__(self, api_resource: dict[str, Any]) -> None: + def __init__(self, api_resource: dict[str, Any] | None = None) -> None: super().__init__(api_resource) - if 'lastModified' in api_resource: - self.lastModified = api_resource['lastModified'] - if 'step' in api_resource: - self.step = api_resource['step'] - if 'sections' in api_resource: - self.sections = api_resource['sections'].copy() - if 'type' in api_resource: - self.type = api_resource['type'] + self._init_fields(api_resource, lastModified=None, step=None, type=None) + self._init_fields(api_resource, _shallow, sections={}) def as_dict(self) -> dict[str, Any]: parent_dict = super().as_dict() @@ -515,41 +481,52 @@ def as_dict(self) -> dict[str, Any]: } return {**parent_dict, **submission_dict} + class WorkspaceItem(InProgressSubmission): pass + class EntityType(AddressableHALResource): """ Extends Addressable HAL Resource to model an entity type (aka item type) used in entities and relationships. For example, Publication, Person, Project and Journal are all common entity types used in DSpace 7+ """ + + label: Any + def __init__(self, api_resource: dict[str, Any]) -> None: super().__init__(api_resource) - if 'label' in api_resource: - self.label = api_resource['label'] - if 'type' in api_resource: - self.type = api_resource['type'] + self._init_fields(api_resource, label=None, type=None) + class RelationshipType(AddressableHALResource): """ TODO: RelationshipType """ + def __init__(self, api_resource: dict[str, Any]) -> None: super().__init__(api_resource) + class License(AddressableHALResource): """ Specific attributes and functions for licenses """ + name: str | None + definition: str | None + confirmation: int + requiredInfo: str | None + licenseLabel: Label | None + extendedLicenseLabel: list[Label] + bitstream: Any + def __init__(self, api_resource: dict[str, Any] | None = None) -> None: super().__init__(api_resource) api_resource = api_resource or {} self.type = 'clarinlicense' - self.name = api_resource.get('name') - self.definition = api_resource.get('definition') - self.confirmation = api_resource.get('confirmation', 0) - self.requiredInfo = api_resource.get('requiredInfo') + self._init_fields(api_resource, name=None, definition=None, + confirmation=0, requiredInfo=None) license_label_value = api_resource.get('clarinLicenseLabel') self.licenseLabel = Label(license_label_value) if license_label_value else None self.extendedLicenseLabel = [Label(label) for label in @@ -571,18 +548,20 @@ class Label(AddressableHALResource): """ Specific attributes and functions for licenses """ + label: str | None + title: str | None + icon: str | None + extended: bool + def __init__(self, api_resource: dict[str, Any] | None = None) -> None: """ Default constructor. Call DSpaceObject init then set label-specific attributes @param api_resource: API result object to use as initial data """ super().__init__(api_resource) - api_resource = api_resource or {} self.type = 'clarinlicenselabel' - self.label = api_resource.get('label') - self.title = api_resource.get('title') - self.icon = api_resource.get('icon') - self.extended = api_resource.get('extended', False) + self._init_fields(api_resource, label=None, title=None, icon=None, + extended=False) def to_dict(self) -> dict[str, Any]: return { @@ -598,19 +577,23 @@ class ResourcePolicy(AddressableHALResource): """ DQ specific. Extends Addressable HAL Resource to model a resource policy. """ + name: str | None + description: str | None + startDate: str | None + endDate: str | None + action: str | None + policyType: str | None + groupName: str | None + groupUUID: str | None + def __init__(self, api_resource: dict[str, Any]) -> None: super().__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') - # Check for direct groupName/groupUUID (cached format from as_dict()) - self.groupName = api_resource.get('groupName') - self.groupUUID = api_resource.get('groupUUID') + # groupName / groupUUID come straight off a cached as_dict(); the live + # API instead nests the group under _embedded (handled below). + self._init_fields(api_resource, name=None, description=None, + startDate=None, endDate=None, type=None, action=None, + policyType=None, groupName=None, groupUUID=None) # 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']: diff --git a/publish.sh b/publish.sh index 7462814..20f3ba1 100755 --- a/publish.sh +++ b/publish.sh @@ -1,3 +1,7 @@ #!/bin/bash -python setup.py bdist_wheel -twine upload dist/dspace_rest_client-0.1.10-py3-none-any.whl \ No newline at end of file +set -euo pipefail + +rm -rf build dist +python -m build +python -m twine check dist/* +python -m twine upload dist/* \ No newline at end of file diff --git a/pyproject.toml b/pyproject.toml index 0459c25..bbae828 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,14 +1,14 @@ [build-system] -requires = ["setuptools>=61.0"] +requires = ["setuptools>=77.0"] build-backend = "setuptools.build_meta" [project] name = "dspace-rest-client" -version = "0.1.10" +version = "0.2.0" description = "A DSpace 7 REST API client library" readme = "README.md" requires-python = ">=3.10" -license = { text = "BSD-3-Clause" } +license = "BSD-3-Clause" authors = [ { name = "Kim Shepherd" }, ] @@ -16,7 +16,6 @@ classifiers = [ "Programming Language :: Python :: 3.10", "Programming Language :: Python :: 3.11", "Programming Language :: Python :: 3.12", - "License :: OSI Approved :: BSD License", "Operating System :: OS Independent", ] dependencies = [ @@ -32,6 +31,10 @@ dev = [ "pylint", "mypy", ] +release = [ + "build>=1.2", + "twine>=5.1", +] [project.urls] Documentation = "https://github.com/the-library-code/dspace-rest-python/blob/main/README.md" @@ -51,7 +54,9 @@ target-version = "py310" [tool.ruff.lint] # E = pycodestyle, F = pyflakes, T20 = flake8-print (no print()) select = ["E", "F", "T20"] -ignore = ["F403", "F405", "E501", "E402", "F841", "E741"] +# E501 only: the upstream sources still carry long docstring/log lines that a +# reflow would churn for no benefit. Everything else is enforced. +ignore = ["E501"] [tool.ruff.lint.per-file-ignores] "tests/**" = ["T20"] @@ -65,9 +70,6 @@ py-version = "3.10" jobs = 0 [tool.pylint.basic] -# DSpace REST fields are camelCase (sizeBytes, checkSum, ...) mirrored verbatim -# as attributes; `id` is a valid short name here. -good-names = ["i", "j", "k", "ex", "Run", "_", "id", "d", "f"] no-docstring-rgx = "^_" [tool.pylint.design] @@ -90,13 +92,16 @@ allow-wildcard-with-all = false logging-format-style = "old" [tool.pylint."messages control"] -# camelCase attrs mirror the DSpace API (invalid-name); f-strings in log calls -# are mandated by the project (logging-fstring-interpolation); broad catches -# guard optional/fallback paths (broad-exception-caught). import-error: pysolr -# is an optional extra not present in every lint env. too-many-lines / -# too-many-public-methods: DSpaceClient is a wide REST facade (one method per -# endpoint); splitting into endpoint mixins is a tracked follow-up, not a -# blocker. fixme: upstream TODO markers are informational, not defects. +# camelCase attrs and method names mirror the DSpace API verbatim +# (invalid-name), so it stays off project-wide and no good-names allowlist is +# needed; f-strings in log calls are mandated by the project +# (logging-fstring-interpolation). import-error: pysolr is an optional extra +# not present in every lint env. too-many-lines / too-many-public-methods: +# DSpaceClient is a wide REST facade (one method per endpoint); splitting into +# endpoint mixins is a tracked follow-up, not a blocker. fixme: upstream TODO +# markers are informational, not defects. Broad `except Exception` catches are +# NOT disabled globally - the few deliberate fallback paths carry an inline +# `# pylint: disable=broad-exception-caught`. disable = [ "missing-function-docstring", "missing-class-docstring", @@ -104,7 +109,6 @@ disable = [ "invalid-name", "import-error", "import-outside-toplevel", - "broad-exception-caught", "logging-fstring-interpolation", "line-too-long", "too-few-public-methods", diff --git a/tests/test_client_read.py b/tests/test_client_read.py index f48b04a..e7a969a 100644 --- a/tests/test_client_read.py +++ b/tests/test_client_read.py @@ -93,10 +93,70 @@ def test_returns_typed_item(self): 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")) + for value in ("not-a-uuid", None): + with self.subTest(value=value): + self.assertIsNone(c.get_item(value)) self.assertEqual(m.call_count, 0) +class TestUuidValidation(unittest.TestCase): + + def test_uuid_endpoints_reject_none_without_request(self): + c = make_client() + cases = ( + ("get_dso", lambda: c.get_dso(f"{API}/core/items", None)), + ("get_resourcepolicy", lambda: c.get_resourcepolicy(None)), + ("create_resourcepolicy_resource", lambda: c.create_resourcepolicy( + resource_uuid=None, group_uuid=BITSTREAM_UUID)), + ("create_resourcepolicy_group", lambda: c.create_resourcepolicy( + resource_uuid=BITSTREAM_UUID, group_uuid=None)), + ("get_owningCollection", lambda: c.get_owningCollection(None)), + ) + with requests_mock.Mocker() as m: + for name, call in cases: + with self.subTest(name=name): + self.assertIsNone(call()) + self.assertEqual(m.call_count, 0) + + def test_uuid_endpoints_reject_non_string_values_without_request(self): + c = make_client() + cases = ( + ("get_item", lambda v: c.get_item(v)), + ("get_dso", lambda v: c.get_dso(f"{API}/core/items", v)), + ("get_resourcepolicy", lambda v: c.get_resourcepolicy(v)), + ("get_owningCollection", lambda v: c.get_owningCollection(v)), + ) + with requests_mock.Mocker() as m: + for name, call in cases: + for value in (None, 42, object(), ["not", "a", "uuid"]): + with self.subTest(name=name, value=type(value).__name__): + # bare UUID(value) raises TypeError (not ValueError) for + # these, which used to escape the caller's except clause + self.assertIsNone(call(value)) + self.assertEqual(m.call_count, 0) + + +class TestListEndpointsOnFailedResponses(unittest.TestCase): + """A non-JSON / error body must not become a TypeError on ``'_embedded' in None``.""" + + def test_list_endpoints_return_none_on_non_json_body(self): + c = make_client() + cases = ( + ("get_communities", f"{API}/core/communities", + lambda: c.get_communities()), + ("get_collections", f"{API}/core/collections", + lambda: c.get_collections()), + ("get_bundle_by_name", f"{API}/core/items/{ITEM_UUID}/bundles", + lambda: c.get_bundle_by_name("ORIGINAL", ITEM_UUID)), + ) + for name, url, call in cases: + for status, body in ((404, "Not Found"), (200, "nope")): + with self.subTest(name=name, status=status), \ + requests_mock.Mocker() as m: + m.get(url, status_code=status, text=body) + self.assertIsNone(call()) + + class TestGetBundles(unittest.TestCase): def test_by_parent_item_lists_bundles(self): diff --git a/tests/test_client_write.py b/tests/test_client_write.py index 9e4815c..8403c87 100644 --- a/tests/test_client_write.py +++ b/tests/test_client_write.py @@ -84,6 +84,41 @@ def test_404_surfaces_as_response(self): self.assertEqual(c.api_delete(url, params=None).status_code, 404) +class TestNonJsonForbiddenResponses(unittest.TestCase): + + def test_low_level_writes_return_non_json_403_without_crashing(self): + c = make_client() + url = f"{API}/forbidden" + cases = ( + ("post", "POST", lambda: c.api_post( + url, params=None, json={}, retry=True)), + ("post_uri", "POST", lambda: c.api_post_uri( + url, params=None, uri_list="http://example.test/resource", retry=True)), + ("put", "PUT", lambda: c.api_put( + url, params=None, json={}, retry=True)), + ("put_uri", "PUT", lambda: c.api_put_uri( + url, params=None, uri_list="http://example.test/resource", retry=True)), + ("delete", "DELETE", lambda: c.api_delete( + url, params=None, retry=True)), + ("patch", "PATCH", lambda: c.api_patch( + url, c.PatchOperation.REMOVE, "/metadata/dc.title", None, + retry=True)), + ) + for name, method, call in cases: + with self.subTest(name=name), requests_mock.Mocker() as m: + m.register_uri(method, url, status_code=403, text="Forbidden") + self.assertEqual(call().status_code, 403) + self.assertEqual(m.call_count, 1) + + +class TestMetadataPatches(unittest.TestCase): + + def test_invalid_add_and_remove_return_none(self): + c = make_client() + self.assertIsNone(c.add_metadata(None, "dc.title", "Title")) + self.assertIsNone(c.remove_metadata(None, "dc.title")) + + class TestCreateBundle(unittest.TestCase): def test_posts_to_item_bundles_and_returns_bundle(self): @@ -192,6 +227,44 @@ def test_server_error_returns_none(self): bundle=bundle, name="a.pdf", path=self.path, mime="application/pdf")) + def test_non_json_403_reauthenticates_and_retries(self): + c = make_client() + bundle = Bundle(bundle_json("bnd")) + upload_url = f"{API}/core/bundles/bnd/bitstreams" + with requests_mock.Mocker() as m: + m.post(upload_url, [ + {"status_code": 403, "text": "Forbidden"}, + {"status_code": 201, + "json": bitstream_json("retried", "a.pdf", size=20)}, + ]) + m.post(f"{API}/authn/login", status_code=200, + headers={"Authorization": "Bearer refreshed"}) + m.get(f"{API}/authn/status", status_code=200, + json={"authenticated": True}) + + bitstream = c.create_bitstream( + bundle=bundle, name="a.pdf", path=self.path, + mime="application/pdf") + + self.assertEqual(bitstream.uuid, "retried") + upload_calls = [request for request in m.request_history + if request.url.split("?")[0] == upload_url] + self.assertEqual(len(upload_calls), 2) + + def test_xsrf_token_from_upload_response_updates_session(self): + # create_bitstream must refresh the CSRF token through the one shared + # update_token() path, exactly like every other write method. + 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, + headers={"DSPACE-XSRF-TOKEN": "fresh-token"}, + json=bitstream_json("bsnew", "a.pdf", size=20)) + c.create_bitstream(bundle=bundle, name="a.pdf", path=self.path, + mime="application/pdf") + self.assertEqual(c.session.headers["X-XSRF-Token"], "fresh-token") + self.assertEqual(c.session.cookies["X-XSRF-Token"], "fresh-token") + class TestCreateClarinAllowances(unittest.TestCase): diff --git a/tests/test_models.py b/tests/test_models.py index 8167548..24f1f1f 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -10,8 +10,10 @@ import unittest import _helpers # noqa: F401 (bootstraps sys.path for direct runs) +from dspace_rest_client import models as models_module from dspace_rest_client.models import ( - Item, Community, Collection, Bundle, Bitstream, ResourcePolicy) + DSpaceObject, HALResource, Item, Community, Collection, Bundle, Bitstream, + Group, User, InProgressSubmission, ResourcePolicy) class TestItem(unittest.TestCase): @@ -44,6 +46,112 @@ def test_as_dict_carries_item_flags(self): (d["inArchive"], d["discoverable"], d["withdrawn"]), (True, True, False)) + def test_from_dso_does_not_alias_metadata(self): + source = DSpaceObject({ + "metadata": {"dc.title": [{"value": "Original"}]}}) + + copied = Item.from_dso(source) + copied.metadata["dc.title"][0]["value"] = "Changed" + + self.assertEqual(source.metadata["dc.title"][0]["value"], "Original") + + def test_dso_constructor_does_not_alias_metadata(self): + source = DSpaceObject({ + "metadata": {"dc.title": [{"value": "Original"}]}}) + + copied = Item(dso=source) + copied.metadata["dc.title"][0]["value"] = "Changed" + + self.assertEqual(source.metadata["dc.title"][0]["value"], "Original") + + +class TestMutableDefaults(unittest.TestCase): + + def test_hal_links_are_isolated_between_instances(self): + first = HALResource() + second = HALResource() + + first.links["next"] = {"href": "http://example.test/next"} + + self.assertNotIn("next", second.links) + + def test_bitstream_checksums_are_isolated_between_instances(self): + first = Bitstream() + second = Bitstream() + + first.checkSum["value"] = "changed" + + self.assertIsNone(second.checkSum["value"]) + + def test_submission_sections_are_isolated_between_instances(self): + first = InProgressSubmission({}) + second = InProgressSubmission({}) + + first.sections["license"] = {"accepted": True} + + self.assertNotIn("license", second.sections) + + def test_no_model_declares_attribute_defaults_at_class_level(self): + """The structural guarantee behind the three tests above. + + A class-level default is shared by every instance, so a mutable one + (links, metadata, checkSum, sections) lets one object's mutation leak + into all the others. Class bodies carry only bare annotations - which + declare a type without creating an attribute - while the values are + assigned per instance in __init__, mostly via _init_fields(). This test + fails if anyone reintroduces a class-body default. + + Note there is deliberately no class-level field-spec constant to exempt + here: the spec lives in the _init_fields() keyword arguments, so this + check can stay absolute rather than carrying an allowlist. + """ + for cls in vars(models_module).values(): + if not isinstance(cls, type) or cls.__module__ != models_module.__name__: + continue + declared = sorted( + name for name, value in vars(cls).items() + if not name.startswith("__") + and not isinstance(value, (property, classmethod, staticmethod)) + and not callable(value)) + with self.subTest(model=cls.__name__): + self.assertEqual(declared, [], ( + f"{cls.__name__} declares {declared} at class level; " + "move the default into __init__ as self. = ...")) + + def test_declared_defaults_are_copied_per_instance(self): + # _fresh() is what stops a default declared once in an _init_fields() + # call from being handed out as the same object to every instance. + first, second = Bitstream(), Bitstream() + self.assertIsNot(first.checkSum, second.checkSum) + + first_sub, second_sub = InProgressSubmission(), InProgressSubmission() + self.assertIsNot(first_sub.sections, second_sub.sections) + + def test_api_values_keep_their_documented_copy_depth(self): + # checkSum/sections were always shallow-copied and metadata deep-copied; + # _init_fields carries that per-field choice rather than flattening it. + checksum = {"checkSumAlgorithm": "MD5", "value": "abc"} + bitstream = Bitstream({"checkSum": checksum}) + self.assertIsNot(bitstream.checkSum, checksum) + self.assertEqual(bitstream.checkSum, checksum) + + metadata = {"dc.title": [{"value": "T"}]} + item = Item({"metadata": metadata}) + item.metadata["dc.title"][0]["value"] = "changed" + self.assertEqual(metadata["dc.title"][0]["value"], "T") + + def test_every_model_instance_defines_the_attributes_its_dict_reports(self): + # nothing may fall back to a class attribute: what __init__ assigns is + # the whole public surface of the instance. + for cls, args in ((HALResource, ()), (DSpaceObject, ()), (Item, ()), + (Community, ()), (Collection, ()), (Bundle, ()), + (Bitstream, ()), (Group, ()), (User, ()), + (InProgressSubmission, ({},))): + with self.subTest(model=cls.__name__): + instance = cls(*args) + for name in ("type", "links", "embedded"): + self.assertIn(name, vars(instance)) + class TestCommunityCollection(unittest.TestCase): @@ -109,6 +217,10 @@ def test_bitstream_from_none_does_not_crash(self): self.assertEqual(b.type, "bitstream") self.assertIsNone(b.uuid) + def test_group_and_user_from_none_do_not_crash(self): + self.assertEqual(Group(None).type, "group") + self.assertEqual(User(None).type, "user") + class TestResourcePolicy(unittest.TestCase): diff --git a/tests/test_public_api.py b/tests/test_public_api.py new file mode 100644 index 0000000..0709a92 --- /dev/null +++ b/tests/test_public_api.py @@ -0,0 +1,18 @@ +import unittest + +import _helpers # noqa: F401 +import dspace_rest_client +from dspace_rest_client import models + + +class TestPublicApi(unittest.TestCase): + + def test_package_and_models_export_the_same_models(self): + self.assertEqual( + set(dspace_rest_client.__all__) - {"DSpaceClient"}, + set(models.__all__), + ) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file diff --git a/tests/test_solr.py b/tests/test_solr.py new file mode 100644 index 0000000..f3acabc --- /dev/null +++ b/tests/test_solr.py @@ -0,0 +1,18 @@ +import unittest + +import _helpers # noqa: F401 +from _helpers import make_client + + +class TestSolr(unittest.TestCase): + + def test_query_without_solr_extra_has_actionable_error(self): + client = make_client() + client.solr = None + + with self.assertRaisesRegex(RuntimeError, r"dspace-rest-client\[solr\]"): + client.solr_query("*:*", rows=10) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file