diff --git a/django/contrib/admin/options.py b/django/contrib/admin/options.py index 6dc707e..8f68eb1 100644 --- a/django/contrib/admin/options.py +++ b/django/contrib/admin/options.py @@ -579,12 +579,12 @@ class ModelAdmin(BaseModelAdmin): """ messages.info(request, message) - def save_form(self, request, form, change, commit=False): + def save_form(self, request, form, change): """ Given a ModelForm return an unsaved instance. ``change`` is True if the object is being changed, and False if it's being added. """ - return form.save(commit=commit) + return form.save(commit=False) def save_model(self, request, obj, form, change): """ @@ -758,11 +758,7 @@ class ModelAdmin(BaseModelAdmin): if request.method == 'POST': form = ModelForm(request.POST, request.FILES) if form.is_valid(): - # Save the object, even if inline formsets haven't been - # validated yet. We need to pass the valid model to the - # formsets for validation. If the formsets do not validate, we - # will delete the object. - new_object = self.save_form(request, form, change=False, commit=True) + new_object = self.save_form(request, form, change=False) form_validated = True else: form_validated = False @@ -779,15 +775,13 @@ class ModelAdmin(BaseModelAdmin): prefix=prefix, queryset=inline.queryset(request)) formsets.append(formset) if all_valid(formsets) and form_validated: + self.save_model(request, new_object, form, change=False) + form.save_m2m() for formset in formsets: self.save_formset(request, form, formset, change=False) self.log_addition(request, new_object) return self.response_add(request, new_object) - elif form_validated: - # The form was valid, but formsets were not, so delete the - # object we saved above. - new_object.delete() else: # Prepare the dict of initial data from the request. # We have to special-case M2Ms as a list of comma-separated PKs. diff --git a/django/contrib/auth/forms.py b/django/contrib/auth/forms.py index dbc55ca..e9a7fb2 100644 --- a/django/contrib/auth/forms.py +++ b/django/contrib/auth/forms.py @@ -1,4 +1,4 @@ -from django.contrib.auth.models import User, UNUSABLE_PASSWORD +from django.contrib.auth.models import User from django.contrib.auth import authenticate from django.contrib.auth.tokens import default_token_generator from django.contrib.sites.models import Site @@ -21,12 +21,6 @@ class UserCreationForm(forms.ModelForm): model = User fields = ("username",) - def clean(self): - # Fill the password field so model validation won't complain about it - # being blank. We'll set it with the real value below. - self.instance.password = UNUSABLE_PASSWORD - super(UserCreationForm, self).clean() - def clean_username(self): username = self.cleaned_data["username"] try: @@ -43,6 +37,10 @@ class UserCreationForm(forms.ModelForm): self.instance.set_password(password1) return password2 + def save(self, *args, **kwargs): + self.instance.set_password(self.cleaned_data["password1"]) + return super(UserCreationForm, self).save(*args, **kwargs) + class UserChangeForm(forms.ModelForm): username = forms.RegexField(label=_("Username"), max_length=30, regex=r'^\w+$', help_text = _("Required. 30 characters or fewer. Alphanumeric characters only (letters, digits and underscores)."), diff --git a/django/core/exceptions.py b/django/core/exceptions.py index fee7db4..bc599df 100644 --- a/django/core/exceptions.py +++ b/django/core/exceptions.py @@ -33,7 +33,7 @@ class FieldError(Exception): pass NON_FIELD_ERRORS = '__all__' -class BaseValidationError(Exception): +class ValidationError(Exception): """An error while validating data.""" def __init__(self, message, code=None, params=None): import operator @@ -64,10 +64,14 @@ class BaseValidationError(Exception): return repr(self.message_dict) return repr(self.messages) -class ValidationError(BaseValidationError): - pass - -class UnresolvableValidationError(BaseValidationError): - """Validation error that cannot be resolved by the user.""" - pass +def update_errors(errors, e): + if hasattr(e, 'message_dict'): + if errors: + for k, v in e.message_dict.items(): + errors.setdefault(k, []).extend(v) + else: + errors = e.message_dict + else: + errors[NON_FIELD_ERRORS] = e.messages + return errors diff --git a/django/db/models/base.py b/django/db/models/base.py index 06db7cc..6a1960b 100644 --- a/django/db/models/base.py +++ b/django/db/models/base.py @@ -3,7 +3,7 @@ import sys import os from itertools import izip import django.db.models.manager # Imported to register signal handler. -from django.core.exceptions import ObjectDoesNotExist, MultipleObjectsReturned, FieldError, ValidationError, NON_FIELD_ERRORS +from django.core.exceptions import ObjectDoesNotExist, MultipleObjectsReturned, FieldError, ValidationError, NON_FIELD_ERRORS, update_errors from django.core import validators from django.db.models.fields import AutoField, FieldDoesNotExist from django.db.models.fields.related import OneToOneRel, ManyToOneRel, OneToOneField @@ -642,17 +642,55 @@ class Model(object): def prepare_database_save(self, unused): return self.pk - def validate(self): + def full_validate(self): """ - Hook for doing any extra model-wide validation after clean() has been - called on every field. Any ValidationError raised by this method will - not be associated with a particular field; it will have a special-case - association with the field defined by NON_FIELD_ERRORS. + Calls validate_fields, validate_unique, and validate on the model, + and combines and reraises any ``ValidationError``s that occur. """ - self.validate_unique() + errors = {} + + try: + self.validate_fields() + except ValidationError, e: + errors = update_errors(errors, e) - def validate_unique(self): - unique_checks, date_checks = self._get_unique_checks() + try: + self.validate_unique() + except ValidationError, e: + errors = update_errors(errors, e) + + # Form.clean() is run even if other validation fails, so do the + # same with Model.validate() for consistency. + try: + self.validate() + except ValidationError, e: + errors = update_errors(errors, e) + + if errors: + raise ValidationError(errors) + + def validate_fields(self, fields=None): + """ + Cleans all fields and raises a ValidationError containing message_dict + of all validation errors if any occur. + + If ``fields`` is supplied, only validate the specified fields. + """ + errors = {} + if fields is None: + fields = self._meta.fields + else: + fields = [self._meta.get_field_by_name(f)[0] for f in fields] + for f in fields: + try: + setattr(self, f.attname, f.clean(getattr(self, f.attname), self)) + except ValidationError, e: + errors[f.name] = e.messages + if errors: + raise ValidationError(errors) + + def validate_unique(self, fields=None): + unique_checks, date_checks = self._get_unique_checks(fields=fields) errors = self._perform_unique_checks(unique_checks) date_errors = self._perform_date_checks(date_checks) @@ -663,17 +701,38 @@ class Model(object): if errors: raise ValidationError(errors) - def _get_unique_checks(self): - from django.db.models.fields import FieldDoesNotExist, Field as ModelField + def _get_unique_checks(self, fields=None): + if fields is None: + fields = [f.name for f in self._meta.fields] + unique_checks = [] + # Gather a list of checks to perform. Since validate_unique could be + # called from a ModelForm, some fields may have been excluded; we can't + # perform a unique check on a model that is missing fields involved + # in that check. It also does not make sense to check data that didn't + # validate, and since NULL does not equal NULL in SQL we should not do + # any unique checking for NULL values. + for check in self._meta.unique_together: + for name in check: + # If this is an excluded field, short circuit and don't a unique_check. + if name not in fields: + break + # Also, Skip fields that don't have a value, we can't check for + # their uniqueness. + if getattr(self, name, None) is None: + break + else: + unique_checks.append(check) - unique_checks = list(self._meta.unique_together) - # these are checks for the unique_for_ + # These are checks for the unique_for_. date_checks = [] # Gather a list of checks for fields declared as unique and add them to # the list of checks. Again, skip empty fields and any that did not validate. for f in self._meta.fields: name = f.name + # Don't add excluded fields to unique for date checks. + if name not in fields: + continue if f.unique: unique_checks.append((name,)) if f.unique_for_date: @@ -684,7 +743,6 @@ class Model(object): date_checks.append(('month', name, f.unique_for_month)) return unique_checks, date_checks - def _perform_unique_checks(self, unique_checks): errors = {} @@ -781,36 +839,14 @@ class Model(object): 'field_label': unicode(field_labels) } - def full_validate(self, exclude=[]): + def validate(self): """ - Cleans all fields and raises ValidationError containing message_dict - of all validation errors if any occur. + Hook for doing any extra model-wide validation after clean() has been + called on every field. Any ValidationError raised by this method will + not be associated with a particular field; it will have a special-case + association with the field defined by NON_FIELD_ERRORS. """ - errors = {} - for f in self._meta.fields: - if f.name in exclude: - continue - try: - setattr(self, f.attname, f.clean(getattr(self, f.attname), self)) - except ValidationError, e: - errors[f.name] = e.messages - - # Form.clean() is run even if other validation fails, so do the - # same with Model.validate() for consistency. - try: - self.validate() - except ValidationError, e: - if hasattr(e, 'message_dict'): - if errors: - for k, v in e.message_dict.items(): - errors.set_default(k, []).extend(v) - else: - errors = e.message_dict - else: - errors[NON_FIELD_ERRORS] = e.messages - - if errors: - raise ValidationError(errors) + pass ############################################ diff --git a/django/db/models/fields/related.py b/django/db/models/fields/related.py index 8fec836..ae6a2f7 100644 --- a/django/db/models/fields/related.py +++ b/django/db/models/fields/related.py @@ -740,6 +740,11 @@ class ForeignKey(RelatedField, Field): def validate(self, value, model_instance): if self.rel.parent_link: return + # Don't validate the field if a value wasn't supplied. This is + # generally the case when saving new inlines in the admin. + # See #12507. + if value is None: + return super(ForeignKey, self).validate(value, model_instance) if not value: return diff --git a/django/forms/models.py b/django/forms/models.py index ff20c93..58d6a10 100644 --- a/django/forms/models.py +++ b/django/forms/models.py @@ -9,7 +9,7 @@ from django.utils.datastructures import SortedDict from django.utils.text import get_text_list, capfirst from django.utils.translation import ugettext_lazy as _, ugettext -from django.core.exceptions import ValidationError, NON_FIELD_ERRORS, UnresolvableValidationError +from django.core.exceptions import ValidationError, update_errors, NON_FIELD_ERRORS from django.core.validators import EMPTY_VALUES from util import ErrorList from forms import BaseForm, get_declared_fields @@ -248,28 +248,48 @@ class BaseModelForm(BaseForm): def clean(self): opts = self._meta self.instance = construct_instance(self, self.instance, opts.fields, opts.exclude) + # For backwards-compatibility, several types of fields need to be + # excluded from model validation. See the following tickets for + # details: #12507, #12521, #12553 + errors = {} + # Build up a list of fields that should be included in model validation + # and unique checks. + fields = [] + for f in self.instance._meta.fields: + field = f.name + # Exclude fields that aren't on the form. The developer may be + # adding these values to the model after form validation. + if field not in self.fields: + continue + # Exclude fields that failed form validation. There's no need for + # the model to validate them as well. + elif field in self._errors.keys(): + continue + # Exclude empty fields that are not required by the form. Model + # validation might complain that they are empty, but the + # developer is reponsible for setting their value after calling + # form validation. + elif (not self.fields[field].required) and self.cleaned_data.get(field, None) is None: + continue + else: + fields.append(field) + try: + self.instance.validate_fields(fields=list(fields)) + except ValidationError, e: + errors = update_errors(errors, e) try: - self.instance.full_validate(exclude=self._errors.keys()) + self.instance.validate_unique(fields=fields) except ValidationError, e: - for k, v in e.message_dict.items(): + errors = update_errors(errors, e) + if errors: + for k, v in errors.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]) - - # If model validation threw errors for fields that aren't on the - # form, the the errors cannot be corrected by the user. Displaying - # those errors would be pointless, so raise another type of - # exception that *won't* be caught and displayed by the form. - if set(e.message_dict.keys()) - set(self.fields.keys() + [NON_FIELD_ERRORS]): - raise UnresolvableValidationError(e.message_dict) - - + if NON_FIELD_ERRORS in errors: + raise ValidationError(errors[NON_FIELD_ERRORS]) return self.cleaned_data def save(self, commit=True): diff --git a/tests/modeltests/model_forms/models.py b/tests/modeltests/model_forms/models.py index ba59f9a..cb2aa14 100644 --- a/tests/modeltests/model_forms/models.py +++ b/tests/modeltests/model_forms/models.py @@ -1114,6 +1114,15 @@ True >>> instance.delete() +# Test the non-required FileField +>>> f = TextFileForm(data={'description': u'Assistance'}) +>>> f.fields['file'].required = False +>>> f.is_valid() +True +>>> instance = f.save() +>>> instance.file + + >>> f = TextFileForm(data={'description': u'Assistance'}, files={'file': SimpleUploadedFile('test3.txt', 'hello world')}, instance=instance) >>> f.is_valid() True @@ -1152,7 +1161,7 @@ True >>> class BigIntForm(forms.ModelForm): ... class Meta: ... model = BigInt -... +... >>> bif = BigIntForm({'biggie': '-9223372036854775808'}) >>> bif.is_valid() True @@ -1425,16 +1434,41 @@ False >>> form._errors {'__all__': [u'Price with this Price and Quantity already exists.']} -# This form is never valid because quantity is blank=False. +This Price instance generated by this form is not valid because the quantity +field is required, but the form is valid because the field is excluded from +the form. This is for backwards compatibility. + >>> class PriceForm(ModelForm): ... class Meta: ... model = Price ... exclude = ('quantity',) >>> form = PriceForm({'price': '6.00'}) >>> form.is_valid() +True +>>> price = form.save(commit=False) +>>> price.full_validate() Traceback (most recent call last): ... -UnresolvableValidationError: {'quantity': [u'This field cannot be null.']} +ValidationError: {'quantity': [u'This field cannot be null.']} + +The form should not validate fields that it doesn't contain even if they are +specified using 'fields', not 'exclude'. +... class Meta: +... model = Price +... fields = ('price',) +>>> form = PriceForm({'price': '6.00'}) +>>> form.is_valid() +True + +The form should still have an instance of a model that is not complete and +not saved into a DB yet. + +>>> form.instance.price +Decimal('6.00') +>>> form.instance.quantity is None +True +>>> form.instance.pk is None +True # Unique & unique together with null values >>> class BookForm(ModelForm): diff --git a/tests/modeltests/model_formsets/models.py b/tests/modeltests/model_formsets/models.py index 5eab202..702523e 100644 --- a/tests/modeltests/model_formsets/models.py +++ b/tests/modeltests/model_formsets/models.py @@ -543,6 +543,10 @@ This is used in the admin for save_as functionality. ... 'book_set-2-title': '', ... } +>>> formset = AuthorBooksFormSet(data, instance=Author(), save_as_new=True) +>>> formset.is_valid() +True + >>> new_author = Author.objects.create(name='Charles Baudelaire') >>> formset = AuthorBooksFormSet(data, instance=new_author, save_as_new=True) >>> [book for book in formset.save() if book.author.pk == new_author.pk] @@ -1031,6 +1035,19 @@ False >>> formset._non_form_errors [u'Please correct the duplicate data for price and quantity, which must be unique.'] +# Only the price field is specified, this should skip any unique checks since +# the unique_together is not fulfilled. This will fail with a KeyError if broken. +>>> FormSet = modelformset_factory(Price, fields=("price",), extra=2) +>>> data = { +... 'form-TOTAL_FORMS': '2', +... 'form-INITIAL_FORMS': '0', +... 'form-0-price': '24', +... 'form-1-price': '24', +... } +>>> formset = FormSet(data) +>>> formset.is_valid() +True + >>> FormSet = inlineformset_factory(Author, Book, extra=0) >>> author = Author.objects.order_by('id')[0] >>> book_ids = author.book_set.values_list('id', flat=True) diff --git a/tests/modeltests/validation/test_unique.py b/tests/modeltests/validation/test_unique.py index cbb56aa..3aea99d 100644 --- a/tests/modeltests/validation/test_unique.py +++ b/tests/modeltests/validation/test_unique.py @@ -1,4 +1,5 @@ import unittest +import datetime from django.conf import settings from django.db import connection from models import CustomPKModel, UniqueTogetherModel, UniqueFieldsModel, UniqueForDateModel, ModelToValidate @@ -13,7 +14,13 @@ class GetUniqueCheckTests(unittest.TestCase): ) def test_unique_together_gets_picked_up(self): - m = UniqueTogetherModel() + # Give the fields values, otherwise they won't be picked up in + # unique checks. + m = UniqueTogetherModel( + cfield='test', + ifield=1, + efield='test@example.com' + ) self.assertEqual( ([('ifield', 'cfield',),('ifield', 'efield'), ('id',), ], []), m._get_unique_checks() @@ -24,10 +31,18 @@ class GetUniqueCheckTests(unittest.TestCase): self.assertEqual(([('my_pk_field',)], []), m._get_unique_checks()) def test_unique_for_date_gets_picked_up(self): - m = UniqueForDateModel() + # Give the fields values, otherwise they won't be picked up in + # unique checks. + m = UniqueForDateModel( + start_date=datetime.date(2010, 1, 9), + end_date=datetime.datetime(2010, 1, 9, 9, 0, 0), + count=1, + order=1, + name='test' + ) self.assertEqual(( - [('id',)], - [('date', 'count', 'start_date'), ('year', 'count', 'end_date'), ('month', 'order', 'end_date')] + [('id',)], + [('date', 'count', 'start_date'), ('year', 'count', 'end_date'), ('month', 'order', 'end_date')] ), m._get_unique_checks() ) @@ -56,3 +71,4 @@ class PerformUniqueChecksTest(unittest.TestCase): mtv = ModelToValidate(number=10, name='Some Name') mtv.full_validate() self.assertEqual(l, len(connection.queries)) + diff --git a/tests/modeltests/validation/tests.py b/tests/modeltests/validation/tests.py index c00070b..002e136 100644 --- a/tests/modeltests/validation/tests.py +++ b/tests/modeltests/validation/tests.py @@ -19,7 +19,7 @@ class BaseModelValidationTests(ValidationTestCase): mtv = ModelToValidate(number=10, name='Some Name') self.assertEqual(None, mtv.full_validate()) - def test_custom_validate_method_is_called(self): + def test_custom_validate_method(self): mtv = ModelToValidate(number=11) self.assertFailsValidation(mtv.full_validate, [NON_FIELD_ERRORS, 'name']) diff --git a/tests/regressiontests/admin_views/tests.py b/tests/regressiontests/admin_views/tests.py index 899fff8..351974b 100644 --- a/tests/regressiontests/admin_views/tests.py +++ b/tests/regressiontests/admin_views/tests.py @@ -4,7 +4,8 @@ import re import datetime from django.core.files import temp as tempfile from django.test import TestCase -from django.contrib.auth.models import User, Permission +from django.contrib.auth import admin # Register auth models with the admin. +from django.contrib.auth.models import User, Permission, UNUSABLE_PASSWORD from django.contrib.contenttypes.models import ContentType from django.contrib.admin.models import LogEntry, DELETION from django.contrib.admin.sites import LOGIN_FORM_KEY @@ -1753,3 +1754,37 @@ class ReadonlyTest(TestCase): self.assertEqual(Post.objects.count(), 2) p = Post.objects.order_by('-id')[0] self.assertEqual(p.posted, datetime.date.today()) + +class IncompleteFormTest(TestCase): + """ + Tests validation of a ModelForm that doesn't explicitly have all data + corresponding to model fields. Model validation shouldn't fail + such a forms. + """ + fixtures = ['admin-views-users.xml'] + + def setUp(self): + self.client.login(username='super', password='secret') + + def tearDown(self): + self.client.logout() + + def test_user_creation(self): + response = self.client.post('/test_admin/admin/auth/user/add/', { + 'username': 'newuser', + 'password1': 'newpassword', + 'password2': 'newpassword', + }) + new_user = User.objects.order_by('-id')[0] + self.assertRedirects(response, '/test_admin/admin/auth/user/%s/' % new_user.pk) + self.assertNotEquals(new_user.password, UNUSABLE_PASSWORD) + + def test_password_mismatch(self): + response = self.client.post('/test_admin/admin/auth/user/add/', { + 'username': 'newuser', + 'password1': 'newpassword', + 'password2': 'mismatch', + }) + self.assertEquals(response.status_code, 200) + self.assert_('password' not in response.context['form'].errors) + self.assertFormError(response, 'form', 'password2', ["The two password fields didn't match."])