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 bd20ced8da6c10293ab72d6e69b0da31333f5597..ca161594723861b2efa83d02f87963fe8e5e2493 100644 (file)
--- 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 63a26c02e81e7f00810938ac18ad91a33033a784..59437af6a189234ec53eb3e405a3e6632db86891 100644 (file)
--- 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 (file)
index 0000000..96bf0c9
--- /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 c098aca4e561cdf0bde9119834396cd46d4ca4f5..493a683337632cbeebb2ad0efe67ca8a73be4dbf 100644 (file)
--- 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 10493579b943b020e184c0ba92531b53daf955f4..0015046891af3406a21c09be039fef1c118e8b16 100644 (file)
--- 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):