piccolo
piccolo copied to clipboard
Fix adding and dropping column with db_column_name in auto migration
Fix #945.
Analysis
Adding Column with db_column_name
When adding a new column with db_column_name
to an existing table, the forward migration succeeds, but a column with name
instead of db_column_name
is created in the actual table.
For example, if adding age
field to the Users
class and then running the forward migration, the created column name is not user_age
but age
.
class Users(Table):
name = Text(db_column_name="user_name")
+ age = SmallInt(db_column_name="user_age")
And more, backward migration after it fails with UndefinedColumnError because db_column_name
in the migration file does not match the column name in the DB.
In my analysis, this is because the migration file is created as expected, but migration process create column with name
instead of db_column_name
.
This issue would be fixed by deleting the following line. https://github.com/piccolo-orm/piccolo/blob/master/piccolo/query/methods/alter.py#L320
In my understanding, there is no problem to delete the line beause if db_column_name
is None, the property returns name
.
Dropping Column with db_column_name
When dropping a column from an existing table, the forward migration fails.
For example, if removing age
field from the Users
class and then running the forward migration, it fails with UndefinedColumnError.
class Users(Table):
name = Text(db_column_name="user_name")
- age = SmallInt(db_column_name="user_age")
In my analysis, this is because the migration file is created as expected, but migration process attempts to drop the column by specifing name
instead of db_column_name
.
This issue would be fixed by changing the argument of the following line to column.db_column_name
or column
.
https://github.com/piccolo-orm/piccolo/blob/master/piccolo/apps/migrations/auto/migration_manager.py#L670
I have updated my comment with my analysis. I hope this helps with the review.
@atkei That's very helpful, thanks 👍