Bug: exception_handler decorator won't return Response with botocore ClientError

[sthuber90]

Expected Behaviour

On botocore ClientError the Lambda function shall return:

{
  "statusCode": 400,
  "headers": {
    "Content-Type": "application/json"
  },
  "body": "{\"statusCode\": 400, \"message\": \"Bad Request\"}",
  "isBase64Encoded": false
}

Current Behaviour

Lambda function raises exception and ends with error, instead of returning HTTP response

Code snippet

import json
import boto3
from botocore.exceptions import ClientError

from aws_lambda_powertools.utilities.data_classes import (
    APIGatewayProxyEventV2,
    event_source,
)
from aws_lambda_powertools.logging import correlation_paths
from aws_lambda_powertools import Logger
from aws_lambda_powertools.event_handler import APIGatewayHttpResolver, content_types
from aws_lambda_powertools.utilities.typing import LambdaContext
from aws_lambda_powertools.event_handler.exceptions import (
    BadRequestError,
    InternalServerError,
    NotFoundError,
)
from aws_lambda_powertools.event_handler.api_gateway import Response

logger = Logger()
app = APIGatewayHttpResolver()
dynamo = boto3.resource("dynamodb")


@logger.inject_lambda_context(correlation_id_path=correlation_paths.API_GATEWAY_HTTP)
@event_source(data_class=APIGatewayProxyEventV2)
def lambda_handler(event: APIGatewayProxyEventV2, context: LambdaContext):
    return app.resolve(event, context)

@app.get("/my/path")
def get_hello_universe():
    table = dynamo.Table("non-existent")
    table.get_item(
        Key={
            "pk": "abc",
            "sk": "123",
        },
    )
    return {"message": "hello universe"}
    

@app.exception_handler(ClientError)
def handle_botocore_error(ex: ClientError):
    metadata = {"path": app.current_event.path}
    logger.error(f"Exception: {ex}", extra=metadata)
    # return Response(
    #     status_code=400,
    #     content_type=content_types.APPLICATION_JSON,
    #     body=json.dumps({"statusCode": 400, "message": "Bad request"}),
    # )

    raise BadRequestError("something bad happened")

Possible Solution

No response

Steps to Reproduce

Use the provided code snippet to see that the Lambda will throw an exception instead of returning a 400 "Bad Request" response.

AWS Lambda Powertools for Python version

latest (Layer version 18)

AWS Lambda function runtime

3.9

Packaging format used

Lambda Layers

Debugging logs

START RequestId: 846da9eb-f64a-41e7-90cf-293386069384 Version: $LATEST
{"level":"ERROR","location":"handle_botocore_error:45","message":"Exception: An error occurred (AccessDeniedException) when calling the GetItem operation: User: arn:aws:sts::373270804851:assumed-role/test-role-hg1im76s/test is not authorized to perform: dynamodb:GetItem on resource: arn:aws:dynamodb:eu-central-1:373270804851:table/non-existent","timestamp":"2022-04-25 17:21:59,753+0000","service":"service_undefined","cold_start":true,"function_name":"test","function_memory_size":"128","function_arn":"arn:aws:lambda:eu-central-1:373270804851:function:test","function_request_id":"846da9eb-f64a-41e7-90cf-293386069384","correlation_id":"id","path":"/my/path","xray_trace_id":"1-6266d8b6-7c8a3e216f62498e0331a61a"}
[ERROR] BadRequestError: something bad happened
Traceback (most recent call last):
  File "/opt/python/aws_lambda_powertools/logging/logger.py", line 354, in decorate
    return lambda_handler(event, context)
  File "/opt/python/aws_lambda_powertools/middleware_factory/factory.py", line 134, in wrapper
    response = middleware()
  File "/opt/python/aws_lambda_powertools/utilities/data_classes/event_source.py", line 39, in event_source
    return handler(data_class(event), context)
  File "/var/task/lambda_function.py", line 28, in lambda_handler
    return app.resolve(event, context)
  File "/opt/python/aws_lambda_powertools/event_handler/api_gateway.py", line 498, in resolve
    return self._resolve().build(self.current_event, self._cors)
  File "/opt/python/aws_lambda_powertools/event_handler/api_gateway.py", line 557, in _resolve
    return self._call_route(route, match_results.groupdict())  # pass fn args
  File "/opt/python/aws_lambda_powertools/event_handler/api_gateway.py", line 613, in _call_route
    response_builder = self._call_exception_handler(exc, route)
  File "/opt/python/aws_lambda_powertools/event_handler/api_gateway.py", line 654, in _call_exception_handler
    return ResponseBuilder(handler(exp), route)
  File "/var/task/lambda_function.py", line 52, in handle_botocore_error
    raise BadRequestError("something bad happened")END RequestId: 846da9eb-f64a-41e7-90cf-293386069384
REPORT RequestId: 846da9eb-f64a-41e7-90cf-293386069384	Duration: 344.21 ms	Billed Duration: 345 ms	Memory Size: 128 MB	Max Memory Used: 67 MB	Init Duration: 630.48 ms

[heitorlessa]

Definitely a bug in the order of response, as the exception handler was added later and we seem to not be handling the exception type like we do earlier in the call chain.

Thanks for spotting another one @sthuber90!

[sthuber90]

Happy to support with a PR unless anybody is already working on it

[heitorlessa]

Please do @sthuber90 i can review and merge it tomorrow morning.

[sthuber90]

Actually it seems that any exception raised within an exception handler route will raise and cause the Lambda function to end with an error. I guess you could say that it's intentional however it doesn't seem intuitive at first.

@heitorlessa should ServiceError type exceptions (BadRequestError, InternalServerError, NotFoundError, UnauthorizedError) be handled differently when raised in an exception handler than other exceptions? Alternatively, we leave it as is and extend the documentation to say that exceptions raised in exception handlers will cause a Lambda function to error.

As @michaelbrewer aparrently implemented that part, I'm interested in his take on this.

Please see the following commit for a proposal from my side: 6aa7c00

I can create a PR later but have to drop off for now

[michaelbrewer]

Ok i will have a look. Sorry if i screwed up here. I will have a deeper look.

[heitorlessa]

Yes to the first question - we should handle ServiceError exceptions differently as customers will instinctively and eventually raise these to signal 4xx

…

On Tue, 26 Apr 2022 at 19:11, Stephan ***@***.***> wrote: Actually it seems that any exception raised within an exception handler route will raise and cause the Lambda function to end with an error. I guess you could say that it's intentional however it doesn't seem intuitive at first. @heitorlessa <https://github.com/heitorlessa> should ServiceError type exceptions (BadRequestError, InternalServerError, NotFoundError, UnauthorizedError) be handled differently when raised in an exception handler than other exceptions? Alternatively, we leave it as is and extend the documentation to say that exceptions raised in exception handlers will cause a Lambda function to error. As @michaelbrewer <https://github.com/michaelbrewer> aparrently implemented that part, I'm interested in his take on this. Please see the following commit for a proposal from my side: 6aa7c00 <6aa7c00> I can create a PR later but have to drop off for now — Reply to this email directly, view it on GitHub <#1153 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAZPQBD5SB6R46VP6RAFADTVHAPUNANCNFSM5UJH6PKQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[michaelbrewer]

@heitorlessa @sthuber90 - the intention in my mind for exception_handler was that you would always handle the error and return your preferred response and not raise another one.

Maybe the docs should indicate this and the typing also. But i can see the case for raising your preferred service error.

[michaelbrewer]

In the diff, you do lose the original exp, So hopefully you capture this as part of the handler.

Another possible solution

diff --git a/aws_lambda_powertools/event_handler/api_gateway.py b/aws_lambda_powertools/event_handler/api_gateway.py
index 5d57c645..f4731dd4 100644
--- a/aws_lambda_powertools/event_handler/api_gateway.py
+++ b/aws_lambda_powertools/event_handler/api_gateway.py
@@ -651,7 +651,10 @@ class ApiGatewayResolver(BaseRouter):
     def _call_exception_handler(self, exp: Exception, route: Route) -> Optional[ResponseBuilder]:
         handler = self._lookup_exception_handler(type(exp))
         if handler:
-            return ResponseBuilder(handler(exp), route)
+            try:
+                return ResponseBuilder(handler(exp), route)
+            except ServiceError as service_error:
+                exp = service_error
 
         if isinstance(exp, ServiceError):
             return ResponseBuilder(
diff --git a/tests/functional/event_handler/test_api_gateway.py b/tests/functional/event_handler/test_api_gateway.py
index 9b5835c8..c0bce92d 100644
--- a/tests/functional/event_handler/test_api_gateway.py
+++ b/tests/functional/event_handler/test_api_gateway.py
@@ -1210,3 +1210,27 @@ def test_exception_handler_not_found_alt():
 
     # THEN call the @app.not_found() function
     assert result["statusCode"] == 404
+
+
+def test_exception_handler_raises_service_error(json_dump):
+    # SCENARIO: Support an exception_handler that raises a ServiceError
+    # GIVEN
+    app = ApiGatewayResolver()
+
+    @app.exception_handler(ValueError)
+    def client_error(ex: ValueError):
+        raise BadRequestError("Bad request")
+
+    @app.get("/my/path")
+    def get_lambda() -> Response:
+        raise ValueError("foo")
+
+    # WHEN calling the event handler
+    # AND a ValueError is raised
+    result = app(LOAD_GW_EVENT, {})
+
+    # THEN call the exception_handler
+    assert result["statusCode"] == 400
+    assert result["headers"]["Content-Type"] == content_types.APPLICATION_JSON
+    expected = {"statusCode": 400, "message": "Bad request"}
+    assert result["body"] == json_dump(expected)

[michaelbrewer]

My PR is just for comment (and if fine review), docs make this clear that nothing should be raised or we support exception_handler raising a new error of type ServiceError only, i am cool with either.

[sthuber90]

I would disagree that exp gets lost in my code as I would argue it's handled within the handler. Nonetheless, I like the simple and clear code from your PR 👍 for me that solves the problem 🚀

[michaelbrewer]

Just that your @app.exception_handler should not swallow the passed ex, but thats just a tip in the docs :P