Code

Opened 2 years ago

Closed 2 years ago

Last modified 21 months ago

#18177 closed New feature (fixed)

Discovering a relation should cache the originator if applicable

Reported by: kaiser.yann@… Owned by: aaugustin
Component: Database layer (models, ORM) Version: master
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@… 2 years ago.
Patch
prefetch_related_cache_with_tests.patch (13.4 KB) - added by kaiser.yann@… 2 years ago.
Patch against r17916 (Forgot to add the tests first time around.)
18177.patch (14.9 KB) - added by aaugustin 2 years ago.

Download all attachments as: .zip

Change History (10)

Changed 2 years ago by kaiser.yann@…

Patch

comment:1 in reply to: ↑ description Changed 2 years ago by kaiser.yann@…

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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)

Changed 2 years ago by kaiser.yann@…

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

comment:2 Changed 2 years ago by aaugustin

  • Triage Stage changed from Unreviewed to Accepted

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

comment:3 Changed 2 years ago by aaugustin

  • Owner changed from nobody to aaugustin

comment:4 Changed 2 years ago by aaugustin

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.
  • It cleans up a little bit the code of the QuerySet class.

NB: I haven't dealt with GFKs.

Version 1, edited 2 years ago by aaugustin (previous) (next) (diff)

Changed 2 years ago by aaugustin

comment:5 Changed 2 years ago by aaugustin

  • Triage Stage changed from Accepted to Ready for checkin

comment:6 Changed 2 years ago by Aymeric Augustin <aymeric.augustin@…>

  • Resolution set to fixed
  • Status changed from new to closed

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 Changed 21 months ago by aaugustin

This was first reported in #3470.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.