#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: master
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


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)

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:

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:1 Changed 16 months ago by Althalus

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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.

Version 0, edited 16 months ago by Althalus (next)

comment:2 Changed 16 months ago by bmispelon

  • Component changed from Uncategorized to Forms
  • Needs documentation set
  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted


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).


comment:3 Changed 16 months ago by Althalus

Sorry for the delay. I've made a new properly described commit in separate branch and corresponding pull request:

I suppose further discussion on this issue moves there?

comment:4 Changed 16 months ago by Althalus

This patch fixes obviously wrong behaviour which can be faced when using ModelChoiceFields 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 Changed 14 months ago by timo

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:6 Changed 14 months ago by merb

  • Triage Stage changed from Accepted to Ready for checkin

made a small review and looks good to me.

comment:7 Changed 14 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from new to closed

In 5e06fa1469180909c51c07151692412269e51ea3:

Fixed #22745 -- Prevented reevaluation of ModelChoiceField's queryset when accesssing BoundField's attrs.

Thanks Christian Schmitt for review.

Note: See TracTickets for help on using tickets.
Back to Top