[Django] #32206: Django cannot detect that a field has been renamed if it has verbose_name set
Django
set
------------------------------------------+------------------------
Reporter: Peter Inglesby | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 3.1
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------+------------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32206>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
verbose_name set
--------------------------------+------------------------------------
Reporter: Peter Inglesby | Owner: nobody
Component: Migrations | Version: 3.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* type: Uncategorized => Bug
* component: Uncategorized => Migrations
* stage: Unreviewed => Accepted
Comment:
Hi Peter. Nice example. Yes, that does seem curious. I can confirm the
behavior. Didn't look into why it happens as yet.
--
Ticket URL: <https://code.djangoproject.com/ticket/32206#comment:1>
Django
verbose_name set
--------------------------------+------------------------------------
Component: Migrations | Version: 3.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
Detection of field renames happens in the auto-detector's
`generate_renamed_fields` method
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]`.
--
Ticket URL: <https://code.djangoproject.com/ticket/32206#comment:2>
Django
verbose_name set
--------------------------------+------------------------------------
Component: Migrations | Version: 3.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32206#comment:3>
Django
verbose_name set
Reporter: Peter Inglesby | Owner: Hasan Ramezani
Type: Bug | Status: assigned
Component: Migrations | Version: 3.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Hasan Ramezani):
* owner: nobody => Hasan Ramezani
* status: new => assigned
* has_patch: 0 => 1
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32206#comment:4>
Django
verbose_name set
Reporter: Peter Inglesby | Owner: Hasan
Cleanup/optimization | Status: assigned
Component: Migrations | Version: 3.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* type: Bug => Cleanup/optimization
Comment:
Replying to [comment:3 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
[https://github.com/django/django/blob/9f72b0970db4be76d14bd7be5a21582d2cc484bc/django/db/backends/base/schema.py#L1061-L1088
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).
--
Ticket URL: <https://code.djangoproject.com/ticket/32206#comment:5>
Django
verbose_name set
-------------------------------------+-------------------------------------
Reporter: Peter Inglesby | Owner: Hasan
Type: | Ramezani
Cleanup/optimization | Status: assigned
Component: Migrations | Version: 3.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
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!)
--
Ticket URL: <https://code.djangoproject.com/ticket/32206#comment:6>
Django
verbose_name set
-------------------------------------+-------------------------------------
Reporter: Peter Inglesby | Owner: Hasan
Type: | Ramezani
Component: Migrations | Version: 3.1
Severity: Normal | Resolution: duplicate
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* status: assigned => closed
* resolution: => duplicate
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32206#comment:7>