diff --git a/docs/releases/3.2.1.md b/docs/releases/3.2.1.md index a05e422e..ea3983f3 100644 --- a/docs/releases/3.2.1.md +++ b/docs/releases/3.2.1.md @@ -100,3 +100,51 @@ instead of at class-definition time. ``` [#632](https://github.com/fgmacedo/python-statemachine/issues/632). + +### Operators without surrounding whitespace in `cond` expressions + +Boolean expressions used in `cond` / `unless` had a fast-path that returned the +whole string as a single variable name when it looked operator-free. The check +was too naive: it only looked for `!`, a literal space and `In(`, so any other +operator written without surrounding whitespace was swallowed into a variable +name. `cond="is_paid^is_shipped"` and `cond="items>0"` were resolved as +variables literally named `is_paid^is_shipped` and `items>0`, failing with +`InvalidDefinition: Did not found name ...` when the machine was built. + +The fast-path now triggers only for a lone Python identifier, so every other +expression goes through the parser and whitespace is never required: + +```py +>>> from statemachine import State, StateMachine + +>>> class Order(StateMachine): +... waiting = State(initial=True) +... completed = State(final=True) +... +... complete = waiting.to(completed, cond="is_paid^items>0") +... +... is_paid: bool = False +... items: int = 2 + +>>> sm = Order() +>>> sm.send("complete") +Traceback (most recent call last): + ... +statemachine.exceptions.TransitionNotAllowed: Can't complete when in Waiting. + +>>> sm.is_paid = True +>>> sm.send("complete") +>>> sm.completed.is_active +True + +``` + +Guards that are a single name (including a bare `v`) keep taking the fast-path, +and `!=` keeps parsing as a comparison rather than a negation. + +As a side effect, a `cond` with a structure that is not valid in a boolean +expression, such as `cond="user.age"`, now raises +`InvalidDefinition: Failed to parse boolean expression 'user.age'` instead of +reporting the whole string as a name that was not found. + +[#639](https://github.com/fgmacedo/python-statemachine/pull/639). diff --git a/statemachine/dispatcher.py b/statemachine/dispatcher.py index 3324f38a..8f9bc369 100644 --- a/statemachine/dispatcher.py +++ b/statemachine/dispatcher.py @@ -14,6 +14,7 @@ from .exceptions import InvalidDefinition from .i18n import _ from .signature import SignatureAdapter +from .spec_parser import UnsupportedExpression from .spec_parser import custom_and from .spec_parser import operator_mapping from .spec_parser import parse_boolean_expr @@ -118,7 +119,10 @@ def build(self, spec: "CallbackSpec"): try: expression = parse_boolean_expr(spec.func, take_callback_partial, operator_mapping) - except SyntaxError as err: + except (SyntaxError, UnsupportedExpression) as err: + # ``UnsupportedExpression`` comes from the AST allowlist rejecting a node + # kind that cannot appear in a boolean expression (e.g. ``"user.age"``). + # Errors raised while resolving names are not parse errors and propagate. raise InvalidDefinition( _("Failed to parse boolean expression '{}'").format(spec.func) ) from err diff --git a/statemachine/spec_parser.py b/statemachine/spec_parser.py index 306a49c3..9f9713af 100644 --- a/statemachine/spec_parser.py +++ b/statemachine/spec_parser.py @@ -19,6 +19,15 @@ } +class UnsupportedExpression(ValueError): + """The expression contains a structure that is not on the parser allowlist. + + Distinguishes a rejected expression from errors raised by the ``variable_hook`` + while resolving names, so callers can report each one properly. Inherits from + ``ValueError`` to keep the previous behavior for callers catching it. + """ + + def _unique_key(left, right, operator) -> str: left_key = getattr(left, "unique_key", "") right_key = getattr(right, "unique_key", "") @@ -128,7 +137,7 @@ def register(func): def get(cls, func_id): func_id = func_id.lower() if func_id not in cls.registry: - raise ValueError(f"Unsupported function: {func_id}") + raise UnsupportedExpression(f"Unsupported function: {func_id}") return cls.registry[func_id] @@ -300,14 +309,16 @@ def recurse(child): # without underscore-attribute access or method calls, it cannot reach # type objects. if attr.startswith("_"): - raise ValueError(f"Attribute access to '{attr}' is not allowed") + raise UnsupportedExpression(f"Attribute access to '{attr}' is not allowed") return build_attribute(recurse(node.value), attr) case ast.Name(id=name): return variable_hook(name) case ast.Constant(value=value): return build_constant(value) case _: - raise ValueError(f"Unsupported expression structure: {node.__class__.__name__}") + raise UnsupportedExpression( + f"Unsupported expression structure: {node.__class__.__name__}" + ) def parse_boolean_expr(expr, variable_hook, operator_mapping): @@ -315,8 +326,10 @@ def parse_boolean_expr(expr, variable_hook, operator_mapping): if expr.strip() == "": raise SyntaxError("Empty expression") - # Optimization trying to avoid parsing the expression if not needed - if "!" not in expr and " " not in expr and "In(" not in expr: + # Optimization: a lone identifier can only be a variable name, so there is + # nothing to parse. Anything else (operators, comparisons, spaces, calls) + # goes through the parser. + if expr.isidentifier(): return variable_hook(expr) expr = replace_operators(expr) tree = ast.parse(expr, mode="eval") diff --git a/tests/test_conditions_algebra.py b/tests/test_conditions_algebra.py index db5e3c7a..bf46f90d 100644 --- a/tests/test_conditions_algebra.py +++ b/tests/test_conditions_algebra.py @@ -66,3 +66,33 @@ class AnyConditionSM(StateChart): with pytest.raises(InvalidDefinition, match="Did not found name 'xxx'"): AnyConditionSM() + + +def test_should_raise_invalid_definition_if_cond_has_unsupported_structure(): + class AnyConditionSM(StateChart): + start = State(initial=True) + end = State(final=True) + + submit = start.to(end, cond="user.age") + + with pytest.raises(InvalidDefinition, match="Failed to parse boolean expression 'user.age'"): + AnyConditionSM() + + +def test_should_not_mask_errors_raised_while_resolving_names(): + # Resolving a name reads attributes from the model, which runs user code. An + # error raised there is not a parse error and must not be reported as one. + class Model: + @property + def is_ready(self): + raise ValueError("the model is not configured") + + class AnyConditionSM(StateChart): + start = State(initial=True) + end = State(final=True) + + submit = start.to(end, cond="is_ready") + + model = Model() + with pytest.raises(ValueError, match="the model is not configured"): + AnyConditionSM(model) diff --git a/tests/test_spec_parser.py b/tests/test_spec_parser.py index 51115bd7..8b4a9db7 100644 --- a/tests/test_spec_parser.py +++ b/tests/test_spec_parser.py @@ -210,6 +210,35 @@ def test_classical_operators_name(): ) # name reflects expression structure +@pytest.mark.parametrize( + ("unspaced", "spaced", "expected"), + [ + ("frodo_has_ring^sam_is_loyal", "frodo_has_ring ^ sam_is_loyal", True), + ("frodo_has_ring^sauron_alive", "frodo_has_ring ^ sauron_alive", False), + ("(frodo_has_ring)v(sauron_alive)", "(frodo_has_ring) v (sauron_alive)", True), + # "!" is an operator, but "!=" is not a negation: it must keep parsing + # as a comparison. Note the "!=51" (and not "!=50"): a misparse resolves + # the whole string as an unknown variable, which defaults to False, so a + # "!=50" case would pass for the wrong reason. + ("frodo_age!=51", "frodo_age != 51", True), + ("frodo_age>=50", "frodo_age >= 50", True), + ], +) +def test_operators_without_surrounding_spaces(unspaced, spaced, expected): + # Operators do not require surrounding whitespace, so an unspaced expression + # must parse just like its spaced equivalent instead of being swallowed into + # a single variable name. + got = parse_boolean_expr(unspaced, variable_hook, operator_mapping)() + want = parse_boolean_expr(spaced, variable_hook, operator_mapping)() + assert got is want is expected + + +def test_bare_v_is_a_variable_name(): + # "v" is only an operator between operands; a lone "v" is a plain variable. + expr = parse_boolean_expr("v", variable_hook, operator_mapping) + assert expr.__name__ == "v" + + def test_empty_expression(): expr = "" with pytest.raises(SyntaxError):