From dcdbcb6d712ee86e12724e35644d8dc2d254a1b6 Mon Sep 17 00:00:00 2001 From: Ruben Fonseca Date: Wed, 14 Jun 2023 17:02:54 +0200 Subject: [PATCH 1/3] fix(event_handler): prioritize full route matching --- .../event_handler/api_gateway.py | 13 ++++++++++- .../event_handler/test_api_gateway.py | 23 +++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/aws_lambda_powertools/event_handler/api_gateway.py b/aws_lambda_powertools/event_handler/api_gateway.py index 05fbc1c06c1..145444abbfe 100644 --- a/aws_lambda_powertools/event_handler/api_gateway.py +++ b/aws_lambda_powertools/event_handler/api_gateway.py @@ -515,7 +515,18 @@ def register_resolver(func: Callable): cors_enabled = cors for item in methods: - self._routes.append(Route(item, self._compile_regex(rule), func, cors_enabled, compress, cache_control)) + _route = Route(item, self._compile_regex(rule), func, cors_enabled, compress, cache_control) + + # If the compiled regex contains groups, we want to prioritize routes that will fully match the path + # directly. To achieve this, we add routes with groups at the end of the list, while routes without + # groups are added at the beginning of the list. + if _route.rule.groups > 0: + # Insert route at the end of the list + self._routes.append(_route) + else: + # Insert route at the beginning of the list + self._routes.insert(0, _route) + route_key = item + rule if route_key in self._route_keys: warnings.warn( diff --git a/tests/functional/event_handler/test_api_gateway.py b/tests/functional/event_handler/test_api_gateway.py index 6faad88d7f1..ba4dcad4f55 100644 --- a/tests/functional/event_handler/test_api_gateway.py +++ b/tests/functional/event_handler/test_api_gateway.py @@ -1647,3 +1647,26 @@ def get_message(): assert response["multiValueHeaders"]["Content-Type"] == [content_types.APPLICATION_JSON] response_body = json.loads(response["body"]) assert response_body["message"] == "success" + + +def test_route_match_prioritize_full_match(): + # GIVEN a Http API V1, with a function registered with two routes + app = APIGatewayRestResolver() + router = Router() + + @router.get("/my/{path}") + def dynamic_handler() -> Response: + return Response(200, content_types.APPLICATION_JSON, json.dumps({"hello": "dynamic"})) + + @router.get("/my/path") + def static_handler() -> Response: + return Response(200, content_types.APPLICATION_JSON, json.dumps({"hello": "static"})) + + app.include_router(router) + + # WHEN calling the event handler with /foo/dynamic + response = app(LOAD_GW_EVENT, {}) + + # THEN the static_handler should have been called, because it fully matches the path directly + response_body = json.loads(response["body"]) + assert response_body["hello"] == "static" From e6a59fac01d8e6977399d7f075482c8695a1a6b8 Mon Sep 17 00:00:00 2001 From: Ruben Fonseca Date: Wed, 14 Jun 2023 17:43:34 +0200 Subject: [PATCH 2/3] fix: separated static and dynamic routes --- .../event_handler/api_gateway.py | 18 +++++++++--------- .../event_handler/test_api_gateway.py | 4 ++-- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/aws_lambda_powertools/event_handler/api_gateway.py b/aws_lambda_powertools/event_handler/api_gateway.py index 145444abbfe..eddccca4da5 100644 --- a/aws_lambda_powertools/event_handler/api_gateway.py +++ b/aws_lambda_powertools/event_handler/api_gateway.py @@ -483,7 +483,8 @@ def __init__( with api gateways with multiple custom mappings. """ self._proxy_type = proxy_type - self._routes: List[Route] = [] + self._dynamic_routes: List[Route] = [] + self._static_routes: List[Route] = [] self._route_keys: List[str] = [] self._exception_handlers: Dict[Type, Callable] = {} self._cors = cors @@ -517,15 +518,14 @@ def register_resolver(func: Callable): for item in methods: _route = Route(item, self._compile_regex(rule), func, cors_enabled, compress, cache_control) - # If the compiled regex contains groups, we want to prioritize routes that will fully match the path - # directly. To achieve this, we add routes with groups at the end of the list, while routes without - # groups are added at the beginning of the list. + # To prioritize routes based on specificity, we handle dynamic and static routes differently. Dynamic + # routes, which contain variable parts, are stored separately from static routes. This allows us to + # first check the static routes for a match before attempting to match the dynamic routes. By + # following this approach, we ensure that the most specific route is prioritized and processed first. if _route.rule.groups > 0: - # Insert route at the end of the list - self._routes.append(_route) + self._dynamic_routes.append(_route) else: - # Insert route at the beginning of the list - self._routes.insert(0, _route) + self._static_routes.append(_route) route_key = item + rule if route_key in self._route_keys: @@ -635,7 +635,7 @@ def _resolve(self) -> ResponseBuilder: """Resolves the response or return the not found response""" method = self.current_event.http_method.upper() path = self._remove_prefix(self.current_event.path) - for route in self._routes: + for route in self._static_routes + self._dynamic_routes: if method != route.method: continue match_results: Optional[Match] = route.rule.match(path) diff --git a/tests/functional/event_handler/test_api_gateway.py b/tests/functional/event_handler/test_api_gateway.py index ba4dcad4f55..c17422f8d94 100644 --- a/tests/functional/event_handler/test_api_gateway.py +++ b/tests/functional/event_handler/test_api_gateway.py @@ -298,7 +298,7 @@ def handler(event, context): return app.resolve(event, context) # Also check the route configurations - routes = app._routes + routes = app._static_routes assert len(routes) == 5 for route in routes: if route.func == get_func: @@ -1205,7 +1205,7 @@ def patch_func(): app.include_router(router) # Also check check the route configurations - routes = app._routes + routes = app._static_routes assert len(routes) == 5 for route in routes: if route.func == get_func: From 3e3c5e76fb2103b61e5947a90fb61290de509ef1 Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Wed, 14 Jun 2023 18:55:52 +0200 Subject: [PATCH 3/3] chore: improve comment wording --- aws_lambda_powertools/event_handler/api_gateway.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/aws_lambda_powertools/event_handler/api_gateway.py b/aws_lambda_powertools/event_handler/api_gateway.py index eddccca4da5..962fd51ccf4 100644 --- a/aws_lambda_powertools/event_handler/api_gateway.py +++ b/aws_lambda_powertools/event_handler/api_gateway.py @@ -518,10 +518,10 @@ def register_resolver(func: Callable): for item in methods: _route = Route(item, self._compile_regex(rule), func, cors_enabled, compress, cache_control) - # To prioritize routes based on specificity, we handle dynamic and static routes differently. Dynamic - # routes, which contain variable parts, are stored separately from static routes. This allows us to - # first check the static routes for a match before attempting to match the dynamic routes. By - # following this approach, we ensure that the most specific route is prioritized and processed first. + # The more specific route wins. + # We store dynamic (/studies/{studyid}) and static routes (/studies/fetch) separately. + # Then attempt a match for static routes before dynamic routes. + # This ensures that the most specific route is prioritized and processed first (studies/fetch). if _route.rule.groups > 0: self._dynamic_routes.append(_route) else: