Skip to content

Bug: RequestValidationError handling behavior should not be affected if we add exception handler for Exception #3841

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
aitchnyu opened this issue Feb 23, 2024 · 7 comments
Labels
event_handlers help wanted Could use a second pair of eyes/hands not-a-bug revisit-in-3-months Requires more customers feedback before making or revisiting a decision triage Pending triage from maintainers

Comments

@aitchnyu
Copy link

aitchnyu commented Feb 23, 2024

Expected Behaviour

I have an exception handler for Exception.

app = APIGatewayRestResolver(enable_validation=True)

@app.exception_handler(Exception)
def handle_exception(ex: Exception):
    ...

When a validation error happens, I want to get the response:

    {
                "detail": [
                    {
                        "loc": ["body", "message"],
                        "type": "missing",
                    },
                    {"loc": ["body", "sendTo"], "type": "value_error"},
                ],
                "statusCode": 422,
            },

Current Behaviour

I get the response of my error handler instead of the pydantic errors.

{'message': 'internal server error'}

Code snippet

Copy paste of above snippet:


@app.exception_handler(Exception)
def handle_exception(ex: Exception):
    ....

Possible Solution

The code responsible is

class ApiGatewayResolver(BaseRouter):
....
    def _lookup_exception_handler(self, exp_type: Type) -> Optional[Callable]:
        # Use "Method Resolution Order" to allow for matching against a base class
        # of an exception
        for cls in exp_type.__mro__:
            if cls in self._exception_handlers:
                return self._exception_handlers[cls]
        return None

    def _call_exception_handler(self, exp: Exception, route: Route) -> Optional[ResponseBuilder]:
        handler = self._lookup_exception_handler(type(exp))
        if handler:
            try:
                return self._response_builder_class(response=handler(exp), serializer=self._serializer, route=route)
            except ServiceError as service_error:
                exp = service_error

        if isinstance(exp, RequestValidationError):
            # For security reasons, we hide msg details (don't leak Python, Pydantic or file names)
            errors = [{"loc": e["loc"], "type": e["type"]} for e in exp.errors()]

            return self._response_builder_class(
                response=Response(
                    status_code=HTTPStatus.UNPROCESSABLE_ENTITY,
                    content_type=content_types.APPLICATION_JSON,
                    body={"statusCode": HTTPStatus.UNPROCESSABLE_ENTITY, "detail": errors},
                ),
                serializer=self._serializer,
                route=route,
            )

My workaround is to have a version of APIGatewayRestResolver that avoids returning error handler for Exception for RequestValidationError.

from aws_lambda_powertools.event_handler import APIGatewayRestResolver as BaseAPIGatewayRestResolver
from aws_lambda_powertools.event_handler.openapi.exceptions import RequestValidationError
...

class APIGatewayRestResolver(BaseAPIGatewayRestResolver):
    def _lookup_exception_handler(self, exp_type: typing.Type) -> typing.Optional[typing.Callable]:
        if exp_type == RequestValidationError:
            return None
        return super()._lookup_exception_handler(exp_type)


app = APIGatewayRestResolver(enable_validation=True)

I understand my workaround may be against the behavior of #3395 .

If I had to suggest a fix, I would suggest moving validation error from _call_exception_handler to its own method, similar to the case of _not_found handler:

    def _not_found(self, method: str) -> ResponseBuilder:
        """Called when no matching route was found and includes support for the cors preflight response"""
....

        handler = self._lookup_exception_handler(NotFoundError)
        if handler:
            return self._response_builder_class(response=handler(NotFoundError()), serializer=self._serializer)

        return self._response_builder_class(
            response=Response(
                status_code=HTTPStatus.NOT_FOUND.value,
                content_type=content_types.APPLICATION_JSON,
                headers=headers,
                body={"statusCode": HTTPStatus.NOT_FOUND.value, "message": "Not found"},
            ),
            serializer=self._serializer,
        )

Steps to Reproduce

Copy paste of above snippet:

@app.exception_handler(Exception)
def handle_exception(ex: Exception):  # pragma: no cover

Then trigger a pydantic validation error.

Powertools for AWS Lambda (Python) version

latest

AWS Lambda function runtime

3.8

Packaging format used

PyPi

Debugging logs

No response

@aitchnyu aitchnyu added bug Something isn't working triage Pending triage from maintainers labels Feb 23, 2024
Copy link

boring-cyborg bot commented Feb 23, 2024

Thanks for opening your first issue here! We'll come back to you as soon as we can.
In the meantime, check out the #python channel on our Powertools for AWS Lambda Discord: Invite link

@heitorlessa
Copy link
Contributor

hey @aitchnyu - thank you for submitting an extensive bug report!! It should work with RequestValidationError exception.

I'll investigate the behaviour today when catching a catch-all Exception like you did, and come back to you.

As per docs, catching RequestValidatioError should work as you want; tested locally too.

from typing import Optional

