Allow base_url without service_name in clients (#786)

base_url can now be used to effectively skip the check for
`service_name == "_base"`, which has two significant impacts.

The first is simply that it is now possible to instantiate a
BaseClient directly, which may prove useful for ad-hoc scripting.
Although not generally a recommended approach (e.g. there is no
benefit to using a BaseClient in lieu of a TransferClient), the
possibility only makes the SDK more flexible.

The second effect is that a class inheriting from BaseClient no longer
needs to set `service_name` if the setting has no appropriate value.
This is relevant for 3rd party clients with nonstandard URL schemes
(e.g. FuncX before the Compute rebranding) and for clients to an array
of potential hosts, like GCSClient and any future
ActionProviderClient.

The only new test ensures that it is possible to instantiate a
BaseClient directly, and the GCSClient.service_name value, although
unchanged, is marked for a v4.0 update.

changelog.d/20230717_162222_sirosen_improve_url_enforcement_logic.rst

@@ -0,0 +1,13 @@
+Changed
+~~~~~~~
+
+- The enforcement logic for URLs in ``BaseClient`` instantiation has been
+  improved to only require that ``service_name`` be set if ``base_url`` is not
+  provided. (:pr:`NUMBER`)
+
+  - This change primarily impacts subclasses, which no longer need to set the
+    ``service_name`` class variable if they ensure that the ``base_url`` is
+    always passed with a non-null value.
+
+  - Direct instantiation of ``BaseClient`` is now possible, although not
+    recommended for most use-cases.

src/globus_sdk/client.py

@@ -27,6 +27,10 @@ class BaseClient:
         semantics of client actions. It is just passed as part of the User-Agent
         string, and may be useful when debugging issues with the Globus Team
     :type app_name: str
+    :param base_url: The URL for the service. Most client types initialize this value
+        intelligently by default. Set it when inheriting from BaseClient or
+        communicating through a proxy.
+    :type base_url: str
     :param transport_params: Options to pass to the transport for this client
     :type transport_params: dict
 
@@ -56,38 +60,41 @@ def __init__(
         app_name: str | None = None,
         transport_params: dict[str, t.Any] | None = None,
     ):
-        # explicitly check the `service_name` to ensure that it was set
-        #
-        # unfortunately, we can't rely on declaring BaseClient as an ABC because it
-        # doesn't have any abstract methods
-        #
-        # if we declare `service_name` without a value, we get AttributeError on access
-        # instead of the (desired) TypeError when instantiating a BaseClient because
-        # it's abstract
-        if self.service_name == "_base":
-            raise NotImplementedError(
-                "Cannot instantiate clients which do not set a 'service_name'"
-            )
-        log.info(
-            f'Creating client of type {type(self)} for service "{self.service_name}"'
-        )
-
         # if an environment was passed, it will be used, but otherwise lookup
         # the env var -- and in the special case of `production` translate to
         # `default`, regardless of the source of that value
         # logs the environment when it isn't `default`
         self.environment = config.get_environment_name(environment)
 
+        if self.service_name == "_base":
+            # service_name=="_base" means that either there was no inheritance (direct
+            # instantiation of BaseClient), or the inheriting class operates outside of
+            # the existing service_name->URL mapping paradigm
+            # in these cases, base_url must be set explicitly
+            log.info(f"Creating client of type {type(self)}")
+            if base_url is None:
+                raise NotImplementedError(
+                    "Cannot instantiate clients which do not set a 'service_name' "
+                    "unless they explicitly set 'base_url'."
+                )
+        else:
+            # if service_name is set, it can be used to deduce a base_url
+            # *if* necessary
+            log.info(
+                f"Creating client of type {type(self)} for "
+                f'service "{self.service_name}"'
+            )
+            if base_url is None:
+                base_url = config.get_service_url(
+                    self.service_name, environment=self.environment
+                )
+
+        # append the base_path to the base URL
+        self.base_url = utils.slash_join(base_url, self.base_path)
+
         self.transport = self.transport_class(**(transport_params or {}))
         log.debug(f"initialized transport of type {type(self.transport)}")
 
-        self.base_url = utils.slash_join(
-            config.get_service_url(self.service_name, environment=self.environment)
-            if base_url is None
-            else base_url,
-            self.base_path,
-        )
-
         self.authorizer = authorizer
 
         # set application name if given

src/globus_sdk/services/gcs/client.py

@@ -36,6 +36,7 @@ class GCSClient(client.BaseClient):
     .. automethodlist:: globus_sdk.GCSClient
     """
 
+    # TODO: under SDK v4.0, service_name should not be set
     service_name = "globus_connect_server"
     error_class = GCSAPIError
 

tests/unit/test_base_client.py

@@ -42,6 +42,14 @@ def test_cannot_instantiate_plain_base_client():
         globus_sdk.BaseClient()
 
 
+def test_can_instantiate_base_client_with_explicit_url():
+    # note how a trailing slash is added due to the default
+    # base_path of '/'
+    # this may change in a future major version, to preserve the base_url exactly
+    client = globus_sdk.BaseClient(base_url="https://example.org")
+    assert client.base_url == "https://example.org/"
+
+
 def test_set_http_timeout(base_client):
     class FooClient(globus_sdk.BaseClient):
         service_name = "foo"