Opened 10 years ago

Closed 10 years ago

Last modified 8 years ago

#23405 closed Bug (fixed)

Blank-able CharFields require default=''

Reported by: Yuval Adam Owned by: Andriy Sokolovskiy
Component: Migrations Version: 1.7
Severity: Normal Keywords:
Cc: Simon Charette, Andriy Sokolovskiy 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 Andriy Sokolovskiy 10 years 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 by Baptiste Mispelon, 10 years ago

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 by Yuval Adam, 10 years ago

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.

in reply to:  2 ; comment:3 by Simon Charette, 10 years ago

Cc: Simon Charette 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.

in reply to:  3 comment:4 by Yuval Adam, 10 years ago

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 10 years ago by Yuval Adam (previous) (diff)

comment:5 by Andrew Godwin, 10 years ago

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.

in reply to:  5 comment:6 by Yuval Adam, 10 years ago

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 by Yuval Adam, 10 years ago

Owner: changed from nobody to Yuval Adam
Status: newassigned

comment:8 by Tim Graham, 10 years ago

Triage Stage: UnreviewedAccepted

comment:9 by Andriy Sokolovskiy, 10 years ago

@yuvadm, any news about this issue?

in reply to:  9 comment:10 by Yuval Adam, 10 years ago

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 by Andriy Sokolovskiy, 10 years ago

Owner: changed from Yuval Adam to Andriy Sokolovskiy

I'll try to fix that.

comment:12 by Berker Peksag, 10 years ago

Has patch: set

comment:13 by Andriy Sokolovskiy, 10 years ago

<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 by Andriy Sokolovskiy, 10 years ago

<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
Last edited 8 years ago by Tim Graham (previous) (diff)

comment:15 by Andrew Godwin, 10 years ago

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.

in reply to:  15 comment:16 by Andriy Sokolovskiy, 10 years ago

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 10 years ago by Andriy Sokolovskiy (previous) (diff)

comment:17 by Andriy Sokolovskiy, 10 years ago

Cc: Andriy Sokolovskiy added

comment:18 by Andrew Godwin, 10 years ago

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.

in reply to:  18 comment:19 by Andriy Sokolovskiy, 10 years ago

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 by Tim Graham, 10 years ago

Triage Stage: AcceptedReady for checkin

comment:21 by Tim Graham, 10 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

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

by Andriy Sokolovskiy, 10 years ago

Attachment: ticket_23405_for_1_7.patch added

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 by Tim Graham <timograham@…>, 10 years ago

Resolution: fixed
Status: assignedclosed

In d8f3b86a7691c8aa0ec8f5a064ad4c3218250fed:

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

A default is no longer required.

comment:23 by Tim Graham <timograham@…>, 10 years ago

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 by Andriy Sokolovskiy, 9 years ago

@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 9 years ago by Andriy Sokolovskiy (previous) (diff)

comment:26 by Israel Saeta Pérez, 9 years ago

@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 by Andriy Sokolovskiy, 9 years ago

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 by Andriy Sokolovskiy, 9 years ago

I opened new ticket for it - #24803.

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