diff --git a/alembic/autogenerate/compare/comments.py b/alembic/autogenerate/compare/comments.py index 8db56606..60d05253 100644 --- a/alembic/autogenerate/compare/comments.py +++ b/alembic/autogenerate/compare/comments.py @@ -36,6 +36,13 @@ def _compare_column_comment( metadata_comment = metadata_col.comment conn_col_comment = conn_col.comment + # An empty-string comment is indistinguishable from no comment: the + # database stores/reflects it as NULL, so normalize '' to None to avoid a + # false-positive diff (#1085). + 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 @@ -67,22 +74,30 @@ def _compare_table_comment( if conn_table is None or metadata_table is None: return PriorityDispatchResult.CONTINUE - if conn_table.comment is None and metadata_table.comment is None: + conn_comment = conn_table.comment + meta_comment = metadata_table.comment + # An empty-string comment is indistinguishable from no comment (stored as + # NULL), so normalize '' to None to avoid a false-positive diff (#1085). + if conn_comment == "": + conn_comment = None + if meta_comment == "": + meta_comment = None + if conn_comment is None and meta_comment is None: return PriorityDispatchResult.CONTINUE - if metadata_table.comment is None and conn_table.comment is not None: + if meta_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 meta_comment != conn_comment: modify_table_ops.ops.append( ops.CreateTableCommentOp( tname, - metadata_table.comment, - existing_comment=conn_table.comment, + meta_comment, + existing_comment=conn_comment, schema=schema, ) ) diff --git a/docs/build/changelog.rst b/docs/build/changelog.rst index 9d392902..38f96cba 100644 --- a/docs/build/changelog.rst +++ b/docs/build/changelog.rst @@ -3,6 +3,20 @@ Changelog ========== +.. changelog:: + :version: unreleased + + .. change:: + :tags: bug, autogenerate + :tickets: 1085 + + Fixed a false positive in autogenerate where a column or table comment + set to the empty string (``comment=""``) in the metadata was reported + as a change against a database state with no comment. Empty comments + are reflected back as ``None``, so the comment comparators now treat + ``""`` and ``None`` as equivalent. + + .. changelog:: :version: 1.20.0 :include_notes_from: unreleased diff --git a/tests/test_autogen_diffs.py b/tests/test_autogen_diffs.py index 6aad9873..42d84133 100644 --- a/tests/test_autogen_diffs.py +++ b/tests/test_autogen_diffs.py @@ -39,6 +39,8 @@ from alembic import autogenerate from alembic import testing from alembic.autogenerate import api +from alembic.autogenerate.compare.comments import _compare_column_comment +from alembic.autogenerate.compare.comments import _compare_table_comment from alembic.autogenerate.compare.tables import _compare_tables from alembic.migration import MigrationContext from alembic.operations import ops @@ -59,6 +61,7 @@ from alembic.testing.suite._autogen_fixtures import AutogenTest from alembic.testing.suite._autogen_fixtures import ModelOne from alembic.util import CommandError +from alembic.util.langhelpers import PriorityDispatchResult if True: from alembic.autogenerate.compare.types import ( @@ -2683,3 +2686,89 @@ 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 CommentEmptyStringNoFalsePositiveTest(TestBase): + """Empty string comments must not diff against no comment. + + Databases store empty comment strings as NULL on reflection, so + comparing '' against None reported a false positive change (#1085). + The comparators are exercised directly with a comment-supporting + dialect stub, so these run on any backend. + """ + + def _ctx(self): + return mock.Mock(dialect=mock.Mock(supports_comments=True)) + + def test_column_comment_empty_string_vs_none(self): + ctx = self._ctx() + for conn_comment, meta_comment in [ + (None, ""), + ("", None), + ("", ""), + (None, None), + ]: + alter = ops.AlterTableOp("t") + conn_col = Column("c", String(), comment=conn_comment) + meta_col = Column("c", String(), comment=meta_comment) + res = _compare_column_comment( + ctx, alter, None, "t", "c", conn_col, meta_col + ) + is_(res, PriorityDispatchResult.CONTINUE) + is_(getattr(alter, "modify_comment", None), None) + + def test_column_comment_real_change_still_detected(self): + ctx = self._ctx() + for conn_comment, meta_comment, expected in [ + (None, "x", "x"), + ("x", None, None), + ("a", "b", "b"), + ]: + alter = ops.AlterTableOp("t") + conn_col = Column("c", String(), comment=conn_comment) + meta_col = Column("c", String(), comment=meta_comment) + res = _compare_column_comment( + ctx, alter, None, "t", "c", conn_col, meta_col + ) + is_(res, PriorityDispatchResult.STOP) + eq_(alter.modify_comment, expected) + + def test_table_comment_empty_string_vs_none(self): + ctx = self._ctx() + for conn_comment, meta_comment in [ + (None, ""), + ("", None), + ("", ""), + (None, None), + ]: + conn_table = Table( + "t", MetaData(), Column("c", String()), comment=conn_comment + ) + meta_table = Table( + "t", MetaData(), Column("c", String()), comment=meta_comment + ) + mops = ops.ModifyTableOps("t", []) + res = _compare_table_comment( + ctx, mops, None, "t", conn_table, meta_table + ) + is_(res, PriorityDispatchResult.CONTINUE) + eq_(len(mops.ops), 0) + + def test_table_comment_real_change_still_detected(self): + ctx = self._ctx() + for conn_comment, meta_comment, op_kind in [ + (None, "x", ops.CreateTableCommentOp), + ("x", None, ops.DropTableCommentOp), + ("a", "b", ops.CreateTableCommentOp), + ]: + conn_table = Table( + "t", MetaData(), Column("c", String()), comment=conn_comment + ) + meta_table = Table( + "t", MetaData(), Column("c", String()), comment=meta_comment + ) + mops = ops.ModifyTableOps("t", []) + res = _compare_table_comment( + ctx, mops, None, "t", conn_table, meta_table + ) + is_(res, PriorityDispatchResult.STOP) + eq_(len(mops.ops), 1) + assert isinstance(mops.ops[0], op_kind)