From: Mike Bayer Date: Tue, 10 Feb 2026 13:50:31 +0000 (-0500) Subject: revert primary_key=True change and replace with inline_primary_key X-Git-Tag: rel_1_18_4~2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=fa35ee9264cb0cbe25729c753595f7a6ebbd3f70;p=thirdparty%2Fsqlalchemy%2Falembic.git revert primary_key=True change and replace with inline_primary_key Reverted the behavior of :meth:`.Operations.add_column` that would automatically render the "PRIMARY KEY" keyword inline when a :class:`.Column` with ``primary_key=True`` is added. The automatic behavior, added in version 1.18.2, is now opt-in via the new :paramref:`.Operations.add_column.inline_primary_key` parameter. This change restores the ability to render a PostgreSQL SERIAL column, which is required to be ``primary_key=True``, while not impacting the ability to render a separate primary key constraint. This also provides consistency with the :paramref:`.Operations.add_column.inline_references` parameter and gives users explicit control over SQL generation. Fixes: #1232 Change-Id: Ia9333c1768caa5d99b0527d59fcc797af2c3249c --- diff --git a/alembic/ddl/base.py b/alembic/ddl/base.py index caab9098..30a3a15a 100644 --- a/alembic/ddl/base.py +++ b/alembic/ddl/base.py @@ -156,11 +156,13 @@ class AddColumn(AlterTable): schema: Optional[Union[quoted_name, str]] = None, if_not_exists: Optional[bool] = None, inline_references: Optional[bool] = None, + inline_primary_key: Optional[bool] = None, ) -> None: super().__init__(name, schema=schema) self.column = column self.if_not_exists = if_not_exists self.inline_references = inline_references + self.inline_primary_key = inline_primary_key class DropColumn(AlterTable): @@ -203,6 +205,7 @@ def visit_add_column(element: AddColumn, compiler: DDLCompiler, **kw) -> str: element.column, if_not_exists=element.if_not_exists, inline_references=element.inline_references, + inline_primary_key=element.inline_primary_key, **kw, ), ) @@ -355,6 +358,7 @@ def add_column( column: Column[Any], if_not_exists: Optional[bool] = None, inline_references: Optional[bool] = None, + inline_primary_key: Optional[bool] = None, **kw, ) -> str: text = "ADD COLUMN %s%s" % ( @@ -362,7 +366,7 @@ def add_column( compiler.get_column_specification(column, **kw), ) - if column.primary_key: + if inline_primary_key and column.primary_key: text += " PRIMARY KEY" # Handle inline REFERENCES if requested diff --git a/alembic/ddl/impl.py b/alembic/ddl/impl.py index a6153106..964cd1f3 100644 --- a/alembic/ddl/impl.py +++ b/alembic/ddl/impl.py @@ -387,6 +387,7 @@ class DefaultImpl(metaclass=ImplMeta): schema: Optional[Union[str, quoted_name]] = None, if_not_exists: Optional[bool] = None, inline_references: Optional[bool] = None, + inline_primary_key: Optional[bool] = None, ) -> None: self._exec( base.AddColumn( @@ -395,6 +396,7 @@ class DefaultImpl(metaclass=ImplMeta): schema=schema, if_not_exists=if_not_exists, inline_references=inline_references, + inline_primary_key=inline_primary_key, ) ) diff --git a/alembic/op.pyi b/alembic/op.pyi index 40a480cc..449798dc 100644 --- a/alembic/op.pyi +++ b/alembic/op.pyi @@ -65,6 +65,7 @@ def add_column( schema: Optional[str] = None, if_not_exists: Optional[bool] = None, inline_references: Optional[bool] = None, + inline_primary_key: Optional[bool] = None, ) -> None: """Issue an "add column" instruction using the current migration context. @@ -163,6 +164,13 @@ def add_column( .. versionadded:: 1.18.2 + :param inline_primary_key: If True, renders the PRIMARY KEY constraint + inline within the ADD COLUMN directive, rather than rendering it + separately. This is a purely syntactic option and should only be + used for single-column primary keys. + + .. versionadded:: 1.18.4 + """ def alter_column( diff --git a/alembic/operations/base.py b/alembic/operations/base.py index 218ea190..4eb25146 100644 --- a/alembic/operations/base.py +++ b/alembic/operations/base.py @@ -631,6 +631,7 @@ class Operations(AbstractOperations): schema: Optional[str] = None, if_not_exists: Optional[bool] = None, inline_references: Optional[bool] = None, + inline_primary_key: Optional[bool] = None, ) -> None: """Issue an "add column" instruction using the current migration context. @@ -729,6 +730,13 @@ class Operations(AbstractOperations): .. versionadded:: 1.18.2 + :param inline_primary_key: If True, renders the PRIMARY KEY constraint + inline within the ADD COLUMN directive, rather than rendering it + separately. This is a purely syntactic option and should only be + used for single-column primary keys. + + .. versionadded:: 1.18.4 + """ # noqa: E501 ... @@ -1691,6 +1699,7 @@ class BatchOperations(AbstractOperations): insert_after: Optional[str] = None, if_not_exists: Optional[bool] = None, inline_references: Optional[bool] = None, + inline_primary_key: Optional[bool] = None, ) -> None: """Issue an "add column" instruction using the current batch migration context. diff --git a/alembic/operations/ops.py b/alembic/operations/ops.py index 575af2a9..a999311d 100644 --- a/alembic/operations/ops.py +++ b/alembic/operations/ops.py @@ -2054,12 +2054,14 @@ class AddColumnOp(AlterTableOp): schema: Optional[str] = None, if_not_exists: Optional[bool] = None, inline_references: Optional[bool] = None, + inline_primary_key: Optional[bool] = None, **kw: Any, ) -> None: super().__init__(table_name, schema=schema) self.column = column self.if_not_exists = if_not_exists self.inline_references = inline_references + self.inline_primary_key = inline_primary_key self.kw = kw def reverse(self) -> DropColumnOp: @@ -2100,6 +2102,7 @@ class AddColumnOp(AlterTableOp): schema: Optional[str] = None, if_not_exists: Optional[bool] = None, inline_references: Optional[bool] = None, + inline_primary_key: Optional[bool] = None, ) -> None: """Issue an "add column" instruction using the current migration context. @@ -2198,6 +2201,13 @@ class AddColumnOp(AlterTableOp): .. versionadded:: 1.18.2 + :param inline_primary_key: If True, renders the PRIMARY KEY constraint + inline within the ADD COLUMN directive, rather than rendering it + separately. This is a purely syntactic option and should only be + used for single-column primary keys. + + .. versionadded:: 1.18.4 + """ op = cls( @@ -2206,6 +2216,7 @@ class AddColumnOp(AlterTableOp): schema=schema, if_not_exists=if_not_exists, inline_references=inline_references, + inline_primary_key=inline_primary_key, ) return operations.invoke(op) @@ -2219,6 +2230,7 @@ class AddColumnOp(AlterTableOp): insert_after: Optional[str] = None, if_not_exists: Optional[bool] = None, inline_references: Optional[bool] = None, + inline_primary_key: Optional[bool] = None, ) -> None: """Issue an "add column" instruction using the current batch migration context. @@ -2241,6 +2253,7 @@ class AddColumnOp(AlterTableOp): schema=operations.impl.schema, if_not_exists=if_not_exists, inline_references=inline_references, + inline_primary_key=inline_primary_key, **kw, ) return operations.invoke(op) diff --git a/alembic/operations/toimpl.py b/alembic/operations/toimpl.py index b9b4b711..85b9d8a3 100644 --- a/alembic/operations/toimpl.py +++ b/alembic/operations/toimpl.py @@ -173,6 +173,7 @@ def add_column(operations: "Operations", operation: "ops.AddColumnOp") -> None: schema = operation.schema kw = operation.kw inline_references = operation.inline_references + inline_primary_key = operation.inline_primary_key if column.table is not None: column = _copy(column) @@ -184,6 +185,7 @@ def add_column(operations: "Operations", operation: "ops.AddColumnOp") -> None: schema=schema, if_not_exists=operation.if_not_exists, inline_references=inline_references, + inline_primary_key=inline_primary_key, **kw, ) diff --git a/docs/build/changelog.rst b/docs/build/changelog.rst index 37434f47..5cad83f6 100644 --- a/docs/build/changelog.rst +++ b/docs/build/changelog.rst @@ -65,6 +65,13 @@ Changelog status with any existing primary key constraint or particular backend limitations on adding columns to the primary key. + .. note:: + + As of version 1.18.4, this behavior has been amended to be opt-in + via the new ``inline_primary_key`` parameter to + :meth:`.Operations.add_column`, rather than occurring automatically + when ``primary_key=True`` is set on the :class:`.Column` object. + .. change:: :tags: bug, typing :tickets: 1669 diff --git a/docs/build/unreleased/1232_amendment.rst b/docs/build/unreleased/1232_amendment.rst new file mode 100644 index 00000000..c83644ea --- /dev/null +++ b/docs/build/unreleased/1232_amendment.rst @@ -0,0 +1,24 @@ +.. change:: + :tags: bug, operations + :tickets: 1232 + + Reverted the behavior of :meth:`.Operations.add_column` that would + automatically render the "PRIMARY KEY" keyword inline when a + :class:`.Column` with ``primary_key=True`` is added. The automatic + behavior, added in version 1.18.2, is now opt-in via the new + :paramref:`.Operations.add_column.inline_primary_key` parameter. This + change restores the ability to render a PostgreSQL SERIAL column, which is + required to be ``primary_key=True``, while not impacting the ability to + render a separate primary key constraint. This also provides consistency + with the :paramref:`.Operations.add_column.inline_references` parameter and + gives users explicit control over SQL generation. + + To render PRIMARY KEY inline, use the + :paramref:`.Operations.add_column.inline_primary_key` parameter set to + ``True``:: + + op.add_column( + "my_table", + Column("id", Integer, primary_key=True), + inline_primary_key=True + ) diff --git a/tests/test_batch.py b/tests/test_batch.py index f56e2d33..fa582829 100644 --- a/tests/test_batch.py +++ b/tests/test_batch.py @@ -940,6 +940,7 @@ class BatchAPITest(TestBase): schema=None, if_not_exists=None, inline_references=None, + inline_primary_key=None, ) in batch.impl.operations.impl.mock_calls ) @@ -1716,11 +1717,21 @@ class BatchRoundTripTest(TestBase): pk_const = inspect(self.conn).get_pk_constraint("foo") eq_(pk_const["constrained_columns"], []) - def test_drop_pk_col_readd_pk_col(self): + @testing.variation( + "use_inline_pk", + [ + True, + (False, exclusions.fails_on(["postgresql", "mysql", "mariadb"])), + ], + ) + def test_drop_pk_col_readd_pk_col(self, use_inline_pk): # drop a column, add it back with primary_key=True, should remain with self.op.batch_alter_table("foo") as batch_op: batch_op.drop_column("id") - batch_op.add_column(Column("id", Integer, primary_key=True)) + batch_op.add_column( + Column("id", Integer, primary_key=True), + inline_primary_key=bool(use_inline_pk), + ) pk_const = inspect(self.conn).get_pk_constraint("foo") eq_(pk_const["constrained_columns"], ["id"]) @@ -2295,9 +2306,6 @@ class BatchRoundTripMySQLTest(BatchRoundTripTest): def _datetime_server_default_fixture(self): return func.current_timestamp() - def test_drop_pk_col_readd_pk_col(self): - super().test_drop_pk_col_readd_pk_col() - @exclusions.fails() def test_drop_pk_col_readd_col_also_pk_const(self): super().test_drop_pk_col_readd_col_also_pk_const() @@ -2351,9 +2359,6 @@ class BatchRoundTripPostgresqlTest(BatchRoundTripTest): def _datetime_server_default_fixture(self): return func.current_timestamp() - def test_drop_pk_col_readd_pk_col(self): - super().test_drop_pk_col_readd_pk_col() - @exclusions.fails() def test_drop_pk_col_readd_col_also_pk_const(self): super().test_drop_pk_col_readd_col_also_pk_const() diff --git a/tests/test_op.py b/tests/test_op.py index 73f81085..704e2e91 100644 --- a/tests/test_op.py +++ b/tests/test_op.py @@ -24,6 +24,7 @@ from sqlalchemy.sql import text from sqlalchemy.sql.schema import quoted_name from alembic import op +from alembic import testing from alembic.operations import MigrateOperation from alembic.operations import Operations from alembic.operations import ops @@ -71,7 +72,33 @@ class OpTest(TestBase): 'ALTER TABLE "some.schema".somename ADD COLUMN colname VARCHAR' ) - def test_add_column_primary_key(self): + @testing.variation("use_inline", [True, False, "none"]) + def test_add_column_primary_key(self, use_inline): + context = op_fixture("postgresql") + + if use_inline.none: + op.add_column( + "somename", + Column("colname", String, primary_key=True), + ) + else: + op.add_column( + "somename", + Column("colname", String, primary_key=True), + inline_primary_key=bool(use_inline), + ) + + if use_inline.use_inline: + context.assert_( + "ALTER TABLE somename ADD COLUMN colname " + "VARCHAR NOT NULL PRIMARY KEY" + ) + else: + context.assert_( + "ALTER TABLE somename ADD COLUMN colname " "VARCHAR NOT NULL" + ) + + def test_add_column_primary_key_not_inline_by_default(self): context = op_fixture("postgresql") op.add_column( "somename", @@ -79,10 +106,19 @@ class OpTest(TestBase): ) context.assert_( - "ALTER TABLE somename ADD COLUMN colname " - "VARCHAR NOT NULL PRIMARY KEY" + "ALTER TABLE somename ADD COLUMN colname VARCHAR NOT NULL" ) + def test_add_column_inline_primary_key_no_pk_flag(self): + context = op_fixture("postgresql") + op.add_column( + "somename", + Column("colname", String), + inline_primary_key=True, + ) + + context.assert_("ALTER TABLE somename ADD COLUMN colname VARCHAR") + def test_rename_table_schema_hard_quoting(self): context = op_fixture("postgresql") op.rename_table( diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index fb20d170..d0304651 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -187,7 +187,11 @@ class PostgresqlOpTest(TestBase): def test_col_w_pk_is_serial(self): context = op_fixture("postgresql") - op.add_column("some_table", Column("q", Integer, primary_key=True)) + op.add_column( + "some_table", + Column("q", Integer, primary_key=True), + inline_primary_key=True, + ) context.assert_( "ALTER TABLE some_table ADD COLUMN q SERIAL NOT NULL PRIMARY KEY" )