From: Mike Bayer Date: Mon, 1 Apr 2013 17:37:35 +0000 (-0400) Subject: - Fixed bug in unit of work whereby a joined-inheritance X-Git-Tag: rel_0_8_1~26^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=82b6e074920cb972a569db4d2d395c8949868a31;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git - Fixed bug in unit of work whereby a joined-inheritance subclass could insert the row for the "sub" table before the parent table, if the two tables had no ForeignKey constraints set up between them. Also in 0.7.11. [ticket:2689] - fix a glitch in the assertsql.CompiledSQL fixture regarding when a multiparam compiledSQL is used within an AllOf - add a new utility function randomize_unitofwork() which does the function of --reversetop --- diff --git a/doc/build/changelog/changelog_07.rst b/doc/build/changelog/changelog_07.rst index a42ef3bb6c..df63654a4a 100644 --- a/doc/build/changelog/changelog_07.rst +++ b/doc/build/changelog/changelog_07.rst @@ -6,6 +6,15 @@ .. changelog:: :version: 0.7.11 + .. change:: + :tags: bug, orm + :tickets: 2689 + + Fixed bug in unit of work whereby a joined-inheritance + subclass could insert the row for the "sub" table + before the parent table, if the two tables had no + ForeignKey constraints set up between them. + .. change:: :tags: feature, postgresql :tickets: 2676 diff --git a/doc/build/changelog/changelog_08.rst b/doc/build/changelog/changelog_08.rst index 634ea4a953..4ef11590f9 100644 --- a/doc/build/changelog/changelog_08.rst +++ b/doc/build/changelog/changelog_08.rst @@ -6,6 +6,16 @@ .. changelog:: :version: 0.8.1 + .. change:: + :tags: bug, orm + :tickets: 2689 + + Fixed bug in unit of work whereby a joined-inheritance + subclass could insert the row for the "sub" table + before the parent table, if the two tables had no + ForeignKey constraints set up between them. + Also in 0.7.11. + .. change:: :tags: bug, mssql :pullreq: 47 diff --git a/lib/sqlalchemy/orm/mapper.py b/lib/sqlalchemy/orm/mapper.py index 2d7f62153a..d258a20b60 100644 --- a/lib/sqlalchemy/orm/mapper.py +++ b/lib/sqlalchemy/orm/mapper.py @@ -2002,10 +2002,20 @@ class Mapper(_InspectionAttr): @_memoized_configured_property def _sorted_tables(self): table_to_mapper = {} + for mapper in self.base_mapper.self_and_descendants: for t in mapper.tables: table_to_mapper.setdefault(t, mapper) + extra_dependencies = [] + for table, mapper in table_to_mapper.items(): + super_ = mapper.inherits + if super_: + extra_dependencies.extend([ + (super_table, table) + for super_table in super_.tables + ]) + def skip(fk): # attempt to skip dependencies that are not # significant to the inheritance chain @@ -2017,7 +2027,7 @@ class Mapper(_InspectionAttr): if parent is not None and \ dep is not None and \ dep is not parent and \ - dep.inherit_condition is not None: + dep.inherit_condition is not None: cols = set(sql_util.find_columns(dep.inherit_condition)) if parent.inherit_condition is not None: cols = cols.union(sql_util.find_columns( @@ -2028,7 +2038,9 @@ class Mapper(_InspectionAttr): return False sorted_ = sql_util.sort_tables(table_to_mapper.iterkeys(), - skip_fn=skip) + skip_fn=skip, + extra_dependencies=extra_dependencies) + ret = util.OrderedDict() for t in sorted_: ret[t] = table_to_mapper[t] diff --git a/lib/sqlalchemy/orm/util.py b/lib/sqlalchemy/orm/util.py index cc9dd6ba5c..f3b8e271d9 100644 --- a/lib/sqlalchemy/orm/util.py +++ b/lib/sqlalchemy/orm/util.py @@ -1265,3 +1265,40 @@ def attribute_str(instance, attribute): def state_attribute_str(state, attribute): return state_str(state) + "." + attribute + + +def randomize_unitofwork(): + """Use random-ordering sets within the unit of work in order + to detect unit of work sorting issues. + + This is a utility function that can be used to help reproduce + inconsistent unit of work sorting issues. For example, + if two kinds of objects A and B are being inserted, and + B has a foreign key reference to A - the A must be inserted first. + However, if there is no relationship between A and B, the unit of work + won't know to perform this sorting, and an operation may or may not + fail, depending on how the ordering works out. Since Python sets + and dictionaries have non-deterministic ordering, such an issue may + occur on some runs and not on others, and in practice it tends to + have a great dependence on the state of the interpreter. This leads + to so-called "heisenbugs" where changing entirely irrelevant aspects + of the test program still cause the failure behavior to change. + + By calling ``randomize_unitofwork()`` when a script first runs, the + ordering of a key series of sets within the unit of work implementation + are randomized, so that the script can be minimized down to the fundamental + mapping and operation that's failing, while still reproducing the issue + on at least some runs. + + This utility is also available when running the test suite via the + ``--reversetop`` flag. + + .. versionadded:: 0.8.1 created a standalone version of the + ``--reversetop`` feature. + + """ + from sqlalchemy.orm import unitofwork, session, mapper, dependency + from sqlalchemy.util import topological + from sqlalchemy.testing.util import RandomSet + topological.set = unitofwork.set = session.set = mapper.set = \ + dependency.set = RandomSet diff --git a/lib/sqlalchemy/sql/util.py b/lib/sqlalchemy/sql/util.py index 27ba0f95b6..520c90f999 100644 --- a/lib/sqlalchemy/sql/util.py +++ b/lib/sqlalchemy/sql/util.py @@ -13,12 +13,14 @@ from collections import deque """Utility functions that build upon SQL and Schema constructs.""" -def sort_tables(tables, skip_fn=None): +def sort_tables(tables, skip_fn=None, extra_dependencies=None): """sort a collection of Table objects in order of their foreign-key dependency.""" tables = list(tables) tuples = [] + if extra_dependencies is not None: + tuples.extend(extra_dependencies) def visit_foreign_key(fkey): if fkey.use_alter: diff --git a/lib/sqlalchemy/testing/assertsql.py b/lib/sqlalchemy/testing/assertsql.py index 864ce5b4df..0e250f356e 100644 --- a/lib/sqlalchemy/testing/assertsql.py +++ b/lib/sqlalchemy/testing/assertsql.py @@ -174,6 +174,8 @@ class CompiledSQL(SQLMatchRule): params = self.params if not isinstance(params, list): params = [params] + else: + params = list(params) all_params = list(params) all_received = list(_received_parameters) while params: diff --git a/lib/sqlalchemy/testing/plugin/noseplugin.py b/lib/sqlalchemy/testing/plugin/noseplugin.py index 6ad884e944..5bd7ff3cda 100644 --- a/lib/sqlalchemy/testing/plugin/noseplugin.py +++ b/lib/sqlalchemy/testing/plugin/noseplugin.py @@ -215,11 +215,8 @@ def _set_table_options(options, file_config): @post def _reverse_topological(options, file_config): if options.reversetop: - from sqlalchemy.orm import unitofwork, session, mapper, dependency - from sqlalchemy.util import topological - from sqlalchemy.testing.util import RandomSet - topological.set = unitofwork.set = session.set = mapper.set = \ - dependency.set = RandomSet + from sqlalchemy.orm.util import randomize_unitofwork + randomize_unitofwork() def _requirements_opt(options, opt_str, value, parser): diff --git a/test/orm/inheritance/test_basic.py b/test/orm/inheritance/test_basic.py index 66991e9228..3cd3db9280 100644 --- a/test/orm/inheritance/test_basic.py +++ b/test/orm/inheritance/test_basic.py @@ -1055,6 +1055,73 @@ class FlushTest(fixtures.MappedTest): sess.flush() assert user_roles.count().scalar() == 1 +class JoinedNoFKSortingTest(fixtures.MappedTest): + @classmethod + def define_tables(cls, metadata): + Table("a", metadata, + Column('id', Integer, primary_key=True, + test_needs_autoincrement=True) + ) + Table("b", metadata, + Column('id', Integer, primary_key=True) + ) + Table("c", metadata, + Column('id', Integer, primary_key=True) + ) + + @classmethod + def setup_classes(cls): + class A(cls.Basic): + pass + class B(A): + pass + class C(A): + pass + + @classmethod + def setup_mappers(cls): + A, B, C = cls.classes.A, cls.classes.B, cls.classes.C + mapper(A, cls.tables.a) + mapper(B, cls.tables.b, inherits=A, + inherit_condition=cls.tables.a.c.id == cls.tables.b.c.id) + mapper(C, cls.tables.c, inherits=A, + inherit_condition=cls.tables.a.c.id == cls.tables.c.c.id) + + def test_ordering(self): + B, C = self.classes.B, self.classes.C + sess = Session() + sess.add_all([B(), C(), B(), C()]) + self.assert_sql_execution( + testing.db, + sess.flush, + CompiledSQL( + "INSERT INTO a () VALUES ()", + {} + ), + CompiledSQL( + "INSERT INTO a () VALUES ()", + {} + ), + CompiledSQL( + "INSERT INTO a () VALUES ()", + {} + ), + CompiledSQL( + "INSERT INTO a () VALUES ()", + {} + ), + AllOf( + CompiledSQL( + "INSERT INTO b (id) VALUES (:id)", + [{"id": 1}, {"id": 3}] + ), + CompiledSQL( + "INSERT INTO c (id) VALUES (:id)", + [{"id": 2}, {"id": 4}] + ) + ) + ) + class VersioningTest(fixtures.MappedTest): @classmethod def define_tables(cls, metadata):