Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

atkei
Copy link

@atkei atkei commented Apr 11, 2024

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
Copy link
Author

atkei commented Jul 12, 2024

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

@dantownsend
Copy link
Member

@atkei That's very helpful, thanks 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using db_column_name when adding columns to existing tables using auto migrations
2 participants