Opened 17 years ago

Closed 7 years ago

Last modified 7 years ago

#4136 closed New feature (fixed)

Save empty, nullable CharField's as null rather than as an empty string

Reported by: David Cramer <dcramer@…> Owned by: Tim Graham <timograham@…>
Component: Forms Version: dev
Severity: Normal Keywords:
Cc: listuser@…, sean@…, ironfroggy@…, Ashutosh Dwivedi, m.r.sopacua@…, riccardo.magliocchetti@…, cmawebsite@…, jon.dufresne@… 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

When Django sends an update to MySQL for a NULL field it does not insert it as NULL (when you are in the admin).

e.g.
You have an openid_key field, which is the url of the users openid url. This url has to be unique and should be marked as such for db integrity.

You go into the admin and edit a user, they have no openid_key. You save the user. All is fine.

You go into the admin and edit a different user, they have no openid_key. You save the user. SQL throws a fit about the key needs to be unique.

Django tried to insert "" instead of NULL for this field.

Attachments (2)

iss_4136.diff (3.2 KB) - added by zefciu 12 years ago.
4136_NullableWidget.py (2.0 KB) - added by frostnzcr4 12 years ago.
Working NullableWidget

Download all attachments as: .zip

Change History (47)

comment:1 Changed 17 years ago by Ramiro Morales

What options are you using for the openid_key field?, is it a CharField? a ForeignKey?. Please post [a fragment with relevant parts to reproduce the problem of] your models.py file to the django-users mailing list.

comment:2 Changed 17 years ago by David Cramer <dcramer@…>

It's a URLField (as it's a URL), but it shouldnt matter what kind of field it is.

comment:3 Changed 17 years ago by James Bennett

Hm. The problem here is that fields which -- in Python -- reduce to a string value have null input stored as empty strings instead of as NULL (this is mentioned in the model docs), and with good reason; as the docs point out, allowing both NULL and an empty string creates ambiguity. I'm not sure what the best solution is here in the face of a nullable but unique field which holds a string.

comment:4 Changed 17 years ago by David Cramer <dcramer@…>

Being that this is supposed to be on top of a DB layer I think a string should be treated as NULL when it's blank. At least if the field is set to null=True. In reality a string being is the same as being empty. I have no problems with this except for the admin interface.

comment:5 Changed 17 years ago by Malcolm Tredinnick

Needs documentation: set
Triage Stage: UnreviewedAccepted

We are not going to interpret empty strings as NULL. They are not the same thing, in general.

I think this is something that needs to be taken care of in the model's save() method or as the data is cleaned (converted from form to Python objects). Not closing yet, because we can probably come up with some documentation and best practices here, but we aren't going to start automatically storing NULLs.

comment:6 Changed 16 years ago by nix

Cc: listuser@… added

Malcolm

I have to respectfully disagree with you on this issue. I have the following in a model:

mac_address = models.CharField(maxlength=12, blank=True, null=True, unique=True)

This causes the admin interface to throw database constraint errors whenever there is more than one empty mac address in the table. If the admin interface correctly left these as NULL then everything would work properly. If you can tell me a better way to get the behaviour I need then I am open to suggestions (Field should be blank or unique) as I use this for a large number of fields and the fact that it works properly on everything except strings is very frustrating.

comment:7 Changed 16 years ago by Chris Beaven

Component: Database wrapperAdmin interface

This seems more of an admin problem than a db wrapper problem. The db handles it fine (as far as I know) if you actually set the value to be None, it is just the fact that the admin can't tell (or accurately display) the difference between a blank string and null.

I think newforms-admin should have a special NullCharField widget for db CharFields which have null=True with two radio options, the first with a label of "None" and the second with the text box. This way, you can make the differentiation between a null and blank string.

comment:8 Changed 14 years ago by Adam Nelson

I tried this model definition:

class Tester(models.Model):

mac_address = models.CharField(max_length=12, blank=True, null=True, default=None, unique=True)

The admin did indeed give a duplicate key error when trying to add a blank value twice. With a default of None (Null), I think the admin should submit None for a blank field, and therefore no key constraint should hit (since None != None or Null != Null). In the case where default is not set or is not None then I think an empty string is the correct value.

comment:9 Changed 13 years ago by Sean Chittenden

Cc: sean@… added

comment:10 Changed 13 years ago by Łukasz Rekucki

Severity: Normal
Type: Bug

comment:11 Changed 12 years ago by anonymous

Easy pickings: unset
UI/UX: unset

Any news about this? Is this still open?

comment:12 Changed 12 years ago by federico.capoano@…

Version: 0.961.3-rc1

comment:13 Changed 12 years ago by federico.capoano@…

How am I supposed to do in a case like:

ipv4_address = models.IPAddressField(verbose_name=_('ipv4 address'), blank=True, null=True, unique=True, default=None)
ipv6_address = models.GenericIPAddressField(protocol='IPv6', verbose_name=_('ipv6 address'), blank=True, null=True, unique=True, default=None)

No idea on how to solve this.

comment:14 Changed 12 years ago by Calvin Spealman

I think this is more appropriately placed in the Forms component, as it can be a problem with any form, not just those in the admin. At the heart of the issue is a mismatch between the option combinations available to form and model fields, and to solve it, form fields would need to understand the difference between submitting an empty value and no value, which HTTP form encoding does not make straight-forward without adding additional flags.

Given that, I am not sure this is something django *should* be dealing with, if it weren't for its own use of forms in the admin, so any solution should be 1) easy to use with any form, 2) but not required nor adding complexity to their existing behavior and usage, and 3) not part of the admin.

I like the idea of providing an OptionalWidget which can wrap other widgets, render them a checkbox to control if they are actually included, and easiyl used in forms without changing any existing usage or behavior.

comment:15 Changed 12 years ago by Calvin Spealman

Cc: ironfroggy@… added

comment:16 Changed 12 years ago by zefciu

Owner: changed from nobody to zefciu
Status: newassigned

comment:17 Changed 12 years ago by zefciu

Component: contrib.adminForms

comment:18 Changed 12 years ago by zefciu

Easy pickings: set
Has patch: set
Needs tests: set
Owner: changed from zefciu to nobody
Patch needs improvement: set
Status: assignednew

I have created a NullableWidget, which accepts as its first argument another Widget. It renders a checkbox and its subwidget alongside and can correctly decompress and get value from datadict. Also wrote some tests. What is still needed:

  • Enabling None in fields.
  • Writing tests for different subwidgets and fields.
  • JavaScript for disabling the subwidgets on uncheck (?)

Changed 12 years ago by zefciu

Attachment: iss_4136.diff added

Changed 12 years ago by frostnzcr4

Attachment: 4136_NullableWidget.py added

Working NullableWidget

comment:19 Changed 12 years ago by frostnzcr4

I update zefciu patch with redefined _has_changed method and now save not throw exceptions anymore. But new problem is that CharField convert None value to empty string (u"") and saving of another model instance with CharField setted to None throws unique error again. So the root of the evil is in null and blank attributes again, not in unique itself?

Last edited 12 years ago by frostnzcr4 (previous) (diff)

comment:20 Changed 12 years ago by Ashutosh Dwivedi

A somewhat related ticket is https://code.djangoproject.com/ticket/5622

comment:21 Changed 12 years ago by Anssi Kääriäinen

Maybe a new form field init argument is needed: blank_as_none? Better name welcome. I don't see any other way forward. Currently it is impossible for the form layer to know if the given empty value should be treated as "" or None.

Maybe we need a matching model field init argument, too. Setting null=True, blank=True can not be translated to blank_as_none, as this would be backwards incompatible.

EDIT: I do see other ways forward, like the NullableWidget presented here...

Last edited 12 years ago by Anssi Kääriäinen (previous) (diff)

comment:22 Changed 12 years ago by Ashutosh Dwivedi

Cc: Ashutosh Dwivedi added

comment:23 Changed 12 years ago by Ashutosh Dwivedi

Owner: changed from nobody to Ashutosh Dwivedi
Status: newassigned

comment:24 Changed 11 years ago by Melvyn Sopacua

Cc: m.r.sopacua@… added

Just to throw in my 2 cents here: if this is a general form problem the answer is not a special widget with a 'NULL' description as users of a site should not be expected to know the difference, which means one should be able to describe in the field's init method what the description of the radio buttons should be, like (..., provided_value="My cat's name", absent_value="I have no cat").

Also the simplest option is to always insert as NULL if null and unique are True as there are very few real world cases where an empty value is an actual valid option. One could also make this case for non-unique but null fields, however there's no actual distinction between a NULL value and an empty string in this case.

And there may be another solution. Backwards compatibility may be an issue but I think solvable. Instead of the blank field being a boolean, make it an integer flag:

  • blank = models.RESTRICT acts as blank=False now
  • blank = models.SET_EMPTY acts as blank=True now and sets field to the 'empty' value
  • blank = models.SET_DEFAULT sets a blank value to the field's default value
  • blank = models.SET_NULL sets a blank value to the NULL if null=True or raises ValueError if null=False

If RESTRICT and SET_EMPTY are 0 and 1 respectively the setting can be set coerced to int and be backwards compatible. All in all I think the cleanest solution in the long run.

Last edited 11 years ago by Melvyn Sopacua (previous) (diff)

comment:25 Changed 11 years ago by anonymous

I do think that the best way forward seems to be msopacua's solution. Please share your opinion on the same so that it can be taken forward and fixed as soon as possible.

comment:26 Changed 11 years ago by Ashutosh Dwivedi

I do think that the best way forward seems to be msopacua's solution. Please share your opinion on the same so that it can be taken forward and fixed as soon as possible.

comment:27 Changed 11 years ago by Hong Shuning

I agree with msopacua's solution, and suggest to change component from Forms to contrib.admin because programmers should write their own form code to control it. The 'blank' parameter may only be used for admin app.

comment:28 Changed 11 years ago by Carl Meyer

Easy pickings: unset

comment:29 Changed 11 years ago by Ankit Bagaria

I want try to solve this issue can someone pleaseguide me as to where do I start..

comment:30 Changed 11 years ago by Carl Meyer

msopacua's suggestion is incomplete because it only considers the model-field API. Form fields don't even have a "blank" argument (they have "required"), so a different API would have to be proposed for them. Ultimately this is a form-field issue (we only need the option at model-field level to give a hint to modelform generation).

The NullableWidget is a bad general solution for the reason msopacua notes: in most cases end users don't know and shouldn't have to care about the difference between storing null and empty string; this is a concern for the application developer.

At the form field level, I think perhaps the best approach is similar to what akaariai suggested; a new default_if_empty option to forms.CharField, defaulting to the empty string, that defines what is returned from to_python if value in self.empty_values.

At the model-field level, my objection to msopacua's suggestion is that it implicitly applies to all field types (since they all have the blank option), but I think this issue is only relevant for CharField. If there were no backwards-compatibility concerns, I think the right behavior would be to simply automatically pass None to the form field's default_if_empty if null=True. I'm somewhat tempted to just go ahead and do this and note it in the backwards-compat notes, because I think in most cases if null=True it is the desired behavior. If that's too much of a back-compat break, I guess we would need a new option to CharField to enable that behavior.

comment:31 Changed 11 years ago by rm_

Cc: riccardo.magliocchetti@… added

comment:32 in reply to:  30 Changed 11 years ago by Melvyn Sopacua

Replying to carljm:

msopacua's suggestion is incomplete because it only considers the model-field API. Form fields don't even have a "blank" argument (they have "required"), so a different API would have to be proposed for them. Ultimately this is a form-field issue (we only need the option at model-field level to give a hint to modelform generation).

I still think this is a model issue. For one, because the issue exists in translating model data to storage. Secondly, because I can think of a case, where the form would not present an empty string but the model might - like a bad word filter. For me the logical place to put such a thing would be in a model's clean(), but a case can be made to do this at the form level.

At the model-field level, my objection to msopacua's suggestion is that it implicitly applies to all field types (since they all have the blank option), but I think this issue is only relevant for CharField.

True. I don't mind this being a charfield only option, if it gets picked up by custom fields that derive from it. You'd name it differently and only apply it to CharField.lll

comment:33 Changed 9 years ago by Collin Anderson

Cc: cmawebsite@… added
Summary: NULL fields and Unique keysCharField(null=True, blank=True, unique=True)

Seems to me this actually applies to any fields that allow empty strings. It's either the form or the model's responsibility to automatically translate the blank strings into None in this case.

I usually work around the problem by putting this in my model's save() method:

self.myfield = self.myfield or None

Last edited 9 years ago by Collin Anderson (previous) (diff)

comment:34 Changed 9 years ago by anentropic

@collinanderson

Can you elaborate? Simply adding that to the model save() method alone seems to be insufficient because commonly we are using a ModelForm and that calls the instance's validate_unique method, which raises ValidationError before the save method is reached...

comment:35 Changed 9 years ago by Collin Anderson

You probably have one entry that has an empty string instead of null. Change that to null/none and that should fix the uniqueness problem. If you use my code in your save method, you just need to call save on that object.

comment:36 Changed 8 years ago by Claude Paroz

Needs documentation: unset
Needs tests: unset
Owner: Ashutosh Dwivedi deleted
Patch needs improvement: unset
Status: assignednew

comment:37 Changed 8 years ago by Tim Graham

Patch needs improvement: set
Summary: CharField(null=True, blank=True, unique=True)Add a way to save null CharField's as null rather than an empty string
Type: BugNew feature
Version: 1.3-rcmaster

PR needs improvement for Oracle.

comment:38 Changed 8 years ago by Jon Dufresne

Cc: jon.dufresne@… added
Patch needs improvement: unset

I have updated the change to check the Oracle feature flag. All feedback welcome. Thanks.

comment:39 Changed 8 years ago by Tim Graham

Patch needs improvement: set

Comments for improvement are on the PR.

comment:40 Changed 8 years ago by Jon Dufresne

Patch needs improvement: unset

Addressed comments. All additional feedback welcome.

comment:41 Changed 7 years ago by Tim Graham

Summary: Add a way to save null CharField's as null rather than an empty stringSave empty, nullable CharField's as null rather than as an empty string
Triage Stage: AcceptedReady for checkin

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

Owner: set to Tim Graham <timograham@…>
Resolution: fixed
Status: newclosed

In 267dc4ad:

Fixed #4136 -- Made ModelForm save empty values for nullable CharFields as NULL.

Previously, empty values were saved as strings.

comment:43 Changed 7 years ago by Matt Braymer-Hayes

Resolution: fixed
Status: closednew

Hi folks, question: Do these changes also impact models.CharField subclasses (CommaSeparatedIntegerField, EmailField, SlugField, and URLField) and forms.CharField subclasses (RegexField, EmailField, URLField, GenericIPAddressField, SlugField, and UUIDField)? Based on my review of the code they do, though I may be missing something. If they are, the docstrings and documentation for each of the forms.CharField subclasses should be updated. I'm happy to make a PR, if this is the case.

Last edited 7 years ago by Matt Braymer-Hayes (previous) (diff)

comment:44 Changed 7 years ago by Simon Charette

Resolution: fixed
Status: newclosed

Hi Matt,

Please submit a new ticket suggesting your documentation adjustments instead of reopening this one to avoid notifying the large number of people that CCed over the past 10 years.

Linking to this ticket by number (e.g. #4136) should be enough to provide context in your report.

Thanks.

comment:45 Changed 7 years ago by Tim Graham

I agree that reopening this ticket isn't appropriate. A new ticket may not be required -- a PR referencing this ticket would probably be fine, although I don't think we generally repeat documentation for a superclass in a subclass.

Follow up ticket is #28009.

Last edited 7 years ago by Tim Graham (previous) (diff)
Note: See TracTickets for help on using tickets.
Back to Top