From bd34d2e4e4daee6e25d3c8a9a43299e883cb8da3 Mon Sep 17 00:00:00 2001
From: Mike Bayer <mike_mp@zzzcomputing.com>
Date: Thu, 4 Mar 2021 10:54:03 -0500
Subject: [PATCH] Adjust mssql accommodate existing_type and type

Fixed bug where the "existing_type" parameter, which the MSSQL dialect
requires in order to change the nullability of a column in the absence of
also changing the column type, would cause an ALTER COLUMN operation to
incorrectly render a second ALTER statement without the nullability if a
new type were also present, as the MSSQL-specific contract did not
anticipate all three of "nullability", "type_" and "existing_type" being
sent at the same time.

Change-Id: Ia95b6c3e9276cc067fd773928f9678ab429d5670
Fixes: #812
---
 alembic/ddl/mssql.py          | 24 +++++++++++++++---------
 docs/build/unreleased/812.rst | 12 ++++++++++++
 tests/test_mssql.py           | 23 ++++++++++++++++++++---
 tests/test_op.py              | 25 +++++++++++++++++++++++++
 4 files changed, 72 insertions(+), 12 deletions(-)
 create mode 100644 docs/build/unreleased/812.rst

diff --git a/alembic/ddl/mssql.py b/alembic/ddl/mssql.py
index 18e09f82..8a99ee6e 100644
--- a/alembic/ddl/mssql.py
+++ b/alembic/ddl/mssql.py
@@ -75,18 +75,24 @@ class MSSQLImpl(DefaultImpl):
         **kw
     ):
 
-        if nullable is not None and existing_type is None:
-            if type_ is not None:
-                existing_type = type_
+        if nullable is not None:
+            if existing_type is None:
+                if type_ is not None:
+                    existing_type = type_
+                    # the NULL/NOT NULL alter will handle
+                    # the type alteration
+                    type_ = None
+                else:
+                    raise util.CommandError(
+                        "MS-SQL ALTER COLUMN operations "
+                        "with NULL or NOT NULL require the "
+                        "existing_type or a new type_ be passed."
+                    )
+            elif type_ is not None:
                 # the NULL/NOT NULL alter will handle
                 # the type alteration
+                existing_type = type_
                 type_ = None
-            else:
-                raise util.CommandError(
-                    "MS-SQL ALTER COLUMN operations "
-                    "with NULL or NOT NULL require the "
-                    "existing_type or a new type_ be passed."
-                )
 
         used_default = False
         if sqla_compat._server_default_is_identity(
diff --git a/docs/build/unreleased/812.rst b/docs/build/unreleased/812.rst
new file mode 100644
index 00000000..d4c80320
--- /dev/null
+++ b/docs/build/unreleased/812.rst
@@ -0,0 +1,12 @@
+.. change::
+    :tags: bug, mssql, operations
+    :tickets: 812
+
+    Fixed bug where the "existing_type" parameter, which the MSSQL dialect
+    requires in order to change the nullability of a column in the absence of
+    also changing the column type, would cause an ALTER COLUMN operation to
+    incorrectly render a second ALTER statement without the nullability if a
+    new type were also present, as the MSSQL-specific contract did not
+    anticipate all three of "nullability", "type_" and "existing_type" being
+    sent at the same time.
+
diff --git a/tests/test_mssql.py b/tests/test_mssql.py
index 0c8b3ae6..e9a74931 100644
--- a/tests/test_mssql.py
+++ b/tests/test_mssql.py
@@ -3,6 +3,7 @@
 from sqlalchemy import Column
 from sqlalchemy import exc
 from sqlalchemy import Integer
+from sqlalchemy import String
 
 from alembic import command
 from alembic import op
@@ -79,10 +80,26 @@ class OpTest(TestBase):
         op.alter_column("t", "c", new_column_name="SomeFancyName")
         context.assert_("EXEC sp_rename 't.c', [SomeFancyName], 'COLUMN'")
 
-    def test_alter_column_new_type(self):
+    @combinations((True,), (False,), argnames="pass_existing_type")
+    @combinations((True,), (False,), argnames="change_nullability")
+    def test_alter_column_type_and_nullability(
+        self, pass_existing_type, change_nullability
+    ):
         context = op_fixture("mssql")
-        op.alter_column("t", "c", type_=Integer)
-        context.assert_("ALTER TABLE t ALTER COLUMN c INTEGER")
+
+        args = dict(type_=Integer)
+        if pass_existing_type:
+            args["existing_type"] = String(15)
+
+        if change_nullability:
+            args["nullable"] = False
+
+        op.alter_column("t", "c", **args)
+
+        if change_nullability:
+            context.assert_("ALTER TABLE t ALTER COLUMN c INTEGER NOT NULL")
+        else:
+            context.assert_("ALTER TABLE t ALTER COLUMN c INTEGER")
 
     def test_alter_column_dont_touch_constraints(self):
         context = op_fixture("mssql")
diff --git a/tests/test_op.py b/tests/test_op.py
index 58a6a860..93bba287 100644
--- a/tests/test_op.py
+++ b/tests/test_op.py
@@ -462,6 +462,31 @@ class OpTest(TestBase):
             "ALTER TABLE foo.t ADD CONSTRAINT xyz CHECK (c IN (0, 1))",
         )
 
+    @combinations((True,), (False,), argnames="pass_existing_type")
+    @combinations((True,), (False,), argnames="change_nullability")
+    def test_generic_alter_column_type_and_nullability(
+        self, pass_existing_type, change_nullability
+    ):
+        # this test is also on the mssql dialect in test_mssql
+        context = op_fixture()
+
+        args = dict(type_=Integer)
+        if pass_existing_type:
+            args["existing_type"] = String(15)
+
+        if change_nullability:
+            args["nullable"] = False
+
+        op.alter_column("t", "c", **args)
+
+        if change_nullability:
+            context.assert_(
+                "ALTER TABLE t ALTER COLUMN c SET NOT NULL",
+                "ALTER TABLE t ALTER COLUMN c TYPE INTEGER",
+            )
+        else:
+            context.assert_("ALTER TABLE t ALTER COLUMN c TYPE INTEGER")
+
     def test_alter_column_schema_type_existing_type(self):
         context = op_fixture("mssql", native_boolean=False)
         op.alter_column(
-- 
2.47.2