Fix #8307 -- Saving a model with an ImageField with width_field or height_field no longer results in an extra read operation

[john-parton]

Trac ticket number

ticket-8307

Branch description

This is an alternate approach from #17775 to solving 8307.

Checklist

• This PR targets the main branch.
• The commit message is written in past tense, mentions the ticket number, and ends with a period.
• I have checked the "Has patch" ticket flag in the Trac system.
• I have added or updated relevant tests.
• I have added or updated relevant docs, including release notes if applicable.
• For UI changes, I have attached screenshots in both light and dark modes.

[john-parton]

Updated docs here.

The reason I moved the logic to the descriptor, is because in order to get the behavior correct, the logic needs both the value before and after the value is set. A previous pull request I had open instead modified the ImageField.update_image_dimensions method, requiring that the initial file be passed in as an additional kwarg. It also required an additional container class to tell the descriptor when to pass the original file.

Should I introduce I setting to opt-in to this new behavior? Or is it safe enough to just update?

[sarahboyce]

Hi @john-parton thank you for this ⭐ I have initial comments

• Close the other PR if this is the preferred approach?
• I wanted to start reviewing this PR as I know you were waiting for some feedback, can you give an overview on how these two relate https://code.djangoproject.com/ticket/35384?

There are some small stylistic things that we need to update about our Python style. Including ending comments with full stops etc.

[john-parton]

Hi @john-parton thank you for this ⭐ I have initial comments

* Close the other PR if this is the preferred approach?

Done

* I wanted to start reviewing this PR as I know you were waiting for some feedback, can you give an overview on how these two relate https://code.djangoproject.com/ticket/35384?

https://code.djangoproject.com/ticket/35384 is unrelated technically. It's just another issue I found while digging through the code. This PR doesn't close that ticket.

There are some small stylistic things that we need to update about our Python style. Including ending comments with full stops etc.

Understood

[john-parton]

Rebased and forced pushed to fix conflict with release notes.

[john-parton]

@sarahboyce I went ahead and removed all of the changes other than the "core" part of the fix you identified. It seems so obvious now, but it definitely isn't. I adjusted the comment slightly to help guide someone reading that part of the code.

I also removed all of the changes to other comments, so it's just those specific lines.

In referring to the documentation change I made:

Name of a model field which will be auto-populated with the height of the image each time the image attribute is set.

This is not because the behavior changed. This has been the behavior forever, but I wanted it to be clear that the height and width fields do not get updated when the model is saved if the image field isn't changed. That might seem obvious, but if you have a cron job that scans all your images and resizes them, then they won't get the updated sizes.

[sarahboyce]

@sarahboyce I went ahead and removed all of the changes other than the "core" part of the fix you identified. It seems so obvious now, but it definitely isn't. I adjusted the comment slightly to help guide someone reading that part of the code.
I also removed all of the changes to other comments, so it's just those specific lines.

Thank you so much this looks great 🥳
I have a few minor comments for us to add the final polish but it's looking really good

This is not because the behavior changed. This has been the behavior forever, but I wanted it to be clear that the height and width fields do not get updated when the model is saved if the image field isn't changed. That might seem obvious, but if you have a cron job that scans all your images and resizes them, then they won't get the updated sizes.

Ok thank you for highlighting. I think, let's pull those couple of lines into a separate PR. The reason being is sometimes document clarifications get back ported (to 5.0 for example) and if this is unrelated to your change, let's pull it out and deal with it separately.

After that, can you squash all commits into the one commit that has a message something like:

Fixed #8307 -- Prevented a read operation when saving ImageFields with width_field and height_field.

I will also get a second opinion on the release notes, docs and the decision not to deprecate.

[john-parton]

I moved the doc change which wasn't directly related to the bugfix here: #18112

I accepted all of your suggestions, and then rewrote the tests to use the model you suggested. Fixed linter errors, rebased and force-pushed with the commit message you suggested.

[sarahboyce]

Thank you John

I will also get a second opinion on the release notes, docs and the decision not to deprecate.

I have started a discussion here to get feedback on whether we need to deprecate.
For context, I am still quite new to the Fellow role so I'm sorry that I'm a little cautious/unsure about how best to proceed.

The PR looks great! If we do need to add a deprecation path, it shouldn't be too much work 👍

[adamchainz]

Great work on this old ticket!!

I have some small suggestions.

[john-parton]

• Merged in suggested changes
• tweaked tests (as requested) so they continue to pass

Looks like based on our discussion in the forum that most likely we'll want an example of how to restore the old behavior. Again, I know I'm a broken record: I can't imagine why you would WANT to do that, but at least we'll have our bases covered.

[john-parton]

Force pushed because CI failed (error in main or a bad merge? Not sure.)

[sarahboyce]

Force pushed because CI failed (error in main or a bad merge? Not sure.)

No problem, agreed unrelated to your changes 👍

[john-parton]

Mostly a note for myself: I mentioned adding docs somewhere to guide users on how to "force" django to re-read the image after it's been persisted to update the width and height fields. Still need to do that.

[adamchainz]

Please squash commits and retitle Fixed #8307 -- Prevented re-read when saving an ImageField. to follow the commit guidelines.

[john-parton]

Rebased, squashed, changed commit message.

In the future, should I go ahead and squash every time I patch? I usually leave the extra commits on in between reviews so that my individual changes can be seen.

[adamchainz]

I always squash every time I change, I think it’s what’s general done on GitHub. Otherwise the merger has to do it for you. The diff can be seen on GitHub by clicking the “Compare” link next to the message “John Parton force-pushed the bugfix/8307-anti-pattern-removal branch from 6dea56e to 9c4a122”.

[john-parton]

That's good info. Thanks. I'll make sure to be more religious with my squashing.