Skip to content

Conversation

joe4dev
Copy link
Member

@joe4dev joe4dev commented Feb 22, 2023

This PR fixes an rest api parity issue related to encoding paths in URL.

Actual behavior

Given the following invocation URL against a rest api (e.g., with {proxy+} integration and a lambda function as backend):

GET https://{restapi_id}.execute-api.{region}.amazonaws.com/{stage_name}/user/test%2Balias@gmail.com/plus/test+alias@gmail.com?email=test%2Balias@gmail.com

  • path: "/{stage_name}/user/test+alias@gmail.com/plus/test+alias@gmail.com?email=test%2Balias@gmail.com"
  • queryStringParameters.email: test+alias@gmail.com

Expected behavior

The base path should not decode URL encoded characters such as %2B => + and looks like this in AWS: /user/test%2Balias@gmail.com/plus/test+alias@gmail.com

Implementation

Instead of relying on decoded request.path or request.full_path from Werkzeug, we need to compose the invocation path manually consisting of a non-decoded base part and a decode params part.

Questions

  • When can url_params be None?
  • Can we assume defaults for non-present stage and path variables or should we fail fast?
  • Does the current solution work with custom domain names where the stage_name is omitted?
  • Any other special cases to consider?

@joe4dev joe4dev requested a review from calvernaz as a code owner February 22, 2023 10:48
@joe4dev joe4dev temporarily deployed to localstack-ext-tests February 22, 2023 10:48 — with GitHub Actions Inactive
@github-actions
Copy link

github-actions bot commented Feb 22, 2023

LocalStack integration with Pro

       3 files  ±  0         3 suites  ±0   1h 40m 49s ⏱️ + 7m 51s
1 753 tests +14  1 385 ✔️ +1  368 💤 +13  0 ±0 
2 471 runs  +14  1 761 ✔️ +1  710 💤 +13  0 ±0 

Results for commit b78bdb8. ± Comparison against base commit 8eaabbd.

♻️ This comment has been updated with latest results.

@calvernaz calvernaz temporarily deployed to localstack-ext-tests February 23, 2023 10:27 — with GitHub Actions Inactive
@calvernaz calvernaz temporarily deployed to localstack-ext-tests February 23, 2023 10:29 — with GitHub Actions Inactive
@calvernaz calvernaz temporarily deployed to localstack-ext-tests February 23, 2023 15:07 — with GitHub Actions Inactive
@coveralls
Copy link

Coverage Status

Coverage: 84.999% (-0.2%) from 85.158% when pulling b78bdb8 on fix-apigateway-path-encoding into 8eaabbd on master.

Copy link
Contributor

@calvernaz calvernaz left a comment

Choose a reason for hiding this comment

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

Thanks for adding the parity test @joe4dev, we should have a third-party approving this PR 👍

@calvernaz calvernaz requested a review from whummer February 23, 2023 17:10
Copy link
Member

@whummer whummer left a comment

Choose a reason for hiding this comment

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

Awesome, great to see this covered with a snapshot @joe4dev @calvernaz ! 🚀

@calvernaz calvernaz merged commit a820acf into master Feb 24, 2023
@calvernaz calvernaz deleted the fix-apigateway-path-encoding branch February 24, 2023 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants