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

@@ -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:

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(

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")
 

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
             )

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.

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