aggregate() crashes when referencing existing aggregations or window expressions through conditional expressions

[Sergey Nesterenko]

I found out that getting refs in QuerySet doesn't work with models.Window when I try to use aggregate method. It happened when I tried to update Django to 4.2.7.

For example:

MyModel.objects.annotate(  
	new_ordering=Window(RowNumber(),  order_by=[F('ordering')])  
).aggregate(  
	new_count=Count('id', filter=Q(ordering=F('new_ordering')))
)

And as a result, it fails with:

Traceback (most recent call last):
  File "/main.py", line 38, in <module>
    result = ordering_query.aggregate(
  File "/python3.10/site-packages/django/db/models/query.py", line 592, in aggregate
    return self.query.chain().get_aggregation(self.db, kwargs)
  File "/python3.10/site-packages/django/db/models/sql/query.py", line 398, in get_aggregation
    aggregate = aggregate_expr.resolve_expression(
  File "/python3.10/site-packages/django/db/models/aggregates.py", line 70, in resolve_expression
    for ref in c.get_refs():
  File "/python3.10/site-packages/django/db/models/expressions.py", line 418, in get_refs
    refs |= expr.get_refs()
  File "/python3.10/site-packages/django/db/models/sql/where.py", line 233, in get_refs
    refs |= child.get_refs()
  File "/python3.10/site-packages/django/db/models/expressions.py", line 418, in get_refs
    refs |= expr.get_refs()
  File "/python3.10/site-packages/django/db/models/expressions.py", line 418, in get_refs
    refs |= expr.get_refs()
AttributeError: 'NoneType' object has no attribute 'get_refs'

It happens because of Window.get_source_expressions return:

def get_source_expressions(self):  
	return [self.source_expression, self.partition_by, self.order_by, self.frame]

and if we don't specify 'frame' (for example) it will be None in the list.

And in models.aggregates.Aggregate.resolve_expression when we try to find all refs ​https://github.com/django/django/blob/47f9b8dca16b1fdc054b338d81eb080eabad0edf/django/db/models/aggregates.py#L70C30-L70C30 it fails because of we have None in Window.get_source_expressions

def get_refs(self):  
	refs = set()  
	for expr in self.get_source_expressions():  
		refs |= expr.get_refs()  
	return refs

[Simon Charette]

Another instance of a crash due to mishandling of get_source_expressions() being Expression | None.

[Simon Charette]

I'm not sure if something else will break as I don't think we have extensive testing for performing filtered aggregation over a window function but does the following patch helps

diff --git a/django/db/models/expressions.py b/django/db/models/expressions.py
index 3a0c75ebf2..74ae9cab8e 100644
--- a/django/db/models/expressions.py
+++ b/django/db/models/expressions.py
@@ -417,6 +417,8 @@ def replace_expressions(self, replacements):
     def get_refs(self):
         refs = set()
         for expr in self.get_source_expressions():
+            if expr is None:
+                continue
             refs |= expr.get_refs()
         return refs

[Sergey Nesterenko]

Replying to Simon Charette:

I'm not sure if something else will break as I don't think we have extensive testing for performing filtered aggregation over a window function but does the following patch helps

diff --git a/django/db/models/expressions.py b/django/db/models/expressions.py
index 3a0c75ebf2..74ae9cab8e 100644
--- a/django/db/models/expressions.py
+++ b/django/db/models/expressions.py
@@ -417,6 +417,8 @@ def replace_expressions(self, replacements):
     def get_refs(self):
         refs = set()
         for expr in self.get_source_expressions():
+            if expr is None:
+                continue
             refs |= expr.get_refs()
         return refs

I thought about it, but I wasn't sure it wouldn't break anything, but it might work.

[Simon Charette]

I've got an idea of how to solve this, it's not clear to me if it's a regression as the underlying cause has little to do with window functions.

In the mean time, can you confirm that switching the order of your lookup resolve the issue. That is doing filter=Q(new_ordering=F('ordering')) instead of filter=Q(ordering=F('new_ordering')).

[Simon Charette]

Confirmed regression in b181cae2e3697b2e53b5b67ac67e59f3b05a6f0d, refs #25307.

[Sergey Nesterenko]

Replying to Simon Charette:

filter=Q(new_ordering=F('ordering')) instead of filter=Q(ordering=F('new_ordering')).

Yes, it solves my problem. Thankx!

UPD: filter=Q(new_ordering=F('ordering'), ordering__isnull=False)

[Mariusz Felisiak <felisiak.mariusz@…>]

In 7530cf39:

Fixed #34975 -- Fixed crash of conditional aggregate() over aggregations.

Adjustments made to solve_lookup_type to defer the resolving of
references for summarized aggregates failed to account for similar
requirements for lookup values which can also reference annotations
through Aggregate.filter.

Regression in b181cae2e3697b2e53b5b67ac67e59f3b05a6f0d.

Refs #25307.

Thanks Sergey Nesterenko for the report.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 15cb3c26:

Refs #34975 -- Complemented rhs filtering aggregations for in lookup.

While this isn't a regression it's clear that similar logic should be
applied when dealing with lists of expressions passed as a lookup value.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 911b161:

Refs #34975 -- Handled optional source expressions in Expression.get_refs().

While no code is directly exercising get_refs in a way that triggers
a crash some expressions such as Window stash None in source_expressions
which can obscure the origin of some bugs.

Handling None values like we do in other source_expression traversing
methods such as .contains_aggregates ensures we don't run into surprises
in the future where get_refs() might be used for a different purpose.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 49f1ced8:

[5.0.x] Fixed #34975 -- Fixed crash of conditional aggregate() over aggregations.

Adjustments made to solve_lookup_type to defer the resolving of
references for summarized aggregates failed to account for similar
requirements for lookup values which can also reference annotations
through Aggregate.filter.

Regression in b181cae2e3697b2e53b5b67ac67e59f3b05a6f0d.

Refs #25307.

Thanks Sergey Nesterenko for the report.

Backport of 7530cf3900ab98104edcde69e8a2a415e82b345a from main

[Mariusz Felisiak <felisiak.mariusz@…>]

In acf4cee:

[4.2.x] Fixed #34975 -- Fixed crash of conditional aggregate() over aggregations.

Adjustments made to solve_lookup_type to defer the resolving of
references for summarized aggregates failed to account for similar
requirements for lookup values which can also reference annotations
through Aggregate.filter.

Regression in b181cae2e3697b2e53b5b67ac67e59f3b05a6f0d.

Refs #25307.

Thanks Sergey Nesterenko for the report.

Backport of 7530cf3900ab98104edcde69e8a2a415e82b345a from main

[Sarah Boyce <42296566+sarahboyce@…>]

In 0bac41f:

Refs #34975 -- Removed unnecessary lookups.In.get_refs().

Now that In.get_source_expression() includes its right-hand-side when it
contains expressions (refs #36025) it no longer requires a specialized
get_refs() method.

[Sarah Boyce <42296566+sarahboyce@…>]

In 0690c060:

[5.2.x] Refs #34975 -- Removed unnecessary lookups.In.get_refs().

Now that In.get_source_expression() includes its right-hand-side when it
contains expressions (refs #36025) it no longer requires a specialized
get_refs() method.

Backport of 0bac41fc7e4a842e8d20319cba31cc645501c245 from main.