Fix handling of deferred fields on django 1.10+ #317

lucaswiman · 2018-04-03T23:24:41Z

Problem

Prior to Django 1.10, deferred attributes were handled by constructing a new model class with custom descriptors for the deferred attributes. After 1.10, deferred fields are tracked by whether the attribute is present in the instance __dict__.

FieldTracker handled tracking on these fields by overwriting the descriptors on the model class. This means that in 1.10+, the instance model class is the model, so FieldTracker can introduce changes to the base class whenever deferred fields are used that persist on other queries. #278 This is reproduced in a test case.

Solution

I updated the finalize_class method to wrap field descriptors. This preserves the custom descriptor behavior, and means that deferring fields does not lead to permanent changes in base model classes. This is only done for django versions after 1.10: the previous behavior is preserved for 1.8 and 1.9.

Since the relevant code branches behavior on unsupported versions of Django, I also removed all branching behavior for versions of django listed as unsupported in the CHANGELOG.

Commandments

Write PEP8 compliant code.
Cover it with tests. Reproduced in test cases, which fail prior to 54cc150 and pass afterwards.
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. The only backwards incompatible change is that the tracker._deferred_fields attribute is not set on django versions >=1.10. Since _deferred_fields is a private attribute, I wouldn't consider this a breaking change.
Update documentation (if relevant). _I don't think it is relevant.

Regarding PEP8, I ran flake8, and it generated a number of errors. I think my code didn't introduce new PEP8 violations, but I'm not sure. If you want, I can submit a separate PR fixing all the PEP8 violations, and rebase onto this branch.

codecov · 2018-04-03T23:28:17Z

Codecov Report

Merging #317 into master will increase coverage by 3.61%.
The diff coverage is 96.87%.

@@            Coverage Diff             @@
##           master     #317      +/-   ##
==========================================
+ Coverage   94.81%   98.42%   +3.61%     
==========================================
  Files           2        2              
  Lines         212      254      +42     
==========================================
+ Hits          201      250      +49     
+ Misses         11        4       -7

