[Django] #29294: ModelAdmin.raw_id_fields should be included in select_related() by change_view and used by raw id widget
Django
change_view and used by raw id widget
------------------------------------------------+------------------------
Reporter: Yurii Zolot'ko | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.admin | Version: 1.11
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
Consider model A having many ForeignKey relationships to other models
B,C,D...Z (common case in many applications).
ModelAdmin has `raw_id_fields` optimization option to avoid querying full
contents of B...Z tables every time.
However in some cases this optimization is not enough as it still invokes
several queries to B... tables to fetch single row from every of them to
display `__str__` and url of related object next to raw input field when
admin's change_view is opened for single object.
So when there are 3 ForeignKeys there will at least 4 queries per request
(1 for object itself, 3 for every ForeignKey). When there are 10
ForeignKeys there will be 11 queries per request and so on resulting in
very high response time for complex objects even using raw_id_fields.
It looks like this problem is not possible to avoid now. I've tried to
override get_queryset() method of ModelAdmin to select_related()
raw_id_fields like this:
{{{
class ObjectAdmin(admin.ModelAdmin):
raw_id_fields = (
'field1', 'field2', ...)
def get_queryset(self, request):
qs = admin.ModelAdmin.get_queryset(self, request)
return qs.select_related(*self.raw_id_fields)
}}}
But
django.contrib.admin.widgets.ForeignKeyRawIdWidget.label_and_url_for_value()
still invokes separate query for every ForeignKey relationship instead of
getting it from attribute of already fetched object resulting in high
response times of admin interface impossible to improve.
It would be significant performance improvement if ModelAdmin could
select_related() `raw_id_fields` by default and ForeignKeyRawIdWidget
wouldn't do extra queries for every related object when all necessary data
is already present.
--
Ticket URL: <https://code.djangoproject.com/ticket/29294>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
change_view and used by raw id widget
Reporter: Yurii Zolot'ko | Owner: nobody
Cleanup/optimization |
Component: contrib.admin | Version: 1.11
Severity: Normal | Resolution: needsinfo
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* status: new => closed
* resolution: => needsinfo
Comment:
I'm not sure that including `raw_id_fields` in `select_related()` will
always be a net performance increase, so I'm skeptical about that
proposal.
The idea about allowing `ForeignKeyRawIdWidget` to reuse the queryset
seems interesting, however, I think you'll have to provide a patch to
convince me that this it's feasible and sufficiently backwards compatible.
Feel free to reopen the ticket if you provide that and someone will take
another look.
--
Ticket URL: <https://code.djangoproject.com/ticket/29294#comment:1>
Django
change_view and used by raw id widget
Reporter: Yurii Zolot'ko | Owner: nobody
Cleanup/optimization |
Component: contrib.admin | Version: 1.11
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jurrian Tromp):
* status: closed => new
* resolution: needsinfo =>
Comment:
I was about to create a new issue for what I think is the same thing so
reopening, I [https://stackoverflow.com/questions/65794839/using-
prefetch-related-in-django-in-combination-with-raw-id-fields also
encountered] this problem.
As stated the label_and_url_for_value() does a get() for each item. When
using the raw_id_fields, this is something you don't want. Especially
because it does not work with prefetching. I worked around the bug by
subclassing it in my own project and actually did a benchmark:
**Benchmark: change view page with 180 inline items with relations:**
1. Using no raw_id_field: 1658 queries
2. With raw_id_field and prefetching in Django 3.1: 384 queries
3. Patched raw_id_field and prefetching: 18 queries
This proves that my patched ForeignKeyRawIdWidget considerably reduces
queries as they already have been prefetched.
The culprit here is django.contrib.admin.widgets:176
{{{obj = self.rel.model._default_manager.using(self.db).get(**{key:
value})}}}
This is a excerpt of what is needed to improve the behaviour which can be
backwards compatible: we just need to try if value.pk exists, if not we
use the default behaviour:
{{{
class ForeignKeyRawIdWidget(widgets.ForeignKeyRawIdWidget):
def format_value(self, value):
"""Try to return the `pk` if value is an object, otherwise just
return
the value as fallback."""
if value == '' or value is None:
return None
try:
return str(value.pk)
except AttributeError:
return str(value)
def label_and_url_for_value(self, value):
"""Instead of the original we do not have do a `get()` anymore
instead
access the instance directly so when value is prefetched this will
prevent additional queries."""
try:
pk = value.pk
meta = value._meta
except AttributeError:
# Fallback for compatibility with plain pk values
return super().label_and_url_for_value(value)
}}}
Note that this is what I made in order to fix it in a project. If accepted
I can provide a more permanent solution. Please let me know if this of
interest.
--
Ticket URL: <https://code.djangoproject.com/ticket/29294#comment:2>
Django
change_view and used by raw id widget
Reporter: Yurii Zolot'ko | Owner: nobody
Cleanup/optimization |
Component: contrib.admin | Version: 1.11
Severity: Normal | Resolution: needsinfo
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* status: new => closed
* resolution: => needsinfo
Comment:
Hi Jurrian
It looks like the code example there isn't complete… 🤔 — Can I ask you to
post a sample project somewhere showing the widget in action, so I can
properly assess it?
If the `value` passed to `label_and_url_for_value` is a model instance
then maybe we can avoid the lookup but I need to see how that all fits
together to say properly.
Thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/29294#comment:3>
Django
change_view and used by raw id widget
Reporter: Yurii Zolot'ko | Owner: nobody
Cleanup/optimization |
Component: contrib.admin | Version: 1.11
Severity: Normal | Resolution: needsinfo
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jurrian Tromp):
* Attachment "raw_id_widget.py" added.
Django
change_view and used by raw id widget
Reporter: Yurii Zolot'ko | Owner: nobody
Cleanup/optimization |
Component: contrib.admin | Version: 1.11
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jurrian Tromp):
* status: closed => new
* resolution: needsinfo =>
Comment:
You are right, I am sorry. I left out the `BoundField` part which is
indeed elemental. See the attachment for a working example,
`ModelChoiceField` and `RawIdWidgetAdminMixin` are just there to patch it
to make it work in my project, they are not part of this.
The way I see it there is just one way to have the `value` supplied to
`label_and_url_for_value` to be a model instance:
{{{
class BoundField(boundfield.BoundField):
def value(self):
if type(self.field.widget) == ForeignKeyRawIdWidget:
try:
return getattr(self.form.instance, self.name)
except AttributeError:
pass
...
}}}
However this feels hacky to me, although I can not find another way to get
the model instance except for from the form. When being rendered, the
`ForeignKeyRawIdWidget` has no access to the form anymore, so it seems the
only place where we can pass it on is in the `BoundField`.
What do you think?
--
Ticket URL: <https://code.djangoproject.com/ticket/29294#comment:4>
Django
change_view and used by raw id widget
Reporter: Yurii Zolot'ko | Owner: nobody
Cleanup/optimization |
Component: contrib.admin | Version: 1.11
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* status: new => closed
* resolution: => wontfix
Comment:
Hi Jurrian — thanks for the follow-up.
> I left out the BoundField part ...
Yes… 🤔 With this it's quite a lot of extra complexity. In particular,
pulling the widget type into bound field feels wrong.
I'm going to say wontfix. Maybe it's worth a follow-up on the
DevelopersMailingList to see if folks have any thoughts on a way forward.
Otherwise though I suspect it's something that may just have to live in
your project.
--
Ticket URL: <https://code.djangoproject.com/ticket/29294#comment:5>