From: Mike Bayer Date: Thu, 7 May 2020 21:13:35 +0000 (-0400) Subject: Warn when sorted_tables is not actually sorting X-Git-Tag: rel_1_3_17~6^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=3e79126404476f2aed000d4fcf1fbe9f7e0a7c27;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git Warn when sorted_tables is not actually sorting A warning is emitted when making use of the :attr:`.MetaData.sorted_tables` attribute as well as the :func:`_schema.sort_tables` function, and the given tables cannot be correctly sorted due to a cyclic dependency between foreign key constraints. In this case, the functions will no longer sort the involved tables by foreign key, and a warning will be emitted. Other tables that are not part of the cycle will still be returned in dependency order. Previously, the sorted_table routines would return a collection that would unconditionally omit all foreign keys when a cycle was detected, and no warning was emitted. Fixes: #5316 Change-Id: I14f72ccf39cb568bc77e8da16d0685718b2b9960 (cherry picked from commit 8782469b789585d3f0c3a642f0bb9519816f6b11) --- diff --git a/doc/build/changelog/unreleased_13/5316.rst b/doc/build/changelog/unreleased_13/5316.rst new file mode 100644 index 0000000000..2edfce10db --- /dev/null +++ b/doc/build/changelog/unreleased_13/5316.rst @@ -0,0 +1,14 @@ +.. change:: + :tags: bug, schema + :tickets: 5316 + + A warning is emitted when making use of the :attr:`.MetaData.sorted_tables` + attribute as well as the :func:`_schema.sort_tables` function, and the + given tables cannot be correctly sorted due to a cyclic dependency between + foreign key constraints. In this case, the functions will no longer sort + the involved tables by foreign key, and a warning will be emitted. Other + tables that are not part of the cycle will still be returned in dependency + order. Previously, the sorted_table routines would return a collection that + would unconditionally omit all foreign keys when a cycle was detected, and + no warning was emitted. + diff --git a/lib/sqlalchemy/sql/ddl.py b/lib/sqlalchemy/sql/ddl.py index 851f536c12..1f337e7abc 100644 --- a/lib/sqlalchemy/sql/ddl.py +++ b/lib/sqlalchemy/sql/ddl.py @@ -1037,7 +1037,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 . @@ -1051,16 +1053,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. @@ -1078,7 +1093,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 """ @@ -1098,14 +1113,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` @@ -1189,9 +1207,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 1de9f39455..f4fa19eedc 100644 --- a/lib/sqlalchemy/sql/schema.py +++ b/lib/sqlalchemy/sql/schema.py @@ -4215,15 +4215,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 98d6b2b868..d19b616938 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, @@ -173,7 +174,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.connect() 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: @@ -246,7 +255,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( diff --git a/test/sql/test_metadata.py b/test/sql/test_metadata.py index 0780749e17..4158cd064f 100644 --- a/test/sql/test_metadata.py +++ b/test/sql/test_metadata.py @@ -610,6 +610,93 @@ class MetaDataTest(fixtures.TestBase, ComparesTables): a.add_is_dependent_on(b) eq_(meta.sorted_tables, [b, c, d, a, e]) + def test_fks_deterministic_order(self): + meta = MetaData() + a = Table("a", meta, Column("foo", Integer, ForeignKey("b.foo"))) + b = Table("b", meta, Column("foo", Integer)) + c = Table("c", meta, Column("foo", Integer)) + d = Table("d", meta, Column("foo", Integer)) + e = Table("e", meta, Column("foo", Integer, ForeignKey("c.foo"))) + + eq_(meta.sorted_tables, [b, c, d, a, e]) + + def test_cycles_fks_warning_one(self): + meta = MetaData() + a = Table("a", meta, Column("foo", Integer, ForeignKey("b.foo"))) + b = Table("b", meta, Column("foo", Integer, ForeignKey("d.foo"))) + c = Table("c", meta, Column("foo", Integer, ForeignKey("b.foo"))) + d = Table("d", meta, Column("foo", Integer, ForeignKey("c.foo"))) + e = Table("e", meta, Column("foo", Integer)) + + with testing.expect_warnings( + "Cannot correctly sort tables; there are unresolvable cycles " + 'between tables "b, c, d", which is usually caused by mutually ' + "dependent foreign key constraints. " + "Foreign key constraints involving these tables will not be " + "considered" + ): + eq_(meta.sorted_tables, [b, c, d, e, a]) + + def test_cycles_fks_warning_two(self): + meta = MetaData() + a = Table("a", meta, Column("foo", Integer, ForeignKey("b.foo"))) + b = Table("b", meta, Column("foo", Integer, ForeignKey("a.foo"))) + c = Table("c", meta, Column("foo", Integer, ForeignKey("e.foo"))) + d = Table("d", meta, Column("foo", Integer)) + e = Table("e", meta, Column("foo", Integer, ForeignKey("d.foo"))) + + with testing.expect_warnings( + "Cannot correctly sort tables; there are unresolvable cycles " + 'between tables "a, b", which is usually caused by mutually ' + "dependent foreign key constraints. " + "Foreign key constraints involving these tables will not be " + "considered" + ): + eq_(meta.sorted_tables, [a, b, d, e, c]) + + def test_cycles_fks_fks_delivered_separately(self): + meta = MetaData() + a = Table("a", meta, Column("foo", Integer, ForeignKey("b.foo"))) + b = Table("b", meta, Column("foo", Integer, ForeignKey("a.foo"))) + c = Table("c", meta, Column("foo", Integer, ForeignKey("e.foo"))) + d = Table("d", meta, Column("foo", Integer)) + e = Table("e", meta, Column("foo", Integer, ForeignKey("d.foo"))) + + results = schema.sort_tables_and_constraints( + sorted(meta.tables.values(), key=lambda t: t.key) + ) + results[-1] = (None, set(results[-1][-1])) + eq_( + results, + [ + (a, set()), + (b, set()), + (d, {fk.constraint for fk in d.foreign_keys}), + (e, {fk.constraint for fk in e.foreign_keys}), + (c, {fk.constraint for fk in c.foreign_keys}), + ( + None, + {fk.constraint for fk in a.foreign_keys}.union( + fk.constraint for fk in b.foreign_keys + ), + ), + ], + ) + + def test_cycles_fks_usealter(self): + meta = MetaData() + a = Table("a", meta, Column("foo", Integer, ForeignKey("b.foo"))) + b = Table( + "b", + meta, + Column("foo", Integer, ForeignKey("d.foo", use_alter=True)), + ) + c = Table("c", meta, Column("foo", Integer, ForeignKey("b.foo"))) + d = Table("d", meta, Column("foo", Integer, ForeignKey("c.foo"))) + e = Table("e", meta, Column("foo", Integer)) + + eq_(meta.sorted_tables, [b, e, a, c, d]) + def test_nonexistent(self): assert_raises( tsa.exc.NoSuchTableError,