-
Notifications
You must be signed in to change notification settings - Fork 27
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: strip trailing slashes in BaseService.set_service_url #130
Conversation
This commit modifies the BaseService.set_service_url() function so that it removes any trailing slashes. Previously, a service URL containing a trailing slash (e.g. "https://myserver.com/api/v1/") would result in a request URL that contains two slashes between the service URL and the operation path (e.g. "https://myserver.com/api/v1//myoperationpath"), and some server implementations might fail to process such a request properly. With the changes in this commit, a request URL of "https://myserver.com/api/v1/myoperationpath" will be used instead.
|
||
|
||
def test_trailing_slash(): |
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.
moved this to test_utils.py
Codecov Report
@@ Coverage Diff @@
## main #130 +/- ##
=======================================
Coverage 99.66% 99.66%
=======================================
Files 24 24
Lines 906 908 +2
=======================================
+ Hits 903 905 +2
Misses 3 3
Continue to review full report at Codecov.
|
assert strip_extra_slashes('https://host/path//') == 'https://host/path/' | ||
assert strip_extra_slashes('https://host//path//') == 'https://host//path/' | ||
|
||
def test_service_url_handling(): |
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.
Removed the direct calls to strip_extra_slashes() so this test will focus on the service and service_url
req = service.prepare_request('POST', | ||
url='/path/', | ||
headers={'X-opt-out': True}, | ||
data={'hello': 'world'}) | ||
assert req.get('url') == 'https://host//path/' |
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.
we can no longer have double-slashes between the service_url and the operation path
Reminder to myself: before we merge this, we need to test this version of the core with existing generated unit tests. Edit: The platform-services python SDK's unit tests ran clean using my local copy of the python core. |
I also did a test with the platform-services python SDK where I ran the global-search integration test with and without the python core fix.
so the path value passed in the HTTP request message does in fact have a leading double-slash (the global-search server still processes this request just fine, but not all servers behave this way). In the "after" scenario (with the python core fix), I see this in the http debug messages:
so the path value now has just a single slash. |
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.
LGTM
## [3.13.1](v3.13.0...v3.13.1) (2021-11-15) ### Bug Fixes * strip trailing slashes in BaseService.set_service_url ([#130](#130)) ([37d0099](37d0099))
🎉 This PR is included in version 3.13.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This commit modifies the BaseService.set_service_url()
function so that it removes any trailing slashes.
Previously, a service URL containing a trailing slash
(e.g. "https://myserver.com/api/v1/") would result in a
request URL that contains two slashes between the service URL
and the operation path (e.g. "https://myserver.com/api/v1//myoperationpath"),
and some server implementations might fail to process such
a request properly.
With the changes in this commit, a request URL of
"https://myserver.com/api/v1/myoperationpath" will be used instead.