piccolo-orm / piccolo

A fast, user friendly ORM and query builder which supports asyncio.
https://piccolo-orm.com/
MIT License
1.45k stars 91 forks source link

Fix adding and dropping column with db_column_name in auto migration #983

Open atkei opened 7 months ago

atkei commented 7 months ago

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

atkei commented 4 months ago

I have updated my comment with my analysis. I hope this helps with the review.

dantownsend commented 4 months ago

@atkei That's very helpful, thanks 👍