Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#18177 closed New feature (fixed)

Discovering a relation should cache the originator if applicable

Reported by: kaiser.yann@… Owned by: Aymeric Augustin
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords: related, cache, ForeignKey, OneToOneField, prefetch_related
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Currently, related fields will not cache the originator object (which holds the field) into the returned objects. This patch makes reverse ForeignKey and OneToOneField relations cache the originator(s) into the queryset, and then in turn to the returned objects.

In example, it validates this test, with Pool objects having a ForeignKey to Tournament objects:

    def test_fk(self):
        with self.assertNumQueries(2):
            tournament = Tournament.objects.get(pk=1)
            pool = tournament.pool_set.all()[0]
            self.assertIs(tournament, pool.tournament)

Without this patch, the expression pool.tournament would trigger a third query and create a different instance. It also works with .prefetch_related() calls.

Attachments (3)

prefetch_related_cache.patch (6.9 KB ) - added by kaiser.yann@… 12 years ago.
Patch
prefetch_related_cache_with_tests.patch (13.4 KB ) - added by kaiser.yann@… 12 years ago.
Patch against r17916 (Forgot to add the tests first time around.)
18177.patch (14.9 KB ) - added by Aymeric Augustin 12 years ago.

Download all attachments as: .zip

Change History (10)

by kaiser.yann@…, 12 years ago

Patch

in reply to:  description comment:1 by kaiser.yann@…, 12 years ago

Er, apparently, this was fixed while I was writing this patch for cases where prefetch_related was not used. The attached patch which applies against r17916 only adds the functionality for when prefetch_related is used. Imagine there's a .prefetch_related('pool_set') on the third line of the example :)

    def test_fk_prefetch_related(self):
        with self.assertNumQueries(2):
            tournament = (
                Tournament.objects.prefetch_related('pool_set')
                .get(pk=1)
                )
            pool = tournament.pool_set.all()[0]
            self.assertIs(tournament, pool.tournament)

by kaiser.yann@…, 12 years ago

Patch against r17916 (Forgot to add the tests first time around.)

comment:2 by Aymeric Augustin, 12 years ago

Triage Stage: UnreviewedAccepted

Yes, I touched that code in r17890 and r17899, but only for single objects, not for sets.

comment:3 by Aymeric Augustin, 12 years ago

Owner: changed from nobody to Aymeric Augustin

comment:4 by Aymeric Augustin, 12 years ago

First, thanks for your work this patch! It was really helpful.

To be honest, I didn't really understand the changes at first sight, I used your tests (they are great) and I tried to get them to pass. The goal was to see if I'd end up with the same patch.

I quickly noticed that, given the internal API of prefetch_related, this problem is difficult to fix. prefetch_one_level will assign a single related object or a list of related objects in a cache. Unfortunately, accessing the cache directly bypasses the getters and setters of the relation descriptors. This gives us two options:

  • emulate the appropriate bits of the setters in get_prefetch_query_set();
  • change the API of get_prefetch_query_set() to return a setter function rather than the name of the cache attribute; then the setter could do whatever necessary.

The first option is the least invasive, and it led me to the patch I'm going to attach in a few minutes.

This patch does three things:

  • It introduces a new attribute, QuerySet._known_related_object. This is only used to automatically set the reverse relation when creating objects through a related manager. When you're accessing tournament.pool_set.all(), tournament.pool_set.all()._known_related_object == ('tournament', tournament), so each pool objects gets its 'tournament' attribute set to the original tournament instance.
  • It caches the reverse relation in several get_prefetch_query_set() methods, as explained above. This isn't implemented for many-to-many relations since there's no way to be sure that we have all the objects we need to build the reverse relation.
  • It cleans up a little bit the code of the QuerySet class.

NB: I haven't dealt with GFKs.

Last edited 12 years ago by Aymeric Augustin (previous) (diff)

by Aymeric Augustin, 12 years ago

Attachment: 18177.patch added

comment:5 by Aymeric Augustin, 12 years ago

Triage Stage: AcceptedReady for checkin

comment:6 by Aymeric Augustin <aymeric.augustin@…>, 12 years ago

Resolution: fixed
Status: newclosed

In [1e6c3368f2517f32b0651d68277ea8c9ef81d9b2]:

Fixed #18177 -- Cached known related instances.

This was recently fixed for one-to-one relations; this patch adds
support for foreign keys. Thanks kaiser.yann for the report and
the initial version of the patch.

comment:7 by Aymeric Augustin, 12 years ago

This was first reported in #3470.

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