Propagate request headers to ApiClient #302

mgyucht · 2023-08-19T05:13:37Z

Changes

Based on changes in databricks/databricks-sdk-go#572.

This PR adds initial support for headers in the ApiClient class. When making a request, the SDK will construct a map of headers to be included in the request. These are passed through a new parameter in the do method.

In the future, to support dynamic headers, we can modify our codegen template to add those fields to the headers map before calling the ApiClient.

Alternatives considered:

Adding headers in request objects directly: this would not require any change in the ApiClient interface. Instead, we would introduce a new annotation for headers, much like what we do for query parameters, and reflectively scan the fields of each request object. We could do this, but for headers that have a fixed value (like Content-Type and Accept), these fields would essentially be private final fields that we could iterate through. Users can never interact with these fields, so it seems a bit unusual to have them in the request structure, a public interface.

Tests

make test run locally
make fmt applied
relevant integration tests applied

# Conflicts: # databricks/sdk/service/catalog.py # databricks/sdk/service/workspace.py

codecov-commenter · 2023-08-19T05:14:45Z

Codecov Report

Patch coverage: 0.21% and project coverage change: -1.05% ⚠️

Comparison is base (d19adf0) 52.73% compared to head (58dc5b0) 51.68%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #302      +/-   ##
==========================================
- Coverage   52.73%   51.68%   -1.05%     
==========================================
  Files          33       33              
  Lines       20291    20800     +509     
==========================================
+ Hits        10700    10751      +51     
- Misses       9591    10049     +458

Files Changed	Coverage Δ
databricks/sdk/service/billing.py	`51.88% <0.00%> (-1.69%)`	⬇️
databricks/sdk/service/catalog.py	`50.74% <0.00%> (-1.68%)`	⬇️
databricks/sdk/service/compute.py	`51.11% <0.00%> (-1.00%)`	⬇️
databricks/sdk/service/files.py	`47.00% <0.00%> (-2.48%)`	⬇️
databricks/sdk/service/iam.py	`41.55% <0.00%> (-2.38%)`	⬇️
databricks/sdk/service/jobs.py	`54.32% <0.00%> (-0.54%)`	⬇️
databricks/sdk/service/ml.py	`47.55% <0.00%> (-1.46%)`	⬇️
databricks/sdk/service/oauth2.py	`49.51% <0.00%> (-2.55%)`	⬇️
databricks/sdk/service/pipelines.py	`49.35% <0.00%> (-0.81%)`	⬇️
databricks/sdk/service/provisioning.py	`46.92% <0.00%> (-1.67%)`	⬇️
... and 6 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

nfx

the diff seems to be larger than required. e.g. in 500 methods, the 'Accept': 'application/json' and 'Content-Type': 'application/json' can be defaults and hidden within ApiClient#do.

nfx · 2023-08-19T14:06:36Z

.codegen/service.py.tmpl

@@ -202,6 +203,12 @@ class {{.Name}}API:{{if .Description}}
        {{- end}}
 {{- end}}

+{{ define "method-headers" -}}


please surround it with if

nfx · 2023-08-19T14:07:24Z

.codegen/service.py.tmpl

+      {{- if .Request.HasQueryField}}, query=query{{end}}
+      {{- if .Request.HasJsonField}}, body=body{{end}}
+  {{end}}
+  , headers=headers)


Suggested change

, headers=headers)

{{if .HasHeaders}}, headers=headers{{end}})

nfx · 2023-08-19T14:07:39Z

databricks/sdk/core.py

           body: dict = None,
           raw: bool = False,
           files=None,
           data=None) -> dict:
-        headers = {'Accept': 'application/json', 'User-Agent': self._user_agent_base}
+        headers |= {'User-Agent': self._user_agent_base}


a) may not work in python 3.7
b) this will crash, as headers default value is empty

# Conflicts: # databricks/sdk/core.py

databricks/sdk/core.py

Signed-off-by: Miles Yucht <miles@databricks.com>

tanmay-db

LGTM

Considered and we can revisit if needed.

## Changes This PR adds support for non-application/json requests and responses. This PR is divided into two parts: **Support setting headers per request**. The `do` method of the ApiClient class is expanded to accept one new parameter, `headers: dict`, and all callers are expected to pass a map of headers to be included in the request. As the allowed content types for requests and responses are known from the OpenAPI specification, callers must construct the request header map in code generation and pass it to the ApiClient. The default "Content-Type: application/json" header is removed, as each request should specify its own Content-Type and Accept headers. This is done in #302. **Add support for streaming request and response bodies to support non-application/json requests/responses**. Today, serialization of requests is done in the ApiClient. This implies that ApiClient needs to be able to serialize a request purely based on the parameters provided to it via headers, the request body, etc. In this proposal, the serialization is specified by the caller depending on whether the `body` or `data` field of `do()` is used: `data` should be used for already-serialized data, such as BinaryIO or file-like objects, and `body` should be used for JSON objects. One important caveat about streaming responses is that callers are required to close the streams when they are done using them. To do this, we may need to expose the raw Response object so that users can use `with w.service.method() as response:` to automatically close the response when they are done reading. ## Tests  - [ ] `make test` run locally - [ ] `make fmt` applied - [ ] relevant integration tests applied

