From 408d4310291cd1287f3dbc05aaeb5d205eba8751 Mon Sep 17 00:00:00 2001 From: Joseph Kocherhans Date: Thu, 21 Jan 2010 02:28:03 +0000 Subject: [PATCH] Fixed #12596. Calling super from a ModelForm's clean method is once again optional. Failing to call super only skips unique validation as documented. Thanks for the initial patch and tests, carljm. git-svn-id: http://code.djangoproject.com/svn/django/trunk@12269 bcc190cf-cafb-0310-a4f2-bffc1f526a37 --- django/forms/forms.py | 10 +++- django/forms/models.py | 54 +++++++++++++++---- .../model_forms_regress/tests.py | 21 ++++++++ 3 files changed, 73 insertions(+), 12 deletions(-) diff --git a/django/forms/forms.py b/django/forms/forms.py index c53a901e73..6f7a8ce97d 100644 --- a/django/forms/forms.py +++ b/django/forms/forms.py @@ -263,6 +263,12 @@ class BaseForm(StrAndUnicode): # 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() + if self._errors: + delattr(self, 'cleaned_data') + + def _clean_fields(self): for name, field in self.fields.items(): # value_from_datadict() gets the data from the data dictionaries. # Each widget type knows how to retrieve its own data, because some @@ -282,12 +288,12 @@ class BaseForm(StrAndUnicode): self._errors[name] = self.error_class(e.messages) if name in self.cleaned_data: del self.cleaned_data[name] + + def _clean_form(self): try: self.cleaned_data = self.clean() except ValidationError, e: self._errors[NON_FIELD_ERRORS] = self.error_class(e.messages) - if self._errors: - delattr(self, 'cleaned_data') def clean(self): """ diff --git a/django/forms/models.py b/django/forms/models.py index cce2319471..ec28b446bd 100644 --- a/django/forms/models.py +++ b/django/forms/models.py @@ -250,6 +250,16 @@ class BaseModelForm(BaseForm): super(BaseModelForm, self).__init__(data, files, auto_id, prefix, object_data, error_class, label_suffix, empty_permitted) + def _update_errors(self, 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): """ @@ -281,21 +291,45 @@ class BaseModelForm(BaseForm): return exclude def clean(self): + self.validate_unique() + return self.cleaned_data + + def _clean_fields(self): + """ + Cleans the form fields, constructs the instance, then cleans the model + fields. + """ + super(BaseModelForm, self)._clean_fields() opts = self._meta self.instance = construct_instance(self, self.instance, opts.fields, opts.exclude) exclude = self._get_validation_exclusions() try: - self.instance.full_clean(exclude=exclude) + self.instance.clean_fields(exclude=exclude) except ValidationError, e: - for k, v in e.message_dict.items(): - if k != NON_FIELD_ERRORS: - self._errors.setdefault(k, ErrorList()).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 e.message_dict: - raise ValidationError(e.message_dict[NON_FIELD_ERRORS]) - return self.cleaned_data + self._update_errors(e.message_dict) + + def _clean_form(self): + """ + Runs the instance's clean method, then the form's. This is becuase the + form will run validate_unique() by default, and we should run the + model's clean method first. + """ + try: + self.instance.clean() + except ValidationError, e: + self._update_errors(e.message_dict) + super(BaseModelForm, self)._clean_form() + + def validate_unique(self): + """ + Calls the instance's validate_unique() method and updates the form's + validation errors if any were raised. + """ + exclude = self._get_validation_exclusions() + try: + self.instance.validate_unique(exclude=exclude) + except ValidationError, e: + self._update_errors(e.message_dict) def save(self, commit=True): """ diff --git a/tests/regressiontests/model_forms_regress/tests.py b/tests/regressiontests/model_forms_regress/tests.py index f8b6511140..57d56551e5 100644 --- a/tests/regressiontests/model_forms_regress/tests.py +++ b/tests/regressiontests/model_forms_regress/tests.py @@ -50,6 +50,27 @@ class UniqueTogetherTests(TestCase): form = TripleForm({'left': '1', 'middle': '3', 'right': '1'}) self.failUnless(form.is_valid()) +class TripleFormWithCleanOverride(forms.ModelForm): + class Meta: + model = Triple + + def clean(self): + if not self.cleaned_data['left'] == self.cleaned_data['right']: + raise forms.ValidationError('Left and right should be equal') + return self.cleaned_data + +class OverrideCleanTests(TestCase): + def test_override_clean(self): + """ + Regression for #12596: Calling super from ModelForm.clean() should be + optional. + """ + form = TripleFormWithCleanOverride({'left': 1, 'middle': 2, 'right': 1}) + self.failUnless(form.is_valid()) + # form.instance.left will be None if the instance was not constructed + # by form.full_clean(). + self.assertEquals(form.instance.left, 1) + class FPForm(forms.ModelForm): class Meta: model = FilePathModel