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 (file)
index 0000000..436f602
--- /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 368bb0897d5f5f4d4611f820337023ef3907fbab..38894c2cf09a7afecb825debb2a6a5a1d97b8619 100644 (file)
--- 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 a44fd07dfa853c2f21387df08fa063d6e1665271..50573b6a347395e80548fe50e91dc74d87422e66 100644 (file)
--- 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)