fixes raised HTTPSuccessfule and added more tests

Author: pcrespov

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

@@ -3,7 +3,6 @@
     SEE  https://gist.github.com/amitripshtos/854da3f4217e3441e8fceea85b0cbd91
 """
 import asyncio
-import json
 import logging
 from collections.abc import Awaitable, Callable
 from typing import Any, Union
@@ -45,17 +44,21 @@ async def _handle_http_error(
     """
     assert request  # nosec
     assert err.reason  # nosec
+    assert str(err) == err.reason  # nosec
 
-    err.content_type = MIMETYPE_APPLICATION_JSON
-    if not err.empty_body and (not err.text or not is_enveloped_from_text(err.text)):
-        error_body = ResponseErrorBody(
-            message=err.reason,
-            # FIXME: these below are not really necessary!
-            status=err.status,
-            errors=[ErrorDetail.from_exception(err)],
-            logs=[LogMessage(message=err.reason, level="ERROR")],
-        )
-        err.text = EnvelopeFactory(error=error_body).as_text()
+    if not err.empty_body:
+        # By default exists if class method empty_body==False
+        assert err.text  # nosec
+
+        if err.text and not is_enveloped_from_text(err.text):
+            error_body = ResponseErrorBody(
+                message=err.reason,  # we do not like default text=`{status}: {reason}`
+                status=err.status,
+                errors=[ErrorDetail.from_exception(err)],
+                logs=[LogMessage(message=err.reason, level="ERROR")],
+            )
+            err.text = EnvelopeFactory(error=error_body).as_text()
+        err.content_type = MIMETYPE_APPLICATION_JSON
 
     return err
 
