♻️ aiohttp deprecation: Using web.json_response to return 2XX responses instead of raising HttpException (#6563)

Author: pcrespov

packages/pytest-simcore/src/pytest_simcore/helpers/assert_checks.py

@@ -7,14 +7,15 @@
 from aiohttp import ClientResponse
 from servicelib.aiohttp import status
 from servicelib.aiohttp.rest_responses import unwrap_envelope
-from servicelib.status_utils import get_code_display_name, is_error
+from servicelib.status_codes_utils import get_code_display_name, is_error
 
 
 async def assert_status(
     response: ClientResponse,
     expected_status_code: int,
     expected_msg: str | None = None,
     expected_error_code: str | None = None,
+    *,
     include_meta: bool | None = False,
     include_links: bool | None = False,
 ) -> tuple[dict, ...]:

packages/pytest-simcore/src/pytest_simcore/helpers/webserver_parametrizations.py

@@ -2,7 +2,7 @@
 from unittest import mock
 
 from servicelib.aiohttp import status
-from servicelib.status_utils import get_code_display_name
+from servicelib.status_codes_utils import get_code_display_name
 from simcore_postgres_database.models.users import UserRole
 
 

packages/service-library/src/servicelib/aiohttp/long_running_tasks/_routes.py

@@ -4,11 +4,11 @@
 from aiohttp import web
 from models_library.utils.json_serialization import json_dumps
 from pydantic import BaseModel
+from servicelib.aiohttp import status
 
 from ...long_running_tasks._errors import TaskNotCompletedError, TaskNotFoundError
 from ...long_running_tasks._models import TaskGet, TaskId, TaskStatus
 from ...long_running_tasks._task import TrackedTask
-from ...mimetype_constants import MIMETYPE_APPLICATION_JSON
 from ..requests_validation import parse_request_path_parameters_as
 from ._dependencies import get_task_context, get_tasks_manager
 
@@ -89,7 +89,7 @@ async def cancel_and_delete_task(request: web.Request) -> web.Response:
     tasks_manager = get_tasks_manager(request.app)
     task_context = get_task_context(request)
     await tasks_manager.remove_task(path_params.task_id, with_task_context=task_context)
-    raise web.HTTPNoContent(content_type=MIMETYPE_APPLICATION_JSON)
+    return web.json_response(status=status.HTTP_204_NO_CONTENT)
 
 
 __all__: tuple[str, ...] = (

packages/service-library/src/servicelib/aiohttp/rest_responses.py

@@ -13,7 +13,7 @@
 from servicelib.aiohttp.status import HTTP_200_OK
 
 from ..mimetype_constants import MIMETYPE_APPLICATION_JSON
-from ..status_utils import get_code_description
+from ..status_codes_utils import get_code_description
 from .rest_models import ErrorItemType, ErrorType
 
 _ENVELOPE_KEYS = ("data", "error")
@@ -151,16 +151,3 @@ def _pred(obj) -> bool:
     assert len(http_statuses) == len(found), "No duplicates"  # nosec
 
     return http_statuses
-
-
-_STATUS_CODE_TO_HTTP_ERRORS: dict[int, type[HTTPError]] = _collect_http_exceptions(
-    HTTPError
-)
-
-
-def get_http_error(status_code: int) -> type[HTTPError] | None:
-    """Returns aiohttp error class corresponding to a 4XX or 5XX status code
-
-    NOTICE that any non-error code (i.e. 2XX, 3XX and 4XX) will return None
-    """
-    return _STATUS_CODE_TO_HTTP_ERRORS.get(status_code)

packages/service-library/src/servicelib/aiohttp/web_exceptions_extension.py

@@ -1,8 +1,85 @@
-from aiohttp.web_exceptions import HTTPClientError
+""" Extends `aiohttp.web_exceptions` classes to match `status` codes
+    and adds helper functions.
+"""
+
+import inspect
+from typing import Any, TypeVar
+
+from aiohttp import web_exceptions
+from aiohttp.web_exceptions import (
+    HTTPClientError,
+    HTTPError,
+    HTTPException,
+    HTTPServerError,
+)
 
 from . import status
 
+assert issubclass(HTTPError, HTTPException)  # nsoec
+
+# NOTE: these are the status codes that DO NOT have an aiohttp.HTTPException associated
+STATUS_CODES_WITHOUT_AIOHTTP_EXCEPTION_CLASS = (
+    status.HTTP_100_CONTINUE,
+    status.HTTP_101_SWITCHING_PROTOCOLS,
+    status.HTTP_102_PROCESSING,
+    status.HTTP_103_EARLY_HINTS,
+    status.HTTP_207_MULTI_STATUS,
+    status.HTTP_208_ALREADY_REPORTED,
+    status.HTTP_226_IM_USED,
+    status.HTTP_306_RESERVED,
+    status.HTTP_418_IM_A_TEAPOT,
+    status.HTTP_425_TOO_EARLY,
+)
+
 
 class HTTPLockedError(HTTPClientError):
     # pylint: disable=too-many-ancestors
     status_code = status.HTTP_423_LOCKED
+
+
+class HTTPLoopDetectedError(HTTPServerError):
+    # pylint: disable=too-many-ancestors
+    status_code = status.HTTP_508_LOOP_DETECTED
+
+
+E = TypeVar("E", bound="HTTPException")
+
+
+def get_all_aiohttp_http_exceptions(
+    base_http_exception_cls: type[E],
+) -> dict[int, type[E]]:
+    # Inverse map from code to HTTPException classes
+
+    def _pred(obj) -> bool:
+        return (
+            inspect.isclass(obj)
+            and issubclass(obj, base_http_exception_cls)
+            and getattr(obj, "status_code", 0) > 0
+        )
+
+    found: list[tuple[str, Any]] = inspect.getmembers(web_exceptions, _pred)
+    assert found  # nosec
+
+    status_to_http_exception_map = {cls.status_code: cls for _, cls in found}
+    assert len(status_to_http_exception_map) == len(found), "No duplicates"  # nosec
+
+    for cls in (
+        HTTPLockedError,
+        HTTPLoopDetectedError,
+    ):
+        status_to_http_exception_map[cls.status_code] = cls
+
+    return status_to_http_exception_map
+
+
+_STATUS_CODE_TO_HTTP_ERRORS: dict[
+    int, type[HTTPError]
+] = get_all_aiohttp_http_exceptions(HTTPError)
+
+
+def get_http_error_class_or_none(status_code: int) -> type[HTTPError] | None:
+    """Returns aiohttp error class corresponding to a 4XX or 5XX status code
+
+    NOTE: any non-error code (i.e. 2XX, 3XX and 4XX) will return None
+    """
+    return _STATUS_CODE_TO_HTTP_ERRORS.get(status_code)

packages/service-library/src/servicelib/status_utils.py renamed to packages/service-library/src/servicelib/status_codes_utils.py

@@ -2,15 +2,15 @@
 
     - on aiohttp services
         from servicelib.aiohttp import status
-        from servicelib.status_utils import is_success
+        from servicelib.status_codes_utils import is_success
 
         assert is_success(status.HTTP_200_OK)
 
 
     - on fastapi services
 
         from fastapi import status
-        from servicelib.status_utils import is_success
+        from servicelib.status_codes_utils import is_success
 
         assert is_success(status.HTTP_200_OK)
 
@@ -37,6 +37,7 @@ def get_code_display_name(status_code: int) -> str:
         return f"HTTP_{status_code}_{code.name}"
     except ValueError:
         if status_code == 306:  # noqa: PLR2004
+            # NOTE: HttpStatus does not include 306
             return "HTTP_306_RESERVED"
         return _INVALID_STATUS_CODE_MSG
 
@@ -65,35 +66,35 @@ def get_code_description(status_code: int) -> str:
     )
 
 
-def is_informational(status_code: int) -> bool:
+def is_1xx_informational(status_code: int) -> bool:
     """
     Returns `True` for 1xx status codes, `False` otherwise.
     """
     return 100 <= status_code <= 199  # noqa: PLR2004
 
 
-def is_success(status_code: int) -> bool:
+def is_2xx_success(status_code: int) -> bool:
     """
     Returns `True` for 2xx status codes, `False` otherwise.
     """
     return 200 <= status_code <= 299  # noqa: PLR2004
 
 
-def is_redirect(status_code: int) -> bool:
+def is_3xx_redirect(status_code: int) -> bool:
     """
     Returns `True` for 3xx status codes, `False` otherwise.
     """
     return 300 <= status_code <= 399  # noqa: PLR2004
 
 
-def is_client_error(status_code: int) -> bool:
+def is_4xx_client_error(status_code: int) -> bool:
     """
     Returns `True` for 4xx status codes, `False` otherwise.
     """
     return 400 <= status_code <= 499  # noqa: PLR2004
 
 
-def is_server_error(status_code: int) -> bool:
+def is_5xx_server_error(status_code: int) -> bool:
     """
     Returns `True` for 5xx status codes, `False` otherwise.
     """

packages/service-library/tests/aiohttp/test_rest_responses.py

@@ -16,16 +16,13 @@
     HTTPOk,
 )
 from servicelib.aiohttp import status
