Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#28555 closed Bug (fixed)

Inconsistent empty value for EmailField(blank=True, null=True) due to strip after check for empty value

Reported by: Olav Morken Owned by: Josh Schneier
Component: Forms Version: master
Severity: Normal Keywords:
Cc: Tim Martin Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Hi,

I just noticed that if I enter a single space in an EmailField in a ModelForm where the backing model field is EmailField(blank=True, null=True), the value is stored as an empty string. On the other hand, if I don't enter anything in the field, it is stored as a None value.

Looking at the code (https://github.com/django/django/blob/1.11.4/django/forms/fields.py#L234-L241), I think the cause is that it first checks for empty values, and then strips the string.

To reproduce this behavior, try something like:

# models.py
from django.db import models

class SomeUser(models.Model):
    address = models.EmailField(null=True, blank=True)


# views.py
import pprint
from django import forms
from django.http import HttpResponse
from django.views.decorators.csrf import csrf_exempt
from .models import SomeUser

class SomeUserForm(forms.ModelForm):
    class Meta:
        fields = [ 'address' ]
        model = SomeUser

@csrf_exempt
def test_view(request):
    form = SomeUserForm(request.POST)
    some_user = form.save()
    return HttpResponse(pprint.pformat(some_user.address) + "\n")

Testing this with curl gives something like this:

$ curl -XPOST -d address= http://127.0.0.1:1253/test
None
curl -XPOST -d address=%20 http://127.0.0.1:1253/test
u''

Change History (10)

comment:1 Changed 3 years ago by Tim Graham

Triage Stage: UnreviewedAccepted

comment:2 Changed 3 years ago by Josh Schneier

Has patch: set
Owner: changed from nobody to Josh Schneier
Status: newassigned
Version: 1.11master

comment:3 Changed 3 years ago by Tim Martin

Cc: Tim Martin added
Patch needs improvement: set

The change looks good to me, but it should be squashed into a single commit.

comment:4 Changed 3 years ago by Tim Graham

Patch needs improvement: unset

Thanks for reviewing. Squashing commits can be done easily enough when merging so you don't need to check "Patch needs improvement" for that.

comment:5 in reply to:  3 Changed 3 years ago by Josh Schneier

Replying to Tim Martin:

The change looks good to me, but it should be squashed into a single commit.

In this case I intentionally did not rebase down because of the additional fix/patch I added. I'm not sure if it's necessary and wanted to leave the history for the reviewer to help with that determination.

Thanks for the review!

comment:6 Changed 3 years ago by Tim Martin

Triage Stage: AcceptedReady for checkin

I think the non_string unit test makes it pretty clear why you reworked it the way you did. At least, I was able to follow without looking through all the patches.

comment:7 Changed 3 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 48c394a6:

Fixed #28555 -- Made CharField convert whitespace-only values to the empty_value when strip is enabled.

comment:8 Changed 3 years ago by Jon Dufresne

I was wondering if this fix could be eligible for a backport to 1.11.x. It fixes a bug in the new features of:

https://docs.djangoproject.com/en/dev/releases/1.11/#forms

The new CharField.empty_value attribute allows specifying the Python value to use to represent “empty”.

https://docs.djangoproject.com/en/dev/releases/1.11/#miscellaneous

In model forms, CharField with null=True now saves NULL for blank values instead of empty strings.

The documentation says the following about fixing bugs in newly introduced features:

Major functionality bugs in new features of the latest stable release.

comment:9 Changed 3 years ago by Tim Graham <timograham@…>

In 1d1a56c:

[1.11.x] Fixed #28555 -- Made CharField convert whitespace-only values to the empty_value when strip is enabled.

Backport of 48c394a6fc2594891f766293afec8f86d63e1015 from master

comment:10 Changed 3 years ago by Tim Graham <timograham@…>

In 3d2c3905:

Refs #28555 -- Forwardported 1.11.6 release note.

Note: See TracTickets for help on using tickets.
Back to Top