Opened 6 years ago

Closed 6 years ago

Last modified 3 years ago

#10404 closed (fixed)

ImageField: height_field and width_field option sometimes doesn't work

Reported by: Lehich Owned by: jacob
Component: File uploads/storage Version: master
Severity: Keywords: ImageField, height_field, width_field
Cc: mitsuhiko Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

I had a problem: height_field and width_field always = 0 in some models, after uploading any image file.
Problem was in fields order: if height_field and width_field goes before image field - this thing happends.

I appears in models save method.
/django/db/models/base.py : 378 (actual for revision: 9962)

values = [(f, None, f.get_db_prep_save(raw and getattr(self, f.attname) or f.pre_save(self, False))) for f in non_pks]

here all fields values are calculated (get_db_prep_save) and copy to values. So, if in models description ImageField goes before height and width fields, everything is fine. But if ImageField goes after this fields, all pic size calculation still in fields but not in values list.

Attachments (3)

10404_test.diff (837 bytes) - added by tclineks 6 years ago.
wherror.diff (844 bytes) - added by kmtracey 6 years ago.
10404.diff (3.0 KB) - added by anonymous 6 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 6 years ago by jacob

  • milestone set to 1.1
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

Changed 6 years ago by tclineks

comment:2 Changed 6 years ago by tclineks

  • Resolution set to worksforme
  • Status changed from new to closed

The attached test patch would expose this problem, it doesn't. Marking worksforme.

comment:3 Changed 6 years ago by kmtracey

  • Resolution worksforme deleted
  • Status changed from closed to reopened

I haven't looked in detail but somehow that existing test with the order reversed doesn't trigger this bug. I, however, can recreate the described problem manually using admin, so I do believe there is a problem here.

comment:4 Changed 6 years ago by kmtracey

Also, this looks to be another r9766 side-effect. While the line of code identified in the description hasn't changed, r9766 effectively moved the setting of these fields from early on (when form data was saved) to much later in the processing. Using r9765 I cannot recreate the error with admin even if the width and height fields are declared before the image field itself.

comment:5 Changed 6 years ago by kmtracey

The reason the patch to the file_storage tests doesn't trigger the error is because those tests call save() on the model image field directly. The problem introduced by r9766 is that the under-the-covers save() for the file/image fields got moved to much later in the processing, so you need a test that doesn't call the field save() directly but rather just attempts to call save() on the model (or form). That's when it becomes apparent that the save() on the image/file field is now happening too late in some cases. I'll attach a patch to the model_forms test that do trigger the error.

Changed 6 years ago by kmtracey

comment:6 Changed 6 years ago by mitsuhiko

  • Owner changed from nobody to mitsuhiko
  • Status changed from reopened to new

comment:7 Changed 6 years ago by anonymous

The attach patch does fix it (test suite runs through fine). Don't ask me if that's the best way to do it :þ

Changed 6 years ago by anonymous

comment:8 Changed 6 years ago by anonymous

  • Has patch set

comment:9 Changed 6 years ago by mitsuhiko

The actual fix for this is in http://github.com/mitsuhiko/django/tree/ticket-10404

It fixes it by using a descriptor in the image field, rather than pre saving multiple times.

comment:10 Changed 6 years ago by jacob

  • Owner changed from mitsuhiko to jacob
  • Status changed from new to assigned

comment:11 Changed 6 years ago by mitsuhiko

  • Cc mitsuhiko added

comment:12 Changed 6 years ago by jacob

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [10737]) Fixed #10404: ImageField height_field and width_field options no longer depend on putting the image field after the height/width fields as they did after r9766.

This bug actually exposed a related handful of inconsistancies in the underlying file handling and wraping, so a few related changes are in here as well:

  • Dimensions are also now calculated the moment the image is assigned to the field instead of upon save.
  • The base File object now when possible delegates its closed attribute down to the os-level file it wrapps.
  • In-memory files' close() now is a no-op. Without this certain APIs that should be able to handle in-memory files were failing.
  • Accessing FieldFile.closed used to open the file. That's silly, and it doesn't any more.
  • Some over-eager error handling was squishing some errors that would normally be raised. One unit test was incorrectly depending on this behavior, so the test was removed.

Thanks to Armin Ronacher for much of this work.

comment:13 Changed 3 years ago by jacob

  • milestone 1.1 deleted

Milestone 1.1 deleted

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