Redundant database query rendering a ModelChoiceField with RadioSelect or CheckboxSelectMultiple

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

[Alex Hill]

Implementation of (2) at ​https://github.com/django/django/compare/master...AlexHill:ticket_27002

That patch builds in #27001, so I haven't opened a PR for it yet.

[Tim Graham]

We're targeting template-based widget rendering, which removes ChoiceFieldRenderer, for 1.11 (​PR). Is this issue still relevant after that patch? Unless this is a regression in Django 1.10, we don't need to fix it there.

[Alex Hill]

Yep this is still relevant. The test included in this patch fails in the 15667 branch. This patch only concerns BoundField – the ChoiceFieldRenderer changes were all in #27001.

Now that #27001 is merged I've rebased on master and opened a ​pull request.

An unnecessary duplicated query is just as much a bug in 1.10 as it will be in 1.11. Why not fix it in 1.10?

[Simon Charette]

An unnecessary duplicated query is just as much a bug in 1.10 as it will be in 1.11. Why not fix it in 1.10?

Per the ​documented backport policy:

Patches applied to the master branch must also be applied to the last feature release branch, to be released in the next patch release of that feature series, when they fix critical problems:

• Security issues.

• Data loss bugs.
• Crashing bugs.
• Major functionality bugs in newly-introduced features.
• Regressions from older versions of Django.

The rule of thumb is that fixes will be backported to the last feature release for bugs that would have prevented a release in the first place (release blockers).

[Alex Hill]

That's very helpful, thanks.

[Tim Graham <timograham@…>]

In 74105b2:

Fixed #27002 -- Prevented double query when rendering ModelChoiceField.