#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 Changed 10 months ago by
Cc: | Simon Charette added |
---|---|
Summary: | Broken usage of ArrayAgg inside Window function → Window() expression with ArrayAgg() crashes. |
Triage Stage: | Unreviewed → Accepted |
comment:2 Changed 10 months ago by
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 Changed 10 months ago by
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.
comment:4 Changed 10 months ago by
Has patch: | set |
---|---|
Owner: | changed from nobody to Mariusz Felisiak |
Status: | new → assigned |
Thanks for the report! Regression in e06dc4571ea9fd5723c8029959b95808be9f8812.