From 5c481db29572a387651681b43d5d4523f96b3793 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anssi=20K=C3=A4=C3=A4ri=C3=A4inen?= Date: Tue, 7 Oct 2014 16:07:46 +0300 Subject: [PATCH] Fixed #23605 -- Fixed nested subquery regression Added relabeled_clone() method to sql.Query to fix the problem. It manifested itself in rare cases where at least double nested subquery's filter condition might target non-existing alias. Thanks to Trac alias ris for reporting the problem. --- django/db/models/sql/compiler.py | 2 +- django/db/models/sql/query.py | 21 ++++++++++++++++- docs/releases/1.7.2.txt | 3 +++ tests/queries/models.py | 15 ++++++++++++ tests/queries/tests.py | 39 +++++++++++++++++++++++++++++++- 5 files changed, 77 insertions(+), 3 deletions(-) diff --git a/django/db/models/sql/compiler.py b/django/db/models/sql/compiler.py index 0b75a9a7b41..4825fae3fa4 100644 --- a/django/db/models/sql/compiler.py +++ b/django/db/models/sql/compiler.py @@ -67,7 +67,7 @@ class SQLCompiler(object): if name in self.quote_cache: return self.quote_cache[name] if ((name in self.query.alias_map and name not in self.query.table_map) or - name in self.query.extra_select): + name in self.query.extra_select or name in self.query.external_aliases): self.quote_cache[name] = name return name r = self.connection.ops.quote_name(name) diff --git a/django/db/models/sql/query.py b/django/db/models/sql/query.py index ce932ddffb1..4702bc1945d 100644 --- a/django/db/models/sql/query.py +++ b/django/db/models/sql/query.py @@ -106,6 +106,10 @@ class Query(object): # type they are. The key is the alias of the joined table (possibly # the table name) and the value is JoinInfo from constants.py. self.alias_map = {} + # Sometimes the query contains references to aliases in outer queries (as + # a result of split_exclude). Correct alias quoting needs to know these + # aliases too. + self.external_aliases = set() self.table_map = {} # Maps table names to list of aliases. self.join_map = {} self.default_cols = True @@ -240,6 +244,7 @@ class Query(object): obj.model = self.model obj.alias_refcount = self.alias_refcount.copy() obj.alias_map = self.alias_map.copy() + obj.external_aliases = self.external_aliases.copy() obj.table_map = self.table_map.copy() obj.join_map = self.join_map.copy() obj.default_cols = self.default_cols @@ -303,6 +308,11 @@ class Query(object): obj._setup_query() return obj + def relabeled_clone(self, change_map): + clone = self.clone() + clone.change_aliases(change_map) + return clone + def get_aggregation(self, using, force_subq=False): """ Returns the dictionary with the values of the existing aggregations. @@ -774,7 +784,9 @@ class Query(object): ident = (change_map.get(ident[0], ident[0]),) + ident[1:] self.join_map[ident] = aliases for old_alias, new_alias in six.iteritems(change_map): - alias_data = self.alias_map[old_alias] + alias_data = self.alias_map.get(old_alias) + if alias_data is None: + continue alias_data = alias_data._replace(rhs_alias=new_alias) self.alias_refcount[new_alias] = self.alias_refcount[old_alias] del self.alias_refcount[old_alias] @@ -801,6 +813,9 @@ class Query(object): data = data._replace(lhs_alias=change_map[lhs]) self.alias_map[alias] = data + self.external_aliases = {change_map.get(alias, alias) + for alias in self.external_aliases} + def bump_prefix(self, outer_query): """ Changes the alias prefix to the next letter in the alphabet in a way @@ -999,6 +1014,9 @@ class Query(object): value = value() elif hasattr(value, 'resolve_expression'): value = value.resolve_expression(self, reuse=can_reuse) + # Subqueries need to use a different set of aliases than the + # outer query. Call bump_prefix to change aliases of the inner + # query (the value). if hasattr(value, 'query') and hasattr(value.query, 'bump_prefix'): value = value._clone() value.query.bump_prefix(self) @@ -1562,6 +1580,7 @@ class Query(object): lookup = lookup_class(Col(query.select[0].col[0], pk, pk), Col(alias, pk, pk)) query.where.add(lookup, AND) + query.external_aliases.add(alias) condition, needed_inner = self.build_filter( ('%s__in' % trimmed_prefix, query), diff --git a/docs/releases/1.7.2.txt b/docs/releases/1.7.2.txt index 8af832d1a56..cfa68b8b3a0 100644 --- a/docs/releases/1.7.2.txt +++ b/docs/releases/1.7.2.txt @@ -71,3 +71,6 @@ Bugfixes * Avoided unnecessary rollbacks of migrations from other apps when migrating backwards (:ticket:`23410`). + +* Fixed a rare query error when using deeply nested subqueries + (:ticket:`23605`). diff --git a/tests/queries/models.py b/tests/queries/models.py index fbd62217327..bc540756fb8 100644 --- a/tests/queries/models.py +++ b/tests/queries/models.py @@ -698,3 +698,18 @@ class Student(models.Model): class Classroom(models.Model): school = models.ForeignKey(School) students = models.ManyToManyField(Student, related_name='classroom') + + +class Ticket23605A(models.Model): + pass + + +class Ticket23605B(models.Model): + modela_fk = models.ForeignKey(Ticket23605A) + modelc_fk = models.ForeignKey("Ticket23605C") + field_b0 = models.IntegerField(null=True) + field_b1 = models.BooleanField(default=False) + + +class Ticket23605C(models.Model): + field_c0 = models.FloatField() diff --git a/tests/queries/tests.py b/tests/queries/tests.py index 7b4766519a0..6aa3770897e 100644 --- a/tests/queries/tests.py +++ b/tests/queries/tests.py @@ -28,7 +28,8 @@ from .models import ( JobResponsibilities, BaseA, FK1, Identifier, Program, Channel, Page, Paragraph, Chapter, Book, MyObject, Order, OrderItem, SharedConnection, Task, Staff, StaffUser, CategoryRelationship, Ticket21203Parent, Ticket21203Child, Person, - Company, Employment, CustomPk, CustomPkTag, Classroom, School, Student) + Company, Employment, CustomPk, CustomPkTag, Classroom, School, Student, + Ticket23605A, Ticket23605B, Ticket23605C) class BaseQuerysetTest(TestCase): @@ -3615,3 +3616,39 @@ class Ticket22429Tests(TestCase): queryset = Student.objects.filter(~Q(classroom__school=F('school'))) self.assertQuerysetEqual(queryset, [st2], lambda x: x) + + +class Ticket23605Tests(TestCase): + def test_ticket_23605(self): + # Test filtering on a complicated q-object from ticket's report. + # The query structure is such that we have multiple nested subqueries. + # The original problem was that the inner queries weren't relabeled + # correctly. + a1 = Ticket23605A.objects.create() + a2 = Ticket23605A.objects.create() + c1 = Ticket23605C.objects.create(field_c0=10000.0) + Ticket23605B.objects.create( + field_b0=10000.0, field_b1=True, + modelc_fk=c1, modela_fk=a1) + complex_q = Q(pk__in=Ticket23605A.objects.filter( + Q( + # True for a1 as field_b0 = 10000, field_c0=10000 + # False for a2 as no ticket23605b found + ticket23605b__field_b0__gte=1000000 / + F("ticket23605b__modelc_fk__field_c0") + ) & + # True for a1 (field_b1=True) + Q(ticket23605b__field_b1=True) & + ~Q(ticket23605b__pk__in=Ticket23605B.objects.filter( + ~( + # Same filters as above commented filters, but + # double-negated (one for Q() above, one for + # parentheses). So, again a1 match, a2 not. + Q(field_b1=True) & + Q(field_b0__gte=1000000 / F("modelc_fk__field_c0")) + ) + ))).filter(ticket23605b__field_b1=True)) + qs1 = Ticket23605A.objects.filter(complex_q) + self.assertQuerysetEqual(qs1, [a1], lambda x: x) + qs2 = Ticket23605A.objects.exclude(complex_q) + self.assertQuerysetEqual(qs2, [a2], lambda x: x)