-from servicelib.aiohttp.rest_responses import (
+from servicelib.aiohttp.rest_responses import create_http_error, exception_to_response
+from servicelib.aiohttp.web_exceptions_extension import (
     _STATUS_CODE_TO_HTTP_ERRORS,
-    create_http_error,
-    exception_to_response,
-    get_http_error,
+    get_http_error_class_or_none,
 )
 from servicelib.mimetype_constants import MIMETYPE_APPLICATION_JSON
 
-#
-
 # SEE https://httpstatuses.com/
 # - below 1xx  -> invalid
 BELOW_1XX = (-5, 0, 5, 99)
@@ -36,17 +33,17 @@
 
 
 @pytest.mark.parametrize(
-    "http_exc", (HTTPBadRequest, HTTPGone, HTTPInternalServerError)
+    "http_exc", [HTTPBadRequest, HTTPGone, HTTPInternalServerError]
 )
 def test_get_http_exception_class_from_code(http_exc: HTTPException):
-    assert get_http_error(http_exc.status_code) == http_exc
+    assert get_http_error_class_or_none(http_exc.status_code) == http_exc
 
 
 @pytest.mark.parametrize(
     "status_code", itertools.chain(BELOW_1XX, NONE_ERRORS, ABOVE_599)
 )
 def test_get_none_for_invalid_or_not_errors_code(status_code):
-    assert get_http_error(status_code) is None
+    assert get_http_error_class_or_none(status_code) is None
 
 
 @pytest.mark.parametrize(

packages/service-library/tests/aiohttp/test_status_utils.py

@@ -1,17 +1,23 @@
 from http import HTTPStatus
 
+import pytest
 from servicelib.aiohttp import status
-from servicelib.status_utils import (
+from servicelib.aiohttp.web_exceptions_extension import (
+    STATUS_CODES_WITHOUT_AIOHTTP_EXCEPTION_CLASS,
+    HTTPException,
+    get_all_aiohttp_http_exceptions,
+)
+from servicelib.status_codes_utils import (
     _INVALID_STATUS_CODE_MSG,
     get_code_description,
     get_code_display_name,
     get_http_status_codes,
-    is_client_error,
+    is_1xx_informational,
+    is_2xx_success,
+    is_3xx_redirect,
+    is_4xx_client_error,
+    is_5xx_server_error,
     is_error,
-    is_informational,
-    is_redirect,
-    is_server_error,
-    is_success,
 )
 
 
@@ -31,12 +37,12 @@ def test_description():
 
 def test_status_codes_checks():
 
-    assert is_informational(status.HTTP_102_PROCESSING)
-    assert is_success(status.HTTP_202_ACCEPTED)
-    assert is_redirect(status.HTTP_301_MOVED_PERMANENTLY)
+    assert is_1xx_informational(status.HTTP_102_PROCESSING)
+    assert is_2xx_success(status.HTTP_202_ACCEPTED)
+    assert is_3xx_redirect(status.HTTP_301_MOVED_PERMANENTLY)
 
-    assert is_client_error(status.HTTP_401_UNAUTHORIZED)
-    assert is_server_error(status.HTTP_503_SERVICE_UNAVAILABLE)
+    assert is_4xx_client_error(status.HTTP_401_UNAUTHORIZED)
+    assert is_5xx_server_error(status.HTTP_503_SERVICE_UNAVAILABLE)
 
     assert is_error(status.HTTP_401_UNAUTHORIZED)
     assert is_error(status.HTTP_503_SERVICE_UNAVAILABLE)
@@ -45,7 +51,7 @@ def test_status_codes_checks():
 def test_predicates_with_status():
 
     # in formational
-    assert get_http_status_codes(status, is_informational) == [
+    assert get_http_status_codes(status, is_1xx_informational) == [
         status.HTTP_100_CONTINUE,
         status.HTTP_101_SWITCHING_PROTOCOLS,
         status.HTTP_102_PROCESSING,
@@ -61,3 +67,17 @@ def test_predicates_with_status():
         for c in get_http_status_codes(status)
         if c != status.HTTP_306_RESERVED
     ]
+
+
+AIOHTTP_EXCEPTION_CLASSES_MAP: dict[
+    int, type[HTTPException]
+] = get_all_aiohttp_http_exceptions(HTTPException)
+
+
+@pytest.mark.parametrize("status_code", get_http_status_codes(status))
+def test_how_status_codes_map_to_aiohttp_exception_class(status_code):
+    aiohttp_exception_cls = AIOHTTP_EXCEPTION_CLASSES_MAP.get(status_code)
+    if status_code in STATUS_CODES_WITHOUT_AIOHTTP_EXCEPTION_CLASS:
+        assert aiohttp_exception_cls is None
+    else:
+        assert aiohttp_exception_cls is not None

services/storage/src/simcore_service_storage/handlers_files.py

@@ -1,7 +1,7 @@
 import asyncio
 import logging
 import urllib.parse
-from typing import NoReturn, cast
+from typing import cast
 
 from aiohttp import web
 from aiohttp.web import RouteTableDef
@@ -25,7 +25,6 @@
     parse_request_path_parameters_as,
     parse_request_query_parameters_as,
 )
-from servicelib.mimetype_constants import MIMETYPE_APPLICATION_JSON
 
 from ._meta import API_VTAG
 from .dsm import get_dsm_provider
@@ -249,7 +248,7 @@ async def upload_file(request: web.Request) -> web.Response:
     f"/{API_VTAG}/locations/{{location_id}}/files/{{file_id}}:abort",
     name="abort_upload_file",
 )
-async def abort_upload_file(request: web.Request) -> NoReturn:
+async def abort_upload_file(request: web.Request) -> web.Response:
     query_params: StorageQueryParamsBase = parse_request_query_parameters_as(
         StorageQueryParamsBase, request
     )
@@ -261,7 +260,7 @@ async def abort_upload_file(request: web.Request) -> NoReturn:
 
     dsm = get_dsm_provider(request.app).get(path_params.location_id)
     await dsm.abort_file_upload(query_params.user_id, path_params.file_id)
-    raise web.HTTPNoContent(content_type=MIMETYPE_APPLICATION_JSON)
+    return web.json_response(status=status.HTTP_204_NO_CONTENT)
 
 
 @routes.post(
@@ -374,7 +373,7 @@ async def is_completed_upload_file(request: web.Request) -> web.Response:
 @routes.delete(
     f"/{API_VTAG}/locations/{{location_id}}/files/{{file_id}}", name="delete_file"
 )
-async def delete_file(request: web.Request) -> NoReturn:
+async def delete_file(request: web.Request) -> web.Response:
     query_params: StorageQueryParamsBase = parse_request_query_parameters_as(
         StorageQueryParamsBase, request
     )
@@ -386,7 +385,7 @@ async def delete_file(request: web.Request) -> NoReturn:
 
     dsm = get_dsm_provider(request.app).get(path_params.location_id)
     await dsm.delete_file(query_params.user_id, path_params.file_id)
-    raise web.HTTPNoContent(content_type=MIMETYPE_APPLICATION_JSON)
+    return web.json_response(status=status.HTTP_204_NO_CONTENT)
 
 
 @routes.post(f"/{API_VTAG}/files/{{file_id}}:soft-copy", name="copy_as_soft_link")