From 01f4d841c180ea3c9b2d6f359b0a8df01edd6e65 Mon Sep 17 00:00:00 2001 From: vinayak Date: Thu, 1 Oct 2026 01:52:00 +0530 Subject: [PATCH 1/5] Preserve empty string comments on create table and add column Fixes #1085: create_table and column render and DDL paths used if comment: which dropped empty string, so reflection returned None and autogenerate looped forever. Change to is not None so comment='' is emitted. Oracle special cased in comparators where '' equals NULL. --- alembic/autogenerate/compare/comments.py | 23 ++++++++++++++++++----- alembic/autogenerate/render.py | 4 ++-- alembic/ddl/impl.py | 4 ++-- alembic/ddl/mysql.py | 2 +- alembic/operations/toimpl.py | 2 +- 5 files changed, 24 insertions(+), 11 deletions(-) diff --git a/alembic/autogenerate/compare/comments.py b/alembic/autogenerate/compare/comments.py index 8db56606..d8cda31e 100644 --- a/alembic/autogenerate/compare/comments.py +++ b/alembic/autogenerate/compare/comments.py @@ -36,6 +36,11 @@ def _compare_column_comment( metadata_comment = metadata_col.comment conn_col_comment = conn_col.comment + if autogen_context.dialect.name == "oracle": + if metadata_comment == "": + metadata_comment = None + if conn_col_comment == "": + conn_col_comment = None if conn_col_comment is None and metadata_comment is None: return PriorityDispatchResult.CONTINUE @@ -70,19 +75,27 @@ def _compare_table_comment( if conn_table.comment is None and metadata_table.comment is None: return PriorityDispatchResult.CONTINUE - if metadata_table.comment is None and conn_table.comment is not None: + conn_comment = conn_table.comment + metadata_comment = metadata_table.comment + if autogen_context.dialect.name == "oracle": + if conn_comment == "": + conn_comment = None + if metadata_comment == "": + metadata_comment = None + + if metadata_comment is None and conn_comment is not None: modify_table_ops.ops.append( ops.DropTableCommentOp( - tname, existing_comment=conn_table.comment, schema=schema + tname, existing_comment=conn_comment, schema=schema ) ) return PriorityDispatchResult.STOP - elif metadata_table.comment != conn_table.comment: + elif metadata_comment != conn_comment: modify_table_ops.ops.append( ops.CreateTableCommentOp( tname, - metadata_table.comment, - existing_comment=conn_table.comment, + metadata_comment, + existing_comment=conn_comment, schema=schema, ) ) diff --git a/alembic/autogenerate/render.py b/alembic/autogenerate/render.py index 84e21bfa..06453838 100644 --- a/alembic/autogenerate/render.py +++ b/alembic/autogenerate/render.py @@ -267,7 +267,7 @@ def _add_table(autogen_context: AutogenContext, op: ops.CreateTableOp) -> str: text += ",\nschema=%r" % _ident(op.schema) comment = table.comment - if comment: + if comment is not None: text += ",\ncomment=%r" % _ident(comment) info = table.info @@ -808,7 +808,7 @@ def _render_column( opts.append(("system", column.system)) comment = column.comment - if comment: + if comment is not None: opts.append(("comment", "%r" % comment)) # TODO: for non-ascii colname, assign a "key" diff --git a/alembic/ddl/impl.py b/alembic/ddl/impl.py index be5dbb5a..347974f2 100644 --- a/alembic/ddl/impl.py +++ b/alembic/ddl/impl.py @@ -442,12 +442,12 @@ def create_table(self, table: Table, **kw: Any) -> None: self.dialect.supports_comments and not self.dialect.inline_comments ) comment = table.comment - if comment and with_comment: + if comment is not None and with_comment: self.create_table_comment(table) for column in table.columns: comment = column.comment - if comment and with_comment: + if comment is not None and with_comment: self.create_column_comment(column) def drop_table(self, table: Table, **kw: Any) -> None: diff --git a/alembic/ddl/mysql.py b/alembic/ddl/mysql.py index 19ef3e82..a2b12716 100644 --- a/alembic/ddl/mysql.py +++ b/alembic/ddl/mysql.py @@ -552,7 +552,7 @@ def _mysql_colspec( spec += " AUTO_INCREMENT" if server_default is not False and server_default is not None: spec += " DEFAULT %s" % format_server_default(compiler, server_default) - if comment: + if comment is not None: spec += " COMMENT %s" % compiler.sql_compiler.render_literal_value( comment, sqltypes.String() ) diff --git a/alembic/operations/toimpl.py b/alembic/operations/toimpl.py index c4eef3d7..7dddbe0a 100644 --- a/alembic/operations/toimpl.py +++ b/alembic/operations/toimpl.py @@ -209,7 +209,7 @@ def add_column(operations: "Operations", operation: "ops.AddColumnOp") -> None: and not operations.impl.dialect.inline_comments ) comment = column.comment - if comment and with_comment: + if comment is not None and with_comment: operations.impl.create_column_comment(column) From 1ce026eb72db0c264ab2581a6ce007bd0fa7e2d3 Mon Sep 17 00:00:00 2001 From: vinayak Date: Thu, 1 Oct 2026 23:46:17 +0530 Subject: [PATCH 2/5] Add empty comment render and compare tests for #1085 --- tests/test_autogen_render.py | 100 +++++++++++++++++++++++++++++++++++ 1 file changed, 100 insertions(+) diff --git a/tests/test_autogen_render.py b/tests/test_autogen_render.py index 7d966934..a1d0822a 100644 --- a/tests/test_autogen_render.py +++ b/tests/test_autogen_render.py @@ -1400,6 +1400,17 @@ def test_render_col_with_comment(self): "comment='This is a comment')", ) + def test_render_col_with_empty_comment(self): + c = Column("some_key", Integer, comment="") + Table("some_table", MetaData(), c) + result = autogenerate.render._render_column(c, self.autogen_context) + eq_ignore_whitespace( + result, + "sa.Column('some_key', sa.Integer(), " + "nullable=True, " + "comment='')", + ) + def test_render_col_comment_with_quote(self): c = Column("some_key", Integer, comment="This is a john's comment") Table("some_table", MetaData(), c) @@ -2299,6 +2310,24 @@ def test_render_table_with_comment(self): ")", ) + def test_render_table_with_empty_comment(self): + m = MetaData() + t = Table( + "test", + m, + Column("id", Integer, primary_key=True, comment=""), + comment="", + ) + op_obj = ops.CreateTableOp.from_table(t) + eq_ignore_whitespace( + autogenerate.render_op_text(self.autogen_context, op_obj), + "op.create_table('test'," + "sa.Column('id', sa.Integer(), nullable=False, comment='')," + "sa.PrimaryKeyConstraint('id')," + "comment=''" + ")", + ) + def test_render_table_with_info(self): m = MetaData() t = Table( @@ -2330,6 +2359,77 @@ def test_render_add_column_with_comment(self): "nullable=True, comment='This is a Column'))", ) + def test_render_add_column_with_empty_comment(self): + op_obj = ops.AddColumnOp("foo", Column("x", Integer, comment="")) + eq_ignore_whitespace( + autogenerate.render_op_text(self.autogen_context, op_obj), + "op.add_column('foo', sa.Column('x', sa.Integer(), " + "nullable=True, comment=''))", + ) + + def test_compare_column_empty_comment_distinct(self): + from types import SimpleNamespace + + from alembic.autogenerate.compare import comments + from alembic.util import PriorityDispatchResult + + for dialect_name, expected in ( + ("postgresql", PriorityDispatchResult.STOP), + ("oracle", PriorityDispatchResult.CONTINUE), + ): + ctx = SimpleNamespace( + dialect=SimpleNamespace( + supports_comments=True, name=dialect_name + ) + ) + alter_op = ops.AlterColumnOp("t", "c") + result = comments._compare_column_comment( + ctx, + alter_op, + None, + "t", + "c", + Column("c", Integer, comment=None), + Column("c", Integer, comment=""), + ) + eq_(result, expected) + + def test_compare_table_empty_comment_distinct(self): + from types import SimpleNamespace + + from alembic.autogenerate.compare import comments + from alembic.util import PriorityDispatchResult + + for dialect_name, expected in ( + ("postgresql", PriorityDispatchResult.STOP), + ("oracle", PriorityDispatchResult.CONTINUE), + ): + ctx = SimpleNamespace( + dialect=SimpleNamespace( + supports_comments=True, name=dialect_name + ) + ) + mops = ops.ModifyTableOps("t", []) + result = comments._compare_table_comment( + ctx, + mops, + None, + "t", + Table( + "t", + MetaData(), + Column("c", Integer), + comment=None, + ), + Table( + "t", + MetaData(), + Column("c", Integer), + comment="", + ), + ) + eq_(result, expected) + def test_render_create_table_comment_op(self): op_obj = ops.CreateTableCommentOp("table_name", "comment") eq_ignore_whitespace( From 60ffb5a34ed415aaf0f9b7540b99e9c6d6505cd0 Mon Sep 17 00:00:00 2001 From: vinayak Date: Thu, 1 Oct 2026 23:53:27 +0530 Subject: [PATCH 3/5] Use normalize_comment impl hook for Oracle empty string handling --- alembic/autogenerate/compare/comments.py | 20 ++++--------- alembic/ddl/impl.py | 3 ++ alembic/ddl/oracle.py | 5 ++++ tests/test_autogen_render.py | 38 ++++++++++++++++-------- 4 files changed, 40 insertions(+), 26 deletions(-) diff --git a/alembic/autogenerate/compare/comments.py b/alembic/autogenerate/compare/comments.py index d8cda31e..8fb2e80b 100644 --- a/alembic/autogenerate/compare/comments.py +++ b/alembic/autogenerate/compare/comments.py @@ -34,13 +34,9 @@ def _compare_column_comment( if not autogen_context.dialect.supports_comments: return PriorityDispatchResult.CONTINUE - metadata_comment = metadata_col.comment - conn_col_comment = conn_col.comment - if autogen_context.dialect.name == "oracle": - if metadata_comment == "": - metadata_comment = None - if conn_col_comment == "": - conn_col_comment = None + impl = autogen_context.migration_context.impl + metadata_comment = impl.normalize_comment(metadata_col.comment) + conn_col_comment = impl.normalize_comment(conn_col.comment) if conn_col_comment is None and metadata_comment is None: return PriorityDispatchResult.CONTINUE @@ -75,13 +71,9 @@ def _compare_table_comment( if conn_table.comment is None and metadata_table.comment is None: return PriorityDispatchResult.CONTINUE - conn_comment = conn_table.comment - metadata_comment = metadata_table.comment - if autogen_context.dialect.name == "oracle": - if conn_comment == "": - conn_comment = None - if metadata_comment == "": - metadata_comment = None + impl = autogen_context.migration_context.impl + conn_comment = impl.normalize_comment(conn_table.comment) + metadata_comment = impl.normalize_comment(metadata_table.comment) if metadata_comment is None and conn_comment is not None: modify_table_ops.ops.append( diff --git a/alembic/ddl/impl.py b/alembic/ddl/impl.py index 347974f2..e7e09f97 100644 --- a/alembic/ddl/impl.py +++ b/alembic/ddl/impl.py @@ -632,6 +632,9 @@ def compare_server_default( ): return rendered_inspector_default != rendered_metadata_default + def normalize_comment(self, comment: str | None) -> str | None: + return comment + def correct_for_autogen_constraints( self, conn_uniques: set[UniqueConstraint], diff --git a/alembic/ddl/oracle.py b/alembic/ddl/oracle.py index c93fa4b5..72df61c8 100644 --- a/alembic/ddl/oracle.py +++ b/alembic/ddl/oracle.py @@ -55,6 +55,11 @@ def _exec(self, construct: Any, *args, **kw) -> CursorResult | None: self.static_output(self.batch_separator) return result + def normalize_comment(self, comment: str | None) -> str | None: + if comment == "": + return None + return comment + def compare_server_default( self, inspector_column, diff --git a/tests/test_autogen_render.py b/tests/test_autogen_render.py index a1d0822a..11db9899 100644 --- a/tests/test_autogen_render.py +++ b/tests/test_autogen_render.py @@ -2371,16 +2371,23 @@ def test_compare_column_empty_comment_distinct(self): from types import SimpleNamespace from alembic.autogenerate.compare import comments + from alembic.ddl.impl import DefaultImpl + from alembic.ddl.oracle import OracleImpl from alembic.util import PriorityDispatchResult - for dialect_name, expected in ( - ("postgresql", PriorityDispatchResult.STOP), - ("oracle", PriorityDispatchResult.CONTINUE), + for impl_cls, expected in ( + (DefaultImpl, PriorityDispatchResult.STOP), + (OracleImpl, PriorityDispatchResult.CONTINUE), ): ctx = SimpleNamespace( - dialect=SimpleNamespace( - supports_comments=True, name=dialect_name - ) + dialect=SimpleNamespace(supports_comments=True), + migration_context=SimpleNamespace( + impl=SimpleNamespace( + normalize_comment=lambda c, _cls=impl_cls: ( + _cls.normalize_comment(None, c) + ) + ) + ), ) alter_op = ops.AlterColumnOp("t", "c") result = comments._compare_column_comment( @@ -2398,16 +2405,23 @@ def test_compare_table_empty_comment_distinct(self): from types import SimpleNamespace from alembic.autogenerate.compare import comments + from alembic.ddl.impl import DefaultImpl + from alembic.ddl.oracle import OracleImpl from alembic.util import PriorityDispatchResult - for dialect_name, expected in ( - ("postgresql", PriorityDispatchResult.STOP), - ("oracle", PriorityDispatchResult.CONTINUE), + for impl_cls, expected in ( + (DefaultImpl, PriorityDispatchResult.STOP), + (OracleImpl, PriorityDispatchResult.CONTINUE), ): ctx = SimpleNamespace( - dialect=SimpleNamespace( - supports_comments=True, name=dialect_name - ) + dialect=SimpleNamespace(supports_comments=True), + migration_context=SimpleNamespace( + impl=SimpleNamespace( + normalize_comment=lambda c, _cls=impl_cls: ( + _cls.normalize_comment(None, c) + ) + ) + ), ) mops = ops.ModifyTableOps("t", []) result = comments._compare_table_comment( From c0c50f381a5ba442b9276625534519ecb9443212 Mon Sep 17 00:00:00 2001 From: vineoy Date: Mon, 5 Oct 2026 23:46:15 +0530 Subject: [PATCH 4/5] test: move imports to module level in empty comment tests Signed-off-by: vineoy --- tests/test_autogen_render.py | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/tests/test_autogen_render.py b/tests/test_autogen_render.py index 11db9899..27a2418e 100644 --- a/tests/test_autogen_render.py +++ b/tests/test_autogen_render.py @@ -1,4 +1,5 @@ import re +from types import SimpleNamespace import sqlalchemy as sa # noqa from sqlalchemy import BigInteger @@ -41,6 +42,9 @@ from alembic import op # noqa from alembic import testing from alembic.autogenerate import api +from alembic.autogenerate.compare import comments +from alembic.ddl.impl import DefaultImpl +from alembic.ddl.oracle import OracleImpl from alembic.migration import MigrationContext from alembic.operations import ops from alembic.testing import assert_raises @@ -52,6 +56,7 @@ from alembic.testing import mock from alembic.testing import TestBase from alembic.testing.fixtures import op_fixture +from alembic.util import PriorityDispatchResult from alembic.util import sqla_compat @@ -2368,13 +2373,6 @@ def test_render_add_column_with_empty_comment(self): ) def test_compare_column_empty_comment_distinct(self): - from types import SimpleNamespace - - from alembic.autogenerate.compare import comments - from alembic.ddl.impl import DefaultImpl - from alembic.ddl.oracle import OracleImpl - from alembic.util import PriorityDispatchResult - for impl_cls, expected in ( (DefaultImpl, PriorityDispatchResult.STOP), (OracleImpl, PriorityDispatchResult.CONTINUE), @@ -2402,13 +2400,6 @@ def test_compare_column_empty_comment_distinct(self): eq_(result, expected) def test_compare_table_empty_comment_distinct(self): - from types import SimpleNamespace - - from alembic.autogenerate.compare import comments - from alembic.ddl.impl import DefaultImpl - from alembic.ddl.oracle import OracleImpl - from alembic.util import PriorityDispatchResult - for impl_cls, expected in ( (DefaultImpl, PriorityDispatchResult.STOP), (OracleImpl, PriorityDispatchResult.CONTINUE), From 5eacb86028259317cc20ee3af078514ec4008314 Mon Sep 17 00:00:00 2001 From: vinayak Date: Thu, 15 Oct 2026 01:35:40 +0530 Subject: [PATCH 5/5] Move empty comment compare tests to backend CompareCommentTest Per review: compare tests should be real autogendiff tests with __backend__ = True running for every backend that supports comments, in test_autogen_diffs. Remove mocked SimpleNamespace tests from test_autogen_render.py. Oracle normalizes empty string to NULL so expects no diff there. --- tests/test_autogen_diffs.py | 75 ++++++++++++++++++++++++++++++++++++ tests/test_autogen_render.py | 68 -------------------------------- 2 files changed, 75 insertions(+), 68 deletions(-) diff --git a/tests/test_autogen_diffs.py b/tests/test_autogen_diffs.py index 2d28b02d..fe7885f6 100644 --- a/tests/test_autogen_diffs.py +++ b/tests/test_autogen_diffs.py @@ -2682,3 +2682,78 @@ def include_name(name, type_, parent_names): diffs = self._fixture(m1, m2, name_filters=include_name) eq_(len(diffs), 1) eq_(diffs[0][0], "remove_fk") + + +class CompareCommentTest(AutogenFixtureTest, TestBase): + """Empty string comments are distinct from NULL, except on Oracle + where empty string equals NULL. See #1085.""" + + __backend__ = True + __requires__ = ("comments",) + + def test_column_empty_comment_no_change(self): + m1 = MetaData() + m2 = MetaData() + + Table("a", m1, Column("x", Integer, comment="")) + Table("a", m2, Column("x", Integer, comment="")) + + diffs = self._fixture(m1, m2) + eq_(diffs, []) + + def test_column_none_to_empty_comment(self): + m1 = MetaData() + m2 = MetaData() + + Table("a", m1, Column("x", Integer)) + Table("a", m2, Column("x", Integer, comment="")) + + diffs = self._fixture(m1, m2) + if config.db.name == "oracle": + eq_(diffs, []) + else: + eq_(len(diffs), 1) + eq_(diffs[0][0][0], "modify_comment") + eq_(diffs[0][0][5], None) + eq_(diffs[0][0][6], "") + + def test_table_empty_comment_no_change(self): + m1 = MetaData() + m2 = MetaData() + + Table( + "some_table", + m1, + Column("x", Integer, primary_key=True), + comment="", + ) + Table( + "some_table", + m2, + Column("x", Integer, primary_key=True), + comment="", + ) + + diffs = self._fixture(m1, m2) + eq_(diffs, []) + + def test_table_none_to_empty_comment(self): + m1 = MetaData() + m2 = MetaData() + + Table("some_table", m1, Column("x", Integer, primary_key=True)) + Table( + "some_table", + m2, + Column("x", Integer, primary_key=True), + comment="", + ) + + diffs = self._fixture(m1, m2) + if config.db.name == "oracle": + eq_(diffs, []) + else: + eq_(len(diffs), 1) + eq_(diffs[0][0], "add_table_comment") + eq_(diffs[0][1].comment, "") + eq_(diffs[0][2], None) diff --git a/tests/test_autogen_render.py b/tests/test_autogen_render.py index 27a2418e..1d42e4fe 100644 --- a/tests/test_autogen_render.py +++ b/tests/test_autogen_render.py @@ -1,5 +1,4 @@ import re -from types import SimpleNamespace import sqlalchemy as sa # noqa from sqlalchemy import BigInteger @@ -42,9 +41,6 @@ from alembic import op # noqa from alembic import testing from alembic.autogenerate import api -from alembic.autogenerate.compare import comments -from alembic.ddl.impl import DefaultImpl -from alembic.ddl.oracle import OracleImpl from alembic.migration import MigrationContext from alembic.operations import ops from alembic.testing import assert_raises @@ -56,7 +52,6 @@ from alembic.testing import mock from alembic.testing import TestBase from alembic.testing.fixtures import op_fixture -from alembic.util import PriorityDispatchResult from alembic.util import sqla_compat @@ -2372,69 +2367,6 @@ def test_render_add_column_with_empty_comment(self): "nullable=True, comment=''))", ) - def test_compare_column_empty_comment_distinct(self): - for impl_cls, expected in ( - (DefaultImpl, PriorityDispatchResult.STOP), - (OracleImpl, PriorityDispatchResult.CONTINUE), - ): - ctx = SimpleNamespace( - dialect=SimpleNamespace(supports_comments=True), - migration_context=SimpleNamespace( - impl=SimpleNamespace( - normalize_comment=lambda c, _cls=impl_cls: ( - _cls.normalize_comment(None, c) - ) - ) - ), - ) - alter_op = ops.AlterColumnOp("t", "c") - result = comments._compare_column_comment( - ctx, - alter_op, - None, - "t", - "c", - Column("c", Integer, comment=None), - Column("c", Integer, comment=""), - ) - eq_(result, expected) - - def test_compare_table_empty_comment_distinct(self): - for impl_cls, expected in ( - (DefaultImpl, PriorityDispatchResult.STOP), - (OracleImpl, PriorityDispatchResult.CONTINUE), - ): - ctx = SimpleNamespace( - dialect=SimpleNamespace(supports_comments=True), - migration_context=SimpleNamespace( - impl=SimpleNamespace( - normalize_comment=lambda c, _cls=impl_cls: ( - _cls.normalize_comment(None, c) - ) - ) - ), - ) - mops = ops.ModifyTableOps("t", []) - result = comments._compare_table_comment( - ctx, - mops, - None, - "t", - Table( - "t", - MetaData(), - Column("c", Integer), - comment=None, - ), - Table( - "t", - MetaData(), - Column("c", Integer), - comment="", - ), - ) - eq_(result, expected) - def test_render_create_table_comment_op(self): op_obj = ops.CreateTableCommentOp("table_name", "comment") eq_ignore_whitespace(