Opened 7 years ago

Closed 7 years ago

Last modified 4 years ago

#8204 closed (wontfix)

FileSystemStorage should make it easy to save other types of files

Reported by: julien Owned by: nobody
Component: Uncategorized Version: master
Severity: Keywords:
Cc: flori@… Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

Description

It is quite common to use PIL to modify an uploaded image, e.g. create a thumbnail:

from PIL import Image

uploaded_image = Image.open(StringIO.StringIO(uploaded_file.read()))
... # Modify image, e.g. create thumbnail
uploaded_image.save('/blah/image.gif')
thumbnail_image.save('/blah/image_thumbnail.gif')

Now that the new file storage system has been merged in trunk, I could not find any easy way to use FileSystemStorage for saving PIL images. The problem is that FileSystemStorage assumes that it's given InMemoryUploadedFile or TemporaryUploadedFile (for example).

It seems quite essential that there should be a fall back option to manage other kinds of file content.

Attachments (4)

8204.storage_save.diff (918 bytes) - added by julien 7 years ago.
Adding a fall back option for other kinds of file contents
8204.storage_save.2.diff (1.0 KB) - added by julien 7 years ago.
First check if the existence of a 'save' method. Raise an exception if content type not recognized.
filestorage.diff (1.3 KB) - added by flosch 7 years ago.
maybe this will work, cannot test it yet, comments?
filestorage.2.diff (1.5 KB) - added by flosch 7 years ago.
I extended ContentFile for usage with PIL or others .. example follows, works for me (dunno whether ContentFile is the right place for that?)

Download all attachments as: .zip

Change History (21)

Changed 7 years ago by julien

Adding a fall back option for other kinds of file contents

comment:1 Changed 7 years ago by julien

  • milestone set to 1.0 maybe
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

1.0 maybe... although I think this *should* be tackled in 1.0. Otherwise, there might be many requests from people manipulating images with PIL.

Changed 7 years ago by julien

First check if the existence of a 'save' method. Raise an exception if content type not recognized.

comment:2 Changed 7 years ago by anonymous

  • Cc flori@… added

comment:3 follow-up: Changed 7 years ago by flosch

Since you don't know whether every "save"-method of any kind of objects would expect a path as an argument we should check whether the instance/content is a PIL image or not. Calling save with the full path could end in mysterious bugs or behaviour when using other kinds of objects than PIL Images or *File-Objects, which also offers save-methods.

comment:4 in reply to: ↑ 3 Changed 7 years ago by julien

  • Patch needs improvement set

Replying to flosch:

Since you don't know whether every "save"-method of any kind of objects would expect a path as an argument we should check whether the instance/content is a PIL image or not. Calling save with the full path could end in mysterious bugs or behaviour when using other kinds of objects than PIL Images or *File-Objects, which also offers save-methods.

I totally agree with that. In fact, in that patch I've simply replicated the test for if hasattr(content, 'temporary_file_path').
To be safe, you'd need to do isinstance (or something like that) checks to be sure that you're dealing with the right classes of objects.

comment:5 Changed 7 years ago by julien

See #8208 for a related issue.

comment:6 Changed 7 years ago by julien

  • milestone changed from 1.0 maybe to 1.0

Following on the example given in the description, what would be really nice to be able to do is:

from PIL import Image
from django.core.files.storage import FileSystemStorage

storage = FileSystemStorage('/myimages')

uploaded_image = Image.open(StringIO.StringIO(uploaded_file.read()))
thumbnail_image = ... # Create thumbnail

storage.save('/blah/image.gif', uploaded_image)
storage.save('/blah/image_thumbnail.gif', thumbnail_image)

comment:7 Changed 7 years ago by flosch

I encountered one problem with checking the content type against a PIL image: If you want do so, you're forced to import the PIL image which makes django requiring PIL in general. I don't know wheter PIL Image objects offering a version-string or something we can check; that would IMO solve the problem. Otherwise we can catch any ImportError while importing PIL and including the result of importing in the content type detection.

Changed 7 years ago by flosch

maybe this will work, cannot test it yet, comments?

comment:8 Changed 7 years ago by flosch

  • Needs tests set

comment:9 Changed 7 years ago by Gulopine

While I'm not categorically opposed to supporting PIL images as valid file types, I'm afraid we're going about this all wrong at the moment. There's no reason for Storage (or any subclass of it) to know anything about PIL. If anything, what we'd need instead is a new File object that can handle PIL images and feed the file data to Storage.

Looking at the documentation, it seems like this could also be approached from the opposite direction. Rather than feeding a PIL image to Storage, it should be possible to pass a File to Image.save(). Then, File would do what it needs to in order to commit the file to the storage system behind the scenes.

I'm afraid I don't use PIL personally, so I'm not the best person to try out some code for it. Could somebody who needs this give that a shot and let me know what problems are encountered? If it works the way I think it will, we shouldn't even need this ticket.

comment:10 Changed 7 years ago by flosch

  • Needs tests unset

comment:11 Changed 7 years ago by julien

I agree that the current approach is not ideal, including the test if hasattr(content, 'temporary_file_path'). Ideally, all tests should be done with isinstance and the PIL image content should be wrapped up in a Django File-like object, so FileSystemStorage would manipulate all files in a consistent way.
I'll have a look at how to approach that.

comment:12 Changed 7 years ago by flosch

I tested my patch and it works for me. Even though I agree with you, Gulopine.

Changed 7 years ago by flosch

I extended ContentFile for usage with PIL or others .. example follows, works for me (dunno whether ContentFile is the right place for that?)

comment:13 Changed 7 years ago by flosch

Works for me with like the following way:

from django.core.files.base import ContentFile
from django.core.files.storage import FileSystemStorage

from PIL import Image

uploaded_image = Image.open(StringIO.StringIO(uploaded_file.read()))
thumbnail_image = uploaded_image.resize((100,100)) # create the thumbnail

cf = ContentFile()
thumbnail_image.save(cf, format = "gif")

storage.save('static/image.gif', uploaded_file)
storage.save('static/image_thumbnail.gif', cf)

comment:14 Changed 7 years ago by Gulopine

That still seems like a lot more work than is really necessary here, but it does prove that there's no need to change core to make this happen. Using that example as a base, I stripped out a lot of the unnecessary reads and copies, coming up with the following, which also works:

from PIL import Image
from django.core.files.storage import default_storage

uploaded_image = Image.open(uploaded_file) # No need to use StringIO
thumbnail_image = uploaded_image.resize((100, 100)) # Create the thumbnail

# Save the original
default_storage.save('static/image.gif', uploaded_file)

# Open a new file to write to
output = default_storage.open('static/image_thumbnail.gif', 'wb')
thumbnail_image.save(output, format='gif')
output.close() # Ensures that it writes to the file

There's no need to read the file into StringIO, nor is there a need to create a temporary ContentFile before saving to the Storage object. File and its various subclasses were designed to work just like files in as many ways as possible, so that things like this can work without having to worry about all those finicky little details.

Given that there are now multiple demonstrated ways to achieve this, the only thing that's left to consider is whether a shortcut for thumbnailing should be included in core. I'm not convinced it should be, but I'll hold off on issuing a wontfix until I can think it a bit further, and see if anyone convinces me.

comment:15 Changed 7 years ago by jacob

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

No, since File is sufficiently file-like, there's no good reason to make Storage adopt domain-specific behavior. And thumnailing, though common, isn't really appropriate to be part of core.

comment:16 Changed 7 years ago by julien

Cool, that's great news. I still believe it's worth a small section in the doc. As you say, thumbnailing is pretty common, and a small example like the one above given by Gulopine would prevent many questions popping up on the forums.

comment:17 Changed 4 years ago by jacob

  • milestone 1.0 deleted

Milestone 1.0 deleted

Note: See TracTickets for help on using tickets.
Back to Top