From: Mike Bayer Date: Tue, 8 Jul 2025 14:05:56 +0000 (-0400) Subject: fix metadata requirement in fk render; remove column.copy() X-Git-Tag: rel_1_16_3~4 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=7c84327e2c02b1fe7f8336d63dc7c8a89b609870;p=thirdparty%2Fsqlalchemy%2Falembic.git fix metadata requirement in fk render; remove column.copy() Fixed autogenerate rendering bug which failed to render foreign key constraints local to a :class:`.CreateTableOp` object if it did not refer to a ``MetaData`` collection via a private constructor argument that would not ordinarily be passed in user-defined rewriter recipes, including ones in the Alembic cookbook section of the docs. Also removed the use of column.copy() from the CreateTableOp rewriter recipe and added a new test for this recipe which includes a table with an FK. it's not clear why copy() was needed here and if we get more reports of issues, we can add to this test suite and adjust. Fixes: #1692 Change-Id: I6cf6c8f62b7eb116cfc78216b3dbcbddad499691 --- diff --git a/alembic/autogenerate/render.py b/alembic/autogenerate/render.py index bd20ced8..ca161594 100644 --- a/alembic/autogenerate/render.py +++ b/alembic/autogenerate/render.py @@ -1000,7 +1000,7 @@ def _render_primary_key( def _fk_colspec( fk: ForeignKey, metadata_schema: Optional[str], - namespace_metadata: MetaData, + namespace_metadata: Optional[MetaData], ) -> str: """Implement a 'safe' version of ForeignKey._get_colspec() that won't fail if the remote table can't be resolved. @@ -1024,7 +1024,10 @@ def _fk_colspec( # the FK constraint needs to be rendered in terms of the column # name. - if table_fullname in namespace_metadata.tables: + if ( + namespace_metadata is not None + and table_fullname in namespace_metadata.tables + ): col = namespace_metadata.tables[table_fullname].c.get(colname) if col is not None: colname = _ident(col.name) # type: ignore[assignment] @@ -1055,7 +1058,7 @@ def _populate_render_fk_opts( def _render_foreign_key( constraint: ForeignKeyConstraint, autogen_context: AutogenContext, - namespace_metadata: MetaData, + namespace_metadata: Optional[MetaData], ) -> Optional[str]: rendered = _user_defined_render("foreign_key", constraint, autogen_context) if rendered is not False: @@ -1069,7 +1072,9 @@ def _render_foreign_key( _populate_render_fk_opts(constraint, opts) - apply_metadata_schema = namespace_metadata.schema + apply_metadata_schema = ( + namespace_metadata.schema if namespace_metadata is not None else None + ) return ( "%(prefix)sForeignKeyConstraint([%(cols)s], " "[%(refcols)s], %(args)s)" diff --git a/docs/build/cookbook.rst b/docs/build/cookbook.rst index 63a26c02..59437af6 100644 --- a/docs/build/cookbook.rst +++ b/docs/build/cookbook.rst @@ -1127,7 +1127,7 @@ copying each column or constraint object and applying a new sorting scheme:: special_names.get(col.key, index) if isinstance(col, Column) else 2000, - col.copy(), + col, ) for index, col in enumerate(op.columns) ] diff --git a/docs/build/unreleased/1692.rst b/docs/build/unreleased/1692.rst new file mode 100644 index 00000000..96bf0c94 --- /dev/null +++ b/docs/build/unreleased/1692.rst @@ -0,0 +1,10 @@ +.. change:: + :tags: bug, autogenerate + :tickets: 1692 + + Fixed autogenerate rendering bug which failed to render foreign key + constraints local to a :class:`.CreateTableOp` object if it did not refer + to a ``MetaData`` collection via a private constructor argument that would + not ordinarily be passed in user-defined rewriter recipes, including ones + in the Alembic cookbook section of the docs. + diff --git a/tests/test_autogen_render.py b/tests/test_autogen_render.py index c098aca4..493a6833 100644 --- a/tests/test_autogen_render.py +++ b/tests/test_autogen_render.py @@ -1843,6 +1843,31 @@ class AutogenRenderTest(TestBase): " # ### end Alembic commands ###", ) + def test_render_foreign_key_no_context(self): + """test #1692""" + + uo = ops.UpgradeOps( + ops=[ + ops.CreateTableOp( + "sometable", + [ + Column("x", Integer), + Column("q", Integer, ForeignKey("othertable.id")), + ], + ) + ] + ) + eq_( + autogenerate.render_python_code(uo), + "# ### commands auto generated by Alembic - please adjust! ###\n" + " op.create_table('sometable',\n" + " sa.Column('x', sa.Integer(), nullable=True),\n" + " sa.Column('q', sa.Integer(), nullable=True),\n" + " sa.ForeignKeyConstraint(['q'], ['othertable.id'], )\n" + " )\n" + " # ### end Alembic commands ###", + ) + def test_render_array_no_context(self): uo = ops.UpgradeOps( ops=[ diff --git a/tests/test_script_production.py b/tests/test_script_production.py index 10493579..00150468 100644 --- a/tests/test_script_production.py +++ b/tests/test_script_production.py @@ -6,9 +6,14 @@ from unittest.mock import patch import sqlalchemy as sa from sqlalchemy import Column +from sqlalchemy import DateTime +from sqlalchemy import ForeignKey from sqlalchemy import inspect +from sqlalchemy import Integer from sqlalchemy import MetaData +from sqlalchemy import String from sqlalchemy import Table +from sqlalchemy import UniqueConstraint from alembic import autogenerate from alembic import command @@ -1178,6 +1183,102 @@ class RewriterTest(TestBase): eq_(diffs[1][0], "execute") eq_(diffs[1][1], "STATEMENT") + def test_reordering_example_wo_copy(self): + """test related to #1692 in that we identified the recipe + for rewriting column ordering was using column.copy()""" + + m = MetaData() + t1 = Table( + "my_table", + m, + Column("data", String(50)), + Column("created_at", DateTime), + Column("id", Integer, primary_key=True), + Column("updated_at", DateTime), + UniqueConstraint("data", name="uq_data"), + ) + + t2 = Table( + "my_other_table", + m, + Column("data", String(50)), + Column("created_at", DateTime), + Column("id", Integer, primary_key=True), + Column("other_id", ForeignKey("my_table.id")), + Column("updated_at", DateTime), + UniqueConstraint("data", name="uq_data"), + ) + + writer = autogenerate.Rewriter() + + @writer.rewrites(ops.CreateTableOp) + def order_columns(context, revision, op): + + special_names = { + "id": -100, + "created_at": 1001, + "updated_at": 1002, + } + + cols_by_key = [ + ( + ( + special_names.get(col.key, index) + if isinstance(col, Column) + else 2000 + ), + col, # note no copy() + ) + for index, col in enumerate(op.columns) + ] + + columns = [ + col + for idx, col in sorted(cols_by_key, key=lambda entry: entry[0]) + ] + return ops.CreateTableOp( + op.table_name, columns, schema=op.schema, **op.kw + ) + + directives = [ + ops.MigrationScript( + util.rev_id(), + ops.UpgradeOps( + ops=[ + ops.CreateTableOp.from_table(t1), + ops.CreateTableOp.from_table(t2), + ] + ), + ops.DowngradeOps(ops=[]), + ) + ] + + ctx, rev = mock.Mock(), mock.Mock() + writer(ctx, rev, directives) + eq_( + autogenerate.render_python_code(directives[0].upgrade_ops), + "# ### commands auto generated by Alembic - please adjust! ###\n" + " op.create_table('my_table',\n" + " sa.Column('id', sa.Integer(), nullable=False),\n" + " sa.Column('data', sa.String(length=50), nullable=True),\n" + " sa.Column('created_at', sa.DateTime(), nullable=True),\n" + " sa.Column('updated_at', sa.DateTime(), nullable=True),\n" + " sa.PrimaryKeyConstraint('id'),\n" + " sa.UniqueConstraint('data', name='uq_data')\n" + " )\n" + " op.create_table('my_other_table',\n" + " sa.Column('id', sa.Integer(), nullable=False),\n" + " sa.Column('data', sa.String(length=50), nullable=True),\n" + " sa.Column('other_id', sa.Integer(), nullable=True),\n" + " sa.Column('created_at', sa.DateTime(), nullable=True),\n" + " sa.Column('updated_at', sa.DateTime(), nullable=True),\n" + " sa.ForeignKeyConstraint(['other_id'], ['my_table.id'], ),\n" + " sa.PrimaryKeyConstraint('id'),\n" + " sa.UniqueConstraint('data', name='uq_data')\n" + " )\n" + " # ### end Alembic commands ###", + ) + class MultiDirRevisionCommandTest(TestBase): def setUp(self):