Code

Opened 5 years ago

Closed 5 years ago

Last modified 14 months ago

#10766 closed (fixed)

Annotate() doesn't error if given a previously defined annotation alias

Reported by: aseering@… Owned by:
Component: Database layer (models, ORM) Version: 1.0
Severity: Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description (last modified by Alex)

I recently wrote the following code:

classes = classes.annotate(num_students=Count('anchor__child_set__userbit_qsc'))
classes = classes.annotate(total_max_student=Sum('sections__num_students'))

The second line above (correctly) generated an error for me because the model referenced by "sections" doesn't contain a field named "num_students".

However, the error message returned by the exception was as follows:

Cannot resolve keyword 'num_students' into field. Choices are: [long valid list of fields], num_students

This confused me for a good while (until I realized that it was my mistake for using the wrong field name). On experimentation, the "num_students" in the list of fields does in fact come from the first annotate call.

It's hardly a critical failure; but it seems like poor form to declare a field as "invalid" then promptly list it as one of the valid options... More generally, it seems like that list just lists all annotated fields, which is only correct if you're not doing a join first.

I suspect that the fix is on line 1678 or so of django/db/models/sql/query.py (in particular, maybe it needs to use a different/new function on the Options class to get the keys properly?); but I don't know enough of those internals to know that I'm doing the right thing there. I could work up a patch that does this if no one more-experienced feels like it or knows of a good reason not to.

Attachments (0)

Change History (5)

comment:1 Changed 5 years ago by Alex

  • Description modified (diff)
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Please use preview :)

comment:2 Changed 5 years ago by russellm

  • Component changed from Uncategorized to ORM aggregation
  • milestone set to 1.1
  • Owner nobody deleted
  • Summary changed from Confusing error message on invalid annotate aggregate field to Annotate() doesn't error if given a previously defined annotation alias
  • Triage Stage changed from Unreviewed to Accepted

You're partially correct here. The example you provide _is_ in error - your second annotation referecnes 'sectionsnum_students', not 'num_students'. The error is being raised because 'sections' isn't a field on your model that can be followed; 'num_students' is on of the list of options because it was a previously added annotation.

However, you are correct in saying that num_students shouldn't be on the list of valid options. It _should_ be on the list of options when looking up filters or F() clauses, but you shouldn't be able to add an annotation based on a previous annotation. If you update your example to use 'num_students' on the second line, you get an SQL error (under postgres: ProgrammingError: column "num_authors" does not exist LINE 1)

I've updated the ticket title to reflect the actual problem; marking for v1.1.

comment:3 Changed 5 years ago by russellm

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

(In [10521]) Fixed #10766 -- Raise an error when annotate() references another aggreagte(). Thanks to aseering@… for the report.

comment:4 Changed 3 years ago by jacob

  • milestone 1.1 deleted

Milestone 1.1 deleted

comment:5 Changed 14 months ago by akaariai

  • Component changed from ORM aggregation to Database layer (models, ORM)

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.