Opened 6 years ago

Closed 6 years ago

Last modified 4 years ago

#11084 closed (fixed)

After r10737, ImageField sets height and width on model creation, every lookup

Reported by: Mnewman Owned by: gwilson
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 Mnewman 6 years ago.
(Contrived) failing test
11084-proposed-fix.patch (1.1 KB) - added by mitsuhiko 6 years ago.
untested proposal for a quick fix.
11084.diff (3.1 KB) - added by jdunck 6 years ago.
Patch was backwards, fixed now with passing tests.
broken.jpg (93.3 KB) - added by onemoreryan 6 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 6 years ago by Mnewman

(Contrived) failing test

comment:1 Changed 6 years ago by Mnewman

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Summary changed from r10737 opens file everytime a variable is looked up to 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 Changed 6 years ago by Mnewman

  • Needs documentation set
  • Needs tests set
  • Patch needs improvement set

Changed 6 years ago by mitsuhiko

untested proposal for a quick fix.

comment:3 Changed 6 years ago by Mnewman

  • Version changed from 1.0 to 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 Changed 6 years ago by Alex

  • milestone set to 1.1
  • Triage Stage changed from Unreviewed to Accepted

comment:5 Changed 6 years ago by arthur@…

  • Cc arthur@… added

comment:6 Changed 6 years ago by jdunck

  • Cc jdunck@… added

Changed 6 years ago by jdunck

Patch was backwards, fixed now with passing tests.

comment:7 Changed 6 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 6 years ago by onemoreryan

Example of a corrupt .jpg; an ImageField that references this file will cause a TypeError on that model's admin changelist

comment:8 Changed 6 years ago by anonymous

  • Cc samuel@… added

comment:9 Changed 6 years ago by gwilson

  • Owner changed from nobody to gwilson
  • Status changed from new to assigned

comment:10 Changed 6 years ago by gwilson

  • Resolution set to fixed
  • Status changed from assigned to 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 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 6 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 4 years ago by jacob

  • milestone 1.1 deleted

Milestone 1.1 deleted

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