[Django] #27119: full_clean() called too many times during formset validation
Django
------------------------------------------------+------------------------
Reporter: claudep | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Forms | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
I have a formset with 3 forms in it. I had to debug some stuff with it,
but then I realized the `full_clean` method is called 21 times when
calling the `formset.is_valid()`, while I would expect 4 times (once for
the formset itself and once for each subform). For example, a validation
occurs each time the `management_form` property is called. While it may
not cause real issues in practice, it's poor design and annoying when
trying to debug and follow the validation workflow.
--
Ticket URL: <https://code.djangoproject.com/ticket/27119>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: claudep | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
I think I'm lacking some details to understand this. Is this for plain
formsets and/or model formsets? Are the duplicated calls to
`BaseFormSet.full_clean()` and/or some other `full_clean()`? For example,
I added some debugging in Django's `test_basic_formset` but only saw
`BaseFormSet.full_clean()` called once.
--
Ticket URL: <https://code.djangoproject.com/ticket/27119#comment:1>
Django
Reporter: claudep | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by claudep):
I think this affects all sort of formsets. The problem might rather affect
the `full_clean()` methods of included subforms (notably for the
management form).
--
Ticket URL: <https://code.djangoproject.com/ticket/27119#comment:2>
Django
Reporter: claudep | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Forms | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Unreviewed => Accepted
Comment:
Making `BaseFormSet.management_form` a `@cached_property` seems to help
(not sure if it fully resolves the issue).
--
Ticket URL: <https://code.djangoproject.com/ticket/27119#comment:3>
Django
Reporter: claudep | Owner: DavidFozo
Type: | Status: assigned
Cleanup/optimization |
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by DavidFozo):
* status: new => assigned
* owner: nobody => DavidFozo
--
Ticket URL: <https://code.djangoproject.com/ticket/27119#comment:4>
Django
Reporter: Claude Paroz | Owner: Karol
Type: | Sztajerwald
Cleanup/optimization | Status: assigned
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Karol Sztajerwald):
* owner: DavidFozo => Karol Sztajerwald
--
Ticket URL: <https://code.djangoproject.com/ticket/27119#comment:5>
Django
Reporter: Claude Paroz | Owner: Karol
Type: | Sztajerwald
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Karol Sztajerwald):
* has_patch: 0 => 1
Comment:
PR here: https://github.com/django/django/pull/7496
--
Ticket URL: <https://code.djangoproject.com/ticket/27119#comment:6>
Django
Reporter: Claude Paroz | Owner: Karol
Type: | Sztajerwald
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Claude Paroz):
* needs_tests: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/27119#comment:7>
Django
Reporter: Claude Paroz | Owner: Karol
Type: | Sztajerwald
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Claude Paroz):
* needs_tests: 1 => 0
Comment:
Tests added in that [https://github.com/django/django/pull/7644 PR].
--
Ticket URL: <https://code.djangoproject.com/ticket/27119#comment:8>
Django
Reporter: Claude Paroz | Owner: Karol
Type: | Sztajerwald
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/27119#comment:9>
Django
Reporter: Claude Paroz | Owner: Karol
Type: | Sztajerwald
Component: Forms | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by GitHub <noreply@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"d49551bc261666fad753b0f55f1150467190e3f7" d49551bc]:
{{{
#!CommitTicketReference repository=""
revision="d49551bc261666fad753b0f55f1150467190e3f7"
Fixed #27119 -- Cached BaseFormSet.management_form property
Thanks Tim Graham for the review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27119#comment:10>