Opened 11 years ago
Closed 4 years ago
#22276 closed Bug (fixed)
BaseFormSet.is_valid() produces ValidationError when there is no management form
Reported by: | anonymous | Owned by: | Patryk Zawadzki |
---|---|---|---|
Component: | Forms | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | patrys@…, jon.dufresne@… | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
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.
Change History (16)
comment:1 by , 11 years ago
comment:2 by , 11 years ago
Resolution: | → invalid |
---|---|
Status: | new → closed |
comment:3 by , 10 years ago
Resolution: | invalid |
---|---|
Status: | closed → new |
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.
comment:4 by , 10 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:5 by , 10 years ago
Triage Stage: | Unreviewed → Accepted |
---|---|
Version: | 1.6 → master |
comment:7 by , 10 years ago
Has patch: | set |
---|---|
Patch needs improvement: | set |
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 documented so we have to consider backwards compatibility here.
comment:8 by , 10 years ago
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 :)
comment:9 by , 10 years ago
Cc: | added |
---|
comment:11 by , 10 years ago
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.
comment:12 by , 10 years ago
Patch needs improvement: | unset |
---|
comment:14 by , 4 years ago
Cc: | added |
---|---|
Patch needs improvement: | unset |
Thanks for the original work Patryk Zawadzki. I've continued it in PR https://github.com/django/django/pull/13607
comment:15 by , 4 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
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.