From 9dec01f77aad64cd194f2a2ae3f7bc9a4e636473 Mon Sep 17 00:00:00 2001 From: Benedikt Bartscher Date: Fri, 16 Aug 2024 22:56:20 +0200 Subject: [PATCH 01/11] fix dynamic route vars silently shadow all other vars --- reflex/state.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/reflex/state.py b/reflex/state.py index b0c6646ce9b..73d3d9d8664 100644 --- a/reflex/state.py +++ b/reflex/state.py @@ -989,6 +989,7 @@ def setup_dynamic_args(cls, args: dict[str, str]): Args: args: a dict of args """ + cls._check_overwritten_dynamic_args(args) def argsingle_factory(param): @ComputedVar @@ -1019,6 +1020,24 @@ def inner_func(self) -> List: # Reinitialize dependency tracking dicts. cls._init_var_dependency_dicts() + @classmethod + def _check_overwritten_dynamic_args(cls, args: dict[str, str]): + """Check if dynamic args are shadowing existing vars. Recursively checks all child states. + + Args: + args: a dict of args + + Raises: + NameError: If a dynamic arg is shadowing an existing var. + """ + for arg in args: + if arg in cls.vars: + raise NameError( + f"Dynamic route arg '{arg}' is shadowing an existing var in {cls.__module__}.{cls.__name__}" + ) + for substate in cls.get_substates(): + substate._check_overwritten_dynamic_args(args) + def __getattribute__(self, name: str) -> Any: """Get the state var. From 95705c896ddc5e4f7cd66f5b9e1967c8f75ed280 Mon Sep 17 00:00:00 2001 From: Benedikt Bartscher Date: Fri, 16 Aug 2024 23:12:03 +0200 Subject: [PATCH 02/11] add test --- tests/test_app.py | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/tests/test_app.py b/tests/test_app.py index 489ace51105..1ba14f6c3f9 100644 --- a/tests/test_app.py +++ b/tests/test_app.py @@ -917,6 +917,32 @@ def comp_dynamic(self) -> str: on_load_internal = OnLoadInternalState.on_load_internal.fn +def test_dynamic_arg_shadow( + index_page, + windows_platform: bool, + token: str, + app_module_mock: unittest.mock.Mock, + mocker, +): + """Create app with dynamic route var and try to add a page with a dynamic arg that shadows a state var. + + Args: + index_page: The index page. + windows_platform: Whether the system is windows. + token: a Token. + app_module_mock: Mocked app module. + mocker: pytest mocker object. + """ + arg_name = "counter" + route = f"/test/[{arg_name}]" + if windows_platform: + route.lstrip("/").replace("/", "\\") + app = app_module_mock.app = App(state=DynamicState) + assert app.state is not None + with pytest.raises(NameError): + app.add_page(index_page, route=route, on_load=DynamicState.on_load) # type: ignore + + @pytest.mark.asyncio async def test_dynamic_route_var_route_change_completed_on_load( index_page, From 0af907a36c1ffdafe42e591ce4ad341356a2cfac Mon Sep 17 00:00:00 2001 From: Benedikt Bartscher Date: Mon, 19 Aug 2024 17:19:39 +0200 Subject: [PATCH 03/11] fix: allow multiple dynamic routes with the same arg --- reflex/state.py | 6 +++++- reflex/vars.py | 3 +++ reflex/vars.pyi | 8 +++++++- 3 files changed, 15 insertions(+), 2 deletions(-) diff --git a/reflex/state.py b/reflex/state.py index 73d3d9d8664..7ff4b7ee7de 100644 --- a/reflex/state.py +++ b/reflex/state.py @@ -996,6 +996,7 @@ def argsingle_factory(param): def inner_func(self) -> str: return self.router.page.params.get(param, "") + inner_func._var_is_dynamic_route = True return inner_func def arglist_factory(param): @@ -1003,6 +1004,7 @@ def arglist_factory(param): def inner_func(self) -> List: return self.router.page.params.get(param, []) + inner_func._var_is_dynamic_route = True return inner_func for param, value in args.items(): @@ -1031,7 +1033,9 @@ def _check_overwritten_dynamic_args(cls, args: dict[str, str]): NameError: If a dynamic arg is shadowing an existing var. """ for arg in args: - if arg in cls.vars: + if ( + arg in cls.computed_vars and not cls.vars[arg]._var_is_dynamic_route + ) or arg in cls.base_vars: raise NameError( f"Dynamic route arg '{arg}' is shadowing an existing var in {cls.__module__}.{cls.__name__}" ) diff --git a/reflex/vars.py b/reflex/vars.py index ffaf164552b..d77e9805c62 100644 --- a/reflex/vars.py +++ b/reflex/vars.py @@ -2194,6 +2194,9 @@ class ComputedVar(Var, property): # Extra metadata associated with the Var _var_data: Optional[VarData] = dataclasses.field(default=None) + # Whether the var is a (automatically created) dynamic route var + _var_is_dynamic_route: bool = dataclasses.field(default=False) + def __init__( self, fget: Callable[[BaseState], Any], diff --git a/reflex/vars.pyi b/reflex/vars.pyi index 47d433374d9..cda596d23ee 100644 --- a/reflex/vars.pyi +++ b/reflex/vars.pyi @@ -168,8 +168,14 @@ class BaseVar(Var): @dataclass(init=False) class ComputedVar(Var): - _var_cache: bool fget: FunctionType + _cache: bool = False + _backend: bool = False + _initial_value: Any | types.Unset = types.Unset() + _static_deps: set[str] + _auto_deps: bool = True + _update_interval: Optional[datetime.timedelta] = None + _var_is_dynamic_route: bool = False @property def _cache_attr(self) -> str: ... def __get__(self, instance, owner): ... From 4c27e812bc6beab39ab044313868efbaa707fd2f Mon Sep 17 00:00:00 2001 From: Benedikt Bartscher Date: Mon, 19 Aug 2024 17:28:55 +0200 Subject: [PATCH 04/11] add test for multiple dynamic args with the same name --- tests/test_app.py | 32 ++++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/tests/test_app.py b/tests/test_app.py index 1ba14f6c3f9..78fcf9c5348 100644 --- a/tests/test_app.py +++ b/tests/test_app.py @@ -67,7 +67,7 @@ class EmptyState(BaseState): @pytest.fixture -def index_page(): +def index_page() -> ComponentCallable: """An index page. Returns: @@ -81,7 +81,7 @@ def index(): @pytest.fixture -def about_page(): +def about_page() -> ComponentCallable: """An about page. Returns: @@ -943,6 +943,34 @@ def test_dynamic_arg_shadow( app.add_page(index_page, route=route, on_load=DynamicState.on_load) # type: ignore +def test_multiple_dynamic_args( + index_page: ComponentCallable, + windows_platform: bool, + token: str, + app_module_mock: unittest.mock.Mock, + mocker, +): + """Create app with multiple dynamic route vars with the same name. + + Args: + index_page: The index page. + windows_platform: Whether the system is windows. + token: a Token. + app_module_mock: Mocked app module. + mocker: pytest mocker object. + """ + arg_name = "my_arg" + route = f"/test/[{arg_name}]" + route2 = f"/test2/[{arg_name}]" + if windows_platform: + route = route.lstrip("/").replace("/", "\\") + route2 = route2.lstrip("/").replace("/", "\\") + app = app_module_mock.app = App(state=DynamicState) + assert app.state is not None + app.add_page(index_page, route=route, on_load=DynamicState.on_load) # type: ignore + app.add_page(index_page, route=route2, on_load=DynamicState.on_load) # type: ignore + + @pytest.mark.asyncio async def test_dynamic_route_var_route_change_completed_on_load( index_page, From b1fb00d86654947586c728ea9dbcb853e4e2a442 Mon Sep 17 00:00:00 2001 From: Benedikt Bartscher Date: Mon, 19 Aug 2024 18:50:37 +0200 Subject: [PATCH 05/11] avoid side-effects with DynamicState tests --- tests/test_app.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/tests/test_app.py b/tests/test_app.py index 78fcf9c5348..15557504ec7 100644 --- a/tests/test_app.py +++ b/tests/test_app.py @@ -918,7 +918,7 @@ def comp_dynamic(self) -> str: def test_dynamic_arg_shadow( - index_page, + index_page: ComponentCallable, windows_platform: bool, token: str, app_module_mock: unittest.mock.Mock, @@ -965,15 +965,14 @@ def test_multiple_dynamic_args( if windows_platform: route = route.lstrip("/").replace("/", "\\") route2 = route2.lstrip("/").replace("/", "\\") - app = app_module_mock.app = App(state=DynamicState) - assert app.state is not None - app.add_page(index_page, route=route, on_load=DynamicState.on_load) # type: ignore - app.add_page(index_page, route=route2, on_load=DynamicState.on_load) # type: ignore + app = app_module_mock.app = App(state=EmptyState) + app.add_page(index_page, route=route) + app.add_page(index_page, route=route2) @pytest.mark.asyncio async def test_dynamic_route_var_route_change_completed_on_load( - index_page, + index_page: ComponentCallable, windows_platform: bool, token: str, app_module_mock: unittest.mock.Mock, From 8ccd2190d5c14c8c21df6e9fe7170ab0d4c98a2b Mon Sep 17 00:00:00 2001 From: Benedikt Bartscher Date: Mon, 19 Aug 2024 23:27:31 +0200 Subject: [PATCH 06/11] fix dynamic route integration test which shadowed a var --- integration/test_dynamic_routes.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/integration/test_dynamic_routes.py b/integration/test_dynamic_routes.py index 0ce4ef2c91b..a1cf88ea7cc 100644 --- a/integration/test_dynamic_routes.py +++ b/integration/test_dynamic_routes.py @@ -23,10 +23,13 @@ def DynamicRoute(): class DynamicState(rx.State): order: List[str] = [] - page_id: str = "" + input_page_id: str = "" def on_load(self): - self.order.append(f"{self.router.page.path}-{self.page_id or 'no page id'}") + self.input_page_id = self.page_id # type: ignore + self.order.append( + f"{self.router.page.path}-{self.input_page_id or 'no page id'}" + ) def on_load_redir(self): query_params = self.router.page.params @@ -36,7 +39,7 @@ def on_load_redir(self): @rx.var def next_page(self) -> str: try: - return str(int(self.page_id) + 1) + return str(int(self.input_page_id) + 1) except ValueError: return "0" @@ -47,7 +50,7 @@ def index(): is_read_only=True, id="token", ), - rc.input(value=DynamicState.page_id, is_read_only=True, id="page_id"), + rc.input(value=DynamicState.input_page_id, is_read_only=True, id="page_id"), rc.input( value=DynamicState.router.page.raw_path, is_read_only=True, From c0539483f0053f485cc447fd0cd3c5b1fa29efaa Mon Sep 17 00:00:00 2001 From: Benedikt Bartscher Date: Tue, 10 Sep 2024 22:49:54 +0200 Subject: [PATCH 07/11] fix darglint --- reflex/ivars/base.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/reflex/ivars/base.py b/reflex/ivars/base.py index 9132983d6fb..be1f13532a9 100644 --- a/reflex/ivars/base.py +++ b/reflex/ivars/base.py @@ -1420,7 +1420,7 @@ def __init__( auto_deps: Whether var dependencies should be auto-determined. interval: Interval at which the computed var should be updated. backend: Whether the computed var is a backend var. - var_is_dynamic_route: Whether the var is a (automatically created) dynamic route var. + _var_is_dynamic_route: Whether the var is a (automatically created) dynamic route var. **kwargs: additional attributes to set on the instance Raises: From 730a5582ddbed672b7071abe8b275e42be575e68 Mon Sep 17 00:00:00 2001 From: Benedikt Bartscher Date: Tue, 10 Sep 2024 23:44:29 +0200 Subject: [PATCH 08/11] refactor to DynamicRouteVar --- reflex/ivars/base.py | 18 +++++++++--------- reflex/state.py | 23 ++++++++++++----------- reflex/utils/types.py | 5 +++++ 3 files changed, 26 insertions(+), 20 deletions(-) diff --git a/reflex/ivars/base.py b/reflex/ivars/base.py index be1f13532a9..9bcfaf1b20e 100644 --- a/reflex/ivars/base.py +++ b/reflex/ivars/base.py @@ -43,7 +43,7 @@ VarValueError, ) from reflex.utils.format import format_state_name -from reflex.utils.types import GenericType, get_origin +from reflex.utils.types import GenericType, Self, get_origin from reflex.vars import ( REPLACED_NAMES, Var, @@ -1391,9 +1391,6 @@ class ImmutableComputedVar(ImmutableVar[RETURN_TYPE]): # Interval at which the computed var should be updated _update_interval: Optional[datetime.timedelta] = dataclasses.field(default=None) - # Whether the var is a (automatically created) dynamic route var - _var_is_dynamic_route: bool = dataclasses.field(default=False) - _fget: Callable[[BaseState], RETURN_TYPE] = dataclasses.field( default_factory=lambda: lambda _: None ) # type: ignore @@ -1407,7 +1404,6 @@ def __init__( auto_deps: bool = True, interval: Optional[Union[int, datetime.timedelta]] = None, backend: bool | None = None, - _var_is_dynamic_route: bool = False, **kwargs, ): """Initialize a ComputedVar. @@ -1420,7 +1416,6 @@ def __init__( auto_deps: Whether var dependencies should be auto-determined. interval: Interval at which the computed var should be updated. backend: Whether the computed var is a backend var. - _var_is_dynamic_route: Whether the var is a (automatically created) dynamic route var. **kwargs: additional attributes to set on the instance Raises: @@ -1450,7 +1445,6 @@ def __init__( interval = datetime.timedelta(seconds=interval) object.__setattr__(self, "_update_interval", interval) - object.__setattr__(self, "_var_is_dynamic_route", _var_is_dynamic_route) if deps is None: deps = [] @@ -1473,7 +1467,7 @@ def __init__( object.__setattr__(self, "_fget", fget) @override - def _replace(self, merge_var_data=None, **kwargs: Any) -> ImmutableComputedVar: + def _replace(self, merge_var_data=None, **kwargs: Any) -> Self: """Replace the attributes of the ComputedVar. Args: @@ -1505,7 +1499,7 @@ def _replace(self, merge_var_data=None, **kwargs: Any) -> ImmutableComputedVar: unexpected_kwargs = ", ".join(kwargs.keys()) raise TypeError(f"Unexpected keyword arguments: {unexpected_kwargs}") - return ImmutableComputedVar(**field_values) + return type(self)(**field_values) @property def _cache_attr(self) -> str: @@ -1779,6 +1773,12 @@ def fget(self) -> Callable[[BaseState], RETURN_TYPE]: return self._fget +class DynamicRouteVar(ImmutableComputedVar[str | List[str]]): + """A ComputedVar that represents a dynamic route.""" + + pass + + if TYPE_CHECKING: BASE_STATE = TypeVar("BASE_STATE", bound=BaseState) diff --git a/reflex/state.py b/reflex/state.py index 465d7d0ccfd..cce394e246f 100644 --- a/reflex/state.py +++ b/reflex/state.py @@ -35,6 +35,7 @@ from reflex.config import get_config from reflex.ivars.base import ( + DynamicRouteVar, ImmutableComputedVar, ImmutableVar, immutable_computed_var, @@ -1023,23 +1024,19 @@ def setup_dynamic_args(cls, args: dict[str, str]): if not args: return - cls._check_overwritten_dynamic_args(args) + cls._check_overwritten_dynamic_args(list(args.keys())) def argsingle_factory(param): def inner_func(self) -> str: return self.router.page.params.get(param, "") - return ImmutableComputedVar( - fget=inner_func, cache=True, _var_is_dynamic_route=True - ) + return DynamicRouteVar(fget=inner_func, cache=True) def arglist_factory(param): - def inner_func(self) -> List: + def inner_func(self) -> List[str]: return self.router.page.params.get(param, []) - return ImmutableComputedVar( - fget=inner_func, cache=True, _var_is_dynamic_route=True - ) + return DynamicRouteVar(fget=inner_func, cache=True) for param, value in args.items(): if value == constants.RouteArgType.SINGLE: @@ -1050,14 +1047,17 @@ def inner_func(self) -> List: continue # to allow passing as a prop, evade python frozen rules (bad practice) object.__setattr__(func, "_var_name", param) - cls.vars[param] = cls.computed_vars[param] = func._var_set_state(cls) # type: ignore + # cls.vars[param] = cls.computed_vars[param] = func._var_set_state(cls) # type: ignore + cls.vars[param] = cls.computed_vars[param] = func._replace( + _var_data=VarData.from_state(cls) + ) setattr(cls, param, func) # Reinitialize dependency tracking dicts. cls._init_var_dependency_dicts() @classmethod - def _check_overwritten_dynamic_args(cls, args: dict[str, str]): + def _check_overwritten_dynamic_args(cls, args: list[str]): """Check if dynamic args are shadowing existing vars. Recursively checks all child states. Args: @@ -1068,7 +1068,8 @@ def _check_overwritten_dynamic_args(cls, args: dict[str, str]): """ for arg in args: if ( - arg in cls.computed_vars and not cls.vars[arg]._var_is_dynamic_route + arg in cls.computed_vars + and not isinstance(cls.computed_vars[arg], DynamicRouteVar) ) or arg in cls.base_vars: raise NameError( f"Dynamic route arg '{arg}' is shadowing an existing var in {cls.__module__}.{cls.__name__}" diff --git a/reflex/utils/types.py b/reflex/utils/types.py index b4b4c109071..1610ba8b88e 100644 --- a/reflex/utils/types.py +++ b/reflex/utils/types.py @@ -111,6 +111,11 @@ def override(func: Callable) -> Callable: "_was_touched", } +if sys.version_info >= (3, 11): + from typing import Self as Self +else: + Self = None + class Unset: """A class to represent an unset value. From eeef695a5ea6d3a95846b585cfa7fde143ebb894 Mon Sep 17 00:00:00 2001 From: Benedikt Bartscher Date: Tue, 10 Sep 2024 23:55:18 +0200 Subject: [PATCH 09/11] old typing stuff again --- reflex/ivars/base.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/reflex/ivars/base.py b/reflex/ivars/base.py index 9bcfaf1b20e..d7f41f2179b 100644 --- a/reflex/ivars/base.py +++ b/reflex/ivars/base.py @@ -1773,7 +1773,7 @@ def fget(self) -> Callable[[BaseState], RETURN_TYPE]: return self._fget -class DynamicRouteVar(ImmutableComputedVar[str | List[str]]): +class DynamicRouteVar(ImmutableComputedVar[Union[str, List[str]]]): """A ComputedVar that represents a dynamic route.""" pass From 5a85de33962855e027e04168edb2b6dcaff1c154 Mon Sep 17 00:00:00 2001 From: Masen Furer Date: Tue, 10 Sep 2024 23:02:41 -0700 Subject: [PATCH 10/11] from typing_extensions import Self try to keep typing backward compatible with older releases we support --- reflex/utils/types.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/reflex/utils/types.py b/reflex/utils/types.py index 1610ba8b88e..f4463fa9200 100644 --- a/reflex/utils/types.py +++ b/reflex/utils/types.py @@ -114,7 +114,7 @@ def override(func: Callable) -> Callable: if sys.version_info >= (3, 11): from typing import Self as Self else: - Self = None + from typing_extensions import Self as Self class Unset: From 9c23d17ca827739f8899491cd530632e36b44e14 Mon Sep 17 00:00:00 2001 From: Masen Furer Date: Tue, 10 Sep 2024 23:08:26 -0700 Subject: [PATCH 11/11] Raise a specific exception when encountering dynamic route arg shadowing --- reflex/state.py | 10 +++++++--- reflex/utils/exceptions.py | 4 ++++ 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/reflex/state.py b/reflex/state.py index cce394e246f..b815fd44f8d 100644 --- a/reflex/state.py +++ b/reflex/state.py @@ -61,7 +61,11 @@ fix_events, ) from reflex.utils import console, format, path_ops, prerequisites, types -from reflex.utils.exceptions import ImmutableStateError, LockExpiredError +from reflex.utils.exceptions import ( + DynamicRouteArgShadowsStateVar, + ImmutableStateError, + LockExpiredError, +) from reflex.utils.exec import is_testing_env from reflex.utils.serializers import SerializedType, serialize, serializer from reflex.utils.types import override @@ -1064,14 +1068,14 @@ def _check_overwritten_dynamic_args(cls, args: list[str]): args: a dict of args Raises: - NameError: If a dynamic arg is shadowing an existing var. + DynamicRouteArgShadowsStateVar: If a dynamic arg is shadowing an existing var. """ for arg in args: if ( arg in cls.computed_vars and not isinstance(cls.computed_vars[arg], DynamicRouteVar) ) or arg in cls.base_vars: - raise NameError( + raise DynamicRouteArgShadowsStateVar( f"Dynamic route arg '{arg}' is shadowing an existing var in {cls.__module__}.{cls.__name__}" ) for substate in cls.get_substates(): diff --git a/reflex/utils/exceptions.py b/reflex/utils/exceptions.py index 8c1a1f07f57..dbab3fb6d93 100644 --- a/reflex/utils/exceptions.py +++ b/reflex/utils/exceptions.py @@ -87,3 +87,7 @@ class EventHandlerArgMismatch(ReflexError, TypeError): class EventFnArgMismatch(ReflexError, TypeError): """Raised when the number of args accepted by a lambda differs from that provided by the event trigger.""" + + +class DynamicRouteArgShadowsStateVar(ReflexError, NameError): + """Raised when a dynamic route arg shadows a state var."""