diff --git a/django/db/models/sql/compiler.py b/django/db/models/sql/compiler.py index 4d846fb438f..4a12d744521 100644 --- a/django/db/models/sql/compiler.py +++ b/django/db/models/sql/compiler.py @@ -6,7 +6,7 @@ from django.db.backends.util import truncate_name 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 @@ class SQLCompiler(object): 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 @@ class SQLCompiler(object): 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 @@ class SQLCompiler(object): 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( diff --git a/django/db/models/sql/constants.py b/django/db/models/sql/constants.py index 1c34f701691..9f82f426edb 100644 --- a/django/db/models/sql/constants.py +++ b/django/db/models/sql/constants.py @@ -44,6 +44,3 @@ ORDER_DIR = { 'ASC': ('ASC', 'DESC'), 'DESC': ('DESC', 'ASC'), } - -# A marker for join-reusability. -REUSE_ALL = object() diff --git a/django/db/models/sql/expressions.py b/django/db/models/sql/expressions.py index af7e45e74e2..a4c1d85c654 100644 --- a/django/db/models/sql/expressions.py +++ b/django/db/models/sql/expressions.py @@ -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 @@ class SQLEvaluator(object): 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: diff --git a/django/db/models/sql/query.py b/django/db/models/sql/query.py index ff56211c5dc..a030112e757 100644 --- a/django/db/models/sql/query.py +++ b/django/db/models/sql/query.py @@ -20,7 +20,7 @@ from django.db.models.fields import FieldDoesNotExist 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 @@ class Query(object): """ 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 @@ class Query(object): 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 @@ class Query(object): """ 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 @@ class Query(object): # 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 @@ class Query(object): 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 @@ class Query(object): 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 @@ class Query(object): 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 @@ class Query(object): 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 @@ class Query(object): 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 @@ class Query(object): 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 diff --git a/django/db/models/sql/subqueries.py b/django/db/models/sql/subqueries.py index 39d1ee01163..60728046977 100644 --- a/django/db/models/sql/subqueries.py +++ b/django/db/models/sql/subqueries.py @@ -232,7 +232,6 @@ class DateQuery(Query): field_name.split(LOOKUP_SEP), self.get_meta(), self.get_initial_alias(), - False ) except FieldError: raise FieldDoesNotExist("%s has no field named '%s'" % ( diff --git a/tests/regressiontests/generic_relations_regress/tests.py b/tests/regressiontests/generic_relations_regress/tests.py index 4c0f024433c..262c2e49179 100644 --- a/tests/regressiontests/generic_relations_regress/tests.py +++ b/tests/regressiontests/generic_relations_regress/tests.py @@ -72,5 +72,11 @@ class GenericRelationTests(TestCase): 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) diff --git a/tests/regressiontests/queries/tests.py b/tests/regressiontests/queries/tests.py index 75e27769b4a..92709558771 100644 --- a/tests/regressiontests/queries/tests.py +++ b/tests/regressiontests/queries/tests.py @@ -2418,3 +2418,36 @@ class ReverseJoinTrimmingTest(TestCase): 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)