From 4049e9466deacff37afcf958275e478b5038c794 Mon Sep 17 00:00:00 2001 From: jm Date: Mon, 17 Aug 2026 21:11:01 +0200 Subject: [PATCH] fix(client): raise informative error on non-404 fetch failure get_bundles and get_bitstreams already return [] on a 404 (deleted item or bundle). On any other failure fetch_resource returns None and the code then subscripted it, surfacing an opaque 'NoneType is not subscriptable' TypeError with no status or url. Raise an explicit RuntimeError carrying the HTTP status and url instead, so a transient 5xx is a legible, retryable error. Addresses a Copilot review note; tests assert the status/url is in the message. Co-Authored-By: Claude Opus 4.8 --- dspace_rest_client/client.py | 34 +++++++++++++++++++++------------- tests/test_client_read.py | 22 ++++++++++++++++++---- 2 files changed, 39 insertions(+), 17 deletions(-) diff --git a/dspace_rest_client/client.py b/dspace_rest_client/client.py index 7049228..c298378 100644 --- a/dspace_rest_client/client.py +++ b/dspace_rest_client/client.py @@ -733,12 +733,17 @@ def get_bundles(self, parent=None, uuid=None, page=0, size=20, sort=None): if sort is not None: params['sort'] = sort r_json = self.fetch_resource(url, params=params) - if r_json is None and getattr(self._last_err, 'status_code', None) == 404: - # the item (or bundle) no longer exists - a deleted item simply has - # no bundles, which is a clean empty result, not a crash. any other - # failure falls through and still surfaces to the caller. - _logger.info(f'No bundles: resource not found (404) [{url}]') - return bundles + if r_json is None: + status = getattr(self._last_err, 'status_code', None) + if status == 404: + # a deleted item (or bundle) simply has no bundles, which is a + # clean empty result, not a crash. + _logger.info(f'No bundles: resource not found (404) [{url}]') + return bundles + # any other failure surfaces with its status + url, not as an opaque + # 'NoneType is not subscriptable' further down, so the caller can + # see what failed and retry. + raise RuntimeError(f'Failed to fetch bundles: HTTP {status} [{url}]') try: if single_result: bundles.append(Bundle(r_json)) @@ -800,13 +805,16 @@ 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 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() + if r_json is None: + status = getattr(self._last_err, 'status_code', None) + if status == 404: + # the bundle (or item) is gone - no bitstreams, a clean empty + # result rather than a crash. Mirrors get_bundles. + _logger.info(f'No bitstreams: resource not found (404) [{url}]') + return list() + # a transient 5xx must NOT masquerade as "no bitstreams"; surface it + # with status + url so the caller can retry, not an opaque TypeError. + raise RuntimeError(f'Failed to fetch bitstreams: HTTP {status} [{url}]') bitstreams = list() if '_embedded' in r_json and 'bitstreams' in r_json['_embedded']: for bitstream_resource in r_json['_embedded']['bitstreams']: diff --git a/tests/test_client_read.py b/tests/test_client_read.py index 2ddceb7..f48b04a 100644 --- a/tests/test_client_read.py +++ b/tests/test_client_read.py @@ -128,6 +128,18 @@ def test_deleted_item_404_returns_empty_list(self): json={"timestamp": "2026-01-01"}) self.assertEqual(c.get_bundles(parent=parent), []) + def test_non_404_error_raises_informative_error(self): + # a non-404 fetch failure surfaces with its status + url so the caller + # can retry, rather than an opaque 'NoneType is not subscriptable'. + 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=500, text="boom") + with self.assertRaises(RuntimeError) as ctx: + c.get_bundles(parent=parent) + self.assertIn("500", str(ctx.exception)) + def test_no_args_returns_empty_without_request(self): c = make_client() with requests_mock.Mocker() as m: @@ -181,16 +193,18 @@ def test_deleted_bundle_404_returns_empty_list(self): 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. + def test_non_404_error_raises_informative_error(self): + # a transient 5xx must NOT masquerade as "no bitstreams"; it surfaces + # with its status + url (not a bare TypeError) so the caller can retry. 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): + with self.assertRaises(RuntimeError) as ctx: c.get_bitstreams(bundle=bundle) + self.assertIn("500", str(ctx.exception)) + self.assertIn("/core/bundles/bnd2/bitstreams", str(ctx.exception)) def test_200_without_bitstreams_returns_empty_list(self): # a well-formed response with no bitstreams -> [] (not None), so callers