@@ -69,16 +72,25 @@ async def _handle_http_successful(
     """
     assert request  # nosec
     assert err.reason  # nosec
-    err.content_type = MIMETYPE_APPLICATION_JSON
+    assert str(err) == err.reason  # nosec
+
     if not err.empty_body:
-        # NOTE: These are scenarios created by a lack of
-        # consistency on how we respond in the request handlers.
-        # This overhead can be avoided by having a more strict
-        # response policy.
-        if not err.text and err.reason:
+        # By default exists if class method empty_body==False
+        assert err.text  # nosec
+
+        if err.text and not is_enveloped_from_text(err.text):
+            # NOTE:
+            # - aiohttp defaults `text={status}: {reason}` if not explictly defined and reason defaults
+            #   in http.HTTPStatus().phrase if not explicitly defined
+            # - These are scenarios created by a lack of
+            #   consistency on how we respond in the request handlers.
+            #   This overhead can be avoided by having a more strict
+            #   response policy.
+            # - Moreover there is an *concerning* asymmetry on how these responses are handled
+            #   depending whether the are returned or raised!!!!
             err.text = json_dumps({"data": err.reason})
-        elif err.text and not is_enveloped_from_text(err.text):
-            err.text = json_dumps({"data": json.loads(err.text)})
+        err.content_type = MIMETYPE_APPLICATION_JSON
+
     return err
 
 

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

@@ -6,6 +6,7 @@
 import asyncio
 import json
 import logging
+from collections.abc import Callable
 from dataclasses import dataclass
 from http import HTTPStatus
 from typing import Any
@@ -31,6 +32,7 @@
 )
 from servicelib.error_codes import parse_error_code
 from servicelib.json_serialization import json_dumps
+from servicelib.mimetype_constants import MIMETYPE_APPLICATION_JSON
 from servicelib.status_codes_utils import (
     get_http_status_codes,
     is_2xx_success,
@@ -157,9 +159,33 @@ async def raise_exception(cls, request: web.Request):
             case _:  # unexpected
                 raise SomeUnexpectedError(cls.EXPECTED_RAISE_UNEXPECTED_REASON)
 
+    @staticmethod
+    async def raise_error(request: web.Request):
+        raise web.HTTPNotFound
+
+    @staticmethod
+    async def raise_error_with_reason(request: web.Request):
+        raise web.HTTPNotFound(reason="I did not find it")
+
+    @staticmethod
+    async def raise_success(request: web.Request):
+        raise web.HTTPOk
+
+    @staticmethod
+    async def raise_success_with_reason(request: web.Request):
+        raise web.HTTPOk(reason="I'm ok")
+
+    @staticmethod
+    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}))
+
 
 @pytest.fixture
-def client(event_loop, aiohttp_client):
+def client(
+    event_loop: asyncio.AbstractEventLoop,
+    aiohttp_client: Callable,
+):
     app = web.Application()
 
     # routes
@@ -176,7 +202,13 @@ def client(event_loop, aiohttp_client):
                 ("/v1/number", Handlers.get_number),
                 ("/v1/mixed", Handlers.get_mixed),
                 ("/v1/get_http_response", Handlers.get_http_response),
+                # custom use cases
                 ("/v1/raise_exception", Handlers.raise_exception),
+                ("/v1/raise_error", Handlers.raise_error),
+                ("/v1/raise_error_with_reason", Handlers.raise_error_with_reason),
+                ("/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),
             ]
         ]
     )
@@ -292,13 +324,14 @@ async def test_fails_with_http_successful(client: TestClient, status_code: int):
     assert response.reason == Handlers.EXPECTED_HTTP_RESPONSE_REASON.format(status_code)
 
     # NOTE: non-json response are sometimes necessary mostly on redirects
-    # NOTE: this is how aiohptt defaults text using status and reason when empty_body is not expected
+    # NOTE: this is how aiohttp defaults text using status and reason when empty_body is not expected
     expected = (
         ""
         if all_aiohttp_http_exceptions[status_code].empty_body
         else f"{response.status}: {response.reason}"
     )
     assert await response.text() == expected
+    # NOTE there is an concerning asymmetry between returning and raising web.HTTPSuccessful!!!
 
 
 @pytest.mark.parametrize(
@@ -408,3 +441,55 @@ async def test_aiohttp_exceptions_construction_policies(client: TestClient):
     text = await response.text()
     assert err.text == f"{err.status}: {err.reason}"
     print(text)
+
+
+async def test_raise_error(client: TestClient):
+    # w/o reason
+    resp1 = await client.get("/v1/raise_error")
+    assert resp1.status == status.HTTP_404_NOT_FOUND
+    assert resp1.content_type == MIMETYPE_APPLICATION_JSON
+    assert resp1.reason == HTTPStatus(resp1.status).phrase
+
+    body = await resp1.json()
+    assert body["error"]["message"] == resp1.reason
+
+    # without
+    resp2 = await client.get("/v1/raise_error_with_reason")
+    assert resp2.status == resp1.status
+    assert resp2.content_type == resp1.content_type
+    assert resp2.reason != resp1.reason
+
+    body = await resp2.json()
+    assert body["error"]["message"] == resp2.reason
+
+
+async def test_raise_success(client: TestClient):
+    # w/o reason
+    resp_default = await client.get("/v1/raise_success")
+    assert resp_default.status == status.HTTP_200_OK
+    assert resp_default.content_type == MIMETYPE_APPLICATION_JSON
+    assert resp_default.reason == HTTPStatus(resp_default.status).phrase
+
+    body = await resp_default.json()
+    assert body["data"] == resp_default.reason
+
+    # without
+    resp2 = await client.get("/v1/raise_success_with_reason")
+    assert resp2.status == resp_default.status
+    assert resp2.content_type == resp_default.content_type
+    assert resp2.reason != resp_default.reason
+
+    body = await resp2.json()
+    assert body["data"] == resp2.reason
+
+    # with text
+    # NOTE: in this case, when we enforce text, then `reason` does not reach front-end anymore!
+    resp3 = await client.get("/v1/raise_success_with_text")
+    assert resp3.status == resp_default.status
+    assert resp3.content_type == resp_default.content_type
+    assert resp3.reason != resp_default.reason
+
+    body = await resp3.json()
+    # explicitly NOT enveloped
+    assert "data" not in body
+    assert body == {"ok": True}

services/storage/src/simcore_service_storage/handlers_simcore_s3.py

@@ -1,4 +1,3 @@
-import json
 import logging
 from typing import NoReturn, cast
 
@@ -7,6 +6,7 @@
 from models_library.api_schemas_storage import FileMetaDataGet, FoldersBody
 from models_library.projects import ProjectID
 from models_library.utils.fastapi_encoders import jsonable_encoder
+from servicelib.aiohttp import status
 from servicelib.aiohttp.long_running_tasks.server import (
     TaskProgress,
     start_long_running_task,
@@ -71,9 +71,7 @@ async def _copy_folders_from_project(
         task_progress=task_progress,
     )
 
-    raise web.HTTPCreated(
-        text=json.dumps(body.destination), content_type=MIMETYPE_APPLICATION_JSON
-    )
+    return web.json_response({"data": body.destination}, status=status.HTTP_201_CREATED)
 
 
 @routes.post(f"/{api_vtag}/simcore-s3/folders", name="copy_folders_from_project")