Storage should reset file's cursor after saving

[Julien Phalip]

I had some trouble finding the cause of that problem. Basically, I was trying to save an uploaded image and then manipulate that image with PIL. But the following didn't work:

from PIL import Image

storage.save('/images/image.gif', uploaded_file) # Save the original
uploaded_image = Image.open(uploaded_file)
...

After debugging PIL I realised that it assumes the given file starts at position 0. Therefore, the following works:

from PIL import Image

storage.save('/images/image.gif', uploaded_file) # Save the original
uploaded_file.seek(0) # >> REWIND FILE <<
uploaded_image = Image.open(uploaded_file)
...

I think it would be good practice to automatically rewind the file after it's been saved, so that other tools can process the file without having to rewind it manually. That would feel more logical from an outside perspective.

Patch attached.

[Julien Phalip]

Patch and regression tests

[Julien Phalip]

Just a note about the tests I've written. I couldn't get temp_file.read() to return a unicode value 'some contents', even if creating it explicitly: ContentFile('some contents').
Maybe I've missed something simple there. What do you think?

[Julien Phalip]

Same patch, but changed the word 'rewind'

[Julien Phalip]

'Rewind' is probably not the best word since we're dealing with direct file access :)

[Julien Phalip]

Hmmm, I'm now wondering if this ticket is in fact valid...

FileSystemStorage._save() is supposed to close the file after it's been saved, right? In that case, it doesn't make sense to reset the cursor's position. Could Marty Alchin, or someone familiar enough with FileSystemStorage confirm this?

I believe that the reason uploaded_file still existed after saving was because I was testing on Windows, and that temporary uploaded files are not cleaned up properly on that platform. See ticket #8203.

If someone can confirm all that, then I guess this ticket is invalid. The right way would be to manipulate the file first, then save the manipulated version, and *lastly* save the original:

manipulated_image = Image.open(uploaded_file)
...
storage.save('/images/manipulated_image.gif', manipulated_image) # Save the manipulated image and close it.
storage.save('/images/image.gif', uploaded_file) # Save the original and close it.

[Julien Phalip]

One question though: should closing the file after saving be enforced to every storage system?

Currently FileSystemStorage._save() seems to do it itself, but Storage.save() doesn't enforce it by default.

[Jacob]

This change causes a test failure:

======================================================================
ERROR: test_large_upload (regressiontests.file_uploads.tests.FileUploadTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/jacob/Projects/Django/upstream/tests/regressiontests/file_uploads/tests.py", line 53, in test_large_upload
    response = self.client.post('/file_uploads/verify/', post_data)
  File "/Users/jacob/Projects/Django/upstream/django/test/client.py", line 285, in post
    return self.request(**r)
  File "/Users/jacob/Projects/Django/upstream/django/core/handlers/base.py", line 86, in get_response
    response = callback(request, *callback_args, **callback_kwargs)
  File "/Users/jacob/Projects/Django/upstream/tests/regressiontests/file_uploads/views.py", line 52, in file_upload_view_verify
    obj.testfile.save(largefile.name, largefile)
  File "/Users/jacob/Projects/Django/upstream/django/db/models/fields/files.py", line 74, in save
    self._name = self.storage.save(name, content)
  File "/Users/jacob/Projects/Django/upstream/django/core/files/storage.py", line 46, in save
    content.seek(0)
  File "/Users/jacob/Projects/Django/upstream/django/core/files/uploadedfile.py", line 88, in seek
    def seek(self, *args):          return self._file.seek(*args)
ValueError: I/O operation on closed file

----------------------------------------------------------------------

This is because depending on what type of file content is, seek() may not be valid after the file's been saved.

So I think you're right that this is invalid; you can always call seek() manually should you need to (or simply save the other file first, like you figured out).

[Jacob]

Milestone 1.0 deleted