diff --git a/src/specify_cli/workflows/expressions.py b/src/specify_cli/workflows/expressions.py index a38cd6cb68..ba6ffbb3db 100644 --- a/src/specify_cli/workflows/expressions.py +++ b/src/specify_cli/workflows/expressions.py @@ -465,7 +465,27 @@ def _evaluate_simple_expression(expr: str, namespace: dict[str, Any]) -> Any: pipe_idx = _find_top_level(expr, "|") if pipe_idx != -1: segments = _split_top_level(expr, "|") - value = _evaluate_simple_expression(segments[0].strip(), namespace) + # The pipe is detected before the operators below, so a filter written on + # the right-hand operand of a comparison was applied to the comparison's + # BOOLEAN RESULT instead: `count > limit | default(5)` evaluated + # `count > limit` first and then `default` on the bool, which is a no-op, + # so the expression silently returned the comparison against the + # *unfiltered* operand. This is the mirror of a filter followed by a + # comparison (`default('7') > '5'`), which this module already refuses + # rather than guessing at the intended precedence. Refuse both the same + # way, so an ambiguous expression is reported instead of quietly + # producing the answer the author did not ask for. + head = segments[0].strip() + for _op in ("!=", "==", ">=", "<=", ">", "<", " not in ", " in ", + " or ", " and "): + if _find_top_level(head, _op) != -1: + raise ValueError( + f"ambiguous filter precedence in '{expr}': " + f"'| {segments[1].strip()}' would apply to the result of " + f"'{head}', not to an operand of '{_op.strip()}'. Filter the " + f"operand in its own expression instead." + ) + value = _evaluate_simple_expression(head, namespace) for segment in segments[1:]: value = _apply_filter(value, segment.strip(), namespace) return value diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 57536081ea..85b8938c7e 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -708,6 +708,46 @@ def test_filter_call_with_trailing_tokens_fails_loudly(self): StepContext(inputs={"tags": ["a", "b"]}), ) + def test_filter_on_a_comparison_operand_is_refused(self): + """A filter mixed with a comparison must be reported, not guessed at. + + The pipe is detected before the operators, so + `count > limit | default(5)` evaluated `count > limit` first and then + applied `default` to the resulting bool — a no-op — silently returning + the comparison against the *unfiltered* operand (False, where the author + meant `10 > 5` = True). + + This is the mirror of a filter followed by a comparison + (`default('7') > '5'`), which this module already refuses rather than + guessing at the intended precedence. Both are now refused the same way. + """ + import pytest + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext(inputs={"count": 10, "name": "x"}) + with pytest.raises(ValueError, match="ambiguous filter precedence"): + evaluate_expression("{{ inputs.count > inputs.limit | default(5) }}", ctx) + with pytest.raises(ValueError, match="ambiguous filter precedence"): + evaluate_expression( + '{{ inputs.name == inputs.other | default("x") }}', ctx + ) + with pytest.raises(ValueError, match="ambiguous filter precedence"): + evaluate_expression("{{ inputs.a and inputs.b | default(1) }}", ctx) + + def test_plain_filters_and_chains_are_unaffected(self): + """Only a filter mixed with an operator is refused.""" + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext(inputs={"items": ["a", "b"], "count": 10, "name": "x"}) + assert evaluate_expression("{{ inputs.missing | default(5) }}", ctx) == 5 + assert evaluate_expression('{{ inputs.items | join(", ") }}', ctx) == "a, b" + assert evaluate_expression("{{ inputs.items | contains('a') }}", ctx) is True + # Operators without a filter, and filters without an operator, both fine. + assert evaluate_expression("{{ inputs.count > 5 }}", ctx) is True + assert evaluate_expression('{{ inputs.name == "x" }}', ctx) is True + def test_chained_filters_apply_left_to_right(self): # Filters chain: each filter's result feeds the next. `map` yields a # list and `join` is the only filter that renders a list to a string,