[Django] #22276: BaseFormSet.is_valid() produces ValidationError when there is no management form
Django
form
----------------------------+--------------------
Reporter: anonymous | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.6
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------+--------------------
I was torn between reporting this as a bug or a feature request, but then
I thought if I make it a feature request it will most likely break a lot
of Django apps and hence I guess that means it's more of a bug...
Anyway so the line in question is django/forms/formsets.py:292 (in Django
version 1.6.1):
{{{
for i in range(0, self.total_form_count()):
}}}
...where the self.total_form_count() executes this line
django/forms/formsets.py:106 (in Django version 1.6.1):
{{{
return min(self.management_form.cleaned_data[TOTAL_FORM_COUNT],
self.absolute_max)
}}}
..which then raises this exception django/forms/formsets.py:87 (in Django
version 1.6.1):
{{{
raise ValidationError(
_('ManagementForm data is missing or has been tampered
with'),
code='missing_management_form',
)
}}}
That stack trace occurs if/when a user submits a formset after stripping
out the management form hidden fields.
I have been using Django for a few years now and have never come across an
exception being raised by a form/formset is_valid() call before. So my
point is that I believe this exception should never be allowed to leave
the BaseFormSet.is_valid() call, because it is an oddball behaviour
compared to the rest of the is_valid() implementations.
I.e. I believe there should be a check in BaseFormSet.is_valid() which
checks for the presence of a valid management form (first) and returns
False if it is not present, as opposed to raising an exception.
Yes I could wrap the is_valid() call in a try/catch, but I believe this is
an unnecessary hack caused by a bad design deviation of the implementation
of the BaseFormSet.is_valid() method.
I didn't bother creating a patch and test cases, because I have a feeling
this will get rejected or something like that, but I just thought I should
bring this up, as I can't find mention of it anywhere and it seems
important to me.
--
Ticket URL: <https://code.djangoproject.com/ticket/22276>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
form
Reporter: anonymous | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.6
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 anonymous):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
So it turns out this oddball behaviour is necessary because the formset
cannot be rendered after the lack of management form has been detected,
hence you should perform special behaviour for these cases of management
form tampering (e.g. redirecting to the same page to make the user start
over).
Sorry for wasting anyone's time on this.
--
Ticket URL: <https://code.djangoproject.com/ticket/22276#comment:1>
Django
form
Reporter: anonymous | Owner: nobody
Type: Bug | Status: closed
Component: Forms | Version: 1.6
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by anonymous):
* status: new => closed
* resolution: => invalid
--
Ticket URL: <https://code.djangoproject.com/ticket/22276#comment:2>
Django
form
Reporter: anonymous | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.6
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by patrys):
* status: closed => new
* resolution: invalid =>
Comment:
I would like to reopen this and propose changing the formset
implementation to never let a `ValidationError` exception escape
`.is_valid()`. The rationale: management form tampering indicates a
problem on the user side and it's not something the site's code can or
should recover from. Have the formset become invalid and otherwise behave
as if it did not contain any forms.
--
Ticket URL: <https://code.djangoproject.com/ticket/22276#comment:3>
Django
form
Reporter: anonymous | Owner: patrys
Type: Bug | Status: assigned
Component: Forms | Version: 1.6
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by patrys):
* owner: nobody => patrys
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/22276#comment:4>
Django
form
Reporter: anonymous | Owner: patrys
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* version: 1.6 => master
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/22276#comment:5>
Django
form
Reporter: anonymous | Owner: patrys
Type: Bug | Status: assigned
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by patrys):
Pull request: https://github.com/django/django/pull/3634
--
Ticket URL: <https://code.djangoproject.com/ticket/22276#comment:6>
Django
form
Reporter: anonymous | Owner: patrys
Type: Bug | Status: assigned
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
Comment:
Actually, I may have been premature in accepting this. It seems this will
make debugging quite a bit tricker (when you forget to include a
management form, the form will be silently invalid, correct?). At the
least, there should be some error message (see also #13060).
Also the current behavior is
[https://docs.djangoproject.com/en/dev/topics/forms/formsets
/#understanding-the-managementform documented] so we have to consider
backwards compatibility here.
--
Ticket URL: <https://code.djangoproject.com/ticket/22276#comment:7>
Django
form
Reporter: anonymous | Owner: patrys
Type: Bug | Status: assigned
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by patrys):
I agree that adding "management form is invalid" to non-form-specific
errors is a nice improvement. Can you provide good wording for that?
As for documentation, it's all damned lies as it used to raise in the
constructor like the docs show but now it raises in `is_valid` instead :)
--
Ticket URL: <https://code.djangoproject.com/ticket/22276#comment:8>
Django
form
Reporter: anonymous | Owner: patrys
Type: Bug | Status: assigned
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by patrys):
* cc: patrys@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/22276#comment:9>
Django
form
Reporter: anonymous | Owner: patrys
Type: Bug | Status: assigned
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by timgraham):
I think we could use the same wording as the existing exception.
--
Ticket URL: <https://code.djangoproject.com/ticket/22276#comment:10>
Django
form
Reporter: anonymous | Owner: patrys
Type: Bug | Status: assigned
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by patrys):
I've added a non-form error to the formset for the case where management
form is not valid. Formset's default string representations do not show
non-form errors but they do render individual field errors for the
management form which should be enough.
--
Ticket URL: <https://code.djangoproject.com/ticket/22276#comment:11>
Django
form
Reporter: anonymous | Owner: patrys
Type: Bug | Status: assigned
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 timgraham):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/22276#comment:12>
Django
form
Reporter: anonymous | Owner: patrys
Type: Bug | Status: assigned
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 timgraham):
* needs_better_patch: 0 => 1
Comment:
Comments for improvement on PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/22276#comment:13>
Django
form
Reporter: anonymous | Owner: Patryk Zawadzki
Type: Bug | Status: assigned
Component: Forms | Version: master
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/22276#comment:15>
Django
form
Reporter: anonymous | Owner: Patryk Zawadzki
Component: Forms | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by GitHub <noreply@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"859cd7c6b43bf70e2852eda10f635c70feeb397f" 859cd7c]:
{{{
#!CommitTicketReference repository=""
revision="859cd7c6b43bf70e2852eda10f635c70feeb397f"
Fixed #22276 -- Fixed crash when formset management form is invalid.
Co-authored-by: Patryk Zawadzki <pat...@room-303.com>
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22276#comment:16>