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

Error resolving path in Api Gateway event handler #520

Closed
marcioemiranda opened this issue Jul 11, 2021 · 7 comments
Closed

Error resolving path in Api Gateway event handler #520

marcioemiranda opened this issue Jul 11, 2021 · 7 comments
Labels
bug Something isn't working

Comments

@marcioemiranda
Copy link

Suppose one has two routes to delete resources like the following:

Use Case 1:

Route in handler

@app.delete("/accounts/<account_id>")
def delete_account(account_id: str) -> None:

Request

{
        "path": "/accounts/123",
        "httpMethod": "DELETE",
        "requestContext": {"requestId": "c6af9ac6-7b61-11e6-9a41-93e8deadbeef"},  # correlation ID
}

Use Case 2:

@app.delete("/accounts/<account_id>/source_networks")
def delete_source_networks(account_id: str) -> Account:

Request

{
        "path": "/accounts/123/source_networks"
        "httpMethod": "DELETE",
        "requestContext": {"requestId": "c6af9ac6-7b61-11e6-9a41-93e8deadbeef"},  # correlation ID
}

Expected Behavior

Expected behavior is that account_id equals 123 in both use cases

Current Behavior

For use case 1 resolver calls the right function and sets account_id = 123
For use case 2, resolver calls the function of use case 1 and sets accound_id = 123/source_networks

Possible Solution

Not sure if this is actually the expected behavior of proxy integration with its greedy regexp or a bug in Lambda Power Tools

Environment

Power Tools version 1.17.1

delete_source_networks_event = {'body': '{"networks": ["172.217.0.0/16", "8.8.8.8"]}', 'httpMethod': 'DELETE', 'path': **'/accounts/123/source_networks'**, ...}
{"level":"INFO","location":"delete:209","message":"Account deleted","timestamp":"2021-07-11 15:12:42,205-0300","service":"admin_api","cold_start":false,"function_name":"FUNCTION_NAME","function_memory_size":1024,"function_arn":"INVOKED_FUNCTION_ARN","function_request_id":"AWS_REQUEST_ID","correlation_id":"c6af9ac6-7b61-11e6-9a41-93e8deadbeef",**"account_id":"123/source_networks"**,"tax_id":"123","service_id":3}
@marcioemiranda marcioemiranda added bug Something isn't working triage Pending triage from maintainers labels Jul 11, 2021
@heitorlessa heitorlessa self-assigned this Jul 16, 2021
@heitorlessa
Copy link
Contributor

Hey @marcioemiranda , thanks for raising it! Could you share how you're configuring API Gateway?

At a first glance this looks like API GW route matching, so I've created this additional test for use case 2 and it works as expected --- I'm creating an API Gateway with the same path to be on the safe side and report back with a repro repo.

def test_nested_dynamic_routes():
    # GIVEN
    app = ApiGatewayResolver()
    event = deepcopy(LOAD_GW_EVENT)

    @app.get("/accounts/<account_id>/source_networks")
    def get_lambda(account_id: str):
        assert account_id == "123"
        return {"message": "okay"}

    event["resource"] = "/accounts/{account_id}/source_networks"
    event["path"] = "/accounts/123/source_networks"
    # WHEN calling the event handler
    result = app(event, {})

    # THEN
    assert result["statusCode"] == 200

@heitorlessa
Copy link
Contributor

heitorlessa commented Jul 16, 2021

Yup, confirmed, it's a bug --- I can replicate that locally with SAM CLI start-api too: https://github.com/heitorlessa/pt-issue-520

Test to reproduce the issue - If I define both functions with similar path in the tests it fails exactly as described

def test_nested_dynamic_routes():
    # GIVEN
    app = ApiGatewayResolver()
    event = deepcopy(LOAD_GW_EVENT)
    event["resource"] = "/accounts/{account_id}/source_networks"
    event["path"] = "/accounts/12345/source_networks"

    # first rule will be a match, hence not calling the right function
    # re.compile('^/accounts/(?P<account_id>.+)$')
    @app.get("/accounts/<account_id>")
    def get_lambda(account_id: str):
        assert account_id == "12345"
        return {"message": f"{account_id}"}

    # re.compile('^/accounts/(?P<account_id>.+)/source_networks$')
    @app.get("/accounts/<account_id>/source_networks")
    def get_lambda(account_id: str):
        assert account_id == "12345"
        return {"message": f"{account_id}"}

    # WHEN calling the event handler
    result = app(event, {})

    # THEN
    assert result["statusCode"] == 200

@heitorlessa heitorlessa added area/event_handlers and removed triage Pending triage from maintainers labels Jul 16, 2021
@heitorlessa heitorlessa removed their assignment Jul 16, 2021
@heitorlessa
Copy link
Contributor

@michaelbrewer could you look into it as discussed?

@marcioemiranda as a workaround until we fix it, you could change the order of the function definition for the longest/nested being the first one.

For example:

@app.get("/accounts/<account_id>/source_networks")
def account_nested(account_id: str):
    print(f"[ACCOUNT NESTED] Account ID received: --> {account_id}")
    return {"account": f"{account_id}"}

@app.get("/accounts/<account_id>")
def account(account_id: str):
    print(f"[ACCOUNT SINGLE] Account ID received: --> {account_id}")
    return {"account": f"{account_id}"}

@marcioemiranda
Copy link
Author

@heitorlessa thanks for the workaround.
API gateway was provisioned by CDK => construct LambdaRestApi in '@aws-cdk/aws-apigateway'
BTW, on a different subject, I wonder when #506 will be released.

@heitorlessa
Copy link
Contributor

We're working on some MyPy fixes and as soon as I complete #517 we'll release it OR the documentation for the new Feature Toggles utility ##494

ETA next Friday if all goes well

heitorlessa added a commit to heitorlessa/aws-lambda-powertools-python that referenced this issue Jul 18, 2021
@heitorlessa
Copy link
Contributor

We've got a fix and will ping back once released this week @marcioemiranda ;)

If you're curious: #533

@heitorlessa heitorlessa added pending-release Fix or implementation already in dev waiting to be released and removed pending-release Fix or implementation already in dev waiting to be released labels Jul 18, 2021
@heitorlessa
Copy link
Contributor

Opa @marcioemiranda - This is now available in 1.18.0

More details in the release notes: https://github.com/awslabs/aws-lambda-powertools-python/releases/tag/v1.18.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants