Opened 8 years ago

Last modified 8 years ago

#27002 closed Cleanup/optimization

Redundant database query rendering a ModelChoiceField with RadioSelect or CheckboxSelectMultiple — at Version 1

Reported by: Alex Hill Owned by: nobody
Component: Forms Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Alex Hill)

Followup to #27001:

Rendering a ModelChoiceField with RadioSelect and CheckboxSelectMultiple, there's a redundant iteration that triggers an unnecessary database call.

Here's the relevant code from ForNode.render (GitHub link):

            if not hasattr(values, '__len__'):  # line 164
                values = list(values)
            len_values = len(values)
            ...
            for i, item in enumerate(values):  # line 177
                ...

In this code, values is a BoundField. BoundField defines __len__ like this:

    def __len__(self):
        return len(list(self.__iter__()))

So len(values) on line 164 triggers an iteration over values (and thus a database query), then the for loop at line 177 triggers another.

The options as I see them are

  1. Remove BoundField.__len__ so that list(values) is called in ForNode.render. This fixes the problem without breaking any tests. It is backwards-incompatible (e.g. with an existing template passing a boundfield to the length template filter), but not with anything documented.
  2. Cache iterator results somewhere internally so that the actual query is only executed once. This is easy to do on BoundField with a cached property. No backward compatibility concerns here.
  3. Unconditionally call list(values) in ForNode. Also works, but a bit heavy-handed and not necessary if one of the first two options are acceptable.

I think (2) is the best choice. It's pretty clean, and it means the fix can be implemented in 1.10 (and 1.8 LTS?) as a bug fix without backward compatibility concerns.

Change History (1)

comment:1 by Alex Hill, 8 years ago

Description: modified (diff)
Note: See TracTickets for help on using tickets.
Back to Top