Skip to content

Commit 31928a4

Browse files
fix(workflows): validate to_json keys and filter arity before evaluating
Both findings from the latest review round hid the real authoring mistake behind a less useful error. to_json relied on sort_keys=True for determinism, so a mapping with mixed key types raised an ordering TypeError that the generic handler reported as "not JSON-serializable", while a mapping with a lone integer key was silently coerced to "1" and could collide with an existing "1" key. JSON objects have string keys, so keys are validated before json.dumps is reached and the message names the key type instead. The walk is iterative with a seen set, leaving circular structures for json.dumps to report. split(',', 1) evaluated the argument expression first. The evaluator has no comma operator, so the fragment evaluated to None and the reported error was "expected a string separator, got NoneType" -- the extra argument never surfaced. Arity for the single-argument filters is now counted on the top-level comma split, before evaluation, so the message names the filter and the argument count. Documented in workflows/README.md, workflows/ARCHITECTURE.md, and docs/reference/workflows.md, with a regression test for each behavior. Assisted-by: opencode (model: mimo-v2.6-flash-free, autonomous)
1 parent 64d1217 commit 31928a4

5 files changed

Lines changed: 121 additions & 10 deletions

File tree

‎docs/reference/workflows.md‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -599,17 +599,18 @@ Available filters: `default`, `join`, `contains`, `map`, `from_json`, `to_json`,
599599
| `contains`| `{{ text \| contains('sub') }}` | Substring or membership check |
600600
| `map` | `{{ list \| map('attr') }}` | Extract an attribute from each item |
601601
| `from_json`| `{{ out \| from_json }}` | Parse a JSON string into a typed value |
602-
| `to_json`| `{{ obj \| to_json }}` | Serialize a value to a JSON string — the inverse of `from_json` |
602+
| `to_json`| `{{ obj \| to_json }}` | Serialize a value to a JSON string — the inverse of `from_json`; mapping keys must be strings |
603603
| `upper` | `{{ text \| upper }}` | Uppercase a string |
604604
| `lower` | `{{ text \| lower }}` | Lowercase a string |
605605
| `split` | `{{ csv \| split(',') }}` | Split a string on a separator into a list of strings |
606606
| `length` | `{{ items \| length }}` | Number of elements in a list, or characters in a string |
607607

608608
Notes on the newer filters:
609609

610-
- **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) is reported as a known filter misused, which is distinct from an entirely unknown filter name. The older filters (`join`, `map`, `contains`) are more permissive and unchanged: `join` stringifies unsupported values and `map`/`contains` return fallbacks rather than raising.
610+
- **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; arity is checked before the argument expression is evaluated, so the extra argument is what gets named. The older filters (`join`, `map`, `contains`) are more permissive and unchanged: `join` stringifies unsupported values and `map`/`contains` return fallbacks rather than raising.
611611
- **`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.
612612
- **`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.
613+
- **`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".
613614
- **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:
614615

615616
```yaml

‎src/specify_cli/workflows/expressions.py‎

Lines changed: 56 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -232,13 +232,52 @@ def _filter_to_json(value: Any) -> str:
232232
is what makes that true for non-finite floats: ``json.dumps`` would
233233
otherwise emit bare ``NaN``/``Infinity``/``-Infinity``, none of which is
234234
valid JSON, and hand downstream parsers a string they must reject.
235+
236+
Mapping keys must be strings, which is what JSON objects have anyway.
237+
``_check_json_keys`` enforces that before ``json.dumps`` is reached, so a
238+
non-string key is reported as the authoring mistake it is rather than
239+
surfacing as an ordering ``TypeError`` from ``sort_keys=True`` (mixed key
240+
types) or as silent ``1`` → ``"1"`` coercion that collides with an existing
241+
``"1"`` key.
235242
"""
243+
_check_json_keys(value)
236244
try:
237245
return json.dumps(value, sort_keys=True, ensure_ascii=False, allow_nan=False)
238246
except (TypeError, ValueError) as exc:
239247
raise ValueError(f"to_json: value is not JSON-serializable: {exc}") from exc
240248

241249

250+
def _check_json_keys(value: Any) -> None:
251+
"""Raise ``ValueError`` if any mapping reachable from *value* has a
252+
non-string key.
253+
254+
Walked iteratively with a ``seen`` set: a self-referential structure is
255+
skipped rather than recursed into, so the circular reference stays for
256+
``json.dumps`` to report with its own clearer message instead of the walk
257+
exhausting the stack first.
258+
"""
259+
seen: set[int] = set()
260+
stack: list[Any] = [value]
261+
while stack:
262+
item = stack.pop()
263+
if isinstance(item, dict):
264+
if id(item) in seen:
265+
continue
266+
seen.add(id(item))
267+
for key, sub_value in item.items():
268+
if not isinstance(key, str):
269+
raise ValueError(
270+
"to_json: mapping keys must be strings, got "
271+
f"{type(key).__name__}: {key!r}"
272+
)
273+
stack.append(sub_value)
274+
elif isinstance(item, (list, tuple)):
275+
if id(item) in seen:
276+
continue
277+
seen.add(id(item))
278+
stack.extend(item)
279+
280+
242281
# Filters that take no arguments and tolerate no trailing tokens. Keyed by name
243282
# so ``_apply_filter`` can recognize a mis-wired form of any of them from the
244283
# leading filter name alone, instead of each needing its own branch. ``default``
@@ -252,6 +291,14 @@ def _filter_to_json(value: Any) -> str:
252291
"to_json": _filter_to_json,
253292
}
254293

294+
# Parenthesized filters that take exactly one argument. Used to report an
295+
# extra argument by name *before* the argument expression is evaluated:
296+
# _evaluate_simple_expression has no comma operator, so ``| split(',', 1)``
297+
# would otherwise hand split an unparseable fragment, which reads back as
298+
# "expected a string separator, got NoneType" -- the arity mistake, which is
299+
# what the author actually got wrong, never surfaces.
300+
_SINGLE_ARG_FILTERS = frozenset({"default", "join", "map", "contains", "split"})
301+
255302

256303
# -- Expression resolution ------------------------------------------------
257304

@@ -612,7 +659,15 @@ def _apply_filter(value: Any, filter_expr: str, namespace: dict[str, Any]) -> An
612659
filter_match = re.fullmatch(r"(\w+)\((.+)\)", filter_expr)
613660
if filter_match:
614661
fname = filter_match.group(1)
615-
farg = _evaluate_simple_expression(filter_match.group(2).strip(), namespace)
662+
farg_text = filter_match.group(2).strip()
663+
if fname in _SINGLE_ARG_FILTERS:
664+
arg_parts = _split_top_level_commas(farg_text)
665+
if len(arg_parts) > 1:
666+
raise ValueError(
667+
f"{fname}: expected exactly one argument, got "
668+
f"{len(arg_parts)}: '| {filter_expr}'"
669+
)
670+
farg = _evaluate_simple_expression(farg_text, namespace)
616671
if fname == "default":
617672
return _filter_default(value, farg)
618673
if fname == "join":

‎tests/test_workflows.py‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -824,6 +824,59 @@ def test_filter_to_json_rejects_non_finite_floats(self):
824824
):
825825
evaluate_expression(f"{{{{ inputs.{name} | to_json }}}}", ctx)
826826

827+
def test_filter_to_json_rejects_non_string_keys(self):
828+
# JSON objects have string keys only. Without this check, json.dumps
829+
# would coerce 1 to "1" -- colliding with an existing "1" key -- and
830+
# sort_keys=True would raise an ordering TypeError on mixed key types,
831+
# which surfaced as a generic "not JSON-serializable" hiding the real
832+
# authoring mistake.
833+
import pytest
834+
from specify_cli.workflows.expressions import evaluate_expression
835+
from specify_cli.workflows.base import StepContext
836+
837+
ctx = StepContext(
838+
inputs={
839+
"mixed": {1: "a", "2": "b"},
840+
"intkey": {1: "a"},
841+
"nested": {"outer": {3: "deep"}},
842+
"strkey": {"1": "a", "2": "b"},
843+
}
844+
)
845+
for name in ("mixed", "intkey", "nested"):
846+
with pytest.raises(
847+
ValueError, match="to_json: mapping keys must be strings"
848+
):
849+
evaluate_expression(f"{{{{ inputs.{name} | to_json }}}}", ctx)
850+
# String keys, digit strings included, still serialize normally.
851+
assert (
852+
evaluate_expression("{{ inputs.strkey | to_json }}", ctx)
853+
== '{"1": "a", "2": "b"}'
854+
)
855+
856+
def test_single_arg_filters_reject_extra_argument(self):
857+
# Arity is checked before the argument expression is evaluated.
858+
# split(',', 1) used to evaluate that fragment first, so the reported
859+
# error was "expected a string separator, got NoneType" -- the arity
860+
# mistake, which is what the author got wrong, stayed hidden behind a
861+
# type error.
862+
import pytest
863+
from specify_cli.workflows.expressions import evaluate_expression
864+
from specify_cli.workflows.base import StepContext
865+
866+
ctx = StepContext(inputs={"s": "a,b,c", "n": 0})
867+
with pytest.raises(
868+
ValueError, match="split: expected exactly one argument, got 2"
869+
):
870+
evaluate_expression("{{ inputs.s | split(',', 1) }}", ctx)
871+
with pytest.raises(
872+
ValueError, match="default: expected exactly one argument, got 2"
873+
):
874+
evaluate_expression("{{ inputs.n | default(0, 'x') }}", ctx)
875+
# A comma inside quotes or brackets is still one argument.
876+
assert (
877+
evaluate_expression("{{ inputs.s | split(',') }}", ctx) == ["a", "b", "c"]
878+
)
879+
827880
def test_zero_arg_filters_reject_miswired_forms(self):
828881
# The strict no-argument branch is shared by from_json/upper/lower/
829882
# length/to_json. Every mis-wired form — parenthesized, accidental arg,

‎workflows/ARCHITECTURE.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -123,15 +123,15 @@ Workflow definitions use Jinja2-like `{{ expression }}` syntax for dynamic value
123123
| Filter: `contains` | `{{ text \| contains('sub') }}` | Substring/membership check |
124124
| Filter: `map` | `{{ list \| map('attr') }}` | Extract attribute from each item |
125125
| Filter: `from_json` | `{{ steps.emit.output.stdout \| from_json }}` | Parse a JSON string into a typed value (raises on invalid JSON) |
126-
| Filter: `to_json` | `{{ obj \| to_json }}` | Serialize any value to a JSON string (inverse of `from_json`) |
126+
| Filter: `to_json` | `{{ obj \| to_json }}` | Serialize a value to a JSON string (inverse of `from_json`; mapping keys must be strings) |
127127
| Filter: `upper` | `{{ text \| upper }}` | Uppercase a string (strings only) |
128128
| Filter: `lower` | `{{ text \| lower }}` | Lowercase a string (strings only) |
129129
| Filter: `split` | `{{ csv \| split(',') }}` | Split a string on a separator into a list |
130130
| Filter: `length` | `{{ items \| length }}` | Length of a list or string (mappings rejected) |
131131

132132
**Single expressions** (`{{ expr }}` only) return typed values. **Mixed templates** (`"text {{ expr }} more"`) return interpolated strings.
133133

134-
**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 and additionally rejects non-finite floats (`NaN`, `Infinity`, `-Infinity`), which `json.dumps` would otherwise emit as bare tokens that are not valid JSON. 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. A filter used with the wrong arity — `| upper('x')`, `| split` with no separator, `| join` bare — is reported as a known filter misused, distinct from an entirely unknown name.
134+
**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.
135135

136136
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.
137137

‎workflows/README.md‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -414,12 +414,14 @@ Supported filters: `default`, `join`, `contains`, `map`, `from_json`, `to_json`,
414414
The new filters validate their input types and raise a `ValueError` naming
415415
the problem instead of coercing — so `upper`/`lower` take strings only, `split`
416416
takes a string value and a non-empty string separator, `length` accepts
417-
lists and strings but rejects mappings, and `to_json` rejects values it
418-
cannot serialize — including non-finite floats (`NaN`, `Infinity`,
419-
`-Infinity`), which would otherwise serialize to tokens that are not valid
420-
JSON. A filter used with the wrong number of
417+
lists and strings but rejects mappings, and `to_json` rejects what it cannot
418+
serialize: non-finite floats (`NaN`, `Infinity`, `-Infinity`), which would
419+
otherwise serialize to tokens that are not valid JSON, and mapping keys that
420+
are not strings, which JSON objects cannot carry. A filter used with the wrong
421+
number of
421422
arguments
422-
(`| upper('x')`, bare `| split`) is reported as a known filter misused, which is
423+
(`| upper('x')`, `| split(',', 1)`, bare `| split`) is reported as a known
424+
filter misused, which is
423425
distinct from an entirely unknown filter name. The older filters are more
424426
permissive and unchanged: `join` stringifies unsupported values, and
425427
`map`/`contains` return fallbacks rather than raising. See

0 commit comments

Comments
 (0)