Opened 13 years ago
Closed 13 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: | dev |
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)
Change History (8)
comment:1 by , 13 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Unreviewed → Accepted |
by , 13 years ago
Attachment: | test_ticket17838.patch added |
---|
comment:2 by , 13 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:3 by , 13 years ago
Has patch: | set |
---|---|
Patch needs improvement: | unset |
Triage Stage: | Accepted → Ready for checkin |
comment:4 by , 13 years ago
Triage Stage: | Ready for checkin → Accepted |
---|
Please don't mark your own patches as RFC; patches must be reviewed by someone who isn't the original author.
comment:5 by , 13 years ago
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.
Test cases against both contrib.comments and the GFK functionality