fix: resolve all 16 code-review findings (correctness, packaging, lint/CI) by vidiecan · Pull Request #25 · dataquest-dev/dspace-rest-python

vidiecan · 2026-09-02T14:16:56Z

Fixes all 16 findings from the code review of this library.

Base branch: this is stacked on feat/pyproject-tooling-typing (PR #21, still open against dtq), because the work builds directly on that branch's pyproject/typing migration. Targeting dtq would have replayed #21's whole diff here. GitHub will retarget this PR to dtq automatically once #21 merges.

A. Correctness — `dspace_rest_client/client.py`

UUID validation is now one idiom. _is_valid_uuid is used by get_resourcepolicy, create_resourcepolicy (both resource_uuid and group_uuid), get_dso, get_item and get_owningCollection (which previously validated nothing). The old UUID(uuid) inside except ValueError let UUID(None) escape as an uncaught TypeError; every site now logs Invalid ... UUID and returns None.
CSRF retry no longer crashes on non-JSON bodies. A new _response_message_contains() helper parses the body defensively; api_post, api_post_uri, api_put, api_put_uri, api_delete, api_patch and create_bitstream all go through it. A 403 with an HTML or empty body now takes the CSRF-refresh/retry path instead of raising TypeError/JSONDecodeError.
_embedded checks are None-guarded. get_communities, get_collections and get_bundle_by_name return None on a failed or non-JSON response instead of raising TypeError, matching their documented contracts and the get_bundles/get_bitstreams idiom.
return self copy-paste bug fixed. add_metadata and remove_metadata returned the client on invalid input; they now return None, which is what callers expect.
create_bitstream XSRF duplication removed — the inline header/cookie refresh is replaced by the shared self.update_token(r).

B. Correctness — `dspace_rest_client/models.py`

Group() / User() are constructible. Both guard api_resource = api_resource or {} like Bitstream, so Group(), User() and create_group/create_user on a None parse result no longer raise TypeError.
No shared mutable class attributes. HALResource.links/.embedded, Bitstream.checkSum and InProgressSubmission.sections are assigned per instance in __init__, so no instance can mutate class-level state and HALResource().embedded exists.
Copy paths no longer alias. Item.from_dso and DSpaceObject(dso=...) deep-copy metadata (and links/embedded), so mutating a copy's metadata[field] list no longer writes through to the source.
models.__all__ completed with the 7 missing names (AddressableHALResource, InProgressSubmission, WorkspaceItem, EntityType, RelationshipType, License, Label); a test pins __all__ parity with the package re-exports.

C. Packaging / release

publish.sh no longer runs the deleted setup.py — it clears artifacts, runs python -m build, validates with twine check and uploads dist/* (no hardcoded 0.1.10 wheel name). MAINTAINING.md points at pyproject.toml.
Version bumped off the already-published 0.1.10 with a CHANGELOG.md entry for this branch. Bumped to 0.2.0 rather than 0.1.11: requires-python moved 3.8 → 3.10, which is a breaking change for consumers, so a minor bump is the honest signal.
pysolr extra documented and enforced. README covers pip install "dspace-rest-client[solr]", and solr_query() raises an actionable RuntimeError naming the extra instead of dying with AttributeError on None.

D. Lint / CI / config hygiene

import json as _json reverted. Grepped both this repo and the consumer repo (src/, mcp/, tests/): nothing outside the library calls api_post/api_put at all, let alone with json= as a keyword. Kept the public parameter name json (zero behaviour change) and switched the module to from json import dumps, so nothing is shadowed and no inline lint suppression is needed.
Stale lint suppressions removed. Ruff's ignore list drops F403, F405, F841, E402 and E741 — only the documented E501 remains (long upstream docstring/log lines). broad-exception-caught is no longer disabled project-wide; the four deliberate fallback catches carry an inline # pylint: disable=broad-exception-caught, so a new accidental except Exception fails lint again. invalid-name stays disabled (the DSpace REST field names are camelCase) and the dead good-names allowlist it never applied to is deleted, making the config coherent.
Dead code deleted — the commented-out data.pop(...) block in update_dso.
Typecheck job folded into lint. The separate typecheck job spun up a second runner for the same environment; mypy is now a step of the lint job with step-level continue-on-error so incremental typing stays non-blocking, and the version is pinned by bumping the mirrors-mypy rev to v2.3.1 (current latest).

Tests

83 tests pass (was 68 before this branch, 80 at the start of this PR). New regression coverage for the behavioural fixes:

UUID endpoints reject None, int, object() and list arguments without issuing a request (these raised TypeError, not the ValueError the callers caught).
Every low-level write method returns a non-JSON 403 instead of crashing, and create_bitstream re-authenticates and retries on one.
get_communities/get_collections/get_bundle_by_name return None for both a 404 and a 200 with an HTML body.
Group(None) / User(None) construct; fresh instances do not share links/checkSum/sections.
Item.from_dso and Item(dso=...) do not write through to the source's metadata.
add_metadata(None, ...) / remove_metadata(None, ...) return None.
solr_query() without the extra raises a RuntimeError naming dspace-rest-client[solr].
create_bitstream refreshes the session CSRF token through update_token.

Ruff 0.12.7 and pylint 3.3.7 (the CI-pinned versions) are clean. The consumer repo's full suite (655 tests) also passes against this branch.

🤖 Generated with Claude Code

Copilot

🟡 Changes recommended

The DSpaceObject(dso=...) path currently drops embedded/backing state, and Solr initialization can now fail client construction even when Solr is meant to be optional.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR tightens correctness and resilience of the DSpace REST client (UUID validation, non-JSON error handling, mutable default fixes), while also modernizing packaging/release flow and consolidating lint/typecheck execution via pre-commit.

Changes:

Hardened client request/response handling (UUID validation, CSRF retry parsing, safer list endpoint behavior, consistent None returns on invalid inputs).
Fixed model construction and copying pitfalls (no shared mutable class attributes; deep-copy to avoid aliasing; expanded models.__all__ and added public API parity test).
Updated packaging/release + CI hygiene (pyproject version bump to 0.2.0, improved publish script/maintainer docs, lint+mypy via pre-commit in CI).

File summaries

File	Description
`dspace_rest_client/client.py`	Adds defensive helpers for UUID/message parsing, improves CSRF retry safety, fixes return semantics, and improves Solr extra behavior.
`dspace_rest_client/models.py`	Removes shared mutable class attributes, deep-copies resource structures, expands `__all__`, and makes constructors tolerant of `None`.
`tests/test_client_read.py`	Adds regression tests for UUID validation and list endpoints returning `None` on failed/non-JSON responses.
`tests/test_client_write.py`	Adds regression tests for non-JSON 403 handling, metadata patch invalid input, and bitstream retry/token refresh.
`tests/test_models.py`	Adds coverage for copy-aliasing fixes and mutable-default isolation; asserts `Group/User(None)` construction.
`tests/test_public_api.py`	Pins parity between package exports and `models.__all__`.
`tests/test_solr.py`	Verifies actionable error when Solr extra is missing.
`pyproject.toml`	Bumps version to `0.2.0`, adjusts license metadata, and tightens ruff/pylint config.
`README.md`	Updates supported Python version and documents Solr extra install.
`CHANGELOG.md`	Adds `0.2.0` entry describing the breaking/support changes and behavior fixes.
`publish.sh`	Switches release build/upload flow to `python -m build` + `twine check` + upload `dist/*`.
`MAINTAINING.md`	Updates release checklist to match `pyproject.toml`-based publishing and new tooling.
`.pre-commit-config.yaml`	Narrows hook file scopes and adds a pinned mypy hook.
`.github/workflows/tests.yml`	Runs ruff/pylint/mypy via pre-commit in a single lint job (non-blocking mypy).

Review details

Files reviewed: 14/14 changed files
Comments generated: 2
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+        if pysolr is not None:
+            self.solr = pysolr.Solr(
+                url=solr_endpoint, always_commit=True, timeout=300, auth=solr_auth)


        if dso is not None:
            api_resource = dso.as_dict()
-            self.links = dso.links.copy()
+            self.links = deepcopy(dso.links)
        if api_resource is not None:


Carry over the pending working-tree pass so the follow-up review fixes land on a committed baseline: - validate UUIDs through _is_valid_uuid in get_resourcepolicy, create_resourcepolicy, get_dso and get_item - add _response_message_contains so a non-JSON 401/403 body still routes through the CSRF-refresh/retry path instead of raising - return None (not the client) from add_metadata/remove_metadata on invalid input - give every model instance its own links/embedded/checkSum/sections and deep-copy metadata on the DSpaceObject/Item copy paths - guard Group()/User() against a None api_resource - export the full model surface from models.__all__ - move release metadata to pyproject.toml, document the [solr] extra and raise a clear RuntimeError from solr_query without it - run ruff/pylint/mypy through pinned pre-commit hooks

Remaining correctness findings from the code review: - get_owningCollection validated nothing; it now short-circuits through _is_valid_uuid like every other UUID-taking method, so a None / non-string argument is logged and returns None instead of raising TypeError. - get_communities, get_collections and get_bundle_by_name tested `'_embedded' in r_json` on a value fetch_resource/parse_json can return as None (404 or non-JSON body), raising TypeError. They now report the failure and return None, matching their documented contract. - create_bitstream carried its own copy of the DSPACE-XSRF-TOKEN header/cookie refresh; it now calls the shared update_token(), so there is one CSRF path. - Dropped the commented-out data.pop(...) block in update_dso. Tests cover the non-string UUID arguments (which used to raise TypeError, not the ValueError the callers caught), the non-JSON/404 list responses, and the token refresh going through update_token.

- Drop the stale E402/E741 ruff ignores (F403/F405/F841 already went); only E501 remains, and it is now documented. Verified clean with ruff 0.12.7. - Stop disabling broad-exception-caught project-wide. The four deliberate fallback catches in client.py carry an inline disable instead, so a new accidental `except Exception` is a lint failure again. - Remove the dead good-names allowlist: invalid-name stays disabled because the DSpace REST field names are camelCase, so the allowlist never applied. Verified with the CI-pinned pylint 3.3.7.

The separate typecheck job spun up a second runner to install the same pre-commit environment. Mypy now runs as a step of the lint job, keeping step-level continue-on-error so incremental typing stays non-blocking, and the mypy version is pinned by bumping the mirrors-mypy rev to v2.3.1.

Every model class kept its attribute defaults in the class body and relied on them as fallbacks until __init__ (or an API resource) overwrote them. That is what made the shared-mutable-dict bug possible in the first place: a class-level `links = {}` / `checkSum = {...}` / `sections = {}` is one object shared by every instance, so one object's mutation leaks into all the others. All of them are now plain instance attributes assigned in __init__ - HALResource (type, links, embedded), AddressableHALResource (id), ExternalDataObject (id, display, value, externalSource, metadata), DSpaceObject (id, uuid, name, handle, lastModified, parent, metadata, type), Item (inArchive, discoverable, withdrawn), Community / Collection / Bundle / Bitstream / Group / User (type and their own fields), InProgressSubmission (lastModified, step, sections, type) and EntityType (label). No model class has a class-level attribute left; a new test asserts that, so finding 7 cannot regress. Verified against both repositories first: nothing reads these attributes off the class (only `Item.from_dso`, a classmethod), nothing builds a model through `object.__new__`, and `to_json`/`to_json_pretty` - the only readers of a model's `__dict__` - have no callers. An exhaustive old-vs-new comparison over every class, constructor form, `dso=` copy path and `as_dict()` reports two differences, both deliberate: `DSpaceObject.id` and `EntityType.label` now default to None instead of raising AttributeError when absent. Behaviour deliberately preserved: Item still stamps `type = 'item'` only when built from an API resource. Its class-level `type = 'item'` was dead - DSpaceObject .__init__ always assigns self.type first, shadowing it on every instance.

Every model repeated the same two-step per attribute: assign a default, then `if 'x' in api_resource: self.x = api_resource['x']`. That was 36 membership blocks across the module, and the shape of it is what let a mutable default drift back to class level. Both steps now go through one shared helper on HALResource: self._init_fields(api_resource, name=None, netid=None, canLogIn=False, ...) Each field is named once, beside its default. `_init_fields` takes the value from the resource when the key is present and otherwise installs a per-instance copy of the default via `_fresh()`, so a declared mutable default can never be shared between instances. An optional positional-only `copy` argument carries the per-field copy depth the old code had - `deepcopy` for metadata, a shallow `_shallow` for checkSum and sections - instead of flattening them to one rule. Attribute *types* stay in the class body as bare annotations, which declare a type without creating a class attribute: mypy and pylint keep resolving every member, and the structural guard test stays absolute (there is deliberately no class-level spec constant needing an allowlist). Genuinely special logic stays written out: the HAL `_links`/`_embedded` envelope and its self-href placeholder, DSpaceObject's `dso=` copy path and its local-only lastModified/parent, Item's inArchive (whose default depends on whether an API resource was supplied at all), the `type` literal stamps, and ResourcePolicy's `_embedded` group fallback. Effect: 36 membership blocks -> 2 (both genuinely special), 369 -> 336 code lines. The exhaustive old-vs-new comparison - every class x {no-arg, {}, full resource, None} x every attribute, plus both `dso=` paths, `Item.from_dso`, `as_dict()` and `to_dict()` - reports exactly one difference: EntityType(None) now builds an empty instance instead of raising TypeError, matching every other model's `api_resource or {}` tolerance. EntityType is never constructed in either repository; the CHANGELOG records it alongside Group()/User().

…ed lookups - get_communities/get_collections returned None on any failed or non-JSON page. A paging caller reads an empty or None page as the last one, so a transient 5xx silently truncated the community/collection tree. They now raise RuntimeError with the HTTP status and url (the get_bundles idiom); only a single uuid lookup that 404s returns None. _last_err is cleared before the fetch, so a stale 404 cannot mask a non-JSON 200. - get_owningCollection ignored verify_response and wrapped the error body in a Collection; it returns None (last_err still records the response). - get_user_by_email turned the 204 for an unknown email into an empty User; it returns None. - create_group/create_user returned a uuid-less, truthy Group/User for a failed create; they return None when the response has no uuid.

_shallow called value.copy(), so a resource with checkSum: null raised AttributeError. Use copy.copy, which leaves None as None.

vidiecan · 2026-10-07T07:36:10Z

Rebased onto the new #21 head (dtq `a62a63e`), plus review fixes

Rebase. The branch is now on top of the rebased feat/pyproject-tooling-typing (64dbd11), which itself is on dtq a62a63e. Conflicts and how they were resolved:

get_resourcepolicy: this branch's _is_valid_uuid guard is combined with dtq fix(client): follow pagination and verify counts in list getters #29's _fetch_all_pages, so pagination and count checks are kept.
create_bitstream: kept dtq's timeout override on the send and the retry, with this branch's update_token(r) and _response_message_contains. Both tests are kept (custom timeout preserved; a non-JSON 403 re-authenticates).
ResourcePolicy: took the _init_fields refactor but kept dtq fix(models): parse resource policies granted to an eperson #27's _embedded handling. A group: null (eperson) policy no longer hits .get() on None, and a policy with no grantee still warns. The fix(models): parse resource policies granted to an eperson #27 tests pass.
pyproject.toml, CHANGELOG.md, README: Python stays >=3.8 (see build: pyproject migration, shared tooling (ruff/pylint/uv), lint cleanup & typing #21). Because of that, setuptools>=61 and license = { text = ... } were kept instead of >=77 and an SPDX string: setuptools 77 needs Python 3.9 or later. The release extra is kept.
MAINTAINING.md: took this branch's version. test_models.py imports: kept both sides.
The leftover tool Co-Authored-By trailers were removed from this branch's commit messages.

Review fixes. Each test was written first and seen failing.

Finding	Change	Test
`get_communities`/`get_collections` returned None on a failed page, and a paging consumer reads that as the last page, so it silently truncated the community/collection tree	They now raise `RuntimeError` with the HTTP status and url (the `get_bundles` idiom). Only a single-`uuid` lookup that 404s returns None. A list 404 also raises, because None there would truncate the same way. `_last_err` is cleared before the fetch so an old 404 cannot hide a non-JSON 200	`test_community_and_collection_lists_raise_on_failed_page` (500/404/HTML), `test_single_community_or_collection_404_returns_none`, `test_earlier_404_does_not_mask_a_later_non_json_response`
`Bitstream` `_shallow(checkSum)` crashed on `checkSum: None`	`_shallow` uses `copy.copy`, so None stays None	`test_bitstream_with_null_checksum`
`get_user_by_email` returned an empty User for a 204 (unknown email)	An empty body returns None	`TestGetUserByEmail`
`get_owningCollection` ignored `verify_response` and returned a `Collection` built from the error body	It returns None on failure, and `last_err` still records the response	`TestOwningCollection`
`create_group`/`create_user` returned a truthy object with no uuid after a failed create	They return None when the response has no uuid; annotated `Optional[...]`	`TestCreateGroupAndUser` (422 JSON, 500 HTML, 201)

The CHANGELOG entry for item 5 was updated to match.

Checks: pytest 123 passed on 3.8 and 3.12. The pre-commit ruff, pylint and mypy hooks pass. Offline consumer check: every DSpaceClient and model member called by dspace-rest-test/tests and by DSpace-ISstag-integration/src exists and its signature binds. Their None/raise contracts match how those callers use them. For example, repo/_audit.py already treats a None from get_owningCollection as a failure, so its 401 re-auth path now actually triggers.

Not done here: the cost of deepcopy on the metadata copy paths, the mypy hook policy, and other CHANGELOG wording nits.

vidiecan requested a lite review from Copilot September 2, 2026 14:46

Copilot started reviewing on behalf of vidiecan September 2, 2026 14:47

View session

Copilot AI reviewed Sep 2, 2026

View reviewed changes

jm added 6 commits October 7, 2026 09:25

vidiecan mentioned this pull request Oct 7, 2026

test: CLARIN consumer-contract suite + main-tailored differential CI #23

Open

jm added 3 commits October 7, 2026 09:31

fix(models): Bitstream accepts checkSum: null

a21c121

_shallow called value.copy(), so a resource with checkSum: null raised AttributeError. Use copy.copy, which leaves None as None.

docs: README states the Python 3.8+ support the package declares

77576f9

vidiecan force-pushed the feat/pyproject-tooling-typing branch from b2af3a6 to 64dbd11 Compare October 7, 2026 07:35

vidiecan force-pushed the fix/review-findings-sweep branch from a93d786 to 77576f9 Compare October 7, 2026 07:35

Navigation Menu

Repository navigation

fix: resolve all 16 code-review findings (correctness, packaging, lint/CI) - #25

fix: resolve all 16 code-review findings (correctness, packaging, lint/CI)#25
vidiecan wants to merge 9 commits into
feat/pyproject-tooling-typingfrom
fix/review-findings-sweep

vidiecan commented Sep 2, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

vidiecan commented Oct 7, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

vidiecan commented Sep 2, 2026

A. Correctness — dspace_rest_client/client.py

B. Correctness — dspace_rest_client/models.py

C. Packaging / release

D. Lint / CI / config hygiene

Tests

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

vidiecan commented Oct 7, 2026

Rebased onto the new #21 head (dtq a62a63e), plus review fixes

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

A. Correctness — `dspace_rest_client/client.py`

B. Correctness — `dspace_rest_client/models.py`

Rebased onto the new #21 head (dtq `a62a63e`), plus review fixes