Is AlterColumnOp.modify_name ignored?

[lenvk]

Hello!

I suspect there is a bug, but I'm not completely sure that it's not "by design" - would be nice if you could take a look.

Context:

• alembic 1.13.1
• asyncpg 0.29.0
• we want to rename a column when the corresponding field is deleted from a model, not to delete the column itself

We found this rewriter feature https://alembic.sqlalchemy.org/en/latest/api/autogenerate.html#fine-grained-autogenerate-generation-with-rewriters
and decided that we can add something like this to our env.py

@writer.rewrites(ops.DropColumnOp)
def drop_column(context, revision, op):
    return [
        ops.AlterColumnOp(
            op.table_name,
            op.column_name,
            modify_name=f'del_{op.column_name}',
        )
    ]

but when we delete a field from model and run alembic revision --autogenerate -m "whatever", autogenerated migration contains
op.alter_column('awesome_table', 'redundant_field')
instead of
op.alter_column('awesome_table', 'redundant_field', new_column_name='del_redundant_field')

I have a strong feeling that it's connected to this refactoring 10 years ago
because it's the moment when has_changes was added to AlterColumnOp and it doesn't contain or self.modify_name is not None even now
https://github.com/sqlalchemy/alembic/blob/main/alembic/operations/ops.py#L1787

If my suspicions are naive, I can try to make a reproducable example, but for now it seems to be redundant :D

Is it possible that I'm doing smth wrong?

upd:

• issue AlterColumnOp.modify_name is ignored #1635
• pr fix AlterColumnOp modify_name ignorance #1636

[zzzeek]

hi -

I think this is a bug that has always been there, because you're looking at the output of render.py -> _alter_column(), and if you look in that function, it isn't even looking at modify_name at all, nor is it looking in that older version etiher.

if you want to open an issue and PR here's a patch

diff --git a/alembic/autogenerate/render.py b/alembic/autogenerate/render.py
index 50c51fa..ffc277d 100644
--- a/alembic/autogenerate/render.py
+++ b/alembic/autogenerate/render.py
@@ -505,6 +505,7 @@ def _alter_column(
     type_ = op.modify_type
     nullable = op.modify_nullable
     comment = op.modify_comment
+    newname = op.modify_name
     autoincrement = op.kw.get("autoincrement", None)
     existing_type = op.existing_type
     existing_nullable = op.existing_nullable
@@ -533,6 +534,8 @@ def _alter_column(
         rendered = _render_server_default(server_default, autogen_context)
         text += ",\n%sserver_default=%s" % (indent, rendered)
 
+    if newname is not None:
+        text += ",\n%snew_column_name=%r" % (indent, newname)
     if type_ is not None:
         text += ",\n%stype_=%s" % (indent, _repr_type(type_, autogen_context))
     if nullable is not None:
diff --git a/tests/test_autogen_render.py b/tests/test_autogen_render.py
index 600e578..ed31183 100644
--- a/tests/test_autogen_render.py
+++ b/tests/test_autogen_render.py
@@ -1327,6 +1327,18 @@ class AutogenRenderTest(TestBase):
             {"from mypackage import MySpecialType"},
         )
 
+    def test_render_modify_name(self):
+        op_obj = ops.AlterColumnOp(
+            "sometable",
+            "somecolumn",
+            modify_name="newcolumnname",
+        )
+        eq_ignore_whitespace(
+            autogenerate.render_op_text(self.autogen_context, op_obj),
+            "op.alter_column('sometable', 'somecolumn', "
+            "new_column_name='newcolumnname')",
+        )
+
     def test_render_modify_type(self):
         op_obj = ops.AlterColumnOp(
             "sometable",

[zzzeek]

downgrade will see an AddColumnOp that you similarly would turn into a rename. I think you can detect that this is a downgrade via the context being passed edit: no you'd have to use a custom process_revision_directives callable

[zzzeek]

if you are trying to detect downgrades you might want to use https://alembic.sqlalchemy.org/en/latest/api/runtime.html#alembic.runtime.environment.EnvironmentContext.configure.params.process_revision_directives hook with a callable directly that looks through MigrationScript.upgrade_ops / downgrade_ops directly since the rewriter doesnt send you that context right now https://alembic.sqlalchemy.org/en/latest/api/autogenerate.html#customizing-revision

[lenvk]

Thank u, i'll give it a try. Gorgeous cat btw!

[zzzeek]

ah she's passed unfortunately. the current partner in crime is this one

[lenvk]

For the record: the final solution with the released fix looks like this (ugly but works)

process_revision_directives in env.py

def process_revision_directives(context, revision, directives):
    script = directives[0]
    for upgrade_op in script.upgrade_ops.ops:
        operations = []
        for op in upgrade_op.ops:
            # not DropColumn -> okay
            if not isinstance(op, ops.DropColumnOp):
                operations.append(op)
            # drop the already renamed column -> skip (we'll drop it manually)
            elif op.column_name.startswith('del_'):
                continue
            # drop smth important -> rename from name to del_name
            else:
                operations.append(ops.AlterColumnOp(
                    op.table_name,
                    op.column_name,
                    modify_name=f'del_{op.column_name}',
                ))
        upgrade_op.ops = operations

    for downgrade_op in script.downgrade_ops.ops:
        operations = []
        for op in downgrade_op.ops:
            # not AddColumn - okay
            if not isinstance(op, ops.AddColumnOp):
                operations.append(op)
            # add the column which we'll remove manually -> skip
            elif op.column.name.startswith('del_'):
                continue
            # add smth important -> rename from del_name to name
            else:
                operations.append(ops.AlterColumnOp(
                    op.table_name,
                    # alembic doesn't know we renamed it in upgrade_ops
                    f'del_{op.column.name}',
                    modify_name=op.column.name,
                ))
        downgrade_op.ops = operations