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
20 changes: 19 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,20 @@ 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 in case there is more than one.
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.
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")
}
}