preserve reverse_related_field caches when casting to a subclass

[screamndigit]

I was going to write a test for this but the tests currently fail up to 4 assertions when I run them without making a change. Anyway, I'm new at this whole pull thing so be gentle.

The issue is if i define a class with a related class:

class Opportunity(models.Model):
    objects=InheritanceManager()
    pass

class Contest(Opportunity):
    pass

class Terms(models.Model):
    opportunity = models.OneToOneField(Opportunity, related_name='contest_terms')

Calling:

contests = Opportunity.objects.select_related('contest_terms').filter(id=1)
t = contests[0].contest_terms
assert(len(connection.queries)==1)

works because it makes one DB hit and can access the contest_terms as it's been cached by select_related()
changing the query in the first line to include select_subclasses:

contests = Opportunity.objects.select_related('contest_terms').select_subclasses('contest').filter(id=1)

fails because the related object cache is lost after subclassing so it makes the additional hit to the DB to get the terms.

The fix I proposed will copy the object caches from the base class when _get_sub_obj_recurse finds the appropriate subclass. This will keep the results of select_related() in the returned model instance.

[carljm]

Hey! Thanks so much for the debugging work and the pull request! I'll need to look at this a bit more to make sure I understand the fix, it's using some pretty gnarly ORM internals that I haven't used before :-) It would really help to have a test. The tests on master are all passing here and on Travis CI -- maybe I could help debug why they aren't passing for you if you post a paste of the failures (and how you are running them).

I also notice that Travis CI is failing this pull request on Django 1.4; it looks like those ORM internals you're using might not exist on 1.4 :( I don't know how hard it will be to write this fix in such a way that it works on 1.4, but I'm not ready to drop Django 1.4 support in model-utils yet, especially since it's just been identified as a "long term support" release of Django. If all else fails, I'd be ok with writing the patch in such a way that this bug is only fixed on 1.5+, but it still needs to be version-guarded so things work as they did before on 1.4. Ideally of course we'd fix it everywhere.

[screamndigit]

understood - yeah, i'm using 1.5.2, but i dont think it'll be hard to figure out how to adapt it to 1.4.x. But you're right, the code is hosed on 1.4.x -- i'll see what i can do when i get a few minutes away from the day job.

The test failures I get (using django 1.4.2 - and they range from 2-5 failures on repeated runs) before changing any code from master are:

======================================================================
FAIL: test_save_changed (model_utils.tests.tests.MonitorFieldTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "c:\projects\django-model-utils\model_utils\tests\tests.py", line 157, in test_save_changed
    self.assertTrue(self.instance.name_changed > self.created)
AssertionError: False is not true

======================================================================
FAIL: test_created (model_utils.tests.tests.StatusModelPlainTupleTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "c:\projects\django-model-utils\model_utils\tests\tests.py", line 684, in test_created
    self.assertTrue(c2.status_changed > c1.status_changed)
AssertionError: False is not true

======================================================================
FAIL: test_created (model_utils.tests.tests.StatusModelTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "c:\projects\django-model-utils\model_utils\tests\tests.py", line 684, in test_created
    self.assertTrue(c2.status_changed > c1.status_changed)
AssertionError: False is not true

======================================================================
FAIL: test_created (model_utils.tests.tests.TimeStampedModelTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "c:\projects\django-model-utils\model_utils\tests\tests.py", line 619, in test_created
    self.assertTrue(t2.created > t1.created)
AssertionError: False is not true

----------------------------------------------------------------------
Ran 197 tests in 0.248s

[carljm]

Hmm interesting. They all seem to be date comparisons. Wonder if we have some tests with race conditions; odd that I've never seen a single failure like that in all the many times I've run the tests, though. Those tests have been there for a long time.

How are you running the tests? Using tox?

[carljm]

You aren't somehow running the tests on MySQL instead of SQLite, are you? That might cause that, due to MySQL's lack of microsecond precision on datetime fields?

[screamndigit]

I paused for a second when you asked about MySQL -- I use it by default, but i didn't do anything in running the tests to switch it from whatever the default is. So my assumption is that it stuck to SQLite.

the issue looks like it's because i was running it on windows. ubuntu tests fine. so ... no need to dig further. :)

[screamndigit]

ok this should now pass tests with 1.4, though i'm having trouble writing a working unit test given the models. at least not a dependable way to check connection query counts inside of the tests.

[carljm]

Django's test cases have self.assertNumQueries as a context manager, is that not working reliably for what you need? (You can find examples of its use in Django's own test suite).

[kezabelle]

I've pushed a private branch which contains a commit which should provide a testcase for the problem/fix as I understand it [rebased against master just now]

It's currently marked as an expected failure, if @screamndigit is interested in merging/copy-pasting/whatever and verifying that the test is fit for purpose, and that the proposed changeset fixes it and is covered by it, we could get it merge-ready.

[kezabelle]

It doesn't look like @screamndigit is going to come back to this.
@carljm once you've decompressed from PyCon, could you have a glance at 838493a and check the test is as you understood the problem? If it's OK, I'll pull the changes in so we get one complete branch for Travis to verify.

[carljm]

@kezabelle Yep, that test looks right to me; left some comments on it. If you'd be able to pull together a new pull request with both test and fix (and close this one), I'd be happy to review that.

Thanks again @screamndigit for the initial report and your work on the fix!

[RadoRado]

Any progress here :? Seems like an important fix for ORM performance.

[auvipy]

closing in favor of #127