🎨♻️ Enhances web-server's error middle-ware for safe status-line and refactors aiohttp response helpers (#7770)

pcrespov · web-flow · commit a72878059b5e · 2025-06-03T19:58:34.000Z
diff --git a/packages/service-library/src/servicelib/aiohttp/rest_middlewares.py b/packages/service-library/src/servicelib/aiohttp/rest_middlewares.py
@@ -18,7 +18,12 @@
 from ..mimetype_constants import MIMETYPE_APPLICATION_JSON
 from ..rest_responses import is_enveloped_from_map, is_enveloped_from_text
 from ..utils import is_production_environ
-from .rest_responses import create_data_response, create_http_error, wrap_as_envelope
+from .rest_responses import (
+    create_data_response,
+    create_http_error,
+    safe_status_message,
+    wrap_as_envelope,
+)
 from .rest_utils import EnvelopeFactory
 from .typing_extension import Handler, Middleware
 
@@ -83,6 +88,8 @@ async def _middleware_handler(request: web.Request, handler: Handler):  # noqa:
         except web.HTTPError as err:
 
             err.content_type = MIMETYPE_APPLICATION_JSON
+            if err.reason:
+                err.set_status(err.status, safe_status_message(message=err.reason))
 
             if not err.text or not is_enveloped_from_text(err.text):
                 error_message = err.text or err.reason or "Unexpected error"
@@ -102,6 +109,9 @@ async def _middleware_handler(request: web.Request, handler: Handler):  # noqa:
 
         except web.HTTPSuccessful as err:
             err.content_type = MIMETYPE_APPLICATION_JSON
+            if err.reason:
+                err.set_status(err.status, safe_status_message(message=err.reason))
+
             if err.text:
                 try:
                     payload = json_loads(err.text)
@@ -176,10 +186,7 @@ async def _middleware_handler(
             return resp
 
         if not isinstance(resp, StreamResponse):
-            resp = create_data_response(
-                data=resp,
-                skip_internal_error_details=_is_prod,
-            )
+            resp = create_data_response(data=resp)
 
         assert isinstance(resp, web.StreamResponse)  # nosec
         return resp
diff --git a/packages/service-library/src/servicelib/aiohttp/rest_responses.py b/packages/service-library/src/servicelib/aiohttp/rest_responses.py
@@ -1,10 +1,7 @@
-"""Utils to check, convert and compose server responses for the RESTApi"""
+from typing import Any, Final, TypedDict
 
-import inspect
-from typing import Any
-
-from aiohttp import web, web_exceptions
-from aiohttp.web_exceptions import HTTPError, HTTPException
+from aiohttp import web
+from aiohttp.web_exceptions import HTTPError
 from common_library.error_codes import ErrorCodeStr
 from common_library.json_serialization import json_dumps
 from models_library.rest_error import ErrorGet, ErrorItemType
@@ -13,46 +10,71 @@
 from ..mimetype_constants import MIMETYPE_APPLICATION_JSON
 from ..rest_constants import RESPONSE_MODEL_POLICY
 from ..rest_responses import is_enveloped
-from ..status_codes_utils import get_code_description
+from ..status_codes_utils import get_code_description, is_error
+
+
+class EnvelopeDict(TypedDict):
+    data: Any
+    error: Any
 
 
 def wrap_as_envelope(
-    data: Any = None,
-    error: Any = None,
-) -> dict[str, Any]:
+    data: Any | None = None,
+    error: Any | None = None,
+) -> EnvelopeDict:
     return {"data": data, "error": error}
 
 
-# RESPONSES FACTORIES -------------------------------
+def create_data_response(data: Any, *, status: int = HTTP_200_OK) -> web.Response:
+    """Creates a JSON response with the given data and ensures it is wrapped in an envelope."""
 
+    assert (  # nosec
+        is_error(status) is False
+    ), f"Expected a non-error status code, got {status=}"
 
-def create_data_response(
-    data: Any, *, skip_internal_error_details=False, status=HTTP_200_OK
-) -> web.Response:
-    response = None
-    try:
-        payload = wrap_as_envelope(data) if not is_enveloped(data) else data
+    enveloped_payload = wrap_as_envelope(data) if not is_enveloped(data) else data
+    return web.json_response(enveloped_payload, dumps=json_dumps, status=status)
 
