Skip to content

Conversation

philiptzou
Copy link
Contributor

Issue number: #5034

Summary

Changes

This PR fixes the issue that OpenAPI validation failed to validate two-element tuple returns, which is allowed by the APIGateway.

User experience

Below test fails before this PR but passes after this PR:

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': {}
    }, None)
    assert response['statusCode'] == 201

Checklist

If your change doesn't seem to apply, please leave them unchecked.

Is this a breaking change?

RFC issue number:

Checklist:

  • Migration process documented
  • Implement warnings (if it can live side by side)

Acknowledgment

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

@philiptzou philiptzou requested a review from a team February 3, 2025 01:39
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 3, 2025
Copy link

boring-cyborg bot commented Feb 3, 2025

Thanks a lot for your first contribution! Please check out our contributing guidelines and don't hesitate to ask whatever you need.
In the meantime, check out the #python channel on our Powertools for AWS Lambda Discord: Invite link

Copy link
Contributor

@leandrodamascena leandrodamascena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @philiptzou! Thanks a lot for submitting this PR. 🚀

I see we have some problems with type checking that is breaking the CI and we need to address them before I make a final review.

@leandrodamascena leandrodamascena changed the title fix(open-api): Add support for the usage of tuple returning type feat(openapi): enhance support for tuple return type validation Feb 3, 2025
@github-actions github-actions bot added feature New feature or functionality and removed internal Maintenance changes labels Feb 3, 2025
@philiptzou philiptzou force-pushed the openapi-with-tuple-returns branch from 6a62cb1 to 536f4cd Compare February 3, 2025 20:36
@boring-cyborg boring-cyborg bot added the internal Maintenance changes label Feb 3, 2025
@github-actions github-actions bot removed the internal Maintenance changes label Feb 3, 2025
Copy link

codecov bot commented Feb 3, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.24%. Comparing base (d12babf) to head (00d110d).
Report is 1 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #5997   +/-   ##
========================================
  Coverage    96.24%   96.24%           
========================================
  Files          234      234           
  Lines        11017    11024    +7     
  Branches       798      800    +2     
========================================
+ Hits         10603    10610    +7     
  Misses         327      327           
  Partials        87       87           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@leandrodamascena leandrodamascena self-requested a review February 3, 2025 21:43
@boring-cyborg boring-cyborg bot added the internal Maintenance changes label Feb 3, 2025
Copy link

sonarqubecloud bot commented Feb 3, 2025

@github-actions github-actions bot removed the internal Maintenance changes label Feb 3, 2025
Copy link
Contributor

@leandrodamascena leandrodamascena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @philiptzou! Thanks for working on this PR and addressing the feedback!

I have a feeling that this PR can breaks the validation for people using dict as return. But I think this is really a bug that we need to fix.

e.g

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

APPROVED!

@leandrodamascena leandrodamascena merged commit 5732a57 into aws-powertools:develop Feb 3, 2025
12 checks passed
Copy link

boring-cyborg bot commented Feb 3, 2025

Awesome work, congrats on your first merged pull request and thank you for helping improve everyone's experience!

@philiptzou
Copy link
Contributor Author

Hey @philiptzou! Thanks for working on this PR and addressing the feedback!

I have a feeling that this PR can breaks the validation for people using dict as return. But I think this is really a bug that we need to fix.

e.g

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

APPROVED!

Well, I don't think this PR will break dict as return since it still falls in the else branch. This example still work, but it irritates mypy anyway.

sinofseven pushed a commit to sinofseven/powertools-lambda-python-my-extend that referenced this pull request Feb 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
event_handlers feature New feature or functionality size/M Denotes a PR that changes 30-99 lines, ignoring generated files. tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: get_field_info_and_type_annotation doesn't respect tuple return types
2 participants