diff --git a/src/specify_cli/workflows/expressions.py b/src/specify_cli/workflows/expressions.py index 198010838e..5c7bb615d4 100644 --- a/src/specify_cli/workflows/expressions.py +++ b/src/specify_cli/workflows/expressions.py @@ -480,6 +480,31 @@ def _apply_filter(value: Any, filter_expr: str, namespace: dict[str, Any]) -> An _COMPARISON_OPERATORS = ("!=", "==", ">=", "<=", ">", "<", " not in ", " in ") +def _is_wrapped_in_parens(text: str) -> bool: + """True when *text* is one parenthesised group, brackets and all. + + ``(a or b)`` is; ``(a) and (b)`` is not, because the opening paren closes + before the end. Quote-aware, so ``('(')`` does not count its own literal. + """ + if not (text.startswith("(") and text.endswith(")")): + return False + quote: str | None = None + depth = 0 + for index, ch in enumerate(text): + if quote is not None: + if ch == quote: + quote = None + elif ch in ("'", '"'): + quote = ch + elif ch == "(": + depth += 1 + elif ch == ")": + depth -= 1 + if depth == 0: + return index == len(text) - 1 + return False + + def _evaluate_simple_expression(expr: str, namespace: dict[str, Any]) -> Any: """Evaluate a simple expression against the namespace. @@ -501,6 +526,16 @@ def _evaluate_simple_expression(expr: str, namespace: dict[str, Any]) -> Any: if expr[:1] in ("'", '"') and expr.find(expr[0], 1) == len(expr) - 1: return expr[1:-1] + # A parenthesised group. The operator scans below deliberately skip over + # bracketed text so an operator inside a quoted or nested operand is not + # split on -- which also means nothing ever looked inside a group that + # wraps the WHOLE expression. `(a or b) and c` split at the top-level + # `and`, then evaluated `(a or b)` as a dot path, found no such key, and + # returned None: the `or` was never evaluated and the whole thing read + # false. Unwrap here so grouping means what it says. + if _is_wrapped_in_parens(expr): + return _evaluate_simple_expression(expr[1:-1], namespace) + # Handle pipe filters. Detect the pipe at the top level only, so a literal # '|' inside a quoted operand (e.g. `inputs.x == 'a|b'`) or nested brackets is # not mistaken for a filter separator — mirroring the operator parsing below. @@ -1144,6 +1179,13 @@ def _unresolvable_term(text: str) -> str | None: if not stripped: return "an operand is empty" + # Mirror the evaluator's group unwrapping. Without this a grouped operand + # reached the path check as literal text, so `(inputs.a or inputs.b) and + # inputs.c` -- which the evaluator resolves -- was reported unresolvable and + # the wrap correction was withheld from a condition that would have worked. + if _is_wrapped_in_parens(stripped): + return _unresolvable_term(stripped[1:-1]) + if _find_top_level(stripped, "|") != -1: segments = _split_top_level(stripped, "|") reason = _unresolvable_term(segments[0]) diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 2299752854..3e2b794238 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -917,6 +917,33 @@ def test_boolean_literal(self): assert evaluate_expression("{{ true }}", ctx) is True assert evaluate_expression("{{ false }}", ctx) is False + def test_parenthesised_grouping(self): + """A parenthesised group is evaluated, not read as a dot path. + + The operator scans skip bracketed text so an operator inside an + operand is not split on. Nothing unwrapped a group spanning the whole + expression, so ``(a or b) and c`` split at the top-level ``and`` and + then looked up ``(a or b)`` as a key, got ``None``, and read false -- + adding parentheses to make precedence explicit silently inverted the + result. + """ + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext(inputs={"a": True, "b": False, "c": True, "n": 5}) + + assert evaluate_expression("{{ (inputs.a or inputs.b) and inputs.c }}", ctx) is True + assert evaluate_expression("{{ (inputs.b or inputs.b) and inputs.c }}", ctx) is False + assert evaluate_expression("{{ (inputs.n) }}", ctx) == 5 + assert evaluate_expression("{{ (inputs.n > 1) }}", ctx) is True + assert evaluate_expression("{{ ((inputs.n)) }}", ctx) == 5 + # A group is still only unwrapped when it spans the whole expression. + assert evaluate_expression("{{ (inputs.a) and (inputs.b) }}", ctx) is False + assert evaluate_expression("{{ (inputs.n) | default(9) }}", ctx) == 5 + # A parenthesis inside a string literal is not a group. + assert evaluate_expression("{{ 'a(b' }}", ctx) == "a(b" + assert evaluate_expression("{{ ('(') }}", ctx) == "(" + def test_list_indexing(self): from specify_cli.workflows.expressions import evaluate_expression from specify_cli.workflows.base import StepContext diff --git a/tests/unit/test_condition_expression_block.py b/tests/unit/test_condition_expression_block.py index 0739a2bc29..3f1a4211a5 100644 --- a/tests/unit/test_condition_expression_block.py +++ b/tests/unit/test_condition_expression_block.py @@ -804,6 +804,36 @@ def test_resolvable_filter_arguments_keep_the_correction(condition): assert _wrapped_evaluates(condition) +@pytest.mark.parametrize( + "condition", + [ + "(inputs.a or inputs.b) and inputs.c", + "(inputs.a)", + "((inputs.a))", + "(inputs.a) and (inputs.c)", + ], +) +def test_a_grouped_bare_condition_keeps_the_correction(condition): + """Remediation tracks the evaluator, which now unwraps a parenthesised group. + + Before the evaluator learned to unwrap, a grouped operand reached the path + check as literal text, so a condition that wrapping would have fixed was + reported unresolvable and the correction was withheld. + """ + ctx = StepContext(inputs={"a": True, "b": False, "c": True}) + assert CORRECTION_OFFERED in format_condition_remediation(condition) + assert evaluate_condition("{{ " + condition + " }}", ctx) is True + + +@pytest.mark.parametrize( + "condition", + ["(bogus or inputs.b) and inputs.c", "(inputs.a or bogus)"], +) +def test_an_unresolvable_name_inside_a_group_still_refuses(condition): + """The unwrap must not become a blanket pass for anything parenthesised.""" + assert CORRECTION_OFFERED not in format_condition_remediation(condition) + + @pytest.mark.parametrize("condition", ["item[0] == 'x'", "item[1] == 'y'"]) def test_an_indexed_item_root_keeps_the_correction(condition): """`item` is the only root that is not always a mapping.