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.

Sign up for GitHub

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

Jump to bottom

Propagate Request Headers to client.go #589

Merged
merged 5 commits into from
Aug 23, 2023
Merged

Propagate Request Headers to client.go #589

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0a6167d
Propagate request headers through to client.go
mgyucht Aug 17, 2023
7793d5f
Merge branch 'main' into propagate-headers-to-apiclient
mgyucht Aug 19, 2023
c458855
tweaks
mgyucht Aug 19, 2023
9b98e66
fix
mgyucht Aug 23, 2023
5c52986
Update service/workspace/utilities.go
mgyucht Aug 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions .codegen/impl.go.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}})
Copy link
Contributor

Choose a reason for hiding this comment

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

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:
image

return {{if .Response}}{{if not .Response.ArrayValue}}&{{end}}{{.Response.CamelName}}, {{end}}err
}
{{end -}}
{{end}}
{{end}}

{{ define "make-header" -}}
headers := make(map[string]string)
{{- range $k, $v := .FixedRequestHeaders }}
headers["{{$k}}"] = "{{$v}}"
{{- end }}
{{- end }}
21 changes: 10 additions & 11 deletions client/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,9 @@ func (c *DatabricksClient) ConfiguredAccountID() string {

// Do sends an HTTP request against path.
func (c *DatabricksClient) Do(ctx context.Context, method, path string,
request, response any, visitors ...func(*http.Request) error) error {
body, err := c.perform(ctx, method, path, request, visitors...)
headers map[string]string, request, response any,
visitors ...func(*http.Request) error) error {
body, err := c.perform(ctx, method, path, headers, request, visitors...)
if err != nil {
return err
}
Expand Down Expand Up @@ -127,11 +128,6 @@ func (c *DatabricksClient) addHostToRequestUrl(r *http.Request) error {
return nil
}

func (c *DatabricksClient) addApplicationJsonContentType(r *http.Request) error {
r.Header.Set("Content-Type", "application/json")
return nil
}

func (c *DatabricksClient) redactedDump(prefix string, body []byte) (res string) {
return bodyLogger{
debugTruncateBytes: c.debugTruncateBytes,
Expand All @@ -142,6 +138,7 @@ func (c *DatabricksClient) attempt(
ctx context.Context,
method string,
requestURL string,
headers map[string]string,
requestBody []byte,
visitors ...func(*http.Request) error,
) func() (*bytes.Buffer, *retries.Err) {
Expand All @@ -150,8 +147,10 @@ func (c *DatabricksClient) attempt(
if err != nil {
return nil, retries.Halt(err)
}
request, err := http.NewRequestWithContext(ctx, method, requestURL,
bytes.NewBuffer(requestBody))
request, err := http.NewRequestWithContext(ctx, method, requestURL, bytes.NewBuffer(requestBody))
for k, v := range headers {
request.Header.Set(k, v)
}
if err != nil {
return nil, retries.Halt(err)
}
Expand Down Expand Up @@ -274,6 +273,7 @@ func (c *DatabricksClient) perform(
ctx context.Context,
method,
requestURL string,
headers map[string]string,
data interface{},
visitors ...func(*http.Request) error,
) ([]byte, error) {
Expand All @@ -284,11 +284,10 @@ func (c *DatabricksClient) perform(
visitors = append([]func(*http.Request) error{
c.Config.Authenticate,
c.addHostToRequestUrl,
c.addApplicationJsonContentType,
c.addAuthHeaderToUserAgent,
}, visitors...)
resp, err := retries.Poll(ctx, c.retryTimeout,
c.attempt(ctx, method, requestURL, requestBody, visitors...))
c.attempt(ctx, method, requestURL, headers, requestBody, visitors...))
if err != nil {
// Don't re-wrap, as upper layers may depend on handling apierr.APIError.
return nil, err
Expand Down
29 changes: 16 additions & 13 deletions client/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,13 +59,16 @@ func TestSimpleRequestFails(t *testing.T) {
assert.Equal(t, "GET", r.Method)
assert.Equal(t, "/a/b", r.URL.Path)
assert.Equal(t, "c=d", r.URL.RawQuery)
assert.Equal(t, "f", r.Header.Get("e"))
auth := r.Header.Get("Authenticated")
assert.Equal(t, "yes", auth)
return nil, fmt.Errorf("nope")
}),
rateLimiter: rate.NewLimiter(rate.Inf, 1),
}
err := c.Do(context.Background(), "GET", "/a/b", map[string]string{
"e": "f",
}, map[string]string{
"c": "d",
}, nil)
assert.EqualError(t, err, "failed request: nope")
Expand All @@ -89,7 +92,7 @@ func TestSimpleRequestSucceeds(t *testing.T) {
rateLimiter: rate.NewLimiter(rate.Inf, 1),
}
var resp Dummy
err := c.Do(context.Background(), "POST", "/c", Dummy{1}, &resp)
err := c.Do(context.Background(), "POST", "/c", nil, Dummy{1}, &resp)
assert.NoError(t, err)
assert.Equal(t, 2, resp.Foo)
}
Expand Down Expand Up @@ -122,7 +125,7 @@ func TestSimpleRequestRetried(t *testing.T) {
rateLimiter: rate.NewLimiter(rate.Inf, 1),
}
var resp Dummy
err := c.Do(context.Background(), "PATCH", "/a", Dummy{1}, &resp)
err := c.Do(context.Background(), "PATCH", "/a", nil, Dummy{1}, &resp)
assert.NoError(t, err)
assert.Equal(t, 2, resp.Foo)
assert.True(t, retried[0], "request was not retried")
Expand All @@ -133,7 +136,7 @@ func TestHaltAttemptForLimit(t *testing.T) {
c := &DatabricksClient{
rateLimiter: &rate.Limiter{},
}
_, rerr := c.attempt(ctx, "GET", "foo", []byte{})()
_, rerr := c.attempt(ctx, "GET", "foo", nil, []byte{})()
assert.NotNil(t, rerr)
assert.Equal(t, true, rerr.Halt)
assert.EqualError(t, rerr.Err, "rate: Wait(n=1) exceeds limiter's burst 0")
Expand All @@ -144,7 +147,7 @@ func TestHaltAttemptForNewRequest(t *testing.T) {
c := &DatabricksClient{
rateLimiter: rate.NewLimiter(rate.Inf, 1),
}
_, rerr := c.attempt(ctx, "🥱", "/", []byte{})()
_, rerr := c.attempt(ctx, "🥱", "/", nil, []byte{})()
assert.NotNil(t, rerr)
assert.Equal(t, true, rerr.Halt)
assert.EqualError(t, rerr.Err, `net/http: invalid method "🥱"`)
Expand All @@ -155,7 +158,7 @@ func TestHaltAttemptForVisitor(t *testing.T) {
c := &DatabricksClient{
rateLimiter: rate.NewLimiter(rate.Inf, 1),
}
_, rerr := c.attempt(ctx, "GET", "/", []byte{},
_, rerr := c.attempt(ctx, "GET", "/", nil, []byte{},
func(r *http.Request) error {
return fmt.Errorf("🥱")
})()
Expand Down Expand Up @@ -232,7 +235,7 @@ func TestFailPerformChannel(t *testing.T) {
c := &DatabricksClient{
rateLimiter: rate.NewLimiter(rate.Inf, 1),
}
_, err := c.perform(ctx, "GET", "/", true)
_, err := c.perform(ctx, "GET", "/", nil, true)
assert.EqualError(t, err, "request marshal: unsupported query string data: true")
}

Expand All @@ -253,7 +256,7 @@ func TestSimpleRequestAPIError(t *testing.T) {
}),
rateLimiter: rate.NewLimiter(rate.Inf, 1),
}
err := c.Do(context.Background(), "PATCH", "/a", map[string]any{}, nil)
err := c.Do(context.Background(), "PATCH", "/a", nil, map[string]any{}, nil)
var aerr *apierr.APIError
if assert.ErrorAs(t, err, &aerr) {
assert.Equal(t, "NOT_FOUND", aerr.ErrorCode)
Expand All @@ -270,7 +273,7 @@ func TestSimpleRequestNilResponseNoError(t *testing.T) {
}),
rateLimiter: rate.NewLimiter(rate.Inf, 1),
}
err := c.Do(context.Background(), "PATCH", "/a", map[string]any{}, nil)
err := c.Do(context.Background(), "PATCH", "/a", nil, map[string]any{}, nil)
assert.EqualError(t, err, "no response: PATCH /a")
}

Expand All @@ -288,7 +291,7 @@ func TestSimpleRequestErrReaderBody(t *testing.T) {
}),
rateLimiter: rate.NewLimiter(rate.Inf, 1),
}
err := c.Do(context.Background(), "PATCH", "/a", map[string]any{}, nil)
err := c.Do(context.Background(), "PATCH", "/a", nil, map[string]any{}, nil)
assert.EqualError(t, err, "response body: test error")
}

Expand All @@ -306,7 +309,7 @@ func TestSimpleRequestErrReaderCloseBody(t *testing.T) {
}),
rateLimiter: rate.NewLimiter(rate.Inf, 1),
}
err := c.Do(context.Background(), "PATCH", "/a", map[string]any{}, nil)
err := c.Do(context.Background(), "PATCH", "/a", nil, map[string]any{}, nil)
assert.EqualError(t, err, "response body: test error")
}

