]> git.ipfire.org Git - thirdparty/sqlalchemy/sqlalchemy.git/commitdiff
send user defined options from the current query
authorMike Bayer <mike_mp@zzzcomputing.com>
Tue, 17 Aug 2021 19:03:57 +0000 (15:03 -0400)
committermike bayer <mike_mp@zzzcomputing.com>
Tue, 17 Aug 2021 19:15:30 +0000 (19:15 +0000)
Revised the means by which the
:attr:`_orm.ORMExecuteState.user_defined_options` accessor receives
:class:`_orm.UserDefinedOption` and related option objects from the
context, with particular emphasis on the "selectinload" on the loader
strategy where this previously was not working; other strategies did not
have this problem. The objects that are associated with the current query
being executed, and not that of a query being cached, are now propagated
unconditionally. This essentially separates them out from the "loader
strategy" options which are explicitly associated with the compiled state
of a query and need to be used in relation to the cached query.

The effect of this fix is that a user-defined option, such as those used
by the dogpile.caching example as well as for other recipes such as
defining a "shard id" for the horizontal sharing extension, will be
correctly propagated to eager and lazy loaders regardless of whether
a cached query was ultimately invoked.

Fixes: #6887
Change-Id: Ieaae5b01c85de26ea732ebd625e6e5823a470492

doc/build/changelog/unreleased_14/6887.rst [new file with mode: 0644]
lib/sqlalchemy/orm/__init__.py
lib/sqlalchemy/orm/strategies.py
lib/sqlalchemy/testing/fixtures.py
test/orm/test_events.py

diff --git a/doc/build/changelog/unreleased_14/6887.rst b/doc/build/changelog/unreleased_14/6887.rst
new file mode 100644 (file)
index 0000000..8992dcc
--- /dev/null
@@ -0,0 +1,21 @@
+.. change::
+    :tags: bug, orm
+    :tickets: 6887
+
+    Revised the means by which the
+    :attr:`_orm.ORMExecuteState.user_defined_options` accessor receives
+    :class:`_orm.UserDefinedOption` and related option objects from the
+    context, with particular emphasis on the "selectinload" on the loader
+    strategy where this previously was not working; other strategies did not
+    have this problem. The objects that are associated with the current query
+    being executed, and not that of a query being cached, are now propagated
+    unconditionally. This essentially separates them out from the "loader
+    strategy" options which are explicitly associated with the compiled state
+    of a query and need to be used in relation to the cached query.
+
+    The effect of this fix is that a user-defined option, such as those used
+    by the dogpile.caching example as well as for other recipes such as
+    defining a "shard id" for the horizontal sharing extension, will be
+    correctly propagated to eager and lazy loaders regardless of whether
+    a cached query was ultimately invoked.
+
index a247b597b21dc8626be7fc7769e9993f29704973..8e964784083c051ec6276af55565ebefd4929ea7 100644 (file)
@@ -44,6 +44,7 @@ from .interfaces import MapperProperty
 from .interfaces import NOT_EXTENSION
 from .interfaces import ONETOMANY
 from .interfaces import PropComparator
+from .interfaces import UserDefinedOption
 from .loading import merge_frozen_result
 from .loading import merge_result
 from .mapper import class_mapper
index bf60c803d23bf6888abd35fbd6d25e173b426051..069e5e667443ea50e715d1de51b6899d5499840c 100644 (file)
@@ -1512,21 +1512,14 @@ class SubqueryLoader(PostLoader):
         loadopt,
     ):
 
-        if orig_query is context.query:
-            options = new_options = orig_query._with_options
-        else:
-            # There's currently no test that exercises the necessity of
-            # this step for subqueryload.  Added in #6881, it is necessary for
-            # selectinload, but its necessity for subqueryload is still
-            # theoretical.
-            options = orig_query._with_options
-
-            new_options = [
-                orig_opt._adjust_for_extra_criteria(context)
-                if orig_opt._is_strategy_option
-                else orig_opt
-                for orig_opt in options
-            ]
+        # note that because the subqueryload object
+        # does not re-use the cached query, instead always making
+        # use of the current invoked query, while we have two queries
+        # here (orig and context.query), they are both non-cached
+        # queries and we can transfer the options as is without
+        # adjusting for new criteria.   Some work on #6881 / #6889
+        # brought this into question.
+        new_options = orig_query._with_options
 
         if loadopt and loadopt._extra_criteria:
 
@@ -2933,9 +2926,12 @@ class SelectInLoader(PostLoader, util.MemoizedSlots):
 
         if orig_query is context.query:
             options = new_options = orig_query._with_options
+            user_defined_options = []
         else:
             options = orig_query._with_options
 
+            # propagate compile state options from the original query,
+            # updating their "extra_criteria" as necessary.
             # note this will create a different cache key than
             # "orig" options if extra_criteria is present, because the copy
             # of extra_criteria will have different boundparam than that of
@@ -2946,6 +2942,14 @@ class SelectInLoader(PostLoader, util.MemoizedSlots):
                 if orig_opt._is_strategy_option
                 else orig_opt
                 for orig_opt in options
+                if orig_opt._is_compile_state or orig_opt._is_legacy_option
+            ]
+
+            # propagate user defined options from the current query
+            user_defined_options = [
+                opt
+                for opt in context.query._with_options
+                if not opt._is_compile_state and not opt._is_legacy_option
             ]
 
         if loadopt and loadopt._extra_criteria:
