Opened 12 months ago

Closed 9 months ago

Last modified 4 months ago

#23405 closed Bug (fixed)

Blank-able CharFields require default=''

Reported by: yuvadm Owned by: coldmind
Component: Migrations Version: 1.7
Severity: Normal Keywords:
Cc: charettes, coldmind Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

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.

Attachments (1)

ticket_23405_for_1_7.patch (4.8 KB) - added by coldmind 9 months ago.
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?

Download all attachments as: .zip

Change History (29)

comment:1 Changed 12 months ago by bmispelon

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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?

comment:2 follow-up: Changed 12 months ago by 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.

comment:3 in reply to: ↑ 2 ; follow-up: Changed 12 months ago by charettes

  • Cc charettes added

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.

comment:4 in reply to: ↑ 3 Changed 12 months ago by yuvadm

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?

Last edited 12 months ago by yuvadm (previous) (diff)

comment:5 follow-up: Changed 12 months ago by andrewgodwin

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.

comment:6 in reply to: ↑ 5 Changed 12 months ago by yuvadm

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?

comment:7 Changed 12 months ago by yuvadm

  • Owner changed from nobody to yuvadm
  • Status changed from new to assigned

comment:8 Changed 12 months ago by timgraham

  • Triage Stage changed from Unreviewed to Accepted

comment:9 follow-up: Changed 9 months ago by coldmind

@yuvadm, any news about this issue?

comment:10 in reply to: ↑ 9 Changed 9 months ago by yuvadm

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

comment:11 Changed 9 months ago by coldmind

  • Owner changed from yuvadm to coldmind

I'll try to fix that.

comment:12 Changed 9 months ago by berkerpeksag

  • Has patch set

comment:13 Changed 9 months ago by coldmind

<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.

comment:14 Changed 9 months ago by coldmind

<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
* ojii has quit (Quit: Leaving)
<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?
* tonebot has quit (Remote host closed the connection)
* tonebot (~nodebot@ec2-54-161-155-85.compute-1.amazonaws.com) has joined #django-dev
<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
<MarkusH> Thank you for tackling the issue

comment:15 follow-up: Changed 9 months ago by 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 there's data in the table.

comment:16 in reply to: ↑ 15 Changed 9 months ago by coldmind

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?

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

comment:17 Changed 9 months ago by coldmind

  • Cc coldmind added

comment:18 follow-up: Changed 9 months ago by 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.

comment:19 in reply to: ↑ 18 Changed 9 months ago by coldmind

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)

comment:20 Changed 9 months ago by timgraham

  • Triage Stage changed from Accepted to Ready for checkin

comment:21 Changed 9 months ago by timgraham

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

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

Changed 9 months ago by coldmind

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?

comment:22 Changed 9 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In d8f3b86a7691c8aa0ec8f5a064ad4c3218250fed:

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

A default is no longer required.

comment:23 Changed 9 months ago by 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

comment:25 Changed 4 months ago by coldmind

@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.

UPD:
I can't run this sql in shell too, maybe it is a but in sqlmigrate command.
Migration works though.

Last edited 4 months ago by coldmind (previous) (diff)

comment:26 Changed 4 months ago by dukebody

@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!

comment:27 Changed 4 months ago by coldmind

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.

comment:28 Changed 4 months ago by coldmind

I opened new ticket for it - https://code.djangoproject.com/ticket/24803

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