Opened 3 years ago

Closed 3 years ago

#17838 closed Bug (fixed)

prefetch_related fails for GenericForeignKeys when related object id is not a CharField/TextField

Reported by: mkai Owned by: anonymous
Component: contrib.contenttypes Version: master
Severity: Release blocker Keywords: prefetch_related
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

After a ton of debugging, I finally found the reason why my prefetching wasn't working. Here's a simple test case with the comments framework:

from django.contrib.comments.models import Comment
for c in Comment.objects.all().prefetch_related('content_object'):
...     print c.content_object
...
 
SELECT * FROM "django_comments" ORDER BY "django_comments"."submit_date" ASC  [3.67ms]
SELECT * FROM "django_content_type" WHERE "django_content_type"."id" = 15  [1.42ms]
SELECT * FROM "items_item" WHERE "items_item"."id" IN (709, 79) ORDER BY "items_item"."created" DESC  [2.48ms]
SELECT * FROM "items_item" WHERE "items_item"."id" = 79  [0.79ms]
SELECT * FROM "items_item" WHERE "items_item"."id" = 709  [1.00ms]

As you can see, it's doing the IN query, but then issues another query for each item. The object_pk field on Comment is a TextField, but the id field on Item is an IntegerField. This has the effect that the cache lookup no longer works. It looks up a tuple like (79, <class Item>), but the cache has a tuple like (u'79', <class Item>) set.

A quick fix is to simply convert the value to an int in contrib.contenttypes.generic.py:

def gfk_key(obj):
    ct_id = getattr(obj, ct_attname)
    if ct_id is None:
        return None
    else:
    	# QUICK FIX: convert to int to fix cache lookups
        # return (getattr(obj, self.fk_field),
        return (int(getattr(obj, self.fk_field)),
                self.get_content_type(id=ct_id,
                                      using=obj._state.db).model_class())

That is probably a very naive solution. Maybe one should check the type of the related object's field and then (safely) convert the value based on that?

Attachments (2)

test_ticket17838.patch (2.6 KB) - added by tmitchell 3 years ago.
Test cases against both contrib.comments and the GFK functionality
fix_and_test_ticket17838.diff (3.4 KB) - added by Przemek Lewandowski <prze.lewandowski@…> 3 years ago.
Improved fix

Download all attachments as: .zip

Change History (8)

comment:1 Changed 3 years ago by tmitchell

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

Changed 3 years ago by tmitchell

Test cases against both contrib.comments and the GFK functionality

Changed 3 years ago by Przemek Lewandowski <prze.lewandowski@…>

Improved fix

comment:2 Changed 3 years ago by Przemek Lewandowski <prze.lewandowski@…>

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

comment:3 Changed 3 years ago by Przemek Lewandowski <prze.lewandowski@…>

  • Has patch set
  • Patch needs improvement unset
  • Triage Stage changed from Accepted to Ready for checkin

comment:4 Changed 3 years ago by aaugustin

  • Triage Stage changed from Ready for checkin to Accepted

Please don't mark your own patches as RFC; patches must be reviewed by someone who isn't the original author.

comment:5 Changed 3 years ago by lukeplant

Thanks for the report, debugging and patch. The patch is nearly correct, but does the _meta.pk lookup on the wrong class. I'll commit a corrected version soon.

comment:6 Changed 3 years ago by lukeplant

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

In [17744]:

Fixed #17838 - prefetch_related fails for GenericForeignKeys when related object id is not a CharField/TextField

Thanks to mkai for the report and debugging, and tmitchell and Przemek
Lewandowski for their work on the patch.

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