From 008a5347a9840ce329fbd417882ffc87ab517c49 Mon Sep 17 00:00:00 2001 From: Stephen Rosen Date: Tue, 18 Jul 2023 12:24:26 -0500 Subject: [PATCH] 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. --- ..._sirosen_improve_url_enforcement_logic.rst | 13 +++++ src/globus_sdk/client.py | 53 +++++++++++-------- src/globus_sdk/services/gcs/client.py | 1 + tests/unit/test_base_client.py | 8 +++ 4 files changed, 52 insertions(+), 23 deletions(-) create mode 100644 changelog.d/20230717_162222_sirosen_improve_url_enforcement_logic.rst diff --git a/changelog.d/20230717_162222_sirosen_improve_url_enforcement_logic.rst b/changelog.d/20230717_162222_sirosen_improve_url_enforcement_logic.rst new file mode 100644 index 000000000..315d53a77 --- /dev/null +++ b/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. diff --git a/src/globus_sdk/client.py b/src/globus_sdk/client.py index 838a6ac8c..e11081904 100644 --- a/src/globus_sdk/client.py +++ b/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 diff --git a/src/globus_sdk/services/gcs/client.py b/src/globus_sdk/services/gcs/client.py index c7e8e8990..27cbb006b 100644 --- a/src/globus_sdk/services/gcs/client.py +++ b/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 diff --git a/tests/unit/test_base_client.py b/tests/unit/test_base_client.py index a50a233dd..ae39d0c16 100644 --- a/tests/unit/test_base_client.py +++ b/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"