extra() tables included twice do not generate aliases

[Ilya Semenov]

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.

[Russell Keith-Magee]

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.

[Ilya 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.

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

[daveycrockett]

Proposed bug fix?

[anonymous]

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.

[Russell Keith-Magee]

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.

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

[Luke Plant]

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.

[daveycrockett]

Patch with fix and test case (for trunk)

[daveycrockett]

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

[daveycrockett]

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

[Anssi Kääriäinen]

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.

[Tim Graham]

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