diff options
| -rw-r--r-- | doc/build/changelog/unreleased_14/6391.rst | 14 | ||||
| -rw-r--r-- | lib/sqlalchemy/sql/compiler.py | 8 | ||||
| -rw-r--r-- | test/sql/test_compiler.py | 98 |
3 files changed, 116 insertions, 4 deletions
diff --git a/doc/build/changelog/unreleased_14/6391.rst b/doc/build/changelog/unreleased_14/6391.rst new file mode 100644 index 000000000..28af9b395 --- /dev/null +++ b/doc/build/changelog/unreleased_14/6391.rst @@ -0,0 +1,14 @@ +.. change:: + :tags: bug, orm, regression, caching + :tickets: 6391 + + Fixed critical regression where bound parameter tracking as used in the SQL + caching system could fail to track all parameters for the case where the + same SQL expression containing a parameter were used in an ORM-related + query using a feature such as class inheritance, which was then embedded in + an enclosing expression which would make use of that same expression + multiple times, such as a UNION. The ORM would individually copy the + individual SELECT statements as part of compilation with class inheritance, + which then embedded in the enclosing statement would fail to accommodate + for all parameters. The logic that tracks this condition has been adjusted + to work for multiple copies of a parameter.
\ No newline at end of file diff --git a/lib/sqlalchemy/sql/compiler.py b/lib/sqlalchemy/sql/compiler.py index 6168248ff..dfdf5832f 100644 --- a/lib/sqlalchemy/sql/compiler.py +++ b/lib/sqlalchemy/sql/compiler.py @@ -715,7 +715,7 @@ class SQLCompiler(Compiled): self._cache_key_bind_match = ckbm = { b.key: b for b in cache_key[1] } - ckbm.update({b: b for b in cache_key[1]}) + ckbm.update({b: [b] for b in cache_key[1]}) # compile INSERT/UPDATE defaults/sequences to expect executemany # style execution, which may mean no pre-execute of defaults, @@ -934,8 +934,9 @@ class SQLCompiler(Compiled): ckbm = self._cache_key_bind_match resolved_extracted = { - ckbm[b]: extracted + bind: extracted for b, extracted in zip(orig_extracted, extracted_parameters) + for bind in ckbm[b] } else: resolved_extracted = None @@ -2242,7 +2243,6 @@ class SQLCompiler(Compiled): render_postcompile=False, **kwargs ): - if not skip_bind_expression: impl = bindparam.type.dialect_impl(self.dialect) if impl._has_bind_expression: @@ -2313,7 +2313,7 @@ class SQLCompiler(Compiled): for bp in bindparam._cloned_set: if bp.key in ckbm: cb = ckbm[bp.key] - ckbm[cb] = bindparam + ckbm[cb].append(bindparam) if bindparam.isoutparam: self.has_out_parameters = True diff --git a/test/sql/test_compiler.py b/test/sql/test_compiler.py index e19d755a8..29b06851a 100644 --- a/test/sql/test_compiler.py +++ b/test/sql/test_compiler.py @@ -3835,6 +3835,104 @@ class BindParameterTest(AssertsCompiledSQL, fixtures.TestBase): {"myid_1": 10}, ) + def test_construct_duped_params_w_bind_clones_post(self): + """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. + test for #6391 + + """ + + inner_stmt = select(table1.c.myid).where(table1.c.myid == 5) + + stmt = union(inner_stmt, inner_stmt, inner_stmt) + + # get the original bindparam. + original_bind = inner_stmt._where_criteria[0].right + + # same bind three times + is_(stmt.selects[0]._where_criteria[0].right, original_bind) + is_(stmt.selects[1]._where_criteria[0].right, original_bind) + is_(stmt.selects[2]._where_criteria[0].right, original_bind) + + # it's anonymous so unique=True + is_true(original_bind.unique) + + # cache key against hte original param + cache_key = stmt._generate_cache_key() + + # now adapt the statement and separately adapt the inner + # SELECTs, since if these subqueries are also ORM then they get adapted + # separately. + stmt_adapted = sql_util.ClauseAdapter(table1).traverse(stmt) + stmt_adapted.selects[0] = sql_util.ClauseAdapter(table1).traverse( + stmt_adapted.selects[0] + ) + stmt_adapted.selects[1] = sql_util.ClauseAdapter(table1).traverse( + stmt_adapted.selects[1] + ) + stmt_adapted.selects[2] = sql_util.ClauseAdapter(table1).traverse( + stmt_adapted.selects[2] + ) + + # new bind parameter has a different key but same + # identifying key + + new_bind_one = stmt_adapted.selects[0]._where_criteria[0].right + new_bind_two = stmt_adapted.selects[1]._where_criteria[0].right + new_bind_three = stmt_adapted.selects[2]._where_criteria[0].right + + for new_bind in (new_bind_one, new_bind_two, new_bind_three): + eq_(original_bind._identifying_key, new_bind._identifying_key) + ne_(original_bind.key, new_bind.key) + + # compile the adapted statement but set the cache key to the one + # generated from the unadapted statement. this will look like + # when the ORM runs clause adaption inside of visit_select, after + # the cache key is generated but before the compiler is given the + # core select statement to actually render. + compiled = stmt_adapted.compile(cache_key=cache_key) + + # the same parameter was split into three distinct ones, due to + # the separate adaption on a per-subquery basis. but they still + # refer to the original in their _cloned_set and this is what + # has to match up to what's in the cache key. + # params set up as 5 + eq_( + compiled.construct_params( + params={}, + ), + {"myid_1": 5, "myid_2": 5, "myid_3": 5}, + ) + + # also works w the original cache key + eq_( + compiled.construct_params( + params={}, extracted_parameters=cache_key[1] + ), + {"myid_1": 5, "myid_2": 5, "myid_3": 5}, + ) + + # now make a totally new statement with the same cache key + new_inner_stmt = select(table1.c.myid).where(table1.c.myid == 10) + new_stmt = union(new_inner_stmt, new_inner_stmt, new_inner_stmt) + + new_cache_key = new_stmt._generate_cache_key() + + # cache keys match + eq_(cache_key.key, new_cache_key.key) + + # ensure we get "10" from construct params. if it matched + # based on .key and not ._identifying_key, it would not see that + # the bind parameter is part of the cache key. + # before #6391 was fixed you would see 5, 5, 10 + eq_( + compiled.construct_params( + params={}, extracted_parameters=new_cache_key[1] + ), + {"myid_1": 10, "myid_2": 10, "myid_3": 10}, + ) + def test_construct_params_w_bind_clones_pre(self): """test that a BindParameter that has been cloned before the cache key was generated, and was doubled up just to make sure it has to |