[Django] #20849: ModelForms don't work well with prefetch_related
Django
-------------------------------+--------------------
Reporter: anonymous | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.5
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
Following the example at
[https://docs.djangoproject.com/en/1.5/ref/models/querysets/#prefetch-
related], ModelForms are a pain to use if you want to eagerly load many
pizza forms. It is more or less worth it to skip writing forms, since
figuring out how to write a manual view that can populate 15 pizza forms
with 3 queries is easy, whereas writing a form that can do it for fewer
than 33 is hard. Manually setting choices on the form can reduce this down
to 17, 1 for the list of pizzas, 1 inner join for each pizza, and 1 master
list for all toppings, plus 1 superfluous inner join for each pizza.
Removing that last bit seems hard and is certainly undocumented.
--
Ticket URL: <https://code.djangoproject.com/ticket/20849>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: anonymous | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.5
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 charettes):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
#18597 is a `InlineFormSet` related issue, we should really eat our own
dog food in `ModelForm`s.
--
Ticket URL: <https://code.djangoproject.com/ticket/20849#comment:1>
Django
Reporter: anonymous | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.5
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by david08uc@…):
I added a patch, which I made to the trunk version.
I ran the test suite, but with many skips and expected failures (and I
believe the relavant code path was not exercised). I don't have the
knowledge or time to fix that.
However, I believe this patch is a good idea, because the current behavior
resorts to a model utility method that always clones the queryset of a
many-to-many manager. This is overkill when the goal is to populate a list
of pk's, especially when the result is frozen as a list anyway.
--
Ticket URL: <https://code.djangoproject.com/ticket/20849#comment:2>
Django
Reporter: anonymous | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Forms | Version: 1.5
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* type: Uncategorized => Cleanup/optimization
* component: Uncategorized => Forms
* stage: Unreviewed => Accepted
Comment:
This would a test that uses `assertNumQueries()` to show that the query
count is reduced.
--
Ticket URL: <https://code.djangoproject.com/ticket/20849#comment:3>
Django
Reporter: anonymous | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Forms | Version: 1.5
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* has_patch: 0 => 1
* needs_tests: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/20849#comment:4>
Django
Reporter: anonymous | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Forms | Version: 1.5
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Jim Bailey):
Hello,
I have also run into this issue, so I fixed it on this branch:
https://github.com/dgym/django/tree/ticket_20849
The commit includes a unit test that now passes.
The fix only changes the behaviour if there is prefetched data, as the
above patch would hurt performance in the non-prefetched case.
--
Ticket URL: <https://code.djangoproject.com/ticket/20849#comment:5>
Django
Reporter: anonymous | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Forms | Version: 1.5
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jim Bailey):
* needs_tests: 1 => 0
Comment:
I have made the following pull request:
https://github.com/django/django/pull/1840
--
Ticket URL: <https://code.djangoproject.com/ticket/20849#comment:6>
Django
Reporter: anonymous | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master
Component: Forms | Resolution:
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by loic84):
* version: 1.5 => master
* stage: Accepted => Ready for checkin
Comment:
Looks good to me.
--
Ticket URL: <https://code.djangoproject.com/ticket/20849#comment:7>
Django
Reporter: anonymous | Owner: nobody
Type: | Status: closed
Cleanup/optimization | Version: master
Component: Forms | Resolution: fixed
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Anssi Kääriäinen <akaariai@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"539e3693d4712b249a95cfad8cfdeecdad1777a6"]:
{{{
#!CommitTicketReference repository=""
revision="539e3693d4712b249a95cfad8cfdeecdad1777a6"
Fixed #20849 -- ModelForms do not work well with prefetch_related.
model_to_dict() (used when rendering forms) queries the database
to get the list of primary keys for ManyToMany fields. This is
unnecessary if the field queryset has been prefetched, all the
keys are already in memory and can be obtained with a simple
iteration.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20849#comment:8>