Repository navigation
feat: allow explicit only event handlers - #7033
benedikt-bartscher wants to merge 20 commits into
Conversation
[Medium impact] No new actionable issue was found in the reviewed changes. SummaryAdds
Reviews (24) · Last reviewed commit: "Merge remote-tracking branch 'upstream/m..." · Reviewed by Greptile |
Merging this PR will improve performance by 4.46%2 warnings
Performance Changes
Tip Curious why performance improved? Comment Comparing |
There was a problem hiding this comment.
All reported issues were addressed across 8 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
b2a522f to
945a53f
Compare
There was a problem hiding this comment.
All reported issues were addressed across 8 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
masenf
left a comment
There was a problem hiding this comment.
great, this gives us a lever and a chicken switch for eventually defaulting to requiring the decorator for event handlers.
need to ensure that reflex-enterprise's auth plugin and other integration points can still work with this new structure, but it seems like it would at first glance.
|
@masenf i think this PR is important from a security perspective - can we get it in? |
FarhanAliRaza
left a comment
There was a problem hiding this comment.
Tested with a small app that sets state_explicit_event_handlers=True. The state has decorated handlers (plain, background=True, stop_propagation=True, an on_load handler, an explicit setter), undecorated helper methods called from the handlers, a mixin with one decorated and one undecorated method, and two rx.ComponentState instances. A computed var lists the state's registered event handler names on the page.
Ran it with reflex run in dev mode and exercised every control in the browser. All decorated handlers, the background handler, stop propagation, the mixin handler, the setter and both component states work. No console errors.
The handler list rendered from the backend still contains the undecorated helpers. The compile process sees the flag, but the backend worker does not, because it imports the app module before any config is loaded. A worker-style import (uvicorn.importer.import_from_string) reproduces this without a browser. I verified a candidate fix in _get_state_flag locally: the worker-style import then registers only the decorated methods, and the state, event and config unit tests pass.
The PR unit tests pass as-is. See the inline comments for the requested changes.
…d workers honor state_explicit_event_handlers
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…oading config during State creation
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
69f329d to
d81a948
Compare
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…in/disable-implicit-event-handlers # Conflicts: # tests/units/test_state.py # tests/units/utils/test_exec.py
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Move marking undecorated state methods out of _item_is_event_handler into _mark_undecorated_methods, which __init_subclass__ runs once in explicit mode for the state and its mixins.
| ) | ||
| if config is not None: | ||
| return getattr(config, name) | ||
| env_val = os.environ.get(Config._prefixes[0] + name.upper()) |
There was a problem hiding this comment.
If the env var is not set and no config is cached, the flag falls back to False with no warning. Every public method then becomes an event handler again. This happens for any entry point other than reflex run, for example uvicorn/gunicorn pointed at the app module, or a test suite that imports the state modules.
Repro with an app that sets state_explicit_event_handlers=True, and no REFLEX_STATE_EXPLICIT_EVENT_HANDLERS in the environment:
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 App.__init__), raise if the config value differs.
| if not frontend_present: | ||
| notify_backend(host) | ||
|
|
||
| get_config()._persist_state_flags() |
There was a problem hiding this comment.
The exported env var also wins over later rxconfig.py edits, because update_from_env applies REFLEX_* over the file values. In dev, the worker reloads with the value from startup.
Repro: start reflex run with state_explicit_event_handlers=False, then set it to True and save. After the reload, step and mixin_helper are still registered handlers. Only a full restart applies the change. The reverse edit (True to False) is also ignored.
Please do not let the persisted value override the file, or document that a change to this flag needs a restart.
| raise EventFnArgMismatchError(msg) | ||
|
|
||
|
|
||
| def _check_not_undecorated_state_method(value: Any, key: str | None) -> None: |
There was a problem hiding this comment.
This check runs only when a trigger is compiled. The runtime paths through Event.from_event_type and fix_events do not call it. Both cases below work in implicit mode, so users who migrate will hit them:
app.add_page(page, on_load=State.undecorated)is accepted. Every load of the page then fails in the backend withValueError: Unexpected event type, <class 'function'>. The browser shows no error, andon_loaddoes not run.- An event handler that does
return State.undecorated(or yields it) fails withTypeError: Your handler State.chain must only return/yield: None, Events....
Neither message mentions @rx.event or state_explicit_event_handlers. Please call this check on the runtime path too, so that on_load and return/yield values get the same targeted error. Please also add tests for both cases.
| 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)) |
There was a problem hiding this comment.
A decorator without functools.wraps on top of @rx.event loses this marker. So the method silently stops being an event handler:
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)EventFnArgMismatchError: Event on_click only provides 1 arguments, but <function require_login.<locals>.inner ...> requires at least 3 arguments to be passed to the event handler.
The wrapper's __qualname__ is require_login.<locals>.inner, so _mark_undecorated_methods also skips it, and the targeted error does not fire. The same code works on main. It also works with functools.wraps, or with @rx.event as the outer decorator.
Please give this case a clear error that names the flag. Or at least document that @rx.event must be the outermost decorator, or that wrappers must use functools.wraps.
| raise ValueError(msg) | ||
| # Lets State tell decorated methods apart when | ||
| # state_explicit_event_handlers is enabled. | ||
| setattr(func, EVENT_MARKER, True) |
There was a problem hiding this comment.
This setattr runs in the default mode too. So rx.event(x) now raises for any x without a __dict__:
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 main, both calls return the value unchanged. I did not find this usage in the repo or the docs, but it is a behavior change for users who do not enable the flag. Please guard the setattr, or give a clear error.
| EVENT_MARKER = EVENT_MARKER | ||
| UNDECORATED_STATE_METHOD_MARKER = UNDECORATED_STATE_METHOD_MARKER |
There was a problem hiding this comment.
These two markers are internal, but adding them to EventNamespace makes them public as rx.event.EVENT_MARKER and rx.event.UNDECORATED_STATE_METHOD_MARKER. reflex/state.py imports them from reflex_base.event directly, so the namespace entries are not needed. Please remove them, so that no new public API has to be supported.
FarhanAliRaza
left a comment
There was a problem hiding this comment.
Re-tested at b9137c6 with the same example app as before, with state_explicit_event_handlers=True. The app has decorated handlers (plain, background, stop_propagation, on_load, a setter), undecorated helpers, a mixin, and two ComponentState instances. I ran it with reflex run in dev mode and drove it in a headless browser.
Both earlier requests are fixed. The backend worker now registers only the decorated handlers. Wiring an undecorated method to a trigger gives a clear error that names @rx.event. I also sent a crafted websocket event for an undecorated method, and the backend rejected it. All the handlers work in the browser, with no console errors.
I also tried edge cases around the flag: on_load, handlers that return an undecorated method, stacked decorators, subclass overrides, decentralized handlers, and a worker-style import without the env var. The targeted state, event, config and exec unit tests pass. ruff and pyright are clean.
See the inline comments for the remaining points.
…ent-handlers # Conflicts: # tests/units/test_state.py # tests/units/utils/test_exec.py
Closes #7032