Make prefer-robust-stmts aware of alembic version update transactions

[DylanGriffith]

Today with Alembic you can write a migration like:

def upgrade() -> None:
    op.add_column('t', sa.Column('test_column', sa.Text(), nullable=True))

    with op.get_context().autocommit_block():
        op.execute("SELECT 1")

def downgrade() -> None:
    """Downgrade schema."""
    pass

This results in the following SQL:

BEGIN;
ALTER TABLE t ADD COLUMN test_column TEXT;
COMMIT;

SELECT 1;

BEGIN;
UPDATE alembic_version SET version_num='bbc810e82b46' WHERE alembic_version.version_num = 'b4d17e0c93aa';
COMMIT;

And it passes the "prefer-robust-stmts" check. But it shouldn't because as soon as you add this autocommit_block anywhere in your migration you are effectively breaking the wrapping transaction for the whole alembic migration.

And slightly surprisingly Alembic still decides to put a transaction at the start and the end of the migration.

This is quite weird behaviour from Alembic. But it is resulting in this prefer-robust-stmts lint rule assuming that everything is "OK" because it sees that the ALTER TABLE t is in "some transaction". But it happens to be the wrong transaction and can definitely be partially failed.

This PR makes a targetted implementation for alembic that looks more closely to work out whether or not the statement is actually in the same statement as the alembic_version update which is our main safety against partial failures.

For now this code only handles alembic but if similar quirky behaviour is possible with other migration frameworks (or we want to protect against users putting explicitly BEGIN ... COMMIT in their migrations) then we might want to extend this implementation to find the relevant markers from the other frameworks.

This PR also removes a comment about IF NOT EXISTS not being supported by alembic but this is out of date as you can see from
https://alembic.sqlalchemy.org/en/latest/ops.html#alembic.operations.Operations.add_column .

[netlify]

👷 Deploy request for squawkhq pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	ed5fc8e

[sbdchd]

wondering if we can avoid this search by keeping track of the alembic_version_updates when we look through on line 26?

[DylanGriffith]

OK I code-golfed it down a little to only check it once now. I'm not totally sure it's more readable but the logic is the same.

[DylanGriffith]

Also it wasn't so easy to change it exactly the way you suggested because I would have had to keep a list of indexes in that line and then loop over them and see if any were inside this range and it seemed a little more awkward than this approach.

[sbdchd]

Oh gotcha, thanks for the PR :D

[sbdchd]

looks good, left a couple comments!

[DylanGriffith]

Thanks @sbdchd for the quick review. I applied both of your suggestions.

[sbdchd]

Ty!