diff --git a/docs/reference/workflows.md b/docs/reference/workflows.md index 2ebe4ff5e6..099420ab02 100644 --- a/docs/reference/workflows.md +++ b/docs/reference/workflows.md @@ -632,7 +632,34 @@ Steps can reference inputs and previous step outputs using `{{ expression }}` sy | `context.run_id` | Current workflow run ID | | `context.workflow_dir` | Resolved absolute path to the workflow source directory. Empty string for string-loaded workflows. | -Available filters: `default`, `join`, `contains`, `map`, `from_json`. +Available filters: `default`, `join`, `contains`, `map`, `from_json`, `to_json`, `upper`, `lower`, `split`, `length`. + +| Filter | Example | Behavior | +| -------- | ------------------------------------------ | ----------------------------------------------------------------------------------------------- | +| `default`| `{{ val \| default('fb') }}` | Fallback for `None` or an empty string | +| `join` | `{{ list \| join(', ') }}` | Join list elements into a string | +| `contains`| `{{ text \| contains('sub') }}` | Substring or membership check | +| `map` | `{{ list \| map('attr') }}` | Extract an attribute from each item | +| `from_json`| `{{ out \| from_json }}` | Parse a JSON string into a typed value | +| `to_json`| `{{ obj \| to_json }}` | Serialize a value to a JSON string — the inverse of `from_json`; mapping keys must be strings | +| `upper` | `{{ text \| upper }}` | Uppercase a string | +| `lower` | `{{ text \| lower }}` | Lowercase a string | +| `split` | `{{ csv \| split(',') }}` | Split a string on a separator into a list of strings | +| `length` | `{{ items \| length }}` | Number of elements in a list, or characters in a string | + +`default` falls back only for `None` and the empty string. Other falsy values — `0`, `false`, `[]`, `{}` — are passed through unchanged, so `{{ count | default(10) }}` still yields `0` for a zero count. Falsy is not the same as empty here. + +Notes on the newer filters: + +- **Types are validated, not coerced.** `upper` and `lower` accept strings only, `split` requires both a string value and a non-empty string separator, and `length` accepts lists and strings but rejects mappings. Anything else raises a `ValueError` naming the problem. Coercion is deliberately not performed: a type mismatch nearly always means the workflow is wired to the wrong variable, and a coerced result would hide that. A filter given the wrong number of arguments (`| upper('x')`, `| split` with no separator, `| split(',', 1)`) is reported as a known filter misused, which is distinct from an entirely unknown filter name: a call carrying more than one argument falls through to that same unsupported-form error rather than being evaluated as a single expression. The older filters (`join`, `map`, `contains`) are more permissive and unchanged: `join` stringifies unsupported values and `map`/`contains` return fallbacks rather than raising. +- **`to_json` output is deterministic.** Object keys are sorted and non-ASCII characters are left as-is rather than escaped, so the same value always serializes to the same bytes. That buys reproducibility only — it does **not** make the result safe to pass through a shell, because [interpolation adds no quoting or escaping](#interpolation-and-shell-safety). Do not interpolate unconstrained JSON into a `run` field. +- **`to_json` rejects non-finite floats.** `NaN`, `Infinity`, and `-Infinity` raise a `ValueError` naming `to_json` instead of serializing to bare tokens. None of the three is valid JSON, so emitting them would hand a standards-compliant downstream parser a string it must reject. +- **`to_json` requires string mapping keys.** JSON objects have string keys, so a mapping with any other key type — `{1: "a"}`, `{True: "a"}` — raises a `ValueError` naming the key type instead. Left to `json.dumps`, an integer key would be coerced to `"1"` and collide with an existing `"1"` key, while mixed key types would fail `sort_keys` with an ordering `TypeError` reported only as "not JSON-serializable". +- **Trailing comparisons after a filter are rejected.** The parser splits on the top-level `|` before looking for operators, so `{{ items | length > 0 }}` raises rather than evaluating. The count does not exist until `length` runs, so there is no way to write that comparison; the supported branching form is the filter's own truthiness in a `condition:`, since `length` returns `0` for an empty input: + + ```yaml + condition: "{{ inputs.items | length }}" # 0 is False, any non-zero count is True + ``` Example: @@ -640,6 +667,8 @@ Example: condition: "{{ steps.test.output.exit_code == 0 }}" args: "{{ inputs.spec }}" message: "{{ status | default('pending') }}" +tag_count: "{{ inputs.tags | split(',') | length }}" +shell_flag: "{{ inputs.branch | upper }}" ``` ### Interpolation and shell safety diff --git a/src/specify_cli/workflows/expressions.py b/src/specify_cli/workflows/expressions.py index 2dc474a140..5410da80b1 100644 --- a/src/specify_cli/workflows/expressions.py +++ b/src/specify_cli/workflows/expressions.py @@ -9,6 +9,7 @@ import json import re +from collections.abc import Callable from contextvars import ContextVar from typing import Any @@ -23,6 +24,11 @@ "map", "contains", "from_json", + "upper", + "lower", + "split", + "length", + "to_json", ) @@ -130,6 +136,162 @@ def _filter_from_json(value: Any) -> Any: raise ValueError(f"from_json: invalid JSON: {exc}") from exc +def _filter_upper(value: Any) -> str: + """Return *value* uppercased. + + Raises ``ValueError`` on non-string input. Without the guard a non-string + value (an authoring mistake like ``| upper`` on a shell exit code) would + reach ``str.upper`` and raise a cryptic ``AttributeError`` that escapes the + evaluator and crashes the whole run, since the engine wraps neither + expression evaluation nor ``execute`` in a try/except. Silently coercing + with ``str(value).upper()`` is rejected for the same reason ``from_json`` + does not coerce: a type mismatch here means the pipeline is wired to the + wrong variable, and rendering ``"0"`` for an int hides that. + """ + if not isinstance(value, str): + raise ValueError(f"upper: expected a string value, got {type(value).__name__}") + return value.upper() + + +def _filter_lower(value: Any) -> str: + """Return *value* lowercased. + + Raises ``ValueError`` on non-string input, for the same reason and with the + same trade-off as ``upper``: a type mismatch means the pipeline is wired to + the wrong variable, and coercing would hide it. + """ + if not isinstance(value, str): + raise ValueError(f"lower: expected a string value, got {type(value).__name__}") + return value.lower() + + +def _filter_split(value: Any, separator: str) -> list[str]: + """Split *value* on *separator* into a list of strings. + + The single-argument ``split(sep)`` form is the only one supported; there is + no maxsplit, because a partial split has no obvious meaning in a workflow + expression and an unused parameter is an authoring mistake worth reporting. + + Raises ``ValueError`` when *value* is not a string, *separator* is not a + string, or *separator* is empty. Without those guards a non-string argument + reaches ``str.split`` and raises a cryptic ``TypeError``, and an empty + separator raises the bare ``ValueError: empty separator`` — neither names + the filter, and both escape the evaluator and crash the whole run, mirroring + the strict argument handling in ``join`` and ``map``. An empty separator has + no meaning anyway: ``str.split("")`` is an error in Python, so it is an + authoring mistake rather than a valid edge case. + """ + if not isinstance(value, str): + raise ValueError(f"split: expected a string value, got {type(value).__name__}") + if not isinstance(separator, str): + raise ValueError( + f"split: expected a string separator, got {type(separator).__name__}" + ) + if separator == "": + raise ValueError("split: separator must not be empty") + return value.split(separator) + + +def _filter_length(value: Any) -> int: + """Return the length of a list or string. + + Follows Jinja2's ``length``, which also counts characters in a string. The + two supported input types are exactly ``list`` and ``str``: dicts are + excluded even though ``len()`` accepts them, because a mapping's length is + rarely what a workflow author means by ``length`` and accepting it would + make ``{{ obj | length }}`` silently return a key count for one shape and a + value count for another. Other types (notably ``bool`` and ``None``) are + authoring mistakes and raise rather than coercing to 0. + """ + if isinstance(value, (list, str)): + return len(value) + raise ValueError( + "length: expected a list or string, got " + f"{type(value).__name__} (mappings are not supported)" + ) + + +def _filter_to_json(value: Any) -> str: + """Serialize *value* to a JSON string — the inverse of ``from_json``. + + Serialization is pinned to ``sort_keys=True`` and ``ensure_ascii=False`` so + the output is byte-stable across runs, platforms, and dict insertion order. + It is why these flags are not left to the default: the default key order + varies with insertion order and escapes non-ASCII as ``\\uXXXX``, so the + same workflow would emit different bytes on different runs and hand + downstream tools mangled text. Determinism buys *reproducibility* — the same + value always serializes identically — and nothing more. It does not make the + result safe to pass through a shell: expression interpolation adds no + quoting or escaping, so JSON quotes and metacharacters are still interpreted + by whatever runs the ``run`` field. Interpolate unconstrained JSON into a + shell step only when you have constrained what it can contain; see the + "Interpolation and shell safety" section of ``docs/reference/workflows.md``. + + Raises ``ValueError`` when *value* is not JSON-serializable, chained from + the underlying error so the offending type stays visible. ``allow_nan=False`` + is what makes that true for non-finite floats: ``json.dumps`` would + otherwise emit bare ``NaN``/``Infinity``/``-Infinity``, none of which is + valid JSON, and hand downstream parsers a string they must reject. + + Mapping keys must be strings, which is what JSON objects have anyway. + ``_check_json_keys`` enforces that before ``json.dumps`` is reached, so a + non-string key is reported as the authoring mistake it is rather than + surfacing as an ordering ``TypeError`` from ``sort_keys=True`` (mixed key + types) or as silent ``1`` → ``"1"`` coercion that collides with an existing + ``"1"`` key. + """ + _check_json_keys(value) + try: + return json.dumps(value, sort_keys=True, ensure_ascii=False, allow_nan=False) + except (TypeError, ValueError) as exc: + raise ValueError(f"to_json: value is not JSON-serializable: {exc}") from exc + + +def _check_json_keys(value: Any) -> None: + """Raise ``ValueError`` if any mapping reachable from *value* has a + non-string key. + + Walked iteratively with a ``seen`` set: a self-referential structure is + skipped rather than recursed into, so the circular reference stays for + ``json.dumps`` to report with its own clearer message instead of the walk + exhausting the stack first. + """ + seen: set[int] = set() + stack: list[Any] = [value] + while stack: + item = stack.pop() + if isinstance(item, dict): + if id(item) in seen: + continue + seen.add(id(item)) + for key, sub_value in item.items(): + if not isinstance(key, str): + raise ValueError( + "to_json: mapping keys must be strings, got " + f"{type(key).__name__}: {key!r}" + ) + stack.append(sub_value) + elif isinstance(item, (list, tuple)): + if id(item) in seen: + continue + seen.add(id(item)) + stack.extend(item) + + +# Filters that take no arguments and tolerate no trailing tokens. Keyed by name +# so ``_apply_filter`` can recognize a mis-wired form of any of them from the +# leading filter name alone, instead of each needing its own branch. ``default`` +# is deliberately absent: it is the one filter that is valid both bare and with +# an argument, so it is dispatched by both of the branches below. +_ZERO_ARG_FILTERS: dict[str, Callable[[Any], Any]] = { + "from_json": _filter_from_json, + "upper": _filter_upper, + "lower": _filter_lower, + "length": _filter_length, + "to_json": _filter_to_json, +} + + # -- Expression resolution ------------------------------------------------ _EXPR_PATTERN = re.compile(r"\{\{(.+?)\}\}") @@ -464,19 +626,21 @@ def _apply_filter(value: Any, filter_expr: str, namespace: dict[str, Any]) -> An silently returning *value* unchanged: a passthrough would turn a mistyped or unsupported filter into a wrong result with no signal. """ - # `from_json` is strict: it takes no arguments and tolerates no trailing - # tokens. Match on the leading filter name and require the whole filter to - # be exactly `from_json`, so every mis-wired form (`from_json()`, - # `from_json('x')`, `from_json)`, `from_json extra`) fails loudly instead of - # silently falling through to the unknown-filter path. + # Zero-argument filters are strict: they take no arguments and tolerate no + # trailing tokens. Match on the leading filter name and require the whole + # filter to be exactly that name, so every mis-wired form (`from_json()`, + # `from_json('x')`, `from_json)`, `from_json extra`, and the same for + # `upper`/`lower`/`length`/`to_json`) fails loudly instead of silently + # falling through to the unknown-filter path. leading = re.match(r"\w+", filter_expr) - if leading and leading.group(0) == "from_json": - if filter_expr != "from_json": + if leading and leading.group(0) in _ZERO_ARG_FILTERS: + fname = leading.group(0) + if filter_expr != fname: raise ValueError( - "from_json: expected '| from_json' with no arguments or " + f"{fname}: expected '| {fname}' with no arguments or " f"trailing tokens, got '| {filter_expr}'" ) - return _filter_from_json(value) + return _ZERO_ARG_FILTERS[fname](value) # Parse filter name and argument. Use fullmatch (not match) so trailing # tokens after the closing paren — e.g. a comparison/boolean operator that @@ -519,6 +683,8 @@ def _apply_filter(value: Any, filter_expr: str, namespace: dict[str, Any]) -> An return _filter_map(value, farg) if fname == "contains": return _filter_contains(value, farg) + if fname == "split": + return _filter_split(value, farg) # Filter without args if filter_expr == "default": return _filter_default(value) @@ -530,7 +696,8 @@ def _apply_filter(value: Any, filter_expr: str, namespace: dict[str, Any]) -> An name = leading.group(0) if leading else filter_expr expected = ( "expected one of default or default('x'), join('sep'), " - "map('attr'), contains('s'), or from_json" + "map('attr'), contains('s'), split('sep'), from_json, upper, " + "lower, length, or to_json" ) if name in _REGISTERED_FILTERS: raise ValueError( @@ -586,7 +753,7 @@ def _evaluate_simple_expression(expr: str, namespace: dict[str, Any]) -> Any: - Comparisons: ``==``, ``!=``, ``>``, ``<``, ``>=``, ``<=`` - Boolean operators: ``and``, ``or``, ``not`` - ``in``, ``not in`` - - Pipe filters: ``| default('...')``, ``| join(', ')``, ``| contains('...')``, ``| from_json``, ``| map('...')`` + - Pipe filters: ``| default('...')``, ``| join(', ')``, ``| contains('...')``, ``| from_json``, ``| map('...')``, ``| split(',')``, ``| upper``, ``| lower``, ``| length``, ``| to_json`` - String and numeric literals """ expr = expr.strip() @@ -889,8 +1056,8 @@ def evaluate_condition(condition: str, context: Any) -> bool: # strip that trailing newline matches neither branch and falls through to # ``bool("false\n")`` -> True, silently taking an ``if`` step's ``then`` # branch (and keeping a ``while``/``do-while`` looping) on a step that - # printed "false". A workflow cannot strip it itself -- the registered - # filters are default/join/map/contains/from_json, there is no ``trim``. + # printed "false". A workflow cannot strip it itself -- no registered + # filter trims whitespace (there is no ``trim``). # ``InitStep._resolve_bool`` and the catalog readers already strip before # matching boolean text. ``bool(result)`` below still sees the raw string, # so no non-boolean text changes truthiness. diff --git a/src/specify_cli/workflows/step/switch/__init__.py b/src/specify_cli/workflows/step/switch/__init__.py index 8a2e4b343e..2421548625 100644 --- a/src/specify_cli/workflows/step/switch/__init__.py +++ b/src/specify_cli/workflows/step/switch/__init__.py @@ -29,8 +29,8 @@ def execute(self, config: dict[str, Any], context: StepContext) -> StepResult: # ``run: echo approve`` resolves to ``"approve\n"`` and matches no # ``approve:`` case -- the switch silently falls through to ``default:`` # while still reporting COMPLETED. A workflow cannot strip it itself: - # the registered filters are default/join/map/contains/from_json, there - # is no ``trim``. ``evaluate_condition`` and ``InitStep._resolve_bool`` + # no registered filter trims whitespace (there is no ``trim``). + # ``evaluate_condition`` and ``InitStep._resolve_bool`` # already strip before matching a resolved string against declared # literals, and case keys are exactly such literals. ``expression_value`` # below still reports the raw value, so nothing downstream loses it. diff --git a/tests/test_workflows.py b/tests/test_workflows.py index acfdaa0b3b..3d16b97f3a 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -598,6 +598,301 @@ def test_filter_from_json_rejects_malformed_forms(self): "{{ steps.emit.output.stdout | " + bad + " }}", ctx ) + def test_filter_upper_and_lower(self): + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext(inputs={"s": "Hello World", "empty": "", "mixed": "aBc"}) + assert evaluate_expression("{{ inputs.s | upper }}", ctx) == "HELLO WORLD" + assert evaluate_expression("{{ inputs.s | lower }}", ctx) == "hello world" + # An empty string is valid input, not a missing value: it must come back + # empty rather than raising or falling through to `default`. + assert evaluate_expression("{{ inputs.empty | upper }}", ctx) == "" + assert evaluate_expression("{{ inputs.empty | lower }}", ctx) == "" + # Case-only transforms are no-ops on already-conforming input. + assert evaluate_expression("{{ inputs.mixed | upper }}", ctx) == "ABC" + # Non-ASCII text uses Unicode case mapping without transliteration or loss. + assert evaluate_expression("{{ inputs.uni | upper }}", StepContext(inputs={"uni": "café"})) == "CAFÉ" + # Filters compose left to right with the rest of the chain. + assert evaluate_expression("{{ inputs.s | upper | lower }}", ctx) == "hello world" + + def test_filter_upper_rejects_non_string(self): + # A non-string value is an authoring mistake (e.g. `| upper` on an exit + # code). It must raise a ValueError naming the problem rather than + # coerce to "3" — which would look like a plausible result and hide the + # mis-wiring — or leak AttributeError and crash the run. + import pytest + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext(inputs={"items": ["a"], "n": 3, "flag": True, "none": None}) + for name in ("items", "n", "flag", "none"): + with pytest.raises(ValueError, match="upper: expected a string value"): + evaluate_expression("{{ inputs." + name + " | upper }}", ctx) + + def test_filter_lower_rejects_non_string(self): + import pytest + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext(inputs={"items": ["a"], "n": 3, "flag": True, "none": None}) + for name in ("items", "n", "flag", "none"): + with pytest.raises(ValueError, match="lower: expected a string value"): + evaluate_expression("{{ inputs." + name + " | lower }}", ctx) + + def test_filter_split(self): + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext( + inputs={ + "csv": "a,b,c", + "multi": "x::y::z", + "empty": "", + "nodelim": "abc", + "trailing": "a,b,", + } + ) + assert evaluate_expression("{{ inputs.csv | split(',') }}", ctx) == ["a", "b", "c"] + assert evaluate_expression("{{ inputs.multi | split('::') }}", ctx) == ["x", "y", "z"] + # An empty string is the one-element list [''], matching str.split. It + # must not be treated as a missing value or return []. + assert evaluate_expression("{{ inputs.empty | split(',') }}", ctx) == [""] + # A separator that never occurs yields the whole string, not []. + assert evaluate_expression("{{ inputs.nodelim | split(',') }}", ctx) == ["abc"] + # Trailing empty field is preserved, so round-tripping is lossless. + assert evaluate_expression("{{ inputs.trailing | split(',') }}", ctx) == ["a", "b", ""] + # split composes with join to convert delimiters. + assert evaluate_expression("{{ inputs.csv | split(',') | join('-') }}", ctx) == "a-b-c" + + def test_filter_split_rejects_non_string_inputs(self): + # Both the value and the separator must be strings. A non-string + # separator would otherwise leak a TypeError/AttributeError from + # str.split and crash the run. + import pytest + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext(inputs={"items": ["a", "b"], "csv": "a,b", "n": 3}) + with pytest.raises(ValueError, match="split: expected a string value"): + evaluate_expression("{{ inputs.items | split(',') }}", ctx) + with pytest.raises(ValueError, match="split: expected a string separator"): + evaluate_expression("{{ inputs.csv | split(5) }}", ctx) + + def test_filter_split_rejects_empty_separator(self): + # `str.split("")` raises the bare `ValueError: empty separator`, which + # names neither the filter nor the expression and escapes the evaluator + # as a raw Python error. An empty separator has no meaning, so it must + # be reported by the filter itself like every other misuse. + import pytest + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext(inputs={"csv": "a,b"}) + with pytest.raises(ValueError, match="split: separator must not be empty"): + evaluate_expression("{{ inputs.csv | split('') }}", ctx) + + def test_filter_length(self): + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext( + inputs={ + "items": ["a", "b", "c"], + "empty_list": [], + "s": "hello", + "empty_str": "", + } + ) + assert evaluate_expression("{{ inputs.items | length }}", ctx) == 3 + assert evaluate_expression("{{ inputs.empty_list | length }}", ctx) == 0 + # Strings count characters, as in Jinja2. + assert evaluate_expression("{{ inputs.s | length }}", ctx) == 5 + assert evaluate_expression("{{ inputs.empty_str | length }}", ctx) == 0 + # Composes with the rest of the chain, which is the motivating use. + assert evaluate_expression("{{ inputs.csv_len | length }}", StepContext(inputs={"csv_len": "a,b,c"})) == 5 + assert evaluate_expression("{{ inputs.s | split('l') | length }}", ctx) == 3 + + def test_filter_length_drives_conditions(self): + # The motivating use: `length` makes an emptiness check expressible as a + # condition, because 0 is falsy and any non-zero count is truthy. + from specify_cli.workflows.expressions import evaluate_condition + from specify_cli.workflows.base import StepContext + + ctx = StepContext(inputs={"items": ["a"], "empty": []}) + assert evaluate_condition("{{ inputs.items | length }}", ctx) is True + assert evaluate_condition("{{ inputs.empty | length }}", ctx) is False + + def test_filter_length_rejects_unsupported_types(self): + # length accepts only list and str. Mappings are excluded on purpose so + # `{{ obj | length }}` cannot silently mean "key count" for one shape and + # "value count" for another; other types are mis-wiring and must raise + # rather than coerce to 0 (which is indistinguishable from "empty"). + import pytest + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext( + inputs={"obj": {"a": 1}, "n": 3, "flag": True, "none": None, "ratio": 1.5} + ) + with pytest.raises(ValueError, match="length: expected a list or string"): + evaluate_expression("{{ inputs.obj | length }}", ctx) + for name in ("n", "flag", "none", "ratio"): + with pytest.raises(ValueError, match="length: expected a list or string"): + evaluate_expression("{{ inputs." + name + " | length }}", ctx) + + def test_filter_to_json(self): + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext( + inputs={ + "obj": {"b": 2, "a": 1}, + "items": [1, 2, 3], + "s": "hi", + "n": 3, + "flag": False, + "none": None, + "uni": "café", + } + ) + # sort_keys=True pins key order, so output does not depend on dict + # insertion order and stays byte-stable across runs. + assert evaluate_expression("{{ inputs.obj | to_json }}", ctx) == '{"a": 1, "b": 2}' + assert evaluate_expression("{{ inputs.items | to_json }}", ctx) == "[1, 2, 3]" + assert evaluate_expression("{{ inputs.s | to_json }}", ctx) == '"hi"' + assert evaluate_expression("{{ inputs.n | to_json }}", ctx) == "3" + assert evaluate_expression("{{ inputs.flag | to_json }}", ctx) == "false" + assert evaluate_expression("{{ inputs.none | to_json }}", ctx) == "null" + # ensure_ascii=False keeps non-ASCII readable instead of \uXXXX-escaped, + # so shell steps receive the original text. + assert evaluate_expression("{{ inputs.uni | to_json }}", ctx) == '"café"' + + def test_filter_to_json_round_trips_from_json(self): + # to_json is the inverse of from_json: a structured value survives an + # evaluator-level round trip. Nothing here exercises a shell, and the + # round trip is not extended to one — interpolation adds no quoting, so + # the output is reproducible but not shell-safe (see docs/reference/ + # workflows.md, "Interpolation and shell safety"). + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext( + inputs={"obj": {"items": [1, 2, 3]}}, + steps={"emit": {"output": {"stdout": '{"items": [1, 2, 3]}'}}}, + ) + assert evaluate_expression( + "{{ steps.emit.output.stdout | from_json | to_json }}", ctx + ) == '{"items": [1, 2, 3]}' + assert evaluate_expression( + "{{ inputs.obj | to_json | from_json }}", ctx + ) == {"items": [1, 2, 3]} + + def test_filter_to_json_rejects_non_serializable(self): + # A non-serializable value must raise a ValueError naming the filter, + # not leak a TypeError from json.dumps and crash the run. + import pytest + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext(inputs={"obj": {"f": object()}, "step": object()}) + with pytest.raises(ValueError, match="to_json: value is not JSON-serializable"): + evaluate_expression("{{ inputs.obj | to_json }}", ctx) + with pytest.raises(ValueError, match="to_json: value is not JSON-serializable"): + evaluate_expression("{{ inputs.step | to_json }}", ctx) + + def test_filter_to_json_rejects_non_finite_floats(self): + # json.dumps defaults to allow_nan=True, which would emit bare + # NaN/Infinity/-Infinity — none of them valid JSON. All three must + # take the ValueError path instead, both at the top level and when + # they are buried inside a container. + import pytest + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext( + inputs={ + "nan": float("nan"), + "inf": float("inf"), + "ninf": float("-inf"), + "nested": [float("nan")], + } + ) + for name in ("nan", "inf", "ninf", "nested"): + with pytest.raises( + ValueError, match="to_json: value is not JSON-serializable" + ): + evaluate_expression(f"{{{{ inputs.{name} | to_json }}}}", ctx) + + def test_filter_to_json_rejects_non_string_keys(self): + # JSON objects have string keys only. Without this check, json.dumps + # would coerce 1 to "1" -- colliding with an existing "1" key -- and + # sort_keys=True would raise an ordering TypeError on mixed key types, + # which surfaced as a generic "not JSON-serializable" hiding the real + # authoring mistake. + import pytest + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext( + inputs={ + "mixed": {1: "a", "2": "b"}, + "intkey": {1: "a"}, + "nested": {"outer": {3: "deep"}}, + "strkey": {"1": "a", "2": "b"}, + } + ) + for name in ("mixed", "intkey", "nested"): + with pytest.raises( + ValueError, match="to_json: mapping keys must be strings" + ): + evaluate_expression(f"{{{{ inputs.{name} | to_json }}}}", ctx) + # String keys, digit strings included, still serialize normally. + assert ( + evaluate_expression("{{ inputs.strkey | to_json }}", ctx) + == '{"1": "a", "2": "b"}' + ) + + def test_split_with_extra_argument_reports_unsupported_form(self): + # split is new in this PR, so main's multi-argument guard has to cover + # it as well. Without that it evaluated the argument fragment first and + # reported "expected a string separator, got NoneType" -- the extra + # argument, which is what the author actually got wrong, never + # surfaced. main already pins default() and join() the same way; this + # pins the filter added here. + import pytest + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext(inputs={"s": "a,b,c"}) + with pytest.raises( + ValueError, match="filter 'split' used in an unsupported form" + ): + evaluate_expression("{{ inputs.s | split(',', 1) }}", ctx) + # A comma inside quotes is still a single argument. + assert evaluate_expression("{{ inputs.s | split(',') }}", ctx) == [ + "a", + "b", + "c", + ] + + def test_zero_arg_filters_reject_miswired_forms(self): + # The strict no-argument branch is shared by from_json/upper/lower/ + # length/to_json. Every mis-wired form — parenthesized, accidental arg, + # or trailing garbage — must raise naming the filter, rather than + # silently falling through to the unknown-filter path. + import pytest + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext(inputs={"s": "hi", "items": ["a"]}) + for fname in ("from_json", "upper", "lower", "length", "to_json"): + for bad in (fname + "()", fname + "('x')", fname + ")", fname + " extra"): + with pytest.raises(ValueError, match=fname + ": expected"): + evaluate_expression( + "{{ inputs.s | " + 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 @@ -607,8 +902,8 @@ def test_filter_unknown_name_raises(self): 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) + with pytest.raises(ValueError, match="unknown filter 'truncate'"): + evaluate_expression("{{ inputs.items | truncate }}", ctx) def test_filter_unknown_name_with_args_raises(self): # The unknown-filter path must also catch the `name(arg)` form, which @@ -618,8 +913,8 @@ def test_filter_unknown_name_with_args_raises(self): 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) + with pytest.raises(ValueError, match="unknown filter 'truncate'"): + evaluate_expression("{{ inputs.text | truncate('x') }}", ctx) def test_filter_map_non_string_attr_raises(self): # A non-string attribute (authoring mistake like `map(5)`) must raise a @@ -667,7 +962,7 @@ def test_filter_contains_non_string_arg_on_list_ok(self): assert evaluate_expression("{{ inputs.nums | contains(9) }}", ctx) is False def test_registered_filters_unaffected(self): - # Regression: all five registered filters keep working unchanged. + # Regression: every registered filter keeps working unchanged. from specify_cli.workflows.expressions import evaluate_expression from specify_cli.workflows.base import StepContext @@ -677,6 +972,8 @@ def test_registered_filters_unaffected(self): "text": "hello world", "missing": "", "rows": [{"id": "a"}, {"id": "b"}], + "csv": "a,b,c", + "obj": {"n": 1}, }, steps={"emit": {"output": {"stdout": '{"n": 1}'}}}, ) @@ -691,6 +988,31 @@ def test_registered_filters_unaffected(self): assert evaluate_expression( "{{ steps.emit.output.stdout | from_json }}", ctx ) == {"n": 1} + assert evaluate_expression("{{ inputs.text | upper }}", ctx) == "HELLO WORLD" + assert evaluate_expression("{{ inputs.text | lower }}", ctx) == "hello world" + assert evaluate_expression("{{ inputs.csv | split(',') }}", ctx) == ["a", "b", "c"] + assert evaluate_expression("{{ inputs.tags | length }}", ctx) == 3 + assert evaluate_expression("{{ inputs.obj | to_json }}", ctx) == '{"n": 1}' + + def test_registered_filter_list_covers_every_implemented_filter(self): + # _REGISTERED_FILTERS drives the "known filter used in an unsupported + # form" message, so it must stay in sync with what is implemented: a + # registered-but-unimplemented name would be advertised in the expected + # list yet raise as unknown. + from specify_cli.workflows.expressions import _REGISTERED_FILTERS + + assert set(_REGISTERED_FILTERS) == { + "default", + "join", + "map", + "contains", + "from_json", + "upper", + "lower", + "split", + "length", + "to_json", + } def test_registered_filter_unsupported_form_raises(self): # A *registered* filter used in an unsupported form (e.g. `| join` with @@ -709,6 +1031,15 @@ def test_registered_filter_unsupported_form_raises(self): ValueError, match="filter 'map' used in an unsupported form" ): evaluate_expression("{{ inputs.tags | map }}", ctx) + # An arg-taking filter must not silently accept the wrong arity. + with pytest.raises( + ValueError, match="filter 'split' used in an unsupported form" + ): + evaluate_expression("{{ inputs.tags | split }}", ctx) + with pytest.raises( + ValueError, match="filter 'contains' used in an unsupported form" + ): + evaluate_expression("{{ inputs.tags | contains }}", ctx) def test_filter_call_with_trailing_tokens_fails_loudly(self): # A trailing operator/token after a filter's closing paren must not be diff --git a/tests/unit/test_condition_expression_block.py b/tests/unit/test_condition_expression_block.py index f13a830526..2085583c61 100644 --- a/tests/unit/test_condition_expression_block.py +++ b/tests/unit/test_condition_expression_block.py @@ -553,7 +553,7 @@ def test_incomplete_operand_covers_every_operator_the_evaluator_splits_on(): ("in inputs.tags", "missing an operand"), # leading word operator ("inputs.f(]", "brackets do not balance"), # matched count, wrong types ("inputs.f(]", "brackets do not balance"), - ("inputs.items | length", "the evaluator rejects it"), + ("inputs.items | truncate", "the evaluator rejects it"), ("inputs.tags | join", "used in an unsupported form"), ('he said "hi" then left', "is not a name the evaluator can resolve"), ("inputs.count+1", "is not a valid path segment"), @@ -612,10 +612,15 @@ def test_the_probe_reports_what_the_evaluator_reports(): Asking the evaluator removes that class: any filter used under an unknown name or in an unsupported form is reported by the code that will run. """ - assert _evaluator_rejects("inputs.items | length") is not None + assert _evaluator_rejects("inputs.items | truncate") is not None assert _evaluator_rejects("inputs.tags | join") is not None assert _evaluator_rejects("inputs.tags | join(',')") is None assert _evaluator_rejects("inputs.count > 100") is None + # A registered filter used in an unsupported form is a rejection too. + assert _evaluator_rejects("inputs.items | split") is not None + # Registered filters that are wired correctly are not. + assert _evaluator_rejects("inputs.items | length") is None + assert _evaluator_rejects("inputs.tags | split(',')") is None @pytest.mark.parametrize( @@ -671,7 +676,12 @@ def test_probe_value_errors_are_not_treated_as_rejections(condition): @pytest.mark.parametrize( "condition", - ["inputs.items | length", "inputs.tags | join"], + [ + "inputs.items | truncate", + "inputs.tags | join", + "inputs.items | split", + "inputs.text | upper('x')", + ], ) def test_filter_wiring_errors_are_still_rejections(condition): """The other half: a filter named wrong or used wrong is the author's text.""" diff --git a/workflows/ARCHITECTURE.md b/workflows/ARCHITECTURE.md index 1b06dc2e66..6070823a2a 100644 --- a/workflows/ARCHITECTURE.md +++ b/workflows/ARCHITECTURE.md @@ -118,14 +118,31 @@ Workflow definitions use Jinja2-like `{{ expression }}` syntax for dynamic value | Boolean logic | `and`, `or`, `not` | `{{ items and status == 'ok' }}` | | Membership | `in`, `not in` | `{{ 'error' not in status }}` | | Literals | strings, numbers, booleans, lists | `{{ true }}`, `{{ [1, 2] }}` | -| Filter: `default` | `{{ val \| default('fallback') }}` | Fallback for None/empty | +| Filter: `default` | `{{ val \| default('fallback') }}` | Fallback for `None` or an empty string | | Filter: `join` | `{{ list \| join(', ') }}` | Join list elements | | Filter: `contains` | `{{ text \| contains('sub') }}` | Substring/membership check | | Filter: `map` | `{{ list \| map('attr') }}` | Extract attribute from each item | | Filter: `from_json` | `{{ steps.emit.output.stdout \| from_json }}` | Parse a JSON string into a typed value (raises on invalid JSON) | +| Filter: `to_json` | `{{ obj \| to_json }}` | Serialize a value to a JSON string (inverse of `from_json`; mapping keys must be strings) | +| Filter: `upper` | `{{ text \| upper }}` | Uppercase a string (strings only) | +| Filter: `lower` | `{{ text \| lower }}` | Lowercase a string (strings only) | +| Filter: `split` | `{{ csv \| split(',') }}` | Split a string on a separator into a list | +| Filter: `length` | `{{ items \| length }}` | Length of a list or string (mappings rejected) | **Single expressions** (`{{ expr }}` only) return typed values. **Mixed templates** (`"text {{ expr }} more"`) return interpolated strings. +**Filter argument strictness (new filters).** The five filters added in #4766 validate their supported inputs and raise `ValueError` naming the problem rather than leaking a Python `TypeError`/`AttributeError`: `upper`/`lower` accept only strings, `split` requires a string value and a non-empty string separator, `length` accepts only lists and strings, and `to_json` requires a JSON-serializable value whose mapping keys are strings, and additionally rejects non-finite floats (`NaN`, `Infinity`, `-Infinity`), which `json.dumps` would otherwise emit as bare tokens that are not valid JSON. Keys are checked before serialization because `json.dumps` would otherwise coerce `1` to `"1"` (colliding with an existing `"1"` key), while `sort_keys=True` raises an ordering `TypeError` on mixed key types — both surfacing as a generic "not JSON-serializable" that hides the authoring mistake. Coercion is deliberately rejected for these filters: a type mismatch almost always means the pipeline is wired to the wrong variable, and a coerced result (e.g. `"3"` for an int) would hide that. Arity is checked before the argument expression is evaluated, so a filter used with the wrong number of arguments — `| upper('x')`, `| split` with no separator, `| split(',', 1)`, `| join` bare — is reported as a known filter misused, distinct from an entirely unknown name. + +The older filters are more permissive and are unchanged: `join` stringifies unsupported value shapes and elements, and `map`/`contains` return fallback values for unsupported inputs rather than raising. + +**`to_json` output is deterministic.** It pins `sort_keys=True` and `ensure_ascii=False`, so the same value always serializes to the same bytes regardless of dict insertion order or platform, and non-ASCII text is not `\uXXXX`-escaped. This buys reproducibility only — it does **not** make the result safe to pass through a shell, because expression interpolation adds no quoting or escaping. See [Interpolation and shell safety](../docs/reference/workflows.md#interpolation-and-shell-safety) before interpolating JSON into a `run` field. + +**Filters and comparisons.** The parser splits on the top-level `|` before looking for operators, so a comparison or other trailing token after a filter is rejected as ambiguous rather than silently evaluated (`{{ items | default(0) > 5 }}` raises) — the same holds for the new filters. The count a `length` filter would produce does not exist before the filter runs, so there is no way to write `{{ items | length > 0 }}`; the only supported branching form is the filtered value's own truthiness in a `condition:`, since `length` returns `0` for an empty input: + +```yaml +condition: "{{ inputs.items | length }}" # 0 -> False, any non-zero count -> True +``` + ### Namespace The expression evaluator builds a namespace from the `StepContext`: diff --git a/workflows/README.md b/workflows/README.md index da045bfdf9..ccca55aee9 100644 --- a/workflows/README.md +++ b/workflows/README.md @@ -406,9 +406,26 @@ condition: "{{ steps.run-tests.output.exit_code != 0 }}" # Filters message: "{{ status | default('pending') }}" +ids: "{{ rows | split(',') | length }}" ``` -Supported filters: `default`, `join`, `contains`, `map`, `from_json`. +Supported filters: `default`, `join`, `contains`, `map`, `from_json`, `to_json`, `upper`, `lower`, `split`, `length`. + +The new filters validate their input types and raise a `ValueError` naming +the problem instead of coercing — so `upper`/`lower` take strings only, `split` +takes a string value and a non-empty string separator, `length` accepts +lists and strings but rejects mappings, and `to_json` rejects what it cannot +serialize: non-finite floats (`NaN`, `Infinity`, `-Infinity`), which would +otherwise serialize to tokens that are not valid JSON, and mapping keys that +are not strings, which JSON objects cannot carry. A filter used with the wrong +number of +arguments +(`| upper('x')`, `| split(',', 1)`, bare `| split`) is reported as a known +filter misused, which is +distinct from an entirely unknown filter name. The older filters are more +permissive and unchanged: `join` stringifies unsupported values, and +`map`/`contains` return fallbacks rather than raising. See +[ARCHITECTURE.md](ARCHITECTURE.md#expression-evaluation) for the full table. ### Runtime Context