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: Fix issue with strip_prefixes #647

Merged
merged 3 commits into from
Sep 2, 2021

Conversation

BVMiko
Copy link
Contributor

@BVMiko BVMiko commented Aug 23, 2021

Issue #, if available: #646

Description of changes:

After removing path prefixes, allow an empty path to return "/" so that it will match a normal routing rule.

Checklist

Breaking change checklist

RFC issue #:

  • Migration process documented
  • Implement warnings (if it can live side by side)

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

@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 23, 2021
@michaelbrewer
Copy link
Contributor

@BVMiko - Nice catch!

Could you add a test case? Something like:

def test_api_gateway_request_path_equals_strip_prefix():
    # GIVEN a strip_prefix matches the request path
    app = ApiGatewayResolver(strip_prefixes=["/foo"])
    event = {"httpMethod": "GET", "path": "/foo"}

    @app.get("/")
    def base():
        return {}

    # WHEN calling the event handler
    # WITH a route "/"
    result = app(event, {})

    # THEN process event correctly
    assert result["statusCode"] == 200
    assert result["headers"]["Content-Type"] == content_types.APPLICATION_JSON

Regarding for the fix, i guess it could also default to "" over "/", but i would defer that to @heitorlessa

@michaelbrewer
Copy link
Contributor

@BVMiko - FYI, Heitor will be about for 5 weeks. How critical would a fix for this be?

@BVMiko
Copy link
Contributor Author

BVMiko commented Aug 23, 2021

For now I have a workaround in place which can remain until this issue is resolved:

app = ApiGatewayResolver(strip_prefixes=["/foo"])

@app.get("/")
def base():
    return {}

def lambda_handler(event, context):
    if event['path'] == '/foo':
        event['path'] = '/foo/'
    return app.resolve(event, context)

@heitorlessa heitorlessa added the bug Something isn't working label Aug 23, 2021
@codecov-commenter
Copy link

Codecov Report

Merging #647 (a8857e0) into develop (c8cf3ba) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #647      +/-   ##
===========================================
- Coverage    99.97%   99.93%   -0.05%     
===========================================
  Files          116      116              
  Lines         4846     4821      -25     
  Branches       265      266       +1     
===========================================
- Hits          4845     4818      -27     
- Misses           0        1       +1     
- Partials         1        2       +1     
Impacted Files Coverage Δ
aws_lambda_powertools/event_handler/api_gateway.py 99.00% <0.00%> (-1.00%) ⬇️
aws_lambda_powertools/logging/logger.py 100.00% <0.00%> (ø)
aws_lambda_powertools/tracing/tracer.py 100.00% <0.00%> (ø)
aws_lambda_powertools/metrics/metrics.py 100.00% <0.00%> (ø)
aws_lambda_powertools/logging/formatter.py 100.00% <0.00%> (ø)
aws_lambda_powertools/utilities/batch/base.py 100.00% <0.00%> (ø)
aws_lambda_powertools/shared/jmespath_utils.py 100.00% <0.00%> (ø)
...ws_lambda_powertools/middleware_factory/factory.py 100.00% <0.00%> (ø)
...s_lambda_powertools/utilities/parser/models/sns.py 100.00% <0.00%> (ø)
..._lambda_powertools/utilities/feature_flags/base.py 100.00% <0.00%> (ø)
... and 5 more

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 c8cf3ba...a8857e0. Read the comment docs.

@michaelbrewer
Copy link
Contributor

@BVMiko - Nice catch!

Could you add a test case? Something like:

def test_api_gateway_request_path_equals_strip_prefix():
    # GIVEN a strip_prefix matches the request path
    app = ApiGatewayResolver(strip_prefixes=["/foo"])
    event = {"httpMethod": "GET", "path": "/foo"}

    @app.get("/")
    def base():
        return {}

    # WHEN calling the event handler
    # WITH a route "/"
    result = app(event, {})

    # THEN process event correctly
    assert result["statusCode"] == 200
    assert result["headers"]["Content-Type"] == content_types.APPLICATION_JSON

Regarding for the fix, i guess it could also default to "" over "/", but i would defer that to @heitorlessa

@BVMiko - would you be able to include a test like this into the api gateway handler test suite?

@pull-request-size pull-request-size bot removed the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 31, 2021
@boring-cyborg boring-cyborg bot added the tests label Aug 31, 2021
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 31, 2021
@BVMiko
Copy link
Contributor Author

BVMiko commented Aug 31, 2021

@michaelbrewer: The test has been added

@boring-cyborg boring-cyborg bot added the documentation Improvements or additions to documentation label Aug 31, 2021
Copy link
Contributor

@michaelbrewer michaelbrewer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks @BVMiko

@to-mc to-mc merged commit 6dee33a into aws-powertools:develop Sep 2, 2021
@boring-cyborg
Copy link

boring-cyborg bot commented Sep 2, 2021

Awesome work, congrats on your first merged pull request and thank you for helping improve everyone's experience!

heitorlessa added a commit to heitorlessa/aws-lambda-powertools-python that referenced this pull request Sep 28, 2021
…tools-python into develop

* 'develop' of https://github.com/awslabs/aws-lambda-powertools-python:
  docs(event-handler): document catch-all routes (aws-powertools#705)
  chore: add python 3.9 support
  docs: add team behind it and email
  ISSUE-693: Use ExpressionAttributeNames in _put_record (aws-powertools#697)
  feat(validator): include missing data elements from a validation error (aws-powertools#686)
  chore(deps-dev): bump mkdocs-material from 7.2.8 to 7.3.0 (aws-powertools#695)
  chore(deps-dev): bump mkdocs-material from 7.2.6 to 7.2.8 (aws-powertools#682)
  chore(deps-dev): bump flake8-bugbear from 21.4.3 to 21.9.1 (aws-powertools#676)
  chore(deps): bump boto3 from 1.18.38 to 1.18.41 (aws-powertools#677)
  chore(deps-dev): bump radon from 4.5.2 to 5.1.0 (aws-powertools#673)
  chore(deps): bump boto3 from 1.18.32 to 1.18.38 (aws-powertools#671)
  refactor(data-classes): clean up internal logic for APIGatewayAuthorizerResponse (aws-powertools#643)
  fix(data-classes): use correct asdict funciton (aws-powertools#666)
  chore(deps-dev): bump xenon from 0.7.3 to 0.8.0 (aws-powertools#669)
  chore: bump to 1.20.2
  fix: Fix issue with strip_prefixes (aws-powertools#647)
  chore(deps-dev): bump mkdocs-material from 7.2.4 to 7.2.6 (aws-powertools#665)
  chore(deps): bump boto3 from 1.18.26 to 1.18.32 (aws-powertools#663)
  chore(deps-dev): bump pytest from 6.2.4 to 6.2.5 (aws-powertools#662)
  chore(license): Add THIRD-PARTY-LICENSES (aws-powertools#641)
@BVMiko BVMiko deleted the strip_prefixes-fix branch October 1, 2021 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation size/S Denotes a PR that changes 10-29 lines, ignoring generated files. tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants