From 267dc4adddd2882182f71a7f285a06b1d4b15af0 Mon Sep 17 00:00:00 2001 From: Jon Dufresne Date: Wed, 18 May 2016 07:30:42 -0700 Subject: [PATCH] Fixed #4136 -- Made ModelForm save empty values for nullable CharFields as NULL. Previously, empty values were saved as strings. --- django/db/models/base.py | 8 ++++--- django/db/models/fields/__init__.py | 3 +++ django/forms/fields.py | 5 +++-- docs/ref/forms/fields.txt | 8 ++++++- docs/ref/models/fields.txt | 8 ++++--- docs/releases/1.11.txt | 10 ++++++++- docs/topics/forms/modelforms.txt | 4 +++- tests/model_forms/models.py | 4 ++++ tests/model_forms/tests.py | 34 +++++++++++++++++++++++------ 9 files changed, 66 insertions(+), 18 deletions(-) diff --git a/django/db/models/base.py b/django/db/models/base.py index 52a96c3ffe..2444c22b01 100644 --- a/django/db/models/base.py +++ b/django/db/models/base.py @@ -13,8 +13,8 @@ from django.core.exceptions import ( ObjectDoesNotExist, ValidationError, ) from django.db import ( - DEFAULT_DB_ALIAS, DJANGO_VERSION_PICKLE_KEY, DatabaseError, connections, - router, transaction, + DEFAULT_DB_ALIAS, DJANGO_VERSION_PICKLE_KEY, DatabaseError, connection, + connections, router, transaction, ) from django.db.models import signals from django.db.models.constants import LOOKUP_SEP @@ -1087,7 +1087,9 @@ class Model(six.with_metaclass(ModelBase)): for field_name in unique_check: f = self._meta.get_field(field_name) lookup_value = getattr(self, f.attname) - if lookup_value is None: + # TODO: Handle multiple backends with different feature flags. + if (lookup_value is None or + (lookup_value == '' and connection.features.interprets_empty_strings_as_nulls)): # no value, skip the lookup continue if f.primary_key and not self._state.adding: diff --git a/django/db/models/fields/__init__.py b/django/db/models/fields/__init__.py index 76e19eb6e9..4fdba3de81 100644 --- a/django/db/models/fields/__init__.py +++ b/django/db/models/fields/__init__.py @@ -1086,6 +1086,9 @@ class CharField(Field): # will be validated twice. This is considered acceptable since we want # the value in the form field (to pass into widget for example). defaults = {'max_length': self.max_length} + # TODO: Handle multiple backends with different feature flags. + if self.null and not connection.features.interprets_empty_strings_as_nulls: + defaults['empty_value'] = None defaults.update(kwargs) return super(CharField, self).formfield(**defaults) diff --git a/django/forms/fields.py b/django/forms/fields.py index c50cd164ed..a20caca870 100644 --- a/django/forms/fields.py +++ b/django/forms/fields.py @@ -214,10 +214,11 @@ class Field(object): class CharField(Field): - def __init__(self, max_length=None, min_length=None, strip=True, *args, **kwargs): + def __init__(self, max_length=None, min_length=None, strip=True, empty_value='', *args, **kwargs): self.max_length = max_length self.min_length = min_length self.strip = strip + self.empty_value = empty_value super(CharField, self).__init__(*args, **kwargs) if min_length is not None: self.validators.append(validators.MinLengthValidator(int(min_length))) @@ -227,7 +228,7 @@ class CharField(Field): def to_python(self, value): "Returns a Unicode object." if value in self.empty_values: - return '' + return self.empty_value value = force_text(value) if self.strip: value = value.strip() diff --git a/docs/ref/forms/fields.txt b/docs/ref/forms/fields.txt index 7144a46a9d..0a5703c46e 100644 --- a/docs/ref/forms/fields.txt +++ b/docs/ref/forms/fields.txt @@ -361,7 +361,7 @@ For each field, we describe the default widget used if you don't specify .. class:: CharField(**kwargs) * Default widget: :class:`TextInput` - * Empty value: ``''`` (an empty string) + * Empty value: Whatever you've given as :attr:`empty_value`. * Normalizes to: A Unicode object. * Validates ``max_length`` or ``min_length``, if they are provided. Otherwise, all inputs are valid. @@ -380,6 +380,12 @@ For each field, we describe the default widget used if you don't specify If ``True`` (default), the value will be stripped of leading and trailing whitespace. + .. attribute:: empty_value + + .. versionadded:: 1.11 + + The value to use to represent "empty". Defaults to an empty string. + ``ChoiceField`` --------------- diff --git a/docs/ref/models/fields.txt b/docs/ref/models/fields.txt index a56c445c31..2435201db8 100644 --- a/docs/ref/models/fields.txt +++ b/docs/ref/models/fields.txt @@ -43,11 +43,13 @@ If ``True``, Django will store empty values as ``NULL`` in the database. Default is ``False``. Avoid using :attr:`~Field.null` on string-based fields such as -:class:`CharField` and :class:`TextField` because empty string values will -always be stored as empty strings, not as ``NULL``. If a string-based field has +:class:`CharField` and :class:`TextField`. If a string-based field has ``null=True``, that means it has two possible values for "no data": ``NULL``, and the empty string. In most cases, it's redundant to have two possible values -for "no data;" the Django convention is to use the empty string, not ``NULL``. +for "no data;" the Django convention is to use the empty string, not +``NULL``. One exception is when a :class:`CharField` has both ``unique=True`` +and ``blank=True`` set. In this situation, ``null=True`` is required to avoid +unique constraint violations when saving multiple objects with blank values. For both string-based and non-string-based fields, you will also need to set ``blank=True`` if you wish to permit empty values in forms, as the diff --git a/docs/releases/1.11.txt b/docs/releases/1.11.txt index d2e3e74fdd..709c4052f1 100644 --- a/docs/releases/1.11.txt +++ b/docs/releases/1.11.txt @@ -154,7 +154,8 @@ File Uploads Forms ~~~~~ -* ... +* The new :attr:`CharField.empty_value ` + attribute allows specifying the Python value to use to represent "empty". Generic Views ~~~~~~~~~~~~~ @@ -258,6 +259,13 @@ Miscellaneous displays the related object's ID. Remove the ``_id`` suffix if you want the old behavior of the string representation of the object. +* In model forms, :class:`~django.db.models.CharField` with ``null=True`` now + saves ``NULL`` for blank values instead of empty strings. + +* On Oracle, :meth:`Model.validate_unique() + ` no longer checks empty strings for + uniqueness as the database interprets the value as ``NULL``. + .. _deprecated-features-1.11: Features deprecated in 1.11 diff --git a/docs/topics/forms/modelforms.txt b/docs/topics/forms/modelforms.txt index 938516050a..9dcb2e3e53 100644 --- a/docs/topics/forms/modelforms.txt +++ b/docs/topics/forms/modelforms.txt @@ -66,7 +66,9 @@ Model field Form field :class:`CharField` :class:`~django.forms.CharField` with ``max_length`` set to the model field's - ``max_length`` + ``max_length`` and + :attr:`~django.forms.CharField.empty_value` + set to ``None`` if ``null=True``. :class:`CommaSeparatedIntegerField` :class:`~django.forms.CharField` diff --git a/tests/model_forms/models.py b/tests/model_forms/models.py index 2c3ff26cbe..ec39e5ad1f 100644 --- a/tests/model_forms/models.py +++ b/tests/model_forms/models.py @@ -483,3 +483,7 @@ class StrictAssignmentAll(models.Model): class Award(models.Model): name = models.CharField(max_length=30) character = models.ForeignKey(Character, models.SET_NULL, blank=False, null=True) + + +class NullableUniqueCharFieldModel(models.Model): + codename = models.CharField(max_length=50, blank=True, null=True, unique=True) diff --git a/tests/model_forms/tests.py b/tests/model_forms/tests.py index 78e7399637..297ccaae3d 100644 --- a/tests/model_forms/tests.py +++ b/tests/model_forms/tests.py @@ -28,9 +28,10 @@ from .models import ( CustomErrorMessage, CustomFF, CustomFieldForExclusionModel, DateTimePost, DerivedBook, DerivedPost, Document, ExplicitPK, FilePathModel, FlexibleDatePost, Homepage, ImprovedArticle, ImprovedArticleWithParentLink, - Inventory, Person, Photo, Post, Price, Product, Publication, - PublicationDefaults, StrictAssignmentAll, StrictAssignmentFieldSpecific, - Student, StumpJoke, TextFile, Triple, Writer, WriterProfile, test_images, + Inventory, NullableUniqueCharFieldModel, Person, Photo, Post, Price, + Product, Publication, PublicationDefaults, StrictAssignmentAll, + StrictAssignmentFieldSpecific, Student, StumpJoke, TextFile, Triple, + Writer, WriterProfile, test_images, ) if test_images: @@ -270,6 +271,21 @@ class ModelFormBaseTest(TestCase): obj = form.save() self.assertEqual(obj.name, '') + def test_save_blank_null_unique_charfield_saves_null(self): + form_class = modelform_factory(model=NullableUniqueCharFieldModel, fields=['codename']) + empty_value = '' if connection.features.interprets_empty_strings_as_nulls else None + + form = form_class(data={'codename': ''}) + self.assertTrue(form.is_valid()) + form.save() + self.assertEqual(form.instance.codename, empty_value) + + # Save a second form to verify there isn't a unique constraint violation. + form = form_class(data={'codename': ''}) + self.assertTrue(form.is_valid()) + form.save() + self.assertEqual(form.instance.codename, empty_value) + def test_missing_fields_attribute(self): message = ( "Creating a ModelForm without either the 'fields' attribute " @@ -800,10 +816,14 @@ class UniqueTest(TestCase): form.save() form = ExplicitPKForm({'key': 'key1', 'desc': ''}) self.assertFalse(form.is_valid()) - self.assertEqual(len(form.errors), 3) - self.assertEqual(form.errors['__all__'], ['Explicit pk with this Key and Desc already exists.']) - self.assertEqual(form.errors['desc'], ['Explicit pk with this Desc already exists.']) - self.assertEqual(form.errors['key'], ['Explicit pk with this Key already exists.']) + if connection.features.interprets_empty_strings_as_nulls: + self.assertEqual(len(form.errors), 1) + self.assertEqual(form.errors['key'], ['Explicit pk with this Key already exists.']) + else: + self.assertEqual(len(form.errors), 3) + self.assertEqual(form.errors['__all__'], ['Explicit pk with this Key and Desc already exists.']) + self.assertEqual(form.errors['desc'], ['Explicit pk with this Desc already exists.']) + self.assertEqual(form.errors['key'], ['Explicit pk with this Key already exists.']) def test_unique_for_date(self): p = Post.objects.create(