- A change in the ordering when columns and constraints are dropped;

autogenerate will now place the "drop constraint" calls *before*
the "drop column" calls, so that columns involved in those constraints
still exist when the constraint is dropped.
fixes #247

3 files changed, 120 insertions, 69 deletions

diff --git a/alembic/autogenerate/compare.py b/alembic/autogenerate/compare.py
index b603212..bba6740 100644
--- a/alembic/autogenerate/compare.py
+++ b/alembic/autogenerate/compare.py
@@ -5,6 +5,7 @@ from .. import compat
 from sqlalchemy.util import OrderedSet
 import re
 from .render import _user_defined_render
+import contextlib
 
 log = logging.getLogger(__name__)
 
@@ -102,14 +103,15 @@ def _compare_tables(conn_table_names, metadata_table_names,
         if _run_filters(
                 metadata_table, tname, "table", False,
                 conn_table, object_filters):
-            _compare_columns(s, tname, object_filters,
-                             conn_table,
-                             metadata_table,
-                             diffs, autogen_context, inspector)
-            _compare_indexes_and_uniques(s, tname, object_filters,
-                                         conn_table,
-                                         metadata_table,
-                                         diffs, autogen_context, inspector)
+            with _compare_columns(
+                s, tname, object_filters,
+                conn_table,
+                metadata_table,
+                    diffs, autogen_context, inspector):
+                _compare_indexes_and_uniques(s, tname, object_filters,
+                                             conn_table,
+                                             metadata_table,
+                                             diffs, autogen_context, inspector)
 
     # TODO:
     # table constraints
@@ -131,6 +133,7 @@ def _make_unique_constraint(params, conn_table):
     )
 
 
+@contextlib.contextmanager
 def _compare_columns(schema, tname, object_filters, conn_table, metadata_table,
                      diffs, autogen_context, inspector):
     name = '%s.%s' % (schema, tname) if schema else tname
@@ -146,14 +149,6 @@ def _compare_columns(schema, tname, object_filters, conn_table, metadata_table,
             )
             log.info("Detected added column '%s.%s'", name, cname)
 
-    for cname in set(conn_col_names).difference(metadata_col_names):
-        if _run_filters(conn_table.c[cname], cname,
-                        "column", True, None, object_filters):
-            diffs.append(
-                ("remove_column", schema, tname, conn_table.c[cname])
-            )
-            log.info("Detected removed column '%s.%s'", name, cname)
-
     for colname in metadata_col_names.intersection(conn_col_names):
         metadata_col = metadata_cols_by_name[colname]
         conn_col = conn_table.c[colname]
