[Internal] Refactor ApiClient into _BaseClient and ApiClient
#785
Conversation
|
|
||
|
|
||
| @contextlib.contextmanager | ||
| def http_fixture_server(handler: typing.Callable[[BaseHTTPRequestHandler], None]): |
There was a problem hiding this comment.
Moved here from test_core.py.
| from .fixture_server import http_fixture_server | ||
|
|
||
|
|
||
| class DummyResponse(requests.Response): |
There was a problem hiding this comment.
Moved here from test_core.py.
| (401, {}, { | ||
| 'error_code': 'UNAUTHORIZED', | ||
| 'message': 'errorMessage', | ||
| }, errors.Unauthenticated('errorMessage', error_code='UNAUTHORIZED')), | ||
| (403, {}, { | ||
| 'error_code': 'FORBIDDEN', | ||
| 'message': 'errorMessage', | ||
| }, errors.PermissionDenied('errorMessage', error_code='FORBIDDEN')), |
There was a problem hiding this comment.
These cases are slightly different because the ApiClient adds a debug string based on the Config when handling 401 and 403 errors. I haven't included that in this PR because that could induce a dependency on the Config.
| def test_error(config, requests_mock, status_code, headers, body, expected_error): | ||
| client = ApiClient(config) | ||
| requests_mock.get("/test", json=body, status_code=status_code, headers=headers) | ||
| with pytest.raises(DatabricksError) as raised: | ||
| client._perform("GET", "http://localhost/test", headers={"test": "test"}) |
There was a problem hiding this comment.
Note that after this PR, it isn't possible to call ApiClient._perform() anymore.
| pool_block=True, | ||
| http_timeout_seconds=cfg.http_timeout_seconds, | ||
| extra_error_customizers=[_AddDebugErrorCustomizer(cfg)], | ||
| clock=cfg.clock) | ||
|
|
||
| @property | ||
| def account_id(self) -> str: |
There was a problem hiding this comment.
What differentiates account_id and is_account_client from the rest of the things moved upwards? They're unused here so I'm assuming they are publicly facing, so I'm just curious on how to decide on which config attributes should be decoupled from ApiClient and which needs to stay.
There was a problem hiding this comment.
The way I thought about it: account_id and is_account_client are not inherently necessary for making API requests. In essence, they are really parameters of the config that happen to be in the ApiClient as well. The current architecture of the SDK doesn't expose the config directly to the *Api classes, so config-level attributes are "exposed" through the ApiClient class instead. However, fixing this would involve more invasive changes in the codegen process, which aren't necessary for now and will probably be revisited anyways with the next SDK version.
It doesn't seem like is_account_client is used within the SDK today, but it is publicly exposed. This is strongly coupled to the Config, because it requires knowledge of a host + logic to compute whether the provided host is an accounts host, which currently resides in the Config. In order to not break any callers, I left it as-is.
All of the other parameters are actually used inside of _BaseClient and affect its behavior, so they need to be included at that level.
### Bug Fixes * Fix Model Serving Test ([#781](#781)). * Include package name for external types when deserializing responses ([#786](#786)). ### Internal Changes * Refactor ApiClient into `_BaseClient` and `ApiClient` ([#785](#785)). * Update to latest OpenAPI spec ([#787](#787)). * revert Support Models in `dbutils.fs` operations ([#750](#750)) ([#778](#778)). ### API Changes: * Added [w.disable_legacy_dbfs](https://databricks-sdk-py.readthedocs.io/en/latest/workspace/settings/disable_legacy_dbfs.html) workspace-level service. * Added `default_source_code_path` and `resources` fields for `databricks.sdk.service.apps.App`. * Added `resources` field for `databricks.sdk.service.apps.CreateAppRequest`. * Added `resources` field for `databricks.sdk.service.apps.UpdateAppRequest`. OpenAPI SHA: bc17b474818138f19b78a7bea0675707dead2b87, Date: 2024-10-07
### Bug Fixes * Fix Model Serving Test ([#781](#781)). * Include package name for external types when deserializing responses ([#786](#786)). ### Internal Changes * Refactor ApiClient into `_BaseClient` and `ApiClient` ([#785](#785)). * Update to latest OpenAPI spec ([#787](#787)). * revert Support Models in `dbutils.fs` operations ([#750](#750)) ([#778](#778)). ### API Changes: * Added [w.disable_legacy_dbfs](https://databricks-sdk-py.readthedocs.io/en/latest/workspace/settings/disable_legacy_dbfs.html) workspace-level service. * Added `default_source_code_path` and `resources` fields for `databricks.sdk.service.apps.App`. * Added `resources` field for `databricks.sdk.service.apps.CreateAppRequest`. * Added `resources` field for `databricks.sdk.service.apps.UpdateAppRequest`. OpenAPI SHA: bc17b474818138f19b78a7bea0675707dead2b87, Date: 2024-10-07
Changes
ApiClientis also coupled to theConfigobject, which means that it can't be used in situations where there is no config. For example, when fetching OIDC endpoints, the user may not have a completeConfiginstance yet. However, failures when requesting from those endpoints should still be retried according to the SDK's retry policy.To address this, I've split the ApiClient into
_BaseClientandApiClient._BaseClientis the core implementation of the client without any dependency on theConfig. This is similar to what @rauchy did in the Java SDK to cut the dependency between theApiClientandDatabricksConfig. The_BaseClientcan then be used when fetching OIDC endpoint information.This will be used in #784 to support retrying OAuth OIDC endpoint fetches.
Tests
make testrun locallymake fmtapplied