Opened 7 years ago
Closed 4 years ago
#29078 closed Cleanup/optimization (fixed)
Serializer handle_m2m_field() should honor any previous prefetch
Reported by: | xx396 | Owned by: | Zeeshan Khan |
---|---|---|---|
Component: | Core (Serialization) | Version: | 2.0 |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
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()
Change History (8)
comment:1 by , 7 years ago
Description: | modified (diff) |
---|
comment:2 by , 7 years ago
Description: | modified (diff) |
---|
comment:3 by , 7 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:4 by , 7 years ago
Easy pickings: | unset |
---|---|
Summary: | Serializer handle_m2m_field should honour any previous prefetch → Serializer handle_m2m_field() should honor any previous prefetch |
Triage Stage: | Unreviewed → Accepted |
Type: | Bug → Cleanup/optimization |
comment:5 by , 5 years ago
Has patch: | set |
---|
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.
comment:6 by , 5 years ago
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
.
I guess it would be nice to fix that but removing
iterator()
negates the memory savings on the non-prefetch case, doesn't it?