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

diff --git a/doc/build/changelog/unreleased_12/4269.rst b/doc/build/changelog/unreleased_12/4269.rst
new file mode 100644 (file)
index 0000000..63dacfe
--- /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 56e42a7029421389f4b92d789b00f62a680958a0..8f6042a2b5d9784a8dd9d21c1d881da3818eef16 100644 (file)
--- 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 d4a122a235a0b429e5e40fc2fa6183de8e95b85e..8804975013109881bb35b57a3458bd68a82f3747 100644 (file)
--- 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)