From 45982b49c649dd7c2c7b532d50dcc1d8a128422d Mon Sep 17 00:00:00 2001 From: Matthieu MN <10926130+gazorby@users.noreply.github.com> Date: Thu, 19 Jan 2023 02:37:20 +0100 Subject: [PATCH] fix(sentry): configure sentry before app instantiation (#258) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 🐛 fix(sentry): call configure before starlite init * chore: changes for rebase on 0.29 Co-authored-by: Peter Schutt --- src/starlite_saqlalchemy/exceptions.py | 8 ++ src/starlite_saqlalchemy/init_plugin.py | 8 +- src/starlite_saqlalchemy/sentry.py | 2 +- tests/unit/__init__.py | 0 tests/unit/no_extras/__init__.py | 0 ...n.py => test_init_plugin_for_no_extras.py} | 3 + .../worker/test_init_plugin_for_worker.py | 27 +++++++ .../sentry/test_init_plugin_for_sentry.py | 60 +++++++++++++++ ...h_check.py => test_health_check_for_sa.py} | 0 tests/unit/test_init_plugin.py | 76 ++++++++++--------- 10 files changed, 146 insertions(+), 38 deletions(-) delete mode 100644 tests/unit/__init__.py delete mode 100644 tests/unit/no_extras/__init__.py rename tests/unit/no_extras/{test_init_plugin.py => test_init_plugin_for_no_extras.py} (97%) create mode 100644 tests/unit/redis/worker/test_init_plugin_for_worker.py create mode 100644 tests/unit/sentry/test_init_plugin_for_sentry.py rename tests/unit/sqlalchemy/{test_health_check.py => test_health_check_for_sa.py} (100%) diff --git a/src/starlite_saqlalchemy/exceptions.py b/src/starlite_saqlalchemy/exceptions.py index 8f25be7a..4639d512 100644 --- a/src/starlite_saqlalchemy/exceptions.py +++ b/src/starlite_saqlalchemy/exceptions.py @@ -30,6 +30,8 @@ __all__ = ( "AuthorizationError", "ConflictError", + "HealthCheckConfigurationError", + "MissingDependencyError", "NotFoundError", "StarliteSaqlalchemyError", "after_exception_hook_handler", @@ -60,6 +62,12 @@ class MissingDependencyError(StarliteSaqlalchemyError, ValueError): """A required dependency is not installed.""" def __init__(self, module: str, config: str | None = None) -> None: + """ + + Args: + module: name of the package that should be installed + config: name of the extra to install the package + """ config = config if config else module super().__init__( f"You enabled {config} configuration but package {module!r} is not installed. " diff --git a/src/starlite_saqlalchemy/init_plugin.py b/src/starlite_saqlalchemy/init_plugin.py index cead9ea6..616d8306 100644 --- a/src/starlite_saqlalchemy/init_plugin.py +++ b/src/starlite_saqlalchemy/init_plugin.py @@ -211,6 +211,9 @@ def __init__(self, config: PluginConfig | None = None) -> None: config: Plugin configuration object. """ self.config = config if config is not None else PluginConfig() + # We must configure sentry before app is instantiated + # because sentry integration replaces Starlite.__init__ + self.configure_sentry() def __call__(self, app_config: AppConfig) -> AppConfig: """Entrypoint to the app config plugin. @@ -231,7 +234,6 @@ def __call__(self, app_config: AppConfig) -> AppConfig: self.configure_exception_handlers(app_config) self.configure_logging(app_config) self.configure_openapi(app_config) - self.configure_sentry(app_config) self.configure_sqlalchemy_plugin(app_config) self.configure_type_encoders(app_config) self.configure_worker(app_config) @@ -368,7 +370,7 @@ def configure_openapi(self, app_config: AppConfig) -> None: if self.config.do_openapi and app_config.openapi_config == DEFAULT_OPENAPI_CONFIG: app_config.openapi_config = openapi.config - def configure_sentry(self, app_config: AppConfig) -> None: + def configure_sentry(self) -> None: """Add handler to configure Sentry integration. Args: @@ -377,7 +379,7 @@ def configure_sentry(self, app_config: AppConfig) -> None: if self.config.do_sentry: from starlite_saqlalchemy import sentry - app_config.on_startup.append(sentry.configure) + sentry.configure() def configure_sqlalchemy_plugin(self, app_config: AppConfig) -> None: """Configure `SQLAlchemy` for the application. diff --git a/src/starlite_saqlalchemy/sentry.py b/src/starlite_saqlalchemy/sentry.py index d968fc63..4941d68b 100644 --- a/src/starlite_saqlalchemy/sentry.py +++ b/src/starlite_saqlalchemy/sentry.py @@ -37,7 +37,7 @@ def configure() -> None: See [SentrySettings][starlite_saqlalchemy.settings.SentrySettings]. """ - sentry_sdk.init( # pragma: no cover + sentry_sdk.init( dsn=settings.sentry.DSN, environment=settings.app.ENVIRONMENT, release=settings.app.BUILD_NUMBER, diff --git a/tests/unit/__init__.py b/tests/unit/__init__.py deleted file mode 100644 index e69de29b..00000000 diff --git a/tests/unit/no_extras/__init__.py b/tests/unit/no_extras/__init__.py deleted file mode 100644 index e69de29b..00000000 diff --git a/tests/unit/no_extras/test_init_plugin.py b/tests/unit/no_extras/test_init_plugin_for_no_extras.py similarity index 97% rename from tests/unit/no_extras/test_init_plugin.py rename to tests/unit/no_extras/test_init_plugin_for_no_extras.py index 66cf2968..22cbf6bd 100644 --- a/tests/unit/no_extras/test_init_plugin.py +++ b/tests/unit/no_extras/test_init_plugin_for_no_extras.py @@ -1,4 +1,7 @@ """Tests for init_plugin.py when no extra dependencies are installed.""" +# pylint:disable=duplicate-code + +from __future__ import annotations import pytest from pydantic import ValidationError diff --git a/tests/unit/redis/worker/test_init_plugin_for_worker.py b/tests/unit/redis/worker/test_init_plugin_for_worker.py new file mode 100644 index 00000000..0e1cc97c --- /dev/null +++ b/tests/unit/redis/worker/test_init_plugin_for_worker.py @@ -0,0 +1,27 @@ +"""Tests for init_plugin.py.""" + +from __future__ import annotations + +from typing import TYPE_CHECKING +from unittest.mock import MagicMock + +from starlite import Starlite + +from starlite_saqlalchemy import init_plugin, worker + +if TYPE_CHECKING: + + from pytest import MonkeyPatch + + +def test_do_worker_but_not_logging(monkeypatch: MonkeyPatch) -> None: + """Tests branch where we can have the worker enabled, but logging + disabled.""" + mock = MagicMock() + monkeypatch.setattr(worker, "create_worker_instance", mock) + config = init_plugin.PluginConfig(do_logging=False) + Starlite(route_handlers=[], on_app_init=[init_plugin.ConfigureApp(config=config)]) + mock.assert_called_once() + call = mock.mock_calls[0] + assert "before_process" not in call.kwargs + assert "after_process" not in call.kwargs diff --git a/tests/unit/sentry/test_init_plugin_for_sentry.py b/tests/unit/sentry/test_init_plugin_for_sentry.py new file mode 100644 index 00000000..7bc32911 --- /dev/null +++ b/tests/unit/sentry/test_init_plugin_for_sentry.py @@ -0,0 +1,60 @@ +"""Tests for init_plugin.py.""" + +from __future__ import annotations + +from typing import TYPE_CHECKING +from unittest.mock import MagicMock + +import pytest +from sentry_sdk.integrations.starlite import SentryStarliteASGIMiddleware +from sentry_sdk.integrations.starlite import ( + exception_handler as sentry_after_exception_handler, +) +from starlite import Starlite +from starlite.handlers import BaseRouteHandler +from starlite.routes.http import HTTPRoute + +from starlite_saqlalchemy import init_plugin, sentry + +if TYPE_CHECKING: + + from pytest import MonkeyPatch + + +@pytest.mark.parametrize( + ("env", "exp"), [("dev", True), ("prod", True), ("local", False), ("test", False)] +) +def test_sentry_environment_gate(env: str, exp: bool, monkeypatch: MonkeyPatch) -> None: + """Test that the sentry integration is configured under different + environment names.""" + monkeypatch.setattr(init_plugin, "IS_LOCAL_ENVIRONMENT", env == "local") + monkeypatch.setattr(init_plugin, "IS_TEST_ENVIRONMENT", env == "test") + mock = MagicMock() + monkeypatch.setattr(sentry, "configure", mock) + Starlite(route_handlers=[], on_app_init=[init_plugin.ConfigureApp()]) + assert mock.call_count == int(exp) + + +def test_do_sentry() -> None: + """Test that do_sentry flag correctly patch Starlite.""" + old_init = Starlite.__init__ + old_route_handle = HTTPRoute.handle + old_resolve_middleware = BaseRouteHandler.resolve_middleware + + app = Starlite( + route_handlers=[], + on_app_init=[init_plugin.ConfigureApp(config=init_plugin.PluginConfig(do_sentry=True))], + ) + after_exception_handlers = [] + for handler in app.after_exception: + handler_fn = handler.ref.value + # If wrapped with `starlite.utils.async_partial` + if hasattr(handler_fn, "func"): + handler_fn = handler_fn.func + after_exception_handlers.append(handler_fn) + assert SentryStarliteASGIMiddleware in app.middleware # type: ignore + assert sentry_after_exception_handler in after_exception_handlers # type: ignore + + Starlite.__init__ = old_init # type: ignore + HTTPRoute.handle = old_route_handle # type: ignore + BaseRouteHandler.resolve_middleware = old_resolve_middleware # type: ignore diff --git a/tests/unit/sqlalchemy/test_health_check.py b/tests/unit/sqlalchemy/test_health_check_for_sa.py similarity index 100% rename from tests/unit/sqlalchemy/test_health_check.py rename to tests/unit/sqlalchemy/test_health_check_for_sa.py diff --git a/tests/unit/test_init_plugin.py b/tests/unit/test_init_plugin.py index f7127f20..ad6321b0 100644 --- a/tests/unit/test_init_plugin.py +++ b/tests/unit/test_init_plugin.py @@ -1,36 +1,59 @@ """Tests for init_plugin.py.""" -# pylint:disable=import-outside-toplevel +# pylint:disable=duplicate-code + from __future__ import annotations from typing import TYPE_CHECKING -from unittest.mock import MagicMock import pytest from starlite import Starlite +from starlite.cache import SimpleCacheBackend from starlite_saqlalchemy import init_plugin -from starlite_saqlalchemy.constants import IS_SAQ_INSTALLED, IS_SENTRY_SDK_INSTALLED +from starlite_saqlalchemy.constants import IS_REDIS_INSTALLED if TYPE_CHECKING: from typing import Any - from pytest import MonkeyPatch - -@pytest.mark.skipif(not IS_SAQ_INSTALLED, reason="saq is not installed") -def test_do_worker_but_not_logging(monkeypatch: MonkeyPatch) -> None: - """Tests branch where we can have the worker enabled, but logging - disabled.""" - from starlite_saqlalchemy import worker - - mock = MagicMock() - monkeypatch.setattr(worker, "create_worker_instance", mock) - config = init_plugin.PluginConfig(do_logging=False, do_worker=True) - Starlite(route_handlers=[], on_app_init=[init_plugin.ConfigureApp(config=config)]) - mock.assert_called_once() - call = mock.mock_calls[0] - assert "before_process" not in call.kwargs - assert "after_process" not in call.kwargs +def test_config_switches() -> None: + """Tests that the app produced with all config switches off is as we + expect.""" + config = init_plugin.PluginConfig( + do_after_exception=False, + do_cache=False, + do_compression=False, + # pyright reckons this parameter doesn't exist, I beg to differ + do_collection_dependencies=False, # pyright:ignore + do_exception_handlers=False, + do_health_check=False, + do_logging=False, + do_openapi=False, + do_sentry=False, + do_set_debug=False, + do_sqlalchemy_plugin=False, + do_type_encoders=False, + do_worker=False, + ) + app = Starlite( + route_handlers=[], + openapi_config=None, + on_app_init=[init_plugin.ConfigureApp(config=config)], + ) + assert app.compression_config is None + assert app.debug is False + assert app.logging_config is None + assert app.openapi_config is None + assert app.response_class is None + assert isinstance(app.cache.backend, SimpleCacheBackend) + # client.close goes in there unconditionally atm + assert len(app.on_shutdown) == 1 if IS_REDIS_INSTALLED is False else 2 + assert not app.after_exception + assert not app.dependencies + assert not app.exception_handlers + assert not app.on_startup + assert not app.plugins + assert not app.routes @pytest.mark.parametrize( @@ -46,18 +69,3 @@ def test_ensure_list(in_: Any, out: Any) -> None: """Test _ensure_list() functionality.""" # pylint: disable=protected-access assert init_plugin.ConfigureApp._ensure_list(in_) == out - - -@pytest.mark.skipif(not IS_SENTRY_SDK_INSTALLED, reason="sentry_sdk is not installed") -@pytest.mark.parametrize( - ("env", "exp"), [("dev", True), ("prod", True), ("local", False), ("test", False)] -) -def test_sentry_environment_gate(env: str, exp: bool, monkeypatch: MonkeyPatch) -> None: - """Test that the sentry integration is configured under different - environment names.""" - from starlite_saqlalchemy import sentry - - monkeypatch.setattr(init_plugin, "IS_LOCAL_ENVIRONMENT", env == "local") - monkeypatch.setattr(init_plugin, "IS_TEST_ENVIRONMENT", env == "test") - app = Starlite(route_handlers=[], on_app_init=[init_plugin.ConfigureApp()]) - assert bool(sentry.configure in app.on_startup) is exp # noqa: SIM901