From 8bb9e2a76323e23b92921160bd34d9d13b386151 Mon Sep 17 00:00:00 2001 From: Tilo Mathes Date: Mon, 13 Jul 2026 18:32:31 +0200 Subject: [PATCH 1/3] Improve Gallery upload error on media-type section mismatch (RSDEV-1192) When a file is uploaded to a Gallery folder in the wrong media-type section, the RSpace API rejects it and the client raised an opaque ApiError. Re-raise as GallerySectionMismatch naming the folder's section (and the file's guessed media type) with the original error preserved. Detection is reactive: the enriched error is only produced after a real server rejection of a folder-targeted upload, so the best-effort filename classifier can never block an otherwise-valid upload. Co-Authored-By: Claude Opus 4.8 --- rspace_client/eln/fs.py | 115 ++++++++++++++++++++++++++++- rspace_client/tests/eln_fs_test.py | 60 ++++++++++++++- 2 files changed, 173 insertions(+), 2 deletions(-) diff --git a/rspace_client/eln/fs.py b/rspace_client/eln/fs.py index 436b9e3..d840d46 100644 --- a/rspace_client/eln/fs.py +++ b/rspace_client/eln/fs.py @@ -1,5 +1,6 @@ from fs.base import FS from rspace_client.eln import eln +from rspace_client.client_base import ClientBase from typing import Optional, List, Text, BinaryIO, Mapping, Any from fs.info import Info from fs.permissions import Permissions @@ -9,6 +10,71 @@ from io import BytesIO from ..fs_utils import path_to_id +# Best-effort mapping of file extension to the RSpace Gallery section that +# RSpace would classify it into. This mirrors the server's own classification +# but is NOT authoritative: it is used only to phrase error messages, never to +# decide whether an upload is allowed. Anything not listed falls through to the +# "Documents" catch-all, matching the server default. +_SECTION_BY_EXTENSION = { + # Images + "png": "Images", "jpg": "Images", "jpeg": "Images", "gif": "Images", + "tif": "Images", "tiff": "Images", "bmp": "Images", "svg": "Images", + "webp": "Images", "heic": "Images", + # Audios + "mp3": "Audios", "wav": "Audios", "flac": "Audios", "ogg": "Audios", + "m4a": "Audios", "aac": "Audios", "wma": "Audios", + # Videos + "mp4": "Videos", "mov": "Videos", "avi": "Videos", "mkv": "Videos", + "wmv": "Videos", "webm": "Videos", "m4v": "Videos", "flv": "Videos", + # Chemistry + "mol": "Chemistry", "mol2": "Chemistry", "rxn": "Chemistry", + "cdx": "Chemistry", "cdxml": "Chemistry", "smi": "Chemistry", + "sdf": "Chemistry", "cml": "Chemistry", +} + + +def classify_media_section(filename: Optional[Text]) -> Optional[Text]: + """ + Best-effort guess of the Gallery section for a filename, mirroring RSpace's + server-side classification. Returns a section name (e.g. "Images"), or + "Documents" as the catch-all when the extension is unrecognised, or None + when no filename/extension is available to guess from. + + The server remains the authority on placement; this is only used to make + error messages and log lines more helpful. + """ + if not filename or "." not in filename: + return None + ext = filename.rsplit(".", 1)[-1].lower() + return _SECTION_BY_EXTENSION.get(ext, "Documents") + + +def _filename_for(file: BinaryIO, options: Mapping[Text, Any]) -> Optional[Text]: + """Best-effort filename for a file object: an explicit ``filename`` option + wins, otherwise the file object's own ``name`` if it is a string.""" + name = options.get("filename") + if name: + return name + name = getattr(file, "name", None) + return name if isinstance(name, str) else None + + +class GallerySectionMismatch(ClientBase.ApiError): + """ + Raised when a file could not be uploaded into the requested Gallery folder + because that folder belongs to a media-type section that does not accept the + file. Carries the folder's section and, when it could be guessed, the file's + media type, so callers can react programmatically as well as read the message. + """ + + def __init__(self, message, *, folder_section=None, folder_global_id=None, + file_media_type=None, response_status_code=None): + super().__init__(message, response_status_code=response_status_code) + self.folder_section = folder_section + self.folder_global_id = folder_global_id + self.file_media_type = file_media_type + + def is_folder(path): return path.split('/')[-1][:2] == "GF" @@ -111,11 +177,58 @@ def download(self, path: Text, file: BinaryIO, chunk_size: Optional[int] = None, else: self.eln_client.download_file(path_to_id(path), file) + def _folder_section(self, folder_id: Text) -> Optional[Text]: + """The Gallery section (mediaType) a folder belongs to, or None if it + cannot be determined. Used to explain upload failures.""" + try: + return self.eln_client.get_folder(folder_id).get("mediaType") + except ClientBase.ApiError: + return None + def upload(self, path: Text, file: BinaryIO, chunk_size: Optional[int] = None, **options: Any) -> None: """ :param path: Global Id of a folder in the appropriate gallery section or else if empty then the upload will be placed in the Api Imports folder of the relevant gallery section :param file: a binary file object to be uploaded + + The RSpace Gallery is split into media-type sections (Images, Documents, + Chemistry, ...) and a file may only be placed in a folder whose section + matches the file's media type. If ``path`` names a folder in the wrong + section the upload is rejected; this method re-raises that as a + :class:`GallerySectionMismatch` naming the folder's section, instead of + an opaque API error. """ - self.eln_client.upload_file(file, path_to_id(path) if path else None) + folder_id = path_to_id(path) if path else None + try: + self.eln_client.upload_file(file, folder_id) + except ClientBase.ApiError as err: + if folder_id is None: + raise + section = self._folder_section(folder_id) + if section is None: + raise + filename = _filename_for(file, options) + guessed = classify_media_section(filename) + named = "'{}'".format(filename) if filename else "the file" + message = ( + "Could not upload {named} to Gallery folder {path}. That folder " + "is in the '{section}' section, which only accepts {section} " + "files".format(named=named, path=path, section=section) + ) + if guessed and guessed != section: + message += ", but {named} looks like a '{guessed}' file".format( + named=named, guessed=guessed + ) + message += ( + ". Upload it to a folder in the matching section, or omit the " + "folder path to let RSpace place it in the correct section " + "automatically. Original API error: {err}".format(err=err) + ) + raise GallerySectionMismatch( + message, + folder_section=section, + folder_global_id="GF" + str(folder_id), + file_media_type=guessed, + response_status_code=getattr(err, "response_status_code", None), + ) from err diff --git a/rspace_client/tests/eln_fs_test.py b/rspace_client/tests/eln_fs_test.py index 4c2f17e..703d3eb 100644 --- a/rspace_client/tests/eln_fs_test.py +++ b/rspace_client/tests/eln_fs_test.py @@ -1,8 +1,26 @@ from unittest.mock import patch, MagicMock, ANY import unittest -from rspace_client.eln.fs import path_to_id, GalleryFilesystem +from rspace_client.eln.fs import ( + path_to_id, + GalleryFilesystem, + GallerySectionMismatch, + classify_media_section, +) +from rspace_client.client_base import ClientBase from io import BytesIO + +def mock_failed_upload_post(url, *args, **kwargs): + """A /files upload that the server rejects (e.g. wrong Gallery section).""" + mock_response = MagicMock() + mock_response.status_code = 400 + mock_response.headers = {'Content-Type': 'application/json'} + mock_response.json.return_value = { + 'message': 'File type not allowed in this folder', 'errors': [] + } + mock_response.raise_for_status.side_effect = Exception('400 Bad Request') + return mock_response + def mock_requests_get(url, *args, **kwargs): mock_response = MagicMock() if url.endswith('/folders/tree'): @@ -180,5 +198,45 @@ def test_upload(self, mock_post): headers=ANY ) + def test_classify_media_section(self): + self.assertEqual('Images', classify_media_section('photo.png')) + self.assertEqual('Images', classify_media_section('photo.JPG')) + self.assertEqual('Videos', classify_media_section('clip.mp4')) + self.assertEqual('Chemistry', classify_media_section('reaction.mol')) + # unrecognised extensions fall through to the Documents catch-all + self.assertEqual('Documents', classify_media_section('report.pdf')) + self.assertEqual('Documents', classify_media_section('data.xyz')) + # no filename / no extension -> cannot guess + self.assertIsNone(classify_media_section('noextension')) + self.assertIsNone(classify_media_section(None)) + + @patch('requests.get', side_effect=mock_requests_get) + @patch('requests.post', side_effect=mock_failed_upload_post) + def test_upload_wrong_section_raises_mismatch(self, mock_post, mock_get): + # Folder GF123 is in the 'Images' section (see mock_requests_get); + # uploading a PDF there is rejected by the server. + file_obj = BytesIO(b'%PDF-1.4 fake') + file_obj.name = 'data.pdf' + with self.assertRaises(GallerySectionMismatch) as ctx: + self.fs.upload('/GF123', file_obj) + err = ctx.exception + self.assertEqual('Images', err.folder_section) + self.assertEqual('GF123', err.folder_global_id) + self.assertEqual('Documents', err.file_media_type) + self.assertIn('Images', str(err)) + self.assertIn('data.pdf', str(err)) + # the original server message is preserved + self.assertIn('File type not allowed', str(err)) + + @patch('requests.post', side_effect=mock_failed_upload_post) + def test_upload_no_folder_reraises_original(self, mock_post): + # With no target folder the server auto-routes; a failure here is not a + # section mismatch and must surface unchanged. + file_obj = BytesIO(b'x') + with self.assertRaises(ClientBase.ApiError) as ctx: + self.fs.upload('', file_obj) + self.assertNotIsInstance(ctx.exception, GallerySectionMismatch) + + if __name__ == '__main__': unittest.main() From 7e8e07e4ece6e4348307b7b9e513834f35fbd632 Mon Sep 17 00:00:00 2001 From: Tilo Mathes Date: Mon, 13 Jul 2026 18:37:26 +0200 Subject: [PATCH 2/3] Add opt-in auto-reroute for Gallery section mismatches (RSDEV-1192) Add an on_mismatch policy to GalleryFilesystem: "raise" (default, the clear error from the previous change) or "reroute", which on a section mismatch re-uploads with no folderId so the server places the file in the correct section's inbox. The policy is set filesystem-wide in the constructor (so it also applies to generic PyFilesystem operations that call upload/openbin) and can be overridden per upload() call. upload() now returns a Placement describing where the file landed (global id, section, human-readable path, and whether it was rerouted). Reroutes are logged at INFO. Document the behaviour in the usage guide. Co-Authored-By: Claude Opus 4.8 --- docs/usage-guide.md | 45 ++++++++++ rspace_client/eln/fs.py | 128 +++++++++++++++++++++++++++-- rspace_client/tests/eln_fs_test.py | 90 +++++++++++++++++++- 3 files changed, 254 insertions(+), 9 deletions(-) diff --git a/docs/usage-guide.md b/docs/usage-guide.md index 9e9bcce..7ebd0ac 100644 --- a/docs/usage-guide.md +++ b/docs/usage-guide.md @@ -45,6 +45,51 @@ for globalId in content: print(rspace_gallery_fs.getinfo(globalId).raw) ``` +#### Uploading into Gallery folders and the media-type section rule + +The RSpace Gallery is split into media-type sections (Images, Documents, +Chemistry, ...) and a file can only be placed in a folder whose section matches +the file's media type. Uploading, say, a PDF into a folder that lives in the +Images section is rejected by the server. + +By default `upload` turns that rejection into a clear `GallerySectionMismatch` +that names the folder's section, rather than an opaque API error: + +```python +from rspace_client.eln.fs import GallerySectionMismatch + +try: + rspace_gallery_fs.upload("/GF123", file_obj) # GF123 is in the Images section +except GallerySectionMismatch as e: + print(e) # explains the section clash + print(e.folder_section) # e.g. "Images" +``` + +Alternatively, opt in to automatic rerouting. On a mismatch the file is placed +in the correct section's inbox instead of failing. Set the policy once when +constructing the filesystem (it then applies to every write, including generic +PyFilesystem operations), or override it per call: + +```python +# filesystem-wide default +rspace_gallery_fs = fs.GalleryFilesystem(url, api_key, on_mismatch="reroute") + +# or per upload +placement = rspace_gallery_fs.upload("/GF123", file_obj, on_mismatch="reroute") +``` + +`upload` returns a `Placement` telling you where the file actually landed: + +```python +placement.rerouted # True if it did not go in the requested folder +placement.section # e.g. "Documents" +placement.path # e.g. "Gallery/Documents/Api Inbox" +placement.file_global_id # e.g. "GL999" +``` + +Note that rerouting places the file in the section's inbox, not a subfolder +matching the one you requested. + To access Inventory attachments construct a `InventoryAttachmentFilesystem` object. ```python from rspace_client.inv import fs diff --git a/rspace_client/eln/fs.py b/rspace_client/eln/fs.py index d840d46..2dd5849 100644 --- a/rspace_client/eln/fs.py +++ b/rspace_client/eln/fs.py @@ -1,3 +1,5 @@ +import logging +from dataclasses import dataclass from fs.base import FS from rspace_client.eln import eln from rspace_client.client_base import ClientBase @@ -10,6 +12,13 @@ from io import BytesIO from ..fs_utils import path_to_id +logger = logging.getLogger(__name__) + +# Accepted values for the GalleryFilesystem ``on_mismatch`` policy. +ON_MISMATCH_RAISE = "raise" +ON_MISMATCH_REROUTE = "reroute" +_ON_MISMATCH_VALUES = (ON_MISMATCH_RAISE, ON_MISMATCH_REROUTE) + # Best-effort mapping of file extension to the RSpace Gallery section that # RSpace would classify it into. This mirrors the server's own classification # but is NOT authoritative: it is used only to phrase error messages, never to @@ -75,6 +84,23 @@ def __init__(self, message, *, folder_section=None, folder_global_id=None, self.file_media_type = file_media_type +@dataclass +class Placement: + """Where an uploaded file actually ended up in the Gallery. + + Returned by :meth:`GalleryFilesystem.upload`. When ``rerouted`` is True the + file did not land in the folder named by ``requested_path`` (its section did + not accept the file) and was placed in the correct section instead; + ``path`` reports the human-readable location it ended up in. + """ + file_global_id: Optional[Text] + folder_global_id: Optional[Text] + section: Optional[Text] + path: Text + rerouted: bool + requested_path: Optional[Text] = None + + def is_folder(path): return path.split('/')[-1][:2] == "GF" @@ -87,8 +113,24 @@ def __init__(self, obj, *args, **kwargs) -> None: class GalleryFilesystem(FS): - def __init__(self, server: str, api_key: str) -> None: + def __init__(self, server: str, api_key: str, on_mismatch: str = ON_MISMATCH_RAISE) -> None: + """ + :param server: RSpace server URL + :param api_key: RSpace API key + :param on_mismatch: what to do when a file is uploaded to a folder whose + Gallery section does not accept it. ``"raise"`` (default) raises a + :class:`GallerySectionMismatch`; ``"reroute"`` instead places the + file in the correct section automatically and reports where it + landed. This is the filesystem-wide default and applies to every + write (including generic PyFilesystem operations); individual + :meth:`upload` calls may override it. + """ super(GalleryFilesystem, self).__init__() + if on_mismatch not in _ON_MISMATCH_VALUES: + raise ValueError( + "on_mismatch must be one of {}".format(_ON_MISMATCH_VALUES) + ) + self.on_mismatch = on_mismatch self.eln_client = eln.ELNClient(server, api_key) self.gallery_id = next(file['id'] for file in self.eln_client.list_folder_tree()['records'] if file['name'] == 'Gallery') @@ -185,23 +227,76 @@ def _folder_section(self, folder_id: Text) -> Optional[Text]: except ClientBase.ApiError: return None - def upload(self, path: Text, file: BinaryIO, chunk_size: Optional[int] = None, **options: Any) -> None: + def _human_path(self, folder: Mapping[Text, Any]) -> Text: + """Best-effort readable path for a folder, e.g. 'Gallery/Documents/Api + Inbox'. Uses the API's pathToRootFolder when present, otherwise falls + back to the section and folder name.""" + trail = folder.get("pathToRootFolder") + if isinstance(trail, list) and trail: + names = [f.get("name") for f in trail if f.get("name")] + if names: + return "/".join(names) + parts = ["Gallery"] + if folder.get("mediaType"): + parts.append(folder["mediaType"]) + if folder.get("name"): + parts.append(folder["name"]) + return "/".join(parts) + + def _placement(self, response: Mapping[Text, Any], requested_path: Optional[Text], + rerouted: bool) -> Placement: + """Build a Placement from an upload response, resolving the parent + folder for section/path feedback where possible.""" + parent_id = response.get("parentFolderId") + section = None + folder_global_id = None + path = "Gallery" + if parent_id is not None: + try: + folder = self.eln_client.get_folder(parent_id) + section = folder.get("mediaType") + folder_global_id = folder.get("globalId") + path = self._human_path(folder) + except ClientBase.ApiError: + pass + return Placement( + file_global_id=response.get("globalId"), + folder_global_id=folder_global_id, + section=section, + path=path, + rerouted=rerouted, + requested_path=requested_path, + ) + + def upload(self, path: Text, file: BinaryIO, chunk_size: Optional[int] = None, + on_mismatch: Optional[str] = None, **options: Any) -> Placement: """ :param path: Global Id of a folder in the appropriate gallery section or else if empty then the upload will be placed in the Api Imports folder of the relevant gallery section :param file: a binary file object to be uploaded + :param on_mismatch: optional override of the filesystem-wide policy for + this call ("raise" or "reroute"); defaults to the value + passed to the constructor. + :return: a :class:`Placement` describing where the file ended up. The RSpace Gallery is split into media-type sections (Images, Documents, Chemistry, ...) and a file may only be placed in a folder whose section matches the file's media type. If ``path`` names a folder in the wrong - section the upload is rejected; this method re-raises that as a - :class:`GallerySectionMismatch` naming the folder's section, instead of - an opaque API error. + section the upload is rejected. Depending on the effective policy this + either raises a :class:`GallerySectionMismatch` naming the folder's + section (``"raise"``) or places the file in the correct section's inbox + and returns a Placement with ``rerouted=True`` (``"reroute"``). """ + policy = on_mismatch if on_mismatch is not None else self.on_mismatch + if policy not in _ON_MISMATCH_VALUES: + raise ValueError( + "on_mismatch must be one of {}".format(_ON_MISMATCH_VALUES) + ) folder_id = path_to_id(path) if path else None try: - self.eln_client.upload_file(file, folder_id) + response = self.eln_client.upload_file(file, folder_id) + return self._placement(response, requested_path=path or None, rerouted=False) except ClientBase.ApiError as err: if folder_id is None: raise @@ -210,6 +305,22 @@ def upload(self, path: Text, file: BinaryIO, chunk_size: Optional[int] = None, * raise filename = _filename_for(file, options) guessed = classify_media_section(filename) + + if policy == ON_MISMATCH_REROUTE: + try: + file.seek(0) + except (AttributeError, OSError, ValueError): + pass + response = self.eln_client.upload_file(file, None) + placement = self._placement(response, requested_path=path, rerouted=True) + logger.info( + "RSpace Gallery: %s could not go in %s (section '%s'); " + "placed in %s instead", + "'{}'".format(filename) if filename else "file", + path, section, placement.path, + ) + return placement + named = "'{}'".format(filename) if filename else "the file" message = ( "Could not upload {named} to Gallery folder {path}. That folder " @@ -221,9 +332,10 @@ def upload(self, path: Text, file: BinaryIO, chunk_size: Optional[int] = None, * named=named, guessed=guessed ) message += ( - ". Upload it to a folder in the matching section, or omit the " + ". Upload it to a folder in the matching section, omit the " "folder path to let RSpace place it in the correct section " - "automatically. Original API error: {err}".format(err=err) + "automatically, or construct the filesystem with " + "on_mismatch='reroute'. Original API error: {err}".format(err=err) ) raise GallerySectionMismatch( message, diff --git a/rspace_client/tests/eln_fs_test.py b/rspace_client/tests/eln_fs_test.py index 703d3eb..5bd9811 100644 --- a/rspace_client/tests/eln_fs_test.py +++ b/rspace_client/tests/eln_fs_test.py @@ -21,6 +21,31 @@ def mock_failed_upload_post(url, *args, **kwargs): mock_response.raise_for_status.side_effect = Exception('400 Bad Request') return mock_response + +def _ok_file_response(parent_folder_id): + mock_response = MagicMock() + mock_response.status_code = 201 + mock_response.headers = {'Content-Type': 'application/json'} + mock_response.raise_for_status.return_value = None + mock_response.json.return_value = { + 'id': '999', 'globalId': 'GL999', 'name': 'data.pdf', + 'parentFolderId': parent_folder_id, + } + return mock_response + + +def mock_reroute_upload_post(url, *args, **kwargs): + """Rejects an upload that targets a folder (wrong section), but accepts the + retry with no folderId (server auto-routes to the correct section inbox).""" + if 'folderId' in kwargs.get('data', {}): + return mock_failed_upload_post(url, *args, **kwargs) + return _ok_file_response(555) + + +def mock_success_upload_post(url, *args, **kwargs): + """An upload the server accepts, landing in the requested folder (123).""" + return _ok_file_response(123) + def mock_requests_get(url, *args, **kwargs): mock_response = MagicMock() if url.endswith('/folders/tree'): @@ -57,6 +82,16 @@ def mock_requests_get(url, *args, **kwargs): 'pathToRootFolder': None, '_links': [{'link': 'http://localhost:8080/api/v1/folders/306', 'rel': 'self'}] } + elif url.endswith('/folders/555'): + mock_response.json.return_value = { + 'id': '555', + 'globalId': 'GF555', + 'name': 'Api Inbox', + 'size': 0, + 'notebook': False, + 'mediaType': 'Documents', + 'pathToRootFolder': None, + } elif url.endswith('/folders/456'): mock_response.json.return_value = { 'id': '456', @@ -187,7 +222,11 @@ def test_download(self, mock_get): @patch('requests.post') def test_upload(self, mock_post): mock_response = MagicMock() - mock_response.json.return_value = {'id': '456'} + mock_response.status_code = 201 + mock_response.headers = {'Content-Type': 'application/json'} + mock_response.raise_for_status.return_value = None + # no parentFolderId -> no follow-up folder lookup needed + mock_response.json.return_value = {'id': '456', 'globalId': 'GL456'} mock_post.return_value = mock_response file_obj = BytesIO(b'test file content') self.fs.upload('/GF123', file_obj) @@ -237,6 +276,55 @@ def test_upload_no_folder_reraises_original(self, mock_post): self.fs.upload('', file_obj) self.assertNotIsInstance(ctx.exception, GallerySectionMismatch) + @patch('requests.get', side_effect=mock_requests_get) + @patch('requests.post', side_effect=mock_success_upload_post) + def test_upload_success_returns_placement(self, mock_post, mock_get): + file_obj = BytesIO(b'x') + file_obj.name = 'a.pdf' + placement = self.fs.upload('/GF123', file_obj) + self.assertFalse(placement.rerouted) + self.assertEqual('GL999', placement.file_global_id) + self.assertEqual('Images', placement.section) + self.assertEqual('GF123', placement.folder_global_id) + self.assertEqual('/GF123', placement.requested_path) + + @patch('requests.get', side_effect=mock_requests_get) + @patch('requests.post', side_effect=mock_reroute_upload_post) + def test_upload_reroute_per_call_override(self, mock_post, mock_get): + # self.fs defaults to "raise"; override to "reroute" on the call. + file_obj = BytesIO(b'%PDF-1.4 fake') + file_obj.name = 'data.pdf' + placement = self.fs.upload('/GF123', file_obj, on_mismatch='reroute') + self.assertTrue(placement.rerouted) + self.assertEqual('GL999', placement.file_global_id) + self.assertEqual('Documents', placement.section) + self.assertEqual('GF555', placement.folder_global_id) + self.assertEqual('/GF123', placement.requested_path) + self.assertEqual('Gallery/Documents/Api Inbox', placement.path) + # first attempt targets the folder, retry drops folderId + self.assertEqual(2, mock_post.call_count) + self.assertEqual({'folderId': 123}, mock_post.call_args_list[0].kwargs['data']) + self.assertEqual({}, mock_post.call_args_list[1].kwargs['data']) + + @patch('requests.get', side_effect=mock_requests_get) + @patch('requests.post', side_effect=mock_reroute_upload_post) + def test_upload_reroute_constructor_policy(self, mock_post, mock_get): + reroute_fs = GalleryFilesystem('https://example.com', 'api_key', on_mismatch='reroute') + file_obj = BytesIO(b'%PDF-1.4 fake') + file_obj.name = 'data.pdf' + placement = reroute_fs.upload('/GF123', file_obj) + self.assertTrue(placement.rerouted) + self.assertEqual('Documents', placement.section) + + @patch('requests.get', side_effect=mock_requests_get) + def test_invalid_constructor_policy_rejected(self, mock_get): + with self.assertRaises(ValueError): + GalleryFilesystem('https://example.com', 'api_key', on_mismatch='bogus') + + def test_invalid_per_call_policy_rejected(self): + with self.assertRaises(ValueError): + self.fs.upload('/GF123', BytesIO(b'x'), on_mismatch='bogus') + if __name__ == '__main__': unittest.main() From 6cb1f600447ff5ec1a55c0544221de0a3e2791d4 Mon Sep 17 00:00:00 2001 From: Tilo Mathes Date: Mon, 13 Jul 2026 18:44:06 +0200 Subject: [PATCH 3/3] Keep Gallery upload working when upload_file return is discarded (RSDEV-1192) Galaxy's RSpace file source monkeypatches eln_client.upload_file with a wrapper that captures the response and returns None, then calls GalleryFilesystem.upload via super(). Change 2 started consuming upload_file's return value to build a Placement, which crashed on that None with AttributeError, breaking every Galaxy->RSpace export. Make _placement tolerate a non-dict/None response and make the best-effort folder-lookup helpers never raise, so a successful upload can never fail during Placement construction. Direct callers still receive a full Placement. Co-Authored-By: Claude Opus 4.8 --- rspace_client/eln/fs.py | 18 +++++++++++++----- rspace_client/tests/eln_fs_test.py | 11 +++++++++++ 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/rspace_client/eln/fs.py b/rspace_client/eln/fs.py index 2dd5849..293d5fc 100644 --- a/rspace_client/eln/fs.py +++ b/rspace_client/eln/fs.py @@ -221,10 +221,11 @@ def download(self, path: Text, file: BinaryIO, chunk_size: Optional[int] = None, def _folder_section(self, folder_id: Text) -> Optional[Text]: """The Gallery section (mediaType) a folder belongs to, or None if it - cannot be determined. Used to explain upload failures.""" + cannot be determined. Best-effort: never raises, so it cannot mask the + real outcome of an upload.""" try: return self.eln_client.get_folder(folder_id).get("mediaType") - except ClientBase.ApiError: + except Exception: return None def _human_path(self, folder: Mapping[Text, Any]) -> Text: @@ -243,10 +244,17 @@ def _human_path(self, folder: Mapping[Text, Any]) -> Text: parts.append(folder["name"]) return "/".join(parts) - def _placement(self, response: Mapping[Text, Any], requested_path: Optional[Text], + def _placement(self, response: Any, requested_path: Optional[Text], rerouted: bool) -> Placement: """Build a Placement from an upload response, resolving the parent - folder for section/path feedback where possible.""" + folder for section/path feedback where possible. + + Tolerates a response that is not a dict (e.g. None): some callers wrap + or replace ``eln_client.upload_file`` and discard its return value, so + Placement construction must never crash a successful upload. + """ + if not isinstance(response, dict): + response = {} parent_id = response.get("parentFolderId") section = None folder_global_id = None @@ -257,7 +265,7 @@ def _placement(self, response: Mapping[Text, Any], requested_path: Optional[Text section = folder.get("mediaType") folder_global_id = folder.get("globalId") path = self._human_path(folder) - except ClientBase.ApiError: + except Exception: pass return Placement( file_global_id=response.get("globalId"), diff --git a/rspace_client/tests/eln_fs_test.py b/rspace_client/tests/eln_fs_test.py index 5bd9811..f4a0cb4 100644 --- a/rspace_client/tests/eln_fs_test.py +++ b/rspace_client/tests/eln_fs_test.py @@ -316,6 +316,17 @@ def test_upload_reroute_constructor_policy(self, mock_post, mock_get): self.assertTrue(placement.rerouted) self.assertEqual('Documents', placement.section) + def test_upload_tolerates_upload_file_returning_none(self): + # Galaxy's file source monkeypatches eln_client.upload_file with a + # wrapper that captures the response and returns None. upload() must not + # crash while building the Placement in that case. + self.fs.eln_client.upload_file = MagicMock(return_value=None) + placement = self.fs.upload('/GF123', BytesIO(b'x')) + self.assertFalse(placement.rerouted) + self.assertIsNone(placement.file_global_id) + self.assertEqual('Gallery', placement.path) + self.fs.eln_client.upload_file.assert_called_once() + @patch('requests.get', side_effect=mock_requests_get) def test_invalid_constructor_policy_rejected(self, mock_get): with self.assertRaises(ValueError):