Skip to content

Commit b646c1e

Browse files
zzzeekGerrit Code Review
zzzeek
authored and
Gerrit Code Review
committed
Merge "Use explicit names for mapper _get_clause parameters"
2 parents 266ffe9 + 5f9c454 commit b646c1e

19 files changed

Lines changed: 216 additions & 119 deletions

‎doc/build/changelog/unreleased_14/6055.rst‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
.. change::
2+
:tags: bug, orm, regression
3+
:tickets: 6055
4+
5+
Fixed a critical regression in the relationship lazy loader where the SQL
6+
criteria used to fetch a related many-to-one object could go stale in
7+
relation to other memoized structures within the loader if the mapper had
8+
configuration changes, such as can occur when mappers are late configured
9+
or configured on demand, producing a comparison to None and returning no
10+
object. Huge thanks to Alan Hamlett for their help tracking this down late
11+
into the night.
12+
13+

‎lib/sqlalchemy/orm/loading.py‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -436,6 +436,14 @@ def load_on_pk_identity(
436436
lambda q: q.where(
437437
sql_util._deep_annotate(_get_clause, {"_orm_adapt": True})
438438
),
439+
# this track_on will allow the lambda to refresh if
440+
# _get_clause goes stale due to reconfigured mapper.
441+
# however, it's not needed as the lambda otherwise tracks
442+
# on the SQL cache key of the expression. the main thing
443+
# is that the bindparam.key stays the same if the cache key
444+
# stays the same, as we are referring to the .key explicitly
445+
# in the params.
446+
# track_on=[id(_get_clause)]
439447
)
440448
else:
441449
q._where_criteria = (

‎lib/sqlalchemy/orm/mapper.py‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2569,8 +2569,11 @@ def _get_clause(self):
25692569
25702570
"""
25712571
params = [
2572-
(primary_key, sql.bindparam(None, type_=primary_key.type))
2573-
for primary_key in self.primary_key
2572+
(
2573+
primary_key,
2574+
sql.bindparam("pk_%d" % idx, type_=primary_key.type),
2575+
)
2576+
for idx, primary_key in enumerate(self.primary_key, 1)
25742577
]
25752578
return (
25762579
sql.and_(*[k == v for (k, v) in params]),

‎lib/sqlalchemy/orm/strategies.py‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -672,6 +672,7 @@ def __init__(self, parent, strategy_key):
672672
and self.entity._get_clause[0].compare(
673673
self._lazywhere,
674674
use_proxies=True,
675+
compare_keys=False,
675676
equivalents=self.mapper._equivalent_columns,
676677
)
677678
)
@@ -2535,6 +2536,7 @@ def __init__(self, parent, strategy_key):
25352536
self.omit_join = self.parent._get_clause[0].compare(
25362537
lazyloader._rev_lazywhere,
25372538
use_proxies=True,
2539+
compare_keys=False,
25382540
equivalents=self.parent._equivalent_columns,
25392541
)
25402542

‎lib/sqlalchemy/sql/elements.py‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1400,14 +1400,14 @@ def __init__(
14001400
else:
14011401
self.type = type_
14021402

1403-
def _with_value(self, value, maintain_key=False):
1403+
def _with_value(self, value, maintain_key=False, required=NO_ARG):
14041404
"""Return a copy of this :class:`.BindParameter` with the given value
14051405
set.
14061406
"""
14071407
cloned = self._clone(maintain_key=maintain_key)
14081408
cloned.value = value
14091409
cloned.callable = None
1410-
cloned.required = False
1410+
cloned.required = required if required is not NO_ARG else self.required
14111411
if cloned.type is type_api.NULLTYPE:
14121412
cloned.type = type_api._resolve_value_to_type(value)
14131413
return cloned
@@ -1826,7 +1826,7 @@ def bindparams(self, *binds, **names_to_values):
18261826
replace_context=err,
18271827
)
18281828
else:
1829-
new_params[key] = existing._with_value(value)
1829+
new_params[key] = existing._with_value(value, required=False)
18301830

18311831
@util.preload_module("sqlalchemy.sql.selectable")
18321832
def columns(self, *cols, **types):

‎lib/sqlalchemy/sql/lambdas.py‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1170,7 +1170,9 @@ def __clause_element__(self):
11701170
to_evaluate = object.__getattribute__(self, "_to_evaluate")
11711171
if param is None:
11721172
name = object.__getattribute__(self, "_name")
1173-
self._param = param = elements.BindParameter(name, unique=True)
1173+
self._param = param = elements.BindParameter(
1174+
name, required=False, unique=True
1175+
)
11741176
self._has_param = True
11751177
param.type = type_api._resolve_value_to_type(to_evaluate)
11761178
return param._with_value(to_evaluate, maintain_key=True)

‎lib/sqlalchemy/sql/traversals.py‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1370,12 +1370,19 @@ def compare_binary(self, left, right, **kw):
13701370
return COMPARE_FAILED
13711371

13721372
def compare_bindparam(self, left, right, **kw):
1373+
compare_keys = kw.pop("compare_keys", True)
13731374
compare_values = kw.pop("compare_values", True)
1375+
13741376
if compare_values:
1375-
return []
1377+
omit = []
13761378
else:
13771379
# this means, "skip these, we already compared"
1378-
return ["callable", "value"]
1380+
omit = ["callable", "value"]
1381+
1382+
if not compare_keys:
1383+
omit.append("key")
1384+
1385+
return omit
13791386

13801387

13811388
class ColIdentityComparatorStrategy(TraversalComparatorStrategy):

‎test/ext/test_baked.py‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -348,7 +348,18 @@ def test_get_includes_getclause(self):
348348
u1 = bq(sess).get(7)
349349
eq_(u1.name, "jack")
350350
sess.close()
351-
eq_(len(bq._bakery), 4)
351+
352+
# this went from 4 to 3 as a result of #6055. by giving a name
353+
# to the bind param in mapper._get_clause, while the baked cache
354+
# here grows by one element, the SQL compiled_cache no longer
355+
# changes because the keys of the bindparam() objects are passed
356+
# explicitly as params to the execute() call as a result of
357+
# _load_on_pk_identity() (either the one in baked or the one in
358+
# loading.py), which then puts them
359+
# in column_keys which makes them part of the cache key. These
360+
# were previously anon names, now they are explicit so they
361+
# stay across resets
362+
eq_(len(bq._bakery), 3)
352363

353364

354365
class ResultPostCriteriaTest(BakedTest):

‎test/orm/inheritance/test_basic.py‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1616,8 +1616,8 @@ def test_c_only(self):
16161616
asserter.assert_(
16171617
CompiledSQL(
16181618
"SELECT a.id AS a_id, a.type AS a_type "
1619-
"FROM a WHERE a.id = :param_1",
1620-
[{"param_1": 1}],
1619+
"FROM a WHERE a.id = :pk_1",
1620+
[{"pk_1": 1}],
16211621
),
16221622
CompiledSQL("DELETE FROM a WHERE a.id = :id", [{"id": 1}]),
16231623
)
@@ -1658,8 +1658,8 @@ def test_b_only(self):
16581658
asserter.assert_(
16591659
CompiledSQL(
16601660
"SELECT a.id AS a_id, a.type AS a_type "
1661-
"FROM a WHERE a.id = :param_1",
1662-
[{"param_1": 1}],
1661+
"FROM a WHERE a.id = :pk_1",
1662+
[{"pk_1": 1}],
16631663
),
16641664
CompiledSQL("DELETE FROM a WHERE a.id = :id", [{"id": 1}]),
16651665
)
@@ -1696,8 +1696,8 @@ def test_a_only(self):
16961696
asserter.assert_(
16971697
CompiledSQL(
16981698
"SELECT a.id AS a_id, a.type AS a_type "
1699-
"FROM a WHERE a.id = :param_1",
1700-
[{"param_1": 1}],
1699+
"FROM a WHERE a.id = :pk_1",
1700+
[{"pk_1": 1}],
17011701
),
17021702
CompiledSQL("DELETE FROM a WHERE a.id = :id", [{"id": 1}]),
17031703
)
@@ -2549,8 +2549,8 @@ def go():
25492549
"base.data AS base_data, base.type AS base_type, "
25502550
"sub.sub AS sub_sub, sub.subcounter2 AS sub_subcounter2 "
25512551
"FROM base LEFT OUTER JOIN sub ON base.id = sub.id "
2552-
"WHERE base.id = :param_1",
2553-
{"param_1": sjb_id},
2552+
"WHERE base.id = :pk_1",
2553+
{"pk_1": sjb_id},
25542554
),
25552555
)
25562556

@@ -2804,8 +2804,8 @@ def go():
28042804
"SELECT base.counter AS base_counter, "
28052805
"sub.subcounter AS sub_subcounter, "
28062806
"sub.subcounter2 AS sub_subcounter2 FROM base JOIN sub "
2807-
"ON base.id = sub.id WHERE base.id = :param_1",
2808-
lambda ctx: {"param_1": s1.id},
2807+
"ON base.id = sub.id WHERE base.id = :pk_1",
2808+
lambda ctx: {"pk_1": s1.id},
28092809
),
28102810
)
28112811

‎test/orm/test_defaults.py‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -311,13 +311,13 @@ def test_insert_computed(self, eager):
311311
),
312312
CompiledSQL(
313313
"SELECT test.bar AS test_bar FROM test "
314-
"WHERE test.id = :param_1",
315-
[{"param_1": 1}],
314+
"WHERE test.id = :pk_1",
315+
[{"pk_1": 1}],
316316
),
317317
CompiledSQL(
318318
"SELECT test.bar AS test_bar FROM test "
319-
"WHERE test.id = :param_1",
320-
[{"param_1": 2}],
319+
"WHERE test.id = :pk_1",
320+
[{"pk_1": 2}],
321321
),
322322
],
323323
)
@@ -385,13 +385,13 @@ def test_update_computed(self, eager):
385385
),
386386
CompiledSQL(
387387
"SELECT test.bar AS test_bar FROM test "
388-
"WHERE test.id = :param_1",
389-
[{"param_1": 1}],
388+
"WHERE test.id = :pk_1",
389+
[{"pk_1": 1}],
390390
),
391391
CompiledSQL(
392392
"SELECT test.bar AS test_bar FROM test "
393-
"WHERE test.id = :param_1",
394-
[{"param_1": 2}],
393+
"WHERE test.id = :pk_1",
394+
[{"pk_1": 2}],
395395
),
396396
)
397397
else:
@@ -402,13 +402,13 @@ def test_update_computed(self, eager):
402402
),
403403
CompiledSQL(
404404
"SELECT test.bar AS test_bar FROM test "
405-
"WHERE test.id = :param_1",
406-
[{"param_1": 1}],
405+
"WHERE test.id = :pk_1",
406+
[{"pk_1": 1}],
407407
),
408408
CompiledSQL(
409409
"SELECT test.bar AS test_bar FROM test "
410-
"WHERE test.id = :param_1",
411-
[{"param_1": 2}],
410+
"WHERE test.id = :pk_1",
411+
[{"pk_1": 2}],
412412
),
413413
)
414414

0 commit comments

Comments
 (0)