Impacted Files	Coverage Δ
model_utils/tracker.py	`97.86% <96.87%> (+5.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 e750fc7...2e92877. Read the comment docs.

lucaswiman · 2018-04-03T23:33:40Z

😬 I'll investigate why code coverage went down by so much.

lucaswiman · 2018-04-04T18:19:27Z

I fixed a few coverage issues, but I think maybe the codecov integration isn't running the full test suite? All of the code which only runs on older versions of django shows as "not covered". I think this is an artifact of the coverage setup, and not reflective of an inadequacy in the tests I added.

lucaswiman · 2018-04-04T18:25:09Z

Hmm, I wish I'd noticed #313 existed before starting on this. cc @jcushman

I think we're fixing different bugs with the handling of deferred fields, but the patches are slightly incompatible.

Support for older versions of django was dropped in 3.0.0.

This commit adds a collection of wrapper classes for tracking fields while still using custom descriptors that may be present. This fixes a bug where deferring a model field with a custom descriptor meant that the descriptor was overridden in all subsequent queries.

As far as I can tell, no changes to documentation is required.

The complications are that when the attribute is set in Django 1.10, it no longer counts as a deferred attribute, and it is not retrieved from the database. Naively updating __set__ to retrieve the value if it is deferred leads to infinite recursion because accessing the attribute involves loading data from the database and trying to set the attribute based on that value. This commit introduces a somewhat hacky flag that records whether we're already trying to set the attribute further up in the call stack.

This should maintain compatibility with the next version of django.

lucaswiman · 2018-06-28T20:34:04Z

@jcushman I think this now includes fixes from #313, updated to work on Django 1.10+. Could you take a look?

@carljm What is required to get this merged and deployed this to pypi?

Hopefully this will combine the coverage reports.

… all tox environments run on the travis environment. Note that it is run as one per python version, but multiple versions of django are tested in each.

lucaswiman · 2018-06-28T21:18:25Z

OK, I got the codecov checks passing by correctly aggregating the different test environments and adding a few assertions to the tests.

…s yields the descriptor object.

jcushman · 2018-06-30T00:49:18Z

Hi! I don't have my head super in this, but I see that my tests are there and passing, which is great. 👍

It sounds like you can join Jazzband and merge this yourself, if you're comfortable with the code at this point: #279 (comment)

Alternatively you might @ a few folks who have merged PRs in the last year for another pair of eyes.

jcushman · 2018-06-30T00:49:53Z

(Oh one nitpick, looks like the merge on CHANGES.rst put a line in the wrong place.)

lucaswiman · 2018-06-30T23:05:54Z

you might @ a few folks who have merged PRs in the last year for another pair of eyes.

@tony Would you care to take a look?

tony · 2018-07-01T18:50:40Z

@jcushman @lucaswiman I don't use the deferred fields part of django-model-utils.

I breezed through the code and it LGTM. I think that's the best I can offer.

I don't want to offer more since I don't have a Django website using that part of django-model-utils to test on fully.

lucaswiman · 2018-07-02T18:30:04Z

Thanks @tony and @jcushman! I'll merge this and fix pep8 in another PR.

Avoid fetching deferred fields in has_changed

d34043f

lucaswiman changed the title ~~Django 1.11 compatibility~~ Fix handling of deferred fields on django 1.10+ Apr 3, 2018

lucaswiman and others added 9 commits June 21, 2018 12:41

Remove version checks for django<1.8.

0fc0b44

Support for older versions of django was dropped in 3.0.0.

Add failing test for deferred attributes.

be52bc9

Update AUTHORS and CHANGES.

be1a7d9

As far as I can tell, no changes to documentation is required.

Improve coverage.

90ed7fc

Merge branch 'defer-has-changed' into django-1.11-compatibility

98a1366

Fix test failures from merge.

5d410e9

Handle API change in DeferredAttribute descriptor in django-trunk.

15f9393

This should maintain compatibility with the next version of django.

Lucas Wiman added 4 commits June 28, 2018 13:41

Update passed environment variables to match codecov documentation.

4740ab4

Hopefully this will combine the coverage reports.

Use --cov-append option in travis build to include coverage data from…

c16a275

… all tox environments run on the travis environment. Note that it is run as one per python version, but multiple versions of django are tested in each.

Correctly clean up recursion sentinel value.

59347ef

Cover a branch in has_changed.

cde1d70

Lucas Wiman added 2 commits June 28, 2018 16:59

Fix coverage for a codepath only executed in <1.10 environments.

ca2fbb4

Increase coverage: verify that accessing the descriptor from the clas…

7d6b45f

…s yields the descriptor object.

Put changelog in the right place.

45502c0

Merge branch 'master' into django-1.11-compatibility

2e92877

lucaswiman merged commit 16dec4d into jazzband:master Jul 2, 2018

lucaswiman deleted the django-1.11-compatibility branch July 2, 2018 18:30

lucaswiman mentioned this pull request Jul 2, 2018

Fix pep8 violations #329

Merged

jcushman mentioned this pull request Aug 3, 2018

Allow FieldTracker on Django user model (and other children of abstract base classes) #331

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix handling of deferred fields on django 1.10+ #317

Fix handling of deferred fields on django 1.10+ #317

lucaswiman commented Apr 3, 2018 •

edited

Loading

codecov bot commented Apr 3, 2018 •

edited

Loading

lucaswiman commented Apr 3, 2018

lucaswiman commented Apr 4, 2018

lucaswiman commented Apr 4, 2018

lucaswiman commented Jun 28, 2018

lucaswiman commented Jun 28, 2018

jcushman commented Jun 30, 2018

jcushman commented Jun 30, 2018

lucaswiman commented Jun 30, 2018

tony commented Jul 1, 2018

lucaswiman commented Jul 2, 2018

Fix handling of deferred fields on django 1.10+ #317

Fix handling of deferred fields on django 1.10+ #317

Conversation

lucaswiman commented Apr 3, 2018 • edited Loading

Problem

Solution

Commandments

codecov bot commented Apr 3, 2018 • edited Loading

Codecov Report

lucaswiman commented Apr 3, 2018

lucaswiman commented Apr 4, 2018

lucaswiman commented Apr 4, 2018

lucaswiman commented Jun 28, 2018

lucaswiman commented Jun 28, 2018

jcushman commented Jun 30, 2018

jcushman commented Jun 30, 2018

lucaswiman commented Jun 30, 2018

tony commented Jul 1, 2018

lucaswiman commented Jul 2, 2018

lucaswiman commented Apr 3, 2018 •

edited

Loading

codecov bot commented Apr 3, 2018 •

edited

Loading