Skip to content

Commit dd84e04

Browse files
fix(workflows): evaluate parenthesised expressions
The operator scans in _evaluate_simple_expression skip over bracketed text, so an operator inside a nested operand is never split on. Nothing then unwrapped a group spanning the whole expression: `(a or b) and c` split at the top-level `and`, evaluated `(a or b)` as a dot path, found no such key, and got None. The `or` was never evaluated and the expression read false. So adding parentheses to make precedence explicit — the usual reason to add them — silently inverted the result: `inputs.a or inputs.b and inputs.c` was true while `(inputs.a or inputs.b) and inputs.c` was false. A step gated on such a condition is skipped with nothing reported; the malformed-condition validators do not flag it, because the syntax is valid. Unwrap a group that spans the whole expression, quote-aware and only when the opening paren closes at the very end, so `(a) and (b)` and a literal paren inside a string are untouched.
1 parent 8d3f64c commit dd84e04

2 files changed

Lines changed: 62 additions & 0 deletions

File tree

‎src/specify_cli/workflows/expressions.py‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -552,6 +552,31 @@ def _apply_filter(value: Any, filter_expr: str, namespace: dict[str, Any]) -> An
552552
_leaf_sink: ContextVar[list[str] | None] = ContextVar("_leaf_sink", default=None)
553553

554554

555+
def _is_wrapped_in_parens(text: str) -> bool:
556+
"""True when *text* is one parenthesised group, brackets and all.
557+
558+
``(a or b)`` is; ``(a) and (b)`` is not, because the opening paren closes
559+
before the end. Quote-aware, so ``('(')`` does not count its own literal.
560+
"""
561+
if not (text.startswith("(") and text.endswith(")")):
562+
return False
563+
quote: str | None = None
564+
depth = 0
565+
for index, ch in enumerate(text):
566+
if quote is not None:
567+
if ch == quote:
568+
quote = None
569+
elif ch in ("'", '"'):
570+
quote = ch
571+
elif ch == "(":
572+
depth += 1
573+
elif ch == ")":
574+
depth -= 1
575+
if depth == 0:
576+
return index == len(text) - 1
577+
return False
578+
579+
555580
def _evaluate_simple_expression(expr: str, namespace: dict[str, Any]) -> Any:
556581
"""Evaluate a simple expression against the namespace.
557582
@@ -573,6 +598,16 @@ def _evaluate_simple_expression(expr: str, namespace: dict[str, Any]) -> Any:
573598
if expr[:1] in ("'", '"') and expr.find(expr[0], 1) == len(expr) - 1:
574599
return expr[1:-1]
575600

601+
# A parenthesised group. The operator scans below deliberately skip over
602+
# bracketed text so an operator inside a quoted or nested operand is not
603+
# split on -- which also means nothing ever looked inside a group that
604+
# wraps the WHOLE expression. `(a or b) and c` split at the top-level
605+
# `and`, then evaluated `(a or b)` as a dot path, found no such key, and
606+
# returned None: the `or` was never evaluated and the whole thing read
607+
# false. Unwrap here so grouping means what it says.
608+
if _is_wrapped_in_parens(expr):
609+
return _evaluate_simple_expression(expr[1:-1], namespace)
610+
576611
# Handle pipe filters. Detect the pipe at the top level only, so a literal
577612
# '|' inside a quoted operand (e.g. `inputs.x == 'a|b'`) or nested brackets is
578613
# not mistaken for a filter separator — mirroring the operator parsing below.

‎tests/test_workflows.py‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1003,6 +1003,33 @@ def test_boolean_literal(self):
10031003
assert evaluate_expression("{{ true }}", ctx) is True
10041004
assert evaluate_expression("{{ false }}", ctx) is False
10051005

1006+
def test_parenthesised_grouping(self):
1007+
"""A parenthesised group is evaluated, not read as a dot path.
1008+
1009+
The operator scans skip bracketed text so an operator inside an
1010+
operand is not split on. Nothing unwrapped a group spanning the whole
1011+
expression, so ``(a or b) and c`` split at the top-level ``and`` and
1012+
then looked up ``(a or b)`` as a key, got ``None``, and read false --
1013+
adding parentheses to make precedence explicit silently inverted the
1014+
result.
1015+
"""
1016+
from specify_cli.workflows.expressions import evaluate_expression
1017+
from specify_cli.workflows.base import StepContext
1018+
1019+
ctx = StepContext(inputs={"a": True, "b": False, "c": True, "n": 5})
1020+
1021+
assert evaluate_expression("{{ (inputs.a or inputs.b) and inputs.c }}", ctx) is True
1022+
assert evaluate_expression("{{ (inputs.b or inputs.b) and inputs.c }}", ctx) is False
1023+
assert evaluate_expression("{{ (inputs.n) }}", ctx) == 5
1024+
assert evaluate_expression("{{ (inputs.n > 1) }}", ctx) is True
1025+
assert evaluate_expression("{{ ((inputs.n)) }}", ctx) == 5
1026+
# A group is still only unwrapped when it spans the whole expression.
1027+
assert evaluate_expression("{{ (inputs.a) and (inputs.b) }}", ctx) is False
1028+
assert evaluate_expression("{{ (inputs.n) | default(9) }}", ctx) == 5
1029+
# A parenthesis inside a string literal is not a group.
1030+
assert evaluate_expression("{{ 'a(b' }}", ctx) == "a(b"
1031+
assert evaluate_expression("{{ ('(') }}", ctx) == "("
1032+
10061033
def test_list_indexing(self):
10071034
from specify_cli.workflows.expressions import evaluate_expression
10081035
from specify_cli.workflows.base import StepContext

0 commit comments

Comments
 (0)