From 7937db17ebd52eca44ee12e8a2bab7e465921b37 Mon Sep 17 00:00:00 2001 From: jm Date: Mon, 17 Aug 2026 18:07:57 +0200 Subject: [PATCH 1/4] test: add unit + integration suite for downstream usage, with CI DSpace-ISstag-integration drives this client from src/{ingest,export,reposync} and mcp/, but nothing here was under test - a library change could silently break that consumer. Add a pytest suite that mocks only the HTTP transport (requests_mock) and exercises the real URL-building and response-parsing for every DSpaceClient method and model the consumer relies on: - test_models: Item/Community/Collection/Bundle/Bitstream/ResourcePolicy construction + accessors (incl. the live _embedded.group -> groupUUID lift) - test_client_auth: constructor + authenticate() True/False semantics - test_client_read: search_objects, get_items, get_item, get_bundles (incl. the 404 -> [] contract from #16), get_bitstreams, get_collections, get_communities, get_resourcepolicy, fetch_resource (last_err on 404) - test_client_write: create_resourcepolicy, api_delete, create_bundle, create_item, create_bitstream (multipart upload) - test_repo_usage_contract: the exact multi-call chains the consumer runs (bitstream export, resource-policy replacement, MCP bundle walk, group-uuid resolution) Adds .github/workflows/tests.yml (pytest on Python 3.10 + 3.12) and requirements-test.txt. 56 tests, no network. Co-Authored-By: Claude Opus 4.8 --- .github/workflows/tests.yml | 40 +++++ .gitignore | 4 + requirements-test.txt | 5 + tests/_helpers.py | 94 +++++++++++ tests/conftest.py | 15 ++ tests/test_client_auth.py | 70 ++++++++ tests/test_client_read.py | 265 ++++++++++++++++++++++++++++++ tests/test_client_write.py | 164 ++++++++++++++++++ tests/test_models.py | 138 ++++++++++++++++ tests/test_repo_usage_contract.py | 154 +++++++++++++++++ 10 files changed, 949 insertions(+) create mode 100644 .github/workflows/tests.yml create mode 100644 requirements-test.txt create mode 100644 tests/_helpers.py create mode 100644 tests/conftest.py create mode 100644 tests/test_client_auth.py create mode 100644 tests/test_client_read.py create mode 100644 tests/test_client_write.py create mode 100644 tests/test_models.py create mode 100644 tests/test_repo_usage_contract.py diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml new file mode 100644 index 0000000..0179525 --- /dev/null +++ b/.github/workflows/tests.yml @@ -0,0 +1,40 @@ +name: Tests + +on: + push: + branches: [ dtq, dtq-dev, main ] + pull_request: + +permissions: + contents: read + +jobs: + test: + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + # Matches the DSpace-ISstag-integration consumer CI matrix; the library + # itself declares support for >=3.8 (see setup.py). + python-version: ["3.10", "3.12"] + + steps: + - uses: actions/checkout@v6 + + - name: Set up Python ${{ matrix.python-version }} + uses: actions/setup-python@v6 + with: + python-version: ${{ matrix.python-version }} + cache: pip + cache-dependency-path: | + setup.py + requirements-test.txt + + - name: Install package + test deps + run: | + python -m pip install --upgrade pip + pip install . + pip install -r requirements-test.txt + + - name: Run tests + run: python -m pytest tests/ -v diff --git a/.gitignore b/.gitignore index 0dc7c57..deba30b 100644 --- a/.gitignore +++ b/.gitignore @@ -1,6 +1,10 @@ __pycache__/ *.py[cod] *$py.class +*.egg-info/ +build/ +dist/ +.pytest_cache/ .python-version Pipfile.lock __pypackages__/ diff --git a/requirements-test.txt b/requirements-test.txt new file mode 100644 index 0000000..f061c74 --- /dev/null +++ b/requirements-test.txt @@ -0,0 +1,5 @@ +# Test-only dependencies for the dspace_rest_client suite. +# The library itself only needs `requests` (see setup.py); these add the test +# runner and an HTTP transport mock so tests never touch a real DSpace server. +pytest>=7.0 +requests-mock>=1.11 diff --git a/tests/_helpers.py b/tests/_helpers.py new file mode 100644 index 0000000..1b39ed7 --- /dev/null +++ b/tests/_helpers.py @@ -0,0 +1,94 @@ +""" +Shared helpers for the client test-suite. + +The tests mock *only* the HTTP transport (via ``requests_mock``) and let the +real ``DSpaceClient`` build URLs, send params and parse responses into model +objects. That way a change to the library that breaks URL construction or +response parsing - the two things downstream code (this repo) depends on - +fails a test instead of silently shipping. +""" +import os +import sys +from urllib.parse import urlparse, parse_qs + +# Make ``dspace_rest_client`` importable when a test module is run directly +# (``python tests/test_x.py``), not just under pytest (see conftest.py). +_ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) +if _ROOT not in sys.path: + sys.path.insert(0, _ROOT) + +from dspace_rest_client.client import DSpaceClient # noqa: E402 + +# Canonical test endpoint. All mocked URLs are built off this so a typo shows +# up as an unmatched request rather than a false pass. +API = "http://dspace.test/server/api" + +# Real, syntactically-valid UUIDs - several client methods validate their UUID +# arguments with ``uuid.UUID(...)`` and short-circuit on a bad one, so tests +# that expect a request to actually go out must use valid values. +ITEM_UUID = "11111111-1111-1111-1111-111111111111" +COLLECTION_UUID = "22222222-2222-2222-2222-222222222222" +BITSTREAM_UUID = "9f54ef33-c454-4d8e-a5fe-79d8291045ba" +ANON_GROUP_UUID = "6ecfd145-3b7d-429e-ab31-ef6905a05763" + + +def make_client(api_endpoint: str = API) -> DSpaceClient: + """A real client with no network touched. + + ``DSpaceClient.__init__`` performs no HTTP (it only creates a + ``requests.Session`` and, optionally, a pysolr handle), so a plain + construction is safe and gives us the genuine object under test. + """ + return DSpaceClient(api_endpoint, "tester@dspace.test", "secret") + + +def sent_params(request) -> dict: + """Case-preserving query params of a captured request. + + ``requests_mock``'s ``request.qs`` lowercases the whole query string, which + would mangle case-sensitive values (eg. ``action=READ``). Parsing the + original ``request.url`` keeps the real casing. + """ + return parse_qs(urlparse(request.url).query) + + +# --- response-body builders (shape mirrors the DSpace 7 REST API) --------- # + +def embedded(key: str, resources: list) -> dict: + """A HAL ``_embedded`` list envelope, eg. ``{"_embedded": {"bundles": [...]}}``.""" + return {"_embedded": {key: resources}} + + +def item_json(uuid: str = ITEM_UUID, name: str = "Thesis", **extra) -> dict: + d = {"uuid": uuid, "name": name, "type": "item", "metadata": {}, + "inArchive": True, "discoverable": True, "withdrawn": False} + d.update(extra) + return d + + +def bundle_json(uuid: str = "bnd", name: str = "ORIGINAL", + bitstreams_href: str = None, **extra) -> dict: + d = {"uuid": uuid, "name": name, "type": "bundle", "metadata": {}} + if bitstreams_href is not None: + d["_links"] = {"bitstreams": {"href": bitstreams_href}} + d.update(extra) + return d + + +def bitstream_json(uuid: str = "bs1", name: str = "thesis.pdf", size: int = 123, + seq: int = 1, checksum: str = "abc", **extra) -> dict: + d = {"uuid": uuid, "name": name, "type": "bitstream", "metadata": {}, + "sizeBytes": size, "sequenceId": seq, + "checkSum": {"checkSumAlgorithm": "MD5", "value": checksum}} + d.update(extra) + return d + + +def policy_json(pid: int = 1, action: str = "READ", group_name: str = "Anonymous", + group_uuid: str = ANON_GROUP_UUID, start_date: str = None) -> dict: + """A resource policy in the *live* API shape (group under ``_embedded``).""" + d = {"id": pid, "action": action, + "_embedded": {"group": {"name": group_name, "uuid": group_uuid}}} + if start_date is not None: + d["startDate"] = start_date + return d diff --git a/tests/conftest.py b/tests/conftest.py new file mode 100644 index 0000000..e1f4654 --- /dev/null +++ b/tests/conftest.py @@ -0,0 +1,15 @@ +""" +Pytest bootstrap for the dspace_rest_client test-suite. + +Ensures the in-tree ``dspace_rest_client`` package is importable when the tests +are run straight from a checkout (``pytest tests/``) without a prior +``pip install``. When the package *is* installed, inserting the source root at +the front of ``sys.path`` means the tests still exercise the working-tree copy, +which is the one we ship and vendor as a submodule. +""" +import os +import sys + +_ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) +if _ROOT not in sys.path: + sys.path.insert(0, _ROOT) diff --git a/tests/test_client_auth.py b/tests/test_client_auth.py new file mode 100644 index 0000000..dae22da --- /dev/null +++ b/tests/test_client_auth.py @@ -0,0 +1,70 @@ +""" +Construction and authentication contract. + +``ingest.dspace_be`` constructs the client from an endpoint/user/password and +calls ``authenticate()``; a False return is turned into a hard ConnectionError, +so the True/False semantics here matter. +""" +import unittest + +import requests_mock + +import _helpers # noqa: F401 +from _helpers import make_client, API + + +class TestConstructor(unittest.TestCase): + + def test_endpoints_derived_from_api_endpoint(self): + c = make_client("http://host:8080/server/api") + self.assertEqual(c.API_ENDPOINT, "http://host:8080/server/api") + self.assertEqual(c.LOGIN_URL, "http://host:8080/server/api/authn/login") + self.assertIsNotNone(c.session) + + def test_default_last_err_is_none(self): + self.assertIsNone(make_client().last_err) + + +class TestAuthenticate(unittest.TestCase): + + def test_success_returns_true_and_propagates_bearer_token(self): + c = make_client() + with requests_mock.Mocker() as m: + m.post(f"{API}/authn/login", status_code=200, + headers={"Authorization": "Bearer tok123"}) + m.get(f"{API}/authn/status", status_code=200, + json={"authenticated": True}) + self.assertTrue(c.authenticate()) + # the bearer token must land on the session for later calls + self.assertEqual(c.session.headers.get("Authorization"), "Bearer tok123") + + def test_invalid_credentials_401_returns_false(self): + c = make_client() + with requests_mock.Mocker() as m: + m.post(f"{API}/authn/login", status_code=401, + json={"message": "invalid"}) + self.assertFalse(c.authenticate()) + + def test_status_not_authenticated_returns_false(self): + c = make_client() + with requests_mock.Mocker() as m: + m.post(f"{API}/authn/login", status_code=200, + headers={"Authorization": "Bearer t"}) + m.get(f"{API}/authn/status", status_code=200, + json={"authenticated": False}) + self.assertFalse(c.authenticate()) + + def test_csrf_403_retries_once_then_gives_up(self): + c = make_client() + with requests_mock.Mocker() as m: + m.post(f"{API}/authn/login", status_code=403, + json={"message": "CSRF token required"}) + self.assertFalse(c.authenticate()) + login_calls = [r for r in m.request_history + if r.path == "/server/api/authn/login"] + # initial attempt + exactly one retry with the refreshed token + self.assertEqual(len(login_calls), 2) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_client_read.py b/tests/test_client_read.py new file mode 100644 index 0000000..f3a8bb7 --- /dev/null +++ b/tests/test_client_read.py @@ -0,0 +1,265 @@ +""" +Read-path contract: every GET method this repo calls. + +Each test stubs only the HTTP response and asserts the real client (a) hits the +right URL with the right query params and (b) parses the response into the model +objects / shapes the callers rely on. +""" +import unittest + +import requests_mock + +import _helpers # noqa: F401 +from _helpers import ( + make_client, sent_params, embedded, item_json, bundle_json, + bitstream_json, policy_json, API, ITEM_UUID, BITSTREAM_UUID) +from dspace_rest_client.models import Item, Bundle, Collection, Community + + +class TestSearchObjects(unittest.TestCase): + + def test_builds_url_params_and_parses_objects(self): + c = make_client() + body = {"_embedded": {"searchResult": { + "page": {"totalElements": 2, "size": 100}, + "_embedded": {"objects": [ + {"_embedded": {"indexableObject": item_json("u1", "A")}}, + {"_embedded": {"indexableObject": item_json("u2", "B")}}, + ]}}}} + with requests_mock.Mocker() as m: + m.get(f"{API}/discover/search/objects", json=body) + details = {} + res = c.search_objects(query="dc.identifier:123", size=100, + page=0, details=details) + self.assertEqual([d.uuid for d in res], ["u1", "u2"]) + p = sent_params(m.last_request) + self.assertEqual(p["query"], ["dc.identifier:123"]) + self.assertEqual(p["size"], ["100"]) + self.assertEqual(p["page"], ["0"]) + # details["page"] is what repo.search.export_iter reads as export_len + self.assertEqual(details["page"]["totalElements"], 2) + + def test_backend_error_returns_empty_list(self): + # fetch_resource returns None on a non-200; search_objects swallows the + # resulting TypeError and yields [] rather than crashing the crawl. + c = make_client() + with requests_mock.Mocker() as m: + m.get(f"{API}/discover/search/objects", status_code=500, text="boom") + self.assertEqual(c.search_objects(query="x"), []) + + def test_empty_result_set_returns_empty_list(self): + c = make_client() + body = {"_embedded": {"searchResult": { + "page": {"totalElements": 0}, + "_embedded": {"objects": []}}}} + with requests_mock.Mocker() as m: + m.get(f"{API}/discover/search/objects", json=body) + self.assertEqual(c.search_objects(query="x"), []) + + +class TestGetItems(unittest.TestCase): + + def test_parses_embedded_items_with_paging_params(self): + c = make_client() + body = embedded("items", [item_json("i1", "one"), item_json("i2", "two")]) + with requests_mock.Mocker() as m: + m.get(f"{API}/core/items", json=body) + items = c.get_items(page=2, size=50) + self.assertEqual([i.uuid for i in items], ["i1", "i2"]) + self.assertTrue(all(isinstance(i, Item) for i in items)) + p = sent_params(m.last_request) + self.assertEqual(p["page"], ["2"]) + self.assertEqual(p["size"], ["50"]) + + +class TestGetItem(unittest.TestCase): + + def test_returns_typed_item(self): + c = make_client() + with requests_mock.Mocker() as m: + m.get(f"{API}/core/items/{ITEM_UUID}", json=item_json(ITEM_UUID, "T")) + it = c.get_item(ITEM_UUID) + self.assertIsInstance(it, Item) + self.assertEqual(it.uuid, ITEM_UUID) + self.assertEqual(it.name, "T") + + def test_invalid_uuid_returns_none_without_request(self): + c = make_client() + with requests_mock.Mocker() as m: + self.assertIsNone(c.get_item("not-a-uuid")) + self.assertEqual(m.call_count, 0) + + +class TestGetBundles(unittest.TestCase): + + def test_by_parent_item_lists_bundles(self): + c = make_client() + parent = Item(item_json(ITEM_UUID)) + body = embedded("bundles", [ + bundle_json("b1", "ORIGINAL"), bundle_json("b2", "THUMBNAIL")]) + with requests_mock.Mocker() as m: + m.get(f"{API}/core/items/{ITEM_UUID}/bundles", json=body) + bundles = c.get_bundles(parent=parent, size=1000) + self.assertEqual([b.name for b in bundles], ["ORIGINAL", "THUMBNAIL"]) + self.assertTrue(all(isinstance(b, Bundle) for b in bundles)) + self.assertEqual(sent_params(m.last_request)["size"], ["1000"]) + + def test_by_uuid_returns_single_wrapped_in_list(self): + c = make_client() + with requests_mock.Mocker() as m: + m.get(f"{API}/core/bundles/b9", json=bundle_json("b9", "ORIGINAL")) + bundles = c.get_bundles(uuid="b9") + self.assertEqual(len(bundles), 1) + self.assertEqual(bundles[0].uuid, "b9") + + def test_deleted_item_404_returns_empty_list(self): + # PR #16 contract: a gone item is a clean empty result, not a crash. + c = make_client() + parent = Item(item_json(ITEM_UUID)) + with requests_mock.Mocker() as m: + m.get(f"{API}/core/items/{ITEM_UUID}/bundles", status_code=404, + json={"timestamp": "2026-01-01"}) + self.assertEqual(c.get_bundles(parent=parent), []) + + def test_no_args_returns_empty_without_request(self): + c = make_client() + with requests_mock.Mocker() as m: + self.assertEqual(c.get_bundles(), []) + self.assertEqual(m.call_count, 0) + + +class TestGetBitstreams(unittest.TestCase): + + def test_by_bundle_uses_embedded_link(self): + c = make_client() + href = f"{API}/core/bundles/bnd/bitstreams" + bundle = Bundle(bundle_json("bnd", bitstreams_href=href)) + body = embedded("bitstreams", [bitstream_json("s1", "a.pdf", size=10)]) + with requests_mock.Mocker() as m: + m.get(href, json=body) + bs = c.get_bitstreams(bundle=bundle, size=500) + self.assertEqual([b.uuid for b in bs], ["s1"]) + self.assertEqual(bs[0].sizeBytes, 10) + self.assertEqual(sent_params(m.last_request)["size"], ["500"]) + + def test_by_bundle_without_link_constructs_url(self): + c = make_client() + bundle = Bundle(bundle_json("bnd2")) # no _links -> manual URL + with requests_mock.Mocker() as m: + m.get(f"{API}/core/bundles/bnd2/bitstreams", + json=embedded("bitstreams", [])) + self.assertEqual(c.get_bitstreams(bundle=bundle), []) + + def test_no_args_returns_empty_list(self): + c = make_client() + with requests_mock.Mocker() as m: + self.assertEqual(c.get_bitstreams(), []) + self.assertEqual(m.call_count, 0) + + +class TestGetCollections(unittest.TestCase): + + def test_for_community_uses_collections_link(self): + c = make_client() + href = f"{API}/core/communities/c1/collections" + com = Community({"uuid": "c1", "name": "Com", "type": "community", + "_links": {"collections": {"href": href}}}) + body = embedded("collections", [ + {"uuid": "col1", "name": "Theses", "handle": "123/1", + "type": "collection"}]) + with requests_mock.Mocker() as m: + m.get(href, json=body) + cols = c.get_collections(community=com) + self.assertEqual([x.name for x in cols], ["Theses"]) + self.assertEqual(cols[0].handle, "123/1") + self.assertTrue(all(isinstance(x, Collection) for x in cols)) + + def test_plain_list(self): + c = make_client() + body = embedded("collections", [ + {"uuid": "col2", "name": "C2", "type": "collection"}]) + with requests_mock.Mocker() as m: + m.get(f"{API}/core/collections", json=body) + cols = c.get_collections() + self.assertEqual([x.uuid for x in cols], ["col2"]) + + +class TestGetCommunities(unittest.TestCase): + + def test_top_uses_search_top_endpoint(self): + c = make_client() + body = embedded("communities", [ + {"uuid": "c1", "name": "Top", "type": "community"}]) + with requests_mock.Mocker() as m: + m.get(f"{API}/core/communities/search/top", json=body) + coms = c.get_communities(top=True) + self.assertEqual([x.name for x in coms], ["Top"]) + self.assertTrue(all(isinstance(x, Community) for x in coms)) + + def test_plain_list(self): + c = make_client() + body = embedded("communities", [ + {"uuid": "c2", "name": "Other", "type": "community"}]) + with requests_mock.Mocker() as m: + m.get(f"{API}/core/communities", json=body) + coms = c.get_communities() + self.assertEqual([x.uuid for x in coms], ["c2"]) + + +class TestGetResourcePolicy(unittest.TestCase): + + def test_parses_live_policies_and_sends_uuid_action(self): + c = make_client() + body = embedded("resourcepolicies", [policy_json(pid=1)]) + with requests_mock.Mocker() as m: + m.get(f"{API}/authz/resourcepolicies/search/resource", json=body) + rps = c.get_resourcepolicy(BITSTREAM_UUID, action="READ") + self.assertEqual(len(rps), 1) + self.assertEqual(rps[0].groupName, "Anonymous") + p = sent_params(m.last_request) + self.assertEqual(p["uuid"], [BITSTREAM_UUID]) + self.assertEqual(p["action"], ["READ"]) + + def test_no_embedded_returns_empty_list(self): + c = make_client() + with requests_mock.Mocker() as m: + m.get(f"{API}/authz/resourcepolicies/search/resource", + json={"page": {"totalElements": 0}}) + self.assertEqual(c.get_resourcepolicy(BITSTREAM_UUID), []) + + def test_invalid_uuid_returns_none_without_request(self): + c = make_client() + with requests_mock.Mocker() as m: + self.assertIsNone(c.get_resourcepolicy("not-a-uuid")) + self.assertEqual(m.call_count, 0) + + def test_fetch_failure_returns_none(self): + c = make_client() + with requests_mock.Mocker() as m: + m.get(f"{API}/authz/resourcepolicies/search/resource", + status_code=500, text="boom") + self.assertIsNone(c.get_resourcepolicy(BITSTREAM_UUID)) + + +class TestFetchResource(unittest.TestCase): + + def test_200_returns_parsed_json(self): + c = make_client() + url = f"{API}/eperson/groups/search/byMetadata" + with requests_mock.Mocker() as m: + m.get(url, json=embedded("groups", [])) + self.assertEqual(c.fetch_resource(url, params={"query": "Anonymous"}), + {"_embedded": {"groups": []}}) + + def test_404_returns_none_and_records_last_err(self): + # group_uuid() and get_bundles() both branch on last_err.status_code. + c = make_client() + url = f"{API}/core/items/{ITEM_UUID}/bundles" + with requests_mock.Mocker() as m: + m.get(url, status_code=404, text="gone") + self.assertIsNone(c.fetch_resource(url)) + self.assertEqual(c.last_err.status_code, 404) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_client_write.py b/tests/test_client_write.py new file mode 100644 index 0000000..0f01e8d --- /dev/null +++ b/tests/test_client_write.py @@ -0,0 +1,164 @@ +""" +Write-path contract: the POST / DELETE methods this repo calls. + +The sync tooling only ever inspects a handful of things off these calls +(``ResourcePolicy.id`` after a create, the response ``status_code`` after a +delete, ``Bitstream.uuid`` after an upload); the tests pin the request that is +sent *and* the object that comes back. +""" +import os +import tempfile +import unittest + +import requests_mock + +import _helpers # noqa: F401 +from _helpers import ( + make_client, sent_params, bundle_json, bitstream_json, item_json, + API, ITEM_UUID, COLLECTION_UUID, BITSTREAM_UUID, ANON_GROUP_UUID) +from dspace_rest_client.models import Item, Bundle, Bitstream + + +class TestCreateResourcePolicy(unittest.TestCase): + + def test_success_sends_params_body_and_returns_policy(self): + c = make_client() + with requests_mock.Mocker() as m: + m.post(f"{API}/authz/resourcepolicies", status_code=201, + json={"id": 42, "action": "READ", "startDate": "2028-05-19", + "_embedded": {"group": {"name": "Anonymous", + "uuid": ANON_GROUP_UUID}}}) + rp = c.create_resourcepolicy( + resource_uuid=BITSTREAM_UUID, group_uuid=ANON_GROUP_UUID, + action="READ", start_date="2028-05-19") + self.assertIsNotNone(rp) + self.assertEqual(rp.id, 42) + p = sent_params(m.last_request) + self.assertEqual(p["resource"], [BITSTREAM_UUID]) + self.assertEqual(p["group"], [ANON_GROUP_UUID]) + body = m.last_request.json() + self.assertEqual(body["action"], "READ") + self.assertEqual(body["type"], "resourcepolicy") + self.assertEqual(body["startDate"], "2028-05-19") + + def test_without_start_date_omits_it_from_body(self): + c = make_client() + with requests_mock.Mocker() as m: + m.post(f"{API}/authz/resourcepolicies", status_code=201, + json={"id": 1, "action": "READ"}) + c.create_resourcepolicy(resource_uuid=BITSTREAM_UUID, + group_uuid=ANON_GROUP_UUID) + self.assertNotIn("startDate", m.last_request.json()) + + def test_invalid_uuid_returns_none_without_request(self): + c = make_client() + with requests_mock.Mocker() as m: + self.assertIsNone(c.create_resourcepolicy( + resource_uuid="bad", group_uuid=ANON_GROUP_UUID)) + self.assertEqual(m.call_count, 0) + + def test_server_error_returns_none(self): + c = make_client() + with requests_mock.Mocker() as m: + m.post(f"{API}/authz/resourcepolicies", status_code=500, text="boom") + self.assertIsNone(c.create_resourcepolicy( + resource_uuid=BITSTREAM_UUID, group_uuid=ANON_GROUP_UUID)) + + +class TestApiDelete(unittest.TestCase): + + def test_returns_response_with_status_code(self): + c = make_client() + url = f"{API}/authz/resourcepolicies/42" + with requests_mock.Mocker() as m: + m.delete(url, status_code=204) + self.assertEqual(c.api_delete(url, params=None).status_code, 204) + + def test_404_surfaces_as_response(self): + # files_access treats a 404 on delete as "already gone" == success. + c = make_client() + url = f"{API}/authz/resourcepolicies/7" + with requests_mock.Mocker() as m: + m.delete(url, status_code=404) + self.assertEqual(c.api_delete(url, params=None).status_code, 404) + + +class TestCreateBundle(unittest.TestCase): + + def test_posts_to_item_bundles_and_returns_bundle(self): + c = make_client() + parent = Item(item_json(ITEM_UUID)) + with requests_mock.Mocker() as m: + m.post(f"{API}/core/items/{ITEM_UUID}/bundles", status_code=201, + json=bundle_json("nb", "ORIGINAL")) + b = c.create_bundle(parent=parent) + self.assertIsInstance(b, Bundle) + self.assertEqual(b.uuid, "nb") + self.assertEqual(m.last_request.json(), + {"name": "ORIGINAL", "metadata": {}}) + + def test_none_parent_returns_none(self): + self.assertIsNone(make_client().create_bundle(parent=None)) + + +class TestCreateItem(unittest.TestCase): + + def test_posts_with_owning_collection_param_and_returns_item(self): + c = make_client() + item = Item({"name": "New thesis", "metadata": {}}) + with requests_mock.Mocker() as m: + m.post(f"{API}/core/items", status_code=201, + json=item_json("newu", "New thesis")) + out = c.create_item(parent=COLLECTION_UUID, item=item) + self.assertIsInstance(out, Item) + self.assertEqual(out.uuid, "newu") + self.assertEqual(sent_params(m.last_request)["owningCollection"], + [COLLECTION_UUID]) + + def test_non_item_returns_none(self): + self.assertIsNone(make_client().create_item( + parent=COLLECTION_UUID, item={"not": "an item"})) + + def test_none_parent_returns_none(self): + item = Item({"name": "x", "metadata": {}}) + self.assertIsNone(make_client().create_item(parent=None, item=item)) + + +class TestCreateBitstream(unittest.TestCase): + + def setUp(self): + fd, self.path = tempfile.mkstemp(suffix=".pdf") + with os.fdopen(fd, "wb") as fh: + fh.write(b"%PDF-1.4 hello world") + self.addCleanup(lambda: os.path.exists(self.path) and os.unlink(self.path)) + + def test_success_multipart_upload_returns_bitstream(self): + c = make_client() + bundle = Bundle(bundle_json("bnd")) + with requests_mock.Mocker() as m: + m.post(f"{API}/core/bundles/bnd/bitstreams", status_code=201, + json=bitstream_json("bsnew", "a.pdf", size=20)) + bs = c.create_bitstream( + bundle=bundle, name="a.pdf", path=self.path, + mime="application/pdf", + metadata={"dc.title": [{"value": "a.pdf"}]}) + self.assertIsInstance(bs, Bitstream) + self.assertEqual(bs.uuid, "bsnew") + self.assertEqual(bs.sizeBytes, 20) + # the request really was a multipart file upload + self.assertIn("multipart/form-data", + m.last_request.headers["Content-Type"]) + + def test_server_error_returns_none(self): + c = make_client() + bundle = Bundle(bundle_json("bnd")) + with requests_mock.Mocker() as m: + m.post(f"{API}/core/bundles/bnd/bitstreams", status_code=500, + text="boom") + self.assertIsNone(c.create_bitstream( + bundle=bundle, name="a.pdf", path=self.path, + mime="application/pdf")) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_models.py b/tests/test_models.py new file mode 100644 index 0000000..d18546d --- /dev/null +++ b/tests/test_models.py @@ -0,0 +1,138 @@ +""" +Model construction / accessor contract. + +This repo builds these model objects straight from cached or live API JSON and +reads a fixed set of attributes off them (``.uuid``, ``.name``, ``.metadata``, +``.sizeBytes``, ``.checkSum``, ``ResourcePolicy.groupUUID`` ...). If a rename or +a parsing change in the library dropped one of those, the audit/export/sync +tooling would break - these tests pin the shape. +""" +import unittest + +import _helpers # noqa: F401 (bootstraps sys.path for direct runs) +from dspace_rest_client.models import ( + Item, Community, Collection, Bundle, Bitstream, ResourcePolicy) + + +class TestItem(unittest.TestCase): + + def test_core_fields_from_api_resource(self): + it = Item({ + "uuid": "u1", "name": "Thesis", "type": "item", + "metadata": {"dc.title": [{"value": "Thesis"}]}, + "_links": {"self": {"href": "http://x/items/u1"}}, + }) + self.assertEqual(it.uuid, "u1") + self.assertEqual(it.name, "Thesis") + self.assertEqual(it.type, "item") + self.assertEqual(it.metadata["dc.title"][0]["value"], "Thesis") + self.assertEqual(it.links["self"]["href"], "http://x/items/u1") + + def test_type_is_item_even_without_explicit_type(self): + # ingest.dspace_be.create_item builds Item(dict) from a hand-made dict + # that has no "type" key; the class must still self-identify as an item. + it = Item({"name": "n", "metadata": {}}) + self.assertEqual(it.type, "item") + + def test_as_dict_carries_item_flags(self): + it = Item({"uuid": "u1", "name": "N", "metadata": {}, + "inArchive": True, "discoverable": True, "withdrawn": False}) + d = it.as_dict() + self.assertEqual(d["uuid"], "u1") + self.assertEqual(d["type"], "item") + self.assertEqual( + (d["inArchive"], d["discoverable"], d["withdrawn"]), + (True, True, False)) + + +class TestCommunityCollection(unittest.TestCase): + + def test_community_fields_and_links(self): + com = Community({ + "uuid": "c1", "name": "Faculty", "type": "community", + "_links": {"collections": {"href": "http://x/communities/c1/collections"}}, + }) + self.assertEqual(com.uuid, "c1") + self.assertEqual(com.name, "Faculty") + self.assertEqual(com.type, "community") + self.assertEqual(com.links["collections"]["href"], + "http://x/communities/c1/collections") + + def test_collection_fields_including_handle(self): + col = Collection({"uuid": "col1", "name": "Theses", + "handle": "123456789/1", "type": "collection"}) + self.assertEqual(col.uuid, "col1") + self.assertEqual(col.name, "Theses") + self.assertEqual(col.handle, "123456789/1") + self.assertEqual(col.type, "collection") + + +class TestBundleBitstream(unittest.TestCase): + + def test_bundle_fields_and_bitstreams_link(self): + # get_bitstreams(bundle=...) prefers this embedded link over a manually + # constructed URL, so it is part of the contract. + b = Bundle({"uuid": "b1", "name": "ORIGINAL", "type": "bundle", + "metadata": {}, + "_links": {"bitstreams": {"href": "http://x/bundles/b1/bitstreams"}}}) + self.assertEqual((b.uuid, b.name, b.type), ("b1", "ORIGINAL", "bundle")) + self.assertEqual(b.links["bitstreams"]["href"], + "http://x/bundles/b1/bitstreams") + + def test_bitstream_file_fields(self): + # export/_dspace serialises exactly these attributes per bitstream. + b = Bitstream({"uuid": "s1", "name": "f.pdf", "type": "bitstream", + "metadata": {"dc.title": [{"value": "f.pdf"}]}, + "sizeBytes": 2048, "sequenceId": 3, + "checkSum": {"checkSumAlgorithm": "MD5", "value": "deadbeef"}}) + self.assertEqual(b.uuid, "s1") + self.assertEqual(b.name, "f.pdf") + self.assertEqual(b.sizeBytes, 2048) + self.assertEqual(b.sequenceId, 3) + self.assertEqual(b.checkSum["value"], "deadbeef") + d = b.as_dict() + self.assertEqual(d["sizeBytes"], 2048) + self.assertEqual(d["checkSum"]["value"], "deadbeef") + self.assertEqual(d["sequenceId"], 3) + + +class TestResourcePolicy(unittest.TestCase): + + def test_direct_cached_format(self): + # the shape produced by ResourcePolicy.as_dict() and re-read from cache + rp = ResourcePolicy({"id": 5, "action": "READ", "groupName": "Anonymous", + "groupUUID": "g1", "startDate": "2028-01-01", + "endDate": None}) + self.assertEqual(rp.id, 5) + self.assertEqual(rp.action, "READ") + self.assertEqual(rp.groupName, "Anonymous") + self.assertEqual(rp.groupUUID, "g1") + self.assertEqual(rp.startDate, "2028-01-01") + self.assertIsNone(rp.endDate) + + def test_live_api_embedded_group_format(self): + # This is what /authz/resourcepolicies actually returns; files_access + # relies on groupName/groupUUID being lifted out of _embedded.group. + rp = ResourcePolicy({"id": 9, "action": "READ", "startDate": "2028-05-19", + "_embedded": {"group": {"name": "Anonymous", + "uuid": "anon-uuid"}}}) + self.assertEqual(rp.groupName, "Anonymous") + self.assertEqual(rp.groupUUID, "anon-uuid") + + def test_as_dict_roundtrip_keeps_group_and_action(self): + rp = ResourcePolicy({"id": 7, "action": "READ", + "_embedded": {"group": {"name": "Anonymous", + "uuid": "anon"}}}) + d = rp.as_dict() + self.assertEqual(d["id"], 7) + self.assertEqual(d["action"], "READ") + self.assertEqual(d["groupName"], "Anonymous") + self.assertEqual(d["groupUUID"], "anon") + # a re-parse of the cached dict must survive the round-trip + rp2 = ResourcePolicy(d) + self.assertEqual(rp2.groupUUID, "anon") + self.assertEqual(rp2.action, "READ") + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_repo_usage_contract.py b/tests/test_repo_usage_contract.py new file mode 100644 index 0000000..460d708 --- /dev/null +++ b/tests/test_repo_usage_contract.py @@ -0,0 +1,154 @@ +""" +Integration contract: the exact multi-call sequences DSpace-ISstag-integration +runs against this library. + +Where the per-method tests pin one call, these replay a whole flow end-to-end +(only the HTTP transport mocked) so that a library change which individually +looks harmless but breaks a *chain* our tooling depends on still fails here. + +Each test names the source it mirrors. +""" +import unittest +from types import SimpleNamespace + +import requests_mock + +import _helpers # noqa: F401 +from _helpers import ( + make_client, embedded, item_json, bundle_json, bitstream_json, + policy_json, API, ITEM_UUID, BITSTREAM_UUID, ANON_GROUP_UUID) +from dspace_rest_client.models import Item + + +class TestBitstreamExportChain(unittest.TestCase): + """Mirrors src/export/_dspace.py :: exporter.export_bitstreams - + get_bundles(parent) -> get_resourcepolicy(bundle) -> get_bitstreams(bundle) + -> get_resourcepolicy(bitstream), then serialises a fixed set of attrs.""" + + # real UUIDs: the export chain calls get_resourcepolicy() on the bundle and + # bitstream, and that method validates (and short-circuits on) its UUID arg. + BUNDLE_UUID = "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa" + + def test_full_chain_yields_serialisable_attributes(self): + c = make_client() + item = Item(item_json(ITEM_UUID)) + bits_href = f"{API}/core/bundles/{self.BUNDLE_UUID}/bitstreams" + with requests_mock.Mocker() as m: + m.get(f"{API}/core/items/{ITEM_UUID}/bundles", + json=embedded("bundles", + [bundle_json(self.BUNDLE_UUID, "ORIGINAL", + bitstreams_href=bits_href)])) + m.get(bits_href, + json=embedded("bitstreams", + [bitstream_json(BITSTREAM_UUID, "thesis.pdf", + size=123, seq=1, checksum="abc")])) + m.get(f"{API}/authz/resourcepolicies/search/resource", + json=embedded("resourcepolicies", [policy_json(pid=1)])) + + bundles = c.get_bundles(parent=item, size=1000) + self.assertEqual(len(bundles), 1) + bundle = bundles[0] + self.assertEqual((bundle.name, bundle.uuid, bundle.type), + ("ORIGINAL", self.BUNDLE_UUID, "bundle")) + + bundle_rp = c.get_resourcepolicy(bundle.uuid) + self.assertEqual([rp.as_dict()["groupName"] for rp in bundle_rp], + ["Anonymous"]) + + bitstreams = c.get_bitstreams(bundle=bundle, size=1000) + self.assertEqual(len(bitstreams), 1) + b = bitstreams[0] + # the exporter reads exactly these off each bitstream + self.assertEqual((b.name, b.uuid, b.sizeBytes, b.sequenceId), + ("thesis.pdf", BITSTREAM_UUID, 123, 1)) + self.assertEqual(b.checkSum["value"], "abc") + + bs_rp = c.get_resourcepolicy(b.uuid) + self.assertEqual(bs_rp[0].as_dict()["groupUUID"], ANON_GROUP_UUID) + + +class TestPolicyReplacementChain(unittest.TestCase): + """Mirrors src/reposync/_files.py :: files_access._set_read_policy - + read live policies, create a fresh Anonymous READ policy, delete the old.""" + + def test_read_create_delete(self): + c = make_client() + with requests_mock.Mocker() as m: + m.get(f"{API}/authz/resourcepolicies/search/resource", + json=embedded("resourcepolicies", [policy_json(pid=11)])) + m.post(f"{API}/authz/resourcepolicies", status_code=201, + json={"id": 99, "action": "READ", "startDate": "2028-05-19", + "_embedded": {"group": {"name": "Anonymous", + "uuid": ANON_GROUP_UUID}}}) + m.delete(f"{API}/authz/resourcepolicies/11", status_code=204) + + live = c.get_resourcepolicy(BITSTREAM_UUID, action="READ") + # _set_read_policy reads id / action / groupName off each live policy + self.assertEqual(live[0].id, 11) + self.assertEqual(live[0].action, "READ") + self.assertEqual(live[0].groupName, "Anonymous") + + new = c.create_resourcepolicy( + resource_uuid=BITSTREAM_UUID, group_uuid=ANON_GROUP_UUID, + action="READ", start_date="2028-05-19") + self.assertEqual(new.id, 99) + + r = c.api_delete( + f"{API}/authz/resourcepolicies/{live[0].id}", params=None) + self.assertEqual(r.status_code, 204) + + +class TestMcpBundleWalk(unittest.TestCase): + """Mirrors mcp/core.py :: make_service_from_env - lookup via search_objects, + then get_item -> get_bundles(SimpleNamespace parent) -> get_bitstreams.""" + + def test_lookup_then_item_bundles_bitstreams(self): + c = make_client() + bits_href = f"{API}/core/bundles/bnd/bitstreams" + with requests_mock.Mocker() as m: + m.get(f"{API}/discover/search/objects", json={"_embedded": { + "searchResult": {"page": {"totalElements": 1}, + "_embedded": {"objects": [ + {"_embedded": {"indexableObject": + item_json(ITEM_UUID, "T")}}]}}}}) + m.get(f"{API}/core/items/{ITEM_UUID}", json=item_json(ITEM_UUID, "T")) + m.get(f"{API}/core/items/{ITEM_UUID}/bundles", + json=embedded("bundles", + [bundle_json("bnd", "ORIGINAL", + bitstreams_href=bits_href)])) + m.get(bits_href, + json=embedded("bitstreams", [bitstream_json("bs1", "a.pdf")])) + + matches = c.search_objects(query="dc.identifier:42") + self.assertEqual(matches[0].uuid, ITEM_UUID) + + item = c.get_item(ITEM_UUID) + self.assertEqual(item.uuid, ITEM_UUID) + + # mcp passes a duck-typed parent (only .uuid), not a real Item + parent = SimpleNamespace(uuid=item.uuid) + bundles = c.get_bundles(parent=parent, size=200) + self.assertEqual(bundles[0].name, "ORIGINAL") + + bitstreams = c.get_bitstreams(bundle=bundles[0], size=500) + self.assertEqual(bitstreams[0].uuid, "bs1") + + +class TestGroupSearchForUuidResolution(unittest.TestCase): + """Mirrors src/ingest/_dspace.py :: dspace_be.group_uuid - a raw + fetch_resource on the group-search endpoint, reading _embedded.groups.""" + + def test_group_search_shape(self): + c = make_client() + url = f"{API}/eperson/groups/search/byMetadata" + with requests_mock.Mocker() as m: + m.get(url, json=embedded("groups", [ + {"name": "Anonymous", "uuid": "anon-uuid"}])) + r = c.fetch_resource(url, params={"query": "Anonymous", "size": 100}) + groups = (r.get("_embedded") or {}).get("groups") or [] + self.assertEqual(groups[0]["name"], "Anonymous") + self.assertEqual(groups[0]["uuid"], "anon-uuid") + + +if __name__ == "__main__": + unittest.main() From 2687c6c4d89f2cb6eec17daff69b130f3f7cd85c Mon Sep 17 00:00:00 2001 From: jm Date: Mon, 17 Aug 2026 18:28:49 +0200 Subject: [PATCH 2/4] test: harden suite after adversarial review (faithfulness + gaps) Acting on a two-reviewer audit (workflow + Opus-5 advisor): Faithfulness fixes (tests that could pass against a broken library): - get_bitstreams embedded-link test used an href byte-identical to the fallback URL, so it could not distinguish the branches; give it a distinct href that the fallback could not produce. - MCP chain fed a link-bearing Bundle to get_bitstreams; the real consumer round-trips through as_dict() (drops _links) and hits the fallback URL - rebuild the Bundle from as_dict() and assert the fallback is used. - export chain fetched policies with action='READ' (raw-client default) but the real exporter goes through a wrapper defaulting to action=None (no filter); call with action=None and assert no action param is sent. - get_resourcepolicy empty test used a no-_embedded body (defensive branch); the live API returns an _embedded envelope even when empty - use that. Coverage / stronger assertions: - get_resourcepolicy action=None omits the filter and returns all actions. - create_item now asserts the POST body (name/metadata/type/flags), not just the uuid; create_bitstream asserts the multipart 'properties' payload (name/bundleName/metadata). - search_objects result now asserts .as_dict() and links['self']['href'], the two accessors every consumer reads. - model tests assert parsed .metadata and checkSum.checkSumAlgorithm (dropped the tautological hard-set .type assertions' reliance). - get_bitstreams non-200, and create_item/create_bundle server-error: characterization tests pinning the current (non-fail-safe) behavior the consumers depend on, flagged in-comment for a future library hardening. 60 tests, still no network. Co-Authored-By: Claude Opus 4.8 --- requirements-test.txt | 6 ++-- tests/_helpers.py | 17 +++++++++ tests/test_client_read.py | 59 +++++++++++++++++++++++++++---- tests/test_client_write.py | 49 +++++++++++++++++++++++-- tests/test_models.py | 9 ++++- tests/test_repo_usage_contract.py | 41 +++++++++++++++------ 6 files changed, 158 insertions(+), 23 deletions(-) diff --git a/requirements-test.txt b/requirements-test.txt index f061c74..575ad6c 100644 --- a/requirements-test.txt +++ b/requirements-test.txt @@ -1,5 +1,7 @@ # Test-only dependencies for the dspace_rest_client suite. -# The library itself only needs `requests` (see setup.py); these add the test -# runner and an HTTP transport mock so tests never touch a real DSpace server. +# The test runner and an HTTP transport mock so tests never touch a real DSpace +# server. `requests` is listed explicitly so `pip install -r requirements-test.txt` +# alone is enough to import the package (CI also `pip install .`s it via setup.py). pytest>=7.0 requests-mock>=1.11 +requests diff --git a/tests/_helpers.py b/tests/_helpers.py index 1b39ed7..6fdbecf 100644 --- a/tests/_helpers.py +++ b/tests/_helpers.py @@ -7,7 +7,9 @@ response parsing - the two things downstream code (this repo) depends on - fails a test instead of silently shipping. """ +import json import os +import re import sys from urllib.parse import urlparse, parse_qs @@ -52,6 +54,21 @@ def sent_params(request) -> dict: return parse_qs(urlparse(request.url).query) +def multipart_properties(request) -> dict: + """Parse the JSON ``properties`` part of a create_bitstream multipart body. + + ``create_bitstream`` sends ``properties = json.dumps({name, metadata, + bundleName}) + ';application/json'`` as a form field. This is what actually + carries the bitstream's metadata to DSpace, so tests assert on it. + """ + body = request.body + if isinstance(body, bytes): + body = body.decode("utf-8", "replace") + m = re.search(r'name="properties"\r?\n\r?\n(.*?);application/json', + body, re.DOTALL) + return json.loads(m.group(1)) if m else None + + # --- response-body builders (shape mirrors the DSpace 7 REST API) --------- # def embedded(key: str, resources: list) -> dict: diff --git a/tests/test_client_read.py b/tests/test_client_read.py index f3a8bb7..ae08c4c 100644 --- a/tests/test_client_read.py +++ b/tests/test_client_read.py @@ -20,10 +20,14 @@ class TestSearchObjects(unittest.TestCase): def test_builds_url_params_and_parses_objects(self): c = make_client() + # every caller's next step is dso.as_dict() + dso.links['self']['href'] + # (repo._search.dso2dict, mcp._dso_to_dict), so the search result must + # carry both - include a self link to prove it survives parsing. + obj1 = item_json("u1", "A", _links={"self": {"href": f"{API}/items/u1"}}) body = {"_embedded": {"searchResult": { "page": {"totalElements": 2, "size": 100}, "_embedded": {"objects": [ - {"_embedded": {"indexableObject": item_json("u1", "A")}}, + {"_embedded": {"indexableObject": obj1}}, {"_embedded": {"indexableObject": item_json("u2", "B")}}, ]}}}} with requests_mock.Mocker() as m: @@ -32,6 +36,9 @@ def test_builds_url_params_and_parses_objects(self): res = c.search_objects(query="dc.identifier:123", size=100, page=0, details=details) self.assertEqual([d.uuid for d in res], ["u1", "u2"]) + # the two accessors every consumer reads off a search hit + self.assertEqual(res[0].links["self"]["href"], f"{API}/items/u1") + self.assertEqual(res[0].as_dict()["uuid"], "u1") p = sent_params(m.last_request) self.assertEqual(p["query"], ["dc.identifier:123"]) self.assertEqual(p["size"], ["100"]) @@ -132,7 +139,10 @@ class TestGetBitstreams(unittest.TestCase): def test_by_bundle_uses_embedded_link(self): c = make_client() - href = f"{API}/core/bundles/bnd/bitstreams" + # href deliberately NOT equal to the fallback URL (.../bundles/bnd/ + # bitstreams): if the embedded-link branch were removed the client would + # build the fallback, which is unmocked, and this test would fail. + href = f"{API}/core/bundles/HREF-ONLY-PATH/bitstreams" bundle = Bundle(bundle_json("bnd", bitstreams_href=href)) body = embedded("bitstreams", [bitstream_json("s1", "a.pdf", size=10)]) with requests_mock.Mocker() as m: @@ -140,6 +150,7 @@ def test_by_bundle_uses_embedded_link(self): bs = c.get_bitstreams(bundle=bundle, size=500) self.assertEqual([b.uuid for b in bs], ["s1"]) self.assertEqual(bs[0].sizeBytes, 10) + self.assertEqual(m.last_request.url.split("?")[0], href) self.assertEqual(sent_params(m.last_request)["size"], ["500"]) def test_by_bundle_without_link_constructs_url(self): @@ -147,8 +158,12 @@ def test_by_bundle_without_link_constructs_url(self): bundle = Bundle(bundle_json("bnd2")) # no _links -> manual URL with requests_mock.Mocker() as m: m.get(f"{API}/core/bundles/bnd2/bitstreams", - json=embedded("bitstreams", [])) - self.assertEqual(c.get_bitstreams(bundle=bundle), []) + json=embedded("bitstreams", + [bitstream_json("s9", "x.pdf", size=7)])) + bs = c.get_bitstreams(bundle=bundle) + # proves both the constructed URL AND parsing on the fallback path + self.assertEqual([b.uuid for b in bs], ["s9"]) + self.assertEqual(bs[0].sizeBytes, 7) def test_no_args_returns_empty_list(self): c = make_client() @@ -156,6 +171,21 @@ def test_no_args_returns_empty_list(self): self.assertEqual(c.get_bitstreams(), []) self.assertEqual(m.call_count, 0) + def test_non_200_currently_raises_no_failsafe(self): + # CHARACTERIZATION of a known sharp edge: unlike get_bundles (which since + # PR #16 returns [] on a 404), get_bitstreams has no fail-safe - a non-200 + # makes fetch_resource return None which this method then subscripts, so + # it raises. The consumers (export/_dspace, reposync/_files) iterate the + # result unguarded, so this is a real crash risk. Pinned deliberately: if + # the library is hardened to return [], update this test to assert that. + c = make_client() + bundle = Bundle(bundle_json("bnd2")) + with requests_mock.Mocker() as m: + m.get(f"{API}/core/bundles/bnd2/bitstreams", + status_code=500, text="boom") + with self.assertRaises(Exception): + c.get_bitstreams(bundle=bundle) + class TestGetCollections(unittest.TestCase): @@ -220,11 +250,28 @@ def test_parses_live_policies_and_sends_uuid_action(self): self.assertEqual(p["uuid"], [BITSTREAM_UUID]) self.assertEqual(p["action"], ["READ"]) - def test_no_embedded_returns_empty_list(self): + def test_action_none_omits_the_action_filter(self): + # The bitstream export path calls this via the ingest wrapper whose + # default is action=None (ingest/_dspace.py get_resourcepolicy), which + # must fetch policies of ALL actions - so no `action` param is sent. + c = make_client() + body = embedded("resourcepolicies", [ + policy_json(pid=1, action="READ"), + policy_json(pid=2, action="WRITE")]) + with requests_mock.Mocker() as m: + m.get(f"{API}/authz/resourcepolicies/search/resource", json=body) + rps = c.get_resourcepolicy(BITSTREAM_UUID, action=None) + self.assertEqual([rp.action for rp in rps], ["READ", "WRITE"]) + p = sent_params(m.last_request) + self.assertEqual(p["uuid"], [BITSTREAM_UUID]) + self.assertNotIn("action", p) + + def test_empty_result_set_returns_empty_list(self): + # The live endpoint returns an _embedded envelope even when empty. c = make_client() with requests_mock.Mocker() as m: m.get(f"{API}/authz/resourcepolicies/search/resource", - json={"page": {"totalElements": 0}}) + json=embedded("resourcepolicies", [])) self.assertEqual(c.get_resourcepolicy(BITSTREAM_UUID), []) def test_invalid_uuid_returns_none_without_request(self): diff --git a/tests/test_client_write.py b/tests/test_client_write.py index 0f01e8d..ee3922e 100644 --- a/tests/test_client_write.py +++ b/tests/test_client_write.py @@ -14,8 +14,9 @@ import _helpers # noqa: F401 from _helpers import ( - make_client, sent_params, bundle_json, bitstream_json, item_json, - API, ITEM_UUID, COLLECTION_UUID, BITSTREAM_UUID, ANON_GROUP_UUID) + make_client, sent_params, multipart_properties, bundle_json, + bitstream_json, item_json, API, ITEM_UUID, COLLECTION_UUID, + BITSTREAM_UUID, ANON_GROUP_UUID) from dspace_rest_client.models import Item, Bundle, Bitstream @@ -100,6 +101,21 @@ def test_posts_to_item_bundles_and_returns_bundle(self): def test_none_parent_returns_none(self): self.assertIsNone(make_client().create_bundle(parent=None)) + def test_server_error_returns_truthy_uuidless_bundle(self): + # CHARACTERIZATION: like create_item, create_bundle wraps the response + # unconditionally -> a truthy Bundle with uuid=None on failure, not None. + # The importer's `if not bundle` guard (reposync/_importer.py:118-120) + # never fires because a Bundle instance is always truthy. Pinned. + c = make_client() + parent = Item(item_json(ITEM_UUID)) + with requests_mock.Mocker() as m: + m.post(f"{API}/core/items/{ITEM_UUID}/bundles", + status_code=500, text="boom") + out = c.create_bundle(parent=parent) + self.assertIsInstance(out, Bundle) + self.assertIsNone(out.uuid) + self.assertTrue(out) + class TestCreateItem(unittest.TestCase): @@ -114,6 +130,27 @@ def test_posts_with_owning_collection_param_and_returns_item(self): self.assertEqual(out.uuid, "newu") self.assertEqual(sent_params(m.last_request)["owningCollection"], [COLLECTION_UUID]) + # the POST body is item.as_dict() - this is how the importer's built + # metadata actually reaches DSpace, so pin it, not just the uuid. + body = m.last_request.json() + self.assertEqual(body["name"], "New thesis") + self.assertEqual(body["type"], "item") + self.assertEqual(body["metadata"], {}) + self.assertIs(body["inArchive"], True) + + def test_server_error_returns_truthy_uuidless_item(self): + # CHARACTERIZATION: create_item wraps the response unconditionally, so a + # failed create yields a truthy Item with uuid=None, NOT None. The + # importer guards with `if dso is None` (reposync/_importer.py:127-129), + # which therefore never fires on failure. Pinned; see the fail-safe note. + c = make_client() + item = Item({"name": "x", "metadata": {}}) + with requests_mock.Mocker() as m: + m.post(f"{API}/core/items", status_code=500, text="boom") + out = c.create_item(parent=COLLECTION_UUID, item=item) + self.assertIsInstance(out, Item) + self.assertIsNone(out.uuid) + self.assertTrue(out) # truthy despite the failure def test_non_item_returns_none(self): self.assertIsNone(make_client().create_item( @@ -145,9 +182,15 @@ def test_success_multipart_upload_returns_bitstream(self): self.assertIsInstance(bs, Bitstream) self.assertEqual(bs.uuid, "bsnew") self.assertEqual(bs.sizeBytes, 20) - # the request really was a multipart file upload + # the request really was a multipart file upload... self.assertIn("multipart/form-data", m.last_request.headers["Content-Type"]) + # ...carrying the name/bundleName/metadata that actually attach the + # bitstream's metadata in DSpace (reposync/_utils.create_new_bitstream) + props = multipart_properties(m.last_request) + self.assertEqual(props["name"], "a.pdf") + self.assertEqual(props["bundleName"], "ORIGINAL") # == bundle.name + self.assertEqual(props["metadata"], {"dc.title": [{"value": "a.pdf"}]}) def test_server_error_returns_none(self): c = make_client() diff --git a/tests/test_models.py b/tests/test_models.py index d18546d..2e06059 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -73,9 +73,12 @@ def test_bundle_fields_and_bitstreams_link(self): # get_bitstreams(bundle=...) prefers this embedded link over a manually # constructed URL, so it is part of the contract. b = Bundle({"uuid": "b1", "name": "ORIGINAL", "type": "bundle", - "metadata": {}, + "metadata": {"dc.title": [{"value": "ORIGINAL"}]}, "_links": {"bitstreams": {"href": "http://x/bundles/b1/bitstreams"}}}) self.assertEqual((b.uuid, b.name, b.type), ("b1", "ORIGINAL", "bundle")) + # .metadata is parsed from the response (unlike .type, a class constant) + # and is serialised by export/_dspace.py:323, so pin it. + self.assertEqual(b.metadata, {"dc.title": [{"value": "ORIGINAL"}]}) self.assertEqual(b.links["bitstreams"]["href"], "http://x/bundles/b1/bitstreams") @@ -90,6 +93,10 @@ def test_bitstream_file_fields(self): self.assertEqual(b.sizeBytes, 2048) self.assertEqual(b.sequenceId, 3) self.assertEqual(b.checkSum["value"], "deadbeef") + # the checksum verifier compares checkSumAlgorithm == "MD5" + # (reposync/_files.py:187-189); .metadata is serialised by the exporter. + self.assertEqual(b.checkSum["checkSumAlgorithm"], "MD5") + self.assertEqual(b.metadata, {"dc.title": [{"value": "f.pdf"}]}) d = b.as_dict() self.assertEqual(d["sizeBytes"], 2048) self.assertEqual(d["checkSum"]["value"], "deadbeef") diff --git a/tests/test_repo_usage_contract.py b/tests/test_repo_usage_contract.py index 460d708..55b57f6 100644 --- a/tests/test_repo_usage_contract.py +++ b/tests/test_repo_usage_contract.py @@ -15,9 +15,9 @@ import _helpers # noqa: F401 from _helpers import ( - make_client, embedded, item_json, bundle_json, bitstream_json, + make_client, sent_params, embedded, item_json, bundle_json, bitstream_json, policy_json, API, ITEM_UUID, BITSTREAM_UUID, ANON_GROUP_UUID) -from dspace_rest_client.models import Item +from dspace_rest_client.models import Item, Bundle class TestBitstreamExportChain(unittest.TestCase): @@ -33,15 +33,19 @@ def test_full_chain_yields_serialisable_attributes(self): c = make_client() item = Item(item_json(ITEM_UUID)) bits_href = f"{API}/core/bundles/{self.BUNDLE_UUID}/bitstreams" + bundle_md = {"dc.title": [{"value": "ORIGINAL"}]} + bs_md = {"dc.description": [{"value": "VŠKP"}]} with requests_mock.Mocker() as m: m.get(f"{API}/core/items/{ITEM_UUID}/bundles", json=embedded("bundles", [bundle_json(self.BUNDLE_UUID, "ORIGINAL", - bitstreams_href=bits_href)])) + bitstreams_href=bits_href, + metadata=bundle_md)])) m.get(bits_href, json=embedded("bitstreams", [bitstream_json(BITSTREAM_UUID, "thesis.pdf", - size=123, seq=1, checksum="abc")])) + size=123, seq=1, checksum="abc", + metadata=bs_md)])) m.get(f"{API}/authz/resourcepolicies/search/resource", json=embedded("resourcepolicies", [policy_json(pid=1)])) @@ -50,20 +54,26 @@ def test_full_chain_yields_serialisable_attributes(self): bundle = bundles[0] self.assertEqual((bundle.name, bundle.uuid, bundle.type), ("ORIGINAL", self.BUNDLE_UUID, "bundle")) + self.assertEqual(bundle.metadata, bundle_md) - bundle_rp = c.get_resourcepolicy(bundle.uuid) + # the exporter calls get_resourcepolicy via a wrapper defaulting to + # action=None (ingest/_dspace.py), so NO action filter is sent. + bundle_rp = c.get_resourcepolicy(bundle.uuid, action=None) self.assertEqual([rp.as_dict()["groupName"] for rp in bundle_rp], ["Anonymous"]) + self.assertNotIn("action", sent_params(m.last_request)) bitstreams = c.get_bitstreams(bundle=bundle, size=1000) self.assertEqual(len(bitstreams), 1) b = bitstreams[0] - # the exporter reads exactly these off each bitstream + # a representative set of the attributes the exporter serialises + # (src/export/_dspace.py:333-340) - name/uuid/size/seq/checksum/meta self.assertEqual((b.name, b.uuid, b.sizeBytes, b.sequenceId), ("thesis.pdf", BITSTREAM_UUID, 123, 1)) self.assertEqual(b.checkSum["value"], "abc") + self.assertEqual(b.metadata, bs_md) - bs_rp = c.get_resourcepolicy(b.uuid) + bs_rp = c.get_resourcepolicy(b.uuid, action=None) self.assertEqual(bs_rp[0].as_dict()["groupUUID"], ANON_GROUP_UUID) @@ -104,7 +114,11 @@ class TestMcpBundleWalk(unittest.TestCase): def test_lookup_then_item_bundles_bitstreams(self): c = make_client() - bits_href = f"{API}/core/bundles/bnd/bitstreams" + # mcp/core.py drops _links (it round-trips bundles through as_dict), so + # get_bitstreams must use the manually-constructed fallback URL, not the + # embedded link. The link below is a decoy that must NOT be requested. + decoy_href = f"{API}/core/bundles/DECOY-LINK/bitstreams" + fallback = f"{API}/core/bundles/bnd/bitstreams" with requests_mock.Mocker() as m: m.get(f"{API}/discover/search/objects", json={"_embedded": { "searchResult": {"page": {"totalElements": 1}, @@ -115,8 +129,8 @@ def test_lookup_then_item_bundles_bitstreams(self): m.get(f"{API}/core/items/{ITEM_UUID}/bundles", json=embedded("bundles", [bundle_json("bnd", "ORIGINAL", - bitstreams_href=bits_href)])) - m.get(bits_href, + bitstreams_href=decoy_href)])) + m.get(fallback, json=embedded("bitstreams", [bitstream_json("bs1", "a.pdf")])) matches = c.search_objects(query="dc.identifier:42") @@ -130,8 +144,13 @@ def test_lookup_then_item_bundles_bitstreams(self): bundles = c.get_bundles(parent=parent, size=200) self.assertEqual(bundles[0].name, "ORIGINAL") - bitstreams = c.get_bitstreams(bundle=bundles[0], size=500) + # mirror mcp: rebuild the Bundle from as_dict() (which strips _links) + # so get_bitstreams takes the fallback-URL branch the consumer hits. + bstub = Bundle(bundles[0].as_dict()) + self.assertNotIn("bitstreams", bstub.links) + bitstreams = c.get_bitstreams(bundle=bstub, size=500) self.assertEqual(bitstreams[0].uuid, "bs1") + self.assertEqual(m.last_request.url.split("?")[0], fallback) class TestGroupSearchForUuidResolution(unittest.TestCase): From ea3e670df354ff07049a5e84749636378a3be2c8 Mon Sep 17 00:00:00 2001 From: jm Date: Mon, 17 Aug 2026 18:45:16 +0200 Subject: [PATCH 3/4] ci: run push builds only on dtq dtq-dev no longer exists, and we only care about dtq for now. PRs still trigger via the pull_request event. Co-Authored-By: Claude Opus 4.8 --- .github/workflows/tests.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 0179525..affd373 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -2,7 +2,7 @@ name: Tests on: push: - branches: [ dtq, dtq-dev, main ] + branches: [ dtq ] pull_request: permissions: From a2b714079c932cd78dd4b2ca94f9094f4eb7081a Mon Sep 17 00:00:00 2001 From: jm Date: Mon, 17 Aug 2026 18:54:48 +0200 Subject: [PATCH 4/4] fix(client): fail-safe get_bitstreams + None on failed create_item/bundle Review follow-ups, each confirmed by the tests: - get_bitstreams: a 404 now returns [] (a gone bundle has no bitstreams), mirroring the get_bundles #16 fix; other errors still surface so a transient 5xx is retried, not swallowed; a 200 with no bitstreams returns [] not None. The consumers (export/_dspace, reposync/_files) iterate the result unguarded. - create_item / create_bundle: return None on a non-2xx response instead of a truthy uuid-less object, so the importer's `if dso is None` / `if not bundle` guards actually fire. Also addresses the Copilot review: the multipart_properties test helper now asserts the 'properties' part exists (fails loudly) instead of returning None. Tests updated to assert the new behavior. 62 tests, no network. Co-Authored-By: Claude Opus 4.8 --- dspace_rest_client/client.py | 33 +++++++++++++++++++++++++-------- tests/_helpers.py | 5 ++++- tests/test_client_read.py | 30 +++++++++++++++++++++++------- tests/test_client_write.py | 26 ++++++++------------------ 4 files changed, 60 insertions(+), 34 deletions(-) diff --git a/dspace_rest_client/client.py b/dspace_rest_client/client.py index 31166cc..7049228 100644 --- a/dspace_rest_client/client.py +++ b/dspace_rest_client/client.py @@ -764,7 +764,13 @@ def create_bundle(self, parent=None, name='ORIGINAL'): if parent is None: return None url = f'{self.API_ENDPOINT}/core/items/{parent.uuid}/bundles' - return Bundle(api_resource=parse_json(self.api_post(url, params=None, json={'name': name, 'metadata': {}}))) + r = self.api_post(url, params=None, json={'name': name, 'metadata': {}}) + if r.status_code not in (200, 201): + # return None on failure (not a uuid-less Bundle) so callers' + # `if not bundle` guards actually fire + _logger.error(f'Failed to create bundle: {r.status_code}: {r.text}') + return None + return Bundle(api_resource=parse_json(r)) # PAGINATION def get_bitstreams(self, uuid=None, bundle=None, page=0, size=20, sort=None): @@ -794,12 +800,18 @@ def get_bitstreams(self, uuid=None, bundle=None, page=0, size=20, sort=None): if sort is not None: params['sort'] = sort r_json = self.fetch_resource(url, params=params) - if '_embedded' in r_json: - if 'bitstreams' in r_json['_embedded']: - bitstreams = list() - for bitstream_resource in r_json['_embedded']['bitstreams']: - bitstreams.append(Bitstream(bitstream_resource)) - return bitstreams + if r_json is None and getattr(self._last_err, 'status_code', None) == 404: + # the bundle (or item) is gone - no bitstreams, a clean empty result + # rather than a crash. Mirrors get_bundles (#16). Any other failure + # (a transient 5xx, say) falls through and still surfaces to the + # caller so it is retried, not silently recorded as "no bitstreams". + _logger.info(f'No bitstreams: resource not found (404) [{url}]') + return list() + bitstreams = list() + if '_embedded' in r_json and 'bitstreams' in r_json['_embedded']: + for bitstream_resource in r_json['_embedded']['bitstreams']: + bitstreams.append(Bitstream(bitstream_resource)) + return bitstreams def create_bitstream(self, bundle=None, name=None, path=None, mime=None, metadata=None, retry=False): """ @@ -1085,7 +1097,12 @@ def create_item(self, parent, item): if not isinstance(item, Item): _logger.error('Need a valid item') return None - return Item(api_resource=parse_json(self.create_dso(url, params=params, data=item.as_dict()))) + r = self.create_dso(url, params=params, data=item.as_dict()) + if r is None or r.status_code != 201: + # return None on failure (not a uuid-less Item) so callers' + # `if dso is None` guards actually fire + return None + return Item(api_resource=parse_json(r)) def update_item(self, item): """ diff --git a/tests/_helpers.py b/tests/_helpers.py index 6fdbecf..d4183cc 100644 --- a/tests/_helpers.py +++ b/tests/_helpers.py @@ -66,7 +66,10 @@ def multipart_properties(request) -> dict: body = body.decode("utf-8", "replace") m = re.search(r'name="properties"\r?\n\r?\n(.*?);application/json', body, re.DOTALL) - return json.loads(m.group(1)) if m else None + # fail the test loudly rather than returning None and deferring the error + assert m is not None, \ + "create_bitstream multipart body has no JSON 'properties' part" + return json.loads(m.group(1)) # --- response-body builders (shape mirrors the DSpace 7 REST API) --------- # diff --git a/tests/test_client_read.py b/tests/test_client_read.py index ae08c4c..2ddceb7 100644 --- a/tests/test_client_read.py +++ b/tests/test_client_read.py @@ -171,13 +171,19 @@ def test_no_args_returns_empty_list(self): self.assertEqual(c.get_bitstreams(), []) self.assertEqual(m.call_count, 0) - def test_non_200_currently_raises_no_failsafe(self): - # CHARACTERIZATION of a known sharp edge: unlike get_bundles (which since - # PR #16 returns [] on a 404), get_bitstreams has no fail-safe - a non-200 - # makes fetch_resource return None which this method then subscripts, so - # it raises. The consumers (export/_dspace, reposync/_files) iterate the - # result unguarded, so this is a real crash risk. Pinned deliberately: if - # the library is hardened to return [], update this test to assert that. + def test_deleted_bundle_404_returns_empty_list(self): + # 404 -> [] fail-safe, mirroring get_bundles (#16): a gone bundle simply + # has no bitstreams, which is a clean empty result, not a crash. + c = make_client() + bundle = Bundle(bundle_json("bnd2")) + with requests_mock.Mocker() as m: + m.get(f"{API}/core/bundles/bnd2/bitstreams", + status_code=404, json={"timestamp": "2026-01-01"}) + self.assertEqual(c.get_bitstreams(bundle=bundle), []) + + def test_non_404_error_still_surfaces(self): + # a transient 5xx must NOT masquerade as "no bitstreams"; it surfaces so + # the caller can retry, exactly as get_bundles does for non-404 errors. c = make_client() bundle = Bundle(bundle_json("bnd2")) with requests_mock.Mocker() as m: @@ -186,6 +192,16 @@ def test_non_200_currently_raises_no_failsafe(self): with self.assertRaises(Exception): c.get_bitstreams(bundle=bundle) + def test_200_without_bitstreams_returns_empty_list(self): + # a well-formed response with no bitstreams -> [] (not None), so callers + # can iterate the result unconditionally. + c = make_client() + bundle = Bundle(bundle_json("bnd2")) + with requests_mock.Mocker() as m: + m.get(f"{API}/core/bundles/bnd2/bitstreams", + json={"page": {"totalElements": 0}}) + self.assertEqual(c.get_bitstreams(bundle=bundle), []) + class TestGetCollections(unittest.TestCase): diff --git a/tests/test_client_write.py b/tests/test_client_write.py index ee3922e..28231e6 100644 --- a/tests/test_client_write.py +++ b/tests/test_client_write.py @@ -101,20 +101,15 @@ def test_posts_to_item_bundles_and_returns_bundle(self): def test_none_parent_returns_none(self): self.assertIsNone(make_client().create_bundle(parent=None)) - def test_server_error_returns_truthy_uuidless_bundle(self): - # CHARACTERIZATION: like create_item, create_bundle wraps the response - # unconditionally -> a truthy Bundle with uuid=None on failure, not None. - # The importer's `if not bundle` guard (reposync/_importer.py:118-120) - # never fires because a Bundle instance is always truthy. Pinned. + def test_server_error_returns_none(self): + # a failed create returns None (not a uuid-less Bundle), so the + # importer's `if not bundle` guard fires correctly. c = make_client() parent = Item(item_json(ITEM_UUID)) with requests_mock.Mocker() as m: m.post(f"{API}/core/items/{ITEM_UUID}/bundles", status_code=500, text="boom") - out = c.create_bundle(parent=parent) - self.assertIsInstance(out, Bundle) - self.assertIsNone(out.uuid) - self.assertTrue(out) + self.assertIsNone(c.create_bundle(parent=parent)) class TestCreateItem(unittest.TestCase): @@ -138,19 +133,14 @@ def test_posts_with_owning_collection_param_and_returns_item(self): self.assertEqual(body["metadata"], {}) self.assertIs(body["inArchive"], True) - def test_server_error_returns_truthy_uuidless_item(self): - # CHARACTERIZATION: create_item wraps the response unconditionally, so a - # failed create yields a truthy Item with uuid=None, NOT None. The - # importer guards with `if dso is None` (reposync/_importer.py:127-129), - # which therefore never fires on failure. Pinned; see the fail-safe note. + def test_server_error_returns_none(self): + # a failed create returns None (not a uuid-less Item), so the importer's + # `if dso is None` guard (reposync/_importer.py:127-129) fires correctly. c = make_client() item = Item({"name": "x", "metadata": {}}) with requests_mock.Mocker() as m: m.post(f"{API}/core/items", status_code=500, text="boom") - out = c.create_item(parent=COLLECTION_UUID, item=item) - self.assertIsInstance(out, Item) - self.assertIsNone(out.uuid) - self.assertTrue(out) # truthy despite the failure + self.assertIsNone(c.create_item(parent=COLLECTION_UUID, item=item)) def test_non_item_returns_none(self): self.assertIsNone(make_client().create_item(