fix: strip trailing slashes in BaseService.set_service_url (#130)

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.

Author: padamstx

ibm_cloud_sdk_core/base_service.py

@@ -224,6 +224,8 @@ def set_service_url(self, service_url: str) -> None:
                 'The service url shouldn\'t start or end with curly brackets or quotes. '
                 'Be sure to remove any {} and \" characters surrounding your service url'
             )
+        if service_url is not None:
+            service_url = service_url.rstrip('/')
         self.service_url = service_url
 
     def get_http_client(self) -> requests.sessions.Session:
@@ -368,6 +370,10 @@ def prepare_request(self,
         # validate the service url is set
         if not self.service_url:
             raise ValueError('The service_url is required')
+
+        # Combine the service_url and operation path to form the request url.
+        # Note: we have already stripped any trailing slashes from the service_url
+        # and we know that the operation path ('url') will start with a slash.
         request['url'] = strip_extra_slashes(self.service_url + url)
 
         headers = remove_null_values(headers) if headers else {}

test/test_base_service.py

@@ -20,7 +20,6 @@
 from ibm_cloud_sdk_core import get_authenticator_from_environment
 from ibm_cloud_sdk_core.authenticators import (IAMAuthenticator, NoAuthAuthenticator, Authenticator,
                                                BasicAuthenticator, CloudPakForDataAuthenticator)
-from ibm_cloud_sdk_core.utils import strip_extra_slashes
 
 
 class IncludeExternalConfigService(BaseService):
@@ -698,6 +697,7 @@ def test_user_agent_header():
     assert response.get_result().request.headers.__getitem__(
         'user-agent') == user_agent_header['User-Agent']
 
+
 @responses.activate
 def test_reserved_keys(caplog):
     service = AnyServiceV1('2021-07-02', authenticator=NoAuthAuthenticator())
@@ -723,6 +723,7 @@ def test_reserved_keys(caplog):
     assert caplog.record_tuples[2][2] == '"headers" has been removed from the request'
     assert caplog.record_tuples[3][2] == '"cookies" has been removed from the request'
 
+
 @responses.activate
 def test_ssl_error():
     responses.add(
@@ -810,56 +811,71 @@ def test_json():
     service = AnyServiceV1('2018-11-20', authenticator=NoAuthAuthenticator())
     req = service.prepare_request('POST', url='', headers={
                                   'X-opt-out': True}, data={'hello': 'world', 'fóó': 'bår'})
-    assert req.get('data') == b'{"hello": "world", "f\\u00f3\\u00f3": "b\\u00e5r"}'
+    assert req.get(
+        'data') == b'{"hello": "world", "f\\u00f3\\u00f3": "b\\u00e5r"}'
 
 
-def test_trailing_slash():
-    assert strip_extra_slashes('') == ''
-    assert strip_extra_slashes('//') == '/'
-    assert strip_extra_slashes('/////') == '/'
-    assert strip_extra_slashes('https://host') == 'https://host'
-    assert strip_extra_slashes('https://host/') == 'https://host/'
-    assert strip_extra_slashes('https://host//') == 'https://host/'
-    assert strip_extra_slashes('https://host/path') == 'https://host/path'
-    assert strip_extra_slashes('https://host/path/') == 'https://host/path/'
-    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():
     service = AnyServiceV1(
-        '2018-11-20', service_url='https://host/', authenticator=NoAuthAuthenticator())
-    assert service.service_url == 'https://host/'
+        '2018-11-20', service_url='https://host///////', authenticator=NoAuthAuthenticator())
+    assert service.service_url == 'https://host'
+
     service.set_service_url('https://host/')
-    assert service.service_url == 'https://host/'
+    assert service.service_url == 'https://host'
+
     req = service.prepare_request('POST',
                                   url='/path/',
                                   headers={'X-opt-out': True},
                                   data={'hello': 'world'})
-    assert req.get('url') == 'https://host//path/'
+    assert req.get('url') == 'https://host/path/'
 
     service = AnyServiceV1(
         '2018-11-20', service_url='https://host/', authenticator=NoAuthAuthenticator())
-    assert service.service_url == 'https://host/'
+    assert service.service_url == 'https://host'
+
     service.set_service_url('https://host/')
-    assert service.service_url == 'https://host/'
+    assert service.service_url == 'https://host'
+
     req = service.prepare_request('POST',
                                   url='/',
                                   headers={'X-opt-out': True},
                                   data={'hello': 'world'})
     assert req.get('url') == 'https://host/'
 
+    req = service.prepare_request('POST',
+                                  url='////',
+                                  headers={'X-opt-out': True},
+                                  data={'hello': 'world'})
+    assert req.get('url') == 'https://host/'
+
     service.set_service_url(None)
     assert service.service_url is None
 
     service = AnyServiceV1('2018-11-20', service_url='/',
                            authenticator=NoAuthAuthenticator())
-    assert service.service_url == '/'
+    assert service.service_url == ''
+
     service.set_service_url('/')
-    assert service.service_url == '/'
-    req = service.prepare_request('POST',
-                                  url='/',
-                                  headers={'X-opt-out': True},
-                                  data={'hello': 'world'})
-    assert req.get('url') == '/'
+    assert service.service_url == ''
+
+    with pytest.raises(ValueError) as err:
+        service.prepare_request('POST',
+                                url='/',
+                                headers={'X-opt-out': True},
+                                data={'hello': 'world'})
+    assert str(err.value) == 'The service_url is required'
+
+
+def test_service_url_slash():
+    service = AnyServiceV1('2018-11-20', service_url='/',
+                           authenticator=NoAuthAuthenticator())
+    assert service.service_url == ''
+    with pytest.raises(ValueError) as err:
+        service.prepare_request('POST',
+                                url='/',
+                                headers={'X-opt-out': True},
+                                data={'hello': 'world'})
+    assert str(err.value) == 'The service_url is required'
 
 
 def test_service_url_not_set():

test/test_utils.py

@@ -28,6 +28,7 @@
 from ibm_cloud_sdk_core import get_query_param
 from ibm_cloud_sdk_core import read_external_sources
 from ibm_cloud_sdk_core.authenticators import Authenticator, BasicAuthenticator, IAMAuthenticator
+from ibm_cloud_sdk_core.utils import strip_extra_slashes
 
 
 def datetime_test(datestr: str, expected: str):
@@ -616,3 +617,18 @@ def test_read_external_sources_2():
 
     config = read_external_sources('service_1')
     assert config.get('URL') == 'service1.com/api'
+
+
+def test_strip_extra_slashes():
+    assert strip_extra_slashes('') == ''
+    assert strip_extra_slashes('//') == '/'
+    assert strip_extra_slashes('/////') == '/'
+    assert strip_extra_slashes('https://host') == 'https://host'
+    assert strip_extra_slashes('https://host/') == 'https://host/'
+    assert strip_extra_slashes('https://host//') == 'https://host/'
+    assert strip_extra_slashes('https://host/path') == 'https://host/path'
+    assert strip_extra_slashes('https://host/path/') == 'https://host/path/'
+    assert strip_extra_slashes('https://host/path//') == 'https://host/path/'
+    assert strip_extra_slashes('https://host//path//') == 'https://host//path/'
+    assert strip_extra_slashes(
+        'https://host//path//////////') == 'https://host//path/'