@@ -2959,6 +2963,8 @@ class SelectInLoader(PostLoader, util.MemoizedSlots):
         q = q.options(*new_options)._update_compile_options(
             {"_current_path": effective_path}
         )
+        if user_defined_options:
+            q = q.options(*user_defined_options)
 
         if context.populate_existing:
             q = q.execution_options(populate_existing=True)
index 30ada71cd32f98e149de42f951bdd00f1a8cbf3e..e6af0c546c4b83d6e887ef3a93925cceda38089a 100644 (file)
@@ -548,7 +548,10 @@ _fixture_sessions = set()
 def fixture_session(**kw):
     kw.setdefault("autoflush", True)
     kw.setdefault("expire_on_commit", True)
-    sess = sa.orm.Session(config.db, **kw)
+
+    bind = kw.pop("bind", config.db)
+
+    sess = sa.orm.Session(bind, **kw)
     _fixture_sessions.add(sess)
     return sess
 
index cd8adf8e35ebcc1df7b40f3ae248a2f2fbb701e8..6831c8b108acfc56d3af396f937bd95069b572d8 100644 (file)
@@ -17,6 +17,8 @@ from sqlalchemy.orm import deferred
 from sqlalchemy.orm import events
 from sqlalchemy.orm import EXT_SKIP
 from sqlalchemy.orm import instrumentation
+from sqlalchemy.orm import joinedload
+from sqlalchemy.orm import lazyload
 from sqlalchemy.orm import Mapper
 from sqlalchemy.orm import mapper
 from sqlalchemy.orm import mapperlib
@@ -26,6 +28,8 @@ from sqlalchemy.orm import selectinload
 from sqlalchemy.orm import Session
 from sqlalchemy.orm import sessionmaker
 from sqlalchemy.orm import subqueryload
+from sqlalchemy.orm import UserDefinedOption
+from sqlalchemy.sql.traversals import NO_CACHE
 from sqlalchemy.testing import assert_raises
 from sqlalchemy.testing import assert_raises_message
 from sqlalchemy.testing import AssertsCompiledSQL
@@ -139,6 +143,97 @@ class ORMExecuteTest(_RemoveListeners, _fixtures.FixtureTest):
             ),
         )
 
+    @testing.combinations(
+        ("fixed",), ("payload",), ("dont_cache"), argnames="key_type"
+    )
+    @testing.combinations(
+        (lazyload, 10),
+        (selectinload, 3),
+        (joinedload, 1),
+        (subqueryload, 3),
+        argnames="loader_opt, num_opts",
+    )
+    @testing.combinations((True,), (False,), argnames="use_query_cache")
+    def test_user_option_propagation(
+        self, key_type, loader_opt, num_opts, use_query_cache
+    ):
+        """test #6887.
+
+        The one case here which would break before the bug was fixed is:
+
+        use_query_cache=True, loader_opt=selectinload, key_type="fixed"
+
+        Meaning with a fixed cache key and query caching in place, the user
+        defined loader option would be cached under the same name each time,
+        leading to use of the stale option when using selectinload, as this
+        strategy transfers query options from the cached ORM select into the
+        one that it generates. no other strategy currently does this.
+
+        """
+        User, Address, Dinagling = self.classes("User", "Address", "Dingaling")
+
+        class SetShardOption(UserDefinedOption):
+            propagate_to_loaders = True
+
+            def _gen_cache_key(self, anon_map, bindparams):
+                if key_type == "fixed":
+                    return (None,)
+                elif key_type == "payload":
+                    return (self.payload,)
+                elif key_type == "dont_cache":
+                    anon_map[NO_CACHE] = True
+                    return None
+                else:
+                    assert False
+
+        if use_query_cache:
+            s = fixture_session()
+        else:
+            s = fixture_session(
+                bind=testing.db.execution_options(compiled_cache=None)
+            )
+
+        m1 = Mock()
+
+        @event.listens_for(s, "do_orm_execute")
+        def go(context):
+            for elem in context.user_defined_options:
+                if isinstance(elem, SetShardOption):
+                    m1.update_execution_options(_sa_shard_id=elem.payload)
+
+        stmt = select(User).options(
+            loader_opt(User.addresses).options(loader_opt(Address.dingaling)),
+            SetShardOption("some_shard"),
+        )
+        for u in s.execute(stmt).unique().scalars():
+            for a in u.addresses:
+                a.dingaling
+
+        s.close()
+
+        stmt = select(User).options(
+            loader_opt(User.addresses).options(loader_opt(Address.dingaling)),
+            SetShardOption("some_other_shard"),
+        )
+        for u in s.execute(stmt).unique().scalars():
+            for a in u.addresses:
+                a.dingaling
+        eq_(
+            m1.mock_calls,
+            (
+                [call.update_execution_options(_sa_shard_id="some_shard")]
+                * num_opts
+            )
+            + (
+                [
+                    call.update_execution_options(
+                        _sa_shard_id="some_other_shard"
+                    )
+                ]
+                * num_opts
+            ),
+        )
+
     def test_chained_events_one(self):
 
         sess = Session(testing.db, future=True)