[Django] #33008: prefetch_related for GenericForeignKey - Object doesn't exist behaviour
Django
-------------------------------------+-------------------------------------
Reporter: Martin | Owner: nobody
Svoboda |
Type: | Status: new
Uncategorized |
Component: Database | Version: 3.2
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
prefetch_related called for GenericForeignKey sets content_type_id and
object_id to None, if the foreign object doesn't exist. This behaviour is
not documented.
GenericForignKey is often used for audit records, so it can keep links to
non-existing objects. Probably prefetch_related shouldn't touch original
values of object_id and content_type_id and only set content_object to
None.
{{{
from django.contrib.auth.models import User
from django.contrib.contenttypes.fields import GenericForeignKey
from django.contrib.contenttypes.models import ContentType
from django.db import models
class TaggedItem(models.Model):
tag = models.SlugField()
content_type = models.ForeignKey(ContentType,
on_delete=models.CASCADE)
object_id = models.PositiveIntegerField()
content_object = GenericForeignKey('content_type', 'object_id')
# init data
guido = User.objects.create(username='Guido')
t = TaggedItem(content_object=guido, tag='test')
t.save()
guido.delete()
# get content_object normally
tags_1 = TaggedItem.objects.filter(tag='test')
tags_1[0].content_object # returns None
tags_1[0].object_id # returns 1
tags_1[0].content_type_id # returns X
# use prefetch_related
tags_2 =
TaggedItem.objects.filter(tag='test').prefetch_related("content_object")
tags_2[0].content_object # returns None
tags_2[0].object_id # returns None
tags_2[0].content_type_id # returns None
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33008>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Martin Svoboda | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.contenttypes | Version: 3.2
Severity: Normal | Resolution:
Keywords: Documentation | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* cc: Simon Charette, Mariusz Felisiak (added)
* keywords: => Documentation
* type: Uncategorized => Cleanup/optimization
* component: Database layer (models, ORM) => contrib.contenttypes
* stage: Unreviewed => Accepted
Comment:
Hi Martin. I do agree that's a little surprising. I'll accept as a
Documentation issue on the content types app, but I'll cc Simon and
Mariusz, in case they want to take it as a bug. Thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/33008#comment:1>
Django
behaviour
--------------------------------------+------------------------------------
Reporter: Martin Svoboda | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.contenttypes | Version: 3.2
Severity: Normal | Resolution:
Keywords: Documentation | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--
Ticket URL: <https://code.djangoproject.com/ticket/33008#comment:2>
Django
behaviour
--------------------------------------+------------------------------------
Reporter: Martin Svoboda | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.contenttypes | Version: 3.2
Severity: Normal | Resolution:
Keywords: Documentation | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
I didn't look at the issue in detail but I assume this is happening due to
the prefetching logic performing a `tags_2[0].content_object = None`
assignment.
How does the following code behaves?
{{{#!python
tags_1 = TaggedItem.objects.filter(tag='test')
tags_1.content_object = None
assert tags_1.object_id is None
assert tags_1.content_type_id is None
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33008#comment:3>
Django
behaviour
--------------------------------------+------------------------------------
Reporter: Martin Svoboda | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.contenttypes | Version: 3.2
Severity: Normal | Resolution:
Keywords: Documentation | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Martin Svoboda):
Thank you, you are right, assignment {{{ tags_2[0].content_object = None
}}} also set object_id and content_type_id is to None. However I am not
sure if "modification" of a source object is the correct behaviour for
prefetch_related.
Replying to [comment:3 Simon Charette]:
> I didn't look at the issue in detail but I assume this is happening due
to the prefetching logic performing a `tags_2[0].content_object = None`
assignment.
>
> How does the following code behaves?
>
> {{{#!python
> tags_1 = TaggedItem.objects.filter(tag='test')
> tags_1.content_object = None
> assert tags_1.object_id is None
> assert tags_1.content_type_id is None
> }}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33008#comment:4>
Django
behaviour
--------------------------------------+------------------------------------
Reporter: Martin Svoboda | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.contenttypes | Version: 3.2
Severity: Normal | Resolution:
Keywords: Documentation | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
Thanks for trying it out.
There must be a way to avoid this assignment overriding somehow as this
analogous situation doesn't result in attribute loss
{{{#!python
class UserRef(models.Model):
user = models.ForeignKey(User, models.DO_NOTHING, null=True,
db_constraint=False)
UserRef.objects.create(user_id=42)
ref = UserRef.objects.prefetch_related('user')[0]
assert ref.user is None
assert ref.user_id == 42
}}}
The distinction between the two is due to
[https://github.com/django/django/blob/3a6431db5461b88e691d770abdba6a2793ef699d/django/db/models/query.py#L1933-L1941
this branch] where `GenericForeignKey.get_prefetch_queryset` sets
`is_descriptor` to `True`.
I haven't tried it out but I suspect that switching `is_descriptor` to
`False`
[https://github.com/django/django/blob/3a6431db5461b88e691d770abdba6a2793ef699d/django/contrib/contenttypes/fields.py#L216
instead] now that `GenericForeignKey` has been changed to use the fields
cache (bfb746f983aa741afa3709794e70f1e0ab6040b5) would address your issue
and unify behaviours.
--
Ticket URL: <https://code.djangoproject.com/ticket/33008#comment:5>
Django
behaviour
--------------------------------------+------------------------------------
Reporter: Martin Svoboda | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.contenttypes | Version: 3.2
Severity: Normal | Resolution:
Keywords: Documentation | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Martin Svoboda):
Thank you, your recommendation actually fix the problem. I propose the
patch https://github.com/django/django/pull/14762
I found one problem with the solution. prefetch_related sets
tag.content_object to None, but access to this tag.content_object actually
cause extra lookup query.
--
Ticket URL: <https://code.djangoproject.com/ticket/33008#comment:6>
Django
behaviour
--------------------------------------+------------------------------------
Reporter: Martin Svoboda | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.contenttypes | Version: 3.2
Severity: Normal | Resolution:
Keywords: Documentation | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
Looks like the fetching of the object is due to the logic in `__get__`
https://github.com/django/django/blob/54a30a7a00fea6c5e3702282ade6e0238e06de3b/django/contrib/contenttypes/fields.py#L231-L245.
Not sure how feasible it would be to change it without breaking backward
compatiblity though, you'd likely need to avoid passing `default=None` to
`get_cached_value` and differentiate the sentinel value from `None`.
--
Ticket URL: <https://code.djangoproject.com/ticket/33008#comment:7>
Django
behaviour
--------------------------------------+------------------------------------
Reporter: Martin Svoboda | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.contenttypes | Version: 3.2
Severity: Normal | Resolution:
Keywords: Documentation | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jacob Walls):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/33008#comment:8>
Django
behaviour
--------------------------------------+------------------------------------
Reporter: Martin Svoboda | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.contenttypes | Version: 3.2
Severity: Normal | Resolution:
Keywords: Documentation | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Martin Svoboda):
I updated the object fetching logic of {{{__get__}}}. It respects
{{{None}}} as value if the instance exists in the cache. Is this solution
ok? It came to me as more readable than passing different {{{None}}}
values.
https://github.com/martinsvoboda/django/commit/7c83cec8b3bb99d9fe1822c3f82e58b373338a63
#diff-76b33afd71c30afb749a0de312cfe4a4578d78abae84a9d5db06388a4b1bc454R231
--
Ticket URL: <https://code.djangoproject.com/ticket/33008#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Martin Svoboda | Owner: Martin
| Svoboda
Type: Bug | Status: assigned
Component: | Version: 3.2
contrib.contenttypes |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => assigned
* owner: nobody => Martin Svoboda
* keywords: Documentation =>
* type: Cleanup/optimization => Bug
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/33008#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Martin Svoboda | Owner: Martin
| Svoboda
Component: | Version: 3.2
contrib.contenttypes |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"cc4cb95beff0b75ec169add7e94cc481624a41e6" cc4cb95b]:
{{{
#!CommitTicketReference repository=""
revision="cc4cb95beff0b75ec169add7e94cc481624a41e6"
Fixed #33008 -- Fixed prefetch_related() for deleted GenericForeignKeys.
Thanks Simon Charette for the implementation idea.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33008#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Martin Svoboda | Owner: Martin
| Svoboda
Type: Bug | Status: closed
Component: | Version: 3.2
contrib.contenttypes |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"dd8945d361d0c0f3183f32dfc729f28726c005f8" dd8945d3]:
{{{
#!CommitTicketReference repository=""
revision="dd8945d361d0c0f3183f32dfc729f28726c005f8"
[4.0.x] Fixed #33008 -- Fixed prefetch_related() for deleted
GenericForeignKeys.
Thanks Simon Charette for the implementation idea.
Backport of cc4cb95beff0b75ec169add7e94cc481624a41e6 from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33008#comment:12>