From: Mike Bayer <mike_mp@zzzcomputing.com>
Date: Fri, 22 Jan 2016 18:04:27 +0000 (-0500)
Subject: - Repaired batch migration support for "schema" types which generate
X-Git-Tag: rel_0_8_5~14
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=9eb7f3283df4364966f6e4614b8ed709c068f1fb;p=thirdparty%2Fsqlalchemy%2Falembic.git

- Repaired batch migration support for "schema" types which generate
constraints, in particular the ``Boolean`` datatype which generates
a CHECK constraint.  Previously, an alter column operation with this
type would fail to correctly accommodate for the CHECK constraint
on change both from and to this type.  In the former case the operation
would fail entirely, in the latter, the CHECK constraint would
not get generated.  Both of these issues are repaired.
fixes #354
- Changing a schema type such as ``Boolean`` to a non-schema type would
emit a drop constraint operation which emits ``NotImplementedError`` for
the MySQL dialect.  This drop constraint operation is now skipped when
the constraint originates from a schema type.
fixes #355
---

diff --git a/alembic/__init__.py b/alembic/__init__.py
index 56f6f5e9..20108970 100644
--- a/alembic/__init__.py
+++ b/alembic/__init__.py
@@ -1,6 +1,6 @@
 from os import path
 
-__version__ = '0.8.4'
+__version__ = '0.8.5'
 
 package_dir = path.abspath(path.dirname(__file__))
 
diff --git a/alembic/ddl/mysql.py b/alembic/ddl/mysql.py
index 9db4273d..c0232cf4 100644
--- a/alembic/ddl/mysql.py
+++ b/alembic/ddl/mysql.py
@@ -74,6 +74,12 @@ class MySQLImpl(DefaultImpl):
                 )
             )
 
+    def drop_constraint(self, const):
+        if isinstance(const, schema.CheckConstraint) and const._type_bound:
+            return
+
+        super(MySQLImpl, self).drop_constraint(const)
+
     def compare_server_default(self, inspector_column,
                                metadata_column,
                                rendered_metadata_default,
diff --git a/alembic/operations/batch.py b/alembic/operations/batch.py
index b3437730..cd57355f 100644
--- a/alembic/operations/batch.py
+++ b/alembic/operations/batch.py
@@ -4,6 +4,8 @@ from sqlalchemy import types as sqltypes
 from sqlalchemy import schema as sql_schema
 from sqlalchemy.util import OrderedDict
 from .. import util
+if util.sqla_08:
+    from sqlalchemy.events import SchemaEventTarget
 from ..util.sqla_compat import _columns_for_constraint, \
     _is_type_bound, _fk_is_self_referential
 
@@ -125,6 +127,10 @@ class ApplyBatchImpl(object):
         for c in self.table.c:
             c_copy = c.copy(schema=schema)
             c_copy.unique = c_copy.index = False
+            # ensure that the type object was copied,
+            # as we may need to modify it in-place
+            if isinstance(c.type, SchemaEventTarget):
+                assert c_copy.type is not c.type
             self.columns[c.name] = c_copy
         self.named_constraints = {}
         self.unnamed_constraints = []
@@ -133,7 +139,7 @@ class ApplyBatchImpl(object):
         for const in self.table.constraints:
             if _is_type_bound(const):
                 continue
-            if const.name:
+            elif const.name:
                 self.named_constraints[const.name] = const
             else:
                 self.unnamed_constraints.append(const)
@@ -279,7 +285,22 @@ class ApplyBatchImpl(object):
 
         if type_ is not None:
             type_ = sqltypes.to_instance(type_)
+            # old type is being discarded so turn off eventing
+            # rules. Alternatively we can
+            # erase the events set up by this type, but this is simpler.
+            # we also ignore the drop_constraint that will come here from
+            # Operations.implementation_for(alter_column)
+            if isinstance(existing.type, SchemaEventTarget):
+                existing.type._create_events = \
+                    existing.type.create_constraint = False
+
             existing.type = type_
+
+            # we *dont* however set events for the new type, because
+            # alter_column is invoked from
+            # Operations.implementation_for(alter_column) which already
+            # will emit an add_constraint()
+
             existing_transfer["expr"] = cast(existing_transfer["expr"], type_)
         if nullable is not None:
             existing.nullable = nullable
@@ -313,6 +334,12 @@ class ApplyBatchImpl(object):
         try:
             del self.named_constraints[const.name]
         except KeyError:
+            if const._type_bound:
+                # type-bound constraints are only included in the new
+                # table via their type object in any case, so ignore the
+                # drop_constraint() that comes here via the
+                # Operations.implementation_for(alter_column)
+                return
             raise ValueError("No such constraint: '%s'" % const.name)
 
     def create_index(self, idx):
diff --git a/docs/build/batch.rst b/docs/build/batch.rst
index b14b8606..7cb7a995 100644
--- a/docs/build/batch.rst
+++ b/docs/build/batch.rst
@@ -192,6 +192,23 @@ them directly, which can be via the
         ):
         batch_op.add_column(Column('foo', Integer))
 
+Changing the Type of Boolean, Enum and other implicit CHECK datatypes
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+The SQLAlchemy types :class:`~sqlalchemy.types.Boolean` and
+:class:`~sqlalchemy.types.Enum` are part of a category of types known as
+"schema" types; this style of type creates other structures along with the
+type itself, most commonly (but not always) a CHECK constraint.
+
+Alembic handles dropping and creating the CHECK constraints here automatically,
+including in the case of batch mode.  When changing the type of an existing
+column, what's necessary is that the existing type be specified fully::
+
+  with self.op.batch_alter_table("some_table"):
+      batch_op.alter_column(
+          'q', type_=Integer,
+          existing_type=Boolean(create_constraint=True, constraint_name="ck1"))
+
 Including CHECK constraints
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
@@ -205,6 +222,10 @@ recreated table::
         batch_op.add_column(Column('foo', Integer))
         batch_op.drop_column('bar')
 
+Note this only includes CHECK constraints that are explicitly stated
+as part of the table definition, not the CHECK constraints that are generated
+by datatypes such as :class:`~sqlalchemy.types.Boolean` or
+:class:`~sqlalchemy.types.Enum`.
 
 Dealing with Referencing Foreign Keys
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/docs/build/changelog.rst b/docs/build/changelog.rst
index 8e66ec5c..1f8265de 100644
--- a/docs/build/changelog.rst
+++ b/docs/build/changelog.rst
@@ -3,6 +3,30 @@
 Changelog
 ==========
 
+.. changelog::
+    :version: 0.8.5
+
+    .. change::
+      :tags: bug, batch
+      :tickets: 354
+
+      Repaired batch migration support for "schema" types which generate
+      constraints, in particular the ``Boolean`` datatype which generates
+      a CHECK constraint.  Previously, an alter column operation with this
+      type would fail to correctly accommodate for the CHECK constraint
+      on change both from and to this type.  In the former case the operation
+      would fail entirely, in the latter, the CHECK constraint would
+      not get generated.  Both of these issues are repaired.
+
+    .. change::
+      :tags: bug, mysql
+      :tickets: 355
+
+      Changing a schema type such as ``Boolean`` to a non-schema type would
+      emit a drop constraint operation which emits ``NotImplementedError`` for
+      the MySQL dialect.  This drop constraint operation is now skipped when
+      the constraint originates from a schema type.
+
 .. changelog::
     :version: 0.8.4
     :released: December 15, 2015
diff --git a/tests/test_batch.py b/tests/test_batch.py
index 181c3dc6..05bd3547 100644
--- a/tests/test_batch.py
+++ b/tests/test_batch.py
@@ -258,6 +258,18 @@ class BatchApplyTest(TestBase):
                 if isinstance(const, CheckConstraint)]),
             1)
 
+    def test_change_type_schematype_to_non(self):
+        impl = self._boolean_fixture()
+        impl.alter_column('tname', 'flag', type_=Integer)
+        new_table = self._assert_impl(
+            impl, colnames=['id', 'flag'],
+            ddl_not_contains="CHECK")
+        assert new_table.c.flag.type._type_affinity is Integer
+
+        # NOTE: we can't do test_change_type_non_to_schematype
+        # at this level because the "add_constraint" part of this
+        # comes from toimpl.py, which we aren't testing here
+
     def test_rename_col_boolean_no_ck(self):
         impl = self._boolean_no_ck_fixture()
         impl.alter_column('tname', 'flag', name='bflag')
@@ -733,6 +745,48 @@ class CopyFromTest(TestBase):
             'ALTER TABLE _alembic_batch_temp RENAME TO foo'
         )
 
