From 4ef23be5d91bbe45c724df5dd8b7db5570f05612 Mon Sep 17 00:00:00 2001 From: Anton Krytskyi Date: Fri, 3 Jul 2026 16:30:00 +0300 Subject: [PATCH 1/4] pass storage meta to provider download method --- waterbutler/core/utils.py | 7 +++- waterbutler/providers/osfstorage/provider.py | 44 ++++++++++++-------- 2 files changed, 32 insertions(+), 19 deletions(-) diff --git a/waterbutler/core/utils.py b/waterbutler/core/utils.py index d319d5b84..983e9b3b6 100644 --- a/waterbutler/core/utils.py +++ b/waterbutler/core/utils.py @@ -250,7 +250,10 @@ async def __anext__(self): else: return path.path.replace(self.parent_path.path, '', 1), EmptyStream() - return path.path.replace(self.parent_path.path, '', 1), await self.provider.download(path) + return ( + path.path.replace(self.parent_path.path, '', 1), + await self.provider.download(path, metadata=current[1]) + ) class ZipStreamGeneratorPaginated(BaseZipStreamGenerator): @@ -308,7 +311,7 @@ async def __anext__(self): return ( path.path.replace(self.parent_path.path, '', 1), - await self.provider.download(path), + await self.provider.download(path, metadata=current[1]), ) diff --git a/waterbutler/providers/osfstorage/provider.py b/waterbutler/providers/osfstorage/provider.py index 77b6e1215..b59e44713 100644 --- a/waterbutler/providers/osfstorage/provider.py +++ b/waterbutler/providers/osfstorage/provider.py @@ -195,7 +195,7 @@ def build_signed_url(method, url, data=None, params=None, ttl=100, **kwargs): return url, data, params - async def download(self, path, version=None, revision=None, mode=None, **kwargs): + async def download(self, path, version=None, revision=None, mode=None, metadata=None, **kwargs): if not path.identifier: raise exceptions.NotFoundError(str(path)) @@ -209,28 +209,37 @@ async def download(self, path, version=None, revision=None, mode=None, **kwargs) # version could be 0 here version = revision - # Capture user_id for analytics if user is logged in - user_param = {} - if self.auth.get('id', None): - user_param = {'user': self.auth['id']} + storage = None + if metadata is not None: + storage = metadata.raw.get('storage', None) - # osf storage metadata will return a virtual path within the provider - resp = await self.make_signed_request( - 'GET', - self.build_url(path.identifier, 'download', version=version, mode=mode), - expects=(200, ), - params=user_param, - throws=exceptions.DownloadError, - ) - data = await resp.json() + if version is None and storage and storage.get('data', None): + data = storage + else: + # Capture user_id for analytics if user is logged in + user_param = {} + if self.auth.get('id', None): + user_param = {'user': self.auth['id']} + + # osf storage metadata will return a virtual path within the provider + resp = await self.make_signed_request( + 'GET', + self.build_url(path.identifier, 'download', version=version, mode=mode), + expects=(200, ), + params=user_param, + throws=exceptions.DownloadError, + ) + data = await resp.json() provider_object = self.make_provider(data['settings']) - name = data['data'].pop('name') - data['data']['path'] = await provider_object.validate_path('/' + data['data']['path']) + file_data = dict(data['data']) + name = file_data.pop('name') + file_data['path'] = await provider_object.validate_path('/' + file_data['path']) download_kwargs = {} download_kwargs.update(kwargs) - download_kwargs.update(data['data']) + download_kwargs.update(file_data) download_kwargs['display_name'] = kwargs.get('display_name') or name + return await provider_object.download(**download_kwargs) async def upload(self, stream, path, **kwargs): @@ -549,6 +558,7 @@ async def _children_metadata(self, path, limit=None, after=None, **kwargs): query = { 'user_id': self.auth.get('id'), 'minimal': kwargs.get('minimal', False), + 'orm': kwargs.get('orm', False) } if limit is not None: query['orm'] = True From f7e0d9d68fbb2992cdc8f91d5b9f847049d681ee Mon Sep 17 00:00:00 2001 From: Anton Krytskyi Date: Fri, 3 Jul 2026 16:43:11 +0300 Subject: [PATCH 2/4] fix tests --- waterbutler/providers/osfstorage/provider.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/waterbutler/providers/osfstorage/provider.py b/waterbutler/providers/osfstorage/provider.py index b59e44713..d6efb6c9f 100644 --- a/waterbutler/providers/osfstorage/provider.py +++ b/waterbutler/providers/osfstorage/provider.py @@ -558,13 +558,14 @@ async def _children_metadata(self, path, limit=None, after=None, **kwargs): query = { 'user_id': self.auth.get('id'), 'minimal': kwargs.get('minimal', False), - 'orm': kwargs.get('orm', False) } if limit is not None: query['orm'] = True query['limit'] = limit if after is not None: query['after'] = after + elif kwargs.get('orm') is not None: + query['orm'] = kwargs['orm'] resp = await self.make_signed_request( 'GET', From ae3a738a1bcc2389742cf26f899ca1d95bcfd590 Mon Sep 17 00:00:00 2001 From: Anton Krytskyi Date: Tue, 7 Jul 2026 15:23:31 +0300 Subject: [PATCH 3/4] add comments and docstrings --- waterbutler/providers/osfstorage/provider.py | 35 ++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/waterbutler/providers/osfstorage/provider.py b/waterbutler/providers/osfstorage/provider.py index d6efb6c9f..a565b7940 100644 --- a/waterbutler/providers/osfstorage/provider.py +++ b/waterbutler/providers/osfstorage/provider.py @@ -6,6 +6,8 @@ import logging from http import HTTPStatus +import sentry_sdk + from waterbutler.core import utils from waterbutler.core import signing from waterbutler.core import streams @@ -213,9 +215,23 @@ async def download(self, path, version=None, revision=None, mode=None, metadata= if metadata is not None: storage = metadata.raw.get('storage', None) + # DAZ passes minimal child metadata with an embedded ``storage`` block so we can + # call the inner storage provider directly. When that payload is usable we skip + # the per-file OSF ``/download`` hop; otherwise we fall back to fetching it. if version is None and storage and storage.get('data', None): data = storage else: + if metadata is not None: + with sentry_sdk.push_scope() as scope: + scope.set_context('osfstorage_data', { + 'node': self.nid, + 'path': str(path), + }) + sentry_sdk.capture_message( + 'osfstorage download fell back to OSF /download endpoint', + level='info', + ) + # Capture user_id for analytics if user is logged in user_param = {} if self.auth.get('id', None): @@ -547,6 +563,12 @@ async def iter_children_pages(self, path, **kwargs): # ========== private ========== async def _item_metadata(self, path, revision=None): + """Fetch metadata for a single file from the OSF. + + :param path: file path whose ``identifier`` is the OSF file node id + :param revision: optional file version identifier passed to OSF as ``revision`` + :return: :class:`OsfStorageFileMetadata` for the file + """ resp = await self.make_signed_request( 'GET', self.build_url(path.identifier, revision=revision), @@ -555,10 +577,23 @@ async def _item_metadata(self, path, revision=None): return OsfStorageFileMetadata((await resp.json()), str(path)) async def _children_metadata(self, path, limit=None, after=None, **kwargs): + """Fetch folder children metadata from the OSF. + + :param path: folder path whose ``identifier`` is the OSF folder node id + :param limit: page size; when set, enables ORM pagination on OSF + :param after: cursor (child node pk) for the next ORM page + :return: list of :class:`OsfStorageFolderMetadata` and + :class:`OsfStorageFileMetadata` instances + """ query = { 'user_id': self.auth.get('id'), 'minimal': kwargs.get('minimal', False), } + + # By default OSF serves minimal children via raw SQL on the backend. The Django + # ORM implementation is used only when ``orm`` is explicitly requested or when + # ``limit`` is set (pagination requires ORM). The raw SQL path may be removed in + # the future in favor of ORM-only responses. if limit is not None: query['orm'] = True query['limit'] = limit From 181d7beabb4e2f96159d6885066e999283566b62 Mon Sep 17 00:00:00 2001 From: Anton Krytskyi Date: Tue, 7 Jul 2026 17:38:37 +0300 Subject: [PATCH 4/4] move sentry call and remove dead code --- tests/providers/osfstorage/test_provider.py | 33 +++----------------- waterbutler/providers/osfstorage/provider.py | 25 ++++++--------- 2 files changed, 15 insertions(+), 43 deletions(-) diff --git a/tests/providers/osfstorage/test_provider.py b/tests/providers/osfstorage/test_provider.py index a3d0117e1..a1a768ad2 100644 --- a/tests/providers/osfstorage/test_provider.py +++ b/tests/providers/osfstorage/test_provider.py @@ -70,13 +70,13 @@ class TestDownload: @pytest.mark.asyncio @pytest.mark.aiohttpretty - async def test_download_with_auth(self, provider_and_mock_one, download_response, - download_path, mock_time): + async def test_download(self, provider_and_mock_one, download_response, + download_path, mock_time): provider, inner_provider = provider_and_mock_one - uri, params = build_signed_url_with_auth(provider, 'GET', download_path.identifier, - 'download', version=None, mode=None) + uri, params = build_signed_url_without_auth(provider, 'GET', download_path.identifier, + 'download', version=None, mode=None) aiohttpretty.register_json_uri('GET', uri, body=download_response, params=params) @@ -92,29 +92,6 @@ async def test_download_with_auth(self, provider_and_mock_one, download_response inner_provider.download.assert_called_once_with(path=expected_path, display_name=expected_display_name) - @pytest.mark.asyncio - @pytest.mark.aiohttpretty - async def test_download_without_auth(self, provider_and_mock_one, download_response, - download_path, mock_time): - provider, inner_provider = provider_and_mock_one - - provider.auth = {} - url, params = build_signed_url_without_auth(provider, 'GET', download_path.identifier, - 'download', version=None, mode=None) - aiohttpretty.register_json_uri('GET', url, params=params, body=download_response) - - await provider.download(download_path) - - assert provider.make_provider.called - assert inner_provider.download.called - assert aiohttpretty.has_call(method='GET', uri=url, params=params) - provider.make_provider.assert_called_once_with(download_response['settings']) - - expected_path = WaterButlerPath('/' + download_response['data']['path']) - expected_display_name = download_response['data']['name'] - inner_provider.download.assert_called_once_with(path=expected_path, - display_name=expected_display_name) - @pytest.mark.asyncio @pytest.mark.aiohttpretty async def test_download_without_id(self, provider_one, download_response, file_path, @@ -141,7 +118,7 @@ async def test_download_with_display_name(self, provider_and_mock_one, download_ provider, inner_provider = provider_and_mock_one - uri, params = build_signed_url_with_auth(provider, 'GET', download_path.identifier, + uri, params = build_signed_url_without_auth(provider, 'GET', download_path.identifier, 'download', version=None, mode=None) aiohttpretty.register_json_uri('GET', uri, body=download_response, params=params) diff --git a/waterbutler/providers/osfstorage/provider.py b/waterbutler/providers/osfstorage/provider.py index a565b7940..39dd7b774 100644 --- a/waterbutler/providers/osfstorage/provider.py +++ b/waterbutler/providers/osfstorage/provider.py @@ -212,17 +212,12 @@ async def download(self, path, version=None, revision=None, mode=None, metadata= version = revision storage = None + use_embedded_storage = False if metadata is not None: storage = metadata.raw.get('storage', None) - - # DAZ passes minimal child metadata with an embedded ``storage`` block so we can - # call the inner storage provider directly. When that payload is usable we skip - # the per-file OSF ``/download`` hop; otherwise we fall back to fetching it. - if version is None and storage and storage.get('data', None): - data = storage - else: - if metadata is not None: - with sentry_sdk.push_scope() as scope: + use_embedded_storage = version is None and bool(storage and storage.get('data', None)) + if not use_embedded_storage: + with sentry_sdk.new_scope() as scope: scope.set_context('osfstorage_data', { 'node': self.nid, 'path': str(path), @@ -232,17 +227,17 @@ async def download(self, path, version=None, revision=None, mode=None, metadata= level='info', ) - # Capture user_id for analytics if user is logged in - user_param = {} - if self.auth.get('id', None): - user_param = {'user': self.auth['id']} - + # DAZ passes minimal child metadata with an embedded ``storage`` block so we can + # call the inner storage provider directly. When that payload is usable we skip + # the per-file OSF ``/download`` hop; otherwise we fall back to fetching it. + if use_embedded_storage: + data = storage + else: # osf storage metadata will return a virtual path within the provider resp = await self.make_signed_request( 'GET', self.build_url(path.identifier, 'download', version=version, mode=mode), expects=(200, ), - params=user_param, throws=exceptions.DownloadError, ) data = await resp.json()