Batch mode ignores naming_convention for reflected constraints (regression surfaced by SQLAlchemy 2.0.45)

[zzzeek]

Batch mode ignores naming_convention for reflected constraints (regression surfaced by SQLAlchemy 2.0.45)

Splitting this out from discussion #1833. This is an alembic bug in batch
mode, exposed (not caused) by a correctness fix in SQLAlchemy 2.0.45.

TL;DR

When batch_alter_table(..., naming_convention=...) recreates a table,
constraints that come back named from reflection are copied verbatim and
the supplied naming_convention is never applied to them. When batch mode
follows an op.rename_table(), those reflected names still embed the old
table name, so the recreated table ends up with constraints like
pk_OldTableName even though the table is now TableName. Passing a naming
convention should make the recreated constraints follow that convention
against the new table name (pk_TableName).

This underlying behavior is not new and is not SQLAlchemy-version-specific.
If you use an all-lowercase constraint name, you get the stale
pk_oldtablename on every SQLAlchemy version, including 2.0.44 — because a
lowercase name needs no quoting, SQLite reflection has always parsed it, and
alembic has always carried it over unchanged.

What SQLAlchemy 2.0.45 changed is only how visible the bug is. Before 2.0.45,
SQLite reflection could not parse quoted constraint names (names with
uppercase letters etc. require quoting), so it returned name=None for them;
that accidentally let alembic's naming convention regenerate those names from
the current (renamed) table. The 2.0.45 fix (#12954) made SQLite reflection
report quoted names correctly, which removed that accidental masking and
exposed the pre-existing alembic behavior for mixed/upper-case names too.

Reproduction

import sqlalchemy as sa
from sqlalchemy import Column, ForeignKeyConstraint, MetaData, String, Table
from alembic.migration import MigrationContext
from alembic.operations import Operations

CONVENTION = {
    "pk": "pk_%(table_name)s",
    "fk": "fk_%(table_name)s_%(column_0_name)s_%(referred_table_name)s",
}

engine = sa.create_engine("sqlite://", echo=True)
with engine.connect() as conn:
    md = MetaData(naming_convention=CONVENTION)
    Table("Other", md, Column("name", String(50), primary_key=True))
    Table(
        "OldTableName",
        md,
        Column("col", String(50), primary_key=True),
        ForeignKeyConstraint(["col"], ["Other.name"], ondelete="CASCADE"),
    )
    md.create_all(conn)
    conn.commit()

    op = Operations(MigrationContext.configure(conn))
    op.rename_table("OldTableName", "TableName")
    with op.batch_alter_table(
        "TableName", naming_convention=CONVENTION, recreate="always"
    ) as batch_op:
        batch_op.alter_column("col", existing_type=String(50))
    conn.commit()

SQLAlchemy 2.0.44 (expected) — the recreated table uses the new name:

CREATE TABLE "_alembic_tmp_TableName" (
    col VARCHAR(50) NOT NULL,
    CONSTRAINT "pk_TableName" PRIMARY KEY (col),
    CONSTRAINT "fk_TableName_col_Other" FOREIGN KEY(col)
        REFERENCES "Other" (name) ON DELETE CASCADE
)

SQLAlchemy 2.0.45+ (regression) — the stale old name survives:

CREATE TABLE "_alembic_tmp_TableName" (
    col VARCHAR(50) NOT NULL,
    CONSTRAINT "pk_OldTableName" PRIMARY KEY (col),
    CONSTRAINT "fk_OldTableName_col_Other" FOREIGN KEY(col)
        REFERENCES "Other" (name) ON DELETE CASCADE
)

The practical fallout is exactly what the original reporter described: a later
migration that does batch_op.drop_constraint("pk_TableName", ...) (a name it
constructs from the convention) fails, because the constraint in the database
is still pk_OldTableName.

The behavior is version-independent for unquoted names

Swap the table/referred names for all-lowercase equivalents (oldtablename,
tablename, other) so the convention produces pk_oldtablename — a name
that needs no quoting. Now every SQLAlchemy version, including 2.0.44,
emits the stale name:

CREATE TABLE _alembic_tmp_tablename (
    col VARCHAR(50) NOT NULL,
    CONSTRAINT pk_oldtablename PRIMARY KEY (col),
    CONSTRAINT fk_oldtablename_col_other FOREIGN KEY(col)
        REFERENCES other (name) ON DELETE CASCADE
)

So the stale-name behavior was never introduced by SQLAlchemy; it has always
been alembic's behavior. Quoted (mixed/upper-case) names were the only ones
that got the "correct" result before 2.0.45, and only by accident.

Where the SQLAlchemy behavior changed

git bisect across rel_2_0_44..rel_2_0_45 (70 commits) points at:

commit 9a91f0b3a1c6ef2b11160549d53e0820f7e9e6b3
    fix sqlite regex for quoted fk, pk names
    Fixes: #12954

That commit updates the SQLite reflection regexes so that quoted constraint
names (names with uppercase letters etc. require quoting) are parsed
correctly. Old pattern:

PK_PATTERN = r"CONSTRAINT (\w+) PRIMARY KEY"

\w+ never matched the surrounding double quotes, so
CONSTRAINT "pk_OldTableName" PRIMARY KEY returned name=None. New pattern:

PK_PATTERN = r'CONSTRAINT +(?:"(.+?)"|(\w+)) +PRIMARY KEY'

This SQLAlchemy change is correct — reflection now reports the name that is
actually stored in the database.

Why I think this is an alembic bug

Batch mode reflects the existing table into a MetaData that carries the
supplied naming convention, on purpose
(alembic/operations/batch.py, BatchOperationsImpl.flush()):

if self.naming_convention:
    m1 = MetaData(naming_convention=self.naming_convention)
else:
    m1 = MetaData()
...
existing_table = Table(
    self.table_name, m1, autoload_with=self.operations.get_bind(), ...
)

The intent is clearly for the convention to govern the recreated table's
constraint names. But a naming convention in SQLAlchemy only fills in names
for constraints that don't already have one, and reflected constraints
normally arrive with an explicit name (always for unquoted names; and, since
2.0.45, for quoted names too). So the convention that alembic went out of its
way to attach is effectively a no-op for every reflected constraint.

Then in ApplyBatchImpl._grab_table_elements() the reflected constraints are
sorted into named_constraints vs unnamed_constraints purely on whether
.name is truthy, and _transfer_elements_to_new_table() copies the named
ones verbatim. Nothing ever re-derives a name from the convention for a
constraint that reflected with a name.

The net effect:

• Passing naming_convention= to batch_alter_table() does nothing for
  pre-existing constraints, even though the API and the internal
  reflect-into-convention-MetaData setup both imply it should.
• After rename_table(), batch mode faithfully reproduces constraint names
  that embed a table name which no longer exists — the exact inconsistency a
  naming convention is supposed to prevent.

Previously this was masked by the SQLite reflection gap; the SQLAlchemy fix
just removed the mask.

Suggested direction

When a naming_convention is supplied to batch_alter_table(), alembic
should let that convention re-derive names for the recreated table rather than
inheriting reflected names wholesale. Concretely, in ApplyBatchImpl for the
reflected case, constraints that are being carried into the new table could
have their names reset so the convention (already present on the target
MetaData) regenerates them against the current table name.

The one thing to be careful about: this must not clobber constraint names the
user set explicitly and does not want regenerated. A couple of options:

1. Only regenerate when a naming_convention was explicitly passed to
   batch_alter_table() (opt-in via the existing parameter — which is
   arguably what passing it already means).
2. Only reset a reflected name when it equals what the convention would have
   produced for some previously-seen table name (i.e. detect the stale
   convention name), leaving hand-picked names alone.

A regression test plus a fix along these lines can follow.

Environment

• alembic: 1.16.x / 1.18.x (repro confirmed on current main)
• SQLAlchemy: good <= 2.0.44, regressed >= 2.0.45
• backend: SQLite (batch "move and copy")

[sqla-tester]

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

apply naming_convention to reflected constraints in batch recreate https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/6832