-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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 requests mock for custom S3 endpoints #7445
Fix requests mock for custom S3 endpoints #7445
Conversation
Prior to the commit that introduced mock_aws (a7f3b36), it was possible to use the requests mock to access S3 presigned URLs that had a custom endpoint. This commit restores that behavior by applying the hostname-remapping logic from BotocoreStubber to CustomRegistry.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7445 +/- ##
==========================================
- Coverage 95.88% 95.80% -0.08%
==========================================
Files 843 854 +11
Lines 82578 83971 +1393
==========================================
+ Hits 79178 80451 +1273
- Misses 3400 3520 +120
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
url_with_standard_aws_domain = urlunparse( | ||
get_equivalent_url_in_aws_domain(request.url) | ||
) | ||
request_with_standard_aws_domain = request.copy() |
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.
Copying and preparing the URL is only necessary in some cases, but it currently happens for every incoming HTTP request. The possible performance downsides of that worry me a little.
Can we make the copy+prepare conditional, and only execute that part if the standardized URL is different from the original?
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.
Yeah that's a good call. There's no guarantee that URLs will roundtrip unchanged through urlparse/urlunparse, so I put the modification check a little further up. That will also save some string copying.
Avoid an unnecessary copy and modification of the request object in cases where no URL remapping occurred.
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 - thank you for fixing this @dhirving!
This is now part of moto >= 5.0.4.dev10 |
Prior to the commit that introduced mock_aws (a7f3b36), it was possible to use the requests mock to access S3 presigned URLs that had a custom endpoint.
This commit restores that behavior by applying the hostname-remapping logic from BotocoreStubber to CustomRegistry.