From 1b12899e836442460665fb46d7aa511ebce72709 Mon Sep 17 00:00:00 2001 From: doquanghuy Date: Sat, 20 Jun 2026 04:00:40 +0700 Subject: [PATCH 1/3] fix: fail loudly on an unknown workflow expression filter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/specify_cli/workflows/expressions.py | 13 ++++++- tests/test_workflows.py | 43 ++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 1 deletion(-) diff --git a/src/specify_cli/workflows/expressions.py b/src/specify_cli/workflows/expressions.py index 6259b59de0..d54d43c9c4 100644 --- a/src/specify_cli/workflows/expressions.py +++ b/src/specify_cli/workflows/expressions.py @@ -192,7 +192,18 @@ 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 — an unknown filter name, or a known + # filter used in an unsupported form. 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. + leading_name = re.match(r"\w+", filter_expr) + name = leading_name.group(0) if leading_name else filter_expr + raise ValueError( + f"unknown filter '{name}': expected one of default('x'), " + "join('sep'), map('attr'), contains('s'), or from_json " + f"(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 a87c09cf05..2dd560eb2e 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -342,6 +342,49 @@ def test_filter_from_json_rejects_malformed_forms(self): "{{ 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: the 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": ""}, + 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.text | contains('world') }}", ctx) is True + ) + assert evaluate_expression( + "{{ steps.emit.output.stdout | from_json }}", ctx + ) == {"n": 1} + def test_condition_evaluation(self): from specify_cli.workflows.expressions import evaluate_condition from specify_cli.workflows.base import StepContext From d37e440e293aa7f76f1f7c56dad9bba7b2c71cf4 Mon Sep 17 00:00:00 2001 From: doquanghuy Date: Mon, 22 Jun 2026 19:54:22 +0700 Subject: [PATCH 2/3] fix: distinguish a misused registered filter from an unknown one; cover map MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/specify_cli/workflows/expressions.py | 34 +++++++++++++++++++----- tests/test_workflows.py | 28 +++++++++++++++++-- 2 files changed, 54 insertions(+), 8 deletions(-) diff --git a/src/specify_cli/workflows/expressions.py b/src/specify_cli/workflows/expressions.py index d54d43c9c4..00f3d762d2 100644 --- a/src/specify_cli/workflows/expressions.py +++ b/src/specify_cli/workflows/expressions.py @@ -12,6 +12,19 @@ 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,17 +205,26 @@ def _evaluate_simple_expression(expr: str, namespace: dict[str, Any]) -> Any: filter_name = filter_expr.strip() if filter_name == "default": return _filter_default(value) - # No recognized filter matched — an unknown filter name, or a known - # filter used in an unsupported form. Fail loudly rather than silently + # 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. + # 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('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 one of default('x'), " - "join('sep'), map('attr'), contains('s'), or from_json " - f"(got '| {filter_expr}')" + f"unknown filter '{name}': {expected} (got '| {filter_expr}')" ) # Boolean operators — parse 'or' first (lower precedence) so that diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 2dd560eb2e..bfb3c1bbe1 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -366,18 +366,24 @@ def test_filter_unknown_name_with_args_raises(self): evaluate_expression("{{ inputs.text | upper('x') }}", ctx) def test_registered_filters_unaffected(self): - # Regression: the five registered filters keep working unchanged. + # 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": ""}, + 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 ) @@ -385,6 +391,24 @@ def test_registered_filters_unaffected(self): "{{ 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 From e7e64ba205884e3e027192782f6b6f5b64c65320 Mon Sep 17 00:00:00 2001 From: doquanghuy Date: Mon, 22 Jun 2026 22:08:16 +0700 Subject: [PATCH 3/3] 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 | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/specify_cli/workflows/expressions.py b/src/specify_cli/workflows/expressions.py index 00f3d762d2..ca10b24d1b 100644 --- a/src/specify_cli/workflows/expressions.py +++ b/src/specify_cli/workflows/expressions.py @@ -215,8 +215,8 @@ def _evaluate_simple_expression(expr: str, namespace: dict[str, Any]) -> Any: leading_name = re.match(r"\w+", filter_expr) name = leading_name.group(0) if leading_name else filter_expr expected = ( - "expected one of default('x'), join('sep'), map('attr'), " - "contains('s'), or from_json" + "expected one of default or default('x'), join('sep'), " + "map('attr'), contains('s'), or from_json" ) if name in _REGISTERED_FILTERS: raise ValueError(