From a7899c44ba15076df8869f5b40d720ccf09d5273 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Fri, 13 Aug 2021 11:07:17 -0400 Subject: rewrite _extra_criteria in selectinload; propagate correctly to Load Fixed issue in :func:`_orm.selectinload` where use of the new :meth:`_orm.PropComparator.and_` feature within options that were nested more than one level deep would fail to update bound parameter values that were in the nested criteria, as a side effect of SQL statement caching. Implementation adds a new step that rewrites the parameters inside of all _extra_criteria when invoking selectinload as well as subqueryload. Additionally, changed how Load() gets "extra_criteria", in that it pulls it from UnboundLoad._extra_criteria instead of re-fetching it from the path elements, which are not updated by this new step. This patch also builds upon the removal of lambda queries for use in loader strategies in #6889. lambdas made this issue much more difficult to diagnose. An attempt to reintroduce lambdas here after finally identifying the "extra_criteria" issue above showed that lambdas still impact the assertsql fixture, meaning we have a statement structure that upon calling .compile() still delivers stale data due to lambdas, even if caching is turned off, and the non-cached test was still failing due to stale data within the lambdas. This is basically the complexity that #6889 fixes and as there's no real performance gain to using lambdas in these strategies on top of the existing statement caching that does most of the work, it should be much less likely going forward to have as many deeply confusing issues as we've had within selectinload/lazyload in the 1.4 series. Fixes: #6881 Change-Id: I919c079d2ed06125def5f8d6d81f3f305e158c04 --- lib/sqlalchemy/testing/assertsql.py | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) (limited to 'lib/sqlalchemy/testing') diff --git a/lib/sqlalchemy/testing/assertsql.py b/lib/sqlalchemy/testing/assertsql.py index 2d41d6dab..4ee4c5844 100644 --- a/lib/sqlalchemy/testing/assertsql.py +++ b/lib/sqlalchemy/testing/assertsql.py @@ -96,6 +96,15 @@ class CompiledSQL(SQLMatchRule): context = execute_observed.context compare_dialect = self._compile_dialect(execute_observed) + # received_statement runs a full compile(). we should not need to + # consider extracted_parameters; if we do this indicates some state + # is being sent from a previous cached query, which some misbehaviors + # in the ORM can cause, see #6881 + cache_key = None # execute_observed.context.compiled.cache_key + extracted_parameters = ( + None # execute_observed.context.extracted_parameters + ) + if "schema_translate_map" in context.execution_options: map_ = context.execution_options["schema_translate_map"] else: @@ -104,10 +113,12 @@ class CompiledSQL(SQLMatchRule): if isinstance(execute_observed.clauseelement, _DDLCompiles): compiled = execute_observed.clauseelement.compile( - dialect=compare_dialect, schema_translate_map=map_ + dialect=compare_dialect, + schema_translate_map=map_, ) else: compiled = execute_observed.clauseelement.compile( + cache_key=cache_key, dialect=compare_dialect, column_keys=context.compiled.column_keys, for_executemany=context.compiled.for_executemany, @@ -117,10 +128,17 @@ class CompiledSQL(SQLMatchRule): parameters = execute_observed.parameters if not parameters: - _received_parameters = [compiled.construct_params()] + _received_parameters = [ + compiled.construct_params( + extracted_parameters=extracted_parameters + ) + ] else: _received_parameters = [ - compiled.construct_params(m) for m in parameters + compiled.construct_params( + m, extracted_parameters=extracted_parameters + ) + for m in parameters ] return _received_statement, _received_parameters -- cgit v1.2.1