#34176 closed Bug (fixed)
Annotation's original field-name can clash with result field name over aggregation
Reported by: | Shai Berger | Owned by: | Simon Charette |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Release blocker | Keywords: | |
Cc: | Simon Charette, David Sanders, Paolo Melchiorre | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
This is a bit of a continuation of #34123
With current Django main, if one aggregates over a model, while adding annotations of related fields; and an annotation comes from a field with the same name as a result field, it breaks.
This is easier to explain by example; consider this test, added to the aggregation_regress
tests, so it uses their models; Book
carries a FK to Publisher
, a M2M to Author
named authors
, and a FK to Author
(supposedly picking one of the related authors) named contact
.
def test_aggregate_and_annotate_duplicate_columns(self): results = Book.objects.values('isbn').annotate( name=F('publisher__name'), contact_name=F('contact__name'), num_authors=Count('authors'), ) self.assertTrue(results)
The field isbn
is not defined as unique, it was chosen in order to make sure there's an aggregation on the main table and not some sort of subquery -- and also, to get the Book
model's own name
field out of the way.
This blows up on Sqlite:
====================================================================== ERROR: test_aggregate_and_annotate_duplicate_columns (aggregation_regress.tests.AggregationTests) ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/django/django/django/db/backends/utils.py", line 89, in _execute return self.cursor.execute(sql, params) File "/home/django/django/django/db/backends/sqlite3/base.py", line 378, in execute return super().execute(query, params) sqlite3.OperationalError: ambiguous column name: name
and on Postgres
====================================================================== ERROR: test_aggregate_and_annotate_duplicate_columns (aggregation_regress.tests.AggregationTests) ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/django/django/django/db/backends/utils.py", line 89, in _execute return self.cursor.execute(sql, params) psycopg2.errors.AmbiguousColumn: column reference "name" is ambiguous LINE 1: ..._id") GROUP BY "aggregation_regress_book"."isbn", "name", "c... ^
Note that if we change the name
above to something else -- e.g.
pub_name=F('publisher__name'),
then things seem to be working fine. Conversely, if we pick another related field for the name annotation:
name=F('publisher__num_awards'),
then things stay broken.
This used to work, and now it doesn't. I've bisected this regression to b7b28c7c189615543218e81319473888bc46d831.
Thanks Matific for letting me work on this.
Change History (9)
comment:1 by , 2 years ago
Triage Stage: | Unreviewed → Accepted |
---|---|
Version: | 4.1 → dev |
comment:2 by , 2 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:3 by , 2 years ago
Has patch: | set |
---|
I didn't know that
Book.objects.values('isbn').annotate( name=F('publisher__name'), )
was allowed but
> Book.objects.values('isbn', name=F('publisher__name')) ValueError: The annotation 'name' conflicts with a field on the model.
wasn't which seems like a bug in #25871?
Anyway, I submitted this MR for review which avoids a complex refactor of get_select
that would have necessitated reference repointing.
This means that group by reference will be disabled for annotations that conflict with a selected column name but I think that's an acceptable compromise given it should only have an incidence on complex annotations that can't be collapsed that are assigned with ambiguous names.
This a compromise we already made in #31377 and has a straightforward workaround for users that must get the group be reference for performance reasons; use an annotation name that doesn't conflict with an already selected column name.
comment:4 by , 2 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
comment:5 by , 2 years ago
Cc: | added |
---|
comment:6 by , 2 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Ready for checkin → Accepted |
comment:7 by , 22 months ago
Patch needs improvement: | unset |
---|---|
Triage Stage: | Accepted → Ready for checkin |
Thanks for the report!
Reproduced at 744a1af7f943106e30d538e6ace55c2c66ccd791.