Opened 10 years ago

Closed 6 years ago

#11387 closed Bug (fixed)

order_by over a GenericRelation generates incorrect SQL

Reported by: Andy Durdin Owned by: Malcolm Tredinnick
Component: contrib.contenttypes Version: master
Severity: Normal Keywords:
Cc: me@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no


When a queryset is ordered by a field in a related model (related by a GenericForeignKey/GenericRelation pair), the SQL generated for the query is incorrect: it has a LEFT JOIN to the related table, whereas it should have an INNER JOIN and also limit the join by the content type. In contrast, a queryset that is filtered by the same field in the related model generates correct SQL.

As a result, the query will return unexpected and/or duplicate rows.

Here's a simple example that demonstrates the problem:

from django.db import models
from django.contrib.contenttypes import generic
from django.contrib.contenttypes.models import ContentType

class Foo(models.Model):
    bar = generic.GenericRelation("Bar")

class Bar(models.Model):
    content_type = models.ForeignKey(ContentType)
    object_id = models.PositiveIntegerField()
    content_object = generic.GenericForeignKey()

class Baz(models.Model):
    bar = generic.GenericRelation("Bar")

test code:

>>> from foo.models import *
>>> foo = Foo.objects.create()
>>> baz = Baz.objects.create()
>>> __ = Bar.objects.create(content_object=foo)
>>> __ = Bar.objects.create(content_object=baz)
>>> from django.db import connection
>>> #
>>> # This queryset has incorrect SQL
>>> qs = Foo.objects.order_by('bar__id')
>>> qs.query.as_sql()
('SELECT "foo_foo"."id" FROM "foo_foo" LEFT OUTER JOIN "foo_bar" ON ("foo_foo"."id" = "foo_bar"."object_id") ORDER BY "foo_bar"."id" ASC', ())
>>> list(qs)
[<Foo: Foo object>, <Foo: Foo object>]
>>> #
>>> # Correct behaviour for comparison
>>> Foo.objects.filter(bar__id=1)
[<Foo: Foo object>]
>>> Foo.objects.filter(bar__id=1).query.as_sql()
('SELECT "foo_foo"."id" FROM "foo_foo" INNER JOIN "foo_bar" ON ("foo_foo"."id" = "foo_bar"."object_id") WHERE ("foo_bar"."id" = %s  AND "foo_bar"."content_type_id" = %s )', (1, 8))

Tested against SVN trunk revision 11103

Attachments (1)

test_11387.diff (2.2 KB) - added by Andy Durdin 10 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 10 years ago by Andy Durdin

Needs tests: set
Triage Stage: UnreviewedDesign decision needed

I’ve been thinking about the problem some more, and that test is wrong, and here’s why:

If you order_by across a reverse ForeignKey relation, a LEFT JOIN is created to the target table. So if you order_by across a reverse GenericForeignKey relation, it should also create a LEFT JOIN to the target table.

There is a deeper problem here, I believe. The correct query for this case should be:

SELECT "foo_foo"."id" FROM "foo_foo" LEFT OUTER JOIN "foo_bar" ON ("foo_foo"."id" = "foo_bar"."object_id" AND "foo_bar"."content_type_id" = %s) ORDER BY "foo_bar"."id" ASC', (8,))

But as far as I have been able to determine, the query generation does not support multiple join conditions, and I don't believe it is possible to transform the above query to move the second condition to the WHERE clause. This means that the query generation code will have to be amended in numerous places to support multiple join conditions. I fear patching this is beyond me.

Changed 10 years ago by Andy Durdin

Attachment: test_11387.diff added

comment:2 Changed 10 years ago by Andy Durdin

Patch needs improvement: set

Replaced faulty test with a correct test--that no longer inspects the query internals, but just checks for correct results of a query.

comment:3 Changed 8 years ago by Malcolm Tredinnick

Triage Stage: Design decision neededAccepted

I kind of believe the ticket description. No need for DDN here. If it's not returning the correct results, we should try to fix that. I'll risk giving myself a nose bleed and have a look at this.

comment:4 Changed 8 years ago by Julien Phalip

Severity: Normal
Type: Bug

comment:5 Changed 8 years ago by anonymous

Component: Database layer (models, ORM)contrib.contenttypes

comment:6 Changed 7 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:7 Changed 7 years ago by Aymeric Augustin

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:8 Changed 6 years ago by Ramiro Morales <cramm0@…>

Resolution: fixed
Status: newclosed

In 2ca37af621c077ab805b8fcb0fb18aef0cfe802d:

Added test to demonstrate issue 11387 isn't there anymore.

Thanks adurdin for the report and patch. Fixes #11387.

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