- The behavioral contract of the :attr:`.ForeignKeyConstraint.columns`

collection has been made consistent; this attribute is now a
:class:`.ColumnCollection` like that of all other constraints and
is initialized at the point when the constraint is associated with
a :class:`.Table`.
fixes #3243

6 files changed, 138 insertions, 46 deletions

diff --git a/doc/build/changelog/changelog_10.rst b/doc/build/changelog/changelog_10.rst
index c0197a691..d0d025011 100644
--- a/doc/build/changelog/changelog_10.rst
+++ b/doc/build/changelog/changelog_10.rst
@@ -22,6 +22,20 @@
     on compatibility concerns, see :doc:`/changelog/migration_10`.
 
     .. change::
+        :tags: bug, sql
+        :tickets: 3243
+
+        The behavioral contract of the :attr:`.ForeignKeyConstraint.columns`
+        collection has been made consistent; this attribute is now a
+        :class:`.ColumnCollection` like that of all other constraints and
+        is initialized at the point when the constraint is associated with
+        a :class:`.Table`.
+
+        .. seealso::
+
+            :ref:`change_3243`
+
+    .. change::
         :tags: bug, orm
         :tickets: 3256
 
diff --git a/doc/build/changelog/migration_10.rst b/doc/build/changelog/migration_10.rst
index bc7fa139f..c4157266b 100644
--- a/doc/build/changelog/migration_10.rst
+++ b/doc/build/changelog/migration_10.rst
@@ -1427,7 +1427,21 @@ A :class:`.Table` can be set up for reflection by passing
 
 :ticket:`3027`
 
-
+.. _change_3243:
+
+ForeignKeyConstraint.columns is now a ColumnCollection
+------------------------------------------------------
+
+:attr:`.ForeignKeyConstraint.columns` was previously a plain list
+containing either strings or :class:`.Column` objects, depending on
+how the :class:`.ForeignKeyConstraint` was constructed and whether it was
+associated with a table.  The collection is now a :class:`.ColumnCollection`,
+and is only initialized after the :class:`.ForeignKeyConstraint` is
+associated with a :class:`.Table`.  A new accessor
+:attr:`.ForeignKeyConstraint.column_keys`
+is added to unconditionally return string keys for the local set of
+columns regardless of how the object was constructed or its current
+state.
 
 Dialect Changes
 ===============
diff --git a/lib/sqlalchemy/dialects/sqlite/base.py b/lib/sqlalchemy/dialects/sqlite/base.py
index 335b35c94..33003297c 100644
--- a/lib/sqlalchemy/dialects/sqlite/base.py
+++ b/lib/sqlalchemy/dialects/sqlite/base.py
@@ -646,8 +646,8 @@ class SQLiteDDLCompiler(compiler.DDLCompiler):
 
     def visit_foreign_key_constraint(self, constraint):
 
-        local_table = list(constraint._elements.values())[0].parent.table
-        remote_table = list(constraint._elements.values())[0].column.table
+        local_table = constraint.elements[0].parent.table
+        remote_table = constraint.elements[0].column.table
 
         if local_table.schema != remote_table.schema:
             return None
diff --git a/lib/sqlalchemy/sql/compiler.py b/lib/sqlalchemy/sql/compiler.py
index 8f3ede25f..b102f0240 100644
--- a/lib/sqlalchemy/sql/compiler.py
+++ b/lib/sqlalchemy/sql/compiler.py
@@ -2286,14 +2286,14 @@ class DDLCompiler(Compiled):
             formatted_name = self.preparer.format_constraint(constraint)
             if formatted_name is not None:
                 text += "CONSTRAINT %s " % formatted_name
-        remote_table = list(constraint._elements.values())[0].column.table
+        remote_table = list(constraint.elements)[0].column.table
         text += "FOREIGN KEY(%s) REFERENCES %s (%s)" % (
             ', '.join(preparer.quote(f.parent.name)
-                      for f in constraint._elements.values()),
+                      for f in constraint.elements),
             self.define_constraint_remote_table(
                 constraint, remote_table, preparer),
             ', '.join(preparer.quote(f.column.name)
-                      for f in constraint._elements.values())
+                      for f in constraint.elements)
         )
         text += self.define_constraint_match(constraint)
         text += self.define_constraint_cascades(constraint)
diff --git a/lib/sqlalchemy/sql/schema.py b/lib/sqlalchemy/sql/schema.py
index 96cabbf4f..8b2eb12f0 100644
--- a/lib/sqlalchemy/sql/schema.py
+++ b/lib/sqlalchemy/sql/schema.py
@@ -1804,7 +1804,7 @@ class ForeignKey(DialectKWArgs, SchemaItem):
                 match=self.match,
                 **self._unvalidated_dialect_kw
             )
-            self.constraint._elements[self.parent] = self
+            self.constraint._append_element(column, self)
             self.constraint._set_parent_with_dispatch(table)
         table.foreign_keys.add(self)
 
@@ -2489,7 +2489,7 @@ class CheckConstraint(Constraint):
         return self._schema_item_copy(c)
 
 
-class ForeignKeyConstraint(Constraint):
+class ForeignKeyConstraint(ColumnCollectionConstraint):
     """A table-level FOREIGN KEY constraint.
 
     Defines a single column or composite FOREIGN KEY ... REFERENCES
@@ -2564,9 +2564,10 @@ class ForeignKeyConstraint(Constraint):
             .. versionadded:: 0.9.2
 
         """
-        super(ForeignKeyConstraint, self).\
-            __init__(name, deferrable, initially, info=info, **dialect_kw)
 
+        Constraint.__init__(
+            self, name=name, deferrable=deferrable, initially=initially,
+            info=info, **dialect_kw)
         self.onupdate = onupdate
         self.ondelete = ondelete
         self.link_to_name = link_to_name
@@ -2575,14 +2576,12 @@ class ForeignKeyConstraint(Constraint):
         self.use_alter = use_alter
         self.match = match
 
-        self._elements = util.OrderedDict()
-
         # standalone ForeignKeyConstraint - create
         # associated ForeignKey objects which will be applied to hosted
         # Column objects (in col.foreign_keys), either now or when attached
         # to the Table for string-specified names
-        for col, refcol in zip(columns, refcolumns):
-            self._elements[col] = ForeignKey(
+        self.elements = [
+            ForeignKey(
                 refcol,
                 _constraint=self,
                 name=self.name,
@@ -2594,25 +2593,36 @@ class ForeignKeyConstraint(Constraint):
                 deferrable=self.deferrable,
                 initially=self.initially,
                 **self.dialect_kwargs
-            )
+            ) for refcol in refcolumns
+        ]
 
+        ColumnCollectionMixin.__init__(self, *columns)
         if table is not None:
+            if hasattr(self, "parent"):
+                assert table is self.parent
             self._set_parent_with_dispatch(table)
-        elif columns and \
-            isinstance(columns[0], Column) and \
-                columns[0].table is not None:
-            self._set_parent_with_dispatch(columns[0].table)
+
+    def _append_element(self, column, fk):
+        self.columns.add(column)
+        self.elements.append(fk)
+
+    @property
+    def _elements(self):
+        # legacy - provide a dictionary view of (column_key, fk)
+        return util.OrderedDict(
+            zip(self.column_keys, self.elements)
+        )
 
     @property
     def _referred_schema(self):
-        for elem in self._elements.values():
+        for elem in self.elements:
             return elem._referred_schema
         else:
             return None
 
     def _validate_dest_table(self, table):
         table_keys = set([elem._table_key()
-                          for elem in self._elements.values()])
+                          for elem in self.elements])
         if None not in table_keys and len(table_keys) > 1:
             elem0, elem1 = sorted(table_keys)[0:2]
             raise exc.ArgumentError(
@@ -2625,38 +2635,48 @@ class ForeignKeyConstraint(Constraint):
                 ))
 
     @property
-    def _col_description(self):
-        return ", ".join(self._elements)
+    def column_keys(self):
+        """Return a list of string keys representing the local
+        columns in this :class:`.ForeignKeyConstraint`.
 
-    @property
-    def columns(self):
-        return list(self._elements)
+        This list is either the original string arguments sent
+        to the constructor of the :class:`.ForeignKeyConstraint`,
+        or if the constraint has been initialized with :class:`.Column`
+        objects, is the string .key of each element.
+
+        .. versionadded:: 1.0.0
+
+        """
+        if hasattr(self, 'table'):
+            return self.columns.keys()
+        else:
+            return [
+                col.key if isinstance(col, ColumnElement)
+                else str(col) for col in self._pending_colargs
+            ]
 
     @property
-    def elements(self):
-        return list(self._elements.values())
+    def _col_description(self):
+        return ", ".join(self.column_keys)
 
     def _set_parent(self, table):
-        super(ForeignKeyConstraint, self)._set_parent(table)
-
-        self._validate_dest_table(table)
+        Constraint._set_parent(self, table)
 
-        for col, fk in self._elements.items():
-            # string-specified column names now get
-            # resolved to Column objects
-            if isinstance(col, util.string_types):
-                try:
-                    col = table.c[col]
-                except KeyError:
-                    raise exc.ArgumentError(
-                        "Can't create ForeignKeyConstraint "
-                        "on table '%s': no column "
-                        "named '%s' is present." % (table.description, col))
+        try:
+            ColumnCollectionConstraint._set_parent(self, table)
+        except KeyError as ke:
+            raise exc.ArgumentError(
+                "Can't create ForeignKeyConstraint "
+                "on table '%s': no column "
+                "named '%s' is present." % (table.description, ke.args[0]))
 
+        for col, fk in zip(self.columns, self.elements):
             if not hasattr(fk, 'parent') or \
                     fk.parent is not col:
                 fk._set_parent_with_dispatch(col)
 
+        self._validate_dest_table(table)
+
         if self.use_alter:
             def supports_alter(ddl, event, schema_item, bind, **kw):
                 return table in set(kw['tables']) and \
@@ -2669,14 +2689,14 @@ class ForeignKeyConstraint(Constraint):
 
     def copy(self, schema=None, target_table=None, **kw):
         fkc = ForeignKeyConstraint(
-            [x.parent.key for x in self._elements.values()],
+            [x.parent.key for x in self.elements],
             [x._get_colspec(
                 schema=schema,
                 table_name=target_table.name
                 if target_table is not None
                 and x._table_key() == x.parent.table.key
                 else None)
-             for x in self._elements.values()],
+             for x in self.elements],
             name=self.name,
             onupdate=self.onupdate,
             ondelete=self.ondelete,
@@ -2687,8 +2707,8 @@ class ForeignKeyConstraint(Constraint):
             match=self.match
         )
         for self_fk, other_fk in zip(
-                self._elements.values(),
-                fkc._elements.values()):
+                self.elements,
+                fkc.elements):
             self_fk._schema_item_copy(other_fk)
         return self._schema_item_copy(fkc)
 
diff --git a/test/sql/test_metadata.py b/test/sql/test_metadata.py
index 3c55242fd..3f24fd07d 100644
--- a/test/sql/test_metadata.py
+++ b/test/sql/test_metadata.py
@@ -227,6 +227,50 @@ class MetaDataTest(fixtures.TestBase, ComparesTables):
         fk1 = ForeignKeyConstraint(('foo', ), ('bar', ), table=t1)
         assert fk1 in t1.constraints
 
+    def test_fk_constraint_col_collection_w_table(self):
+        c1 = Column('foo', Integer)
+        c2 = Column('bar', Integer)
+        m = MetaData()
+        t1 = Table('t', m, c1, c2)
+        fk1 = ForeignKeyConstraint(('foo', ), ('bar', ), table=t1)
+        eq_(dict(fk1.columns), {"foo": c1})
+
+    def test_fk_constraint_col_collection_no_table(self):
+        fk1 = ForeignKeyConstraint(('foo', 'bat'), ('bar', 'hoho'))
+        eq_(dict(fk1.columns), {})
+        eq_(fk1.column_keys, ['foo', 'bat'])
+        eq_(fk1._col_description, 'foo, bat')
+        eq_(fk1._elements, {"foo": fk1.elements[0], "bat": fk1.elements[1]})
+
+    def test_fk_constraint_col_collection_no_table_real_cols(self):
+        c1 = Column('foo', Integer)
+        c2 = Column('bar', Integer)
+        fk1 = ForeignKeyConstraint((c1, ), (c2, ))
+        eq_(dict(fk1.columns), {})
+        eq_(fk1.column_keys, ['foo'])
+        eq_(fk1._col_description, 'foo')
+        eq_(fk1._elements, {"foo": fk1.elements[0]})
+
+    def test_fk_constraint_col_collection_added_to_table(self):
+        c1 = Column('foo', Integer)
+        m = MetaData()
+        fk1 = ForeignKeyConstraint(('foo', ), ('bar', ))
+        Table('t', m, c1, fk1)
+        eq_(dict(fk1.columns), {"foo": c1})
+        eq_(fk1._elements, {"foo": fk1.elements[0]})
+
+    def test_fk_constraint_col_collection_via_fk(self):
+        fk = ForeignKey('bar')
+        c1 = Column('foo', Integer, fk)
+        m = MetaData()
+        t1 = Table('t', m, c1)
+        fk1 = fk.constraint
+        eq_(fk1.column_keys, ['foo'])
+        assert fk1 in t1.constraints
+        eq_(fk1.column_keys, ['foo'])
+        eq_(dict(fk1.columns), {"foo": c1})
+        eq_(fk1._elements, {"foo": fk})
+
     def test_fk_no_such_parent_col_error(self):
         meta = MetaData()
         a = Table('a', meta, Column('a', Integer))