Opened 9 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 )
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 , 9 years ago
Summary: | Minor FieldFile (proxy for FieldFile) improvement? → Minor FieldFile (proxy for FileField) improvement? |
---|
comment:2 by , 9 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: | Unreviewed → Accepted |
comment:4 by , 9 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 , 9 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 , 9 years ago
Could we just have the implementation wrap it if needed before sending it to the storage backend?
comment:7 by , 9 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 , 9 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 , 4 years ago
Has patch: | set |
---|---|
Owner: | changed from | to
Status: | new → assigned |
If I have understood this correctly I think this already works.
PR with tests showing conversion to Django's File
type.
comment:10 by , 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 , 4 years ago
Resolution: | → needsinfo |
---|---|
Status: | assigned → closed |
The
_size
removal looks okay to me: PRNot sure about whether this solves the issue of not requiring Django's
File
subclass, but accepting for further investigation.