+    def test_change_type_from_schematype(self):
+        context = self._fixture()
+        self.table.append_column(
+            Column('y', Boolean(
+                create_constraint=True, name="ck1")))
+
+        with self.op.batch_alter_table(
+                "foo", copy_from=self.table) as batch_op:
+            batch_op.alter_column(
+                'y', type_=Integer,
+                existing_type=Boolean(
+                    create_constraint=True, name="ck1"))
+        context.assert_(
+            'CREATE TABLE _alembic_batch_temp (id INTEGER NOT NULL, '
+            'data VARCHAR(50), x INTEGER, y INTEGER, PRIMARY KEY (id))',
+            'INSERT INTO _alembic_batch_temp (id, data, x, y) SELECT foo.id, '
+            'foo.data, foo.x, CAST(foo.y AS INTEGER) AS anon_1 FROM foo',
+            'DROP TABLE foo',
+            'ALTER TABLE _alembic_batch_temp RENAME TO foo'
+        )
+
+    def test_change_type_to_schematype(self):
+        context = self._fixture()
+        self.table.append_column(
+            Column('y', Integer))
+
+        with self.op.batch_alter_table(
+                "foo", copy_from=self.table) as batch_op:
+            batch_op.alter_column(
+                'y', existing_type=Integer,
+                type_=Boolean(
+                    create_constraint=True, name="ck1"))
+        context.assert_(
+            'CREATE TABLE _alembic_batch_temp (id INTEGER NOT NULL, '
+            'data VARCHAR(50), x INTEGER, y BOOLEAN, PRIMARY KEY (id), '
+            'CONSTRAINT ck1 CHECK (y IN (0, 1)))',
+            'INSERT INTO _alembic_batch_temp (id, data, x, y) SELECT foo.id, '
+            'foo.data, foo.x, CAST(foo.y AS BOOLEAN) AS anon_1 FROM foo',
+            'DROP TABLE foo',
+            'ALTER TABLE _alembic_batch_temp RENAME TO foo'
+        )
+
     def test_create_drop_index_w_always(self):
         context = self._fixture()
         with self.op.batch_alter_table(
@@ -892,6 +946,69 @@ class BatchRoundTripTest(TestBase):
         t.create(self.conn)
         return t
 
+    def _boolean_fixture(self):
+        t = Table(
+            'hasbool', self.metadata,
+            Column('x', Boolean(create_constraint=True, name='ck1')),
+            Column('y', Integer)
+        )
+        t.create(self.conn)
+
+    def _int_to_boolean_fixture(self):
+        t = Table(
+            'hasbool', self.metadata,
+            Column('x', Integer)
+        )
+        t.create(self.conn)
+
+    def test_change_type_boolean_to_int(self):
+        self._boolean_fixture()
+        with self.op.batch_alter_table(
+                "hasbool"
+        ) as batch_op:
+            batch_op.alter_column(
+                'x', type_=Integer, existing_type=Boolean(
+                    create_constraint=True, name='ck1'))
+        insp = Inspector.from_engine(config.db)
+
+        eq_(
+            [c['type']._type_affinity for c in insp.get_columns('hasbool')
+             if c['name'] == 'x'],
+            [Integer]
+        )
+
+    def test_drop_col_schematype(self):
+        self._boolean_fixture()
+        with self.op.batch_alter_table(
+                "hasbool"
+        ) as batch_op:
+            batch_op.drop_column('x')
+        insp = Inspector.from_engine(config.db)
+
+        assert 'x' not in (c['name'] for c in insp.get_columns('hasbool'))
+
+    def test_change_type_int_to_boolean(self):
+        self._int_to_boolean_fixture()
+        with self.op.batch_alter_table(
+                "hasbool"
+        ) as batch_op:
+            batch_op.alter_column(
+                'x', type_=Boolean(create_constraint=True, name='ck1'))
+        insp = Inspector.from_engine(config.db)
+
+        if exclusions.against(config, "sqlite"):
+            eq_(
+                [c['type']._type_affinity for
+                 c in insp.get_columns('hasbool') if c['name'] == 'x'],
+                [Boolean]
+            )
+        elif exclusions.against(config, "mysql"):
+            eq_(
+                [c['type']._type_affinity for
+                 c in insp.get_columns('hasbool') if c['name'] == 'x'],
+                [Integer]
+            )
+
     def tearDown(self):
         self.metadata.drop_all(self.conn)
         self.conn.close()
@@ -1267,3 +1384,13 @@ class BatchRoundTripPostgresqlTest(BatchRoundTripTest):
 
     def test_create_drop_index(self):
         super(BatchRoundTripPostgresqlTest, self).test_create_drop_index()
+
+    @exclusions.fails()
+    def test_change_type_int_to_boolean(self):
+        super(BatchRoundTripPostgresqlTest, self).\
+            test_change_type_int_to_boolean()
+
+    @exclusions.fails()
+    def test_change_type_boolean_to_int(self):
+        super(BatchRoundTripPostgresqlTest, self).\
+            test_change_type_boolean_to_int()
diff --git a/tests/test_mysql.py b/tests/test_mysql.py
index 2dc88387..7600ebf0 100644
--- a/tests/test_mysql.py
+++ b/tests/test_mysql.py
@@ -1,4 +1,4 @@
-from sqlalchemy import Integer, func
+from sqlalchemy import Integer, func, Boolean
 from alembic.testing.fixtures import TestBase
 from alembic.testing import config
 from sqlalchemy import TIMESTAMP, MetaData, Table, Column, text
@@ -102,6 +102,17 @@ class MySQLOpTest(TestBase):
             'ALTER TABLE t ALTER COLUMN c DROP DEFAULT'
         )
 
+    def test_alter_column_remove_schematype(self):
+        context = op_fixture('mysql')
+        op.alter_column(
+            "t", "c",
+            type_=Integer,
+            existing_type=Boolean(create_constraint=True, name="ck1"),
+            server_default=None)
+        context.assert_(
+            'ALTER TABLE t MODIFY c INTEGER NULL'
+        )
+
     def test_alter_column_modify_default(self):
         context = op_fixture('mysql')
         # notice we dont need the existing type on this one...