fix: Allow Proxy Paths to have arbitrary names #505
Conversation
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.
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.
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.
nit: PATH_PARAMS suffix isn't too clear. Consider dropping the suffix or changing to something else
There was a problem hiding this comment.
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.
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+}
|
| # 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.
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>") |
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.