-        response = web.json_response(payload, dumps=json_dumps, status=status)
-    except (TypeError, ValueError) as err:
-        response = exception_to_response(
-            create_http_error(
-                [
-                    err,
-                ],
-                str(err),
-                web.HTTPInternalServerError,
-                skip_internal_error_details=skip_internal_error_details,
-            )
-        )
-    return response
+
+MAX_STATUS_MESSAGE_LENGTH: Final[int] = 100
+
+
+def safe_status_message(
+    message: str | None, max_length: int = MAX_STATUS_MESSAGE_LENGTH
+) -> str | None:
+    """
+    Truncates a status-message (i.e. `reason` in HTTP errors) to a maximum length, replacing newlines with spaces.
+
+    If the message is longer than max_length, it will be truncated and "..." will be appended.
+
+    This prevents issues such as:
+        - `aiohttp.http_exceptions.LineTooLong`: 400, message: Got more than 8190 bytes when reading Status line is too long.
+        - Multiline not allowed in HTTP reason attribute (aiohttp now raises ValueError).
+
+    See:
+        - When to use http status and/or text messages https://github.com/ITISFoundation/osparc-simcore/pull/7760
+        - [RFC 9112, Section 4.1: HTTP/1.1 Message Syntax and Routing](https://datatracker.ietf.org/doc/html/rfc9112#section-4.1) (status line length limits)
+        - [RFC 9110, Section 15.5: Reason Phrase](https://datatracker.ietf.org/doc/html/rfc9110#section-15.5) (reason phrase definition)
+    """
+    assert max_length > 0  # nosec
+
+    if not message:
+        return None
+
+    flat_message = message.replace("\n", " ")
+    if len(flat_message) <= max_length:
+        return flat_message
+
+    # Truncate and add ellipsis
+    return flat_message[: max_length - 3] + "..."
 
 
 def create_http_error(
     errors: list[Exception] | Exception,
-    reason: str | None = None,
+    error_message: str | None = None,
     http_error_cls: type[HTTPError] = web.HTTPInternalServerError,
     *,
+    status_reason: str | None = None,
     skip_internal_error_details: bool = False,
     error_code: ErrorCodeStr | None = None,
 ) -> HTTPError:
@@ -64,14 +86,18 @@ def create_http_error(
     if not isinstance(errors, list):
         errors = [errors]
 
-    is_internal_error: bool = http_error_cls == web.HTTPInternalServerError
-    default_message = reason or get_code_description(http_error_cls.status_code)
+    is_internal_error = bool(http_error_cls == web.HTTPInternalServerError)
+
+    status_reason = status_reason or get_code_description(http_error_cls.status_code)
+    error_message = error_message or get_code_description(http_error_cls.status_code)
+
+    assert len(status_reason) < MAX_STATUS_MESSAGE_LENGTH  # nosec
 
     if is_internal_error and skip_internal_error_details:
         error = ErrorGet.model_validate(
             {
                 "status": http_error_cls.status_code,
-                "message": default_message,
+                "message": error_message,
                 "support_id": error_code,
             }
         )
@@ -81,7 +107,7 @@ def create_http_error(
             {
                 "errors": items,  # NOTE: deprecated!
                 "status": http_error_cls.status_code,
-                "message": default_message,
+                "message": error_message,
                 "support_id": error_code,
             }
         )
@@ -92,7 +118,7 @@ def create_http_error(
     )
 
     return http_error_cls(
-        reason=safe_status_message(reason),
+        reason=safe_status_message(status_reason),
         text=json_dumps(
             payload,
         ),
@@ -110,47 +136,3 @@ def exception_to_response(exc: HTTPError) -> web.Response:
         reason=exc.reason,
         text=exc.text,
     )
-
-
-# Inverse map from code to HTTPException classes
-def _collect_http_exceptions(exception_cls: type[HTTPException] = HTTPException):
-    def _pred(obj) -> bool:
-        return (
-            inspect.isclass(obj)
-            and issubclass(obj, exception_cls)
-            and getattr(obj, "status_code", 0) > 0
-        )
-
-    found: list[tuple[str, Any]] = inspect.getmembers(web_exceptions, _pred)
-    assert found  # nosec
-
-    http_statuses = {cls.status_code: cls for _, cls in found}
-    assert len(http_statuses) == len(found), "No duplicates"  # nosec
-
-    return http_statuses
-
-
-def safe_status_message(message: str | None, max_length: int = 50) -> str | None:
-    """
-    Truncates a status-message (i.e. `reason` in HTTP errors) to a maximum length, replacing newlines with spaces.
-
-    If the message is longer than max_length, it will be truncated and "..." will be appended.
-
-    This prevents issues such as:
-        - `aiohttp.http_exceptions.LineTooLong`: 400, message: Got more than 8190 bytes when reading Status line is too long.
-        - Multiline not allowed in HTTP reason attribute (aiohttp now raises ValueError).
-
-    See:
-        - When to use http status and/or text messages https://github.com/ITISFoundation/osparc-simcore/pull/7760
-        - [RFC 9112, Section 4.1: HTTP/1.1 Message Syntax and Routing](https://datatracker.ietf.org/doc/html/rfc9112#section-4.1) (status line length limits)
-        - [RFC 9110, Section 15.5: Reason Phrase](https://datatracker.ietf.org/doc/html/rfc9110#section-15.5) (reason phrase definition)
-    """
-    if not message:
-        return None
-
-    flat_message = message.replace("\n", " ")
-    if len(flat_message) <= max_length:
-        return flat_message
-
-    # Truncate and add ellipsis
-    return flat_message[: max_length - 3] + "..."
diff --git a/packages/service-library/tests/aiohttp/test_rest_responses.py b/packages/service-library/tests/aiohttp/test_rest_responses.py
@@ -17,8 +17,11 @@
     HTTPOk,
 )
 from common_library.error_codes import ErrorCodeStr, create_error_code
