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:1 by Althalus, 10 years ago

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.

Last edited 10 years ago by Althalus (previous) (diff)

comment:2 by Baptiste Mispelon, 10 years ago

Component: UncategorizedForms
Needs documentation: set
Needs tests: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

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 Althalus, 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 Althalus, 10 years ago

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 by Tim Graham, 10 years ago

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

comment:6 by merb, 10 years ago

Triage Stage: AcceptedReady for checkin

made a small review and looks good to me.

comment:7 by Tim Graham <timograham@…>, 10 years ago

Resolution: fixed
Status: newclosed

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