Opened 16 years ago

Closed 16 years ago

Last modified 12 years ago

#5937 closed (fixed)

Generic Relations backward lookups generate invalid SQL

Reported by: Max Naude Owned by: Malcolm Tredinnick
Component: Contrib apps Version: dev
Severity: Keywords: generic relation, qs-rf
Cc: jag@…, drackett@…, aaron@…, matt@…, wonlay@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Given the following model is would seem that the Generic Relations' GenericRelatedObjectManager generates invalid SQL, as the doctest in the following snippet proves:

from django.db import models
from django.contrib.contenttypes.generic import GenericForeignKey, GenericRelation
from django.contrib.contenttypes.models import ContentType

#==============================================================================
class Alert(models.Model):
    """
    Alerts for all kinds of widgets
    
    # create a blue widget and add an alert
    >>> blue = BlueWidget.objects.create(name='Blue 1')
    >>> alert = Alert(name='alert 1')
    >>> alert.object = blue
    >>> alert.save()
    
    # create a red widget and add an alert
    >>> red = RedWidget.objects.create(name='Red 1')
    >>> alert = Alert(name='alert 2')
    >>> alert.object = red
    >>> alert.save()
    
    # create a red widget with no alert
    >>> red = RedWidget.objects.create(name='Red 2')
    
    # create a blue widget and add an alert
    >>> blue = BlueWidget.objects.create(name='Blue 2')
    >>> alert = Alert(name='alert 3')
    >>> alert.object = blue
    >>> alert.save()
    
    # get a list of red widgets with alerts - there should be only 1
    >>> reds = RedWidget.objects.filter(alerts__name__contains='alert').distinct()
    >>> reds
    [<RedWidget: RedWidget Red 1>]
    
    # this currently yields: [<RedWidget: RedWidget Red 1>, <RedWidget: RedWidget Red 2>]
    
    """
    created = models.DateTimeField(auto_now_add=True)
    name = models.CharField(maxlength=128)
    content_type = models.ForeignKey(ContentType)
    object_id = models.PositiveIntegerField()
    object = GenericForeignKey('content_type', 'object_id')
    
    #--------------------------------------------------------------------------
    def __str__(self):
        return '%s alert for %s widget' % (self.name, self.object)
    
#==============================================================================
class RedWidget(models.Model):
    """
    The red kind of widget
    """
    created = models.DateTimeField(auto_now_add=True)
    name = models.CharField(maxlength=128)
    alerts = GenericRelation(Alert)
    
    #--------------------------------------------------------------------------
    def __str__(self):
        return 'RedWidget %s' % (self.name)
    
#==============================================================================
class BlueWidget(models.Model):
    """
    The blue kind of widget
    """
    created = models.DateTimeField(auto_now_add=True)
    name = models.CharField(maxlength=128)
    alerts = GenericRelation(Alert)
    
    #--------------------------------------------------------------------------
    def __str__(self):
        return 'BlueWidget %s' % (self.name)    
    

Following the doctest above, the generated SQL after the last statement in the doctest:

reds = RedWidget.objects.filter(alerts__name__contains='alert').distinct()

is this:

SELECT DISTINCT `testapp_redwidget`.`id`,`testapp_redwidget`.`created`,`testapp_redwidget`.`name` 
FROM `testapp_redwidget` 
LEFT OUTER JOIN `testapp_alert` AS `m2m_testapp_redwidget__alerts` ON `testapp_redwidget`.`id` = `m2m_testapp_redwidget__alerts`.`object_id` 
INNER JOIN `testapp_alert` AS `testapp_redwidget__alerts` ON `m2m_testapp_redwidget__alerts`.`id` = `testapp_redwidget__alerts`.`id` 
WHERE (`testapp_redwidget__alerts`.`name` LIKE '%alert%')

That is - although it is linking to the right table (redwidget) it is not filtering on content type in the alert table. As a result it picks up the id of the second blue widget (2), which matches the id of the second, alert-less red widget (2) and links it in the query to produce the buggy result set.

One would expect this SQL to rather look like this:

SELECT DISTINCT `testapp_redwidget`.`id`,`testapp_redwidget`.`created`,`testapp_redwidget`.`name` 
FROM `testapp_redwidget` 
INNER JOIN `testapp_alert` AS `testapp_redwidget__alerts` ON `testapp_redwidget`.`id` = `testapp_redwidget__alerts`.`object_id` 
INNER JOIN `django_content_type` AS `testapp_redwidget__django_content_type` ON `testapp_redwidget__alerts`.`content_type_id` = `testapp_redwidget__django_content_type`.`id`
WHERE (`testapp_redwidget__alerts`.`name` LIKE '%alert%')
AND (`testapp_redwidget__django_content_type`.`app_label` = 'testapp')
AND (`testapp_redwidget__django_content_type`.`model` = 'redwidget')

.. or, even better (and because I don't quite understand the reason for the LEFT OUTER join):

SELECT DISTINCT `testapp_redwidget`.`id`,`testapp_redwidget`.`created`,`testapp_redwidget`.`name` 
FROM `testapp_redwidget` 
INNER JOIN `testapp_alert` AS `testapp_redwidget__alerts` ON `testapp_redwidget`.`id` = `testapp_redwidget__alerts`.`object_id` 
WHERE (`testapp_redwidget__alerts`.`name` LIKE '%alert%')
AND (`testapp_redwidget__alerts`.`content_type_id` = 9)

The only current workaround for this is to explicitly filter on content type id - this works:

>>> ctype = ContentType.objects.get_for_model(RedWidget)
>>> reds = RedWidget.objects.filter(alerts__name__contains='alert', alerts__content_type=ctype).distinct()
>>> reds
[<RedWidget: RedWidget Red 1>]

Attachments (1)

django-generic-relation-query.patch (2.4 KB ) - added by Joshua 'jag' Ginsberg <jag@…> 16 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 by Malcolm Tredinnick, 16 years ago

Keywords: qs-rf added
Triage Stage: UnreviewedAccepted

comment:2 by Joshua 'jag' Ginsberg <jag@…>, 16 years ago

Cc: jag@… added
Has patch: set

I believe the attached patch fixes the issue.

-jag

by Joshua 'jag' Ginsberg <jag@…>, 16 years ago

comment:3 by korpios, 16 years ago

Cc: korpios@… added

comment:4 by anonymous, 16 years ago

Cc: drackett@… added

comment:5 by Malcolm Tredinnick, 16 years ago

Owner: changed from nobody to Malcolm Tredinnick

Somehow I forgot to grab this. It's still being worked on.

comment:6 by anonymous, 16 years ago

Cc: aaron@… added

comment:7 by Matt Grayson, 16 years ago

Cc: matt@… added

comment:8 by Malcolm Tredinnick, 16 years ago

milestone: 1.0

comment:9 by anonymous, 16 years ago

Cc: wonlay@… added

comment:10 by korpios, 16 years ago

Cc: korpios@… removed

comment:11 by Malcolm Tredinnick, 16 years ago

Fixed in [8608].

comment:12 by Malcolm Tredinnick, 16 years ago

Resolution: fixed
Status: newclosed

comment:13 by Malcolm Tredinnick, 16 years ago

Resolution: fixed
Status: closedreopened

This wasn't properly fixed by the above commit. exclude() causes an infinite loop and there are some problems with the efficiency (number of table joins) with the resulting query. I'm working on this now, but wanted to reopen in case anybody rediscovered the problem.

comment:14 by Malcolm Tredinnick, 16 years ago

Resolution: fixed
Status: reopenedclosed

(In [8644]) Improvements to [8608] to fix an infinite loop (for exclude(generic_relation)).
Also comes with approximately 67% less stupidity in the table joins for
filtering on generic relations.

Fixed #5937, hopefully for good, this time.

comment:15 by Jacob, 12 years ago

milestone: 1.0

Milestone 1.0 deleted

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