Opened 8 years ago

Closed 6 years ago

Last modified 4 years ago

#11256 closed (fixed)

Fail loudly and clearly when an Annotation alias matches a field name.

Reported by: donspaulding Owned by:
Component: Database layer (models, ORM) Version: master
Severity: Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Continued from a django-users thread

Given the following two models:

class Author(models.Model):
  last_updated = models.DateField()

class Book(models.Model):
  author = models.ForeignKey(Author, related_name='books')
  pubdate = models.DateField()

The following annotation query will produce unexpected results:

authors = Author.objects.annotate(last_updated=Max('books__pubdate'))

And it may not raise an error until you combine it with other parts of the queryset api, like order_by:

authors = Author.objects.annotate(last_updated=Max('books__pubdate')).order_by('last_updated')

While it's true that these invalid queries are the developer's fault, it's not at all obvious what's gone wrong. Unless there is some valid case for clobbering your own namespace, I suggest that instead of leaving this behavior defined by the particular DB and query, an Exception be raised to let the developer know they've made a mistake before it hits the SQL level.

Attachments (3)

aggregation-validationl.diff (1.9 KB) - added by Alex Gaynor 8 years ago.
aggregate-validation.diff (1.9 KB) - added by Alex Gaynor 7 years ago.
11256_r13984.diff (1.9 KB) - added by Carl Meyer 6 years ago.
updated patch

Download all attachments as: .zip

Change History (13)

comment:1 Changed 8 years ago by Russell Keith-Magee

Triage Stage: UnreviewedAccepted

Agreed - an attempt to annotate using a name that already exists on the model should raise an exception at time of query construction.

Changed 8 years ago by Alex Gaynor

comment:2 Changed 8 years ago by Alex Gaynor

Has patch: set

Changed 7 years ago by Alex Gaynor

Attachment: aggregate-validation.diff added

comment:3 Changed 6 years ago by Carl Meyer

milestone: 1.3

#14373 reported this same issue, with the additional factor that under certain (not yet entirely clear) circumstances with m2m fields, this can lead to data loss. Bumping to 1.3 milestone.

Changed 6 years ago by Carl Meyer

Attachment: 11256_r13984.diff added

updated patch

comment:4 Changed 6 years ago by Carl Meyer

New patch updates tests to apply post-doctest-removal, and modifies the duplicate-alias error message to be a bit more informative (tells you what alias is a dupe).

comment:5 Changed 6 years ago by Russell Keith-Magee

Triage Stage: AcceptedReady for checkin

Looks good to me.

comment:6 Changed 6 years ago by Carl Meyer

Resolution: fixed
Status: newclosed

Doh. Forgot fixes ref in commit message; how embarrassing. Fixed in r14092, backported to 1.2.X in r14093.

comment:7 Changed 6 years ago by Carl Meyer

(In [14116]) Refs #11256 -- Extended the annotation field name conflict check to cover m2ms and reverse related descriptors as well. This is needed to actually cover the case raised by #14373.

comment:8 Changed 6 years ago by Carl Meyer

(In [14117]) [1.2.X] Refs #11256 -- Extended the annotation field name conflict check to cover m2ms and reverse related descriptors as well. This is needed to actually cover the case raised by #14373. Backport of [14116].

comment:9 Changed 5 years ago by Jacob

milestone: 1.3

Milestone 1.3 deleted

comment:10 Changed 4 years ago by Anssi Kääriäinen

Component: ORM aggregationDatabase layer (models, ORM)
Note: See TracTickets for help on using tickets.
Back to Top