Support inputstream for request and response bodies by mgyucht · Pull Request #138 · databricks/databricks-sdk-java

mgyucht · 2023-08-18T12:23:41Z

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

Integration test for Files IT exercises uploading and downloading files using the streaming pathways.
Existing integration tests cover the pre-existing non-streaming pathways.

…m-for-request-and-response-bodies

…-bodies # Conflicts: # .codegen/_openapi_sha

mgyucht · 2023-08-18T14:10:55Z

  @Override
  public int hashCode() {
-    return Objects.hash(method, url, query, body);
+    return Objects.hash(method, url, query, debugBody);


leave comment for testing only

mgyucht · 2023-08-18T14:18:38Z

        }
        // check if object is a primitive type or a collection of some kind
-        if (primitiveTypes.contains(f.getType()) || Iterable.class.isAssignableFrom(f.getType())) {
+        if (primitiveTypes.contains(f.getType()) || Iterable.class.isAssignableFrom(f.getType()) || f.getType().isEnum()) {


Move to another PR

mgyucht · 2023-08-18T14:24:56Z

+    // If the API call fails, the server will respond with an application/json response, which
+    // the client reads fully and closes, even if the request Accept header is not application/json.
+    // The error is then thrown as an exception in ApiClient.


This needs to be implemented properly

Change to if expected not json && got not json <=> stream

mgyucht · 2023-08-18T14:32:42Z

  }
+
+  private static String convert(InputStream in) {
+    try (BufferedReader br =


Put into ByteArrayOutputStream

mgyucht · 2023-08-18T14:35:43Z

+    return impl.getStatus(request);
+  }
+
+  public void uploadFile(InputStream contents, String filePath) {


Move body parameter to the end

mgyucht · 2023-08-18T14:53:41Z

Need to split into two PRs.

pietern

Approach of making API client be streaming aware LGTM.

pietern · 2023-08-18T15:02:14Z

-      return String.format("[unable to marshal: %s]", e.getMessage());
+      // Unable to unmarshal means the body isn't JSON
+      if (body.length() > maxBytes) {
+        return body.substring(0, maxBytes - 3) + "...";


This value should be escaped, seeing as it ends up in log files. If we log a files API response it can be any type of binary data and it can mess up your terminal.

For now, I think this is safe-ish. The main value that triggers this case is "", since that is what we set debugBody to when streaming a response. Otherwise, it behaves the same as existing logging. I'll change this impl to reuse onlyNBytes, and we can pick this up as a follow-up task.

Also for future reference: the implementation currently splits the debug body string by newlines and prepend the > prefix, so it should not be possible for attackers to trigger printing out specific log files that don't have that prefix.

…m-for-request-and-response-bodies

tanmay-db

Went over the PR with Miles, LGTM.

mgyucht · 2023-08-23T09:34:11Z

  }
+
+  private static String convert(InputStream in) {
+    ByteArrayOutputStream out = new ByteArrayOutputStream();


IOUtils.toString

mgyucht · 2023-08-23T09:34:46Z

+    }
+
+    try (InputStream inputStream = entity.getContent()) {
+      String body = IOUtils.toString(inputStream, Charset.defaultCharset());


mgyucht · 2023-08-23T09:36:17Z

    }

-    Matcher messageMatcher = HTML_ERROR_REGEX.matcher(response.getBody());
+    String body = convert(response.getBody());


Comment that error responses are fully consumed so don't need to be closed.

* 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

## Changes #138 changed the CommonsHttpClient implementation to use `InputStreamEntity` for all request bodies sent using the Java SDK. As a result, requests are sent with a `Transfer-Encoding: chunked` header, and the request body is chunked accordingly. However, the Databricks REST API only tolerates chunked requests for certain APIs; for others, it ignores the request body if Content-Length is not specified. Auth falls under that category, which caused #154. To fix this, Request will keep track of string entities and input stream entities separately. Previously, Request had separate fields for the body and debugBody, but they were both always set, so it wasn't possible to distinguish between these two cases. Now, HTTP clients can have different behavior based on whether the request body can be fully materialized as a string or is lazily read as with input streams. As part of this, I have removed `SimpleHttpServer`. This was used before we were sure whether it was possible to use the HttpServer built into the JDK. Now that we are confident that we can (see usage in ExternalBrowserCredentialsProvider.java), I've replaced it with HttpServer and a custom HttpHandler in FixtureServer (the only place where it is used now). One other small change: I've updated the logging configuration and dependency for the cli auth demo app. This ensures that users can see debug output (which I used when debugging this). Closes #154. ## Tests I've refactored FixtureServer to support more advanced validation on the HTTP requests sent by clients for testing purposes. Now, users can assert that a request has a specific method, path, headers, body, and check for headers that should not be present. One downside of this change is that users need to call the `.with()` method once per API call, rather than one time, at the start of each test. Unit tests cover both cases (request body should be chunked when specified as a String; request body should not be chunked when specified as an InputStream).

mgyucht added 16 commits August 14, 2023 20:07

Propagate headers through API Client

f019a94

remove unused code

269105e

almost there

13a15c9

tests are passing

2e7b3d0

better test compat

a9c6b98

Mostly there

6de6e0f

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

78589ff

Merge branch 'propagate-headers-to-apiclient' into support-inputstrea…

3153753

…m-for-request-and-response-bodies

tweaks

1a3aff8

some work

851ee5b

some more work

618ef9e

tweaks

1468c85

update log4j.properties

293ac5c

Merge branch 'main' into support-inputstream-for-request-and-response…

3030303

…-bodies # Conflicts: # .codegen/_openapi_sha

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

6f68bbf

regen

9b0c29c

mgyucht changed the base branch from main to propagate-headers-to-apiclient August 18, 2023 14:01

mgyucht commented Aug 18, 2023

View reviewed changes

pietern reviewed Aug 18, 2023

View reviewed changes

mgyucht added 10 commits August 18, 2023 21:34

some cleanup

858ffb5

some fixes

bb84dff

tweaks

de469bb

use main

628a50a

Looking good

596fed6

Merge branch 'propagate-headers-to-apiclient' into support-inputstrea…

0a2ef75

…m-for-request-and-response-bodies

run on master

ec87fcb

on this branch

4200686

address comments

0d30290

Merge branch 'propagate-headers-to-apiclient' into support-inputstrea…

8a522ba

…m-for-request-and-response-bodies

mgyucht added 3 commits August 18, 2023 22:12

address comments

29109c1

remove extra file

e390283

Merge branch 'propagate-headers-to-apiclient' into support-inputstrea…

2a6cfaa

…m-for-request-and-response-bodies

Base automatically changed from propagate-headers-to-apiclient to main August 22, 2023 13:18

mgyucht added 2 commits August 23, 2023 05:00

resolve conflicts

006c66d

use requestBodyField

82abf4f

mgyucht marked this pull request as ready for review August 23, 2023 03:06

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

tanmay-db approved these changes Aug 23, 2023

View reviewed changes

mgyucht commented Aug 23, 2023

View reviewed changes

Address feedbackg

5d8f212

mgyucht enabled auto-merge August 23, 2023 10:23

format

fb3abd5

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

Merged via the queue into main with commit 2dc895b Aug 23, 2023

mgyucht deleted the support-inputstream-for-request-and-response-bodies branch August 23, 2023 10:34

mgyucht mentioned this pull request Aug 29, 2023

Release v0.7.0 #143

Merged

mgyucht mentioned this pull request Sep 26, 2023

Chunk streaming request bodies only #157

Merged

Uh oh!

Conversation

mgyucht commented Aug 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Tests

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgyucht commented Aug 18, 2023

Uh oh!

pietern left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgyucht Aug 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tanmay-db left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mgyucht commented Aug 18, 2023 •

edited

Loading

mgyucht Aug 18, 2023 •

edited

Loading