From 90c7aa074095311862b71f3b4ee7220369785375 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anssi=20K=C3=A4=C3=A4ri=C3=A4inen?= Date: Thu, 22 Nov 2012 20:27:28 +0200 Subject: [PATCH] [1.5.x] Fixed #18375 -- Removed dict-ordering dependency for F-expressions F() expressions reuse joins like any lookup in a .filter() call - reuse multijoins generated in the same .filter() call else generate new joins. Also, lookups can now reuse joins generated by F(). This change is backwards incompatible, but it is required to prevent dict randomization from generating different queries depending on .filter() kwarg ordering. The new way is also more consistent in how joins are reused. Backpatch of 90b86291d022a09031d1df397d7aaebc30e435f7 --- django/db/models/sql/expressions.py | 11 +++++--- django/db/models/sql/query.py | 2 +- docs/releases/1.5.txt | 6 +++++ tests/modeltests/expressions/tests.py | 39 +++++++++++++++++++++++++++ 4 files changed, 53 insertions(+), 5 deletions(-) diff --git a/django/db/models/sql/expressions.py b/django/db/models/sql/expressions.py index 374509914d..284e09a49a 100644 --- a/django/db/models/sql/expressions.py +++ b/django/db/models/sql/expressions.py @@ -3,12 +3,13 @@ from django.db.models.constants import LOOKUP_SEP from django.db.models.fields import FieldDoesNotExist class SQLEvaluator(object): - def __init__(self, expression, query, allow_joins=True): + def __init__(self, expression, query, allow_joins=True, reuse=None): self.expression = expression self.opts = query.get_meta() self.cols = [] self.contains_aggregate = False + self.reuse = reuse self.expression.prepare(self, query, allow_joins) def prepare(self): @@ -48,11 +49,13 @@ class SQLEvaluator(object): self.cols.append((node, query.aggregate_select[node.name])) else: try: + dupe_multis = False if self.reuse is None else True field, source, opts, join_list, last, _ = query.setup_joins( - field_list, query.get_meta(), - query.get_initial_alias(), False) + field_list, query.get_meta(), query.get_initial_alias(), + dupe_multis, can_reuse=self.reuse) col, _, join_list = query.trim_joins(source, join_list, last, False) - + if self.reuse is not None: + self.reuse.update(join_list) self.cols.append((node, (join_list[-1], col))) except FieldDoesNotExist: raise FieldError("Cannot resolve keyword %r into field. " diff --git a/django/db/models/sql/query.py b/django/db/models/sql/query.py index f5a477f2a3..0d79230391 100644 --- a/django/db/models/sql/query.py +++ b/django/db/models/sql/query.py @@ -1099,7 +1099,7 @@ class Query(object): value = value() elif isinstance(value, ExpressionNode): # If value is a query expression, evaluate it - value = SQLEvaluator(value, self) + value = SQLEvaluator(value, self, reuse=can_reuse) having_clause = value.contains_aggregate for alias, aggregate in self.aggregates.items(): diff --git a/docs/releases/1.5.txt b/docs/releases/1.5.txt index e26b927ae7..ca6b3acfd9 100644 --- a/docs/releases/1.5.txt +++ b/docs/releases/1.5.txt @@ -589,6 +589,12 @@ Miscellaneous :ref:`Q() expressions ` and ``QuerySet`` combining where the operators are used as boolean AND and OR operators. +* In a ``filter()`` call, when :ref:`F() expressions ` + contained lookups spanning multi-valued relations, they didn't always reuse + the same relations as other lookups along the same chain. This was changed, + and now F() expressions will always use the same relations as other lookups + within the same ``filter()`` call. + * The :ttag:`csrf_token` template tag is no longer enclosed in a div. If you need HTML validation against pre-HTML5 Strict DTDs, you should add a div around it in your pages. diff --git a/tests/modeltests/expressions/tests.py b/tests/modeltests/expressions/tests.py index 99eb07e370..ca47521ccd 100644 --- a/tests/modeltests/expressions/tests.py +++ b/tests/modeltests/expressions/tests.py @@ -219,3 +219,42 @@ class ExpressionsTests(TestCase): ) acme.num_employees = F("num_employees") + 16 self.assertRaises(TypeError, acme.save) + + def test_ticket_18375_join_reuse(self): + # Test that reverse multijoin F() references and the lookup target + # the same join. Pre #18375 the F() join was generated first, and the + # lookup couldn't reuse that join. + qs = Employee.objects.filter( + company_ceo_set__num_chairs=F('company_ceo_set__num_employees')) + self.assertEqual(str(qs.query).count('JOIN'), 1) + + def test_ticket_18375_kwarg_ordering(self): + # The next query was dict-randomization dependent - if the "gte=1" + # was seen first, then the F() will reuse the join generated by the + # gte lookup, if F() was seen first, then it generated a join the + # other lookups could not reuse. + qs = Employee.objects.filter( + company_ceo_set__num_chairs=F('company_ceo_set__num_employees'), + company_ceo_set__num_chairs__gte=1) + self.assertEqual(str(qs.query).count('JOIN'), 1) + + def test_ticket_18375_kwarg_ordering_2(self): + # Another similar case for F() than above. Now we have the same join + # in two filter kwargs, one in the lhs lookup, one in F. Here pre + # #18375 the amount of joins generated was random if dict + # randomization was enabled, that is the generated query dependend + # on which clause was seen first. + qs = Employee.objects.filter( + company_ceo_set__num_employees=F('pk'), + pk=F('company_ceo_set__num_employees') + ) + self.assertEqual(str(qs.query).count('JOIN'), 1) + + def test_ticket_18375_chained_filters(self): + # Test that F() expressions do not reuse joins from previous filter. + qs = Employee.objects.filter( + company_ceo_set__num_employees=F('pk') + ).filter( + company_ceo_set__num_employees=F('company_ceo_set__num_employees') + ) + self.assertEqual(str(qs.query).count('JOIN'), 2)