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

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