Opened 10 years ago

Closed 3 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 anonymous, 10 years ago

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 by anonymous, 10 years ago

Resolution: invalid
Status: newclosed

comment:3 by Patryk Zawadzki, 9 years ago

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 by Patryk Zawadzki, 9 years ago

Owner: changed from nobody to Patryk Zawadzki
Status: newassigned

comment:5 by Tim Graham, 9 years ago

Triage Stage: UnreviewedAccepted
Version: 1.6master

comment:7 by Tim Graham, 9 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 Patryk Zawadzki, 9 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 Patryk Zawadzki, 9 years ago

Cc: patrys@… added

comment:10 by Tim Graham, 9 years ago

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

comment:11 by Patryk Zawadzki, 9 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 Tim Graham, 9 years ago

Patch needs improvement: unset

comment:13 by Tim Graham, 9 years ago

Patch needs improvement: set

Comments for improvement on PR.

comment:14 by Jon Dufresne, 3 years ago

Cc: jon.dufresne@… 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 Carlton Gibson, 3 years ago

Triage Stage: AcceptedReady for checkin

comment:16 by GitHub <noreply@…>, 3 years ago

Resolution: fixed
Status: assignedclosed

In 859cd7c:

Fixed #22276 -- Fixed crash when formset management form is invalid.

Co-authored-by: Patryk Zawadzki <patrys@…>

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