[Django] #29078: Serializer handle_m2m_field should honour any previous prefetch
Django
------------------------------------------------+------------------------
Reporter: xx396 | Owner: nobody
Type: Bug | Status: new
Component: Core (Serialization) | Version: 2.0
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 1
UI/UX: 0 |
------------------------------------------------+------------------------
Example:
from django.contrib.auth.models import Group
from django.core import serializers
groups = Group.objects.prefetch_related('permissions__content_type').all()
serializers.serialize('json', groups)
This will N+1 query the permissions as handle_m2m_field uses iterator()
which bypasses any cache. Suggest serializers/python.py line 77 replaces
iterator() with all()
self._current[field.name] = [
m2m_value(related) for related in getattr(obj,
field.name).iterator()
]
--
Ticket URL: <https://code.djangoproject.com/ticket/29078>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: xx396 | Owner: nobody
Type: Bug | Status: new
(Serialization) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Description changed by xx396:
Old description:
> Example:
>
> from django.contrib.auth.models import Group
> from django.core import serializers
>
> groups =
> Group.objects.prefetch_related('permissions__content_type').all()
> serializers.serialize('json', groups)
>
> This will N+1 query the permissions as handle_m2m_field uses iterator()
> which bypasses any cache. Suggest serializers/python.py line 77 replaces
> iterator() with all()
>
> self._current[field.name] = [
> m2m_value(related) for related in getattr(obj,
> field.name).iterator()
> ]
New description:
Example:
from django.contrib.auth.models import Group
from django.core import serializers
groups = Group.objects.prefetch_related('permissions').all()
serializers.serialize('json', groups)
This will N+1 query the permissions as handle_m2m_field uses iterator()
which bypasses any cache. Suggest serializers/python.py line 77 replaces
iterator() with all()
--
--
Ticket URL: <https://code.djangoproject.com/ticket/29078#comment:1>
Django
Reporter: xx396 | Owner: nobody
Type: Bug | Status: new
(Serialization) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Description changed by xx396:
Old description:
> Example:
>
> from django.contrib.auth.models import Group
> from django.core import serializers
>
> groups = Group.objects.prefetch_related('permissions').all()
> serializers.serialize('json', groups)
>
> This will N+1 query the permissions as handle_m2m_field uses iterator()
> which bypasses any cache. Suggest serializers/python.py line 77 replaces
> iterator() with all()
New description:
Example:
from django.contrib.auth.models import Group
from django.core import serializers
groups = Group.objects.prefetch_related('permissions').all()
serializers.serialize('json', groups)
This will N+1 query the permissions as handle_m2m_field uses iterator()
which bypasses any cache. Suggest serializers/python.py line 78 replaces
iterator() with all()
--
--
Ticket URL: <https://code.djangoproject.com/ticket/29078#comment:2>
Django
Reporter: xx396 | Owner: Zeeshan
| Khan
Type: Bug | Status: assigned
Component: Core | Version: 2.0
(Serialization) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Zeeshan Khan):
* status: new => assigned
* owner: nobody => Zeeshan Khan
--
Ticket URL: <https://code.djangoproject.com/ticket/29078#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: xx396 | Owner: Zeeshan
Cleanup/optimization | Status: assigned
Component: Core | Version: 2.0
(Serialization) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Tim Graham):
* stage: Unreviewed => Accepted
* type: Bug => Cleanup/optimization
* easy: 1 => 0
Comment:
I guess it would be nice to fix that but removing `iterator()` negates the
memory savings on the non-prefetch case, doesn't it?
--
Ticket URL: <https://code.djangoproject.com/ticket/29078#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: xx396 | Owner: Zeeshan
Type: | Khan
Cleanup/optimization | Status: assigned
Component: Core | Version: 2.0
(Serialization) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
I have two suggestion commits for possible fixes:
- https://github.com/claudep/django/commit/7173c7de
- https://github.com/claudep/django/commit/68bbb8ff
I can turn one of them into a PR if an ORM informed person could validate
either approach.
--
Ticket URL: <https://code.djangoproject.com/ticket/29078#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: xx396 | Owner: Zeeshan
Type: | Khan
Cleanup/optimization | Status: assigned
Component: Core | Version: 2.0
(Serialization) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
Claude, I think https://github.com/claudep/django/commit/7173c7de makes
more sense as the API to retrieve cached many-to-many results should
likely be added to `RelatedManager.all()` instead of `iterator`.
--
Ticket URL: <https://code.djangoproject.com/ticket/29078#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: xx396 | Owner: Zeeshan
Type: | Khan
Cleanup/optimization | Status: assigned
Component: Core | Version: 2.0
(Serialization) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Claude Paroz):
Thanks, [https://github.com/django/django/pull/12966 PR].
--
Ticket URL: <https://code.djangoproject.com/ticket/29078#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: xx396 | Owner: Zeeshan
Type: | Khan
Component: Core | Version: 2.0
(Serialization) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"adf58311b84f2aa92ca57661a8ba8679481ac930" adf58311]:
{{{
#!CommitTicketReference repository=""
revision="adf58311b84f2aa92ca57661a8ba8679481ac930"
Fixed #29078 -- Made serializers respect prefetch_related() for m2m
fields.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29078#comment:8>