-from servicelib.aiohttp import status
-from servicelib.aiohttp.rest_responses import create_http_error, exception_to_response
+from servicelib.aiohttp.rest_responses import (
+    MAX_STATUS_MESSAGE_LENGTH,
+    create_http_error,
+    exception_to_response,
+)
 from servicelib.aiohttp.web_exceptions_extension import (
     _STATUS_CODE_TO_HTTP_ERRORS,
     get_http_error_class_or_none,
@@ -61,16 +64,43 @@ def test_collected_http_errors_map(status_code: int, http_error_cls: type[HTTPEr
 
 @pytest.mark.parametrize("skip_details", [True, False])
 @pytest.mark.parametrize("error_code", [None, create_error_code(Exception("fake"))])
-def tests_exception_to_response(skip_details: bool, error_code: ErrorCodeStr | None):
-
-    expected_reason = "Something whent wrong !"
+@pytest.mark.parametrize(
+    "http_error_cls",
+    [
+        web.HTTPBadRequest,  # 400
+        web.HTTPUnauthorized,  # 401
+        web.HTTPForbidden,  # 403
+        web.HTTPNotFound,  # 404
+        web.HTTPGone,  # 410
+        web.HTTPInternalServerError,  # 500
+        web.HTTPBadGateway,  # 502
+        web.HTTPServiceUnavailable,  # 503
+    ],
+    ids=[
+        "400",
+        "401",
+        "403",
+        "404",
+        "410",
+        "500",
+        "502",
+        "503",
+    ],
+)
+def tests_exception_to_response(
+    skip_details: bool, error_code: ErrorCodeStr | None, http_error_cls: type[HTTPError]
+):
+    expected_status_reason = "SHORT REASON"
+    expected_error_message = "Something whent wrong !"
     expected_exceptions: list[Exception] = [RuntimeError("foo")]
 
     http_error = create_http_error(
         errors=expected_exceptions,
-        reason=expected_reason,
-        http_error_cls=web.HTTPInternalServerError,
-        skip_internal_error_details=skip_details,
+        error_message=expected_error_message,
+        status_reason=expected_status_reason,
+        http_error_cls=http_error_cls,
+        skip_internal_error_details=skip_details
+        and (http_error_cls == web.HTTPInternalServerError),
         error_code=error_code,
     )
 
@@ -87,16 +117,17 @@ def tests_exception_to_response(skip_details: bool, error_code: ErrorCodeStr | N
 
     # checks response components
     assert response.content_type == MIMETYPE_APPLICATION_JSON
-    assert response.status == status.HTTP_500_INTERNAL_SERVER_ERROR
+    assert response.status == http_error_cls.status_code
     assert response.text
     assert response.body
 
     # checks response model
     response_json = json.loads(response.text)
     assert response_json["data"] is None
-    assert response_json["error"]["message"] == expected_reason
+    assert response_json["error"]["message"] == expected_error_message
     assert response_json["error"]["supportId"] == error_code
     assert response_json["error"]["status"] == response.status
+    assert response.reason == expected_status_reason
 
 
 @pytest.mark.parametrize(
@@ -109,10 +140,13 @@ def tests_exception_to_response(skip_details: bool, error_code: ErrorCodeStr | N
             "Message\nwith\nnewlines",
             "Message with newlines",
         ),  # Newlines are replaced with spaces
-        ("A" * 100, "A" * 47 + "..."),  # Long message gets truncated with ellipsis
         (
-            "Line1\nLine2\nLine3" + "X" * 100,
-            "Line1 Line2 Line3" + "X" * 30 + "...",
+            "A" * (MAX_STATUS_MESSAGE_LENGTH + 1),
+            "A" * (MAX_STATUS_MESSAGE_LENGTH - 3) + "...",
+        ),  # Long message gets truncated with ellipsis
+        (
+            "Line1\nLine2\nLine3" + "X" * (MAX_STATUS_MESSAGE_LENGTH + 1),
+            "Line1 Line2 Line3" + "X" * (MAX_STATUS_MESSAGE_LENGTH - 20) + "...",
         ),  # Combined case: newlines and truncation with ellipsis
     ],
     ids=[
diff --git a/services/web/server/src/simcore_service_webserver/director_v2/_controller/_rest_exceptions.py b/services/web/server/src/simcore_service_webserver/director_v2/_controller/_rest_exceptions.py
@@ -30,6 +30,11 @@
 async def _handler_director_service_error_as_503_or_4xx(
     request: web.Request, exception: Exception
 ) -> web.Response:
+    """
+    Handles DirectorV2ServiceError exceptions by returning:
+      - HTTP 503 Service Unavailable if the underlying director-v2 service returns a 5XX server error,
+      - or the original 4XX client error status and message if it is a client error.
+    """
     assert isinstance(exception, DirectorV2ServiceError)  # nosec
     assert status_codes_utils.is_error(
         exception.status
