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

fix(api-gateway): non-greedy route pattern regex #533

Conversation

heitorlessa
Copy link
Contributor

@heitorlessa heitorlessa commented Jul 18, 2021

Issue #, if available: #520

Description of changes:

This fixes a regex bug that used a greedy pattern ending with incorrect path resolution, as any path after a pattern would be included in the argument.

Excerpt:

@app.get("/accounts/<account_id>")
def get_account(account_id: str):
    print(f"Account ID ({account_id}) would be 123")

# Greedy regex would inject the incorrect function parameter
@app.get("/accounts/<account_id>/source_networks")
def get_account_networks(account_id: str):
    print(f"Account ID ({account_id}) would be 123/source_networks")

In this example, say we have a GET request as /accounts/123 and another as /accounts/123/source_networks, we'd have the following effect prior to this fix:

Function Regex Effective account_id value
get_account r'^/accounts/(?P<account_id>.+)$' 123
get_account_networks r'^/accounts/(?P<account_id>.+)/source_networks$' 123/source_networks

With this fix, account_id parameter would be 123 in both occasions due to word boundary not being non-greedy. This also allows an arbitrary number of dynamic route paths and static route paths.

Function Regex Effective account_id value
get_account r'^/accounts/(?P<account_id>\\w+\\b)$' 123
get_account_networks r'^/accounts/(?P<account_id>\\w+\\b)/source_networks$' 123

Checklist

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

@codecov-commenter
Copy link

codecov-commenter commented Jul 18, 2021

Codecov Report

Merging #533 (59d91e9) into develop (018a91b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #533   +/-   ##
========================================
  Coverage    99.19%   99.19%           
========================================
  Files          113      113           
  Lines         4476     4478    +2     
  Branches       243      243           
========================================
+ Hits          4440     4442    +2     
  Misses          24       24           
  Partials        12       12           
Impacted Files Coverage Δ
aws_lambda_powertools/event_handler/api_gateway.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 018a91b...59d91e9. Read the comment docs.

@heitorlessa heitorlessa added this to the 1.18.0 milestone Jul 18, 2021
@heitorlessa
Copy link
Contributor Author

Note: one other bug and potential minor issue to address as part of 1.18 release or the next one after that

  1. Raise an exception if the same function has been registered for a separate route rule

  2. App.route decorator doesn't preserve fn signature and will likely fail Mypy like tracer.capture_method older bug

@heitorlessa heitorlessa merged commit 89e8151 into aws-powertools:develop Jul 18, 2021
@heitorlessa heitorlessa deleted the fix/event-handler-api-nested-routing branch July 19, 2021 07:40
heitorlessa added a commit to whardier/aws-lambda-powertools-python that referenced this pull request Jul 19, 2021
…ent-subclass

* develop:
  fix(api-gateway): non-greedy route pattern regex (aws-powertools#533)
  chore(deps): bump boto3 from 1.18.0 to 1.18.1 (aws-powertools#528)
  fix(tracer): mypy generic to preserve decorated method signature (aws-powertools#529)
  fix(parser): Make ApiGateway version, authorizer fields optional (aws-powertools#532)
  fix(mypy): fixes to resolve no implicit optional errors (aws-powertools#521)
  chore(deps): bump boto3 from 1.17.110 to 1.18.0 (aws-powertools#527)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants