From d299efe31ec3487d0152ff4a892826d45f88ba72 Mon Sep 17 00:00:00 2001 From: Arijit Basu Date: Tue, 23 Jul 2024 13:15:04 +0530 Subject: [PATCH 1/6] Fix Honeybadger error reporting Closes: https://github.com/stckme/apphelpers/issues/107 --- CONTRIBUTING.rst | 2 ++ apphelpers/rest/common.py | 34 ++++++++++++++++------- apphelpers/rest/fastapi.py | 54 ++++++++++++++++++++++++------------ apphelpers/rest/hug.py | 16 +---------- default_settings.py | 3 ++ docker-compose.yml | 15 ++++++++++ fastapi_tests/test_rest.py | 57 +++++++++++++++++++++++++++++++++++++- requirements_dev.txt | 1 + tests/app/endpoints.py | 3 -- tests/test_rest.py | 53 +++++++++++++++++++++++++++++++++++ 10 files changed, 192 insertions(+), 46 deletions(-) create mode 100644 docker-compose.yml diff --git a/CONTRIBUTING.rst b/CONTRIBUTING.rst index 1443c8d..6c13b9b 100644 --- a/CONTRIBUTING.rst +++ b/CONTRIBUTING.rst @@ -82,6 +82,7 @@ Ready to contribute? Here's how to set up `apphelpers` for local development. $ export SETTINGS_DIR=. + $ docker-compose up -d # start postgres and redis $ gunicorn tests.service:__hug_wsgi__ $ pytest tests @@ -90,6 +91,7 @@ Ready to contribute? Here's how to set up `apphelpers` for local development. $ export SETTINGS_DIR=. + $ docker-compose up -d # start postgres and redis $ uvicorn fastapi_tests.service:app --host 0.0.0.0 --port 5000 $ pytest fastapi_tests diff --git a/apphelpers/rest/common.py b/apphelpers/rest/common.py index 2d2825c..1ecc3f8 100644 --- a/apphelpers/rest/common.py +++ b/apphelpers/rest/common.py @@ -1,15 +1,13 @@ from __future__ import annotations -from dataclasses import ( - asdict, - dataclass, - field, -) -from typing import ( - Dict, - List, - Optional, -) +from dataclasses import asdict, dataclass, field +from typing import Dict, List, Optional + +from converge import settings +from requests.exceptions import HTTPError + +if settings.get("HONEYBADGER_API_KEY"): + from honeybadger.utils import filter_dict def phony(f): @@ -32,3 +30,19 @@ def to_dict(self): def __bool__(self): return self.id is not None + + +def notify_honeybadger(honeybadger, error, func, args, kwargs): + try: + honeybadger.notify( + error, + context={ + "func": func.__name__, + "args": args, + "kwargs": filter_dict(kwargs, settings.HB_PARAM_FILTERS), + }, + ) + except HTTPError as e: + if e.response.status_code != 403: + # Ignore 403 Forbidden errors. We get alerted by HB anyway. + raise e diff --git a/apphelpers/rest/fastapi.py b/apphelpers/rest/fastapi.py index 4fb7fb4..e948e97 100644 --- a/apphelpers/rest/fastapi.py +++ b/apphelpers/rest/fastapi.py @@ -8,18 +8,18 @@ from apphelpers.db import dbtransaction_ctx from apphelpers.errors.fastapi import ( + BaseError, HTTP401Unauthorized, HTTP403Forbidden, HTTP404NotFound, InvalidSessionError, ) from apphelpers.rest import endpoint as ep -from apphelpers.rest.common import User, phony +from apphelpers.rest.common import User, notify_honeybadger, phony from apphelpers.sessions import SessionDBHandler if settings.get("HONEYBADGER_API_KEY"): from honeybadger import Honeybadger - from honeybadger.utils import filter_dict def raise_not_found_on_none(f): @@ -54,25 +54,45 @@ def honeybadger_wrapper(hb): """ def wrapper(f): - @wraps(f) - def f_wrapped(*args, **kw): - try: - ret = f(*args, **kw) - except Exception as e: + if inspect.iscoroutinefunction(f): + + @wraps(f) + async def async_f_wrapped(*args, **kw): try: - hb.notify( - e, - context={ - "func": f.__name__, - "args": args, - "kwargs": filter_dict(kw, settings.HB_PARAM_FILTERS), - }, + return await f(*args, **kw) + except BaseError as e: + if e.report: + notify_honeybadger( + honeybadger=hb, error=e, func=f, args=args, kwargs=kw + ) + raise e + except Exception as e: + notify_honeybadger( + honeybadger=hb, error=e, func=f, args=args, kwargs=kw + ) + raise e + + return async_f_wrapped + + else: + + @wraps(f) + def f_wrapped(*args, **kw): + try: + return f(*args, **kw) + except BaseError as e: + if e.report: + notify_honeybadger( + honeybadger=hb, error=e, func=f, args=args, kwargs=kw + ) + raise e + except Exception as e: + notify_honeybadger( + honeybadger=hb, error=e, func=f, args=args, kwargs=kw ) - finally: raise e - return ret - return f_wrapped + return f_wrapped return wrapper diff --git a/apphelpers/rest/hug.py b/apphelpers/rest/hug.py index 6385801..d7ef5f2 100644 --- a/apphelpers/rest/hug.py +++ b/apphelpers/rest/hug.py @@ -11,11 +11,11 @@ from apphelpers.errors.hug import BaseError, InvalidSessionError from apphelpers.loggers import api_logger from apphelpers.rest import endpoint as ep +from apphelpers.rest.common import notify_honeybadger from apphelpers.sessions import SessionDBHandler if settings.get("HONEYBADGER_API_KEY"): from honeybadger import Honeybadger - from honeybadger.utils import filter_dict def phony(f): @@ -36,20 +36,6 @@ def wrapper(*ar, **kw): return f -def notify_honeybadger(honeybadger, error, func, args, kwargs): - try: - honeybadger.notify( - error, - context={ - "func": func.__name__, - "args": args, - "kwargs": filter_dict(kwargs, settings.HB_PARAM_FILTERS), - }, - ) - finally: - pass - - def honeybadger_wrapper(hb): """ wrapper that executes the function in a try/except diff --git a/default_settings.py b/default_settings.py index 0d3074f..0481187 100644 --- a/default_settings.py +++ b/default_settings.py @@ -15,6 +15,9 @@ SMTP_USERNAME = None SMTP_KEY = "" +HONEYBADGER_API_KEY = "secret" +HB_PARAM_FILTERS = ["password", "passwd", "secret"] + class API_LOGGER: ENABLED = False diff --git a/docker-compose.yml b/docker-compose.yml new file mode 100644 index 0000000..a664f01 --- /dev/null +++ b/docker-compose.yml @@ -0,0 +1,15 @@ +version: "3.1" +services: + postgres: + image: postgres + ports: + - 5432:5432 + environment: + POSTGRES_DB: defaultdb + POSTGRES_USER: postgres + POSTGRES_PASSWORD: postgres + + redis: + image: redis + ports: + - 6379:6379 diff --git a/fastapi_tests/test_rest.py b/fastapi_tests/test_rest.py index 1f694a8..a5eeda9 100644 --- a/fastapi_tests/test_rest.py +++ b/fastapi_tests/test_rest.py @@ -1,8 +1,14 @@ +import asyncio +from unittest import mock + +import pytest import requests from converge import settings import apphelpers.sessions as sessionslib -from apphelpers.db.piccolo import setup_db_from_basetable, destroy_db_from_basetable +from apphelpers.db.piccolo import destroy_db_from_basetable, setup_db_from_basetable +from apphelpers.errors.fastapi import BaseError +from apphelpers.rest.fastapi import honeybadger_wrapper from fastapi_tests.app.models import BaseTable base_url = "http://127.0.0.1:5000/" @@ -312,3 +318,52 @@ def test_piccolo(): url = base_url + "count-books" assert requests.get(url).json() == 3 + + +def test_honeybadger_wrapper(): + + mocked_honeybadger = mock.MagicMock() + wrapper = honeybadger_wrapper(mocked_honeybadger) + + def good_endpoint(foo): + return foo + + class IgnorableError(BaseError): + report = False + + async def bad_endpoint(foo): + raise IgnorableError() + + async def worse_endpoint(foo, password): + raise BaseError() + + async def worst_endpoint(foo): + raise RuntimeError() + + wrapped_good_endpoint = wrapper(good_endpoint) + wrapped_bad_endpoint = wrapper(bad_endpoint) + wrapped_worse_endpoint = wrapper(worse_endpoint) + wrapped_worst_endpoint = wrapper(worst_endpoint) + + assert wrapped_good_endpoint(1) == 1 + assert not mocked_honeybadger.notify.called + + with pytest.raises(IgnorableError): + asyncio.run(wrapped_bad_endpoint(1)) + assert not mocked_honeybadger.notify.called + + with pytest.raises(BaseError) as e: + asyncio.run(wrapped_worse_endpoint(1, password="secret")) + mocked_honeybadger.notify.assert_called_once_with( + e.value, + context={ + "func": "worse_endpoint", + "args": (1,), + "kwargs": {"password": "[FILTERED]"}, + }, + ) + assert mocked_honeybadger.notify.call_count == 1 + + with pytest.raises(RuntimeError): + asyncio.run(wrapped_worst_endpoint(1)) + assert mocked_honeybadger.notify.call_count == 2 diff --git a/requirements_dev.txt b/requirements_dev.txt index 6817075..ba38a4a 100644 --- a/requirements_dev.txt +++ b/requirements_dev.txt @@ -30,3 +30,4 @@ types-redis redis loguru piccolo[postgres] +honeybadger diff --git a/tests/app/endpoints.py b/tests/app/endpoints.py index 161f4af..46deabe 100644 --- a/tests/app/endpoints.py +++ b/tests/app/endpoints.py @@ -1,8 +1,5 @@ -from typing import Dict, Optional - import hug import hug.directives -from pydantic import BaseModel from apphelpers.rest import endpoint as ep from apphelpers.rest.hug import user_id diff --git a/tests/test_rest.py b/tests/test_rest.py index 07a7a2f..0039942 100644 --- a/tests/test_rest.py +++ b/tests/test_rest.py @@ -1,10 +1,14 @@ import os import time +from unittest import mock +import pytest import requests from converge import settings import apphelpers.sessions as sessionslib +from apphelpers.errors.hug import BaseError +from apphelpers.rest.hug import honeybadger_wrapper from .app.models import globalgroups, sitegroups @@ -311,3 +315,52 @@ def test_custom_authorization_access(): url = urls.echo_for_custom_authorization + "/unauthorized" assert requests.get(url, headers=headers).status_code == 403 + + +def test_honeybadger_wrapper(): + + mocked_honeybadger = mock.MagicMock() + wrapper = honeybadger_wrapper(mocked_honeybadger) + + def good_endpoint(foo): + return foo + + class IgnorableError(BaseError): + report = False + + def bad_endpoint(foo): + raise IgnorableError() + + def worse_endpoint(foo, password): + raise BaseError() + + def worst_endpoint(foo): + raise RuntimeError() + + wrapped_good_endpoint = wrapper(good_endpoint) + wrapped_bad_endpoint = wrapper(bad_endpoint) + wrapped_worse_endpoint = wrapper(worse_endpoint) + wrapped_worst_endpoint = wrapper(worst_endpoint) + + assert wrapped_good_endpoint(1) == 1 + assert not mocked_honeybadger.notify.called + + with pytest.raises(IgnorableError): + wrapped_bad_endpoint(1) + assert not mocked_honeybadger.notify.called + + with pytest.raises(BaseError) as e: + wrapped_worse_endpoint(1, password="secret") + mocked_honeybadger.notify.assert_called_once_with( + e.value, + context={ + "func": "worse_endpoint", + "args": (1,), + "kwargs": {"password": "[FILTERED]"}, + }, + ) + assert mocked_honeybadger.notify.call_count == 1 + + with pytest.raises(RuntimeError): + wrapped_worst_endpoint(1) + assert mocked_honeybadger.notify.call_count == 2 From 28f24f89a3b71bc5fcd58a6b4cecae7559afe2a1 Mon Sep 17 00:00:00 2001 From: Arijit Basu Date: Thu, 25 Jul 2024 16:02:00 +0530 Subject: [PATCH 2/6] Call notify_honeybadger in finally block --- apphelpers/rest/common.py | 4 +++- apphelpers/rest/fastapi.py | 37 +++++++++++++++++++++++++------------ apphelpers/rest/hug.py | 19 ++++++++++++------- fastapi_tests/test_rest.py | 8 ++++++++ tests/test_rest.py | 8 ++++++++ 5 files changed, 56 insertions(+), 20 deletions(-) diff --git a/apphelpers/rest/common.py b/apphelpers/rest/common.py index 1ecc3f8..d4a4470 100644 --- a/apphelpers/rest/common.py +++ b/apphelpers/rest/common.py @@ -43,6 +43,8 @@ def notify_honeybadger(honeybadger, error, func, args, kwargs): }, ) except HTTPError as e: - if e.response.status_code != 403: + if e.response.status_code == 403: # Ignore 403 Forbidden errors. We get alerted by HB anyway. + pass + else: raise e diff --git a/apphelpers/rest/fastapi.py b/apphelpers/rest/fastapi.py index e948e97..c17339c 100644 --- a/apphelpers/rest/fastapi.py +++ b/apphelpers/rest/fastapi.py @@ -58,19 +58,25 @@ def wrapper(f): @wraps(f) async def async_f_wrapped(*args, **kw): + err_to_report = None try: return await f(*args, **kw) except BaseError as e: if e.report: - notify_honeybadger( - honeybadger=hb, error=e, func=f, args=args, kwargs=kw - ) + err_to_report = e raise e except Exception as e: - notify_honeybadger( - honeybadger=hb, error=e, func=f, args=args, kwargs=kw - ) + err_to_report = e raise e + finally: + if err_to_report: + notify_honeybadger( + honeybadger=hb, + error=err_to_report, + func=f, + args=args, + kwargs=kw, + ) return async_f_wrapped @@ -78,20 +84,27 @@ async def async_f_wrapped(*args, **kw): @wraps(f) def f_wrapped(*args, **kw): + err_to_report = None try: return f(*args, **kw) except BaseError as e: if e.report: - notify_honeybadger( - honeybadger=hb, error=e, func=f, args=args, kwargs=kw - ) + err_to_report = e raise e except Exception as e: - notify_honeybadger( - honeybadger=hb, error=e, func=f, args=args, kwargs=kw - ) + err_to_report = e raise e + finally: + if err_to_report: + notify_honeybadger( + honeybadger=hb, + error=err_to_report, + func=f, + args=args, + kwargs=kw, + ) + return f_wrapped return wrapper diff --git a/apphelpers/rest/hug.py b/apphelpers/rest/hug.py index d7ef5f2..818c03c 100644 --- a/apphelpers/rest/hug.py +++ b/apphelpers/rest/hug.py @@ -45,20 +45,25 @@ def honeybadger_wrapper(hb): def wrapper(f): @wraps(f) def f_wrapped(*args, **kw): + err_to_report = None try: return f(*args, **kw) except BaseError as e: if e.report: - notify_honeybadger( - honeybadger=hb, error=e, func=f, args=args, kwargs=kw - ) + err_to_report = e raise e - except Exception as e: - notify_honeybadger( - honeybadger=hb, error=e, func=f, args=args, kwargs=kw - ) + err_to_report = e raise e + finally: + if err_to_report: + notify_honeybadger( + honeybadger=hb, + error=err_to_report, + func=f, + args=args, + kwargs=kw, + ) return f_wrapped diff --git a/fastapi_tests/test_rest.py b/fastapi_tests/test_rest.py index a5eeda9..d170f92 100644 --- a/fastapi_tests/test_rest.py +++ b/fastapi_tests/test_rest.py @@ -367,3 +367,11 @@ async def worst_endpoint(foo): with pytest.raises(RuntimeError): asyncio.run(wrapped_worst_endpoint(1)) assert mocked_honeybadger.notify.call_count == 2 + + mocked_honeybadger.notify.side_effect = requests.exceptions.HTTPError( + response=mock.MagicMock(status_code=403) + ) + with pytest.raises(RuntimeError) as e: + asyncio.run(wrapped_worst_endpoint(1)) + assert "HttpError" in str(e) + assert mocked_honeybadger.notify.call_count == 3 diff --git a/tests/test_rest.py b/tests/test_rest.py index 0039942..f4cf947 100644 --- a/tests/test_rest.py +++ b/tests/test_rest.py @@ -364,3 +364,11 @@ def worst_endpoint(foo): with pytest.raises(RuntimeError): wrapped_worst_endpoint(1) assert mocked_honeybadger.notify.call_count == 2 + + mocked_honeybadger.notify.side_effect = requests.exceptions.HTTPError( + response=mock.MagicMock(status_code=403) + ) + with pytest.raises(RuntimeError) as e: + wrapped_worst_endpoint(1) + assert "HttpError" in str(e) + assert mocked_honeybadger.notify.call_count == 3 From 6badce8e30c1fe9b61bbbd654e21a1cfd600ba19 Mon Sep 17 00:00:00 2001 From: Arijit Basu Date: Thu, 25 Jul 2024 16:51:30 +0530 Subject: [PATCH 3/6] Tests --- fastapi_tests/test_rest.py | 15 ++++++++++++--- tests/test_rest.py | 16 +++++++++++++--- 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/fastapi_tests/test_rest.py b/fastapi_tests/test_rest.py index d170f92..866f443 100644 --- a/fastapi_tests/test_rest.py +++ b/fastapi_tests/test_rest.py @@ -4,6 +4,7 @@ import pytest import requests from converge import settings +from requests.exceptions import HTTPError import apphelpers.sessions as sessionslib from apphelpers.db.piccolo import destroy_db_from_basetable, setup_db_from_basetable @@ -368,10 +369,18 @@ async def worst_endpoint(foo): asyncio.run(wrapped_worst_endpoint(1)) assert mocked_honeybadger.notify.call_count == 2 - mocked_honeybadger.notify.side_effect = requests.exceptions.HTTPError( + mocked_honeybadger.notify.side_effect = HTTPError( response=mock.MagicMock(status_code=403) ) - with pytest.raises(RuntimeError) as e: + with pytest.raises(RuntimeError): asyncio.run(wrapped_worst_endpoint(1)) - assert "HttpError" in str(e) + # TODO: How to check nested exception? assert mocked_honeybadger.notify.call_count == 3 + + mocked_honeybadger.notify.side_effect = HTTPError( + response=mock.MagicMock(status_code=401) + ) + with pytest.raises(HTTPError): + asyncio.run(wrapped_worst_endpoint(1)) + # TODO: How to check nested exception? + assert mocked_honeybadger.notify.call_count == 4 diff --git a/tests/test_rest.py b/tests/test_rest.py index f4cf947..9f5fa53 100644 --- a/tests/test_rest.py +++ b/tests/test_rest.py @@ -5,6 +5,8 @@ import pytest import requests from converge import settings +from exceptiongroup import ExceptionGroup +from requests.exceptions import HTTPError import apphelpers.sessions as sessionslib from apphelpers.errors.hug import BaseError @@ -365,10 +367,18 @@ def worst_endpoint(foo): wrapped_worst_endpoint(1) assert mocked_honeybadger.notify.call_count == 2 - mocked_honeybadger.notify.side_effect = requests.exceptions.HTTPError( + mocked_honeybadger.notify.side_effect = HTTPError( response=mock.MagicMock(status_code=403) ) - with pytest.raises(RuntimeError) as e: + with pytest.raises(RuntimeError): wrapped_worst_endpoint(1) - assert "HttpError" in str(e) + # TODO: How to check nested exception? assert mocked_honeybadger.notify.call_count == 3 + + mocked_honeybadger.notify.side_effect = HTTPError( + response=mock.MagicMock(status_code=401) + ) + with pytest.raises(HTTPError): + wrapped_worst_endpoint(1) + # TODO: How to check nested exception? + assert mocked_honeybadger.notify.call_count == 4 From 89436c33a84ffce327b47df3528a8544ca0be9ad Mon Sep 17 00:00:00 2001 From: Arijit Basu Date: Thu, 25 Jul 2024 16:58:49 +0530 Subject: [PATCH 4/6] Minor --- tests/test_rest.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/test_rest.py b/tests/test_rest.py index 9f5fa53..30ebb81 100644 --- a/tests/test_rest.py +++ b/tests/test_rest.py @@ -5,7 +5,6 @@ import pytest import requests from converge import settings -from exceptiongroup import ExceptionGroup from requests.exceptions import HTTPError import apphelpers.sessions as sessionslib From 0d811c83f8c5613ba2f77dbcbc49574ee7e4ef99 Mon Sep 17 00:00:00 2001 From: Arijit Basu Date: Thu, 25 Jul 2024 17:06:31 +0530 Subject: [PATCH 5/6] Update changelog --- CHANGELOG.rst | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index a9d6d67..b6d2a4b 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -2,6 +2,12 @@ History ======= +0.96.0 (2024-07-25) +------------------- +* Possible fix for Honeybadger exception masking the actual exceptions. +* Honeybadger 403 errors will not be raised anymore. +* Improved FastAPI honeybadger integration. + 0.95.1 (2024-07-02) ------------------- * Added socialauth.goog.fetch_info_using_jwt for fetching user info using Google JWT From 252f8228ba467b55e2730dc5faa16400431ba846 Mon Sep 17 00:00:00 2001 From: Arijit Basu Date: Thu, 25 Jul 2024 17:06:37 +0530 Subject: [PATCH 6/6] =?UTF-8?q?Bump=20version:=200.95.1=20=E2=86=92=200.96?= =?UTF-8?q?.0?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- apphelpers/__init__.py | 2 +- setup.cfg | 2 +- setup.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/apphelpers/__init__.py b/apphelpers/__init__.py index a2a81be..5445763 100644 --- a/apphelpers/__init__.py +++ b/apphelpers/__init__.py @@ -4,4 +4,4 @@ __author__ = """Scroll Tech""" __email__ = "engg@stck.me" -__version__ = "0.95.1" +__version__ = "0.96.0" diff --git a/setup.cfg b/setup.cfg index 64e6e34..6fae66a 100644 --- a/setup.cfg +++ b/setup.cfg @@ -1,5 +1,5 @@ [bumpversion] -current_version = 0.95.1 +current_version = 0.96.0 commit = True tag = True diff --git a/setup.py b/setup.py index 7fcea94..2ae9e61 100644 --- a/setup.py +++ b/setup.py @@ -42,6 +42,6 @@ test_suite="tests", tests_require=test_requirements, url="https://github.com/scrolltech/apphelpers", - version="0.95.1", + version="0.96.0", zip_safe=False, )