#33898 closed Bug (fixed)
Window() expression with ArrayAgg() crashes.
Reported by: | Kia | Owned by: | Mariusz Felisiak |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 4.1 |
Severity: | Release blocker | Keywords: | ArrayAgg, Window function, extend, tuple, params |
Cc: | Simon Charette | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
The following query raises an AttributeError.
from django.contrib.postgres.aggregates import ArrayAgg from django.db.models.expressions import Window SampleModel.objects.all().annotate( sample_field=Window( expression=ArrayAgg("field_three"), partition_by=["field_one", "field_two"], ) )
Traceback:
File "/usr/local/lib/python3.10/site-packages/django/db/models/expressions.py", line 1693, in as_sql params.extend(window_params) AttributeError: 'tuple' object has no attribute 'extend'
Works in Django 4.0.6 (maybe even in 4.0.7, didn't try) but broken in Django 4.1.
Seems like in OrderableAggMixin (https://github.com/django/django/blob/4.1/django/contrib/postgres/aggregates/mixins.py#L23) it used to return a list
as second value, but now it's a tuple
that breaks params.extend()
in https://github.com/django/django/blob/4.1/django/db/models/expressions.py#L1693
Change History (6)
comment:1 by , 2 years ago
Cc: | added |
---|---|
Summary: | Broken usage of ArrayAgg inside Window function → Window() expression with ArrayAgg() crashes. |
Triage Stage: | Unreviewed → Accepted |
comment:2 by , 2 years ago
This has become a true whack-a-mole game over the years.
I think we should just bite the bullet and audit all of our as_sql
functions to make sure they deal with params in tuple
and not in list
. Otherwise the only true way to cover all cases is to have a unit test for every combination of expression Django provides. Any thoughts on that Mariusz?
Should we keep this issue focused on the Window(ArrayAgg)
case or perform a larger audit meant to be backported? We could at least make sure all as_sql
implementation are able to deal with tuple | list
for now.
comment:3 by , 2 years ago
Should we keep this issue focused on the Window(ArrayAgg) case or perform a larger audit meant to be backported?
Yes we should focus this on fixing Window(ArrayAgg)
and backport do 4.1. Larger audit can be made on the main
branch.
Thanks for the report! Regression in e06dc4571ea9fd5723c8029959b95808be9f8812.