Opened 8 years ago

Closed 7 years ago

Last modified 4 years ago

#5937 closed (fixed)

Generic Relations backward lookups generate invalid SQL

Reported by: max Owned by: mtredinnick
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@…> 8 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 8 years ago by mtredinnick

  • Keywords qs-rf added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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

  • Cc jag@… added
  • Has patch set

I believe the attached patch fixes the issue.

-jag

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

comment:3 Changed 8 years ago by korpios

  • Cc korpios@… added

comment:4 Changed 7 years ago by anonymous

  • Cc drackett@… added

comment:5 Changed 7 years ago by mtredinnick

  • Owner changed from nobody to mtredinnick

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

comment:6 Changed 7 years ago by anonymous

  • Cc aaron@… added

comment:7 Changed 7 years ago by mattgrayson

  • Cc matt@… added

comment:8 Changed 7 years ago by mtredinnick

  • milestone set to 1.0

comment:9 Changed 7 years ago by anonymous

  • Cc wonlay@… added

comment:10 Changed 7 years ago by korpios

  • Cc korpios@… removed

comment:11 Changed 7 years ago by mtredinnick

Fixed in [8608].

comment:12 Changed 7 years ago by mtredinnick

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

comment:13 Changed 7 years ago by mtredinnick

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

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

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

  • milestone 1.0 deleted

Milestone 1.0 deleted

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