[Django] #36167: Default value in migrations allows None/NULL

[Django]

#36167: Default value in migrations allows None/NULL
-----------------------------------------+--------------------------
Reporter: ivasilyev1 | Owner: (none)
Type: Uncategorized | Status: assigned
Component: Uncategorized | Version: 5.1
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+--------------------------
When adding/altering a non-null model field the prompt for default value
allows user input of string `"None"`. Django then creates a migration
with `default_value=None` and everything seems ok.

If there is existing data in the DB and the generated migration is
attempted to be applied, a runtime constraint error will be thrown.

Simple proof:

- Create test model with `null=True` field

{{{
class TestModel(models.Model):
test_field = models.TextField(null=True, help_text='Test field')
}}}

and `makemigrations` to create initial migration

{{{
class Migration(migrations.Migration):
initial = True
dependencies = [
]
operations = [
migrations.CreateModel(
name='TestModel',
fields=[
('id', models.BigAutoField(auto_created=True,
primary_key=True, serialize=False, verbose_name='ID')),
('test_field', models.TextField(help_text='Test field',
null=True)),
],
),
]
}}}

- Create any arbitrary dummy record

{{{
>>> from testapp.models import *
>>> TestModel().save()
>>> exit()
}}}

- Switch the field to `null=False` and `makemigrations` again, selecting
the default value option and entering `None`

{{{
$ python3 manage.py makemigrations
It is impossible to change a nullable field 'test_field' on testmodel to
non-nullable without providing a default. This is because 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 with a
null value for this column)
2) Ignore for now. Existing rows that contain NULL values will have to be
handled manually, for example with a RunPython or RunSQL operation.
3) Quit and manually define a default value in models.py.
Select an option: 1
Please enter the default value as valid Python.
The datetime and django.utils.timezone modules are available, so it is
possible to provide e.g. timezone.now as a value.
Type 'exit' to exit this prompt
>>> None
Migrations for 'testapp':
testapp/migrations/0002_alter_testmodel_test_field.py
~ Alter field test_field on testmodel
}}}

which creates the following migration without error

{{{
operations = [
migrations.AlterField(
model_name="testmodel",
name="test_field",
field=models.TextField(default=None, help_text="Test field"),
preserve_default=False,
),
]
}}}

- Apply the new migration with `migrate`

{{{
python3 manage.py migrate
Operations to perform:
Apply all migrations: admin, auth, contenttypes, sessions, testapp
Running migrations:
Applying testapp.0002_alter_testmodel_test_field...Traceback (most
recent call last):
...
django.db.utils.IntegrityError: column "test_field" of relation
"testapp_testmodel" contains null values
}}}


A bit of an edge case because if there is no data present in the table it
seems to work fine. But the combination of `default=None` doesn't really
make sense to be allowed in the first place if the goal is to prevent NULL
values.

The default value prompter should probably not allow this input
--
Ticket URL: <https://code.djangoproject.com/ticket/36167>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36167: Default value in migrations allows None/NULL

-------------------------------+--------------------------------------
Reporter: ivasilyev1 | Owner: ivasilyev1

Type: Uncategorized | Status: assigned
Component: Uncategorized | Version: 5.1

Severity: Normal | Resolution:

Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0

Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------+--------------------------------------
Changes (by ivasilyev1):

* easy: 0 => 1
* needs_tests: 0 => 1
* owner: (none) => ivasilyev1

--
Ticket URL: <https://code.djangoproject.com/ticket/36167#comment:1>

[Django]

#36167: Default value in migrations allows None/NULL
-------------------------------+--------------------------------------
Reporter: ivasilyev1 | Owner: ivasilyev1
Type: Uncategorized | Status: assigned
Component: Uncategorized | Version: 5.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------+--------------------------------------

Comment (by ivasilyev1):

PR: https://github.com/django/django/pull/19139
--
Ticket URL: <https://code.djangoproject.com/ticket/36167#comment:2>

[Django]

#36167: Default value in migrations allows None/NULL
-------------------------------+--------------------------------------
Reporter: ivasilyev1 | Owner: ivasilyev1

Type: Uncategorized | Status: closed
Component: Uncategorized | Version: 5.1
Severity: Normal | Resolution: wontfix

Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------+--------------------------------------

Changes (by Simon Charette):

* resolution: => wontfix
* status: assigned => closed

Comment:

> But the combination of default=None doesn't really make sense to be
allowed in the first place if the goal is to prevent NULL values.

I understand where you are coming from but I'm not sure we should venture
down the path of allowing or disallowing developer provided values.

We allow string to be specified for `IntegerField`, strings longer than
the specified `max_length` of a `CharField`, negative integer for
`PositiveIntegerField`, and other values that are enforced at the database
level through a constraint just like `NOT NULL` is.

In other words, I don't think we should special case `None` for
`null=False` fields here. We should either perform model field validation
on the provided input as
[https://docs.djangoproject.com/en/5.1/internals/contributing/bugs-and-
features/#requesting-features a new feature request discussed on the
forum] or keep things as they are. Given we decided not to check
`Field.default` at all when defined by the developer (see
ticket:25417#comment:26) I don't think we should get in the way of
developers providing potentially bogus values through `makemigrations`
either.

Closing as ''wont-fix'' pending further discussions.
--
Ticket URL: <https://code.djangoproject.com/ticket/36167#comment:3>

[Django]

#36167: Default value in migrations allows None/NULL
-------------------------------+--------------------------------------
Reporter: ivasilyev1 | Owner: ivasilyev1
Type: Uncategorized | Status: closed
Component: Uncategorized | Version: 5.1
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------+--------------------------------------

Comment (by ivasilyev1):

Replying to [comment:3 Simon Charette]:

> > But the combination of default=None doesn't really make sense to be
allowed in the first place if the goal is to prevent NULL values.
>
> I understand where you are coming from but I'm not sure we should
venture down the path of allowing or disallowing developer provided
values.
>
> We allow string to be specified for `IntegerField`, strings longer than
the specified `max_length` of a `CharField`, negative integer for
`PositiveIntegerField`, and other values that are enforced at the database
level through a constraint just like `NOT NULL` is.
>
> In other words, I don't think we should special case `None` for
`null=False` fields here. We should either perform model field validation
on the provided input as
[https://docs.djangoproject.com/en/5.1/internals/contributing/bugs-and-
features/#requesting-features a new feature request discussed on the
forum] or keep things as they are. Given we decided not to check
`Field.default` at all when defined by the developer (see
ticket:25417#comment:26) I don't think we should get in the way of
developers providing potentially bogus values through `makemigrations`
either.
>
> Closing as ''wont-fix'' pending further discussions.

Ok, understand. Thanks for the background and some more context
--
Ticket URL: <https://code.djangoproject.com/ticket/36167#comment:4>