From 4cbffd843a5b684b34a4b73aedbc2f9ea455d315 Mon Sep 17 00:00:00 2001 From: David McIntosh Date: Fri, 29 May 2020 17:43:06 -0700 Subject: [PATCH 1/4] Return http.Response from DownloadContents Fixes #1531 --- github/repos_contents.go | 16 +++++++----- github/repos_contents_test.go | 47 ++++++++++++++++++++++++++++++++--- 2 files changed, 54 insertions(+), 9 deletions(-) diff --git a/github/repos_contents.go b/github/repos_contents.go index 02d45a12ce0..093229589a8 100644 --- a/github/repos_contents.go +++ b/github/repos_contents.go @@ -117,26 +117,30 @@ func (s *RepositoriesService) GetReadme(ctx context.Context, owner, repo string, // specified file. This function will work with files of any size, as opposed // to GetContents which is limited to 1 Mb files. It is the caller's // responsibility to close the ReadCloser. -func (s *RepositoriesService) DownloadContents(ctx context.Context, owner, repo, filepath string, opts *RepositoryContentGetOptions) (io.ReadCloser, error) { +// +// It is possible for the download to result in a failed response. Callers +// should check the returned http.Response status code to verify the content +// is from a successful response. +func (s *RepositoriesService) DownloadContents(ctx context.Context, owner, repo, filepath string, opts *RepositoryContentGetOptions) (io.ReadCloser, *http.Response, error) { dir := path.Dir(filepath) filename := path.Base(filepath) _, dirContents, _, err := s.GetContents(ctx, owner, repo, dir, opts) if err != nil { - return nil, err + return nil, nil, err } for _, contents := range dirContents { if *contents.Name == filename { if contents.DownloadURL == nil || *contents.DownloadURL == "" { - return nil, fmt.Errorf("No download link found for %s", filepath) + return nil, nil, fmt.Errorf("No download link found for %s", filepath) } resp, err := s.client.client.Get(*contents.DownloadURL) if err != nil { - return nil, err + return nil, resp, err } - return resp.Body, nil + return resp.Body, resp, nil } } - return nil, fmt.Errorf("No file named %s found in %s", filename, dir) + return nil, nil, fmt.Errorf("No file named %s found in %s", filename, dir) } // GetContents can return either the metadata and content of a single file diff --git a/github/repos_contents_test.go b/github/repos_contents_test.go index f122525800a..3c1baa7b53c 100644 --- a/github/repos_contents_test.go +++ b/github/repos_contents_test.go @@ -118,11 +118,15 @@ func TestRepositoriesService_DownloadContents_Success(t *testing.T) { fmt.Fprint(w, "foo") }) - r, err := client.Repositories.DownloadContents(context.Background(), "o", "r", "d/f", nil) + r, resp, err := client.Repositories.DownloadContents(context.Background(), "o", "r", "d/f", nil) if err != nil { t.Errorf("Repositories.DownloadContents returned error: %v", err) } + if got, want := resp.StatusCode, http.StatusOK; got != want { + t.Errorf("Repositories.DownloadContents returned status code %v, want %v", got, want) + } + bytes, err := ioutil.ReadAll(r) if err != nil { t.Errorf("Error reading response body: %v", err) @@ -134,6 +138,43 @@ func TestRepositoriesService_DownloadContents_Success(t *testing.T) { } } +func TestRepositoriesService_DownloadContents_FailedResponse(t *testing.T) { + client, mux, serverURL, teardown := setup() + defer teardown() + mux.HandleFunc("/repos/o/r/contents/d", func(w http.ResponseWriter, r *http.Request) { + testMethod(t, r, "GET") + fmt.Fprint(w, `[{ + "type": "file", + "name": "f", + "download_url": "`+serverURL+baseURLPath+`/download/f" + }]`) + }) + mux.HandleFunc("/download/f", func(w http.ResponseWriter, r *http.Request) { + testMethod(t, r, "GET") + w.WriteHeader(http.StatusInternalServerError) + fmt.Fprint(w, "foo error") + }) + + r, resp, err := client.Repositories.DownloadContents(context.Background(), "o", "r", "d/f", nil) + if err != nil { + t.Errorf("Repositories.DownloadContents returned error: %v", err) + } + + if got, want := resp.StatusCode, http.StatusInternalServerError; got != want { + t.Errorf("Repositories.DownloadContents returned status code %v, want %v", got, want) + } + + bytes, err := ioutil.ReadAll(r) + if err != nil { + t.Errorf("Error reading response body: %v", err) + } + r.Close() + + if got, want := string(bytes), "foo error"; got != want { + t.Errorf("Repositories.DownloadContents returned %v, want %v", got, want) + } +} + func TestRepositoriesService_DownloadContents_NoDownloadURL(t *testing.T) { client, mux, _, teardown := setup() defer teardown() @@ -145,7 +186,7 @@ func TestRepositoriesService_DownloadContents_NoDownloadURL(t *testing.T) { }]`) }) - _, err := client.Repositories.DownloadContents(context.Background(), "o", "r", "d/f", nil) + _, _, err := client.Repositories.DownloadContents(context.Background(), "o", "r", "d/f", nil) if err == nil { t.Errorf("Repositories.DownloadContents did not return expected error") } @@ -159,7 +200,7 @@ func TestRepositoriesService_DownloadContents_NoFile(t *testing.T) { fmt.Fprint(w, `[]`) }) - _, err := client.Repositories.DownloadContents(context.Background(), "o", "r", "d/f", nil) + _, _, err := client.Repositories.DownloadContents(context.Background(), "o", "r", "d/f", nil) if err == nil { t.Errorf("Repositories.DownloadContents did not return expected error") } From b0a9465a5b7569f122905afcdef615288e1b8ba3 Mon Sep 17 00:00:00 2001 From: David McIntosh Date: Sat, 30 May 2020 16:20:38 -0700 Subject: [PATCH 2/4] Return github.Response from DownloadContents instead of http.Response --- github/repos_contents.go | 20 ++++++++++---------- github/repos_contents_test.go | 16 ++++++++++++---- 2 files changed, 22 insertions(+), 14 deletions(-) diff --git a/github/repos_contents.go b/github/repos_contents.go index 093229589a8..ae8291f42b0 100644 --- a/github/repos_contents.go +++ b/github/repos_contents.go @@ -118,29 +118,29 @@ func (s *RepositoriesService) GetReadme(ctx context.Context, owner, repo string, // to GetContents which is limited to 1 Mb files. It is the caller's // responsibility to close the ReadCloser. // -// It is possible for the download to result in a failed response. Callers -// should check the returned http.Response status code to verify the content -// is from a successful response. -func (s *RepositoriesService) DownloadContents(ctx context.Context, owner, repo, filepath string, opts *RepositoryContentGetOptions) (io.ReadCloser, *http.Response, error) { +// It is possible for the download to result in a failed response when the +// returned error is nil. Callers should check the returned http.Response +// status code to verify the content is from a successful response. +func (s *RepositoriesService) DownloadContents(ctx context.Context, owner, repo, filepath string, opts *RepositoryContentGetOptions) (io.ReadCloser, *Response, error) { dir := path.Dir(filepath) filename := path.Base(filepath) - _, dirContents, _, err := s.GetContents(ctx, owner, repo, dir, opts) + _, dirContents, resp, err := s.GetContents(ctx, owner, repo, dir, opts) if err != nil { - return nil, nil, err + return nil, resp, err } for _, contents := range dirContents { if *contents.Name == filename { if contents.DownloadURL == nil || *contents.DownloadURL == "" { return nil, nil, fmt.Errorf("No download link found for %s", filepath) } - resp, err := s.client.client.Get(*contents.DownloadURL) + dlResp, err := s.client.client.Get(*contents.DownloadURL) if err != nil { - return nil, resp, err + return nil, &Response{Response: dlResp}, err } - return resp.Body, resp, nil + return dlResp.Body, &Response{Response: dlResp}, nil } } - return nil, nil, fmt.Errorf("No file named %s found in %s", filename, dir) + return nil, resp, fmt.Errorf("No file named %s found in %s", filename, dir) } // GetContents can return either the metadata and content of a single file diff --git a/github/repos_contents_test.go b/github/repos_contents_test.go index 3c1baa7b53c..a0c4695c202 100644 --- a/github/repos_contents_test.go +++ b/github/repos_contents_test.go @@ -123,7 +123,7 @@ func TestRepositoriesService_DownloadContents_Success(t *testing.T) { t.Errorf("Repositories.DownloadContents returned error: %v", err) } - if got, want := resp.StatusCode, http.StatusOK; got != want { + if got, want := resp.Response.StatusCode, http.StatusOK; got != want { t.Errorf("Repositories.DownloadContents returned status code %v, want %v", got, want) } @@ -160,7 +160,7 @@ func TestRepositoriesService_DownloadContents_FailedResponse(t *testing.T) { t.Errorf("Repositories.DownloadContents returned error: %v", err) } - if got, want := resp.StatusCode, http.StatusInternalServerError; got != want { + if got, want := resp.Response.StatusCode, http.StatusInternalServerError; got != want { t.Errorf("Repositories.DownloadContents returned status code %v, want %v", got, want) } @@ -186,10 +186,14 @@ func TestRepositoriesService_DownloadContents_NoDownloadURL(t *testing.T) { }]`) }) - _, _, err := client.Repositories.DownloadContents(context.Background(), "o", "r", "d/f", nil) + _, resp, err := client.Repositories.DownloadContents(context.Background(), "o", "r", "d/f", nil) if err == nil { t.Errorf("Repositories.DownloadContents did not return expected error") } + + if resp == nil { + t.Errorf("Repositories.DownloadContents did not return expected response") + } } func TestRepositoriesService_DownloadContents_NoFile(t *testing.T) { @@ -200,10 +204,14 @@ func TestRepositoriesService_DownloadContents_NoFile(t *testing.T) { fmt.Fprint(w, `[]`) }) - _, _, err := client.Repositories.DownloadContents(context.Background(), "o", "r", "d/f", nil) + _, resp, err := client.Repositories.DownloadContents(context.Background(), "o", "r", "d/f", nil) if err == nil { t.Errorf("Repositories.DownloadContents did not return expected error") } + + if resp == nil { + t.Errorf("Repositories.DownloadContents did not return expected response") + } } func TestRepositoriesService_GetContents_File(t *testing.T) { From 05fcb0a465da3360833787f51c4bd77ae48f469c Mon Sep 17 00:00:00 2001 From: David McIntosh Date: Sat, 30 May 2020 16:31:44 -0700 Subject: [PATCH 3/4] Return resp when no download link is found --- github/repos_contents.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/github/repos_contents.go b/github/repos_contents.go index ae8291f42b0..8c48eb9d80f 100644 --- a/github/repos_contents.go +++ b/github/repos_contents.go @@ -131,7 +131,7 @@ func (s *RepositoriesService) DownloadContents(ctx context.Context, owner, repo, for _, contents := range dirContents { if *contents.Name == filename { if contents.DownloadURL == nil || *contents.DownloadURL == "" { - return nil, nil, fmt.Errorf("No download link found for %s", filepath) + return nil, resp, fmt.Errorf("No download link found for %s", filepath) } dlResp, err := s.client.client.Get(*contents.DownloadURL) if err != nil { From 7802c660866015d2bc5d9c76cfbbf14dc01f14ee Mon Sep 17 00:00:00 2001 From: David McIntosh Date: Sat, 30 May 2020 16:35:12 -0700 Subject: [PATCH 4/4] Update doc to reference Response instead of http.Response --- github/repos_contents.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/github/repos_contents.go b/github/repos_contents.go index 8c48eb9d80f..782daf74226 100644 --- a/github/repos_contents.go +++ b/github/repos_contents.go @@ -119,8 +119,8 @@ func (s *RepositoriesService) GetReadme(ctx context.Context, owner, repo string, // responsibility to close the ReadCloser. // // It is possible for the download to result in a failed response when the -// returned error is nil. Callers should check the returned http.Response -// status code to verify the content is from a successful response. +// returned error is nil. Callers should check the returned Response status +// code to verify the content is from a successful response. func (s *RepositoriesService) DownloadContents(ctx context.Context, owner, repo, filepath string, opts *RepositoryContentGetOptions) (io.ReadCloser, *Response, error) { dir := path.Dir(filepath) filename := path.Base(filepath)