Core: Fix BaseResponse.uri_to_regexp #8193
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8193 +/- ##
=======================================
Coverage 94.53% 94.53%
=======================================
Files 1155 1155
Lines 99714 99707 -7
=======================================
- Hits 94266 94261 -5
+ Misses 5448 5446 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
bblommers
added
the
moto-core
|
Hello @bblommers , I checked the failing tests and they are also failing without my changes. (Using the current moto main branch). The first 9 tests have in common that they use the old requests library 0.15. The tests using 0.17.0 succeed. Can you please check? |
moto/core/responses.py
Outdated
| if not re.match("^{.*}$", elem): | ||
| # URL-parts sometimes contain a $ | ||
| # Like Greengrass: /../deployments/$reset | ||
| # We don't want to our regex to think this marks an end-of-line, so let's escape it | ||
| return elem.replace("$", r"\$") | ||
|
|
||
| # When the element ends with +} the parameter can contain a / otherwise not. | ||
| no_slash_allowed = False if "+" == elem[-2:-1] else True |
There was a problem hiding this comment.
Any objections to making the naming positive, and having a more direct check? Something like this is more readable IMO:
slash_allowed = elem.endswith("+}")
if slash_allowed:
return ...
return ...
|
Don't worry about the tests @MSSputnik. I've restarted them a few times to check the performance, as regex changes can make quite a big impact. But that all looks good as far as I'm concerned - the tests just happen to be particularly flaky today. It is a good change though! I did look into this recently as well, because some ARN's weren't being matched properly, but I didn't notice the difference in behaviour between The only question I had was around variable naming essentially - having a negative variable isn't always easy to read, and having a double negative ( |
|
I updated the condition. |
bblommers
changed the title
|
This is now part of moto >= 5.0.17.dev16 |
Found an issue on how the URIs send by the boto3 client converted into function names.
I have 2 URI:
/accounts/{AwsAccountId}/namespaces/{Namespace}/groups/accounts/{AwsAccountId}/namespaces/{Namespace}/users/{UserName}/groupsThese URIs were converted into these regular expressions:
/accounts/(?P<AwsAccountId>.*)/namespaces/(?P<Namespace>.*)/groups/accounts/(?P<AwsAccountId>.*)/namespaces/(?P<Namespace>.*)/users/(?P<UserName>.*)/groupsUnfortunately both request URIs match regular expression 1:
/accounts/1235/namespaces/default/groups/accounts/123456789012/namespaces/default/users/authoremail%40example.com/groupsInvestigating on how AWS defines the requestUri in the service.json files I found that there are 2 types of parameters.
/accounts/{AwsAccountId}/namespaces/{Namespace}/users/{UserName}/groups/portals/{portalArn+}/trustStoresAs you can see in the requestUri from workspaceweb there is a + at the end of the parameter. Checking the code this means, that the parameter can contain a / as it is a Arn. The same I found in the S3 API.
To fix this issue I changed the function BaseResponse.uri_to_regexp to replace the parameter without the + at the end with
(?P<{name}>[^/]+)to allow any text but no /. For the parameter with a + I used(?P<{name}>.+)to allow any character with minimum length of 1.I found the usage of .* is not recommended as this causes issues with URIs form the mediastoredata API where
^/$is ListItems and^/{Path+}$is GetObject.On my development machine most of the test ran successfully. In local mode 2 tests fail and in server mode 3 tests fail. Checking this failuers, I think they are unrelated to this change and therefore I created this pull request.