Opened 14 years ago

Closed 9 years ago

#12890 closed Bug (wontfix)

extra() tables included twice do not generate aliases

Reported by: Ilya Semenov Owned by: daveycrockett
Component: Database layer (models, ORM) Version: 1.3
Severity: Normal Keywords: QuerySet.extra
Cc: daveycrockett, michael@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The documentation at http://docs.djangoproject.com/en/1.1/ref/models/querysets/#extra-select-none-where-none-params-none-tables-none-order-by-none-select-params-none says: "When you add extra tables via the tables parameter, Django assumes you want that table included an extra time, if it is already included. That creates a problem, since the table name will then be given an alias."

That is not true even for simple cases:

print User.objects\
   .extra(tables=['auth_user_group'], where=['auth_user.ud=auth_user_group.user_id'])\
   .extra(tables=['auth_user_group'], where=['auth_user.ud=auth_user_group.user_id']).all()
# crashes with OperationalError(1066, "Not unique table/alias: 'auth_user_group'")

This is a real-life example (with different models, of course) from my project. I have to run an extra() query against the same table twice to achieve the double filtration, but it's not working and doesn't even allow me to specify the aliases manually. A usual double filter() trick would probably work but it is also broken due to #12885.

Attachments (3)

extra-tables-1.2.3.patch (847 bytes ) - added by daveycrockett 13 years ago.
Proposed bug fix?
12890.Django-1.3.diff (5.5 KB ) - added by daveycrockett 13 years ago.
Patch with fix and test case (for trunk)
12890.Django-1.2.5.diff (5.5 KB ) - added by daveycrockett 13 years ago.
Patch with fix and test case (for v1.2.5)

Download all attachments as: .zip

Change History (20)

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

Triage Stage: UnreviewedAccepted

I'm not convinced that this isn't a user-error. .extra() is a place in the API where we have opened up holes, and let users drop things into those holes, but the onus is on the user to put meaningful things into those holes. Since this is a freeform part of the query, there are limits on what we can do. Building an alias for an extra() specified table isn't an option, because we can't rewrite the extra where clause.

However, we should probably raise an error so this is caught before it hits the database.

comment:2 by Ilya Semenov, 14 years ago

russelim, I'm not picking against the current behavior. I agree that extra() is an API which allows to push raw SQL parts and thus it's up to a user to use it properly.

The problem is that extra() is documented to do particular things but doesn't do them as advertised. I'm not quite sure why do you say "Building an alias for an extra() specified table isn't an option". According to the online documentation, extra() tables are supposed to get aliases. Moreover, using aliases is suggested to a user as a last-chance solution: "Finally, if all else fails, look at the query produced and rewrite your where addition to use the alias given to your extra table. The alias will be the same each time you construct the queryset in the same way, so you can rely upon the alias name to not change."

We should either make it work as described, or adjust the documentation accordingly.

comment:3 by Luke Plant, 13 years ago

Type: Bug

comment:4 by Luke Plant, 13 years ago

Severity: Normal

comment:5 by daveycrockett, 13 years ago

I see some fairly slow activity (15 months!) on this ticket and have a...vested interest in seeing it resolved, all of a sudden. The current chain of conversation is exactly what I'm trying to implement...the behavior of extra() currently *contradicts* the documented behavior for the tables parameter. I'm working to test the above patch further, but just wanted to post my results immediately; it meets my use-cases and doesn't break existing functionality.

This seems especially useful when doing fun things with django-mptt, self-joins of mptt models are useful for aggregating over a particular level of a tree (just as a completely hypothetical example, ahem).

by daveycrockett, 13 years ago

Attachment: extra-tables-1.2.3.patch added

Proposed bug fix?

comment:6 by anonymous, 13 years ago

Easy pickings: unset

I'm still waiting to hear back on this patch...would the core contributors respond more quickly to a 1.3 patch? I'd just like some word on whether this will be rolled into the core or not. Thanks.

comment:7 by daveycrockett, 13 years ago

Has patch: set
Triage Stage: AcceptedUnreviewed
Version: 1.11.2

comment:8 by daveycrockett, 13 years ago

Cc: daveycrockett added

comment:9 by Russell Keith-Magee, 13 years ago

Triage Stage: UnreviewedAccepted

Reverting stage change. Ticket has already been accepted. The Unreviewed state refers to that triage status of the ticket, not the review status of an individual patch.

comment:10 by daveycrockett, 13 years ago

Thank you for the insight on the django ticketing process?

I now understand that the ticket itself has been "reviewed" and "accepted" as a legitimate bug. My next question would be when will the patch be "reviewed"?

in reply to:  10 comment:11 by Luke Plant, 13 years ago

Needs tests: set
Patch needs improvement: set

Replying to daveycrockett:

I now understand that the ticket itself has been "reviewed" and "accepted" as a legitimate bug. My next question would be when will the patch be "reviewed"?

Reviewing is done by the community, and we can't make guarantees about that I'm afraid. Please see https://docs.djangoproject.com/en/dev/internals/contributing/ or ask on django-devs for other questions about this process.

However, I can tell you right now that the patch needs some work, particularly because it has no tests, and will not get in without them. That would help in reviewing. It also needs to be in unified diff format - you have to manually edit it just to even attempt to apply it without that. It must apply to trunk as well - I've no idea if it does or not.

by daveycrockett, 13 years ago

Attachment: 12890.Django-1.3.diff added

Patch with fix and test case (for trunk)

by daveycrockett, 13 years ago

Attachment: 12890.Django-1.2.5.diff added

Patch with fix and test case (for v1.2.5)

comment:12 by daveycrockett, 13 years ago

Owner: changed from nobody to daveycrockett
UI/UX: unset
Version: 1.21.3

This bug also manifests in 1.3, supplied patches for both 1.2.5 and 1.3 with tests!

comment:13 by anonymous, 13 years ago

Has patch: unset

comment:14 by anonymous, 13 years ago

Has patch: set
Patch needs improvement: unset

comment:15 by miracle2k, 13 years ago

Cc: michael@… added

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

The proposed patch will cause backwards incompatibilities.

I think the way to implement this is to have explicit alias syntax, something like .extra(tables=[('queries_nestednode', 'T1')]). I am not too happy to add more .extra() features as it would be better to replace the whole thing with something better. But it has been some time and that something better isn't implemented yet, so a little patch to .extra() could be accepted.

comment:17 by Tim Graham, 9 years ago

Keywords: QuerySet.extra added
Resolution: wontfix
Status: newclosed

We are no longer fixing bugs with QuerySet.extra() per discussion on django-developers.

Note: See TracTickets for help on using tickets.
Back to Top