Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Fixed #19500 -- Solved a regression in join reuse

The ORM didn't reuse joins for direct foreign key traversals when using
chained filters. For example:
    qs.filter(fk__somefield=1).filter(fk__somefield=2))
produced two joins.

As a bonus, reverse onetoone filters can now reuse joins correctly

The regression was caused by the join() method refactor in commit
6884713

Thanks for Simon Charette for spotting some issues with the first draft
of the patch.
  • Loading branch information...
commit 3dcd435a0eb4380a3846e9c65140df480975687e 1 parent c04c03d
Anssi Kääriäinen akaariai authored
8 django/db/models/sql/compiler.py
View
@@ -6,7 +6,7 @@
from django.db.models.constants import LOOKUP_SEP
from django.db.models.query_utils import select_related_descend
from django.db.models.sql.constants import (SINGLE, MULTI, ORDER_DIR,
- GET_ITERATOR_CHUNK_SIZE, REUSE_ALL, SelectInfo)
+ GET_ITERATOR_CHUNK_SIZE, SelectInfo)
from django.db.models.sql.datastructures import EmptyResultSet
from django.db.models.sql.expressions import SQLEvaluator
from django.db.models.sql.query import get_order_dir, Query
@@ -317,7 +317,7 @@ def get_distinct(self):
for name in self.query.distinct_fields:
parts = name.split(LOOKUP_SEP)
- field, col, alias, _, _ = self._setup_joins(parts, opts, None)
+ field, col, alias, _, _ = self._setup_joins(parts, opts)
col, alias = self._final_join_removal(col, alias)
result.append("%s.%s" % (qn(alias), qn2(col)))
return result
@@ -450,7 +450,7 @@ def _setup_joins(self, pieces, opts, alias):
if not alias:
alias = self.query.get_initial_alias()
field, target, opts, joins, _ = self.query.setup_joins(
- pieces, opts, alias, REUSE_ALL)
+ pieces, opts, alias)
# We will later on need to promote those joins that were added to the
# query afresh above.
joins_to_promote = [j for j in joins if self.query.alias_refcount[j] < 2]
@@ -688,7 +688,7 @@ def fill_related_selections(self, opts=None, root_alias=None, cur_depth=1,
int_opts = int_model._meta
alias = self.query.join(
(alias, int_opts.db_table, lhs_col, int_opts.pk.column),
- promote=True,
+ promote=True
)
alias_chain.append(alias)
alias = self.query.join(
3  django/db/models/sql/constants.py
View
@@ -44,6 +44,3 @@
'ASC': ('ASC', 'DESC'),
'DESC': ('DESC', 'ASC'),
}
-
-# A marker for join-reusability.
-REUSE_ALL = object()
5 django/db/models/sql/expressions.py
View
@@ -1,10 +1,9 @@
from django.core.exceptions import FieldError
from django.db.models.constants import LOOKUP_SEP
from django.db.models.fields import FieldDoesNotExist
-from django.db.models.sql.constants import REUSE_ALL
class SQLEvaluator(object):
- def __init__(self, expression, query, allow_joins=True, reuse=REUSE_ALL):
+ def __init__(self, expression, query, allow_joins=True, reuse=None):
self.expression = expression
self.opts = query.get_meta()
self.cols = []
@@ -54,7 +53,7 @@ def prepare_leaf(self, node, query, allow_joins):
field_list, query.get_meta(),
query.get_initial_alias(), self.reuse)
col, _, join_list = query.trim_joins(source, join_list, path)
- if self.reuse is not None and self.reuse != REUSE_ALL:
+ if self.reuse is not None:
self.reuse.update(join_list)
self.cols.append((node, (join_list[-1], col)))
except FieldDoesNotExist:
39 django/db/models/sql/query.py
View
@@ -20,7 +20,7 @@
from django.db.models.loading import get_model
from django.db.models.sql import aggregates as base_aggregates_module
from django.db.models.sql.constants import (QUERY_TERMS, ORDER_DIR, SINGLE,
- ORDER_PATTERN, REUSE_ALL, JoinInfo, SelectInfo, PathInfo)
+ ORDER_PATTERN, JoinInfo, SelectInfo, PathInfo)
from django.db.models.sql.datastructures import EmptyResultSet, Empty, MultiJoin
from django.db.models.sql.expressions import SQLEvaluator
from django.db.models.sql.where import (WhereNode, Constraint, EverythingNode,
@@ -891,7 +891,7 @@ def count_active_tables(self):
"""
return len([1 for count in self.alias_refcount.values() if count])
- def join(self, connection, reuse=REUSE_ALL, promote=False,
+ def join(self, connection, reuse=None, promote=False,
outer_if_first=False, nullable=False, join_field=None):
"""
Returns an alias for the join in 'connection', either reusing an
@@ -902,10 +902,9 @@ def join(self, connection, reuse=REUSE_ALL, promote=False,
lhs.lhs_col = table.col
- The 'reuse' parameter can be used in three ways: it can be REUSE_ALL
- which means all joins (matching the connection) are reusable, it can
- be a set containing the aliases that can be reused, or it can be None
- which means a new join is always created.
+ The 'reuse' parameter can be either None which means all joins
+ (matching the connection) are reusable, or it can be a set containing
+ the aliases that can be reused.
If 'promote' is True, the join type for the alias will be LOUTER (if
the alias previously existed, the join type will be promoted from INNER
@@ -926,10 +925,8 @@ def join(self, connection, reuse=REUSE_ALL, promote=False,
"""
lhs, table, lhs_col, col = connection
existing = self.join_map.get(connection, ())
- if reuse == REUSE_ALL:
+ if reuse is None:
reuse = existing
- elif reuse is None:
- reuse = set()
else:
reuse = [a for a in existing if a in reuse]
for alias in reuse:
@@ -1040,7 +1037,7 @@ def add_aggregate(self, aggregate, model, alias, is_summary):
# then we need to explore the joins that are required.
field, source, opts, join_list, path = self.setup_joins(
- field_list, opts, self.get_initial_alias(), REUSE_ALL)
+ field_list, opts, self.get_initial_alias())
# Process the join chain to see if it can be trimmed
col, _, join_list = self.trim_joins(source, join_list, path)
@@ -1441,7 +1438,7 @@ def names_to_path(self, names, opts, allow_many=False,
raise MultiJoin(multijoin_pos + 1)
return path, final_field, target
- def setup_joins(self, names, opts, alias, can_reuse, allow_many=True,
+ def setup_joins(self, names, opts, alias, can_reuse=None, allow_many=True,
allow_explicit_fk=False):
"""
Compute the necessary table joins for the passage through the fields
@@ -1450,9 +1447,9 @@ def setup_joins(self, names, opts, alias, can_reuse, allow_many=True,
the table to start the joining from.
The 'can_reuse' defines the reverse foreign key joins we can reuse. It
- can be sql.constants.REUSE_ALL in which case all joins are reusable
- or a set of aliases that can be reused. Note that Non-reverse foreign
- keys are always reusable.
+ can be None in which case all joins are reusable or a set of aliases
+ that can be reused. Note that non-reverse foreign keys are always
+ reusable when using setup_joins().
If 'allow_many' is False, then any reverse foreign key seen will
generate a MultiJoin exception.
@@ -1485,8 +1482,9 @@ def setup_joins(self, names, opts, alias, can_reuse, allow_many=True,
else:
nullable = True
connection = alias, opts.db_table, from_field.column, to_field.column
- alias = self.join(connection, reuse=can_reuse, nullable=nullable,
- join_field=join_field)
+ reuse = None if direct or to_field.unique else can_reuse
+ alias = self.join(connection, reuse=reuse,
+ nullable=nullable, join_field=join_field)
joins.append(alias)
return final_field, target, opts, joins, path
@@ -1643,7 +1641,7 @@ def add_fields(self, field_names, allow_m2m=True):
try:
for name in field_names:
field, target, u2, joins, u3 = self.setup_joins(
- name.split(LOOKUP_SEP), opts, alias, REUSE_ALL, allow_m2m,
+ name.split(LOOKUP_SEP), opts, alias, None, allow_m2m,
True)
final_alias = joins[-1]
col = target.column
@@ -1729,8 +1727,9 @@ def add_count_column(self):
else:
opts = self.model._meta
if not self.select:
- count = self.aggregates_module.Count((self.join((None, opts.db_table, None, None)), opts.pk.column),
- is_summary=True, distinct=True)
+ count = self.aggregates_module.Count(
+ (self.join((None, opts.db_table, None, None)), opts.pk.column),
+ is_summary=True, distinct=True)
else:
# Because of SQL portability issues, multi-column, distinct
# counts need a sub-query -- see get_count() for details.
@@ -1934,7 +1933,7 @@ def set_start(self, start):
opts = self.model._meta
alias = self.get_initial_alias()
field, col, opts, joins, extra = self.setup_joins(
- start.split(LOOKUP_SEP), opts, alias, REUSE_ALL)
+ start.split(LOOKUP_SEP), opts, alias)
select_col = self.alias_map[joins[1]].lhs_join_col
select_alias = alias
1  django/db/models/sql/subqueries.py
View
@@ -232,7 +232,6 @@ def add_date_select(self, field_name, lookup_type, order='ASC'):
field_name.split(LOOKUP_SEP),
self.get_meta(),
self.get_initial_alias(),
- False
)
except FieldError:
raise FieldDoesNotExist("%s has no field named '%s'" % (
8 tests/regressiontests/generic_relations_regress/tests.py
View
@@ -72,5 +72,11 @@ def test_q_object_or(self):
Q(notes__note__icontains=r'other note'))
self.assertTrue(org_contact in qs)
-
+ def test_join_reuse(self):
+ qs = Person.objects.filter(
+ addresses__street='foo'
+ ).filter(
+ addresses__street='bar'
+ )
+ self.assertEqual(str(qs.query).count('JOIN'), 2)
33 tests/regressiontests/queries/tests.py
View
@@ -2418,3 +2418,36 @@ def test_reverse_trimming(self):
qs = Tag.objects.filter(annotation__tag=t.pk)
self.assertIn('INNER JOIN', str(qs.query))
self.assertEquals(list(qs), [])
+
+class JoinReuseTest(TestCase):
+ """
+ Test that the queries reuse joins sensibly (for example, direct joins
+ are always reused).
+ """
+ def test_fk_reuse(self):
+ qs = Annotation.objects.filter(tag__name='foo').filter(tag__name='bar')
+ self.assertEqual(str(qs.query).count('JOIN'), 1)
+
+ def test_fk_reuse_select_related(self):
+ qs = Annotation.objects.filter(tag__name='foo').select_related('tag')
+ self.assertEqual(str(qs.query).count('JOIN'), 1)
+
+ def test_fk_reuse_annotation(self):
+ qs = Annotation.objects.filter(tag__name='foo').annotate(cnt=Count('tag__name'))
+ self.assertEqual(str(qs.query).count('JOIN'), 1)
+
+ def test_fk_reuse_disjunction(self):
+ qs = Annotation.objects.filter(Q(tag__name='foo') | Q(tag__name='bar'))
+ self.assertEqual(str(qs.query).count('JOIN'), 1)
+
+ def test_fk_reuse_order_by(self):
+ qs = Annotation.objects.filter(tag__name='foo').order_by('tag__name')
+ self.assertEqual(str(qs.query).count('JOIN'), 1)
+
+ def test_revo2o_reuse(self):
+ qs = Detail.objects.filter(member__name='foo').filter(member__name='foo')
+ self.assertEqual(str(qs.query).count('JOIN'), 1)
+
+ def test_revfk_noreuse(self):
+ qs = Author.objects.filter(report__name='r4').filter(report__name='r1')
+ self.assertEqual(str(qs.query).count('JOIN'), 2)
Please sign in to comment.
Something went wrong with that request. Please try again.