#34016: QuerySet.values_list() crash on simple ArrayAgg.
----------------------------------+------------------------------------
Reporter: Alex Kerkum | Owner: (none)
Type: Bug | Status: new
Component: contrib.postgres | Version: 4.1
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------+------------------------------------
Comment (by Simon Charette):
This approach will work as long no `OrderableAggMixin` subclass use this
`get_source_expression` entry removal trick as well which I believe is the
case for our internal usages.
Somewhat related but the `Window` expression is an example of a composite
that has some components that could be missing (`partition_by`, `frame`,
`order_by`) and cannot use the `len` of the expressions to determine which
ones were provided back (e.g. does 3 source expressions mean
`(source_expression, partition_by, frame)` or `(source_expression, frame,
order_by)`?).
It solves this problem by somewhat violating the `get_source_expressions`
API by returning `None` placeholders which makes use some special casing
in a few locations
-
https://github.com/django/django/blob/bc33b047848759e7ae90bf85c94ff6cc5913e1ee/django/db/models/expressions.py#L397-L402
-
https://github.com/django/django/blob/bc33b047848759e7ae90bf85c94ff6cc5913e1ee/django/db/models/expressions.py#L384-L389
-
https://github.com/django/django/blob/bc33b047848759e7ae90bf85c94ff6cc5913e1ee/django/db/models/expressions.py#L274-L281
But not in all of them!
-
https://github.com/django/django/blob/bc33b047848759e7ae90bf85c94ff6cc5913e1ee/django/db/models/expressions.py#L410-L417
-
https://github.com/django/django/blob/bc33b047848759e7ae90bf85c94ff6cc5913e1ee/django/db/models/expressions.py#L424-L425
-
https://github.com/django/django/blob/bc33b047848759e7ae90bf85c94ff6cc5913e1ee/django/db/models/expressions.py#L236-L246
All that so say that we should probably settle on what the signature of
`get_source_expressions` is; `list[Optional[Expression]]` or
`list[Expression]`?
If we go with `list[Optional[Expression]]` then we don't have to do any
`len` based special casing when dealing with optional sub-expressions but
it means that checks of the form `len(source_expressions)` are not
adequate; they must exclude `None` values. I guess we could provide a
property/method that excludes them if this is a common pattern.
If we go with `list[Expression]` then we should adjust `Window` other
expressions violating this guarantee and drop special casing for `None`
value. I guess `Window.get_source_expressions()` could do something along
the lines of
{{{#!python
def get_source_expressions(self):
expressions = self.source_expression
if self.partition_by:
expressions.append(self.partition_by)
...
return expressions
def set_source_expressions(self, expressions):
self.source_expression = expressions.pop(0)
if self.partition_by:
self.partition_by = expressions.pop(0)
...
}}}
Given we've had these special cases in place for a while I think a better
long term solution, and one to happen to maintain backward compatibility,
would be to fully embrace the `list[Optional[Expression]]` approach by
using `None` placeholders to address this problem. It doesn't have to be
in this patch but at least agreeing on which signature we should support
in `main` and going forward would be beneficial.
From having more experience playing with Python typing in the past years,
this is the kind of issues `mypy` and friends could help us properly
enforce. [https://github.com/django/deps/pull/65 Maybe it's time for us to
re-visit adding typing at least to some parts of Django]?
--
Ticket URL: <https://code.djangoproject.com/ticket/34016#comment:6>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-updates/0107018346bc92ee-2472f9bd-a09f-495b-90d6-ab34e70d4e4f-000000%40eu-central-1.amazonses.com.
