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

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 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

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. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

- 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. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

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. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

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:


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. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

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(). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

jm and others added 4 commits September 2, 2026 16:11

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 and others added 2 commits September 2, 2026 18:59

Navigation Menu

Uh oh!

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 6 commits into
feat/pyproject-tooling-typingfrom
fix/review-findings-sweep

vidiecan commented Sep 2, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

A. Correctness — `dspace_rest_client/client.py`

B. Correctness — `dspace_rest_client/models.py`