OpenConceptLab/ocl_issues#2454 | Export API to return 302 with Location (#841)

* OpenConceptLab/ocl_issues#2454 | Export API to return 302 with Location

* OpenConceptLab/ocl_issues#2454 | remove unused streaming response method

* OpenConceptLab/ocl_issues#2454 | 500 on unable to generate signed url

Author: snyaggarwal

core/common/mixins.py

@@ -9,7 +9,7 @@
 from django.db import transaction
 from django.db.models import Q, F, QuerySet
 from django.http import HttpResponseForbidden, Http404
-from django.shortcuts import get_object_or_404
+from django.shortcuts import get_object_or_404, redirect
 from django.urls import resolve, Resolver404
 from django.utils.functional import cached_property
 from ocldev.checksum import Checksum
@@ -984,7 +984,19 @@ def get(self, request, *args, **kwargs):  # pylint: disable=unused-argument
             return Response(status=status.HTTP_208_ALREADY_REPORTED)
 
         if version.has_export():
-            return get_export_service().get_streaming_response(version.get_export_path())
+            export_path = version.get_export_path()
+            export_url = get_export_service().url_for(export_path)
+            if export_url:
+                return redirect(export_url)
+            logger.error(
+                'Export exists for %s version %s but no signed URL was generated.',
+                self.entity.lower(),
+                version.version,
+            )
+            return Response(
+                {'detail': 'Export exists but could not generate a download URL.'},
+                status=status.HTTP_500_INTERNAL_SERVER_ERROR,
+            )
 
         return Response(status=status.HTTP_204_NO_CONTENT)
 

core/integration_tests/tests_collections.py

@@ -2,7 +2,6 @@
 import zipfile
 
 from celery_once import AlreadyQueued
-from django.http import StreamingHttpResponse
 from mock import patch, Mock, ANY
 from rest_framework.exceptions import ErrorDetail
 
@@ -2608,12 +2607,12 @@ def test_get_204_for_version(self, s3_has_path_mock):
         self.assertEqual(response.status_code, 204)
         s3_has_path_mock.assert_called_once_with("users/username/username_coll_v1.")
 
-    @patch('core.services.storages.cloud.aws.S3.get_streaming_response')
+    @patch('core.services.storages.cloud.aws.S3.url_for')
     @patch('core.services.storages.cloud.aws.S3.get_last_key_from_path')
     @patch('core.services.storages.cloud.aws.S3.has_path')
-    def test_get_200_version(self, s3_has_path_mock, s3_get_last_key_from_path_mock, s3_streaming_response):
+    def test_get_302_version(self, s3_has_path_mock, s3_get_last_key_from_path_mock, s3_url_for_mock):
         s3_has_path_mock.return_value = True
