Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions docs/reference/workflows.md
Original file line number Diff line number Diff line change
Expand Up @@ -1052,6 +1052,8 @@ Notes on the newer filters:
condition: "{{ inputs.items | length }}" # 0 is False, any non-zero count is True
```

`and` and `or` return `true` or `false` and evaluate their right side only when the left side does not decide the result: `and` skips it when the left side is false, and `or` skips it when the left side is true. A condition can therefore guard a filter that would fail on an empty value. `{{ inputs.payload != '' and (inputs.payload | from_json | length) }}` is false for an empty optional `payload` instead of failing the run on invalid JSON. When the right side is evaluated, its errors still fail the run.

Example:

```yaml
Expand Down
18 changes: 14 additions & 4 deletions src/specify_cli/workflows/expressions.py
Original file line number Diff line number Diff line change
Expand Up @@ -884,12 +884,17 @@ def _evaluate_simple_expression(expr: str, namespace: dict[str, Any]) -> Any:
or_idx = _find_top_level(expr, " or ")
if or_idx != -1:
left = _evaluate_simple_expression(expr[:or_idx].strip(), namespace)
# Validation probes must still visit operands that runtime can skip.
if bool(left) and not isinstance(namespace, _ProbeNamespace):
return True
right = _evaluate_simple_expression(expr[or_idx + 4:].strip(), namespace)
return bool(left) or bool(right)

and_idx = _find_top_level(expr, " and ")
if and_idx != -1:
left = _evaluate_simple_expression(expr[:and_idx].strip(), namespace)
if not bool(left) and not isinstance(namespace, _ProbeNamespace):
return False
right = _evaluate_simple_expression(expr[and_idx + 5:].strip(), namespace)
return bool(left) and bool(right)

Expand Down Expand Up @@ -1392,6 +1397,11 @@ class _ProbeNamespace(dict):
real inputs. Deliberately *not* resolving leaves to a sentinel value: a probe
that answers every lookup also answers ``inputs.count+1``, which is the
malformed shape the probe is meant to expose.
The probes also pass one as the outer namespace, which tells
``_evaluate_simple_expression`` to evaluate both operands of ``and``/``or``
instead of short-circuiting, so a leaf or filter in a skipped operand is
still checked.
"""

def __missing__(self, key: str) -> "_ProbeNamespace": # noqa: UP037 # pragma: no cover
Expand All @@ -1414,7 +1424,7 @@ def _evaluator_rejects(text: str) -> str | None:
"""
try:
_evaluate_simple_expression(
text, {root: _ProbeNamespace() for root in _NAMESPACE_ROOTS}
text, _ProbeNamespace({root: _ProbeNamespace() for root in _NAMESPACE_ROOTS})
)
except ValueError as exc:
message = str(exc)
Expand All @@ -1436,8 +1446,8 @@ def _collect_leaves(text: str) -> list[str]:
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
and list elements all do -- probe evaluation visits both sides of
``or``/``and`` eagerly rather than short-circuiting, so a leaf is
recorded whatever the other side is worth.
A probe run can still raise on its own placeholder values, which is what
Expand All @@ -1450,7 +1460,7 @@ def _collect_leaves(text: str) -> list[str]:
token = _leaf_sink.set(leaves)
try:
_evaluate_simple_expression(
text, {root: _ProbeNamespace() for root in _NAMESPACE_ROOTS}
text, _ProbeNamespace({root: _ProbeNamespace() for root in _NAMESPACE_ROOTS})
)
except Exception: # noqa: BLE001 - probe values, reported by _evaluator_rejects
pass
Expand Down
39 changes: 39 additions & 0 deletions tests/specify_cli/workflows/test_command_run.py
Original file line number Diff line number Diff line change
Expand Up @@ -1392,3 +1392,42 @@ def test_run_json_invalid_input_not_on_stdout(self, tmp_path):
assert result.exit_code == 1
assert "Invalid input format" not in result.stdout
assert "Invalid input format" in result.stderr


@pytest.mark.parametrize(("condition", "empty_branch"), [
("inputs.payload != '' and (inputs.payload | from_json | length)", "absent"),
("inputs.payload == '' or (inputs.payload | from_json | length)", "provided"),
])
@pytest.mark.parametrize("payload", ["", "[1]", "broken"])
def test_run_short_circuits_optional_json(tmp_path, monkeypatch, condition, empty_branch, payload):
from typer.testing import CliRunner
from specify_cli import app

workflow = tmp_path / "optional.yml"
workflow.write_text(yaml.safe_dump({
"schema_version": "1.0",
"workflow": {"id": "optional", "name": "Optional payload", "version": "1.0.0"},
"inputs": {"payload": {"type": "string", "default": ""}},
"steps": [{
"id": "check", "type": "if", "condition": "{{ " + condition + " }}",
"then": [{"id": "provided", "type": "shell", "run": "echo provided"}],
"else": [{"id": "absent", "type": "shell", "run": "echo absent"}],
}],
}), encoding="utf-8")
monkeypatch.chdir(tmp_path)
result = CliRunner().invoke(app, [
"workflow", "run", str(workflow), "--json", "--input", f"payload={payload}",
])
if payload == "broken":
assert result.exit_code == 1
assert "from_json: invalid JSON" in result.output
return
assert result.exit_code == 0, result.output
outcome = json.loads(result.stdout)
assert outcome["status"] == "completed"
state = json.loads((
tmp_path / ".specify" / "workflows" / "runs" / outcome["run_id"] / "state.json"
).read_text(encoding="utf-8"))
branch = "provided" if payload else empty_branch
assert state["step_results"][branch]["output"]["stdout"].strip() == branch
assert set(state["step_results"]) == {"check", branch}
29 changes: 29 additions & 0 deletions tests/test_workflows.py
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,35 @@ def test_boolean_or(self):
ctx = StepContext(inputs={"a": False, "b": True})
assert evaluate_expression("{{ inputs.a or inputs.b }}", ctx) is True

@pytest.mark.parametrize(("expression", "expected"), [
("inputs.payload != '' and (inputs.payload | from_json | length)", False),
("inputs.payload == '' or (inputs.payload | from_json | length)", True),
("false and true or true", True),
("true or (false and (inputs.payload | from_json))", True),
])
def test_boolean_short_circuits(self, expression, expected):
from specify_cli.workflows.expressions import evaluate_expression
from specify_cli.workflows.base import StepContext

assert evaluate_expression(
"{{ " + expression + " }}", StepContext(inputs={"payload": ""})
) is expected

@pytest.mark.parametrize("expression", [
"true and (inputs.payload | from_json)",
"false or (inputs.payload | from_json)",
"(inputs.payload | from_json) and false",
"(inputs.payload | from_json) or true",
])
def test_boolean_required_operand_still_raises(self, expression):
from specify_cli.workflows.expressions import evaluate_expression
from specify_cli.workflows.base import StepContext

with pytest.raises(ValueError, match="from_json: invalid JSON"):
evaluate_expression(
"{{ " + expression + " }}", StepContext(inputs={"payload": ""})
)

def test_list_literal_preserves_quoted_commas(self):
from specify_cli.workflows.expressions import evaluate_expression
from specify_cli.workflows.base import StepContext
Expand Down
15 changes: 10 additions & 5 deletions tests/unit/test_condition_expression_block.py
Original file line number Diff line number Diff line change
Expand Up @@ -888,16 +888,21 @@ def test_the_sink_is_cleared_even_when_the_probe_raises():


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.
"""
"""Probes must visit both operands even when runtime short-circuits."""
assert _collect_leaves("inputs.a or bogus") == ["inputs.a", "bogus"]
assert _collect_leaves("true or bogus") == ["bogus"]
assert _collect_leaves("false and bogus") == ["bogus"]
assert _unresolvable_term("false and bogus") is not None


@pytest.mark.parametrize("template", ["true or ({})", "false and ({})", "true or (false and ({}))"])
@pytest.mark.parametrize("filtered", ["inputs.x | nosuchfilter", "inputs.x | join", "inputs.x | length(1)"])
def test_filter_probe_checks_short_circuited_operands(template, filtered):
expression = template.format(filtered)
assert _evaluator_rejects(expression) is not None
assert "evaluator rejects" in format_condition_remediation(expression)


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
Expand Down
Loading