diff --git a/packages/reflex-base/news/+config-reload-state-imports.bugfix.md b/packages/reflex-base/news/+config-reload-state-imports.bugfix.md new file mode 100644 index 00000000000..1e768777645 --- /dev/null +++ b/packages/reflex-base/news/+config-reload-state-imports.bugfix.md @@ -0,0 +1 @@ +Prevent `reload_config()` from raising a duplicate-state error for state modules imported by `rxconfig.py`. diff --git a/packages/reflex-base/src/reflex_base/config.py b/packages/reflex-base/src/reflex_base/config.py index 2665b6eeabf..c2c5f489571 100644 --- a/packages/reflex-base/src/reflex_base/config.py +++ b/packages/reflex-base/src/reflex_base/config.py @@ -7,7 +7,7 @@ import sys import threading import urllib.parse -from collections.abc import Iterator, Sequence +from collections.abc import Iterable, Iterator, Sequence from contextlib import contextmanager from importlib.util import find_spec from pathlib import Path @@ -803,12 +803,26 @@ def _set_persistent(self, **kwargs): self._replace_defaults(**kwargs) -# Project-local modules first imported while loading rxconfig.py; evicted -# before the next load so projects don't reuse each other's dependencies. -# Only mutated under _load_config_lock. +# Project-local modules first imported while loading rxconfig.py. Only mutated +# under _load_config_lock. _config_module_deps: set[str] = set() +def _get_registered_state_modules(ctx: RegistrationContext) -> set[str]: + """Return modules defining states already registered in a context. + + Args: + ctx: The active registration context. + + Returns: + Module names whose objects must survive a config reload. + """ + state_types = set(ctx.base_states.values()) + for substates in ctx.base_state_substates.values(): + state_types.update(substates) + return {state_type.__module__ for state_type in state_types} + + class _ImportRecorder: """Meta-path finder that records import attempts made on one thread. @@ -846,6 +860,23 @@ def find_spec(self, fullname: str, path: Any = None, target: Any = None) -> None _import_recorder = _ImportRecorder() +def _record_project_modules(names: Iterable[str], project_root: Path) -> None: + """Add project-local modules from an import attempt to the dependency set. + + Args: + names: Module names observed by the import recorder. + project_root: Root used to classify project-local modules. + """ + for name in names: + origin = getattr(sys.modules.get(name), "__file__", None) + if ( + origin + and (path := Path(origin)).is_relative_to(project_root) + and "site-packages" not in path.parts + ): + _config_module_deps.add(name) + + @contextmanager def _record_imports() -> Iterator[_ImportRecorder]: """Record imports made on the current thread while rxconfig loads. @@ -894,7 +925,9 @@ def get_state_auto_setters() -> bool: return False -def _get_config(project_root: Path | None = None) -> Config: +def _get_config( + project_root: Path | None = None, *, reload_dependencies: bool = True +) -> Config: """Import rxconfig.py fresh from the project root and return its config. The project root is prepended to sys.path for the duration of the import so @@ -907,10 +940,14 @@ def _get_config(project_root: Path | None = None) -> Config: current working directory, resolved once up front so an rxconfig.py that changes the cwd cannot move the root that the sys.path entry and the dependency classification below are based on. + reload_dependencies: Whether to reload project-local modules imported by + rxconfig.py. A config reload in an existing RegistrationContext + keeps only modules that define already-registered state classes. Returns: The app config. """ + ctx = RegistrationContext.ensure_context() project_root = (project_root or Path.cwd()).resolve() with _load_config_lock: # A fresh str object, so the exact inserted entry can be removed by @@ -919,16 +956,38 @@ def _get_config(project_root: Path | None = None) -> Config: cwd = str(project_root) sys.path.insert(0, cwd) try: - # Never cache rxconfig or its project-local dependencies — each load - # goes to disk so different RegistrationContexts hold independent - # Config instances resolved against the current project. Evict - # before probing: find_spec answers from sys.modules, so modules - # left behind by another project directory would fake the existence - # check below. + # Restore context-owned modules on reload so forked state classes are + # not redefined while rxconfig.py is imported again. sys.modules.pop(constants.Config.MODULE, None) - for dep in _config_module_deps: - sys.modules.pop(dep, None) - _config_module_deps.clear() + if reload_dependencies or ctx._config_module_deps_root != project_root: + for dep in _config_module_deps: + sys.modules.pop(dep, None) + _config_module_deps.clear() + else: + for dep in _config_module_deps: + sys.modules.pop(dep, None) + _config_module_deps.clear() + state_modules = _get_registered_state_modules(ctx) + package_modules = { + dep + for dep in ctx._config_module_deps + if any( + state_module.startswith(f"{dep}.") + for state_module in state_modules + ) + } + for dep, module in ctx._config_module_deps.items(): + if dep in state_modules or dep in package_modules: + sys.modules[dep] = module + _config_module_deps.add(dep) + with _record_imports() as recorder: + try: + for dep in sorted( + package_modules, key=lambda name: name.count(".") + ): + importlib.reload(ctx._config_module_deps[dep]) + finally: + _record_project_modules(recorder.names, project_root) # only import the module if it exists. If a module spec exists then # the module exists. if not find_spec(constants.Config.MODULE): @@ -940,14 +999,15 @@ def _get_config(project_root: Path | None = None) -> Config: rxconfig = importlib.import_module(constants.Config.MODULE) finally: # Record even on failure so a retry evicts partially-imported deps. - for name in recorder.names: - origin = getattr(sys.modules.get(name), "__file__", None) - if ( - origin - and (path := Path(origin)).is_relative_to(project_root) - and "site-packages" not in path.parts - ): - _config_module_deps.add(name) + _record_project_modules(recorder.names, project_root) + ctx._config_module_deps.clear() + ctx._config_module_deps.update({ + name: module + for name in _config_module_deps + if name != constants.Config.MODULE + and (module := sys.modules.get(name)) is not None + }) + object.__setattr__(ctx, "_config_module_deps_root", project_root) return rxconfig.config finally: for i, entry in enumerate(sys.path): @@ -1021,6 +1081,7 @@ def reload_config() -> Config: The freshly loaded app config. """ ctx = RegistrationContext.ensure_context() - config = _get_config() + preserve_dependencies = bool(ctx._config_module_deps or ctx.base_states) + config = _get_config(reload_dependencies=not preserve_dependencies) ctx._set_config(config) return config diff --git a/packages/reflex-base/src/reflex_base/registry.py b/packages/reflex-base/src/reflex_base/registry.py index 61963cb538f..74a316051ff 100644 --- a/packages/reflex-base/src/reflex_base/registry.py +++ b/packages/reflex-base/src/reflex_base/registry.py @@ -3,6 +3,8 @@ from __future__ import annotations import dataclasses +from pathlib import Path +from types import ModuleType from typing import TYPE_CHECKING, Any from typing_extensions import Self @@ -60,6 +62,10 @@ class RegistrationContext(BaseContext): repr=False, ) _config: Config | None = dataclasses.field(default=None, repr=False) + _config_module_deps: dict[str, ModuleType] = dataclasses.field( + default_factory=dict, repr=False + ) + _config_module_deps_root: Path | None = dataclasses.field(default=None, repr=False) decorated_pages: list[tuple[Callable, dict[str, Any]]] = dataclasses.field( default_factory=list, repr=False, @@ -138,6 +144,8 @@ def fork(self) -> Self: base_state_substates={ k: set(v) for k, v in self.base_state_substates.items() }, + _config_module_deps=dict(self._config_module_deps), + _config_module_deps_root=self._config_module_deps_root, decorated_pages=list(self.decorated_pages), bundled_libraries=list(self.bundled_libraries), ) diff --git a/tests/units/test_config.py b/tests/units/test_config.py index 8333325d3b1..d07c42c1611 100644 --- a/tests/units/test_config.py +++ b/tests/units/test_config.py @@ -29,6 +29,14 @@ interpret_int_env, ) +CONFIG_MODULE = "rxconfig" +STATE_MODULE = "config_reload_state_module" +DEPENDENCY_MODULE = "config_reload_dependency" +PACKAGE_NAME = "config_reload_package" +PACKAGE_STATE_MODULE = f"{PACKAGE_NAME}.state" +PACKAGE_SETTINGS_MODULE = f"{PACKAGE_NAME}.settings" +CONFIG_RELOAD_FAIL_ENV = "CONFIG_RELOAD_FAIL" + def test_requires_app_name(): """Test that a config requires an app_name.""" @@ -1036,6 +1044,264 @@ def test_get_config_accepts_explicit_project_root( assert reflex_base.config._get_config(project).app_name == "explicit" +def _write_state_config(project: Path, app_name: str = "state_reload") -> None: + """Write a config that imports a module defining a state class. + + Args: + project: The project directory to populate. + app_name: The app name written to the config. + """ + project.mkdir(exist_ok=True) + (project / f"{STATE_MODULE}.py").write_text( + "import reflex as rx\n\nclass MyState(rx.State):\n value: str = ''\n" + ) + (project / "rxconfig.py").write_text( + f"import {STATE_MODULE}\nimport reflex as rx\n\n" + f"config = rx.Config(app_name={app_name!r})\n" + ) + + +def test_reload_config_does_not_redefine_project_state( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, clean_config_modules: None +): + """Reloading config does not re-import project modules that define state. + + Args: + tmp_path: The pytest tmp_path fixture. + monkeypatch: The pytest monkeypatch fixture. + clean_config_modules: Cleanup for modules left behind by the load. + """ + from reflex_base.registry import RegistrationContext + + _write_state_config(tmp_path) + monkeypatch.chdir(tmp_path) + + with RegistrationContext() as context: + assert reflex_base.config.get_config().app_name == "state_reload" + assert reflex_base.config.reload_config().app_name == "state_reload" + + with context.fork(): + assert reflex_base.config.reload_config().app_name == "state_reload" + + +def test_get_config_reloads_project_state_for_each_context( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, clean_config_modules: None +): + """Initial config loads register project state in each context. + + Args: + tmp_path: The pytest tmp_path fixture. + monkeypatch: The pytest monkeypatch fixture. + clean_config_modules: Cleanup for modules left behind by the load. + """ + from reflex_base.registry import RegistrationContext + + _write_state_config(tmp_path) + monkeypatch.chdir(tmp_path) + + with RegistrationContext() as first_context: + reflex_base.config.get_config() + first_state = next( + state + for state in first_context.base_states.values() + if state.__module__ == STATE_MODULE + ) + + with RegistrationContext() as second_context: + reflex_base.config.get_config() + second_state = next( + state + for state in second_context.base_states.values() + if state.__module__ == STATE_MODULE + ) + + assert second_state is not first_state + + +def test_reload_config_restores_modules_for_an_older_context( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, clean_config_modules: None +): + """Reloading an older context restores its project-local modules. + + Args: + tmp_path: The pytest tmp_path fixture. + monkeypatch: The pytest monkeypatch fixture. + clean_config_modules: Cleanup for modules left behind by the load. + """ + from reflex_base.registry import RegistrationContext + + first_project = tmp_path / "first" + second_project = tmp_path / "second" + _write_state_config(first_project, "first") + _write_state_config(second_project, "second") + + first_context = RegistrationContext() + with first_context: + monkeypatch.chdir(first_project) + assert reflex_base.config.get_config().app_name == "first" + first_module = sys.modules[STATE_MODULE] + + with RegistrationContext(): + monkeypatch.chdir(second_project) + assert reflex_base.config.get_config().app_name == "second" + assert sys.modules[STATE_MODULE] is not first_module + + with first_context: + monkeypatch.chdir(first_project) + assert reflex_base.config.reload_config().app_name == "first" + assert sys.modules[STATE_MODULE] is first_module + + +def test_reload_config_restores_state_package( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, clean_config_modules: None +): + """Reloading config preserves a package containing a registered state. + + Args: + tmp_path: The pytest tmp_path fixture. + monkeypatch: The pytest monkeypatch fixture. + clean_config_modules: Cleanup for modules left behind by the load. + """ + from reflex_base.registry import RegistrationContext + + package = tmp_path / PACKAGE_NAME + package.mkdir() + (package / "__init__.py").write_text( + f"from .{PACKAGE_SETTINGS_MODULE.rsplit('.', maxsplit=1)[-1]} import APP_NAME\n" + ) + (package / "settings.py").write_text("APP_NAME = 'first'\n") + (package / "state.py").write_text( + "import reflex as rx\n\nclass MyState(rx.State):\n value: str = ''\n" + ) + (tmp_path / f"{CONFIG_MODULE}.py").write_text( + f"import {PACKAGE_STATE_MODULE}\nimport reflex as rx\n\n" + f"import {PACKAGE_NAME}\n\n" + f"config = rx.Config(app_name={PACKAGE_NAME}.APP_NAME)\n" + ) + monkeypatch.chdir(tmp_path) + + with RegistrationContext(): + assert reflex_base.config.get_config().app_name == "first" + (package / "settings.py").write_text("APP_NAME = 'second'\n") + assert reflex_base.config.reload_config().app_name == "second" + (package / "settings.py").write_text("APP_NAME = 'third'\n") + assert reflex_base.config.reload_config().app_name == "third" + assert PACKAGE_STATE_MODULE in sys.modules + assert PACKAGE_NAME in sys.modules + + +def test_reload_config_preserves_state_defined_in_package( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, clean_config_modules: None +): + """Reloading config does not redefine a state defined in a package. + + Args: + tmp_path: The pytest tmp_path fixture. + monkeypatch: The pytest monkeypatch fixture. + clean_config_modules: Cleanup for modules left behind by the load. + """ + from reflex_base.registry import RegistrationContext + + package = tmp_path / PACKAGE_NAME + package.mkdir() + (package / "__init__.py").write_text( + "import reflex as rx\n\nclass PackageState(rx.State):\n value: str = ''\n" + ) + (tmp_path / f"{CONFIG_MODULE}.py").write_text( + f"import {PACKAGE_NAME}\nimport reflex as rx\n\n" + "config = rx.Config(app_name='package_state')\n" + ) + monkeypatch.chdir(tmp_path) + + with RegistrationContext(): + assert reflex_base.config.get_config().app_name == "package_state" + assert reflex_base.config.reload_config().app_name == "package_state" + + +def test_reload_config_records_package_imports_after_failure( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, clean_config_modules: None +): + """A failed package reload still tracks imports for the next retry. + + Args: + tmp_path: The pytest tmp_path fixture. + monkeypatch: The pytest monkeypatch fixture. + clean_config_modules: Cleanup for modules left behind by the load. + """ + from reflex_base.registry import RegistrationContext + + package = tmp_path / PACKAGE_NAME + package.mkdir() + (package / "settings.py").write_text("APP_NAME = 'first'\n") + (package / "state.py").write_text( + "import reflex as rx\n\nclass PackageState(rx.State):\n value: str = ''\n" + ) + (package / "__init__.py").write_text( + "import os\n" + "from .settings import APP_NAME\n\n" + f"if os.environ.get({CONFIG_RELOAD_FAIL_ENV!r}):\n" + " raise RuntimeError('reload failed')\n" + ) + (tmp_path / f"{CONFIG_MODULE}.py").write_text( + f"import {PACKAGE_NAME}.state\nimport {PACKAGE_NAME}\n" + "import reflex as rx\n\n" + f"config = rx.Config(app_name={PACKAGE_NAME}.APP_NAME)\n" + ) + monkeypatch.chdir(tmp_path) + + with RegistrationContext(): + assert reflex_base.config.get_config().app_name == "first" + monkeypatch.setenv(CONFIG_RELOAD_FAIL_ENV, "1") + with pytest.raises(RuntimeError, match="reload failed"): + reflex_base.config.reload_config() + monkeypatch.delenv(CONFIG_RELOAD_FAIL_ENV) + (package / "settings.py").write_text("APP_NAME = 'second'\n") + assert reflex_base.config.reload_config().app_name == "second" + + +def _write_dependency_config(project: Path, app_name: str) -> None: + """Write a config that imports a project-local non-state dependency. + + Args: + project: The project directory to populate. + app_name: The app name exposed by the dependency. + """ + project.mkdir() + (project / f"{DEPENDENCY_MODULE}.py").write_text(f"APP_NAME = {app_name!r}\n") + (project / "rxconfig.py").write_text( + f"import {DEPENDENCY_MODULE}\nimport reflex as rx\n\n" + f"config = rx.Config(app_name={DEPENDENCY_MODULE}.APP_NAME)\n" + ) + + +def test_reload_config_evicts_modules_when_project_changes( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, clean_config_modules: None +): + """Reloading a context after changing projects imports the new modules. + + Args: + tmp_path: The pytest tmp_path fixture. + monkeypatch: The pytest monkeypatch fixture. + clean_config_modules: Cleanup for modules left behind by the load. + """ + from reflex_base.registry import RegistrationContext + + first_project = tmp_path / "first" + second_project = tmp_path / "second" + _write_dependency_config(first_project, "first") + _write_dependency_config(second_project, "second") + + with RegistrationContext(): + monkeypatch.chdir(first_project) + assert reflex_base.config.get_config().app_name == "first" + (first_project / f"{DEPENDENCY_MODULE}.py").write_text("APP_NAME = 'updated'\n") + assert reflex_base.config.reload_config().app_name == "updated" + monkeypatch.chdir(second_project) + assert reflex_base.config.reload_config().app_name == "second" + dependency = sys.modules[DEPENDENCY_MODULE] + assert Path(dependency.__file__ or "").is_relative_to(second_project) + + @pytest.fixture def clean_config_modules() -> Generator[None, None, None]: """Drop the modules and dep records a real rxconfig load leaves behind. @@ -1043,7 +1309,16 @@ def clean_config_modules() -> Generator[None, None, None]: Yields: None, once the module table is clean. """ - names = ("rxconfig", "side_module", "chdir_dep_module") + names = ( + CONFIG_MODULE, + "side_module", + "chdir_dep_module", + STATE_MODULE, + DEPENDENCY_MODULE, + PACKAGE_NAME, + PACKAGE_STATE_MODULE, + PACKAGE_SETTINGS_MODULE, + ) try: yield finally: