Opened 9 years ago

Closed 8 years ago

Last modified 5 years ago

#5937 closed (fixed)

Generic Relations backward lookups generate invalid SQL

Reported by: Max Naude Owned by: Malcolm Tredinnick
Component: Contrib apps Version: master
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: UI/UX:

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@…> 9 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 9 years ago by Malcolm Tredinnick

Keywords: qs-rf added
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedAccepted

comment:2 Changed 9 years ago by Joshua 'jag' Ginsberg <jag@…>

Cc: jag@… added
Has patch: set

I believe the attached patch fixes the issue.

-jag

Changed 9 years ago by Joshua 'jag' Ginsberg <jag@…>

comment:3 Changed 9 years ago by korpios

Cc: korpios@… added

comment:4 Changed 9 years ago by anonymous

Cc: drackett@… added

comment:5 Changed 8 years ago by Malcolm Tredinnick

Owner: changed from nobody to Malcolm Tredinnick

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

comment:6 Changed 8 years ago by anonymous

Cc: aaron@… added

comment:7 Changed 8 years ago by Matt Grayson

Cc: matt@… added

comment:8 Changed 8 years ago by Malcolm Tredinnick

milestone: 1.0

comment:9 Changed 8 years ago by anonymous

Cc: wonlay@… added

comment:10 Changed 8 years ago by korpios

Cc: korpios@… removed

comment:11 Changed 8 years ago by Malcolm Tredinnick

Fixed in [8608].

comment:12 Changed 8 years ago by Malcolm Tredinnick

Resolution: fixed
Status: newclosed

comment:13 Changed 8 years ago by Malcolm Tredinnick

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 Changed 8 years ago by Malcolm Tredinnick

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 Changed 5 years ago by Jacob

milestone: 1.0

Milestone 1.0 deleted

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