Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Propagate Request Headers through API Client #135

Merged
merged 11 commits into from
Aug 22, 2023

Conversation

mgyucht
Copy link
Contributor

@mgyucht mgyucht commented Aug 14, 2023

Changes

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

This PR adds initial support for headers in ApiClient.java. The *Impl classes, when making a request, will construct a map of headers to be included in the request. These are passed through a new parameter in each of GET/PUT/POST/PATCH/DELETE.

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

@mgyucht mgyucht changed the title [WIP] Propagate Request Headers through API Client Propagate Request Headers through API Client Aug 18, 2023
@mgyucht mgyucht marked this pull request as ready for review August 18, 2023 19:59
nfx
nfx previously requested changes Aug 19, 2023
{{if .Response -}}
{{- if .Response.ArrayValue -}} return apiClient.getCollection(path, null, {{template "type" .Response.ArrayValue}}.class);
{{- else if .Response.MapValue -}} return apiClient.getStringMap(path, request);
{{- if .Response.ArrayValue -}} return apiClient.getCollection(path, null, {{template "type" .Response.ArrayValue}}.class, headers);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the diff seems to be larger than it should be. did you try solving it with method overloading?

Suggested change
{{- if .Response.ArrayValue -}} return apiClient.getCollection(path, null, {{template "type" .Response.ArrayValue}}.class, headers);
{{- if .Response.ArrayValue -}} return apiClient.getCollection(path, null, {{template "type" .Response.ArrayValue}}.class{{if .HasRequestHeaders}}, headers{{end}});

Copy link
Contributor Author

@mgyucht mgyucht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some old comment?

.codegen/impl.java.tmpl Outdated Show resolved Hide resolved
@mgyucht mgyucht dismissed nfx’s stale review August 22, 2023 13:15

For simplicity, we will always pass headers to the ApiClient.

@mgyucht mgyucht added this pull request to the merge queue Aug 22, 2023
Merged via the queue into main with commit 4c6599c Aug 22, 2023
@mgyucht mgyucht deleted the propagate-headers-to-apiclient branch August 22, 2023 13:18
github-merge-queue bot pushed a commit that referenced this pull request Aug 22, 2023
## Changes
In #135 there was
a refactor to ApiClient's methods by introducing `prepareRequest`. This
accidentally removed the serialization of the body. This PR corrects
that issue.

## Tests
Ran failing integration tests, and they passed.
github-merge-queue bot pushed a commit that referenced this pull request Aug 23, 2023
## 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 main methods of the
ApiClient interface are expanded to accept one new
parameter,`Map<String, String> headers`, 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, impls 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 merged in
#135.)

**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 coupled to the request type: InputStream indicates that
the client has already serialized the request, so it can be provided
directly to the underlying HttpClient library, and any other type will
be passed through Jackson's ObjectMapper for serialization to JSON.
Internally, request and response bodies are modeled as InputStreams
rather than byte arrays to support streaming requests and responses,
though for non-streaming requests and responses, a string value for the
body is captured in the `debugBody` field of Request and Response,
respectively.

One important caveat about streaming responses is that callers are
required to close the streams when they are done using them. This
releases the underlying HTTP connection back to the connection pool for
use in subsequent requests. Otherwise, customers could encounter
deadlocks waiting for connections. For this, we recommend that users use
the try-with-resources model as demonstrated in integration test
FilesIT.java.

