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

Allow skipping option and header resolution for api clients #56

Merged
merged 2 commits into from Jul 25, 2022
Merged

Allow skipping option and header resolution for api clients #56

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
daf108b
Allow skipping option and header resolution for api clients
samcoe Jul 25, 2022
02d6382
Address PR comments
samcoe Jul 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
28 changes: 20 additions & 8 deletions internal/api/http.go
View file
Open in desktop
Expand Up @@ -112,6 +112,12 @@ func NewHTTPClient(opts *api.ClientOptions) http.Client {
transport = logger.RoundTripper(transport)
}

if opts.Headers == nil {
opts.Headers = map[string]string{}
}
if !opts.SkipDefaultHeaders {
resolveHeaders(opts.Headers)
}
transport = newHeaderRoundTripper(opts.Host, opts.AuthToken, opts.Headers, transport)

return http.Client{Transport: transport, Timeout: opts.Timeout}
Expand Down Expand Up @@ -148,10 +154,7 @@ type headerRoundTripper struct {
rt http.RoundTripper
}

func newHeaderRoundTripper(host string, authToken string, headers map[string]string, rt http.RoundTripper) http.RoundTripper {
if headers == nil {
headers = map[string]string{}
}
func resolveHeaders(headers map[string]string) {
if _, ok := headers[contentType]; !ok {
headers[contentType] = jsonContentType
}
Expand All @@ -167,11 +170,11 @@ func newHeaderRoundTripper(host string, authToken string, headers map[string]str
}
}
}
if _, ok := headers[authorization]; !ok && authToken != "" {
headers[authorization] = fmt.Sprintf("token %s", authToken)
}
if _, ok := headers[timeZone]; !ok {
headers[timeZone] = currentTimeZone()
tz := currentTimeZone()
if tz != "" {
headers[timeZone] = tz
Comment on lines +175 to +176
Copy link
Contributor

Choose a reason for hiding this comment

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

This alone looks like it's sufficient to solve the original issue (sending the blank Time-Zone header). How come additional changes to the functionality are needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason that is not sufficient to solve the bug is because this line is explicitly telling go-gh not to resolve the "Time-Zone" header and leave it set to an empty string. We could detect that in go-gh and remove the "Time-Zone" header if it set to an empty string but that felt off since we have been following the rule that any user supplied values are not modified. Adding the SkipResolution options feels like a cleaner solution and also addresses the confusion you correctly identified here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough, but if there is going to be an additional config option, maybe it could be better documented? Right now I don't think it's clear what "resolution" is, since the user probably isn't familiar with the implementation of go-gh. Since the configuration option only affects whether default headers will be added, maybe the option could be called skipDefaultHeaders?

Copy link
Contributor Author

@samcoe samcoe Jul 25, 2022 •
edited

Choose a reason for hiding this comment

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

Great suggestion, that is a much more descriptive name.

}
}
if _, ok := headers[accept]; !ok {
// Preview for PullRequest.mergeStateStatus.
Expand All @@ -180,6 +183,15 @@ func newHeaderRoundTripper(host string, authToken string, headers map[string]str
a += ", application/vnd.github.nebula-preview"
headers[accept] = a
}
}

func newHeaderRoundTripper(host string, authToken string, headers map[string]string, rt http.RoundTripper) http.RoundTripper {
if _, ok := headers[authorization]; !ok && authToken != "" {
headers[authorization] = fmt.Sprintf("token %s", authToken)
}
if len(headers) == 0 {
return rt
}
return headerRoundTripper{host: host, headers: headers, rt: rt}
}

Expand Down
22 changes: 18 additions & 4 deletions internal/api/http_test.go
View file
Open in desktop
Expand Up @@ -30,6 +30,7 @@ func TestNewHTTPClient(t *testing.T) {
log *bytes.Buffer
host string
headers map[string]string
skipHeaders bool
wantHeaders http.Header
}{
{
Expand Down Expand Up @@ -94,6 +95,18 @@ func TestNewHTTPClient(t *testing.T) {
host: "TeSt.CoM",
wantHeaders: defaultHeaders(),
},
{
name: "skips default headers",
skipHeaders: true,
wantHeaders: func() http.Header {
h := defaultHeaders()
h.Del(accept)
h.Del(contentType)
h.Del(timeZone)
h.Del(userAgent)
return h
}(),
},
}

for _, tt := range tests {
Expand All @@ -102,10 +115,11 @@ func TestNewHTTPClient(t *testing.T) {
tt.host = "test.com"
}
opts := api.ClientOptions{
Host: tt.host,
AuthToken: "oauth_token",
Headers: tt.headers,
Transport: reflectHTTP,
Host: tt.host,
AuthToken: "oauth_token",
Headers: tt.headers,
SkipDefaultHeaders: tt.skipHeaders,
Transport: reflectHTTP,
}
if tt.enableLog {
opts.Log = tt.log
Expand Down
5 changes: 4 additions & 1 deletion pkg/api/client.go
View file
Open in desktop
Expand Up @@ -27,7 +27,7 @@ type ClientOptions struct {
EnableCache bool

// Headers are the headers that will be sent with every API request.
// Default headers set are Accept, Authorization, Content-Type, Time-Zone, and User-Agent.
// Default headers set are Accept, Content-Type, Time-Zone, and User-Agent.
// Default headers will be overridden by keys specified in Headers.
Headers map[string]string

Expand All @@ -38,6 +38,9 @@ type ClientOptions struct {
// Default is no logging.
Log io.Writer

// SkipDefaultHeaders disables setting of the default headers.
SkipDefaultHeaders bool

// Timeout specifies a time limit for each API request.
// Default is no timeout.
Timeout time.Duration
Expand Down