[Django] #22745: ModelChoiceField and ModelMultipleChoiceField with RadioSelect and CheckboxSelectMultiple widgets produce unnecessary db queries while rendering form (particularily in contrib.admin)
Django
CheckboxSelectMultiple widgets produce unnecessary db queries while
rendering form (particularily in contrib.admin)
--------------------------------------+--------------------
Reporter: Althalus | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Uncategorized | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------------+--------------------
So we have some admin change form (I was testing it only with my custom
admin forms but it obviously can be reproduced anywhere) with
ModelChoiceField(widget=RadioSelect) and/or
ModelMultipleChoiceField(widget=CheckboxSelectMultiple).
Form fields are rendered in
[https://github.com/django/django/blob/master/django/contrib/admin/templates/admin/includes/fieldset.html
admin/includes/fieldset.html] template. And calls of `{{ field.field }}`
perform extra queries (coz field reevaluates its queryset). So we get
**4** queries instead of 1.
According to django-debug-toolbar we have get 2 extra queries on 7th line
in fieldset.html (I suppose in `{% if field.field.name %}` and `{% if
field.field.is_hidden %}`) and 1 in `{% if field.field.help_text %}`
(except the main query in `{{ field.field }}` that is only one actually
needed).
Here is my stacktrace of what is really happens here.
{{{
lib/python3.4/site-packages/django/template/base.py in
_resolve_lookup(764)
current = current[bit]
lib/python3.4/site-packages/django/forms/forms.py in __getitem__(526)
return list(self.__iter__())[idx]
lib/python3.4/site-packages/django/forms/forms.py in __iter__(519)
for subwidget in self.field.widget.subwidgets(self.html_name,
self.value(), attrs):
lib/python3.4/site-packages/django/forms/widgets.py in subwidgets(727)
for widget in self.get_renderer(name, value, attrs, choices):
lib/python3.4/site-packages/django/forms/widgets.py in get_renderer(735)
choices = list(chain(self.choices, choices))
lib/python3.4/site-packages/django/forms/models.py in __iter__(1074)
for obj in self.queryset.all():
lib/python3.4/site-packages/django/db/models/query.py in __iter__(141)
self._fetch_all()
}}}
Consider relevant forms.py part.
{{{#!python
class BoundField(object):
...
def __iter__(self):
"""
Yields rendered strings that comprise all widgets in this
BoundField.
This really is only useful for RadioSelect widgets, so that you
can
iterate over individual radio buttons in a template.
"""
id_ = self.field.widget.attrs.get('id') or self.auto_id
attrs = {'id': id_} if id_ else {}
for subwidget in self.field.widget.subwidgets(self.html_name,
self.value(), attrs):
yield subwidget
def __len__(self):
return len(list(self.__iter__()))
def __getitem__(self, idx):
return list(self.__iter__())[idx]
}}}
As we know django template system do `.` lookups in the following order:
- Dictionary lookup
- Attribute lookup
- Method call
- List-index lookup
So when we try to access name, help_text, is_hidden etc. we evaluate
`list(self.__iter__())` coz `__getitem__` method is called first on our
BoundField.
I've made a simple patch which must not clash with anything while prevents
reevaluation:
https://github.com/Vincent-
Vega/django/commit/88478ef9d93af08e78de5e2755f8da036ab54b6f
One remaining issue is accessing BoundField object by indexes will still
hit db but I'm not sure if it important. Iterating is needed and I suppose
is used by many people.
But accessing individual radios or checkboxes by index? Anyway we have
__getitem__ so somebody could have used it in temapltes. I mean removing
this method could solve the problem too but it would be incompatible
change.
--
Ticket URL: <https://code.djangoproject.com/ticket/22745>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
CheckboxSelectMultiple widgets produce unnecessary db queries while
rendering form (particularily in contrib.admin)
Reporter: Althalus | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master
Component: Uncategorized | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Althalus):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
Also I think that this change is fully compatible because both in python
code and in templates new code raises the same exception as before
(`TypeError` for non-integers). It is just doing it earlier. My github
commit is not described properly yet but I'm ready to do it and make pull
request is this ticket will be accepted.
--
Ticket URL: <https://code.djangoproject.com/ticket/22745#comment:1>
Django
CheckboxSelectMultiple widgets produce unnecessary db queries while
rendering form (particularily in contrib.admin)
Reporter: Althalus | Owner: nobody
Type: Cleanup/optimization | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by bmispelon):
* needs_better_patch: 0 => 1
* needs_docs: 0 => 1
* component: Uncategorized => Forms
* needs_tests: 0 => 1
* stage: Unreviewed => Accepted
Comment:
Hi,
While I'm not too convinced by your approach of using `isinstance`, I
agree that it would be nice to be able to fix this.
In any case, a proper patch will require some tests and documentation (a
mention in the release notes for 1.8 at this point).
Thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/22745#comment:2>
Django
CheckboxSelectMultiple widgets produce unnecessary db queries while
rendering form (particularily in contrib.admin)
Reporter: Althalus | Owner: nobody
Type: Cleanup/optimization | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Althalus):
Sorry for the delay. I've made a new properly described commit in separate
branch and corresponding pull request:
https://github.com/django/django/pull/2816
I suppose further discussion on this issue moves there?
--
Ticket URL: <https://code.djangoproject.com/ticket/22745#comment:3>
Django
CheckboxSelectMultiple widgets produce unnecessary db queries while
rendering form (particularily in contrib.admin)
Reporter: Althalus | Owner: nobody
Type: Cleanup/optimization | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Althalus):
This patch fixes obviously wrong behaviour which can be faced when using
`ModelChoiceField`s in a common way (including admin).
But still accessing certain widget values from templates by index will
trigger qs every time. May be we can use some cache here?
--
Ticket URL: <https://code.djangoproject.com/ticket/22745#comment:4>
Django
CheckboxSelectMultiple widgets produce unnecessary db queries while
rendering form (particularily in contrib.admin)
Reporter: Althalus | Owner: nobody
Type: Cleanup/optimization | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Changes (by timo):
* needs_better_patch: 1 => 0
* needs_tests: 1 => 0
* needs_docs: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/22745#comment:5>
Django
CheckboxSelectMultiple widgets produce unnecessary db queries while
rendering form (particularily in contrib.admin)
Reporter: Althalus | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by merb):
* stage: Accepted => Ready for checkin
Comment:
made a small review and looks good to me.
--
Ticket URL: <https://code.djangoproject.com/ticket/22745#comment:6>
Django
CheckboxSelectMultiple widgets produce unnecessary db queries while
rendering form (particularily in contrib.admin)
Reporter: Althalus | Owner: nobody
Type: | Status: closed
Cleanup/optimization | Version: master
Component: Forms | Resolution: fixed
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"5e06fa1469180909c51c07151692412269e51ea3"]:
{{{
#!CommitTicketReference repository=""
revision="5e06fa1469180909c51c07151692412269e51ea3"
Fixed #22745 -- Prevented reevaluation of ModelChoiceField's queryset when
accesssing BoundField's attrs.
Thanks Christian Schmitt for review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22745#comment:7>