Handle deferred fields in FieldTracker.has_changed() and previous()

[jcushman]

Problem

Currently has_changed() and previous() do not return correct values when working with Django deferred fields.

• previous() always returns None, even if the deferred value in the database is not None.

• has_changed() always returns True, even if the field has not changed. It also has the side-effect of unnecessarily fetching deferred fields from the database.

• This means that calling tracker.changed() unnecessarily loads all deferred fields, which can have a meaningful performance impact.

Here is a simplified real-life example that shows why this would be useful to fix:

for instance in MyModel.objects.defer('giant_text_field'):
    # ...
    if instance.needs_update:
        instance.giant_text_field = download_giant_text_file(instance)
    # ...
    if instance.tracker.changed():
        instance.save()

The goal here is to use defer() combined with tracker.changed() to avoid fetching and re-saving giant_text_field unless necessary, which has a huge performance impact for this kind of code. But because has_changed() always returns True for deferred fields, this code will fetch and store an unchanged giant_text_field for every instance.

Solution

I updated FieldTrackerTests.test_with_deferred() to show the different ways defer() can interact with has_changed() and previous(). I then updated has_changed() and previous() to handle those cases. The new behavior is that previous() returns the correct previous value for a deferred field, instead of None, and has_changed() correctly returns whether the field has had a different value assigned to it, while avoiding unnecessary database access.

Backwards compatibility

The new code will not affect anyone who is not using FieldTracker with deferred fields.

The change could impact existing users if they are relying on the old behavior, in particular if they are relying on previous() always returning None for deferred values. I don't think it would make sense to do that, but maybe it's possible?

I did not attempt to apply these fixed for the old ModelTracker.

Implementation details/coding decisions

There are two cases we have to handle here. The simpler is the read-only case: an object is fetched with defer() and then the tracker is consulted, without any local assignments to the deferred field. In that case we can just return False from has_changed(), because a deferred field by definition hasn't changed. For previous() we can just examine the value of the field and thereby un-defer it.

Things get more complicated if local code writes to the deferred field, as in my instance.giant_text_field = example above. If local code has written a value, then has_changed() should go ahead with the comparison using previous(). To return a correct result, previous() then has to actually fetch the old value from the database to find out if the new value is different.

So that leads to two implementation details. First, to test whether local code has written to the deferred field, I'm checking whether the field is in instance.__dict__, which is the test used by Django's get_deferred_fields() and thus is probably correct.

Second, to fetch the old value from the database after assignment, I'm using Django's refresh_from_db() function with a temp variable to shuffle around the values. This is messy, but I think cleaner than doing the fetch ourselves and re-implementing all the cases handled by refresh_from_db().

Commandments

• Write PEP8 compliant code.
• Cover it with tests.
• Update CHANGES.rst file to describe the changes, and quote according issue with GH-<issue_number>.
• Pay attention to backward compatibility, or if it breaks it, explain why.
• Update documentation (if relevant).

[codecov]

Codecov Report

Merging #313 into master will increase coverage by 0.21%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #313      +/-   ##
==========================================
+ Coverage   94.81%   95.02%   +0.21%     
==========================================
  Files           2        2              
  Lines         212      221       +9     
==========================================
+ Hits          201      210       +9     
  Misses         11       11

Impacted Files	Coverage Δ
model_utils/tracker.py	92.85% <100%> (+0.44%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1eff6d0...d34043f. Read the comment docs.