@@ -182,6 +177,17 @@ def _compare_columns(schema, tname, object_filters, conn_table, metadata_table,
         if col_diff:
             diffs.append(col_diff)
 
+    yield
+
+    for cname in set(conn_col_names).difference(metadata_col_names):
+        if _run_filters(conn_table.c[cname], cname,
+                        "column", True, None, object_filters):
+            diffs.append(
+                ("remove_column", schema, tname, conn_table.c[cname])
+            )
+            log.info("Detected removed column '%s.%s'", name, cname)
+
+
 
 class _constraint_sig(object):
 
diff --git a/docs/build/changelog.rst b/docs/build/changelog.rst
index 3cc3026..0fb0bb4 100644
--- a/docs/build/changelog.rst
+++ b/docs/build/changelog.rst
@@ -57,6 +57,15 @@ Changelog
           :ref:`batch_migrations`
 
     .. change::
+      :tags: bug, autogenerate
+      :tickets: 247
+
+      A change in the ordering when columns and constraints are dropped;
+      autogenerate will now place the "drop constraint" calls *before*
+      the "drop column" calls, so that columns involved in those constraints
+      still exist when the constraint is dropped.
+
+    .. change::
       :tags: feature, commands
 
       New commands added: ``alembic show``, ``alembic heads`` and
diff --git a/tests/test_autogenerate.py b/tests/test_autogenerate.py
index e9951a7..60b3a3e 100644
--- a/tests/test_autogenerate.py
+++ b/tests/test_autogenerate.py
@@ -2,7 +2,7 @@ import re
 import sys
 
 from sqlalchemy import MetaData, Column, Table, Integer, String, Text, \
-    Numeric, CHAR, ForeignKey, INTEGER, \
+    Numeric, CHAR, ForeignKey, INTEGER, Index, UniqueConstraint, \
     TypeDecorator, CheckConstraint, text, PrimaryKeyConstraint
 from sqlalchemy.types import NULLTYPE
 from sqlalchemy.engine.reflection import Inspector
@@ -320,7 +320,8 @@ class ModelOne(object):
               Column('id', Integer, primary_key=True),
               Column('name', String(50)),
               Column('a1', Text),
-              Column("pw", String(50))
+              Column("pw", String(50)),
+              Index('pw_idx', 'pw')
               )
 
         Table('address', m,
@@ -358,6 +359,7 @@ class ModelOne(object):
               Column('id', Integer, primary_key=True),
               Column('email_address', String(100), nullable=False),
               Column('street', String(50)),
+              UniqueConstraint('email_address', name="uq_email")
               )
 
         Table('order', m,
@@ -405,20 +407,20 @@ class AutogenerateDiffTest(ModelOne, AutogenTest, TestBase):
         eq_(diffs[2][2], "address")
         eq_(diffs[2][3], metadata.tables['address'].c.street)
 
-        eq_(diffs[3][0], "add_column")
-        eq_(diffs[3][1], None)
-        eq_(diffs[3][2], "order")
-        eq_(diffs[3][3], metadata.tables['order'].c.user_id)
+        eq_(diffs[3][0], "add_constraint")
+        eq_(diffs[3][1].name, "uq_email")
 
-        eq_(diffs[4][0][0], "modify_type")
-        eq_(diffs[4][0][1], None)
-        eq_(diffs[4][0][2], "order")
-        eq_(diffs[4][0][3], "amount")
-        eq_(repr(diffs[4][0][5]), "NUMERIC(precision=8, scale=2)")
-        eq_(repr(diffs[4][0][6]), "Numeric(precision=10, scale=2)")
+        eq_(diffs[4][0], "add_column")
+        eq_(diffs[4][1], None)
+        eq_(diffs[4][2], "order")
+        eq_(diffs[4][3], metadata.tables['order'].c.user_id)
 
-        eq_(diffs[5][0], 'remove_column')
-        eq_(diffs[5][3].name, 'pw')
+        eq_(diffs[5][0][0], "modify_type")
+        eq_(diffs[5][0][1], None)
+        eq_(diffs[5][0][2], "order")
+        eq_(diffs[5][0][3], "amount")
+        eq_(repr(diffs[5][0][5]), "NUMERIC(precision=8, scale=2)")
+        eq_(repr(diffs[5][0][6]), "Numeric(precision=10, scale=2)")
 
         eq_(diffs[6][0][0], "modify_default")
         eq_(diffs[6][0][1], None)
@@ -430,6 +432,12 @@ class AutogenerateDiffTest(ModelOne, AutogenTest, TestBase):
         eq_(diffs[7][0][5], True)
         eq_(diffs[7][0][6], False)
 
+        eq_(diffs[8][0], 'remove_index')
+        eq_(diffs[8][1].name, 'pw_idx')
+
+        eq_(diffs[9][0], 'remove_column')
+        eq_(diffs[9][3].name, 'pw')
+
     def test_render_nothing(self):
         context = MigrationContext.configure(
             connection=self.bind.connect(),
@@ -501,13 +509,13 @@ class AutogenerateDiffTest(ModelOne, AutogenTest, TestBase):
     op.drop_table('extra')
     op.add_column('address', sa.Column('street', sa.String(length=50), \
 nullable=True))
+    op.create_unique_constraint('uq_email', 'address', ['email_address'])
     op.add_column('order', sa.Column('user_id', sa.Integer(), nullable=True))
     op.alter_column('order', 'amount',
                existing_type=sa.NUMERIC(precision=8, scale=2),
                type_=sa.Numeric(precision=10, scale=2),
                nullable=True,
                existing_server_default=sa.text('0'))
-    op.drop_column('user', 'pw')
     op.alter_column('user', 'a1',
                existing_type=sa.TEXT(),
                server_default='x',
@@ -515,10 +523,15 @@ nullable=True))
     op.alter_column('user', 'name',
                existing_type=sa.VARCHAR(length=50),
                nullable=False)
+    op.drop_index('pw_idx', table_name='user')
+    op.drop_column('user', 'pw')
     ### end Alembic commands ###""")
 
         eq_(re.sub(r"u'", "'", template_args['downgrades']),
             """### commands auto generated by Alembic - please adjust! ###
+    op.add_column('user', sa.Column('pw', sa.VARCHAR(length=50), \
+nullable=True))
+    op.create_index('pw_idx', 'user', ['pw'], unique=False)
     op.alter_column('user', 'name',
                existing_type=sa.VARCHAR(length=50),
                nullable=True)
@@ -526,14 +539,13 @@ nullable=True))
                existing_type=sa.TEXT(),
                server_default=None,
                existing_nullable=True)
-    op.add_column('user', sa.Column('pw', sa.VARCHAR(length=50), \
-nullable=True))
     op.alter_column('order', 'amount',
                existing_type=sa.Numeric(precision=10, scale=2),
                type_=sa.NUMERIC(precision=8, scale=2),
                nullable=False,
                existing_server_default=sa.text('0'))
     op.drop_column('order', 'user_id')
+    op.drop_constraint('uq_email', 'address')
     op.drop_column('address', 'street')
     op.create_table('extra',
     sa.Column('x', sa.CHAR(), nullable=True),
@@ -564,6 +576,7 @@ nullable=True))
     op.drop_table('extra')
     with op.batch_alter_table('address', schema=None) as batch_op:
         batch_op.add_column(sa.Column('street', sa.String(length=50), nullable=True))
+        batch_op.create_unique_constraint('uq_email', ['email_address'])
 
     with op.batch_alter_table('order', schema=None) as batch_op:
         batch_op.add_column(sa.Column('user_id', sa.Integer(), nullable=True))
@@ -574,7 +587,6 @@ nullable=True))
                existing_server_default=sa.text('0'))
 
     with op.batch_alter_table('user', schema=None) as batch_op:
-        batch_op.drop_column('pw')
         batch_op.alter_column('a1',
                existing_type=sa.TEXT(),
                server_default='x',
@@ -582,12 +594,16 @@ nullable=True))
         batch_op.alter_column('name',
                existing_type=sa.VARCHAR(length=50),
                nullable=False)
+        batch_op.drop_index('pw_idx', table_name='user')
+        batch_op.drop_column('pw')
 
     ### end Alembic commands ###""")
 
         eq_(re.sub(r"u'", "'", template_args['downgrades']),
             """### commands auto generated by Alembic - please adjust! ###
     with op.batch_alter_table('user', schema=None) as batch_op:
+        batch_op.add_column(sa.Column('pw', sa.VARCHAR(length=50), nullable=True))
+        batch_op.create_index('pw_idx', 'user', ['pw'], unique=False)
         batch_op.alter_column('name',
                existing_type=sa.VARCHAR(length=50),
                nullable=True)
@@ -595,7 +611,6 @@ nullable=True))
                existing_type=sa.TEXT(),
                server_default=None,
                existing_nullable=True)
-        batch_op.add_column(sa.Column('pw', sa.VARCHAR(length=50), nullable=True))
 
     with op.batch_alter_table('order', schema=None) as batch_op:
         batch_op.alter_column('amount',
@@ -606,6 +621,7 @@ nullable=True))
         batch_op.drop_column('user_id')
 
     with op.batch_alter_table('address', schema=None) as batch_op:
+        batch_op.drop_constraint('uq_email')
         batch_op.drop_column('street')
 
     op.create_table('extra',
@@ -772,20 +788,20 @@ class AutogenerateDiffTestWSchema(ModelOne, AutogenTest, TestBase):
         eq_(diffs[2][2], "address")
         eq_(diffs[2][3], metadata.tables['%s.address' % self.schema].c.street)
 
-        eq_(diffs[3][0], "add_column")
-        eq_(diffs[3][1], self.schema)
-        eq_(diffs[3][2], "order")
-        eq_(diffs[3][3], metadata.tables['%s.order' % self.schema].c.user_id)
+        eq_(diffs[3][0], "add_constraint")
+        eq_(diffs[3][1].name, "uq_email")
 
-        eq_(diffs[4][0][0], "modify_type")
-        eq_(diffs[4][0][1], self.schema)
-        eq_(diffs[4][0][2], "order")
-        eq_(diffs[4][0][3], "amount")
-        eq_(repr(diffs[4][0][5]), "NUMERIC(precision=8, scale=2)")
-        eq_(repr(diffs[4][0][6]), "Numeric(precision=10, scale=2)")
+        eq_(diffs[4][0], "add_column")
+        eq_(diffs[4][1], self.schema)
+        eq_(diffs[4][2], "order")
+        eq_(diffs[4][3], metadata.tables['%s.order' % self.schema].c.user_id)
 
-        eq_(diffs[5][0], 'remove_column')
-        eq_(diffs[5][3].name, 'pw')
+        eq_(diffs[5][0][0], "modify_type")
+        eq_(diffs[5][0][1], self.schema)
+        eq_(diffs[5][0][2], "order")
+        eq_(diffs[5][0][3], "amount")
+        eq_(repr(diffs[5][0][5]), "NUMERIC(precision=8, scale=2)")
+        eq_(repr(diffs[5][0][6]), "Numeric(precision=10, scale=2)")
 
         eq_(diffs[6][0][0], "modify_default")
         eq_(diffs[6][0][1], self.schema)
@@ -797,6 +813,12 @@ class AutogenerateDiffTestWSchema(ModelOne, AutogenTest, TestBase):
         eq_(diffs[7][0][5], True)
         eq_(diffs[7][0][6], False)
 
+        eq_(diffs[8][0], 'remove_index')
+        eq_(diffs[8][1].name, 'pw_idx')
+
+        eq_(diffs[9][0], 'remove_column')
+        eq_(diffs[9][3].name, 'pw')
+
     def test_render_nothing(self):
         context = MigrationContext.configure(
             connection=self.bind.connect(),
@@ -848,6 +870,8 @@ class AutogenerateDiffTestWSchema(ModelOne, AutogenTest, TestBase):
     op.drop_table('extra', schema='%(schema)s')
     op.add_column('address', sa.Column('street', sa.String(length=50), \
 nullable=True), schema='%(schema)s')
+    op.create_unique_constraint('uq_email', 'address', ['email_address'], \
+schema='test_schema')
     op.add_column('order', sa.Column('user_id', sa.Integer(), nullable=True), \
 schema='%(schema)s')
     op.alter_column('order', 'amount',
@@ -856,7 +880,6 @@ schema='%(schema)s')
                nullable=True,
                existing_server_default=sa.text('0'),
                schema='%(schema)s')
-    op.drop_column('user', 'pw', schema='%(schema)s')
     op.alter_column('user', 'a1',
                existing_type=sa.TEXT(),
                server_default='x',
@@ -866,10 +889,15 @@ schema='%(schema)s')
                existing_type=sa.VARCHAR(length=50),
                nullable=False,
                schema='%(schema)s')
+    op.drop_index('pw_idx', table_name='user', schema='test_schema')
+    op.drop_column('user', 'pw', schema='%(schema)s')
     ### end Alembic commands ###""" % {"schema": self.schema})
 
         eq_(re.sub(r"u'", "'", template_args['downgrades']),
             """### commands auto generated by Alembic - please adjust! ###
+    op.add_column('user', sa.Column('pw', sa.VARCHAR(length=50), \
+autoincrement=False, nullable=True), schema='%(schema)s')
+    op.create_index('pw_idx', 'user', ['pw'], unique=False, schema='%(schema)s')
     op.alter_column('user', 'name',
                existing_type=sa.VARCHAR(length=50),
                nullable=True,
@@ -879,8 +907,6 @@ schema='%(schema)s')
                server_default=None,
                existing_nullable=True,
                schema='%(schema)s')
-    op.add_column('user', sa.Column('pw', sa.VARCHAR(length=50), \
-autoincrement=False, nullable=True), schema='%(schema)s')
     op.alter_column('order', 'amount',
                existing_type=sa.Numeric(precision=10, scale=2),
                type_=sa.NUMERIC(precision=8, scale=2),
@@ -888,6 +914,7 @@ autoincrement=False, nullable=True), schema='%(schema)s')
                existing_server_default=sa.text('0'),
                schema='%(schema)s')
     op.drop_column('order', 'user_id', schema='%(schema)s')
+    op.drop_constraint('uq_email', 'address', schema='test_schema')
     op.drop_column('address', 'street', schema='%(schema)s')
     op.create_table('extra',
     sa.Column('x', sa.CHAR(length=1), autoincrement=False, nullable=True),
@@ -1162,20 +1189,20 @@ class CompareMetadataTest(ModelOne, AutogenTest, TestBase):
         eq_(diffs[2][2], "address")
         eq_(diffs[2][3], metadata.tables['address'].c.street)
 
-        eq_(diffs[3][0], "add_column")
-        eq_(diffs[3][1], None)
-        eq_(diffs[3][2], "order")
-        eq_(diffs[3][3], metadata.tables['order'].c.user_id)
+        eq_(diffs[3][0], "add_constraint")
+        eq_(diffs[3][1].name, "uq_email")
 
-        eq_(diffs[4][0][0], "modify_type")
-        eq_(diffs[4][0][1], None)
-        eq_(diffs[4][0][2], "order")
-        eq_(diffs[4][0][3], "amount")
-        eq_(repr(diffs[4][0][5]), "NUMERIC(precision=8, scale=2)")
-        eq_(repr(diffs[4][0][6]), "Numeric(precision=10, scale=2)")
+        eq_(diffs[4][0], "add_column")
+        eq_(diffs[4][1], None)
+        eq_(diffs[4][2], "order")
+        eq_(diffs[4][3], metadata.tables['order'].c.user_id)
 
-        eq_(diffs[5][0], 'remove_column')
-        eq_(diffs[5][3].name, 'pw')
+        eq_(diffs[5][0][0], "modify_type")
+        eq_(diffs[5][0][1], None)
+        eq_(diffs[5][0][2], "order")
+        eq_(diffs[5][0][3], "amount")
+        eq_(repr(diffs[5][0][5]), "NUMERIC(precision=8, scale=2)")
+        eq_(repr(diffs[5][0][6]), "Numeric(precision=10, scale=2)")
 
         eq_(diffs[6][0][0], "modify_default")
         eq_(diffs[6][0][1], None)
@@ -1187,6 +1214,12 @@ class CompareMetadataTest(ModelOne, AutogenTest, TestBase):
         eq_(diffs[7][0][5], True)
         eq_(diffs[7][0][6], False)
 
+        eq_(diffs[8][0], 'remove_index')
+        eq_(diffs[8][1].name, 'pw_idx')
+
+        eq_(diffs[9][0], 'remove_column')
+        eq_(diffs[9][3].name, 'pw')
+
     def test_compare_metadata_include_object(self):
         metadata = self.m2
 
@@ -1284,11 +1317,14 @@ class PGCompareMetaData(ModelOne, AutogenTest, TestBase):
         eq_(diffs[2][2], "address")
         eq_(diffs[2][3], metadata.tables['test_schema.address'].c.street)
 
-        eq_(diffs[3][0], "add_column")
-        eq_(diffs[3][1], "test_schema")
-        eq_(diffs[3][2], "order")
-        eq_(diffs[3][3], metadata.tables['test_schema.order'].c.user_id)
+        eq_(diffs[3][0], "add_constraint")
+        eq_(diffs[3][1].name, "uq_email")
+
+        eq_(diffs[4][0], "add_column")
+        eq_(diffs[4][1], "test_schema")
+        eq_(diffs[4][2], "order")
+        eq_(diffs[4][3], metadata.tables['test_schema.order'].c.user_id)
 
-        eq_(diffs[4][0][0], 'modify_nullable')
-        eq_(diffs[4][0][5], False)
-        eq_(diffs[4][0][6], True)
+        eq_(diffs[5][0][0], 'modify_nullable')
+        eq_(diffs[5][0][5], False)
+        eq_(diffs[5][0][6], True)