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: Add support for middlewares in event handler #953

Closed
BVMiko opened this issue Jan 12, 2022 · 15 comments · Fixed by #2917
Closed

Feature request: Add support for middlewares in event handler #953

BVMiko opened this issue Jan 12, 2022 · 15 comments · Fixed by #2917
Assignees
Labels

Comments

@BVMiko
Copy link
Contributor

BVMiko commented Jan 12, 2022

Is your feature request related to a problem? Please describe.
Event handlers (specifically ApiGatewayResolver) are incompatible with middleware decorators. The lambda_handler_decorator is almost exactly what would be needed, except for the function signature.

Describe the solution you'd like
I'd like to be able to add middleware when using a router, and for the middleware to have access to the processed event:

@router_decorator
def db_middleware(handler, router, **kwargs):
    db = router.current_event.get_query_string_value(name="dbname", default_value='default_value')
    return handler(db=db, **kwargs)
    
@router.get("/users")
@db_middleware(router=router)
def get_users(db, foo) -> Dict:
    pagination_limit = router.current_event.get_query_string_value(name="limit", default_value=10)
    return {"pagination_limit": pagination_limit, "db": db}

Describe alternatives you've considered
Using a regular lambda_handler_decorator outside of the router.get is executed even when the router isn't called. The decorator wrapper could be stored in individual project code, but since it's so similar to the existing one and would be useful for any router usage, it makes sense to consider it for the aws-lambda-powertools library.

Additional context

@BVMiko BVMiko added feature-request feature request triage Pending triage from maintainers labels Jan 12, 2022
@heitorlessa
Copy link
Contributor

Hey @BVMiko - thanks for opening this feature request.

Perhaps a generic middleware factory decorator for any Python function with an arbitrary number of args, kwargs?

That would help make it more explicit so we have one for Lambda and for any function - it also makes it easier to preserve types and function signature (ParamSpec is only supported in later Python versions)

It's something I wanted to do for a long time but couldn't prioritise earlier

Thanks a lot btw!

@heitorlessa heitorlessa removed the triage Pending triage from maintainers label Jan 20, 2022
@heitorlessa heitorlessa changed the title Allow middleware support for event handlers Feature: Allow middleware support for event handlers Jan 20, 2022
@karanhere3962
Copy link

karanhere3962 commented Jan 25, 2022

@heitorlessa Here is what would make sense for middleware :
Defining middleware :

        def auth_middleware(app, handler):

        ### auth logic here ###

        setattr(app.current_event, "user", user_obj) # or app.current_event.extras["user"] = user_obj - a well defined place through which extra information can be passed on to routes.

        return handler()  # I haven't seen the code base so I don't know how difficult/easy it would be to implement this`

Registering middleware :

app.register_middleware(auth_middleware)

@heitorlessa
Copy link
Contributor

heitorlessa commented Jan 25, 2022

Thanks a lot @karanhere3962 -- that's similar to FastAPI.

As this would run for every request, what's the UX you have in mind for a middleware that only runs on a given path?

Example:

  • Register as part of route definition: @app.get("/path", middlewares=[]
  • Register anywhere else: app.include_middleware(middleware, route="/path")

The details are important so we keep maintenance and extensibility at heart to follow our tenets.

Any other ideas would be great.

Cc @marcioemiranda who proposed this in the early days

@karanhere3962
Copy link

Both the options that you have given seem great.

@heitorlessa heitorlessa added the help wanted Could use a second pair of eyes/hands label Aug 1, 2022
@heitorlessa heitorlessa pinned this issue Mar 1, 2023
@heitorlessa heitorlessa changed the title Feature: Allow middleware support for event handlers Feature request: Allow middleware support for event handlers Mar 1, 2023
@heitorlessa heitorlessa changed the title Feature request: Allow middleware support for event handlers Feature request: Add support for middlewares in event handlers Mar 1, 2023
@heitorlessa heitorlessa changed the title Feature request: Add support for middlewares in event handlers Feature request: Add support for middlewares in event handler Mar 1, 2023
@heitorlessa
Copy link
Contributor

Reviving this feature request now that we finished with all V2 post-release tasks.

These are questions I'd like us to answer and get a POC moving.

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.
    • alternative: middleware could use append_context feature so the underlying event always stays true avoiding complications - cons: typing gymnastics depending on what a middleware wants to inject

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?

Built-in middlewares

  • What are some of the must-have middlewares we should have available within Powertools? e.g., schema validation

Suggestion: I think we should revisit the UX for creating/registering middlewares and get inspired about some of these libraries[1] decisions, so that we keep our Tenets in mind and performance.

[1] Chalice middlewares, FastAPI Middleware, Django Middleware, Starlite

@heitorlessa
Copy link
Contributor

Prioritizing it for our next iteration May 8th-May 19th

@heitorlessa heitorlessa removed the help wanted Could use a second pair of eyes/hands label Apr 25, 2023
@sthulb sthulb moved this from Triage to Next iteration in Powertools for AWS Lambda (Python) Jun 19, 2023
@leandrodamascena leandrodamascena moved this from Next iteration to Backlog in Powertools for AWS Lambda (Python) Jun 20, 2023
@heitorlessa
Copy link
Contributor

reassigning as I'm starting a POC tomorrow, apologies for the delay everyone - security and operations took priority (as it should)

@heitorlessa heitorlessa unpinned this issue Jul 6, 2023
@heitorlessa
Copy link
Contributor

UPDATE: we had to reprioritize it due to Pydantic v2 launch and the weight it brought in the ecosystem (breaking changes) - @leandrodamascena is on it, but I took on-call responsibilities until the end of next week.

Realistically speaking, only resuming a local POC I got by the end of the month. Meanwhile, @rubenfonseca continues to work on OpenAPI + SwaggerUI auto-generation RFC + POC.

Apologies for the delays here...

@heitorlessa heitorlessa moved this from Backlog to Next iteration in Powertools for AWS Lambda (Python) Jul 7, 2023
@walmsles
Copy link
Contributor

Thoughts on Middleware (based on @heitorlessa's questions above):

Reviving this feature request now that we finished with all V2 post-release tasks.

These are questions I'd like us to answer and get a POC moving.

Writing a middleware

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

I like this signature - **kwargs enables possible dependency injection from middleware so resolves additional use-cases

  • What does short-circuiting should look like? e.g., if schema validation fail we should return early and not call any subsequent registered middleware
  • with middleware calling the next get_response(app, **kwargs) - any middleware can raise an exception to halt the continuation of request processing. Any prior middleware wanting to catch/handle exceptions may do so using try: ... except: so becomes a simple language every dev understands
  • 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.
  • I feel the original event should remain immutable.
  • alternative: middleware could use append_context feature so the underlying event always stays true avoiding complications - cons: typing gymnastics depending on what a middleware wants to inject
  • I like this idea, con is a concern - might need more input here to understand better - although **kwargs above could do away with this need - given devs could inject whatever they wanted to the route handlers with this mechanism, and it is completely in their control

Registering a middleware

  • Per route: Should we add a single parameter, namely middlewares=[]?
  • Yes - this is nice and simple, devs also set the order in the array, and that's how they are executed
  • Per route: Should we add a before_request and after_request parameters? e.g., advantage being simpler to write/read middlewares
  • No, I dislike the use of before_request, after_request and error - properly constructed middleware stack naturally has this behaviour via: before/after - due to the placement of code around calling get_response(app, context) and errorviatry:...except:` language blocks
  • Per request: Given per route and global middlewares were registered (App+Routers), which order do we resolve?
  • Resolution order should be global followed by route - global would usually represent cross-cutting concerns that benefit from the earlier resolution

Built-in middlewares

  • What are some of the must-have middlewares we should have available within Powertools? _e.g., schema validation
  • schema validation
  • ResponseCaching
  • Idempotency (??)
  • custom authorizers (with care)
  • ResponseStreaming (????? 👀)
  • refactor built_in features to core middleware (backward compatibility alert)
    - Cors
    - Compression
    - Cache-Control

[1] Chalice middlewares, FastAPI Middleware, Django Middleware, Starlite

Inspiration: Starlette

Fast API, Chalice & Starlette implementation is very similar - I like the approach (classic middleware implementation that is easy to reason about).

@walmsles
Copy link
Contributor

walmsles commented Aug 1, 2023

Looking deeper: We need to consider how to bring middleware to appsync resolvers - I feel the same DX works. I feel I need to dig deeper into AppSync and Powertools, its very separate from other resolvers (and likely with good cause).

@heitorlessa
Copy link
Contributor

Sounds great to me @walmsles! On AppSync, @leandrodamascena @mploski are working on a refactor to support Batch event and partial failure, so I'd ignore middleware support in AppSync Event Handler until this work is complete #1303. Let's get Event Handler working POC first then we see how much is transferable as-is so it doesn't become a blocker.

@walmsles
Copy link
Contributor

walmsles commented Aug 2, 2023

Thanks @heitorlessa - agree -that was my thinking.

@heitorlessa
Copy link
Contributor

Sharing progress for anyone following - we're currently writing docs, probably 8 more hours and then a final pass before we release it. We're looking to make it available by early September

image

@github-project-automation github-project-automation bot moved this from Working on it to Coming soon in Powertools for AWS Lambda (Python) Sep 7, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2023

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

@github-actions github-actions bot added the pending-release Fix or implementation already in dev waiting to be released label Sep 7, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Sep 8, 2023

This is now released under 2.24.0 version!

@github-actions github-actions bot removed the pending-release Fix or implementation already in dev waiting to be released label Sep 8, 2023
@leandrodamascena leandrodamascena moved this from Coming soon to Shipped in Powertools for AWS Lambda (Python) Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Shipped
Development

Successfully merging a pull request may close this issue.

5 participants