Skip to content
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

Feature request: HTTP response of an unsuccessful response validation must be a server-side error #5888

Open
amin-farjadi opened this issue Jan 20, 2025 · 11 comments · May be fixed by #6189
Open
Assignees
Labels

Comments

@amin-farjadi
Copy link

Expected Behaviour

When:

  • using APIGatewayResolver, with enable_validation=True and,
  • a route's function fails to validate its return value against its return type

Expected:

  • HTTP response with a server error status code (>499)

Current Behaviour

  • HTTP response has a status code of 422
  • despite the client payload being valid

Code snippet

from typing import Literal
from http import HTTPStatus

from aws_lambda_powertools.event_handler.api_gateway import APIGatewayRestResolver
from pydantic import BaseModel
import pytest


app = APIGatewayRestResolver(enable_validation=True)


class Todo(BaseModel):
    title: str
    status: Literal["Pending", "Done"]


@app.get("/non_empty_string")
def return_non_empty_string() -> Todo:
    return "hello"


@app.get("/non_conforming_dict")
def return_non_conforming_dict() -> Todo:
    return {"title": "fix_response_validation"}


def lambda_handler(event, context):
    return app.resolve(event, context)


# --- Tests below ---


@pytest.fixture()
def event_factory():
    def _factory(path: str):
        return {
            "httpMethod": "GET",
            "path": path,
        }

    yield _factory


@pytest.mark.parametrize(
    "path",
    [
        ("/non_empty_string"),
        ("/non_conforming_dict"),
    ],
    ids=["non_empty_string", "non_conforming_dict"],
)
def test_correct_serialization_failure(path, event_factory):
    """Tests to demonstrate cases when response serialization fails, as expected."""
    event = event_factory(path)

    response = lambda_handler(event, None)
    assert response["statusCode"] == HTTPStatus.UNPROCESSABLE_ENTITY


if __name__ == "__main__":
    pytest.main(["-v", __file__])

Possible Solution

_call_exception_handler method of APIGatewayResolver class returns a 422 response if RequestValidationError is raised. Change response from 422 to a more appropriate response—possibly a 500.

Steps to Reproduce

  • save snippet above to a file
  • install the following dependencies
    • "aws-lambda-powertools==3.4.1"
    • "pydantic==2.10.5"
    • "pytest==8.3.4"
  • run the file

Powertools for AWS Lambda (Python) version

latest, 3.4.1

AWS Lambda function runtime

3.12

Packaging format used

PyPi

Debugging logs

@amin-farjadi amin-farjadi added bug Something isn't working triage Pending triage from maintainers labels Jan 20, 2025
@leandrodamascena
Copy link
Contributor

Hi @amin-farjadi! Thanks for opening this issue! This is actually not a bug, it's what you can expect when the request/response is not validated as expected, so you'll get an HTTP 422. While this is expected behavior, I'm open to accepting a PR implementing a default replacement for 422 for any other HTTP StatusCode when working with data validation. Are you interested in submitting this PR to implement this?

If you want to change this behavior using current features, you can intercept RequestValidationError and return a new HTTP status code. We document it here.

Please take a look in this and see if works for you:

from http import HTTPStatus
from typing import Literal

from aws_lambda_powertools.event_handler.api_gateway import APIGatewayRestResolver
from aws_lambda_powertools.event_handler.api_gateway import RequestValidationError
from aws_lambda_powertools.event_handler import APIGatewayRestResolver, Response, content_types

from pydantic import BaseModel

app = APIGatewayRestResolver(enable_validation=True)

class Todo(BaseModel):
    title: str
    status: Literal["Pending", "Done"]

@app.exception_handler(RequestValidationError)  
def handle_validation_error(ex: RequestValidationError):
    return Response(
        status_code=HTTPStatus.INTERNAL_SERVER_ERROR,
        content_type=content_types.APPLICATION_JSON,
        body={"msg": ex.errors()}
    )


@app.get("/non_empty_string")
def return_non_empty_string() -> Todo:
    return "hello"


@app.get("/non_conforming_dict")
def return_non_conforming_dict() -> Todo:
    return {"title": "fix_response_validation"}


def lambda_handler(event, context):
    return app.resolve(event, context)

@leandrodamascena leandrodamascena added feature-request feature request openapi-schema and removed bug Something isn't working triage Pending triage from maintainers labels Jan 20, 2025
@leandrodamascena leandrodamascena moved this from Triage to Pending customer in Powertools for AWS Lambda (Python) Jan 20, 2025
@leandrodamascena leandrodamascena changed the title Bug: HTTP response of an unsuccessful response validation must be a client-side error Feature request: HTTP response of an unsuccessful response validation must be a client-side error Jan 20, 2025
@amin-farjadi
Copy link
Author

Hi @leandrodamascena, yeah happy to submit a PR. I understand and agree with the 422 http response when client payload cannot be validated. But, just think that if, on the backend we are putting together some data and, somehow those data end up not adhering to the anticipated structure that is a backend (server side) issue and the status code should reflect that.

Am I unsure what the best status code for it would be—a 500 might be good.

@amin-farjadi amin-farjadi changed the title Feature request: HTTP response of an unsuccessful response validation must be a client-side error Feature request: HTTP response of an unsuccessful response validation must be a server-side error Jan 20, 2025
@leandrodamascena
Copy link
Contributor

Hi @leandrodamascena, yeah happy to submit a PR. I understand and agree with the 422 http response when client payload cannot be validated. But, just think that if, on the backend we are putting together some data and, somehow those data end up not adhering to the anticipated structure that is a backend (server side) issue and the status code should reflect that.

