[Django] #27002: Redundant database query rendering a ModelChoiceField with RadioSelect or CheckboxSelectMultiple
Django
CheckboxSelectMultiple
----------------------------+--------------------
Reporter: AlexHill | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.10
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------+--------------------
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
([https://github.com/django/django/blob/ccd5a23fba91e66656bb19b60bb6d737cab728f5/django/template/defaulttags.py#L164
GitHub link]):
{{{#!python
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:
{{{#!python
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27002>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
CheckboxSelectMultiple
Reporter: AlexHill | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.10
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by AlexHill):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Old description:
New description:
Followup to #27001:
--
--
Ticket URL: <https://code.djangoproject.com/ticket/27002#comment:1>
Django
CheckboxSelectMultiple
Reporter: AlexHill | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.10
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by AlexHill):
* has_patch: 0 => 1
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27002#comment:2>
Django
CheckboxSelectMultiple
Reporter: AlexHill | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.10
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
We're targeting template-based widget rendering, which removes
`ChoiceFieldRenderer`, for 1.11
([https://github.com/django/django/pull/6498 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27002#comment:3>
Django
CheckboxSelectMultiple
Reporter: AlexHill | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.10
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by AlexHill):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/27002#comment:4>
Django
CheckboxSelectMultiple
Reporter: AlexHill | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.10
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by charettes):
> 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 [https://docs.djangoproject.com/en/1.10/internals/release-process
/#supported-versions 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).
--
Ticket URL: <https://code.djangoproject.com/ticket/27002#comment:5>
Django
CheckboxSelectMultiple
Reporter: AlexHill | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* version: 1.10 => master
* type: Bug => Cleanup/optimization
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/27002#comment:6>
Django
CheckboxSelectMultiple
Reporter: AlexHill | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by AlexHill):
That's very helpful, thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/27002#comment:7>
Django
CheckboxSelectMultiple
Reporter: AlexHill | Owner: nobody
Type: Cleanup/optimization | Status: closed
Component: Forms | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"74105b26365c6e862f347656cc085faf18cc0bb1" 74105b2]:
{{{
#!CommitTicketReference repository=""
revision="74105b26365c6e862f347656cc085faf18cc0bb1"
Fixed #27002 -- Prevented double query when rendering ModelChoiceField.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27002#comment:8>