diff options
| -rw-r--r-- | doc/build/changelog/unreleased_14/6881.rst | 9 | ||||
| -rw-r--r-- | lib/sqlalchemy/orm/interfaces.py | 2 | ||||
| -rw-r--r-- | lib/sqlalchemy/orm/strategies.py | 42 | ||||
| -rw-r--r-- | lib/sqlalchemy/orm/strategy_options.py | 97 | ||||
| -rw-r--r-- | lib/sqlalchemy/testing/assertsql.py | 24 | ||||
| -rw-r--r-- | test/orm/test_eager_relations.py | 1 | ||||
| -rw-r--r-- | test/orm/test_relationship_criteria.py | 124 |
7 files changed, 268 insertions, 31 deletions
diff --git a/doc/build/changelog/unreleased_14/6881.rst b/doc/build/changelog/unreleased_14/6881.rst new file mode 100644 index 000000000..aab17e396 --- /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 f17cc6159..4826354dc 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 955cd6dd2..bf60c803d 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 ea1e5ea2a..b7ed4e89b 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 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 diff --git a/test/orm/test_eager_relations.py b/test/orm/test_eager_relations.py index 8a4116083..41c715efa 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 f9b4335df..8cf02adac 100644 --- a/test/orm/test_relationship_criteria.py +++ b/test/orm/test_relationship_criteria.py @@ -73,6 +73,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 |