import requests
from pydantic import BaseModel, Field

from aws_lambda_powertools import Logger, Tracer
from aws_lambda_powertools.event_handler import APIGatewayRestResolver, Response, content_types
from aws_lambda_powertools.event_handler.openapi.exceptions import RequestValidationError
from aws_lambda_powertools.logging import correlation_paths
from aws_lambda_powertools.utilities.typing import LambdaContext

tracer = Tracer()
logger = Logger()
app = APIGatewayRestResolver(enable_validation=True)


class Todo(BaseModel):
    userId: int
    id_: Optional[int] = Field(alias="id", default=None)
    title: str
    completed: bool


@app.exception_handler(RequestValidationError)  
def handle_validation_error(ex: RequestValidationError):
    logger.error("Request failed validation", path=app.current_event.path, errors=ex.errors())

    return Response(
        status_code=422,
        content_type=content_types.APPLICATION_JSON,
        body="Invalid data",
    )


@app.post("/todos")
def create_todo(todo: Todo) -> int:
    response = requests.post("https://jsonplaceholder.typicode.com/todos", json=todo.dict(by_alias=True))
    response.raise_for_status()

    return response.json()["id"]


@logger.inject_lambda_context(correlation_id_path=correlation_paths.API_GATEWAY_HTTP)
@tracer.capture_lambda_handler
def lambda_handler(event: dict, context: LambdaContext) -> dict:
    return app.resolve(event, context)
image

@heitorlessa heitorlessa removed the triage Pending triage from maintainers label Feb 26, 2024
@heitorlessa heitorlessa self-assigned this Feb 26, 2024
@heitorlessa heitorlessa added not-a-bug and removed bug Something isn't working labels Feb 26, 2024
@heitorlessa heitorlessa moved this from Triage to Pending customer in Powertools for AWS Lambda (Python) Feb 26, 2024
@heitorlessa
Copy link
Contributor

Can't reproduce with either general Exception and the suggested RequestValidationException -- need more details.

Example below on how to get those exact details what the documentation suggests with RequestValidationError.

from typing import Annotated, Optional

import requests
from aws_lambda_powertools import Logger, Tracer
from aws_lambda_powertools.event_handler import (
    APIGatewayRestResolver,
    Response,
    content_types,
)
from aws_lambda_powertools.event_handler.openapi.exceptions import (
    RequestValidationError,
)
from aws_lambda_powertools.event_handler.openapi.params import Path, Query
from aws_lambda_powertools.logging import correlation_paths
from aws_lambda_powertools.utilities.typing import LambdaContext
from aws_lambda_powertools.utilities.parser import Field, BaseModel


tracer = Tracer()
logger = Logger()
app = APIGatewayRestResolver(enable_validation=True)
app.enable_swagger(path="/swagger")


class Todo(BaseModel):
    userId: int
    id_: Optional[int] = Field(alias="id", default=None)
    title: str
    completed: bool


@app.exception_handler(RequestValidationError)
def catch_validation(ex: RequestValidationError):
    logger.info("Catch all exception handler", error=ex)

    err = ex.errors()[0]
    location, message, type = err["loc"], err["msg"], err["type"]

    return Response(
        status_code=400,
        content_type=content_types.TEXT_PLAIN,
        body=f"Uh oh! Received an exception",
    )


@app.get("/todos")
@tracer.capture_method
def get_todos(
    completed: Annotated[str | None, Query(min_length=4)] = None
) -> list[Todo]:
    url = "https://jsonplaceholder.typicode.com/todos"

    if completed is not None:
        url = f"{url}/?completed={completed}"

    todo = requests.get(url)
    todo.raise_for_status()

    return todo.json()


@logger.inject_lambda_context(correlation_id_path=correlation_paths.API_GATEWAY_REST)
@tracer.capture_lambda_handler
def lambda_handler(event: dict, context: LambdaContext) -> dict:
    return app.resolve(event, context)

@aitchnyu
Copy link
Author

aitchnyu commented Mar 5, 2024

My workaround is having handlers for both RequestValidationError and Exception. If I were to disable the RequestValidationError handler, the Exception would handle it (against my expectations).

app = APIGatewayRestResolver(enable_validation=True)
...

@app.exception_handler(Exception)
def handle_exception(ex: Exception):  # pragma: no cover
    ...
    return Response(
        status_code=HTTPStatus.INTERNAL_SERVER_ERROR,
        content_type=content_types.APPLICATION_JSON,
        body=json.dumps({"message": "internal server error"}),
    )


@app.exception_handler(RequestValidationError)
def handle_request_validation_error(ex: RequestValidationError):
    # Copy of ApiGatewayResolver _call_exception_handler
    errors = [{"loc": e["loc"], "type": e["type"]} for e in ex.errors()]
    return Response(
        status_code=HTTPStatus.UNPROCESSABLE_ENTITY,
        content_type=content_types.APPLICATION_JSON,
        body={"statusCode": HTTPStatus.UNPROCESSABLE_ENTITY, "detail": errors},
    )

