diff options
| -rw-r--r-- | doc/build/changelog/changelog_11.rst | 15 | ||||
| -rw-r--r-- | lib/sqlalchemy/sql/elements.py | 21 | ||||
| -rw-r--r-- | lib/sqlalchemy/testing/__init__.py | 2 | ||||
| -rw-r--r-- | lib/sqlalchemy/testing/assertions.py | 8 | ||||
| -rw-r--r-- | test/orm/test_lazy_relations.py | 69 | ||||
| -rw-r--r-- | test/sql/test_utils.py | 78 |
6 files changed, 186 insertions, 7 deletions
diff --git a/doc/build/changelog/changelog_11.rst b/doc/build/changelog/changelog_11.rst index c038966c5..a09703489 100644 --- a/doc/build/changelog/changelog_11.rst +++ b/doc/build/changelog/changelog_11.rst @@ -22,6 +22,21 @@ :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: 3789 diff --git a/lib/sqlalchemy/sql/elements.py b/lib/sqlalchemy/sql/elements.py index 75d5368d5..cff57372c 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 f4a23d238..8f3d063be 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 bd1ccaa51..84653da5c 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 f2e1db2da..56d1b8323 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 000000000..09d7e98af --- /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)) + |