From 0f9b513498d68a9c21930759a11bad1f59a3e180 Mon Sep 17 00:00:00 2001 From: James Wah Date: Wed, 21 Aug 2019 21:02:49 +1000 Subject: [PATCH] util: use the uploaded werkzeug.FileStorage class directly In newer versions of Werkzeug the uploaded file's stream is a SpooledTemporaryFile. That class doesn't implement the .seekable method, which is called by the Python zipfile library, and everything falls over. This patch avoids the issue by using the FileStorage class directly; as of Werkzeug 0.15 it proxies this method correctly to whatever object is backing the SpooledTemporaryFile. (At least according to the relevant GH issue: https://github.com/pallets/werkzeug/issues/1344 ). --- hostthedocs/__init__.py | 2 +- hostthedocs/util.py | 68 +++++++++++----------------------------- tests/test_filekeeper.py | 24 +++++++------- tests/test_util.py | 6 ++-- 4 files changed, 35 insertions(+), 65 deletions(-) diff --git a/hostthedocs/__init__.py b/hostthedocs/__init__.py index 67e09cd..0126d56 100644 --- a/hostthedocs/__init__.py +++ b/hostthedocs/__init__.py @@ -18,7 +18,7 @@ def hmfd(): if request.method == 'POST': if not request.files: return abort(400, 'Request is missing a zip/tar file.') - uploaded_file = util.UploadedFile.from_request(request) + uploaded_file = util.file_from_request(request) unpack_project( uploaded_file, request.form, diff --git a/hostthedocs/util.py b/hostthedocs/util.py index f863a50..d2a1f09 100644 --- a/hostthedocs/util.py +++ b/hostthedocs/util.py @@ -10,56 +10,24 @@ logger = logging.getLogger() - -class UploadedFile(object): - """ - UploadedFile represents a file uploaded during a POST request. +def file_from_request(request): """ + Get the uploaded file from a POST request, which should contain exactly one file. - def __init__(self, filename, stream): - """Instantiate an UploadedFile - - :param str filename: The name of the file. - :param stream: The open file stream. - """ - self._filename = filename - self._stream = stream - - @classmethod - def from_request(cls, request): - """ - Instantiate an UploadedFile from the first file in a request. - - :param werkzeug.wrappers.BaseRequest request: The POST request. - :return: The instantiated UploadedFile. - :raises ValueError: if no files exist within the request. - """ - uploaded_files = list(request.files.values()) - if len(uploaded_files) > 1: - logger.warning( - 'Only one file can be uploaded for each request. ' - 'Only the first file will be used.' - ) - elif len(uploaded_files) == 0: - raise ValueError('Request does not contain uploaded file') - - current_file = uploaded_files[0] - return cls(current_file.filename, current_file.stream) - - def get_filename(self): - return self._filename - - def get_stream(self): - return self._stream - - def close(self): - """close the file stream - """ - try: - self._stream.close() - except: - pass + :param werkzeug.wrappers.BaseRequest request: The POST request. + :return: The instantiated UploadedFile. + :raises ValueError: if no files exist within the request. + """ + uploaded_files = list(request.files.values()) + if len(uploaded_files) > 1: + logger.warning( + 'Only one file can be uploaded for each request. ' + 'Only the first file will be used.' + ) + elif len(uploaded_files) == 0: + raise ValueError('Request does not contain uploaded file') + return uploaded_files[0] class FileExpander(object): """ @@ -97,11 +65,11 @@ def detect_compression_method(cls, filename): raise ValueError('Unknown compression method for %s' % filename) def __enter__(self): - method = self.detect_compression_method(self._file.get_filename()) + method = self.detect_compression_method(self._file.filename) if method == 'zip': - self._handle = zipfile.ZipFile(self._file.get_stream()) + self._handle = zipfile.ZipFile(self._file) elif method == 'tar': - self._handle = tarfile.open(fileobj=self._file.get_stream(), mode='r:*') + self._handle = tarfile.open(fileobj=self._file, mode='r:*') else: raise ValueError('Unsupported method %s' % method) diff --git a/tests/test_filekeeper.py b/tests/test_filekeeper.py index 87240ab..14910fc 100644 --- a/tests/test_filekeeper.py +++ b/tests/test_filekeeper.py @@ -5,6 +5,7 @@ import tempfile import unittest import natsort +import werkzeug.datastructures from hostthedocs import filekeeper as fk from hostthedocs import util @@ -15,6 +16,10 @@ TARGZFILE = os.path.join(THISDIR, 'project.tar.gz') TARBZ2FILE = os.path.join(THISDIR, 'project.tar.bz2') +def make_uploaded_file(filename=ZIPFILE): + stream = open(filename, 'rb') + basename = os.path.basename(filename) + return werkzeug.datastructures.FileStorage(stream=stream, filename=basename) class TestParseDocfiles(unittest.TestCase): def test_parses(self): @@ -109,28 +114,25 @@ def do_unpacks(self, uploaded_file): self.assert_exists('proj/description.txt', exists=os.path.isfile) def test_unpack_zip(self): - uploaded_file = util.UploadedFile(ZIPFILE, open(ZIPFILE, mode='rb')) + uploaded_file = make_uploaded_file(ZIPFILE) self.do_unpacks(uploaded_file) def test_unpack_tar(self): - uploaded_file = util.UploadedFile(TARFILE, open(TARFILE, mode='rb')) + uploaded_file = make_uploaded_file(TARFILE) self.do_unpacks(uploaded_file) def test_unpack_tar_gz(self): - uploaded_file = util.UploadedFile(TARGZFILE, open(TARGZFILE, mode='rb')) + uploaded_file = make_uploaded_file(TARGZFILE) self.do_unpacks(uploaded_file) def test_unpack_tar_bz(self): - uploaded_file = util.UploadedFile(TARBZ2FILE, open(TARBZ2FILE, mode='rb')) + uploaded_file = make_uploaded_file(TARBZ2FILE) self.do_unpacks(uploaded_file) class TestUnpackProjectDescr(BaseTestUnpackProject): - def make_uploaded_file(self): - return util.UploadedFile(ZIPFILE, open(ZIPFILE, mode='rb')) - def test_ensure_description_file(self): - uploaded_file = self.make_uploaded_file() + uploaded_file = make_uploaded_file() tempd = self.make_temp_dir() metad = {'name': 'proj', 'version': '1.1', 'description': 'descr'} fk.unpack_project(uploaded_file, metad, tempd) @@ -138,7 +140,7 @@ def test_ensure_description_file(self): self.assert_contains('proj/description.txt', metad['description']) def test_update_description_file(self): - uploaded_file = self.make_uploaded_file() + uploaded_file = make_uploaded_file() tempd = self.make_temp_dir() metad = {'name': 'proj', 'version': '1.1', 'description': 'descr_old'} fk.unpack_project(uploaded_file, metad, tempd) @@ -148,7 +150,7 @@ def test_update_description_file(self): self.assert_contains('proj/description.txt', metad['description']) def test_no_update_description_file(self): - uploaded_file = self.make_uploaded_file() + uploaded_file = make_uploaded_file() tempd = self.make_temp_dir() metad = {'name': 'proj', 'version': '1.1', 'description': 'descr_old'} fk.unpack_project(uploaded_file, metad, tempd) @@ -158,7 +160,7 @@ def test_no_update_description_file(self): self.assert_contains('proj/description.txt', 'descr_old') def test_no_description_file(self): - uploaded_file = self.make_uploaded_file() + uploaded_file = make_uploaded_file() tempd = self.make_temp_dir() metad = {'name': 'proj', 'version': '1.1'} fk.unpack_project(uploaded_file, metad, tempd) diff --git a/tests/test_util.py b/tests/test_util.py index 5a0b01a..c0bf286 100644 --- a/tests/test_util.py +++ b/tests/test_util.py @@ -22,10 +22,10 @@ def test_extracting_filestream_from_request(self): request = Request(builder.get_environ()) # WHEN we process the request. - filestream = util.UploadedFile.from_request(request) + filestream = util.file_from_request(request) # THEN the file-stream should correspond to the provided file. - self.assertEqual(content, filestream.get_stream().read()) + self.assertEqual(content, filestream.read()) @raises(ValueError) def test_exception_thrown_if_request_contains_no_file(self): @@ -34,4 +34,4 @@ def test_exception_thrown_if_request_contains_no_file(self): request = Request(builder.get_environ()) # EXPECT an exception is thrown when we process the request. - util.UploadedFile.from_request(request) + util.file_from_request(request)