Repository navigation
feat: allow explicit only event handlers #7033
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
945a53f
689dc48
4ca1a0a
acc796d
bfa63ac
77fe965
abd7ca3
c20404d
d81a948
ded1464
1e31e0c
7f4cec0
a80e5a1
5fa41a1
d87a81b
b9137c6
6d256f5
6004f74
964e0da
b9c3e71
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Add the `state_explicit_event_handlers` config option. When enabled, only methods decorated with `@rx.event` become event handlers; other public state methods stay plain Python methods. | ||
|
benedikt-bartscher marked this conversation as resolved.
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Add the `state_explicit_event_handlers` config option. When enabled, only methods decorated with `@rx.event` become event handlers; other public state methods stay plain Python methods. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ | |
| import urllib.parse | ||
|
cubic-dev-ai[bot] marked this conversation as resolved.
|
||
| from collections.abc import Iterable, Iterator, Sequence | ||
| from contextlib import contextmanager | ||
| from contextvars import ContextVar | ||
| from importlib.machinery import PathFinder | ||
| from pathlib import Path, PureWindowsPath | ||
| from types import ModuleType | ||
|
|
@@ -186,6 +187,7 @@ class BaseConfig: | |
| redis_token_expiration: Token expiration time for redis state manager. | ||
| env_file: Path to file containing key-values pairs to load into the environment; Dotenv format. Multiple files may be separated by os.pathsep. Requires the python-dotenv package. | ||
| state_auto_setters: Whether to automatically create setters for state base vars. | ||
| state_explicit_event_handlers: Whether only methods decorated with `@rx.event` become event handlers. By default every public method of a user-defined state is an event handler. | ||
| default_color_mode: The default color mode for the app: "system" (follow the OS preference), "light", or "dark". Applies to the built-in color mode switcher and `color_mode_cond` without requiring a radix theme. | ||
| show_built_with_reflex: Whether to display the sticky "Built with Reflex" badge on all pages. | ||
| is_reflex_cloud: Whether the app is running in the reflex cloud environment. | ||
|
|
@@ -269,6 +271,8 @@ class BaseConfig: | |
|
|
||
| state_auto_setters: bool = False | ||
|
|
||
| state_explicit_event_handlers: bool = False | ||
|
|
||
| default_color_mode: LiteralColorMode = "system" | ||
|
|
||
| show_built_with_reflex: bool | None = None | ||
|
|
@@ -436,10 +440,10 @@ def _post_init(self, **kwargs): | |
| self._non_default_attributes = set(kwargs.keys()) | ||
| self._replace_defaults(**kwargs) | ||
|
|
||
| # Publish for State-class creation so it never re-enters get_config() | ||
| # (which AttributeErrors if a State is defined while rxconfig.py is mid-import). | ||
| global _state_auto_setters | ||
| _state_auto_setters = self.state_auto_setters | ||
| # Publish to the in-progress rxconfig.py import, if any, so States defined | ||
| # after this Config in rxconfig.py resolve their flags from it. | ||
| if (load := _config_load.get()) is not None: | ||
|
benedikt-bartscher marked this conversation as resolved.
|
||
| load.config = self | ||
|
benedikt-bartscher marked this conversation as resolved.
|
||
|
|
||
| if ( | ||
| self.state_manager_mode == constants.StateManagerMode.REDIS | ||
|
|
@@ -854,6 +858,14 @@ def _set_persistent(self, **kwargs): | |
| self._non_default_attributes.update(kwargs) | ||
| self._replace_defaults(**kwargs) | ||
|
|
||
| def _persist_state_flags(self): | ||
| """Export the State-class creation flags to the environment for subprocesses. | ||
|
|
||
| Backend workers import the app module before any config is loaded, so | ||
| their user States resolve these flags from the environment. | ||
| """ | ||
| self._set_persistent(**{name: getattr(self, name) for name in _STATE_FLAGS}) | ||
|
benedikt-bartscher marked this conversation as resolved.
|
||
|
|
||
|
|
||
| # Project-local modules first imported while loading rxconfig.py, and the | ||
| # project root they were recorded under. Evicted before a load from a different | ||
|
|
@@ -949,29 +961,68 @@ def _record_imports() -> Iterator[_ImportRecorder]: | |
| # Protect sys.path from concurrent modification during config loading. | ||
| _load_config_lock = threading.RLock() | ||
|
|
||
| # Cached state_auto_setters so State-class creation never re-enters get_config(). | ||
| _state_auto_setters: bool | None = None | ||
| # Config fields resolved at State-class creation time via _get_state_flag. | ||
| _STATE_FLAGS = ("state_auto_setters", "state_explicit_event_handlers") | ||
|
|
||
|
|
||
| def get_state_auto_setters() -> bool: | ||
| """Return whether state auto-setters are enabled, without importing rxconfig. | ||
| @dataclasses.dataclass | ||
| class _ConfigLoad: | ||
| """An in-progress rxconfig.py import, holding its Config once constructed.""" | ||
|
|
||
| config: Config | None = None | ||
|
|
||
|
|
||
| Reads the value cached when the Config was built. Before any Config exists | ||
| (e.g. a State defined inside rxconfig.py during its import), falls back to the | ||
| REFLEX_STATE_AUTO_SETTERS env var, then the default (False). This never calls | ||
| get_config() or imports rxconfig, so it cannot re-enter config loading. | ||
| # Set for the duration of _get_config() so States defined inside rxconfig.py see | ||
| # the Config being loaded (or env/default before it exists), never a stale one. | ||
| _config_load: ContextVar[_ConfigLoad | None] = ContextVar("_config_load", default=None) | ||
|
|
||
|
|
||
| def _get_state_flag(name: str) -> bool: | ||
| """Resolve a boolean State-class creation flag without loading rxconfig. | ||
|
|
||
| Uses the Config of the in-progress rxconfig.py import, else the Config cached | ||
| on the active RegistrationContext, else the REFLEX_<NAME> env var (which | ||
| `reflex run` sets for its backend workers), then False. Never loads rxconfig, | ||
| since an rxconfig.py may import the very module defining the State. | ||
|
|
||
| Args: | ||
| name: The config field name. | ||
|
|
||
| Returns: | ||
| Whether state auto-setters are enabled. | ||
| The resolved flag value. | ||
| """ | ||
| if _state_auto_setters is not None: | ||
| return _state_auto_setters | ||
| env_val = os.environ.get(Config._prefixes[0] + "STATE_AUTO_SETTERS") | ||
| load = _config_load.get() | ||
| config = ( | ||
| load.config | ||
| if load is not None | ||
| else RegistrationContext.ensure_context()._config | ||
| ) | ||
| if config is not None: | ||
| return getattr(config, name) | ||
| env_val = os.environ.get(Config._prefixes[0] + name.upper()) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the env var is not set and no config is cached, the flag falls back to Repro with an app that sets from uvicorn.importer import import_from_string
import_from_string("pr7033_repro.pr7033_repro:app")
sorted(ExplicitState.event_handlers)
# [..., 'mixin_helper', ..., 'step', ...] <- undecorated helpers are handlersThe docs mention this. But for a security switch, a silent fallback to the permissive mode is the wrong failure. Please make it fail closed. For example: record the value the first user State was built with. When the config is loaded (e.g. in |
||
| if env_val and env_val.strip(): | ||
| return interpret_env_var_value(env_val, bool, "state_auto_setters") | ||
| return interpret_env_var_value(env_val, bool, name) | ||
| return False | ||
|
|
||
|
|
||
| def get_state_auto_setters() -> bool: | ||
| """Return whether state auto-setters are enabled, without importing rxconfig. | ||
|
|
||
| Returns: | ||
| Whether state auto-setters are enabled. | ||
| """ | ||
| return _get_state_flag("state_auto_setters") | ||
|
|
||
|
|
||
| def get_state_explicit_event_handlers() -> bool: | ||
| """Return whether only `@rx.event` methods become event handlers, without importing rxconfig. | ||
|
|
||
| Returns: | ||
| Whether explicit event handlers are required. | ||
| """ | ||
| return _get_state_flag("state_explicit_event_handlers") | ||
|
|
||
|
|
||
| def _get_config(project_root: Path | None = None) -> Config: | ||
| """Import rxconfig.py fresh from the project root and return its config. | ||
|
|
||
|
|
@@ -998,6 +1049,7 @@ def _get_config(project_root: Path | None = None) -> Config: | |
| # which removal by value could confuse with caller-owned ones. | ||
| cwd = str(project_root) | ||
| sys.path.insert(0, cwd) | ||
| load_token = _config_load.set(_ConfigLoad()) | ||
| try: | ||
| # Never cache rxconfig itself — each load goes to disk so different | ||
| # RegistrationContexts hold independent Config instances. | ||
|
|
@@ -1030,6 +1082,7 @@ def _get_config(project_root: Path | None = None) -> Config: | |
| ) | ||
| return rxconfig.config | ||
| finally: | ||
| _config_load.reset(load_token) | ||
| for i, entry in enumerate(sys.path): | ||
| if entry is cwd: | ||
| del sys.path[i] | ||
|
|
@@ -1094,13 +1147,16 @@ def get_config(reload: bool = False) -> Config: | |
| def reload_config() -> Config: | ||
| """Force a fresh load of the config into the current RegistrationContext. | ||
|
|
||
| Clears any cached config on the current context and reloads rxconfig.py | ||
| from disk. | ||
| Reloads rxconfig.py from disk and replaces any cached config on the current | ||
| context. If the load fails, the context keeps its previous config. | ||
|
|
||
| Returns: | ||
| The freshly loaded app config. | ||
| """ | ||
| ctx = RegistrationContext.ensure_context() | ||
| config = _get_config() | ||
| ctx._set_config(config) | ||
| # Load and publish under one lock so concurrent reloads of a shared context | ||
| # cannot publish an older load over a newer one. | ||
| with _load_config_lock: | ||
| config = _get_config() | ||
| ctx._set_config(config) | ||
| return config | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -297,6 +297,8 @@ def _scan_detach(value: Any, memo: dict[int, Any], active: set[int]) -> Any: | |
| BACKGROUND_TASK_MARKER = "_reflex_background_task" | ||
| SUPERSEDES_MARKER = "_reflex_supersedes" | ||
| EVENT_ACTIONS_MARKER = "_rx_event_actions" | ||
| EVENT_MARKER = "_rx_event" | ||
| UNDECORATED_STATE_METHOD_MARKER = "_rx_undecorated_state_method" | ||
| UPLOAD_FILES_CLIENT_HANDLER = "uploadFiles" | ||
|
|
||
| # Payload key listing the names of the extra bound handler args in an upload | ||
|
|
@@ -2395,6 +2397,28 @@ def check_fn_match_arg_spec( | |
| raise EventFnArgMismatchError(msg) | ||
|
|
||
|
|
||
| def _check_not_undecorated_state_method(value: Any, key: str | None) -> None: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This check runs only when a trigger is compiled. The runtime paths through
Neither message mentions |
||
| """Reject a state method that explicit event handler mode left undecorated. | ||
|
|
||
| Args: | ||
| value: The value wired to the event trigger. | ||
| key: The name of the event trigger. | ||
|
|
||
| Raises: | ||
| EventHandlerValueError: If value is an undecorated state method. | ||
| """ | ||
| if getattr(value, UNDECORATED_STATE_METHOD_MARKER, False): | ||
| from reflex_base.utils.exceptions import EventHandlerValueError | ||
|
|
||
| msg = ( | ||
| f"{value.__qualname__} is wired to {key or 'an event trigger'} but is not an " | ||
| "event handler. Decorate it with `@rx.event`: with " | ||
| "`state_explicit_event_handlers` enabled, undecorated methods stay plain " | ||
| "methods." | ||
| ) | ||
| raise EventHandlerValueError(msg) | ||
|
|
||
|
|
||
| def event_sequence(events: Any) -> Sequence[Any]: | ||
| """Get events as a sequence, wrapping a single event-like value in a list. | ||
|
|
||
|
|
@@ -2436,12 +2460,15 @@ def call_event_fn( | |
| The event-like values from calling the function. | ||
|
|
||
| Raises: | ||
| EventHandlerValueError: If the lambda returns an unusable value. | ||
| EventHandlerValueError: If fn is an undecorated state method or returns | ||
| an unusable value. | ||
| """ | ||
| # Import here to avoid circular imports. | ||
| from reflex_base.event import EventHandler, EventSpec | ||
| from reflex_base.utils.exceptions import EventHandlerValueError | ||
|
|
||
| _check_not_undecorated_state_method(fn, key) | ||
|
|
||
| parsed_args, _ = parse_args_spec(arg_spec) | ||
|
|
||
| parameters = inspect.signature(fn).parameters | ||
|
|
@@ -2528,6 +2555,7 @@ def _dispatch_mixed_event_var(event_like_var: Var) -> FunctionVar: | |
|
|
||
| # Make sure the event spec is valid. | ||
| if not isinstance(e, (EventSpec, FunctionVar, EventVar)): | ||
| _check_not_undecorated_state_method(e, key) | ||
| hint = "" | ||
| if isinstance(e, VarOperationCall): | ||
| hint = " Hint: use `fn.partial(...)` instead of calling the FunctionVar directly." | ||
|
|
@@ -3163,6 +3191,8 @@ class EventNamespace: | |
| BACKGROUND_TASK_MARKER = BACKGROUND_TASK_MARKER | ||
| SUPERSEDES_MARKER = SUPERSEDES_MARKER | ||
| EVENT_ACTIONS_MARKER = EVENT_ACTIONS_MARKER | ||
| EVENT_MARKER = EVENT_MARKER | ||
| UNDECORATED_STATE_METHOD_MARKER = UNDECORATED_STATE_METHOD_MARKER | ||
|
Comment on lines
+3194
to
+3195
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These two markers are internal, but adding them to |
||
| _EVENT_FIELDS = _EVENT_FIELDS | ||
| FORM_DATA = FORM_DATA | ||
| FORM_SUBMIT_MAPPING = FORM_SUBMIT_MAPPING | ||
|
|
@@ -3287,6 +3317,9 @@ def wrapper( | |
| if getattr(func, "__name__", "").startswith("_"): | ||
| msg = "Event handlers cannot be private." | ||
| raise ValueError(msg) | ||
| # Lets State tell decorated methods apart when | ||
| # state_explicit_event_handlers is enabled. | ||
| setattr(func, EVENT_MARKER, True) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This rx.event(Obj().m)
# AttributeError: 'method' object has no attribute '_rx_event' and no __dict__ for setting new attributes
rx.event(len)
# AttributeError: 'builtin_function_or_method' object has no attribute '_rx_event' ...On |
||
|
|
||
| qualname: str | None = getattr(func, "__qualname__", None) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,10 +32,13 @@ | |
| ) | ||
|
|
||
| from reflex_base import constants | ||
| from reflex_base.config import get_state_auto_setters, get_state_explicit_event_handlers | ||
| from reflex_base.constants.state import FIELD_MARKER | ||
| from reflex_base.environment import PerformanceMode, auto_reload_cooldown, environment | ||
| from reflex_base.event import ( | ||
| EVENT_ACTIONS_MARKER, | ||
| EVENT_MARKER, | ||
| UNDECORATED_STATE_METHOD_MARKER, | ||
| Event, | ||
| EventHandler, | ||
| EventSpec, | ||
|
|
@@ -768,8 +771,11 @@ def __init_subclass__(cls, mixin: bool = False, **kwargs): | |
| ) | ||
| raise StateValueError(msg) | ||
|
|
||
| explicit = cls.is_user_defined() and get_state_explicit_event_handlers() | ||
| cls._bind_fields() | ||
| cls._bind_mixin_members() | ||
| cls._bind_mixin_members(explicit) | ||
| if explicit: | ||
| cls._mark_undecorated_methods() | ||
|
|
||
| # Set the base and computed vars. | ||
| cls.base_vars = { | ||
|
|
@@ -803,7 +809,7 @@ def __init_subclass__(cls, mixin: bool = False, **kwargs): | |
|
|
||
| # Set up the event handlers. | ||
| for name, fn in list(cls.__dict__.items()): | ||
| if cls._item_is_event_handler(name, fn): | ||
| if cls._item_is_event_handler(name, fn, explicit): | ||
| handler = cls._create_event_handler(fn) | ||
| cls.event_handlers[name] = handler | ||
| setattr(cls, name, handler) | ||
|
|
@@ -845,11 +851,14 @@ def _bind_fields(cls) -> None: | |
| fields[name] = f | ||
|
|
||
| @classmethod | ||
| def _bind_mixin_members(cls) -> None: | ||
| def _bind_mixin_members(cls, explicit: bool) -> None: | ||
| """Copy the computed vars and event handler functions of mixins onto this class. | ||
|
|
||
| A member is copied only where this class resolves the name to the mixin's | ||
| member, so this class or an earlier base overrides it as usual. | ||
|
|
||
| Args: | ||
| explicit: Only copy event handler functions decorated with `@rx.event`. | ||
| """ | ||
| for mixin_cls in cls._mixins(): | ||
| for name, value in mixin_cls.__dict__.items(): | ||
|
|
@@ -864,7 +873,7 @@ def _bind_mixin_members(cls) -> None: | |
| _var_data=VarData.from_state(cls), | ||
| ), | ||
| ) | ||
| elif cls._item_is_event_handler(name, value): | ||
| elif cls._item_is_event_handler(name, value, explicit): | ||
| fn = cls._copy_fn(value) | ||
| fn.__qualname__ = f"{cls.__name__}.{name}" | ||
| setattr(cls, name, fn) | ||
|
|
@@ -909,12 +918,13 @@ def _copy_fn(fn: Callable) -> Callable: | |
| return newfn | ||
|
|
||
| @staticmethod | ||
| def _item_is_event_handler(name: str, value: Any) -> bool: | ||
| def _item_is_event_handler(name: str, value: Any, explicit: bool = False) -> bool: | ||
| """Check if the item is an event handler. | ||
|
|
||
| Args: | ||
| name: The name of the item. | ||
| value: The value of the item. | ||
| explicit: Only count functions decorated with `@rx.event`. | ||
|
|
||
| Returns: | ||
| Whether the item is an event handler. | ||
|
|
@@ -925,8 +935,27 @@ def _item_is_event_handler(name: str, value: Any) -> bool: | |
| and not isinstance(value, EventHandler) | ||
| and not getattr(value, "__override_base_method__", False) | ||
| and hasattr(value, "__code__") | ||
| and (not explicit or getattr(value, EVENT_MARKER, False)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A decorator without def require_login(f):
def inner(self, *a, **k):
return f(self, *a, **k)
return inner
class S(rx.State):
@require_login
@rx.event
def save(self): ...
rx.button(on_click=S.save)The wrapper's Please give this case a clear error that names the flag. Or at least document that |
||
| ) | ||
|
|
||
| @classmethod | ||
| def _mark_undecorated_methods(cls) -> None: | ||
| """Mark the undecorated methods of this class and its mixins. | ||
|
|
||
| Wiring a marked method to an event trigger raises a targeted error. Only | ||
| methods defined in the class body are marked: a callback defined | ||
| elsewhere may be shared. | ||
| """ | ||
| for owner in (cls, *cls._mixins()): | ||
| prefix = owner.__dict__.get("__original_qualname__", owner.__qualname__) | ||
| for name, value in owner.__dict__.items(): | ||
| if ( | ||
| cls._item_is_event_handler(name, value) | ||
| and not getattr(value, EVENT_MARKER, False) | ||
| and value.__qualname__ == f"{prefix}.{name}" | ||
| ): | ||
| setattr(value, UNDECORATED_STATE_METHOD_MARKER, True) | ||
|
|
||
| @classmethod | ||
| def _evaluate(cls, f: Callable[[Self], Any], of_type: type | None = None) -> Var: | ||
| """Evaluate a function to a ComputedVar. Experimental. | ||
|
|
@@ -1008,6 +1037,7 @@ def _handle_local_def(cls): | |
| proposed_name = f"{cls.__name__}_{ix}" | ||
| setattr(reflex.istate.dynamic, proposed_name, cls) | ||
| cls.__original_name__ = cls.__name__ | ||
| cls.__original_qualname__ = cls.__qualname__ | ||
| cls.__original_module__ = cls.__module__ | ||
| cls.__name__ = cls.__qualname__ = proposed_name | ||
| cls.__module__ = reflex.istate.dynamic.__name__ | ||
|
|
@@ -1292,7 +1322,6 @@ def _init_var(cls, name: str, prop: Var): | |
| Raises: | ||
| VarTypeError: if the variable has an incorrect type | ||
| """ | ||
| from reflex_base.config import get_state_auto_setters | ||
| from reflex_base.utils.exceptions import VarTypeError | ||
|
|
||
| if not types.is_valid_var_type(prop._var_type): | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.