diff --git a/docs/state/overview.md b/docs/state/overview.md index e5e040a5aee..ec8ebfb10a4 100644 --- a/docs/state/overview.md +++ b/docs/state/overview.md @@ -52,6 +52,19 @@ A state class is made up of two parts: vars and event handlers. **Event handlers** are functions that modify these vars in response to events. +State declarations cannot reuse framework method or bookkeeping names, such as +`get_state`, `_get_was_touched`, or `dirty_vars`. Reflex checks these names when +creating a state class and when adding vars, event handlers, or route arguments +dynamically. Rename a conflicting declaration and update its references. Ordinary +backend names such as `_count` remain supported. + +For existing apps, setting `REFLEX_STATE_ALLOW_RESERVED_NAMES=1` before starting +Reflex temporarily restores legacy handling of conflicting vars and emits a +deprecation warning. This compatibility option will be removed in Reflex 1.0. +It preserves the old behavior, including any crashes caused by a collision; rename +the conflicting members to resolve those crashes. Existing restrictions on +overriding framework methods still apply. + These are the main concepts to understand how state works in Reflex: ```python eval diff --git a/news/+reserved-state-names.breaking.md b/news/+reserved-state-names.breaking.md new file mode 100644 index 00000000000..50bd1bb2773 --- /dev/null +++ b/news/+reserved-state-names.breaking.md @@ -0,0 +1 @@ +State vars, event handlers, and dynamic route arguments now reject names reserved by framework methods and bookkeeping before registration. Rename conflicting members; `REFLEX_STATE_ALLOW_RESERVED_NAMES=1` temporarily preserves legacy behavior with a deprecation warning until Reflex 1.0. diff --git a/packages/reflex-base/news/+reserved-state-names.deprecation.md b/packages/reflex-base/news/+reserved-state-names.deprecation.md new file mode 100644 index 00000000000..b577e1b8a0b --- /dev/null +++ b/packages/reflex-base/news/+reserved-state-names.deprecation.md @@ -0,0 +1 @@ +Add the temporary `REFLEX_STATE_ALLOW_RESERVED_NAMES` compatibility option for apps migrating away from reserved state names. It defaults to false and will be removed in Reflex 1.0. diff --git a/packages/reflex-base/src/reflex_base/environment.py b/packages/reflex-base/src/reflex_base/environment.py index 3bb80d6970b..b6b0fb72af6 100644 --- a/packages/reflex-base/src/reflex_base/environment.py +++ b/packages/reflex-base/src/reflex_base/environment.py @@ -673,6 +673,9 @@ class EnvironmentVariables: # The maximum size of the reflex state in kilobytes. REFLEX_STATE_SIZE_LIMIT: EnvVar[int] = env_var(1000) + # Temporary compatibility for state declarations that shadow framework members. + REFLEX_STATE_ALLOW_RESERVED_NAMES: EnvVar[bool] = env_var(False) + # Additional paths to include in the hot reload. Separated by a colon. REFLEX_HOT_RELOAD_INCLUDE_PATHS: EnvVar[list[Path]] = env_var([]) diff --git a/reflex/istate/validation.py b/reflex/istate/validation.py new file mode 100644 index 00000000000..e87b66f0cbd --- /dev/null +++ b/reflex/istate/validation.py @@ -0,0 +1,125 @@ +"""Validate the framework namespace before constructing or extending a state.""" + +from functools import cache +from types import FunctionType +from typing import Any + +from reflex_base.environment import environment +from reflex_base.utils import console +from reflex_base.utils.compat import annotations_from_namespace +from reflex_base.utils.exceptions import ( + EventHandlerShadowsBuiltInStateMethodError, + StateValueError, +) +from reflex_base.vars.base import ( + BaseStateMeta, + EvenMoreBasicBaseState, + _linearize_bases, +) + +_FIELD_MAP_NAMES = frozenset({"__fields__", "__own_fields__", "__inherited_fields__"}) + + +@cache +def _reserved_state_members() -> dict[str, Any]: + """Return framework members, excluding state vars and Python protocols. + + Returns: + Reserved names and their original descriptors, without invoking them. + """ + # BaseState must exist before its namespace can be inspected. + from reflex.state import BaseState + + members = {} + for base in reversed(BaseState.__mro__[:-1]): + namespace = vars(base) + members.update( + (name, namespace.get(name)) + for name in namespace.keys() | annotations_from_namespace(namespace).keys() + if not name.startswith("__") or name in _FIELD_MAP_NAMES + ) + for name, field in BaseState.__fields__.items(): + if field.is_var: + members.pop(name, None) + return members + + +def _validate_state_name(name: str, value: Any = None) -> None: + """Reject declarations that replace framework methods or bookkeeping. + + Args: + name: The declared or dynamically registered name. + value: The raw class declaration, when available. + + Raises: + StateValueError: If a declaration uses a reserved name. + EventHandlerShadowsBuiltInStateMethodError: If a method overrides a builtin. + """ + members = _reserved_state_members() + if name not in members: + return + method = value.__func__ if isinstance(value, (classmethod, staticmethod)) else value + if isinstance(method, FunctionType): + if value is members[name] or getattr(method, "__override_base_method__", False): + return + msg = f"The event handler name `{name}` shadows a builtin State method; use a different name instead" + raise EventHandlerShadowsBuiltInStateMethodError(msg) + reason = ( + f"State name `{name}` is reserved by BaseState; use a different name instead." + ) + if environment.REFLEX_STATE_ALLOW_RESERVED_NAMES.get(): + console.deprecate( + feature_name="REFLEX_STATE_ALLOW_RESERVED_NAMES", + reason=reason, + deprecation_version="0.9.12", + removal_version="1.0", + ) + return + msg = f"{reason} Set REFLEX_STATE_ALLOW_RESERVED_NAMES=1 temporarily to retain legacy behavior." + raise StateValueError(msg) + + +class _StateMeta(BaseStateMeta): + """Check state declarations before field collection and subclass initialization.""" + + def __new__( + cls, + name: str, + bases: tuple[type, ...], + namespace: dict[str, Any], + mixin: bool = False, + ) -> type: + """Construct a state after checking its declarations and Python mixins. + + Args: + name: The class name. + bases: The parent classes. + namespace: The unmodified class namespace. + mixin: Whether the class is a state mixin. + + Returns: + The validated state class. + """ + if any(isinstance(base, _StateMeta) for base in bases): + seen = namespace.keys() | annotations_from_namespace(namespace).keys() + for member in seen: + _validate_state_name(member, namespace.get(member)) + for base in _linearize_bases(bases): + if not isinstance(base, _StateMeta) and base not in ( + EvenMoreBasicBaseState, + object, + ): + if isinstance(base, BaseStateMeta): + # Model fields are inherited even when an earlier base + # masks their class attributes in the MRO. + for member in base.__own_fields__: + _validate_state_name(member) + seen.update(base.__own_fields__) + for member, value in vars(base).items(): + if member not in seen and not ( + isinstance(base, BaseStateMeta) + and (member in _FIELD_MAP_NAMES or member == "_mixin") + ): + _validate_state_name(member, value) + seen.update(vars(base)) + return super().__new__(cls, name, bases, namespace, mixin=mixin) diff --git a/reflex/state.py b/reflex/state.py index 1ea080cc863..8f1ec19a1b9 100644 --- a/reflex/state.py +++ b/reflex/state.py @@ -44,7 +44,6 @@ ComputedVarShadowsStateVarError, DynamicComponentInvalidSignatureError, DynamicRouteArgShadowsStateVarError, - EventHandlerShadowsBuiltInStateMethodError, ReflexRuntimeError, SetUndefinedStateVarError, StateMismatchError, @@ -77,6 +76,7 @@ from reflex.istate.proxy import ImmutableMutableProxy as ImmutableMutableProxy from reflex.istate.proxy import MutableProxy, is_mutable_type from reflex.istate.storage import ClientStorageBase +from reflex.istate.validation import _StateMeta, _validate_state_name from reflex.utils import console, format, types from reflex.utils.exec import is_testing_env @@ -426,7 +426,7 @@ def _is_user_descriptor(value: Any) -> bool: }) -class BaseState(EvenMoreBasicBaseState): +class BaseState(EvenMoreBasicBaseState, metaclass=_StateMeta): """The state of the app.""" # A map from the var name to the var. @@ -616,9 +616,6 @@ def __init_subclass__(cls, mixin: bool = False, **kwargs): # Validate the module name. cls._validate_module_name() - # Event handlers should not shadow builtin state methods. - cls._check_overridden_methods() - # Computed vars should not shadow builtin state props. cls._check_overridden_basevars() @@ -825,6 +822,7 @@ def _add_event_handler( name: The name of the event handler. fn: The function to call when the event is triggered. """ + _validate_state_name(name) handler = cls._create_event_handler(fn) cls.event_handlers[name] = handler setattr(cls, name, handler) @@ -1059,29 +1057,6 @@ def _iter_functions(cls) -> Iterator[tuple[str, FunctionType]]: if isinstance(value, FunctionType): yield name, value - @classmethod - def _check_overridden_methods(cls): - """Check for shadow methods and raise error if any. - - Raises: - EventHandlerShadowsBuiltInStateMethodError: When an event handler shadows an inbuilt state method. - """ - overridden_methods = set() - state_base_functions = cls._get_base_functions() - for name, method in cls._iter_functions(): - # Check if the method is overridden and not a dunder method - if ( - not name.startswith("__") - and method.__name__ in state_base_functions - and state_base_functions[method.__name__] != method - and not getattr(method, "__override_base_method__", False) - ): - overridden_methods.add(method.__name__) - - for method_name in overridden_methods: - msg = f"The event handler name `{method_name}` shadows a builtin State method; use a different name instead" - raise EventHandlerShadowsBuiltInStateMethodError(msg) - @classmethod def _check_overridden_basevars(cls): """Check for shadow base vars and raise error if any. @@ -1290,6 +1265,19 @@ def _init_var(cls, name: str, prop: Var): cls._create_setter(name, prop) cls._set_default_value(name, prop) + @classmethod + @_override_base_method + def add_field(cls, name: str, var: Var, default_value: Any): + """Validate a dynamically added field before updating the field map. + + Args: + name: The name of the field to add. + var: The variable to add a field for. + default_value: The default value of the field. + """ + _validate_state_name(name) + super().add_field(name, var, default_value) + @classmethod def add_var(cls, name: str, type_: Any, default_value: Any = None): """Add dynamically a variable to the State. @@ -1442,19 +1430,6 @@ def _get_var_default(cls, name: str, annotation_value: Any) -> Any: except TypeError: return None - @staticmethod - def _get_base_functions() -> builtins.dict[str, FunctionType]: - """Get all functions of the state class excluding dunder methods. - - Returns: - The functions of rx.State class as a dict. - """ - return { - func[0]: func[1] - for func in inspect.getmembers(BaseState, predicate=inspect.isfunction) - if not func[0].startswith("__") - } - @classmethod def _update_substate_inherited_vars(cls, vars_to_add: builtins.dict[str, Var]): """Update the inherited vars of substates recursively when new vars are added. @@ -1507,6 +1482,8 @@ def setup_dynamic_args(cls, args: builtins.dict[str, str]): if not args: return + for name in args: + _validate_state_name(name) cls._check_overwritten_dynamic_args(list(args.keys())) def argsingle_factory(param: str): diff --git a/tests/units/istate/test_proxy.py b/tests/units/istate/test_proxy.py index 285349688b0..ab0412d75c5 100644 --- a/tests/units/istate/test_proxy.py +++ b/tests/units/istate/test_proxy.py @@ -1003,7 +1003,7 @@ def timed(self) -> int: assert IntervalState._interval_computed_var_names == frozenset({"timed"}) -def test_fast_path_skips_names_a_subclass_defines(): +def test_fast_path_skips_names_a_subclass_defines(monkeypatch: pytest.MonkeyPatch): """A subclass defining a fast-pathed framework name keeps the full lookup for it. The fast path bypasses var resolution, so it must not apply to a name the @@ -1011,9 +1011,14 @@ def test_fast_path_skips_names_a_subclass_defines(): backend var named like a framework method). The class is a detached root (not a substate of ``State``) so the shadowed method never reaches the framework paths that other tests exercise on the shared state tree. + + Args: + monkeypatch: Enable legacy reserved names for this lookup regression. """ from reflex.state import BaseState + monkeypatch.setenv("REFLEX_STATE_ALLOW_RESERVED_NAMES", "1") + def get_value(self, key: str): return f"shadow:{key}" @@ -1039,12 +1044,20 @@ def get_value(self, key: str): assert state._get_was_touched == 7 -def test_fast_path_prunes_names_registered_after_class_creation(): - """Vars and handlers added after class creation also leave the fast path.""" +def test_fast_path_prunes_names_registered_after_class_creation( + monkeypatch: pytest.MonkeyPatch, +): + """Vars and handlers added after class creation also leave the fast path. + + Args: + monkeypatch: Enable legacy reserved names for this lookup regression. + """ from reflex_base.constants import RouteArgType from reflex.state import BaseState + monkeypatch.setenv("REFLEX_STATE_ALLOW_RESERVED_NAMES", "1") + DynamicState = type( "DynamicState", (BaseState,), diff --git a/tests/units/istate/test_validation.py b/tests/units/istate/test_validation.py new file mode 100644 index 00000000000..1b12ccd31bc --- /dev/null +++ b/tests/units/istate/test_validation.py @@ -0,0 +1,210 @@ +"""Tests for reserved state names at class creation and dynamic registration.""" + +from unittest.mock import patch + +import pytest +from reflex_base.constants import RouteArgType +from reflex_base.utils.exceptions import StateValueError +from reflex_base.vars.base import EvenMoreBasicBaseState, LiteralVar, computed_var + +from reflex.state import BaseState, _override_base_method + + +@pytest.mark.parametrize( + "name", + [ + "_get_was_touched", + "_update_was_touched", + "_was_touched", + "dirty_vars", + "get_fields", + "get_full_name", + "backend_vars", + "__fields__", + "setvar", + ], +) +@pytest.mark.parametrize("annotated", [False, True]) +def test_reserved_state_var(name: str, annotated: bool, clean_registration_context): + """Reject framework names before state initialization can call them. + + Args: + name: The reserved member to shadow. + annotated: Whether to explicitly annotate the variable. + clean_registration_context: An isolated state registry. + """ + namespace = {"__module__": __name__, "__qualname__": "ShadowState", name: 7} + if annotated: + namespace["__annotations__"] = {name: int} + with pytest.raises(StateValueError, match=name): + type("ShadowState", (BaseState,), namespace) + + +def test_reserved_annotation_only(clean_registration_context): + """Reject a reserved var even when no default is declared. + + Args: + clean_registration_context: An isolated state registry. + """ + with pytest.raises(StateValueError, match="_get_was_touched"): + + class ShadowState(BaseState): + _get_was_touched: int + + +@pytest.mark.parametrize("state_mixin", [False, True]) +def test_reserved_mixin_var(state_mixin: bool, clean_registration_context): + """Reject collisions from both ordinary Python mixins and state mixins. + + Args: + state_mixin: Whether the mixin subclasses BaseState. + clean_registration_context: An isolated state registry. + """ + with pytest.raises(StateValueError, match="_update_was_touched"): + mixin = type( + "Mixin", + (BaseState,) if state_mixin else (), + {"__module__": __name__, "_update_was_touched": 7}, + **({"mixin": True} if state_mixin else {}), + ) + type("MixedState", (mixin, BaseState), {"__module__": __name__}) + + +@pytest.mark.parametrize("name", ["_get_was_touched", "get_fields"]) +def test_reserved_computed_var(name: str, clean_registration_context): + """Reject computed vars that replace framework methods. + + Args: + name: The reserved method to replace. + clean_registration_context: An isolated state registry. + """ + + def value(self) -> int: + """Return a constant computed value.""" + return 7 + + value.__name__ = name + with pytest.raises(StateValueError, match=name): + type( + "ComputedState", + (BaseState,), + {"__module__": __name__, name: computed_var(value)}, + ) + + +@pytest.mark.parametrize("registration", ["var", "route", "event", "field"]) +def test_dynamic_reserved_name(registration: str, clean_registration_context): + """Reject dynamic collisions before any field or event map is changed. + + Args: + registration: The dynamic registration path to exercise. + clean_registration_context: An isolated state registry. + """ + + class DynamicState(BaseState): + """State receiving a dynamic declaration.""" + + fields = dict(DynamicState.get_fields()) + with pytest.raises(StateValueError, match="get_state"): + if registration == "var": + DynamicState.add_var("get_state", int, 7) + elif registration == "route": + DynamicState.setup_dynamic_args({"get_state": RouteArgType.SINGLE}) + elif registration == "event": + DynamicState._add_event_handler("get_state", lambda self: None) + else: + DynamicState.add_field("get_state", LiteralVar.create(7), 7) + assert DynamicState.get_fields() == fields + assert "get_state" not in DynamicState.vars + assert "get_state" not in DynamicState.event_handlers + assert "get_state" not in DynamicState.__dict__ + + +def test_user_vars_and_marked_override(clean_registration_context): + """Keep normal vars, inherited vars, and explicitly marked method overrides. + + Args: + clean_registration_context: An isolated state registry. + """ + + class Parent(BaseState): + value: int = 1 + _backend: int = 2 + + class Child(Parent): + @_override_base_method + def get_value(self, key: str): + """Return a value through a supported framework override.""" + return f"override:{key}" + + parent = Parent() + child = parent.substates[Child.get_name()] + assert isinstance(child, Child) + assert child.value == 1 + assert child._backend == 2 + assert child.get_value("value") == "override:value" + + +def test_non_state_models_keep_their_namespace(): + """Do not reserve Reflex state names on unrelated base models.""" + + class Model(EvenMoreBasicBaseState): + get_state: int = 7 + + assert Model().get_state == 7 + + +def test_legacy_state_names( + monkeypatch: pytest.MonkeyPatch, clean_registration_context +): + """Preserve the old lookup behavior only with the deprecated legacy opt-in. + + Args: + monkeypatch: Set the temporary compatibility environment variable. + clean_registration_context: An isolated state registry. + """ + monkeypatch.setenv("REFLEX_STATE_ALLOW_RESERVED_NAMES", "1") + with patch("reflex_base.utils.console.deprecate") as deprecate: + + class LegacyState(BaseState): + _get_was_touched: int = 7 + + assert LegacyState()._get_was_touched == 7 + deprecate.assert_called_once() + assert deprecate.call_args.kwargs["removal_version"] == "1.0" + + +@pytest.mark.parametrize("name", ["get_fields", "_get_was_touched"]) +@pytest.mark.parametrize("state_first", [False, True]) +def test_reserved_model_mixin(name: str, state_first: bool, clean_registration_context): + """Reject inherited model fields before the field collector sees them. + + Args: + name: The framework name declared as a model field. + state_first: Whether BaseState precedes the model in the MRO. + clean_registration_context: An isolated state registry. + """ + model = type("Model", (EvenMoreBasicBaseState,), {"__module__": __name__, name: 7}) + bases = (BaseState, model) if state_first else (model, BaseState) + with pytest.raises(StateValueError, match=name): + type("MixedState", bases, {"__module__": __name__}) + + +def test_reserved_descriptor(clean_registration_context): + """Reject a descriptor without executing its class access behavior. + + Args: + clean_registration_context: An isolated state registry. + """ + + class Descriptor: + def __get__(self, instance, owner): + """Fail if validation invokes this descriptor.""" + pytest.fail("Reserved descriptor was evaluated") + + with pytest.raises(StateValueError, match="get_fields"): + type( + "DescriptorState", + (BaseState,), + {"__module__": __name__, "get_fields": Descriptor()}, + )