## Tests
- [x] Integration test for Files IT exercises uploading and downloading
files using the streaming pathways.
- [x] Existing integration tests cover the pre-existing non-streaming
pathways.
mgyucht added a commit that referenced this pull request Aug 29, 2023
* Added support for propagating Request Headers through API Client ([#135](#135)).
* Added support for InputStream for streaming request and response bodies ([#138](#138)).
* Fixed query param serialization for requests with enums ([#140](#140)).

Breaking API Changes:
 * Changed `list()` method for `accountClient.storageCredentials()` service to return `com.databricks.sdk.service.catalog.StorageCredentialInfoList` class.
 * Removed `workspaceClient.securableTags()` service and all related classes.
 * Removed `workspaceClient.subentityTags()` service and all related classes.
 * Renamed `provisioningState` field for `com.databricks.sdk.service.catalog.ConnectionInfo` to `provisioningInfo`.
 * Removed `instancePoolFleetAttributes` field for `com.databricks.sdk.service.compute.CreateInstancePool`.
 * Removed `instancePoolFleetAttributes` field for `com.databricks.sdk.service.compute.EditInstancePool`.
 * Removed `com.databricks.sdk.service.compute.FleetLaunchTemplateOverride` class.
 * Removed `com.databricks.sdk.service.compute.FleetOnDemandOption` class.
 * Removed `com.databricks.sdk.service.compute.FleetOnDemandOptionAllocationStrategy` class.
 * Removed `com.databricks.sdk.service.compute.FleetSpotOption` class.
 * Removed `com.databricks.sdk.service.compute.FleetSpotOptionAllocationStrategy` class.
 * Removed `instancePoolFleetAttributes` field for `com.databricks.sdk.service.compute.GetInstancePool`.
 * Removed `instancePoolFleetAttributes` field for `com.databricks.sdk.service.compute.InstancePoolAndStats`.
 * Removed `com.databricks.sdk.service.compute.InstancePoolFleetAttributes` class.
 * Changed `getByName()` method for `workspaceClient.experiments()` service to return `com.databricks.sdk.service.ml.GetExperimentResponse` class.
 * Changed `getExperiment()` method for `workspaceClient.experiments()` service to return `com.databricks.sdk.service.ml.GetExperimentResponse` class.
 * Renamed `com.databricks.sdk.service.ml.GetExperimentByNameResponse` class to `com.databricks.sdk.service.ml.GetExperimentResponse`.

API Changes:

 * Added `workspaceClient.modelVersions()` service.
 * Added `workspaceClient.registeredModels()` service.
 * Added `browseOnly` field for `com.databricks.sdk.service.catalog.CatalogInfo`.
 * Added `fullName` field for `com.databricks.sdk.service.catalog.CatalogInfo`.
 * Added `provisioningInfo` field for `com.databricks.sdk.service.catalog.CatalogInfo`.
 * Added `securableKind` field for `com.databricks.sdk.service.catalog.CatalogInfo`.
 * Added `securableType` field for `com.databricks.sdk.service.catalog.CatalogInfo`.
 * Added `options` field for `com.databricks.sdk.service.catalog.CreateCatalog`.
 * Added `options` field for `com.databricks.sdk.service.catalog.UpdateCatalog`.
 * Added `com.databricks.sdk.service.catalog.CatalogInfoSecurableKind` class.
 * Added `com.databricks.sdk.service.catalog.CreateRegisteredModelRequest` class.
 * Added `com.databricks.sdk.service.catalog.DeleteAliasRequest` class.
 * Added `com.databricks.sdk.service.catalog.DeleteModelVersionRequest` class.
 * Added `com.databricks.sdk.service.catalog.DeleteRegisteredModelRequest` class.
 * Added `com.databricks.sdk.service.catalog.GetByAliasRequest` class.
 * Added `com.databricks.sdk.service.catalog.GetModelVersionRequest` class.
 * Added `com.databricks.sdk.service.catalog.GetRegisteredModelRequest` class.
 * Added `com.databricks.sdk.service.catalog.ListModelVersionsRequest` class.
 * Added `com.databricks.sdk.service.catalog.ListModelVersionsResponse` class.
 * Added `com.databricks.sdk.service.catalog.ListRegisteredModelsRequest` class.
 * Added `com.databricks.sdk.service.catalog.ListRegisteredModelsResponse` class.
 * Added `com.databricks.sdk.service.catalog.ModelVersionInfo` class.
 * Added `com.databricks.sdk.service.catalog.ModelVersionInfoStatus` class.
 * Added `com.databricks.sdk.service.catalog.ProvisioningInfo` class.
 * Added `com.databricks.sdk.service.catalog.ProvisioningInfoState` class.
 * Added `com.databricks.sdk.service.catalog.RegisteredModelAlias` class.
 * Added `com.databricks.sdk.service.catalog.RegisteredModelInfo` class.
 * Added `com.databricks.sdk.service.catalog.SetRegisteredModelAliasRequest` class.
 * Added `com.databricks.sdk.service.catalog.UpdateModelVersionRequest` class.
 * Added `com.databricks.sdk.service.catalog.UpdateRegisteredModelRequest` class.
 * Added `volumes` field for `com.databricks.sdk.service.compute.InitScriptInfo`.
 * Added `com.databricks.sdk.service.compute.VolumesStorageInfo` class.
 * Added `workspaceClient.files()` service.
 * Added `com.databricks.sdk.service.files.DeleteFileRequest` class.
 * Added `com.databricks.sdk.service.files.DownloadRequest` class.
 * Added `com.databricks.sdk.service.files.DownloadResponse` class.
 * Added `com.databricks.sdk.service.files.UploadRequest` class.
 * Added `customTags` field for `com.databricks.sdk.service.provisioning.CreateWorkspaceRequest`.
 * Added `customTags` field for `com.databricks.sdk.service.provisioning.UpdateWorkspaceRequest`.
 * Added `customTags` field for `com.databricks.sdk.service.provisioning.Workspace`.
 * Added `com.databricks.sdk.service.provisioning.CustomTags` class.
 * Added `parameters` field for `com.databricks.sdk.service.sql.ExecuteStatementRequest`.
 * Added `rowLimit` field for `com.databricks.sdk.service.sql.ExecuteStatementRequest`.
 * Added `com.databricks.sdk.service.sql.StatementParameterListItem` class.

OpenAPI SHA: 5d0ccbb790d341eae8e85321a685a9e9e2d5bf24, Date: 2023-08-29
@mgyucht mgyucht mentioned this pull request Aug 29, 2023
github-merge-queue bot pushed a commit that referenced this pull request Aug 29, 2023
* Added support for propagating Request Headers through API Client
([#135](#135)).
* Added support for InputStream for streaming request and response
bodies
([#138](#138)).
* Fixed query param serialization for requests with enums
([#140](#140)).

Breaking API Changes:
* Changed `list()` method for `accountClient.storageCredentials()`
service to return
`com.databricks.sdk.service.catalog.StorageCredentialInfoList` class.
* Removed `workspaceClient.securableTags()` service and all related
classes.
* Removed `workspaceClient.subentityTags()` service and all related
classes.
* Renamed `provisioningState` field for
`com.databricks.sdk.service.catalog.ConnectionInfo` to
`provisioningInfo`.
* Removed `instancePoolFleetAttributes` field for
`com.databricks.sdk.service.compute.CreateInstancePool`.
* Removed `instancePoolFleetAttributes` field for
`com.databricks.sdk.service.compute.EditInstancePool`.
* Removed
`com.databricks.sdk.service.compute.FleetLaunchTemplateOverride` class.
* Removed `com.databricks.sdk.service.compute.FleetOnDemandOption`
class.
* Removed
`com.databricks.sdk.service.compute.FleetOnDemandOptionAllocationStrategy`
class.
 * Removed `com.databricks.sdk.service.compute.FleetSpotOption` class.
* Removed
`com.databricks.sdk.service.compute.FleetSpotOptionAllocationStrategy`
class.
* Removed `instancePoolFleetAttributes` field for
`com.databricks.sdk.service.compute.GetInstancePool`.
* Removed `instancePoolFleetAttributes` field for
`com.databricks.sdk.service.compute.InstancePoolAndStats`.
* Removed
`com.databricks.sdk.service.compute.InstancePoolFleetAttributes` class.
* Changed `getByName()` method for `workspaceClient.experiments()`
service to return `com.databricks.sdk.service.ml.GetExperimentResponse`
class.
* Changed `getExperiment()` method for `workspaceClient.experiments()`
service to return `com.databricks.sdk.service.ml.GetExperimentResponse`
class.
* Renamed `com.databricks.sdk.service.ml.GetExperimentByNameResponse`
class to `com.databricks.sdk.service.ml.GetExperimentResponse`.

API Changes:

 * Added `workspaceClient.modelVersions()` service.
 * Added `workspaceClient.registeredModels()` service.
* Added `browseOnly` field for
`com.databricks.sdk.service.catalog.CatalogInfo`.
* Added `fullName` field for
`com.databricks.sdk.service.catalog.CatalogInfo`.
* Added `provisioningInfo` field for
`com.databricks.sdk.service.catalog.CatalogInfo`.
* Added `securableKind` field for
`com.databricks.sdk.service.catalog.CatalogInfo`.
* Added `securableType` field for
`com.databricks.sdk.service.catalog.CatalogInfo`.
* Added `options` field for
`com.databricks.sdk.service.catalog.CreateCatalog`.
* Added `options` field for
`com.databricks.sdk.service.catalog.UpdateCatalog`.
* Added `com.databricks.sdk.service.catalog.CatalogInfoSecurableKind`
class.
* Added
`com.databricks.sdk.service.catalog.CreateRegisteredModelRequest` class.
 * Added `com.databricks.sdk.service.catalog.DeleteAliasRequest` class.
* Added `com.databricks.sdk.service.catalog.DeleteModelVersionRequest`
class.
* Added
`com.databricks.sdk.service.catalog.DeleteRegisteredModelRequest` class.
 * Added `com.databricks.sdk.service.catalog.GetByAliasRequest` class.
* Added `com.databricks.sdk.service.catalog.GetModelVersionRequest`
class.
* Added `com.databricks.sdk.service.catalog.GetRegisteredModelRequest`
class.
* Added `com.databricks.sdk.service.catalog.ListModelVersionsRequest`
class.
* Added `com.databricks.sdk.service.catalog.ListModelVersionsResponse`
class.
* Added `com.databricks.sdk.service.catalog.ListRegisteredModelsRequest`
class.
* Added
`com.databricks.sdk.service.catalog.ListRegisteredModelsResponse` class.
 * Added `com.databricks.sdk.service.catalog.ModelVersionInfo` class.
* Added `com.databricks.sdk.service.catalog.ModelVersionInfoStatus`
class.
 * Added `com.databricks.sdk.service.catalog.ProvisioningInfo` class.
* Added `com.databricks.sdk.service.catalog.ProvisioningInfoState`
class.
* Added `com.databricks.sdk.service.catalog.RegisteredModelAlias` class.
 * Added `com.databricks.sdk.service.catalog.RegisteredModelInfo` class.
* Added
`com.databricks.sdk.service.catalog.SetRegisteredModelAliasRequest`
class.
* Added `com.databricks.sdk.service.catalog.UpdateModelVersionRequest`
class.
* Added
`com.databricks.sdk.service.catalog.UpdateRegisteredModelRequest` class.
* Added `volumes` field for
`com.databricks.sdk.service.compute.InitScriptInfo`.
 * Added `com.databricks.sdk.service.compute.VolumesStorageInfo` class.
 * Added `workspaceClient.files()` service.
 * Added `com.databricks.sdk.service.files.DeleteFileRequest` class.
 * Added `com.databricks.sdk.service.files.DownloadRequest` class.
 * Added `com.databricks.sdk.service.files.DownloadResponse` class.
 * Added `com.databricks.sdk.service.files.UploadRequest` class.
* Added `customTags` field for
`com.databricks.sdk.service.provisioning.CreateWorkspaceRequest`.
* Added `customTags` field for
`com.databricks.sdk.service.provisioning.UpdateWorkspaceRequest`.
* Added `customTags` field for
`com.databricks.sdk.service.provisioning.Workspace`.
 * Added `com.databricks.sdk.service.provisioning.CustomTags` class.
* Added `parameters` field for
`com.databricks.sdk.service.sql.ExecuteStatementRequest`.
* Added `rowLimit` field for
`com.databricks.sdk.service.sql.ExecuteStatementRequest`.
* Added `com.databricks.sdk.service.sql.StatementParameterListItem`
class.

OpenAPI SHA: 5d0ccbb790d341eae8e85321a685a9e9e2d5bf24, Date: 2023-08-29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants