[Django] #23405: Blank-able CharFields require default=''
Django
----------------------------+--------------------
Reporter: yuvadm | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------+--------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23405>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: yuvadm | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by bmispelon):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/23405#comment:1>
Django
Reporter: yuvadm | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23405#comment:2>
Django
Reporter: yuvadm | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* cc: charettes (added)
Comment:
Replying to [comment:2 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23405#comment:3>
Django
Reporter: yuvadm | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by yuvadm):
Replying to [comment:3 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.
This happens even for new fields being added. I would argue that any
existing field change is a data migration anyway, and requires manual
intervention.
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/23405#comment:4>
Django
Reporter: yuvadm | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23405#comment:5>
Django
Reporter: yuvadm | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by yuvadm):
Replying to [comment:5 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?
--
Ticket URL: <https://code.djangoproject.com/ticket/23405#comment:6>
Django
Reporter: yuvadm | Owner: yuvadm
Type: Bug | Status: assigned
Component: Migrations | Version: 1.7
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by yuvadm):
* status: new => assigned
* owner: nobody => yuvadm
--
Ticket URL: <https://code.djangoproject.com/ticket/23405#comment:7>
Django
Reporter: yuvadm | Owner: yuvadm
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/23405#comment:8>
Django
Reporter: yuvadm | Owner: yuvadm
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by coldmind):
@yuvadm, any news about this issue?
--
Ticket URL: <https://code.djangoproject.com/ticket/23405#comment:9>
Django
Reporter: yuvadm | Owner: yuvadm
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by yuvadm):
Replying to [comment:9 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
--
Ticket URL: <https://code.djangoproject.com/ticket/23405#comment:10>
Django
Reporter: yuvadm | Owner: coldmind
Type: Bug | Status: assigned
Component: Migrations | Version: 1.7
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by coldmind):
* owner: yuvadm => coldmind
Comment:
I'll try to fix that.
--
Ticket URL: <https://code.djangoproject.com/ticket/23405#comment:11>
Django
Reporter: yuvadm | Owner: coldmind
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by berkerpeksag):
* has_patch: 0 => 1
Comment:
PR is here: https://github.com/django/django/pull/3700
--
Ticket URL: <https://code.djangoproject.com/ticket/23405#comment:12>
Django
Reporter: yuvadm | Owner: coldmind
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23405#comment:13>
Django
Reporter: yuvadm | Owner: coldmind
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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 (~nod...@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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23405#comment:14>
Django
Reporter: yuvadm | Owner: coldmind
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23405#comment:15>
Django
Reporter: yuvadm | Owner: coldmind
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by coldmind):
Replying to [comment:15 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" ?
--
Ticket URL: <https://code.djangoproject.com/ticket/23405#comment:16>
Django
Reporter: yuvadm | Owner: coldmind
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by coldmind):
* cc: coldmind (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/23405#comment:17>
Django
Reporter: yuvadm | Owner: coldmind
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23405#comment:18>
Django
Reporter: yuvadm | Owner: coldmind
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by coldmind):
Replying to [comment:18 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)
--
Ticket URL: <https://code.djangoproject.com/ticket/23405#comment:19>
Django
Reporter: yuvadm | Owner: coldmind
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/23405#comment:20>
Django
Reporter: yuvadm | Owner: coldmind
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
* stage: Ready for checkin => Accepted
Comment:
On testing this, I found a case where the generated migration results in a
crash; see PR for details.
--
Ticket URL: <https://code.djangoproject.com/ticket/23405#comment:21>
Django
Reporter: yuvadm | Owner: coldmind
Component: Migrations | Version: 1.7
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"d8f3b86a7691c8aa0ec8f5a064ad4c3218250fed"]:
{{{
#!CommitTicketReference repository=""
revision="d8f3b86a7691c8aa0ec8f5a064ad4c3218250fed"
Fixed #23405 -- Fixed makemigrations prompt when adding Text/CharField.
A default is no longer required.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23405#comment:22>
Django
Reporter: yuvadm | Owner: coldmind
Type: Bug | Status: closed
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by Tim Graham <timograham@…>):
In [changeset:"fdf4dc6cea0cdfea061f2d72a368adf6a8d24157"]:
{{{
#!CommitTicketReference repository=""
revision="fdf4dc6cea0cdfea061f2d72a368adf6a8d24157"
[1.7.x] Fixed #23405 -- Fixed makemigrations prompt when adding
Text/CharField.
A default is no longer required.
Backport of d8f3b86a7691c8aa0ec8f5a064ad4c3218250fed from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23405#comment:23>
Django
Reporter: yuvadm | Owner: coldmind
Type: Bug | Status: closed
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by dukebody):
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
--
Ticket URL: <https://code.djangoproject.com/ticket/23405#comment:24>
Django
Reporter: yuvadm | Owner: coldmind
Type: Bug | Status: closed
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23405#comment:25>
Django
----------------------------+------------------------------------
Reporter: yuvadm | Owner: coldmind
Type: Bug | Status: closed
Component: Migrations | Version: 1.7
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
----------------------------+------------------------------------
Comment (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!
--
Ticket URL: <https://code.djangoproject.com/ticket/23405#comment:26>
Django
----------------------------+------------------------------------
Reporter: yuvadm | Owner: coldmind
Type: Bug | Status: closed
Component: Migrations | Version: 1.7
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
----------------------------+------------------------------------
Comment (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`.
--
Ticket URL: <https://code.djangoproject.com/ticket/23405#comment:27>
Django
----------------------------+------------------------------------
Reporter: yuvadm | Owner: coldmind
Type: Bug | Status: closed
Component: Migrations | Version: 1.7
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
----------------------------+------------------------------------
Comment (by coldmind):
I opened new ticket for it - https://code.djangoproject.com/ticket/24803
--
Ticket URL: <https://code.djangoproject.com/ticket/23405#comment:28>