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

ApiGatewayResolver root routes and strip_prefixes #646

Closed
BVMiko opened this issue Aug 23, 2021 · 6 comments
Closed

ApiGatewayResolver root routes and strip_prefixes #646

BVMiko opened this issue Aug 23, 2021 · 6 comments
Assignees
Labels
bug Something isn't working

Comments

@BVMiko
Copy link
Contributor

BVMiko commented Aug 23, 2021

I ran into an issue when using a route with the rule "/" along with strip_prefixes feature. This rule is unable to trigger due to the path variable being reduced down to an empty string instead of a forward slash.

from aws_lambda_powertools.event_handler.api_gateway import ApiGatewayResolver
app = ApiGatewayResolver(strip_prefixes=["/some-path"])
@app.get('/')
def my_func():
    return {'execution_status':'success'}

Expected Behavior

Accessing domain.com/some-path would execute my_func().

Current Behavior

Accessing domain.com/some-path returns a 404. Of note though, accessing domain.com/some-path/ (note the trailing slash) will execute my_func().

Possible Solution

Add an conditional here which will map an empty string to "/".

Steps to Reproduce (for bugs)

  1. Set up the code example above
  2. Set up ApiGateway to use a custom domain, and bind the Lambda to a custom path of some-path
  3. Visit https://CUSTOMDOMAIN/some-path and https://CUSTOMDOMAIN/some-path/ and note the different results
@BVMiko BVMiko added bug Something isn't working triage Pending triage from maintainers labels Aug 23, 2021
@michaelbrewer
Copy link
Contributor

Nice one @BVMiko , i put in a review comment for adding a test case.

Otherwise, i was thinking whether it should be "" instead of "/"? Then this would work when the request is /some-path

from aws_lambda_powertools.event_handler.api_gateway import ApiGatewayResolver
app = ApiGatewayResolver(strip_prefixes=["/some-path"])
@app.get("")
def my_func():
    return {'execution_status':'success'}

As the original request path of /some-path does different from /some-path/. Although @app.get("") does like strange.

@to-mc to-mc self-assigned this Aug 24, 2021
@BVMiko
Copy link
Contributor Author

BVMiko commented Aug 25, 2021

I believe that the common standard among other routing frameworks is that all paths should have a leading slash, so @api.get("/") is usually the root path. Unfortunately the Flask and FastAPI docs don't explicitly state this, though all examples are written this way. I did find this Flask issue describing a related oversight of forgetting to enforce a leading slash though.

As for implementation, Flask and FastAPI both seem to do things differently by building and caching the full paths within each route. However ExpressJS, looks like it handles routing similar to the current ApiGatewayRouter though it does ensure a leading slash.

@michaelbrewer
Copy link
Contributor

@BVMiko - agreed. But strip_prefix is supposed to remove the matching prefix from the request.

And that this is on the context of "strip_prefixes" is "/custom-mapping", and the actual request is without a trailing "/" and would look like "/custom-mapping". So naturally I would except the resulting path to be "".

Internally this was not happening due to the addition of a "/" for the starts with check. Because we wanted to allow for "/pay" and "/payment" strip prefixes to be set in any order. As "/pay" would be a partial match. It would remove before "/payment".

So the ultimate solution could just be to reverse the order of the prefixes and remove the "/" we add in the starts with check.

@michaelbrewer
Copy link
Contributor

@BVMiko - thinking it through. If the request path resolves to a empty string (""), after the strip prefixes. Then it makes sense that the base is now actually "/".

We just need to document that there is no difference between a request path of "/pay" and "/pay/" when the strip prefix is "/pay".

@BVMiko
Copy link
Contributor Author

BVMiko commented Aug 31, 2021

@michaelbrewer - I updated the testcase and the docs to reflect the new explicit change; let me know if you'd like the wording modified.

@to-mc
Copy link
Contributor

to-mc commented Sep 10, 2021

Fixed in v1.20.2.

@to-mc to-mc closed this as completed Sep 10, 2021
@to-mc to-mc removed the triage Pending triage from maintainers label Sep 10, 2021
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

3 participants