]> git.ipfire.org Git - thirdparty/sqlalchemy/sqlalchemy.git/commitdiff
- repair a regression caused by #3282, where we no longer were
authorMike Bayer <mike_mp@zzzcomputing.com>
Wed, 22 Apr 2015 18:14:11 +0000 (14:14 -0400)
committerMike Bayer <mike_mp@zzzcomputing.com>
Wed, 22 Apr 2015 18:14:11 +0000 (14:14 -0400)
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

doc/build/changelog/changelog_10.rst
doc/build/changelog/migration_10.rst
lib/sqlalchemy/sql/ddl.py
lib/sqlalchemy/testing/assertsql.py
lib/sqlalchemy/testing/engines.py
lib/sqlalchemy/testing/util.py
test/sql/test_constraints.py

index 52b35c9311d0371f6fec907f26b57f8b9a54aff6..19c956079fcae4a5ac8ba4544d606d2ac852e90a 100644 (file)
 .. changelog::
     :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
index 49ca37b2babe1337538feb25d6539baed88ed633..392c3f66f02cda92d6e04492753f0521c42971a8 100644 (file)
@@ -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.
index bbac6456ef4c2ed06dc36731cdd531bc44e30951..a0841b13c8658efc8c12a98d8a9921d7029d0805 100644 (file)
@@ -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
index e544adad26069093644efcf8e502f0deb68b13b0..24349360710b01e07dabc8dd3310727901c28803 100644 (file)
@@ -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
index 3a8303546ec12c890548a2e16eb659d5160af981..8bd1becbf10e429039752265cfa671cc3baa3a63 100644 (file)
@@ -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
index 6d6fa094eeb835d6710d6bbb42aedd80f6f1a22e..1842e58a5ce00834196b4f98b2037cfea6710297 100644 (file)
@@ -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
 
 
index d024e1a27f7d52b72fa1eb813466f58b98ba9c05..47f81a50c3a4b9e6f11f5e76c3bd872c430ba378 100644 (file)
@@ -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