From 38575b007a722d6af510ea46d46393a4cda9ca29 Mon Sep 17 00:00:00 2001 From: Paulo Date: Thu, 2 Jun 2016 17:44:47 -0700 Subject: [PATCH] Fixed #15250 -- Avoided extra query on some multi-table inheritance queries. Thanks marekw2143 for the initial patch and carljm for support. --- django/db/models/fields/related.py | 9 ++-- .../db/models/fields/related_descriptors.py | 46 ++++++++++++++----- tests/model_inheritance_regress/tests.py | 32 +++++++++++++ tests/proxy_models/tests.py | 2 +- 4 files changed, 74 insertions(+), 15 deletions(-) diff --git a/django/db/models/fields/related.py b/django/db/models/fields/related.py index 8add38d6b0..44e053847a 100644 --- a/django/db/models/fields/related.py +++ b/django/db/models/fields/related.py @@ -21,8 +21,9 @@ from django.utils.version import get_docs_version from . import Field from .related_descriptors import ( - ForwardManyToOneDescriptor, ManyToManyDescriptor, - ReverseManyToOneDescriptor, ReverseOneToOneDescriptor, + ForwardManyToOneDescriptor, ForwardOneToOneDescriptor, + ManyToManyDescriptor, ReverseManyToOneDescriptor, + ReverseOneToOneDescriptor, ) from .related_lookups import ( RelatedExact, RelatedGreaterThan, RelatedGreaterThanOrEqual, RelatedIn, @@ -437,6 +438,7 @@ class ForeignObject(RelatedField): requires_unique_target = True related_accessor_class = ReverseManyToOneDescriptor + forward_related_accessor_class = ForwardManyToOneDescriptor rel_class = ForeignObjectRel def __init__(self, to, on_delete, from_fields, to_fields, rel=None, related_name=None, @@ -698,7 +700,7 @@ class ForeignObject(RelatedField): def contribute_to_class(self, cls, name, private_only=False, **kwargs): super(ForeignObject, self).contribute_to_class(cls, name, private_only=private_only, **kwargs) - setattr(cls, self.name, ForwardManyToOneDescriptor(self)) + setattr(cls, self.name, self.forward_related_accessor_class(self)) def contribute_to_related_class(self, cls, related): # Internal FK's - i.e., those with a related name ending with '+' - @@ -969,6 +971,7 @@ class OneToOneField(ForeignKey): one_to_one = True related_accessor_class = ReverseOneToOneDescriptor + forward_related_accessor_class = ForwardOneToOneDescriptor rel_class = OneToOneRel description = _("One-to-one relationship") diff --git a/django/db/models/fields/related_descriptors.py b/django/db/models/fields/related_descriptors.py index 2bac02c003..82610508c6 100644 --- a/django/db/models/fields/related_descriptors.py +++ b/django/db/models/fields/related_descriptors.py @@ -23,18 +23,21 @@ reverse many-to-one relation. There are three types of relations (many-to-one, one-to-one, and many-to-many) and two directions (forward and reverse) for a total of six combinations. -1. Related instance on the forward side of a many-to-one or one-to-one - relation: ``ForwardManyToOneDescriptor``. +1. Related instance on the forward side of a many-to-one relation: + ``ForwardManyToOneDescriptor``. Uniqueness of foreign key values is irrelevant to accessing the related instance, making the many-to-one and one-to-one cases identical as far as the descriptor is concerned. The constraint is checked upstream (unicity validation in forms) or downstream (unique indexes in the database). - If you're looking for ``ForwardOneToOneDescriptor``, use - ``ForwardManyToOneDescriptor`` instead. +2. Related instance on the forward side of a one-to-one + relation: ``ForwardOneToOneDescriptor``. -2. Related instance on the reverse side of a one-to-one relation: + It avoids querying the database when accessing the parent link field in + a multi-table inheritance scenario. + +3. Related instance on the reverse side of a one-to-one relation: ``ReverseOneToOneDescriptor``. One-to-one relations are asymmetrical, despite the apparent symmetry of the @@ -42,13 +45,13 @@ and two directions (forward and reverse) for a total of six combinations. one table to another. As a consequence ``ReverseOneToOneDescriptor`` is slightly different from ``ForwardManyToOneDescriptor``. -3. Related objects manager for related instances on the reverse side of a +4. Related objects manager for related instances on the reverse side of a many-to-one relation: ``ReverseManyToOneDescriptor``. Unlike the previous two classes, this one provides access to a collection of objects. It returns a manager rather than an instance. -4. Related objects manager for related instances on the forward or reverse +5. Related objects manager for related instances on the forward or reverse sides of a many-to-many relation: ``ManyToManyDescriptor``. Many-to-many relations are symmetrical. The syntax of Django models @@ -151,6 +154,11 @@ class ForwardManyToOneDescriptor(object): setattr(rel_obj, rel_obj_cache_name, instance) return queryset, rel_obj_attr, instance_attr, True, self.cache_name + def get_object(self, instance): + qs = self.get_queryset(instance=instance) + # Assuming the database enforces foreign keys, this won't fail. + return qs.get(self.field.get_reverse_related_filter(instance)) + def __get__(self, instance, cls=None): """ Get the related instance through the forward relation. @@ -174,10 +182,7 @@ class ForwardManyToOneDescriptor(object): if None in val: rel_obj = None else: - qs = self.get_queryset(instance=instance) - qs = qs.filter(self.field.get_reverse_related_filter(instance)) - # Assuming the database enforces foreign keys, this won't fail. - rel_obj = qs.get() + rel_obj = self.get_object(instance) # If this is a one-to-one relation, set the reverse accessor # cache on the related object to the current instance to avoid # an extra SQL query if it's accessed later on. @@ -259,6 +264,25 @@ class ForwardManyToOneDescriptor(object): setattr(value, self.field.remote_field.get_cache_name(), instance) +class ForwardOneToOneDescriptor(ForwardManyToOneDescriptor): + + def get_object(self, instance): + if self.field.remote_field.parent_link: + deferred = instance.get_deferred_fields() + # Because it's a parent link, all the data is available in the + # instance, so populate the parent model with this data. + rel_model = self.field.remote_field.model + fields = [field.attname for field in rel_model._meta.concrete_fields] + + # If any of the related model's fields are deferred, fallback to + # fetching all fields from the related model. This avoids a query + # on the related model for every deferred field. + if not any(field in fields for field in deferred): + kwargs = {field: getattr(instance, field) for field in fields} + return rel_model(**kwargs) + return super(ForwardOneToOneDescriptor, self).get_object(instance) + + class ReverseOneToOneDescriptor(object): """ Accessor to the related object on the reverse side of a one-to-one diff --git a/tests/model_inheritance_regress/tests.py b/tests/model_inheritance_regress/tests.py index 7499e4e42f..d2a1fc29fd 100644 --- a/tests/model_inheritance_regress/tests.py +++ b/tests/model_inheritance_regress/tests.py @@ -496,3 +496,35 @@ class ModelInheritanceTest(TestCase): r.supplier_set.all(), [s], lambda x: x, ) + + def test_queries_on_parent_access(self): + italian_restaurant = ItalianRestaurant.objects.create( + name="Guido's House of Pasta", + address='944 W. Fullerton', + serves_hot_dogs=True, + serves_pizza=False, + serves_gnocchi=True, + ) + + # No queries are made when accessing the parent objects. + italian_restaurant = ItalianRestaurant.objects.get(pk=italian_restaurant.pk) + with self.assertNumQueries(0): + restaurant = italian_restaurant.restaurant_ptr + self.assertEqual(restaurant.place_ptr.restaurant, restaurant) + self.assertEqual(restaurant.italianrestaurant, italian_restaurant) + + # One query is made when accessing the parent objects when the instance + # is deferred. + italian_restaurant = ItalianRestaurant.objects.only('serves_gnocchi').get(pk=italian_restaurant.pk) + with self.assertNumQueries(1): + restaurant = italian_restaurant.restaurant_ptr + self.assertEqual(restaurant.place_ptr.restaurant, restaurant) + self.assertEqual(restaurant.italianrestaurant, italian_restaurant) + + # No queries are made when accessing the parent objects when the + # instance has deferred a field not present in the parent table. + italian_restaurant = ItalianRestaurant.objects.defer('serves_gnocchi').get(pk=italian_restaurant.pk) + with self.assertNumQueries(0): + restaurant = italian_restaurant.restaurant_ptr + self.assertEqual(restaurant.place_ptr.restaurant, restaurant) + self.assertEqual(restaurant.italianrestaurant, italian_restaurant) diff --git a/tests/proxy_models/tests.py b/tests/proxy_models/tests.py index ecbb70c7c4..833cdf3956 100644 --- a/tests/proxy_models/tests.py +++ b/tests/proxy_models/tests.py @@ -384,7 +384,7 @@ class ProxyModelAdminTests(TestCase): tracker_user = TrackerUser.objects.all()[0] base_user = BaseUser.objects.all()[0] issue = Issue.objects.all()[0] - with self.assertNumQueries(7): + with self.assertNumQueries(6): collector = admin.utils.NestedObjects('default') collector.collect(ProxyTrackerUser.objects.all()) self.assertIn(tracker_user, collector.edges.get(None, ()))