Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

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

Open
atkei wants to merge 2 commits into piccolo-orm:master
base: master
Choose a base branch
Loading
from atkei:fix-db-column-name-migration

Conversation

@atkei
Copy link

@atkei atkei commented Apr 11, 2024
edited
Loading

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

dantownsend and sinisaos reacted with thumbs up emoji
Copy link
Author

atkei commented Jul 12, 2024

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

dantownsend reacted with thumbs up emoji

Copy link
Member

@atkei That's very helpful, thanks 👍

atkei reacted with thumbs up emoji

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Using db_column_name when adding columns to existing tables using auto migrations

AltStyle によって変換されたページ (->オリジナル) /