Skip to content

Fixed SpooledTemporaryFile bug in File class. #2455

Closed
wants to merge 2 commits into from

2 participants

@hlawrenz

Added condition to prevent checking the existence of a file name of a
file like object when the name attribute is None. This is necessary
because a SpooledTemporaryFile won't exist on the file system or have a
name until it has reached its max_size. Also added tests.

@hlawrenz hlawrenz Fixed SpooledTemporaryFile bug in File class.
Added condition to prevent checking the existence of a file name of a
file like object when the name attribute is None. This is necessary
because a SpooledTemporaryFile won't exist on the file system or have a
name until it has reached its max_size. Also added tests.
5848562
@bmispelon
Django member

Hi,

Can you open a ticket on trac for this: https://code.djangoproject.com/newticket (our policy is that anything more complex than a typo should be on trac, see https://github.com/django/django/blob/master/CONTRIBUTING.rst).

Thanks.

@bmispelon bmispelon commented on an outdated diff Mar 21, 2014
tests/files/tests.py
@@ -205,3 +205,17 @@ def test_file_move_overwrite(self):
os.close(handle_a)
os.close(handle_b)
+
+
+class SpooledTempTests(unittest.TestCase):
+ def test_in_memory_spooled_temp(self):
+ with tempfile.SpooledTemporaryFile() as temp:
+ temp.write("foo bar baz quux\n")
@bmispelon
Django member
bmispelon added a note Mar 21, 2014

For Python3 compatibility, you should use b"foo bar baz quux\n"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bmispelon bmispelon commented on an outdated diff Mar 21, 2014
tests/files/tests.py
@@ -205,3 +205,17 @@ def test_file_move_overwrite(self):
os.close(handle_a)
os.close(handle_b)
+
+
+class SpooledTempTests(unittest.TestCase):
+ def test_in_memory_spooled_temp(self):
+ with tempfile.SpooledTemporaryFile() as temp:
+ temp.write("foo bar baz quux\n")
+ django_file = File(temp, name="something.txt")
+ self.assertTrue(django_file.size, 17)
@bmispelon
Django member
bmispelon added a note Mar 21, 2014

Shouldn't this be assertEqual?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@hlawrenz

I pushed fixes to the issues issues mentioned. In retrospect, I'm not sure if the second test should be in there, it's not actually testing the issue that this patch fixes but more verified for me that django was working correctly when the file was large enough to be written. Let me know if you think it should be removed.

@bmispelon
Django member

I think the second test is fine. It's true that it already passes with the current codebase but it's short and it covers the case when the file is actually written to disk.

I've reworked the commit message to fit our guidelines (https://docs.djangoproject.com/en/1.6/internals/contributing/committing-code/#committing-guidelines), squashed the commits and pushed it as 918a16b.

Thanks!

@bmispelon bmispelon closed this Mar 21, 2014
@hlawrenz

Excellent, thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.