[Django] #34819: GenericForeignKey.get_prefetch_queryset()
Django
------------------------------------------------+------------------------
Reporter: Richard Laager | Owner: nobody
Type: Bug | Status: new
Component: contrib.contenttypes | Version: 3.2
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
`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.
--
Ticket URL: <https://code.djangoproject.com/ticket/34819>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: Richard Laager | Owner: nobody
Type: Bug | Status: new
contrib.contenttypes |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Richard Laager):
* Attachment "gfk-prefetch.patch" added.
Django
-------------------------------------+-------------------------------------
Reporter: Richard Laager | Owner: nobody
Type: Bug | Status: new
Component: | Version: 3.2
contrib.contenttypes |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Natalia Bidart):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/34819#comment:1>
Django
--------------------------------------+------------------------------------
Reporter: Richard Laager | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* has_patch: 1 => 0
* stage: Unreviewed => Accepted
Comment:
Thanks , sounds valid. Would you like to prepare a patch (via GitHub PR)?
a regression test is needed.
--
Ticket URL: <https://code.djangoproject.com/ticket/34819#comment:2>
Django
--------------------------------------+------------------------------------
Reporter: Richard Laager | Owner: nobody
Type: Bug | Status: new
Component: contrib.contenttypes | Version: 3.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by Oguzhan Akan):
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.
1. In the first test, `Bar1`'s `object_id` is a `CustomCharField`, which
has its `get_prep_value` function customized.
2. Then, the `content_object` is set to `None` as
`GenericForeignKey.get_prefetch_queryset` function returns empty list for
`ret_val`. This happens because the `get_all_objects_for_this_type`
function
([https://github.com/django/django/blob/a7c73b944f51d6c92ec876fd7e0a171e7c01657d/django/contrib/contenttypes/fields.py#L200
#L200]) uses `fkeys` as the filter. The `fkeys` have the `prep`d values
where the database stores the original strings as the primary key.
3. The suggested solution (running `get_prep_value` in `instance_attr`
function) does not solve the problem as the `prep` function for Foo1 is
different than the Bar1's.
4. In the second test, `Bar2`'s `object_id` is a default `CharField`,
while Foo2 has a `CustomCharField` as its primary key.
5. 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/34819#comment:3>
Django
--------------------------------------+------------------------------------
Reporter: Richard Laager | Owner: nobody
Type: Bug | Status: new
Component: contrib.contenttypes | Version: 3.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by Richard Laager):
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:
1. The patch I've provided.
2. 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.
3. 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/34819#comment:4>
Django
--------------------------------------+------------------------------------
Reporter: Richard Laager | Owner: nobody
Type: Bug | Status: new
Component: contrib.contenttypes | Version: 3.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by rajdesai24):
Hey Mariusz, can I work on this issue if the patch is not yet provided?
--
Ticket URL: <https://code.djangoproject.com/ticket/34819#comment:5>
Django
--------------------------------------+------------------------------------
Reporter: Richard Laager | Owner: nobody
Type: Bug | Status: new
Component: contrib.contenttypes | Version: 3.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by Mariusz Felisiak):
Replying to [comment:5 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/34819#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Richard Laager | Owner:
Type: Bug | Status: assigned
Component: | Version: 3.2
contrib.contenttypes |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by rajdesai24):
* owner: nobody => rajdesai24
* status: new => assigned
Comment:
I agree with what Richard is suggesting, and it works. I will generate a
PR regarding the solution provided and a valid test case
--
Ticket URL: <https://code.djangoproject.com/ticket/34819#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Richard Laager | Owner: Clifford
| Gama
Component: | Version: 3.2
contrib.contenttypes |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
* owner: rajdesai24 => Clifford Gama
Comment:
[https://github.com/django/django/pull/19273 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/34819#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Richard Laager | Owner: Clifford
| Gama
Type: Bug | Status: assigned
Component: | Version: 3.2
contrib.contenttypes |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
Comment:
Left some comments for improvements on the PR to adhere to the
`pk.value_to_string`
[https://github.com/django/django/pull/18863/files#r1907222089 conclusion
reached by work] on #35941 to add composite `GenericForeignKey` support.
--
Ticket URL: <https://code.djangoproject.com/ticket/34819#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Richard Laager | Owner: Clifford
| Gama
Type: Bug | Status: assigned
Component: | Version: 3.2
contrib.contenttypes |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/34819#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Richard Laager | Owner: Clifford
| Gama
Type: Bug | Status: assigned
Component: | Version: 3.2
contrib.contenttypes |
Severity: Normal | Resolution:
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/34819#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Richard Laager | Owner: Clifford
| Gama
Component: | Version: 3.2
contrib.contenttypes |
Severity: Normal | Resolution: fixed
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* resolution: => fixed
* status: assigned => closed
Comment:
In [changeset:"d5c19f9b327fccc1f707e10de648fe9d4cdacb9b" d5c19f9b]:
{{{#!CommitTicketReference repository=""
revision="d5c19f9b327fccc1f707e10de648fe9d4cdacb9b"
Fixed #34819 -- Made GenericForeignKey prefetching use matching pk
representations.
Ensured that rel_obj_attr and instance_attr return matching (pk, cls)
tuples
in GenericForeignKey.get_prefetch_queryset(), preventing mismatches when
prefetching related objects where pk and get_prep_value() differ. Using
value_to_string() also makes this code compatible with composite primary
keys.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34819#comment:12>