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

Allow preserve comment on column rename in MySQL #594

Closed
randallpittman opened this issue Aug 23, 2019 · 5 comments

Comments

@randallpittman
Copy link

commented Aug 23, 2019

Versions:
Python: 3.7.4 (Linux/x64/conda)
Alembic: 1.0.11
SQLAlchemy: 1.3.5
MariaDB: 10.3.13

Steps to reproduce

  1. Have a table storage_drive with Integer column drive_partition, nullable, and comment If more than one partition, the 1-based partition index.
  2. In alembic upgrade() migration function, issue the following command to change the column name to partition_:
op.alter_column(
    "storage_drive",
    "drive_partition",
    existing_type=sa.Integer(),
    existing_comment="If more than one partition, the 1-based partition index.",
    new_column_name="partition_",
)

Expected Result:
Column is renamed to partition_ and the comment is preserved.

Actual Result:
Column is renamed to partition_ and the comment disappears.

Investigation

  • I used my interactive debugger to step through the alter_table command, and eventually got to alembic/ddl/mysql.py line 47.
  • Since name is not None evaluated to True , the call to MySQLChangeColumn at line 51 was executed.
  • Notably, this call does not include the comment keyword argument (vs. the MySQLModifyColumn call at line 77), so existing_comment was dropped.

Workaround
Add another alter_column call to reinstate the comment.

op.alter_column(
    "storage_drive",
    "partition_",
    existing_type=sa.Integer(),
    comment="If more than one partition, the 1-based partition index.",
)

@randallpittman randallpittman changed the title Allow preserve comment on column rename Allow preserve comment on column rename in MySQL Aug 23, 2019

@randallpittman

This comment has been minimized.

Copy link
Author

commented Aug 23, 2019

Note: In MySQL, the following two commands:

ALTER TABLE storage_drive CHANGE drive_partition partition_ INTEGER NULL;
ALTER TABLE storage_drive MODIFY partition_ INTEGER NULL COMMENT 'If more than one partition, the 1-based partition index.';

Can be combined into one:

ALTER TABLE storage_drive CHANGE drive_partition partition_ INTEGER NULL COMMENT 'If more than one partition, the 1-based partition index.';
@randallpittman

This comment has been minimized.

Copy link
Author

commented Aug 23, 2019

Really, I'd think preserving comments would be default behavior, but it isn't default behavior in the database so I can see why it isn't default in alembic.

@sqla-tester

This comment has been minimized.

Copy link
Collaborator

commented Aug 24, 2019

Mike Bayer has proposed a fix for this issue in the master branch:

Include existing_comment in MySQLChangeColumn https://gerrit.sqlalchemy.org/1431

@zzzeek

This comment has been minimized.

Copy link
Member

commented Aug 24, 2019

Can you confirm this patch solves your issue:

diff --git a/alembic/ddl/mysql.py b/alembic/ddl/mysql.py
index 17ae2de..e919599 100644
--- a/alembic/ddl/mysql.py
+++ b/alembic/ddl/mysql.py
@@ -64,6 +64,9 @@ class MySQLImpl(DefaultImpl):
                     autoincrement=autoincrement
                     if autoincrement is not None
                     else existing_autoincrement,
+                    comment=comment
+                    if comment is not False
+                    else existing_comment,
                 )
             )
         elif (
@randallpittman

This comment has been minimized.

Copy link
Author

commented Aug 26, 2019

Yes, your patch does indeed solve the issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.