aws-powertools / powertools-lambda-python

A developer toolkit to implement Serverless best practices and increase developer velocity.
https://docs.powertools.aws.dev/lambda/python/latest/
MIT No Attribution
2.83k stars 391 forks source link

Bug: get_field_info_and_type_annotation doesn't respect tuple return types #5034

Open philiptzou opened 1 month ago

philiptzou commented 1 month ago

Expected Behaviour

The get_field_info_and_type_annotation function should respect tuple response types since it is supported since 2.7.0. See #1845 and #1853.

Current Behaviour

Validation error (HTTP 422) happens when the endpoint function returning type annotation is a tuple, if we had enable_validation=True for OpenAPI validation.

Code snippet

from pydantic import BaseModel
from aws_lambda_powertools.event_handler import ALBResolver

app = ALBResolver(enable_validation=True)

class Result(BaseModel):
    payload: str

@app.post('/foobar')
def foobar() -> tuple[Result, int]:
    return Result(payload='foobar'), 201

def test_foobar():
    response = app.resolve({
        'path': '/foobar',
        'httpMethod': 'POST',
        'queryStringParameters': {},
        'headers': {}
    })
    assert response['statusCode'] == 201

Instead of 201 Created, the endpoint currently returns 422 error with following body:

{"statusCode":422,"detail":[{"loc":["response"],"type":"tuple_type"}]}

Possible Solution

Update here to support tuple type for a response. In addition, distinguishing is_response_param is necessary for avoiding changing behaviors for other types of annotations.

https://github.com/aws-powertools/powertools-lambda-python/blob/8251bb09b3c95401faafd45d09cfe9072aed8765/aws_lambda_powertools/event_handler/openapi/params.py#L969-L987

Steps to Reproduce

Please see above code snippet.

Powertools for AWS Lambda (Python) version

latest

AWS Lambda function runtime

3.11

Packaging format used

PyPi

Debugging logs

No response

boring-cyborg[bot] commented 1 month ago

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

leandrodamascena commented 1 month ago

Hey @philiptzou! Thanks for opening this discussion because I see room to improve our documentation.

Actually this is not a bug, this is an expected behavior but I agree that it's not very clear in our documentation. Before agreeing with next steps, let me explain why this is working as expected.

Prior to PR #1853, customers who wanted to set a specific response code for methods must respond to a method using the Response object or a dict like {"statusCode": 201...}. In PR #1853, we introduced a concept used in other frameworks like Flask, where you can simply use return body, status_code, and we transform it into a Response object. So, based on that, your function will always returns a JSON and that's why data validation is complaining that you are setting the return type to tuple.

Your code should be something like this:

@app.get('/foobar')
def foobar() -> dict:
    return Result(payload='foobar'), 201

While I don't consider this a bug, I do see that we need to improve our documentation to make this clearer and prevent customers from interpreting it as an error.

I'll add the documentation label to this issue and think about a solution. In the meantime, please test it with the code above and see if it works.

leandrodamascena commented 1 month ago

Hey @philiptzou! Please reopen if you have any additional questions.

Thanks

github-actions[bot] commented 1 month ago

⚠️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.

philiptzou commented 1 month ago

@leandrodamascena

Thanks. But for project that have strict typing lint enabled, this solution will trigger linter error by tools such as mypy, since the returning value doesn't match the expected returning type.

I'll re-open this issue.

leandrodamascena commented 1 month ago

Hi @philiptzou, thanks for replying. although we try to make the project as strictly typed as possible, unfortunately we cannot guarantee that we are fully typed, so things like this may happen somewhere in the project.

I don't know if we have a solution for this, since we need to convert this into a json object and this not return tuple. If you have a solution for this, PR are welcome to fix. Otherwise I think you'll need to mark this line as ignore or change the return to use the Response object instead of this tuple.

philiptzou commented 1 month ago

@leandrodamascena My current work around is still returning tuple but wrap it in app._to_response(). This way I can define the returning type as Response[MyDataSchema]. I do have some thought on the solution but just kinda lazy of writing test cases. I may eventually submit a PR.

leandrodamascena commented 1 month ago

@philiptzou Is it possible to share a branch with this code? I can check it and might incorporate in our codebase + tests.

Reopening this issue.