From a7899c44ba15076df8869f5b40d720ccf09d5273 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Fri, 13 Aug 2021 11:07:17 -0400 Subject: [PATCH] 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 --- doc/build/changelog/unreleased_14/6881.rst | 9 ++ lib/sqlalchemy/orm/interfaces.py | 2 + lib/sqlalchemy/orm/strategies.py | 42 ++++++- lib/sqlalchemy/orm/strategy_options.py | 97 +++++++++++++++- lib/sqlalchemy/testing/assertsql.py | 24 +++- test/orm/test_eager_relations.py | 1 + test/orm/test_relationship_criteria.py | 124 +++++++++++++++++---- 7 files changed, 268 insertions(+), 31 deletions(-) create mode 100644 doc/build/changelog/unreleased_14/6881.rst diff --git a/doc/build/changelog/unreleased_14/6881.rst b/doc/build/changelog/unreleased_14/6881.rst new file mode 100644 index 0000000000..aab17e3965 --- /dev/null +++ b/doc/build/changelog/unreleased_14/6881.rst @@ -0,0 +1,9 @@ +.. change:: + :tags: bug, orm + :tickets: 6881 + + 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. + diff --git a/lib/sqlalchemy/orm/interfaces.py b/lib/sqlalchemy/orm/interfaces.py index f17cc6159f..4826354dc2 100644 --- a/lib/sqlalchemy/orm/interfaces.py +++ b/lib/sqlalchemy/orm/interfaces.py @@ -740,6 +740,8 @@ class ORMOption(ExecutableOption): _is_criteria_option = False + _is_strategy_option = False + class LoaderOption(ORMOption): """Describe a loader modification to an ORM statement at compilation time. diff --git a/lib/sqlalchemy/orm/strategies.py b/lib/sqlalchemy/orm/strategies.py index 955cd6dd2e..bf60c803d2 100644 --- a/lib/sqlalchemy/orm/strategies.py +++ b/lib/sqlalchemy/orm/strategies.py @@ -1511,11 +1511,26 @@ class SubqueryLoader(PostLoader): effective_entity, loadopt, ): - opts = orig_query._with_options + + 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 + ] if loadopt and loadopt._extra_criteria: - opts += ( + new_options += ( orm_util.LoaderCriteriaOption( self.entity, loadopt._generate_extra_criteria(context), @@ -1525,7 +1540,7 @@ class SubqueryLoader(PostLoader): # propagate loader options etc. to the new query. # these will fire relative to subq_path. q = q._with_current_path(rewritten_path) - q = q.options(*opts) + q = q.options(*new_options) return q @@ -2916,17 +2931,32 @@ class SelectInLoader(PostLoader, util.MemoizedSlots): effective_path = path[self.parent_property] - options = orig_query._with_options + if orig_query is context.query: + options = new_options = orig_query._with_options + else: + options = orig_query._with_options + + # 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 + # the QueryableAttribute in the path + + new_options = [ + orig_opt._adjust_for_extra_criteria(context) + if orig_opt._is_strategy_option + else orig_opt + for orig_opt in options + ] if loadopt and loadopt._extra_criteria: - options += ( + new_options += ( orm_util.LoaderCriteriaOption( effective_entity, loadopt._generate_extra_criteria(context), ), ) - q = q.options(*options)._update_compile_options( + q = q.options(*new_options)._update_compile_options( {"_current_path": effective_path} ) diff --git a/lib/sqlalchemy/orm/strategy_options.py b/lib/sqlalchemy/orm/strategy_options.py index ea1e5ea2a8..b7ed4e89bb 100644 --- a/lib/sqlalchemy/orm/strategy_options.py +++ b/lib/sqlalchemy/orm/strategy_options.py @@ -77,6 +77,8 @@ class Load(Generative, LoaderOption): """ + _is_strategy_option = True + _cache_key_traversal = [ ("path", visitors.ExtendedInternalTraversal.dp_has_cache_key), ("strategy", visitors.ExtendedInternalTraversal.dp_plain_obj), @@ -115,7 +117,7 @@ class Load(Generative, LoaderOption): def _generate_extra_criteria(self, context): """Apply the current bound parameters in a QueryContext to the - "extra_criteria" stored with this Load object. + immediate "extra_criteria" stored with this Load object. Load objects are typically pulled from the cached version of the statement from a QueryContext. The statement currently being @@ -150,6 +152,69 @@ class Load(Generative, LoaderOption): return k2._apply_params_to_element(k1, and_(*self._extra_criteria)) + def _adjust_for_extra_criteria(self, context): + """Apply the current bound parameters in a QueryContext to all + occurrences "extra_criteria" stored within al this Load object; + copying in place. + + """ + orig_query = context.compile_state.select_statement + + applied = {} + + ck = [None, None] + + def process(opt): + if not opt._extra_criteria: + return + + if ck[0] is None: + ck[:] = ( + orig_query._generate_cache_key(), + context.query._generate_cache_key(), + ) + k1, k2 = ck + + opt._extra_criteria = tuple( + k2._apply_params_to_element(k1, crit) + for crit in opt._extra_criteria + ) + + return self._deep_clone(applied, process) + + def _deep_clone(self, applied, process): + if self in applied: + return applied[self] + + cloned = self._generate() + + applied[self] = cloned + + cloned.strategy = self.strategy + + assert cloned.propagate_to_loaders == self.propagate_to_loaders + assert cloned.is_class_strategy == self.is_class_strategy + assert cloned.is_opts_only == self.is_opts_only + + if self.context: + cloned.context = util.OrderedDict( + [ + ( + key, + value._deep_clone(applied, process) + if isinstance(value, Load) + else value, + ) + for key, value in self.context.items() + ] + ) + + cloned.local_opts.update(self.local_opts) + + process(cloned) + + return cloned + @property def _context_cache_key(self): serialized = [] @@ -345,7 +410,10 @@ class Load(Generative, LoaderOption): else: return None - if attr._extra_criteria: + if attr._extra_criteria and not self._extra_criteria: + # in most cases, the process that brings us here will have + # already established _extra_criteria. however if not, + # and it's present on the attribute, then use that. self._extra_criteria = attr._extra_criteria if getattr(attr, "_of_type", None): @@ -708,6 +776,30 @@ class _UnboundLoad(Load): # anonymous clone of the Load / UnboundLoad object since #5056 self._to_bind = None + def _deep_clone(self, applied, process): + if self in applied: + return applied[self] + + cloned = self._generate() + + applied[self] = cloned + + cloned.strategy = self.strategy + + assert cloned.propagate_to_loaders == self.propagate_to_loaders + assert cloned.is_class_strategy == self.is_class_strategy + assert cloned.is_opts_only == self.is_opts_only + + cloned._to_bind = [ + elem._deep_clone(applied, process) for elem in self._to_bind or () + ] + + cloned.local_opts.update(self.local_opts) + + process(cloned) + + return cloned + def _apply_to_parent(self, parent, applied, bound, to_bind=None): if self in applied: return applied[self] @@ -984,6 +1076,7 @@ class _UnboundLoad(Load): loader.strategy = self.strategy loader.is_opts_only = self.is_opts_only loader.is_class_strategy = self.is_class_strategy + loader._extra_criteria = self._extra_criteria path = loader.path diff --git a/lib/sqlalchemy/testing/assertsql.py b/lib/sqlalchemy/testing/assertsql.py index 2d41d6dab8..4ee4c5844b 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 diff --git a/test/orm/test_eager_relations.py b/test/orm/test_eager_relations.py index 8a41160838..41c715efa3 100644 --- a/test/orm/test_eager_relations.py +++ b/test/orm/test_eager_relations.py @@ -6078,6 +6078,7 @@ class LazyLoadOptSpecificityTest(fixtures.DeclarativeMappedTest): def test_lazyload_plus_joined_aliased_abs_bcs(self): A, B, C = self.classes("A", "B", "C") + s = fixture_session() aa = aliased(A) q = ( diff --git a/test/orm/test_relationship_criteria.py b/test/orm/test_relationship_criteria.py index f9b4335df1..8cf02adaca 100644 --- a/test/orm/test_relationship_criteria.py +++ b/test/orm/test_relationship_criteria.py @@ -72,6 +72,32 @@ class _Fixtures(_fixtures.FixtureTest): return Order, Item + @testing.fixture + def user_order_item_fixture(self): + User, Order, Item = self.classes("User", "Order", "Item") + users, orders, items, order_items = self.tables( + "users", "orders", "items", "order_items" + ) + + mapper( + User, + users, + properties={"orders": relationship(Order, order_by=orders.c.id)}, + ) + mapper( + Order, + orders, + properties={ + # m2m + "items": relationship( + Item, secondary=order_items, order_by=items.c.id + ), + }, + ) + mapper(Item, items) + + return User, Order, Item + @testing.fixture def mixin_fixture(self): users = self.tables.users @@ -976,26 +1002,6 @@ class TemporalFixtureTest(testing.fixtures.DeclarativeMappedTest): class RelationshipCriteriaTest(_Fixtures, testing.AssertsCompiledSQL): __dialect__ = "default" - @testing.fixture - def user_address_fixture(self): - users, Address, addresses, User = ( - self.tables.users, - self.classes.Address, - self.tables.addresses, - self.classes.User, - ) - - mapper( - User, - users, - properties={ - "addresses": relationship( - mapper(Address, addresses), order_by=Address.id - ) - }, - ) - return User, Address - def _user_minus_edwood(self, User, Address): return [ User( @@ -1177,6 +1183,84 @@ class RelationshipCriteriaTest(_Fixtures, testing.AssertsCompiledSQL): ), ) + @testing.combinations((True,), (False,), argnames="use_compiled_cache") + def test_selectinload_nested_criteria( + self, user_order_item_fixture, use_compiled_cache + ): + User, Order, Item = user_order_item_fixture + + if not use_compiled_cache: + s = Session( + testing.db.execution_options(compiled_cache=None), future=True + ) + else: + s = Session(testing.db, future=True) + + def go(order_description, item_description): + stmt = ( + select(User) + .where(User.id == 7) + .options( + selectinload( + User.orders.and_( + Order.description == order_description + ) + ).joinedload( + Order.items.and_(Item.description == item_description) + ), + ) + ) + return s.execute(stmt) + + for order_description, item_description, oid, iid in ( + ("order 3", "item 3", 3, 3), + ("order 3", "item 4", 3, 4), + ("order 3", "item 4", 3, 4), + ("order 5", "item 5", 5, 5), + ("order 3", "item 3", 3, 3), + ("order 5", "item 5", 5, 5), + ): + s.close() + with self.sql_execution_asserter() as asserter: + result = go(order_description, item_description) + + eq_( + result.scalars().unique().all(), + [User(id=7, orders=[Order(id=oid, items=[Item(id=iid)])])], + ) + asserter.assert_( + CompiledSQL( + "SELECT users.id, users.name FROM users " + "WHERE users.id = :id_1", + [{"id_1": 7}], + ), + CompiledSQL( + "SELECT orders.user_id AS orders_user_id, " + "orders.id AS orders_id, " + "orders.address_id AS orders_address_id, " + "orders.description AS orders_description, " + "orders.isopen AS orders_isopen, " + "items_1.id AS items_1_id, " + "items_1.description AS items_1_description " + "FROM orders LEFT OUTER JOIN " + "(order_items AS order_items_1 " + "JOIN items AS items_1 " + "ON items_1.id = order_items_1.item_id " + "AND items_1.description = :description_1) " + "ON orders.id = order_items_1.order_id " + "WHERE orders.user_id IN ([POSTCOMPILE_primary_keys]) " + "AND orders.description = :description_2 " + "ORDER BY orders.id, items_1.id", + [ + { + "description_1": item_description, + "primary_keys": [7], + "description_2": order_description, + } + ], + ), + ) + def test_lazyload_local_criteria(self, user_address_fixture): User, Address = user_address_fixture -- 2.47.2