From: Mike Bayer Date: Fri, 25 Oct 2019 15:34:37 +0000 (-0400) Subject: Don't cache a query that has before_compile modifications X-Git-Tag: rel_1_3_11~21 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=c92f166792a8119bd9243aa8a2fd2038413996f4;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git Don't cache a query that has before_compile modifications 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 (cherry picked from commit d6db28556b095dc85fff3e0e09b0e70358a9538b) --- diff --git a/doc/build/changelog/unreleased_13/4947.rst b/doc/build/changelog/unreleased_13/4947.rst new file mode 100644 index 0000000000..afae0766b7 --- /dev/null +++ b/doc/build/changelog/unreleased_13/4947.rst @@ -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. diff --git a/doc/build/orm/extensions/baked.rst b/doc/build/orm/extensions/baked.rst index 543bff1fad..10a0bbe3d6 100644 --- a/doc/build/orm/extensions/baked.rst +++ b/doc/build/orm/extensions/baked.rst @@ -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 ------------------------------------ diff --git a/lib/sqlalchemy/ext/baked.py b/lib/sqlalchemy/ext/baked.py index 2af42820ee..2bff5da308 100644 --- a/lib/sqlalchemy/ext/baked.py +++ b/lib/sqlalchemy/ext/baked.py @@ -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): @@ -331,6 +338,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) diff --git a/lib/sqlalchemy/orm/events.py b/lib/sqlalchemy/orm/events.py index 8b560804f9..5810cc72c3 100644 --- a/lib/sqlalchemy/orm/events.py +++ b/lib/sqlalchemy/orm/events.py @@ -2391,12 +2391,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:: @@ -2404,6 +2423,7 @@ class QueryEvents(event.Events): :meth:`.QueryEvents.before_compile_delete` + :ref:`baked_with_before_compile` """ @@ -2488,7 +2508,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: @@ -2502,5 +2522,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) diff --git a/lib/sqlalchemy/orm/query.py b/lib/sqlalchemy/orm/query.py index 1ee68b7685..f4cf34458c 100644 --- a/lib/sqlalchemy/orm/query.py +++ b/lib/sqlalchemy/orm/query.py @@ -125,6 +125,7 @@ class Query(object): _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 @@ -3862,8 +3863,10 @@ class Query(object): 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) diff --git a/test/ext/test_baked.py b/test/ext/test_baked.py index 55cd9376bf..0a9a3e3554 100644 --- a/test/ext/test_baked.py +++ b/test/ext/test_baked.py @@ -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 @@ -917,10 +919,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 @@ -977,6 +1046,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") diff --git a/test/orm/test_events.py b/test/orm/test_events.py index 037d5bd9dc..623ab64243 100644 --- a/test/orm/test_events.py +++ b/test/orm/test_events.py @@ -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