@leandrodamascena
Copy link
Contributor

Hi @heitorlessa and @aitchnyu, after investigating further I discovered that the issue goes beyond fixing the RequestValidationError or not. Our exception_handler function captures a list of exceptions passed to it, however, it is important to note that most exceptions in Python inherit from Exception (some from BaseException). So if we set exception_handler to catch Exception, it will also catch exceptions like ValueError, KeyError, TypeError, RequestValidationError, and others.

Ideally, it's advisable to catch more specific exceptions, especially if you want to generate custom messages for specific error types like RequestValidationError. I'm not sure if there's a straightforward solution at the moment, I'll need to investigate further. One potential action arising from this issue is to update our documentation to reflect this behavior, where catching Exception acts as a catch-all mechanism for all exceptions.

code

from aws_lambda_powertools import Logger, Tracer
from aws_lambda_powertools.event_handler import APIGatewayRestResolver
from aws_lambda_powertools.logging import correlation_paths
from aws_lambda_powertools.utilities.typing import LambdaContext
from aws_lambda_powertools.event_handler.api_gateway import Response


tracer = Tracer()
logger = Logger()
app = APIGatewayRestResolver()


@app.get("/hello")
def get_todos():
    raise ValueError

@app.exception_handler(Exception)
def handle_invalid_limit_qs(ex: Exception):  # receives exception raised

    return Response(
        status_code=400,
        body="Catching Exception, but raising ValueError",
    )

@logger.inject_lambda_context(correlation_id_path=correlation_paths.API_GATEWAY_REST)
def lambda_handler(event: dict, context: LambdaContext) -> dict:
    return app.resolve(event, context)

execution

sam local invoke --event events/event.json --skip-pull-image
Invoking app.lambda_handler (python3.11)                                                                                                                                                                                                                                                                                 
Requested to skip pulling images ...                                                                                                                                                                                                                                                                                     
                                                                                                                                                                                                                                                                                                                         
Mounting /home/leandro/DEVEL-PYTHON/tmp/exception-different-router/.aws-sam/build/HelloWorldFunction as /var/task:ro,delegated, inside runtime container                                                                                                                                                                 
START RequestId: e408630c-0660-4d24-a3c5-d6e590f6d959 Version: $LATEST
START RequestId: e408630c-0660-4d24-a3c5-d6e590f6d959 Version: $LATEST
END RequestId: e408630c-0660-4d24-a3c5-d6e590f6d959
REPORT RequestId: e408630c-0660-4d24-a3c5-d6e590f6d959	Init Duration: 0.11 ms	Duration: 407.67 ms	Billed Duration: 408 ms	Memory Size: 128 MB	Max Memory Used: 128 MB	
{"statusCode": 400, "body": "Catching Exception, but raising ValueError ", "isBase64Encoded": false, "multiValueHeaders": {}}

I'm adding labels such as “need help” and “revisit” to keep this issue on our radar.

Thank you

@leandrodamascena leandrodamascena added help wanted Could use a second pair of eyes/hands revisit-in-3-months Requires more customers feedback before making or revisiting a decision event_handlers labels Mar 18, 2024
@heitorlessa heitorlessa removed their assignment Jun 9, 2024
@heitorlessa heitorlessa added the triage Pending triage from maintainers label Jun 9, 2024
@leandrodamascena
Copy link
Contributor

Hey @heitorlessa and @aitchnyu!

After a long investigation, we have concluded that this is a fundamental aspect of how Python handles exceptions. In Python, most exceptions inherit from the base class Exception, which means that catching Exception will capture a wide range of exception types, including ValueError, KeyError, TypeError, and RequestValidationError. This behavior is an integral part of the Python language design. Modifying how Powertools for AWS Lambda handles and catches exceptions is not a viable solution at this time, and even if we attempted to change it, we have no idea of the potential side effects.

For cases like this, the recommended approach is to catch more specific exceptions, to better handle and respond to different error types.

I'm closing this issue and please reopen if you find a solution or have an idea how to address this.

Thanks

@github-project-automation github-project-automation bot moved this from Pending customer to Coming soon in Powertools for AWS Lambda (Python) Jun 11, 2024
Copy link
Contributor

⚠️COMMENT VISIBILITY WARNING⚠️

This issue is now closed. Please be mindful that future comments are hard for our team to see.

If you need more assistance, please either tag a team member or open a new issue that references this one.

If you wish to keep having a conversation with other community members under this issue feel free to do so.

@heitorlessa heitorlessa moved this from Coming soon to Shipped in Powertools for AWS Lambda (Python) Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
event_handlers help wanted Could use a second pair of eyes/hands not-a-bug revisit-in-3-months Requires more customers feedback before making or revisiting a decision triage Pending triage from maintainers
Projects
Status: Shipped
Development

No branches or pull requests

3 participants