]> git.ipfire.org Git - thirdparty/sqlalchemy/sqlalchemy.git/commitdiff
maintain complete cloned_set for BindParameter
authorMike Bayer <mike_mp@zzzcomputing.com>
Wed, 6 Apr 2022 13:41:11 +0000 (09:41 -0400)
committerMike Bayer <mike_mp@zzzcomputing.com>
Wed, 6 Apr 2022 14:01:43 +0000 (10:01 -0400)
Fixed regression caused by :ticket:`7823` which impacted the caching
system, such that bound parameters that had been "cloned" within ORM
operations, such as polymorphic loading, would in some cases not acquire
their correct execution-time value leading to incorrect bind values being
rendered.

Fixes: #7903
Change-Id: I61c802749b859bebeb127d24e66d6e77d13ce57a

doc/build/changelog/unreleased_14/7903.rst [new file with mode: 0644]
lib/sqlalchemy/sql/elements.py
lib/sqlalchemy/testing/fixtures.py
test/aaa_profiling/test_memusage.py
test/orm/inheritance/test_polymorphic_rel.py
test/sql/test_compiler.py

diff --git a/doc/build/changelog/unreleased_14/7903.rst b/doc/build/changelog/unreleased_14/7903.rst
new file mode 100644 (file)
index 0000000..c2a4e00
--- /dev/null
@@ -0,0 +1,9 @@
+.. change::
+    :tags: bug, regression, caching
+    :tickets: 7903
+
+    Fixed regression caused by :ticket:`7823` which impacted the caching
+    system, such that bound parameters that had been "cloned" within ORM
+    operations, such as polymorphic loading, would in some cases not acquire
+    their correct execution-time value leading to incorrect bind values being
+    rendered.
index aec29d1b2e2107c48d1c24952061a7f6993191e6..309c01e400be6b0277372196c2eaf99966795d09 100644 (file)
@@ -370,7 +370,6 @@ class ClauseElement(
         # old table.
         cc = self._is_clone_of
         c._is_clone_of = cc if cc is not None else self
-
         return c
 
     def _negate_in_binary(self, negated_op, original_op):
@@ -1942,6 +1941,15 @@ class BindParameter(roles.InElementRole, ColumnElement[_T]):
         self: SelfBindParameter, maintain_key: bool = False, **kw: Any
     ) -> SelfBindParameter:
         c = ClauseElement._clone(self, **kw)
+        # ensure all the BindParameter objects stay in cloned set.
+        # in #7823, we changed "clone" so that a clone only keeps a reference
+        # to the "original" element, since for column correspondence, that's
+        # all we need.   However, for BindParam, _cloned_set is used by
+        # the "cache key bind match" lookup, which means if any of those
+        # interim BindParameter objects became part of a cache key in the
+        # cache, we need it.  So here, make sure all clones keep carrying
+        # forward.
+        c._cloned_set.update(self._cloned_set)
         if not maintain_key and self.unique:
             c.key = _anonymous_label.safe_construct(
                 id(c), c._orig_key or "param", sanitize_key=True
index 8c0120bcc4322a8bb5c718774cab84c9f3d2215a..b4b9cab5fd3b65bb1342645019782d96725a8102 100644 (file)
@@ -51,6 +51,13 @@ class TestBase:
     def assert_(self, val, msg=None):
         assert val, msg
 
+    @config.fixture()
+    def nocache(self):
+        _cache = config.db._compiled_cache
+        config.db._compiled_cache = None
+        yield
+        config.db._compiled_cache = _cache
+
     @config.fixture()
     def connection_no_trans(self):
         eng = getattr(self, "bind", None) or config.db
index 24cc0b99b480684b1ea2d8c7e4841d9f3ad4da9c..e90c428f70584380f5d91e1b35dcb01c5c9d688d 100644 (file)
@@ -348,8 +348,15 @@ class MemUsageTest(EnsureZeroed):
         go()
 
     def test_clone_expression(self):
+        # this test is for the memory issue "fixed" in #7823, where clones
+        # no longer carry along all past elements.
+        # However, due to #7903, we can't at the moment use a
+        # BindParameter here - these have to continue to carry along all
+        # the previous clones for now.  So the test here only works with
+        # expressions that dont have BindParameter objects in them.
+
+        root_expr = column("x", Integer) == column("y", Integer)
 
-        root_expr = column("x", Integer) == 12
         expr = root_expr
 
         @profile_memory()
index 8a218847df5571de5187590f62400ae296ebf737..5eac274cf4cacaf336e3dab32345ccc4ac2a6d7b 100644 (file)
@@ -10,10 +10,10 @@ from sqlalchemy.orm import join
 from sqlalchemy.orm import joinedload
 from sqlalchemy.orm import selectinload
 from sqlalchemy.orm import subqueryload
+from sqlalchemy.orm import with_parent
 from sqlalchemy.orm import with_polymorphic
 from sqlalchemy.testing import assert_raises
 from sqlalchemy.testing import eq_
-from sqlalchemy.testing import fixtures
 from sqlalchemy.testing.assertsql import CompiledSQL
 from sqlalchemy.testing.fixtures import fixture_session
 from ._poly_fixtures import _Polymorphic
@@ -30,7 +30,7 @@ from ._poly_fixtures import Paperwork
 from ._poly_fixtures import Person
 
 
-class _PolymorphicTestBase(fixtures.NoCache):
+class _PolymorphicTestBase:
     __backend__ = True
     __dialect__ = "default_enhanced"
 
@@ -195,6 +195,34 @@ class _PolymorphicTestBase(fixtures.NoCache):
             Boss(name="pointy haired boss", golf_swing="fore"),
         )
 
+    def test_lazyload_related_w_cache_check(self):
+        sess = fixture_session()
+
+        c1 = sess.get(Company, 1)
+        c2 = sess.get(Company, 2)
+
+        q1 = (
+            sess.query(Person)
+            .filter(with_parent(c1, Company.employees))
+            .order_by(Person.person_id)
+        )
+        eq_(
+            q1.all(),
+            [
+                Engineer(name="dilbert"),
+                Engineer(name="wally"),
+                Boss(name="pointy haired boss"),
+                Manager(name="dogbert"),
+            ],
+        )
+
+        q2 = (
+            sess.query(Person)
+            .filter(with_parent(c2, Company.employees))
+            .order_by(Person.person_id)
+        )
+        eq_(q2.all(), [Engineer(name="vlad")])
+
     def test_multi_join(self):
         sess = fixture_session()
         e = aliased(Person)
@@ -881,7 +909,7 @@ class _PolymorphicTestBase(fixtures.NoCache):
 
         self.assert_sql_count(testing.db, go, 1)
 
-    def test_with_polymorphic_three_future(self):
+    def test_with_polymorphic_three_future(self, nocache):
         sess = fixture_session()
 
         def go():
@@ -2246,6 +2274,12 @@ class PolymorphicPolymorphicTest(
 
 
 class PolymorphicUnionsTest(_PolymorphicTestBase, _PolymorphicUnions):
+    @testing.skip_if(
+        lambda: True, "join condition doesn't work w/ this mapping"
+    )
+    def test_lazyload_related_w_cache_check(self):
+        pass
+
     def test_with_polymorphic_two_future_default_wp(self):
         """test #7262
 
@@ -2347,6 +2381,12 @@ class PolymorphicUnionsTest(_PolymorphicTestBase, _PolymorphicUnions):
 class PolymorphicAliasedJoinsTest(
     _PolymorphicTestBase, _PolymorphicAliasedJoins
 ):
+    @testing.skip_if(
+        lambda: True, "join condition doesn't work w/ this mapping"
+    )
+    def test_lazyload_related_w_cache_check(self):
+        pass
+
     def test_with_polymorphic_two_future_default_wp(self):
         """test #7262
 
index 6ca06dc0e16896e1379f8b6d171fd31a4464905f..bf31824e56783bf3feadccebd70b299d7e1b07c0 100644 (file)
@@ -3945,7 +3945,8 @@ class BindParameterTest(AssertsCompiledSQL, fixtures.TestBase):
             extracted_parameters=s1_cache_key[1],
         )
 
-    def test_construct_params_w_bind_clones_post(self):
+    @testing.combinations(True, False, argnames="adapt_before_key")
+    def test_construct_params_w_bind_clones_post(self, adapt_before_key):
         """test that a BindParameter that has been cloned after the cache
         key was generated still matches up when construct_params()
         is called with an extracted parameter collection.
@@ -3969,6 +3970,11 @@ class BindParameterTest(AssertsCompiledSQL, fixtures.TestBase):
         # it's anonymous so unique=True
         is_true(original_bind.unique)
 
+        # test #7903 - adapt the statement *before* we make the cache
+        # key also
+        if adapt_before_key:
+            stmt = sql_util.ClauseAdapter(table1).traverse(stmt)
+
         # cache key against the original param
         cache_key = stmt._generate_cache_key()
 
@@ -4021,7 +4027,8 @@ class BindParameterTest(AssertsCompiledSQL, fixtures.TestBase):
             {"myid_1": 10},
         )
 
-    def test_construct_duped_params_w_bind_clones_post(self):
+    @testing.combinations(True, False, argnames="adapt_before_key")
+    def test_construct_duped_params_w_bind_clones_post(self, adapt_before_key):
         """same as previous test_construct_params_w_bind_clones_post but
         where the binds have been used
         repeatedly, and the adaption occurs on a per-subquery basis.
@@ -4044,6 +4051,10 @@ class BindParameterTest(AssertsCompiledSQL, fixtures.TestBase):
         # it's anonymous so unique=True
         is_true(original_bind.unique)
 
+        # variant that exercises #7903
+        if adapt_before_key:
+            stmt = sql_util.ClauseAdapter(table1).traverse(stmt)
+
         # cache key against the original param
         cache_key = stmt._generate_cache_key()
 
@@ -4125,7 +4136,7 @@ class BindParameterTest(AssertsCompiledSQL, fixtures.TestBase):
         be unique, still matches up when construct_params()
         is called with an extracted parameter collection.
 
-        other ORM feaures like optimized_compare() end up doing something
+        other ORM features like optimized_compare() end up doing something
         like this, such as if there are multiple "has()" or "any()" which would
         have cloned the join condition and changed the values of bound
         parameters.