diff --git a/src/specify_cli/workflows/expressions.py b/src/specify_cli/workflows/expressions.py index 198010838e..fe8098f321 100644 --- a/src/specify_cli/workflows/expressions.py +++ b/src/specify_cli/workflows/expressions.py @@ -9,6 +9,7 @@ import json import re +from contextvars import ContextVar from typing import Any @@ -134,6 +135,15 @@ def _filter_from_json(value: Any) -> Any: _EXPR_PATTERN = re.compile(r"\{\{(.+?)\}\}") +# The one definition of an indexed path segment. _resolve_dot_path matches +# against it, and the condition gate below reuses it rather than describing the +# same shape a second time, so widening what indexing accepts cannot leave the +# evaluator and the gate disagreeing. +_INDEXED_SEGMENT = re.compile(r"^([\w-]+)\[(\d+)\]$") + +_PLAIN_SEGMENT = re.compile(r"^[\w-]+$") + + def _resolve_dot_path(obj: Any, path: str) -> Any: """Resolve a dotted path like ``steps.specify.output.file`` against *obj*. @@ -143,7 +153,7 @@ def _resolve_dot_path(obj: Any, path: str) -> Any: current = obj for part in parts: # Handle list indexing: name[0] - idx_match = re.match(r"^([\w-]+)\[(\d+)\]$", part) + idx_match = _INDEXED_SEGMENT.match(part) if idx_match: key, idx = idx_match.group(1), int(idx_match.group(2)) if isinstance(current, dict): @@ -479,6 +489,11 @@ def _apply_filter(value: Any, filter_expr: str, namespace: dict[str, Any]) -> An # evaluator will actually split on. _COMPARISON_OPERATORS = ("!=", "==", ">=", "<=", ">", "<", " not in ", " in ") +# Set only while `_collect_leaves` probes an expression; None everywhere else, so +# a normal evaluation costs one `.get()`. A ContextVar rather than a module global +# so concurrent probes cannot append into each other's list. +_leaf_sink: ContextVar[list[str] | None] = ContextVar("_leaf_sink", default=None) + def _evaluate_simple_expression(expr: str, namespace: dict[str, Any]) -> Any: """Evaluate a simple expression against the namespace. @@ -545,8 +560,27 @@ def _evaluate_simple_expression(expr: str, namespace: dict[str, Any]) -> Any: f"operand in its own expression instead." ) value = _evaluate_simple_expression(head, namespace) + sink = _leaf_sink.get() for segment in segments[1:]: - value = _apply_filter(value, segment.strip(), namespace) + if sink is None: + value = _apply_filter(value, segment.strip(), namespace) + continue + # Probing. A filter handed a placeholder can raise on it -- from_json + # on a mapping is the common one -- and letting that end the walk + # hides every leaf further along the chain, which is the one thing + # this collection exists to report: `inputs.blob | from_json | + # contains(bogus)` recorded `inputs.blob` and stopped, so `bogus` + # was never offered to _unresolvable_leaf. _apply_filter evaluates a + # filter's argument before it can raise on the value, so the leaves + # of the failing segment are already recorded when we get here. + # Carry a fresh placeholder so the next filter sees the same kind of + # unknown the namespace hands out. Real evaluation is untouched: the + # sink is armed only by _collect_leaves, and _evaluator_rejects runs + # its own probe without it, so a mis-wired filter is still reported. + try: + value = _apply_filter(value, segment.strip(), namespace) + except Exception: # noqa: BLE001 - probe values, not the author's text + value = _ProbeNamespace() return value # Boolean operators — parse 'or' first (lower precedence) so that @@ -627,7 +661,12 @@ def _evaluate_simple_expression(expr: str, namespace: dict[str, Any]) -> Any: ] return items - # Variable reference (dot-path) + # Variable reference (dot-path). This is the one place a substring stops being + # grammar and becomes a name to resolve, so it is where a probe can learn what + # the evaluator will actually look up. Literals have all returned above. + sink = _leaf_sink.get() + if sink is not None: + sink.append(expr) return _resolve_dot_path(namespace, expr) @@ -1047,8 +1086,9 @@ def _has_incomplete_operand(text: str) -> bool: # None, so a correction built on one turns a truthy condition false. _NAMESPACE_ROOTS = ("inputs", "steps", "item", "fan_in", "context") -# Exactly what _resolve_dot_path accepts: a name, optionally one numeric index. -_PATH_SEGMENT = re.compile(r"^[\w-]+(\[\d+\])?$") +def _is_path_segment(segment: str) -> bool: + """Whether _resolve_dot_path can walk *segment*: a name, or a name it indexes.""" + return bool(_PLAIN_SEGMENT.match(segment) or _INDEXED_SEGMENT.match(segment)) class _ProbeNamespace(dict): @@ -1097,115 +1137,43 @@ def _evaluator_rejects(text: str) -> str | None: -def _looks_numeric(text: str) -> bool: - """Mirror the evaluator's numeric literal test exactly. - - `_evaluate_simple_expression` only calls `float()` when a `.` is present and - `int()` otherwise, so `1e3` is not a number to it -- it falls through to a path - lookup and resolves to None. A bare `float()` here accepted `1e3` and the - correction turned a truthy condition false. - """ - try: - if "." in text: - float(text) - else: - int(text) - except (ValueError, TypeError): - return False - return True - +def _collect_leaves(text: str) -> list[str]: + """Every substring *text* hands to the evaluator as a name to resolve. -def _is_literal(text: str) -> bool: - """Mirror the evaluator's literal tests exactly. + Runs the same probe ``_evaluator_rejects`` uses, with the leaf sink armed. + Literals never reach the dot-path resolution, and operands, filter arguments + and list elements all do -- ``_evaluate_simple_expression`` evaluates both + sides of ``or``/``and`` eagerly rather than short-circuiting, so a leaf is + recorded whatever the other side is worth. - The string case is the opening quote's *matching close being the final - character*, not first/last-character equality: `'a' 'b'` passes the latter but - is two literals to the evaluator, which falls through to a path lookup. + A probe run can still raise on its own placeholder values, which is what + ``_evaluator_rejects`` sorts out. The leaves seen before that point are real + -- the evaluator reached them -- so they are kept rather than discarded: + ``inputs.tags | join(bogus)`` records ``bogus`` and only then trips over the + placeholder handed to ``join``. """ - if text[:1] in ("'", '"') and text.find(text[0], 1) == len(text) - 1: - return True - return text.lower() in ("true", "false", "none", "null") or _looks_numeric(text) - + leaves: list[str] = [] + token = _leaf_sink.set(leaves) + try: + _evaluate_simple_expression( + text, {root: _ProbeNamespace() for root in _NAMESPACE_ROOTS} + ) + except Exception: # noqa: BLE001 - probe values, reported by _evaluator_rejects + pass + finally: + _leaf_sink.reset(token) + return leaves -def _unresolvable_term(text: str) -> str | None: - """The first operand in *text* the evaluator cannot resolve, or ``None``. - Walks operands the way ``_evaluate_simple_expression`` does -- filters, then - ``or``/``and``/``not``, then comparisons -- and checks each leaf. A leaf must be - a literal or a dotted path rooted in ``_NAMESPACE_ROOTS``. +def _unresolvable_leaf(leaf: str) -> str | None: + """Why the evaluator cannot resolve the name *leaf*, or ``None``. - Enumerating broken shapes is what made this take several rounds: each new gate - only knew the shapes named so far. ``inputs.a === inputs.b`` split cleanly on - ``==`` and looked complete, while the evaluator read ``= inputs.b`` as a path - and resolved it to ``None``; ``bogus == 'x'`` passed for the same reason one - level up. Recursing to the leaves covers both without naming either. + Namespace knowledge only. Everything about where operands live now comes from + the evaluator itself, so nothing here restates the grammar. """ - stripped = text.strip() - if not stripped: - return "an operand is empty" - - if _find_top_level(stripped, "|") != -1: - segments = _split_top_level(stripped, "|") - reason = _unresolvable_term(segments[0]) - if reason is not None: - return reason - # A filter argument is an ordinary operand to `_apply_filter`, which - # evaluates it with `_evaluate_simple_expression` like any other. Skipping - # it let `inputs.tags | join(bogus)` be offered as paste-ready: `bogus` is - # no namespace root, resolves to None, and the wrapped form then raises - # `join: expected a string separator, got NoneType`. Parse with the same - # pattern `_apply_filter` uses, so a form this does not recognize is left - # to the evaluator probe rather than guessed at here. - for segment in segments[1:]: - match = re.fullmatch(r"(\w+)\((.+)\)", segment.strip()) - if match is None: - continue - reason = _unresolvable_term(match.group(2)) - if reason is not None: - return reason - return None - - for op in (" or ", " and "): - idx = _find_top_level(stripped, op) - if idx != -1: - return _unresolvable_term(stripped[:idx]) or _unresolvable_term( - stripped[idx + len(op):] - ) - - if stripped.startswith("not "): - return _unresolvable_term(stripped[4:]) - - for op in _COMPARISON_OPERATORS: - idx = _find_top_level(stripped, op) - if idx != -1: - return _unresolvable_term(stripped[:idx]) or _unresolvable_term( - stripped[idx + len(op):] - ) - - if _is_literal(stripped): - return None - - # A list literal is a term the evaluator understands, and it recurses into the - # elements rather than resolving the brackets as a name. Not mirroring that - # denied the correction to `inputs.tag in ['x', 'y']` -- a condition wrapping - # repairs completely -- while reporting the list as an unresolvable name. The - # empty-segment skip matches `_evaluate_simple_expression`, which drops them so - # `[1, 2,]` is `[1, 2]` rather than `[1, 2, None]`. - if stripped.startswith("[") and stripped.endswith("]"): - inner = stripped[1:-1].strip() - if not inner: - return None - for element in _split_top_level_commas(inner): - if not element.strip(): - continue - reason = _unresolvable_term(element) - if reason is not None: - return reason - return None - - segments = _split_top_level(stripped, ".") - if not _PATH_SEGMENT.match(segments[0].strip()): - return f"{stripped!r} is not a name the evaluator can resolve" + segments = _split_top_level(leaf, ".") + if not _is_path_segment(segments[0].strip()): + return f"{leaf!r} is not a name the evaluator can resolve" # `item` is the only root that is not always a mapping: `StepContext.item` is # `Any` and a fan-out assigns the item value itself, so when that value is a # list `_resolve_dot_path` indexes it and `item[0] == 'x'` resolves. Every @@ -1213,7 +1181,7 @@ def _unresolvable_term(text: str) -> str | None: # branch returns None for those however it is written -- so the index is # stripped for `item` alone rather than for roots in general. root = segments[0].strip() - indexed_root = re.fullmatch(r"([\w-]+)\[\d+\]", root) + indexed_root = _INDEXED_SEGMENT.match(root) if indexed_root is not None and indexed_root.group(1) == "item": root = indexed_root.group(1) if root not in _NAMESPACE_ROOTS: @@ -1222,11 +1190,38 @@ def _unresolvable_term(text: str) -> str | None: f"({', '.join(_NAMESPACE_ROOTS)})" ) for segment in segments[1:]: - if not _PATH_SEGMENT.match(segment.strip()): + if not _is_path_segment(segment.strip()): return f"{segment.strip()!r} is not a valid path segment" return None +def _unresolvable_term(text: str) -> str | None: + """The first name in *text* the evaluator cannot resolve, or ``None``. + + Asks the evaluator which names it will look up, then applies the namespace + rules to those. The previous implementation derived the names itself by + re-walking the grammar -- filters, then ``or``/``and``/``not``, then + comparisons, then list literals -- and had to be kept in step with + ``_evaluate_simple_expression`` by hand. + + That is what made this take several rounds: each round fixed one shape the + walk disagreed about (``inputs.a === inputs.b``, ``bogus == 'x'``, list + elements, filter arguments, a newline before ``and``) and nothing stopped the + next one. Reading the leaves off the evaluator removes the class rather than + another instance of it: the two cannot disagree about where the operands are + when only one of them decides. + """ + stripped = text.strip() + if not stripped: + return "an operand is empty" + + for leaf in _collect_leaves(stripped): + reason = _unresolvable_leaf(leaf) + if reason is not None: + return reason + return None + + def _wrapping_would_not_repair(core: str) -> str | None: """Why wrapping *core* in ``{{ }}`` would not yield the expression intended. diff --git a/tests/unit/test_condition_expression_block.py b/tests/unit/test_condition_expression_block.py index 0739a2bc29..7a30a2f42f 100644 --- a/tests/unit/test_condition_expression_block.py +++ b/tests/unit/test_condition_expression_block.py @@ -1,8 +1,11 @@ """A string condition with no ``{{ }}`` block is never evaluated (always true).""" +import re + import pytest import yaml +from specify_cli.workflows import expressions from specify_cli.workflows.base import StepContext from specify_cli.workflows.expressions import ( condition_has_malformed_expression_block, @@ -15,8 +18,9 @@ _has_unbalanced_bracket, _has_incomplete_operand, _unresolvable_term, + _collect_leaves, + _leaf_sink, _evaluator_rejects, - _is_literal, _strip_stray_delimiters, _COMPARISON_OPERATORS, _WORD_OPERATORS, @@ -676,26 +680,36 @@ def test_filter_wiring_errors_are_still_rejections(condition): @pytest.mark.parametrize( - "condition,literal", + "condition,resolvable", [ ("42", True), ("3.14", True), ("-7", True), # `1e3` has no "." so the evaluator calls int() on it, which fails; it then - # falls through to a path lookup. float() alone accepted it here. + # falls through to a path lookup and resolves to None. The gate used to + # decide this for itself with a float() test that accepted `1e3`, and the + # correction turned a truthy condition false. Now the evaluator reaches the + # dot-path resolution with `1e3` and reports it, so the two cannot disagree. ("1e3", False), ("'one'", True), ('"one"', True), # Two literals, not one: the evaluator requires the opening quote's match to # be the final character, which first/last-character equality does not. ("'a' 'b'", False), - ("'a' == 'b'", False), + ("'a' == 'b'", True), ("true", True), - ("inputs.name", False), + ("inputs.name", True), + ("bogus", False), ], ) -def test_literal_test_mirrors_the_evaluator(condition, literal): - assert _is_literal(condition) is literal +def test_literal_handling_comes_from_the_evaluator(condition, resolvable): + """What `_is_literal` used to assert, asserted through the gate instead. + + The helper existed only to restate the evaluator's literal tests, and its test + could pass while the two had drifted. Asking whether the gate accepts the + condition tests the property that actually matters. + """ + assert (_unresolvable_term(condition) is None) is resolvable @pytest.mark.parametrize( @@ -827,3 +841,142 @@ def test_indexing_an_always_mapping_root_still_loses_the_correction(condition): ctx = StepContext(inputs={"a": 1}, item=["x", "y"]) assert CORRECTION_OFFERED not in format_condition_remediation(condition) assert evaluate_condition("{{ " + condition + " }}", ctx) is False + + +# --- what the leaf sink rests on ---------------------------------------------- + + +def test_the_sink_is_off_outside_a_probe(): + """A normal evaluation must not pay for, or be observed by, the gate.""" + assert _leaf_sink.get() is None + + evaluate_expression("{{ inputs.name }}", StepContext(inputs={"name": "x"})) + + assert _leaf_sink.get() is None + + +def test_the_sink_is_cleared_even_when_the_probe_raises(): + """`_collect_leaves` swallows probe errors; it must still reset the var.""" + _collect_leaves("inputs.tags | nosuchfilter") + + assert _leaf_sink.get() is None + + +def test_both_sides_of_a_boolean_are_reported(): + """The load-bearing property: `_evaluate_simple_expression` evaluates both + operands of `and`/`or` and only then combines them. If it ever + short-circuits, the gate would stop seeing the right-hand operand and go + quietly blind -- so assert it here rather than rely on it silently. + """ + assert _collect_leaves("inputs.a or bogus") == ["inputs.a", "bogus"] + assert _collect_leaves("false and bogus") == ["bogus"] + assert _unresolvable_term("false and bogus") is not None + + +def test_leaves_seen_before_a_probe_error_are_kept(): + """The probe hands `join` a placeholder and it raises. The leaves reached + before that are real, so discarding them would lose `bogus` -- the filter + argument case an earlier round of #4230 had to add by hand. + """ + assert "bogus" in _collect_leaves("inputs.tags | join(bogus)") + assert _unresolvable_term("inputs.tags | join(bogus)") is not None + + +def test_a_probe_error_does_not_end_the_filter_chain(): + """Keeping the leaves seen so far is not enough -- the walk has to go on. + + `from_json` receives the probe's placeholder mapping and raises. Stopping + there loses every leaf further along the chain, which is the one thing this + collection exists to report, and it is a step *backwards* from the + hand-written walk this refactor replaces: that walk read `bogus` straight + out of its own grammar rules and reported it. + """ + expr = "inputs.blob | from_json | contains(bogus)" + + assert _collect_leaves(expr) == ["inputs.blob", "bogus"] + assert _unresolvable_term(expr) is not None + + +def test_every_later_link_of_a_chain_is_still_walked(): + """Not merely the next link: two failing filters must not hide the third.""" + expr = "inputs.blob | from_json | map(bogus) | join(alsobogus)" + + leaves = _collect_leaves(expr) + + assert "bogus" in leaves + assert "alsobogus" in leaves + assert _unresolvable_term(expr) is not None + + +def test_only_the_probe_carries_on_past_a_filter_error(): + """The continue-on-error is armed by the sink and nothing else. + + A real evaluation must still fail loudly: `_apply_filter` raises rather than + return the unfiltered value precisely so a mis-wired filter cannot become a + quietly wrong answer, and the probe must not soften that. + """ + context = StepContext(inputs={"blob": "not json", "tags": ["a"]}) + + with pytest.raises(ValueError, match="invalid JSON"): + evaluate_expression("{{ inputs.blob | from_json | contains('x') }}", context) + + with pytest.raises(ValueError, match="unknown filter"): + evaluate_expression("{{ inputs.tags | nosuchfilter }}", context) + + # And the rejection probe, which runs without the sink, still reports it. + assert _evaluator_rejects("inputs.tags | nosuchfilter") is not None + + +def test_a_literal_never_reaches_the_resolver(): + """Why the gate needs no literal test of its own any more.""" + assert _collect_leaves("'a literal'") == [] + assert _collect_leaves("42") == [] + assert _collect_leaves("true") == [] + + +# --- The gate reads the evaluator's definitions, it does not restate them --- +# +# The point of this refactor is that widening what the evaluator accepts reaches +# the validation gate for free. That is easy to claim and easy to lose: a second +# copy of the grammar in the gate keeps every existing test green while silently +# reintroducing the drift. These two pin the wiring by moving the evaluator's own +# definitions and asserting the gate follows. + + +def test_gate_reads_the_shared_indexed_segment_definition(monkeypatch): + """Widening `_INDEXED_SEGMENT` alone must reach the gate. + + `steps.…​.task_list[-1]` is rejected today because `_INDEXED_SEGMENT` — the + one place `_resolve_dot_path` says what an index looks like — accepts digits + only. Widening it there and nowhere else must be enough; if the gate keeps + its own copy of the shape (as `_PATH_SEGMENT` used to), this fails. + """ + path = "steps.tasks.output.task_list[-1].file" + assert expressions._unresolvable_term(path) is not None + + monkeypatch.setattr( + expressions, "_INDEXED_SEGMENT", re.compile(r"^([\w-]+)\[(-?\d+)\]$") + ) + assert expressions._unresolvable_term(path) is None + + +def test_gate_reports_the_leaves_the_evaluator_actually_reached(monkeypatch): + """The gate's operands come from the evaluator's own walk, not a second parse. + + If `_evaluate_simple_expression` stops treating something as a leaf — which + is what unwrapping a parenthesised group does — the gate stops checking it, + with no change to the gate itself. + """ + grouped = "(inputs.a or inputs.b) and inputs.c" + assert expressions._unresolvable_term(grouped) is not None + + real = expressions._evaluate_simple_expression + + def unwrapping(expr, namespace): + stripped = expr.strip() + if stripped.startswith("(") and stripped.endswith(")"): + return unwrapping(stripped[1:-1], namespace) + return real(expr, namespace) + + monkeypatch.setattr(expressions, "_evaluate_simple_expression", unwrapping) + assert expressions._unresolvable_term(grouped) is None