Merge "Remove ORM elements from annotations at the schema level."

6 files changed, 152 insertions, 59 deletions

diff --git a/doc/build/changelog/unreleased_14/5001.rst b/doc/build/changelog/unreleased_14/5001.rst
new file mode 100644
index 000000000..986fec8d4
--- /dev/null
+++ b/doc/build/changelog/unreleased_14/5001.rst
@@ -0,0 +1,12 @@
+.. change::
+    :tags: bug, sql
+    :tickets: 5001
+
+    Fixed issue where when constructing constraints from ORM-bound columns,
+    primarily :class:`.ForeignKey` objects but also :class:`.UniqueConstraint`,
+    :class:`.CheckConstraint` and others, the ORM-level
+    :class:`.InstrumentedAttribute` is discarded entirely, and all ORM-level
+    annotations from the columns are removed; this is so that the constraints
+    are still fully pickleable without the ORM-level entities being pulled in.
+    These annotations are not necessary to be present at the schema/metadata
+    level.
diff --git a/lib/sqlalchemy/sql/coercions.py b/lib/sqlalchemy/sql/coercions.py
index 95aee0468..b45ef3991 100644
--- a/lib/sqlalchemy/sql/coercions.py
+++ b/lib/sqlalchemy/sql/coercions.py
@@ -147,6 +147,13 @@ class RoleImpl(object):
         raise exc.ArgumentError(msg, code=code)
 
 
+class _Deannotate(object):
+    def _post_coercion(self, resolved, **kw):
+        from .util import _deep_deannotate
+
+        return _deep_deannotate(resolved)
+
+
 class _StringOnly(object):
     def _resolve_for_clause_element(self, element, argname=None, **kw):
         return self._literal_coercion(element, **kw)
@@ -461,7 +468,9 @@ class TruncatedLabelImpl(_StringOnly, RoleImpl, roles.TruncatedLabelRole):
             return elements._truncated_label(element)
 
 
-class DDLExpressionImpl(_CoerceLiterals, RoleImpl, roles.DDLExpressionRole):
+class DDLExpressionImpl(
+    _Deannotate, _CoerceLiterals, RoleImpl, roles.DDLExpressionRole
+):
 
     _coerce_consts = True
 
@@ -470,11 +479,15 @@ class DDLExpressionImpl(_CoerceLiterals, RoleImpl, roles.DDLExpressionRole):
 
 
 class DDLConstraintColumnImpl(
-    _ReturnsStringKey, RoleImpl, roles.DDLConstraintColumnRole
+    _Deannotate, _ReturnsStringKey, RoleImpl, roles.DDLConstraintColumnRole
 ):
     pass
 
 
+class DDLReferredColumnImpl(DDLConstraintColumnImpl):
+    pass
+
+
 class LimitOffsetImpl(RoleImpl, roles.LimitOffsetRole):
     def _implicit_coercions(self, element, resolved, argname=None, **kw):
         if resolved is None:
diff --git a/lib/sqlalchemy/sql/elements.py b/lib/sqlalchemy/sql/elements.py
index ba615bc3f..eda31dc61 100644
--- a/lib/sqlalchemy/sql/elements.py
+++ b/lib/sqlalchemy/sql/elements.py
@@ -3246,7 +3246,7 @@ class BinaryExpression(ColumnElement):
         # refer to BinaryExpression directly and pass strings
         if isinstance(operator, util.string_types):
             operator = operators.custom_op(operator)
-        self._orig = (left, right)
+        self._orig = (hash(left), hash(right))
         self.left = left.self_group(against=operator)
         self.right = right.self_group(against=operator)
         self.operator = operator
@@ -3261,7 +3261,7 @@ class BinaryExpression(ColumnElement):
 
     def __bool__(self):
         if self.operator in (operator.eq, operator.ne):
-            return self.operator(hash(self._orig[0]), hash(self._orig[1]))
+            return self.operator(self._orig[0], self._orig[1])
         else:
             raise TypeError("Boolean value of this clause is not defined")
 
@@ -3946,7 +3946,12 @@ class Label(HasMemoized, roles.LabeledColumnExprRole, ColumnElement):
         return key, e
 
 
-class ColumnClause(roles.LabeledColumnExprRole, Immutable, ColumnElement):
+class ColumnClause(
+    roles.DDLReferredColumnRole,
+    roles.LabeledColumnExprRole,
+    Immutable,
+    ColumnElement,
+):
     """Represents a column expression from any textual string.
 
     The :class:`.ColumnClause`, a lightweight analogue to the
diff --git a/lib/sqlalchemy/sql/roles.py b/lib/sqlalchemy/sql/roles.py
index 55c52d401..caa68d0ab 100644
--- a/lib/sqlalchemy/sql/roles.py
+++ b/lib/sqlalchemy/sql/roles.py
@@ -186,4 +186,10 @@ class DDLExpressionRole(StructuralRole):
 
 
 class DDLConstraintColumnRole(SQLRole):
-    _role_name = "String column name or column object for DDL constraint"
+    _role_name = "String column name or column expression for DDL constraint"
+
+
+class DDLReferredColumnRole(DDLConstraintColumnRole):
+    _role_name = (
+        "String column name or Column object for DDL foreign key constraint"
+    )
diff --git a/lib/sqlalchemy/sql/schema.py b/lib/sqlalchemy/sql/schema.py
index c7d699a5f..44c8499b8 100644
--- a/lib/sqlalchemy/sql/schema.py
+++ b/lib/sqlalchemy/sql/schema.py
@@ -1721,21 +1721,14 @@ class ForeignKey(DialectKWArgs, SchemaItem):
 
         """
 
-        self._colspec = column
+        self._colspec = coercions.expect(roles.DDLReferredColumnRole, column)
+
         if isinstance(self._colspec, util.string_types):
             self._table_column = None
         else:
-            if hasattr(self._colspec, "__clause_element__"):
-                self._table_column = self._colspec.__clause_element__()
-            else:
-                self._table_column = self._colspec
+            self._table_column = self._colspec
 
