]> git.ipfire.org Git - thirdparty/sqlalchemy/sqlalchemy.git/commitdiff
Don't cache a query that has before_compile modifications
authorMike Bayer <mike_mp@zzzcomputing.com>
Fri, 25 Oct 2019 15:34:37 +0000 (11:34 -0400)
committerMike Bayer <mike_mp@zzzcomputing.com>
Sat, 26 Oct 2019 22:16:02 +0000 (18:16 -0400)
The :class:`.BakedQuery` will not cache a query that was modified by a
:meth:`.QueryEvents.before_compile` event, so that compilation hooks that
may be applying ad-hoc modifications to queries will take effect on each
run.  In particular this is helpful for events that modify queries used in
lazy loading as well as eager loading such as "select in" loading.  In
order to re-enable caching for a query modified by this event, a new
flag ``bake_ok`` is added; see :ref:`baked_with_before_compile` for
details.

A longer term plan to provide a new form of SQL caching should solve this
kind of issue more comprehensively.

Fixes: #4947
Change-Id: I5823c4fa00e7b6d46a2e8461b02d8b16605a6ed0

doc/build/changelog/unreleased_13/4947.rst [new file with mode: 0644]
doc/build/orm/extensions/baked.rst
lib/sqlalchemy/ext/baked.py
lib/sqlalchemy/orm/events.py
lib/sqlalchemy/orm/query.py
test/ext/test_baked.py
test/orm/test_events.py

diff --git a/doc/build/changelog/unreleased_13/4947.rst b/doc/build/changelog/unreleased_13/4947.rst
new file mode 100644 (file)
index 0000000..afae076
--- /dev/null
@@ -0,0 +1,15 @@
+.. change::
+    :tags: bug, orm
+    :tickets: 4947
+
+    The :class:`.BakedQuery` will not cache a query that was modified by a
+    :meth:`.QueryEvents.before_compile` event, so that compilation hooks that
+    may be applying ad-hoc modifications to queries will take effect on each
+    run.  In particular this is helpful for events that modify queries used in
+    lazy loading as well as eager loading such as "select in" loading.  In
+    order to re-enable caching for a query modified by this event, a new
+    flag ``bake_ok`` is added; see :ref:`baked_with_before_compile` for
+    details.
+
+    A longer term plan to provide a new form of SQL caching should solve this
+    kind of issue more comprehensively.
index 543bff1fadb95e6f35be38c6f4b935f3be999957..10a0bbe3d60c64efc9ea718caf04fbd3826ba0fb 100644 (file)
@@ -394,6 +394,40 @@ of the baked query::
 
 .. versionadded:: 1.3
 
+.. _baked_with_before_compile:
+
+Using the before_compile event
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+As of SQLAlchemy 1.3.11, the use of the :meth:`.QueryEvents.before_compile`
+event against a particular :class:`.Query` will disallow the baked query
+system from caching the query, if the event hook returns a new :class:`.Query`
+object that is different from the one passed in.  This is so that the
+:meth:`.QueryEvents.before_compile` hook may be invoked against a particular
+:class:`.Query` every time it is used, to accommodate for hooks that
+alter the query differently each time.    To allow a
+:meth:`.QueryEvents.before_compile` to alter a :meth:`.Query` object, but
+still to allow the result to be cached, the event can be registered
+passing the ``bake_ok=True`` flag::
+
+    @event.listens_for(
+        Query, "before_compile", retval=True, bake_ok=True)
+    def my_event(query):
+        for desc in query.column_descriptions:
+            if desc['type'] is User:
+                entity = desc['entity']
+                query = query.filter(entity.deleted == False)
+        return query
+
+The above strategy is appropriate for an event that will modify a
+given :class:`.Query` in exactly the same way every time, not dependent
+on specific parameters or external state that changes.
+
+.. versionadded:: 1.3.11  - added the "bake_ok" flag to the
+   :meth:`.QueryEvents.before_compile` event and disallowed caching via
+   the "baked" extension from occurring for event handlers that
+   return  a new :class:`.Query` object if this flag is not set.
+
 
 Disabling Baked Queries Session-wide
 ------------------------------------
index 44e28d04548848e61aa59685830e0a581a9693f2..d18a35a4072938d8b9bd49317f227d950a610d54 100644 (file)
@@ -225,6 +225,7 @@ class BakedQuery(object):
         query = self._as_query(session)
 
         context = query._compile_context()
+
         self._bake_subquery_loaders(session, context)
         context.session = None
         context.query = query = context.query.with_session(None)
@@ -242,7 +243,13 @@ class BakedQuery(object):
             "_joinpoint",
         ):
             query.__dict__.pop(attr, None)
-        self._bakery[self._effective_key(session)] = context
+
+        # if the query is not safe to cache, we still do everything as though
+        # we did cache it, since the receiver of _bake() assumes subqueryload
+        # context was set up, etc.
+        if context.query._bake_ok:
+            self._bakery[self._effective_key(session)] = context
+
         return context
 
     def to_query(self, query_or_session):
@@ -332,6 +339,9 @@ class BakedQuery(object):
         like a Query object.
 
         """
+        if "baked_queries" not in context.attributes:
+            return
+
         for k, cache_key, query in context.attributes["baked_queries"]:
             bk = BakedQuery(
                 self._bakery, lambda sess, q=query: q.with_session(sess)
index 5bb67b68ff5e8aa38dea249c41a8454c93d060bd..2998a76398f26dcf90466172149e61e6dc55b55c 100644 (file)
@@ -2397,12 +2397,31 @@ class QueryEvents(event.Events):
         The event should normally be listened with the ``retval=True``
         parameter set, so that the modified query may be returned.
 
-        .. warning::  If the :meth:`.QueryEvents.before_compile` event is to
-           be applied to :class:`.Query` objects that are used for lazy loading
-           of :func:`.relationships` (as described at :ref:`lazy_loading`),
-           it may be necessary to set :paramref:`.relationship.bake_queries`
-           to ``False``, else the :meth:`.QueryEvents.before_compile` event
-           will not be invoked for each lazy load operation.
+        The :meth:`.QueryEvents.before_compile` event by default
+        will disallow "baked" queries from caching a query, if the event
+        hook returns a new :class:`.Query` object.   This affects both direct
+        use of the baked query extension as well as its operation within
+        lazy loaders and eager loaders for relationships.  In order to
+        re-establish the query being cached, apply the event adding the
+        ``bake_ok`` flag::
+
+            @event.listens_for(
+                Query, "before_compile", retval=True, bake_ok=True)
+            def my_event(query):
+                for desc in query.column_descriptions:
+                    if desc['type'] is User:
+                        entity = desc['entity']
+                        query = query.filter(entity.deleted == False)
+                return query
+
+        When ``bake_ok`` is set to True, the event hook will only be invoked
+        once, and not called for subsequent invocations of a particular query
+        that is being cached.
+
+        .. versionadded:: 1.3.11  - added the "bake_ok" flag to the
+           :meth:`.QueryEvents.before_compile` event and disallowed caching via
+           the "baked" extension from occurring for event handlers that
+           return  a new :class:`.Query` object if this flag is not set.
 
         .. seealso::
 
@@ -2410,6 +2429,7 @@ class QueryEvents(event.Events):
 
             :meth:`.QueryEvents.before_compile_delete`
 
+            :ref:`baked_with_before_compile`
 
         """
 
@@ -2494,7 +2514,7 @@ class QueryEvents(event.Events):
         """
 
     @classmethod
-    def _listen(cls, event_key, retval=False, **kw):
+    def _listen(cls, event_key, retval=False, bake_ok=False, **kw):
         fn = event_key._listen_fn
 
         if not retval:
@@ -2508,5 +2528,13 @@ class QueryEvents(event.Events):
                     return fn(*arg, **kw)
 
             event_key = event_key.with_wrapper(wrap)
+        else:
+            # don't assume we can apply an attribute to the callable
+            def wrap(*arg, **kw):
+                return fn(*arg, **kw)
+
+            event_key = event_key.with_wrapper(wrap)
+
+        wrap._bake_ok = bake_ok
 
         event_key.base_listen(**kw)
index 92f9ee9525820b854c1d947fa85fe2e6adc5e4f2..fc443d053e8c862dbbfe849b4cc80475656c57ab 100644 (file)
@@ -127,6 +127,7 @@ class Query(Generative):
     _orm_only_from_obj_alias = True
     _current_path = _path_registry
     _has_mapper_entities = False
+    _bake_ok = True
 
     lazy_loaded_from = None
     """An :class:`.InstanceState` that is using this :class:`.Query` for a
@@ -3812,8 +3813,10 @@ class Query(Generative):
         if self.dispatch.before_compile:
             for fn in self.dispatch.before_compile:
                 new_query = fn(self)
-                if new_query is not None:
+                if new_query is not None and new_query is not self:
                     self = new_query
+                    if not fn._bake_ok:
+                        self._bake_ok = False
 
         context = QueryContext(self)
 
index 6750b189107121dbeb29879e3b3dffdf44e86feb..acefe625aba41bd75f6abedec717f6a0639d36f7 100644 (file)
@@ -2,8 +2,10 @@ import contextlib
 import itertools
 
 from sqlalchemy import bindparam
+from sqlalchemy import event
 from sqlalchemy import exc as sa_exc
 from sqlalchemy import func
+from sqlalchemy import literal_column
 from sqlalchemy import testing
 from sqlalchemy.ext import baked
 from sqlalchemy.orm import aliased
@@ -919,10 +921,77 @@ class ResultTest(BakedTest):
 
         self.assert_sql_count(testing.db, go, 2)
 
+    @testing.fixture()
+    def before_compile_nobake_fixture(self):
+        @event.listens_for(Query, "before_compile", retval=True)
+        def _modify_query(query):
+            query = query.enable_assertions(False)
+            return query
+
+        yield
+        event.remove(Query, "before_compile", _modify_query)
+
+    def test_subqueryload_post_context_w_cancelling_event(
+        self, before_compile_nobake_fixture
+    ):
+        User = self.classes.User
+        Address = self.classes.Address
+
+        assert_result = [
+            User(
+                id=7, addresses=[Address(id=1, email_address="jack@bean.com")]
+            )
+        ]
+
+        self.bakery = baked.bakery(size=3)
+
+        bq = self.bakery(lambda s: s.query(User))
+
+        bq += lambda q: q.options(subqueryload(User.addresses))
+        bq += lambda q: q.order_by(User.id)
+        bq += lambda q: q.filter(User.name == bindparam("name"))
+        sess = Session()
+
+        def set_params(q):
+            return q.params(name="jack")
+
+        # test that the changes we make using with_post_criteria()
+        # are also applied to the subqueryload query.
+        def go():
+            result = bq(sess).with_post_criteria(set_params).all()
+            eq_(assert_result, result)
+
+        self.assert_sql_count(testing.db, go, 2)
+
 
 class LazyLoaderTest(testing.AssertsCompiledSQL, BakedTest):
     run_setup_mappers = "each"
 
+    @testing.fixture
+    def modify_query_fixture(self):
+        def set_event(bake_ok):
+
+            event.listen(
+                Query,
+                "before_compile",
+                _modify_query,
+                retval=True,
+                bake_ok=bake_ok,
+            )
+            return m1
+
+        m1 = mock.Mock()
+
+        def _modify_query(query):
+            m1(query.column_descriptions[0]["entity"])
+            query = query.enable_assertions(False).filter(
+                literal_column("1") == 1
+            )
+            return query
+
+        yield set_event
+        event.remove(Query, "before_compile", _modify_query)
+
     def _o2m_fixture(self, lazy="select", **kw):
         User = self.classes.User
         Address = self.classes.Address
@@ -979,6 +1048,45 @@ class LazyLoaderTest(testing.AssertsCompiledSQL, BakedTest):
         )
         return User, Address
 
+    def test_no_cache_for_event(self, modify_query_fixture):
+
+        m1 = modify_query_fixture(False)
+
+        User, Address = self._o2m_fixture()
+
+        sess = Session()
+        u1 = sess.query(User).filter(User.id == 7).first()
+
+        u1.addresses
+
+        eq_(m1.mock_calls, [mock.call(User), mock.call(Address)])
+
+        sess.expire(u1, ["addresses"])
+
+        u1.addresses
+        eq_(
+            m1.mock_calls,
+            [mock.call(User), mock.call(Address), mock.call(Address)],
+        )
+
+    def test_cache_ok_for_event(self, modify_query_fixture):
+
+        m1 = modify_query_fixture(True)
+
+        User, Address = self._o2m_fixture()
+
+        sess = Session()
+        u1 = sess.query(User).filter(User.id == 7).first()
+
+        u1.addresses
+
+        eq_(m1.mock_calls, [mock.call(User), mock.call(Address)])
+
+        sess.expire(u1, ["addresses"])
+
+        u1.addresses
+        eq_(m1.mock_calls, [mock.call(User), mock.call(Address)])
+
     def test_unsafe_unbound_option_cancels_bake(self):
         User, Address, Dingaling = self._o2m_twolevel_fixture(lazy="joined")
 
index 3cbbb517e075490ca27f39101087e55b3b2ae9a0..b0b140ffa5e08672a2e7339ac3e087a3c7d093a4 100644 (file)
@@ -2431,6 +2431,21 @@ class QueryEventsTest(
             checkparams={"id_2": 10, "id_1": 7},
         )
 
+    def test_before_compile_no_retval(self):
+        counter = [0]
+
+        @event.listens_for(query.Query, "before_compile")
+        def count(query):
+            counter[0] += 1
+
+        User = self.classes.User
+        s = Session()
+
+        q = s.query(User).filter_by(id=7)
+        str(q)
+        str(q)
+        eq_(counter, [2])
+
     def test_alters_entities(self):
         User = self.classes.User