#11084 closed (fixed)
After r10737, ImageField sets height and width on model creation, every lookup
Reported by: | Michael Newman | Owned by: | Gary Wilson |
---|---|---|---|
Component: | File uploads/storage | Version: | dev |
Severity: | Keywords: | ||
Cc: | arthur@…, jdunck@…, samuel@… | Triage Stage: | Accepted |
Has patch: | no | Needs documentation: | yes |
Needs tests: | yes | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
I am going to give this a little more time, but I wanted to file quickly.
r10737 was a large commit that changes the way that files variables are handled. On every lookup of ImageField, the image is opened on the OS. This causes huge IO overhead. Also if the file is unavailable, an uncaught OSError is raised, creating errors in templates if the filesystem is unavailable.
Attachments (4)
Change History (16)
by , 15 years ago
Attachment: | 11084-failing-tests.diff added |
---|
comment:1 by , 15 years ago
Summary: | r10737 opens file everytime a variable is looked up → After r10737, ImageField sets height and width on model creation, every lookup |
---|
So after a little digging I have figured out that it has to do with the ImageFileDescriptor, http://code.djangoproject.com/browser/django/trunk/django/db/models/fields/files.py#L297 , where set is called on creation of the instance, but also on database lookups down the line, meaning that if you have the height and width field defined, any time you want to use the ImageField on a model, it opens the Image file.
comment:2 by , 15 years ago
Needs documentation: | set |
---|---|
Needs tests: | set |
Patch needs improvement: | set |
comment:3 by , 15 years ago
Version: | 1.0 → SVN |
---|
My test, which checks if the dimensions are accessed after the python object is initialized, passes with this patch, but the second problem, which is that the image height and width are calculated every time the object is puled from the database, isn't fixed.
I am going to look into writing more tests for this problem.
comment:4 by , 15 years ago
milestone: | → 1.1 |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:5 by , 15 years ago
Cc: | added |
---|
comment:6 by , 15 years ago
Cc: | added |
---|
comment:7 by , 15 years ago
Just ran up against this issue today, came to file a ticket, and found this one.
I have a Photo model, and one record's ImageField referenced a .jpg that was corrupt. The file was unable to open, so even bringing up the changelist in the admin would trigger a TypeError of can't unpack non-sequence. This .jpg won't upload via normal Django admin functions, but a process that creates a Photo record without going through the normal upload can (and did) still get it into the system. Obviously something we need to address, but previous to r10737, this would only cause an error on the single record rather than borking the entire changelist.
This TypeError was thrown by line 327 here -- http://code.djangoproject.com/browser/django/trunk/django/db/models/fields/files.py#L327 -- because it couldn't open the file to grab a height, width tuple. Removing the width_field and height_field args from my model's ImageField makes the problem disappear, naturally, as does adding an Except within ImageFileDescriptor. Not suggesting these are good long-term solutions, just trying to provide more information about this behavior. Will attach a broken.jpg here in case anyone wants to use it in testing.
by , 15 years ago
Attachment: | broken.jpg added |
---|
Example of a corrupt .jpg; an ImageField that references this file will cause a TypeError on that model's admin changelist
comment:8 by , 15 years ago
Cc: | added |
---|
comment:9 by , 15 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:10 by , 15 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
(In [10858]) Changes to ImageFileDescriptor
and ImageField
to fix a few cases of setting image dimension fields.
- Moved dimension field update logic out of
ImageFileDescriptor.__set__
and into its own method onImageField
. - New
ImageField.update_dimension_fields
method is attached to model instance'spost_init
signal so that:- Dimension fields are set when defined before the ImageField.
- Dimension fields are set when the field is assigned in the model constructor (fixes #11196), but only if the dimension fields don't already have values, so we avoid updating the dimensions every time an object is loaded from the database (fixes #11084).
- Clear dimension fields when the ImageField is set to None, which also causes dimension fields to be cleared when
ImageFieldFile.delete()
is used. - Added many more tests for ImageField that test edge cases we weren't testing before, and moved the ImageField tests out of
file_storage
and into their own module withinmodel_fields
.
comment:11 by , 15 years ago
(In [10987]) Changes to ImageFileDescriptor
and ImageField
to fix a few cases of setting image dimension fields.
- Moved dimension field update logic out of
ImageFileDescriptor.__set__
and into its own method onImageField
. - New
ImageField.update_dimension_fields
method is attached to model instance'spost_init
signal so that:- Dimension fields are set when defined before the ImageField.
- Dimension fields are set when the field is assigned in the model constructor (fixes #11196), but only if the dimension fields don't already have values, so we avoid updating the dimensions every time an object is loaded from the database (fixes #11084).
- Clear dimension fields when the ImageField is set to None, which also causes dimension fields to be cleared when
ImageFieldFile.delete()
is used. - Added many more tests for ImageField that test edge cases we weren't testing before, and moved the ImageField tests out of
file_storage
and into their own module withinmodel_fields
.
(Contrived) failing test