Skip to content

Commit

Permalink
Fix final Py3 test failure.
Browse files Browse the repository at this point in the history
This is a combination of three fixes. The tests had to be adjusted, as
the cgi module in Python 3 won't work without a content-length header
anymore. This is a side-effect of python/cpython@5c23b8e.
Without the content length, the limit / read_bytes calculation is off,
and we end up with three fieldstorage instances each with an empty byte,
rather than one instance with a content of `test\n`.

The second part is a heavy handed workaround for closing the BytesIO
instance underlying the FieldStorage too early. In Python 2.7, there
wasn't a `__del__` defined yet. There's a good chance, a proper fix
could also be related to http://bugs.python.org/issue23700 and
http://bugs.python.org/issue18879, but I couldn't work out if
something similar was indeed the issue here.

Finally the TemporaryFileWrapper no longer needs special closing/del
logic under Python 3, as the implementation changed. There is no
`close_called` attribute anymore, but rather a more complex `_closer`
indirection.
  • Loading branch information
hannosch committed May 31, 2017
1 parent e9c7a52 commit 2c6568a
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 17 deletions.
28 changes: 18 additions & 10 deletions src/ZPublisher/HTTPRequest.py
Original file line number Diff line number Diff line change
Expand Up @@ -1644,18 +1644,19 @@ class TemporaryFileWrapper(_TemporaryFileWrapper):
and doesn't complain about such a move either.
"""

unlink = staticmethod(unlink)
isfile = staticmethod(isfile)
if PY2:
unlink = staticmethod(unlink)
isfile = staticmethod(isfile)

def close(self):
if not self.close_called:
self.close_called = True
self.file.close()
def close(self):
if not self.close_called:
self.close_called = True
self.file.close()

def __del__(self):
self.close()
if self.isfile(self.name):
self.unlink(self.name)
def __del__(self):
self.close()
if self.isfile(self.name):
self.unlink(self.name)


class ZopeFieldStorage(FieldStorage):
Expand All @@ -1664,6 +1665,13 @@ def make_file(self, binary=None):
handle, name = mkstemp()
return TemporaryFileWrapper(os.fdopen(handle, 'w+b'), name)

def __del__(self):
# Only call close on file object, cStringIO/BytesIO objects
# would be closed too early.
if (self.file is not None and
isinstance(self.file, TemporaryFileWrapper)):
self.file.close()


# Original version: zope.publisher.browser.FileUpload
class FileUpload(object):
Expand Down
20 changes: 13 additions & 7 deletions src/ZPublisher/tests/testHTTPRequest.py
Original file line number Diff line number Diff line change
Expand Up @@ -749,7 +749,9 @@ def test_close_removes_stdin_references(self):
s = BytesIO(TEST_FILE_DATA)
start_count = sys.getrefcount(s)

req = self._makeOne(stdin=s, environ=TEST_ENVIRON.copy())
environ = TEST_ENVIRON.copy()
environ['CONTENT_LENGTH'] = str(len(TEST_FILE_DATA))
req = self._makeOne(stdin=s, environ=environ)
req.processInputs()
self.assertNotEqual(start_count, sys.getrefcount(s)) # Precondition
req.close()
Expand All @@ -759,7 +761,9 @@ def test_processInputs_w_large_input_gets_tempfile(self):
# checks fileupload object supports the filename
s = BytesIO(TEST_LARGEFILE_DATA)

req = self._makeOne(stdin=s, environ=TEST_ENVIRON.copy())
environ = TEST_ENVIRON.copy()
environ['CONTENT_LENGTH'] = str(len(TEST_LARGEFILE_DATA))
req = self._makeOne(stdin=s, environ=environ)
req.processInputs()
f = req.form.get('largefile')
self.assertTrue(f.name)
Expand All @@ -769,12 +773,14 @@ def test_processInputs_with_file_upload_gets_iterator(self):
# collector entry 1837
s = BytesIO(TEST_FILE_DATA)

req = self._makeOne(stdin=s, environ=TEST_ENVIRON.copy())
environ = TEST_ENVIRON.copy()
environ['CONTENT_LENGTH'] = str(len(TEST_FILE_DATA))
req = self._makeOne(stdin=s, environ=environ)
req.processInputs()
f = req.form.get('file')
self.assertEqual(list(f), ['test\n'])
f = req.form.get('smallfile')
self.assertEqual(list(f), [b'test\n'])
f.seek(0)
self.assertEqual(next(f), 'test\n')
self.assertEqual(next(f), b'test\n')

def test__authUserPW_simple(self):
user_id = 'user'
Expand Down Expand Up @@ -1144,7 +1150,7 @@ def test_no_traversal_of_view_request_attribute(self):

TEST_FILE_DATA = b'''
--12345
Content-Disposition: form-data; name="file"; filename="file"
Content-Disposition: form-data; name="smallfile"; filename="smallfile"
Content-Type: application/octet-stream
test
Expand Down

0 comments on commit 2c6568a

Please sign in to comment.