[Django] #33197: Providing prior implicit field name to db_column should be a noop
Django
------------------------------------------------+------------------------
Reporter: Jacob Walls | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Migrations | Version: dev
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 |
------------------------------------------------+------------------------
Similar to #31826, I would expect no migration changes detected when a
`db_column` is added that matches the implicit field name. I tested with
and without renaming the field on SQLite and MySQL 5.7.31. I'm not
familiar enough with the review of #31826 to know whether these are
distinct cases versus if it should be reopened.
{{{
class Apple(models.Model):
core = models.BooleanField()
}}}
{{{
class Apple(models.Model):
core_renamed = models.BooleanField(db_column='core')
}}}
{{{
Was apple.core renamed to apple.core_renamed (a BooleanField)? [y/N] y
Migrations for 'renamez':
renamez/migrations/0002_rename_core_apple_core_renamed_and_more.py
- Rename field core on apple to core_renamed
- Alter field core_renamed on apple
}}}
`python manage.py sqlmigrate renamez 0002` showing unnecessary SQL:
{{{
BEGIN;
--
-- Rename field core on apple to core_renamed
--
ALTER TABLE "renamez_apple" RENAME COLUMN "core" TO "core_renamed";
--
-- Alter field core_renamed on apple
--
ALTER TABLE "renamez_apple" RENAME COLUMN "core_renamed" TO "core";
COMMIT;
}}}
Without renaming the field, follow the same flow and get an `AlterField`
migration without SQL:
{{{
BEGIN;
--
-- Alter field core on apple
--
COMMIT;
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33197>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Jacob Walls | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Migrations | Version: dev
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 Simon Charette):
* stage: Unreviewed => Accepted
Comment:
I think this is due to how the auto-detector
[https://github.com/django/django/blob/514c16e85f7ac2512235f3b6413646627420e969/django/db/migrations/autodetector.py#L182-L185
generates field renames before field alterations]. I assume the issue goes
away if you swap the order of `operations` in your migration?
[https://github.com/django/django/blob/514c16e85f7ac2512235f3b6413646627420e969/tests/migrations/test_autodetector.py#L985-L1011
As this test exemplifies] the `RenameField` operation happens before the
`AlterField` operation does so we'd need to adjust
`generate_renamed_fields` to add an `AlterField` and prevents
`generate_altered_fields` for doing so when this happens.
--
Ticket URL: <https://code.djangoproject.com/ticket/33197#comment:1>
Django
noop
--------------------------------------+------------------------------------
Reporter: Jacob Walls | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Migrations | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Old description:
New description:
Renaming a field and setting the prior implicit field name as the
`db_column` to avoid db operations creates a migration emitting
unnecessary SQL. Similar to #31826, which handled a very similar scenario
but where there is no field rename, I would expect a SQL noop. I tested
with SQLite and MySQL 5.7.31.
{{{
class Apple(models.Model):
core = models.BooleanField()
}}}
{{{
class Apple(models.Model):
core_renamed = models.BooleanField(db_column='core')
}}}
{{{
Was apple.core renamed to apple.core_renamed (a BooleanField)? [y/N] y
Migrations for 'renamez':
renamez/migrations/0002_rename_core_apple_core_renamed_and_more.py
- Rename field core on apple to core_renamed
- Alter field core_renamed on apple
}}}
`python manage.py sqlmigrate renamez 0002` showing unnecessary SQL:
{{{
BEGIN;
--
-- Rename field core on apple to core_renamed
--
ALTER TABLE "renamez_apple" RENAME COLUMN "core" TO "core_renamed";
--
-- Alter field core_renamed on apple
--
ALTER TABLE "renamez_apple" RENAME COLUMN "core_renamed" TO "core";
COMMIT;
}}}
Without renaming the field, follow the same flow and get an `AlterField`
migration without SQL, which is what #31826 intended:
{{{
BEGIN;
--
-- Alter field core on apple
--
COMMIT;
}}}
--
Comment (by Jacob Walls):
Thanks for having a look. I see now the scope of #31826 was just for flows
where the field is not renamed. So that makes this ticket a request to
extend this to field renames, which looks like was discussed as 3 and 4
[https://code.djangoproject.com/ticket/29245#comment:1 here].
> I assume the issue goes away if you swap the order of operations in your
migration?
If I switch the order to have `AlterField` followed by `RenameField`,
`FieldDoesNotExist` is raised when migrating. These are the operations:
{{{
operations = [
migrations.RenameField(
model_name='apple',
old_name='core',
new_name='core_renamed',
),
migrations.AlterField(
model_name='apple',
name='core_renamed',
field=models.BooleanField(db_column='core'),
),
]
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33197#comment:2>
Django
noop
--------------------------------------+------------------------------------
Reporter: Jacob Walls | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Migrations | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
You'll want to adjust `AlterField.name` accordingly if you swap the order
of operations; change `name='core_renamed'` to `name='core'`.
{{{#!python
operations = [
migrations.AlterField(
model_name='apple',
name='core',
field=models.BooleanField(db_column='core'),
),
migrations.RenameField(
model_name='apple',
old_name='core',
new_name='core_renamed',
),
]
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33197#comment:3>
Django
noop
--------------------------------------+------------------------------------
Reporter: Jacob Walls | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Migrations | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/33197#comment:4>
Django
noop
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Simon
Type: | Charette
Cleanup/optimization | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* owner: nobody => Simon Charette
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/33197#comment:5>
Django
noop
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Simon
Type: | Charette
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/33197#comment:6>
Django
noop
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Simon
Type: | Charette
Component: Migrations | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"5896aa8367d545c60bb544c31d7ecaaecc321045" 5896aa8]:
{{{
#!CommitTicketReference repository=""
revision="5896aa8367d545c60bb544c31d7ecaaecc321045"
Fixed #33197 -- Made field rename with prior matching db_column change a
noop.
Thanks Jacob Walls for the report.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33197#comment:7>