From 26f8752a6c34c849221fa3c8c686ff67cd68e328 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Thu, 22 Dec 2022 18:41:20 -0500 Subject: [PATCH] dont use repr to quote string in compare_server_default Fixed issue where server default compare would not work for string defaults that contained backslashes, due to mis-rendering of these values when comparing their contents. The server default comparison still has a lot of not-robust behaviors, however at least get in place a parameterized test suite so that we can add new scenarios quickly. Made a slight adjustment to SQLite's compare server default implementation to better handle defaults with or without parens around them, from both the reflected and the local metadata side. Implemented basic server default comparison for the Oracle backend; previously, Oracle's formatting of reflected defaults prevented any matches from occurring. Change-Id: If5a69eec4b22d243a564d2c89e78ae33ba5be88f Fixes: #1145 --- alembic/autogenerate/compare.py | 2 +- alembic/ddl/mssql.py | 30 +++++++++----- alembic/ddl/oracle.py | 29 +++++++++++++ alembic/ddl/sqlite.py | 4 ++ docs/build/unreleased/1145.rst | 29 +++++++++++++ tests/test_autogen_diffs.py | 73 +++++++++++++++++++++++++++++++++ 6 files changed, 156 insertions(+), 11 deletions(-) create mode 100644 docs/build/unreleased/1145.rst diff --git a/alembic/autogenerate/compare.py b/alembic/autogenerate/compare.py index c9971ea3..8301e34c 100644 --- a/alembic/autogenerate/compare.py +++ b/alembic/autogenerate/compare.py @@ -1022,7 +1022,7 @@ def _render_server_default_for_compare( if isinstance(metadata_default, str): if metadata_col.type._type_affinity is sqltypes.String: metadata_default = re.sub(r"^'|'$", "", metadata_default) - return repr(metadata_default) + return f"'{metadata_default}'" else: return metadata_default else: diff --git a/alembic/ddl/mssql.py b/alembic/ddl/mssql.py index 6a208ec6..00c5f0ed 100644 --- a/alembic/ddl/mssql.py +++ b/alembic/ddl/mssql.py @@ -1,5 +1,6 @@ from __future__ import annotations +import re from typing import Any from typing import List from typing import Optional @@ -230,16 +231,25 @@ def compare_server_default( rendered_metadata_default, rendered_inspector_default, ): - def clean(value): - if value is not None: - value = value.strip() - while value[0] == "(" and value[-1] == ")": - value = value[1:-1] - return value - - return clean(rendered_inspector_default) != clean( - rendered_metadata_default - ) + if rendered_metadata_default is not None: + rendered_metadata_default = re.sub( + r"^\((.+)\)$", r"\1", rendered_metadata_default + ) + + rendered_metadata_default = re.sub( + r"^\"?'(.+)'\"?$", r"\1", rendered_metadata_default + ) + + if rendered_inspector_default is not None: + rendered_inspector_default = re.sub( + r"^\(+(.+?)\)+$", r"\1", rendered_inspector_default + ) + + rendered_inspector_default = re.sub( + r"^\"?'(.+)'\"?$", r"\1", rendered_inspector_default + ) + + return rendered_inspector_default != rendered_metadata_default def _compare_identity_default(self, metadata_identity, inspector_identity): diff, ignored, is_alter = super()._compare_identity_default( diff --git a/alembic/ddl/oracle.py b/alembic/ddl/oracle.py index 920b70ae..9715c1e8 100644 --- a/alembic/ddl/oracle.py +++ b/alembic/ddl/oracle.py @@ -1,5 +1,6 @@ from __future__ import annotations +import re from typing import Any from typing import Optional from typing import TYPE_CHECKING @@ -52,6 +53,34 @@ def _exec(self, construct: Any, *args, **kw) -> Optional[CursorResult]: self.static_output(self.batch_separator) return result + def compare_server_default( + self, + inspector_column, + metadata_column, + rendered_metadata_default, + rendered_inspector_default, + ): + if rendered_metadata_default is not None: + rendered_metadata_default = re.sub( + r"^\((.+)\)$", r"\1", rendered_metadata_default + ) + + rendered_metadata_default = re.sub( + r"^\"?'(.+)'\"?$", r"\1", rendered_metadata_default + ) + + if rendered_inspector_default is not None: + rendered_inspector_default = re.sub( + r"^\((.+)\)$", r"\1", rendered_inspector_default + ) + + rendered_inspector_default = re.sub( + r"^\"?'(.+)'\"?$", r"\1", rendered_inspector_default + ) + + rendered_inspector_default = rendered_inspector_default.strip() + return rendered_inspector_default != rendered_metadata_default + def emit_begin(self) -> None: self._exec("SET TRANSACTION READ WRITE") diff --git a/alembic/ddl/sqlite.py b/alembic/ddl/sqlite.py index 51233326..6b8c2939 100644 --- a/alembic/ddl/sqlite.py +++ b/alembic/ddl/sqlite.py @@ -111,6 +111,10 @@ def compare_server_default( ) if rendered_inspector_default is not None: + rendered_inspector_default = re.sub( + r"^\((.+)\)$", r"\1", rendered_inspector_default + ) + rendered_inspector_default = re.sub( r"^\"?'(.+)'\"?$", r"\1", rendered_inspector_default ) diff --git a/docs/build/unreleased/1145.rst b/docs/build/unreleased/1145.rst new file mode 100644 index 00000000..eb11a0a0 --- /dev/null +++ b/docs/build/unreleased/1145.rst @@ -0,0 +1,29 @@ +.. change:: + :tags: bug, autogenerate + :tickets: 1145 + + Fixed issue where server default compare would not work for string defaults + that contained backslashes, due to mis-rendering of these values when + comparing their contents. + + +.. change:: + :tags: bug, oracle + + Implemented basic server default comparison for the Oracle backend; + previously, Oracle's formatting of reflected defaults prevented any + matches from occurring. + +.. change:: + :tags: bug, sqlite + + Adjusted SQLite's compare server default implementation to better handle + defaults with or without parens around them, from both the reflected and + the local metadata side. + +.. change:: + :tags: bug, mssql + + Adjusted SQL Server's compare server default implementation to better + handle defaults with or without parens around them, from both the reflected + and the local metadata side. diff --git a/tests/test_autogen_diffs.py b/tests/test_autogen_diffs.py index 86b2460c..88806ff4 100644 --- a/tests/test_autogen_diffs.py +++ b/tests/test_autogen_diffs.py @@ -862,6 +862,79 @@ def test_compare_type( ) +class CompareServerDefaultTest(TestBase): + __backend__ = True + + @testing.fixture() + def connection(self): + with config.db.begin() as conn: + yield conn + + @testing.fixture() + def metadata(self, connection): + m = MetaData() + yield m + m.drop_all(connection) + + @testing.combinations( + (VARCHAR(30), text("'some default'"), text("'some new default'")), + (VARCHAR(30), "some default", "some new default"), + (VARCHAR(30), text("'//slash'"), text("'s//l//ash'")), + (Integer(), text("15"), text("20")), + (Integer(), "15", "20"), + id_="sss", + argnames="type_,default_text,new_default_text", + ) + def test_server_default_yes_positives( + self, type_, default_text, new_default_text, connection, metadata + ): + t1 = Table( + "t1", metadata, Column("x", type_, server_default=default_text) + ) + t1.create(connection) + + new_metadata = MetaData() + Table( + "t1", + new_metadata, + Column("x", type_, server_default=new_default_text), + ) + + mc = MigrationContext.configure( + connection, opts={"compare_server_default": True} + ) + + diff = api.compare_metadata(mc, new_metadata) + eq_(len(diff), 1) + eq_(diff[0][0][0], "modify_default") + + @testing.combinations( + (VARCHAR(30), text("'some default'")), + (VARCHAR(30), "some default"), + (VARCHAR(30), text("'//slash'")), + (VARCHAR(30), text("'has '' quote'")), + (Integer(), text("15")), + (Integer(), "15"), + id_="ss", + argnames="type_,default_text", + ) + def test_server_default_no_false_positives( + self, type_, default_text, connection, metadata + ): + t1 = Table( + "t1", metadata, Column("x", type_, server_default=default_text) + ) + t1.create(connection) + + mc = MigrationContext.configure( + connection, opts={"compare_server_default": True} + ) + + diff = api.compare_metadata(mc, metadata) + + assert not diff + + class CompareMetadataToInspectorTest(TestBase): __backend__ = True