diff --git a/src/specify_cli/workflows/expressions.py b/src/specify_cli/workflows/expressions.py index 38a29890ae..b362d7664d 100644 --- a/src/specify_cli/workflows/expressions.py +++ b/src/specify_cli/workflows/expressions.py @@ -465,7 +465,40 @@ 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() + # Unary ``not`` is a leading prefix, not an infix token, so it has no + # surrounding space for the scan below to match -- it has to be checked + # the same way the parser itself does (``expr.startswith("not ")``). + # Without this, ``not inputs.missing | default(1)`` still evaluated + # ``not inputs.missing`` first and applied the filter to that boolean, + # which is the exact mis-binding this guard exists to reject. + # (A ``not`` that follows ``and``/``or`` is already caught by those + # tokens below.) + _ambiguous_op = "not" if head.startswith("not ") else None + if _ambiguous_op is None: + for _op in ("!=", "==", ">=", "<=", ">", "<", " not in ", " in ", + " or ", " and "): + if _find_top_level(head, _op) != -1: + _ambiguous_op = _op.strip() + break + if _ambiguous_op is not None: + 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 '{_ambiguous_op}'. 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 afd70adecf..98d3915f78 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -711,6 +711,74 @@ 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_filter_after_a_unary_not_is_refused(self): + """Unary `not` mis-binds the same way and must be caught too. + + `not` is a leading prefix rather than an infix token (the parser tests + it with `expr.startswith("not ")`), so it has no surrounding space for + the operator scan to match. Without an explicit check, + `not inputs.missing | default(1)` evaluated `not inputs.missing` first + and applied `default` to that boolean — a no-op — silently returning + True where the author meant `not 1` = False. + """ + import pytest + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext(inputs={"value": 0, "flag": True}) + with pytest.raises(ValueError, match="ambiguous filter precedence"): + evaluate_expression("{{ not inputs.missing | default(1) }}", ctx) + with pytest.raises(ValueError, match="ambiguous filter precedence"): + evaluate_expression("{{ not inputs.value | default(1) }}", ctx) + # A `not` that follows and/or is already caught by that token. + with pytest.raises(ValueError, match="ambiguous filter precedence"): + evaluate_expression( + "{{ inputs.flag and not inputs.value | default(1) }}", ctx + ) + # Plain unary `not`, with no filter, is untouched. + assert evaluate_expression("{{ not inputs.value }}", ctx) is True + assert evaluate_expression("{{ not inputs.flag }}", ctx) is False + + 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,