From f37c5bd2a283b5c11c4b7d73dd5da37fbc90580a Mon Sep 17 00:00:00 2001 From: Evgenii Alekseev Date: Wed, 22 Jul 2026 16:33:59 +0300 Subject: [PATCH] refactor: implement generic optional import mechanisms this project is actively using optional imports, which leads to duplicate handling here and there. This commit implements logic in special method to reduce complexity and simplify tests paths --- .../src/ahriman/core/log/journal_handler.py | 8 +++--- .../src/ahriman/core/log/log_loader.py | 12 ++++----- .../src/ahriman/core/module_loader.py | 19 ++++++++++++-- .../tests/ahriman/core/test_module_loader.py | 16 +++++++++++- ahriman-web/src/ahriman/core/auth/helpers.py | 26 ++++++++----------- .../src/ahriman/web/apispec/__init__.py | 15 +++++++---- .../src/ahriman/web/apispec/decorators.py | 2 +- ahriman-web/src/ahriman/web/apispec/info.py | 9 +++---- .../web/middlewares/metrics_handler.py | 13 +++++----- ahriman-web/src/ahriman/web/server_info.py | 2 +- ahriman-web/src/ahriman/web/views/api/docs.py | 2 +- .../src/ahriman/web/views/api/swagger.py | 2 +- .../src/ahriman/web/views/v1/user/login.py | 9 +++---- .../tests/ahriman/web/apispec/test_info.py | 3 ++- .../views/v1/user/test_view_v1_user_login.py | 2 +- .../ahriman_fixtures/__init__.py | 26 +------------------ 16 files changed, 83 insertions(+), 83 deletions(-) diff --git a/ahriman-core/src/ahriman/core/log/journal_handler.py b/ahriman-core/src/ahriman/core/log/journal_handler.py index d123ee06..0c1fcc91 100644 --- a/ahriman-core/src/ahriman/core/log/journal_handler.py +++ b/ahriman-core/src/ahriman/core/log/journal_handler.py @@ -21,6 +21,8 @@ from logging import NullHandler from typing import Any +from ahriman.core.module_loader import optional_module + __all__ = ["JournalHandler"] @@ -40,7 +42,5 @@ class _JournalHandler(NullHandler): del args, kwargs -try: - from systemd.journal import JournalHandler # type: ignore[import-untyped] -except ImportError: - JournalHandler = _JournalHandler +systemd_journal = optional_module("systemd.journal") +JournalHandler = systemd_journal.JournalHandler if systemd_journal else _JournalHandler diff --git a/ahriman-core/src/ahriman/core/log/log_loader.py b/ahriman-core/src/ahriman/core/log/log_loader.py index 6662c43a..9db4bc50 100644 --- a/ahriman-core/src/ahriman/core/log/log_loader.py +++ b/ahriman-core/src/ahriman/core/log/log_loader.py @@ -26,6 +26,7 @@ from typing import ClassVar, Literal from ahriman.core.configuration import Configuration from ahriman.core.log.http_log_handler import HttpLogHandler from ahriman.core.log.log_context import LogContext +from ahriman.core.module_loader import optional_module from ahriman.models.log_handler import LogHandler from ahriman.models.repository_id import RepositoryId @@ -65,14 +66,11 @@ class LogLoader: if selected is not None: return selected - try: - from systemd.journal import JournalHandler # type: ignore[import-untyped] - del JournalHandler + if optional_module("systemd.journal"): return LogHandler.Journald # journald import was found - except ImportError: - if LogLoader.DEFAULT_SYSLOG_DEVICE.exists(): - return LogHandler.Syslog - return LogHandler.Console + if LogLoader.DEFAULT_SYSLOG_DEVICE.exists(): + return LogHandler.Syslog + return LogHandler.Console @staticmethod def load(repository_id: RepositoryId, configuration: Configuration, handler: LogHandler, *, diff --git a/ahriman-core/src/ahriman/core/module_loader.py b/ahriman-core/src/ahriman/core/module_loader.py index ed66e71e..9cb72e93 100644 --- a/ahriman-core/src/ahriman/core/module_loader.py +++ b/ahriman-core/src/ahriman/core/module_loader.py @@ -26,8 +26,7 @@ from pkgutil import ModuleInfo, walk_packages from types import ModuleType from typing import Any, TypeGuard, TypeVar - -__all__ = ["implementations"] +__all__ = ["implementations", "optional_module"] T = TypeVar("T") @@ -74,3 +73,19 @@ def implementations(root_module: ModuleType, base_class: type[T]) -> Iterator[ty for _, attribute in inspect.getmembers(module, is_base_class): yield attribute + + +def optional_module(module_name: str) -> ModuleType | None: + """ + import an optional module + + Args: + module_name(str): fully qualified module name + + Returns: + ModuleType | None: imported module or ``None`` when it cannot be imported + """ + try: + return import_module(module_name) + except ImportError: + return None diff --git a/ahriman-core/tests/ahriman/core/test_module_loader.py b/ahriman-core/tests/ahriman/core/test_module_loader.py index 72c34929..a1d47682 100644 --- a/ahriman-core/tests/ahriman/core/test_module_loader.py +++ b/ahriman-core/tests/ahriman/core/test_module_loader.py @@ -2,7 +2,7 @@ import ahriman.web.views from pathlib import Path -from ahriman.core.module_loader import _modules, implementations +from ahriman.core.module_loader import _modules, implementations, optional_module from ahriman.web.views.base import BaseView @@ -23,3 +23,17 @@ def test_implementations() -> None: assert routes assert all(isinstance(view, type) for view in routes) assert all(issubclass(view, BaseView) for view in routes) + + +def test_optional_module() -> None: + """ + must import an available module + """ + assert optional_module("ahriman.web.views") is ahriman.web.views + + +def test_optional_module_fallback() -> None: + """ + must return none when the module cannot be imported + """ + assert optional_module("missing_ahriman_module") is None diff --git a/ahriman-web/src/ahriman/core/auth/helpers.py b/ahriman-web/src/ahriman/core/auth/helpers.py index 617f4404..6e91af2c 100644 --- a/ahriman-web/src/ahriman/core/auth/helpers.py +++ b/ahriman-web/src/ahriman/core/auth/helpers.py @@ -17,18 +17,10 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see . # -try: - import aiohttp_security -except ImportError: - aiohttp_security = None # type: ignore[assignment] - -try: - import aiohttp_session -except ImportError: - aiohttp_session = None # type: ignore[assignment] - from typing import Any +from ahriman.core.module_loader import optional_module + __all__ = [ "authorized_userid", @@ -39,6 +31,10 @@ __all__ = [ ] +aiohttp_security = optional_module("aiohttp_security") +aiohttp_session = optional_module("aiohttp_session") + + async def authorized_userid(*args: Any, **kwargs: Any) -> Any: """ handle aiohttp security methods @@ -50,7 +46,7 @@ async def authorized_userid(*args: Any, **kwargs: Any) -> Any: Returns: Any: ``None`` in case if no aiohttp_security module found and function call otherwise """ - if aiohttp_security is not None: + if aiohttp_security: return await aiohttp_security.authorized_userid(*args, **kwargs) # pylint: disable=no-value-for-parameter return None @@ -66,7 +62,7 @@ async def check_authorized(*args: Any, **kwargs: Any) -> Any: Returns: Any: ``None`` in case if no aiohttp_security module found and function call otherwise """ - if aiohttp_security is not None: + if aiohttp_security: return await aiohttp_security.check_authorized(*args, **kwargs) # pylint: disable=no-value-for-parameter return None @@ -82,7 +78,7 @@ async def forget(*args: Any, **kwargs: Any) -> Any: Returns: Any: ``None`` in case if no aiohttp_security module found and function call otherwise """ - if aiohttp_security is not None: + if aiohttp_security: return await aiohttp_security.forget(*args, **kwargs) # pylint: disable=no-value-for-parameter return None @@ -98,7 +94,7 @@ async def get_session(*args: Any, **kwargs: Any) -> Any: Returns: Any: empty dictionary in case if no aiohttp_session module found and function call otherwise """ - if aiohttp_session is not None: + if aiohttp_session: return await aiohttp_session.get_session(*args, **kwargs) return {} @@ -114,6 +110,6 @@ async def remember(*args: Any, **kwargs: Any) -> Any: Returns: Any: ``None`` in case if no aiohttp_security module found and function call otherwise """ - if aiohttp_security is not None: + if aiohttp_security: return await aiohttp_security.remember(*args, **kwargs) # pylint: disable=no-value-for-parameter return None diff --git a/ahriman-web/src/ahriman/web/apispec/__init__.py b/ahriman-web/src/ahriman/web/apispec/__init__.py index 1839b1f1..613e9001 100644 --- a/ahriman-web/src/ahriman/web/apispec/__init__.py +++ b/ahriman-web/src/ahriman/web/apispec/__init__.py @@ -17,15 +17,20 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see . # -try: - import aiohttp_apispec # type: ignore[import-untyped] +from ahriman.core.module_loader import optional_module - from marshmallow import Schema, fields -except ImportError: + +aiohttp_apispec = optional_module("aiohttp_apispec") +marshmallow = optional_module("marshmallow") + + +if aiohttp_apispec and marshmallow: + Schema = marshmallow.Schema + fields = marshmallow.fields +else: from unittest.mock import Mock Schema = Mock # type: ignore[misc] - aiohttp_apispec = None fields = Mock() diff --git a/ahriman-web/src/ahriman/web/apispec/decorators.py b/ahriman-web/src/ahriman/web/apispec/decorators.py index adc9baaf..7c5a787f 100644 --- a/ahriman-web/src/ahriman/web/apispec/decorators.py +++ b/ahriman-web/src/ahriman/web/apispec/decorators.py @@ -115,7 +115,7 @@ def apidocs(*, authorization_required = permission != UserAccess.Unauthorized def wrapper(handler: Callable[..., Any]) -> Callable[..., Any]: - if aiohttp_apispec is None: + if not aiohttp_apispec: return handler # apispec is disabled responses = _response_schema( diff --git a/ahriman-web/src/ahriman/web/apispec/info.py b/ahriman-web/src/ahriman/web/apispec/info.py index 4812d3fa..72374acd 100644 --- a/ahriman-web/src/ahriman/web/apispec/info.py +++ b/ahriman-web/src/ahriman/web/apispec/info.py @@ -100,20 +100,17 @@ def _servers(application: Application) -> list[dict[str, Any]]: }] -def setup_apispec(application: Application) -> Any: +def setup_apispec(application: Application) -> None: """ setup swagger api specification Args: application(Application): web application instance - - Returns: - Any: created specification instance if module is available """ if aiohttp_apispec is None: - return None + return - return aiohttp_apispec.setup_aiohttp_apispec( + aiohttp_apispec.setup_aiohttp_apispec( application, url="/api-docs/swagger.json", openapi_version="3.0.2", diff --git a/ahriman-web/src/ahriman/web/middlewares/metrics_handler.py b/ahriman-web/src/ahriman/web/middlewares/metrics_handler.py index 1fba10d3..de29f123 100644 --- a/ahriman-web/src/ahriman/web/middlewares/metrics_handler.py +++ b/ahriman-web/src/ahriman/web/middlewares/metrics_handler.py @@ -17,14 +17,10 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see . # -try: - import aiohttp_openmetrics -except ImportError: - aiohttp_openmetrics = None # type: ignore[assignment] - from aiohttp.typedefs import Middleware from aiohttp.web import HTTPNotFound, Request, Response, StreamResponse, middleware +from ahriman.core.module_loader import optional_module from ahriman.web.middlewares import HandlerType @@ -34,6 +30,9 @@ __all__ = [ ] +aiohttp_openmetrics = optional_module("aiohttp_openmetrics") + + async def metrics(request: Request) -> Response: """ handler for returning metrics @@ -47,7 +46,7 @@ async def metrics(request: Request) -> Response: Raises: HTTPNotFound: endpoint is disabled """ - if aiohttp_openmetrics is None: + if not aiohttp_openmetrics: raise HTTPNotFound return await aiohttp_openmetrics.metrics(request) @@ -59,7 +58,7 @@ def metrics_handler() -> Middleware: Returns: Middleware: middleware function to handle server metrics """ - if aiohttp_openmetrics is not None: + if aiohttp_openmetrics: return aiohttp_openmetrics.metrics_middleware @middleware diff --git a/ahriman-web/src/ahriman/web/server_info.py b/ahriman-web/src/ahriman/web/server_info.py index 63677d03..7c08322b 100644 --- a/ahriman-web/src/ahriman/web/server_info.py +++ b/ahriman-web/src/ahriman/web/server_info.py @@ -60,7 +60,7 @@ async def server_info(view: BaseView) -> dict[str, Any]: "username": await authorized_userid(view.request), }, "autorefresh_intervals": sorted(autorefresh_intervals, key=comparator), - "docs_enabled": aiohttp_apispec is not None, + "docs_enabled": bool(aiohttp_apispec), "index_url": view.configuration.get("web", "index_url", fallback=None), "repositories": [ { diff --git a/ahriman-web/src/ahriman/web/views/api/docs.py b/ahriman-web/src/ahriman/web/views/api/docs.py index fa873349..bac36094 100644 --- a/ahriman-web/src/ahriman/web/views/api/docs.py +++ b/ahriman-web/src/ahriman/web/views/api/docs.py @@ -50,7 +50,7 @@ class DocsView(BaseView): list[str]: list of routes defined for the view. By default, it tries to read :attr:`ROUTES` option if set and returns empty list otherwise """ - if aiohttp_apispec is None: + if not aiohttp_apispec: return [] return cls.ROUTES diff --git a/ahriman-web/src/ahriman/web/views/api/swagger.py b/ahriman-web/src/ahriman/web/views/api/swagger.py index c1b91a5c..3599496e 100644 --- a/ahriman-web/src/ahriman/web/views/api/swagger.py +++ b/ahriman-web/src/ahriman/web/views/api/swagger.py @@ -51,7 +51,7 @@ class SwaggerView(BaseView): list[str]: list of routes defined for the view. By default, it tries to read :attr:`ROUTES` option if set and returns empty list otherwise """ - if aiohttp_apispec is None: + if not aiohttp_apispec: return [] return cls.ROUTES diff --git a/ahriman-web/src/ahriman/web/views/v1/user/login.py b/ahriman-web/src/ahriman/web/views/v1/user/login.py index 47742306..feb693c5 100644 --- a/ahriman-web/src/ahriman/web/views/v1/user/login.py +++ b/ahriman-web/src/ahriman/web/views/v1/user/login.py @@ -22,6 +22,7 @@ from secrets import token_urlsafe from typing import ClassVar from ahriman.core.auth.helpers import get_session, remember +from ahriman.core.module_loader import optional_module from ahriman.models.user_access import UserAccess from ahriman.web.apispec.decorators import apidocs from ahriman.web.schemas import LoginSchema, OAuth2Schema @@ -62,14 +63,12 @@ class LoginView(BaseView): HTTPMethodNotAllowed: in case if method is used, but OAuth is disabled HTTPUnauthorized: if case of authorization error """ - try: - from ahriman.core.auth.oauth import OAuth - except ImportError: - # no aioauth library found + oauth = optional_module("ahriman.core.auth.oauth") + if not oauth: raise HTTPMethodNotAllowed(self.request.method, ["POST"]) oauth_provider = self.validator - if not isinstance(oauth_provider, OAuth): + if not isinstance(oauth_provider, oauth.OAuth): raise HTTPMethodNotAllowed(self.request.method, ["POST"]) session = await get_session(self.request) diff --git a/ahriman-web/tests/ahriman/web/apispec/test_info.py b/ahriman-web/tests/ahriman/web/apispec/test_info.py index 5372ddaf..e4f3023c 100644 --- a/ahriman-web/tests/ahriman/web/apispec/test_info.py +++ b/ahriman-web/tests/ahriman/web/apispec/test_info.py @@ -48,7 +48,8 @@ def test_setup_apispec(application: Application, mocker: MockerFixture) -> None: must set api specification """ apispec_mock = mocker.patch("aiohttp_apispec.setup_aiohttp_apispec") - assert setup_apispec(application) + + setup_apispec(application) apispec_mock.assert_called_once_with( application, url="/api-docs/swagger.json", diff --git a/ahriman-web/tests/ahriman/web/views/v1/user/test_view_v1_user_login.py b/ahriman-web/tests/ahriman/web/views/v1/user/test_view_v1_user_login.py index 5ff6d26d..d64dd02d 100644 --- a/ahriman-web/tests/ahriman/web/views/v1/user/test_view_v1_user_login.py +++ b/ahriman-web/tests/ahriman/web/views/v1/user/test_view_v1_user_login.py @@ -37,7 +37,7 @@ async def test_get_import_error(client_with_auth: TestClient, mocker: MockerFixt """ must return 405 on import error """ - pytest.helpers.import_error("ahriman.core.auth.oauth", ["OAuth"], mocker) + mocker.patch("ahriman.web.views.v1.user.login.optional_module", return_value=None) response = await client_with_auth.get("/api/v1/login") assert response.status == 405 diff --git a/tools/pytest_plugins/ahriman_fixtures/__init__.py b/tools/pytest_plugins/ahriman_fixtures/__init__.py index 80d202f7..102d3734 100644 --- a/tools/pytest_plugins/ahriman_fixtures/__init__.py +++ b/tools/pytest_plugins/ahriman_fixtures/__init__.py @@ -117,36 +117,12 @@ def get_package_status_extended(package: Package) -> dict[str, Any]: return {"status": BuildStatus().view(), "package": package.view()} -def import_error(package: str, components: list[str], mocker: MockerFixture) -> MagicMock: - """ - mock import error - - Args: - package(str): package name to import - components(list[str]): component to import if any (e.g. from ... import ...) - mocker(MockerFixture): mocker object - - Returns: - MagicMock: mocked object - """ - import builtins - _import = builtins.__import__ - - # pylint: disable=redefined-builtin - def test_import(name: str, globals: Any, locals: Any, from_list: list[str], level: Any): - if name == package and (not components or any(component in from_list for component in components)): - raise ImportError - return _import(name, globals, locals, from_list, level) - - return mocker.patch.object(builtins, "__import__", test_import) - - @pytest.hookimpl(trylast=True) def pytest_configure() -> None: """ register helpers after pytest-helpers-namespace has initialized """ - for helper in (anyvar, get_package_status, get_package_status_extended, import_error): + for helper in (anyvar, get_package_status, get_package_status_extended): pytest.helpers.register(helper)