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

* pass storage meta to provider download method
* fix tests
* add comments and docstrings
* move sentry call and remove dead code

Author: antkryt

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)

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]),
         )
 
 

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
@@ -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))
 
@@ -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
+        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):
@@ -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),
@@ -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',