Opened 4 years ago

Closed 4 years ago

#32206 closed Cleanup/optimization (duplicate)

Incorrect migration generated if field has been renamed when it has verbose_name set

Reported by: Peter Inglesby Owned by: Hasan Ramezani
Component: Migrations Version: 3.1
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

If you start with following model:

class Thing(models.Model):
    name = models.CharField(max_length=100, default="xxx", verbose_name="name")

and change it to:

class Thing(models.Model):
    label = models.CharField(max_length=100, default="xxx", verbose_name="name")

Django asks:

Did you rename thing.name to thing.label (a CharField)? [y/N] y

and if you say yes, it creates a migration with:

    operations = [
        migrations.RenameField(
            model_name='thing',
            old_name='name',
            new_name='label',
        ),
    ]

But if you change it to:

class thing(models.model):
    label = models.CharField(max_length=100, default="xxx", verbose_name="label")

if creates a migration with:

    operations = [
        migrations.RemoveField(
            model_name='thing',
            name='name',
        ),
        migrations.AddField(
            model_name='thing',
            name='label',
            field=models.CharField(default='xxx', max_length=100, verbose_name='label'),
        ),
    ]

If you were to run this migration without thinking, you would risk losing data.

Change History (7)

comment:1 by Carlton Gibson, 4 years ago

Component: UncategorizedMigrations
Summary: Django cannot detect that a field has been renamed if it has verbose_name setIncorrect migration generated if field has been renamed when it has verbose_name set
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

Hi Peter. Nice example. Yes, that does seem curious. I can confirm the behavior. Didn't look into why it happens as yet.

comment:2 by Simon Charette, 4 years ago

Detection of field renames happens in the auto-detector's generate_renamed_fields method

https://github.com/django/django/blob/4a412c2e659f9295973434c65b785b03acee7251/django/db/migrations/autodetector.py#L837-L841

I guess it could be adjusted to ignore verbose_name when comparing fields like we do with db_column by storing old_field_verbose_name = old_field.verbose_name and setting verbose_name=old_field_verbose_name in the comparison against field_dec[2].

comment:3 by Simon Charette, 4 years ago

It feels like it's more of an optimization than a bug though as this behaviour is due to the initial design decision that the migration framework should not hold an allow list of attributes that affect database state or not. The same problem is present when altering validators for example.

comment:4 by Hasan Ramezani, 4 years ago

Has patch: set
Owner: changed from nobody to Hasan Ramezani
Status: newassigned

Thanks, Simon for the proposed solution to fix the problem.
I just thought maybe we can remove verbose_name from old_field_dec and field_dec and then compare them.
I don't know this is the right approach or not.

Version 0, edited 4 years ago by Hasan Ramezani (next)

in reply to:  3 comment:5 by Mariusz Felisiak, 4 years ago

Type: BugCleanup/optimization

Replying to Simon Charette:

It feels like it's more of an optimization than a bug though as this behaviour is due to the initial design decision that the migration framework should not hold an allow list of attributes that affect database state or not. The same problem is present when altering validators for example.

Yes it's not a bug, maybe a cleanup, but I'm not sure about changing the current behavior. There are multiple attributes that don't affect database state, see BaseDatabaseSchemaEditor._field_should_be_altered(), but we cannot guarantee this for 3rd-party backends. I would close this as wontfix and keep the current flow, so if you want to rename a field and change its non-database attributes you need to do this in two steps:

  • rename field (RenameField),
  • change attributes (AlterField, which is a noop operation for such attributes, see #25253 and #31826).

comment:6 by Carlton Gibson, 4 years ago

Hi All — thanks for the input.

Can I ask, can we warn in these kind of cases somehow?

So, a user should do it in two steps, and users should review generated migrations to make sure the operations are what's required, but I think this probably counts as a loaded foot-gun.

Perhaps we should re-open #31700 which suggested adding additional output for destructive operations? (Mailing list summary there was "Yes, maybe something at the makemigrations stage...) — What do we think? (Thanks!)

comment:7 by Carlton Gibson, 4 years ago

Resolution: duplicate
Status: assignedclosed

OK, given Simon ans Mariusz' comments lets take this as a duplicate of #31700 and aim to provide a better warning for this kind of case.

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