From 1b120563905e29f9769fd6d2ce39922d15cebf7f Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Wed, 6 May 2015 17:07:24 -0400 Subject: [PATCH] - Fixed unexpected-use regression where in the odd case that the primaryjoin of a relationship involved comparison to an unhashable type such as an HSTORE, lazy loads would fail due to a hash-oriented check on the statement parameters, modified in 1.0 as a result of :ticket:`3061` to use hashing and modified in :ticket:`3368` to occur in cases more common than "load on pending". The values are now checked for the ``__hash__`` attribute beforehand. fixes #3416 --- doc/build/changelog/changelog_10.rst | 12 +++++ lib/sqlalchemy/orm/strategies.py | 8 +-- lib/sqlalchemy/util/__init__.py | 2 +- lib/sqlalchemy/util/_collections.py | 13 +++++ test/orm/test_lazy_relations.py | 81 ++++++++++++++++++++++++++++ 5 files changed, 112 insertions(+), 4 deletions(-) diff --git a/doc/build/changelog/changelog_10.rst b/doc/build/changelog/changelog_10.rst index 65a0511362..e51d885605 100644 --- a/doc/build/changelog/changelog_10.rst +++ b/doc/build/changelog/changelog_10.rst @@ -18,6 +18,18 @@ .. changelog:: :version: 1.0.4 + .. change:: + :tags: bug, orm + :tickets: 3416 + + Fixed unexpected-use regression where in the odd case that the + primaryjoin of a relationship involved comparison to an unhashable + type such as an HSTORE, lazy loads would fail due to a hash-oriented + check on the statement parameters, modified in 1.0 as a result of + :ticket:`3061` to use hashing and modified in :ticket:`3368` + to occur in cases more common than "load on pending". + The values are now checked for the ``__hash__`` attribute beforehand. + .. change:: :tags: bug, orm :tickets: 3412, 3347 diff --git a/lib/sqlalchemy/orm/strategies.py b/lib/sqlalchemy/orm/strategies.py index 85d233a05e..78e9293458 100644 --- a/lib/sqlalchemy/orm/strategies.py +++ b/lib/sqlalchemy/orm/strategies.py @@ -586,9 +586,11 @@ class LazyLoader(AbstractRelationshipLoader, util.MemoizedSlots): lazy_clause, params = self._generate_lazy_clause( state, passive=passive) - if pending and orm_util._none_set.intersection(params.values()): - return None - elif orm_util._never_set.intersection(params.values()): + if pending: + if util.has_intersection( + orm_util._none_set, params.values()): + return None + elif util.has_intersection(orm_util._never_set, params.values()): return None q = q.filter(lazy_clause).params(params) diff --git a/lib/sqlalchemy/util/__init__.py b/lib/sqlalchemy/util/__init__.py index d777d2e06f..ed968f1681 100644 --- a/lib/sqlalchemy/util/__init__.py +++ b/lib/sqlalchemy/util/__init__.py @@ -19,7 +19,7 @@ from ._collections import KeyedTuple, ImmutableContainer, immutabledict, \ OrderedSet, IdentitySet, OrderedIdentitySet, column_set, \ column_dict, ordered_column_set, populate_column_dict, unique_list, \ UniqueAppender, PopulateDict, EMPTY_SET, to_list, to_set, \ - to_column_set, update_copy, flatten_iterator, \ + to_column_set, update_copy, flatten_iterator, has_intersection, \ LRUCache, ScopedRegistry, ThreadLocalRegistry, WeakSequence, \ coerce_generator_arg, lightweight_named_tuple diff --git a/lib/sqlalchemy/util/_collections.py b/lib/sqlalchemy/util/_collections.py index db2c21949b..5c62ebed86 100644 --- a/lib/sqlalchemy/util/_collections.py +++ b/lib/sqlalchemy/util/_collections.py @@ -800,6 +800,19 @@ def to_list(x, default=None): return list(x) +def has_intersection(set_, iterable): + """return True if any items of set_ are present in iterable. + + Goes through special effort to ensure __hash__ is not called + on items in iterable that don't support it. + + """ + # TODO: optimize, write in C, etc. + return bool( + set_.intersection([i for i in iterable if i.__hash__]) + ) + + def to_set(x): if x is None: return set() diff --git a/test/orm/test_lazy_relations.py b/test/orm/test_lazy_relations.py index 166ee90cfc..cfdfdf47c1 100644 --- a/test/orm/test_lazy_relations.py +++ b/test/orm/test_lazy_relations.py @@ -9,6 +9,7 @@ from sqlalchemy import Integer, String, ForeignKey, SmallInteger, Boolean 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 fixtures @@ -559,6 +560,58 @@ class GetterStateTest(_fixtures.FixtureTest): run_inserts = None + def _unhashable_fixture(self, metadata, load_on_pending=False): + class MyHashType(sa.TypeDecorator): + impl = sa.String(100) + + def process_bind_param(self, value, dialect): + return ";".join( + "%s=%s" % (k, v) + for k, v in sorted(value.items(), lambda key: key[0])) + + def process_result_value(self, value, dialect): + return dict(elem.split("=", 1) for elem in value.split(";")) + + category = Table( + 'category', metadata, + Column('id', Integer, primary_key=True), + Column('data', MyHashType()) + ) + article = Table( + 'article', metadata, + Column('id', Integer, primary_key=True), + Column('data', MyHashType()) + ) + + class Category(fixtures.ComparableEntity): + pass + + class Article(fixtures.ComparableEntity): + pass + + mapper(Category, category) + mapper(Article, article, properties={ + "category": relationship( + Category, + primaryjoin=orm.foreign(article.c.data) == category.c.data, + load_on_pending=load_on_pending + ) + }) + + metadata.create_all() + sess = Session(autoflush=False) + data = {"im": "unhashable"} + a1 = Article(id=1, data=data) + c1 = Category(id=1, data=data) + if load_on_pending: + sess.add(c1) + else: + sess.add_all([c1, a1]) + sess.flush() + if load_on_pending: + sess.add(a1) + return Category, Article, sess, a1, c1 + def _u_ad_fixture(self, populate_user, dont_use_get=False): users, Address, addresses, User = ( self.tables.users, @@ -602,6 +655,34 @@ class GetterStateTest(_fixtures.FixtureTest): 0 ) + @testing.provide_metadata + def test_no_use_get_params_not_hashable(self): + Category, Article, sess, a1, c1 = \ + self._unhashable_fixture(self.metadata) + + def go(): + eq_(a1.category, c1) + + self.assert_sql_count( + testing.db, + go, + 1 + ) + + @testing.provide_metadata + def test_no_use_get_params_not_hashable_on_pending(self): + Category, Article, sess, a1, c1 = \ + self._unhashable_fixture(self.metadata, load_on_pending=True) + + def go(): + eq_(a1.category, c1) + + self.assert_sql_count( + testing.db, + go, + 1 + ) + def test_get_empty_passive_return_never_set(self): User, Address, sess, a1 = self._u_ad_fixture(False) eq_( -- 2.47.3