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 <noreply@anthropic.com>

Author: jm

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):
         """

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) --------- #

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):
 

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(