refactor(proxy): use on-path tracking in resolve_nested_groups

Previously the visited set tracked every node ever seen during a top-level
resolution. That meant DAG-shared subtrees (A -> [B, C], B -> D, C -> D)
triggered a spurious 'cycle detected' warning on D's second visit.

Switch to on-path tracking: discard the node from visited when its frame
unwinds. Real cycles still fire the warning; DAG re-traversal is silent.
Caller already dedups the final list.

Adds a regression test (test_dag_shared_group_subtree_no_false_cycle_warning)
that asserts no warning is logged for the legitimate DAG case.

Refs #28032
This commit is contained in:
Ashwin Upadhyay 2026-05-17 00:04:26 +05:30
parent 88dba5415c
commit e0b843eb1f
2 changed files with 49 additions and 13 deletions

View file

@ -52,8 +52,14 @@ def resolve_nested_groups(
Expand a group name to the full list of model names it transitively includes,
following parent -> child edges in `group_memberships`.
Runs on the auth path, so cyclic edges are logged and skipped rather than raised:
a malformed row must not 500 the proxy.
Cycle handling: `visited` tracks the *current recursion path* (on-path),
not every node ever seen. A revisit while still on the path is a real
cycle - we log a warning and skip the cyclic edge. DAG-shared subtrees
(e.g. A -> [B, C], B -> [D], C -> [D]) re-traverse D, and the caller
deduplicates the final list.
Cyclic edges are logged and skipped rather than raised: this runs on the
auth path and a malformed row must not 500 the proxy.
"""
if group_name in visited:
verbose_proxy_logger.warning(
@ -62,18 +68,20 @@ def resolve_nested_groups(
)
return []
visited.add(group_name)
resolved: List[str] = list(model_access_groups.get(group_name, []))
for child in group_memberships.get(group_name, []):
resolved.extend(
resolve_nested_groups(
group_name=child,
model_access_groups=model_access_groups,
group_memberships=group_memberships,
visited=visited,
try:
resolved: List[str] = list(model_access_groups.get(group_name, []))
for child in group_memberships.get(group_name, []):
resolved.extend(
resolve_nested_groups(
group_name=child,
model_access_groups=model_access_groups,
group_memberships=group_memberships,
visited=visited,
)
)
)
return resolved
return resolved
finally:
visited.discard(group_name)
def _get_models_from_access_groups(

View file

@ -244,6 +244,34 @@ def test_validate_models_exist_backwards_compatible_without_groups():
# ---------------------------------------------------------------------------
def test_dag_shared_group_subtree_no_false_cycle_warning(caplog):
"""
DAG with a shared group subtree:
A -> [B, C]
B -> [D]
C -> [D]
D -> [model-1]
On-path cycle tracking lets D be re-traversed via both B and C without
a spurious 'cycle detected' warning. Caller dedups.
"""
model_access_groups = {"D": ["model-1"]}
group_memberships = {"A": ["B", "C"], "B": ["D"], "C": ["D"]}
with caplog.at_level("WARNING"):
result = resolve_nested_groups(
group_name="A",
model_access_groups=model_access_groups,
group_memberships=group_memberships,
visited=set(),
)
assert sorted(set(result)) == ["model-1"]
assert not any(
"cycle detected" in m.lower() for m in caplog.messages
), "DAG-shared subtree must not trigger cycle warnings"
def test_diamond_inheritance_resolves_once():
"""
Diamond shape: