Opened 15 years ago

Closed 13 years ago

Last modified 11 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: dev
Severity: Keywords:
Cc: 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

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 15 years ago.
aggregate-validation.diff (1.9 KB ) - added by Alex Gaynor 14 years ago.
11256_r13984.diff (1.9 KB ) - added by Carl Meyer 13 years ago.
updated patch

Download all attachments as: .zip

Change History (13)

comment:1 by Russell Keith-Magee, 15 years ago

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.

by Alex Gaynor, 15 years ago

comment:2 by Alex Gaynor, 15 years ago

Has patch: set

by Alex Gaynor, 14 years ago

Attachment: aggregate-validation.diff added

comment:3 by Carl Meyer, 13 years ago

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.

by Carl Meyer, 13 years ago

Attachment: 11256_r13984.diff added

updated patch

comment:4 by Carl Meyer, 13 years ago

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 by Russell Keith-Magee, 13 years ago

Triage Stage: AcceptedReady for checkin

Looks good to me.

comment:6 by Carl Meyer, 13 years ago

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 by Carl Meyer, 13 years ago

(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 by Carl Meyer, 13 years ago

(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 by Jacob, 12 years ago

milestone: 1.3

Milestone 1.3 deleted

comment:10 by Anssi Kääriäinen, 11 years ago

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