From fd9050589f5f99d61bf1c68917a9157c1e8c3752 Mon Sep 17 00:00:00 2001 From: Simon Charette Date: Tue, 17 Dec 2019 01:21:13 -0500 Subject: [PATCH] [3.0.x] Fixed #31094 -- Included columns referenced by subqueries in GROUP BY on aggregations. Thanks Johannes Hoppe for the report. Regression in fb3f034f1c63160c0ff13c609acd01c18be12f80. Co-authored-by: Mariusz Felisiak Backport of 5a4d7285bd10bd40d9f7e574a7c421eb21094858 from master --- django/db/models/expressions.py | 2 +- django/db/models/sql/compiler.py | 9 ++++++- django/db/models/sql/query.py | 42 ++++++++++++++++++++------------ docs/releases/3.0.2.txt | 3 ++- tests/aggregation/tests.py | 20 +++++++++++++++ 5 files changed, 58 insertions(+), 18 deletions(-) diff --git a/django/db/models/expressions.py b/django/db/models/expressions.py index 2b59dd301a9..3adec334f70 100644 --- a/django/db/models/expressions.py +++ b/django/db/models/expressions.py @@ -1068,7 +1068,7 @@ class Subquery(Expression): def get_group_by_cols(self, alias=None): if alias: return [Ref(alias, self)] - return [] + return self.query.get_external_cols() class Exists(Subquery): diff --git a/django/db/models/sql/compiler.py b/django/db/models/sql/compiler.py index 2f4c5bed589..bceba45b7c7 100644 --- a/django/db/models/sql/compiler.py +++ b/django/db/models/sql/compiler.py @@ -110,7 +110,14 @@ class SQLCompiler: # Note that even if the group_by is set, it is only the minimal # set to group by. So, we need to add cols in select, order_by, and # having into the select in any case. + ref_sources = { + expr.source for expr in expressions if isinstance(expr, Ref) + } for expr, _, _ in select: + # Skip members of the select clause that are already included + # by reference. + if expr in ref_sources: + continue cols = expr.get_group_by_cols() for col in cols: expressions.append(col) @@ -400,7 +407,7 @@ class SQLCompiler: 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 or ( - name in self.query.external_aliases and name not in self.query.table_map)): + self.query.external_aliases.get(name) and name not in self.query.table_map)): 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 bbd32e29b10..cc1d4b1d382 100644 --- a/django/db/models/sql/query.py +++ b/django/db/models/sql/query.py @@ -163,7 +163,8 @@ class Query(BaseExpression): # 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() + # Map external tables to whether they are aliased. + self.external_aliases = {} self.table_map = {} # Maps table names to list of aliases. self.default_cols = True self.default_ordering = True @@ -856,8 +857,11 @@ class Query(BaseExpression): if alias == old_alias: table_aliases[pos] = new_alias break - self.external_aliases = {change_map.get(alias, alias) - for alias in self.external_aliases} + self.external_aliases = { + # Table is aliased or it's being changed and thus is aliased. + change_map.get(alias, alias): (aliased or alias in change_map) + for alias, aliased in self.external_aliases.items() + } def bump_prefix(self, outer_query): """ @@ -1031,19 +1035,23 @@ class Query(BaseExpression): for key, value in clone.annotations.items(): resolved = value.resolve_expression(query, *args, **kwargs) if hasattr(resolved, 'external_aliases'): - resolved.external_aliases.update(clone.alias_map) + resolved.external_aliases.update(clone.external_aliases) clone.annotations[key] = resolved # Outer query's aliases are considered external. - clone.external_aliases.update( - alias for alias, table in query.alias_map.items() - if ( - isinstance(table, Join) and table.join_field.related_model._meta.db_table != alias - ) or ( - isinstance(table, BaseTable) and table.table_name != table.table_alias + for alias, table in query.alias_map.items(): + clone.external_aliases[alias] = ( + (isinstance(table, Join) and table.join_field.related_model._meta.db_table != alias) or + (isinstance(table, BaseTable) and table.table_name != table.table_alias) ) - ) return clone + def get_external_cols(self): + exprs = chain(self.annotations.values(), self.where.children) + return [ + col for col in self._gen_cols(exprs) + if col.alias in self.external_aliases + ] + def as_sql(self, compiler, connection): sql, params = self.get_compiler(connection=connection).as_sql() if self.subquery: @@ -1638,12 +1646,16 @@ class Query(BaseExpression): return targets, joins[-1], joins @classmethod - def _gen_col_aliases(cls, exprs): + def _gen_cols(cls, exprs): for expr in exprs: if isinstance(expr, Col): - yield expr.alias + yield expr else: - yield from cls._gen_col_aliases(expr.get_source_expressions()) + yield from cls._gen_cols(expr.get_source_expressions()) + + @classmethod + def _gen_col_aliases(cls, exprs): + yield from (expr.alias for expr in cls._gen_cols(exprs)) def resolve_ref(self, name, allow_joins=True, reuse=None, summarize=False, simple_col=False): if not allow_joins and LOOKUP_SEP in name: @@ -1737,7 +1749,7 @@ class Query(BaseExpression): lookup = lookup_class(pk.get_col(query.select[0].alias), pk.get_col(alias)) query.where.add(lookup, AND) - query.external_aliases.add(alias) + query.external_aliases[alias] = True condition, needed_inner = self.build_filter( ('%s__in' % trimmed_prefix, query), diff --git a/docs/releases/3.0.2.txt b/docs/releases/3.0.2.txt index e99dbb1bae4..53b066838a0 100644 --- a/docs/releases/3.0.2.txt +++ b/docs/releases/3.0.2.txt @@ -9,4 +9,5 @@ Django 3.0.2 fixes several bugs in 3.0.1. Bugfixes ======== -* ... +* Fixed a regression in Django 3.0 that didn't include columns referenced by a + ``Subquery()`` in the ``GROUP BY`` clause (:ticket:`31094`). diff --git a/tests/aggregation/tests.py b/tests/aggregation/tests.py index 0f799c4bc3f..5ba2e180e05 100644 --- a/tests/aggregation/tests.py +++ b/tests/aggregation/tests.py @@ -1170,3 +1170,23 @@ class AggregateTestCase(TestCase): Exists(long_books_qs), ).annotate(total=Count('*')) self.assertEqual(dict(has_long_books_breakdown), {True: 2, False: 3}) + + def test_aggregation_subquery_annotation_related_field(self): + publisher = Publisher.objects.create(name=self.a9.name, num_awards=2) + book = Book.objects.create( + isbn='159059999', name='Test book.', pages=819, rating=2.5, + price=Decimal('14.44'), contact=self.a9, publisher=publisher, + pubdate=datetime.date(2019, 12, 6), + ) + book.authors.add(self.a5, self.a6, self.a7) + books_qs = Book.objects.annotate( + contact_publisher=Subquery( + Publisher.objects.filter( + pk=OuterRef('publisher'), + name=OuterRef('contact__name'), + ).values('name')[:1], + ) + ).filter( + contact_publisher__isnull=False, + ).annotate(count=Count('authors')) + self.assertSequenceEqual(books_qs, [book])