- repair a regression caused by #3282, where we no longer were

applying any topological sort to tables on SQLite.  See the
changelog for details, but we now continue to sort
tables for SQLite on DROP, prohibit the sort from considering
alter, and only warn if we encounter an unresolvable cycle, in
which case, then we forego the ordering.  use_alter as always
is used to break such a cycle.
fixes #3378

7 files changed, 374 insertions, 98 deletions

diff --git a/doc/build/changelog/changelog_10.rst b/doc/build/changelog/changelog_10.rst
index 52b35c931..19c956079 100644
--- a/doc/build/changelog/changelog_10.rst
+++ b/doc/build/changelog/changelog_10.rst
@@ -19,6 +19,42 @@
     :version: 1.0.1
 
     .. change::
+        :tags: bug, sqlite
+        :tickets: 3378
+
+        Fixed a regression due to :ticket:`3282`, where due to the fact that
+        we attempt to assume the availability of ALTER when creating/dropping
+        schemas, in the case of SQLite we simply said to not worry about
+        foreign keys at all, since ALTER is not available, when creating
+        and dropping tables.  This meant that the sorting of tables was
+        basically skipped in the case of SQLite, and for the vast majority
+        of SQLite use cases, this is not an issue.
+
+        However, users who were doing DROPs on SQLite
+        with tables that contained data and with referential integrity
+        turned on would then experience errors, as the
+        dependency sorting *does* matter in the case of DROP with
+        enforced constraints, when those tables have data (SQLite will still
+        happily let you create foreign keys to nonexistent tables and drop
+        tables referring to existing ones with constraints enabled, as long as
+        there's no data being referenced).
+
+        In order to maintain the new feature of :ticket:`3282` while still
+        allowing a SQLite DROP operation to maintain ordering, we now
+        do the sort with full FKs taken under consideration, and if we encounter
+        an unresolvable cycle, only *then* do we forego attempting to sort
+        the tables; we instead emit a warning and go with the unsorted list.
+        If an environment needs both ordered DROPs *and* has foreign key
+        cycles, then the warning notes they will need to restore the
+        ``use_alter`` flag to their :class:`.ForeignKey` and
+        :class:`.ForeignKeyConstraint` objects so that just those objects will
+        be omitted from the dependency sort.
+
+        .. seealso::
+
+            :ref:`feature_3282` - contains an updated note about SQLite.
+
+    .. change::
         :tags: bug, core
         :tickets: 3372
 
diff --git a/doc/build/changelog/migration_10.rst b/doc/build/changelog/migration_10.rst
index 49ca37b2b..392c3f66f 100644
--- a/doc/build/changelog/migration_10.rst
+++ b/doc/build/changelog/migration_10.rst
@@ -609,8 +609,8 @@ than the integer value.
 
 .. _feature_3282:
 
-The ``use_alter`` flag on ``ForeignKeyConstraint`` is no longer needed
-----------------------------------------------------------------------
+The ``use_alter`` flag on ``ForeignKeyConstraint`` is (usually) no longer needed
+--------------------------------------------------------------------------------
 
 The :meth:`.MetaData.create_all` and :meth:`.MetaData.drop_all` methods will
 now make use of a system that automatically renders an ALTER statement
@@ -629,6 +629,16 @@ The :paramref:`.ForeignKeyConstraint.use_alter` and
 the same effect of establishing those constraints for which ALTER is
 required during a CREATE/DROP scenario.
 
+As of version 1.0.1, special logic takes over in the case of SQLite, which
+does not support ALTER, in the case that during a DROP, the given tables have
+an unresolvable cycle; in this case a warning is emitted, and the tables
+are dropped with **no** ordering, which is usually fine on SQLite unless
+constraints are enabled. To resolve the warning and proceed with at least
+a partial ordering on a SQLite database, particuarly one where constraints
+are enabled, re-apply "use_alter" flags to those
+:class:`.ForeignKey` and :class:`.ForeignKeyConstraint` objects which should
+be explicitly omitted from the sort.
+
 .. seealso::
 
     :ref:`use_alter` - full description of the new behavior.
diff --git a/lib/sqlalchemy/sql/ddl.py b/lib/sqlalchemy/sql/ddl.py
index bbac6456e..a0841b13c 100644
--- a/lib/sqlalchemy/sql/ddl.py
+++ b/lib/sqlalchemy/sql/ddl.py
@@ -803,32 +803,50 @@ class SchemaDropper(DDLBase):
             tables = list(metadata.tables.values())
 
         try:
+            unsorted_tables = [t for t in tables if self._can_drop_table(t)]
             collection = reversed(
                 sort_tables_and_constraints(
-                    [t for t in tables if self._can_drop_table(t)],
-                    filter_fn=
-                    lambda constraint: True if not self.dialect.supports_alter
-                    else False if constraint.name is None
+                    unsorted_tables,
+                    filter_fn=lambda constraint: False
+                    if not self.dialect.supports_alter
+                    or constraint.name is None
                     else None
                 )
             )
         except exc.CircularDependencyError as err2:
-            util.raise_from_cause(
-                exc.CircularDependencyError(
-                    err2.args[0],
-                    err2.cycles, err2.edges,
-                    msg="Can't sort tables for DROP; an "
+            if not self.dialect.supports_alter:
+                util.warn(
+                    "Can't sort tables for DROP; an "
                     "unresolvable foreign key "
-                    "dependency exists between tables: %s.  Please ensure "
-                    "that the ForeignKey and ForeignKeyConstraint objects "
-                    "involved in the cycle have "
-                    "names so that they can be dropped using DROP CONSTRAINT."
+                    "dependency exists between tables: %s, and backend does "
+                    "not support ALTER.  To restore at least a partial sort, "
+                    "apply use_alter=True to ForeignKey and "
+                    "ForeignKeyConstraint "
+                    "objects involved in the cycle to mark these as known "
+                    "cycles that will be ignored."
                     % (
                         ", ".join(sorted([t.fullname for t in err2.cycles]))
                     )
+                )
+                collection = [(t, ()) for t in unsorted_tables]
+            else:
+                util.raise_from_cause(
+                    exc.CircularDependencyError(
+                        err2.args[0],
+                        err2.cycles, err2.edges,
+                        msg="Can't sort tables for DROP; an "
+                        "unresolvable foreign key "
+                        "dependency exists between tables: %s.  Please ensure "
+                        "that the ForeignKey and ForeignKeyConstraint objects "
+                        "involved in the cycle have "
+                        "names so that they can be dropped using "
+                        "DROP CONSTRAINT."
+                        % (
+                            ", ".join(sorted([t.fullname for t in err2.cycles]))
+                        )
 
+                    )
                 )
-            )
 
         seq_coll = [
             s
diff --git a/lib/sqlalchemy/testing/assertsql.py b/lib/sqlalchemy/testing/assertsql.py
index e544adad2..243493607 100644
--- a/lib/sqlalchemy/testing/assertsql.py
+++ b/lib/sqlalchemy/testing/assertsql.py
@@ -188,21 +188,27 @@ class DialectSQL(CompiledSQL):
     def _compile_dialect(self, execute_observed):
         return execute_observed.context.dialect
 
+    def _compare_no_space(self, real_stmt, received_stmt):
+        stmt = re.sub(r'[\n\t]', '', real_stmt)
+        return received_stmt == stmt
+
     def _received_statement(self, execute_observed):
         received_stmt, received_params = super(DialectSQL, self).\
             _received_statement(execute_observed)
+
+        # TODO: why do we need this part?
         for real_stmt in execute_observed.statements:
-            if real_stmt.statement == received_stmt:
+            if self._compare_no_space(real_stmt.statement, received_stmt):
                 break
         else:
             raise AssertionError(
                 "Can't locate compiled statement %r in list of "
                 "statements actually invoked" % received_stmt)
+
         return received_stmt, execute_observed.context.compiled_parameters
 
     def _compare_sql(self, execute_observed, received_statement):
         stmt = re.sub(r'[\n\t]', '', self.statement)
-
         # convert our comparison statement to have the
         # paramstyle of the received
         paramstyle = execute_observed.context.dialect.paramstyle
diff --git a/lib/sqlalchemy/testing/engines.py b/lib/sqlalchemy/testing/engines.py
index 3a8303546..8bd1becbf 100644
--- a/lib/sqlalchemy/testing/engines.py
+++ b/lib/sqlalchemy/testing/engines.py
@@ -98,7 +98,14 @@ def drop_all_tables(metadata, bind):
     testing_reaper.close_all()
     if hasattr(bind, 'close'):
         bind.close()
-    metadata.drop_all(bind)
+
+    if not config.db.dialect.supports_alter:
+        from . import assertions
+        with assertions.expect_warnings(
+                "Can't sort tables", assert_=False):
+            metadata.drop_all(bind)
+    else:
+        metadata.drop_all(bind)
 
 
 @decorator
diff --git a/lib/sqlalchemy/testing/util.py b/lib/sqlalchemy/testing/util.py
index 6d6fa094e..1842e58a5 100644
--- a/lib/sqlalchemy/testing/util.py
+++ b/lib/sqlalchemy/testing/util.py
@@ -185,6 +185,7 @@ def provide_metadata(fn, *args, **kw):
     """Provide bound MetaData for a single test, dropping afterwards."""
 
     from . import config
+    from . import engines
     from sqlalchemy import schema
 
     metadata = schema.MetaData(config.db)
@@ -194,7 +195,7 @@ def provide_metadata(fn, *args, **kw):
     try:
         return fn(*args, **kw)
     finally:
-        metadata.drop_all()
+        engines.drop_all_tables(metadata, config.db)
         self.metadata = prev_meta
 
 
diff --git a/test/sql/test_constraints.py b/test/sql/test_constraints.py
index d024e1a27..47f81a50c 100644
--- a/test/sql/test_constraints.py
+++ b/test/sql/test_constraints.py
@@ -8,8 +8,9 @@ from sqlalchemy.testing import fixtures, AssertsExecutionResults, \
 from sqlalchemy import testing
 from sqlalchemy.engine import default
 from sqlalchemy.testing import engines
+from sqlalchemy.testing.assertions import expect_warnings
 from sqlalchemy.testing import eq_
-from sqlalchemy.testing.assertsql import AllOf, RegexSQL, CompiledSQL
+from sqlalchemy.testing.assertsql import AllOf, RegexSQL, CompiledSQL, DialectSQL
 from sqlalchemy.sql import table, column
 
 
@@ -84,9 +85,11 @@ class ConstraintGenTest(fixtures.TestBase, AssertsExecutionResults):
                 metadata.drop_all, testing.db
             )
         else:
-
-            with self.sql_execution_asserter() as asserter:
-                metadata.drop_all(testing.db, checkfirst=False)
+            with expect_warnings(
+                    "Can't sort tables for DROP; an unresolvable "
+                    "foreign key dependency "):
+                with self.sql_execution_asserter() as asserter:
+                    metadata.drop_all(testing.db, checkfirst=False)
 
             asserter.assert_(
                 AllOf(
@@ -109,10 +112,11 @@ class ConstraintGenTest(fixtures.TestBase, AssertsExecutionResults):
             Column('id', Integer, primary_key=True),
             Column("aid", Integer),
             ForeignKeyConstraint(["aid"], ["a.id"], name="bfk"))
-        self._assert_cyclic_constraint(metadata, auto=True)
+        self._assert_cyclic_constraint(
+            metadata, auto=True, sqlite_warning=True)
 
     @testing.provide_metadata
-    def test_fk_column_auto_alter_constraint_create(self):
+    def test_fk_column_auto_alter_inline_constraint_create(self):
         metadata = self.metadata
 
         Table("a", metadata,
@@ -125,7 +129,24 @@ class ConstraintGenTest(fixtures.TestBase, AssertsExecutionResults):
                      ForeignKey("a.id", name="bfk")
                      ),
               )
-        self._assert_cyclic_constraint(metadata, auto=True)
+        self._assert_cyclic_constraint(
+            metadata, auto=True, sqlite_warning=True)
+
+    @testing.provide_metadata
+    def test_fk_column_use_alter_inline_constraint_create(self):
+        metadata = self.metadata
+
+        Table("a", metadata,
+              Column('id', Integer, primary_key=True),
+              Column('bid', Integer, ForeignKey("b.id")),
+              )
+        Table("b", metadata,
+              Column('id', Integer, primary_key=True),
+              Column("aid", Integer,
+                     ForeignKey("a.id", name="bfk", use_alter=True)
+                     ),
+              )
+        self._assert_cyclic_constraint(metadata, auto=False)
 
     @testing.provide_metadata
     def test_fk_table_use_alter_constraint_create(self):
@@ -137,9 +158,10 @@ class ConstraintGenTest(fixtures.TestBase, AssertsExecutionResults):
               ForeignKeyConstraint(["bid"], ["b.id"])
               )
         Table(
-            "b", metadata, Column(
-                'id', Integer, primary_key=True), Column(
-                "aid", Integer), ForeignKeyConstraint(
+            "b", metadata,
+            Column('id', Integer, primary_key=True),
+            Column("aid", Integer),
+            ForeignKeyConstraint(
                 ["aid"], ["a.id"], use_alter=True, name="bfk"))
         self._assert_cyclic_constraint(metadata)
 
@@ -157,63 +179,42 @@ class ConstraintGenTest(fixtures.TestBase, AssertsExecutionResults):
                      ForeignKey("a.id", use_alter=True, name="bfk")
                      ),
               )
-        self._assert_cyclic_constraint(metadata)
+        self._assert_cyclic_constraint(metadata, auto=False)
+
+    def _assert_cyclic_constraint(
+            self, metadata, auto=False, sqlite_warning=False):
+        if testing.db.dialect.supports_alter:
+            self._assert_cyclic_constraint_supports_alter(metadata, auto=auto)
+        else:
+            self._assert_cyclic_constraint_no_alter(
+                metadata, auto=auto, sqlite_warning=sqlite_warning)
 
-    def _assert_cyclic_constraint(self, metadata, auto=False):
+    def _assert_cyclic_constraint_supports_alter(self, metadata, auto=False):
         table_assertions = []
         if auto:
-            if testing.db.dialect.supports_alter:
-                table_assertions.append(
-                    CompiledSQL('CREATE TABLE b ('
-                                'id INTEGER NOT NULL, '
-                                'aid INTEGER, '
-                                'PRIMARY KEY (id)'
-                                ')'
-                                )
-                )
-            else:
-                table_assertions.append(
-                    CompiledSQL(
-                        'CREATE TABLE b ('
-                        'id INTEGER NOT NULL, '
-                        'aid INTEGER, '
-                        'PRIMARY KEY (id), '
-                        'CONSTRAINT bfk FOREIGN KEY(aid) REFERENCES a (id)'
-                        ')'
-                    )
-                )
-
-            if testing.db.dialect.supports_alter:
-                table_assertions.append(
-                    CompiledSQL(
-                        'CREATE TABLE a ('
-                        'id INTEGER NOT NULL, '
-                        'bid INTEGER, '
-                        'PRIMARY KEY (id)'
-                        ')'
-                    )
-                )
-            else:
-                table_assertions.append(
-                    CompiledSQL(
-                        'CREATE TABLE a ('
-                        'id INTEGER NOT NULL, '
-                        'bid INTEGER, '
-                        'PRIMARY KEY (id), '
-                        'FOREIGN KEY(bid) REFERENCES b (id)'
-                        ')'
-                    )
+            table_assertions = [
+                CompiledSQL('CREATE TABLE b ('
+                            'id INTEGER NOT NULL, '
+                            'aid INTEGER, '
+                            'PRIMARY KEY (id)'
+                            ')'
+                            ),
+                CompiledSQL(
+                    'CREATE TABLE a ('
+                    'id INTEGER NOT NULL, '
+                    'bid INTEGER, '
+                    'PRIMARY KEY (id)'
+                    ')'
                 )
+            ]
         else:
-            table_assertions.append(
+            table_assertions = [
                 CompiledSQL('CREATE TABLE b ('
                             'id INTEGER NOT NULL, '
                             'aid INTEGER, '
                             'PRIMARY KEY (id)'
                             ')'
-                            )
-            )
-            table_assertions.append(
+                            ),
                 CompiledSQL(
                     'CREATE TABLE a ('
                     'id INTEGER NOT NULL, '
@@ -222,41 +223,238 @@ class ConstraintGenTest(fixtures.TestBase, AssertsExecutionResults):
                     'FOREIGN KEY(bid) REFERENCES b (id)'
                     ')'
                 )
-            )
+            ]
 
         assertions = [AllOf(*table_assertions)]
-        if testing.db.dialect.supports_alter:
-            fk_assertions = []
+        fk_assertions = []
+        fk_assertions.append(
+            CompiledSQL('ALTER TABLE b ADD CONSTRAINT bfk '
+                        'FOREIGN KEY(aid) REFERENCES a (id)')
+        )
+        if auto:
             fk_assertions.append(
-                CompiledSQL('ALTER TABLE b ADD CONSTRAINT bfk '
-                            'FOREIGN KEY(aid) REFERENCES a (id)')
+                CompiledSQL('ALTER TABLE a ADD '
+                            'FOREIGN KEY(bid) REFERENCES b (id)')
             )
-            if auto:
-                fk_assertions.append(
-                    CompiledSQL('ALTER TABLE a ADD '
-                                'FOREIGN KEY(bid) REFERENCES b (id)')
+        assertions.append(AllOf(*fk_assertions))
+
+        with self.sql_execution_asserter() as asserter:
+            metadata.create_all(checkfirst=False)
+        asserter.assert_(*assertions)
+
+        assertions = [
+            CompiledSQL('ALTER TABLE b DROP CONSTRAINT bfk'),
+            CompiledSQL("DROP TABLE a"),
+            CompiledSQL("DROP TABLE b")
+        ]
+
+        with self.sql_execution_asserter() as asserter:
+            metadata.drop_all(checkfirst=False),
+        asserter.assert_(*assertions)
+
+    def _assert_cyclic_constraint_no_alter(
+            self, metadata, auto=False, sqlite_warning=False):
+        table_assertions = []
+        if auto:
+            table_assertions.append(
+                DialectSQL(
+                    'CREATE TABLE b ('
+                    'id INTEGER NOT NULL, '
+                    'aid INTEGER, '
+                    'PRIMARY KEY (id), '
+                    'CONSTRAINT bfk FOREIGN KEY(aid) REFERENCES a (id)'
+                    ')'
+                )
+            )
+            table_assertions.append(
+                DialectSQL(
+                    'CREATE TABLE a ('
+                    'id INTEGER NOT NULL, '
+                    'bid INTEGER, '
+                    'PRIMARY KEY (id), '
+                    'FOREIGN KEY(bid) REFERENCES b (id)'
+                    ')'
+                )
+            )
+        else:
+            table_assertions.append(
+                DialectSQL(
+                    'CREATE TABLE b ('
+                    'id INTEGER NOT NULL, '
+                    'aid INTEGER, '
+                    'PRIMARY KEY (id), '
+                    'CONSTRAINT bfk FOREIGN KEY(aid) REFERENCES a (id)'
+                    ')'
+                )
+            )
+
+            table_assertions.append(
+                DialectSQL(
+                    'CREATE TABLE a ('
+                    'id INTEGER NOT NULL, '
+                    'bid INTEGER, '
+                    'PRIMARY KEY (id), '
+                    'FOREIGN KEY(bid) REFERENCES b (id)'
+                    ')'
                 )
-            assertions.append(AllOf(*fk_assertions))
+            )
+
+        assertions = [AllOf(*table_assertions)]
 
         with self.sql_execution_asserter() as asserter:
             metadata.create_all(checkfirst=False)
         asserter.assert_(*assertions)
 
+        assertions = [AllOf(
+            CompiledSQL("DROP TABLE a"),
+            CompiledSQL("DROP TABLE b")
+        )]
+
+        if sqlite_warning:
+            with expect_warnings("Can't sort tables for DROP; "):
+                with self.sql_execution_asserter() as asserter:
+                    metadata.drop_all(checkfirst=False),
+        else:
+            with self.sql_execution_asserter() as asserter:
+                metadata.drop_all(checkfirst=False),
+        asserter.assert_(*assertions)
+
+    @testing.force_drop_names("a", "b")
+    def test_cycle_unnamed_fks(self):
+        metadata = MetaData(testing.db)
+
+        Table("a", metadata,
+              Column('id', Integer, primary_key=True),
+              Column('bid', Integer, ForeignKey("b.id")),
+              )
+
+        Table("b", metadata,
+              Column('id', Integer, primary_key=True),
+              Column("aid", Integer, ForeignKey("a.id")),
+              )
+
+        assertions = [
+            AllOf(
+                CompiledSQL(
+                    'CREATE TABLE b ('
+                    'id INTEGER NOT NULL, '
+                    'aid INTEGER, '
+                    'PRIMARY KEY (id)'
+                    ')'
+                ),
+                CompiledSQL(
+                    'CREATE TABLE a ('
+                    'id INTEGER NOT NULL, '
+                    'bid INTEGER, '
+                    'PRIMARY KEY (id)'
+                    ')'
+                )
+            ),
+            AllOf(
+                CompiledSQL('ALTER TABLE b ADD '
+                            'FOREIGN KEY(aid) REFERENCES a (id)'),
+                CompiledSQL('ALTER TABLE a ADD '
+                            'FOREIGN KEY(bid) REFERENCES b (id)')
+            ),
+        ]
+        with self.sql_execution_asserter() as asserter:
+            metadata.create_all(checkfirst=False)
+
         if testing.db.dialect.supports_alter:
-            assertions = [
-                CompiledSQL('ALTER TABLE b DROP CONSTRAINT bfk'),
-                CompiledSQL("DROP TABLE a"),
-                CompiledSQL("DROP TABLE b")
-            ]
+            asserter.assert_(*assertions)
+
+            assert_raises_message(
+                exc.CircularDependencyError,
+                "Can't sort tables for DROP; an unresolvable foreign key "
+                "dependency exists between tables: a, b.  "
+                "Please ensure that the "
+                "ForeignKey and ForeignKeyConstraint objects involved in the "
+                "cycle have names so that they can be dropped using "
+                "DROP CONSTRAINT.",
+                metadata.drop_all, checkfirst=False
+            )
         else:
-            assertions = [AllOf(
-                CompiledSQL("DROP TABLE a"),
-                CompiledSQL("DROP TABLE b")
-            )]
+            with expect_warnings(
+                    "Can't sort tables for DROP; an unresolvable "
+                    "foreign key dependency exists between tables"):
+                with self.sql_execution_asserter() as asserter:
+                    metadata.drop_all(checkfirst=False)
+
+            asserter.assert_(
+                AllOf(
+                    CompiledSQL("DROP TABLE b"),
+                    CompiledSQL("DROP TABLE a"),
+                )
+            )
+
+    @testing.force_drop_names("a", "b")
+    def test_cycle_named_fks(self):
+        metadata = MetaData(testing.db)
 
+        Table("a", metadata,
+              Column('id', Integer, primary_key=True),
+              Column('bid', Integer, ForeignKey("b.id")),
+              )
+
+        Table("b", metadata,
+              Column('id', Integer, primary_key=True),
+              Column(
+                  "aid", Integer,
+                  ForeignKey("a.id", use_alter=True, name='aidfk')),
+              )
+
+        assertions = [
+            AllOf(
+                CompiledSQL(
+                    'CREATE TABLE b ('
+                    'id INTEGER NOT NULL, '
+                    'aid INTEGER, '
+                    'PRIMARY KEY (id)'
+                    ')'
+                ),
+                CompiledSQL(
+                    'CREATE TABLE a ('
+                    'id INTEGER NOT NULL, '
+                    'bid INTEGER, '
+                    'PRIMARY KEY (id), '
+                    'FOREIGN KEY(bid) REFERENCES b (id)'
+                    ')'
+                )
+            ),
+            CompiledSQL('ALTER TABLE b ADD CONSTRAINT aidfk '
+                        'FOREIGN KEY(aid) REFERENCES a (id)'),
+        ]
         with self.sql_execution_asserter() as asserter:
-            metadata.drop_all(checkfirst=False),
-        asserter.assert_(*assertions)
+            metadata.create_all(checkfirst=False)
+
+        if testing.db.dialect.supports_alter:
+            asserter.assert_(*assertions)
+
+            with self.sql_execution_asserter() as asserter:
+                metadata.drop_all(checkfirst=False)
+
+            asserter.assert_(
+                CompiledSQL("ALTER TABLE b DROP CONSTRAINT aidfk"),
+                AllOf(
+                    CompiledSQL("DROP TABLE b"),
+                    CompiledSQL("DROP TABLE a"),
+                )
+            )
+        else:
+            with self.sql_execution_asserter() as asserter:
+                metadata.drop_all(checkfirst=False)
+
+            asserter.assert_(
+                AllOf(
+                    CompiledSQL("DROP TABLE b"),
+                    CompiledSQL("DROP TABLE a"),
+                ),
+            )
+
+
+
+
+
 
     @testing.requires.check_constraints
     @testing.provide_metadata