🐛♻️ webserver error handling: Fix incorrect error logging for web.HTTPCreated; cleanup success response handling (#7952)

Author: pcrespov

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

@@ -67,7 +67,7 @@ def handle_validation_as_http_error(
                 }
                 for e in details
             ]
-            error_str = json_dumps(
+            error_json_str = json_dumps(
                 {
                     "error": {
                         "status": status.HTTP_422_UNPROCESSABLE_ENTITY,
@@ -77,7 +77,7 @@ def handle_validation_as_http_error(
             )
         else:
             # NEW proposed error for https://github.com/ITISFoundation/osparc-simcore/issues/443
-            error_str = json_dumps(
+            error_json_str = json_dumps(
                 {
                     "error": {
                         "msg": user_error_message,
@@ -88,7 +88,7 @@ def handle_validation_as_http_error(
             )
 
         raise web.HTTPUnprocessableEntity(  # 422
-            text=error_str,
+            text=error_json_str,
             content_type=MIMETYPE_APPLICATION_JSON,
         ) from err
 

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

@@ -21,7 +21,7 @@
 
 from ..logging_errors import create_troubleshootting_log_kwargs
 from ..mimetype_constants import MIMETYPE_APPLICATION_JSON
-from ..rest_responses import is_enveloped_from_map, is_enveloped_from_text
+from ..rest_responses import is_enveloped_from_text
 from ..status_codes_utils import get_code_description
 from . import status
 from .rest_responses import (
@@ -167,11 +167,10 @@ def _handle_http_successful(
             exception.status, safe_status_message(message=exception.reason)
         )
 
-    if exception.text:
-        payload = json_loads(exception.text)
-        if not is_enveloped_from_map(payload):
-            payload = wrap_as_envelope(data=payload)
-            exception.text = json_dumps(payload)
+    if exception.text and not is_enveloped_from_text(exception.text):
+        # Ensures that the response is enveloped
+        data = json_loads(exception.text)
+        exception.text = json_dumps({"data": data})
 
     return exception
 

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

@@ -133,13 +133,19 @@ def create_http_error(
     )
 
 
-def exception_to_response(exc: HTTPError) -> web.Response:
+def exception_to_response(exception: HTTPError) -> web.Response:
     # Returning web.HTTPException is deprecated so here we have a converter to a response
     # so it can be used as
     # SEE https://github.com/aio-libs/aiohttp/issues/2415
+
+    if exception.reason:
+        reason = safe_status_message(exception.reason)
+    else:
+        reason = get_code_description(exception.status)
+
     return web.Response(
-        status=exc.status,
-        headers=exc.headers,
-        reason=exc.reason,
-        text=exc.text,
+        status=exception.status,
+        headers=exception.headers,
+        reason=reason,
+        text=exception.text,
     )

packages/service-library/src/servicelib/long_running_tasks/http_endpoint_responses.py

@@ -44,7 +44,7 @@ async def get_task_result(
     except Exception as exc:
         _logger.exception(
             **create_troubleshootting_log_kwargs(
-                user_error_msg=f"{task_id=} raised an exception",
+                user_error_msg=f"{task_id=} raised an exception while getting its result",
                 error=exc,
                 error_code=create_error_code(exc),
                 error_context={"task_context": task_context, "task_id": task_id},

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

@@ -128,6 +128,10 @@ async def raise_success_with_text(_request: web.Request):
         # NOTE: explicitly NOT enveloped!
         raise web.HTTPOk(reason="I'm ok", text=json.dumps({"ok": True}))
 
+    @staticmethod
+    async def raise_success_with_raw_text(_request: web.Request):
+        raise web.HTTPOk(text="I'm ok")  # NOT ALLOWED!
+
 
 @pytest.fixture
 async def client(
@@ -158,6 +162,10 @@ async def client(
                 ("/v1/raise_success", Handlers.raise_success),
                 ("/v1/raise_success_with_reason", Handlers.raise_success_with_reason),
                 ("/v1/raise_success_with_text", Handlers.raise_success_with_text),
+                (
+                    "/v1/raise_success_with_raw_text",
+                    Handlers.raise_success_with_raw_text,
+                ),
             ]
         ]
     )
@@ -330,6 +338,7 @@ async def test_http_ok_with_text_is_enveloped(client: TestClient):
     """Test that HTTPOk with text is properly enveloped."""
     response = await client.get("/v1/raise_success_with_text")
     assert response.status == status.HTTP_200_OK
+    assert response.reason == "I'm ok"
 
     # Should be enveloped
     payload = await response.json()
@@ -342,6 +351,11 @@ async def test_http_ok_with_text_is_enveloped(client: TestClient):
     assert data.get("ok") is True
 
 
+async def test_http_ok_with_raw_text_is_not_allowed(client: TestClient):
+    response = await client.get("/v1/raise_success_with_raw_text")
+    assert response.status == status.HTTP_500_INTERNAL_SERVER_ERROR
+
+
 async def test_exception_in_handler_returns_500(
     client: TestClient, mocker: MockerFixture
 ):

services/web/server/src/simcore_service_webserver/exception_handling/_factory.py

@@ -8,7 +8,11 @@
 from servicelib.aiohttp.rest_responses import safe_status_message
 from servicelib.aiohttp.web_exceptions_extension import get_all_aiohttp_http_exceptions
 from servicelib.logging_errors import create_troubleshootting_log_kwargs
-from servicelib.status_codes_utils import is_5xx_server_error, is_error
+from servicelib.status_codes_utils import (
+    get_code_display_name,
+    is_5xx_server_error,
+    is_error,
+)
 
 from ._base import AiohttpExceptionHandler, ExceptionHandlersMap
 
@@ -50,7 +54,7 @@ def create_error_response(error: ErrorGet, status_code: int) -> web.Response:
     return web.json_response(
         data={"error": error.model_dump(exclude_unset=True, mode="json")},
         dumps=json_dumps,
-        reason=safe_status_message(error.message),
+        reason=safe_status_message(get_code_display_name(status_code)),
         status=status_code,
     )
 

services/web/server/src/simcore_service_webserver/projects/_crud_api_create.py

@@ -263,7 +263,7 @@ async def create_project(  # pylint: disable=too-many-arguments,too-many-branche
     simcore_user_agent: str,
     parent_project_uuid: ProjectID | None,
     parent_node_id: NodeID | None,
-) -> None:
+) -> web.HTTPCreated:
     """Implements TaskProtocol for 'create_projects' handler
 
     Arguments:
@@ -278,7 +278,6 @@ async def create_project(  # pylint: disable=too-many-arguments,too-many-branche
         predefined_project -- project in request body
 
     Raises:
-        web.HTTPCreated: succeeded
         web.HTTPBadRequest:
         web.HTTPNotFound:
         web.HTTPUnauthorized:
@@ -481,7 +480,7 @@ async def create_project(  # pylint: disable=too-many-arguments,too-many-branche
             **RESPONSE_MODEL_POLICY
         )
 
-        raise web.HTTPCreated(
+        return web.HTTPCreated(
             text=json_dumps({"data": data}),
             content_type=MIMETYPE_APPLICATION_JSON,
         )

services/web/server/tests/unit/isolated/test_exception_handling_factory.py

@@ -13,6 +13,7 @@
 from aiohttp.test_utils import make_mocked_request
 from servicelib.aiohttp import status
 from servicelib.mimetype_constants import MIMETYPE_APPLICATION_JSON
+from servicelib.status_codes_utils import get_code_display_name
 from simcore_service_webserver.errors import WebServerBaseError
 from simcore_service_webserver.exception_handling._base import (
     ExceptionHandlingContextManager,
@@ -28,16 +29,13 @@
 # Some custom errors in my service
 
 
-class BaseError(WebServerBaseError):
-    ...
+class BaseError(WebServerBaseError): ...
 
 
-class OneError(BaseError):
-    ...
+class OneError(BaseError): ...
 
 
-class OtherError(BaseError):
-    ...
+class OtherError(BaseError): ...
 
 
 @pytest.fixture
@@ -58,7 +56,7 @@ async def test_factory__create_exception_handler_from_http_error(
     response = await one_error_to_404(fake_request, caught)
     assert response.status == status.HTTP_404_NOT_FOUND
     assert response.text is not None
-    assert "one error message" in response.reason
+    assert response.reason == get_code_display_name(response.status)
     assert response.content_type == MIMETYPE_APPLICATION_JSON
 
 
@@ -82,9 +80,7 @@ async def test_handling_different_exceptions_with_context(
         response = cm.get_response_or_none()
         assert response is not None
         assert response.status == status.HTTP_400_BAD_REQUEST
-        assert response.reason == exc_to_http_error_map[OneError].msg_template.format(
-            code="WebServerBaseError.BaseError.OneError"
-        )
+        assert response.reason == get_code_display_name(response.status)
         assert not caplog.records
 
         # unhandled -> reraises
@@ -103,9 +99,7 @@ async def test_handling_different_exceptions_with_context(
         response = cm.get_response_or_none()
         assert response is not None
         assert response.status == status.HTTP_500_INTERNAL_SERVER_ERROR
-        assert response.reason == exc_to_http_error_map[OtherError].msg_template.format(
-            code="WebServerBaseError.BaseError.OtherError"
-        )
+        assert response.reason == get_code_display_name(response.status)
         assert caplog.records, "Expected 5XX troubleshooting logged as error"
         assert caplog.records[0].levelno == logging.ERROR