Skip to content

Commit dc2e654

Browse files
jr-rkclaude
andcommitted
fix: record last_err on get_resource_policy / get_user_by_email failures
Address Copilot review: both methods returned None on an HTTP error without retaining the failing Response, so a caller could not tell a real HTTP error from a genuine "no policy" / "no such user" (both None). Record self._last_err on the non-200 path in each, matching the last_err bookkeeping the rest of this PR relies on. Also stop logging a plain 404 miss at error level in get_user_by_email, and drop the raw email address from the error message. Tests extended to assert last_err is set (and carries the status code) on both failure paths. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 6faffa8 commit dc2e654

2 files changed

Lines changed: 23 additions & 6 deletions

File tree

dspace_rest_client/client.py

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1396,9 +1396,15 @@ def get_resource_policy(self, bundle_uuid):
13961396
"""
13971397
url = f'{self.API_ENDPOINT}/authz/resourcepolicies/search/resource?uuid={bundle_uuid}&embed=eperson&embed=group'
13981398
r = self.api_get(url)
1399+
# record the failing response so a caller can tell an HTTP error apart
1400+
# from a genuine "no policy" (empty list) result - both return None
1401+
if r.status_code != 200:
1402+
self._last_err = r
1403+
_logger.error(f'Error fetching resource policy [{bundle_uuid}]: {r.status_code}')
1404+
return None
13991405
r_json = parse_json(r)
1400-
# guard against a failed request (r_json is None) and against an empty
1401-
# policy list - both must be a clean None, not a TypeError/IndexError
1406+
# guard against an unparseable body and against an empty policy list -
1407+
# both are a clean None, not a TypeError/IndexError
14021408
if r_json and '_embedded' in r_json and 'resourcepolicies' in r_json['_embedded']:
14031409
policies = r_json['_embedded']['resourcepolicies']
14041410
if policies:
@@ -1497,9 +1503,13 @@ def get_user_by_email(self, email):
14971503
response = self.api_get(url, params=params)
14981504
# a miss returns 404 (with a JSON error body): building a User from
14991505
# that yields a truthy, uuid-less object that passes `if user:` and
1500-
# fails much later. Treat any non-200 as "no such user" -> None.
1506+
# fails much later. Treat any non-200 as "no such user" -> None, but
1507+
# record last_err so callers keep the HTTP-error diagnostics, and
1508+
# don't log a plain 404 miss at error level (nor the raw email).
15011509
if response.status_code != 200:
1502-
_logger.error(f"No user for email {email}: {response.status_code}")
1510+
self._last_err = response
1511+
if response.status_code != 404:
1512+
_logger.error(f"Error retrieving user by email: HTTP {response.status_code}")
15031513
return None
15041514
user_data = parse_json(response)
15051515
if not user_data:

tests/test_clarin_read.py

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -61,12 +61,16 @@ def test_empty_list_returns_none(self):
6161
self.assertIsNone(c.get_resource_policy(BUNDLE_UUID))
6262

6363
@pytest.mark.dtq_only
64-
def test_non_200_returns_none(self):
65-
"""D3: a failed request must be None, not a NoneType subscript crash."""
64+
def test_non_200_returns_none_and_records_last_err(self):
65+
"""D3: a failed request must be None, not a NoneType subscript crash -
66+
and last_err is recorded so a caller can tell an HTTP error apart from a
67+
genuine empty result (both return None)."""
6668
c = make_client()
6769
with requests_mock.Mocker() as m:
6870
m.get(self.URL, status_code=500, text="upstream boom")
6971
self.assertIsNone(c.get_resource_policy(BUNDLE_UUID))
72+
self.assertIsNotNone(c.last_err)
73+
self.assertEqual(c.last_err.status_code, 500)
7074

7175

7276
class TestGetBundleByName(unittest.TestCase):
@@ -192,6 +196,9 @@ def test_404_returns_none(self):
192196
u = c.get_user_by_email("nobody@nowhere")
193197
self.assertIsNone(u)
194198
self.assertFalse(bool(u))
199+
# the failing response is retained for callers, even for a 404 miss
200+
self.assertIsNotNone(c.last_err)
201+
self.assertEqual(c.last_err.status_code, 404)
195202

196203

197204
class TestGetClarinAllowances(unittest.TestCase):

0 commit comments

Comments
 (0)