Opened 17 months ago

Last modified 7 months ago

#22276 assigned Bug

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

Reported by: anonymous Owned by: patrys
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 17 months 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 17 months ago by anonymous

  • Resolution set to invalid
  • Status changed from new to closed

comment:3 Changed 9 months ago by patrys

  • Resolution invalid deleted
  • Status changed from closed to 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 Changed 9 months ago by patrys

  • Owner changed from nobody to patrys
  • Status changed from new to assigned

comment:5 Changed 8 months ago by timgraham

  • Triage Stage changed from Unreviewed to Accepted
  • Version changed from 1.6 to master

comment:7 Changed 8 months ago by timgraham

  • 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 8 months ago 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 :)

comment:9 Changed 8 months ago by patrys

  • Cc patrys@… added

comment:10 Changed 8 months ago by timgraham

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

comment:11 Changed 8 months ago 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.

comment:12 Changed 8 months ago by timgraham

  • Patch needs improvement unset

comment:13 Changed 7 months ago by timgraham

  • Patch needs improvement set

Comments for improvement on PR.

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