-        s3_streaming_response.return_value = StreamingHttpResponse(content_type='application/zip')
+        s3_url_for_mock.return_value = 'https://signed.example/coll-v1.zip'
         s3_get_last_key_from_path_mock.return_value = f'users/username/username_coll_v1.{self.v1_updated_at}.zip'
 
         response = self.client.get(
@@ -2622,24 +2621,45 @@ def test_get_200_version(self, s3_has_path_mock, s3_get_last_key_from_path_mock,
             format='json'
         )
 
-        self.assertEqual(response.status_code, 200)
+        self.assertEqual(response.status_code, 302)
+        self.assertEqual(response['Location'], 'https://signed.example/coll-v1.zip')
         s3_has_path_mock.assert_called_once_with("users/username/username_coll_v1.")
         s3_get_last_key_from_path_mock.assert_called_once_with("users/username/username_coll_v1.")
+        s3_url_for_mock.assert_called_once_with(f'users/username/username_coll_v1.{self.v1_updated_at}.zip')
 
-    @patch('core.services.storages.cloud.aws.S3.get_streaming_response')
+    @patch('core.services.storages.cloud.aws.S3.url_for')
     @patch('core.services.storages.cloud.aws.S3.exists')
-    def test_get_200_head(self, s3_exists_mock, s3_streaming_response):
+    def test_get_302_head(self, s3_exists_mock, s3_url_for_mock):
         s3_exists_mock.return_value = True
-        s3_streaming_response.return_value = StreamingHttpResponse(content_type='application/zip')
+        s3_url_for_mock.return_value = 'https://signed.example/coll-head.zip'
 
         response = self.client.get(
             self.collection.uri + 'HEAD/export/',
             HTTP_AUTHORIZATION='Token ' + self.admin_token,
             format='json'
         )
 
-        self.assertEqual(response.status_code, 200)
+        self.assertEqual(response.status_code, 302)
+        self.assertEqual(response['Location'], 'https://signed.example/coll-head.zip')
+        s3_exists_mock.assert_called_once_with(f"users/username/username_coll_vHEAD.{self.HEAD_updated_at}.zip")
+        s3_url_for_mock.assert_called_once_with(f"users/username/username_coll_vHEAD.{self.HEAD_updated_at}.zip")
+
+    @patch('core.services.storages.cloud.aws.S3.url_for')
+    @patch('core.services.storages.cloud.aws.S3.exists')
+    def test_get_500_head_when_signed_url_generation_fails(self, s3_exists_mock, s3_url_for_mock):
+        s3_exists_mock.return_value = True
+        s3_url_for_mock.return_value = None
+
+        response = self.client.get(
+            self.collection.uri + 'HEAD/export/',
+            HTTP_AUTHORIZATION='Token ' + self.admin_token,
+            format='json'
+        )
+
+        self.assertEqual(response.status_code, 500)
+        self.assertEqual(response.data, {'detail': 'Export exists but could not generate a download URL.'})
         s3_exists_mock.assert_called_once_with(f"users/username/username_coll_vHEAD.{self.HEAD_updated_at}.zip")
+        s3_url_for_mock.assert_called_once_with(f"users/username/username_coll_vHEAD.{self.HEAD_updated_at}.zip")
 
     def test_get_405(self):
         response = self.client.get(

core/integration_tests/tests_sources.py

@@ -6,7 +6,6 @@
 from celery_once import AlreadyQueued
 from django.conf import settings
 from django.db import transaction
-from django.http import StreamingHttpResponse
 from mock import patch, Mock, ANY, PropertyMock
 from mock.mock import call
 from rest_framework.exceptions import ErrorDetail
@@ -1004,11 +1003,10 @@ def test_get_204_version(self, s3_has_path_mock):
         self.assertEqual(response.status_code, 204)
         s3_has_path_mock.assert_called_once_with("users/username/username_source1_v1.")
 
-    @patch('core.services.storages.cloud.aws.S3.get_streaming_response')
+    @patch('core.services.storages.cloud.aws.S3.url_for')
     @patch('core.services.storages.cloud.aws.S3.exists')
-    def test_get_200_head(self, s3_exists_mock, s3_streaming_response):
-        response = StreamingHttpResponse()
-        s3_streaming_response.return_value = response
+    def test_get_302_head(self, s3_exists_mock, s3_url_for_mock):
+        s3_url_for_mock.return_value = 'https://signed.example/head.zip'
         s3_exists_mock.return_value = True
 
         response = self.client.get(
@@ -1017,15 +1015,16 @@ def test_get_200_head(self, s3_exists_mock, s3_streaming_response):
             format='json'
         )
 
-        self.assertEqual(response.status_code, 200)
+        self.assertEqual(response.status_code, 302)
+        self.assertEqual(response['Location'], 'https://signed.example/head.zip')
         s3_exists_mock.assert_called_once_with(f"users/username/username_source1_vHEAD.{self.HEAD_updated_at}.zip")
+        s3_url_for_mock.assert_called_once_with(f"users/username/username_source1_vHEAD.{self.HEAD_updated_at}.zip")
 
-    @patch('core.services.storages.cloud.aws.S3.get_streaming_response')
+    @patch('core.services.storages.cloud.aws.S3.url_for')
     @patch('core.services.storages.cloud.aws.S3.get_last_key_from_path')
     @patch('core.services.storages.cloud.aws.S3.has_path')
-    def test_get_200_version(self, s3_has_path_mock, s3_get_last_key_from_path_mock, s3_streaming_response):
-        response = StreamingHttpResponse()
-        s3_streaming_response.return_value = response
+    def test_get_302_version(self, s3_has_path_mock, s3_get_last_key_from_path_mock, s3_url_for_mock):
+        s3_url_for_mock.return_value = 'https://signed.example/v1.zip'
         s3_has_path_mock.return_value = True
         s3_get_last_key_from_path_mock.return_value = f'users/username/username_source1_v1.{self.v1_updated_at}.zip'
 
@@ -1035,10 +1034,28 @@ def test_get_200_version(self, s3_has_path_mock, s3_get_last_key_from_path_mock,
             format='json'
         )
 
-        self.assertEqual(response.status_code, 200)
-        self.assertEqual(response, response)
+        self.assertEqual(response.status_code, 302)
+        self.assertEqual(response['Location'], 'https://signed.example/v1.zip')
         s3_has_path_mock.assert_called_once_with("users/username/username_source1_v1.")
         s3_get_last_key_from_path_mock.assert_called_once_with("users/username/username_source1_v1.")
+        s3_url_for_mock.assert_called_once_with(f'users/username/username_source1_v1.{self.v1_updated_at}.zip')
+
+    @patch('core.services.storages.cloud.aws.S3.url_for')
+    @patch('core.services.storages.cloud.aws.S3.exists')
+    def test_get_500_head_when_signed_url_generation_fails(self, s3_exists_mock, s3_url_for_mock):
+        s3_exists_mock.return_value = True
+        s3_url_for_mock.return_value = None
+
+        response = self.client.get(
+            self.source.uri + 'HEAD/export/',
+            HTTP_AUTHORIZATION='Token ' + self.admin_token,
+            format='json'
+        )
+
+        self.assertEqual(response.status_code, 500)
+        self.assertEqual(response.data, {'detail': 'Export exists but could not generate a download URL.'})
+        s3_exists_mock.assert_called_once_with(f"users/username/username_source1_vHEAD.{self.HEAD_updated_at}.zip")
+        s3_url_for_mock.assert_called_once_with(f"users/username/username_source1_vHEAD.{self.HEAD_updated_at}.zip")
 
     @patch('core.sources.models.Source.is_exporting', new_callable=PropertyMock)
     @patch('core.services.storages.cloud.aws.S3.exists')

core/services/storages/cloud/aws.py

@@ -5,7 +5,6 @@
 from botocore.exceptions import ClientError, NoCredentialsError, WaiterError
 from django.conf import settings
 from django.core.files.base import ContentFile
-from django.http import StreamingHttpResponse
 from pydash import get
 
 from core.services.storages.cloud.core import CloudStorageServiceInterface
@@ -119,16 +118,6 @@ def get_object(self, key):
     def file_iterator(response):
         yield from response['Body'].iter_chunks()
 
-    def get_streaming_response(self, key):
-        s3_response = self.get_object(key)
-        response = StreamingHttpResponse(
-            self.file_iterator(s3_response),
-            content_type=s3_response['ContentType']
-        )
-        response['Content-Disposition'] = f'attachment; filename={key.split("/")[-1]}'
-
-        return response
-
     # private
     def _generate_signed_url(self, accessor, key, metadata=None):
         params = {

core/services/storages/cloud/azure.py

@@ -3,7 +3,6 @@
 from azure.storage.blob import BlobServiceClient, ContentSettings, BlobPrefix
 from django.conf import settings
 from django.core.files.base import ContentFile
-from django.http import StreamingHttpResponse
 from pydash import get
 
 from core.services.storages.cloud.core import CloudStorageServiceInterface
@@ -114,17 +113,6 @@ def file_iterator(blob_client):
         data = stream.readall()
         yield data
 
-    def get_streaming_response(self, key):
-        blob_client = self.get_object(key)
-        blob_properties = blob_client.get_blob_properties()
-        response = StreamingHttpResponse(
-            self.file_iterator(blob_client),
-            content_type=blob_properties.content_settings.content_type
-        )
-        response['Content-Disposition'] = f'attachment; filename={key.split("/")[-1]}'
-
-        return response
-
     # private
     def _upload(self, blob_name, file_content=None, file_path=None, read_directive=None, metadata=None):  # pylint: disable=too-many-arguments
         if not file_path and not file_content:

core/services/storages/cloud/minio.py

@@ -2,7 +2,6 @@
 import mimetypes
 from io import BytesIO
 
-from django.http import StreamingHttpResponse
 from minio import Minio, S3Error
 from minio.deleteobjects import DeleteObject
 from pydash import get
@@ -168,21 +167,6 @@ def get_object(self, key):
         response = self.client.get_object(bucket_name=self.bucket_name, object_name=key)
         return response
 
-    def get_streaming_response(self, key):
-        """
-        Streams the file from MinIO using Django's StreamingHttpResponse.
-        """
-        try:
-            response = self.get_object(key)
-            streaming_http_response = StreamingHttpResponse(
-                self.file_iterator(response),
-                content_type=response.headers['Content-Type']
-            )
-            streaming_http_response['Content-Disposition'] = f'attachment; filename={key.split("/")[-1]}'
-            return streaming_http_response
-        except S3Error as e:
-            raise FileNotFoundError(f"File {key} not found in bucket {self.bucket_name}. Error: {e}") from e
-
     @staticmethod
     def file_iterator(file_obj, chunk_size=8192):
         """

core/services/storages/cloud/tests.py

@@ -8,7 +8,6 @@
 from azure.storage.blob import BlobPrefix
 from botocore.exceptions import ClientError
 from django.core.files.base import ContentFile
-from django.http import StreamingHttpResponse
 from django.test import TestCase
 from django.utils import timezone
 from minio.deleteobjects import DeleteError
@@ -501,25 +500,6 @@ def test_delete_objects(self, mock_minio_client):
         # Assert that remove_objects was called with the correct file keys
         mock_client.remove_objects.assert_called_once()
 
-    @patch("core.services.storages.cloud.minio.Minio")
-    def test_get_streaming_response(self, mock_minio_client):
-        # Mock get_object method
-        mock_client = mock_minio_client.return_value
-        mock_file_obj = MagicMock()
-        mock_file_obj.read = MagicMock(side_effect=[b"chunk1", b"chunk2", b""])
-        mock_client.get_object = MagicMock(return_value=mock_file_obj)
-        MinIO.file_iterator(mock_file_obj)
-
-        client = MinIO()
-        # Call the get_streaming_response method
-        response = client.get_streaming_response('test-file1.txt')
-
-        # Assert that the response is a StreamingHttpResponse
-        self.assertIsInstance(response, StreamingHttpResponse)
-        # Convert response content into a list for easier assertion
-        response_content = b"".join(list(response.streaming_content))
-        self.assertEqual(response_content, b"chunk1chunk2")
-
     @patch("core.services.storages.cloud.minio.Minio", Mock())
     def test_file_iterator(self):
         # Mock file object with read method