render drops for named objects before creates to accommodate for name conflicts

[CompPhy]

Describe the bug
We have a table that is currently un-versioned, and inconsistent. When running autogenerate against this table, there is an index that is changing case. As per MySQL documentation, see link below, "Column, index, stored routine, and event names are not case-sensitive on any platform, nor are column aliases.". However, alembic is trying to create the new index first, before dropping the old one, which results in a "Duplicate key name" error.

https://dev.mysql.com/doc/refman/5.7/en/identifier-case-sensitivity.html

Expected behavior
I think there are two options here. The first is probably the better option. Even if the engine ignores case, there are good reasons that developers use things like "camel case" in naming conventions, and we want the table definitions to be consistent with this.

1. The expected behaviour here should be that alembic does the drop operation first, and then the create. This way the index can be renamed properly. I was able to work around this error by manually swapping the statements in the autogenerate output. However, I don't think it's acceptable to have to look for these over manually every time. We have hundreds of tables to audit here and shouldn't have to manually verify every index in the script.

2. The other option is to just ignore case, like MySQL does, and do nothing to the index. Although, in this case, the index will not match naming that's in the Python class definition; which could cause other issues elsewhere.

To Reproduce
Create this table in an existing database before alembic migration:

CREATE TABLE `SorterHistory` (
  `eventType` char(20) NOT NULL,
  `lpn` char(20) DEFAULT NULL,
  `carton` char(20) DEFAULT NULL,
  `orderNumber` char(20) DEFAULT NULL,
  `chute` char(10) DEFAULT NULL,
  `tray` char(6) DEFAULT NULL,
  `remSortQty` int(11) DEFAULT NULL,
  `reason` char(20) DEFAULT NULL,
  `workList` char(20) DEFAULT NULL,
  `gaylordNumber` char(20) DEFAULT NULL,
  `shipVia` char(4) DEFAULT NULL,
  `createTime` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP,
  `idx` int(10) unsigned NOT NULL AUTO_INCREMENT,
  PRIMARY KEY (`idx`),
  KEY `lpn` (`lpn`),
  KEY `carton` (`carton`),
  KEY `OrderNumber` (`orderNumber`),
  KEY `createTime` (`createTime`),
  KEY `eventType` (`eventType`)
) ENGINE=InnoDB DEFAULT CHARSET=latin1;

This is the new schema we're trying to apply using alembic:

naming_convention={
    'ix': '%(column_0N_name)s',
    'uq': '%(column_0N_name)s',
    'ck': '%(column_0N_name)s',
    'fk': 'fk_%(table_name)s_%(referred_table_name)s',
    'pk': '%(column_0N_name)s',
}

SorterHistory_base = declarative_base(metadata=MetaData(naming_convention=naming_convention))
metadata = SorterHistory_base.metadata


class SorterHistory(SorterHistory_base):
    __tablename__ = 'SorterHistory'
    __table_args__ = {'mysql_engine': 'InnoDB'}

    eventType = Column(String(20), nullable=False, index=True)
    lpn = Column(String(20), index=True)
    carton = Column(String(20), index=True)
    orderNumber = Column(String(20), index=True)
    chute = Column(String(10))
    tray = Column(String(6))
    remSortQty = Column(Integer)
    reason = Column(String(20))
    workList = Column(String(20))
    gaylordNumber = Column(String(20))
    shipVia = Column(String(4))
    cartonNumber = Column(String(20))
    createTime = Column(TIMESTAMP, nullable=False, index=True, server_default=text("CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP"))
    idx = Column(BIGINT(unsigned=True), primary_key=True, autoincrement=True)

After running alembic revision --autogenerate we get this in our version script. You can clearly see that the order of operations is going to cause an error here because of case insensitivity on the index name.

op.create_index(op.f('orderNumber'), 'SorterHistory', ['orderNumber'], unique=False)
op.drop_index('OrderNumber', table_name='SorterHistory')

Here is the full stack trace when running alembic upgrade head.

