diff --git a/docs/reference/workflows.md b/docs/reference/workflows.md index 6262717c71..8ca425c825 100644 --- a/docs/reference/workflows.md +++ b/docs/reference/workflows.md @@ -1292,6 +1292,8 @@ Notes on the newer filters: condition: "{{ inputs.items | length }}" # 0 is False, any non-zero count is True ``` +- **Unsupported syntax raises, it does not resolve to `None`.** Text the evaluator cannot parse — a backslash-escaped quote in a string literal (`'it\'s'`; quote such a literal with the other quote type, `"it's"`), or a word operator without a space on both sides (`'x'and inputs.b`) — raises a `ValueError` naming the unsupported syntax at run time, instead of being looked up as a variable, resolving to `None`, and silently steering a condition. A well-formed path that merely finds nothing (`inputs.missing`) still resolves to `None`: missing data is not a syntax fault. Numeric literals accept the exponent form (`1e3`), both as literals and when a numeric string is coerced in a comparison, and indexed paths may chain (`matrix[0][1]`). + `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: diff --git a/src/specify_cli/workflows/expressions/engine_current.py b/src/specify_cli/workflows/expressions/engine_current.py index e3c74eda70..5f8df2ac45 100644 --- a/src/specify_cli/workflows/expressions/engine_current.py +++ b/src/specify_cli/workflows/expressions/engine_current.py @@ -23,7 +23,7 @@ _filter_map, _filter_split, ) -from .namespace import _NAMESPACE_ROOTS, _resolve_dot_path +from .namespace import _NAMESPACE_ROOTS, _is_path_segment, _resolve_dot_path from .operators import _safe_compare, _safe_membership from .template import ( _collapse_whitespace, @@ -168,7 +168,11 @@ def _evaluate_simple_expression(expr: str, namespace: dict[str, Any]) -> Any: - Boolean operators: ``and``, ``or``, ``not`` - ``in``, ``not in`` - Pipe filters: ``| default('...')``, ``| join(', ')``, ``| contains('...')``, ``| from_json``, ``| map('...')``, ``| split(',')``, ``| upper``, ``| lower``, ``| length``, ``| to_json`` - - String and numeric literals + - String and numeric literals (including the exponent form ``1e3``) + + Text that is none of the above and is not a resolvable variable path + raises ``ValueError`` naming the unsupported syntax, rather than silently + resolving to ``None``. """ expr = _collapse_whitespace(expr) @@ -307,13 +311,20 @@ def _evaluate_simple_expression(expr: str, namespace: dict[str, Any]) -> Any: if op == " not in ": return _safe_membership(left, right, negate=True) - # Numeric literal + # Numeric literal. int() first so integer text stays an int; float() then + # admits the decimal and exponent forms ("1.5", "1e3"), which the previous + # "." check missed -- `1e3` was looked up as a variable and became None + # (#5073). Only digit-bearing text may become a float, so the words + # float() also accepts ("inf", "nan") keep resolving as variable names. try: - if "." in expr: - return float(expr) return int(expr) except (ValueError, TypeError): pass + if any(ch.isdigit() for ch in expr): + try: + return float(expr) + except (ValueError, TypeError): + pass # Boolean literal if expr.lower() == "true": @@ -346,6 +357,34 @@ def _evaluate_simple_expression(expr: str, namespace: dict[str, Any]) -> Any: sink = _leaf_sink.get() if sink is not None: sink.append(expr) + # Text that is not a resolvable variable path used to be looked up as a + # name anyway, resolve to None, and silently hand a condition the wrong + # answer -- `'it\'s'`, `'x'and inputs.b` (#5073). Raise at run time + # instead, naming the text, the way mis-wired filters already do. Probe + # runs are exempt: the leaf is recorded above, and the validation gate + # owns the authoring diagnostics (`_unresolvable_leaf`), so validation + # messages do not change. + if ( + expr + and not isinstance(namespace, _ProbeNamespace) + and not all(_is_path_segment(seg.strip()) for seg in expr.split(".")) + ): + hint = "" + if re.search(r"['\"](?:and|or|in|not)\b|\b(?:and|or|in|not)['\"]", expr): + hint = ( + "; the word operators and/or/in/not need a space on both " + "sides to be recognized" + ) + elif "'" in expr or '"' in expr: + hint = ( + "; a string literal must be one wholly quoted string -- " + "backslash-escaped quotes are not supported, so quote a " + "literal containing a quote with the other quote type" + ) + raise ValueError( + f"unsupported expression syntax: {expr!r} is not a supported " + f"literal, operator expression, or variable path{hint}" + ) return _resolve_dot_path(namespace, expr) diff --git a/src/specify_cli/workflows/expressions/namespace.py b/src/specify_cli/workflows/expressions/namespace.py index 596c45fb39..d1cae4bd77 100644 --- a/src/specify_cli/workflows/expressions/namespace.py +++ b/src/specify_cli/workflows/expressions/namespace.py @@ -16,7 +16,12 @@ # 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+)\]$") +_INDEXED_SEGMENT = re.compile(r"^([\w-]+)((?:\[-?\d+\])+)$") + +# The integer tokens inside an indexed segment's bracket suffix. Parsed from the +# matched text rather than a capture group so a widened _INDEXED_SEGMENT (the +# wiring test swaps in one with a different group shape) still yields indices. +_INDEX_TOKEN = re.compile(r"-?\d+") _PLAIN_SEGMENT = re.compile(r"^[\w-]+$") @@ -25,23 +30,28 @@ def _resolve_dot_path(obj: Any, path: str) -> Any: """Resolve a dotted path like ``steps.specify.output.file`` against *obj*. Supports dict key access and list indexing, including the negative form - Python and Jinja2 both accept (e.g., ``task_list[0]``, ``task_list[-1]``). + Python and Jinja2 both accept (e.g., ``task_list[0]``, ``task_list[-1]``) + and the chained form (``matrix[0][1]``), which used to fall outside + ``_INDEXED_SEGMENT`` and be looked up as the literal key ``"matrix[0][1]"`` + (#5073). """ parts = path.split(".") current = obj for part in parts: - # Handle list indexing: name[0], name[-1] + # Handle list indexing: name[0], name[-1], name[0][1] idx_match = _INDEXED_SEGMENT.match(part) if idx_match: - key, idx = idx_match.group(1), int(idx_match.group(2)) + key = idx_match.group(1) if isinstance(current, dict): current = current.get(key) else: return None - if isinstance(current, list) and -len(current) <= idx < len(current): - current = current[idx] - else: - return None + for token in _INDEX_TOKEN.findall(part[len(key):]): + idx = int(token) + if isinstance(current, list) and -len(current) <= idx < len(current): + current = current[idx] + else: + return None elif isinstance(current, dict): current = current.get(part) else: diff --git a/src/specify_cli/workflows/expressions/operators.py b/src/specify_cli/workflows/expressions/operators.py index bdf26128b6..fdb03a71d6 100644 --- a/src/specify_cli/workflows/expressions/operators.py +++ b/src/specify_cli/workflows/expressions/operators.py @@ -10,12 +10,25 @@ def _coerce_number(value: Any) -> Any: - """Return *value* as int/float if it is a numeric string, else unchanged.""" + """Return *value* as int/float if it is a numeric string, else unchanged. + + ``int()`` first so integer text stays an int; ``float()`` then admits the + decimal and exponent forms (``"1.5"``, ``"1e3"``), matching what workflow + input coercion accepts as a number (``WorkflowEngine._coerce_input``), so + the same text cannot be a number as an input but a string in a comparison + (#5073). ``float()`` also accepts the words ``"inf"``/``"nan"``; those + carry no digit and stay ordinary strings here. + """ if isinstance(value, str): try: - return float(value) if "." in value else int(value) + return int(value) except ValueError: - return value + pass + if any(ch.isdigit() for ch in value): + try: + return float(value) + except ValueError: + return value return value diff --git a/tests/test_workflows.py b/tests/test_workflows.py index c1dc248439..108d0790e9 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -452,15 +452,19 @@ def test_list_literal_followed_by_index_is_not_misparsed_as_one_literal(self): outer brackets from ``[1,2,3][1]`` yields ``1,2,3][1``, which then silently evaluates to ``[1, 2, None]`` instead of raising or resolving the index. It must not be misclassified as one literal; - falling through to unresolvable (``None``) is safe, unlike silently - returning a wrong-looking list. + since #5073 the fall-through raises as unsupported syntax, which is + safe, unlike silently returning a wrong-looking list. """ + import pytest + from specify_cli.workflows.expressions import evaluate_expression from specify_cli.workflows.base import StepContext ctx = StepContext() - assert evaluate_expression("{{ [1,2,3][1] }}", ctx) is None - assert evaluate_expression("{{ [1,2][0] }}", ctx) is None + with pytest.raises(ValueError, match="unsupported expression syntax"): + evaluate_expression("{{ [1,2,3][1] }}", ctx) + with pytest.raises(ValueError, match="unsupported expression syntax"): + evaluate_expression("{{ [1,2][0] }}", ctx) # Genuine list literals -- including ones a bracket-depth scan must # still recognize as ending exactly at the final character -- are # unaffected. @@ -504,6 +508,78 @@ def test_operator_splitting_is_quote_aware(self): assert evaluate_expression("{{ inputs.a == 9 or inputs.b == 2 }}", plain) is True assert evaluate_expression("{{ inputs.missing | default('a and b') }}", plain) == "a and b" + def test_exponent_numeric_literals_evaluate_as_numbers(self): + """#5073 case 1: `1e3` is a number, as in Python/Jinja2 and as workflow + input coercion already accepts. It used to be looked up as a variable + and become None, and the numeric string "1e3" stayed a string in + comparisons while coercing to 1000 as an input.""" + from specify_cli.workflows.expressions import ( + evaluate_condition, + evaluate_expression, + ) + from specify_cli.workflows.base import StepContext + + ctx = StepContext(inputs={"n": 5, "s": "1e3"}) + assert evaluate_expression("{{ 1e3 }}", ctx) == 1000.0 + assert evaluate_expression("{{ 2.5e-1 }}", ctx) == 0.25 + assert evaluate_condition("{{ inputs.n < 1e3 }}", ctx) is True + # A numeric string in the exponent form coerces in comparisons the way + # plain "10" already does (_coerce_number). + assert evaluate_condition("{{ inputs.s > 100 }}", ctx) is True + # The words float() also accepts are still variable names, not numbers. + assert evaluate_expression("{{ inf }}", ctx) is None + assert evaluate_expression("{{ nan }}", ctx) is None + # Plain integers stay ints. + assert evaluate_expression("{{ 10 }}", ctx) == 10 + assert isinstance(evaluate_expression("{{ 10 }}", ctx), int) + + def test_chained_indexing_resolves_like_jinja(self): + """#5073 case 3: `matrix[0][1]` walks both indices. It used to fall + outside the one-index segment pattern and be looked up as the literal + key "matrix[0][1]", returning None.""" + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext( + steps={"plan": {"output": {"matrix": [["a", "b"], ["c", "d"]]}}} + ) + assert evaluate_expression("{{ steps.plan.output.matrix[0] }}", ctx) == ["a", "b"] + assert evaluate_expression("{{ steps.plan.output.matrix[0][1] }}", ctx) == "b" + assert evaluate_expression("{{ steps.plan.output.matrix[-1][0] }}", ctx) == "c" + # An out-of-range index is missing data, not syntax: still None. + assert evaluate_expression("{{ steps.plan.output.matrix[0][9] }}", ctx) is None + assert evaluate_expression("{{ steps.plan.output.matrix[9][0] }}", ctx) is None + + def test_unsupported_syntax_raises_instead_of_silent_none(self): + """#5073 cases 2 and 4: text the evaluator does not parse used to be + looked up as a variable path, resolve to None, and silently give a + condition the wrong answer. It now raises a ValueError naming the + unsupported syntax, the way mis-wired filters already do.""" + import pytest + + from specify_cli.workflows.expressions import evaluate_expression + from specify_cli.workflows.base import StepContext + + ctx = StepContext(inputs={"name": "it's", "a": "x", "b": True}) + for template in ( + r"{{ 'it\'s' }}", # escaped quote in a literal + r"{{ inputs.name == 'it\'s' }}", + "{{ inputs.a=='x'and inputs.b }}", # word operator without spaces + "{{ 'x'in inputs.a }}", + ): + with pytest.raises(ValueError, match="unsupported expression syntax"): + evaluate_expression(template, ctx) + + # The error's advice works: the other quote type spells the literal. + assert evaluate_expression("{{ \"it's\" }}", ctx) == "it's" + assert evaluate_expression("{{ inputs.name == \"it's\" }}", ctx) is True + # Properly spaced operators are untouched... + assert evaluate_expression("{{ inputs.a == 'x' and inputs.b }}", ctx) is True + # ...and an unknown but well-formed name still resolves to None, not an + # error: missing data is not a syntax fault. + assert evaluate_expression("{{ inputs.missing }}", ctx) is None + assert evaluate_expression("{{ bogus.path[3] }}", ctx) is None + def test_pipe_detection_is_quote_aware(self): from specify_cli.workflows.expressions import evaluate_expression from specify_cli.workflows.base import StepContext diff --git a/tests/unit/test_condition_expression_block.py b/tests/unit/test_condition_expression_block.py index 0f20431edc..4a4b08684d 100644 --- a/tests/unit/test_condition_expression_block.py +++ b/tests/unit/test_condition_expression_block.py @@ -421,13 +421,18 @@ def test_wrapping_whitespace_would_invert_the_condition(): def test_wrapping_an_open_quote_inverts_the_condition(): """Why the unbalanced-quote case gets advice instead of a suggestion. - The raw-close fallback evaluates a truncated comparison and yields the string - "False", which evaluate_condition then reads as the `false` keyword. Pasting - the "correction" flips the condition rather than repairing it. + The raw-close fallback evaluates a truncated comparison, whose dangling + `'abc` operand used to resolve to None and yield the string "False", which + evaluate_condition then read as the `false` keyword -- pasting the + "correction" flipped the condition rather than repairing it. Since #5073 + the truncated operand raises as unsupported syntax instead of silently + inverting; the advice (rather than a paste-ready suggestion) is still + right, because wrapping the text as written cannot repair it. """ ctx = StepContext(inputs={"name": "Bob"}) assert evaluate_condition("{{ inputs.name == 'abc", ctx) is True - assert evaluate_condition("{{ inputs.name == 'abc }}", ctx) is False + with pytest.raises(ValueError, match="unsupported expression syntax"): + evaluate_condition("{{ inputs.name == 'abc }}", ctx) @pytest.mark.parametrize( @@ -655,7 +660,8 @@ def test_the_probe_reports_what_the_evaluator_reports(): # _resolve_dot_path keys on [w-]+, so a key literally named "2bad" resolves. ("inputs.2bad", False), ("inputs.tags[foo]", True), - ("inputs.matrix[0][1]", True), + # Chained indexing is part of the indexed-segment shape since #5073. + ("inputs.matrix[0][1]", False), # Round 7: an operand one level down, which the single-term gate never saw. ("inputs.a === inputs.b", True), ("bogus", True), @@ -711,12 +717,10 @@ def test_filter_wiring_errors_are_still_rejections(condition): ("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 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), + # `1e3` is a numeric literal since #5073 -- the evaluator's float() + # branch accepts the exponent form, and the gate reads that straight + # off the evaluator's walk, so the two cannot disagree. + ("1e3", True), ("'one'", True), ('"one"', True), # Two literals, not one: the evaluator requires the opening quote's match to @@ -745,7 +749,6 @@ def test_literal_handling_comes_from_the_evaluator(condition, resolvable): # to None however the index is written. "inputs[0]", "steps[1]", - "1e3", "'a' 'b'", ], )