Opened 6 years ago

Closed 2 years ago

#11387 closed Bug (fixed)

order_by over a GenericRelation generates incorrect SQL

Reported by: adurdin Owned by: mtredinnick
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

Description

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:

models.py:

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 adurdin 6 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 6 years ago by adurdin

  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design 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 6 years ago by adurdin

comment:2 Changed 6 years ago by adurdin

  • 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 5 years ago by mtredinnick

  • Triage Stage changed from Design decision needed to Accepted

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

  • Severity set to Normal
  • Type set to Bug

comment:5 Changed 4 years ago by anonymous

  • Component changed from Database layer (models, ORM) to contrib.contenttypes

comment:6 Changed 3 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:7 Changed 3 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

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

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

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