-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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: Allow Proxy Paths to have arbitrary names #505
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add more test cases? The regex is comprehensive (ie. uses (.*)
), so it will be helpful to verify that the regex doesn't eat the whole string :)
samcli/local/apigw/path_converter.py
Outdated
# The regex captures any path information before the {proxy+}. This is to support paths that have other params in | ||
# them. Example: /id/{id}/user/{proxy+}. Otherwise the regex will match the first { with the last +} giving incorrect | ||
# results. | ||
APIGW_PATH_PARAMS_ESCAPED = r"(.*/){(.*)\+}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not any path param, but only wildcard ones ie. the ones with + at the end. Right?
Can you update the comment and variable name to be more descriptive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated name and added a full example.
The comment seems correct though. It is describing that it captures the path before the {proxy+}. Without this capture, the regex will match the first '{' and the last '+}', which is incorrect if there are path parameters in the full path.
Is there a part of this that is confusing that could be cleared up further?
samcli/local/apigw/path_converter.py
Outdated
# The regex replaces what was captured with APIGW_PATH_PARAMS_ESCAPED to construct the full path with the {proxy+} | ||
# replaces. The first group is anything before {proxy+}, while the second group is the name given to the proxy. | ||
# Example: /id/{id}/user/{resource+}; g<1> = '/id/{id}/user/'; g<2> = 'resource' | ||
FLASK_PATH_PARAMS = r"\g<1><path:\g<2>>" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: PATH_PARAMS
suffix isn't too clear. Consider dropping the suffix or changing to something else
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to: FLASK_CAPTURE_ALL_PATH
@@ -19,6 +19,13 @@ def test_proxy_path(self): | |||
|
|||
self.assertEquals(flask_path, "/<path:proxy>") | |||
|
|||
def test_proxy_path_with_different_name(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also test the following cases (mostly testing combination of single & multiple path params with wildcard
/a/{id}/b/{resource+}
/a/b/{proxy}/{resource+}
/{id}/{something+}
/{a}/{b}/{c}/{d+}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure can do :)
|
# The regex captures any path information before the {proxy+}. This is to support paths that have other params in | ||
# them. Otherwise the regex will match the first { with the last +} giving incorrect results. | ||
# Example: /id/{id}/user/{proxy+}. g<1> = '/id/{id}/user/' g<2> = 'proxy' | ||
PROXY_PATH_PARAMS_ESCAPED = r"(.*/){(.*)\+}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated example looks better to me :)
("/a/{id}/b/{resource+}", "/a/<id>/b/<path:resource>"), | ||
("/a/b/{proxy}/{resource+}", "/a/b/<proxy>/<path:resource>"), | ||
("/{id}/{something+}", "/<id>/<path:something>"), | ||
("/{a}/{b}/{c}/{d+}", "/<a>/<b>/<c>/<path:d>") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thanks for all the additional tests
Issue #, if available:
Addressed the bug from #457 where SAM CLI only accepted proxy paths with '{proxy+}'. This PR will allow 'proxy' to be any arbitrary name.
I ran the functional tests locally in both Py2 and Py3 (all passed in both).
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.