From f563c339ca2eed81706ab17726c79a6f00d7c553 Mon Sep 17 00:00:00 2001 From: Loic Bistuer Date: Tue, 12 Nov 2013 00:56:01 +0700 Subject: [PATCH] Fixed #20867 -- Added the Form.add_error() method. Refs #20199 #16986. Thanks @akaariai, @bmispelon, @mjtamlyn, @timgraham for the reviews. --- django/core/exceptions.py | 90 ++++++++++++++++----------- django/db/models/base.py | 2 +- django/forms/forms.py | 52 ++++++++++++++-- django/forms/models.py | 43 ++++++------- docs/ref/forms/api.txt | 20 ++++++ docs/ref/forms/validation.txt | 28 +++++++++ docs/releases/1.7.txt | 3 + tests/forms_tests/tests/test_forms.py | 50 +++++++++++++-- 8 files changed, 214 insertions(+), 74 deletions(-) diff --git a/django/core/exceptions.py b/django/core/exceptions.py index efec22850b..87b173b9b0 100644 --- a/django/core/exceptions.py +++ b/django/core/exceptions.py @@ -77,64 +77,78 @@ class ValidationError(Exception): """An error while validating data.""" def __init__(self, message, code=None, params=None): """ - ValidationError can be passed any object that can be printed (usually - a string), a list of objects or a dictionary. + The `message` argument can be a single error, a list of errors, or a + dictionary that maps field names to lists of errors. What we define as + an "error" can be either a simple string or an instance of + ValidationError with its message attribute set, and what we define as + list or dictionary can be an actual `list` or `dict` or an instance + of ValidationError with its `error_list` or `error_dict` attribute set. """ + if isinstance(message, ValidationError): + if hasattr(message, 'error_dict'): + message = message.error_dict + elif not hasattr(message, 'message'): + message = message.error_list + else: + message, code, params = message.message, message.code, message.params + if isinstance(message, dict): - self.error_dict = message + self.error_dict = {} + for field, messages in message.items(): + if not isinstance(messages, ValidationError): + messages = ValidationError(messages) + self.error_dict[field] = messages.error_list + elif isinstance(message, list): - self.error_list = message + self.error_list = [] + for message in message: + # Normalize plain strings to instances of ValidationError. + if not isinstance(message, ValidationError): + message = ValidationError(message) + self.error_list.extend(message.error_list) + else: + self.message = message self.code = code self.params = params - self.message = message self.error_list = [self] @property def message_dict(self): - message_dict = {} - for field, messages in self.error_dict.items(): - message_dict[field] = [] - for message in messages: - if isinstance(message, ValidationError): - message_dict[field].extend(message.messages) - else: - message_dict[field].append(force_text(message)) - return message_dict + return dict(self) @property def messages(self): if hasattr(self, 'error_dict'): - message_list = reduce(operator.add, self.error_dict.values()) - else: - message_list = self.error_list - - messages = [] - for message in message_list: - if isinstance(message, ValidationError): - params = message.params - message = message.message - if params: - message %= params - message = force_text(message) - messages.append(message) - return messages - - def __str__(self): - if hasattr(self, 'error_dict'): - return repr(self.message_dict) - return repr(self.messages) - - def __repr__(self): - return 'ValidationError(%s)' % self + return reduce(operator.add, dict(self).values()) + return list(self) def update_error_dict(self, error_dict): if hasattr(self, 'error_dict'): if error_dict: - for k, v in self.error_dict.items(): - error_dict.setdefault(k, []).extend(v) + for field, errors in self.error_dict.items(): + error_dict.setdefault(field, []).extend(errors) else: error_dict = self.error_dict else: error_dict[NON_FIELD_ERRORS] = self.error_list return error_dict + + def __iter__(self): + if hasattr(self, 'error_dict'): + for field, errors in self.error_dict.items(): + yield field, list(ValidationError(errors)) + else: + for error in self.error_list: + message = error.message + if error.params: + message %= error.params + yield force_text(message) + + def __str__(self): + if hasattr(self, 'error_dict'): + return repr(dict(self)) + return repr(list(self)) + + def __repr__(self): + return 'ValidationError(%s)' % self diff --git a/django/db/models/base.py b/django/db/models/base.py index ce3f095055..015fc9ff3c 100644 --- a/django/db/models/base.py +++ b/django/db/models/base.py @@ -987,7 +987,7 @@ class Model(six.with_metaclass(ModelBase)): def clean_fields(self, exclude=None): """ - Cleans all fields and raises a ValidationError containing message_dict + Cleans all fields and raises a ValidationError containing a dict of all validation errors if any occur. """ if exclude is None: diff --git a/django/forms/forms.py b/django/forms/forms.py index 15bbff229b..350b8b8993 100644 --- a/django/forms/forms.py +++ b/django/forms/forms.py @@ -290,6 +290,51 @@ class BaseForm(object): prefix = self.add_prefix(fieldname) return field.widget.value_from_datadict(self.data, self.files, prefix) + def add_error(self, field, error): + """ + Update the content of `self._errors`. + + The `field` argument is the name of the field to which the errors + should be added. If its value is None the errors will be treated as + NON_FIELD_ERRORS. + + The `error` argument can be a single error, a list of errors, or a + dictionary that maps field names to lists of errors. What we define as + an "error" can be either a simple string or an instance of + ValidationError with its message attribute set and what we define as + list or dictionary can be an actual `list` or `dict` or an instance + of ValidationError with its `error_list` or `error_dict` attribute set. + + If `error` is a dictionary, the `field` argument *must* be None and + errors will be added to the fields that correspond to the keys of the + dictionary. + """ + if not isinstance(error, ValidationError): + # Normalize to ValidationError and let its constructor + # do the hard work of making sense of the input. + error = ValidationError(error) + + if hasattr(error, 'error_dict'): + if field is not None: + raise TypeError( + "The argument `field` must be `None` when the `error` " + "argument contains errors for multiple fields." + ) + else: + error = dict(error) + else: + error = {field or NON_FIELD_ERRORS: list(error)} + + for field, error_list in error.items(): + if field not in self.errors: + if field != NON_FIELD_ERRORS and field not in self.fields: + raise ValueError( + "'%s' has no field named '%s'." % (self.__class__.__name__, field)) + self._errors[field] = self.error_class() + self._errors[field].extend(error_list) + if field in self.cleaned_data: + del self.cleaned_data[field] + def full_clean(self): """ Cleans all of self.data and populates self._errors and @@ -303,6 +348,7 @@ class BaseForm(object): # changed from the initial data, short circuit any validation. if self.empty_permitted and not self.has_changed(): return + self._clean_fields() self._clean_form() self._post_clean() @@ -324,15 +370,13 @@ class BaseForm(object): value = getattr(self, 'clean_%s' % name)() self.cleaned_data[name] = value except ValidationError as e: - self._errors[name] = self.error_class(e.messages) - if name in self.cleaned_data: - del self.cleaned_data[name] + self.add_error(name, e) def _clean_form(self): try: cleaned_data = self.clean() except ValidationError as e: - self._errors[NON_FIELD_ERRORS] = self.error_class(e.messages) + self.add_error(None, e) else: if cleaned_data is not None: self.cleaned_data = cleaned_data diff --git a/django/forms/models.py b/django/forms/models.py index b14ea7a265..5c2c77cbf2 100644 --- a/django/forms/models.py +++ b/django/forms/models.py @@ -326,27 +326,6 @@ class BaseModelForm(BaseForm): super(BaseModelForm, self).__init__(data, files, auto_id, prefix, object_data, error_class, label_suffix, empty_permitted) - def _update_errors(self, errors): - for field, messages in errors.error_dict.items(): - if field not in self.fields: - continue - field = self.fields[field] - for message in messages: - if isinstance(message, ValidationError): - if message.code in field.error_messages: - message.message = field.error_messages[message.code] - - message_dict = errors.message_dict - for k, v in message_dict.items(): - if k != NON_FIELD_ERRORS: - self._errors.setdefault(k, self.error_class()).extend(v) - # Remove the data from the cleaned_data dict since it was invalid - if k in self.cleaned_data: - del self.cleaned_data[k] - if NON_FIELD_ERRORS in message_dict: - messages = message_dict[NON_FIELD_ERRORS] - self._errors.setdefault(NON_FIELD_ERRORS, self.error_class()).extend(messages) - def _get_validation_exclusions(self): """ For backwards-compatibility, several types of fields need to be @@ -393,6 +372,20 @@ class BaseModelForm(BaseForm): self._validate_unique = True return self.cleaned_data + def _update_errors(self, errors): + # Override any validation error messages defined at the model level + # with those defined on the form fields. + for field, messages in errors.error_dict.items(): + if field not in self.fields: + continue + field = self.fields[field] + for message in messages: + if (isinstance(message, ValidationError) and + message.code in field.error_messages): + message.message = field.error_messages[message.code] + + self.add_error(None, errors) + def _post_clean(self): opts = self._meta # Update the model instance with self.cleaned_data. @@ -407,13 +400,12 @@ class BaseModelForm(BaseForm): # object being referred to may not yet fully exist (#12749). # However, these fields *must* be included in uniqueness checks, # so this can't be part of _get_validation_exclusions(). - for f_name, field in self.fields.items(): + for name, field in self.fields.items(): if isinstance(field, InlineForeignKeyField): - exclude.append(f_name) + exclude.append(name) try: - self.instance.full_clean(exclude=exclude, - validate_unique=False) + self.instance.full_clean(exclude=exclude, validate_unique=False) except ValidationError as e: self._update_errors(e) @@ -695,6 +687,7 @@ class BaseModelFormSet(BaseFormSet): del form.cleaned_data[field] # mark the data as seen seen_data.add(data) + if errors: raise ValidationError(errors) diff --git a/docs/ref/forms/api.txt b/docs/ref/forms/api.txt index c15f748308..c06a836bef 100644 --- a/docs/ref/forms/api.txt +++ b/docs/ref/forms/api.txt @@ -117,6 +117,26 @@ The validation routines will only get called once, regardless of how many times you access :attr:`~Form.errors` or call :meth:`~Form.is_valid`. This means that if validation has side effects, those side effects will only be triggered once. +.. method:: Form.add_error(field, error) + +.. versionadded:: 1.7 + +This method allows adding errors to specific fields from within the +``Form.clean()`` method, or from outside the form altogether; for instance +from a view. This is a better alternative to fiddling directly with +``Form._errors`` as described in :ref:`modifying-field-errors`. + +The ``field`` argument is the name of the field to which the errors +should be added. If its value is ``None`` the error will be treated as +a non-field error as returned by ``Form.non_field_errors()``. + +The ``error`` argument can be a simple string, or preferably an instance of +``ValidationError``. See :ref:`raising-validation-error` for best practices +when defining form errors. + +Note that ``Form.add_error()`` automatically removes the relevant field from +``cleaned_data``. + Behavior of unbound forms ~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/docs/ref/forms/validation.txt b/docs/ref/forms/validation.txt index ea294c4108..3c7715dfcb 100644 --- a/docs/ref/forms/validation.txt +++ b/docs/ref/forms/validation.txt @@ -464,3 +464,31 @@ Secondly, once we have decided that the combined data in the two fields we are considering aren't valid, we must remember to remove them from the ``cleaned_data``. `cleaned_data`` is present even if the form doesn't validate, but it contains only field values that did validate. + +.. versionchanged:: 1.7 + +In lieu of manipulating ``_errors`` directly, it's now possible to add errors +to specific fields with :meth:`django.forms.Form.add_error()`:: + + from django import forms + + class ContactForm(forms.Form): + # Everything as before. + ... + + def clean(self): + cleaned_data = super(ContactForm, self).clean() + cc_myself = cleaned_data.get("cc_myself") + subject = cleaned_data.get("subject") + + if cc_myself and subject and "help" not in subject: + msg = u"Must put 'help' in subject when cc'ing yourself." + self.add_error('cc_myself', msg) + self.add_error('subject', msg) + +The second argument of ``add_error()`` can be a simple string, or preferably +an instance of ``ValidationError``. See :ref:`raising-validation-error` for +more details. + +Unlike the ``_errors`` approach, ``add_error()` automatically removes the field +from ``cleaned_data``. diff --git a/docs/releases/1.7.txt b/docs/releases/1.7.txt index 9cb907aded..0767659540 100644 --- a/docs/releases/1.7.txt +++ b/docs/releases/1.7.txt @@ -350,6 +350,9 @@ Forms * It's now possible to opt-out from a ``Form`` field declared in a parent class by shadowing it with a non-``Field`` value. +* The new :meth:`~django.forms.Form.add_error()` method allows adding errors + to specific form fields. + Internationalization ^^^^^^^^^^^^^^^^^^^^ diff --git a/tests/forms_tests/tests/test_forms.py b/tests/forms_tests/tests/test_forms.py index b30742106b..9fec9ddcaa 100644 --- a/tests/forms_tests/tests/test_forms.py +++ b/tests/forms_tests/tests/test_forms.py @@ -657,25 +657,49 @@ class FormsTestCase(TestCase): self.assertEqual(f.cleaned_data['password2'], 'foo') # Another way of doing multiple-field validation is by implementing the - # Form's clean() method. If you do this, any ValidationError raised by that - # method will not be associated with a particular field; it will have a - # special-case association with the field named '__all__'. - # Note that in Form.clean(), you have access to self.cleaned_data, a dictionary of - # all the fields/values that have *not* raised a ValidationError. Also note - # Form.clean() is required to return a dictionary of all clean data. + # Form's clean() method. Usually ValidationError raised by that method + # will not be associated with a particular field and will have a + # special-case association with the field named '__all__'. It's + # possible to associate the errors to particular field with the + # Form.add_error() method or by passing a dictionary that maps each + # field to one or more errors. + # + # Note that in Form.clean(), you have access to self.cleaned_data, a + # dictionary of all the fields/values that have *not* raised a + # ValidationError. Also note Form.clean() is required to return a + # dictionary of all clean data. class UserRegistration(Form): username = CharField(max_length=10) password1 = CharField(widget=PasswordInput) password2 = CharField(widget=PasswordInput) def clean(self): + # Test raising a ValidationError as NON_FIELD_ERRORS. if self.cleaned_data.get('password1') and self.cleaned_data.get('password2') and self.cleaned_data['password1'] != self.cleaned_data['password2']: raise ValidationError('Please make sure your passwords match.') + # Test raising ValidationError that targets multiple fields. + errors = {} + if self.cleaned_data.get('password1') == 'FORBIDDEN_VALUE': + errors['password1'] = 'Forbidden value.' + if self.cleaned_data.get('password2') == 'FORBIDDEN_VALUE': + errors['password2'] = ['Forbidden value.'] + if errors: + raise ValidationError(errors) + + # Test Form.add_error() + if self.cleaned_data.get('password1') == 'FORBIDDEN_VALUE2': + self.add_error(None, 'Non-field error 1.') + self.add_error('password1', 'Forbidden value 2.') + if self.cleaned_data.get('password2') == 'FORBIDDEN_VALUE2': + self.add_error('password2', 'Forbidden value 2.') + raise ValidationError('Non-field error 2.') + return self.cleaned_data f = UserRegistration(auto_id=False) self.assertEqual(f.errors, {}) + f = UserRegistration({}, auto_id=False) self.assertHTMLEqual(f.as_table(), """Username: Password1: @@ -683,6 +707,7 @@ class FormsTestCase(TestCase): self.assertEqual(f.errors['username'], ['This field is required.']) self.assertEqual(f.errors['password1'], ['This field is required.']) self.assertEqual(f.errors['password2'], ['This field is required.']) + f = UserRegistration({'username': 'adrian', 'password1': 'foo', 'password2': 'bar'}, auto_id=False) self.assertEqual(f.errors['__all__'], ['Please make sure your passwords match.']) self.assertHTMLEqual(f.as_table(), """ @@ -693,12 +718,25 @@ class FormsTestCase(TestCase):
  • Username:
  • Password1:
  • Password2:
    • """) + f = UserRegistration({'username': 'adrian', 'password1': 'foo', 'password2': 'foo'}, auto_id=False) self.assertEqual(f.errors, {}) self.assertEqual(f.cleaned_data['username'], 'adrian') self.assertEqual(f.cleaned_data['password1'], 'foo') self.assertEqual(f.cleaned_data['password2'], 'foo') + f = UserRegistration({'username': 'adrian', 'password1': 'FORBIDDEN_VALUE', 'password2': 'FORBIDDEN_VALUE'}, auto_id=False) + self.assertEqual(f.errors['password1'], ['Forbidden value.']) + self.assertEqual(f.errors['password2'], ['Forbidden value.']) + + f = UserRegistration({'username': 'adrian', 'password1': 'FORBIDDEN_VALUE2', 'password2': 'FORBIDDEN_VALUE2'}, auto_id=False) + self.assertEqual(f.errors['__all__'], ['Non-field error 1.', 'Non-field error 2.']) + self.assertEqual(f.errors['password1'], ['Forbidden value 2.']) + self.assertEqual(f.errors['password2'], ['Forbidden value 2.']) + + with six.assertRaisesRegex(self, ValueError, "has no field named"): + f.add_error('missing_field', 'Some error.') + def test_dynamic_construction(self): # It's possible to construct a Form dynamically by adding to the self.fields # dictionary in __init__(). Don't forget to call Form.__init__() within the