diff --git a/src/specify_cli/workflows/expressions.py b/src/specify_cli/workflows/expressions.py index 9953f5ff61..c18e1cf431 100644 --- a/src/specify_cli/workflows/expressions.py +++ b/src/specify_cli/workflows/expressions.py @@ -35,14 +35,38 @@ def _filter_default(value: Any, default_value: Any = "") -> Any: def _filter_join(value: Any, separator: str = ", ") -> str: - """Join a list into a string with *separator*.""" + """Join a list into a string with *separator*. + + Raises ``ValueError`` when *separator* is not a string. Without the guard a + non-string separator (an authoring mistake like ``| join(5)``) reaches + ``str.join`` and raises a cryptic ``AttributeError: 'int' object has no + attribute 'join'`` that escapes the evaluator and crashes the whole run, + since the engine wraps neither expression evaluation nor ``execute`` in a + try/except. Mirrors the strict argument handling in ``from_json``. + """ + if not isinstance(separator, str): + raise ValueError( + f"join: expected a string separator, got {type(separator).__name__}" + ) if isinstance(value, list): return separator.join(str(v) for v in value) return str(value) def _filter_map(value: Any, attr: str) -> list[Any]: - """Map a list of dicts to a specific attribute.""" + """Map a list of dicts to a specific attribute. + + Raises ``ValueError`` when *attr* is not a string. Without the guard a + non-string attribute (an authoring mistake like ``| map(5)``) reaches + ``attr.split(".")`` and raises a cryptic ``AttributeError: 'int' object has + no attribute 'split'`` that escapes the evaluator and crashes the whole run, + since the engine wraps neither expression evaluation nor ``execute`` in a + try/except. Mirrors the strict argument handling in ``from_json``. + """ + if not isinstance(attr, str): + raise ValueError( + f"map: expected a string attribute name, got {type(attr).__name__}" + ) if isinstance(value, list): result = [] for item in value: @@ -63,9 +87,25 @@ def _filter_map(value: Any, attr: str) -> list[Any]: return [] -def _filter_contains(value: Any, substring: str) -> bool: - """Check if a string or list contains *substring*.""" +def _filter_contains(value: Any, substring: Any) -> bool: + """Check if a string or list contains *substring*. + + For a string *value*, *substring* must itself be a string: ``x in y`` on a + string requires a string left operand, so a non-string argument (an + authoring mistake like ``| contains(5)``) would otherwise raise a cryptic + ``TypeError`` that escapes the evaluator and crashes the whole run, since + the engine wraps neither expression evaluation nor ``execute`` in a + try/except. Raise a ``ValueError`` naming the problem instead, mirroring the + strict argument handling in ``from_json``. For a list *value*, membership of + any element type is legitimate (``5 in [1, 2, 5]``), so that branch is left + unguarded. + """ if isinstance(value, str): + if not isinstance(substring, str): + raise ValueError( + "contains: expected a string argument when the value is a " + f"string, got {type(substring).__name__}" + ) return substring in value if isinstance(value, list): return substring in value diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 8fefad740e..16b140b309 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -586,6 +586,51 @@ def test_filter_unknown_name_with_args_raises(self): with pytest.raises(ValueError, match="unknown filter 'upper'"): evaluate_expression("{{ inputs.text | upper('x') }}", ctx) + def test_filter_map_non_string_attr_raises(self): + # A non-string attribute (authoring mistake like `map(5)`) must raise a + # ValueError naming the problem, not leak the cryptic AttributeError + # from attr.split() that would escape the evaluator and crash the run. + import pytest + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext(inputs={"rows": [{"id": "a"}, {"id": "b"}]}) + with pytest.raises(ValueError, match="map: expected a string attribute name"): + evaluate_expression("{{ inputs.rows | map(5) }}", ctx) + + def test_filter_join_non_string_separator_raises(self): + # A non-string separator (authoring mistake like `join(5)`) must raise a + # ValueError, not leak the cryptic AttributeError from str.join. + import pytest + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext(inputs={"tags": ["a", "b"]}) + with pytest.raises(ValueError, match="join: expected a string separator"): + evaluate_expression("{{ inputs.tags | join(5) }}", ctx) + + def test_filter_contains_non_string_arg_on_string_raises(self): + # For a string value, `contains` requires a string argument: `x in y` on + # a string needs a string left operand. A non-string argument must raise + # a ValueError, not leak the cryptic TypeError that would crash the run. + 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="contains: expected a string argument"): + evaluate_expression("{{ inputs.text | contains(5) }}", ctx) + + def test_filter_contains_non_string_arg_on_list_ok(self): + # For a list value, membership of any element type is legitimate, so a + # non-string argument stays valid and is not rejected. + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext(inputs={"nums": [1, 2, 5]}) + assert evaluate_expression("{{ inputs.nums | contains(5) }}", ctx) is True + assert evaluate_expression("{{ inputs.nums | contains(9) }}", ctx) is False + def test_registered_filters_unaffected(self): # Regression: all five registered filters keep working unchanged. from specify_cli.workflows.expressions import evaluate_expression