From 842612b8ead21ea2a12084d24c8d3e565042a2c1 Mon Sep 17 00:00:00 2001 From: Kshitij Sobti Date: Fri, 6 Apr 2018 02:43:32 +0530 Subject: [PATCH] Only use export_output_handler if artifact storage is explicitly FileSystemStorage Use StreamingHttpResponse to support larger file downloads --- .../contentstore/views/import_export.py | 16 ++-- .../views/tests/test_import_export.py | 76 +++++++++++++++++++ cms/envs/common.py | 5 ++ 3 files changed, 90 insertions(+), 7 deletions(-) diff --git a/cms/djangoapps/contentstore/views/import_export.py b/cms/djangoapps/contentstore/views/import_export.py index 9a9d5cefed..4bee4b776d 100644 --- a/cms/djangoapps/contentstore/views/import_export.py +++ b/cms/djangoapps/contentstore/views/import_export.py @@ -13,8 +13,9 @@ from django.conf import settings from django.contrib.auth.decorators import login_required from django.core.exceptions import PermissionDenied from django.core.files import File +from django.core.files.storage import FileSystemStorage from django.db import transaction -from django.http import Http404, HttpResponse, HttpResponseNotFound +from django.http import Http404, HttpResponse, HttpResponseNotFound, StreamingHttpResponse from django.utils.translation import ugettext as _ from django.views.decorators.csrf import ensure_csrf_cookie from django.views.decorators.http import require_GET, require_http_methods @@ -22,6 +23,7 @@ from opaque_keys.edx.keys import CourseKey from opaque_keys.edx.locator import LibraryLocator from path import Path as path from six import text_type +from storages.backends.s3boto import S3BotoStorage from user_tasks.conf import settings as user_tasks_settings from user_tasks.models import UserTaskArtifact, UserTaskStatus from wsgiref.util import FileWrapper @@ -273,8 +275,8 @@ def send_tarball(tarball, size): """ Renders a tarball to response, for use when sending a tar.gz file to the user. """ - wrapper = FileWrapper(tarball) - response = HttpResponse(wrapper, content_type='application/x-tgz') + wrapper = FileWrapper(tarball, settings.COURSE_EXPORT_DOWNLOAD_CHUNK_SIZE) + response = StreamingHttpResponse(wrapper, content_type='application/x-tgz') response['Content-Disposition'] = 'attachment; filename=%s' % os.path.basename(tarball.name.encode('utf-8')) response['Content-Length'] = size return response @@ -369,7 +371,9 @@ def export_status_handler(request, course_key_string): elif task_status.state == UserTaskStatus.SUCCEEDED: status = 3 artifact = UserTaskArtifact.objects.get(status=task_status, name='Output') - if hasattr(artifact.file.storage, 'bucket'): + if isinstance(artifact.file.storage, FileSystemStorage): + output_url = reverse_course_url('export_output_handler', course_key) + elif isinstance(artifact.file.storage, S3BotoStorage): filename = os.path.basename(artifact.file.name).encode('utf-8') disposition = 'attachment; filename="{}"'.format(filename) output_url = artifact.file.storage.url(artifact.file.name, response_headers={ @@ -378,9 +382,7 @@ def export_status_handler(request, course_key_string): 'response-content-type': 'application/x-tgz' }) else: - # local file, serve from the authorization wrapper view - output_url = reverse_course_url('export_output_handler', course_key) - + output_url = artifact.file.storage.url(artifact.file.name) elif task_status.state in (UserTaskStatus.FAILED, UserTaskStatus.CANCELED): status = max(-(task_status.completed_steps + 1), -2) errors = UserTaskArtifact.objects.filter(status=task_status, name='Error') diff --git a/cms/djangoapps/contentstore/views/tests/test_import_export.py b/cms/djangoapps/contentstore/views/tests/test_import_export.py index 28eb68a7e9..b789c21f90 100644 --- a/cms/djangoapps/contentstore/views/tests/test_import_export.py +++ b/cms/djangoapps/contentstore/views/tests/test_import_export.py @@ -13,10 +13,14 @@ from uuid import uuid4 import ddt import lxml from django.conf import settings +from django.core.files.storage import FileSystemStorage from django.test.utils import override_settings from milestones.tests.utils import MilestonesTestCaseMixin +from mock import Mock, patch from opaque_keys.edx.locator import LibraryLocator from path import Path as path +from storages.backends.s3boto import S3BotoStorage +from user_tasks.models import UserTaskStatus from contentstore.tests.test_libraries import LibraryTestCase from contentstore.tests.utils import CourseTestCase @@ -132,6 +136,9 @@ class ImportTestCase(CourseTestCase): """ Unit tests for importing a course or Library """ + + CREATE_USER = True + def setUp(self): super(ImportTestCase, self).setUp() self.url = reverse_course_url('import_handler', self.course.id) @@ -689,6 +696,75 @@ class ExportTestCase(CourseTestCase): resp = client.get(reverse_course_url('export_output_handler', self.course.id)) self.assertEqual(resp.status_code, 403) + def _mock_artifact(self, spec=None, file_url=None): + """ + Creates a Mock of the UserTaskArtifact model for testing exports handler + code without touching the database. + """ + mock_artifact = Mock() + mock_artifact.file.name = 'testfile.tar.gz' + mock_artifact.file.storage = Mock(spec=spec) + mock_artifact.file.storage.url.return_value = file_url + return mock_artifact + + @patch('contentstore.views.import_export._latest_task_status') + @patch('user_tasks.models.UserTaskArtifact.objects.get') + def test_export_status_handler_other( + self, + mock_get_user_task_artifact, + mock_latest_task_status, + ): + """ + Verify that the export status handler generates the correct export path + for storage providers other than ``FileSystemStorage`` and + ``S3BotoStorage`` + """ + mock_latest_task_status.return_value = Mock(state=UserTaskStatus.SUCCEEDED) + mock_get_user_task_artifact.return_value = self._mock_artifact( + file_url='/path/to/testfile.tar.gz', + ) + resp = self.client.get(self.status_url) + result = json.loads(resp.content) + self.assertEqual(result['ExportOutput'], '/path/to/testfile.tar.gz') + + @patch('contentstore.views.import_export._latest_task_status') + @patch('user_tasks.models.UserTaskArtifact.objects.get') + def test_export_status_handler_s3( + self, + mock_get_user_task_artifact, + mock_latest_task_status, + ): + """ + Verify that the export status handler generates the correct export path + for the ``S3BotoStorage`` storage provider + """ + mock_latest_task_status.return_value = Mock(state=UserTaskStatus.SUCCEEDED) + mock_get_user_task_artifact.return_value = self._mock_artifact( + spec=S3BotoStorage, + file_url='/s3/file/path/testfile.tar.gz', + ) + resp = self.client.get(self.status_url) + result = json.loads(resp.content) + self.assertEqual(result['ExportOutput'], '/s3/file/path/testfile.tar.gz') + + @patch('contentstore.views.import_export._latest_task_status') + @patch('user_tasks.models.UserTaskArtifact.objects.get') + def test_export_status_handler_filesystem( + self, + mock_get_user_task_artifact, + mock_latest_task_status, + ): + """ + Verify that the export status handler generates the correct export path + for the ``FileSystemStorage`` storage provider + """ + mock_latest_task_status.return_value = Mock(state=UserTaskStatus.SUCCEEDED) + mock_get_user_task_artifact.return_value = self._mock_artifact(spec=FileSystemStorage) + resp = self.client.get(self.status_url) + result = json.loads(resp.content) + file_export_output_url = reverse_course_url('export_output_handler', self.course.id) + self.assertEqual(result['ExportOutput'], file_export_output_url) + @override_settings(CONTENTSTORE=TEST_DATA_CONTENTSTORE) class TestLibraryImportExport(CourseTestCase): diff --git a/cms/envs/common.py b/cms/envs/common.py index 9a1b1f6f5c..f957ed258a 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -1536,3 +1536,8 @@ COMPLETION_VIDEO_COMPLETE_PERCENTAGE = 0.95 from openedx.core.djangoapps.plugins import plugin_apps, plugin_settings, constants as plugin_constants INSTALLED_APPS.extend(plugin_apps.get_apps(plugin_constants.ProjectType.CMS)) plugin_settings.add_plugins(__name__, plugin_constants.ProjectType.CMS, plugin_constants.SettingsType.COMMON) + +# Course exports streamed in blocks of this size. 8192 or 8kb is the default +# setting for the FileWrapper class used to iterate over the export file data. +# See: https://docs.python.org/2/library/wsgiref.html#wsgiref.util.FileWrapper +COURSE_EXPORT_DOWNLOAD_CHUNK_SIZE = 8192