Sort per-column comparisons in metadata order

Improved the rendering of ``op.add_column()`` operations when adding
multiple columns to an existing table, so that the order of these
statements matches the order in which the columns were declared in the
application's table metadata. Previously the added columns were being
sorted alphabetically.

Change-Id: Ia2a603c934ab101d102d63e715e92f9be9b747a7
Fixes: #827

diff --git a/alembic/autogenerate/compare.py b/alembic/autogenerate/compare.py
index 7ae7b6191f762a9b18461bad14607426851b305f..dd71515ee894efdf44bb4fc9a1ad571a2c69298a 100644 (file)
--- a/alembic/autogenerate/compare.py
+++ b/alembic/autogenerate/compare.py
@@ -295,17 +295,20 @@ def _compare_columns(
     inspector,
 ):
     name = "%s.%s" % (schema, tname) if schema else tname
-    metadata_cols_by_name = dict(
-        (c.name, c) for c in metadata_table.c if not c.system
+    metadata_col_names = OrderedSet(
+        c.name for c in metadata_table.c if not c.system
     )
-    conn_col_names = dict(
-        (c.name, c)
+    metadata_cols_by_name = {
+        c.name: c for c in metadata_table.c if not c.system
+    }
+
+    conn_col_names = {
+        c.name: c
         for c in conn_table.c
         if autogen_context.run_name_filters(
             c.name, "column", {"table_name": tname, "schema_name": schema}
         )
-    )
-    metadata_col_names = OrderedSet(sorted(metadata_cols_by_name))
+    }
 
     for cname in metadata_col_names.difference(conn_col_names):
         if autogen_context.run_object_filters(

diff --git a/docs/build/unreleased/827.rst b/docs/build/unreleased/827.rst
new file mode 100644 (file)
index 0000000..d92fff8
--- /dev/null
+++ b/docs/build/unreleased/827.rst
@@ -0,0 +1,10 @@
+.. change::
+    :tags: bug, autogenerate
+    :tickets: 827
+
+    Improved the rendering of ``op.add_column()`` operations when adding
+    multiple columns to an existing table, so that the order of these
+    statements matches the order in which the columns were declared in the
+    application's table metadata. Previously the added columns were being
+    sorted alphabetically.
+

diff --git a/tests/test_autogen_composition.py b/tests/test_autogen_composition.py
index c37ecbaa1e5d0a1541eab7203099d227ab8ce8ce..d7b001e8ed0379dbadcb00db17cc530e474a26df 100644 (file)
--- a/tests/test_autogen_composition.py
+++ b/tests/test_autogen_composition.py
@@ -1,5 +1,12 @@
 import re
 
+from sqlalchemy import Column
+from sqlalchemy import Integer
+from sqlalchemy import MetaData
+from sqlalchemy import String
+from sqlalchemy import Table
+from sqlalchemy.sql.sqltypes import DateTime
+
 from alembic import autogenerate
 from alembic.migration import MigrationContext
 from alembic.testing import eq_
@@ -97,13 +104,13 @@ nullable=True))
                nullable=True,
                existing_server_default=sa.text('0'))
     op.create_foreign_key(None, 'order', 'user', ['user_id'], ['id'])
+    op.alter_column('user', 'name',
+               existing_type=sa.VARCHAR(length=50),
+               nullable=False)
     op.alter_column('user', 'a1',
                existing_type=sa.TEXT(),
                server_default='x',
                existing_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 ###""",
@@ -115,13 +122,13 @@ nullable=True))
     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)
     op.alter_column('user', 'a1',
                existing_type=sa.TEXT(),
                server_default=None,
                existing_nullable=True)
+    op.alter_column('user', 'name',
+               existing_type=sa.VARCHAR(length=50),
+               nullable=True)
     op.drop_constraint(None, 'order', type_='foreignkey')
     op.alter_column('order', 'amount',
                existing_type=sa.Numeric(precision=10, scale=2),
@@ -173,13 +180,13 @@ nullable=True))
         batch_op.create_foreign_key(None, 'user', ['user_id'], ['id'])
 
     with op.batch_alter_table('user', schema=None) as batch_op:
+        batch_op.alter_column('name',
+               existing_type=sa.VARCHAR(length=50),
+               nullable=False)
         batch_op.alter_column('a1',
                existing_type=sa.TEXT(),
                server_default='x',
                existing_nullable=True)
-        batch_op.alter_column('name',
-               existing_type=sa.VARCHAR(length=50),
-               nullable=False)
         batch_op.drop_index('pw_idx')
         batch_op.drop_column('pw')
 
@@ -192,13 +199,13 @@ nullable=True))
     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', ['pw'], unique=False)
-        batch_op.alter_column('name',
-               existing_type=sa.VARCHAR(length=50),
-               nullable=True)
         batch_op.alter_column('a1',
                existing_type=sa.TEXT(),
                server_default=None,
                existing_nullable=True)
+        batch_op.alter_column('name',
+               existing_type=sa.VARCHAR(length=50),
+               nullable=True)
 
     with op.batch_alter_table('order', schema=None) as batch_op:
         batch_op.drop_constraint(None, type_='foreignkey')
@@ -245,6 +252,60 @@ nullable=True))
         )
 
 
+class AddColumnOrderTest(AutogenTest, TestBase):
+    @classmethod
+    def _get_db_schema(cls):
+        m = MetaData()
+
+        Table(
+            "user",
+            m,
+            Column("id", Integer, primary_key=True),
+            Column("name", String(50)),
+        )
+
+        return m
+
+    @classmethod
+    def _get_model_schema(cls):
+        m = MetaData()
+
+        Table(
+            "user",
+            m,
+            Column("id", Integer, primary_key=True),
+            Column("name", String(50)),
+            Column("username", String(50)),
+            Column("password_hash", String(32)),
+            Column("timestamp", DateTime),
+        )
+
+        return m
+
+    def test_render_add_columns(self):
+        """test #827"""
+
+        template_args = {}
+        autogenerate._render_migration_diffs(self.context, template_args)
+        eq_(
+            re.sub(r"u'", "'", template_args["upgrades"]),
+            """# ### commands auto generated by Alembic - please adjust! ###
+    op.add_column('user', sa.Column('username', sa.String(length=50), nullable=True))
+    op.add_column('user', sa.Column('password_hash', sa.String(length=32), nullable=True))
+    op.add_column('user', sa.Column('timestamp', sa.DateTime(), nullable=True))
+    # ### end Alembic commands ###""",  # noqa E501
+        )
+
+        eq_(
+            re.sub(r"u'", "'", template_args["downgrades"]),
+            """# ### commands auto generated by Alembic - please adjust! ###
+    op.drop_column('user', 'timestamp')
+    op.drop_column('user', 'password_hash')
+    op.drop_column('user', 'username')
+    # ### end Alembic commands ###""",
+        )
+
+
 class AutogenerateDiffTestWSchema(ModelOne, AutogenTest, TestBase):
     __only_on__ = "postgresql"
     schema = "test_schema"
@@ -318,15 +379,15 @@ schema='%(schema)s')
                schema='%(schema)s')
     op.create_foreign_key(None, 'order', 'user', ['user_id'], ['id'], \
 source_schema='%(schema)s', referent_schema='%(schema)s')
+    op.alter_column('user', 'name',
+               existing_type=sa.VARCHAR(length=50),
+               nullable=False,
+               schema='%(schema)s')
     op.alter_column('user', 'a1',
                existing_type=sa.TEXT(),
                server_default='x',
                existing_nullable=True,
                schema='%(schema)s')
-    op.alter_column('user', 'name',
-               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 ###"""
@@ -339,15 +400,15 @@ source_schema='%(schema)s', referent_schema='%(schema)s')
     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,
-               schema='%(schema)s')
     op.alter_column('user', 'a1',
                existing_type=sa.TEXT(),
                server_default=None,
                existing_nullable=True,
                schema='%(schema)s')
+    op.alter_column('user', 'name',
+               existing_type=sa.VARCHAR(length=50),
+               nullable=True,
+               schema='%(schema)s')
     op.drop_constraint(None, 'order', schema='%(schema)s', type_='foreignkey')
     op.alter_column('order', 'amount',
                existing_type=sa.Numeric(precision=10, scale=2),

diff --git a/tests/test_autogen_diffs.py b/tests/test_autogen_diffs.py
index 94a916736099527a60757b575d0d44312f86101b..2a1d762bf433b8e72cbe4b163bd600bdc7ee7c82 100644 (file)
--- a/tests/test_autogen_diffs.py
+++ b/tests/test_autogen_diffs.py
@@ -462,15 +462,15 @@ class AutogenerateDiffTest(ModelOne, AutogenTest, TestBase):
             diffs[6], "add_fk", "order", ["user_id"], "user", ["id"]
         )
 
-        eq_(diffs[7][0][0], "modify_default")
-        eq_(diffs[7][0][1], None)
-        eq_(diffs[7][0][2], "user")
-        eq_(diffs[7][0][3], "a1")
-        eq_(diffs[7][0][6].arg, "x")
+        eq_(diffs[7][0][0], "modify_nullable")
+        eq_(diffs[7][0][5], True)
+        eq_(diffs[7][0][6], False)
 
-        eq_(diffs[8][0][0], "modify_nullable")
-        eq_(diffs[8][0][5], True)
-        eq_(diffs[8][0][6], False)
+        eq_(diffs[8][0][0], "modify_default")
+        eq_(diffs[8][0][1], None)
+        eq_(diffs[8][0][2], "user")
+        eq_(diffs[8][0][3], "a1")
+        eq_(diffs[8][0][6].arg, "x")
 
         eq_(diffs[9][0], "remove_index")
         eq_(diffs[9][1].name, "pw_idx")
@@ -777,15 +777,15 @@ class AutogenerateDiffTestWSchema(ModelOne, AutogenTest, TestBase):
             source_schema=config.test_schema,
         )
 
-        eq_(diffs[7][0][0], "modify_default")
-        eq_(diffs[7][0][1], self.schema)
-        eq_(diffs[7][0][2], "user")
-        eq_(diffs[7][0][3], "a1")
-        eq_(diffs[7][0][6].arg, "x")
+        eq_(diffs[7][0][0], "modify_nullable")
+        eq_(diffs[7][0][5], True)
+        eq_(diffs[7][0][6], False)
 
-        eq_(diffs[8][0][0], "modify_nullable")
-        eq_(diffs[8][0][5], True)
-        eq_(diffs[8][0][6], False)
+        eq_(diffs[8][0][0], "modify_default")
+        eq_(diffs[8][0][1], self.schema)
+        eq_(diffs[8][0][2], "user")
+        eq_(diffs[8][0][3], "a1")
+        eq_(diffs[8][0][6].arg, "x")
 
         eq_(diffs[9][0], "remove_index")
         eq_(diffs[9][1].name, "pw_idx")
@@ -1558,15 +1558,15 @@ class CompareMetadataTest(ModelOne, AutogenTest, TestBase):
             diffs[6], "add_fk", "order", ["user_id"], "user", ["id"]
         )
 
-        eq_(diffs[7][0][0], "modify_default")
-        eq_(diffs[7][0][1], None)
-        eq_(diffs[7][0][2], "user")
-        eq_(diffs[7][0][3], "a1")
-        eq_(diffs[7][0][6].arg, "x")
+        eq_(diffs[7][0][0], "modify_nullable")
+        eq_(diffs[7][0][5], True)
+        eq_(diffs[7][0][6], False)
 
-        eq_(diffs[8][0][0], "modify_nullable")
-        eq_(diffs[8][0][5], True)
-        eq_(diffs[8][0][6], False)
+        eq_(diffs[8][0][0], "modify_default")
+        eq_(diffs[8][0][1], None)
+        eq_(diffs[8][0][2], "user")
+        eq_(diffs[8][0][3], "a1")
+        eq_(diffs[8][0][6].arg, "x")
 
         eq_(diffs[9][0], "remove_index")
         eq_(diffs[9][1].name, "pw_idx")