From 7ccf95fd65dcf32716527bacea62eb3a8a2869c1 Mon Sep 17 00:00:00 2001 From: Fernando Macedo Date: Mon, 12 Oct 2026 13:45:48 -0300 Subject: [PATCH] fix: register one event per id a prefixed attribute expands to `_expand_event_id` turned `error_foo` into the string `"error_foo error.foo"`, and the callers registered a single event under that glued id. The transition carried both ids, so `send("error.foo")` worked, but `sm.error_foo()` raised `TypeError` on a bare transition and was a no-op on an `Event(...)`, and `allowed_events` / `enabled_events()` raised `AttributeError` looking up `error.execution` on the instance. Inside a nested state body the glued id was split by `Events.add`, which dropped the `name`, `delay` and `internal` declared on the `Event`. The `@source.to(target)` decorator form never expanded at all. The expansion now returns the ids as a list and each call site, including the decorator form, registers one event per id, binding each as a class attribute. An `Event()` declared without transitions and referenced through `event=` maps to every id it expands to, so `_events_to_update` keeps a list of replacements per placeholder and looks the placeholder up by membership rather than by the prefix matcher, which crashed on a `"*"` transition. `AnyState` adds every id to a single copy per state instead of copying the transition once per id. Fixes #654. Also fixes question 1 of #644. Signed-off-by: Fernando Macedo --- docs/error_handling.md | 6 +- docs/events.md | 46 ++++++- docs/releases/3.2.2.md | 41 ++++++ statemachine/event.py | 18 +-- statemachine/events.py | 7 +- statemachine/factory.py | 46 +++---- statemachine/state.py | 40 ++++-- tests/test_events.py | 24 ++++ tests/test_expanded_event_ids.py | 217 ++++++++++++++++++++++++++++++ tests/test_statechart_compound.py | 13 +- 10 files changed, 392 insertions(+), 66 deletions(-) create mode 100644 tests/test_expanded_event_ids.py diff --git a/docs/error_handling.md b/docs/error_handling.md index 9c43ae0c..80725aab 100644 --- a/docs/error_handling.md +++ b/docs/error_handling.md @@ -66,9 +66,9 @@ recovering. ### The `error_` naming convention Since Python identifiers cannot contain dots, any event attribute starting -with `error_` automatically matches both the underscore and dot-notation -forms. For example, `error_execution` matches both `"error_execution"` and -`"error.execution"`: +with `error_` automatically registers both the underscore and dot-notation +forms as events. For example, `error_execution` declares both `"error_execution"` and +`"error.execution"`, and either id triggers the transition: ```py >>> from statemachine import State, StateChart diff --git a/docs/events.md b/docs/events.md index 05778e39..d9e50cbb 100644 --- a/docs/events.md +++ b/docs/events.md @@ -429,17 +429,53 @@ patterns, `after` as a finalize hook, and nested error scenarios. SCXML uses dot-separated event names (`done.state.quest`, `error.execution`), but Python identifiers cannot contain dots. The library provides prefix-based -naming conventions that automatically register both forms: +naming conventions that automatically register both forms, each as an event of +its own, whichever declaration form you use (assignment, `Event(...)` or the +`@source.to(target)` decorator): both are listed by `events`, `allowed_events` +and `enabled_events()`, both can be sent by id, and the attribute name is also +callable as a method: + +```py +>>> class ErrorLogger(StateChart): +... running = State(initial=True) +... failed = State(final=True) +... +... process = running.to(running, on="do_process") +... error_execution = running.to(failed, on="log_error") +... +... def do_process(self): +... raise ValueError("bad data") +... +... def log_error(self, error): +... self.last_error = error + +>>> [event.id for event in ErrorLogger.events] +['process', 'error_execution', 'error.execution'] + +>>> sm = ErrorLogger() +>>> [event.id for event in sm.enabled_events()] +['process', 'error_execution', 'error.execution'] + +>>> sm.send("process") +>>> sm.last_error +ValueError('bad data') + +>>> sm = ErrorLogger() +>>> sm.error_execution(error=RuntimeError("sent by hand")) +>>> sm.last_error +RuntimeError('sent by hand') + +``` (done-state-convention)= ### `done_state_` prefix -Any event attribute starting with `done_state_` matches both the underscore +Any event attribute starting with `done_state_` registers both the underscore form and the dot-notation form. Only the prefix is replaced — the suffix is kept as-is, preserving multi-word state names: -| Attribute name | Matches event names | +| Attribute name | Registers event ids | |-------------------------------|---------------------| | `done_state_quest` | `"done_state_quest"` and `"done.state.quest"` | | `done_state_lonely_mountain` | `"done_state_lonely_mountain"` and `"done.state.lonely_mountain"` | @@ -447,11 +483,11 @@ kept as-is, preserving multi-word state names: ### `error_` prefix -Any event attribute starting with `error_` matches both the underscore form +Any event attribute starting with `error_` registers both the underscore form and the dot-notation form. Unlike `done_state_`, **all** underscores after the prefix are replaced with dots: -| Attribute name | Matches event names | +| Attribute name | Registers event ids | |----------------------|---------------------| | `error_execution` | `"error_execution"` and `"error.execution"` | diff --git a/docs/releases/3.2.2.md b/docs/releases/3.2.2.md index c4f7266a..501eccdc 100644 --- a/docs/releases/3.2.2.md +++ b/docs/releases/3.2.2.md @@ -110,6 +110,47 @@ A pint for Frodo [#646](https://github.com/fgmacedo/python-statemachine/issues/646). +### Prefixed events were registered under a glued id + +The `error_` / `done_state_` / `done_invoke_` prefixes declare two event ids, the attribute +name and its SCXML dotted form. Both reached the transition, so `send("error.execution")` +worked, but the chart registered a single event whose id was the two ids joined by a space. +`sm.error_execution()` raised `TypeError` on a bare transition and silently did nothing on an +`Event(...)`, and `allowed_events` / `enabled_events()` raised `AttributeError` looking up an +attribute named `error.execution`. The expansion also dropped the `name`, `delay` and +`internal` declared on an `Event` inside a nested state body. + +Each id is now an event of its own, so both are listed, both are sendable, the attribute name +is callable as a method, and the declared arguments are kept on both. The +`@source.to(target)` decorator form, which registered only the attribute name, expands too: + +```py +>>> from statemachine import Event, State, StateChart + +>>> class ErrorLogger(StateChart): +... running = State(initial=True) +... failed = State(final=True) +... +... error_execution = Event(running.to(failed), name="Boom") + +>>> [(event.id, event.name) for event in ErrorLogger.events] +[('error_execution', 'Boom'), ('error.execution', 'Boom')] + +>>> sm = ErrorLogger() +>>> [event.id for event in sm.enabled_events()] +['error_execution', 'error.execution'] + +>>> sm.error_execution() +>>> sm.failed.is_active +True + +``` + +Reported by [@Dolecor](https://github.com/Dolecor). + +[#644](https://github.com/fgmacedo/python-statemachine/issues/644), +[#654](https://github.com/fgmacedo/python-statemachine/issues/654). + ### Events due at the same moment kept their order only by chance Both event queues are priority queues ordered by each trigger's due time, and triggers due at diff --git a/statemachine/event.py b/statemachine/event.py index 4ae6e2b3..ccd355ca 100644 --- a/statemachine/event.py +++ b/statemachine/event.py @@ -16,22 +16,22 @@ from .transition_list import TransitionList -def _expand_event_id(key: str) -> str: - """Apply naming conventions for special event prefixes. +def _expand_event_id(key: str) -> list[str]: + """Return the event ids an attribute name declares. - Converts underscore-based Python attribute names to their dot-separated - event equivalents. Returns a space-separated string so ``Events.add()`` - registers both forms. + ``done_invoke_X``, ``done_state_X`` and ``error_X`` also declare their SCXML dotted form + (``done.invoke.X``, ``done.state.X``, ``error.X``), each registered as its own event. Any + other name declares only itself. """ if key.startswith("done_invoke_"): suffix = key[len("done_invoke_") :] - return f"{key} done.invoke.{suffix}" + return [key, f"done.invoke.{suffix}"] if key.startswith("done_state_"): suffix = key[len("done_state_") :] - return f"{key} done.state.{suffix}" + return [key, f"done.state.{suffix}"] if key.startswith("error_"): - return f"{key} {key.replace('_', '.')}" - return key + return [key, key.replace("_", ".")] + return [key] _event_data_kwargs = { diff --git a/statemachine/events.py b/statemachine/events.py index 16cd681e..3b2ffbab 100644 --- a/statemachine/events.py +++ b/statemachine/events.py @@ -18,6 +18,9 @@ def __repr__(self): def __iter__(self): return iter(self._items) + def __contains__(self, event): + return event in self._items + def add(self, events): if events is None: return self @@ -39,9 +42,9 @@ def match(self, event: "str | None"): return self.is_empty return any(e.match(event) for e in self) - def _replace(self, old, new): + def _replace(self, old, new_events): self._items.remove(old) - self._items.append(new) + self.add(new_events) @property def is_empty(self): diff --git a/statemachine/factory.py b/statemachine/factory.py index ae736a48..999c8c07 100644 --- a/statemachine/factory.py +++ b/statemachine/factory.py @@ -51,7 +51,7 @@ def __init__( cls._abstract = True cls._events: dict[Event, None] = {} # used Dict to preserve order and avoid duplicates cls._protected_attrs: set = set() - cls._events_to_update: dict[Event, Event | None] = {} + cls._events_to_update: dict[Event, list[Event]] = {} cls._specs = CallbackSpecList() cls.prepare = cls._specs.grouper(CallbackGroup.PREPARE).add( "prepare_event", priority=CallbackPriority.GENERIC, is_convention=True @@ -305,23 +305,21 @@ def add_from_attributes(cls, attrs): # noqa: C901 if isinstance(value, State): cls.add_state(key, value) elif isinstance(value, (Transition, TransitionList)): - event_id = _expand_event_id(key) - cls.add_event(event=Event(transitions=value, id=event_id)) + for event_id in _expand_event_id(key): + cls.add_event(event=Event(transitions=value, id=event_id)) elif isinstance(value, (Event,)): - if value._has_real_id: - event_id = value.id - else: - event_id = _expand_event_id(key) - new_event = Event( - transitions=value._transitions, - id=event_id, - name=value.name, - delay=value.delay, - internal=value.internal, - ) - cls.add_event(event=new_event, old_event=value) + event_ids = [value.id] if value._has_real_id else _expand_event_id(key) + for event_id in event_ids: + new_event = Event( + transitions=value._transitions, + id=event_id, + name=value.name, + delay=value.delay, + internal=value.internal, + ) + cls.add_event(event=new_event, old_event=value) # Ensure the event is accessible by the Python attribute name - if event_id != key: + if key not in event_ids: setattr(cls, key, new_event) elif getattr(value, "attr_name", None): cls._add_unbounded_callback(key, value) @@ -336,7 +334,8 @@ def _add_unbounded_callback(cls, attr_name, func): # machinery that is stored at ``func.attr_name`` setattr(cls, func.attr_name, func) if func.is_event: - cls.add_event(event=Event(func._transitions, id=attr_name)) + for event_id in _expand_event_id(attr_name): + cls.add_event(event=Event(func._transitions, id=event_id)) def add_state(cls, id, state: State): state._set_id(id) @@ -359,8 +358,7 @@ def add_event( old_event: "Event | None" = None, ): if not event._has_real_id: - if event not in cls._events_to_update: - cls._events_to_update[event] = None + cls._events_to_update.setdefault(event, []) return transitions = event._transitions @@ -372,20 +370,20 @@ def add_event( setattr(cls, event.id, event) if old_event is not None: - cls._events_to_update[old_event] = event + cls._events_to_update.setdefault(old_event, []).append(event) return cls._events[event] def _update_event_references(cls): - for old_event, new_event in cls._events_to_update.items(): + for old_event, new_events in cls._events_to_update.items(): for state in cls.states: for transition in state.transitions: - if transition._events.match(old_event): - if new_event is None: + if old_event in transition._events: + if not new_events: raise InvalidDefinition( _("An event in the '{}' has no id.").format(transition) ) - transition.events._replace(old_event, new_event) + transition.events._replace(old_event, new_events) cls._events_to_update = {} diff --git a/statemachine/state.py b/statemachine/state.py index f5e9b549..399bbd6a 100644 --- a/statemachine/state.py +++ b/statemachine/state.py @@ -92,21 +92,24 @@ def __new__( # type: ignore [misc] # noqa: C901 value._set_id(key) states.append(value) elif isinstance(value, TransitionList): - value.add_event(_expand_event_id(key)) + for event_id in _expand_event_id(key): + value.add_event(event_id) elif isinstance(value, Event): if value._transitions is not None: - event_id = value.id if value._has_real_id else _expand_event_id(key) - value._transitions.add_event( - Event( - id=event_id, - name=value.name, - delay=value.delay, - internal=value.internal, + event_ids = [value.id] if value._has_real_id else _expand_event_id(key) + for event_id in event_ids: + value._transitions.add_event( + Event( + id=event_id, + name=value.name, + delay=value.delay, + internal=value.internal, + ) ) - ) elif getattr(value, "attr_name", None): if value.is_event: - value._transitions.add_event(key) + for event_id in _expand_event_id(key): + value._transitions.add_event(event_id) callbacks[value.attr_name] = value elif callable(value): callbacks[key] = value @@ -439,13 +442,24 @@ class AnyState(State): until the state machine class is evaluated. """ + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self._copies: dict[tuple[int, int], Transition] = {} + """Transition copied onto each state, keyed by ``(id(transition), id(state))``, so an + attribute that declares several ids adds them all to one copy.""" + def _on_event_defined(self, event: str, transition: Transition, states: list[State]): for state in states: if state.final: continue - new_transition = transition._copy_with_args(source=state, event=event) - - state.transitions.add_transitions(new_transition) + key = (id(transition), id(state)) + copy = self._copies.get(key) + if copy is None: + copy = transition._copy_with_args(source=state, event=event) + self._copies[key] = copy + state.transitions.add_transitions(copy) + else: + copy.add_event(event) class HistoryType(str, Enum): diff --git a/tests/test_events.py b/tests/test_events.py index 7fd97c12..2dd6104d 100644 --- a/tests/test_events.py +++ b/tests/test_events.py @@ -253,6 +253,30 @@ def on_cycle(self, event_data, event: str): assert sm.send("cycle") == "Running cycle from yellow to red" assert sm.send("cycle") == "Running cycle from red to green" + def test_placeholder_bound_to_several_names_declares_each_as_an_id(self): + class TrafficLightMachine(StateChart): + green = State(initial=True) + yellow = State(final=True) + + cycle = Event() + slowdown = cycle + green.to(yellow, event=cycle) + + assert [event.id for event in TrafficLightMachine.events] == ["cycle", "slowdown"] + assert [str(t.events) for t in TrafficLightMachine.green.transitions] == ["cycle slowdown"] + + def test_wildcard_transition_coexists_with_an_event_attribute(self): + class TrafficLightMachine(StateChart): + green = State(initial=True) + yellow = State() + red = State(final=True) + + cycle = Event(green.to(yellow), name="Loop") + yellow.to(red, event="*") + + assert [str(t.events) for t in TrafficLightMachine.yellow.transitions] == ["*"] + assert {event.id for event in TrafficLightMachine.events} == {"cycle", "*"} + def test_allow_registering_callbacks_using_decorator(self): class TrafficLightMachine(StateChart): "A traffic light machine" diff --git a/tests/test_expanded_event_ids.py b/tests/test_expanded_event_ids.py new file mode 100644 index 00000000..51de67c7 --- /dev/null +++ b/tests/test_expanded_event_ids.py @@ -0,0 +1,217 @@ +"""Prefixed event attributes register one event per id they expand to. + +``error_X``, ``done_state_X`` and ``done_invoke_X`` declare both the attribute name and the +SCXML dotted form, each as its own event: reachable as a method, sendable by id, and listed by +``allowed_events`` / ``enabled_events()``. + +Theme: a hobbit leaving Bag End. +""" + +from inspect import isawaitable + +import pytest +from statemachine.contrib.diagram.extract import extract +from statemachine.event import _expand_event_id +from statemachine.state import NestedStateFactory +from statemachine.utils import humanize_id + +from statemachine import Event +from statemachine import State +from statemachine import StateChart + +PREFIXES = [ + pytest.param("error_foo", "error.foo", id="error"), + pytest.param("done_state_foo", "done.state.foo", id="done_state"), + pytest.param("done_invoke_foo", "done.invoke.foo", id="done_invoke"), +] + + +def declare_then_reference(s, t): + event = Event() + s.to(t, event=event) + return event + + +def decorate(s, t): + @s.to(t) + def action(self): + return "acted" + + return action + + +FORMS = [ + pytest.param(lambda s, t: s.to(t), id="bare"), + pytest.param(lambda s, t: Event(s.to(t), name="Boom"), id="event"), + pytest.param(declare_then_reference, id="referenced"), + pytest.param(decorate, id="decorator"), +] + +NESTED = [pytest.param(False, id="top-level"), pytest.param(True, id="nested")] + + +def build_chart(attr, declare, nested): + """Build a chart where ``attr`` holds the declaration of ``bag_end -> green_dragon``.""" + if nested and declare is declare_then_reference: + pytest.skip("an Event with no transitions is not supported inside a nested body") + bag_end = State(initial=True) + green_dragon = State(final=True) + body = {"bag_end": bag_end, "green_dragon": green_dragon, attr: declare(bag_end, green_dragon)} + if nested: + body = {"shire": NestedStateFactory("shire", (State.Compound,), body)} + return type("Hobbiton", (StateChart,), body) + + +def source_state(chart): + return chart.states_map["bag_end"] + + +async def awaited(result): + if isawaitable(result): + result = await result + return result + + +class TestExpandEventId: + def test_plain_name_expands_to_itself(self): + assert _expand_event_id("go") == ["go"] + + def test_error_prefix_dots_every_underscore(self): + assert _expand_event_id("error_foo_bar") == ["error_foo_bar", "error.foo.bar"] + + def test_done_prefixes_keep_the_suffix(self): + assert _expand_event_id("done_state_lonely_mountain") == [ + "done_state_lonely_mountain", + "done.state.lonely_mountain", + ] + assert _expand_event_id("done_invoke_child_1") == [ + "done_invoke_child_1", + "done.invoke.child_1", + ] + + +@pytest.mark.parametrize("nested", NESTED) +@pytest.mark.parametrize("declare", FORMS) +@pytest.mark.parametrize(("attr", "dotted"), PREFIXES) +class TestExpandedIdsRegisterOneEventEach: + def test_both_ids_are_events_of_the_chart(self, attr, dotted, declare, nested): + chart = build_chart(attr, declare, nested) + + assert [event.id for event in chart.events] == [attr, dotted] + assert isinstance(getattr(chart, attr), Event) + assert getattr(chart, attr).id == attr + assert getattr(chart, dotted).id == dotted + + def test_transition_carries_each_id_once(self, attr, dotted, declare, nested): + chart = build_chart(attr, declare, nested) + + (transition,) = source_state(chart).transitions + assert [event.id for event in transition.events] == [attr, dotted] + + async def test_method_call_runs_the_transition(self, attr, dotted, declare, nested, sm_runner): + sm = await sm_runner.start(build_chart(attr, declare, nested)) + assert sm.bag_end.is_active + + await awaited(getattr(sm, attr)()) + + assert sm.green_dragon.is_active + + async def test_send_dotted_id_runs_the_transition( + self, attr, dotted, declare, nested, sm_runner + ): + sm = await sm_runner.start(build_chart(attr, declare, nested)) + + await sm_runner.send(sm, dotted) + + assert sm.green_dragon.is_active + + async def test_allowed_and_enabled_events_list_both_ids( + self, attr, dotted, declare, nested, sm_runner + ): + sm = await sm_runner.start(build_chart(attr, declare, nested)) + + assert [event.id for event in sm.allowed_events] == [attr, dotted] + enabled = await awaited(sm.enabled_events()) + assert [event.id for event in enabled] == [attr, dotted] + + +@pytest.mark.parametrize("nested", NESTED) +@pytest.mark.parametrize(("attr", "dotted"), PREFIXES) +class TestDeclaredArgumentsAreKeptOnEveryId: + def test_name_and_delay(self, attr, dotted, nested): + chart = build_chart(attr, lambda s, t: Event(s.to(t), name="Boom", delay=50), nested) + + assert [(e.id, e.name, e.delay) for e in chart.events] == [ + (attr, "Boom", 50), + (dotted, "Boom", 50), + ] + assert getattr(chart, attr).name == "Boom" + assert getattr(chart, attr).delay == 50 + + def test_internal(self, attr, dotted, nested): + chart = build_chart(attr, lambda s, t: Event(s.to(t), internal=True), nested) + + assert [(e.id, e.internal) for e in chart.events] == [(attr, True), (dotted, True)] + + def test_name_defaults_to_the_humanized_id(self, attr, dotted, nested): + chart = build_chart(attr, lambda s, t: Event(s.to(t)), nested) + + assert [e.name for e in chart.events] == [humanize_id(attr), humanize_id(dotted)] + + +@pytest.mark.parametrize( + "declare", + [ + pytest.param(lambda t: t.from_.any(), id="bare"), + pytest.param(lambda t: Event(t.from_.any(), name="Boom"), id="event"), + ], +) +class TestAnyStateSource: + """``from_.any()`` copies its transition onto every non-final state once, with every id.""" + + @staticmethod + def build(declare): + class Hobbiton(StateChart): + bag_end = State(initial=True) + green_dragon = State() + mordor = State(final=True) + + leave = bag_end.to(green_dragon) + error_foo = declare(mordor) + + return Hobbiton + + def test_one_transition_per_state_carrying_both_ids(self, declare): + chart = self.build(declare) + + for state in (chart.bag_end, chart.green_dragon): + copies = [t for t in state.transitions if t.target is chart.mordor] + assert [str(t.events) for t in copies] == ["error_foo error.foo"] + + def test_diagram_draws_each_copy_once(self, declare): + chart = self.build(declare) + + edges = [(t.source, t.targets) for t in extract(chart).transitions] + assert edges == [ + ("bag_end", ["green_dragon"]), + ("bag_end", ["mordor"]), + ("green_dragon", ["mordor"]), + ] + + async def test_method_call_runs_from_any_state(self, declare, sm_runner): + sm = await sm_runner.start(self.build(declare)) + await sm_runner.send(sm, "leave") + + await awaited(sm.error_foo()) + + assert sm.mordor.is_active + + def test_alias_of_the_same_list_joins_the_copy(self, declare): + class Hobbiton(StateChart): + bag_end = State(initial=True) + mordor = State(final=True) + + flee = declare(mordor) + escape = flee + + assert [str(t.events) for t in Hobbiton.bag_end.transitions] == ["flee escape"] diff --git a/tests/test_statechart_compound.py b/tests/test_statechart_compound.py index 18fe867a..50fdf05e 100644 --- a/tests/test_statechart_compound.py +++ b/tests/test_statechart_compound.py @@ -357,14 +357,7 @@ class shire(State.Compound): assert (registered.delay, registered.internal) == expected assert NamedEvent.visit_pub.name == "Visit the pub" - def test_expanded_id_drops_the_declared_arguments(self): - """A space-separated id declares distinct events, so each one names itself. - - The ``error_`` prefix expands to ``"error_foo error.foo"``, which reuses that format - to mean two spellings of a single event. See - ``test_multiple_ids_from_the_same_event_will_be_converted_to_multiple_events``. - """ - + def test_expanded_id_keeps_the_declared_arguments_on_both_ids(self): class ErrorEvent(StateChart): class shire(State.Compound): bag_end = State(initial=True) @@ -373,8 +366,8 @@ class shire(State.Compound): error_foo = Event(bag_end.to(green_dragon), name="Boom", delay=50) assert [event.id for event in ErrorEvent.events] == ["error_foo", "error.foo"] - assert [event.name for event in ErrorEvent.events] == ["Error foo", "Error foo"] - assert [event.delay for event in ErrorEvent.events] == [0, 0] + assert [event.name for event in ErrorEvent.events] == ["Boom", "Boom"] + assert [event.delay for event in ErrorEvent.events] == [50, 50] async def test_explicit_id_wins_over_the_attribute_name(self, sm_runner): class ExplicitId(StateChart):