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.

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

Robust Header Check #13

Closed
wants to merge 12 commits into from
2 changes: 1 addition & 1 deletion appsTransport.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ func (t *AppsTransport) RoundTrip(req *http.Request) (*http.Response, error) {
}

req.Header.Set("Authorization", "Bearer "+ss)
req.Header.Set("Accept", acceptHeader)
addAcceptHeader(req.Header)

resp, err := t.tr.RoundTrip(req)
return resp, err
Expand Down
19 changes: 18 additions & 1 deletion transport.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"fmt"
"io/ioutil"
"net/http"
"strings"
"sync"
"time"
)
Expand Down Expand Up @@ -90,7 +91,7 @@ func (t *Transport) RoundTrip(req *http.Request) (*http.Response, error) {
}

req.Header.Set("Authorization", "token "+token)
req.Header.Add("Accept", acceptHeader) // We add to "Accept" header to avoid overwriting existing req headers.
addAcceptHeader(req.Header)
resp, err := t.tr.RoundTrip(req)
return resp, err
}
Expand Down Expand Up @@ -134,3 +135,19 @@ func (t *Transport) refreshToken() error {

return nil
}

func addAcceptHeader(headers http.Header) {
Copy link

@dmitshur dmitshur May 10, 2018

Choose a reason for hiding this comment

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

It would be nice to document what the function does. Perhaps something like this:

// addAcceptHeader adds acceptHeader value to "Accept" header, but only
// if the current "Accept" header is not set, or if it already accepts JSON.
func addAcceptHeader(headers http.Header) {

Copy link
Author

Choose a reason for hiding this comment

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

Copied it directly (seemed like a good description) and pushed.

if headers.Get("Accept") == "" {
headers.Set("Accept", acceptHeader)
return
}

//Need to loop through all Accept headers incase there is more than one.

Choose a reason for hiding this comment

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

Minor style issue. Since this is a normal comment for humans rather than for the compiler, it should have a space after //, like so:

// Need to ...

See https://dmitri.shuralyov.com/idiomatic-go#comments-for-humans-always-have-a-single-space-after-the-slashes for rationale.

Also, “incase” should be two words I think.

Choose a reason for hiding this comment

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

@cpheps Did you see this comment?

Copy link
Author

Choose a reason for hiding this comment

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

Missed the second one, just pushed a fix.

Choose a reason for hiding this comment

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

What about the first part? I still see some comments that don't have a space after //.

for _, header := range headers["Accept"] {
//Looks as though all media types (https://developer.github.com/v3/media/) that can accept json end with "json". Only doing a suffix check to see if a json header already exists.

Choose a reason for hiding this comment

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

Here as well:

// Looks as though ...

You might also want to wrap the line at around 80-100 columns, so it’s more comfortable to read.

if strings.HasSuffix(header, "json") {
headers.Add("Accept", acceptHeader) // We add to "Accept" header to avoid overwriting existing req headers.
return
}
}
}
76 changes: 76 additions & 0 deletions transport_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"net/http"
"net/http/httptest"
"os"
"strings"
"testing"
"time"
)
Expand Down Expand Up @@ -173,3 +174,78 @@ func TestNew_appendHeader(t *testing.T) {
t.Errorf("could not find %v in request's accept headers: %v", myheader, headers["Accept"])
}
}

func TestSingleAssetBinary_AddAcceptHeader(t *testing.T) {
req, err := http.NewRequest(http.MethodGet, "https://api.github.com/repos/bradleyfalzon/ghinstallation/releases/assets/1", http.NoBody)
if err != nil {
t.Fatalf("Failed to create test request: %s", err.Error())
}

req.Header.Set("Accept", "application/octet-stream")

addAcceptHeader(req.Header)

for headerName, headers := range req.Header {
if strings.ToLower(headerName) == "accept" {
for _, header := range headers {
if header == acceptHeader {
fmt.Printf("Header Name: %s, Header: %s", headerName, header)
t.Error("Set Accept header improperly for single asset call")
}
}
}
}
}

func TestNormal_AddAcceptHeader(t *testing.T) {
req, err := http.NewRequest(http.MethodGet, "https://api.github.com/repos", http.NoBody)
if err != nil {
t.Fatalf("Failed to create test request: %s", err.Error())
}

addAcceptHeader(req.Header)

if req.Header.Get("Accept") != acceptHeader {
t.Error("Did not correctly set 'Accept' header")
}
}

func TestAlreadyExists_AddAcceptHeader(t *testing.T) {
req, err := http.NewRequest(http.MethodGet, "https://api.github.com/repos", http.NoBody)
if err != nil {
t.Fatalf("Failed to create test request: %s", err.Error())
}

req.Header.Set("accept", "application/vnd.github.v3+json")

addAcceptHeader(req.Header)

found := false
for headerName, headers := range req.Header {
if strings.ToLower(headerName) == "accept" {
for _, header := range headers {
if header == acceptHeader {
found = true
break
}
}
}
}

if !found {
t.Errorf("Did not correctly append %s header", acceptHeader)
}
}

func TestSingleAssetJSON_AddAcceptHeader(t *testing.T) {
req, err := http.NewRequest(http.MethodGet, "https://api.github.com/repos/bradleyfalzon/ghinstallation/releases/assets/1", http.NoBody)
if err != nil {
t.Fatalf("Failed to create test request: %s", err.Error())
}

addAcceptHeader(req.Header)

if req.Header.Get("Accept") != acceptHeader {
t.Error("Did not correctly set 'Accept' header")
}
}