Support io.ReadCloser for request and response bodies #590

mgyucht · 2023-08-20T04:07:06Z

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 DatabricksClient is expanded to accept one new parameter, headers map[string]string, 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 client. 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 #589.

Add support for streaming request and response bodies to support non-application/json requests/responses. Today, serialization of requests is done in the client. 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 value passed to the request and response parameters is an io.ReadCloser or not.

One important caveat about streaming responses is that callers are required to close the streams when they are done using them.

Tests

make test passing
make fmt applied
relevant integration tests applied

# Conflicts: # .codegen/_openapi_sha # service/catalog/impl.go # service/ml/impl.go # service/ml/model_registry_usage_test.go # service/workspace/impl.go

codecov-commenter · 2023-08-20T04:09:01Z

Codecov Report

Patch coverage: 4.80% and project coverage change: -1.63% ⚠️

Comparison is base (8fdffbe) 18.26% compared to head (fd8048d) 16.63%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #590      +/-   ##
==========================================
- Coverage   18.26%   16.63%   -1.63%     
==========================================
  Files          85       85              
  Lines        9987    11150    +1163     
==========================================
+ Hits         1824     1855      +31     
- Misses       8004     9131    +1127     
- Partials      159      164       +5

Files Changed	Coverage Δ
service/billing/impl.go	`0.00% <0.00%> (ø)`
service/catalog/impl.go	`0.00% <0.00%> (ø)`
service/iam/impl.go	`0.00% <0.00%> (ø)`
service/jobs/impl.go	`0.00% <0.00%> (ø)`
service/ml/impl.go	`0.00% <0.00%> (ø)`
service/pipelines/impl.go	`0.00% <0.00%> (ø)`
service/provisioning/impl.go	`0.00% <0.00%> (ø)`
service/settings/impl.go	`0.00% <0.00%> (ø)`
service/sharing/impl.go	`0.00% <0.00%> (ø)`
service/sql/impl.go	`0.00% <0.00%> (ø)`
... and 6 more

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

nfx · 2023-08-20T09:20:49Z

By the way, shouldn’t we do io.Writer for requests and io.Reader for responses? It sounds more logical

mgyucht · 2023-08-23T08:30:11Z

io.Reader allows the client to consume data from a given source. If you're reading data from a file for example, you'll get back an io.Reader, which you can pass directly to the client. Otherwise, you'd have to read from the reader and write to the writer to pass to the client. io.Writer would work for an API where you don't have the data at the moment you call uploadFile but later want to write it out somewhere.

…-bodies

mgyucht · 2023-08-23T10:57:58Z

apierr/errors.go

-func CheckForRetry(ctx context.Context, resp *http.Response, respErr error, body []byte, bodyErr error) (bool, error) {
-	if ue, ok := respErr.(*url.Error); ok {
-		apiError := &APIError{
-			ErrorCode:  "IO_ERROR",


Make this a top-level method in this package

mgyucht · 2023-08-23T11:02:39Z

client/client.go

 		}

-		retry, err := apierr.CheckForRetry(ctx, response, err, responseBody.Bytes(), responseBodyErr)
+		retry, err := apierr.CheckForRetry(ctx, response, err, responseBody.ReadCloser)


err is always nil here, so we can remove

mgyucht · 2023-08-23T11:04:41Z

client/client.go

 		if err == nil && response == nil {
 			err = fmt.Errorf("no response: %s %s", method, requestURL)
 		}


Should be guaranteed by http.Client:

If the returned error is nil, the Response will contain a non-nil Body which the user is expected to close.

mgyucht · 2023-08-23T11:10:28Z

client/client.go

+			apiError := &apierr.APIError{
+				ErrorCode:  "IO_ERROR",
+				StatusCode: 523,
+				Message:    ue.Error(),
+			}


This could be refactored

mgyucht · 2023-08-23T11:11:47Z

client/client_test.go

+			return &http.Response{
+				Request: r,
+			}, fmt.Errorf("nope")


Return nil but wrap err with URLError

## Changes #590 made several changes in the error messages returned from the client. It turns out that many users depend on these error messages exactly. I'll revert this change for now to unblock users who depended on these error messages exactly. Going forward, we recommend that users check if the returned error is an API Error, and then to use the ErrorCode to make decisions about how to handle errors in the APIs. ## Tests - [x] Unit tests are reverted and no longer reference these nested fields. - [x] Upgraded TF provider to use this commit locally and unit tests pass.

mgyucht added 7 commits August 17, 2023 20:32

Propagate request headers through to client.go

0a6167d

First attempt

d12ff14

fix test compilation

83882a2

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

7793d5f

# Conflicts: # .codegen/_openapi_sha # service/catalog/impl.go # service/ml/impl.go # service/ml/model_registry_usage_test.go # service/workspace/impl.go

tweaks

c458855

some work

dc7a735

tests passing

66fc980

some

fd8048d

mgyucht added 2 commits August 23, 2023 10:58

update

bf959b5

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

99bb5ff

…-bodies

mgyucht requested a review from tanmay-db August 23, 2023 10:30

fix

91cc9bb

tanmay-db approved these changes Aug 23, 2023

View reviewed changes

mgyucht commented Aug 23, 2023

View reviewed changes

more testing and address comments

5423d7a

mgyucht enabled auto-merge August 23, 2023 13:56

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

Merged via the queue into main with commit 6a8a0ab Aug 23, 2023
3 checks passed

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

mgyucht mentioned this pull request Aug 29, 2023

Release v0.18.0 #593

Merged

mgyucht mentioned this pull request Sep 5, 2023

Revert error message changes from client.go #604

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support io.ReadCloser for request and response bodies #590

Support io.ReadCloser for request and response bodies #590

mgyucht commented Aug 20, 2023 •

edited

codecov-commenter commented Aug 20, 2023 •

edited

nfx commented Aug 20, 2023

mgyucht commented Aug 23, 2023

mgyucht Aug 23, 2023

mgyucht Aug 23, 2023

mgyucht Aug 23, 2023

mgyucht Aug 23, 2023

mgyucht Aug 23, 2023

Support io.ReadCloser for request and response bodies #590

Support io.ReadCloser for request and response bodies #590

Conversation

mgyucht commented Aug 20, 2023 • edited

Changes

Tests

codecov-commenter commented Aug 20, 2023 • edited

Codecov Report

nfx commented Aug 20, 2023

mgyucht commented Aug 23, 2023

mgyucht Aug 23, 2023

Choose a reason for hiding this comment

mgyucht Aug 23, 2023

Choose a reason for hiding this comment

mgyucht Aug 23, 2023

Choose a reason for hiding this comment

mgyucht Aug 23, 2023

Choose a reason for hiding this comment

mgyucht Aug 23, 2023

Choose a reason for hiding this comment

mgyucht commented Aug 20, 2023 •

edited

codecov-commenter commented Aug 20, 2023 •

edited