diff --git a/dspace_rest_client/client.py b/dspace_rest_client/client.py index 8a53b57..cb0cf6d 100644 --- a/dspace_rest_client/client.py +++ b/dspace_rest_client/client.py @@ -1165,12 +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) - self.verify_response(r, f"item:{item_uuid}", True) - 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): """ @@ -1278,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): @@ -1317,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): """ @@ -1326,7 +1341,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 and j.get('uuid'): + return Group(j) return None def add_member(self, group, eperson): @@ -1349,7 +1369,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; 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: return True @@ -1417,8 +1439,11 @@ 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) + 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']: @@ -1436,7 +1461,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 +1476,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): """ @@ -1542,10 +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) - user_data = parse_json(response) - 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/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..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 @@ -52,6 +53,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 +95,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 +131,17 @@ 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_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.assertIsNone(c.get_items_from_collection(COLLECTION_UUID)) + self.assertEqual(c.last_err.status_code, 500) + class TestGetItemByHandle(unittest.TestCase): """Mirrors dspace-rest-test - get_item_by_handle(handle).""" @@ -150,6 +190,40 @@ 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_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, exc=requests.exceptions.ConnectTimeout) + with self.assertRaises(requests.exceptions.ConnectTimeout): + c.get_user_by_email("nobody@nowhere") + class TestGetClarinAllowances(unittest.TestCase): """Mirrors dspace-rest-test - get_clarinlruallowances[_by_bitstream_and_user].""" @@ -197,6 +271,31 @@ 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) + + 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 7dbd04e..b34bea4 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: + # 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) + # 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..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 @@ -145,6 +145,54 @@ 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())) + + + @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).""" 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()