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

diff --git a/alembic/autogenerate/compare.py b/alembic/autogenerate/compare.py
index c9971ea321fe09eb9f1b1271e43eb203cfb6f451..8301e34c80a69cab6fc135729cae71a3382af0c4 100644 (file)
--- 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 6a208ec6852bb22047413f81132c3d4a8d3e27ef..00c5f0ed703c9bb721b13e09621be90ada469e01 100644 (file)
--- 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 @@ class MSSQLImpl(DefaultImpl):
         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 920b70ae06dfa30e9d1856d7b5a32c98c6520388..9715c1e81a7e67ae741dbc02b75e99e41fc3c097 100644 (file)
--- 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 @@ class OracleImpl(DefaultImpl):
             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 51233326c6aebb6781184290dffcac2b60874d06..6b8c2939976750d3c27b639bc256a7c238008323 100644 (file)
--- a/alembic/ddl/sqlite.py
+++ b/alembic/ddl/sqlite.py
@@ -111,6 +111,10 @@ class SQLiteImpl(DefaultImpl):
             )
 
         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 (file)
index 0000000..eb11a0a
--- /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 86b2460ca23773befb3c0c20df2f643b305a7d50..88806ff4dd9ed924e39ffb767d38a6770be56d17 100644 (file)
--- a/tests/test_autogen_diffs.py
+++ b/tests/test_autogen_diffs.py
@@ -862,6 +862,79 @@ class CompareTypeSpecificityTest(TestBase):
         )
 
 
+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