Opened 8 years ago

Closed 4 years ago

#26367 closed Cleanup/optimization (needsinfo)

Assess if FieldFile can work with stdlib File instead of requiring Django's File

Reported by: Cristiano Coelho Owned by: David Smith
Component: File uploads/storage Version: 1.9
Severity: Normal Keywords: file storage
Cc: Cristiano Coelho Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Tim Graham)

The FileField docs (https://docs.djangoproject.com/es/1.9/ref/models/fields/#django.db.models.FileField) state that in order to save a file into a FileField (through FieldFile proxy) you need a django File object and not a regular one.

Looking at the code (https://github.com/django/django/blob/28bcff82c5ed4694f4761c303294ffafbd7096ce/django/db/models/fields/files.py#L92 ) it seems like the only reason to have a File object is because it needs the 'size' property which isn't available on every file-like object.
Looking into the same file, it doesn't seem anyone uses _size anymore, only its parent class, BUT, this class actually overrides the size property so it is not calling the parent class .size (and if it did, it would cache it).

Would it be possible to remove any _size from FieldFile and then test against regular file-like objects rather than "File" and if everything works change the docs?
The only other place a File is required is at FileSystemStorage._save, however, since you don't call ._save but instead .save which already converts any file into a File (and it is also stated in the docs that the Storage.save method can accept any file), the requierement for a File seems unnecessary.

There's also a topic here https://groups.google.com/forum/#!topic/django-developers/CjoPZq8lZms that might suggest that the storage and file field classes require a few improvements.

Change History (11)

comment:1 by Cristiano Coelho, 8 years ago

Summary: Minor FieldFile (proxy for FieldFile) improvement?Minor FieldFile (proxy for FileField) improvement?

comment:2 by Tim Graham, 8 years ago

Description: modified (diff)
Summary: Minor FieldFile (proxy for FileField) improvement?Assess if FieldFile can work with stdlib File instead of requiring Django's File
Triage Stage: UnreviewedAccepted

The _size removal looks okay to me: PR

Not sure about whether this solves the issue of not requiring Django's File subclass, but accepting for further investigation.

comment:3 by Tim Graham <timograham@…>, 8 years ago

In f15f4b8b:

Refs #26367 -- Removed obsolete _size cache on FieldField.

The _size attribute is used in File.size but FieldFile overrides it.

comment:4 by Cristiano Coelho, 8 years ago

Hey Tim, thanks for the quick change for _size.

The requirement of File might be even more difficult to confirm, even if it is not a requirement for the django implementation, what if other people's own storage relies on it? Still there doesn't seem to be any checks for File on the save method so it is either blowing up already for people using normal files or everyone is guarded with a File object. So maybe as long as it works fine with django we shouldn't worry about other people's storages.

It seems like the File object is required at FieldFile for its open method and size property which is not present on regular files. However, if the FieldFile.save method handled the conversion to File of content, it should work fine from there, removing the requierement that the .save method is always called with a File object. Other requierements of File would still apply ( for all the storage class methods) but it would definetly make saving files less verbose.

Looking at the save method is it possible that it also has an issue? After saving, the current 'cached' file instance is never refrshed, only the file name of the instance: https://github.com/django/django/blob/f15f4b8bb6f45dfdcb74f71e8dcc30c0aaa6ac80/django/db/models/fields/files.py#L89

comment:5 by Tim Graham, 8 years ago

I don't have any motivation to do further work on this ticket. Feel free to investigate and submit patches if you like. It would be helpful if you motivated the issue a bit more clearly (what is the use case, e.g. why is using Django's File a problem).

comment:6 by Collin Anderson, 8 years ago

Could we just have the implementation wrap it if needed before sending it to the storage backend?

comment:7 by Cristiano Coelho, 8 years ago

Would it be as easy as wrap it in the save method of FieldFile? Note that the storage would still be required to return a File object on retrieval.
That simple change would remove the need of importing File (which is already hard to find!) and wraping every file in File when saving a FileField or ImageField.

comment:8 by Cristiano Coelho, 8 years ago

Looks like something as simple as

if not isinstance(content, File):
    content = File(content)

at the FieldFile.save method doesn't break anything and would remove the requierement from the docs "Note that the content argument should be an instance of django.core.files.File, not Python’s built-in file object. You can construct a File from an existing Python file object like this:" making it simpler to save a file.

comment:9 by David Smith, 4 years ago

Has patch: set
Owner: changed from nobody to David Smith
Status: newassigned

If I have understood this correctly I think this already works.

PR with tests showing conversion to Django's File type.

comment:10 by Mariusz Felisiak, 4 years ago

David, this issue is still valid because we don't have a conversion in the FieldFile.save(). Conversion is made in the Storage class, but can be omitted by custom storages. I'm not sure why using Django's File is an issue. I think we should close it unless someone can provide the use case and motivation.

comment:11 by Mariusz Felisiak, 4 years ago

Resolution: needsinfo
Status: assignedclosed
Note: See TracTickets for help on using tickets.
Back to Top