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

feat(event_handler): add support for additional response models in OpenAPI schema #3591

Merged
merged 9 commits into from
Jan 16, 2024

Conversation

rubenfonseca
Copy link
Contributor

Issue number: #3542

Summary

Changes

Please provide a summary of what's being changed

This PR adds support for additional response models on the OpenAPI specification.

User experience

Please share what the user experience looks like before and after this change

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.

@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 4, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jan 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (37a84c9) 95.50% compared to head (d2ceb2c) 95.51%.
Report is 2 commits behind head on develop.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3591      +/-   ##
===========================================
+ Coverage    95.50%   95.51%   +0.01%     
===========================================
  Files          211      211              
  Lines         9825     9860      +35     
  Branches      1792     1802      +10     
===========================================
+ Hits          9383     9418      +35     
  Misses         329      329              
  Partials       113      113              

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

@rubenfonseca
Copy link
Contributor Author

Note: still need to update documentation before it's ready for review

@ran-isenberg
Copy link
Contributor

ran-isenberg commented Jan 6, 2024

@rubenfonseca I dunno if it helps or not but i was able to add custom responses with pydantic models. See this example:
https://github.com/ran-isenberg/aws-lambda-handler-cookbook/pull/784/files#diff-03a550a2c5158da1b1459e42e5799bdfe3809d2563dfb16014d3ea3bcd7cd043R20

501: {
            'description': 'Internal server error',
            'content': {'application/json': {'schema': InternalServerErrorOutput.model_json_schema()}},
        },

@leandrodamascena
Copy link
Contributor

@rubenfonseca I dunno if it helps or not but i was able to add custom responses with pydantic models. See this example: ran-isenberg/aws-lambda-handler-cookbook#784 (files)

501: {
            'description': 'Internal server error',
            'content': {'application/json': {'schema': InternalServerErrorOutput.model_json_schema()}},
        },

Hi @ran-isenberg! You can currently use a Pydantic model by dumping it as JSON. But we don't want to force customers to serialize their models manually and need to add more code and stuff, we want to provide a seamless experience serializing/deserializing Pydantic models, Dataclass, scalar types, Python classes, and others. So, after merging this PR you can just pass your Model and we serialize it like this:

@app.get(
        "/",
        responses={
            200: {"description": "200 Response", "content": {"application/json": {"model": User}}},
            202: {"description": "202 Response", "content": {"application/json": {"model": Order}}},
        },
    )
def handler() -> Response[Union[User, Order]]:
    if randbelow(2) > 0:
        return Response(status_code=200, body=User())
    else:
        return Response(status_code=202, body=Order())

Thanks

@ran-isenberg
Copy link
Contributor

@rubenfonseca I dunno if it helps or not but i was able to add custom responses with pydantic models. See this example: ran-isenberg/aws-lambda-handler-cookbook#784 (files)

501: {
            'description': 'Internal server error',
            'content': {'application/json': {'schema': InternalServerErrorOutput.model_json_schema()}},
        },

Hi @ran-isenberg! You can currently use a Pydantic model by dumping it as JSON. But we don't want to force customers to serialize their models manually and need to add more code and stuff, we want to provide a seamless experience serializing/deserializing Pydantic models, Dataclass, scalar types, Python classes, and others. So, after merging this PR you can just pass your Model and we serialize it like this:

@app.get(
        "/",
        responses={
            200: {"description": "200 Response", "content": {"application/json": {"model": User}}},
            202: {"description": "202 Response", "content": {"application/json": {"model": Order}}},
        },
    )
def handler() -> Response[Union[User, Order]]:
    if randbelow(2) > 0:
        return Response(status_code=200, body=User())
    else:
        return Response(status_code=202, body=Order())

Thanks

Cool , it's a cleaner experience.
Did you try it with a more complicated Pydantic model? I tried it and it failed.
I modeled the 422 response model and pydantic builds you two schemas, where one refrences the other but it cant find the other one, so it throws an error and shows it as empty dict.
can you try this use case @leandrodamascena ?

class Error(BaseModel):
  x: str


class HttpErrors(BaseModel):
  details: List[Error]

so HttpErrors schema will point a $ref to Error schema which does not exist

@rubenfonseca rubenfonseca force-pushed the rf/add-openapi-additional-models branch from 901daf3 to 04adc76 Compare January 8, 2024 15:18
@rubenfonseca rubenfonseca marked this pull request as ready for review January 8, 2024 15:19
@rubenfonseca rubenfonseca requested a review from a team as a code owner January 8, 2024 15:19
@leandrodamascena
Copy link
Contributor

