From 1cb935997c1ab4cbecabfcb3aa8b901bce64e9e1 Mon Sep 17 00:00:00 2001 From: Huy Do Date: Mon, 22 Jun 2026 22:44:23 +0700 Subject: [PATCH] fix: fail loudly on an unknown workflow expression filter (#3074) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: fail loudly on an unknown workflow expression filter The expression evaluator's filter dispatch fell through to `return value` for any unregistered filter, so a typo'd or unsupported filter such as `{{ items | length }}` rendered the value unchanged with no error and the run completed — a silent wrong result. Raise a clear ValueError instead, naming the offending filter and the valid ones, mirroring the strict handling already used for `from_json`. The five registered filters (default/join/map/contains/from_json) are unchanged; the `name(arg)` form of an unknown filter is now caught too. * fix: distinguish a misused registered filter from an unknown one; cover map Address the review feedback on the unknown-filter fail-loud path: - A *registered* filter used in an unsupported form (e.g. `| join` or `| map` with no argument) raised the misleading "unknown filter ''" — the filter is registered, the syntax isn't. It now raises a message naming it as a known filter misused. A new `_REGISTERED_FILTERS` constant drives the distinction. - `test_registered_filters_unaffected` now also exercises `map('attr')`, which it previously claimed to cover but didn't. Add `test_registered_filter_unsupported_form_raises` to pin the new path. * fix: include the no-arg default form in the filter-error hint Copilot review: the hint listed default('x') but omitted the valid no-argument default form (| default), which this module supports. --- src/specify_cli/workflows/expressions.py | 35 ++++++++++++- tests/test_workflows.py | 67 ++++++++++++++++++++++++ 2 files changed, 101 insertions(+), 1 deletion(-) diff --git a/src/specify_cli/workflows/expressions.py b/src/specify_cli/workflows/expressions.py index 6259b59de..ca10b24d1 100644 --- a/src/specify_cli/workflows/expressions.py +++ b/src/specify_cli/workflows/expressions.py @@ -12,6 +12,19 @@ import re from typing import Any +# The filters the expression evaluator recognizes. Used to tell a +# *registered* filter used in an unsupported form (e.g. `| join` with no +# argument) apart from a genuinely unknown filter name, so each raises an +# error that names the real problem. +_REGISTERED_FILTERS: tuple[str, ...] = ( + "default", + "join", + "map", + "contains", + "from_json", +) + + # -- Custom filters ------------------------------------------------------- def _filter_default(value: Any, default_value: Any = "") -> Any: @@ -192,7 +205,27 @@ def _evaluate_simple_expression(expr: str, namespace: dict[str, Any]) -> Any: filter_name = filter_expr.strip() if filter_name == "default": return _filter_default(value) - return value + # No recognized filter matched. Fail loudly rather than silently + # returning the unfiltered value: a passthrough turns a mis-typed or + # unsupported filter into a wrong result with no signal. Mirrors the + # strict `from_json` handling above. Distinguish a *registered* filter + # used in an unsupported form (e.g. `| join` or `| map` with no + # argument) from a genuinely unknown filter name, so the message names + # the real problem instead of calling a known filter "unknown". + leading_name = re.match(r"\w+", filter_expr) + name = leading_name.group(0) if leading_name else filter_expr + expected = ( + "expected one of default or default('x'), join('sep'), " + "map('attr'), contains('s'), or from_json" + ) + if name in _REGISTERED_FILTERS: + raise ValueError( + f"filter '{name}' used in an unsupported form (got " + f"'| {filter_expr}'): {expected}" + ) + raise ValueError( + f"unknown filter '{name}': {expected} (got '| {filter_expr}')" + ) # Boolean operators — parse 'or' first (lower precedence) so that # 'a or b and c' is evaluated as 'a or (b and c)'. diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 8cbd4a6e8..512b35415 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -342,6 +342,73 @@ class TestExpressions: "{{ steps.emit.output.stdout | " + bad + " }}", ctx ) + def test_filter_unknown_name_raises(self): + # An unregistered filter name must fail loudly rather than silently + # returning the unfiltered value (which hides a typo / unsupported + # filter as a wrong result). + import pytest + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext(inputs={"items": [1, 2, 3]}) + with pytest.raises(ValueError, match="unknown filter 'length'"): + evaluate_expression("{{ inputs.items | length }}", ctx) + + def test_filter_unknown_name_with_args_raises(self): + # The unknown-filter path must also catch the `name(arg)` form, which + # otherwise falls through the recognized-args branch silently. + import pytest + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext(inputs={"text": "hello"}) + with pytest.raises(ValueError, match="unknown filter 'upper'"): + evaluate_expression("{{ inputs.text | upper('x') }}", ctx) + + def test_registered_filters_unaffected(self): + # Regression: all five registered filters keep working unchanged. + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext( + inputs={ + "tags": ["a", "b", "c"], + "text": "hello world", + "missing": "", + "rows": [{"id": "a"}, {"id": "b"}], + }, + steps={"emit": {"output": {"stdout": '{"n": 1}'}}}, + ) + assert ( + evaluate_expression("{{ inputs.missing | default('fb') }}", ctx) == "fb" + ) + assert evaluate_expression("{{ inputs.tags | join(', ') }}", ctx) == "a, b, c" + assert evaluate_expression("{{ inputs.rows | map('id') }}", ctx) == ["a", "b"] + assert ( + evaluate_expression("{{ inputs.text | contains('world') }}", ctx) is True + ) + assert evaluate_expression( + "{{ steps.emit.output.stdout | from_json }}", ctx + ) == {"n": 1} + + def test_registered_filter_unsupported_form_raises(self): + # A *registered* filter used in an unsupported form (e.g. `| join` with + # no argument) must fail loudly with a message that names it as a known + # filter misused, not as an "unknown filter". + import pytest + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext(inputs={"tags": ["a", "b", "c"]}) + with pytest.raises( + ValueError, match="filter 'join' used in an unsupported form" + ): + evaluate_expression("{{ inputs.tags | join }}", ctx) + with pytest.raises( + ValueError, match="filter 'map' used in an unsupported form" + ): + evaluate_expression("{{ inputs.tags | map }}", ctx) + def test_condition_evaluation(self): from specify_cli.workflows.expressions import evaluate_condition from specify_cli.workflows.base import StepContext