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: Event Handler - enhance route functions - to carry event & context #1955

Closed
2 tasks done
Muthuveerappanv opened this issue Feb 26, 2023 · 17 comments
Closed
2 tasks done
Labels

Comments

@Muthuveerappanv
Copy link

Use case

Event Handler Router - Imrprovements

Currently the route endpoints of event_handler don't take any function parameters other than url path variables as positional parameters

app = APIGatewayRestResolver()

@app.get("/todos/<todo_id>")
@tracer.capture_method
def get_todo_by_id(todo_id: str):

This makes it difficult for introducing many use-cases within the route as the body, query-params can be accessed from only the app variable.

parser utility is a great tool but cant be used readily with routes. For different endpoints in event_handler the body might be different, in which case the event_parser doesn't help much as it can be applied only at the handler level.
A router_parser utility would greatly help at the route function level, but due to the limitation mentioned about in the event_handler the route functions aren't extendible.

Solution/User Experience

Passing the event and context to the routes as named arguments, will help overcome the limitation

User Experience

@app.route("/todos", method=["PUT", "POST"])
@tracer.capture_method
def create_todo(event: APIGatewayProxyEvent, context: LambdaContext):
    todo_data: dict = event.json_body  # deserialize json str to dict
    todo: Response = requests.post("https://jsonplaceholder.typicode.com/todos", data=todo_data)
    todo.raise_for_status()

    return {"todo": todo.json()}

Change Proposed

app.resolve invokes the route as shown here, requesting it to be changed to below

def _call_route(self, route: Route, args: Dict[str, str]) -> ResponseBuilder:
        """Actually call the matching route with any provided keyword arguments."""
        try:
             kwargs = {"event": self.current_event, "context": self.context}
             return ResponseBuilder(self._to_response(route.func(**args, **kwargs)), route)

This change will pave to route_parser like utilities that will help take away a lot of boiler plate implementation of pydantic parser. It will also enable routes to take full advantage of the parser utility, which is not the case today as parser decorators can only be applied at the handler

proposal for the route_parser would roughly look like this

@app.post("/todo")
@route_parser(model=Todo, envelope=envelopers.ApiGatewayEnvelope) # pydantic
def create_todo(data: Todo):
    todo: Response = requests.post("https://jsonplaceholder.typicode.com/todos", data=data.dict())

    # Returns the created todo object, with a HTTP 201 Created status
    return {"todo": todo.json()}, 201

Alternative solutions

No response

Acknowledgment

@Muthuveerappanv Muthuveerappanv added feature-request feature request triage Pending triage from maintainers labels Feb 26, 2023
@boring-cyborg
Copy link

boring-cyborg bot commented Feb 26, 2023

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 AWS Lambda Powertools Discord: Invite link

@Muthuveerappanv
Copy link
Author

Draft PR - #1956

@heitorlessa
Copy link
Contributor

hey @Muthuveerappanv thanks a lot for having a great level of detail and for proactively sending a draft PR - I'll make one major comment in the PR but I'm happy with it as it's a no brainier.

For route_parser idea, I think we can do something more elaborate so we can handle this older and more complex ask albeit related: #1236. I'd appreciate a RFC on this so get the ball rolling and gather community feedback.

@heitorlessa heitorlessa removed the triage Pending triage from maintainers label Mar 1, 2023
@heitorlessa heitorlessa self-assigned this Mar 1, 2023
@heitorlessa
Copy link
Contributor

heitorlessa commented Mar 1, 2023

Actually, I take it back. I think I read too quickly when glancing at the kwargs aspect, thinking you'd want to pass arbitrary arguments and named arguments in order to support decorator stacking.

Instead - please correct me - you're looking to explicitly pass event and context to the function being decorated, so you could build an extension to receive the event, parse using Pydantic, access query strings and anything related to the incoming request, so that the decorated view would receive the Model already parsed.

If that's the case, there's been discussions on adding support for Middlewares in Event Handler - #953, the RFC #1566, a related use case #1236, and a similar feature in Chalice that we collaborated to integrate Powertools. I haven't managed to put much thought into this yet, but reviewing feedbacks one more time we're looking for:

  • Ability to define a middleware to handle before and/or after requests, e.g. authorizations, request enrichment, validation, etc.
  • Ability to include reusable middlewares defined outside the project, e.g., cross-cutting concerns across the company
  • Ability to easily validate incoming requests with a schema - e.g., JSON Schema, Pydantic Models
  • Support for FastAPI-like experience through parameter injection via type annotation - reflection has a cost and was the reason we didn't go with that approach first (can be reconsidered in a specialized Resolver)
  • Support for FastAPI-like API docs auto-generation, since API Gateway isn't catering for latest JSON Schema Drafts and innovations like Pydantic

Suggested actions from here:

  • Drop the PR and let's focus instead on adding support for middlewares - welcome any UX ideas or in addition to the ones suggested here: Feature request: Add support for middlewares in event handler #953 (comment)
  • Think of a simple UX to validate (JSON Schema) and/or parse a model to reduce boilerplate - alternatively, this could be a set of built-in reusable and extensible middlewares provided by Powertools already
  • Add support for global middlewares that should always be called before/after requests - reduce boilerplate for cross-cutting concerns where one has to annotate several routes

@Muthuveerappanv
Copy link
Author

Muthuveerappanv commented Mar 1, 2023

Actually, I take it back. I think I read too quickly when glancing at the kwargs aspect, thinking you'd want to pass arbitrary arguments and named arguments in order to support decorator stacking.

Instead - please correct me - you're looking to explicitly pass event and context to the function being decorated, so you could build an extension to receive the event, parse using Pydantic, access query strings and anything related to the incoming request, so that the decorated view would receive the Model already parsed.

If that's the case, there's been discussions on adding support for Middlewares in Event Handler - #953, the RFC #1566, a related use case #1236, and a similar feature in Chalice that we collaborated to integrate Powertools. I haven't managed to put much thought into this yet, but reviewing feedbacks one more time we're looking for:

  • Ability to define a middleware to handle before and/or after requests, e.g. authorizations, request enrichment, validation, etc.

yes this will be a good option. A middleware factory for api resolvers applicable to its routes

  • Ability to include reusable middlewares defined outside the project, e.g., cross-cutting concerns across the company

thumbs up to this 👍🏼

  • Ability to easily validate incoming requests with a schema - e.g., JSON Schema, Pydantic Models

event_parser and validation can be extended to support this

  • Support for FastAPI-like experience through parameter injection via type annotation - reflection has a cost and was the reason we didn't go with that approach first (can be reconsidered in a specialized Resolver)

can we go for a kwargs in this case (body, headers, query_params) to start with then probably think about reflection in next phase ?

  • Support for FastAPI-like API docs auto-generation, since API Gateway isn't catering for latest JSON Schema Drafts and innovations like Pydantic

swaggers docs, Redoc, generators are good to have, once we have the middleware factory & a better route UX, we can come back to it ?

Suggested actions from here:

there are lot of issues left open-ended, so was not sure which one to piggyback on. Also the draft PR I made for local testing purpose and used it show what I was trying to explain in the issue, I can close it.

  • Think of a simple UX to validate (JSON Schema) and/or parse a model to reduce boilerplate - alternatively, this could be a set of built-in reusable and extensible middlewares provided by Powertools already

middleware factory makes more sense, we had a similar challenge with typescript (aws-powertools/powertools-lambda-typescript#413) too. Middleware factory for event_handler (api_resolver routes) was the solution that came to our mind, but we (@karthikeyanjp) didn't pursue it considering python didn't have one (parity) 😀

  • Add support for global middlewares that should always be called before/after requests - reduce boilerplate for cross-cutting concerns where one has to annotate several routes

yeah makes sense!

@heitorlessa
Copy link
Contributor

(on a call, reading fully shortly).. btw we have a middleware factory (our first feature in Python) -> https://awslabs.github.io/aws-lambda-powertools-python/2.9.0/utilities/middleware_factory/

@heitorlessa
Copy link
Contributor

Great to hear that we're capturing the key use cases - I agree it's scattered and this is the first attempt in aggregating them. I closed the RFC, and pinned the other two issues: Middleware support (#953), API auto-doc from routes (#1236).

Some answers from your questions and comments - as it's longer than I'd imagined, I'll create a new comment with a suggested order on what next steps looks like off this conversation (thank you!!)

Ability to easily validate incoming requests with a schema - e.g., JSON Schema, Pydantic Models

event_parser and validation can be extended to support this

I'd like to avoid making yet another decorator in the routes, as one of the feedbacks we get from non-developers is that they sometimes get confused with the order (decorator stacking). Instead, it'd be great to offer an UX where you might (A) have an additional parameter in the route definition to validate input/output, or (B) built-in middlewares customers can plug-n-play.

I like the latter as it's one less set of parameters to remember, and Event Handler would take care of passing a copy of the App instance to the middleware so they can access the immutable event, body, query strings, params, Lambda context, etc. They could also build one themselves since both Parser and Validator have standalone functions.

Rough example

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

app = APIGatewayRestResolver()

# Factory that 
# 1.  Acts as a thin wrapper for the Validation utility as a thin wrapper
# 2. Returns an Event Handler middleware that can validate the body, querystring, or params since it has access to the App instance
schema_validator = json_schema_validator(inbound_schema=SCHEMA_IN, outbound_schema=SCHEMA_OUT, query_strings=True, payload=True, **other_options)

@app.get("/todos", middlewares=[schema_validator])
@tracer.capture_method
def get_todos():
    ...

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

Support for FastAPI-like experience through parameter injection via type annotation - reflection has a cost and was the reason we didn't go with that approach first (can be reconsidered in a specialized Resolver)

can we go for a kwargs in this case (body, headers, query_params) to start with then probably think about reflection in next phase ?

The part I'm trying to wrap my head around on kwargs is - when would this be more appropriate vs accessing app instance? I might be missing something here.

As in, if we add middleware support, do we still need it? Is it primarily convenience to not have to type app.current_event.json_body, app.current_event.query_string_parameters?

Support for FastAPI-like API docs auto-generation, since API Gateway isn't catering for latest JSON Schema Drafts and innovations like Pydantic

swaggers docs, Redoc, generators are good to have, once we have the middleware factory & a better route UX, we can come back to it ?

Yes! Middleware is more important as it unlocks a range of use cases and areas we need to get better at before coming to this.

While I see the pattern emerging for lift-n-shift and onboarding developers to Serverless quickly, I'm still on the fence about this feature if I'm honest due to the trade-offs on having a single function for this to be worthy (I could be wrong).

@heitorlessa
Copy link
Contributor

Suggested next steps:

  1. Let's revive the middleware support feature request (Feature request: Add support for middlewares in event handler #953) with suggestions on the following areas:

Writing a middleware

  • What should the signature looks like? e.g., def my_middleware(app, get_response, **kwargs) -> Response
  • What does short-circuiting should look like? e.g., if schema validation fail we should return early and not call any subsequent registered middleware
  • Should we allow mutability on query strings/headers/body? e.g., app.current_event properties are immutable today for various reasons, but we can have setters if needed.

Registering a middleware

  • Per route: Should we add a single parameter, namely middlewares=[]?
  • Per route: Should we add a before_request and after_request parameters? e.g., advantage being simpler to write/read middlewares
    • Or keep it simple with middlewares=[] only? in the future we could have a class-based resolver to not over-complicate things.
  • Per request: Given per route and global middlewares were registered (App+Routers), which order do we resolve?
  1. Let's create a POC after we have these questions answered since feat(middleware_factory): support router #954 hasn't been updated for a year.
  2. We revisit the kwargs - in case I missed anything
  3. We revive the issue Feature request: Add support for API doc auto-gen (SwaggerUI/OpenAPI Specification) from routes #1236 on API doc auto-gen, as this will take a while to get it right and I'm concerned about performance

@heitorlessa heitorlessa added need-customer-feedback Requires more customers feedback before making or revisiting a decision event_handlers labels Mar 1, 2023
@Muthuveerappanv
Copy link
Author

Muthuveerappanv commented Mar 11, 2023

Suggested next steps:

  1. Let's revive the middleware support feature request (Feature request: Add support for middlewares in event handler #953) with suggestions on the following areas:

Writing a middleware

What should the signature looks like? e.g., def my_middleware(app, get_response, **kwargs) -> Response

  • the example syntax looks good for a def based approach
  • one other suggestion would be to have a base decorator similar to @lambda_handler_decorator. something called @event_handler_middleware and the scope of this decorator would be just to apply on top of the event_handler routes

What does short-circuiting should look like? e.g., if schema validation fail we should return early and not call any subsequent registered middleware

  • default behavior should be to throw an exception when the middleware's functionality fails
  • an optional override flag would help if exception has to be ignored.

Should we allow mutability on query strings/headers/body? e.g., app.current_event properties are immutable today for various reasons, but we can have setters if needed.

  • we can leave app.current_event immutable as long as we provide other options for mutations
  • this can be done by providing certain reserved kwargs like (body, headers, query_params)

Registering a middleware

Per route: Should we add a single parameter, namely middlewares=[]?

  • yes a list of middlewares. usually middlewares are executed in the stack order, so this list will be self intuitive.
  • middleware decorators on top of the route method can be an option as well.

Per route: Should we add a before_request and after_request parameters? e.g., advantage being simpler to write/read middlewares

Or keep it simple with middlewares=[] only? in the future we could have a class-based resolver to not over-complicate things.

  • I prefer keeping it simple without before or after

Per request: Given per route and global middlewares were registered (App+Routers), which order do we resolve?

  • global first, routes next (this should be the tenet for middleware, this will make the UX simple)
  1. Let's create a POC after we have these questions answered since feat(middleware_factory): support router #954 hasn't been updated for a year.

  2. We revisit the kwargs - in case I missed anything

  3. We revive the issue Feature request: Add support for API doc auto-gen (SwaggerUI/OpenAPI Specification) from routes #1236 on API doc auto-gen, as this will take a while to get it right and I'm concerned about performance

@heitorlessa
Copy link
Contributor

These are great @Muthuveerappanv - agree with all of them. We're good to go for an implementation now - 1 PR to implement a decorator factory in Middleware Factory utility, and 1 PR to implement Middleware support in Event Handler (before/after, etc.)

Adding a help wanted label now to signal we welcome contributions - pinging you separately as I have ideas on how we can prioritize this faster and work more closely with your team for a fast feedback loop to get this ready for April the latest.

@heitorlessa
Copy link
Contributor

Prioritizing it for our next iteration May 8th-May 19th. We'll check how much of a perf penalty this adds in practice. If it doesn't we'll work and launch within that timeframe :)

Otherwise, we'll look into adding that support in a newer Resolver with Pydantic support + OpenAPI docs auto-generation

@heitorlessa
Copy link
Contributor

cc @rafaelgsr as we discussed to prioritize the middleware first, then a POC create a contract to integrate with Chalice-Spec for OpenAPI auto-generation

@walmsles
Copy link
Contributor

walmsles commented Nov 3, 2023

I wanted to return to this one and see if the Event Handler middleware implementation resolves this issue for @Muthuveerappanv. Are there still gaps for you?

The middleware handlers will enable the injection of named parameters (if you want), or you can add to the context object using append_context which I find to be a super-useful feature. Powertools clears this context after each route invocation.

@guywilsonjr
Copy link

@walmsles Thanks for contributing the SchemaValidationMiddleware Is there a way it can be used to valid query string parameters? I'm not sure if the validation or parser modules support it well without some custom coding

@walmsles
Copy link
Contributor

@walmsles Thanks for contributing the SchemaValidationMiddleware Is there a way it can be used to valid query string parameters? I'm not sure if the validation or parser modules support it well without some custom coding

Hi @guywilsonjr - thank you for using it. I recommend raising this request as a separate feature request. The existing implementation restricts the JSON schema validation to the request body, so it is payload validation only as-is, and the query parameters will not be included. Raising this as a separate request will make this issue more visible and open for wider comment.

let me know if I can help at all.

@heitorlessa
Copy link
Contributor

I completely missed resolving this as it was added last year re:Invent -- Data Validation feature. https://docs.powertools.aws.dev/lambda/python/latest/core/event_handler/api_gateway/#data-validation

We now handle both data validation (Pydantic), inject request data (body/header/query_string/etc), and auto-serialize response based on type annotations.

Something else we briefly discussed - having a Request object - will be handled in a newer and more explicit issue: #4609

@github-project-automation github-project-automation bot moved this from Backlog to Coming soon in Powertools for AWS Lambda (Python) Jun 24, 2024
@heitorlessa heitorlessa removed the need-customer-feedback Requires more customers feedback before making or revisiting a decision label Jun 24, 2024
@heitorlessa heitorlessa moved this from Coming soon to Shipped in Powertools for AWS Lambda (Python) Jun 24, 2024
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Shipped
Development

No branches or pull requests

5 participants