From: goutamadwant Date: Mon, 10 Aug 2026 13:27:21 +0000 (-0400) Subject: Copy column defaults in Table.to_metadata X-Git-Url: http://git.ipfire.org/index.cgi?a=commitdiff_plain;h=df5583de9c44ae785a61e75f626bfb16b615420e;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git Copy column defaults in Table.to_metadata ``Table.to_metadata()`` reused the original column's ``default``, ``onupdate``, ``server_default``, and ``server_onupdate`` objects. Attaching those objects to the copied column changed their ``column`` reference, leaving the original column's defaults associated with the copy. This change copies each default generator before constructing the new column. Sequence defaults are copied into the target metadata collection so both the original and copied metadata retain their own sequence objects. Regression tests verify ownership for client-side defaults, server-side defaults, on-update values, and sequence defaults. Existing merge-copy expectations now assert that each copied default remains associated with its own column. Fixes: #13481 Closes: #13482 Pull-request: https://github.com/sqlalchemy/sqlalchemy/pull/13482 Pull-request-sha: 3dfbe0d91c99cd33bb059937478b9ecb398d3613 Change-Id: Ifceb7802028ff8560937b265c2f55ed93e657fc5 --- diff --git a/doc/build/changelog/unreleased_20/13481.rst b/doc/build/changelog/unreleased_20/13481.rst new file mode 100644 index 0000000000..436f602148 --- /dev/null +++ b/doc/build/changelog/unreleased_20/13481.rst @@ -0,0 +1,12 @@ +.. change:: + :tags: bug, schema + :tickets: 13481 + + Fixed an issue where :meth:`_schema.Table.to_metadata` reused column + default and on-update objects, causing the defaults on the original + columns to refer to the copied columns. Default generators, including + sequences, and server-side defaults are now copied and remain associated + with their respective columns and metadata collections. Applications that + inspected these objects will now see distinct defaults on the copied table + instead of the objects owned by the original table. Pull request courtesy + Goutam Adwant. diff --git a/lib/sqlalchemy/sql/schema.py b/lib/sqlalchemy/sql/schema.py index 368bb0897d..38894c2cf0 100644 --- a/lib/sqlalchemy/sql/schema.py +++ b/lib/sqlalchemy/sql/schema.py @@ -2758,23 +2758,27 @@ class Column(DialectKWArgs, SchemaItem, ColumnClause[_T], Named[_T]): dialect_option_value ) + default = self.default + if default is not None: + default = default._copy() + onupdate = self.onupdate + if onupdate is not None: + onupdate = onupdate._copy() server_default = self.server_default server_onupdate = self.server_onupdate if isinstance(server_default, (Computed, Identity)): - # TODO: likely should be copied in all cases - # TODO: if a Sequence, we would need to transfer the Sequence - # .metadata as well args.append(server_default._copy(**kw)) server_default = server_onupdate = None + else: + if server_default is not None: + server_default = server_default._copy() + if server_onupdate is not None: + server_onupdate = server_onupdate._copy() type_ = self.type if isinstance(type_, SchemaEventTarget): type_ = type_.copy(**kw) - # TODO: DefaultGenerator is not copied here! it's just used again - # with _set_parent() pointing to the old column. see the new - # use of _copy() in the new _merge() method - c = self._constructor( name=self.name, type_=type_, @@ -2785,9 +2789,9 @@ class Column(DialectKWArgs, SchemaItem, ColumnClause[_T], Named[_T]): # quote=self.quote, # disabled 2013-08-27 (commit 031ef080) index=self.index, autoincrement=self.autoincrement, - default=self.default, + default=default, server_default=server_default, - onupdate=self.onupdate, + onupdate=onupdate, server_onupdate=server_onupdate, doc=self.doc, comment=self.comment, @@ -4304,7 +4308,7 @@ class Sequence(HasSchemaAttr, IdentityOptions, DefaultGenerator): schema=self.schema, data_type=self.data_type, optional=self.optional, - metadata=self.metadata, + metadata=None, for_update=self.for_update, **self._as_dict(), **self.dialect_kwargs, @@ -4389,8 +4393,8 @@ class FetchedValue(SchemaEventTarget): else: return self._clone(for_update) - def _copy(self) -> FetchedValue: - return FetchedValue(self.for_update) + def _copy(self) -> Self: + return self._clone(self.for_update) def _clone(self, for_update: bool) -> Self: n = self.__class__.__new__(self.__class__) @@ -4456,11 +4460,6 @@ class DefaultClause(FetchedValue): and self.arg.monotonic ) - def _copy(self) -> DefaultClause: - return DefaultClause( - arg=self.arg, for_update=self.for_update, _reflected=self.reflected - ) - def __repr__(self) -> str: return "DefaultClause(%r, for_update=%r)" % (self.arg, self.for_update) diff --git a/test/sql/test_metadata.py b/test/sql/test_metadata.py index a44fd07dfa..50573b6a34 100644 --- a/test/sql/test_metadata.py +++ b/test/sql/test_metadata.py @@ -70,6 +70,7 @@ from sqlalchemy.testing import expect_warnings from sqlalchemy.testing import fixtures from sqlalchemy.testing import is_ from sqlalchemy.testing import is_false +from sqlalchemy.testing import is_not_ from sqlalchemy.testing import is_true from sqlalchemy.testing import mock from sqlalchemy.testing import Variation @@ -884,6 +885,142 @@ class MetaDataTest(fixtures.TestBase, ComparesTables): class ToMetaDataTest(fixtures.TestBase, AssertsCompiledSQL, ComparesTables): + def test_defaults_are_copied(self): + table = Table( + "t", + MetaData(), + Column( + "x", + Integer, + default=1, + onupdate=2, + server_default="3", + server_onupdate="4", + ), + ) + + copied = table.to_metadata(MetaData()) + + is_not_(table.c.x.default, copied.c.x.default) + is_(table.c.x.default.column, table.c.x) + is_(copied.c.x.default.column, copied.c.x) + is_not_(table.c.x.onupdate, copied.c.x.onupdate) + is_(table.c.x.onupdate.column, table.c.x) + is_(copied.c.x.onupdate.column, copied.c.x) + is_not_(table.c.x.server_default, copied.c.x.server_default) + is_(table.c.x.server_default.column, table.c.x) + is_(copied.c.x.server_default.column, copied.c.x) + is_not_(table.c.x.server_onupdate, copied.c.x.server_onupdate) + is_(table.c.x.server_onupdate.column, table.c.x) + is_(copied.c.x.server_onupdate.column, copied.c.x) + + def test_callable_and_expression_defaults_are_copied(self): + def default_value(): + return 1 + + table = Table( + "t", + MetaData(), + Column("callable", Integer, default=default_value), + Column("expression", Integer, default=func.some_default()), + ) + + copied = table.to_metadata(MetaData()) + + is_not_(table.c.callable.default, copied.c.callable.default) + is_(table.c.callable.default.column, table.c.callable) + is_(copied.c.callable.default.column, copied.c.callable) + is_not_(table.c.expression.default, copied.c.expression.default) + is_(table.c.expression.default.column, table.c.expression) + is_(copied.c.expression.default.column, copied.c.expression) + + def test_fetched_value_subclass_is_copied(self): + class MyFetchedValue(schema.FetchedValue): + def __init__(self, tag, for_update=False): + super().__init__(for_update) + self.tag = tag + + table = Table( + "t", + MetaData(), + Column("x", Integer, server_default=MyFetchedValue("custom")), + ) + + copied = table.to_metadata(MetaData()) + + assert isinstance(copied.c.x.server_default, MyFetchedValue) + eq_(copied.c.x.server_default.tag, "custom") + is_(table.c.x.server_default.column, table.c.x) + is_(copied.c.x.server_default.column, copied.c.x) + + def test_sequence_default_is_copied(self): + metadata = MetaData() + sequence = Sequence("x_seq") + table = Table("t", metadata, Column("x", Integer, sequence)) + + copied_metadata = MetaData() + copied = table.to_metadata(copied_metadata) + copied_sequence = copied.c.x.default + + is_not_(sequence, copied_sequence) + is_(sequence.column, table.c.x) + is_(sequence.metadata, metadata) + is_(metadata._sequences["x_seq"], sequence) + is_(copied_sequence.column, copied.c.x) + is_(copied_sequence.metadata, copied_metadata) + is_(copied_metadata._sequences["x_seq"], copied_sequence) + + def test_sequence_schema_is_preserved(self): + table = Table( + "t", + MetaData(), + Column("x", Integer, Sequence("x_seq", schema="foo")), + schema="foo", + ) + + copied_metadata = MetaData() + copied = table.to_metadata(copied_metadata, schema="bar") + + eq_(copied.c.x.default.schema, "foo") + is_(copied.c.x.default.metadata, copied_metadata) + is_(copied_metadata._sequences["foo.x_seq"], copied.c.x.default) + + def test_shared_sequence_is_copied_per_table(self): + metadata = MetaData() + sequence = Sequence("shared_seq") + table_one = Table("t1", metadata, Column("x", Integer, sequence)) + table_two = Table("t2", metadata, Column("y", Integer, sequence)) + + copied_metadata = MetaData() + copied_one = table_one.to_metadata(copied_metadata) + copied_two = table_two.to_metadata(copied_metadata) + + is_not_(copied_one.c.x.default, copied_two.c.y.default) + is_(copied_one.c.x.default.metadata, copied_metadata) + is_(copied_two.c.y.default.metadata, copied_metadata) + is_( + copied_metadata._sequences["shared_seq"], + copied_two.c.y.default, + ) + + def test_merge_sequence_registers_with_target_metadata(self): + source_metadata = MetaData() + source_sequence = Sequence("x_seq") + source = Table( + "source", + source_metadata, + Column("x", Integer, source_sequence), + ) + target_column = Column("x", Integer) + + source.c.x._merge(target_column) + target_metadata = MetaData() + target = Table("target", target_metadata, target_column) + + is_(source_metadata._sequences["x_seq"], source_sequence) + is_(target.c.x.default.metadata, target_metadata) + is_(target_metadata._sequences["x_seq"], target.c.x.default) + @testing.fixture def copy_fixture(self, metadata): from sqlalchemy.testing.schema import Table @@ -4856,10 +4993,7 @@ class ColumnDefinitionTest(AssertsCompiledSQL, fixtures.TestBase): is_(default.column, col) elif isinstance(value, Sequence): default = col.default - - # TODO: sequence mutated in place - is_(default.column, target_copy) - + is_(default.column, col) assert isinstance(default, type(value)) elif paramname in ( @@ -4870,11 +5004,7 @@ class ColumnDefinitionTest(AssertsCompiledSQL, fixtures.TestBase): ): default = getattr(col, paramname) is_(default.arg, value) - - # TODO: _copy() seems to note that it isn't copying - # server defaults or defaults outside of Computed, Identity, - # so here it's getting mutated in place. this is a bug - is_(default.column, target_copy) + is_(default.column, col) elif paramname in ("info",): eq_(col.info, value)