feat(router): Ignore trailing slashes in route resolvers inherited from APIGatewayResolver

[walmsles]

Issue number: #1552

Summary

Ingore trailing slash for routers inheriting from APIGatewayResolver

Changes

AWS API Gateway routes the following 2 paths in the same way "/my/path", "/my/path/", i.e. when routing "/my/path/" it routes exactly as if "/my/path" had been called. The Lambda Powertools router currently treats these as 2 distinct paths so "/my/path/" returns HTTP statusCode of 404.

User experience

No change to user experience. Users will create routes as normal. In the given sample below making API gateway call to path "/my/path/" will correctly ensure the "get_lambda" route function is correctly called even though it is defined as "/my/path"

app = APIGatewayRestResolver()

    @app.get("/my/path")
    def get_lambda() -> Response:
        assert isinstance(app.current_event, APIGatewayProxyEvent)
        assert app.lambda_context == {}
        assert app.current_event.request_context.domain_name == "id.execute-api.us-east-1.amazonaws.com"
        return Response(200, content_types.APPLICATION_JSON, json.dumps({"foo": "value"}))

The code below will work in the same way when called using path "/my/path", "/my/path/"

app = APIGatewayRestResolver()

    @app.get("/my/path/")
    def get_lambda() -> Response:
        assert isinstance(app.current_event, APIGatewayProxyEvent)
        assert app.lambda_context == {}
        assert app.current_event.request_context.domain_name == "id.execute-api.us-east-1.amazonaws.com"
        return Response(200, content_types.APPLICATION_JSON, json.dumps({"foo": "value"}))

Checklist

If your change doesn't seem to apply, please leave them unchecked.

• Meet tenets criteria
• I have performed a self-review of this change
• Changes have been tested
• Changes are documented
• PR title follows conventional commit semantics

Acknowledgment

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

---

View rendered docs/core/event_handler/api_gateway.md

[walmsles]

Note: I started working on E2E tests - these are unfinished and will take some more time to complete.

@heitorlessa you mentioned ALB E2E tests already being defined - I was not able to find can you share a link to the test?

[heitorlessa]

Note: I started working on E2E tests - these are unfinished and will take some more time to complete.

@heitorlessa you mentioned ALB E2E tests already being defined - I was not able to find can you share a link to the test?

My bad I forgot to mention it's in the V2 branch: https://github.com/awslabs/aws-lambda-powertools-python/blob/v2/tests/e2e/event_handler/test_header_serializer.py#L39

This was for a breaking change feature @rubenfonseca was doing, but we didn't merge the tests (and optimizations) to develop because of V2 proximity -- Would you mind changing the base to v2 as we're close to launch?

There's only one major item pending for V2 (Serverless Application Repository) and we should be good to go.

[codecov-commenter]

Codecov Report

Base: 99.74% // Head: 99.74% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (eac4c1a) compared to base (a5755d4).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #1577   +/-   ##
========================================
  Coverage    99.74%   99.74%           
========================================
  Files          124      124           
  Lines         5784     5785    +1     
  Branches       658      658           
========================================
+ Hits          5769     5770    +1     
  Misses           8        8           
  Partials         7        7           

Impacted Files	Coverage Δ
aws_lambda_powertools/event_handler/api_gateway.py	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[walmsles]

My bad I forgot to mention it's in the V2 branch: https://github.com/awslabs/aws-lambda-powertools-python/blob/v2/tests/e2e/event_handler/test_header_serializer.py#L39

This was for a breaking change feature @rubenfonseca was doing, but we didn't merge the tests (and optimizations) to develop because of V2 proximity -- Would you mind changing the base to v2 as we're close to launch?

Ahhh, that makes sense now - nice - it's all in place, so can add my tests pretty quickly - let me move it across.

[walmsles]

Hope my ramblings below make sense, the TL;DR version goes like this:

I have decided to restrict the implementation of this feature to REST API Gateway (V1) since this is the default behaviour of this gateway implementation for a defined route i.e. both "/hello" and "/hello/" resolve to the same lambda integration. I will write E2E tests covering all the API behaviours around this feature so if anything changes it will be picked up.

The E2E tests are amazing! I have been playing with APIs in various guises today (Much appreciation to the person responsible for writing those!) and have come to some conclusions:

• The only use-case this feature applies to IS the REST API (V1) Implementation - which works as per my original description
• ALB and Lambda URL - all routing is deferred to the actual Lambda, so we should be faithful to RFC 3986 on Path. so as not to close off possible customer use cases.
• HTTP API (V2) - actually does not allow the creation of a route with a trailing "/" character and behaves by sending back a "404" response when an invalid path is requested (unlike V1 which is a default 403 - forbidden). If you setup a default route then the HTTP API will behave in similar fashion to ALB and Lambda URL - so again RFC 3986 should apply.

The code in this PR is not up to date as yet since have switched my feature branch to be from V2. Will hopefully push changes tomorrow.

[walmsles]

@heitorlessa - This PR is a little mucked up (my fault) - will re-issue a new PR based on V2 and my feature branch, which is required to fix this up.

Have raised new PR - #1609

[heitorlessa]

Closing in favour of #1609