Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#31206 closed New feature (duplicate)

Migration crashes when altering a field to use the db_returning flag and default value returned from database.

Reported by: Tom Kazimiers Owned by: Johannes Maron
Component: Database layer (models, ORM) Version: 3.0
Severity: Normal Keywords:
Cc: Johannes Maron Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I was happy to see PR 9983 merged before Django 3 was released, because it adds the db_returning attribute to the Field classes. This makes it possible to return multiple fields from an INSERT or UPDATE, which is especially useful to use database transaction timestamps (see issue #29444).

It seems to work very well, except for migrations: if I have a field like CreatedField() as shown in tests/queries/models.py (part of the original PR), and create a new migration that sets the new default value on the field (e.g. django.contrib.postgres.functions.TransactionNow), manage.py migrate fails with this error:

  File "./manage.py", line 11, in <module>
    execute_from_command_line(sys.argv)
  File "/home/tom/.virtualenvs/catmaid/lib/python3.6/site-packages/django/core/management/__init__.py", line 401, in execute_from_command_line
    utility.execute()
  File "/home/tom/.virtualenvs/catmaid/lib/python3.6/site-packages/django/core/management/__init__.py", line 395, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/home/tom/.virtualenvs/catmaid/lib/python3.6/site-packages/django/core/management/base.py", line 328, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/home/tom/.virtualenvs/catmaid/lib/python3.6/site-packages/django/core/management/base.py", line 369, in execute
    output = self.handle(*args, **options)
  File "/home/tom/.virtualenvs/catmaid/lib/python3.6/site-packages/django/core/management/base.py", line 83, in wrapped
    res = handle_func(*args, **kwargs)
  File "/home/tom/.virtualenvs/catmaid/lib/python3.6/site-packages/django/core/management/commands/migrate.py", line 233, in handle
    fake_initial=fake_initial,
  File "/home/tom/.virtualenvs/catmaid/lib/python3.6/site-packages/django/db/migrations/executor.py", line 117, in migrate
    state = self._migrate_all_forwards(state, plan, full_plan, fake=fake, fake_initial=fake_initial)
  File "/home/tom/.virtualenvs/catmaid/lib/python3.6/site-packages/django/db/migrations/executor.py", line 147, in _migrate_all_forwards
    state = self.apply_migration(state, migration, fake=fake, fake_initial=fake_initial)
  File "/home/tom/.virtualenvs/catmaid/lib/python3.6/site-packages/django/db/migrations/executor.py", line 245, in apply_migration
    state = migration.apply(state, schema_editor)
  File "/home/tom/.virtualenvs/catmaid/lib/python3.6/site-packages/django/db/migrations/migration.py", line 124, in apply
    operation.database_forwards(self.app_label, schema_editor, old_state, project_state)
  File "/home/tom/.virtualenvs/catmaid/lib/python3.6/site-packages/django/db/migrations/operations/fields.py", line 249, in database_forwards
    schema_editor.alter_field(from_model, from_field, to_field)
  File "/home/tom/.virtualenvs/catmaid/lib/python3.6/site-packages/django/db/backends/base/schema.py", line 565, in alter_field
    old_db_params, new_db_params, strict)
  File "/home/tom/.virtualenvs/catmaid/lib/python3.6/site-packages/django/db/backends/postgresql/schema.py", line 154, in _alter_field
    new_db_params, strict,
  File "/home/tom/.virtualenvs/catmaid/lib/python3.6/site-packages/django/db/backends/base/schema.py", line 679, in _alter_field
    new_default = self.effective_default(new_field)
  File "/home/tom/.virtualenvs/catmaid/lib/python3.6/site-packages/django/db/backends/base/schema.py", line 303, in effective_default
    return field.get_db_prep_save(self._effective_default(field), self.connection)
  File "/home/tom/.virtualenvs/catmaid/lib/python3.6/site-packages/django/db/models/fields/__init__.py", line 817, in get_db_prep_save
    return self.get_db_prep_value(value, connection=connection, prepared=False)
  File "/home/tom/.virtualenvs/catmaid/lib/python3.6/site-packages/django/db/models/fields/__init__.py", line 1372, in get_db_prep_value
    value = self.get_prep_value(value)
  File "/home/tom/.virtualenvs/catmaid/lib/python3.6/site-packages/django/db/models/fields/__init__.py", line 1351, in get_prep_value
    value = super().get_prep_value(value)
  File "/home/tom/.virtualenvs/catmaid/lib/python3.6/site-packages/django/db/models/fields/__init__.py", line 1211, in get_prep_value
    return self.to_python(value)
  File "/home/tom/.virtualenvs/catmaid/lib/python3.6/site-packages/django/db/models/fields/__init__.py", line 1312, in to_python
    parsed = parse_datetime(value)
  File "/home/tom/.virtualenvs/catmaid/lib/python3.6/site-packages/django/utils/dateparse.py", line 107, in parse_datetime
    match = datetime_re.match(value)
TypeError: expected string or bytes-like object

Of course, there is technically no change required on the database level during such a migration. Therefore it seems fine to only run those default updating changes as state_operations during a migration, and don't have Django execute them as regular operations:

class Migration(migrations.Migration):

    dependencies = [
        ('catmaid', '0099_make_concept_ids_64_bit'),
    ]

    operations = [
        migrations.RunSQL(migrations.RunSQL.noop, migrations.RunSQL.noop, [
            migrations.AlterField(
                model_name='cardinalityrestriction',
                name='creation_time',
                field=catmaid.fields.DbDefaultDateTimeField(default=django.contrib.postgres.functions.TransactionNow),
            ),
        ]),
    ] 

This works for now as workaround, but it would be nice if Django could figure this out on its own.

Change History (5)

comment:1 by Mariusz Felisiak, 5 years ago

Cc: Johannes Maron added
Summary: Migration fails when a fields has db_returning = True setMigration crashes when altering a field to use the db_returning flag and default value returned from database.
Triage Stage: UnreviewedAccepted

Thanks for this ticket, we should fix handling "effective" defaults in such cases. It's not a release blocker because db_returning is still a part of undocumented and private API.

comment:2 by Ashutosh Chauhan, 5 years ago

I would like to work on this issue. Can anyone help me where to start. I would look into the ticket:29444 to get some idea.

comment:3 by Johannes Maron, 5 years ago

Has patch: set
Owner: changed from nobody to Johannes Maron
Status: newassigned

Hi there,

while looking into it, I discovered a simple fix as well as a way to test it. So please excuse me snatching that ticket. But it was simpler to just push my branch instead of explaining how to fix it.

Best
-Joe

Version 0, edited 5 years ago by Johannes Maron (next)

comment:4 by Mariusz Felisiak, 5 years ago

Has patch: unset
Resolution: duplicate
Status: assignedclosed
Type: BugNew feature

After reconsideration I'm closing this ticket as a duplicate of #30032, because we don't officially support using expressions in Field.default. This should be fixed as a part of #30032.

comment:5 by Johannes Maron, 5 years ago

Ok, I will include it in the default patch.

Note: See TracTickets for help on using tickets.
Back to Top