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

Co-authored-by: Patryk Zawadzki <patrys@room-303.com>

django/forms/formsets.py

@@ -6,7 +6,7 @@ from django.forms.widgets import HiddenInput, NumberInput
 from django.utils.functional import cached_property
 from django.utils.html import html_safe
 from django.utils.safestring import mark_safe
-from django.utils.translation import gettext as _, ngettext
+from django.utils.translation import gettext_lazy as _, ngettext
 
 __all__ = ('BaseFormSet', 'formset_factory', 'all_valid')
 
@@ -41,6 +41,14 @@ class ManagementForm(Form):
         self.base_fields[MAX_NUM_FORM_COUNT] = IntegerField(required=False, widget=HiddenInput)
         super().__init__(*args, **kwargs)
 
+    def clean(self):
+        cleaned_data = super().clean()
+        # When the management form is invalid, we don't know how many forms
+        # were submitted.
+        cleaned_data.setdefault(TOTAL_FORM_COUNT, 0)
+        cleaned_data.setdefault(INITIAL_FORM_COUNT, 0)
+        return cleaned_data
+
 
 @html_safe
 class BaseFormSet:
@@ -48,9 +56,16 @@ class BaseFormSet:
     A collection of instances of the same Form class.
     """
     ordering_widget = NumberInput
+    default_error_messages = {
+        'missing_management_form': _(
+            'ManagementForm data is missing or has been tampered with. Missing fields: '
+            '%(field_names)s. You may need to file a bug report if the issue persists.'
+        ),
+    }
 
     def __init__(self, data=None, files=None, auto_id='id_%s', prefix=None,
-                 initial=None, error_class=ErrorList, form_kwargs=None):
+                 initial=None, error_class=ErrorList, form_kwargs=None,
+                 error_messages=None):
         self.is_bound = data is not None or files is not None
         self.prefix = prefix or self.get_default_prefix()
         self.auto_id = auto_id
@@ -62,6 +77,13 @@ class BaseFormSet:
         self._errors = None
         self._non_form_errors = None
 
+        messages = {}
+        for cls in reversed(type(self).__mro__):
+            messages.update(getattr(cls, 'default_error_messages', {}))
+        if error_messages is not None:
+            messages.update(error_messages)
+        self.error_messages = messages
+
     def __str__(self):
         return self.as_table()
 
@@ -88,18 +110,7 @@ class BaseFormSet:
         """Return the ManagementForm instance for this FormSet."""
         if self.is_bound:
             form = ManagementForm(self.data, auto_id=self.auto_id, prefix=self.prefix)
-            if not form.is_valid():
+            form.full_clean()
-                raise ValidationError(
-                    _(
-                        'ManagementForm data is missing or has been tampered '
-                        'with. Missing fields: %(field_names)s'
-                    ) % {
-                        'field_names': ', '.join(
-                            form.add_prefix(field_name) for field_name in form.errors
-                        ),
-                    },
-                    code='missing_management_form',
-                )
         else:
             form = ManagementForm(auto_id=self.auto_id, prefix=self.prefix, initial={
                 TOTAL_FORM_COUNT: self.total_form_count(),
@@ -327,6 +338,20 @@ class BaseFormSet:
 
         if not self.is_bound:  # Stop further processing.
             return
+
+        if not self.management_form.is_valid():
+            error = ValidationError(
+                self.error_messages['missing_management_form'],
+                params={
+                    'field_names': ', '.join(
+                        self.management_form.add_prefix(field_name)
+                        for field_name in self.management_form.errors
+                    ),
+                },
+                code='missing_management_form',
+            )
+            self._non_form_errors.append(error)
+
         for i, form in enumerate(self.forms):
             # Empty forms are unchanged forms beyond those with initial data.
             if not form.has_changed() and i >= self.initial_form_count():

docs/releases/3.2.txt

@@ -233,6 +233,12 @@ Forms
   removal of the option to delete extra forms. See
   :attr:`~.BaseFormSet.can_delete_extra` for more information.
 
+* :class:`~django.forms.formsets.BaseFormSet` now reports a user facing error,
+  rather than raising an exception, when the management form is missing or has
+  been tampered with. To customize this error message, pass the
+  ``error_messages`` argument with the key ``'missing_management_form'`` when
+  instantiating the formset.
+
 Generic Views
 ~~~~~~~~~~~~~
 

docs/topics/forms/formsets.txt

@@ -242,7 +242,7 @@ You may have noticed the additional data (``form-TOTAL_FORMS``,
 in the formset's data above. This data is required for the
 ``ManagementForm``. This form is used by the formset to manage the
 collection of forms contained in the formset. If you don't provide
-this management data, an exception will be raised::
+this management data, the formset will be invalid::
 
     >>> data = {
     ...     'form-0-title': 'Test',
@@ -250,9 +250,7 @@ this management data, an exception will be raised::
     ... }
     >>> formset = ArticleFormSet(data)
     >>> formset.is_valid()
-    Traceback (most recent call last):
+    False
-    ...
-    django.core.exceptions.ValidationError: ['ManagementForm data is missing or has been tampered with']
 
 It is used to keep track of how many form instances are being displayed. If
 you are adding new forms via JavaScript, you should increment the count fields
@@ -266,6 +264,11 @@ itself. When rendering a formset in a template, you can include all
 the management data by rendering ``{{ my_formset.management_form }}``
 (substituting the name of your formset as appropriate).
 
+.. versionchanged:: 3.2
+
+    ``formset.is_valid()`` now returns ``False`` rather than raising an
+    exception when the management form is missing or has been tampered with.
+
 ``total_form_count`` and ``initial_form_count``
 -----------------------------------------------
 
@@ -287,6 +290,30 @@ sure you understand what they do before doing so.
 a form instance with a prefix of ``__prefix__`` for easier use in dynamic
 forms with JavaScript.
 
+``error_messages``
+------------------
+
+.. versionadded:: 3.2
+
+The ``error_messages`` argument lets you override the default messages that the
+formset will raise. Pass in a dictionary with keys matching the error messages
+you want to override. For example, here is the default error message when the
+management form is missing::
+
+    >>> formset = ArticleFormSet({})
+    >>> formset.is_valid()
+    False
+    >>> formset.non_form_errors()
+    ['ManagementForm data is missing or has been tampered with. Missing fields: form-TOTAL_FORMS, form-INITIAL_FORMS. You may need to file a bug report if the issue persists.']
+
+And here is a custom error message::
+
+    >>> formset = ArticleFormSet({}, error_messages={'missing_management_form': 'Sorry, something went wrong.'})
+    >>> formset.is_valid()
+    False
+    >>> formset.non_form_errors()
+    ['Sorry, something went wrong.']
+
 Custom formset validation
 -------------------------
 

tests/forms_tests/tests/test_formsets.py

@@ -1300,13 +1300,69 @@ ArticleFormSet = formset_factory(ArticleForm)
 
 
 class TestIsBoundBehavior(SimpleTestCase):
-    def test_no_data_raises_validation_error(self):
+    def test_no_data_error(self):
-        msg = (
+        formset = ArticleFormSet({})
+        self.assertIs(formset.is_valid(), False)
+        self.assertEqual(
+            formset.non_form_errors(),
+            [
                 'ManagementForm data is missing or has been tampered with. '
-            'Missing fields: form-TOTAL_FORMS, form-INITIAL_FORMS'
+                'Missing fields: form-TOTAL_FORMS, form-INITIAL_FORMS. '
+                'You may need to file a bug report if the issue persists.',
+            ],
         )
-        with self.assertRaisesMessage(ValidationError, msg):
+        self.assertEqual(formset.errors, [])
-            ArticleFormSet({}).is_valid()
+        # Can still render the formset.
+        self.assertEqual(
+            str(formset),
+            '<tr><td colspan="2">'
+            '<ul class="errorlist nonfield">'
+            '<li>(Hidden field TOTAL_FORMS) This field is required.</li>'
+            '<li>(Hidden field INITIAL_FORMS) This field is required.</li>'
+            '</ul>'
+            '<input type="hidden" name="form-TOTAL_FORMS" id="id_form-TOTAL_FORMS">'
+            '<input type="hidden" name="form-INITIAL_FORMS" id="id_form-INITIAL_FORMS">'
+            '<input type="hidden" name="form-MIN_NUM_FORMS" id="id_form-MIN_NUM_FORMS">'
+            '<input type="hidden" name="form-MAX_NUM_FORMS" id="id_form-MAX_NUM_FORMS">'
+            '</td></tr>\n'
+        )
+
+    def test_management_form_invalid_data(self):
+        data = {
+            'form-TOTAL_FORMS': 'two',
+            'form-INITIAL_FORMS': 'one',
+        }
+        formset = ArticleFormSet(data)
+        self.assertIs(formset.is_valid(), False)
+        self.assertEqual(
+            formset.non_form_errors(),
+            [
+                'ManagementForm data is missing or has been tampered with. '
+                'Missing fields: form-TOTAL_FORMS, form-INITIAL_FORMS. '
+                'You may need to file a bug report if the issue persists.',
+            ],
+        )
+        self.assertEqual(formset.errors, [])
+        # Can still render the formset.
+        self.assertEqual(
+            str(formset),
+            '<tr><td colspan="2">'
+            '<ul class="errorlist nonfield">'
+            '<li>(Hidden field TOTAL_FORMS) Enter a whole number.</li>'
+            '<li>(Hidden field INITIAL_FORMS) Enter a whole number.</li>'
+            '</ul>'
+            '<input type="hidden" name="form-TOTAL_FORMS" value="two" id="id_form-TOTAL_FORMS">'
+            '<input type="hidden" name="form-INITIAL_FORMS" value="one" id="id_form-INITIAL_FORMS">'
+            '<input type="hidden" name="form-MIN_NUM_FORMS" id="id_form-MIN_NUM_FORMS">'
+            '<input type="hidden" name="form-MAX_NUM_FORMS" id="id_form-MAX_NUM_FORMS">'
+            '</td></tr>\n',
+        )
+
+    def test_customize_management_form_error(self):
+        formset = ArticleFormSet({}, error_messages={'missing_management_form': 'customized'})
+        self.assertIs(formset.is_valid(), False)
+        self.assertEqual(formset.non_form_errors(), ['customized'])
+        self.assertEqual(formset.errors, [])
 
     def test_with_management_data_attrs_work_fine(self):
         data = {

tests/model_formsets/tests.py

@@ -4,7 +4,7 @@ from datetime import date
 from decimal import Decimal
 
 from django import forms
-from django.core.exceptions import ImproperlyConfigured, ValidationError
+from django.core.exceptions import ImproperlyConfigured
 from django.db import models
 from django.forms.models import (
     BaseModelFormSet, _get_foreign_key, inlineformset_factory,
@@ -1783,11 +1783,10 @@ class ModelFormsetTest(TestCase):
             [{'id': ['Select a valid choice. That choice is not one of the available choices.']}],
         )
 
-    def test_initial_form_count_empty_data_raises_validation_error(self):
+    def test_initial_form_count_empty_data(self):
         AuthorFormSet = modelformset_factory(Author, fields='__all__')
-        msg = 'ManagementForm data is missing or has been tampered with'
+        formset = AuthorFormSet({})
-        with self.assertRaisesMessage(ValidationError, msg):
+        self.assertEqual(formset.initial_form_count(), 0)
-            AuthorFormSet({}).initial_form_count()
 
 
 class TestModelFormsetOverridesTroughFormMeta(TestCase):