Opened 3 years ago

Last modified 21 months ago

#22276 assigned Bug

BaseFormSet.is_valid() produces ValidationError when there is no management form

Reported by: anonymous Owned by: Patryk Zawadzki
Component: Forms Version: master
Severity: Normal Keywords:
Cc: patrys@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
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 (13)

comment:1 Changed 3 years ago by anonymous

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

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.

comment:2 Changed 3 years ago by anonymous

Resolution: invalid
Status: newclosed

comment:3 Changed 23 months ago by Patryk Zawadzki

Resolution: invalid
Status: closednew

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 Changed 23 months ago by Patryk Zawadzki

Owner: changed from nobody to Patryk Zawadzki
Status: newassigned

comment:5 Changed 22 months ago by Tim Graham

Triage Stage: UnreviewedAccepted
Version: 1.6master

comment:6 Changed 22 months ago by Patryk Zawadzki

comment:7 Changed 22 months ago by Tim Graham

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 Changed 22 months ago by Patryk Zawadzki

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 Changed 22 months ago by Patryk Zawadzki

Cc: patrys@… added

comment:10 Changed 22 months ago by Tim Graham

I think we could use the same wording as the existing exception.

comment:11 Changed 22 months ago by Patryk Zawadzki

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 Changed 22 months ago by Tim Graham

Patch needs improvement: unset

comment:13 Changed 21 months ago by Tim Graham

Patch needs improvement: set

Comments for improvement on PR.

Note: See TracTickets for help on using tickets.
Back to Top