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

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

Open
philiptzou opened this issue Aug 20, 2024 · 11 comments
Open
Assignees
Labels
documentation Improvements or additions to documentation event_sources Event Source Data Class utility help wanted Could use a second pair of eyes/hands not-a-bug revisit Maintainer to provide update or revisit prioritization in the next cycle

Comments

@philiptzou
Copy link

philiptzou commented Aug 20, 2024

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.

def get_field_info_and_type_annotation(annotation, value, is_path_param: bool) -> Tuple[Optional[FieldInfo], Any]:
"""
Get the FieldInfo and type annotation from an annotation and value.
"""
field_info: Optional[FieldInfo] = None
type_annotation: Any = Any
if annotation is not inspect.Signature.empty:
# If the annotation is an Annotated type, we need to extract the type annotation and the FieldInfo
if get_origin(annotation) is Annotated:
field_info, type_annotation = get_field_info_annotated_type(annotation, value, is_path_param)
# If the annotation is a Response type, we recursively call this function with the inner type
elif get_origin(annotation) is Response:
field_info, type_annotation = get_field_info_response_type(annotation, value)
# If the annotation is not an Annotated type, we use it as the type annotation
else:
type_annotation = annotation
return field_info, type_annotation

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

@philiptzou philiptzou added bug Something isn't working triage Pending triage from maintainers labels Aug 20, 2024
Copy link

boring-cyborg bot commented Aug 20, 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

@leandrodamascena
Copy link
Contributor

leandrodamascena commented Aug 21, 2024

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 leandrodamascena self-assigned this Aug 21, 2024
@leandrodamascena leandrodamascena added documentation Improvements or additions to documentation not-a-bug event_sources Event Source Data Class utility and removed bug Something isn't working triage Pending triage from maintainers labels Aug 21, 2024
@leandrodamascena
Copy link
Contributor

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

Thanks

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.

@philiptzou
Copy link
Author

@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
Copy link
Contributor

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
Copy link
Author

@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
Copy link
Contributor

@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.

@github-project-automation github-project-automation bot moved this from Coming soon to Pending review in Powertools for AWS Lambda (Python) Aug 23, 2024
@leandrodamascena leandrodamascena added the revisit Maintainer to provide update or revisit prioritization in the next cycle label Aug 26, 2024
@leandrodamascena leandrodamascena added the help wanted Could use a second pair of eyes/hands label Oct 5, 2024
@philiptzou
Copy link
Author

@leandrodamascena Sorry I missed the last message you posted after the re-opening. Unfortunately the repository is private and I can not share it. However, this is basically the workaround I took (please compare with the original code snippet):

from pydantic import BaseModel
from aws_lambda_powertools.event_handler import Response  # <-- change 1
from aws_lambda_powertools.event_handler import ALBResolver

app = ALBResolver(enable_validation=True)

class Result(BaseModel):
    payload: str

@app.post('/foobar')
def foobar() -> Response[Result]:    # <-- change 2
    return app._to_response(  # <-- change 3
      (Result(payload='foobar'), 201)
    )

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

@philiptzou
Copy link
Author

Oh I also just remembered that I can submit a PR. If you can help working on the test cases I'll do it soon.

@leandrodamascena
Copy link
Contributor

Hey @philiptzou! Yeah, please submit the PR and we can work together in the tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation event_sources Event Source Data Class utility help wanted Could use a second pair of eyes/hands not-a-bug revisit Maintainer to provide update or revisit prioritization in the next cycle
Projects
Status: Pending review
Development

No branches or pull requests

2 participants