[Django] #26367: Minor FieldFile (proxy for FieldFile) improvement?
Django
--------------------------------------+--------------------------
Reporter: cristianocca | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: File uploads/storage | Version: 1.9
Severity: Normal | Keywords: file storage
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+--------------------------
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/master/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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26367>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: cristianocca | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: File | Version: 1.9
uploads/storage |
Severity: Normal | Resolution:
Keywords: file storage | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by cristianocca):
* needs_docs: => 0
* needs_tests: => 0
* needs_better_patch: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/26367#comment:1>
Django
File
--------------------------------------+------------------------------------
Reporter: cristianocca | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: File uploads/storage | Version: 1.9
Keywords: file storage | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Unreviewed => Accepted
Old description:
> 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/master/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.
New description:
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.
--
Comment:
The `_size` removal looks okay to me:
[https://github.com/django/django/pull/6305 PR]
Not sure about whether this solves the issue of not requiring Django's
`File` subclass, but accepting for further investigation.
--
Ticket URL: <https://code.djangoproject.com/ticket/26367#comment:2>
Django
File
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: File uploads/storage | Version: 1.9
Keywords: file storage | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"f15f4b8bb6f45dfdcb74f71e8dcc30c0aaa6ac80" f15f4b8b]:
{{{
#!CommitTicketReference repository=""
revision="f15f4b8bb6f45dfdcb74f71e8dcc30c0aaa6ac80"
Refs #26367 -- Removed obsolete _size cache on FieldField.
The _size attribute is used in File.size but FieldFile overrides it.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26367#comment:3>
Django
File
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: File uploads/storage | Version: 1.9
Keywords: file storage | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by cristianocca):
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
--
Ticket URL: <https://code.djangoproject.com/ticket/26367#comment:4>
Django
File
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: File uploads/storage | Version: 1.9
Keywords: file storage | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
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).
--
Ticket URL: <https://code.djangoproject.com/ticket/26367#comment:5>
Django
File
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: File uploads/storage | Version: 1.9
Keywords: file storage | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by collinanderson):
Could we just have the implementation wrap it if needed before sending it
to the storage backend?
--
Ticket URL: <https://code.djangoproject.com/ticket/26367#comment:6>
Django
File
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: File uploads/storage | Version: 1.9
Keywords: file storage | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by cristianocca):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26367#comment:7>
Django
File
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: File uploads/storage | Version: 1.9
Keywords: file storage | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by cristianocca):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26367#comment:8>
Django
File
Reporter: Cristiano Coelho | Owner: David
Type: | Smith
Cleanup/optimization | Status: assigned
Component: File | Version: 1.9
uploads/storage |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by David Smith):
* owner: nobody => David Smith
* status: new => assigned
* has_patch: 0 => 1
Comment:
If I have understood this correctly'' I think'' this already works.
[https://code.djangoproject.com/ticket/26367 PR] with tests showing
conversion to Django's `File` type.
--
Ticket URL: <https://code.djangoproject.com/ticket/26367#comment:9>
Django
File
-------------------------------------+-------------------------------------
Reporter: Cristiano Coelho | Owner: David
Type: | Smith
Cleanup/optimization | Status: assigned
Component: File | Version: 1.9
uploads/storage |
Severity: Normal | Resolution:
Keywords: file storage | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by felixxm):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26367#comment:10>
Django
File
-------------------------------------+-------------------------------------
Reporter: Cristiano Coelho | Owner: David
Type: | Smith
Component: File | Version: 1.9
uploads/storage |
Keywords: file storage | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* status: assigned => closed
* resolution: => needsinfo
--
Ticket URL: <https://code.djangoproject.com/ticket/26367#comment:11>