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
(cherry picked from commit 17758bc8f6e8024e58177159941509eaa66bd074)

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 b893bf7245cfbbd268e190bef9c6ea4f14a094c3..002ca0efb701d616c70c11b2b77b2a6bf40901ad 100644 (file)
--- a/lib/sqlalchemy/sql/schema.py
+++ b/lib/sqlalchemy/sql/schema.py
@@ -2483,23 +2483,27 @@ class Column(DialectKWArgs, SchemaItem, ColumnClause[_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_,
@@ -2510,9 +2514,9 @@ class Column(DialectKWArgs, SchemaItem, ColumnClause[_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,
@@ -3951,7 +3955,7 @@ class Sequence(HasSchemaAttr, IdentityOptions, DefaultGenerator):
             order=self.order,
             data_type=self.data_type,
             optional=self.optional,
-            metadata=self.metadata,
+            metadata=None,
             for_update=self.for_update,
         )
 
@@ -4019,8 +4023,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__)
@@ -4077,11 +4081,6 @@ class DefaultClause(FetchedValue):
         self.arg = arg
         self.reflected = _reflected
 
-    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 f39510728ae75d45e712910ae696c6db96121cc7..8821ae02a4a969c342a77efcb2755434b6d5d94c 100644 (file)
--- a/test/sql/test_metadata.py
+++ b/test/sql/test_metadata.py
@@ -62,6 +62,7 @@ from sqlalchemy.testing import expect_raises_message
 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
@@ -812,6 +813,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
@@ -4521,10 +4658,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 (
@@ -4535,11 +4669,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)