From: Mike Bayer Date: Tue, 21 Dec 2021 23:08:33 +0000 (-0500) Subject: accommodate for "clone" of ColumnClause X-Git-Tag: rel_2_0_0b1~590 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=267e9cbf6e3c165a4e953b49d979d7f4ddc533f9;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git accommodate for "clone" of ColumnClause for use with the ClauseElement.params() method, altered ColumnClause._clone() so that while the element stays immutable, if the column is associated with a subquery, it returns a new version of itself as corresponding to a clone of the subquery. this allows processing functions to access the parameters in the subquery and produce a copy of it. The use case here is the expanded use of .params() within loader strategies that use HasCacheKey._apply_params_to_element(). Fixed issue in new "loader criteria" method :meth:`_orm.PropComparator.and_` where usage with a loader strategy like :func:`_orm.selectinload` against a column that was a member of the ``.c.`` collection of a subquery object, where the subquery would be dynamically added to the FROM clause of the statement, would be subject to stale parameter values within the subquery in the SQL statement cache, as the process used by the loader strategy to replace the parameters at execution time would fail to accommodate the subquery when received in this form. Fixes: #7489 Change-Id: Ibb3b6af140b8a62a2c8d05b2ac92e86ca3013c46 --- diff --git a/doc/build/changelog/unreleased_14/7489.rst b/doc/build/changelog/unreleased_14/7489.rst new file mode 100644 index 0000000000..4af33f4298 --- /dev/null +++ b/doc/build/changelog/unreleased_14/7489.rst @@ -0,0 +1,13 @@ +.. change:: + :tags: bug, orm + :tickets: 7489 + + Fixed issue in new "loader criteria" method + :meth:`_orm.PropComparator.and_` where usage with a loader strategy like + :func:`_orm.selectinload` against a column that was a member of the ``.c.`` + collection of a subquery object, where the subquery would be dynamically + added to the FROM clause of the statement, would be subject to stale + parameter values within the subquery in the SQL statement cache, as the + process used by the loader strategy to replace the parameters at execution + time would fail to accommodate the subquery when received in this form. + diff --git a/lib/sqlalchemy/sql/elements.py b/lib/sqlalchemy/sql/elements.py index 00270c9b58..75798502a7 100644 --- a/lib/sqlalchemy/sql/elements.py +++ b/lib/sqlalchemy/sql/elements.py @@ -439,6 +439,7 @@ class ClauseElement( return self._replace_params(False, optionaldict, kwargs) def _replace_params(self, unique, optionaldict, kwargs): + if len(optionaldict) == 1: kwargs.update(optionaldict[0]) elif len(optionaldict) > 1: @@ -454,7 +455,9 @@ class ClauseElement( bind._convert_to_unique() return cloned_traverse( - self, {"maintain_key": True}, {"bindparam": visit_bindparam} + self, + {"maintain_key": True, "detect_subquery_cols": True}, + {"bindparam": visit_bindparam}, ) def compare(self, other, **kw): @@ -4869,6 +4872,19 @@ class ColumnClause( else: return super(ColumnClause, self).entity_namespace + def _clone(self, detect_subquery_cols=False, **kw): + if ( + detect_subquery_cols + and self.table is not None + and self.table._is_subquery + ): + clone = kw.pop("clone") + table = clone(self.table, **kw) + new = table.c.corresponding_column(self) + return new + + return super(ColumnClause, self)._clone(**kw) + @HasMemoized.memoized_attribute def _from_objects(self): t = self.table diff --git a/lib/sqlalchemy/sql/traversals.py b/lib/sqlalchemy/sql/traversals.py index d58b5c2bb3..12a5486d22 100644 --- a/lib/sqlalchemy/sql/traversals.py +++ b/lib/sqlalchemy/sql/traversals.py @@ -271,7 +271,6 @@ class HasCacheKey: result += meth( attrname, obj, self, anon_map, bindparams ) - return result def _generate_cache_key(self): diff --git a/lib/sqlalchemy/sql/visitors.py b/lib/sqlalchemy/sql/visitors.py index b080807535..30103bc8ef 100644 --- a/lib/sqlalchemy/sql/visitors.py +++ b/lib/sqlalchemy/sql/visitors.py @@ -774,7 +774,7 @@ def cloned_traverse(obj, opts, visitors): cloned[id(elem)] = newelem return newelem - cloned[id(elem)] = newelem = elem._clone(**kw) + cloned[id(elem)] = newelem = elem._clone(clone=clone, **kw) newelem._copy_internals(clone=clone, **kw) meth = visitors.get(newelem.__visit_name__, None) if meth: diff --git a/test/orm/test_relationship_criteria.py b/test/orm/test_relationship_criteria.py index b2c16da057..b5c9be0382 100644 --- a/test/orm/test_relationship_criteria.py +++ b/test/orm/test_relationship_criteria.py @@ -1240,6 +1240,66 @@ class RelationshipCriteriaTest(_Fixtures, testing.AssertsCompiledSQL): ), ) + def test_selectinload_local_criteria_subquery(self, user_address_fixture): + """test #7489""" + User, Address = user_address_fixture + + s = Session(testing.db, future=True) + + def go(value): + a1 = aliased(Address) + subq = select(a1.id).where(a1.email_address != value).subquery() + stmt = ( + select(User) + .options( + selectinload(User.addresses.and_(Address.id == subq.c.id)), + ) + .order_by(User.id) + ) + result = s.execute(stmt) + return result + + for value in ( + "ed@wood.com", + "ed@lala.com", + "ed@wood.com", + "ed@lala.com", + ): + s.close() + with self.sql_execution_asserter() as asserter: + result = go(value) + + eq_( + result.scalars().unique().all(), + self._user_minus_edwood(*user_address_fixture) + if value == "ed@wood.com" + else self._user_minus_edlala(*user_address_fixture), + ) + + asserter.assert_( + CompiledSQL( + "SELECT users.id, users.name FROM users ORDER BY users.id" + ), + CompiledSQL( + "SELECT addresses.user_id AS addresses_user_id, " + "addresses.id AS addresses_id, " + "addresses.email_address AS addresses_email_address " + # note the comma-separated FROM clause + "FROM addresses, (SELECT addresses_1.id AS id FROM " + "addresses AS addresses_1 " + "WHERE addresses_1.email_address != :email_address_1) " + "AS anon_1 WHERE addresses.user_id " + "IN (__[POSTCOMPILE_primary_keys]) " + "AND addresses.id = anon_1.id ORDER BY addresses.id", + [ + { + "primary_keys": [7, 8, 9, 10], + "email_address_1": value, + } + ], + ), + ) + @testing.combinations((True,), (False,), argnames="use_compiled_cache") def test_selectinload_nested_criteria( self, user_order_item_fixture, use_compiled_cache diff --git a/test/sql/test_external_traversal.py b/test/sql/test_external_traversal.py index 0ac73fc165..ace618e501 100644 --- a/test/sql/test_external_traversal.py +++ b/test/sql/test_external_traversal.py @@ -827,6 +827,48 @@ class ClauseTest(fixtures.TestBase, AssertsCompiledSQL): sel._generate_cache_key()[1], ) + def test_params_on_expr_against_subquery(self): + """test #7489""" + + meta = MetaData() + + b = Table("b", meta, Column("id", Integer), Column("data", String)) + + subq = select(b.c.id).where(b.c.data == "some data").subquery() + criteria = b.c.id == subq.c.id + + stmt = select(b).where(criteria) + param_key = stmt._generate_cache_key()[1][0].key + + self.assert_compile( + stmt, + "SELECT b.id, b.data FROM b, (SELECT b.id AS id " + "FROM b WHERE b.data = :data_1) AS anon_1 WHERE b.id = anon_1.id", + checkparams={"data_1": "some data"}, + ) + eq_( + [ + eq_clause_element(bindparam(param_key, value="some data")), + ], + stmt._generate_cache_key()[1], + ) + + stmt = select(b).where(criteria.params({param_key: "some other data"})) + self.assert_compile( + stmt, + "SELECT b.id, b.data FROM b, (SELECT b.id AS id " + "FROM b WHERE b.data = :data_1) AS anon_1 WHERE b.id = anon_1.id", + checkparams={"data_1": "some other data"}, + ) + eq_( + [ + eq_clause_element( + bindparam(param_key, value="some other data") + ), + ], + stmt._generate_cache_key()[1], + ) + def test_params_subqueries_in_joins_one(self): """test #7055"""