From 02402f3cbd3f8a1fa5638510694916a7c9b9c50c Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Fri, 1 Jun 2018 16:54:11 -0500 Subject: [PATCH] Support undocumented non-entity sequence Query argument Fixed regression caused by :ticket:`4256` (itself a regression fix for :ticket:`4228`) which breaks an undocumented behavior which converted for a non-sequence of entities passed directly to the :class:`.Query` constructor into a single-element sequence. While this behavior was never supported or documented, it's already in use so has been added as a behavioral contract to :class:`.Query`. Change-Id: I97546f5ab5af29f37c86321f39d564f98a12daf5 Fixes: #4269 (cherry picked from commit 5628da627c4f248a817eafd72ecdf4793809f68d) --- doc/build/changelog/unreleased_12/4269.rst | 10 +++++ lib/sqlalchemy/orm/query.py | 22 +++++++++- test/orm/test_query.py | 49 ++++++++++++++++++++++ 3 files changed, 80 insertions(+), 1 deletion(-) create mode 100644 doc/build/changelog/unreleased_12/4269.rst diff --git a/doc/build/changelog/unreleased_12/4269.rst b/doc/build/changelog/unreleased_12/4269.rst new file mode 100644 index 0000000000..63dacfe02c --- /dev/null +++ b/doc/build/changelog/unreleased_12/4269.rst @@ -0,0 +1,10 @@ +.. change:: + :tags: bug, orm + :tickets: 4269 + + Fixed regression caused by :ticket:`4256` (itself a regression fix for + :ticket:`4228`) which breaks an undocumented behavior which converted for a + non-sequence of entities passed directly to the :class:`.Query` constructor + into a single-element sequence. While this behavior was never supported or + documented, it's already in use so has been added as a behavioral contract + to :class:`.Query`. diff --git a/lib/sqlalchemy/orm/query.py b/lib/sqlalchemy/orm/query.py index 1c61de1b11..707f9b4f9b 100644 --- a/lib/sqlalchemy/orm/query.py +++ b/lib/sqlalchemy/orm/query.py @@ -147,7 +147,27 @@ class Query(object): self._entities = [] self._primary_entity = None self._has_mapper_entities = False - if entities: + + # 1. don't run util.to_list() or _set_entity_selectables + # if no entities were passed - major performance bottleneck + # from lazy loader implementation when it seeks to use Query + # class for an identity lookup, causes test_orm.py to fail + # with thousands of extra function calls, see issue #4228 + # for why this use had to be added + # 2. can't use classmethod on Query because session.query_cls + # is an arbitrary callable in some user recipes, not + # necessarily a class, so we don't have the class available. + # see issue #4256 + # 3. can't do "if entities is not None" because we usually get here + # from session.query() which takes in *entities. + # 4. can't do "if entities" because users make use of undocumented + # to_list() behavior here and they pass clause expressions that + # can't be evaluated as boolean. See issue #4269. + # 5. the empty tuple is a singleton in cPython, take advantage of this + # so that we can skip for the empty "*entities" case without using + # any Python overloadable operators. + # + if entities is not (): for ent in util.to_list(entities): entity_wrapper(self, ent) diff --git a/test/orm/test_query.py b/test/orm/test_query.py index d4a122a235..8804975013 100644 --- a/test/orm/test_query.py +++ b/test/orm/test_query.py @@ -4544,6 +4544,9 @@ class QueryClsTest(QueryTest): return MyQueryFactory() + def _plain_fixture(self): + return Query + def _test_get(self, fixture): User = self.classes.User @@ -4570,6 +4573,26 @@ class QueryClsTest(QueryTest): a1 = s.query(Address).filter(Address.id == 1).first() eq_(a1.user, User(id=7)) + def _test_expr(self, fixture): + User, Address = self.classes('User', 'Address') + + s = Session(query_cls=fixture()) + + q = s.query(func.max(User.id).label('max')) + eq_(q.scalar(), 10) + + def _test_expr_undocumented_query_constructor(self, fixture): + # see #4269. not documented but already out there. + User, Address = self.classes('User', 'Address') + + s = Session(query_cls=fixture()) + + q = Query(func.max(User.id).label('max')).with_session(s) + eq_(q.scalar(), 10) + + def test_plain_get(self): + self._test_get(self._plain_fixture) + def test_callable_get(self): self._test_get(self._callable_fixture) @@ -4579,6 +4602,32 @@ class QueryClsTest(QueryTest): def test_fn_get(self): self._test_get(self._fn_fixture) + def test_plain_expr(self): + self._test_expr(self._plain_fixture) + + def test_callable_expr(self): + self._test_expr(self._callable_fixture) + + def test_subclass_expr(self): + self._test_expr(self._subclass_fixture) + + def test_fn_expr(self): + self._test_expr(self._fn_fixture) + + def test_plain_expr_undocumented_query_constructor(self): + self._test_expr_undocumented_query_constructor(self._plain_fixture) + + def test_callable_expr_undocumented_query_constructor(self): + self._test_expr_undocumented_query_constructor( + self._callable_fixture) + + def test_subclass_expr_undocumented_query_constructor(self): + self._test_expr_undocumented_query_constructor( + self._subclass_fixture) + + def test_fn_expr_undocumented_query_constructor(self): + self._test_expr_undocumented_query_constructor(self._fn_fixture) + def test_callable_o2m_lazyload(self): self._test_o2m_lazyload(self._callable_fixture) -- 2.47.2