From e3f1ce94fd6d52396afccb6317a47cd1ffc3ca07 Mon Sep 17 00:00:00 2001 From: Bart Skowron Date: Tue, 18 May 2021 00:32:56 +0200 Subject: [PATCH] Integrate only when the object has been initialized Previous integrations in the SDK were done by importing rollbar.contrib.'framework'. Since `rollbar._build_person_data()` needs to import Starlette integration, this sets the Starlette hook for the SDK even if not explicility integrated. Under certain cicrumstances, this can result in erroneously add Starlette framework information to the payload and associate occurence with Starlette integration. For now, integration with SDK is make only if integration function/class is called. Fix [ch85137] --- rollbar/contrib/asgi/__init__.py | 9 ---- rollbar/contrib/asgi/middleware.py | 6 ++- rollbar/contrib/fastapi/__init__.py | 10 ---- rollbar/contrib/fastapi/logger.py | 3 ++ rollbar/contrib/fastapi/middleware.py | 4 ++ rollbar/contrib/fastapi/routing.py | 4 +- rollbar/contrib/starlette/__init__.py | 10 ---- rollbar/contrib/starlette/logger.py | 3 ++ rollbar/contrib/starlette/middleware.py | 3 ++ rollbar/test/asgi_tests/test_integration.py | 42 +++++++++------- rollbar/test/asgi_tests/test_middleware.py | 19 +++++++- .../test/fastapi_tests/test_integration.py | 48 ------------------- rollbar/test/fastapi_tests/test_logger.py | 24 +++++++++- rollbar/test/fastapi_tests/test_middleware.py | 24 +++++++++- rollbar/test/fastapi_tests/test_routing.py | 24 +++++++++- .../test/starlette_tests/test_integration.py | 45 ----------------- rollbar/test/starlette_tests/test_logger.py | 25 +++++++++- .../test/starlette_tests/test_middleware.py | 24 +++++++++- 18 files changed, 174 insertions(+), 153 deletions(-) delete mode 100644 rollbar/test/fastapi_tests/test_integration.py delete mode 100644 rollbar/test/starlette_tests/test_integration.py diff --git a/rollbar/contrib/asgi/__init__.py b/rollbar/contrib/asgi/__init__.py index 56b4db03..1dde4ba6 100644 --- a/rollbar/contrib/asgi/__init__.py +++ b/rollbar/contrib/asgi/__init__.py @@ -1,12 +1,3 @@ __all__ = ['ReporterMiddleware'] -import rollbar - from .middleware import ReporterMiddleware - - -def _hook(request, data): - data['framework'] = 'asgi' - - -rollbar.BASE_DATA_HOOK = _hook diff --git a/rollbar/contrib/asgi/middleware.py b/rollbar/contrib/asgi/middleware.py index 1483619c..f965a0cf 100644 --- a/rollbar/contrib/asgi/middleware.py +++ b/rollbar/contrib/asgi/middleware.py @@ -1,12 +1,16 @@ import sys import rollbar +from .integration import IntegrationBase, integrate from .types import ASGIApp, Receive, Scope, Send from rollbar.lib._async import RollbarAsyncError, try_report -class ReporterMiddleware: +@integrate(framework_name='asgi') +class ReporterMiddleware(IntegrationBase): def __init__(self, app: ASGIApp) -> None: + super().__init__() + self.app = app async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: diff --git a/rollbar/contrib/fastapi/__init__.py b/rollbar/contrib/fastapi/__init__.py index 139d59a6..3c96c6e4 100644 --- a/rollbar/contrib/fastapi/__init__.py +++ b/rollbar/contrib/fastapi/__init__.py @@ -1,18 +1,8 @@ __all__ = ['add_to', 'ReporterMiddleware', 'LoggerMiddleware', 'get_current_request'] -from fastapi import __version__ - -import rollbar from .middleware import ReporterMiddleware from .logger import LoggerMiddleware from .routing import add_to # Do not modify the returned request object from rollbar.contrib.starlette import get_current_request - - -def _hook(request, data): - data['framework'] = f'fastapi {__version__}' - - -rollbar.BASE_DATA_HOOK = _hook diff --git a/rollbar/contrib/fastapi/logger.py b/rollbar/contrib/fastapi/logger.py index 816262c4..d0d6d52e 100644 --- a/rollbar/contrib/fastapi/logger.py +++ b/rollbar/contrib/fastapi/logger.py @@ -1,11 +1,14 @@ __all__ = ['LoggerMiddleware'] import logging +from fastapi import __version__ +from rollbar.contrib.asgi.integration import integrate from rollbar.contrib.starlette import LoggerMiddleware as StarletteLoggerMiddleware log = logging.getLogger(__name__) +@integrate(framework_name=f'fastapi {__version__}') class LoggerMiddleware(StarletteLoggerMiddleware): ... diff --git a/rollbar/contrib/fastapi/middleware.py b/rollbar/contrib/fastapi/middleware.py index 5650f0ad..2be1b9e4 100644 --- a/rollbar/contrib/fastapi/middleware.py +++ b/rollbar/contrib/fastapi/middleware.py @@ -1,7 +1,11 @@ __all__ = ['ReporterMiddleware'] +from fastapi import __version__ + +from rollbar.contrib.asgi.integration import integrate from rollbar.contrib.starlette import ReporterMiddleware as StarletteReporterMiddleware +@integrate(framework_name=f'fastapi {__version__}') class ReporterMiddleware(StarletteReporterMiddleware): ... diff --git a/rollbar/contrib/fastapi/routing.py b/rollbar/contrib/fastapi/routing.py index 51354fcc..9cc6f28c 100644 --- a/rollbar/contrib/fastapi/routing.py +++ b/rollbar/contrib/fastapi/routing.py @@ -4,7 +4,7 @@ import sys from typing import Callable, Optional, Type, Union -from fastapi import APIRouter, FastAPI +from fastapi import APIRouter, FastAPI, __version__ from fastapi.routing import APIRoute try: @@ -16,6 +16,7 @@ import rollbar from .utils import fastapi_min_version, get_installed_middlewares, has_bare_routing +from rollbar.contrib.asgi.integration import integrate from rollbar.contrib.starlette.requests import store_current_request from rollbar.lib._async import RollbarAsyncError, try_report @@ -23,6 +24,7 @@ @fastapi_min_version('0.41.0') +@integrate(framework_name=f'fastapi {__version__}') def add_to(app_or_router: Union[FastAPI, APIRouter]) -> Optional[Type[APIRoute]]: """ Adds RollbarLoggingRoute handler to the router app. diff --git a/rollbar/contrib/starlette/__init__.py b/rollbar/contrib/starlette/__init__.py index 3291e01c..f2a14559 100644 --- a/rollbar/contrib/starlette/__init__.py +++ b/rollbar/contrib/starlette/__init__.py @@ -1,17 +1,7 @@ __all__ = ['ReporterMiddleware', 'LoggerMiddleware', 'get_current_request'] -from starlette import __version__ - -import rollbar from .middleware import ReporterMiddleware from .logger import LoggerMiddleware # Do not modify the returned request object from .requests import get_current_request - - -def _hook(request, data): - data['framework'] = f'starlette {__version__}' - - -rollbar.BASE_DATA_HOOK = _hook diff --git a/rollbar/contrib/starlette/logger.py b/rollbar/contrib/starlette/logger.py index ce3b37b9..4804d376 100644 --- a/rollbar/contrib/starlette/logger.py +++ b/rollbar/contrib/starlette/logger.py @@ -3,14 +3,17 @@ import logging import sys +from starlette import __version__ from starlette.types import ASGIApp, Receive, Scope, Send from rollbar.contrib.asgi import ReporterMiddleware as ASGIReporterMiddleware +from rollbar.contrib.asgi.integration import integrate from rollbar.contrib.starlette.requests import store_current_request log = logging.getLogger(__name__) +@integrate(framework_name=f'starlette {__version__}') class LoggerMiddleware(ASGIReporterMiddleware): def __init__(self, app: ASGIApp) -> None: if sys.version_info < (3, 7): diff --git a/rollbar/contrib/starlette/middleware.py b/rollbar/contrib/starlette/middleware.py index 274622e7..4087d98f 100644 --- a/rollbar/contrib/starlette/middleware.py +++ b/rollbar/contrib/starlette/middleware.py @@ -1,14 +1,17 @@ import sys +from starlette import __version__ from starlette.requests import Request from starlette.types import Receive, Scope, Send import rollbar from .requests import store_current_request from rollbar.contrib.asgi import ReporterMiddleware as ASGIReporterMiddleware +from rollbar.contrib.asgi.integration import integrate from rollbar.lib._async import RollbarAsyncError, try_report +@integrate(framework_name=f'starlette {__version__}') class ReporterMiddleware(ASGIReporterMiddleware): async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: try: diff --git a/rollbar/test/asgi_tests/test_integration.py b/rollbar/test/asgi_tests/test_integration.py index c55e2f2e..84578094 100644 --- a/rollbar/test/asgi_tests/test_integration.py +++ b/rollbar/test/asgi_tests/test_integration.py @@ -1,24 +1,32 @@ -import importlib -import sys +from rollbar.test import BaseTest -try: - from unittest import mock -except ImportError: - import mock -import unittest2 +class IntegrationTest(BaseTest): + def test_should_integrate_if__integrate_defined(self): + from rollbar.contrib.asgi.integration import IntegrationBase -import rollbar -import rollbar.contrib.asgi -from rollbar.test import BaseTest + called = False # cannot patch local objects + + class Integration(IntegrationBase): + def _integrate(self): + nonlocal called + called = True + + self.assertTrue(hasattr(Integration, '_integrate')) + + obj = Integration() + + self.assertTrue(called) + self.assertTrue(hasattr(obj, '_integrate')) + + def test_should_not_fail_if__integrate_not_exists(self): + from rollbar.contrib.asgi.integration import IntegrationBase -ALLOWED_PYTHON_VERSION = sys.version_info >= (3, 5) + class WrongIntegration(IntegrationBase): + ... + self.assertFalse(hasattr(WrongIntegration, '_integrate')) -@unittest2.skipUnless(ALLOWED_PYTHON_VERSION, 'ASGI implementation requires Python3.5+') -class ASGIIntegrationTest(BaseTest): - def setUp(self): - importlib.reload(rollbar.contrib.asgi) + obj = WrongIntegration() - def test_should_set_asgi_hook(self): - self.assertEqual(rollbar.BASE_DATA_HOOK, rollbar.contrib.asgi._hook) + self.assertFalse(hasattr(obj, '_integrate')) diff --git a/rollbar/test/asgi_tests/test_middleware.py b/rollbar/test/asgi_tests/test_middleware.py index 3abb218e..e92103c1 100644 --- a/rollbar/test/asgi_tests/test_middleware.py +++ b/rollbar/test/asgi_tests/test_middleware.py @@ -10,7 +10,6 @@ import unittest2 import rollbar -import rollbar.contrib.asgi from rollbar.lib._async import AsyncMock from rollbar.test import BaseTest @@ -23,9 +22,9 @@ class ReporterMiddlewareTest(BaseTest): default_settings = copy.deepcopy(rollbar.SETTINGS) def setUp(self): + importlib.reload(rollbar) rollbar.SETTINGS = copy.deepcopy(self.default_settings) rollbar.SETTINGS['handler'] = 'async' - importlib.reload(rollbar.contrib.asgi) @mock.patch('rollbar.report_exc_info') def test_should_catch_and_report_errors(self, mock_report): @@ -47,6 +46,22 @@ def test_should_catch_and_report_errors(self, mock_report): self.assertEqual(exc_type, RuntimeError) self.assertIsInstance(exc_value, RuntimeError) + @mock.patch('rollbar._check_config', return_value=True) + @mock.patch('rollbar.send_payload') + def test_should_add_framework_name_to_payload(self, mock_send_payload, *mocks): + import rollbar + from rollbar.contrib.asgi.middleware import ReporterMiddleware + + self.assertIsNone(rollbar.BASE_DATA_HOOK) + + ReporterMiddleware(None) # invoke integration + rollbar.report_exc_info() + + mock_send_payload.assert_called_once() + payload = mock_send_payload.call_args[0][0] + + self.assertIn('asgi', payload['data']['framework']) + @unittest2.skipUnless(ASYNC_REPORT_ENABLED, 'Requires Python 3.6+') @mock.patch('rollbar.lib._async.report_exc_info', new_callable=AsyncMock) @mock.patch('rollbar.report_exc_info') diff --git a/rollbar/test/fastapi_tests/test_integration.py b/rollbar/test/fastapi_tests/test_integration.py deleted file mode 100644 index 01c1cda3..00000000 --- a/rollbar/test/fastapi_tests/test_integration.py +++ /dev/null @@ -1,48 +0,0 @@ -import importlib -import sys - -try: - from unittest import mock -except ImportError: - import mock - -try: - import fastapi - import rollbar.contrib.fastapi - - FASTAPI_INSTALLED = True -except ImportError: - FASTAPI_INSTALLED = False - -import unittest2 - -import rollbar -from rollbar.test import BaseTest - -ALLOWED_PYTHON_VERSION = sys.version_info >= (3, 6) - - -@unittest2.skipUnless( - FASTAPI_INSTALLED and ALLOWED_PYTHON_VERSION, 'FastAPI requires Python3.6+' -) -class FastAPIIntegrationTest(BaseTest): - def setUp(self): - importlib.reload(rollbar.contrib.fastapi) - - def test_should_set_fastapi_hook(self): - import rollbar.contrib.fastapi - - self.assertEqual(rollbar.BASE_DATA_HOOK, rollbar.contrib.fastapi._hook) - - @mock.patch('rollbar._check_config', return_value=True) - @mock.patch('rollbar.send_payload') - def test_should_add_fastapi_version_to_payload(self, mock_send_payload, *mocks): - import fastapi - import rollbar.contrib.fastapi - - rollbar.report_exc_info() - - mock_send_payload.assert_called_once() - payload = mock_send_payload.call_args[0][0] - - self.assertIn(fastapi.__version__, payload['data']['framework']) diff --git a/rollbar/test/fastapi_tests/test_logger.py b/rollbar/test/fastapi_tests/test_logger.py index 2f727500..310ec877 100644 --- a/rollbar/test/fastapi_tests/test_logger.py +++ b/rollbar/test/fastapi_tests/test_logger.py @@ -8,7 +8,6 @@ try: import fastapi - import rollbar.contrib.fastapi FASTAPI_INSTALLED = True except ImportError: @@ -28,7 +27,28 @@ ) class LoggerMiddlewareTest(BaseTest): def setUp(self): - importlib.reload(rollbar.contrib.fastapi) + importlib.reload(rollbar) + + @mock.patch('rollbar._check_config', return_value=True) + @mock.patch('rollbar.send_payload') + def test_should_add_framework_version_to_payload(self, mock_send_payload, *mocks): + import fastapi + from fastapi import FastAPI + import rollbar + from rollbar.contrib.fastapi.logger import LoggerMiddleware + + self.assertIsNone(rollbar.BASE_DATA_HOOK) + + app = FastAPI() + app.add_middleware(LoggerMiddleware) + + rollbar.report_exc_info() + + mock_send_payload.assert_called_once() + payload = mock_send_payload.call_args[0][0] + + self.assertIn('fastapi', payload['data']['framework']) + self.assertIn(fastapi.__version__, payload['data']['framework']) def test_should_support_type_hints(self): from starlette.types import Receive, Scope, Send diff --git a/rollbar/test/fastapi_tests/test_middleware.py b/rollbar/test/fastapi_tests/test_middleware.py index a8fe11dc..5b21c770 100644 --- a/rollbar/test/fastapi_tests/test_middleware.py +++ b/rollbar/test/fastapi_tests/test_middleware.py @@ -9,7 +9,6 @@ try: import fastapi - import rollbar.contrib.fastapi FASTAPI_INSTALLED = True except ImportError: @@ -31,9 +30,9 @@ class ReporterMiddlewareTest(BaseTest): default_settings = copy.deepcopy(rollbar.SETTINGS) def setUp(self): + importlib.reload(rollbar) rollbar.SETTINGS = copy.deepcopy(self.default_settings) rollbar.SETTINGS['handler'] = 'async' - importlib.reload(rollbar.contrib.fastapi) @mock.patch('rollbar.report_exc_info') def test_should_catch_and_report_errors(self, mock_report): @@ -144,6 +143,27 @@ def read_root(path): }, ) + @mock.patch('rollbar._check_config', return_value=True) + @mock.patch('rollbar.send_payload') + def test_should_add_framework_version_to_payload(self, mock_send_payload, *mocks): + import fastapi + from fastapi import FastAPI + import rollbar + from rollbar.contrib.fastapi.middleware import ReporterMiddleware + + self.assertIsNone(rollbar.BASE_DATA_HOOK) + + app = FastAPI() + app.add_middleware(ReporterMiddleware) + + rollbar.report_exc_info() + + mock_send_payload.assert_called_once() + payload = mock_send_payload.call_args[0][0] + + self.assertIn('fastapi', payload['data']['framework']) + self.assertIn(fastapi.__version__, payload['data']['framework']) + @mock.patch('rollbar.lib._async.report_exc_info', new_callable=AsyncMock) @mock.patch('rollbar.report_exc_info') def test_should_use_async_report_exc_info_if_default_handler( diff --git a/rollbar/test/fastapi_tests/test_routing.py b/rollbar/test/fastapi_tests/test_routing.py index f5162f8a..381b0cb7 100644 --- a/rollbar/test/fastapi_tests/test_routing.py +++ b/rollbar/test/fastapi_tests/test_routing.py @@ -10,7 +10,6 @@ try: import fastapi - import rollbar.contrib.fastapi FASTAPI_INSTALLED = True ALLOWED_FASTAPI_VERSION = fastapi.__version__ >= '0.41.0' @@ -73,9 +72,9 @@ class LoggingRouteTest(BaseTest): default_settings = copy.deepcopy(rollbar.SETTINGS) def setUp(self): + importlib.reload(rollbar) rollbar.SETTINGS = copy.deepcopy(self.default_settings) rollbar.SETTINGS['handler'] = 'async' - importlib.reload(rollbar.contrib.fastapi) @mock.patch('rollbar.report_exc_info') def test_should_catch_and_report_errors(self, mock_report): @@ -287,6 +286,27 @@ def read_root(param1: str = Form(...), param2: str = Form(...)): }, ) + @mock.patch('rollbar._check_config', return_value=True) + @mock.patch('rollbar.send_payload') + def test_should_add_framework_version_to_payload(self, mock_send_payload, *mocks): + import fastapi + from fastapi import FastAPI + import rollbar + from rollbar.contrib.fastapi.routing import add_to as rollbar_add_to + + self.assertIsNone(rollbar.BASE_DATA_HOOK) + + app = FastAPI() + rollbar_add_to(app) + + rollbar.report_exc_info() + + mock_send_payload.assert_called_once() + payload = mock_send_payload.call_args[0][0] + + self.assertIn('fastapi', payload['data']['framework']) + self.assertIn(fastapi.__version__, payload['data']['framework']) + @mock.patch('rollbar.lib._async.report_exc_info', new_callable=AsyncMock) @mock.patch('rollbar.report_exc_info') def test_should_use_async_report_exc_info_if_default_handler( diff --git a/rollbar/test/starlette_tests/test_integration.py b/rollbar/test/starlette_tests/test_integration.py deleted file mode 100644 index 3c57f315..00000000 --- a/rollbar/test/starlette_tests/test_integration.py +++ /dev/null @@ -1,45 +0,0 @@ -import importlib -import sys - -try: - from unittest import mock -except ImportError: - import mock - -try: - import starlette - import rollbar.contrib.starlette - - STARLETTE_INSTALLED = True -except ImportError: - STARLETTE_INSTALLED = False - -import unittest2 - -import rollbar -from rollbar.test import BaseTest - -ALLOWED_PYTHON_VERSION = sys.version_info >= (3, 6) - - -@unittest2.skipUnless( - STARLETTE_INSTALLED and ALLOWED_PYTHON_VERSION, 'Starlette requires Python3.6+' -) -class StarletteIntegrationTest(BaseTest): - def setUp(self): - importlib.reload(rollbar.contrib.starlette) - - def test_should_set_starlette_hook(self): - self.assertEqual(rollbar.BASE_DATA_HOOK, rollbar.contrib.starlette._hook) - - @mock.patch('rollbar._check_config', return_value=True) - @mock.patch('rollbar.send_payload') - def test_should_add_starlette_version_to_payload(self, mock_send_payload, *mocks): - import starlette - - rollbar.report_exc_info() - - mock_send_payload.assert_called_once() - payload = mock_send_payload.call_args[0][0] - - self.assertIn(starlette.__version__, payload['data']['framework']) diff --git a/rollbar/test/starlette_tests/test_logger.py b/rollbar/test/starlette_tests/test_logger.py index 4692d053..f2d4e076 100644 --- a/rollbar/test/starlette_tests/test_logger.py +++ b/rollbar/test/starlette_tests/test_logger.py @@ -8,7 +8,6 @@ try: import starlette - import rollbar.contrib.starlette STARLETTE_INSTALLED = True except ImportError: @@ -28,10 +27,32 @@ ) class LoggerMiddlewareTest(BaseTest): def setUp(self): - importlib.reload(rollbar.contrib.starlette) + importlib.reload(rollbar) + + @mock.patch('rollbar._check_config', return_value=True) + @mock.patch('rollbar.send_payload') + def test_should_add_framework_version_to_payload(self, mock_send_payload, *mocks): + import starlette + from starlette.applications import Starlette + import rollbar + from rollbar.contrib.starlette.logger import LoggerMiddleware + + self.assertIsNone(rollbar.BASE_DATA_HOOK) + + app = Starlette() + app.add_middleware(LoggerMiddleware) + + rollbar.report_exc_info() + + mock_send_payload.assert_called_once() + payload = mock_send_payload.call_args[0][0] + + self.assertIn('starlette', payload['data']['framework']) + self.assertIn(starlette.__version__, payload['data']['framework']) def test_should_support_type_hints(self): from starlette.types import Receive, Scope, Send + import rollbar.contrib.starlette.logger self.assertDictEqual( rollbar.contrib.starlette.logger.LoggerMiddleware.__call__.__annotations__, diff --git a/rollbar/test/starlette_tests/test_middleware.py b/rollbar/test/starlette_tests/test_middleware.py index 507a2dcd..88a6f5f4 100644 --- a/rollbar/test/starlette_tests/test_middleware.py +++ b/rollbar/test/starlette_tests/test_middleware.py @@ -9,7 +9,6 @@ try: import starlette - import rollbar.contrib.starlette STARLETTE_INSTALLED = True except ImportError: @@ -31,9 +30,9 @@ class ReporterMiddlewareTest(BaseTest): default_settings = copy.deepcopy(rollbar.SETTINGS) def setUp(self): + importlib.reload(rollbar) rollbar.SETTINGS = copy.deepcopy(self.default_settings) rollbar.SETTINGS['handler'] = 'async' - importlib.reload(rollbar.contrib.starlette) @mock.patch('rollbar.report_exc_info') def test_should_catch_and_report_errors(self, mock_report): @@ -130,6 +129,27 @@ async def root(request): }, ) + @mock.patch('rollbar._check_config', return_value=True) + @mock.patch('rollbar.send_payload') + def test_should_add_framework_version_to_payload(self, mock_send_payload, *mocks): + import starlette + from starlette.applications import Starlette + import rollbar + from rollbar.contrib.starlette.middleware import ReporterMiddleware + + self.assertIsNone(rollbar.BASE_DATA_HOOK) + + app = Starlette() + app.add_middleware(ReporterMiddleware) + + rollbar.report_exc_info() + + mock_send_payload.assert_called_once() + payload = mock_send_payload.call_args[0][0] + + self.assertIn('starlette', payload['data']['framework']) + self.assertIn(starlette.__version__, payload['data']['framework']) + @mock.patch('rollbar.lib._async.report_exc_info', new_callable=AsyncMock) @mock.patch('rollbar.report_exc_info') def test_should_use_async_report_exc_info_if_default_handler(