Opened 8 years ago

Closed 8 years ago

Last modified 5 years ago

#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: master
Severity: Keywords:
Cc: arthur@…, jdunck@…, samuel@… Triage Stage: Accepted
Has patch: no Needs documentation: yes
Needs tests: yes Patch needs improvement: yes
Easy pickings: UI/UX:

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)

11084-failing-tests.diff (644 bytes) - added by Michael Newman 8 years ago.
(Contrived) failing test
11084-proposed-fix.patch (1.1 KB) - added by Armin Ronacher 8 years ago.
untested proposal for a quick fix.
11084.diff (3.1 KB) - added by Jeremy Dunck 8 years ago.
Patch was backwards, fixed now with passing tests.
broken.jpg (93.3 KB) - added by onemoreryan 8 years ago.
Example of a corrupt .jpg; an ImageField that references this file will cause a TypeError on that model's admin changelist

Download all attachments as: .zip

Change History (16)

Changed 8 years ago by Michael Newman

Attachment: 11084-failing-tests.diff added

(Contrived) failing test

comment:1 Changed 8 years ago by Michael Newman

Summary: r10737 opens file everytime a variable is looked upAfter 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 Changed 8 years ago by Michael Newman

Needs documentation: set
Needs tests: set
Patch needs improvement: set

Changed 8 years ago by Armin Ronacher

Attachment: 11084-proposed-fix.patch added

untested proposal for a quick fix.

comment:3 Changed 8 years ago by Michael Newman

Version: 1.0SVN

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 Changed 8 years ago by Alex Gaynor

milestone: 1.1
Triage Stage: UnreviewedAccepted

comment:5 Changed 8 years ago by arthur@…

Cc: arthur@… added

comment:6 Changed 8 years ago by Jeremy Dunck

Cc: jdunck@… added

Changed 8 years ago by Jeremy Dunck

Attachment: 11084.diff added

Patch was backwards, fixed now with passing tests.

comment:7 Changed 8 years ago by onemoreryan

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.

Changed 8 years ago by onemoreryan

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 Changed 8 years ago by anonymous

Cc: samuel@… added

comment:9 Changed 8 years ago by Gary Wilson

Owner: changed from nobody to Gary Wilson
Status: newassigned

comment:10 Changed 8 years ago by Gary Wilson

Resolution: fixed
Status: assignedclosed

(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 on ImageField.
  • New ImageField.update_dimension_fields method is attached to model instance's post_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 within model_fields.

comment:11 Changed 7 years ago by ccahoon

(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 on ImageField.
  • New ImageField.update_dimension_fields method is attached to model instance's post_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 within model_fields.

comment:12 Changed 5 years ago by Jacob

milestone: 1.1

Milestone 1.1 deleted

Note: See TracTickets for help on using tickets.
Back to Top