diff --git a/core/wren/src/wren/policy.py b/core/wren/src/wren/policy.py index 4bf4158f70..d02e1a6f4a 100644 --- a/core/wren/src/wren/policy.py +++ b/core/wren/src/wren/policy.py @@ -161,6 +161,14 @@ def resolve_model_name( case-sensitively; an unquoted identifier prefers an exact case match but falls back to a case-insensitive scan. Returns ``None`` if no model matches. + + When an unquoted reference matches more than one model case-insensitively + (e.g. both ``Users`` and ``users`` are defined), the one picked is the + lexicographically smallest rather than whichever ``model_names`` happens + to iterate first: for a ``set`` of strings that iteration order is not + the insertion order, it depends on string hashing and varies across + processes under ``PYTHONHASHSEED``, so the same query could silently + bind a different model on every run. """ model_set = ( model_names if isinstance(model_names, (set, frozenset)) else set(model_names) @@ -170,10 +178,14 @@ def resolve_model_name( if quoted: return None name_lower = name.lower() + # Single pass, no intermediate list: a tie only needs `<` against the + # best candidate seen so far, same cost as the exact-match scan this + # replaces. + best: str | None = None for candidate in model_set: - if candidate.lower() == name_lower: - return candidate - return None + if candidate.lower() == name_lower and (best is None or candidate < best): + best = candidate + return best # Statement shapes that are read-only queries. ``SetOperation`` is the base class diff --git a/core/wren/tests/unit/test_policy.py b/core/wren/tests/unit/test_policy.py index 54cb2e9215..3fc9d59233 100644 --- a/core/wren/tests/unit/test_policy.py +++ b/core/wren/tests/unit/test_policy.py @@ -5,12 +5,16 @@ from __future__ import annotations +import os +import subprocess +import sys + import pytest from sqlglot import exp, parse_one from wren.config import WrenConfig from wren.model.error import ErrorCode, WrenError -from wren.policy import validate_planned_sql, validate_sql_policy +from wren.policy import resolve_model_name, validate_planned_sql, validate_sql_policy pytestmark = pytest.mark.unit @@ -669,3 +673,53 @@ def test_planned_sql_fails_open_when_unparseable(): open where the input direction fails closed. """ validate_planned_sql("SELCT a FRM orders", "postgres") + + +# ── resolve_model_name, case-insensitive fallback ────────────────────────── + + +def test_resolve_model_name_exact_match_wins(): + assert resolve_model_name("orders", False, {"orders", "Orders"}) == "orders" + + +def test_resolve_model_name_quoted_is_case_sensitive(): + assert resolve_model_name("Orders", True, {"orders"}) is None + + +def test_resolve_model_name_unquoted_case_insensitive_fallback(): + assert resolve_model_name("ORDERS", False, {"orders"}) == "orders" + + +def test_resolve_model_name_no_match_returns_none(): + assert resolve_model_name("missing", False, {"orders"}) is None + + +def test_resolve_model_name_case_collision_is_deterministic(): + """When two model names differ only in case, an unquoted reference that + matches neither exactly must resolve the same way every time. + + Before the fix this depended on ``set`` iteration order, which varies by + process under ``PYTHONHASHSEED``, not within one: a fixed hash seed makes + iteration order stable for the lifetime of a process, so a same-process + loop passes on the buggy code too, every time. The bug only shows up + across fresh interpreters, so the regression spawns one per + ``PYTHONHASHSEED`` instead, matching how this was reproduced originally + (8 fresh subprocesses split 6/2 between ``'Orders'`` and ``'orders'``). + Sorted order is an arbitrary tie-break (the two names are equally + "right", this pair may legitimately be two distinct models, see + test_case_sensitivity.py); the point is only that it no longer varies. + """ + script = ( + "from wren.policy import resolve_model_name; " + "print(resolve_model_name('ORDERS', False, {'Orders', 'orders'}))" + ) + for seed in range(8): + env = {**os.environ, "PYTHONHASHSEED": str(seed)} + result = subprocess.run( + [sys.executable, "-c", script], + env=env, + capture_output=True, + text=True, + check=True, + ) + assert result.stdout.strip() == "Orders"