Propagate Request Headers to client.go #589

mgyucht · 2023-08-19T07:58:54Z

Changes

Based on changes in #572.

This PR adds initial support for headers in client.go. The impl.go implementations, when making a request, will construct a map of headers to be included in the request. These are passed through a new parameter in client.Do().

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

Alternatives considered:

Adding headers in request objects directly: this would not require any change in the client 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 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-19T08:00:40Z

Codecov Report

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

Comparison is base (8fdffbe) 18.26% compared to head (5c52986) 16.63%.
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #589      +/-   ##
==========================================
- Coverage   18.26%   16.63%   -1.63%     
==========================================
  Files          85       85              
  Lines        9987    11174    +1187     
==========================================
+ Hits         1824     1859      +35     
- Misses       8004     9149    +1145     
- Partials      159      166       +7

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/iam/model.go	`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%> (ø)`
... and 5 more

... and 4 files with indirect coverage changes

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

nfx

could you use request visitor for adding request headers, that doesn't require the diff in every callsite?

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

.codegen/impl.go.tmpl

@@ -24,8 +24,16 @@ func (a *{{.Service.CamelName}}Impl) {{.PascalName}}(ctx context.Context{{if .Re
 	path := {{if .PathParts -}}
 		fmt.Sprintf("{{range  .PathParts}}{{.Prefix}}{{if or .Field .IsAccountId}}%v{{end}}{{ end }}"{{ range .PathParts }}{{if .Field}}, request.{{.Field.PascalName}}{{ else if .IsAccountId }}, a.client.ConfiguredAccountID(){{end}}{{ end }})
 	{{- else}}"{{.Path}}"{{end}}
-	err := a.client.Do(ctx, http.Method{{.TitleVerb}}, path, {{if .Request}}request{{else}}nil{{end}}, {{if .Response}}&{{.Response.CamelName}}{{else}}nil{{end}})
+	{{ template "make-header" . }}
+	err := a.client.Do(ctx, http.Method{{.TitleVerb}}, path, headers, {{if .Request}}request{{else}}nil{{end}}, {{if .Response}}&{{.Response.CamelName}}{{else}}nil{{end}})


adding headers makes unnecessarily large diff. did you try doing it with the request visitor? we're adding other headers with that mechanism already. This could would work without the need to change signature of the method (and this unnecessary diff) -

Suggested change

err := a.client.Do(ctx, http.Method{{.TitleVerb}}, path, headers, {{if .Request}}request{{else}}nil{{end}}, {{if .Response}}&{{.Response.CamelName}}{{else}}nil{{end}})

err := a.client.Do(ctx, http.Method{{.TitleVerb}}, path, {{if .Request}}request{{else}}nil{{end}}, {{if .Response}}&{{.Response.CamelName}}{{else}}nil{{end}}{{with .FixedRequestHeaders}}, func(r *http.Request) error {

{{- range $k, $v := . }}

r.Header["{{$k}}"] = "{{$v}}"

{{- end }}

return nil

}{{end}})

you've even touched the code following this pattern:

service/workspace/utilities.go

tanmay-db

LGTM

Opting for simpler pattern rather than most expressive as 99% of callers are code-generated.

## 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

mgyucht added 3 commits August 17, 2023 20:32

Propagate request headers through to client.go

0a6167d

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

mgyucht changed the title ~~Propagate headers to apiclient~~ Propagate Request Headers to client.go Aug 19, 2023

nfx previously requested changes Aug 19, 2023

View reviewed changes

nfx added the codegen Related to code generation label Aug 19, 2023

mgyucht requested a review from tanmay-db August 23, 2023 08:27

mgyucht mentioned this pull request Aug 23, 2023

Support io.ReadCloser for request and response bodies #590

Merged

3 tasks

fix

9b98e66

mgyucht commented Aug 23, 2023

View reviewed changes

service/workspace/utilities.go Outdated Show resolved Hide resolved

Update service/workspace/utilities.go

5c52986

tanmay-db approved these changes Aug 23, 2023

View reviewed changes

mgyucht enabled auto-merge August 23, 2023 09:04

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

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

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

mgyucht mentioned this pull request Aug 29, 2023

Release v0.18.0 #593

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Propagate Request Headers to client.go #589

Propagate Request Headers to client.go #589

mgyucht commented Aug 19, 2023

codecov-commenter commented Aug 19, 2023 •

edited

Loading

nfx left a comment

nfx Aug 19, 2023

tanmay-db left a comment

-	err := a.client.Do(ctx, http.Method{{.TitleVerb}}, path, headers, {{if .Request}}request{{else}}nil{{end}}, {{if .Response}}&{{.Response.CamelName}}{{else}}nil{{end}})
+	err := a.client.Do(ctx, http.Method{{.TitleVerb}}, path, {{if .Request}}request{{else}}nil{{end}}, {{if .Response}}&{{.Response.CamelName}}{{else}}nil{{end}}{{with .FixedRequestHeaders}}, func(r *http.Request) error {
+	{{- range $k, $v := . }}
+	r.Header["{{$k}}"] = "{{$v}}"
+	{{- end }}
+	return nil
+	}{{end}})

Propagate Request Headers to client.go #589

Propagate Request Headers to client.go #589

Conversation

mgyucht commented Aug 19, 2023

Changes

Tests

codecov-commenter commented Aug 19, 2023 • edited Loading

Codecov Report

nfx left a comment

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

Loading