From 254326098923e0afc5ccb2c7e00e16ce07968921 Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Wed, 25 Jun 2025 22:18:04 +0200 Subject: [PATCH 1/8] =?UTF-8?q?=E2=9C=A8=20[Refactor]=20Enhance=20applicat?= =?UTF-8?q?ion=20setup=20with=20context=20manager=20for=20timing=20and=20l?= =?UTF-8?q?ogging?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../servicelib/aiohttp/application_setup.py | 212 ++++++++++++------ 1 file changed, 138 insertions(+), 74 deletions(-) diff --git a/packages/service-library/src/servicelib/aiohttp/application_setup.py b/packages/service-library/src/servicelib/aiohttp/application_setup.py index 0d52603f9651..4bd3981c8ee2 100644 --- a/packages/service-library/src/servicelib/aiohttp/application_setup.py +++ b/packages/service-library/src/servicelib/aiohttp/application_setup.py @@ -2,9 +2,12 @@ import inspect import logging from collections.abc import Callable +from contextlib import ContextDecorator from copy import deepcopy +from datetime import datetime from enum import Enum -from typing import Any, Protocol +from types import TracebackType +from typing import Any, Final, Protocol import arrow from aiohttp import web @@ -15,21 +18,19 @@ from .application_keys import APP_CONFIG_KEY, APP_SETTINGS_KEY -log = logging.getLogger(__name__) +_logger = logging.getLogger(__name__) -APP_SETUP_COMPLETED_KEY = f"{__name__ }.setup" +APP_SETUP_COMPLETED_KEY: Final[str] = f"{__name__ }.setup" class _SetupFunc(Protocol): __name__: str - def __call__(self, app: web.Application, *args: Any, **kwds: Any) -> bool: - ... + def __call__(self, app: web.Application, *args: Any, **kwds: Any) -> bool: ... class _ApplicationSettings(Protocol): - def is_enabled(self, field_name: str) -> bool: - ... + def is_enabled(self, field_name: str) -> bool: ... class ModuleCategory(Enum): @@ -46,12 +47,10 @@ def __init__(self, *, reason) -> None: super().__init__(reason) -class ApplicationSetupError(Exception): - ... +class ApplicationSetupError(Exception): ... -class DependencyError(ApplicationSetupError): - ... +class DependencyError(ApplicationSetupError): ... class SetupMetadataDict(TypedDict): @@ -134,6 +133,53 @@ def _get_app_settings_and_field_name( return app_settings, settings_field_name +class _SetupTimingContext(ContextDecorator): + """Context manager/decorator for timing and logging module setup operations.""" + + def __init__( + self, + module_name: str, + *, + logger: logging.Logger, + category: ModuleCategory | None = None, + depends: list[str] | None = None, + ) -> None: + """Initialize timing context. + + :param module_name: Name of the module being set up + :param category: Optional module category for detailed logging + :param depends: Optional dependencies for detailed logging + """ + self.module_name = module_name + self.category = category + self.depends = depends + self.started: datetime | None = None + self.head_msg = f"Setup of {module_name}" + self.logger = logger + + def __enter__(self) -> None: + self.started = arrow.utcnow().datetime + if self.category is not None: + self.logger.info( + "%s (%s, %s) started ... ", + self.head_msg, + f"{self.category.name=}", + f"{self.depends}", + ) + else: + self.logger.info("%s started ...", self.head_msg) + + def __exit__( + self, + exc_type: type[BaseException] | None, + exc_val: BaseException | None, + exc_tb: TracebackType | None, + ) -> None: + if self.started: + elapsed = (arrow.utcnow() - self.started).total_seconds() + _logger.info("%s completed [Elapsed: %3.1f secs]", self.head_msg, elapsed) + + # PUBLIC API ------------------------------------------------------------------ @@ -141,13 +187,68 @@ def is_setup_completed(module_name: str, app: web.Application) -> bool: return module_name in app[APP_SETUP_COMPLETED_KEY] +def ensure_single_setup( + module_name: str, + *, + logger: logging.Logger, +) -> Callable[[Callable[..., Any]], Callable[..., Any]]: + """Ensures a setup function is executed only once per application and handles completion. + + :param module_name: Name of the module being set up + """ + + def _log_skip(reason: str) -> bool: + logger.info("Skipping '%s' setup: %s", module_name, reason) + return False + + def decorator(setup_func: _SetupFunc) -> _SetupFunc: + + @functools.wraps(setup_func) + def _wrapper(app: web.Application, *args: Any, **kwargs: Any) -> bool: + + # pre-setup init + if APP_SETUP_COMPLETED_KEY not in app: + app[APP_SETUP_COMPLETED_KEY] = [] + + # check + if is_setup_completed(module_name, app): + _log_skip( + f"'{module_name}' was already initialized in {app}." + " Setup can only be executed once per app." + ) + return False + + try: + completed = setup_func(app, *args, **kwargs) + + # post-setup handling + if completed is None: + completed = True + + if completed: # registers completed setup + app[APP_SETUP_COMPLETED_KEY].append(module_name) + return completed + + assert not completed # nosec + _log_skip("Undefined (setup function returned false)") + return False + + except SkipModuleSetupError as err: + _log_skip(err.reason) + return False + + return _wrapper + + return decorator + + def app_module_setup( module_name: str, category: ModuleCategory, *, settings_name: str | None = None, depends: list[str] | None = None, - logger: logging.Logger = log, + logger: logging.Logger = _logger, # TODO: SEE https://github.com/ITISFoundation/osparc-simcore/issues/2008 # TODO: - settings_name becomes module_name!! # TODO: - plugin base should be aware of setup and settings -> model instead of function? @@ -190,35 +291,27 @@ def setup(app: web.Application): module_name, depends, config_section, config_enabled ) - def _decorate(setup_func: _SetupFunc): - if "setup" not in setup_func.__name__: - logger.warning("Rename '%s' to contain 'setup'", setup_func.__name__) - - # metadata info - def setup_metadata() -> SetupMetadataDict: - return SetupMetadataDict( - module_name=module_name, - dependencies=depends, - config_section=section, - config_enabled=config_enabled, - ) + # metadata info + def _setup_metadata() -> SetupMetadataDict: + return SetupMetadataDict( + module_name=module_name, + dependencies=depends, + config_section=section, + config_enabled=config_enabled, + ) - # wrapper - @functools.wraps(setup_func) - def _wrapper(app: web.Application, *args, **kargs) -> bool: - # pre-setup - head_msg = f"Setup of {module_name}" - started = arrow.utcnow() - logger.info( - "%s (%s, %s) started ... ", - head_msg, - f"{category.name=}", - f"{depends}", - ) + def decorator(setup_func: _SetupFunc) -> _SetupFunc: - if APP_SETUP_COMPLETED_KEY not in app: - app[APP_SETUP_COMPLETED_KEY] = [] + assert setup_func.__name__.startswith( # nosec + "setup_" + ), f"Rename '{setup_func.__name__}' start with 'setup_$(plugin-name)'" + @functools.wraps(setup_func) + @ensure_single_setup(module_name, logger=logger) + @_SetupTimingContext( + module_name, category=category, depends=depends, logger=logger + ) + def _wrapper(app: web.Application, *args, **kargs) -> bool: if category == ModuleCategory.ADDON: # ONLY addons can be enabled/disabled @@ -258,7 +351,6 @@ def _wrapper(app: web.Application, *args, **kargs) -> bool: return False if depends: - # TODO: no need to enforce. Use to deduce order instead. uninitialized = [ dep for dep in depends if not is_setup_completed(dep, app) ] @@ -266,48 +358,20 @@ def _wrapper(app: web.Application, *args, **kargs) -> bool: msg = f"Cannot setup app module '{module_name}' because the following dependencies are still uninitialized: {uninitialized}" raise DependencyError(msg) - # execution of setup - try: - if is_setup_completed(module_name, app): - raise SkipModuleSetupError( # noqa: TRY301 - reason=f"'{module_name}' was already initialized in {app}." - " Setup can only be executed once per app." - ) - - completed = setup_func(app, *args, **kargs) - - # post-setup - if completed is None: - completed = True - - if completed: # registers completed setup - app[APP_SETUP_COMPLETED_KEY].append(module_name) - else: - raise SkipModuleSetupError( # noqa: TRY301 - reason="Undefined (setup function returned false)" - ) - - except SkipModuleSetupError as exc: - logger.info("Skipping '%s' setup: %s", module_name, exc.reason) - completed = False + # execution of setup with module name + completed: bool = setup_func(app, *args, **kargs) - elapsed = arrow.utcnow() - started - logger.info( - "%s %s [Elapsed: %3.1f secs]", - head_msg, - "completed" if completed else "skipped", - elapsed.total_seconds(), - ) return completed - _wrapper.metadata = setup_metadata # type: ignore[attr-defined] + _wrapper.metadata = _setup_metadata # type: ignore[attr-defined] _wrapper.mark_as_simcore_servicelib_setup_func = True # type: ignore[attr-defined] - # NOTE: this is added by functools.wraps decorated - assert _wrapper.__wrapped__ == setup_func # nosec + assert ( + _wrapper.__wrapped__ == setup_func + ), "this is added by functools.wraps decorator" # nosec return _wrapper - return _decorate + return decorator def is_setup_function(fun: Callable) -> bool: From 714381ebda55a00a234ac4857f6f0e6e2cb2fb48 Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Wed, 25 Jun 2025 22:22:46 +0200 Subject: [PATCH 2/8] minor refactor --- .../src/servicelib/aiohttp/application_setup.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/service-library/src/servicelib/aiohttp/application_setup.py b/packages/service-library/src/servicelib/aiohttp/application_setup.py index 4bd3981c8ee2..cf3c3fef9c3a 100644 --- a/packages/service-library/src/servicelib/aiohttp/application_setup.py +++ b/packages/service-library/src/servicelib/aiohttp/application_setup.py @@ -363,12 +363,13 @@ def _wrapper(app: web.Application, *args, **kargs) -> bool: return completed - _wrapper.metadata = _setup_metadata # type: ignore[attr-defined] - _wrapper.mark_as_simcore_servicelib_setup_func = True # type: ignore[attr-defined] assert ( _wrapper.__wrapped__ == setup_func ), "this is added by functools.wraps decorator" # nosec + setattr(_wrapper, "metadata", _setup_metadata) # noqa: B010 + setattr(_wrapper, "mark_as_simcore_servicelib_setup_func", True) # noqa: B010 + return _wrapper return decorator From f0574fb822458ee8f3a18041a1d0614c1e41ca39 Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Wed, 25 Jun 2025 22:30:03 +0200 Subject: [PATCH 3/8] minor --- .../servicelib/aiohttp/application_setup.py | 47 +++++++------------ 1 file changed, 18 insertions(+), 29 deletions(-) diff --git a/packages/service-library/src/servicelib/aiohttp/application_setup.py b/packages/service-library/src/servicelib/aiohttp/application_setup.py index cf3c3fef9c3a..4f43aae5c995 100644 --- a/packages/service-library/src/servicelib/aiohttp/application_setup.py +++ b/packages/service-library/src/servicelib/aiohttp/application_setup.py @@ -1,6 +1,7 @@ import functools import inspect import logging +import warnings from collections.abc import Callable from contextlib import ContextDecorator from copy import deepcopy @@ -257,42 +258,27 @@ def app_module_setup( config_section: str | None = None, config_enabled: str | None = None, ) -> Callable: - """Decorator that marks a function as 'a setup function' for a given module in an application - - - Marks a function as 'setup' of a given module in an application - - Ensures setup executed ONLY ONCE per app - - Addon modules: - - toggles run using 'enabled' entry in config file - - logs execution - - See packages/service-library/tests/test_application_setup.py - - :param module_name: typically __name__ - :param depends: list of module_names that must be called first, defaults to None - :param config_section: explicit configuration section, defaults to None (i.e. the name of the module, or last entry of the name if dotted) - :param config_enabled: option in config to enable, defaults to None which is '$(module-section).enabled' (config_section and config_enabled are mutually exclusive) - :param settings_name: field name in the app's settings that corresponds to this module. Defaults to the name of the module with app prefix. - :raises DependencyError - :raises ApplicationSetupError - :return: True if setup was completed or False if setup was skipped + """Decorator marking a function as a module setup for an application. + + Ensures one-time execution and tracks setup completion. For addons, setup can be + toggled via config [deprecated] or settings. + + :param settings_name: Field name in app settings for this module + :raises DependencyError: If required dependent modules are not initialized + :raises ApplicationSetupError: If setup fails + :return: True if setup completed, False if skipped :rtype: bool :Example: - from servicelib.aiohttp.application_setup import app_module_setup - @app_module_setup('mysubsystem', ModuleCategory.SYSTEM, logger=log) - def setup(app: web.Application): + def setup_mysubsystem(app: web.Application): ... """ - # TODO: resilience to failure. if this setup fails, then considering dependencies, is it fatal or app can start? - # TODO: enforce signature as def setup(app: web.Application, **kwargs) -> web.Application - module_name, depends, section, config_enabled = _parse_and_validate_arguments( module_name, depends, config_section, config_enabled ) - # metadata info - def _setup_metadata() -> SetupMetadataDict: + def _get_metadata() -> SetupMetadataDict: return SetupMetadataDict( module_name=module_name, dependencies=depends, @@ -317,7 +303,11 @@ def _wrapper(app: web.Application, *args, **kargs) -> bool: if settings_name is None: # Fall back to config if settings_name is not explicitly defined - # TODO: deprecate + warnings.warn( + f"Using config-based enabling/disabling for addon '{module_name}' is deprecated. Please use settings instead.", + DeprecationWarning, + stacklevel=2, + ) cfg = app[APP_CONFIG_KEY] is_enabled = _is_addon_enabled_from_config( cfg, config_enabled, section @@ -367,7 +357,7 @@ def _wrapper(app: web.Application, *args, **kargs) -> bool: _wrapper.__wrapped__ == setup_func ), "this is added by functools.wraps decorator" # nosec - setattr(_wrapper, "metadata", _setup_metadata) # noqa: B010 + setattr(_wrapper, "metadata", _get_metadata) # noqa: B010 setattr(_wrapper, "mark_as_simcore_servicelib_setup_func", True) # noqa: B010 return _wrapper @@ -376,7 +366,6 @@ def _wrapper(app: web.Application, *args, **kargs) -> bool: def is_setup_function(fun: Callable) -> bool: - # TODO: use _SetupFunc protocol to check in runtime return ( inspect.isfunction(fun) and hasattr(fun, "mark_as_simcore_servicelib_setup_func") From 7e40b6e711a0911650acf2ff38aeba67eb4073e9 Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Wed, 25 Jun 2025 22:44:40 +0200 Subject: [PATCH 4/8] =?UTF-8?q?=E2=9C=A8=20[Refactor]=20Use=20setattr=20fo?= =?UTF-8?q?r=20metadata=20assignment=20in=20app=5Fmodule=5Fsetup?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../service-library/src/servicelib/aiohttp/application_setup.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/service-library/src/servicelib/aiohttp/application_setup.py b/packages/service-library/src/servicelib/aiohttp/application_setup.py index 4f43aae5c995..c058c907e656 100644 --- a/packages/service-library/src/servicelib/aiohttp/application_setup.py +++ b/packages/service-library/src/servicelib/aiohttp/application_setup.py @@ -260,7 +260,7 @@ def app_module_setup( ) -> Callable: """Decorator marking a function as a module setup for an application. - Ensures one-time execution and tracks setup completion. For addons, setup can be + Ensures one-time execution (idempotent) and tracks setup completion. For addons, setup can be toggled via config [deprecated] or settings. :param settings_name: Field name in app settings for this module From ca68db430ca016d7f1039f90cfc4d1cfc163f3b9 Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Wed, 25 Jun 2025 23:18:43 +0200 Subject: [PATCH 5/8] =?UTF-8?q?=E2=9C=A8=20[Refactor]=20Improve=20app=5Fmo?= =?UTF-8?q?dule=5Fsetup=20decorator=20documentation=20and=20enhance=20test?= =?UTF-8?q?=20fixtures?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../servicelib/aiohttp/application_setup.py | 46 +++-- .../tests/aiohttp/test_application_setup.py | 190 ++++++++++++------ 2 files changed, 162 insertions(+), 74 deletions(-) diff --git a/packages/service-library/src/servicelib/aiohttp/application_setup.py b/packages/service-library/src/servicelib/aiohttp/application_setup.py index c058c907e656..45ae86b9f27b 100644 --- a/packages/service-library/src/servicelib/aiohttp/application_setup.py +++ b/packages/service-library/src/servicelib/aiohttp/application_setup.py @@ -1,7 +1,6 @@ import functools import inspect import logging -import warnings from collections.abc import Callable from contextlib import ContextDecorator from copy import deepcopy @@ -258,27 +257,42 @@ def app_module_setup( config_section: str | None = None, config_enabled: str | None = None, ) -> Callable: - """Decorator marking a function as a module setup for an application. - - Ensures one-time execution (idempotent) and tracks setup completion. For addons, setup can be - toggled via config [deprecated] or settings. - - :param settings_name: Field name in app settings for this module - :raises DependencyError: If required dependent modules are not initialized - :raises ApplicationSetupError: If setup fails - :return: True if setup completed, False if skipped + """Decorator that marks a function as 'a setup function' for a given module in an application + + - Marks a function as 'setup' of a given module in an application + - Ensures setup executed ONLY ONCE per app + - Addon modules: + - toggles run using 'enabled' entry in config file + - logs execution + + See packages/service-library/tests/test_application_setup.py + + :param module_name: typically __name__ + :param depends: list of module_names that must be called first, defaults to None + :param config_section: explicit configuration section, defaults to None (i.e. the name of the module, or last entry of the name if dotted) + :param config_enabled: option in config to enable, defaults to None which is '$(module-section).enabled' (config_section and config_enabled are mutually exclusive) + :param settings_name: field name in the app's settings that corresponds to this module. Defaults to the name of the module with app prefix. + :raises DependencyError + :raises ApplicationSetupError + :return: True if setup was completed or False if setup was skipped :rtype: bool :Example: + from servicelib.aiohttp.application_setup import app_module_setup + @app_module_setup('mysubsystem', ModuleCategory.SYSTEM, logger=log) - def setup_mysubsystem(app: web.Application): + def setup(app: web.Application): ... """ + # TODO: resilience to failure. if this setup fails, then considering dependencies, is it fatal or app can start? + # TODO: enforce signature as def setup(app: web.Application, **kwargs) -> web.Application + module_name, depends, section, config_enabled = _parse_and_validate_arguments( module_name, depends, config_section, config_enabled ) - def _get_metadata() -> SetupMetadataDict: + # metadata info + def _setup_metadata() -> SetupMetadataDict: return SetupMetadataDict( module_name=module_name, dependencies=depends, @@ -303,11 +317,7 @@ def _wrapper(app: web.Application, *args, **kargs) -> bool: if settings_name is None: # Fall back to config if settings_name is not explicitly defined - warnings.warn( - f"Using config-based enabling/disabling for addon '{module_name}' is deprecated. Please use settings instead.", - DeprecationWarning, - stacklevel=2, - ) + # TODO: deprecate cfg = app[APP_CONFIG_KEY] is_enabled = _is_addon_enabled_from_config( cfg, config_enabled, section @@ -357,7 +367,7 @@ def _wrapper(app: web.Application, *args, **kargs) -> bool: _wrapper.__wrapped__ == setup_func ), "this is added by functools.wraps decorator" # nosec - setattr(_wrapper, "metadata", _get_metadata) # noqa: B010 + setattr(_wrapper, "metadata", _setup_metadata) # noqa: B010 setattr(_wrapper, "mark_as_simcore_servicelib_setup_func", True) # noqa: B010 return _wrapper diff --git a/packages/service-library/tests/aiohttp/test_application_setup.py b/packages/service-library/tests/aiohttp/test_application_setup.py index 94af1c07a335..d576ff66f323 100644 --- a/packages/service-library/tests/aiohttp/test_application_setup.py +++ b/packages/service-library/tests/aiohttp/test_application_setup.py @@ -1,52 +1,29 @@ -# pylint:disable=unused-variable -# pylint:disable=unused-argument -# pylint:disable=redefined-outer-name +# pylint: disable=protected-access +# pylint: disable=redefined-outer-name +# pylint: disable=too-many-arguments +# pylint: disable=unused-argument +# pylint: disable=unused-variable -from unittest.mock import Mock + +from collections.abc import Callable import pytest from aiohttp import web +from pytest_mock import MockerFixture, MockType from servicelib.aiohttp.application_keys import APP_CONFIG_KEY from servicelib.aiohttp.application_setup import ( DependencyError, ModuleCategory, SkipModuleSetupError, app_module_setup, + ensure_single_setup, is_setup_completed, ) -log = Mock() - - -@app_module_setup("package.bar", ModuleCategory.ADDON, logger=log) -def setup_bar(app: web.Application, arg1, *, raise_skip: bool = False): - return True - - -@app_module_setup("package.foo", ModuleCategory.ADDON, logger=log) -def setup_foo(app: web.Application, arg1, kargs=33, *, raise_skip: bool = False): - if raise_skip: - raise SkipModuleSetupError(reason="explicit skip") - return True - -@app_module_setup( - "package.zee", ModuleCategory.ADDON, config_enabled="main.zee_enabled", logger=log -) -def setup_zee(app: web.Application, arg1, kargs=55): - return True - - -@app_module_setup( - "package.needs_foo", - ModuleCategory.SYSTEM, - depends=[ - "package.foo", - ], - logger=log, -) -def setup_needs_foo(app: web.Application, arg1, kargs=55): - return True +@pytest.fixture +def mock_logger(mocker: MockerFixture) -> MockType: + return mocker.patch("logging.getLogger", autospec=True) @pytest.fixture @@ -59,36 +36,107 @@ def app_config() -> dict: @pytest.fixture -def app(app_config): +def app(app_config) -> web.Application: _app = web.Application() _app[APP_CONFIG_KEY] = app_config return _app -def test_setup_config_enabled(app_config, app): - assert setup_zee(app, 1) +@pytest.fixture +def create_setup_bar() -> Callable[[web.Application, str, bool], bool]: + @app_module_setup("package.bar", ModuleCategory.ADDON) + def _setup_bar( + app: web.Application, arg1: str, *, raise_skip: bool = False + ) -> bool: + return True + + return _setup_bar + + +@pytest.fixture +def create_setup_foo() -> Callable[[web.Application, str, bool], bool]: + @app_module_setup("package.foo", ModuleCategory.ADDON) + def _setup_foo( + app: web.Application, arg1: str, *, raise_skip: bool = False, **kwargs + ) -> bool: + if raise_skip: + raise SkipModuleSetupError(reason="explicit skip") + return True + + return _setup_foo + + +@pytest.fixture +def create_setup_zee() -> Callable[[web.Application, str, int], bool]: + @app_module_setup( + "package.zee", ModuleCategory.ADDON, config_enabled="main.zee_enabled" + ) + def _setup_zee(app: web.Application, arg1: str, kargs: int = 55) -> bool: + return True + return _setup_zee + + +@pytest.fixture +def create_setup_needs_foo() -> Callable[[web.Application, str, int], bool]: + @app_module_setup( + "package.needs_foo", + ModuleCategory.SYSTEM, + depends=[ + "package.foo", + ], + ) + def _setup_needs_foo(app: web.Application, arg1: str, kargs: int = 55) -> bool: + return True + + return _setup_needs_foo + + +def setup_basic(app: web.Application) -> bool: + return True + + +def setup_that_raises(app: web.Application) -> bool: + error_msg = "Setup failed" + raise ValueError(error_msg) + + +def test_setup_config_enabled( + app_config: dict, app: web.Application, create_setup_zee: Callable +): + setup_zee = create_setup_zee() + + assert setup_zee(app, 1) assert setup_zee.metadata()["config_enabled"] == "main.zee_enabled" app_config["main"]["zee_enabled"] = False assert not setup_zee(app, 2) -def test_setup_dependencies(app_config, app): +def test_setup_dependencies( + app: web.Application, + create_setup_needs_foo: Callable, + create_setup_foo: Callable, +) -> None: + setup_needs_foo = create_setup_needs_foo() + setup_foo = create_setup_foo() with pytest.raises(DependencyError): - setup_needs_foo(app, 1) + setup_needs_foo(app, "1") - assert setup_foo(app, 1) - assert setup_needs_foo(app, 2) + assert setup_foo(app, "1") + assert setup_needs_foo(app, "2") assert setup_needs_foo.metadata()["dependencies"] == [ setup_foo.metadata()["module_name"], ] -def test_marked_setup(app_config, app): - assert setup_foo(app, 1) +def test_marked_setup( + app_config: dict, app: web.Application, create_setup_foo: Callable +): + setup_foo = create_setup_foo() + assert setup_foo(app, 1) assert setup_foo.metadata()["module_name"] == "package.foo" assert is_setup_completed(setup_foo.metadata()["module_name"], app) @@ -96,18 +144,48 @@ def test_marked_setup(app_config, app): assert not setup_foo(app, 2) -def test_skip_setup(app_config, app): - try: - log.reset_mock() +def test_skip_setup( + app: web.Application, mock_logger: MockType, create_setup_foo: Callable +): + setup_foo = create_setup_foo() + assert not setup_foo(app, 1, raise_skip=True) + assert setup_foo(app, 1) + mock_logger.info.assert_called() + + +def test_ensure_single_setup_runs_once(app: web.Application, mock_logger: Mock) -> None: + decorated = ensure_single_setup("test.module", logger=mock_logger)(setup_basic) + + # First call succeeds + assert decorated(app) + assert is_setup_completed("test.module", app) + + # Second call skips + assert not decorated(app) + mock_logger.info.assert_called_with( + "Skipping '%s' setup: %s", + "test.module", + "'test.module' was already initialized in . Setup can only be executed once per app.", + ) + + +def test_ensure_single_setup_error_handling( + app: web.Application, mock_logger: Mock +) -> None: + decorated = ensure_single_setup("test.error", logger=mock_logger)(setup_that_raises) + + with pytest.raises(ValueError, match="Setup failed"): + decorated(app) + assert not is_setup_completed("test.error", app) - assert not setup_foo(app, 1, raise_skip=True) - # FIXME: mock logger - # assert log.warning.called - # warn_msg = log.warning.call_args()[0] - # assert "package.foo" in warn_msg - # assert "explicit skip" in warn_msg +def test_ensure_single_setup_multiple_modules( + app: web.Application, mock_logger: Mock +) -> None: + decorated1 = ensure_single_setup("module1", logger=mock_logger)(setup_basic) + decorated2 = ensure_single_setup("module2", logger=mock_logger)(setup_basic) - assert setup_foo(app, 1) - finally: - log.reset_mock() + assert decorated1(app) + assert decorated2(app) + assert is_setup_completed("module1", app) + assert is_setup_completed("module2", app) From 9061f1cff77d5d6fd1e7a6069f5c7f07d3e9b9d2 Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Wed, 25 Jun 2025 23:30:49 +0200 Subject: [PATCH 6/8] fixing tests --- .../tests/aiohttp/test_application_setup.py | 34 +++++++++---------- 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/packages/service-library/tests/aiohttp/test_application_setup.py b/packages/service-library/tests/aiohttp/test_application_setup.py index d576ff66f323..96c1fc6dd9ee 100644 --- a/packages/service-library/tests/aiohttp/test_application_setup.py +++ b/packages/service-library/tests/aiohttp/test_application_setup.py @@ -36,45 +36,41 @@ def app_config() -> dict: @pytest.fixture -def app(app_config) -> web.Application: +def app(app_config: dict) -> web.Application: _app = web.Application() _app[APP_CONFIG_KEY] = app_config return _app @pytest.fixture -def create_setup_bar() -> Callable[[web.Application, str, bool], bool]: +def create_setup_bar() -> Callable[[web.Application], bool]: @app_module_setup("package.bar", ModuleCategory.ADDON) - def _setup_bar( - app: web.Application, arg1: str, *, raise_skip: bool = False - ) -> bool: + def setup_bar(app: web.Application) -> bool: return True - return _setup_bar + return setup_bar @pytest.fixture -def create_setup_foo() -> Callable[[web.Application, str, bool], bool]: - @app_module_setup("package.foo", ModuleCategory.ADDON) - def _setup_foo( - app: web.Application, arg1: str, *, raise_skip: bool = False, **kwargs - ) -> bool: +def create_setup_foo(mock_logger: MockType) -> Callable[[web.Application], bool]: + @app_module_setup("package.foo", ModuleCategory.ADDON, logger=mock_logger) + def setup_foo(app: web.Application, *, raise_skip: bool = False) -> bool: if raise_skip: raise SkipModuleSetupError(reason="explicit skip") return True - return _setup_foo + return setup_foo @pytest.fixture -def create_setup_zee() -> Callable[[web.Application, str, int], bool]: +def create_setup_zee() -> Callable[[web.Application, object, int], bool]: @app_module_setup( "package.zee", ModuleCategory.ADDON, config_enabled="main.zee_enabled" ) - def _setup_zee(app: web.Application, arg1: str, kargs: int = 55) -> bool: + def setup_zee(app: web.Application, arg1, kargs=55) -> bool: return True - return _setup_zee + return setup_zee @pytest.fixture @@ -153,7 +149,9 @@ def test_skip_setup( mock_logger.info.assert_called() -def test_ensure_single_setup_runs_once(app: web.Application, mock_logger: Mock) -> None: +def test_ensure_single_setup_runs_once( + app: web.Application, mock_logger: MockType +) -> None: decorated = ensure_single_setup("test.module", logger=mock_logger)(setup_basic) # First call succeeds @@ -170,7 +168,7 @@ def test_ensure_single_setup_runs_once(app: web.Application, mock_logger: Mock) def test_ensure_single_setup_error_handling( - app: web.Application, mock_logger: Mock + app: web.Application, mock_logger: MockType ) -> None: decorated = ensure_single_setup("test.error", logger=mock_logger)(setup_that_raises) @@ -180,7 +178,7 @@ def test_ensure_single_setup_error_handling( def test_ensure_single_setup_multiple_modules( - app: web.Application, mock_logger: Mock + app: web.Application, mock_logger: MockType ) -> None: decorated1 = ensure_single_setup("module1", logger=mock_logger)(setup_basic) decorated2 = ensure_single_setup("module2", logger=mock_logger)(setup_basic) From 9d69b0dc3ad256a4ca8a78fbf7a5f9108b22e2ea Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Thu, 26 Jun 2025 09:35:52 +0200 Subject: [PATCH 7/8] using ensure single setup --- .../server/src/simcore_service_webserver/login/plugin.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/services/web/server/src/simcore_service_webserver/login/plugin.py b/services/web/server/src/simcore_service_webserver/login/plugin.py index 60c8db47b6fc..fa7c7b452480 100644 --- a/services/web/server/src/simcore_service_webserver/login/plugin.py +++ b/services/web/server/src/simcore_service_webserver/login/plugin.py @@ -4,7 +4,11 @@ import asyncpg from aiohttp import web from pydantic import ValidationError -from servicelib.aiohttp.application_setup import ModuleCategory, app_module_setup +from servicelib.aiohttp.application_setup import ( + ModuleCategory, + app_module_setup, + ensure_single_setup, +) from settings_library.email import SMTPSettings from settings_library.postgres import PostgresSettings @@ -61,11 +65,13 @@ async def _setup_login_storage_ctx(app: web.Application): yield # ---------------- +@ensure_single_setup(f"{__name__}.setup_login_storage", logger=log) def setup_login_storage(app: web.Application): if _setup_login_storage_ctx not in app.cleanup_ctx: app.cleanup_ctx.append(_setup_login_storage_ctx) +@ensure_single_setup(f"{__name__}._setup_login_options", logger=log) def _setup_login_options(app: web.Application): settings: SMTPSettings = get_email_plugin_settings(app) From 5b9cf941850dae2ef9f3cf58139f427a0eb852ca Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Thu, 26 Jun 2025 10:04:07 +0200 Subject: [PATCH 8/8] fixes tsts --- .../servicelib/aiohttp/application_setup.py | 6 +- .../tests/aiohttp/test_application_setup.py | 143 ++++++++---------- 2 files changed, 62 insertions(+), 87 deletions(-) diff --git a/packages/service-library/src/servicelib/aiohttp/application_setup.py b/packages/service-library/src/servicelib/aiohttp/application_setup.py index 45ae86b9f27b..330e341313db 100644 --- a/packages/service-library/src/servicelib/aiohttp/application_setup.py +++ b/packages/service-library/src/servicelib/aiohttp/application_setup.py @@ -302,9 +302,9 @@ def _setup_metadata() -> SetupMetadataDict: def decorator(setup_func: _SetupFunc) -> _SetupFunc: - assert setup_func.__name__.startswith( # nosec - "setup_" - ), f"Rename '{setup_func.__name__}' start with 'setup_$(plugin-name)'" + assert ( # nosec + "setup_" in setup_func.__name__ + ), f"Rename '{setup_func.__name__}' like 'setup_$(plugin-name)'" @functools.wraps(setup_func) @ensure_single_setup(module_name, logger=logger) diff --git a/packages/service-library/tests/aiohttp/test_application_setup.py b/packages/service-library/tests/aiohttp/test_application_setup.py index 96c1fc6dd9ee..55f92820eddb 100644 --- a/packages/service-library/tests/aiohttp/test_application_setup.py +++ b/packages/service-library/tests/aiohttp/test_application_setup.py @@ -5,7 +5,7 @@ # pylint: disable=unused-variable -from collections.abc import Callable +import logging import pytest from aiohttp import web @@ -23,7 +23,8 @@ @pytest.fixture def mock_logger(mocker: MockerFixture) -> MockType: - return mocker.patch("logging.getLogger", autospec=True) + logger_mock: MockType = mocker.create_autospec(logging.Logger, instance=True) + return logger_mock @pytest.fixture @@ -42,116 +43,95 @@ def app(app_config: dict) -> web.Application: return _app -@pytest.fixture -def create_setup_bar() -> Callable[[web.Application], bool]: - @app_module_setup("package.bar", ModuleCategory.ADDON) - def setup_bar(app: web.Application) -> bool: +def test_setup_config_enabled(app_config: dict, app: web.Application): + + @app_module_setup( + "package.zee", + ModuleCategory.ADDON, + # legacy support for config_enabled + config_enabled="main.zee_enabled", + ) + def setup_zee(app: web.Application, arg) -> bool: + assert arg return True - return setup_bar + assert setup_zee(app, 1) + assert setup_zee.metadata()["config_enabled"] == "main.zee_enabled" + app_config["main"]["zee_enabled"] = False -@pytest.fixture -def create_setup_foo(mock_logger: MockType) -> Callable[[web.Application], bool]: - @app_module_setup("package.foo", ModuleCategory.ADDON, logger=mock_logger) - def setup_foo(app: web.Application, *, raise_skip: bool = False) -> bool: - if raise_skip: - raise SkipModuleSetupError(reason="explicit skip") - return True + assert not setup_zee(app, 2) - return setup_foo +def test_setup_dependencies(app: web.Application): -@pytest.fixture -def create_setup_zee() -> Callable[[web.Application, object, int], bool]: - @app_module_setup( - "package.zee", ModuleCategory.ADDON, config_enabled="main.zee_enabled" - ) - def setup_zee(app: web.Application, arg1, kargs=55) -> bool: + @app_module_setup("package.foo", ModuleCategory.ADDON) + def setup_foo(app: web.Application) -> bool: return True - return setup_zee - - -@pytest.fixture -def create_setup_needs_foo() -> Callable[[web.Application, str, int], bool]: @app_module_setup( "package.needs_foo", ModuleCategory.SYSTEM, depends=[ + # This module needs foo to be setup first "package.foo", ], ) - def _setup_needs_foo(app: web.Application, arg1: str, kargs: int = 55) -> bool: + def setup_needs_foo(app: web.Application) -> bool: return True - return _setup_needs_foo - - -def setup_basic(app: web.Application) -> bool: - return True - - -def setup_that_raises(app: web.Application) -> bool: - error_msg = "Setup failed" - raise ValueError(error_msg) - - -def test_setup_config_enabled( - app_config: dict, app: web.Application, create_setup_zee: Callable -): - setup_zee = create_setup_zee() - - assert setup_zee(app, 1) - assert setup_zee.metadata()["config_enabled"] == "main.zee_enabled" - app_config["main"]["zee_enabled"] = False - assert not setup_zee(app, 2) - - -def test_setup_dependencies( - app: web.Application, - create_setup_needs_foo: Callable, - create_setup_foo: Callable, -) -> None: - setup_needs_foo = create_setup_needs_foo() - setup_foo = create_setup_foo() - + # setup_foo is not called yet with pytest.raises(DependencyError): - setup_needs_foo(app, "1") + setup_needs_foo(app) - assert setup_foo(app, "1") - assert setup_needs_foo(app, "2") + # ok + assert setup_foo(app) + assert setup_needs_foo(app) + # meta assert setup_needs_foo.metadata()["dependencies"] == [ setup_foo.metadata()["module_name"], ] -def test_marked_setup( - app_config: dict, app: web.Application, create_setup_foo: Callable -): - setup_foo = create_setup_foo() +def test_marked_setup(app_config: dict, app: web.Application): + @app_module_setup("package.foo", ModuleCategory.ADDON) + def setup_foo(app: web.Application) -> bool: + return True - assert setup_foo(app, 1) + assert setup_foo(app) assert setup_foo.metadata()["module_name"] == "package.foo" assert is_setup_completed(setup_foo.metadata()["module_name"], app) app_config["foo"]["enabled"] = False - assert not setup_foo(app, 2) + assert not setup_foo(app) -def test_skip_setup( - app: web.Application, mock_logger: MockType, create_setup_foo: Callable -): - setup_foo = create_setup_foo() - assert not setup_foo(app, 1, raise_skip=True) - assert setup_foo(app, 1) - mock_logger.info.assert_called() +def test_skip_setup(app: web.Application, mock_logger: MockType): + @app_module_setup("package.foo", ModuleCategory.ADDON, logger=mock_logger) + def setup_foo(app: web.Application, *, raise_skip: bool = False) -> bool: + if raise_skip: + raise SkipModuleSetupError(reason="explicit skip") + return True + assert not setup_foo(app, raise_skip=True) + assert setup_foo(app) -def test_ensure_single_setup_runs_once( - app: web.Application, mock_logger: MockType -) -> None: + assert mock_logger.info.called + args = [call.args[-1] for call in mock_logger.info.mock_calls] + assert any("explicit skip" in arg for arg in args) + + +def setup_basic(app: web.Application) -> bool: + return True + + +def setup_that_raises(app: web.Application) -> bool: + error_msg = "Setup failed" + raise ValueError(error_msg) + + +def test_ensure_single_setup_runs_once(app: web.Application, mock_logger: MockType): decorated = ensure_single_setup("test.module", logger=mock_logger)(setup_basic) # First call succeeds @@ -160,16 +140,11 @@ def test_ensure_single_setup_runs_once( # Second call skips assert not decorated(app) - mock_logger.info.assert_called_with( - "Skipping '%s' setup: %s", - "test.module", - "'test.module' was already initialized in . Setup can only be executed once per app.", - ) def test_ensure_single_setup_error_handling( app: web.Application, mock_logger: MockType -) -> None: +): decorated = ensure_single_setup("test.error", logger=mock_logger)(setup_that_raises) with pytest.raises(ValueError, match="Setup failed"): @@ -179,7 +154,7 @@ def test_ensure_single_setup_error_handling( def test_ensure_single_setup_multiple_modules( app: web.Application, mock_logger: MockType -) -> None: +): decorated1 = ensure_single_setup("module1", logger=mock_logger)(setup_basic) decorated2 = ensure_single_setup("module2", logger=mock_logger)(setup_basic)