Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#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 Mariusz Felisiak, 2 years ago

Cc: Simon Charette added
Summary: Broken usage of ArrayAgg inside Window functionWindow() expression with ArrayAgg() crashes.
Triage Stage: UnreviewedAccepted

Thanks for the report! Regression in e06dc4571ea9fd5723c8029959b95808be9f8812.

comment:2 by Simon Charette, 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 Mariusz Felisiak, 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.

comment:4 by Mariusz Felisiak, 2 years ago

Has patch: set
Owner: changed from nobody to Mariusz Felisiak
Status: newassigned

comment:5 by GitHub <noreply@…>, 2 years ago

Resolution: fixed
Status: assignedclosed

In fd93db97:

Fixed #33898 -- Fixed Window() expression crash with ArrayAgg().

Thanks Kia for the report.

Regression in e06dc4571ea9fd5723c8029959b95808be9f8812.

comment:6 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

In d9ace347:

[4.1.x] Fixed #33898 -- Fixed Window() expression crash with ArrayAgg().

Thanks Kia for the report.

Regression in e06dc4571ea9fd5723c8029959b95808be9f8812.
Backport of fd93db97c7228b16a4f92f97ef05b0d72418d952 from main

Note: See TracTickets for help on using tickets.
Back to Top