From: Mike Bayer Date: Wed, 6 Apr 2022 13:41:11 +0000 (-0400) Subject: maintain complete cloned_set for BindParameter X-Git-Tag: rel_2_0_0b1~374 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=2168a64affb2e299b9a37079af7b2a8d4ae0ff64;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git maintain complete cloned_set for BindParameter 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 --- diff --git a/doc/build/changelog/unreleased_14/7903.rst b/doc/build/changelog/unreleased_14/7903.rst new file mode 100644 index 0000000000..c2a4e00787 --- /dev/null +++ b/doc/build/changelog/unreleased_14/7903.rst @@ -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. diff --git a/lib/sqlalchemy/sql/elements.py b/lib/sqlalchemy/sql/elements.py index aec29d1b2e..309c01e400 100644 --- a/lib/sqlalchemy/sql/elements.py +++ b/lib/sqlalchemy/sql/elements.py @@ -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 diff --git a/lib/sqlalchemy/testing/fixtures.py b/lib/sqlalchemy/testing/fixtures.py index 8c0120bcc4..b4b9cab5fd 100644 --- a/lib/sqlalchemy/testing/fixtures.py +++ b/lib/sqlalchemy/testing/fixtures.py @@ -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 diff --git a/test/aaa_profiling/test_memusage.py b/test/aaa_profiling/test_memusage.py index 24cc0b99b4..e90c428f70 100644 --- a/test/aaa_profiling/test_memusage.py +++ b/test/aaa_profiling/test_memusage.py @@ -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() diff --git a/test/orm/inheritance/test_polymorphic_rel.py b/test/orm/inheritance/test_polymorphic_rel.py index 8a218847df..5eac274cf4 100644 --- a/test/orm/inheritance/test_polymorphic_rel.py +++ b/test/orm/inheritance/test_polymorphic_rel.py @@ -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 diff --git a/test/sql/test_compiler.py b/test/sql/test_compiler.py index 6ca06dc0e1..bf31824e56 100644 --- a/test/sql/test_compiler.py +++ b/test/sql/test_compiler.py @@ -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.