From d8523bd83b778d75ed534dc619781517a4f7ec6b Mon Sep 17 00:00:00 2001 From: Evgeniy Alekseev Date: Wed, 20 Oct 2021 02:12:39 +0300 Subject: [PATCH] always return json in responses --- .../web/middlewares/exception_handler.py | 17 +++++--- src/ahriman/web/views/service/add.py | 7 ++- src/ahriman/web/views/service/remove.py | 7 ++- src/ahriman/web/views/service/request.py | 7 ++- src/ahriman/web/views/service/search.py | 4 +- src/ahriman/web/views/status/ahriman.py | 7 ++- src/ahriman/web/views/status/package.py | 6 +-- .../web/middlewares/test_exception_handler.py | 43 +++++++++++++++---- 8 files changed, 62 insertions(+), 36 deletions(-) diff --git a/src/ahriman/web/middlewares/exception_handler.py b/src/ahriman/web/middlewares/exception_handler.py index 342c2fc7..e7a6b3bf 100644 --- a/src/ahriman/web/middlewares/exception_handler.py +++ b/src/ahriman/web/middlewares/exception_handler.py @@ -18,8 +18,8 @@ # along with this program. If not, see . # from aiohttp.web import middleware, Request -from aiohttp.web_exceptions import HTTPException -from aiohttp.web_response import StreamResponse +from aiohttp.web_exceptions import HTTPClientError, HTTPException, HTTPServerError +from aiohttp.web_response import json_response, StreamResponse from logging import Logger from ahriman.web.middlewares import HandlerType, MiddlewareType @@ -35,10 +35,15 @@ def exception_handler(logger: Logger) -> MiddlewareType: async def handle(request: Request, handler: HandlerType) -> StreamResponse: try: return await handler(request) + except HTTPClientError as e: + return json_response(data={"error": e.reason}, status=e.status_code) + except HTTPServerError as e: + logger.exception("server exception during performing request to %s", request.path) + return json_response(data={"error": e.reason}, status=e.status_code) except HTTPException: - raise # we do not raise 5xx exceptions actually so it should be fine - except Exception: - logger.exception("exception during performing request to %s", request.path) - raise + raise # just raise 2xx and 3xx codes + except Exception as e: + logger.exception("unknown exception during performing request to %s", request.path) + return json_response(data={"error": str(e)}, status=500) return handle diff --git a/src/ahriman/web/views/service/add.py b/src/ahriman/web/views/service/add.py index 63a52426..83168c44 100644 --- a/src/ahriman/web/views/service/add.py +++ b/src/ahriman/web/views/service/add.py @@ -17,7 +17,7 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see . # -from aiohttp.web import HTTPFound, Response, json_response +from aiohttp.web import HTTPBadRequest, HTTPFound, Response from ahriman.models.user_access import UserAccess from ahriman.web.views.base import BaseView @@ -42,12 +42,11 @@ class AddView(BaseView): :return: redirect to main page on success """ - data = await self.extract_data(["packages"]) - try: + data = await self.extract_data(["packages"]) packages = data["packages"] except Exception as e: - return json_response(data=str(e), status=400) + raise HTTPBadRequest(reason=str(e)) self.spawner.packages_add(packages, now=True) diff --git a/src/ahriman/web/views/service/remove.py b/src/ahriman/web/views/service/remove.py index cbbf2ed6..19bb3b00 100644 --- a/src/ahriman/web/views/service/remove.py +++ b/src/ahriman/web/views/service/remove.py @@ -17,7 +17,7 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see . # -from aiohttp.web import HTTPFound, Response, json_response +from aiohttp.web import HTTPBadRequest, HTTPFound, Response from ahriman.models.user_access import UserAccess from ahriman.web.views.base import BaseView @@ -42,12 +42,11 @@ class RemoveView(BaseView): :return: redirect to main page on success """ - data = await self.extract_data(["packages"]) - try: + data = await self.extract_data(["packages"]) packages = data["packages"] except Exception as e: - return json_response(data=str(e), status=400) + raise HTTPBadRequest(reason=str(e)) self.spawner.packages_remove(packages) diff --git a/src/ahriman/web/views/service/request.py b/src/ahriman/web/views/service/request.py index 611ca7fa..340e9f73 100644 --- a/src/ahriman/web/views/service/request.py +++ b/src/ahriman/web/views/service/request.py @@ -17,7 +17,7 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see . # -from aiohttp.web import HTTPFound, Response, json_response +from aiohttp.web import HTTPBadRequest, HTTPFound, Response from ahriman.models.user_access import UserAccess from ahriman.web.views.base import BaseView @@ -42,12 +42,11 @@ class RequestView(BaseView): :return: redirect to main page on success """ - data = await self.extract_data(["packages"]) - try: + data = await self.extract_data(["packages"]) packages = data["packages"] except Exception as e: - return json_response(data=str(e), status=400) + raise HTTPBadRequest(reason=str(e)) self.spawner.packages_add(packages, now=False) diff --git a/src/ahriman/web/views/service/search.py b/src/ahriman/web/views/service/search.py index d53c84e6..863f021d 100644 --- a/src/ahriman/web/views/service/search.py +++ b/src/ahriman/web/views/service/search.py @@ -19,7 +19,7 @@ # import aur # type: ignore -from aiohttp.web import Response, json_response +from aiohttp.web import HTTPBadRequest, Response, json_response from typing import Callable, Iterator from ahriman.models.user_access import UserAccess @@ -47,7 +47,7 @@ class SearchView(BaseView): search_string = " ".join(search) if not search_string: - return json_response(data="Search string must not be empty", status=400) + raise HTTPBadRequest(reason="Search string must not be empty") packages = aur.search(search_string) comparator: Callable[[aur.Package], str] = lambda item: str(item.package_base) diff --git a/src/ahriman/web/views/status/ahriman.py b/src/ahriman/web/views/status/ahriman.py index 40e0d101..f9963850 100644 --- a/src/ahriman/web/views/status/ahriman.py +++ b/src/ahriman/web/views/status/ahriman.py @@ -17,7 +17,7 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see . # -from aiohttp.web import HTTPNoContent, Response, json_response +from aiohttp.web import HTTPBadRequest, HTTPNoContent, Response, json_response from ahriman.models.build_status import BuildStatusEnum from ahriman.models.user_access import UserAccess @@ -53,12 +53,11 @@ class AhrimanView(BaseView): :return: 204 on success """ - data = await self.extract_data() - try: + data = await self.extract_data() status = BuildStatusEnum(data["status"]) except Exception as e: - return json_response(data=str(e), status=400) + raise HTTPBadRequest(reason=str(e)) self.service.update_self(status) diff --git a/src/ahriman/web/views/status/package.py b/src/ahriman/web/views/status/package.py index ad814481..1afcb757 100644 --- a/src/ahriman/web/views/status/package.py +++ b/src/ahriman/web/views/status/package.py @@ -17,7 +17,7 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see . # -from aiohttp.web import HTTPNoContent, HTTPNotFound, Response, json_response +from aiohttp.web import HTTPBadRequest, HTTPNoContent, HTTPNotFound, Response, json_response from ahriman.core.exceptions import UnknownPackage from ahriman.models.build_status import BuildStatusEnum @@ -88,11 +88,11 @@ class PackageView(BaseView): package = Package.from_json(data["package"]) if "package" in data else None status = BuildStatusEnum(data["status"]) except Exception as e: - return json_response(data=str(e), status=400) + raise HTTPBadRequest(reason=str(e)) try: self.service.update(base, status, package) except UnknownPackage: - return json_response(data=f"Package {base} is unknown, but no package body set", status=400) + raise HTTPBadRequest(reason=f"Package {base} is unknown, but no package body set") raise HTTPNoContent() diff --git a/tests/ahriman/web/middlewares/test_exception_handler.py b/tests/ahriman/web/middlewares/test_exception_handler.py index ea281093..feb36966 100644 --- a/tests/ahriman/web/middlewares/test_exception_handler.py +++ b/tests/ahriman/web/middlewares/test_exception_handler.py @@ -1,13 +1,24 @@ +import json import logging import pytest -from aiohttp.web_exceptions import HTTPBadRequest, HTTPNoContent +from aiohttp.web_exceptions import HTTPBadRequest, HTTPInternalServerError, HTTPNoContent from pytest_mock import MockerFixture +from typing import Any from unittest.mock import AsyncMock from ahriman.web.middlewares.exception_handler import exception_handler +def _extract_body(response: Any) -> Any: + """ + extract json body from given object + :param response: response (any actually) object + :return: body key from the object converted to json + """ + return json.loads(getattr(response, "body")) + + async def test_exception_handler(mocker: MockerFixture) -> None: """ must pass success response @@ -23,7 +34,7 @@ async def test_exception_handler(mocker: MockerFixture) -> None: async def test_exception_handler_success(mocker: MockerFixture) -> None: """ - must pass client exception + must pass 2xx and 3xx codes """ request = pytest.helpers.request("", "", "") request_handler = AsyncMock(side_effect=HTTPNoContent()) @@ -37,27 +48,41 @@ async def test_exception_handler_success(mocker: MockerFixture) -> None: async def test_exception_handler_client_error(mocker: MockerFixture) -> None: """ - must pass client exception + must handle client exception """ request = pytest.helpers.request("", "", "") request_handler = AsyncMock(side_effect=HTTPBadRequest()) logging_mock = mocker.patch("logging.Logger.exception") handler = exception_handler(logging.getLogger()) - with pytest.raises(HTTPBadRequest): - await handler(request, request_handler) + response = await handler(request, request_handler) + assert _extract_body(response) == {"error": HTTPBadRequest().reason} logging_mock.assert_not_called() async def test_exception_handler_server_error(mocker: MockerFixture) -> None: """ - must log server exception and re-raise it + must handle server exception """ request = pytest.helpers.request("", "", "") - request_handler = AsyncMock(side_effect=Exception()) + request_handler = AsyncMock(side_effect=HTTPInternalServerError()) logging_mock = mocker.patch("logging.Logger.exception") handler = exception_handler(logging.getLogger()) - with pytest.raises(Exception): - await handler(request, request_handler) + response = await handler(request, request_handler) + assert _extract_body(response) == {"error": HTTPInternalServerError().reason} + logging_mock.assert_called_once() + + +async def test_exception_handler_unknown_error(mocker: MockerFixture) -> None: + """ + must log server exception and re-raise it + """ + request = pytest.helpers.request("", "", "") + request_handler = AsyncMock(side_effect=Exception("An error")) + logging_mock = mocker.patch("logging.Logger.exception") + + handler = exception_handler(logging.getLogger()) + response = await handler(request, request_handler) + assert _extract_body(response) == {"error": "An error"} logging_mock.assert_called_once()