FileSystemStorage should make it easy to save other types of files

[Julien Phalip]

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.

[Julien Phalip]

Adding a fall back option for other kinds of file contents

[Julien Phalip]

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.

[Julien Phalip]

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

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

[Julien Phalip]

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.

[Julien Phalip]

See #8208 for a related issue.

[Julien Phalip]

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)

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

[flosch]

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

[Marty Alchin]

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.

[Julien Phalip]

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.

[flosch]

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

[flosch]

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

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

[Marty Alchin]

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.

[Jacob]

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.

[Julien Phalip]

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.

[Jacob]

Milestone 1.0 deleted