Opened 7 years ago

Closed 7 years ago

Last modified 2 years ago

#8222 closed Uncategorized (wontfix)

Storage should reset file's cursor after saving

Reported by: julien Owned by: nobody
Component: File uploads/storage Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Attachments (2)

8222.file.storage.rewind.diff (1.1 KB) - added by julien 7 years ago.
Patch and regression tests
8222.storage.save.diff (1.2 KB) - added by julien 7 years ago.
Same patch, but changed the word 'rewind'

Download all attachments as: .zip

Change History (10)

Changed 7 years ago by julien

Patch and regression tests

comment:1 Changed 7 years ago by julien

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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?

Changed 7 years ago by julien

Same patch, but changed the word 'rewind'

comment:2 Changed 7 years ago by julien

  • Summary changed from Storage should rewind files after saving to Storage should reset file's cursor after saving

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

comment:3 Changed 7 years ago by jacob

  • Component changed from Uncategorized to File uploads/storage
  • Triage Stage changed from Unreviewed to Accepted

comment:4 Changed 7 years ago by julien

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.

comment:5 Changed 7 years ago by julien

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.

comment:6 Changed 7 years ago by jacob

  • Resolution set to wontfix
  • Status changed from new to closed

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).

comment:7 Changed 4 years ago by jacob

  • milestone 1.0 deleted

Milestone 1.0 deleted

comment:8 Changed 2 years ago by anonymous

  • Easy pickings unset
  • Severity set to Normal
  • Type set to Uncategorized
  • UI/UX unset
Note: See TracTickets for help on using tickets.
Back to Top