Fix batch mode ignoring naming_convention for reflected constraints - #1840
Open
Karthick-dev-cart wants to merge 1 commit into
Conversation
Fixes: sqlalchemy#1834 A reflected constraint always comes back from reflection with an explicit name -- the one stored in the database -- so a naming_convention passed to batch_alter_table() previously had no effect on it: naming conventions only fill in names for *unnamed* constraints. Most visible after rename_table(): a recreated table's constraints carried over names embedding the *old* table name instead of following the convention against the new one. Reflected constraints whose type has an entry in the supplied naming_convention now have their name reset and are re-attached to the (correctly, finally-named) existing table, re-firing SQLAlchemy's naming-convention machinery against the real target name -- not the "_alembic_tmp_*" name used internally during the copy, which an earlier version of this fix incorrectly baked in instead. Constraint types the naming_convention doesn't mention are left alone, so this can't turn an unrelated constraint into an unnamed one.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes: #1834
Description
batch_alter_table(..., naming_convention=...)reflects the existing table into aMetaData(naming_convention=...), clearly intending for the convention to govern the recreated table's constraint names. But reflection always returns constraints with an explicit.name(whatever is actually stored in the database), and SQLAlchemy naming conventions only fill in names for constraints that don't already have one — so the convention silently had no effect on any reflected constraint.This was most visible after
rename_table(): batch mode would faithfully carry over reflected constraint names that still embed the old table name (e.g.pk_OldTableNameafter renaming toTableName), even though anaming_conventionwas explicitly passed to make names track the current table.Fix
In
ApplyBatchImpl._grab_table_elements(), a reflected constraint whose type has an entry in the suppliednaming_convention(pk/fk/uq/ck) now has its name reset toNoneand is re-attached toself.table— which already carries the correct, final table name and the convention — so SQLAlchemy's own naming-convention event recomputes the name against the right target immediately, before the constraint gets copied onto the temp-named table used internally during the "move and copy". (An earlier version of this fix deferred the regeneration to copy time instead, which "fixed" the stale-old-name bug but introduced a different one — baking_alembic_tmp_<name>into the constraint name. Caught via a test that isolates the no-rename case and fixed by regenerating againstself.tableup front.)Constraint types the convention doesn't mention are left untouched, so this can't turn some other constraint into an unnamed one it wasn't asked to govern.
_transfer_elements_to_new_table()now also carriesnaming_conventiononto the new table'sMetaData(previously always plainMetaData()), which the reset-name mechanism above relies on.Checklist
Fixes: #1834included abovetests/test_batch.py::BatchRoundTripTest::test_naming_convention_applies_to_reflected_constraints— reproduces the issue's exact scenario (rename + batch recreate with a naming_convention covering pk/fk), asserts the recreated table's PK and FK names track the new table name. Verified this test fails against unpatchedmainwith the exact bug signature (pk_oldtablename != pk_tablename).Test plan
mainwith the issue's exact symptom.pytest tests/→ 1799 passed, 132 skipped (pre-existing, dialect-specific), 0 failed.black --check/flake8clean on both changed files.docs/build/unreleased/1834.rst.