Expand All @@ -325,7 +328,7 @@ func TestSimpleRequestRawResponse(t *testing.T) {
rateLimiter: rate.NewLimiter(rate.Inf, 1),
}
var raw []byte
err := c.Do(context.Background(), "GET", "/a", nil, &raw)
err := c.Do(context.Background(), "GET", "/a", nil, nil, &raw)
assert.NoError(t, err)
assert.Equal(t, "Hello, world!", string(raw))
}
Expand Down Expand Up @@ -399,7 +402,7 @@ func TestSimpleResponseRedaction(t *testing.T) {
debugHeaders: true,
rateLimiter: rate.NewLimiter(rate.Inf, 1),
}
err := c.Do(context.Background(), "GET", "/a", map[string]any{
err := c.Do(context.Background(), "GET", "/a", nil, map[string]any{
"b": 0,
"a": 3,
"c": 23,
Expand Down Expand Up @@ -437,7 +440,7 @@ func TestInlineArrayDebugging(t *testing.T) {
debugTruncateBytes: 2048,
rateLimiter: rate.NewLimiter(rate.Inf, 1),
}
err := c.Do(context.Background(), "GET", "/a", map[string]any{
err := c.Do(context.Background(), "GET", "/a", nil, map[string]any{
"b": 0,
"a": 3,
"c": 23,
Expand Down
43 changes: 33 additions & 10 deletions service/billing/impl.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading