Make the autodetector validate the type of one-off default values

[Nikos Katsos]

As the title says Django doesn't check the type of one-off value. Recently I had a field like this

    start_time = models.DateTimeField(editable=False)

1) I gave to the field an one-off value

>>> 0 

2) then the migration file had those operations

    operations = [
        migrations.AddField(
            model_name='x',
            name='outcome',
            field=models.CharField(default='TO', max_length=16),
            preserve_default=False,
        ),
        migrations.AddField(
            model_name='x',
            name='start_time',
            field=models.DateTimeField(default=0 editable=False),
            preserve_default=False,
        ),
    ]

when I found my error i couldn't really do anything
I changed

            field=models.DateTimeField(default=0, editable=False),

to

            field=models.DateTimeField(default=timezone.now, editable=False),

so after updating my migration file, I got an error "django.db.utils.OperationalError: (1060, "Duplicate column name "outcome")".
I could neither re-migrate because the operations isn't a bunch of queries in transaction, nor create a new migration.
Only reverting to zero and apply all migrations again worked.

This has to have a workaround.

[Jeff]

If anyone is willing to give me a hand, I am close to a viable patch, see ​commit, however I am currently wrangling with an existing test tests/migrations/test_commands.py - MakeMigrationsTests.test_makemigrations_auto_now_add_interactive. The test is failing like so:

======================================================================
ERROR: test_makemigrations_auto_now_add_interactive (migrations.test_commands.MakeMigrationsTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python3.6/unittest/case.py", line 59, in testPartExecutor
    yield
  File "/usr/lib/python3.6/unittest/case.py", line 605, in run
    testMethod()
  File "/usr/lib/python3.6/unittest/mock.py", line 1179, in patched
    return func(*args, **keywargs)
  File "/home/jy/Django/forks/django-repo/tests/migrations/test_commands.py", line 1300, in test_makemigrations_auto_now_add_interactive
    call_command('makemigrations', 'migrations', interactive=True, stdout=out)
  File "/home/jy/Django/forks/django-repo/django/core/management/__init__.py", line 148, in call_command
    return command.execute(*args, **defaults)
  File "/home/jy/Django/forks/django-repo/django/core/management/base.py", line 354, in execute
    output = self.handle(*args, **options)
  File "/home/jy/Django/forks/django-repo/django/core/management/base.py", line 83, in wrapped
    res = handle_func(*args, **kwargs)
  File "/home/jy/Django/forks/django-repo/django/core/management/commands/makemigrations.py", line 161, in handle
    migration_name=self.migration_name,
  File "/home/jy/Django/forks/django-repo/django/db/migrations/autodetector.py", line 44, in changes
    changes = self._detect_changes(convert_apps, graph)
  File "/home/jy/Django/forks/django-repo/django/db/migrations/autodetector.py", line 183, in _detect_changes
    self.generate_added_fields()
  File "/home/jy/Django/forks/django-repo/django/db/migrations/autodetector.py", line 835, in generate_added_fields
    self._generate_added_field(app_label, model_name, field_name)
  File "/home/jy/Django/forks/django-repo/django/db/migrations/autodetector.py", line 854, in _generate_added_field
    field.default = self.questioner.ask_auto_now_add_addition(field, model_name)
  File "/home/jy/Django/forks/django-repo/django/db/migrations/questioner.py", line 224, in ask_auto_now_add_addition
    return self._ask_default(field_instance, default='timezone.now')
  File "/home/jy/Django/forks/django-repo/django/db/migrations/questioner.py", line 139, in _ask_default
    if field_instance.to_python(code):
  File "/home/jy/Django/forks/django-repo/django/db/models/fields/__init__.py", line 1369, in to_python
    parsed = parse_datetime(value)
  File "/home/jy/Django/forks/django-repo/django/utils/dateparse.py", line 106, in parse_datetime
    match = datetime_re.match(value)
TypeError: expected string or bytes-like object

I have confirmed that at this point the value being passed to to_python is the integer 1, which should indeed be an invalid default/one-off value to pass for a DateTime field.

At the moment what seems to be blocking me, I believe, is figuring out how unittest.mock.patch() works, and how that value is being set for the interactive test:

@mock.patch('builtins.input', return_value='1')
@mock.patch('django.db.migrations.questioner.sys.stdin', mock.MagicMock(encoding=sys.getdefaultencoding()))
def test_makemigrations_auto_now_add_interactive(self, *args):
    """
    makemigrations prompts the user when adding auto_now_add to an existing
    model.
    """
    class Entry(models.Model):
        title = models.CharField(max_length=255)
        creation_date = models.DateTimeField(auto_now_add=True)

        class Meta:
            app_label = 'migrations'

    # Monkeypatch interactive questioner to auto accept
    with mock.patch('django.db.migrations.questioner.sys.stdout', new_callable=io.StringIO) as prompt_stdout:
        out = io.StringIO()
        with self.temporary_migration_module(module='migrations.test_auto_now_add'):
            call_command('makemigrations', 'migrations', interactive=True, stdout=out)
        output = out.getvalue()
        prompt_output = prompt_stdout.getvalue()
        self.assertIn("You can accept the default 'timezone.now' by pressing 'Enter'", prompt_output)
        self.assertIn("Add field creation_date to entry", output)

If anyone could provide some guidance on how to properly work with this test, I'd appreciate it.

[Tim Graham]

The value comes from return_value='1'. That test could be updated to use a valid value, however, it seems that DateTimeField.to_python() (and all to_python() methods) would need to be able to handle arbitrary Python types (and raise ValidationError) -- which may be difficult -- unless there's some broader exception catching in the migrations questioner.