Opened 16 years ago
Last modified 6 years ago
#9631 new Cleanup/optimization
FieldFile assumes efficient Storage.size
Reported by: | Peter Sagerson | Owned by: | nobody |
---|---|---|---|
Component: | File uploads/storage | Version: | 1.11 |
Severity: | Normal | Keywords: | file upload compression |
Cc: | Kevin Turner, Collin Anderson | Triage Stage: | Design decision needed |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
FieldFile's _get_size, like many of its methods, passes the operation on to its Storage object. However, in this case, assuming that a storage mechanism can easily discover the size of a file by name is rather rash. If the file is compressed, for instance, it may be necessary to decompress the entire file to determine the size. Remote storage could suffer from similar issues.
Fortunately, FieldFile has a File object provided by the storage. The Storage derivative is responsible for returning a valid File object, which means it must be able to correctly report its size. By asking the File object for its size directly, we provide opportunities for caching so that a size request followed by a read operation need not trigger two independent retrievals of the data.
Attachments (1)
Change History (11)
by , 16 years ago
Attachment: | file_size.diff added |
---|
comment:1 by , 16 years ago
Triage Stage: | Unreviewed → Design decision needed |
---|
follow-up: 3 comment:2 by , 16 years ago
Version: | 1.0 → 1.1-beta-1 |
---|
I've run into problem related to this bug. If we try to access file size in save() method of model, we are unable to get size, at least in case of InMemoryUploadedFile, because default storage unable to find file on FS.
comment:3 by , 16 years ago
comment:4 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → Cleanup/optimization |
comment:7 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
I think this was fixed as part of #10300. If that's not true, please reopen.
comment:8 by , 7 years ago
Cc: | added |
---|---|
Resolution: | fixed |
Status: | closed → new |
Version: | 1.1-beta → 1.11 |
This is still present in Django 1.11.11; FieldFile.size usually delegates to Storage instead of allowing its File to report (and, at its discretion, cache) the size:
https://github.com/django/django/blob/1.11.11/django/db/models/fields/files.py#L73
I don't understand what the if self._committed
is doing in there. Why wouldn't it be appropriate to return file.size
whether it's committed or not? That's the implementation suggested by the initial patch here.
comment:9 by , 6 years ago
Patch needs improvement: | set |
---|
Why wouldn't it be appropriate to return file.size whether it's committed or not? That's the implementation suggested by the initial patch here.
self.file
could be None
unless the file has been opened. But maybe that check should instead be for _file
, like the open()
method. It looks like the code path for using file.size
was added for efficiency in the first place in 68a890e79f660484d05482902663b6168f0bd71e.
comment:10 by , 6 years ago
Cc: | added |
---|
Change in FieldFile._get_size