Code

Opened 5 years ago

Closed 15 months ago

#11263 closed Bug (fixed)

Bug in lookup with generic relation in model inheritance

Reported by: veena Owned by: nobody
Component: contrib.contenttypes Version: master
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 5 years ago.
generic_relations_inheritance_order_test.2.diff (2.9 KB) - added by amarkor 3 years ago.
updated test

Download all attachments as: .zip

Change History (28)

comment:1 Changed 5 years ago by anonymous

  • milestone set to 1.2
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 5 years ago by anonymous

  • Component changed from Uncategorized to Database layer (models, ORM)
  • Needs documentation set
  • Needs tests set

comment:3 Changed 5 years ago by Alex

  • Triage Stage changed from Unreviewed to Accepted

comment:4 Changed 5 years ago by tobias

  • Owner changed from nobody to tobias
  • Status changed from new to assigned

verifying this error

comment:5 Changed 5 years ago by tobias

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

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 )

comment:7 Changed 5 years ago by tobias

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

  • Owner changed from tobias to nobody
  • Status changed from assigned to new

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

When it will be fixed?

comment:10 Changed 4 years ago by jawn

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

comment:11 Changed 4 years ago by russellm

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

comment:12 Changed 4 years ago by ramusus

  • Cc ramusus@… added

comment:13 follow-up: Changed 4 years ago by paltman

  • Has patch set

comment:14 Changed 4 years ago by russellm

  • milestone changed from 1.2 to 1.3

Not critical for 1.2

comment:15 Changed 4 years ago by nator

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

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

  • Cc nator added

Changed 3 years ago by amarkor

updated test

comment:18 Changed 3 years ago by amarkor

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

  • Severity set to Normal
  • Type set to Bug

comment:20 Changed 3 years ago by anonymous

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

comment:21 Changed 3 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

comment:11 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:12 Changed 2 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:13 follow-up: Changed 22 months ago by jaklaassen

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.

comment:14 in reply to: ↑ 13 Changed 15 months ago by ramiro

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 Changed 15 months ago by Ramiro Morales <cramm0@…>

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

In c4b6659269260e0a9d75859bbe430b0971b1c15d:

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

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.