Traceback (most recent call last):
  File "/home/kshutt/mandate/bin/alembic", line 11, in <module>
    sys.exit(main())
  File "/home/kshutt/mandate/lib/python2.7/site-packages/alembic/config.py", line 559, in main
    CommandLine(prog=prog).main(argv=argv)
  File "/home/kshutt/mandate/lib/python2.7/site-packages/alembic/config.py", line 553, in main
    self.run_cmd(cfg, options)
  File "/home/kshutt/mandate/lib/python2.7/site-packages/alembic/config.py", line 533, in run_cmd
    **dict((k, getattr(options, k, None)) for k in kwarg)
  File "/home/kshutt/mandate/lib/python2.7/site-packages/alembic/command.py", line 294, in upgrade
    script.run_env()
  File "/home/kshutt/mandate/lib/python2.7/site-packages/alembic/script/base.py", line 481, in run_env
    util.load_python_file(self.dir, "env.py")
  File "/home/kshutt/mandate/lib/python2.7/site-packages/alembic/util/pyfiles.py", line 97, in load_python_file
    module = load_module_py(module_id, path)
  File "/home/kshutt/mandate/lib/python2.7/site-packages/alembic/util/compat.py", line 217, in load_module_py
    mod = imp.load_source(module_id, path, fp)
  File "alembic/env.py", line 204, in <module>
    run_migrations_online()
  File "alembic/env.py", line 180, in run_migrations_online
    context.run_migrations(engine_name=name)
  File "<string>", line 8, in run_migrations
  File "/home/kshutt/mandate/lib/python2.7/site-packages/alembic/runtime/environment.py", line 813, in run_migrations
    self.get_context().run_migrations(**kw)
  File "/home/kshutt/mandate/lib/python2.7/site-packages/alembic/runtime/migration.py", line 560, in run_migrations
    step.migration_fn(**kw)
  File "/home/kshutt/cofe/database/migrations/WineDirect/alembic/versions/da62181f1b57_.py", line 20, in upgrade
    globals()["upgrade_%s" % engine_name]()
  File "/home/kshutt/cofe/database/migrations/WineDirect/alembic/versions/da62181f1b57_.py", line 453, in upgrade_WD_ECOM
    op.create_index(op.f('orderNumber'), 'SorterHistory', ['orderNumber'], unique=False)
  File "<string>", line 8, in create_index
  File "<string>", line 3, in create_index
  File "/home/kshutt/mandate/lib/python2.7/site-packages/alembic/operations/ops.py", line 871, in create_index
    return operations.invoke(op)
  File "/home/kshutt/mandate/lib/python2.7/site-packages/alembic/operations/base.py", line 354, in invoke
    return fn(self, operation)
  File "/home/kshutt/mandate/lib/python2.7/site-packages/alembic/operations/toimpl.py", line 87, in create_index
    operations.impl.create_index(idx)
  File "/home/kshutt/mandate/lib/python2.7/site-packages/alembic/ddl/impl.py", line 300, in create_index
    self._exec(schema.CreateIndex(index))
  File "/home/kshutt/mandate/lib/python2.7/site-packages/alembic/ddl/impl.py", line 146, in _exec
    return conn.execute(construct, multiparams)
  File "/home/kshutt/mandate/lib/python2.7/site-packages/sqlalchemy/engine/base.py", line 1011, in execute
    return meth(self, multiparams, params)
  File "/home/kshutt/mandate/lib/python2.7/site-packages/sqlalchemy/sql/ddl.py", line 72, in _execute_on_connection
    return connection._execute_ddl(self, multiparams, params)
  File "/home/kshutt/mandate/lib/python2.7/site-packages/sqlalchemy/engine/base.py", line 1073, in _execute_ddl
    compiled,
  File "/home/kshutt/mandate/lib/python2.7/site-packages/sqlalchemy/engine/base.py", line 1317, in _execute_context
    e, statement, parameters, cursor, context
  File "/home/kshutt/mandate/lib/python2.7/site-packages/sqlalchemy/engine/base.py", line 1511, in _handle_dbapi_exception
    sqlalchemy_exception, with_traceback=exc_info[2], from_=e
  File "/home/kshutt/mandate/lib/python2.7/site-packages/sqlalchemy/engine/base.py", line 1277, in _execute_context
    cursor, statement, parameters, context
  File "/home/kshutt/mandate/lib/python2.7/site-packages/sqlalchemy/engine/default.py", line 609, in do_execute
    cursor.execute(statement, parameters)
  File "/home/kshutt/mandate/lib/python2.7/site-packages/MySQLdb/cursors.py", line 209, in execute
    res = self._query(query)
  File "/home/kshutt/mandate/lib/python2.7/site-packages/MySQLdb/cursors.py", line 315, in _query
    db.query(q)
  File "/home/kshutt/mandate/lib/python2.7/site-packages/MySQLdb/connections.py", line 239, in query
    _mysql.connection.query(self, query)
