Opened 15 months ago
Last modified 11 months ago
#34819 assigned Bug
GenericForeignKey.get_prefetch_queryset()
Reported by: | Richard Laager | Owned by: | rajdesai24 |
---|---|---|---|
Component: | contrib.contenttypes | Version: | 3.2 |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
get_prefetch_queryset()
returns two functions. The caller calls them rel_obj_attr
and instance_attr
. They return a tuple of (pk, cls)
. They need to match so that objects can be pulled from the cache. In GenericForeignKey.get_prefetch_query()
, there is a bug where gfk_key()
(the rel_obj_attr
) returns a get_prep_value()
d value but the instance_attr
lambda returns obj.pk
.
Accordingly, for objects where the prepped value and the Python representation are not the same (e.g. the database uses a string and the Python representation is an object instance), these will not match. This makes things like Model.objects.prefetch_related('content_object').get(id=123)
clear (set to None
) the content_object
.
The fix is to call get_prep_value()
in the instance_attr
code path.
Attachments (1)
Change History (8)
by , 15 months ago
Attachment: | gfk-prefetch.patch added |
---|
comment:1 by , 15 months ago
comment:2 by , 15 months ago
Has patch: | unset |
---|---|
Triage Stage: | Unreviewed → Accepted |
Thanks , sounds valid. Would you like to prepare a patch (via GitHub PR)? a regression test is needed.
comment:3 by , 15 months ago
I was able to reproduce the issue, but am not 100% sure if the suggested fix is correct.
First let's see the following example:
# ../tests/generic_relations_regress/models.py class CustomCharField(models.CharField): def get_prep_value(self, value): value = super().get_prep_value(value) return self.to_python(value) + "-pk" class Foo1(models.Model): id = models.CharField(primary_key=True, max_length=25) class Bar1(models.Model): content_type = models.ForeignKey(ContentType, models.CASCADE) object_id = CustomCharField(max_length=25) content_object = GenericForeignKey() class Foo2(models.Model): id = models.CharField(primary_key=True, max_length=25) class Bar2(models.Model): content_type = models.ForeignKey(ContentType, models.CASCADE) object_id = models.CharField(max_length=25) content_object = GenericForeignKey()
with the test functions
# ../tests/generic_relations_regress/tests.py # ..imports class GenericRelationTests(TestCase): def test_custom_prep_value_access_1(self): foo = Foo1.objects.create(pk="some-test") bar = Bar1.objects.create(content_object=foo) self.assertEqual( foo, Bar1.objects.prefetch_related("content_object").get(pk=1).content_object, ) # fails def test_custom_prep_value_access_2(self): foo = Foo2.objects.create(pk="some-test") bar = Bar2.objects.create(content_object=foo) self.assertEqual( foo, Bar2.objects.prefetch_related("content_object").get(pk=1).content_object, ) # passes
From the example above, the first test fails while the second passes.
- In the first test,
Bar1
'sobject_id
is aCustomCharField
, which has itsget_prep_value
function customized. - Then, the
content_object
is set toNone
asGenericForeignKey.get_prefetch_queryset
function returns empty list forret_val
. This happens because theget_all_objects_for_this_type
function (#L200) usesfkeys
as the filter. Thefkeys
have theprep
d values where the database stores the original strings as the primary key. - The suggested solution (running
get_prep_value
ininstance_attr
function) does not solve the problem as theprep
function for Foo1 is different than the Bar1's.
- In the second test,
Bar2
'sobject_id
is a defaultCharField
, while Foo2 has aCustomCharField
as its primary key. - The test passes as the
GenericForeignKey.get_prefetch_queryset
returns a non-empty return value.
I believe one should not use a custom field for object_id
referring a foreign key. Maybe a warning in docs would suffice? I can work more on this upon your suggestions.
comment:4 by , 14 months ago
That's an interesting point. That's the opposite of the scenario I care about. My scenario is real world, and IMHO reasonable.
I believe one should not use a custom field for object_id referring a foreign key.
I agree. I don't think it is reasonable to be messing with an object_id for a GenericForeignKey. That's supposed to be a plain old string so that it can represent primary keys of other objects.
Maybe a warning in docs would suffice?
For that part of it, yes.
So I think a reasonable fix here is:
- The patch I've provided.
- A test case. That can be similar to your test case, except that CustomCharField() should be used on the object pointed to by the GenericForeignKey: i.e. the Foo object, not the Bar object.
- A doc warning that the object_id for a GenericForeignKey() should be an ordinary CharField or the same field type as the primary key of the objects it will point to.
follow-up: 6 comment:5 by , 11 months ago
Hey Mariusz, can I work on this issue if the patch is not yet provided?
comment:6 by , 11 months ago
Replying to rajdesai24:
Hey Mariusz, can I work on this issue if the patch is not yet provided?
Sure, if you have an idea how to fix it.
comment:7 by , 11 months ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
I agree with what Richard is suggesting, and it works. I will generate a PR regarding the solution provided and a valid test case
Hello Richard, thanks for the ticket. In order for us to properly triage this report, would you please send a reproducer that we can use? Either a test case or a minimal Django project would suffice.