Looking at this now.

@leandrodamascena
Copy link
Contributor

Cool , it's a cleaner experience. Did you try it with a more complicated Pydantic model? I tried it and it failed. I modeled the 422 response model and pydantic builds you two schemas, where one refrences the other but it cant find the other one, so it throws an error and shows it as empty dict. can you try this use case @leandrodamascena ?

class Error(BaseModel):
  x: str


class HttpErrors(BaseModel):
  details: List[Error]

so HttpErrors schema will point a $ref to Error schema which does not exist

Hi @ran-isenberg! I don't see this issue in this PR. It may happen in your local code, but when we merge this PR you will no longer need to "hack" to have Pydantic models in your responses.

from typing import List

import requests
from pydantic import BaseModel, Field

from aws_lambda_powertools.event_handler import APIGatewayRestResolver
from aws_lambda_powertools.utilities.typing import LambdaContext

app = APIGatewayRestResolver(enable_validation=True)
app.enable_swagger(path="/swagger")


class Error(BaseModel):
  x: str


class HttpErrors(BaseModel):
  details: List[Error]

class Todo(BaseModel):
    userId: int
    id_: int = Field(alias="id")
    title: str
    completed: bool


@app.get("/hello", responses={
            200: {"description": "200 Response"},
            500: {"description": "500 Response", "content": {"application/json": {"model": HttpErrors}}},
        })
def get_todos_by_email(email: str) -> List[Todo]:
    todos = requests.get(f"https://jsonplaceholder.typicode.com/todos?email={email}")
    todos.raise_for_status()

    return todos.json()


def lambda_handler(event: dict, context: LambdaContext) -> dict:
    print(app.get_openapi_json_schema())
    return app.resolve(event, context)

SWAGGER

image

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.

Hello @rubenfonseca! Amazing work once again 😄

Please, can you update the documentation and add a new example?
image

@ran-isenberg
Copy link
Contributor

Cool , it's a cleaner experience. Did you try it with a more complicated Pydantic model? I tried it and it failed. I modeled the 422 response model and pydantic builds you two schemas, where one refrences the other but it cant find the other one, so it throws an error and shows it as empty dict. can you try this use case @leandrodamascena ?

class Error(BaseModel):
  x: str


class HttpErrors(BaseModel):
  details: List[Error]

so HttpErrors schema will point a $ref to Error schema which does not exist

Hi @ran-isenberg! I don't see this issue in this PR. It may happen in your local code, but when we merge this PR you will no longer need to "hack" to have Pydantic models in your responses.

from typing import List

import requests
from pydantic import BaseModel, Field

from aws_lambda_powertools.event_handler import APIGatewayRestResolver
from aws_lambda_powertools.utilities.typing import LambdaContext

app = APIGatewayRestResolver(enable_validation=True)
app.enable_swagger(path="/swagger")


class Error(BaseModel):
  x: str


class HttpErrors(BaseModel):
  details: List[Error]

class Todo(BaseModel):
    userId: int
    id_: int = Field(alias="id")
    title: str
    completed: bool


@app.get("/hello", responses={
            200: {"description": "200 Response"},
            500: {"description": "500 Response", "content": {"application/json": {"model": HttpErrors}}},
        })
def get_todos_by_email(email: str) -> List[Todo]:
    todos = requests.get(f"https://jsonplaceholder.typicode.com/todos?email={email}")
    todos.raise_for_status()

    return todos.json()


def lambda_handler(event: dict, context: LambdaContext) -> dict:
    print(app.get_openapi_json_schema())
    return app.resolve(event, context)

SWAGGER

image

nice! looking forward to adding this to the cookbook and blog post very soon!

@boring-cyborg boring-cyborg bot added the documentation Improvements or additions to documentation label Jan 16, 2024
Copy link

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

5 New issues
0 Security Hotspots
No data about Coverage
3.4% Duplication on New Code

See analysis details on SonarCloud

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.

Amazing work @rubenfonseca!

@leandrodamascena leandrodamascena merged commit 1409499 into develop Jan 16, 2024
16 checks passed
@leandrodamascena leandrodamascena deleted the rf/add-openapi-additional-models branch January 16, 2024 16:36
@leandrodamascena leandrodamascena changed the title feat(event_handler): add support for additional response models feat(event_handler): add support for additional response models in OpenAPI schema Jan 16, 2024
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_handlers size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: ability to define additional response models in OpenAPI
4 participants