Skip to content

Commit

Permalink
Fixed #4136 -- Made ModelForm save empty values for nullable CharFiel…
Browse files Browse the repository at this point in the history
…ds as NULL.

Previously, empty values were saved as strings.
  • Loading branch information
jdufresne authored and timgraham committed Jun 13, 2016
1 parent f2c0eb1 commit 267dc4a
Show file tree
Hide file tree
Showing 9 changed files with 66 additions and 18 deletions.
8 changes: 5 additions & 3 deletions django/db/models/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
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
Expand Down Expand Up @@ -1087,7 +1087,9 @@ def _perform_unique_checks(self, unique_checks):
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:
Expand Down
3 changes: 3 additions & 0 deletions django/db/models/fields/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -1086,6 +1086,9 @@ def formfield(self, **kwargs):
# 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)

Expand Down
5 changes: 3 additions & 2 deletions django/forms/fields.py
Original file line number Diff line number Diff line change
Expand Up @@ -214,10 +214,11 @@ def __deepcopy__(self, memo):


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)))
Expand All @@ -227,7 +228,7 @@ def __init__(self, max_length=None, min_length=None, strip=True, *args, **kwargs
def to_python(self, value):
"Returns a Unicode object."
if value in self.empty_values:
return ''
return self.empty_value

This comment has been minimized.

Copy link
@vin-ai

vin-ai Jun 30, 2016

Returning None instead empty string causing bug at line 700.

'NoneType' object has no attribute 'strip'

Please add a None value check for both EmailField and URLField

value = force_text(value)
if self.strip:
value = value.strip()
Expand Down
8 changes: 7 additions & 1 deletion docs/ref/forms/fields.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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``
---------------

Expand Down
8 changes: 5 additions & 3 deletions docs/ref/models/fields.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
10 changes: 9 additions & 1 deletion docs/releases/1.11.txt
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,8 @@ File Uploads
Forms
~~~~~

* ...
* The new :attr:`CharField.empty_value <django.forms.CharField.empty_value>`
attribute allows specifying the Python value to use to represent "empty".

Generic Views
~~~~~~~~~~~~~
Expand Down Expand Up @@ -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()
<django.db.models.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
Expand Down
4 changes: 3 additions & 1 deletion docs/topics/forms/modelforms.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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`

Expand Down
4 changes: 4 additions & 0 deletions tests/model_forms/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -483,3 +483,7 @@ def __setattr__(self, key, value):
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)
34 changes: 27 additions & 7 deletions tests/model_forms/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,10 @@
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:
Expand Down Expand Up @@ -270,6 +271,21 @@ def test_save_blank_false_with_required_false(self):
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 "
Expand Down Expand Up @@ -800,10 +816,14 @@ def test_explicitpk_unique(self):
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(
Expand Down

0 comments on commit 267dc4a

Please sign in to comment.