Blank-able CharFields require default=''

[Yuval Adam]

There's a weird behavior in Django 1.7 migrations as opposed to 1.6 + South.

Consider a simple blank-able CharField:

comment = models.CharField(max_length=20, blank=True)

In Django 1.6 + South, adding a migration to add this field would auto-generate default='' in the migration. In Django 1.7 the migration now throws: "You are trying to add a non-nullable field 'test' to question without a default".

My understanding is that this is the proper convention for blank-able fields, and in that case the migrations need to support it properly.

[Baptiste Mispelon]

Hi,

In this case, the migrations framework provides you with two choices:

$ python manage.py makemigrations
You are trying to add a non-nullable field 'bar' to foo without a default;
we can't do that (the database needs something to populate existing rows).
Please select a fix:
 1) Provide a one-off default now (will be set on all existing rows)
 2) Quit, and let me add a default in models.py
Select an option: 

It seems to me like a better approach than guessing default='' (explicit being better than implicit and all).

Is that not sufficient for your use-case?

[Yuval Adam]

Of course I can add '' as the default value. It just kind of goes against the usual convention that CharField should never be null and always use '' as a default for null/blank values. It also goes against what South would do, that's the core of my confusion.

[Simon Charette]

Replying to yuvadm:

Of course I can add '' as the default value. It just kind of goes against the usual convention that CharField should never be null and always use '' as a default for null/blank values. It also goes against what South would do, that's the core of my confusion.

I'd advocate the new behavior makes more sense. If I add a status = models.CharField(max_length=20, blank=True) field I might not want the existing rows of the altered table to be equals to '' but a totally different value (e.g. 'active') even if it's not an explicit default value used by the application.

I think Django should continue to prompt the user instead of guessing like South did.

[Yuval Adam]

Replying to charettes:

I might not want the existing rows of the altered table to be equals to '' but a totally different value (e.g. 'active') even if it's not an explicit default value used by the application.

I think Django should continue to prompt the user instead of guessing like South did.

You're talking about having one default for new records, and another for existing records. That's clearly a data migration.

In any case, my complaint pertains to new fields being introduced, and in which case default='' is alread assumed by Django in any other aspect. So why not in this aspect as well?

[Andrew Godwin]

The original author is correct, CharFields are unique in their behaviour that the empty value for new NOT NULL instances is assumed to "", much like BooleanFields used to be False (but of course, that's the database default anyway).

We should just modify the SchemaEditor to use "" as a default value if the field is stringable and NOT NULL. I'm pretty sure most code to do this already exists.

[Yuval Adam]

Replying to andrewgodwin:

We should just modify the SchemaEditor to use "" as a default value if the field is stringable and NOT NULL. I'm pretty sure most code to do this already exists.

Cool. Would you mind if I take a stab at this and try to patch this myself?

[Andriy Sokolovskiy]

@yuvadm, any news about this issue?

[Yuval Adam]

Replying to coldmind:

@yuvadm, any news about this issue?

Yeah, sorry I haven't been able to deliver a patch. If anyone with more knowledge of the SchemaEditor than me wants to take a shot at this, please do that by all means.

Here's a short IRC discussion I had with Andrew about how to fix this, hope it will be of use: ​https://gist.github.com/yuvadm/08227e1a8bd0d4e6cdf9

[Andriy Sokolovskiy]

I'll try to fix that.

[Berker Peksag]

PR is here: ​https://github.com/django/django/pull/3700

[Andriy Sokolovskiy]

<truecoldmind> MarkusH, this behavior is in conflict in South behavior and can be annoying. What if I need to add few dozens of textfields, I will be forced to type empty string for each of this field
<MarkusH> Django's migrations don't behave like south in many ways.
<MarkusH> be explicit and define a default on the field in the model
<MarkusH> I don't think this should be handled by the migration framework.
<mYk> MarkusH: did you see comment 5 L
<mYk> s/L/?/
<truecoldmind> MarkusH, for now I'm also doubt if it check should be in autodetector. But, it is the known fact that default value for textfield and charfield is empty string, and I think it should be used if user didn't specified other default value. But where this check should be placed?
<MarkusH> mYk: yes
<MarkusH> mYk: What if I have a custom field that inherits from neither Char nor TextField but has the same internal type as CharField?
<truecoldmind> I asked you a question in a pullrequest - can we check for internal type instead of class instance check?
<MarkusH> Checking for internal type shoudn't be done outside of backends, imo
<truecoldmind> MarkusH, Seems reasonable. But what check in autodetecor should be placed? We will need one
<MarkusH> gimme a sec. trying something out
<MarkusH> truecoldmind: https://dpaste.de/GmrV
<MarkusH> https://dpaste.de/4i0j
<MarkusH> maybe even skip the "self.blank"
<MarkusH> but I think that's a better way to go
<MarkusH> You'd need to figure out though what the implications of that are
<MarkusH> changes to internal Django migrations
<truecoldmind> MarkusH, looks good, but what about autodetector? 
<MarkusH> no change required
<truecoldmind> MarkusH, `field.has_default()`, of course.
<truecoldmind> What do you mean with " implications " ?
<MarkusH> yah
<MarkusH> What happens to projects that upgrade? Does this generate a new migration file?
<MarkusH> Are there situations that might break?
<truecoldmind> MarkusH, thank you, I'll do some research. I will post that chat log as comment to pull request.

[Andriy Sokolovskiy]

<truecoldmind> MarkusH, consider the field models.CharField(max_length=100, null=True, blank=True). In that case default after deconstruct will be empty string, this is not correct.
<truecoldmind>  The check can be "if self.blank and not self.null and self.default is NOT_PROVIDED"
<MarkusH> truecoldmind: what has blank to do with database default anyway?
<MarkusH> but I agree, you need to check for null
<truecoldmind> I'm thinking due to https://docs.djangoproject.com/en/dev/ref/models/fields/#null. Docs recommends to not use `null=True` for char and text fields. It may not be obvious that `blank=True` without `null` will give empty string, but, we should definitely not set empty string as default if neither `blank` nor the `null` was specified
<MarkusH> blank is only used for form validation, iirc
<truecoldmind> Yup. But, if no `blank` and `null` specified, user must specify default by himself. We are now returned to South behavior (it sets empty string if only `blank` was specified), but I don't know how to make thing to be obvious
<MarkusH> truecoldmind: which turns us back to one of my previous questions: Django's migrations are not equal to South's. Do we want to have the same behavior here?
<truecoldmind> MarkusH, it will be good (reasons are explained in https://code.djangoproject.com/ticket/23405#comment:5). Due to https://docs.djangoproject.com/en/dev/ref/models/fields/#blank,  `blank` is not necessarily associated with forms
<truecoldmind> So I think that check in deconstuct "if self.blank and not self.null and self.default is NOT_PROVIDED" will be right
<MarkusH> truecoldmind: gnaah, sorry. Missed half the comment when reading if before.
<MarkusH> not self.has_default() instead, right?
<MarkusH> but yes, that should work
<truecoldmind> MarkusH, okay, thanks for conversation. I will do more research about how changes in deconstruct will affect migrations and update PR later

[Andrew Godwin]

Just to put some input in on this - the weird quirk of defaulting to empty string is a feature of the text fields themselves, and it's not just text fields (for example, one should argue that IntegerFields with blank=True, null=False should have a default value of 0 rather than prompting).

Thus, I think it needs to be addressed on the fields themselves. I doubt we can change what has_default() returns, but there could be an argument made for adding a new needs_default() method, which returns if the field can happily live without a default (so, for example, a CharField would return False, and the new BooleanField would return True as it needs an explicit default, though that would get caught in validation).

Then, autodetector can be modified to also check for needs_default() = True. We should also probably upgrade it and warn if it has unique set on it, as doing this when you're adding a NOT NULL column is usually a bad idea if there's data in the table.

[Andriy Sokolovskiy]

Replying to andrewgodwin:

Just to put some input in on this - the weird quirk of defaulting to empty string is a feature of the text fields themselves, and it's not just text fields (for example, one should argue that IntegerFields with blank=True, null=False should have a default value of 0 rather than prompting).

Thus, I think it needs to be addressed on the fields themselves. I doubt we can change what has_default() returns, but there could be an argument made for adding a new needs_default() method, which returns if the field can happily live without a default (so, for example, a CharField would return False, and the new BooleanField would return True as it needs an explicit default, though that would get caught in validation).

Then, autodetector can be modified to also check for needs_default() = True. We should also probably upgrade it and warn if it has unique set on it, as doing this when you're adding a NOT NULL column is usually a bad idea if tsuch checkshere's data in the table.

Thanks, this is a good idea.
Few questions about it.

1. needs_default() should return value according to field attributes? For example, for TextField, it be like

    def needs_default(self):
        if self.blank and not self.null and self.default is NOT_PROVIDED:
            return False
        return True

, right?

2. If 1 question is right, the default value for needs_default should be True?
3. Do we need override needs_default for all fields, or only for "special" ?
4. Where the default value for each field will be taken? (deconstruct of field is not the right place, I described in PR why)

[Andrew Godwin]

Thinking about this again, needs_default isn't quite enough as you also need to give the database a default value to work with when there's none provided, and we don't have provision for that; plus, the schema backends aren't clever enough to check for this (currently, they use field.empty_strings_allowed).

Thus, the patch should probably just fix the autodetector to skip fields with empty_strings_allowed declared, which should get the intended result.

[Andriy Sokolovskiy]

Replying to andrewgodwin:

Thinking about this again, needs_default isn't quite enough as you also need to give the database a default value to work with when there's none provided, and we don't have provision for that; plus, the schema backends aren't clever enough to check for this (currently, they use field.empty_strings_allowed).

Thus, the patch should probably just fix the autodetector to skip fields with empty_strings_allowed declared, which should get the intended result.

I'have updated PR.
But I'm doubt if autodetector should skip field, if the check is only if field.empty_strings_allowed, or if field.blank and field.empty_strings_allowed (to reproduce South behavior in that case)

[Tim Graham]

On testing this, I found a case where the generated migration results in a crash; see PR for details.

[Andriy Sokolovskiy]

Tim Graham: Could you also provide a patch in the comments here that I could apply on top of this one when backporting to 1.7 that replaces the usage of mock since we don't have it available there?

[Tim Graham <timograham@…>]

In d8f3b86a7691c8aa0ec8f5a064ad4c3218250fed:

Fixed #23405 -- Fixed makemigrations prompt when adding Text/CharField.

A default is no longer required.

[Tim Graham <timograham@…>]

In fdf4dc6cea0cdfea061f2d72a368adf6a8d24157:

[1.7.x] Fixed #23405 -- Fixed makemigrations prompt when adding Text/CharField.

A default is no longer required.

Backport of d8f3b86a7691c8aa0ec8f5a064ad4c3218250fed from master

[Israel Saeta Pérez]

Could this fix have generated a new bug? See ​http://stackoverflow.com/questions/30239490/invalid-migration-sql-when-adding-blank-true-charfield-in-django-1-7-8

[Andriy Sokolovskiy]

@dukeboy I don't think so, if all tests are passing.
Also installed mysql local, SQL of my migration is:

BEGIN;
ALTER TABLE `blankapp_modelb` ADD COLUMN `chr_field` varchar(10) DEFAULT  NOT NULL;
ALTER TABLE `blankapp_modelb` ALTER COLUMN `chr_field` DROP DEFAULT;

COMMIT;

And all is without errors.
What version of mysql are you using? Please give more info about your environment.

[Israel Saeta Pérez]

@coldmind ok, really sounds like it must be a bug in sqlmigrate. I've been looking at the source code and it seems that the default is passed as a parameter in MySQL.

I'll check with 1.8 and open a new bug report if needed. Thanks!

[Andriy Sokolovskiy]

Next SQL works for me

ALTER TABLE `blankapp_modelb` ADD COLUMN `chr_field` varchar(10) DEFAULT '' NOT NULL;

(empty string must be set by default)

1.8 gives wrong results too.
I think it is a bug in sqlmigrate.

[Andriy Sokolovskiy]

I opened new ticket for it - #24803.