From: Mike Bayer Date: Fri, 2 Sep 2016 15:27:58 +0000 (-0400) Subject: Repair clauselist comparison to account for clause ordering X-Git-Tag: rel_1_1_0~37 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=ce577d48449588d3e5395c08c7f4d04cb8bb325f;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git Repair clauselist comparison to account for clause ordering Fixed bug where the "simple many-to-one" condition that allows lazy loading to use get() from identity map would fail to be invoked if the primaryjoin of the relationship had multiple clauses separated by AND which were not in the same order as that of the primary key columns being compared in each clause. This ordering difference occurs for a composite foreign key where the table-bound columns on the referencing side were not in the same order in the .c collection as the primary key columns on the referenced side....which in turn occurs a lot if one is using declarative mixins and/or declared_attr to set up columns. Change-Id: I66cce74f614c04ed693dc0d58ac8c952b2f8ae54 Fixes: #3788 --- diff --git a/doc/build/changelog/changelog_11.rst b/doc/build/changelog/changelog_11.rst index 88f1ebb244..669f081884 100644 --- a/doc/build/changelog/changelog_11.rst +++ b/doc/build/changelog/changelog_11.rst @@ -21,6 +21,21 @@ .. changelog:: :version: 1.1.0 + .. change:: + :tags: bug, orm + :tickets: 3788 + + Fixed bug where the "simple many-to-one" condition that allows lazy + loading to use get() from identity map would fail to be invoked if the + primaryjoin of the relationship had multiple clauses separated by AND + which were not in the same order as that of the primary key columns + being compared in each clause. This ordering + difference occurs for a composite foreign key where the table-bound + columns on the referencing side were not in the same order in the .c + collection as the primary key columns on the referenced side....which + in turn occurs a lot if one is using declarative mixins and/or + declared_attr to set up columns. + .. change:: :tags: bug, sql :tickets: 3786 diff --git a/lib/sqlalchemy/sql/elements.py b/lib/sqlalchemy/sql/elements.py index 75d5368d58..cff57372cf 100644 --- a/lib/sqlalchemy/sql/elements.py +++ b/lib/sqlalchemy/sql/elements.py @@ -1828,12 +1828,23 @@ class ClauseList(ClauseElement): if not isinstance(other, ClauseList) and len(self.clauses) == 1: return self.clauses[0].compare(other, **kw) elif isinstance(other, ClauseList) and \ - len(self.clauses) == len(other.clauses): - for i in range(0, len(self.clauses)): - if not self.clauses[i].compare(other.clauses[i], **kw): - return False + len(self.clauses) == len(other.clauses) and \ + self.operator is other.operator: + + if self.operator in (operators.and_, operators.or_): + completed = set() + for clause in self.clauses: + for other_clause in set(other.clauses).difference(completed): + if clause.compare(other_clause, **kw): + completed.add(other_clause) + break + return len(completed) == len(other.clauses) else: - return self.operator == other.operator + for i in range(0, len(self.clauses)): + if not self.clauses[i].compare(other.clauses[i], **kw): + return False + else: + return True else: return False diff --git a/lib/sqlalchemy/testing/__init__.py b/lib/sqlalchemy/testing/__init__.py index f4a23d2381..8f3d063be3 100644 --- a/lib/sqlalchemy/testing/__init__.py +++ b/lib/sqlalchemy/testing/__init__.py @@ -22,7 +22,7 @@ from .assertions import emits_warning, emits_warning_on, uses_deprecated, \ eq_, ne_, le_, is_, is_not_, startswith_, assert_raises, \ assert_raises_message, AssertsCompiledSQL, ComparesTables, \ AssertsExecutionResults, expect_deprecated, expect_warnings, \ - in_, not_in_, eq_ignore_whitespace, eq_regex + in_, not_in_, eq_ignore_whitespace, eq_regex, is_true, is_false from .util import run_as_contextmanager, rowset, fail, \ provide_metadata, adict, force_drop_names, \ diff --git a/lib/sqlalchemy/testing/assertions.py b/lib/sqlalchemy/testing/assertions.py index bd1ccaa519..84653da5c6 100644 --- a/lib/sqlalchemy/testing/assertions.py +++ b/lib/sqlalchemy/testing/assertions.py @@ -224,6 +224,14 @@ def le_(a, b, msg=None): assert a <= b, msg or "%r != %r" % (a, b) +def is_true(a, msg=None): + is_(a, True, msg=msg) + + +def is_false(a, msg=None): + is_(a, False, msg=msg) + + def is_(a, b, msg=None): """Assert a is b, with repr messaging on failure.""" assert a is b, msg or "%r is not %r" % (a, b) diff --git a/test/orm/test_lazy_relations.py b/test/orm/test_lazy_relations.py index f2e1db2da3..56d1b83234 100644 --- a/test/orm/test_lazy_relations.py +++ b/test/orm/test_lazy_relations.py @@ -6,12 +6,13 @@ from sqlalchemy.orm import attributes, exc as orm_exc, configure_mappers import sqlalchemy as sa from sqlalchemy import testing, and_ from sqlalchemy import Integer, String, ForeignKey, SmallInteger, Boolean +from sqlalchemy import ForeignKeyConstraint from sqlalchemy.types import TypeDecorator from sqlalchemy.testing.schema import Table from sqlalchemy.testing.schema import Column from sqlalchemy import orm from sqlalchemy.orm import mapper, relationship, create_session, Session -from sqlalchemy.testing import eq_ +from sqlalchemy.testing import eq_, is_true, is_false from sqlalchemy.testing import fixtures from test.orm import _fixtures from sqlalchemy.testing.assertsql import CompiledSQL @@ -1148,3 +1149,69 @@ class TypeCoerceTest(fixtures.MappedTest, testing.AssertsExecutionResults,): [{'param_1': 5}] ) ) + + +class CompositeSimpleM2OTest(fixtures.MappedTest): + """ORM-level test for [ticket:3788]""" + + @classmethod + def define_tables(cls, metadata): + Table( + 'a', metadata, + Column("id1", Integer, primary_key=True), + Column("id2", Integer, primary_key=True), + ) + + Table( + "b_sameorder", metadata, + Column("id", Integer, primary_key=True), + Column('a_id1', Integer), + Column('a_id2', Integer), + ForeignKeyConstraint(['a_id1', 'a_id2'], ['a.id1', 'a.id2']) + ) + + Table( + "b_differentorder", metadata, + Column("id", Integer, primary_key=True), + Column('a_id1', Integer), + Column('a_id2', Integer), + ForeignKeyConstraint(['a_id1', 'a_id2'], ['a.id1', 'a.id2']) + ) + + @classmethod + def setup_classes(cls): + class A(cls.Basic): + pass + + class B(cls.Basic): + pass + + def test_use_get_sameorder(self): + mapper(self.classes.A, self.tables.a) + m_b = mapper(self.classes.B, self.tables.b_sameorder, properties={ + 'a': relationship(self.classes.A) + }) + + configure_mappers() + is_true(m_b.relationships.a.strategy.use_get) + + def test_use_get_reverseorder(self): + mapper(self.classes.A, self.tables.a) + m_b = mapper(self.classes.B, self.tables.b_differentorder, properties={ + 'a': relationship(self.classes.A) + }) + + configure_mappers() + is_true(m_b.relationships.a.strategy.use_get) + + def test_dont_use_get_pj_is_different(self): + mapper(self.classes.A, self.tables.a) + m_b = mapper(self.classes.B, self.tables.b_sameorder, properties={ + 'a': relationship(self.classes.A, primaryjoin=and_( + self.tables.a.c.id1 == self.tables.b_sameorder.c.a_id1, + self.tables.a.c.id2 == 12 + )) + }) + + configure_mappers() + is_false(m_b.relationships.a.strategy.use_get) diff --git a/test/sql/test_utils.py b/test/sql/test_utils.py new file mode 100644 index 0000000000..09d7e98afd --- /dev/null +++ b/test/sql/test_utils.py @@ -0,0 +1,78 @@ +from sqlalchemy.testing import fixtures, is_true, is_false +from sqlalchemy import MetaData, Table, Column, Integer +from sqlalchemy import and_, or_ +from sqlalchemy.sql.elements import ClauseList +from sqlalchemy.sql import operators + + +class CompareClausesTest(fixtures.TestBase): + def setup(self): + m = MetaData() + self.a = Table( + 'a', m, + Column('x', Integer), + Column('y', Integer) + ) + + self.b = Table( + 'b', m, + Column('y', Integer), + Column('z', Integer) + ) + + def test_compare_clauselist_associative(self): + + l1 = and_( + self.a.c.x == self.b.c.y, + self.a.c.y == self.b.c.z + ) + + l2 = and_( + self.a.c.y == self.b.c.z, + self.a.c.x == self.b.c.y, + ) + + l3 = and_( + self.a.c.x == self.b.c.z, + self.a.c.y == self.b.c.y + ) + + is_true(l1.compare(l1)) + is_true(l1.compare(l2)) + is_false(l1.compare(l3)) + + def test_compare_clauselist_not_associative(self): + + l1 = ClauseList( + self.a.c.x, self.a.c.y, self.b.c.y, operator=operators.sub) + + l2 = ClauseList( + self.b.c.y, self.a.c.x, self.a.c.y, operator=operators.sub) + + is_true(l1.compare(l1)) + is_false(l1.compare(l2)) + + def test_compare_clauselist_assoc_different_operator(self): + + l1 = and_( + self.a.c.x == self.b.c.y, + self.a.c.y == self.b.c.z + ) + + l2 = or_( + self.a.c.y == self.b.c.z, + self.a.c.x == self.b.c.y, + ) + + is_false(l1.compare(l2)) + + def test_compare_clauselist_not_assoc_different_operator(self): + + l1 = ClauseList( + self.a.c.x, self.a.c.y, self.b.c.y, operator=operators.sub) + + l2 = ClauseList( + self.a.c.x, self.a.c.y, self.b.c.y, operator=operators.div) + + is_false(l1.compare(l2)) +