sqlalchemy.exc.OperationalError: (MySQLdb._exceptions.OperationalError) (1061, "Duplicate key name 'orderNumber'")
[SQL: CREATE INDEX `orderNumber` ON `SorterHistory` (`orderNumber`)]
(Background on this error at: http://sqlalche.me/e/13/e3q8)

Versions.

• OS: CentOS Linux release 7.9.2009 (all patches applied up to 1/27/2021)
• Python: 2.7.5
• Alembic: 1.5.2
• SQLAlchemy: 1.3.22
• Database: mysql Ver 14.14 Distrib 5.7.32-35, for Linux (x86_64) using 6.2
• DBAPI: mysqlclient 1.4.6, MySQL-python 1.2.5

Additional context
Please feel free to ask questions if additional information is needed here.

Have a nice day!

[CompPhy]

Also note.... It appears that alembic creates this same error on both the upgrade() and downgrade() blocks.

[zzzeek]

hi there -

First off, a general note about " I don't think it's acceptable to have to look for these over manually every time. ". In your case, you have a widespread set of changes that all touch upon the same normally extremely unusual circumstance. So while Alembic will always require that a green-field migration be manually inspected as we can never guarantee a foolproof result, in your case specifically, you can solve your problem right now by using the documented hooks to alter the output for this specific case, using the process_revision_directives hook to search for adds/drops of indexes and reordering them. So in the absence of this behavior being improved on the Alembic side, you can apply a little code here to get your job done.

Regarding the behavior, it looks like for foriegn key constraints we do already place the "remove" operations before the "adds", so moving indexes and unique constraints (which are handled together) to behave similarly is likely a change we can release a a point release, as we just put out alembic 1.5 which would have been a really good place for a change like this as it will alter the results of diffs. I'll push a code review up for this now which will run Alembic against a few third party applications that use it very heavily and we'll see if it breaks anyone's assumptions.

for tables, we also put the table creates before the table drops, but in this case people usually arent dropping and recreating tables to rename them, they would be using rename_table() which as also stated in the docs is another condition we can't detect.

So the change would be limited to just indexes and uniques, let's see if that can be done for a trivial release.

[CompPhy]

Thank you for pointing me to the right place in the documentation. Let me see if I can come up with a work around here.

[sqla-tester]

Mike Bayer has proposed a fix for this issue in the master branch:

Place index/uq drops before creates https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/2512

[zzzeek]

we can likely have this for a 1.5.3 release, the patch in the linked gerrit above is avaiable for testing if you wanted to see if this solved the problem completely for you.

[CompPhy]

This was a good work around for now. It's a round about way to do it, but seems to work fine. I will try to find time to test with your patch file later this week. Thanks for the quick response.

            # When case is changing on an index, always make sure drop index happens first.
            if isinstance(op, ops.ModifyTableOps):
                index_creates = set()
                index_drops = set()
                for index, sub_op in enumerate(op.ops):
                    if isinstance(sub_op, ops.CreateIndexOp):
                        index_creates.add((op.table_name, sub_op.index_name, index))
                    if isinstance(sub_op, ops.DropIndexOp):
                        index_drops.add((op.table_name, sub_op.index_name, index))
                op.ops = _scan_index_ops(op.ops, index_creates, index_drops)

def _scan_index_ops(ops, creates, drops):
    for create_table, create_name, create_pos in creates:
        for drop_table, drop_name, drop_pos in drops:
            create_lower = create_name.lower()
            drop_lower = drop_name.lower()
            if create_table == drop_table and create_lower == drop_lower and create_pos < drop_pos:
                # Drop should come first, so swap them.
                ops = _swap_elements(ops, create_pos, drop_pos)
    return ops

[CompPhy]

I can confirm that your patch file worked for me. In fact, it does better than my work around. Your solution puts all the drops first, even when there's not a name collision to worry about. Thanks again for the quick response on this.

[zzzeek]

yeah, I wasn't about to dig into naming collision rules and stuff like that, that would be a total mess :) just put the drops first, problem solved