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 1 commit
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
29 changes: 29 additions & 0 deletions example_gh_test.go
View file
Open in desktop
Expand Up @@ -125,3 +125,32 @@ func ExampleCurrentRepository() {
}
fmt.Printf("%s/%s/%s\n", repo.Host(), repo.Owner(), repo.Name())
}

// Use SkipResolution ClientOption to change a http.Client into a api.RESTClient.
func ExampleHTTPClient_skipResolution() {
host := "github.com"
httpOpts := api.ClientOptions{
Host: host,
}
httpClient, err := HTTPClient(&httpOpts)
if err != nil {
log.Fatal(err)
}
// Use SkipResolution as our http.Client does the handling of
// options and headers.
restOpts := api.ClientOptions{
SkipResolution: true,
Host: host,
Transport: httpClient.Transport,
}
restClient, err := RESTClient(&restOpts)
if err != nil {
log.Fatal(err)
}
response := []struct{ Name string }{}
err = restClient.Get("repos/cli/cli/tags", &response)
if err != nil {
log.Fatal(err)
}
fmt.Println(response)
}
3 changes: 3 additions & 0 deletions gh.go
View file
Open in desktop
Expand Up @@ -141,6 +141,9 @@ func CurrentRepository() (repo.Repository, error) {
}

func optionsNeedResolution(opts *api.ClientOptions) bool {
if opts.SkipResolution {
return false
}
if opts.Host == "" {
return true
}
Expand Down
7 changes: 7 additions & 0 deletions gh_test.go
View file
Open in desktop
Expand Up @@ -262,6 +262,13 @@ func TestOptionsNeedResolution(t *testing.T) {
},
out: true,
},
{
name: "SkipResolution specified",
opts: &api.ClientOptions{
SkipResolution: true,
},
out: false,
},
}

for _, tt := range tests {
Expand Down
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.SkipResolution {
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
34 changes: 24 additions & 10 deletions internal/api/http_test.go
View file
Open in desktop
Expand Up @@ -25,12 +25,13 @@ func TestNewHTTPClient(t *testing.T) {
}

tests := []struct {
name string
enableLog bool
log *bytes.Buffer
host string
headers map[string]string
wantHeaders http.Header
name string
enableLog bool
log *bytes.Buffer
host string
headers map[string]string
skipResolution bool
wantHeaders http.Header
}{
{
name: "sets default headers",
Expand Down Expand Up @@ -94,6 +95,18 @@ func TestNewHTTPClient(t *testing.T) {
host: "TeSt.CoM",
wantHeaders: defaultHeaders(),
},
{
name: "skips resolving headers",
skipResolution: 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,
SkipResolution: tt.skipResolution,
Transport: reflectHTTP,
}
if tt.enableLog {
opts.Log = tt.log
Expand Down
6 changes: 6 additions & 0 deletions pkg/api/client.go
View file
Open in desktop
Expand Up @@ -38,6 +38,12 @@ type ClientOptions struct {
// Default is no logging.
Log io.Writer

// SkipResolution disables all automatic resolution of options and headers.
// This option is best used in conjunction with the Transport option,
// where the Transport mechanism already provides the necessary information
// for interacting with the API.
SkipResolution bool

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