feat(apigateway): add Router to allow large routing composition #645
Conversation
pull-request-size
bot
added
the
size/M
|
Thanks a lot for your first contribution! Please check out our contributing guidelines and don't hesitate to ask whatever you need. |
|
I modified the blueprint to not be a callable; but instead to use a |
|
I swapped the registration from being a function of the blueprint to a function of the app; specifically: blueprint.register_to_app(app)to app.register_blueprint(blueprint) |
|
Thanks a lot @BVMiko - I'll review when I get back from vacation (Sep 27th). cc @saragerion @pankajagrawal16 @t1agob for their thoughts on whether they know a more explicit name to avoid confusion when ported to other languages (Java, TS, C#) |
|
hey @am29d - could you help review this one? I like what the feature enables as some customers might have multiple routes and can still use multiple functions. However, Blueprint is Flask terminology specific and might not be 1/ easy to transfer to other upcoming runtimes, and 2/ not immediately clear what the purpose is for those not experienced with Flask. @BVMiko I'm back from vacation now, so we'll be looking into this shortly. Thank you so much for the help so far! |
|
I have no particular fondness towards the name Blueprint; it was only selected to mirror the first routing framework that I found via a Google search for a similar feature. I'll gladly update the PR if you have an alternate name in mind. |
|
Discussed during our hackathon that @michaelbrewer will be looking at a more explicit name (we don't knot yet :/), and whether there are other ways we could achieve the same result too |
|
@heitorlessa @BVMiko - i have gone through the PR, other than the conflicts from the
Here is patch diff. diff --git a/aws_lambda_powertools/event_handler/api_gateway.py b/aws_lambda_powertools/event_handler/api_gateway.py
index 7de2583..3e2545d 100644
--- a/aws_lambda_powertools/event_handler/api_gateway.py
+++ b/aws_lambda_powertools/event_handler/api_gateway.py
@@ -654,8 +654,8 @@ class Blueprint:
def actual_decorator(func: Callable):
@wraps(func)
def wrapper(app: ApiGatewayResolver):
- def inner_wrapper():
- return func(app)
+ def inner_wrapper(**kwargs):
+ return func(app, **kwargs)
return inner_wrapper
diff --git a/tests/functional/event_handler/test_api_gateway.py b/tests/functional/event_handler/test_api_gateway.py
index 9920001..c15b669 100644
--- a/tests/functional/event_handler/test_api_gateway.py
+++ b/tests/functional/event_handler/test_api_gateway.py
@@ -879,3 +879,27 @@ def test_api_gateway_app_proxy():
# THEN process event correctly
assert result["statusCode"] == 200
assert result["headers"]["Content-Type"] == content_types.APPLICATION_JSON
+
+
+def test_api_gateway_app_proxy_with_params():
+ # GIVEN a Blueprint with registered routes
+ app = ApiGatewayResolver()
+ blueprint = Blueprint()
+ req = "foo"
+ event = deepcopy(LOAD_GW_EVENT)
+ event["resource"] = "/accounts/{account_id}"
+ event["path"] = f"/accounts/{req}"
+
+ @blueprint.route(rule="/accounts/<account_id>", method=["GET", "POST"])
+ def foo(app: ApiGatewayResolver, account_id):
+ assert app.current_event.raw_event == event
+ assert account_id == f"{req}"
+ return {}
+
+ app.register_blueprint(blueprint)
+ # WHEN calling the event handler after applying routes from blueprint object
+ result = app(event, {})
+
+ # THEN process event correctly
+ assert result["statusCode"] == 200
+ assert result["headers"]["Content-Type"] == content_types.APPLICATION_JSON |
Add Blueprint class to ApiGatewayResolver
pull-request-size
bot
added
size/L
|
I've rebased on the latest |
Codecov Report
@@ Coverage Diff @@
## develop #645 +/- ##
===========================================
- Coverage 99.93% 99.85% -0.09%
===========================================
Files 116 116
Lines 4876 4942 +66
Branches 268 276 +8
===========================================
+ Hits 4873 4935 +62
- Misses 1 5 +4
Partials 2 2
Continue to review full report at Codecov.
|
|
Naming wise @heitorlessa and @cakepietoast had the impression we don't want to confuse people that this would be a drop in replacement for flask. So i will defer that part to @heitorlessa . After this is merged, we would need to work on the docs with some nice examples. |
|
At the suggestion of a colleague, I added an additional prefix parameter when registering sub apps; tests are included. |
|
@BVMiko - a little more code coverage missing to test the different supported routes I do see that if the diff --git a/tests/functional/event_handler/test_api_gateway.py b/tests/functional/event_handler/test_api_gateway.py
index 8d8b5a64..8a6978e2 100644
--- a/tests/functional/event_handler/test_api_gateway.py
+++ b/tests/functional/event_handler/test_api_gateway.py
@@ -942,3 +942,53 @@ def test_api_gateway_app_proxy_with_prefix_equals_path():
# THEN process event correctly
assert result["statusCode"] == 200
assert result["headers"]["Content-Type"] == content_types.APPLICATION_JSON
+
+
+def test_api_gateway_app_proxy_with_different_methods():
+ # GIVEN a Blueprint with all the possible HTTP methods
+ app = ApiGatewayResolver()
+ blueprint = Blueprint()
+
+ @blueprint.get("/not_matching_get")
+ def get_func():
+ raise RuntimeError()
+
+ @blueprint.post("/no_matching_post")
+ def post_func():
+ raise RuntimeError()
+
+ @blueprint.put("/no_matching_put")
+ def put_func():
+ raise RuntimeError()
+
+ @blueprint.delete("/no_matching_delete")
+ def delete_func():
+ raise RuntimeError()
+
+ @blueprint.patch("/no_matching_patch")
+ def patch_func():
+ raise RuntimeError()
+
+ app.register_blueprint(blueprint)
+
+ # Also check check the route configurations
+ routes = app._routes
+ assert len(routes) == 5
+ for route in routes:
+ if route.func == get_func:
+ assert route.method == "GET"
+ elif route.func == post_func:
+ assert route.method == "POST"
+ elif route.func == put_func:
+ assert route.method == "PUT"
+ elif route.func == delete_func:
+ assert route.method == "DELETE"
+ elif route.func == patch_func:
+ assert route.method == "PATCH"
+
+ # WHEN calling the handler
+ # THEN return a 404
+ result = app(LOAD_GW_EVENT, None)
+ assert result["statusCode"] == 404
+ # AND cors headers are not returned
+ assert "Access-Control-Allow-Origin" not in result["headers"] |
|
Otherwise you might want to the registering of the routes outside of the handler ie: from aws_lambda_powertools import Logger, Tracer
from aws_lambda_powertools.event_handler.api_gateway import ApiGatewayResolver
from aws_lambda_powertools.logging import correlation_paths
from . import endpoint1
tracer = Tracer()
logger = Logger()
app = ApiGatewayResolver()
app.register_blueprint(endpoint1.blueprint)
@logger.inject_lambda_context(correlation_id_path=correlation_paths.API_GATEWAY_REST)
@tracer.capture_lambda_handler
def lambda_handler(event, context):
return app.resolve(event, context) |
|
@heitorlessa @BVMiko - as a side note, i do like the developer experience of this library for Appsync resolvers (https://github.com/twkiiim/sleemo). Basically you just register a directory where you resolvers are. @heitorlessa - i trust we can come up with a meaning for name for this, like ProxyResolver. |
|
@heitorlessa sent me another good implementation of this: |
|
Having looked a little at the FastAPI implementation i see some nice things about it:
|
|
I haven’t had a chance to look into this yet properly, and it seems it’s
evolving quickly — sounds like we missed an opportunity to have a RFC to
discuss it.
What about Router as a name?
Then we can pass a Router instance to the parent App. Parent App then
injects the request context in the router instance. All functions using
Router now have the same UX as if they were using the parent App.
If customers later have a change of heart and prefer thin Lambda functions
or that their routes best fit in a single file, they simply change from
Router to ApigatewayResolver.
I’ll sleep on it. I’d like to have a release by Wed the latest - if we
can’t have a good solution by then let’s not rush it, we can do it for
another release in 10 days or so, since this is important.
Thank you for suggesting and working on this all along once again
…On Sat, 2 Oct 2021 at 18:30, Michael Brewer ***@***.***> wrote:
Having looked a little at the FastAPI implementation i see some nice
things about it:
- APIRoute is a better name over all than BluePrint
- Setting the prefix="/admin" in both the APIRoute and when you add a
route app.include_router( makes sense
- FastAPI documentation for this feature is nicely laid out, with some
good folder layouts
- Passing the context info is not done via the app handle by rather by starlette
context <https://github.com/tomwojcik/starlette-context>. But maybe
there can be another way to do this part
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#645 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZPQBCONM3DCHDOMYJSYYDUE4XSFANCNFSM5CTPYMXA>
.
|
|
@BVMiko @heitorlessa - I have made the suggested changes in this patch diff --git a/aws_lambda_powertools/event_handler/api_gateway.py b/aws_lambda_powertools/event_handler/api_gateway.py
index a296874f..d3a79761 100644
--- a/aws_lambda_powertools/event_handler/api_gateway.py
+++ b/aws_lambda_powertools/event_handler/api_gateway.py
@@ -631,22 +631,33 @@ class ApiGatewayResolver:
def _json_dump(self, obj: Any) -> str:
return self._serializer(obj)
- def register_blueprint(self, blueprint: "Blueprint", prefix: Optional[str] = None) -> None:
- """Adds all routes defined in a blueprint"""
- for route, func in blueprint.api.items():
+ def include_router(self, router: "Router", prefix: Optional[str] = None) -> None:
+ """Adds all routes defined in a router"""
+ router._app = self
+ for route, func in router.api.items():
if prefix and route[0] == "/":
route = (prefix, *route[1:])
elif prefix:
route = (f"{prefix}{route[0]}", *route[1:])
- self.route(*route)(func(app=self))
+ self.route(*route)(func())
-class Blueprint:
- """Blueprint helper class to allow splitting ApiGatewayResolver into multiple files"""
+class Router:
+ """Router helper class to allow splitting ApiGatewayResolver into multiple files"""
+
+ _app: ApiGatewayResolver
def __init__(self):
self.api: Dict[tuple, Callable] = {}
+ @property
+ def current_event(self) -> BaseProxyEvent:
+ return self._app.current_event
+
+ @property
+ def lambda_context(self) -> LambdaContext:
+ return self._app.lambda_context
+
def route(
self,
rule: str,
@@ -657,9 +668,9 @@ class Blueprint:
):
def actual_decorator(func: Callable):
@wraps(func)
- def wrapper(app: ApiGatewayResolver):
+ def wrapper():
def inner_wrapper(**kwargs):
- return func(app, **kwargs)
+ return func(**kwargs)
return inner_wrapper
diff --git a/tests/functional/event_handler/test_api_gateway.py b/tests/functional/event_handler/test_api_gateway.py
index 8d8b5a64..afc97906 100644
--- a/tests/functional/event_handler/test_api_gateway.py
+++ b/tests/functional/event_handler/test_api_gateway.py
@@ -13,11 +13,11 @@ import pytest
from aws_lambda_powertools.event_handler import content_types
from aws_lambda_powertools.event_handler.api_gateway import (
ApiGatewayResolver,
- Blueprint,
CORSConfig,
ProxyEventType,
Response,
ResponseBuilder,
+ Router,
)
from aws_lambda_powertools.event_handler.exceptions import (
BadRequestError,
@@ -863,17 +863,17 @@ def test_api_gateway_request_path_equals_strip_prefix():
assert result["headers"]["Content-Type"] == content_types.APPLICATION_JSON
-def test_api_gateway_app_proxy():
- # GIVEN a Blueprint with registered routes
+def test_api_gateway_app_router():
+ # GIVEN a Router with registered routes
app = ApiGatewayResolver()
- blueprint = Blueprint()
+ router = Router()
- @blueprint.get("/my/path")
- def foo(app: ApiGatewayResolver):
+ @router.get("/my/path")
+ def foo():
return {}
- app.register_blueprint(blueprint)
- # WHEN calling the event handler after applying routes from blueprint object
+ app.include_router(router)
+ # WHEN calling the event handler after applying routes from router object
result = app(LOAD_GW_EVENT, {})
# THEN process event correctly
@@ -881,42 +881,44 @@ def test_api_gateway_app_proxy():
assert result["headers"]["Content-Type"] == content_types.APPLICATION_JSON
-def test_api_gateway_app_proxy_with_params():
- # GIVEN a Blueprint with registered routes
+def test_api_gateway_app_router_with_params():
+ # GIVEN a Router with registered routes
app = ApiGatewayResolver()
- blueprint = Blueprint()
+ router = Router()
req = "foo"
event = deepcopy(LOAD_GW_EVENT)
event["resource"] = "/accounts/{account_id}"
event["path"] = f"/accounts/{req}"
+ lambda_context = {}
- @blueprint.route(rule="/accounts/<account_id>", method=["GET", "POST"])
- def foo(app: ApiGatewayResolver, account_id):
- assert app.current_event.raw_event == event
+ @router.route(rule="/accounts/<account_id>", method=["GET", "POST"])
+ def foo(account_id):
+ assert router.current_event.raw_event == event
+ assert router.lambda_context == lambda_context
assert account_id == f"{req}"
return {}
- app.register_blueprint(blueprint)
- # WHEN calling the event handler after applying routes from blueprint object
- result = app(event, {})
+ app.include_router(router)
+ # WHEN calling the event handler after applying routes from router object
+ result = app(event, lambda_context)
# THEN process event correctly
assert result["statusCode"] == 200
assert result["headers"]["Content-Type"] == content_types.APPLICATION_JSON
-def test_api_gateway_app_proxy_with_prefix():
- # GIVEN a Blueprint with registered routes
+def test_api_gateway_app_router_with_prefix():
+ # GIVEN a Router with registered routes
# AND a prefix is defined during the registration
app = ApiGatewayResolver()
- blueprint = Blueprint()
+ router = Router()
- @blueprint.get(rule="/path")
- def foo(app: ApiGatewayResolver):
+ @router.get(rule="/path")
+ def foo():
return {}
- app.register_blueprint(blueprint, prefix="/my")
- # WHEN calling the event handler after applying routes from blueprint object
+ app.include_router(router, prefix="/my")
+ # WHEN calling the event handler after applying routes from router object
result = app(LOAD_GW_EVENT, {})
# THEN process event correctly
@@ -924,21 +926,71 @@ def test_api_gateway_app_proxy_with_prefix():
assert result["headers"]["Content-Type"] == content_types.APPLICATION_JSON
-def test_api_gateway_app_proxy_with_prefix_equals_path():
- # GIVEN a Blueprint with registered routes
+def test_api_gateway_app_router_with_prefix_equals_path():
+ # GIVEN a Router with registered routes
# AND a prefix is defined during the registration
app = ApiGatewayResolver()
- blueprint = Blueprint()
+ router = Router()
- @blueprint.get(rule="/")
- def foo(app: ApiGatewayResolver):
+ @router.get(rule="/")
+ def foo():
return {}
- app.register_blueprint(blueprint, prefix="/my/path")
- # WHEN calling the event handler after applying routes from blueprint object
+ app.include_router(router, prefix="/my/path")
+ # WHEN calling the event handler after applying routes from router object
# WITH the request path matching the registration prefix
result = app(LOAD_GW_EVENT, {})
# THEN process event correctly
assert result["statusCode"] == 200
assert result["headers"]["Content-Type"] == content_types.APPLICATION_JSON
+
+
+def test_api_gateway_app_router_with_different_methods():
+ # GIVEN a Router with all the possible HTTP methods
+ app = ApiGatewayResolver()
+ router = Router()
+
+ @router.get("/not_matching_get")
+ def get_func():
+ raise RuntimeError()
+
+ @router.post("/no_matching_post")
+ def post_func():
+ raise RuntimeError()
+
+ @router.put("/no_matching_put")
+ def put_func():
+ raise RuntimeError()
+
+ @router.delete("/no_matching_delete")
+ def delete_func():
+ raise RuntimeError()
+
+ @router.patch("/no_matching_patch")
+ def patch_func():
+ raise RuntimeError()
+
+ app.include_router(router)
+
+ # Also check check the route configurations
+ routes = app._routes
+ assert len(routes) == 5
+ for route in routes:
+ if route.func == get_func:
+ assert route.method == "GET"
+ elif route.func == post_func:
+ assert route.method == "POST"
+ elif route.func == put_func:
+ assert route.method == "PUT"
+ elif route.func == delete_func:
+ assert route.method == "DELETE"
+ elif route.func == patch_func:
+ assert route.method == "PATCH"
+
+ # WHEN calling the handler
+ # THEN return a 404
+ result = app(LOAD_GW_EVENT, None)
+ assert result["statusCode"] == 404
+ # AND cors headers are not returned
+ assert "Access-Control-Allow-Origin" not in result["headers"]The updated user experience will be: app/router/items.py from aws_lambda_powertools import Logger, Tracer
from aws_lambda_powertools.event_handler.api_gateway import ApiGatewayResolver, Router
tracer = Tracer()
logger = Logger(child=True)
router = Router()
@router.get('/hello')
@tracer.capture_method
def get_hello():
return {
"message": ['/hello', 'GET'],
"query_string": router.current_event.query_string_parameters
}
@router.route('/world', ('GET','POST'))
@tracer.capture_method
def multi_world():
return {
"message": ['/world', 'GET/POST'],
"query_string": router.current_event.query_string_parameters
}app/main.py from aws_lambda_powertools import Logger, Tracer
from aws_lambda_powertools.event_handler.api_gateway import ApiGatewayResolver
from aws_lambda_powertools.logging import correlation_paths
from .routers import users, items
tracer = Tracer()
logger = Logger()
app = ApiGatewayResolver()
app.include_router(users.router)
app.include_router(items.router, prefix="/items")
@logger.inject_lambda_context(correlation_id_path=correlation_paths.API_GATEWAY_REST)
@tracer.capture_lambda_handler
def lambda_handler(event, context):
return app.resolve(event, context)Recommended folder structure (based on FastApi) |
|
@BVMiko 👋 just a minor note from someone new to this library who is trying to get a grasp on various PRs / discussions going on. Imo would be nice to explain in the Pr/ commit description what this functionality will give to a new user; in other words why should they use it
me sitting in my corner and thinking: what is the gap/ missing feature in the current release and what will this PR add / extend ... maybe an example of the logs output as a sample will help more? |
|
@michaelbrewer: I've used FastAPI before and found it a very nice experience. Those changes look good to me, I'll update the PR in a few hours. @DanyC97: I described the problem in more detail in the issue #644; essentially the problem is that using ApiGatewayResolver as it is today requires each of the different routes to be defined in the same file as the main lambda entrypoint, like this: app = ApiGatewayResolver()
@app.get('/hello')
def get_hello():
...
@app.get('/world')
def get_world():
...
def lambda_handler(event, context):
return app.resolve(event, context)This is not ideal for larger projects with many different paths defined; as adding a new path requires modifying the main lambda entrypoint file. This PR is helpful for larger projects, as it introduces a way to separate distinct portions of the project into their own files/folders and it allows adding a new path without modifying the main Python entrypoint file each time. |
|
Just released 1.21 and I can now look at this properly starting this Friday. I'll likely create a separate PR with docs and get everyone's eyes to review to make sure our guidance is on point on how to use it. Thank you so much everyone so far |
heitorlessa
changed the title
|
Thank you so much everyone for this contribution and review! Three things before it's released in the next version, which can be done in other PRs @michaelbrewer
|
…tools-python into develop * 'develop' of https://github.com/awslabs/aws-lambda-powertools-python: feat(apigateway): add Router to allow large routing composition (aws-powertools#645)
Add Router class to ApiGatewayResolver
Issue #, if available: #644
Description of changes:
Add a
Routerclass to be used as a proxy forApiGatewayResolver. The class is able to at as a placeholder for the app instance and collect routes to later be applied; it includes all the functionality of current routes.To access query strings or headers the original
eventandcontextare made available asrouter.current_eventandrouter.lambda_contextrespectively. Also, to improve performance of a function which uses an endpoint with multiple methods, it offers a slight variation of theroute()method's signature allowing alist/tuple.Usage example:
app/main.py:
app/routers/items.py:
Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.