Opened 6 years ago

Closed 5 years ago

Last modified 2 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 6 years ago.
aggregate-validation.diff (1.9 KB) - added by Alex 5 years ago.
11256_r13984.diff (1.9 KB) - added by carljm 5 years ago.
updated patch

Download all attachments as: .zip

Change History (13)

comment:1 Changed 6 years ago by russellm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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

Changed 6 years ago by Alex

comment:2 Changed 6 years ago by Alex

  • Has patch set

Changed 5 years ago by Alex

comment:3 Changed 5 years ago by carljm

  • milestone set to 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 5 years ago by carljm

updated patch

comment:4 Changed 5 years ago by carljm

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 5 years ago by russellm

  • Triage Stage changed from Accepted to Ready for checkin

Looks good to me.

comment:6 Changed 5 years ago by carljm

  • Resolution set to fixed
  • Status changed from new to closed

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

comment:7 Changed 5 years ago by carljm

(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 5 years ago by carljm

(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 4 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

comment:10 Changed 2 years ago by akaariai

  • Component changed from ORM aggregation to Database layer (models, ORM)
Note: See TracTickets for help on using tickets.
Back to Top