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

RFC: Pass custom context dict to resolve method #1547

Closed
1 of 2 tasks
MaartenUreel opened this issue Sep 27, 2022 · 27 comments · Fixed by #1567
Closed
1 of 2 tasks

RFC: Pass custom context dict to resolve method #1547

MaartenUreel opened this issue Sep 27, 2022 · 27 comments · Fixed by #1567
Assignees
Labels

Comments

@MaartenUreel
Copy link

MaartenUreel commented Sep 27, 2022

Is this related to an existing feature request or issue?

#1546

Which AWS Lambda Powertools utility does this relate to?

Event Handler - REST API

Summary

It would be useful if you could provide an optional dict to the app.resolve() method, that would then be available in the handlers themselves.
This would contain generic information that is needed for all requests.

Use case

We are implementing callbacks for Twilio. For every event, we need to:

  1. Decode the body
  2. Parse the parameters
  3. Validate the request

Next, we need the same parameters from step 2 in almost every request.

Proposal

Provide an extra parameter to the resolve() method:

app.py

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

import todos


app = APIGatewayRestResolver()
app.include_router(todos.router)


def lambda_handler(event: dict, context: LambdaContext) -> dict:
    some_info = {"some_custom": "data"}
    return app.resolve(event, context, route_context=some_info)

In the handler itself, it could then be accessible via route_context:

todos.py

import requests

from aws_lambda_powertools.event_handler.api_gateway import Router
from requests import Response

router = Router()


@router.get("/todos")
def get_todos():
    some_data = router.route_context.get("some_custom")
   
    todos: Response = requests.get("https://jsonplaceholder.typicode.com/todos")
    todos.raise_for_status()

    return {"todos": todos.json()[:10]}

Out of scope

n/a

Potential challenges

n/a

Dependencies and Integrations

No response

Alternative solutions

Currently I'm passing the data in app.lambda_context.__dict__.

Acknowledgment

@MaartenUreel MaartenUreel added RFC triage Pending triage from maintainers labels Sep 27, 2022
@boring-cyborg
Copy link

boring-cyborg bot commented Sep 27, 2022

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

@heitorlessa heitorlessa removed the triage Pending triage from maintainers label Sep 28, 2022
@heitorlessa heitorlessa self-assigned this Sep 28, 2022
@heitorlessa
Copy link
Contributor

Hey @MaartenUreel thanks a lot for this! I like this idea more than an actual Dependency Injection mechanism ala FastAPI, since that would increase the bar to entry and over-complicate things.

Question: How are you envisioning route_context cleanup to prevent stale objects? Destroy route_context at the end of every request resolved?

If you have the bandwidth, feel free to start a draft implementation and a test. I'd expect route_context to be an empty dict by default to prevent basic TypeErrors when accessing, and help customers use sentinel values if a given context key isn't available.

@heitorlessa
Copy link
Contributor

@walmsles @gwlester @benbridts - I'd be interested in your thoughts here.

@benbridts
Copy link

benbridts commented Sep 28, 2022

Would getting access to the original event (new feature, I think) also solve this (you could also add middelware instead of doing it in the handler?

I don't think this is necessarily better than the original proposal (as it's a bit more of a hack), but might be used for other things too.

def lambda_handler(event: dict, context: LambdaContext) -> dict:
    event["_extra_context"] = {"some_custom": "data"}
    return app.resolve(event, context)

# ...
@router.get("/todos")
def get_todos():
    return app.orignal_event["_extra_context"]["some_custom"]

My first reaction was "I might want this as a decorator instead of something in the handler - potentionally a middelware function"

# not sure if this is possible

@router.get("/todos")
@extra_context   # middelware you have te create yourself, fills in extra_context based on the original event
def get_todos(extra_context=None):
    pass

@gwlester
Copy link
Contributor

My read is this is a request to add "user data" object to pass around with some application-specific (not event-specific) information.

Seems reasonable.

The workaround I've seen is to create a singleton class that has the "user data" in it and get the instance every place you need it.

Of course, I could be off in left feel and missing what is being asked for.

@heitorlessa
Copy link
Contributor

What @gwlester said. This opens the door for a newer future we couldn't make progress yet (middleware support), so you could add additional context (user data) on a before/after request basis.. typically used in AuthZ, validation, etc.

@benbridts for event access, we provide that feature since launch ;)

@gwlester
Copy link
Contributor

From the router we have access to the resolver. From the resolver don't we already have access to the event and context??

@heitorlessa
Copy link
Contributor

Yes, you do. Both resolver and router inherit from BaseRouter, allowing you to access both app.current_event, app.lambda_context: https://github.com/awslabs/aws-lambda-powertools-python/blob/develop/aws_lambda_powertools/event_handler/api_gateway.py#L238. This RFC would implement a third class attribute to access "user data" (route_context, whatever naming we end up with)

@MaartenUreel
Copy link
Author

My read is this is a request to add "user data" object to pass around with some application-specific (not event-specific) information.

Seems reasonable.

The workaround I've seen is to create a singleton class that has the "user data" in it and get the instance every place you need it.

Of course, I could be off in left feel and missing what is being asked for.

I was aiming at event-specific data, e.g. in my use case a dataclass.

@heitorlessa
Copy link
Contributor

@MaartenUreel from the initial discussion you had, @gwlester is saying the same thing.. it's just the word "event-specific" that is confusing the intent.

One thing is to mutate the incoming event a Lambda receives (available at app.current_event). Another is to pass your CallContext you shared earlier within route_context -- technically, that would be "user data", not mutating the event.

@walmsles
Copy link
Contributor

walmsles commented Sep 29, 2022

@heitorlessa, have been thinking about this and agree with @benbridts with the middleware/decorator approach rather than polluting the resolver.resolve() method with additional overhead. This feels like a custom use-case and today it's a dict, tomorrow it could be something different - middleware kind of resolves the ambiguity without opening that maintenance doorway.

Providing an easy mechanism to create route middleware I feel would be an approach that is more usable and is a utility that is useful for AWS Lambda powertools to provide.

I kind of played today at "rolling my own" and came up with the following (excuse the messy code):

from aws_lambda_powertools.event_handler import APIGatewayRestResolver
from aws_lambda_powertools import Logger
import functools

app = APIGatewayRestResolver()
logger = Logger()

def route_decorator(route_func):
    """Define route decorator middleware function to add my user data"""
    @functools.wraps(route_func)
    def wrapper(*args, **kwargs):
        kwargs["user_data"] = {"param1":"value1"}
        logger.info("calling route_decorator")
        return route_func(*args, **kwargs)
    
    return wrapper

@app.get("/hello")
@route_decorator
def hello_world(user_data=None):
    return {"message": "Hello World", "user_data": user_data}

@logger.inject_lambda_context(log_event=True)
def lambda_handler(event, context):
    return app.resolve(event, context)

The decorator has to sit after the route definition and at this point, the app.current_event object is populated so can do custom processing across this to parse parameters or other things. This also achieves the DRY principle and enables different processing for different routes which adds more flexibility IMO.

@heitorlessa
Copy link
Contributor

heitorlessa commented Sep 29, 2022

Thank you as always @walmsles

Question: How would you access request information in the route_decorator as it's unaware of the "app" object?

I think we have two non-mutually exclusive use cases here:

  • Middleware. Define logic to run before/after request and/or per route specific. Cross-cutting concern that we want to enable but need a separate RFC to achieve minimal boilerplate.

  • Share data. You want to share data that is only valid at the request level for thread-safe reasons. This reduces the need for singletons and optimisations client-side whilst enhancing middlewares when available -- e.g., heavy computation once, later used by @restricted_area middleware in several routes.

The former is similar to @walmsles snippet, where we could provide a "before_request", "after_request" decorator factory, and accept a list of callable sat route definition too -- here we can pass request/context info to you.

The latter is more closely to Flask G object, but accessed as "app.route_context". This is also a problem outside Event Handler, where you'd like to access Lambda Context anywhere in your code (harder to solve).

https://tedboy.github.io/flask/interface_api.app_globals.html

For this RFC, did I capture this right?

Shall we move towards middleware and let customers handle state isolation themselves? Add your +1

Great discussion! It makes me wonder how to best utilize Discord Audio channels to speed up decisions.

@walmsles
Copy link
Contributor

walmsles commented Sep 29, 2022

Question: How would you access request information in the route_decorator as it's unaware of the "app" object?

The actual resolver is instantiated as a global var, so will be in scope for the route_decorator to reference in the same way it is used by the function routes.

Middleware for routes is a nice feature - agree needs to be designed well to minimise setup and make it easily digestible and to also expose the inner event data easily.

Question: With Share data - How does this help with heavy compute in comparison to middleware solution given the share_data is executed for each lambda invocation?

@walmsles
Copy link
Contributor

Great discussion! It makes me wonder how to best utilize Discord Audio channels to speed up decisions.

