#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)
Change History (29)
comment:1 by , 10 years ago
follow-up: 3 comment:2 by , 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.
follow-up: 4 comment:3 by , 10 years ago
Cc: | 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 by , 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 explicitdefault
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?
follow-up: 6 comment:5 by , 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.
comment:6 by , 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 , 10 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:8 by , 10 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:10 by , 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:13 by , 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 , 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
follow-up: 16 comment:15 by , 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.
comment:16 by , 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 newneeds_default()
method, which returns if the field can happily live without a default (so, for example, aCharField
would returnFalse
, and the newBooleanField
would returnTrue
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 hasunique
set on it, as doing this when you're adding aNOT 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.
needs_default()
should return value according to field attributes? For example, forTextField
, 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?
- If 1 question is right, the default value for
needs_default
should beTrue
? - Do we need override
needs_default
for all fields, or only for "special" ? - Where the default value for each field will be taken? (
deconstruct
of field is not the right place, I described in PR why)
comment:17 by , 10 years ago
Cc: | added |
---|
follow-up: 19 comment:18 by , 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.
comment:19 by , 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 , 10 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
comment:21 by , 10 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Ready for checkin → Accepted |
On testing this, I found a case where the generated migration results in a crash; see PR for details.
by , 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 , 10 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
comment:24 by , 9 years ago
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
comment:25 by , 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.
comment:26 by , 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 , 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 , 9 years ago
I opened new ticket for it - https://code.djangoproject.com/ticket/24803
Hi,
In this case, the migrations framework provides you with two choices:
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?