I still think this is a client error of a validation/serialization when we talk about request validation. Pydantic models/dataclasses should validate the structure of a given model, not do validation on the data itself, I mean, they should validate if a string is a string, not if a string is a string and exists in the database. If I have an API that expects a JSON body of name: str and age: int and someone sends a payload without age, for example, then this is a malformed payload and should be 400, 422.

In case of talking about response validation, I see cases where you annotate the function to return a float, but for some reason it is returning a string, so it would be an HTTP 500 yes if the customer wants.

Am I unsure what the best status code for it would be—a 500 might be good.

I think we can leave it open for the customers to decide. We keep 422 and the default response, but the customer can decide the best error code for that specific resolver or route.

I'm just not sure about the experience, I don't know if it's the best

Configure at route level

from aws_lambda_powertools.event_handler.api_gateway import APIGatewayRestResolver
app = APIGatewayRestResolver(enable_validation=True)

@app.get("/none", error_validation_http_code=500)
def return_x() -> int:
    return "a"

Configure at resolver level

from aws_lambda_powertools.event_handler.api_gateway import APIGatewayRestResolver
app = APIGatewayRestResolver(enable_validation=True)
app.set_error_validation_http_code(status=500)

@app.get("/none")
def return_x() -> int:
    return "a"

Both

from aws_lambda_powertools.event_handler.api_gateway import APIGatewayRestResolver
app = APIGatewayRestResolver(enable_validation=True)
app.set_error_validation_http_code(status=500)

@app.get("/none", error_validation_http_code=400) # Will return 400
def return_x() -> int:
    return "a"

@app.get("/none") # Will return 500
def return_y() -> int:
    return "a"

What do you think?

@amin-farjadi
Copy link
Author

amin-farjadi commented Jan 20, 2025

Problem Statement

Sorry I might have been vague with my explanation.

Say I have designed an API endpoint that has a POST method and accepts payload of type PayloadModel. If I POST a payload other than PayloadModel, the endpoint responds with a 422—client error. ✅

For simplicity say that our endpoint is designed to just act as a proxy to another SaaS provider. The SaaS provider's response body is of type ResponseModel which, would be the body that our endpoint will be returning.

So, we have:

@app.post("/endpoint")
def proxy_saas(payload: PayloadModel) -> ResponseModel:
    return requests.post("www.saas_provider.example", json=payload.model_dump()).json() # expected to be of type ResponseModel

The issue then arises if the SaaS provider has a bad API and, out of nowhere, returns a None. I expect the validation of my APIGatewayResolver to catch that and return a server-side error to the consumer of my API.

Configuration

The above configuration looks like a great feature. However, I personally like to distinguish between the http status codes resulted by RequestValidationError and SerializationError.

@leandrodamascena
Copy link
Contributor

Hi @amin-farjadi, I see! Thanks for clarifying.

We only have a problem if we replace RequestValidationError with SerializationError in the case of responses. Currently many customers are already relying on RequestValidationError to catch request/response errors and that would be a breaking change that we can't do right now.

I agree with your approach and it's a good improvement, but we need to find a way to keep the default behavior as RequestValidationError and if the customer wants - maybe by setting it per route - configure this new behavior.

WDYT?

@amin-farjadi
Copy link
Author

amin-farjadi commented Jan 21, 2025

RequestValidationError and SerializationError are kept separate and well distinguished in the code but, they both surface as an HTTP.UNPROCESSABLE_CONTENT.

I agree that changing the SerializationError to surface as a HTTP.INTERNAL_SERVER_ERROR, for instance, would be a breaking change and thus, must be implemented as a configuration option (something like what you suggested) at least for version 4 of powertools.

@leandrodamascena
Copy link
Contributor

RequestValidationError and SerializationError are kept separate and well distinguished in the code but, they both surface as an HTTP.UNPROCESSABLE_CONTENT.

Yes, but SerializationError is intended to be used when we cannot serialize the object because we don't know how to serialize it. For example SQLAlchemy models and others, where you need to bring your own custom serializer. What you are trying to achieve here is that you have a function annotation with a Pydantic model, but you are returning a string or None for example, in this case we will not raise SerializationError, but rather RequestValidationError.

I agree that changing the SerializationError to surface as a HTTP.INTERNAL_SERVER_ERROR, for instance, would be a breaking change and thus, must be implemented as a configuration option (something like what you suggested) at least for version 4 of powertools.

Yes, lets make this an optional behavior and change it to the default behavior in Powertools v4.

Please let me know if you need any help to send this PR. We can work together to get it merged.

@anafalcao
Copy link
Contributor

Thank you for opening this issue. Since we haven't heard back, we'll be closing it for now. Please don't hesitate to open a new one or start a new discussion if you have any updates or further thoughts. We appreciate your input!

@anafalcao anafalcao closed this as not planned Won't fix, can't repro, duplicate, stale Feb 17, 2025
@github-project-automation github-project-automation bot moved this from Pending customer to Coming soon in Powertools for AWS Lambda (Python) Feb 17, 2025
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.

@leandrodamascena leandrodamascena moved this from Coming soon to Closed in Powertools for AWS Lambda (Python) Feb 17, 2025
@amin-farjadi
Copy link
Author

Hi @leandrodamascena, @anafalcao

Found some time and happy to implement this feature. Please open this issue if you could.

@leandrodamascena
Copy link
Contributor

Hey @amin-farjadi! Sure, go ahead! Please let me know if you need any help.

Reopening this issue!

@leandrodamascena leandrodamascena moved this from Pending review to Working on it in Powertools for AWS Lambda (Python) Feb 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Working on it
3 participants