Timezones Uh-Oh (I am on the wrong continent still 😁)

@heitorlessa
Copy link
Contributor

Question: With Share data - How does this help with heavy compute in comparison to middleware solution given the share_data is executed for each lambda invocation?

You'd do the computation in the global scope (cold start) because compute resources are allocated without slicing. For Middleware, customers will typically do that at middleware level for completeness, where we sliced compute resources (once Handler is called).

As I write this, I could equally make the case that a middleware author could do the same (global scope computation). In practicality, it's more common for middlewares to be self-contained and not rely on global scope vs a loose share_data approach

@benbridts
Copy link

@benbridts for event access, we provide that feature since launch ;)

Apologies, I usually use external routing ;). I assumed current_event only have the unwrapped fields available.

I agree with @walmsles, but we should make sure that this solves @MaartenUreel's problem

@heitorlessa
Copy link
Contributor

Apologies, I usually use external routing ;). I assumed current_event only have the unwrapped fields available.

That's great feedback, Ben! We can do a better job in calling out customers can also use external routing (most do!) while still benefiting from the DX, and more importantly that app.current_event is an instance of an event source data class with handy methods.

@heitorlessa
Copy link
Contributor

Had more thoughts on this, discussed with @rubenfonseca and @leandrodamascena too. I'd like @MaartenUreel (main customer) and @walmsles final weight in here, before we decide Go/No Go.

Proposal: Add a new method called append_context in App/Router. This would be a Dict available under app.context and router.context. It's similar to Logger.append_keys to ease cognitive load. It's flexible enough to hold varying data and typing needs, keys might be overridden/cleared, and it'll be cleared at the end of each invocation (safety). This keeps app.resolve() clean to its purpose whilst being a stepping stone for Middleware in the future (Progressive tenet).

Why: As you grow your number of related routes for your service (micro-Lambda or mono-Lambda), it's common to end up with a few hundred lines of code in a single file - not everyone is comfortable with that. As you split, you can end up in a circular import scenario when dealing with global objects, if you're not careful - testing becomes harder too.

Why not middleware: They complement each other. Context solves simple data passing while a middleware can use or further enrich that contextual data. For example, a fine-grained AuthZ middleware could append context data (e.g., Lambda function URL, not API GW as it's a problem solved). It also helps isolate incoming event to data related to processing that event, where you don't want to mix the two for logical or incompatibility reasons (event validation).

If you agree, I can help guide the implementation details for anyone with the bandwidth. I'd also welcome a RFC to support Middleware in Event Handler.

Thanks a lot everyone <3

@MaartenUreel
Copy link
Author

Sounds like a perfect solution to me!

@walmsles
Copy link
Contributor

walmsles commented Oct 3, 2022

LGTM too! Love the clarity of thinking in the writeup, always impressed.

@heitorlessa
Copy link
Contributor

Fantastic. @MaartenUreel, would you like to do the honours and contribute it so we can credit you appropriately?

If you don't have the bandwidth, I can do that and tag you in the release notes regardless. Hoping we can incorporate this feature in this week's release (Friday the latest)

@MaartenUreel
Copy link
Author

The challenge is that I would have to look into the structure of the project etc, it might be faster if you guys handle it.
I'm not here for the credits 😬

@heitorlessa
Copy link
Contributor

heitorlessa commented Oct 4, 2022

Leave with me then.

I'm not here for the credits 😬

I understand. I'd like to emphasize that all maintainers should always give the authors priority in the implementation regardless :)

Every contribution is valid. Every little help helps - from docs, feature requests, bug reporting, discussions, etc. We should always credit people from the community ;)

@heitorlessa
Copy link
Contributor

Doing the implementation now, you can follow the PR if you're interested in the details: #1567

@heitorlessa heitorlessa linked a pull request Oct 4, 2022 that will close this issue
8 tasks
@heitorlessa
Copy link
Contributor

Done, just need a review from any of the maintainers (or a community member). I've also made sure to future proof for middlewares, so you can also call append_context at any point in your code, not just inside the lambda_handler.

NOTE. The same experience will work for ALB, API Gateway REST, API Gateway HTTP, and AppSync resolvers.


Docs preview

image

image

@github-actions
Copy link
Contributor

github-actions bot commented Oct 4, 2022

⚠️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 Oct 4, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2022

This is now released under 1.30.0 version!

@github-actions github-actions bot removed the pending-release Fix or implementation already in dev waiting to be released label Oct 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants