Opened 15 years ago

Closed 11 years ago

#11263 closed Bug (fixed)

Bug in lookup with generic relation in model inheritance

Reported by: veena Owned by: nobody
Component: contrib.contenttypes Version: dev
Severity: Normal Keywords: generic relations, model inheritance
Cc: ramusus@…, nator Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

class Order(models.Model):
    content_type = models.ForeignKey(ContentType)    
    object_id = models.PositiveIntegerField()
    content_object = generic.GenericForeignKey()
    status = models.CharField(max_length=10, default='ordered')
    
class Visitor(Person):
    order = generic.GenericRelation(Order)
    

Visitor.objects.filter(order__status='ordered').count()

It creates this broken query:

SELECT COUNT(*)
FROM "expo_visitor"
INNER JOIN "expo_order" ON ("expo_visitor"."person_ptr_id" = "expo_order"."object_id")
INNER JOIN "expo_order" T3 ON ("expo_order"."person_ptr_id" = T3."id")
WHERE (T3."status" = %s AND T3."content_type_id" = %s )'

There is non existent column "expo_order"."person_ptr_id"

Attachments (2)

generic_relations_inheritance_order_test.diff (1.9 KB ) - added by Tobias McNulty 15 years ago.
generic_relations_inheritance_order_test.2.diff (2.9 KB ) - added by amarkor 13 years ago.
updated test

Download all attachments as: .zip

Change History (28)

comment:1 by anonymous, 15 years ago

milestone: 1.2

comment:2 by anonymous, 15 years ago

Component: UncategorizedDatabase layer (models, ORM)
Needs documentation: set
Needs tests: set

comment:3 by Alex Gaynor, 15 years ago

Triage Stage: UnreviewedAccepted

comment:4 by Tobias McNulty, 15 years ago

Owner: changed from nobody to Tobias McNulty
Status: newassigned

verifying this error

comment:5 by Tobias McNulty, 15 years ago

verified that WITHOUT inheritance this works OK:

class Order(models.Model):
    content_type = models.ForeignKey(ContentType)
    object_id = models.PositiveIntegerField()
    content_object = generic.GenericForeignKey()
    status = models.CharField(max_length=10, default='ordered')

class Visitor(models.Model): # DOES NOT have parent model
    order = generic.GenericRelation(Order)
SELECT COUNT(*) 
FROM "testapp_visitor" 
INNER JOIN "testapp_order" ON ("testapp_visitor"."id" = "testapp_order"."object_id") 
WHERE ("testapp_order"."status" = E\'ordered\'  AND "testapp_order"."content_type_id" = 9 )

comment:6 by Tobias McNulty, 15 years ago

WITH inheritance it still breaks in trunk:

rom django.db import models
from django.contrib.contenttypes.models import ContentType
from django.contrib.contenttypes import generic
from django.contrib.auth.models import User

class Order(models.Model):
    content_type = models.ForeignKey(ContentType)
    object_id = models.PositiveIntegerField()
    content_object = generic.GenericForeignKey()
    status = models.CharField(max_length=10, default='ordered')

class Visitor(User): # HAS a parent model
    order = generic.GenericRelation(Order)


Visitor.objects.filter(order__status='ordered').count()
# generates:
ProgrammingError: column testapp_order.user_ptr_id does not exist
SELECT COUNT(*) 
FROM "testapp_visitor" 
INNER JOIN "testapp_order" ON ("testapp_visitor"."user_ptr_id" = "testapp_order"."object_id") 
INNER JOIN "testapp_order" T3 ON ("testapp_order"."user_ptr_id" = T3."id") 
WHERE (T3."status" = E\'ordered\'  AND T3."content_type_id" = 9 )

by Tobias McNulty, 15 years ago

comment:7 by Tobias McNulty, 15 years ago

added a patch with a test for this bug. (it's there but it doesn't show up in the Trac diff viewer - any idea why?)

comment:8 by Tobias McNulty, 15 years ago

Owner: changed from Tobias McNulty to nobody
Status: assignednew

I may be able to fix this but bumping back to nobody in case someone else gets to it before I do. I'll re-accept the ticket if/when I have time to work on it.

Any pointers to what code generates this SQL would be appreciated :)

comment:9 by cheslavv, 14 years ago

When it will be fixed?

comment:10 by jawn, 14 years ago

Uhh...this is a pretty big fucking bug in the ORM... Any chance of you gusy looking at this??

comment:11 by Russell Keith-Magee, 14 years ago

@jawn: Offers of assistance are welcome. Abuse and foul language is not.

comment:12 by ramusus, 14 years ago

Cc: ramusus@… added

comment:13 by paltman, 14 years ago

Has patch: set

comment:14 by Russell Keith-Magee, 14 years ago

milestone: 1.21.3

Not critical for 1.2

comment:15 by nator, 14 years ago

Caveat: I'm new to Django and Python.

That said, I did some poking around in django/db/models/sql/query.py and found a tweak that "fixes" this. In the setup_joins function there's a block of code that looks like this (it's within the test block for direct, and then within the test for m2m):

                    if int_alias == table2 and from_col2 == to_col2:
                        joins.append(int_alias)
                        alias = int_alias
                    else:
                        alias = self.join(
                                (int_alias, table2, from_col2, to_col2),
                                dupe_multis, exclusions, nullable=True,
                                reuse=can_reuse)
                        joins.extend([int_alias, alias])

The problem with the bad SQL being generated is a redundant and wrong inner_join. If I change that first test to:

                    if int_alias == table2 and from_col2 == from_col1:

it works. (basically changing the test to check for this redundant situation)

I don't really recommend this as a patch since I don't fully understand the ramifications of the change - what else am I breaking by doing this?

comment:16 by nator, 14 years ago

I also did some testing with various ways of doing the inheritance. Say we have a non-abstract superclass and a subclass, and instead of adding the m2m relationship to the subclass we add it to the superclass -- does it still break that way?

class Order(models.Model):
    content_type = models.ForeignKey(ContentType)    
    object_id = models.PositiveIntegerField()
    content_object = generic.GenericForeignKey()
    status = models.CharField(max_length=10, default='ordered')
    
class Media(models.Model):
    order = generic.GenericRelation(Order)

class Photo(Media):
    title = models.CharField()

Photo.objects.filter(order__status='ordered').count()
--> doesn't break, but always returns 0.

So the answer is yes, it's still broken, but differently. The basic problem is that when the Order object is created Django inserts it into the DB using the content_type of Photo, but when the select is run Django realizes Order actually lives in Media and tries to use that content_type. So the inserts all "work", but the count() never finds them. It's a mismatch between the content_type_id when it runs the insert and select.

After digging through django/config/contenttypes/generic.py a bit I feel like the correct answer is to change the inserts to use the content_type of the class that actually contains the m2m relationship. (Media, in this case) Some quick hacking proves this work, but as with the above note I know I'm not familiar enough with the logic here to safely make this change.

Sorry to not be more help, but I think between this note and the last it should give someone a good shot at fixing this if they understand the codebase better than me.

comment:17 by nator, 14 years ago

Cc: nator added

by amarkor, 13 years ago

updated test

comment:18 by amarkor, 13 years ago

Can confirm that at least the content_type_id between the two queries is different.

Photo.objects.filter(order__status='ordered').query
  SELECT "generic_relations_regress_media"."id",
       "generic_relations_regress_photo"."media_ptr_id"
   FROM "generic_relations_regress_photo"
   INNER JOIN "generic_relations_regress_media"
      ON ("generic_relations_regress_photo"."media_ptr_id" =
          "generic_relations_regress_media"."id")
   INNER JOIN "generic_relations_regress_order"
      ON ("generic_relations_regress_media"."id" =
          "generic_relations_regress_order"."object_id")
   WHERE ("generic_relations_regress_order"."status" = ordered
   AND "generic_relations_regress_order"."content_type_id" = 26 )
photo.order.filter(status='ordered').query
SELECT "generic_relations_regress_order"."id",
       "generic_relations_regress_order"."content_type_id",
       "generic_relations_regress_order"."object_id",
       "generic_relations_regress_order"."status"
   FROM "generic_relations_regress_order"
   WHERE ("generic_relations_regress_order"."object_id" = 1
   AND "generic_relations_regress_order"."content_type_id" = 27
   AND "generic_relations_regress_order"."status" = ordered )

patch attached.

comment:19 by Julien Phalip, 13 years ago

Severity: Normal
Type: Bug

comment:20 by anonymous, 13 years ago

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

comment:21 by Jacob, 12 years ago

milestone: 1.3

Milestone 1.3 deleted

comment:11 by Aymeric Augustin, 12 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:12 by Aymeric Augustin, 12 years ago

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:13 by jaklaassen, 12 years ago

There are two separate issues:

  • The original reporter's issue with invalid SQL
  • The issue raised by nator with filtering concrete model subclasses based on a generic relation in the parent.

Patch is here: https://github.com/jaklaassen/django/compare/ticket_11263

The original issue appears to be fixed in trunk and the patch includes a passing test for this.

The second issue happens because the incorrect content_type_id is used in the query.
This is still happening in trunk, and is fixed by the patch.

in reply to:  13 comment:14 by Ramiro Morales, 11 years ago

Replying to jaklaassen:

  • The issue raised by nator with filtering concrete model subclasses based on a generic relation in the parent.

Patch is here: https://github.com/jaklaassen/django/compare/ticket_11263

The second issue happens because the incorrect content_type_id is used in the query.
This is still happening in trunk, and is fixed by the patch.

I've opened #19722 to track that issue.

comment:15 by Ramiro Morales <cramm0@…>, 11 years ago

Resolution: fixed
Status: newclosed

In c4b6659269260e0a9d75859bbe430b0971b1c15d:

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

Thanks veena for the report and jaklaassen for the patch. Fixes #11263.

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