Merge "Warn when sorted_tables is not actually sorting"

3 files changed, 81 insertions, 25 deletions

diff --git a/lib/sqlalchemy/sql/ddl.py b/lib/sqlalchemy/sql/ddl.py
index 51526173f..569030651 100644
--- a/lib/sqlalchemy/sql/ddl.py
+++ b/lib/sqlalchemy/sql/ddl.py
@@ -949,7 +949,9 @@ class SchemaDropper(DDLBase):
         self.connection.execute(DropSequence(sequence))
 
 
-def sort_tables(tables, skip_fn=None, extra_dependencies=None):
+def sort_tables(
+    tables, skip_fn=None, extra_dependencies=None,
+):
     """sort a collection of :class:`_schema.Table` objects based on dependency
     .
 
@@ -963,16 +965,29 @@ def sort_tables(tables, skip_fn=None, extra_dependencies=None):
 
     .. warning::
 
-        The :func:`.sort_tables` function cannot by itself accommodate
-        automatic resolution of dependency cycles between tables, which
-        are usually caused by mutually dependent foreign key constraints.
-        To resolve these cycles, either the
-        :paramref:`_schema.ForeignKeyConstraint.use_alter`
-        parameter may be applied
-        to those constraints, or use the
-        :func:`_expression.sort_tables_and_constraints`
-        function which will break
-        out foreign key constraints involved in cycles separately.
+        The :func:`._schema.sort_tables` function cannot by itself
+        accommodate automatic resolution of dependency cycles between
+        tables, which are usually caused by mutually dependent foreign key
+        constraints. When these cycles are detected, the foreign keys
+        of these tables are omitted from consideration in the sort.
+        A warning is emitted when this condition occurs, which will be an
+        exception raise in a future release.   Tables which are not part
+        of the cycle will still be returned in dependency order.
+
+        To resolve these cycles, the
+        :paramref:`_schema.ForeignKeyConstraint.use_alter` parameter may be
+        applied to those constraints which create a cycle.  Alternatively,
+        the :func:`_schema.sort_tables_and_constraints` function will
+        automatically return foreign key constraints in a separate
+        collection when cycles are detected so that they may be applied
+        to a schema separately.
+
+        .. versionchanged:: 1.3.17 - a warning is emitted when
+           :func:`_schema.sort_tables` cannot perform a proper sort due to
+           cyclical dependencies.  This will be an exception in a future
+           release.  Additionally, the sort will continue to return
+           other tables not involved in the cycle in dependency order
+           which was not the case previously.
 
     :param tables: a sequence of :class:`_schema.Table` objects.
 
@@ -990,7 +1005,7 @@ def sort_tables(tables, skip_fn=None, extra_dependencies=None):
 
         :func:`.sort_tables_and_constraints`
 
-        :meth:`_schema.MetaData.sorted_tables` - uses this function to sort
+        :attr:`_schema.MetaData.sorted_tables` - uses this function to sort
 
 
     """
@@ -1010,14 +1025,17 @@ def sort_tables(tables, skip_fn=None, extra_dependencies=None):
     return [
         t
         for (t, fkcs) in sort_tables_and_constraints(
-            tables, filter_fn=_skip_fn, extra_dependencies=extra_dependencies
+            tables,
+            filter_fn=_skip_fn,
+            extra_dependencies=extra_dependencies,
+            _warn_for_cycles=True,
         )
         if t is not None
     ]
 
 
 def sort_tables_and_constraints(
-    tables, filter_fn=None, extra_dependencies=None
+    tables, filter_fn=None, extra_dependencies=None, _warn_for_cycles=False
 ):
     """sort a collection of :class:`_schema.Table`  /
     :class:`_schema.ForeignKeyConstraint`
@@ -1101,9 +1119,20 @@ def sort_tables_and_constraints(
             )
         )
     except exc.CircularDependencyError as err:
+        if _warn_for_cycles:
+            util.warn(
+                "Cannot correctly sort tables; there are unresolvable cycles "
+                'between tables "%s", which is usually caused by mutually '
+                "dependent foreign key constraints.  Foreign key constraints "
+                "involving these tables will not be considered; this warning "
+                "may raise an error in a future release."
+                % (", ".join(sorted(t.fullname for t in err.cycles)),)
+            )
         for edge in err.edges:
             if edge in mutable_dependencies:
                 table = edge[1]
+                if table not in err.cycles:
+                    continue
                 can_remove = [
                     fkc
                     for fkc in table.foreign_key_constraints
diff --git a/lib/sqlalchemy/sql/schema.py b/lib/sqlalchemy/sql/schema.py
index b7ac16b0a..08dc487d4 100644
--- a/lib/sqlalchemy/sql/schema.py
+++ b/lib/sqlalchemy/sql/schema.py
@@ -4087,15 +4087,29 @@ class MetaData(SchemaItem):
 
         .. warning::
 
-            The :attr:`.sorted_tables` accessor cannot by itself accommodate
-            automatic resolution of dependency cycles between tables, which
-            are usually caused by mutually dependent foreign key constraints.
-            To resolve these cycles, either the
-            :paramref:`_schema.ForeignKeyConstraint.use_alter`
-            parameter may be
-            applied to those constraints, or use the
-            :func:`_schema.sort_tables_and_constraints` function which will
-            break out foreign key constraints involved in cycles separately.
+            The :attr:`.MetaData.sorted_tables` attribute cannot by itself
+            accommodate automatic resolution of dependency cycles between
+            tables, which are usually caused by mutually dependent foreign key
+            constraints. When these cycles are detected, the foreign keys
+            of these tables are omitted from consideration in the sort.
+            A warning is emitted when this condition occurs, which will be an
+            exception raise in a future release.   Tables which are not part
+            of the cycle will still be returned in dependency order.
+
+            To resolve these cycles, the
+            :paramref:`_schema.ForeignKeyConstraint.use_alter` parameter may be
+            applied to those constraints which create a cycle.  Alternatively,
+            the :func:`_schema.sort_tables_and_constraints` function will
+            automatically return foreign key constraints in a separate
+            collection when cycles are detected so that they may be applied
+            to a schema separately.
+
+            .. versionchanged:: 1.3.17 - a warning is emitted when
+               :attr:`.MetaData.sorted_tables` cannot perform a proper sort
+               due to cyclical dependencies.  This will be an exception in a
+               future release.  Additionally, the sort will continue to return
+               other tables not involved in the cycle in dependency order which
+               was not the case previously.
 
         .. seealso::
 
diff --git a/lib/sqlalchemy/testing/fixtures.py b/lib/sqlalchemy/testing/fixtures.py
index 1fa7daed6..041daf35e 100644
--- a/lib/sqlalchemy/testing/fixtures.py
+++ b/lib/sqlalchemy/testing/fixtures.py
@@ -20,6 +20,7 @@ from .. import event
 from .. import util
 from ..ext.declarative import declarative_base
 from ..ext.declarative import DeclarativeMeta
+from ..schema import sort_tables_and_constraints
 
 
 # whether or not we use unittest changes things dramatically,
@@ -199,7 +200,15 @@ class TablesTest(TestBase):
         # no need to run deletes if tables are recreated on setup
         if self.run_define_tables != "each" and self.run_deletes == "each":
             with self.bind.begin() as conn:
-                for table in reversed(self.metadata.sorted_tables):
+                for table in reversed(
+                    [
+                        t
+                        for (t, fks) in sort_tables_and_constraints(
+                            self.metadata.tables.values()
+                        )
+                        if t is not None
+                    ]
+                ):
                     try:
                         conn.execute(table.delete())
                     except sa.exc.DBAPIError as ex:
@@ -272,7 +281,11 @@ class TablesTest(TestBase):
                 table = cls.tables[table]
             headers[table] = data[0]
             rows[table] = data[1:]
-        for table in cls.metadata.sorted_tables:
+        for table, fks in sort_tables_and_constraints(
+            cls.metadata.tables.values()
+        ):
+            if table is None:
+                continue
             if table not in headers:
                 continue
             cls.bind.execute(