[Django] #33114: StringAgg always fails on TextField
Django
-------------------------------------+-------------------------------------
Reporter: Swen | Owner: nobody
Kooij |
Type: | Status: new
Uncategorized |
Component: Database | Version: 3.2
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Example:
{{{#!python
class MyModel(models.Model):
name = models.TextField()
list(MyModel.objects.annotate(names=StringAgg('name', ',')))
}}}
Results in the error:
{{{
django.core.exceptions.FieldError: Expression contains mixed types:
TextField, CharField. You must set output_field.
}}}
There are two possible work-arounds:
1. Derive from `StringAgg` and set the `output_field`:
{{{#!python
class TextStringAgg(StringAgg):
output_field = TextField()
}}}
2. Wrap the expression in `ExpressionWrapper`:
{{{#!python
ExpressionWrapper(StringAgg('name', ','), output_field=TextField())
}}}
This seems to be a regression introduced in 3.2 as part of this change:
https://github.com/django/django/commit/1e38f1191de21b6e96736f58df57dfb851a28c1f
--
Ticket URL: <https://code.djangoproject.com/ticket/33114>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Uncategorized | Status: new
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Simon Charette (added)
Comment:
This side-effect is documented, see
9369f0cebba1f65909a14dec6aa3515ec1eb2557 and #31967. Also, you can set
`output_field` directly in the `StringAgg()`, e.g.
{{{
StringAgg('field', ';', output_field=TextField())
}}}
I'm not sure if there is anything specific that we could do.
--
Ticket URL: <https://code.djangoproject.com/ticket/33114#comment:1>
Django
-------------------------------------+-------------------------------------
Type: Uncategorized | Status: new
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Swen Kooij):
Sorry, I didn't quite grasp from that part of the release notes that this
is expected.
Setting a default `output_field` on `StringAgg` would help. It would
restore the original behaviour. It's a bit strange to get this error when
dealing with strings. Whether it's a `CharField` or `TextField`, you still
end up with a string in Python. Having to explicitly add an `output_field`
for `StringAgg` seems to be just an annoyance.
--
Ticket URL: <https://code.djangoproject.com/ticket/33114#comment:2>
Django
-------------------------------------+-------------------------------------
Type: Uncategorized | Status: new
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
This will be an issue for all expressions that accept mixed string types.
Since PostgreSQL [https://www.postgresql.org/docs/current/functions-
aggregate.html documents] that `string_agg ( value text, delimiter text )
→ text` I guess we should default to `output_field = models.TextField()`
to limit this annoyance.
--
Ticket URL: <https://code.djangoproject.com/ticket/33114#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Swen Kooij | Owner: nobody
Cleanup/optimization |
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* type: Uncategorized => Cleanup/optimization
* stage: Unreviewed => Accepted
Comment:
Replying to [comment:3 Simon Charette]:
> This will be an issue for all expressions that accept mixed string
types.
>
> Since PostgreSQL [https://www.postgresql.org/docs/current/functions-
aggregate.html documents] that `string_agg ( value text, delimiter text )
→ text` I guess we should default to `output_field = models.TextField()`
to limit this annoyance.
OK, agreed. Swen, would you like to prepare a patch (with tests)?
--
Ticket URL: <https://code.djangoproject.com/ticket/33114#comment:4>
Django
Reporter: Swen Kooij | Owner: nobody
Component: contrib.postgres | Version: 3.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* component: Database layer (models, ORM) => contrib.postgres
--
Ticket URL: <https://code.djangoproject.com/ticket/33114#comment:5>
Django
Reporter: Swen Kooij | Owner: ali
Type: | sayyah
Cleanup/optimization | Status: assigned
Component: contrib.postgres | Version: 3.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => ali sayyah
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/33114#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Swen Kooij | Owner: ali
Type: | sayyah
Cleanup/optimization | Status: assigned
Component: contrib.postgres | Version: 3.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by ali sayyah):
Should I make the patch for 3.2 or the main branch?
--
Ticket URL: <https://code.djangoproject.com/ticket/33114#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Swen Kooij | Owner: ali
Type: | sayyah
Cleanup/optimization | Status: assigned
Component: contrib.postgres | Version: 3.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Claude Paroz):
main branch (which will target 4.1, now that 4.0 is feature frozen).
--
Ticket URL: <https://code.djangoproject.com/ticket/33114#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Swen Kooij | Owner: ali
Type: | sayyah
Cleanup/optimization | Status: assigned
Component: contrib.postgres | Version: 3.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by ali sayyah):
Replying to [comment:8 Claude Paroz]:
> main branch (which will target 4.1, now that 4.0 is feature frozen).
Thank you.
--
Ticket URL: <https://code.djangoproject.com/ticket/33114#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Swen Kooij | Owner: ali
Type: | sayyah
Cleanup/optimization | Status: assigned
Component: contrib.postgres | Version: 3.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/33114#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Swen Kooij | Owner: ali
Type: | sayyah
Cleanup/optimization | Status: assigned
Component: contrib.postgres | Version: 3.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by ali sayyah):
PR with the fix and a unittest:
[https://github.com/django/django/pull/14898]
--
Ticket URL: <https://code.djangoproject.com/ticket/33114#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Swen Kooij | Owner: ali
Type: | sayyah
Cleanup/optimization | Status: assigned
Component: contrib.postgres | Version: 3.2
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/33114#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: Swen Kooij | Owner: ali
Type: | sayyah
Component: contrib.postgres | Version: 3.2
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"ca583783900d98188c488add21f77702a94a7922" ca583783]:
{{{
#!CommitTicketReference repository=""
revision="ca583783900d98188c488add21f77702a94a7922"
Fixed #33114 -- Defined default output_field of StringAgg.
Thanks Simon Charette for the review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33114#comment:13>