From 88c4c56ad2e7a81ffc61d0661d577a8129c3ce00 Mon Sep 17 00:00:00 2001 From: Juraj Roka <95219754+jr-rk@users.noreply.github.com> Date: Thu, 20 Aug 2026 10:35:26 +0200 Subject: [PATCH 1/2] fix: harden CLARIN shared surface + dtq owningCollection reauth Characterization-tested fixes (test written to the intended behaviour first, then the method corrected) to methods the main-lineage consumers call: get_bundle_by_name / get_items_from_collection / get_resource_policy non-200 or empty body -> clean None/[] instead of a NoneType subscript crash; get_resource_policy also records last_err so a caller can tell an HTTP error from a genuine empty result. get_user_by_email 404/any non-200 -> None (+ last_err), not a truthy uuid-less User that slips past a consumer `if user:` guard. create_submit_group 201 with an empty body -> None, not Group(None). add_member post the canonical /eperson/epersons/{uuid} href; the bare /epersons/ path 404s and DSpace rejects the uri-list. Group(None) / User(None) empty object, not a TypeError. get_owningCollection (dtq-only) non-200 -> None + last_err, so ISstag's src/repo/_audit.py `owning_col is None and last_err.status_code == 401` reauth retry stops being dead code. Stacked on the test/CI PR, which supplies the _helpers builders, pytest.ini markers and the differential-contract job these tests run under. Merge that PR first. The failure-mode assertions are marked dtq_only (deselected on the main leg): main carries the same bugs, but propagating these fixes to main is deliberately out of scope here. Co-Authored-By: Claude Opus 4.8 --- dspace_rest_client/client.py | 54 +++++++++++++++++++++---- dspace_rest_client/models.py | 2 + tests/test_clarin_read.py | 61 +++++++++++++++++++++++++++++ tests/test_clarin_usage_contract.py | 25 ++++++++++++ tests/test_clarin_write.py | 10 +++++ tests/test_models_clarin.py | 13 ++++++ tests/test_repo_usage_contract.py | 24 ++++++++++++ 7 files changed, 181 insertions(+), 8 deletions(-) diff --git a/dspace_rest_client/client.py b/dspace_rest_client/client.py index 8a53b57..90a26d4 100644 --- a/dspace_rest_client/client.py +++ b/dspace_rest_client/client.py @@ -1165,7 +1165,12 @@ def get_owningCollection(self, item_uuid): url = f'{self.API_ENDPOINT}/core/items/{item_uuid}/owningCollection' try: r = self.api_get(url, None, None) - self.verify_response(r, f"item:{item_uuid}", True) + # On a non-200, verify_response records self._last_err and returns + # False - return None here (not an empty, truthy Collection) so the + # caller's `owning_col is None and last_err.status_code == 401` + # reauth path (src/repo/_audit.py) actually fires. + if not self.verify_response(r, f"item:{item_uuid}", True): + return None r_json = parse_json(response=r) return Collection(r_json) except ValueError: @@ -1326,7 +1331,12 @@ def create_submit_group(self, collection): url = f'{self.API_ENDPOINT}/core/collections/{collection.uuid}/submittersGroup' r = self.api_post(url, json={}, params=None) if r.status_code == 201: - return Group(parse_json(r)) + # a 201 with an empty/invalid body would make Group(None) here; return + # None instead so a caller's `if not group` guard fires cleanly rather + # than passing a uuid-less Group into add_member() + j = parse_json(r) + if j: + return Group(j) return None def add_member(self, group, eperson): @@ -1349,7 +1359,9 @@ def add_member(self, group, eperson): return False url = f'{self.API_ENDPOINT}/eperson/groups/{group.uuid}/epersons' - eperson_uri = f'{self.API_ENDPOINT}/epersons/{eperson.uuid}' + # canonical eperson href is /eperson/epersons/{uuid}; a bare /epersons/ + # path does not resolve and DSpace rejects the uri-list with a 422 + eperson_uri = f'{self.API_ENDPOINT}/eperson/epersons/{eperson.uuid}' r = self.api_post_uri(url, params=None, uri_list=eperson_uri) if r.status_code == 204: return True @@ -1419,7 +1431,9 @@ def get_items_from_collection(self, collection_id, page=0, size=1000): items = list() r = self.api_get(url) r_json = parse_json(r) - if '_embedded' in r_json: + # a failed request parses to None; return the empty list rather than + # crashing on `'_embedded' in None` + if r_json and '_embedded' in r_json: if 'searchResult' in r_json['_embedded']: if '_embedded' in r_json['_embedded']['searchResult']: for item_resource in r_json['_embedded']['searchResult']['_embedded']['objects']: @@ -1436,7 +1450,9 @@ def get_bundle_by_name(self, name, item_uuid): """ url = f'{self.API_ENDPOINT}/core/items/{item_uuid}/bundles' r_json = self.fetch_resource(url, params=None) - if '_embedded' in r_json: + # fetch_resource returns None on any non-200 (records self._last_err); + # guard so a failed lookup is a clean None, not a NoneType subscript crash + if r_json and '_embedded' in r_json: if 'bundles' in r_json['_embedded']: for bundle in r_json['_embedded']['bundles']: if bundle['name'] == name: @@ -1449,10 +1465,20 @@ def get_resource_policy(self, bundle_uuid): """ url = f'{self.API_ENDPOINT}/authz/resourcepolicies/search/resource?uuid={bundle_uuid}&embed=eperson&embed=group' r = self.api_get(url) + # record the failing response so a caller can tell an HTTP error apart + # from a genuine "no policy" (empty list) result - both return None + if r.status_code != 200: + self._last_err = r + _logger.error(f'Error fetching resource policy [{bundle_uuid}]: {r.status_code}') + return None r_json = parse_json(r) - if '_embedded' in r_json: - if 'resourcepolicies' in r_json['_embedded']: - return r_json['_embedded']['resourcepolicies'][0] + # guard against an unparseable body and against an empty policy list - + # both are a clean None, not a TypeError/IndexError + if r_json and '_embedded' in r_json and 'resourcepolicies' in r_json['_embedded']: + policies = r_json['_embedded']['resourcepolicies'] + if policies: + return policies[0] + return None def create_resource_policy(self, resource_uuid, data, group_uuid=None, eperson_uuid=None): """ @@ -1544,7 +1570,19 @@ def get_user_by_email(self, email): params = {'email': email} try: response = self.api_get(url, params=params) + # a miss returns 404 (with a JSON error body): building a User from + # that yields a truthy, uuid-less object that passes `if user:` and + # fails much later. Treat any non-200 as "no such user" -> None, but + # record last_err so callers keep the HTTP-error diagnostics, and + # don't log a plain 404 miss at error level (nor the raw email). + if response.status_code != 200: + self._last_err = response + if response.status_code != 404: + _logger.error(f"Error retrieving user by email: HTTP {response.status_code}") + return None user_data = parse_json(response) + if not user_data: + return None return User(user_data) except Exception as e: _logger.error(f"Error retrieving user by email {email}: {e}") diff --git a/dspace_rest_client/models.py b/dspace_rest_client/models.py index ca8cb46..5c8e9a2 100644 --- a/dspace_rest_client/models.py +++ b/dspace_rest_client/models.py @@ -412,6 +412,7 @@ def __init__(self, api_resource=None): @param api_resource: API result object to use as initial data """ super().__init__(api_resource) + api_resource = api_resource or {} self.type = 'group' if 'name' in api_resource: self.name = api_resource['name'] @@ -447,6 +448,7 @@ def __init__(self, api_resource=None): @param api_resource: API result object to use as initial data """ super().__init__(api_resource) + api_resource = api_resource or {} self.type = 'user' if 'name' in api_resource: self.name = api_resource['name'] diff --git a/tests/test_clarin_read.py b/tests/test_clarin_read.py index 78663c4..d7fe79d 100644 --- a/tests/test_clarin_read.py +++ b/tests/test_clarin_read.py @@ -52,6 +52,26 @@ def test_sends_uuid_and_both_embeds(self): self.assertEqual(qs["uuid"], [BUNDLE_UUID]) self.assertEqual(sorted(qs["embed"]), ["eperson", "group"]) + @pytest.mark.dtq_only + def test_empty_list_returns_none(self): + """D2: an empty policy list must be a clean None, not an IndexError.""" + c = make_client() + with requests_mock.Mocker() as m: + m.get(self.URL, json=embedded("resourcepolicies", [])) + self.assertIsNone(c.get_resource_policy(BUNDLE_UUID)) + + @pytest.mark.dtq_only + def test_non_200_returns_none_and_records_last_err(self): + """D3: a failed request must be None, not a NoneType subscript crash - + and last_err is recorded so a caller can tell an HTTP error apart from a + genuine empty result (both return None).""" + c = make_client() + with requests_mock.Mocker() as m: + m.get(self.URL, status_code=500, text="upstream boom") + self.assertIsNone(c.get_resource_policy(BUNDLE_UUID)) + self.assertIsNotNone(c.last_err) + self.assertEqual(c.last_err.status_code, 500) + class TestGetBundleByName(unittest.TestCase): """Mirrors dspace-import-clarin - get_bundle_by_name('ORIGINAL', item).""" @@ -74,6 +94,14 @@ def test_no_match_returns_none(self): m.get(self.URL, json=embedded("bundles", [bundle_json("b1", "LICENSE")])) self.assertIsNone(c.get_bundle_by_name("ORIGINAL", ITEM_UUID)) + @pytest.mark.dtq_only + def test_non_200_returns_none(self): + """D1: a failed lookup must be None, not a NoneType subscript crash.""" + c = make_client() + with requests_mock.Mocker() as m: + m.get(self.URL, status_code=500, text="boom") + self.assertIsNone(c.get_bundle_by_name("ORIGINAL", ITEM_UUID)) + class TestGetItemsFromCollection(unittest.TestCase): """Mirrors dspace-import-clarin - get_items_from_collection(collection).""" @@ -102,6 +130,14 @@ def test_sends_scope_dsotype_sort_embed(self): self.assertEqual(qs["sort"], ["dc.date.accessioned,DESC"]) self.assertEqual(qs["embed"], ["thumbnail"]) + @pytest.mark.dtq_only + def test_non_200_returns_empty(self): + """D6: a failed request must yield [], not a NoneType subscript crash.""" + c = make_client() + with requests_mock.Mocker() as m: + m.get(self.URL, status_code=500, text="boom") + self.assertEqual(c.get_items_from_collection(COLLECTION_UUID), []) + class TestGetItemByHandle(unittest.TestCase): """Mirrors dspace-rest-test - get_item_by_handle(handle).""" @@ -150,6 +186,20 @@ def test_returns_user_with_uuid_and_email(self): self.assertEqual((u.uuid, u.email), (EPERSON_UUID, "a@b.c")) self.assertEqual(sent_params(m.last_request)["email"], ["a@b.c"]) + @pytest.mark.dtq_only + def test_404_returns_none(self): + """D4: a miss (404) must be falsy, not a truthy uuid-less User that + slips past the consumer's `if user:` guard.""" + c = make_client() + with requests_mock.Mocker() as m: + m.get(self.URL, status_code=404, json={"timestamp": "now"}) + u = c.get_user_by_email("nobody@nowhere") + self.assertIsNone(u) + self.assertFalse(bool(u)) + # the failing response is retained for callers, even for a 404 miss + self.assertIsNotNone(c.last_err) + self.assertEqual(c.last_err.status_code, 404) + class TestGetClarinAllowances(unittest.TestCase): """Mirrors dspace-rest-test - get_clarinlruallowances[_by_bitstream_and_user].""" @@ -197,6 +247,17 @@ def test_returns_typed_collection(self): self.assertIsInstance(col, Collection) self.assertEqual(col.uuid, COLLECTION_UUID) + def test_non_200_returns_none_and_sets_last_err(self): + """D8: on 401 the method must return None (not an empty truthy + Collection) and expose last_err, or _audit.py's reauth branch is dead.""" + c = make_client() + with requests_mock.Mocker() as m: + m.get(self.URL, status_code=401, json={"message": "Unauthorized"}) + col = c.get_owningCollection(ITEM_UUID) + self.assertIsNone(col) + self.assertIsNotNone(c.last_err) + self.assertEqual(c.last_err.status_code, 401) + if __name__ == "__main__": unittest.main() diff --git a/tests/test_clarin_usage_contract.py b/tests/test_clarin_usage_contract.py index 7dbd04e..9de1442 100644 --- a/tests/test_clarin_usage_contract.py +++ b/tests/test_clarin_usage_contract.py @@ -53,6 +53,16 @@ def test_bundle_then_policy_dict_then_group_update(self): self.assertEqual(r.status_code, 200) self.assertEqual(m.last_request.method, "PUT") + @pytest.mark.dtq_only + def test_missing_bundle_aborts_cleanly(self): + """D1: when the bundle lookup fails, get_bundle_by_name is None and the + chain stops - it must not crash on a NoneType subscript.""" + c = make_client() + with requests_mock.Mocker() as m: + m.get(f"{API}/core/items/{ITEM_UUID}/bundles", + status_code=500, text="boom") + self.assertIsNone(c.get_bundle_by_name("ORIGINAL", ITEM_UUID)) + class TestImportClarinLicenseIngest(unittest.TestCase): """Mirrors dspace-import-clarin - read a collection's items, then build the @@ -119,6 +129,21 @@ def test_email_group_member(self): self.assertTrue(c.add_member(group, user)) + @pytest.mark.dtq_only + def test_unknown_email_stops_before_add_member(self): + """D4: an unknown email must resolve to None so the consumer's + `if user:` guard skips group creation - not proceed with a uuid-less + User and fail deep inside add_member.""" + c = make_client() + with requests_mock.Mocker() as m: + m.get(f"{API}/eperson/epersons/search/byEmail", + status_code=404, json={"timestamp": "now"}) + + user = c.get_user_by_email("ghost@nowhere") + self.assertIsNone(user) + # consumer guard: nothing past the lookup should have been requested + self.assertEqual(len(m.request_history), 1) + class TestRestTestBitstreamPolicyFlow(unittest.TestCase): """Mirrors dspace-rest-test - resolve an item by handle, find its ORIGINAL diff --git a/tests/test_clarin_write.py b/tests/test_clarin_write.py index 5251f15..ecc606f 100644 --- a/tests/test_clarin_write.py +++ b/tests/test_clarin_write.py @@ -145,6 +145,16 @@ def test_non_201_returns_none(self): m.post(url, status_code=500, text="boom") self.assertIsNone(c.create_submit_group(self._collection())) + @pytest.mark.dtq_only + def test_empty_body_returns_none(self): + """D5: a 201 with an empty body must be None, not a Group(None) crash / + a uuid-less Group that would poison a following add_member call.""" + c = make_client() + url = f"{API}/core/collections/{COLLECTION_UUID}/submittersGroup" + with requests_mock.Mocker() as m: + m.post(url, status_code=201, text="") + self.assertIsNone(c.create_submit_group(self._collection())) + class TestAddMember(unittest.TestCase): """Mirrors dspace-rest-test - add_member(group, eperson).""" diff --git a/tests/test_models_clarin.py b/tests/test_models_clarin.py index 89f3d4c..04052e0 100644 --- a/tests/test_models_clarin.py +++ b/tests/test_models_clarin.py @@ -79,6 +79,13 @@ def test_fields_and_as_dict(self): self.assertEqual((g.uuid, g.name, g.permanent), (GROUP_UUID, "submitters", True)) self.assertEqual(g.as_dict()["name"], "submitters") + @pytest.mark.dtq_only + def test_from_none_does_not_crash(self): + """D7: Group(None) must yield an empty object, not a TypeError - the + create_submit_group / parse-failure paths can hand it None.""" + g = Group(None) + self.assertIsNone(g.name) + class TestUser(unittest.TestCase): """User is built by get_user_by_email / consumed by add_member.""" @@ -89,6 +96,12 @@ def test_fields_and_as_dict(self): self.assertTrue(u.canLogIn) self.assertEqual(u.as_dict()["email"], "a@b.c") + @pytest.mark.dtq_only + def test_from_none_does_not_crash(self): + """D7: User(None) must yield an empty object, not a TypeError.""" + u = User(None) + self.assertIsNone(u.email) + if __name__ == "__main__": unittest.main() diff --git a/tests/test_repo_usage_contract.py b/tests/test_repo_usage_contract.py index 55b57f6..ff26023 100644 --- a/tests/test_repo_usage_contract.py +++ b/tests/test_repo_usage_contract.py @@ -169,5 +169,29 @@ def test_group_search_shape(self): self.assertEqual(groups[0]["uuid"], "anon-uuid") +class TestOwningCollectionReauthContract(unittest.TestCase): + """Mirrors src/repo/_audit.py:105-111 - get_owningCollection, then the + `owning_col is None and last_err.status_code == 401` reauth decision. If the + method ever returns a truthy empty Collection on failure, that branch dies.""" + + def test_success_returns_collection(self): + c = make_client() + url = f"{API}/core/items/{ITEM_UUID}/owningCollection" + with requests_mock.Mocker() as m: + m.get(url, json={"uuid": "col-1", "name": "Coll", "type": "collection"}) + col = c.get_owningCollection(ITEM_UUID) + self.assertEqual(col.uuid, "col-1") + + def test_401_yields_none_and_last_err_drives_reauth(self): + c = make_client() + url = f"{API}/core/items/{ITEM_UUID}/owningCollection" + with requests_mock.Mocker() as m: + m.get(url, status_code=401, json={"message": "Unauthorized"}) + col = c.get_owningCollection(ITEM_UUID) + # the exact predicate _audit.py evaluates + self.assertTrue(col is None and c.last_err is not None + and c.last_err.status_code == 401) + + if __name__ == "__main__": unittest.main() From 4eaf68882ddf44dc4a231cd64973cb8ab43e2354 Mon Sep 17 00:00:00 2001 From: jm Date: Wed, 7 Oct 2026 09:24:31 +0200 Subject: [PATCH 2/2] fix: address review - error vs miss on CLARIN getters, uuid-less creates - get_items_from_collection: a failed request returned [], the consumers' end-of-collection signal, silently truncating their paging loops. It now goes through fetch_resource and returns None (last_err set on a non-200). - get_user_by_email: DSpace answers an unknown email with 204 No Content. 204 is the clean miss (None, no log, no last_err); 404 and any other non-user answer return None but log ERROR and keep last_err; the raw email is never logged. Transport errors propagate instead of looking like a miss. - create_group / create_user / create_submit_group: return None when the parsed body has no uuid, since Group(None)/User(None) are now accepted and would be truthy, uuid-less objects. - get_owningCollection: an unparseable 200 records last_err; a transport error returns None (logged) so one timeout does not abort a prefetch loop. - add_member: correct the comment - DSpace resolves the posted eperson by its trailing UUID, the bare /epersons/ href was not broken. --- dspace_rest_client/client.py | 83 ++++++++++++++++------------- tests/test_clarin_read.py | 66 ++++++++++++++++++----- tests/test_clarin_usage_contract.py | 4 +- tests/test_clarin_write.py | 40 +++++++++++++- 4 files changed, 138 insertions(+), 55 deletions(-) diff --git a/dspace_rest_client/client.py b/dspace_rest_client/client.py index 90a26d4..cb0cf6d 100644 --- a/dspace_rest_client/client.py +++ b/dspace_rest_client/client.py @@ -1165,17 +1165,19 @@ def get_owningCollection(self, item_uuid): url = f'{self.API_ENDPOINT}/core/items/{item_uuid}/owningCollection' try: r = self.api_get(url, None, None) - # On a non-200, verify_response records self._last_err and returns - # False - return None here (not an empty, truthy Collection) so the - # caller's `owning_col is None and last_err.status_code == 401` - # reauth path (src/repo/_audit.py) actually fires. - if not self.verify_response(r, f"item:{item_uuid}", True): - return None - r_json = parse_json(response=r) - return Collection(r_json) - except ValueError: - _logger.error(f'Invalid owningCollection for UUID: {item_uuid}') + except requests.RequestException as e: + # one timeout must not abort a caller's prefetch loop; there is no + # response to record, so last_err stays None + _logger.error(f'Error fetching owningCollection [item:{item_uuid}]: {e}') + return None + # Return None (not an empty, truthy Collection) with last_err set, so the + # caller's `owning_col is None and last_err.status_code == 401` reauth + # path (src/repo/_audit.py) fires. verify_response records last_err only + # for a non-200, so an unparseable 200 is recorded here. + if not self.verify_response(r, f"item:{item_uuid}", True): + self._last_err = r return None + return Collection(parse_json(response=r)) def create_item(self, parent, item): """ @@ -1283,7 +1285,11 @@ def create_user(self, user, token=None): params = None if token is not None: params = {'token': token} - return User(api_resource=parse_json(self.create_dso(url, params=params, data=data))) + r_json = parse_json(self.create_dso(url, params=params, data=data)) + # User(None) / User(error body) would be truthy and uuid-less + if not r_json or not r_json.get('uuid'): + return None + return User(api_resource=r_json) def delete_user(self, user): if not isinstance(user, User): @@ -1322,7 +1328,11 @@ def create_group(self, group): data = group.as_dict() # TODO: Validation. Note, at least here I will just allow a dict instead of the pointless cast<->cast # that you see for other DSO types - still figuring out the best way - return Group(api_resource=parse_json(self.create_dso(url, params=None, data=data))) + r_json = parse_json(self.create_dso(url, params=None, data=data)) + # Group(None) / Group(error body) would be truthy and uuid-less + if not r_json or not r_json.get('uuid'): + return None + return Group(api_resource=r_json) def create_submit_group(self, collection): """ @@ -1335,7 +1345,7 @@ def create_submit_group(self, collection): # None instead so a caller's `if not group` guard fires cleanly rather # than passing a uuid-less Group into add_member() j = parse_json(r) - if j: + if j and j.get('uuid'): return Group(j) return None @@ -1359,8 +1369,8 @@ def add_member(self, group, eperson): return False url = f'{self.API_ENDPOINT}/eperson/groups/{group.uuid}/epersons' - # canonical eperson href is /eperson/epersons/{uuid}; a bare /epersons/ - # path does not resolve and DSpace rejects the uri-list with a 422 + # canonical eperson href; DSpace resolves the uri-list by its trailing + # UUID only, so the former bare /epersons/{uuid} worked too eperson_uri = f'{self.API_ENDPOINT}/eperson/epersons/{eperson.uuid}' r = self.api_post_uri(url, params=None, uri_list=eperson_uri) if r.status_code == 204: @@ -1429,11 +1439,12 @@ def get_items_from_collection(self, collection_id, page=0, size=1000): url = f'{self.API_ENDPOINT}/discover/search/objects?sort=dc.date.accessioned,DESC&page={page}&size={size}&scope={collection_id}&dsoType=ITEM&embed=thumbnail' items = list() - r = self.api_get(url) - r_json = parse_json(r) - # a failed request parses to None; return the empty list rather than - # crashing on `'_embedded' in None` - if r_json and '_embedded' in r_json: + r_json = self.fetch_resource(url) + # [] is the callers' end-of-collection signal: a failed page must be + # None (with last_err on a non-200) or their paging loop silently stops + if r_json is None: + return None + if '_embedded' in r_json: if 'searchResult' in r_json['_embedded']: if '_embedded' in r_json['_embedded']['searchResult']: for item_resource in r_json['_embedded']['searchResult']['_embedded']['objects']: @@ -1568,22 +1579,18 @@ def get_user_by_email(self, email): """ url = f'{self.API_ENDPOINT}/eperson/epersons/search/byEmail' params = {'email': email} - try: - response = self.api_get(url, params=params) - # a miss returns 404 (with a JSON error body): building a User from - # that yields a truthy, uuid-less object that passes `if user:` and - # fails much later. Treat any non-200 as "no such user" -> None, but - # record last_err so callers keep the HTTP-error diagnostics, and - # don't log a plain 404 miss at error level (nor the raw email). - if response.status_code != 200: - self._last_err = response - if response.status_code != 404: - _logger.error(f"Error retrieving user by email: HTTP {response.status_code}") - return None - user_data = parse_json(response) - if not user_data: - return None - return User(user_data) - except Exception as e: - _logger.error(f"Error retrieving user by email {email}: {e}") + # a transport error (eg. a timeout) propagates: returning None would + # make it indistinguishable from "no such user" + response = self.api_get(url, params=params) + # DSpace answers an unknown email with 204 No Content: a clean miss + if response.status_code == 204: + return None + # anything else that is not a user (404 = misconfigured endpoint, + # 401/403/5xx, an unparseable or uuid-less body) is an error: None, but + # logged and kept in last_err. The raw email is never logged. + user_data = parse_json(response) if response.status_code == 200 else None + if not user_data or not user_data.get('uuid'): + self._last_err = response + _logger.error(f"Error retrieving user by email: HTTP {response.status_code}") return None + return User(user_data) diff --git a/tests/test_clarin_read.py b/tests/test_clarin_read.py index d7fe79d..fc3c377 100644 --- a/tests/test_clarin_read.py +++ b/tests/test_clarin_read.py @@ -14,11 +14,12 @@ import unittest import pytest +import requests import requests_mock import _helpers # noqa: F401 from _helpers import ( - make_client, sent_params, embedded, item_json, bundle_json, raw_policy_json, + make_client, sent_params, embedded, no_warnings_logged, item_json, bundle_json, raw_policy_json, user_json, clarin_allowance_json, search_envelope, API, ITEM_UUID, COLLECTION_UUID, BUNDLE_UUID, EPERSON_UUID) from dspace_rest_client.models import Bundle, Item, Collection, User @@ -131,12 +132,15 @@ def test_sends_scope_dsotype_sort_embed(self): self.assertEqual(qs["embed"], ["thumbnail"]) @pytest.mark.dtq_only - def test_non_200_returns_empty(self): - """D6: a failed request must yield [], not a NoneType subscript crash.""" + def test_non_200_returns_none_and_records_last_err(self): + """D6: a failed page must not look like an empty one - [] is the + consumers' end-of-collection signal, so an error would silently + truncate their paging loop. None + last_err tells the two apart.""" c = make_client() with requests_mock.Mocker() as m: m.get(self.URL, status_code=500, text="boom") - self.assertEqual(c.get_items_from_collection(COLLECTION_UUID), []) + self.assertIsNone(c.get_items_from_collection(COLLECTION_UUID)) + self.assertEqual(c.last_err.status_code, 500) class TestGetItemByHandle(unittest.TestCase): @@ -187,18 +191,38 @@ def test_returns_user_with_uuid_and_email(self): self.assertEqual(sent_params(m.last_request)["email"], ["a@b.c"]) @pytest.mark.dtq_only - def test_404_returns_none(self): - """D4: a miss (404) must be falsy, not a truthy uuid-less User that - slips past the consumer's `if user:` guard.""" + def test_204_no_content_is_a_clean_miss(self): + """D4: DSpace answers an unknown email with 204 No Content. That is a + clean miss - None, nothing logged, no last_err - not a truthy + uuid-less User that slips past the consumer's `if user:` guard.""" + c = make_client() + with requests_mock.Mocker() as m, no_warnings_logged(self, "dspace.client"): + m.get(self.URL, status_code=204) + self.assertIsNone(c.get_user_by_email("nobody@nowhere")) + self.assertIsNone(c.last_err) + + @pytest.mark.dtq_only + def test_error_status_returns_none_logs_error_and_records_last_err(self): + """A 404 means a misconfigured endpoint, 401/403/5xx a real failure: + None, but logged at ERROR (without the raw email) and kept in last_err + so it is distinguishable from the clean 204 miss.""" + for status in (404, 401, 403, 500): + with self.subTest(status=status): + c = make_client() + with requests_mock.Mocker() as m, self.assertLogs("dspace.client", "ERROR") as logs: + m.get(self.URL, status_code=status, json={"timestamp": "now"}) + self.assertIsNone(c.get_user_by_email("nobody@nowhere")) + self.assertEqual(c.last_err.status_code, status) + self.assertNotIn("nobody@nowhere", " ".join(logs.output)) + + @pytest.mark.dtq_only + def test_transport_error_is_not_a_clean_miss(self): + """A timeout must not return the clean-miss None: it propagates.""" c = make_client() with requests_mock.Mocker() as m: - m.get(self.URL, status_code=404, json={"timestamp": "now"}) - u = c.get_user_by_email("nobody@nowhere") - self.assertIsNone(u) - self.assertFalse(bool(u)) - # the failing response is retained for callers, even for a 404 miss - self.assertIsNotNone(c.last_err) - self.assertEqual(c.last_err.status_code, 404) + m.get(self.URL, exc=requests.exceptions.ConnectTimeout) + with self.assertRaises(requests.exceptions.ConnectTimeout): + c.get_user_by_email("nobody@nowhere") class TestGetClarinAllowances(unittest.TestCase): @@ -258,6 +282,20 @@ def test_non_200_returns_none_and_sets_last_err(self): self.assertIsNotNone(c.last_err) self.assertEqual(c.last_err.status_code, 401) + def test_unparseable_200_returns_none_and_sets_last_err(self): + c = make_client() + with requests_mock.Mocker() as m: + m.get(self.URL, status_code=200, text="proxy page") + self.assertIsNone(c.get_owningCollection(ITEM_UUID)) + self.assertEqual(c.last_err.status_code, 200) + + def test_transport_error_returns_none_without_raising(self): + """One timeout must not abort the consumer's prefetch loop.""" + c = make_client() + with requests_mock.Mocker() as m, self.assertLogs("dspace.client", "ERROR"): + m.get(self.URL, exc=requests.exceptions.ReadTimeout) + self.assertIsNone(c.get_owningCollection(ITEM_UUID)) + if __name__ == "__main__": unittest.main() diff --git a/tests/test_clarin_usage_contract.py b/tests/test_clarin_usage_contract.py index 9de1442..b34bea4 100644 --- a/tests/test_clarin_usage_contract.py +++ b/tests/test_clarin_usage_contract.py @@ -136,8 +136,8 @@ def test_unknown_email_stops_before_add_member(self): User and fail deep inside add_member.""" c = make_client() with requests_mock.Mocker() as m: - m.get(f"{API}/eperson/epersons/search/byEmail", - status_code=404, json={"timestamp": "now"}) + # DSpace answers an unknown email with 204 No Content + m.get(f"{API}/eperson/epersons/search/byEmail", status_code=204) user = c.get_user_by_email("ghost@nowhere") self.assertIsNone(user) diff --git a/tests/test_clarin_write.py b/tests/test_clarin_write.py index ecc606f..4768b58 100644 --- a/tests/test_clarin_write.py +++ b/tests/test_clarin_write.py @@ -13,7 +13,7 @@ import _helpers # noqa: F401 from _helpers import ( - make_client, sent_params, item_json, embedded, + make_client, sent_params, item_json, embedded, group_json, user_json, API, ITEM_UUID, BITSTREAM_UUID, COLLECTION_UUID, EPERSON_UUID, GROUP_UUID) from dspace_rest_client.models import Group, User, Collection, Item, DSpaceObject @@ -156,6 +156,44 @@ def test_empty_body_returns_none(self): self.assertIsNone(c.create_submit_group(self._collection())) + @pytest.mark.dtq_only + def test_201_without_uuid_returns_none(self): + """A parsed body without a uuid is not a usable group either.""" + c = make_client() + url = f"{API}/core/collections/{COLLECTION_UUID}/submittersGroup" + with requests_mock.Mocker() as m: + m.post(url, status_code=201, json={"type": "group"}) + self.assertIsNone(c.create_submit_group(self._collection())) + + +class TestCreateGroupAndUser(unittest.TestCase): + """Group(None)/User(None) are accepted, so a failed create must not hand + back a truthy uuid-less object that passes an `if group:` guard.""" + + CASES = (("create_group", "eperson/groups", Group, group_json), + ("create_user", "eperson/epersons", User, user_json)) + + def test_201_returns_typed_object(self): + for method, path, cls, body in self.CASES: + with self.subTest(method=method): + c = make_client() + with requests_mock.Mocker() as m: + m.post(f"{API}/{path}", status_code=201, json=body()) + obj = getattr(c, method)({"name": "x"}) + self.assertIsInstance(obj, cls) + self.assertIsNotNone(obj.uuid) + + @pytest.mark.dtq_only + def test_failed_create_returns_none(self): + for method, path, _cls, _body in self.CASES: + with self.subTest(method=method): + c = make_client() + with requests_mock.Mocker() as m: + m.post(f"{API}/{path}", status_code=422, + json={"message": "Unprocessable"}) + self.assertIsNone(getattr(c, method)({"name": "x"})) + + class TestAddMember(unittest.TestCase): """Mirrors dspace-rest-test - add_member(group, eperson)."""