Opened 8 years ago

Closed 8 years ago

#27002 closed Cleanup/optimization (fixed)

Redundant database query rendering a ModelChoiceField with RadioSelect or CheckboxSelectMultiple

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 (8)

comment:1 by Alex Hill, 8 years ago

Description: modified (diff)

comment:2 by Alex Hill, 8 years ago

Has patch: set

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.

comment:3 by Tim Graham, 8 years ago

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.

comment:4 by Alex Hill, 8 years ago

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?

Last edited 8 years ago by Alex Hill (previous) (diff)

comment:5 by Simon Charette, 8 years ago

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

comment:6 by Simon Charette, 8 years ago

Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization
Version: 1.10master

comment:7 by Alex Hill, 8 years ago

That's very helpful, thanks.

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

Resolution: fixed
Status: newclosed

In 74105b2:

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

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