* Added support for GZIP'ed streaming responses ([#306](#306)). * Added support for per-method request headers to ApiClient ([#302](#302)). * Added support for BinaryIO for streaming request and response bodies ([#303](#303)). * Added a link to the API reference ([#311](#311)). * Check workspaceUrl explicitly in runtime repl auth ([#312](#312)). Breaking Changes: * Added support for the Files API (using application/octet-stream) in OpenAPI. The names of parameters have changed from `src` to `contents`, and `w.files.download()` now returns a `files.DownloadResponse`, whose `contents` field is a `BinaryIO` object. When reading a download, the user must explicitly close this object to allow the connection to return to the connection pool. Breaking API Changes: * Changed `list()` method for [a.account_storage_credentials](https://databricks-sdk-py.readthedocs.io/en/latest/account/account_storage_credentials.html) account-level service to return `databricks.sdk.service.catalog.StorageCredentialInfoList` dataclass. * Removed [w.securable_tags](https://databricks-sdk-py.readthedocs.io/en/latest/workspace/securable_tags.html) workspace-level service and all associated classes. * Removed [w.subentity_tags](https://databricks-sdk-py.readthedocs.io/en/latest/workspace/subentity_tags.html) workspace-level service and all associated classes. * Removed `instance_pool_fleet_attributes` field for `databricks.sdk.service.compute.CreateInstancePool`. * Removed `instance_pool_fleet_attributes` field for `databricks.sdk.service.compute.EditInstancePool`. * Removed `databricks.sdk.service.compute.FleetLaunchTemplateOverride` dataclass. * Removed `databricks.sdk.service.compute.FleetOnDemandOption` dataclass. * Removed `databricks.sdk.service.compute.FleetOnDemandOptionAllocationStrategy` dataclass. * Removed `databricks.sdk.service.compute.FleetSpotOption` dataclass. * Removed `databricks.sdk.service.compute.FleetSpotOptionAllocationStrategy` dataclass. * Removed `instance_pool_fleet_attributes` field for `databricks.sdk.service.compute.GetInstancePool`. * Removed `instance_pool_fleet_attributes` field for `databricks.sdk.service.compute.InstancePoolAndStats`. * Removed `databricks.sdk.service.compute.InstancePoolFleetAttributes` dataclass. * Changed `get_by_name()` method for [w.experiments](https://databricks-sdk-py.readthedocs.io/en/latest/workspace/experiments.html) workspace-level service to return `databricks.sdk.service.ml.GetExperimentResponse` dataclass. * Changed `get_experiment()` method for [w.experiments](https://databricks-sdk-py.readthedocs.io/en/latest/workspace/experiments.html) workspace-level service to return `databricks.sdk.service.ml.GetExperimentResponse` dataclass. * Renamed `databricks.sdk.service.ml.GetExperimentByNameResponse` dataclass to `databricks.sdk.service.ml.GetExperimentResponse`. * Renamed `databricks.sdk.service.catalog.ProvisioningState` to `databricks.sdk.service.catalog.ProvisioningInfoState` dataclass. API Changes: * Added [w.model_versions](https://databricks-sdk-py.readthedocs.io/en/latest/workspace/model_versions.html) workspace-level service. * Added [w.registered_models](https://databricks-sdk-py.readthedocs.io/en/latest/workspace/registered_models.html) workspace-level service. * Added `browse_only` field for `databricks.sdk.service.catalog.CatalogInfo`. * Added `full_name` field for `databricks.sdk.service.catalog.CatalogInfo`. * Added `provisioning_info` field for `databricks.sdk.service.catalog.CatalogInfo`. * Added `securable_kind` field for `databricks.sdk.service.catalog.CatalogInfo`. * Added `securable_type` field for `databricks.sdk.service.catalog.CatalogInfo`. * Added `provisioning_info` field for `databricks.sdk.service.catalog.ConnectionInfo`. * Added `options` field for `databricks.sdk.service.catalog.CreateCatalog`. * Added `options` field for `databricks.sdk.service.catalog.UpdateCatalog`. * Added `databricks.sdk.service.catalog.CatalogInfoSecurableKind` dataclass. * Added `databricks.sdk.service.catalog.CreateRegisteredModelRequest` dataclass. * Added `databricks.sdk.service.catalog.DeleteAliasRequest` dataclass. * Added `databricks.sdk.service.catalog.DeleteModelVersionRequest` dataclass. * Added `databricks.sdk.service.catalog.DeleteRegisteredModelRequest` dataclass. * Added `databricks.sdk.service.catalog.GetByAliasRequest` dataclass. * Added `databricks.sdk.service.catalog.GetModelVersionRequest` dataclass. * Added `databricks.sdk.service.catalog.GetRegisteredModelRequest` dataclass. * Added `databricks.sdk.service.catalog.ListModelVersionsRequest` dataclass. * Added `databricks.sdk.service.catalog.ListModelVersionsResponse` dataclass. * Added `databricks.sdk.service.catalog.ListRegisteredModelsRequest` dataclass. * Added `databricks.sdk.service.catalog.ListRegisteredModelsResponse` dataclass. * Added `databricks.sdk.service.catalog.ModelVersionInfo` dataclass. * Added `databricks.sdk.service.catalog.ModelVersionInfoStatus` dataclass. * Added `databricks.sdk.service.catalog.ProvisioningInfo` dataclass. * Added `databricks.sdk.service.catalog.RegisteredModelAlias` dataclass. * Added `databricks.sdk.service.catalog.RegisteredModelInfo` dataclass. * Added `databricks.sdk.service.catalog.SetRegisteredModelAliasRequest` dataclass. * Added `databricks.sdk.service.catalog.UpdateModelVersionRequest` dataclass. * Added `databricks.sdk.service.catalog.UpdateRegisteredModelRequest` dataclass. * Added `volumes` field for `databricks.sdk.service.compute.InitScriptInfo`. * Added `databricks.sdk.service.compute.VolumesStorageInfo` dataclass. * Added [w.files](https://databricks-sdk-py.readthedocs.io/en/latest/workspace/files.html) workspace-level service. * Added `databricks.sdk.service.files.DeleteFileRequest` dataclass. * Added `databricks.sdk.service.files.DownloadRequest` dataclass. * Added `databricks.sdk.service.files.DownloadResponse` dataclass. * Added `databricks.sdk.service.files.UploadRequest` dataclass. * Added `custom_tags` field for `databricks.sdk.service.provisioning.CreateWorkspaceRequest`. * Added `custom_tags` field for `databricks.sdk.service.provisioning.UpdateWorkspaceRequest`. * Added `custom_tags` field for `databricks.sdk.service.provisioning.Workspace`. * Added `databricks.sdk.service.provisioning.CustomTags` dataclass. * Added `parameters` field for `databricks.sdk.service.sql.ExecuteStatementRequest`. * Added `row_limit` field for `databricks.sdk.service.sql.ExecuteStatementRequest`. * Added `databricks.sdk.service.sql.StatementParameterListItem` dataclass. SDK Internal Changes: * Skip Graviton runtimes for testing notebook native auth ([#294](#294)). * Fixed integration tests to not use beta DBR ([#309](#309)). OpenAPI SHA: 5d0ccbb790d341eae8e85321a685a9e9e2d5bf24, Date: 2023-08-29

mgyucht added 7 commits August 16, 2023 20:34

Propagate headers into API Client

91591a0

Use methods from OpenAPI tooling

8aca8c2

build off of master

efaab28

alignment

855a9f7

clean build

cc67efa

Merge branch 'main' into propagate-headers-to-apiclient

2fa3e8a

# Conflicts: # databricks/sdk/service/catalog.py # databricks/sdk/service/workspace.py

do not reset content type header

1a86237

nfx previously requested changes Aug 19, 2023

View reviewed changes

mgyucht added 6 commits August 22, 2023 15:21

check for headers is None

f221c2f

Merge branch 'main' into propagate-headers-to-apiclient

81a3812

# Conflicts: # databricks/sdk/core.py

add unit test

c77f37a

remove extra spaces

cb8c734

test fmt

a8240a7

remove extra newlines

58dc5b0

mgyucht requested a review from tanmay-db August 23, 2023 07:13

mgyucht mentioned this pull request Aug 23, 2023

Support BinaryIO for streaming request and response bodies #303

Merged

3 tasks

mgyucht commented Aug 23, 2023

View reviewed changes

databricks/sdk/core.py Outdated Show resolved Hide resolved

Update databricks/sdk/core.py

0be43cd

Signed-off-by: Miles Yucht <miles@databricks.com>

tanmay-db approved these changes Aug 23, 2023

View reviewed changes

mgyucht enabled auto-merge August 23, 2023 08:52

mgyucht added this pull request to the merge queue Aug 23, 2023

Merged via the queue into main with commit 27356dd Aug 23, 2023
7 checks passed

mgyucht deleted the propagate-headers-to-apiclient branch August 23, 2023 08:54

mgyucht mentioned this pull request Aug 29, 2023

Release v0.7.0 #316

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Propagate request headers to ApiClient #302

Propagate request headers to ApiClient #302

mgyucht commented Aug 19, 2023

codecov-commenter commented Aug 19, 2023 •

edited

nfx left a comment

nfx Aug 19, 2023

nfx Aug 19, 2023

nfx Aug 19, 2023

tanmay-db left a comment

	, headers=headers)
	{{if .HasHeaders}}, headers=headers{{end}})

Propagate request headers to ApiClient #302

Propagate request headers to ApiClient #302

Conversation

mgyucht commented Aug 19, 2023

Changes

Tests

codecov-commenter commented Aug 19, 2023 • edited

Codecov Report

nfx left a comment

Choose a reason for hiding this comment

nfx Aug 19, 2023

Choose a reason for hiding this comment

nfx Aug 19, 2023

Choose a reason for hiding this comment

nfx Aug 19, 2023

Choose a reason for hiding this comment

tanmay-db left a comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 19, 2023 •

edited