Skip to content

[ENG-11581] [ENG-11576] Update BE to use the best option by default #11809

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 1 commit into from
Jul 15, 2026
+38 −102
Merged

[ENG-11581] [ENG-11576] Update BE to use the best option by default #11809

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
74 changes: 30 additions & 44 deletions addons/osfstorage/tests/test_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,15 +155,13 @@ def test_metadata_not_found_lots_of_slashes(self):
@pytest.mark.django_db
class TestGetChildrenHook(HookTestCase):

def get_children(self, parent, target=None, *, orm=False, **query_params):
def get_children(self, parent, target=None, **query_params):
params = {
'fid': parent._id,
'user_id': self.user._id,
'minimal': 'true',
**query_params,
}
if orm:
params['orm'] = 'true'
return self.send_hook(
'osfstorage_get_children',
params,
Expand Down Expand Up @@ -258,44 +256,33 @@ def test_children_metadata_preprint(self):
assert res_date_created == expected_date_created

def test_minimal_child_fields(self):
for orm in (False, True):
with self.subTest(orm='orm' if orm else 'sql'):
parent = self.node_settings.get_root().append_folder(
f'minimal-child-fields-{"orm" if orm else "sql"}'
)
record = self._create_child_file(parent, 'magíc.mp3')
folder = parent.append_folder('nested')
res = self.get_children(parent, orm=orm)
assert res.status_code == 200
assert len(res.json) == 2
assert {item['name'] for item in res.json} == {record.name, folder.name}
expected_keys = {'kind', 'name', 'path', 'storage'}
if orm:
expected_keys.add('id')
for item in res.json:
assert set(item.keys()) == expected_keys
if item['kind'] == 'file':
assert item['storage'] == {
'data': {
'name': record.name,
'path': record.versions.first().location_hash,
},
'settings': {
storage_settings.WATERBUTLER_RESOURCE: record.versions.first().location[
storage_settings.WATERBUTLER_RESOURCE
],
},
}
else:
assert item['storage'] is None
if item['kind'] == 'file':
assert item['path'] == f'/{record._id}'
if orm:
assert item['id'] == record.id
else:
assert item['path'] == f'/{folder._id}/'
if orm:
assert item['id'] == folder.id
parent = self.node_settings.get_root().append_folder('minimal-child-fields')
record = self._create_child_file(parent, 'magíc.mp3')
folder = parent.append_folder('nested')
res = self.get_children(parent)
assert res.status_code == 200
assert len(res.json) == 2
assert {item['name'] for item in res.json} == {record.name, folder.name}
for item in res.json:
assert set(item.keys()) == {'id', 'kind', 'name', 'path', 'storage'}
if item['kind'] == 'file':
assert item['storage'] == {
'data': {
'name': record.name,
'path': record.versions.first().location_hash,
},
'settings': {
storage_settings.WATERBUTLER_RESOURCE: record.versions.first().location[
storage_settings.WATERBUTLER_RESOURCE
],
},
}
assert item['path'] == f'/{record._id}'
assert item['id'] == record.id
else:
assert item['storage'] is None
assert item['path'] == f'/{folder._id}/'
assert item['id'] == folder.id

def test_minimal_pagination(self):
parent = self.node_settings.get_root().append_folder('pagination')
Expand All @@ -304,12 +291,11 @@ def test_minimal_pagination(self):
for name in ('a.mp3', 'b.mp3', 'c.mp3')
]
children.sort(key=lambda c: c.id)
first_page = self.get_children(parent, orm=True, limit=2)
first_page = self.get_children(parent, limit=2)
assert first_page.status_code == 200
assert [item['id'] for item in first_page.json] == [children[0].id, children[1].id]
second_page = self.get_children(
parent,
orm=True,
limit='2',
after=str(children[1].id),
)
Expand All @@ -320,7 +306,7 @@ def test_minimal_pagination(self):
def test_minimal_pagination_after_beyond_last_child(self):
parent = self.node_settings.get_root()
record = self._create_child_file(parent, 'only.mp3')
res = self.get_children(parent, orm=True, after=str(record.id))
res = self.get_children(parent, after=str(record.id))
assert res.status_code == 200
assert res.json == []

Expand Down
66 changes: 8 additions & 58 deletions addons/osfstorage/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,57 +187,7 @@ def osfstorage_get_metadata(file_node, **kwargs):
return file_node.serialize(version=version, include_full=True)


# TODO: Remove _osfstorage_minimal_metadata_sql if the Django ORM implementation
# performs well in production benchmarks and zip/DAZ workloads.
def _osfstorage_minimal_metadata_sql(file_node):
waterbutler_resource = osf_storage_settings.WATERBUTLER_RESOURCE
with connection.cursor() as cursor:
cursor.execute("""
SELECT json_agg(
json_build_object(
'kind', CASE
WHEN F.type = 'osf.osfstoragefile' THEN 'file'
ELSE 'folder'
END,
'name', F.name,
'path', CASE
WHEN F.type = 'osf.osfstoragefile' THEN '/' || F._id
ELSE '/' || F._id || '/'
END,
'storage', CASE
WHEN F.type = 'osf.osfstoragefile' THEN
json_build_object(
'data', json_build_object(
'name', COALESCE(LATEST_VERSION.version_name, F.name),
'path', LATEST_VERSION.location ->> 'object'
),
'settings', json_build_object(
%s, LATEST_VERSION.location ->> %s
)
)
ELSE NULL
END
)
)
FROM osf_basefilenode AS F
LEFT JOIN LATERAL (
SELECT
osf_fileversion.location,
osf_basefileversionsthrough.version_name
FROM osf_fileversion
JOIN osf_basefileversionsthrough
ON osf_fileversion.id = osf_basefileversionsthrough.fileversion_id
WHERE osf_basefileversionsthrough.basefilenode_id = F.id
ORDER BY osf_fileversion.created DESC
LIMIT 1
) LATEST_VERSION ON F.type = 'osf.osfstoragefile'
WHERE F.parent_id = %s
AND F.type IN ('osf.osfstoragefile', 'osf.osfstoragefolder')
""", [waterbutler_resource, waterbutler_resource, file_node.id])
return cursor.fetchone()[0] or []


def _osfstorage_minimal_metadata_orm(file_node, *, limit=None, after=None):
def _osfstorage_minimal_metadata(file_node, *, limit=None, after=None):
waterbutler_resource = osf_storage_settings.WATERBUTLER_RESOURCE
latest_version = BaseFileVersionsThrough.objects.filter(
basefilenode_id=OuterRef('pk'),
Expand Down Expand Up @@ -283,6 +233,8 @@ def _osfstorage_minimal_metadata_orm(file_node, *, limit=None, after=None):
if after is not None:
qs = qs.filter(id__gt=after)

# Always return files/folders in the same order
# It helps make waterbutler file streaming more stable
qs = qs.order_by('id')

if limit is not None:
Expand Down Expand Up @@ -402,13 +354,11 @@ def _osfstorage_full_metadata(file_node, user_id):
@decorators.autoload_filenode(must_be='folder')
def osfstorage_get_children(file_node, **kwargs):
if is_truthy(request.args.get('minimal')):
if is_truthy(request.args.get('orm')):
return _osfstorage_minimal_metadata_orm(
file_node,
limit=request.args.get('limit', type=int, default=None),
after=request.args.get('after', type=int, default=None),
)
return _osfstorage_minimal_metadata_sql(file_node)
return _osfstorage_minimal_metadata(
file_node,
limit=request.args.get('limit', type=int, default=None),
after=request.args.get('after', type=int, default=None),
)
return _osfstorage_full_metadata(file_node, request.args.get('user_id'))


Expand Down