-            if not isinstance(self._table_column, ColumnClause):
-                raise exc.ArgumentError(
-                    "String, Column, or Column-bound argument "
-                    "expected, got %r" % self._table_column
-                )
-            elif not isinstance(
+            if not isinstance(
                 self._table_column.table, (util.NoneType, TableClause)
             ):
                 raise exc.ArgumentError(
@@ -2692,25 +2685,6 @@ class Constraint(DialectKWArgs, SchemaItem):
         raise NotImplementedError()
 
 
-def _to_schema_column(element):
-    if hasattr(element, "__clause_element__"):
-        element = element.__clause_element__()
-    if not isinstance(element, Column):
-        raise exc.ArgumentError("schema.Column object expected")
-    return element
-
-
-def _to_schema_column_or_string(element):
-    if element is None:
-        return element
-    elif hasattr(element, "__clause_element__"):
-        element = element.__clause_element__()
-    if not isinstance(element, util.string_types + (ColumnElement,)):
-        msg = "Element %r is not a string name or column element"
-        raise exc.ArgumentError(msg % element)
-    return element
-
-
 class ColumnCollectionMixin(object):
 
     columns = None
@@ -2727,9 +2701,26 @@ class ColumnCollectionMixin(object):
         _autoattach = kw.pop("_autoattach", True)
         self._column_flag = kw.pop("_column_flag", False)
         self.columns = DedupeColumnCollection()
-        self._pending_colargs = [
-            _to_schema_column_or_string(c) for c in columns
-        ]
+
+        processed_expressions = kw.pop("_gather_expressions", None)
+        if processed_expressions is not None:
+            self._pending_colargs = []
+            for (
+                expr,
+                column,
+                strname,
+                add_element,
+            ) in coercions.expect_col_expression_collection(
+                roles.DDLConstraintColumnRole, columns
+            ):
+                self._pending_colargs.append(add_element)
+                processed_expressions.append(expr)
+        else:
+            self._pending_colargs = [
+                coercions.expect(roles.DDLConstraintColumnRole, column)
+                for column in columns
+            ]
+
         if _autoattach and self._pending_colargs:
             self._check_attach()
 
@@ -2917,7 +2908,6 @@ class CheckConstraint(ColumnCollectionConstraint):
         """
 
         self.sqltext = coercions.expect(roles.DDLExpressionRole, sqltext)
-
         columns = []
         visitors.traverse(self.sqltext, {}, {"column": columns.append})
 
@@ -3576,20 +3566,6 @@ class Index(DialectKWArgs, ColumnCollectionMixin, SchemaItem):
         """
         self.table = table = None
 
-        columns = []
-        processed_expressions = []
-        for (
-            expr,
-            column,
-            strname,
-            add_element,
-        ) in coercions.expect_col_expression_collection(
-            roles.DDLConstraintColumnRole, expressions
-        ):
-            columns.append(add_element)
-            processed_expressions.append(expr)
-
-        self.expressions = processed_expressions
         self.name = quoted_name(name, kw.pop("quote", None))
         self.unique = kw.pop("unique", False)
         _column_flag = kw.pop("_column_flag", False)
@@ -3603,10 +3579,14 @@ class Index(DialectKWArgs, ColumnCollectionMixin, SchemaItem):
 
         self._validate_dialect_kwargs(kw)
 
+        self.expressions = []
         # will call _set_parent() if table-bound column
         # objects are present
         ColumnCollectionMixin.__init__(
-            self, *columns, _column_flag=_column_flag
+            self,
+            *expressions,
+            _column_flag=_column_flag,
+            _gather_expressions=self.expressions
         )
 
         if table is not None:
diff --git a/test/sql/test_metadata.py b/test/sql/test_metadata.py
index 05e5ec3c2..3f4333750 100644
--- a/test/sql/test_metadata.py
+++ b/test/sql/test_metadata.py
@@ -35,6 +35,7 @@ from sqlalchemy.schema import AddConstraint
 from sqlalchemy.schema import CreateIndex
 from sqlalchemy.schema import DropIndex
 from sqlalchemy.sql import naming
+from sqlalchemy.sql import operators
 from sqlalchemy.sql.elements import _NONE_NAME
 from sqlalchemy.testing import assert_raises
 from sqlalchemy.testing import assert_raises_message
@@ -340,7 +341,8 @@ class MetaDataTest(fixtures.TestBase, ComparesTables):
         not_a_col = bindparam("x")
         assert_raises_message(
             exc.ArgumentError,
-            "String, Column, or Column-bound argument expected, got Bind",
+            "String column name or Column object for DDL foreign "
+            "key constraint expected, got .*Bind",
             ForeignKey,
             not_a_col,
         )
@@ -352,7 +354,8 @@ class MetaDataTest(fixtures.TestBase, ComparesTables):
 
         assert_raises_message(
             exc.ArgumentError,
-            "String, Column, or Column-bound argument expected, got Bind",
+            "String column name or Column object for DDL foreign "
+            "key constraint expected, got .*Foo",
             ForeignKey,
             Foo(),
         )
@@ -2656,7 +2659,7 @@ class ConstraintTest(fixtures.TestBase):
 
         idx = Index("bar", MyThing(), t.c.y)
 
-        is_(idx.expressions[0], expr1)
+        is_true(idx.expressions[0].compare(expr1))
         is_(idx.expressions[1], t.c.y)
 
     def test_table_references(self):
@@ -3415,13 +3418,87 @@ class ConstraintTest(fixtures.TestBase):
 
         assert_raises_message(
             exc.ArgumentError,
-            r"String column name or column object for DDL constraint "
+            r"String column name or column expression for DDL constraint "
             r"expected, got .*SomeClass",
             Index,
             "foo",
             SomeClass(),
         )
 
+    @testing.fixture
+    def no_pickle_annotated(self):
+        class NoPickle(object):
+            def __reduce__(self):
+                raise NotImplementedError()
+
+        class ClauseElement(operators.ColumnOperators):
+            def __init__(self, col):
+                self.col = col._annotate({"bar": NoPickle()})
+
+            def __clause_element__(self):
+                return self.col
+
+            def operate(self, op, *other, **kwargs):
+                return self.col.operate(op, *other, **kwargs)
+
+        m = MetaData()
+        t = Table("t", m, Column("q", Integer))
+        return t, ClauseElement(t.c.q)
+
+    def test_pickle_fk_annotated_col(self, no_pickle_annotated):
+
+        t, q_col = no_pickle_annotated
+
+        t2 = Table("t2", t.metadata, Column("p", ForeignKey(q_col)))
+        assert t2.c.p.references(t.c.q)
+
+        m2 = pickle.loads(pickle.dumps(t.metadata))
+
+        m2_t, m2_t2 = m2.tables["t"], m2.tables["t2"]
+
+        is_true(m2_t2.c.p.references(m2_t.c.q))
+
+    def test_pickle_uq_annotated_col(self, no_pickle_annotated):
+        t, q_col = no_pickle_annotated
+
+        t.append_constraint(UniqueConstraint(q_col))
+
+        m2 = pickle.loads(pickle.dumps(t.metadata))
+
+        const = [
+            c
+            for c in m2.tables["t"].constraints
+            if isinstance(c, UniqueConstraint)
+        ][0]
+
+        is_true(const.columns[0].compare(t.c.q))
+
+    def test_pickle_idx_expr_annotated_col(self, no_pickle_annotated):
+        t, q_col = no_pickle_annotated
+
+        expr = q_col > 5
+        t.append_constraint(Index("conditional_index", expr))
+
+        m2 = pickle.loads(pickle.dumps(t.metadata))
+
+        const = list(m2.tables["t"].indexes)[0]
+
+        is_true(const.expressions[0].compare(expr))
+
+    def test_pickle_ck_binary_annotated_col(self, no_pickle_annotated):
+        t, q_col = no_pickle_annotated
+
+        ck = CheckConstraint(q_col > 5)
+        t.append_constraint(ck)
+
+        m2 = pickle.loads(pickle.dumps(t.metadata))
+        const = [
+            c
+            for c in m2.tables["t"].constraints
+            if isinstance(c, CheckConstraint)
+        ][0]
+        is_true(const.sqltext.compare(ck.sqltext))
+
 
 class ColumnDefinitionTest(AssertsCompiledSQL, fixtures.TestBase):