Code

Opened 4 years ago

Last modified 11 months ago

#12890 new Bug

extra() tables included twice do not generate aliases

Reported by: semenov Owned by: daveycrockett
Component: Database layer (models, ORM) Version: 1.3
Severity: Normal Keywords:
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 3 years ago.
Proposed bug fix?
12890.Django-1.3.diff (5.5 KB) - added by daveycrockett 3 years ago.
Patch with fix and test case (for trunk)
12890.Django-1.2.5.diff (5.5 KB) - added by daveycrockett 3 years ago.
Patch with fix and test case (for v1.2.5)

Download all attachments as: .zip

Change History (19)

comment:1 Changed 4 years ago by russellm

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

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 Changed 4 years ago by semenov

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 Changed 3 years ago by lukeplant

  • Type set to Bug

comment:4 Changed 3 years ago by lukeplant

  • Severity set to Normal

comment:5 Changed 3 years ago by daveycrockett

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).

Changed 3 years ago by daveycrockett

Proposed bug fix?

comment:6 Changed 3 years ago by anonymous

  • 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 Changed 3 years ago by daveycrockett

  • Has patch set
  • Triage Stage changed from Accepted to Unreviewed
  • Version changed from 1.1 to 1.2

comment:8 Changed 3 years ago by daveycrockett

  • Cc daveycrockett added

comment:9 Changed 3 years ago by russellm

  • Triage Stage changed from Unreviewed to Accepted

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 follow-up: Changed 3 years ago by daveycrockett

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"?

comment:11 in reply to: ↑ 10 Changed 3 years ago by lukeplant

  • 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.

Changed 3 years ago by daveycrockett

Patch with fix and test case (for trunk)

Changed 3 years ago by daveycrockett

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

comment:12 Changed 3 years ago by daveycrockett

  • Owner changed from nobody to daveycrockett
  • UI/UX unset
  • Version changed from 1.2 to 1.3

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

comment:13 Changed 3 years ago by anonymous

  • Has patch unset

comment:14 Changed 3 years ago by anonymous

  • Has patch set
  • Patch needs improvement unset

comment:15 Changed 3 years ago by miracle2k

  • Cc michael@… added

comment:16 Changed 11 months ago by akaariai

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from daveycrockett to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


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

 
Note: See TracTickets for help on using tickets.