Skip to content

[ENG-11516] Eliminate the request between metadata and download #490

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
cslzchen merged 4 commits into from
Jul 7, 2026
Merged

[ENG-11516] Eliminate the request between metadata and download #490

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4ef23be
pass storage meta to provider download method
antkryt Jul 3, 2026
f7e0d9d
fix tests
antkryt Jul 3, 2026
ae3a738
add comments and docstrings
antkryt Jul 7, 2026
181d7be
move sentry call and remove dead code
antkryt Jul 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 5 additions & 28 deletions tests/providers/osfstorage/test_provider.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand All @@ -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,
Expand All @@ -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)
Expand Down
7 changes: 5 additions & 2 deletions waterbutler/core/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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]),
)


Expand Down
77 changes: 59 additions & 18 deletions waterbutler/providers/osfstorage/provider.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -195,7 +197,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))

Expand All @@ -209,28 +211,46 @@ 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']}

# 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()
storage = None
use_embedded_storage = False
Comment thread
cslzchen marked this conversation as resolved.
if metadata is not None:
storage = metadata.raw.get('storage', None)
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),
})
sentry_sdk.capture_message(
'osfstorage download fell back to OSF /download endpoint',
level='info',
)

# 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, ),
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):
Expand Down Expand Up @@ -538,6 +558,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),
Expand All @@ -546,15 +572,30 @@ 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
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',
Expand Down
Loading