diff --git a/CHANGELOG.md b/CHANGELOG.md index 8a67dcd61..eb26b023e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,9 @@ All notable changes to this project are documented in this file. The format is b ### Fixed - Database: discard `on_commit` callbacks registered inside a rolled-back savepoint, including released child savepoints, while preserving callbacks from surviving work (#631). +- Role-permission edits, module settings updates/clears and system/tenant feature-flag + overrides now apply only after commit and reload across workers through the + invalidation bus. Periodic reloads bound staleness when messages are missed (#621). - Keycloak user-cache inserts and profile updates now commit in an independent transaction, preserving user IDs across logins and bearer requests. Concurrent first logins reuse the winning cache row; bearer requests throttle last-login writes to once per five minutes (#618). diff --git a/docs/framework/invalidation.md b/docs/framework/invalidation.md index cf28a3dd2..fa9903c90 100644 --- a/docs/framework/invalidation.md +++ b/docs/framework/invalidation.md @@ -12,11 +12,13 @@ Several modules cache a row that is cheap to read and expensive to read *often*: |---|---|---|---| | `User.session_version` | nearly every authenticated request | 30s | `users.session_version_cache` | | aggregate storage size | the files index | 30s | `file_storage` | -| hydrated settings objects | every request that reads a setting | **never** | `settings` | +| hydrated settings objects | every request that reads a setting | 30s periodic reload | `settings` | +| role permission overlays | every permission check | 30s periodic reload | `permissions` | +| feature flag overrides | every flag check | 30s periodic reload | `feature_flags` | The first two share one failure shape: the worker that performed the write dropped its own entry immediately and **every other worker kept serving the stale value until its own entry expired**. Acceptable for "sign out everywhere"; much less so for a password change made because an account is believed compromised ([#318](https://github.com/antosubash/simple_module_python/issues/318)). -The third is worse, and is worth stating separately because it is easy to assume it has a TTL too. `settings.reload.apply_changes_and_reload` swaps the settings object on `app.state.` in the worker that handled the save and announces it on the **in-process** `EventBus`. There is no TTL and no re-read, so every other worker serves the old configuration **until it is restarted** — an admin editing a setting on a four-worker deployment changes it for one worker in four. That needs a re-read rather than a `pop`, which is why it is not wired here (see *Still per-process*). +Role overlays, module settings and feature-flag overrides now reload from the committed DB on invalidation (#621). The writing worker publishes only from `db.on_commit`; rollback changes no live state. Reloads replace snapshots so deleted overrides disappear too. Settings reloads emit a local `SettingsReloaded` event when values change. A serialized periodic reload every 30 seconds provides the fallback when a message is lost or broadcasting is disabled; failed reads keep the last snapshot and retry on the next interval. The obvious fix — publish on Redis pub/sub — was not reachable from any of those modules. Redis belongs to the `background_tasks` plugin, so `users` would have to open a second connection with its own settings that can disagree with it; `SM009` makes a `framework/*` → plugin import an error, so the framework cannot borrow the plugin's client either. Hence the split below. @@ -107,4 +109,4 @@ A standalone worker never builds the FastAPI app, so it installs no transport: i ## Still per-process -Only `users.session_version` is wired so far. `file_storage`'s aggregate cache and `settings`' hot-swap have the same staleness shape and the same fix available; they are separate changes because each needs its own decision about what "forget" means for it — a settings hot-swap in particular has to *re-read*, not just drop, so it needs more than a `pop`. +`file_storage`'s aggregate cache remains per-process. Role overlays (`permissions.role_grants`), module settings (`settings.modules`, keyed by package) and feature flags (`feature_flags.overrides`, both scopes) subscribe through their modules' `register_invalidations` hooks. Their refresh tasks start and stop with module lifespan. diff --git a/framework/core/simple_module_core/feature_flags.py b/framework/core/simple_module_core/feature_flags.py index da8befab8..da76223d1 100644 --- a/framework/core/simple_module_core/feature_flags.py +++ b/framework/core/simple_module_core/feature_flags.py @@ -71,6 +71,13 @@ def clear_override(self, name: str, tenant_id: str | None = None) -> None: else: self._tenant_overrides.pop((name, tenant_id), None) + def replace_overrides( + self, system: dict[str, bool], tenants: dict[tuple[str, str], bool] + ) -> None: + """Replace a complete DB snapshot, removing overrides absent from it.""" + self._system_overrides = dict(system) + self._tenant_overrides = dict(tenants) + def tenant_override(self, name: str, tenant_id: str) -> bool | None: """Return the per-tenant override for ``(name, tenant_id)`` if set, else None.""" return self._tenant_overrides.get((name, tenant_id)) diff --git a/framework/core/simple_module_core/invalidation.py b/framework/core/simple_module_core/invalidation.py index 582f4110d..ac0fb10d6 100644 --- a/framework/core/simple_module_core/invalidation.py +++ b/framework/core/simple_module_core/invalidation.py @@ -9,11 +9,9 @@ immediately, and **every other worker keeps serving the stale value until its own entry expires**. -``settings`` has the same shape without the TTL — ``apply_changes_and_reload`` -hot-swaps the settings object in the worker that handled the save and announces it -on the in-process :class:`~simple_module_core.events.EventBus`, so the others serve -the old configuration until restart. It needs a re-read rather than an eviction, so -it is a consumer this bus makes *possible* rather than one it already serves. +``settings``, role overlays and feature-flag overrides had it too (#621); they now +re-read the committed DB state on invalidation (and every 30 s as a fallback, see +:mod:`simple_module_core.invalidation_refresh`) rather than evicting an entry. The obvious fix — publish the write on Redis pub/sub so every worker drops its entry at once — was not reachable from any of those modules. Redis belongs to diff --git a/framework/core/simple_module_core/invalidation_refresh.py b/framework/core/simple_module_core/invalidation_refresh.py new file mode 100644 index 000000000..3d757a949 --- /dev/null +++ b/framework/core/simple_module_core/invalidation_refresh.py @@ -0,0 +1,51 @@ +"""Reload DB-backed process state on invalidation and periodically after missed messages.""" + +from __future__ import annotations + +import asyncio +import logging +from collections.abc import Awaitable, Callable +from contextlib import suppress + +from simple_module_core.invalidation import Invalidation, InvalidationBus + +logger = logging.getLogger(__name__) +REFRESH_SECONDS = 30 + + +class InvalidationRefresh: + """Serialize reloads and own the bounded-staleness fallback's lifecycle.""" + + def __init__( + self, bus: InvalidationBus, channel: str, reload: Callable[[str | None], Awaitable[None]] + ) -> None: + self._reload = reload + self._lock = asyncio.Lock() + self._task: asyncio.Task | None = None + bus.subscribe(channel, self._receive) + + async def _receive(self, message: Invalidation) -> None: + await self.refresh(message.key) + + async def refresh(self, key: str | None = None) -> None: + async with self._lock: + await self._reload(key) + + def start(self) -> None: + if self._task is None: + self._task = asyncio.create_task(self._periodic()) + + async def stop(self) -> None: + if self._task is not None: + self._task.cancel() + with suppress(asyncio.CancelledError): + await self._task + self._task = None + + async def _periodic(self) -> None: + while True: + await asyncio.sleep(REFRESH_SECONDS) + try: + await self.refresh() + except Exception: + logger.exception("Periodic state reload failed; retrying on the next interval") diff --git a/framework/core/simple_module_core/permissions.py b/framework/core/simple_module_core/permissions.py index e48c91c9d..524a54501 100644 --- a/framework/core/simple_module_core/permissions.py +++ b/framework/core/simple_module_core/permissions.py @@ -208,6 +208,14 @@ def set_role_overlay(self, role: str, permissions: Collection[str]) -> None: self._role_overlay[role] = set(permissions) self._invalidate() + def replace_role_overlays(self, overlays: dict[str, list[str]]) -> None: + """Replace the complete persisted snapshot, including removed grants.""" + snapshot = {role: set(keys) for role, keys in overlays.items()} + if snapshot == self._role_overlay: # periodic reloads: keep derived caches warm + return + self._role_overlay = snapshot + self._invalidate() + @property def role_map(self) -> dict[str, list[str]]: """Merged role→permission mapping (``DEFAULT_ROLE_PERMISSIONS`` + module maps).""" diff --git a/framework/core/tests/test_core_invalidation_refresh.py b/framework/core/tests/test_core_invalidation_refresh.py new file mode 100644 index 000000000..aa6c44cf7 --- /dev/null +++ b/framework/core/tests/test_core_invalidation_refresh.py @@ -0,0 +1,54 @@ +"""Missed messages and transient reload failures must not leave state stale forever.""" + +import asyncio + +from simple_module_core.invalidation import InvalidationBus +from simple_module_core.invalidation_refresh import InvalidationRefresh + + +async def test_periodic_refresh_retries_without_a_message_and_stops(monkeypatch): + monkeypatch.setattr("simple_module_core.invalidation_refresh.REFRESH_SECONDS", 0.001) + attempts = [] + refreshed = asyncio.Event() + + async def reload(key): + attempts.append(key) + if len(attempts) == 1: + raise RuntimeError("transient DB failure") + refreshed.set() + + refresh = InvalidationRefresh(InvalidationBus(), "test.snapshot", reload) + refresh.start() + try: + await asyncio.wait_for(refreshed.wait(), timeout=1) + finally: + await refresh.stop() + assert attempts[:2] == [None, None] + count = len(attempts) + await asyncio.sleep(0.01) # intentional-sleep: proves no reload runs after stop() + assert len(attempts) == count + + +async def test_reload_messages_are_serialized(): + bus = InvalidationBus() + entered = asyncio.Event() + release = asyncio.Event() + keys = [] + + async def reload(key): + keys.append(key) + if key == "first": + entered.set() + await release.wait() + + InvalidationRefresh(bus, "test.snapshot", reload) + first = asyncio.create_task(bus.publish("test.snapshot", key="first")) + await asyncio.wait_for(entered.wait(), timeout=1) + second = asyncio.create_task(bus.publish("test.snapshot", key="second")) + try: + await asyncio.sleep(0) + assert keys == ["first"] + finally: + release.set() + await asyncio.wait_for(asyncio.gather(first, second), timeout=1) + assert keys == ["first", "second"] diff --git a/framework/core/tests/test_feature_flags.py b/framework/core/tests/test_feature_flags.py index 83eda3217..cf8c7b958 100644 --- a/framework/core/tests/test_feature_flags.py +++ b/framework/core/tests/test_feature_flags.py @@ -194,3 +194,16 @@ async def test_is_flag_enabled_reads_tenant_from_request_state(self): request.state.tenant_id = "other" assert is_flag_enabled(request, "beta_ui") is False + + +async def test_reloading_overrides_removes_deleted_values_in_both_scopes(): + registry = FeatureFlagRegistry() + registry.add(FeatureFlagDefinition("switch", default_enabled=True)) + registry.set_override("switch", False) + registry.set_override("switch", False, tenant_id="acme") + registry.replace_overrides({}, {("switch", "globex"): False}) + assert registry.is_enabled("switch") is True + assert registry.is_enabled("switch", "acme") is True + assert registry.is_enabled("switch", "globex") is False + registry.replace_overrides({}, {}) + assert registry.is_enabled("switch", "globex") is True diff --git a/framework/core/tests/test_permissions.py b/framework/core/tests/test_permissions.py index 6a65e2a11..5f7aed5b2 100644 --- a/framework/core/tests/test_permissions.py +++ b/framework/core/tests/test_permissions.py @@ -141,3 +141,13 @@ async def test_it_reads_any_collection(self): """Callers hand it whatever they have — the middleware caches a set, a test hands a list.""" assert grants(["users.manage"], "users.manage") is True + + +async def test_reloading_role_overlays_removes_deleted_grants_but_keeps_code_grants(): + registry = PermissionRegistry() + registry.map_role("editor", ["code.view"]) + registry.set_role_overlay("editor", ["db.view"]) + registry.replace_role_overlays({"reviewer": ["db.review"]}) + assert registry.role_map["editor"] == ["code.view"] + assert registry.role_map["reviewer"] == ["db.review"] + assert registry.role_map["admin"] == ["*"] diff --git a/framework/testing/simple_module_test/_schema.py b/framework/testing/simple_module_test/_schema.py index c4d7d6260..978bc691c 100644 --- a/framework/testing/simple_module_test/_schema.py +++ b/framework/testing/simple_module_test/_schema.py @@ -29,9 +29,17 @@ def _ensure_models_imported() -> list: return list(all_module_bases) -@lru_cache(maxsize=1) +_heads_cache: tuple[str, ...] | None = None + + def _alembic_heads() -> tuple[str, ...]: - """Cached head revisions — cannot change within a pytest run. + """Head revisions — cannot change within a pytest run, once found. + + Only a non-empty result is cached. ``resolve_head_revisions`` returns ``()`` + when ``alembic.ini`` is not found, which is what happens under a test that + points ``SM_PROJECT_ROOT`` at a tmp dir; caching that would leave every + later test in the worker unstamped while its app, resolving the real ini, + demands the heads ("Database is N revision(s) behind"). Plural: each module's first migration sets its own ``branch_labels``, so the history has one head per module and a real upgraded database carries @@ -39,9 +47,15 @@ def _alembic_heads() -> tuple[str, ...]: looking un-applied, which now reads as a behind-head schema and puts every test app behind the setup gate. """ + global _heads_cache from simple_module_hosting.migrations import resolve_head_revisions - return resolve_head_revisions() + if _heads_cache is None: + heads = resolve_head_revisions() + if not heads: + return heads + _heads_cache = heads + return _heads_cache async def _create_all_tables(engine) -> None: diff --git a/modules/branding/branding/service.py b/modules/branding/branding/service.py index 938f52666..26ac24344 100644 --- a/modules/branding/branding/service.py +++ b/modules/branding/branding/service.py @@ -8,7 +8,7 @@ from __future__ import annotations -from typing import TYPE_CHECKING, Any +from typing import TYPE_CHECKING, Any, cast from branding.constants import FAVICON_URL, LOGO_DARK_URL, LOGO_URL, PACKAGE, TENANT_IMAGE_FIELDS from branding.contracts.schemas import BrandingOut @@ -73,8 +73,11 @@ async def apply(self, changes: dict[str, Any]) -> BrandingOut: invalidation = getattr(self.app.state.sm, "invalidation", None) store = SettingsStore(SettingService(self.db, invalidation=invalidation)) bus = self.app.state.sm.event_bus - await apply_changes_and_reload(self.app, bus, store, package=PACKAGE, changes=changes) - return self.current() + validated = await apply_changes_and_reload( + self.app, bus, store, package=PACKAGE, changes=changes + ) + # Registered for PACKAGE, so the validated object is BrandingSettings. + return to_out(cast("BrandingSettings", validated)) async def _swap_asset(self, field: str, file_id: str) -> BrandingOut: """Point *field* at *file_id* ("" to clear) and reap what it replaced. @@ -86,9 +89,22 @@ async def _swap_asset(self, field: str, file_id: str) -> BrandingOut: file that is gone. A file another system image field still references (logo and dark logo sharing one upload) is kept. """ - previous = getattr(self.app.state.branding.settings, field, "") + # The pre-save value comes from the DB-backed settings, not app.state: + # the in-process object only changes after commit. + from settings.service import SettingService + from settings.store import SettingsStore + from simple_module_core.settings_hydrate import hydrate_settings + + from branding.settings import BrandingSettings + + before = await hydrate_settings( + BrandingSettings, SettingsStore(SettingService(self.db)), PACKAGE, lenient=True + ) + previous = getattr(before, field, "") out = await self.apply({field: file_id}) - live = self.app.state.branding.settings + live = await hydrate_settings( + BrandingSettings, SettingsStore(SettingService(self.db)), PACKAGE, lenient=True + ) still_used = {getattr(live, name, "") for name in TENANT_IMAGE_FIELDS} # No storage handed in (a hand-built service): nothing is reaped. replaced = previous and previous != file_id and previous not in still_used diff --git a/modules/branding/tests/test_presets.py b/modules/branding/tests/test_presets.py index c095ee7e2..3fb985b06 100644 --- a/modules/branding/tests/test_presets.py +++ b/modules/branding/tests/test_presets.py @@ -11,6 +11,9 @@ import httpx import pytest from branding.presets import BUILTIN_PRESETS, PRESET_FIELDS, BrandingPreset, find_preset +from settings.invalidation import reload_package +from settings.service import SettingService +from settings.store import SettingsStore # ── Unit: definitions ────────────────────────────────────────────────── @@ -104,7 +107,15 @@ async def test_applying_a_preset_preserves_identity_and_banner( async def test_applying_a_preset_keeps_an_uploaded_logo( app, authenticated_client: httpx.AsyncClient ) -> None: - app.state.branding.settings.logo_file_id = "11111111-1111-1111-1111-111111111111" + # Persist the logo: settings now reload from the DB after commit, so an + # in-memory-only assignment would (correctly) be discarded. + async with app.state.sm.db.session_factory() as db: + store = SettingsStore(SettingService(db)) + await store.set_override( + "branding", "logo_file_id", "11111111-1111-1111-1111-111111111111", "string" + ) + await db.commit() + await reload_package(app, app.state.sm.event_bus, store, "branding") await authenticated_client.post(f"/api/branding/presets/{BUILTIN_PRESETS[0].key}") diff --git a/modules/feature_flags/feature_flags/deps.py b/modules/feature_flags/feature_flags/deps.py index b2ee8d9fb..264f225fe 100644 --- a/modules/feature_flags/feature_flags/deps.py +++ b/modules/feature_flags/feature_flags/deps.py @@ -14,9 +14,10 @@ async def get_feature_flag_service( + request: Request, db: AsyncSession = Depends(get_db), ) -> FeatureFlagService: - return FeatureFlagService(db) + return FeatureFlagService(db, app=request.app) async def get_feature_flag_registry(request: Request) -> FeatureFlagRegistry: diff --git a/modules/feature_flags/feature_flags/endpoints/api.py b/modules/feature_flags/feature_flags/endpoints/api.py index 76e1d7ba9..d6a575c61 100644 --- a/modules/feature_flags/feature_flags/endpoints/api.py +++ b/modules/feature_flags/feature_flags/endpoints/api.py @@ -70,6 +70,8 @@ async def set_override( await service.set_override(name, body.enabled, registry=registry) view = service.build_view(registry, name) assert view is not None # _ensure_registered already proved it exists + view.enabled = body.enabled + view.overridden = True return view @@ -125,6 +127,8 @@ async def set_tenant_override( ) view = service.build_view(registry, name, tenant_id=tenant_id) assert view is not None # _ensure_registered already proved it exists + view.enabled = body.enabled + view.overridden = True return view diff --git a/modules/feature_flags/feature_flags/invalidation.py b/modules/feature_flags/feature_flags/invalidation.py new file mode 100644 index 000000000..da325a58a --- /dev/null +++ b/modules/feature_flags/feature_flags/invalidation.py @@ -0,0 +1,17 @@ +"""Re-read flag overrides after commit or a remote invalidation.""" + +from fastapi import FastAPI +from simple_module_core.invalidation import InvalidationBus +from simple_module_core.invalidation_refresh import InvalidationRefresh + +from feature_flags.service import FeatureFlagService + +CHANNEL = "feature_flags.overrides" + + +def subscribe(bus: InvalidationBus, app: FastAPI) -> InvalidationRefresh: + async def reload(_key: str | None) -> None: + async with app.state.sm.db.session_factory() as db: + await FeatureFlagService(db).hydrate_registry(app.state.sm.feature_flags) + + return InvalidationRefresh(bus, CHANNEL, reload) diff --git a/modules/feature_flags/feature_flags/module.py b/modules/feature_flags/feature_flags/module.py index 8ac84ee83..3fc05652a 100644 --- a/modules/feature_flags/feature_flags/module.py +++ b/modules/feature_flags/feature_flags/module.py @@ -30,6 +30,7 @@ if TYPE_CHECKING: from simple_module_core.events import EventBus + from simple_module_core.invalidation import InvalidationBus from sqlalchemy.ext.asyncio import AsyncSession _logger = logging.getLogger(__name__) @@ -75,6 +76,11 @@ def register_event_handlers(self, bus: EventBus, app: FastAPI | None = None) -> bus.subscribe_durable(TenantDeleted, purge_tenant_data) + def register_invalidations(self, bus: InvalidationBus, app: FastAPI) -> None: + from feature_flags.invalidation import subscribe + + self._refresh = subscribe(bus, app) + def register_routes(self, api_router: APIRouter, view_router: APIRouter) -> None: from feature_flags.endpoints.api import router as api from feature_flags.endpoints.views import router as views @@ -157,3 +163,10 @@ async def on_startup(self, app: FastAPI) -> None: await service.hydrate_registry(sm.feature_flags) except Exception: _logger.exception("feature_flags.hydrate_failed — continuing with registry defaults") + + if hasattr(self, "_refresh"): + self._refresh.start() + + async def on_shutdown(self, app: FastAPI) -> None: + if hasattr(self, "_refresh"): + await self._refresh.stop() diff --git a/modules/feature_flags/feature_flags/service.py b/modules/feature_flags/feature_flags/service.py index 842576b4a..e6e7b7b6a 100644 --- a/modules/feature_flags/feature_flags/service.py +++ b/modules/feature_flags/feature_flags/service.py @@ -9,6 +9,7 @@ from collections.abc import Callable +from fastapi import FastAPI from simple_module_core.feature_flags import FeatureFlagDefinition, FeatureFlagRegistry from sqlalchemy import select from sqlalchemy.ext.asyncio import AsyncSession @@ -60,8 +61,9 @@ class FeatureFlagService: extra DB hit per request. """ - def __init__(self, db: AsyncSession) -> None: + def __init__(self, db: AsyncSession, app: FastAPI | None = None) -> None: self.db = db + self.app = app async def list_overrides(self) -> list[FeatureFlagOverrideOut]: """Every persisted override across all scopes, ordered for stable display.""" @@ -179,7 +181,7 @@ async def set_override( existing.enabled = enabled await self.db.flush() if registry is not None: - registry.set_override(name, enabled, tenant_id=_registry_tenant_id(scope, scope_id)) + self._on_commit(registry, name, enabled, scope, scope_id) return FeatureFlagOverrideOut.model_validate(existing) async def clear_override( @@ -195,16 +197,34 @@ async def clear_override( return False await self.db.delete(existing) if registry is not None: - registry.clear_override(name, tenant_id=_registry_tenant_id(scope, scope_id)) + self._on_commit(registry, name, None, scope, scope_id) return True async def hydrate_registry(self, registry: FeatureFlagRegistry) -> int: """Load every persisted override (system + tenant) into the registry at boot.""" overrides = await self.list_overrides() - for ovr in overrides: - registry.set_override( - ovr.name, - ovr.enabled, - tenant_id=_registry_tenant_id(ovr.scope, ovr.scope_id), - ) + registry.replace_overrides( + {o.name: o.enabled for o in overrides if o.scope == SCOPE_SYSTEM}, + {(o.name, o.scope_id): o.enabled for o in overrides if o.scope == SCOPE_TENANT}, + ) return len(overrides) + + def _on_commit( + self, + registry: FeatureFlagRegistry, + name: str, + enabled: bool | None, + scope: str, + scope_id: str, + ) -> None: + async def committed() -> None: + if self.app is not None: + from feature_flags.invalidation import CHANNEL + + await self.app.state.sm.invalidation.publish(CHANNEL, key=name) + elif enabled is None: + registry.clear_override(name, tenant_id=_registry_tenant_id(scope, scope_id)) + else: + registry.set_override(name, enabled, tenant_id=_registry_tenant_id(scope, scope_id)) + + self.db.on_commit(committed) diff --git a/modules/feature_flags/tests/test_feature_flags_commit_invalidation.py b/modules/feature_flags/tests/test_feature_flags_commit_invalidation.py new file mode 100644 index 000000000..634b85064 --- /dev/null +++ b/modules/feature_flags/tests/test_feature_flags_commit_invalidation.py @@ -0,0 +1,78 @@ +"""Flag changes must survive commit before reaching any worker's registry.""" + +import pytest +from feature_flags.models import FeatureFlagOverride +from feature_flags.service import FeatureFlagService +from simple_module_core.feature_flags import FeatureFlagDefinition, FeatureFlagRegistry +from simple_module_core.invalidation import Invalidation +from simple_module_db.transaction import finalize_session, rollback_session + + +@pytest.mark.parametrize("commit", [False, True]) +@pytest.mark.parametrize("scope,scope_id", [("system", ""), ("tenant", "acme")]) +@pytest.mark.parametrize("clear", [False, True]) +async def test_flag_mutation_waits_for_commit(db_session, commit, scope, scope_id, clear): + registry = FeatureFlagRegistry() + registry.add(FeatureFlagDefinition("switch")) + tenant_id = scope_id or None + if clear: + db_session.add( + FeatureFlagOverride(scope=scope, scope_id=scope_id, name="switch", enabled=True) + ) + await db_session.commit() + registry.set_override("switch", True, tenant_id=tenant_id) + service = FeatureFlagService(db_session) + if clear: + await service.clear_override("switch", registry, scope, scope_id) + else: + await service.set_override("switch", True, registry, scope, scope_id) + assert registry.is_enabled("switch", tenant_id) is clear + if commit: + await finalize_session(db_session) + else: + await rollback_session(db_session) + assert registry.is_enabled("switch", tenant_id) is (clear != commit) + + +@pytest.mark.parametrize("scope,scope_id", [("system", ""), ("tenant", "acme")]) +async def test_remote_flag_changes_and_deletions_reload(app, scope, scope_id): + registry = app.state.sm.feature_flags + registry.add(FeatureFlagDefinition("switch")) + factory = app.state.sm.db.session_factory + async with factory() as db: + row = FeatureFlagOverride(scope=scope, scope_id=scope_id, name="switch", enabled=True) + db.add(row) + await db.commit() + row_id = row.id + bus = app.state.sm.invalidation + message = Invalidation(channel="feature_flags.overrides", origin="other") + await bus.deliver(message.to_wire()) + assert registry.is_enabled("switch", scope_id or None) is True + async with factory() as db: + await db.delete(await db.get(FeatureFlagOverride, row_id)) + await db.commit() + await bus.deliver(message.to_wire()) + assert registry.is_enabled("switch", scope_id or None) is False + + +@pytest.mark.parametrize("commit", [False, True]) +@pytest.mark.parametrize("scope,scope_id", [("system", ""), ("tenant", "acme")]) +async def test_flag_request_publishes_only_after_commit(app, commit, scope, scope_id): + from feature_flags.deps import get_feature_flag_service + from starlette.requests import Request + + registry = app.state.sm.feature_flags + registry.add(FeatureFlagDefinition("switch")) + messages = [] + app.state.sm.invalidation.subscribe("feature_flags.overrides", messages.append) + async with app.state.sm.db.session_factory() as db: + service = await get_feature_flag_service(Request({"type": "http", "app": app}), db) + await service.set_override("switch", True, registry, scope, scope_id) + assert registry.is_enabled("switch", scope_id or None) is False + assert messages == [] + if commit: + await finalize_session(db) + else: + await rollback_session(db) + assert registry.is_enabled("switch", scope_id or None) is commit + assert len(messages) == int(commit) diff --git a/modules/feature_flags/tests/test_feature_flags_service.py b/modules/feature_flags/tests/test_feature_flags_service.py index 5625fa648..009589b40 100644 --- a/modules/feature_flags/tests/test_feature_flags_service.py +++ b/modules/feature_flags/tests/test_feature_flags_service.py @@ -5,6 +5,7 @@ from feature_flags.constants import SCOPE_TENANT from feature_flags.service import FeatureFlagService from simple_module_core.feature_flags import FeatureFlagDefinition, FeatureFlagRegistry +from simple_module_db.transaction import finalize_session from sqlalchemy.ext.asyncio import AsyncSession @@ -25,6 +26,7 @@ async def test_set_override_inserts_row_and_mirrors_to_registry(self, db_session assert result.name == "beta_ui" assert result.scope == "system" assert result.enabled is True + await finalize_session(db_session) assert reg.is_enabled("beta_ui") is True async def test_set_override_twice_updates_in_place(self, db_session: AsyncSession): @@ -37,6 +39,7 @@ async def test_set_override_twice_updates_in_place(self, db_session: AsyncSessio rows = await svc.list_overrides() assert len(rows) == 1 assert rows[0].enabled is False + await finalize_session(db_session) assert reg.is_enabled("beta_ui") is False async def test_clear_override_removes_row_and_reverts_registry(self, db_session: AsyncSession): @@ -48,6 +51,7 @@ async def test_clear_override_removes_row_and_reverts_registry(self, db_session: assert cleared is True assert await svc.list_overrides() == [] + await finalize_session(db_session) assert reg.is_enabled("beta_ui") is False # back to default async def test_clear_override_unknown_flag_is_noop(self, db_session: AsyncSession): @@ -62,6 +66,7 @@ async def test_list_flags_joins_registry_with_overrides(self, db_session: AsyncS ) await svc.set_override("a.flag", True, registry=reg) + await finalize_session(db_session) views = await svc.list_flags(reg) by_name = {v.name: v for v in views} @@ -82,6 +87,7 @@ async def test_list_flags_drops_stale_overrides(self, db_session: AsyncSession): await svc.set_override("legacy", True, registry=reg_old) reg_new = _registry_with(("current", "", False)) + await finalize_session(db_session) views = await svc.list_flags(reg_new) assert [v.name for v in views] == ["current"] @@ -98,6 +104,7 @@ async def test_tenant_override_stored_separately_from_system(self, db_session: A rows = await svc.list_overrides() assert len(rows) == 2 # tenant row beats system for that tenant + await finalize_session(db_session) assert reg.is_enabled("beta_ui", tenant_id="acme") is False # other tenants fall back to system assert reg.is_enabled("beta_ui", tenant_id="other") is True @@ -115,6 +122,7 @@ async def test_clear_tenant_override_only_clears_that_tenant(self, db_session: A ) assert cleared is True + await finalize_session(db_session) assert reg.is_enabled("beta_ui", tenant_id="acme") is False assert reg.is_enabled("beta_ui", tenant_id="globex") is True @@ -124,6 +132,7 @@ async def test_list_flags_for_tenant_shows_resolved_and_system(self, db_session: await svc.set_override("beta_ui", True, registry=reg) # system on await svc.set_override("beta_ui", False, registry=reg, scope=SCOPE_TENANT, scope_id="acme") + await finalize_session(db_session) views = await svc.list_flags(reg, tenant_id="acme") view = next(v for v in views if v.name == "beta_ui") @@ -138,6 +147,7 @@ async def test_list_flags_for_tenant_without_override_inherits_system( reg = _registry_with(("beta_ui", "", False)) await svc.set_override("beta_ui", True, registry=reg) # system on + await finalize_session(db_session) views = await svc.list_flags(reg, tenant_id="acme") view = next(v for v in views if v.name == "beta_ui") diff --git a/modules/mail/tests/test_mail_lifecycle.py b/modules/mail/tests/test_mail_lifecycle.py index 0e4649af9..155e07c82 100644 --- a/modules/mail/tests/test_mail_lifecycle.py +++ b/modules/mail/tests/test_mail_lifecycle.py @@ -9,6 +9,7 @@ from settings.store import SettingsStore from simple_module_core import mail as core_mail from simple_module_core.mail import ConsoleMailSender, resolve_mail_sender +from simple_module_db.transaction import finalize_session async def test_startup_installs_the_configured_sender(app) -> None: @@ -17,15 +18,18 @@ async def test_startup_installs_the_configured_sender(app) -> None: assert sender.include_body is True # the explicit backend, not the fallback -async def test_a_settings_reload_reinstalls_the_sender(app, db_session) -> None: - store = SettingsStore(SettingService(db_session)) - await apply_changes_and_reload( - app, - app.state.sm.event_bus, - store, - package="mail", - changes={"backend": "smtp", "smtp_host": "smtp.test"}, - ) +async def test_a_settings_reload_reinstalls_the_sender(app) -> None: + # Use the app's own session: the reload re-reads the committed DB state. + async with app.state.sm.db.session_factory() as db: + store = SettingsStore(SettingService(db)) + await apply_changes_and_reload( + app, + app.state.sm.event_bus, + store, + package="mail", + changes={"backend": "smtp", "smtp_host": "smtp.test"}, + ) + await finalize_session(db) # the reload runs after commit assert isinstance(core_mail.current_mail_sender(), SmtpMailSender) diff --git a/modules/permissions/permissions/_registry_sync.py b/modules/permissions/permissions/_registry_sync.py index 079c8bfbb..61341d133 100644 --- a/modules/permissions/permissions/_registry_sync.py +++ b/modules/permissions/permissions/_registry_sync.py @@ -88,8 +88,7 @@ async def load_all_into_registry(self) -> None: by_role: dict[str, list[str]] = {} for name, key in result.all(): by_role.setdefault(name, []).append(key) - for name, keys in by_role.items(): - self.registry.set_role_overlay(name, keys) + self.registry.replace_role_overlays(by_role) async def sync_admin_all_permissions(self, assigned_by: str | None = None) -> None: """Ensure the admin role holds every registered permission key. diff --git a/modules/permissions/permissions/deps.py b/modules/permissions/permissions/deps.py index 81c25f883..c2e9eb976 100644 --- a/modules/permissions/permissions/deps.py +++ b/modules/permissions/permissions/deps.py @@ -30,7 +30,7 @@ async def get_permission_service( db: AsyncSession = Depends(get_db), registry: PermissionRegistry = Depends(get_permission_registry), ) -> PermissionService: - return PermissionService(db, registry, get_user_directory(request.app)) + return PermissionService(db, registry, get_user_directory(request.app), app=request.app) def assigned_by(request: Request) -> str | None: diff --git a/modules/permissions/permissions/module.py b/modules/permissions/permissions/module.py index dd78658ca..76bf2cdd2 100644 --- a/modules/permissions/permissions/module.py +++ b/modules/permissions/permissions/module.py @@ -94,6 +94,9 @@ def register_invalidations(self, bus: InvalidationBus, app: FastAPI) -> None: from permissions.grants import subscribe subscribe(bus) + from permissions.role_invalidation import subscribe as subscribe_roles + + self._refresh = subscribe_roles(bus, app) def locale_dirs(self) -> dict[str, Path]: base = Path(str(importlib.resources.files(__package__) / "locales")) @@ -108,3 +111,10 @@ async def on_startup(self, app: FastAPI) -> None: service = PermissionService(db, registry) await service.load_all_into_registry() await service.sync_admin_all_permissions() + + if hasattr(self, "_refresh"): + self._refresh.start() + + async def on_shutdown(self, app: FastAPI) -> None: + if hasattr(self, "_refresh"): + await self._refresh.stop() diff --git a/modules/permissions/permissions/role_invalidation.py b/modules/permissions/permissions/role_invalidation.py new file mode 100644 index 000000000..5bb81f203 --- /dev/null +++ b/modules/permissions/permissions/role_invalidation.py @@ -0,0 +1,17 @@ +"""Reload committed role grants in every worker, preserving code-registered mappings.""" + +from fastapi import FastAPI +from simple_module_core.invalidation import InvalidationBus +from simple_module_core.invalidation_refresh import InvalidationRefresh + +from permissions.service import PermissionService + +CHANNEL = "permissions.role_grants" + + +def subscribe(bus: InvalidationBus, app: FastAPI) -> InvalidationRefresh: + async def reload(_key: str | None) -> None: + async with app.state.sm.db.session_factory() as db: + await PermissionService(db, app.state.sm.permissions).load_all_into_registry() + + return InvalidationRefresh(bus, CHANNEL, reload) diff --git a/modules/permissions/permissions/service.py b/modules/permissions/permissions/service.py index 345f64af7..83c9a70b5 100644 --- a/modules/permissions/permissions/service.py +++ b/modules/permissions/permissions/service.py @@ -6,6 +6,7 @@ from collections.abc import Callable from typing import TYPE_CHECKING +from fastapi import FastAPI from simple_module_core.user_directory import DirectoryUser, NullUserDirectory, UserDirectory from sqlalchemy import delete, func, select from sqlalchemy.ext.asyncio import AsyncSession @@ -48,7 +49,9 @@ def __init__( db: AsyncSession, registry: PermissionRegistry, directory: UserDirectory | None = None, + app: FastAPI | None = None, ) -> None: + self.app = app self.db = db self.registry = registry self.directory: UserDirectory = directory or NullUserDirectory() @@ -125,9 +128,18 @@ async def set_role_permissions( for key in wanted - existing ) await self.db.flush() + # Replace only the DB overlay: code-registered mappings stay in effect, removed # DB keys drop without a restart, and unregistered stored grants stay mapped. - self.registry.set_role_overlay(role.name, wanted | (existing - registered)) + async def committed() -> None: + if self.app is None: + self.registry.set_role_overlay(role.name, wanted | (existing - registered)) + else: + from permissions.role_invalidation import CHANNEL + + await self.app.state.sm.invalidation.publish(CHANNEL, key=role.name) + + self.db.on_commit(committed) return RolePermissionsOut(role=role, permissions=sorted(wanted)) diff --git a/modules/permissions/tests/test_permissions_module.py b/modules/permissions/tests/test_permissions_module.py index 3debeb5f2..3b3b3ef12 100644 --- a/modules/permissions/tests/test_permissions_module.py +++ b/modules/permissions/tests/test_permissions_module.py @@ -11,6 +11,7 @@ from permissions.module import PermissionsModule from permissions.service import PermissionService from simple_module_core.permissions import PermissionRegistry +from simple_module_db.transaction import finalize_session from sqlalchemy.ext.asyncio import AsyncSession from users.directory import UsersDirectory @@ -126,6 +127,7 @@ async def test_set_updates_registry_role_map( role_id = await _seed_role(db_session) svc = PermissionService(db_session, registry, UsersDirectory()) await svc.set_role_permissions(role_id, ["products.view"]) + await finalize_session(db_session) assert "products.view" in registry.role_map.get("user", []) async def test_load_all_into_registry( @@ -200,6 +202,7 @@ async def test_resolve_effective_permissions_union( await svc.set_role_permissions(role_id, ["products.view"]) await svc.set_user_permissions(user_id, ["products.create"]) + await finalize_session(db_session) effective = await svc.resolve_effective_permissions(user_id) assert effective == {"products.view", "products.create"} diff --git a/modules/permissions/tests/test_permissions_role_invalidation.py b/modules/permissions/tests/test_permissions_role_invalidation.py new file mode 100644 index 000000000..ac318238e --- /dev/null +++ b/modules/permissions/tests/test_permissions_role_invalidation.py @@ -0,0 +1,78 @@ +"""Role overlays follow durable DB changes, including another worker's revocation.""" + +import uuid + +import pytest +from permissions.models import RolePermission +from permissions.service import PermissionService +from simple_module_core.invalidation import Invalidation +from simple_module_core.permissions import PermissionRegistry +from simple_module_db.transaction import finalize_session, rollback_session +from users.models import Role + + +@pytest.mark.parametrize("commit", [False, True]) +@pytest.mark.parametrize("clear", [False, True]) +async def test_role_overlay_waits_for_commit(db_session, commit, clear): + role = Role(id=uuid.uuid4(), name="editor") + db_session.add(role) + await db_session.flush() + registry = PermissionRegistry() + registry.add_group("Test", ["test.view"]) + role_id = role.id + if clear: + db_session.add(RolePermission(role_name="editor", permission_key="test.view")) + await db_session.commit() + registry.set_role_overlay("editor", ["test.view"]) + service = PermissionService(db_session, registry) + await service.set_role_permissions(role_id, [] if clear else ["test.view"]) + assert ("test.view" in registry.role_map.get("editor", [])) is clear + if commit: + await finalize_session(db_session) + else: + await rollback_session(db_session) + assert ("test.view" in registry.role_map.get("editor", [])) is (clear != commit) + + +async def test_remote_role_change_and_removal_reload_overlay(app): + registry = app.state.sm.permissions + registry.map_role("editor", ["code.grant"]) + factory = app.state.sm.db.session_factory + async with factory() as db: + row = RolePermission(role_name="editor", permission_key="db.grant") + db.add(row) + await db.commit() + bus = app.state.sm.invalidation + message = Invalidation(channel="permissions.role_grants", key="editor", origin="other") + await bus.deliver(message.to_wire()) + assert set(registry.role_map["editor"]) == {"code.grant", "db.grant"} + async with factory() as db: + await db.delete(await db.get(RolePermission, ("editor", "db.grant"))) + await db.commit() + await bus.deliver(message.to_wire()) + assert registry.role_map["editor"] == ["code.grant"] + + +@pytest.mark.parametrize("commit", [False, True]) +async def test_role_request_publishes_only_after_commit(app, commit): + from permissions.deps import get_permission_service + from starlette.requests import Request + + registry = app.state.sm.permissions + registry.add_group("Test", ["test.view"]) + messages = [] + app.state.sm.invalidation.subscribe("permissions.role_grants", messages.append) + async with app.state.sm.db.session_factory() as db: + role = Role(id=uuid.uuid4(), name="editor") + db.add(role) + await db.flush() + service = await get_permission_service(Request({"type": "http", "app": app}), db, registry) + await service.set_role_permissions(role.id, ["test.view"]) + assert "test.view" not in registry.role_map.get("editor", []) + assert messages == [] + if commit: + await finalize_session(db) + else: + await rollback_session(db) + assert ("test.view" in registry.role_map.get("editor", [])) is commit + assert len(messages) == int(commit) diff --git a/modules/settings/settings/endpoints/module_api.py b/modules/settings/settings/endpoints/module_api.py index 2cde6e7e8..4b74d38ca 100644 --- a/modules/settings/settings/endpoints/module_api.py +++ b/modules/settings/settings/endpoints/module_api.py @@ -12,7 +12,6 @@ from fastapi import APIRouter, Depends, FastAPI, HTTPException, Request, Response, status from pydantic import ValidationError from simple_module_core.module_settings import module_settings_registry -from simple_module_core.settings_hydrate import hydrate_settings from simple_module_hosting.i18n_deps import TranslatorDep from simple_module_hosting.permissions import RequiresPermission @@ -23,8 +22,8 @@ ) from settings._module_settings_props import serialize from settings.constants import PERM_DELETE, PERM_EDIT, PERM_VIEW -from settings.contracts.events import SettingsReloaded from settings.deps import get_setting_service +from settings.invalidation import reload_on_commit from settings.reload import EnvPinnedError, apply_changes_and_reload from settings.service import SettingService from settings.store import SettingsStore @@ -162,11 +161,6 @@ async def clear_module_field( store = SettingsStore(service) await store.clear_override(package, field) - hydrated = await hydrate_settings(cls, store, package) - services = getattr(request.app.state, package) - services.settings = hydrated - - bus = request.app.state.sm.event_bus - await bus.publish(SettingsReloaded(package=package, changed=(field,))) + reload_on_commit(request.app, request.app.state.sm.event_bus, store, package) return Response(status_code=status.HTTP_204_NO_CONTENT) diff --git a/modules/settings/settings/invalidation.py b/modules/settings/settings/invalidation.py new file mode 100644 index 000000000..08d281eb5 --- /dev/null +++ b/modules/settings/settings/invalidation.py @@ -0,0 +1,70 @@ +"""Rehydrate module settings and notify local consumers only after durable writes.""" + +import logging + +from fastapi import FastAPI +from simple_module_core.app_state import get_services +from simple_module_core.events import EventBus +from simple_module_core.invalidation import InvalidationBus +from simple_module_core.invalidation_refresh import InvalidationRefresh +from simple_module_core.module_settings import module_settings_registry +from simple_module_core.settings_hydrate import hydrate_settings +from simple_module_db.stored_settings import settings_table_exists + +from settings.contracts.events import SettingsReloaded +from settings.store import SettingsStore + +CHANNEL = "settings.modules" +logger = logging.getLogger(__name__) + + +async def reload_package(app: FastAPI, bus: EventBus, store: SettingsStore, package: str) -> None: + cls = module_settings_registry(app).get(package) + if cls is None: + return + services = getattr(app.state, package, None) + if services is None: + return + hydrated = await hydrate_settings(cls, store, package, lenient=True) + changed = tuple( + sorted( + name + for name in cls.model_fields + if getattr(services.settings, name) != getattr(hydrated, name) + ) + ) + services.settings = hydrated + if changed: + await bus.publish(SettingsReloaded(package=package, changed=changed)) + + +def reload_on_commit(app: FastAPI, bus: EventBus, store: SettingsStore, package: str) -> None: + async def committed() -> None: + invalidation = getattr(get_services(app), "invalidation", None) + if invalidation is None: + await reload_package(app, bus, store, package) + else: + await invalidation.publish(CHANNEL, key=package) + + store.on_commit(committed) + + +def subscribe(bus: InvalidationBus, app: FastAPI) -> InvalidationRefresh: + from settings.service import SettingService + + async def reload(key: str | None) -> None: + async with app.state.sm.db.session_factory() as db: + if not await settings_table_exists(db): # fresh install, before migrations + return + store = SettingsStore(SettingService(db)) + packages = [key] if key is not None else module_settings_registry(app).all_packages() + for package in packages: + # One package's failed read or invalid cross-field rule must not + # starve the packages after it on every later cycle. + try: + await reload_package(app, app.state.sm.event_bus, store, package) + except Exception: + logger.exception("Reloading %s settings failed; keeping current ones", package) + await db.rollback() + + return InvalidationRefresh(bus, CHANNEL, reload) diff --git a/modules/settings/settings/module.py b/modules/settings/settings/module.py index df158af16..cf149c22f 100644 --- a/modules/settings/settings/module.py +++ b/modules/settings/settings/module.py @@ -29,6 +29,7 @@ ) if TYPE_CHECKING: + from simple_module_core.invalidation import InvalidationBus from sqlalchemy.ext.asyncio import AsyncSession @@ -79,6 +80,19 @@ def register_settings(self, app: FastAPI) -> None: # Self-register so the UI lists our own settings alongside other modules. services.module_registry.register(MODULE_PACKAGE, SettingsSettings) + def register_invalidations(self, bus: InvalidationBus, app: FastAPI) -> None: + from settings.invalidation import subscribe + + self._refresh = subscribe(bus, app) + + async def on_startup(self, app: FastAPI) -> None: + if hasattr(self, "_refresh"): + self._refresh.start() + + async def on_shutdown(self, app: FastAPI) -> None: + if hasattr(self, "_refresh"): + await self._refresh.stop() + def register_exception_handlers(self, app: FastAPI) -> None: from settings.errors import install_exception_handlers diff --git a/modules/settings/settings/reload.py b/modules/settings/settings/reload.py index 7fda67e99..9d010c53d 100644 --- a/modules/settings/settings/reload.py +++ b/modules/settings/settings/reload.py @@ -11,9 +11,13 @@ from pydantic_settings import BaseSettings from simple_module_core.events import EventBus from simple_module_core.module_settings import module_settings_registry -from simple_module_core.settings_hydrate import env_supplied_fields, value_type_for_field +from simple_module_core.settings_hydrate import ( + env_supplied_fields, + hydrate_settings, + value_type_for_field, +) -from settings.contracts.events import SettingsReloaded +from settings.invalidation import reload_on_commit from settings.store import SettingsStore @@ -87,7 +91,7 @@ async def apply_changes_and_reload( package: str, changes: dict[str, Any], ) -> BaseSettings: - """Validate, persist, hot-swap, and publish ``SettingsReloaded``.""" + """Validate and persist; reload and publish ``SettingsReloaded`` after commit.""" registry = module_settings_registry(app) cls = registry.get(package) if cls is None: @@ -97,8 +101,8 @@ async def apply_changes_and_reload( if unknown: raise KeyError(f"Unknown field(s) for {package!r}: {sorted(unknown)}") - services = getattr(app.state, package) - current = services.settings + # Include prior writes in this transaction without exposing them to other requests. + current = await hydrate_settings(cls, store, package, lenient=True) # Compare against the *dumped* current value, not the attribute: a field # typed as a list of models (branding's ``footer_links``) holds model # instances, while ``changes`` carries the plain dicts a DTO dumps to. The @@ -118,6 +122,7 @@ async def apply_changes_and_reload( await store.clear_override(package, name) if not diff: + reload_on_commit(app, bus, store, package) return current # Only a field that would actually change is refused: a client that echoes @@ -153,6 +158,5 @@ async def apply_changes_and_reload( except ValidationError as exc: raise _relocate(exc, field_name) from exc - services.settings = validated - await bus.publish(SettingsReloaded(package=package, changed=tuple(sorted(diff)))) + reload_on_commit(app, bus, store, package) return validated diff --git a/modules/settings/settings/store.py b/modules/settings/settings/store.py index 4a7cbbba4..5e1d8d91c 100644 --- a/modules/settings/settings/store.py +++ b/modules/settings/settings/store.py @@ -18,6 +18,7 @@ from __future__ import annotations +from simple_module_db.callbacks import OnCommitCallback from simple_module_db.stored_settings import ( # noqa: F401 (re-exported, #500) SYSTEM_SCOPE_ID, get_overrides_sync, @@ -41,6 +42,9 @@ class SettingsStore: def __init__(self, service: SettingService) -> None: self._service = service + def on_commit(self, callback: OnCommitCallback) -> None: + self._service.db.on_commit(callback) + async def get_overrides(self, package: str) -> dict[str, tuple[str, str]]: """Return ``{field_name: (raw_value, value_type)}`` for a package.""" items = await self._service.list_by_scope_unmasked(SettingScope.SYSTEM, SYSTEM_SCOPE_ID) diff --git a/modules/settings/tests/test_reload.py b/modules/settings/tests/test_reload.py index 99a1a1f5f..a2fe1c376 100644 --- a/modules/settings/tests/test_reload.py +++ b/modules/settings/tests/test_reload.py @@ -14,6 +14,7 @@ from settings.store import SettingsStore from simple_module_core.events import EventBus from simple_module_core.module_settings import register_module_settings +from simple_module_db.transaction import finalize_session class _UsersCfg(BaseSettings): @@ -56,7 +57,10 @@ async def handler(evt: SettingsReloaded) -> None: assert new_settings.allow_signup is True assert new_settings.smtp_port == 587 - assert app.state.users.settings is new_settings + assert app.state.users.settings.allow_signup is False + assert received == [] + await finalize_session(db_session) + assert app.state.users.settings == new_settings assert received == [SettingsReloaded(package="users", changed=("allow_signup", "smtp_port"))] persisted = await store.get_overrides("users") @@ -96,3 +100,15 @@ async def test_apply_changes_unknown_package_raises(app_and_bus, db_session): package="unknown", changes={"x": 1}, ) + + +async def test_multiple_settings_edits_in_one_transaction_keep_both(app_and_bus, db_session): + app, bus = app_and_bus + store = SettingsStore(SettingService(db_session)) + await apply_changes_and_reload(app, bus, store, package="users", changes={"allow_signup": True}) + await apply_changes_and_reload(app, bus, store, package="users", changes={"smtp_port": 587}) + assert app.state.users.settings.allow_signup is False + assert app.state.users.settings.smtp_port == 25 + await finalize_session(db_session) + assert app.state.users.settings.allow_signup is True + assert app.state.users.settings.smtp_port == 587 diff --git a/modules/settings/tests/test_settings_commit_invalidation.py b/modules/settings/tests/test_settings_commit_invalidation.py new file mode 100644 index 000000000..cd70c4f4e --- /dev/null +++ b/modules/settings/tests/test_settings_commit_invalidation.py @@ -0,0 +1,99 @@ +"""Module settings and reload events only reflect committed state.""" + +import pytest +from settings.contracts.events import SettingsReloaded +from settings.reload import apply_changes_and_reload +from settings.service import SettingService +from settings.store import SettingsStore +from simple_module_core.invalidation import Invalidation +from simple_module_db.transaction import finalize_session, rollback_session + + +@pytest.mark.parametrize("commit", [False, True]) +async def test_settings_and_events_wait_for_commit(app, commit): + received = [] + bus = app.state.sm.event_bus + + async def record(event): + received.append(event) + + bus.subscribe(SettingsReloaded, record) + original = app.state.users.settings + wanted = not original.allow_signup + async with app.state.sm.db.session_factory() as db: + await apply_changes_and_reload( + app, + bus, + SettingsStore(SettingService(db)), + package="users", + changes={"allow_signup": wanted}, + ) + assert app.state.users.settings is original + assert received == [] + if commit: + await finalize_session(db) + else: + await rollback_session(db) + assert app.state.users.settings.allow_signup is (wanted if commit else original.allow_signup) + assert len(received) == int(commit) + if commit: + assert received[0].changed == ("allow_signup",) + + +async def test_remote_settings_changes_and_clears_reload_and_emit_event(app): + received = [] + + async def record(event): + received.append(event) + + app.state.sm.event_bus.subscribe(SettingsReloaded, record) + original = app.state.users.settings.allow_signup + factory = app.state.sm.db.session_factory + async with factory() as db: + await SettingsStore(SettingService(db)).set_override( + "users", "allow_signup", str(not original), "bool" + ) + await db.commit() + message = Invalidation(channel="settings.modules", key="users", origin="other") + await app.state.sm.invalidation.deliver(message.to_wire()) + assert app.state.users.settings.allow_signup is not original + assert received[-1].changed == ("allow_signup",) + async with factory() as db: + await SettingsStore(SettingService(db)).clear_override("users", "allow_signup") + await db.commit() + await app.state.sm.invalidation.deliver(message.to_wire()) + assert app.state.users.settings.allow_signup is original + assert len(received) == 2 + + +@pytest.mark.parametrize("commit", [False, True]) +async def test_clearing_module_setting_waits_for_commit(app, commit): + from settings.endpoints.module_api import clear_module_field + from starlette.requests import Request + + original = app.state.users.settings.allow_signup + factory = app.state.sm.db.session_factory + async with factory() as db: + await SettingsStore(SettingService(db)).set_override( + "users", "allow_signup", str(not original), "bool" + ) + await db.commit() + await app.state.sm.invalidation.publish("settings.modules", key="users") + received = [] + + async def record(event): + received.append(event) + + app.state.sm.event_bus.subscribe(SettingsReloaded, record) + async with factory() as db: + await clear_module_field( + "users", "allow_signup", Request({"type": "http", "app": app}), SettingService(db) + ) + assert app.state.users.settings.allow_signup is not original + assert received == [] + if commit: + await finalize_session(db) + else: + await rollback_session(db) + assert app.state.users.settings.allow_signup is (original if commit else not original) + assert len(received) == int(commit) diff --git a/modules/settings/tests/test_settings_secret_json_roundtrip.py b/modules/settings/tests/test_settings_secret_json_roundtrip.py index 0b8e3e8f7..46b30cec7 100644 --- a/modules/settings/tests/test_settings_secret_json_roundtrip.py +++ b/modules/settings/tests/test_settings_secret_json_roundtrip.py @@ -25,6 +25,13 @@ class _SecretCfg(BaseSettings): class _Store: def __init__(self) -> None: self.rows: dict[str, tuple[str, str]] = {} + self.callbacks = [] + + def on_commit(self, callback) -> None: + self.callbacks.append(callback) + + async def get_overrides(self, package): + return dict(self.rows) async def set_override(self, package, field, raw, value_type) -> None: self.rows[field] = (raw, value_type)