Opened 10 years ago
Closed 10 years ago
#22745 closed Cleanup/optimization (fixed)
ModelChoiceField and ModelMultipleChoiceField with RadioSelect and CheckboxSelectMultiple widgets produce unnecessary db queries while rendering form (particularily in contrib.admin)
Reported by: | Althalus | Owned by: | nobody |
---|---|---|---|
Component: | Forms | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Ready for checkin | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
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 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.
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.
Change History (7)
comment:2 by , 10 years ago
Component: | Uncategorized → Forms |
---|---|
Needs documentation: | set |
Needs tests: | set |
Patch needs improvement: | set |
Triage Stage: | Unreviewed → Accepted |
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.
comment:3 by , 10 years ago
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?
comment:4 by , 10 years ago
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?
comment:5 by , 10 years ago
Needs documentation: | unset |
---|---|
Needs tests: | unset |
Patch needs improvement: | unset |
comment:6 by , 10 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
made a small review and looks good to me.
comment:7 by , 10 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
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 if this ticket will be accepted.