From e4cbdda6529dd2f8cf3bcd00a2d0103906e503ca Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Tue, 5 Jan 2021 14:45:36 +0100 Subject: [PATCH 1/6] client.Do: when v (potentially receiving interface) is nil return the body without closing it This allows the user to also treat bodies as plain go streams. For more flexibility. Before this passing a nil body would do the request but close the body. Now it is up to the caller to close resp.Body. --- github/github.go | 45 ++++++++++++++++++++++++--------------------- 1 file changed, 24 insertions(+), 21 deletions(-) diff --git a/github/github.go b/github/github.go index 957ac0c7ff8..815058dcba0 100644 --- a/github/github.go +++ b/github/github.go @@ -514,13 +514,14 @@ func parseRate(r *http.Response) Rate { // Do sends an API request and returns the API response. The API response is // JSON decoded and stored in the value pointed to by v, or returned as an -// error if an API error has occurred. If v implements the io.Writer -// interface, the raw response body will be written to v, without attempting to -// first decode it. If rate limit is exceeded and reset time is in the future, -// Do returns *RateLimitError immediately without making a network API call. +// error if an API error has occurred. If v implements the io.Writer interface, +// the raw response body will be written to v, without attempting to first +// decode it. If v is nil, and no error hapens, the response is returned as is. +// If rate limit is exceeded and reset time is in the future, Do returns +// *RateLimitError immediately without making a network API call. // -// The provided ctx must be non-nil, if it is nil an error is returned. If it is canceled or times out, -// ctx.Err() will be returned. +// The provided ctx must be non-nil, if it is nil an error is returned. If it +// is canceled or times out, ctx.Err() will be returned. func (c *Client) Do(ctx context.Context, req *http.Request, v interface{}) (*Response, error) { if ctx == nil { return nil, errors.New("context must be non-nil") @@ -558,8 +559,6 @@ func (c *Client) Do(ctx context.Context, req *http.Request, v interface{}) (*Res return nil, err } - defer resp.Body.Close() - response := newResponse(resp) c.rateMu.Lock() @@ -568,6 +567,7 @@ func (c *Client) Do(ctx context.Context, req *http.Request, v interface{}) (*Res err = CheckResponse(resp) if err != nil { + defer resp.Body.Close() // Special case for AcceptedErrors. If an AcceptedError // has been encountered, the response's payload will be // added to the AcceptedError and returned. @@ -587,21 +587,24 @@ func (c *Client) Do(ctx context.Context, req *http.Request, v interface{}) (*Res return response, err } - if v != nil { - if w, ok := v.(io.Writer); ok { - io.Copy(w, resp.Body) - } else { - decErr := json.NewDecoder(resp.Body).Decode(v) - if decErr == io.EOF { - decErr = nil // ignore EOF errors caused by empty response body - } - if decErr != nil { - err = decErr - } + switch v := v.(type) { + case nil: + return response, err + case io.Writer: + defer resp.Body.Close() + _, _ = io.Copy(v, resp.Body) + return response, err + default: + defer resp.Body.Close() + decErr := json.NewDecoder(resp.Body).Decode(v) + if decErr == io.EOF { + decErr = nil // ignore EOF errors caused by empty response body + } + if decErr != nil { + err = decErr } + return response, err } - - return response, err } // checkRateLimitBeforeDo does not make any network calls, but uses existing knowledge from From f7773038d4233ebb1b82c90c30da744a9f046d78 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Wed, 6 Jan 2021 12:53:53 +0100 Subject: [PATCH 2/6] add a new `BareDo` that returns a bare body --- github/github.go | 39 ++++++++++++++++++++++++--------------- 1 file changed, 24 insertions(+), 15 deletions(-) diff --git a/github/github.go b/github/github.go index 815058dcba0..01c3e3867ce 100644 --- a/github/github.go +++ b/github/github.go @@ -512,17 +512,13 @@ func parseRate(r *http.Response) Rate { return rate } -// Do sends an API request and returns the API response. The API response is -// JSON decoded and stored in the value pointed to by v, or returned as an -// error if an API error has occurred. If v implements the io.Writer interface, -// the raw response body will be written to v, without attempting to first -// decode it. If v is nil, and no error hapens, the response is returned as is. -// If rate limit is exceeded and reset time is in the future, Do returns -// *RateLimitError immediately without making a network API call. +// BareDo sends an API request and lets you handle the api response. If an +// error or API Error occurs, the error will contain more information. +// Otherwise you are supposed to read and close the response's Body. // // The provided ctx must be non-nil, if it is nil an error is returned. If it // is canceled or times out, ctx.Err() will be returned. -func (c *Client) Do(ctx context.Context, req *http.Request, v interface{}) (*Response, error) { +func (c *Client) BareDo(ctx context.Context, req *http.Request) (*Response, error) { if ctx == nil { return nil, errors.New("context must be non-nil") } @@ -581,21 +577,34 @@ func (c *Client) Do(ctx context.Context, req *http.Request, v interface{}) (*Res } aerr.Raw = b - return response, aerr + err = aerr } + } + return response, err +} - return response, err +// Do sends an API request and returns the API response. The API response is +// JSON decoded and stored in the value pointed to by v, or returned as an +// error if an API error has occurred. If v implements the io.Writer interface, +// the raw response body will be written to v, without attempting to first +// decode it. If v is nil, and no error hapens, the response is returned as is. +// If rate limit is exceeded and reset time is in the future, Do returns +// *RateLimitError immediately without making a network API call. +// +// The provided ctx must be non-nil, if it is nil an error is returned. If it +// is canceled or times out, ctx.Err() will be returned. +func (c *Client) Do(ctx context.Context, req *http.Request, v interface{}) (*Response, error) { + resp, err := c.BareDo(ctx, req) + if err != nil { + return resp, err } + defer resp.Body.Close() switch v := v.(type) { case nil: - return response, err case io.Writer: - defer resp.Body.Close() _, _ = io.Copy(v, resp.Body) - return response, err default: - defer resp.Body.Close() decErr := json.NewDecoder(resp.Body).Decode(v) if decErr == io.EOF { decErr = nil // ignore EOF errors caused by empty response body @@ -603,8 +612,8 @@ func (c *Client) Do(ctx context.Context, req *http.Request, v interface{}) (*Res if decErr != nil { err = decErr } - return response, err } + return resp, err } // checkRateLimitBeforeDo does not make any network calls, but uses existing knowledge from From 301b39489554ea7d18becb630766ff37319b26ec Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Mon, 11 Jan 2021 10:48:46 +0100 Subject: [PATCH 3/6] BareDo: say what happens in case of rate limiting. --- github/github.go | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/github/github.go b/github/github.go index 01c3e3867ce..35ac16ac562 100644 --- a/github/github.go +++ b/github/github.go @@ -512,12 +512,14 @@ func parseRate(r *http.Response) Rate { return rate } -// BareDo sends an API request and lets you handle the api response. If an -// error or API Error occurs, the error will contain more information. -// Otherwise you are supposed to read and close the response's Body. +// BareDo sends an API request and lets you handle the api response. If an error +// or API Error occurs, the error will contain more information. Otherwise you +// are supposed to read and close the response's Body. If rate limit is exceeded +// and reset time is in the future, BareDo returns *RateLimitError immediately +// without making a network API call. // -// The provided ctx must be non-nil, if it is nil an error is returned. If it -// is canceled or times out, ctx.Err() will be returned. +// The provided ctx must be non-nil, if it is nil an error is returned. If it is +// canceled or times out, ctx.Err() will be returned. func (c *Client) BareDo(ctx context.Context, req *http.Request) (*Response, error) { if ctx == nil { return nil, errors.New("context must be non-nil") From 495e1710a46c0f208cb282f748774495c42effae Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Mon, 11 Jan 2021 10:51:29 +0100 Subject: [PATCH 4/6] Do: don't ignore a potential error when copying to v --- github/github.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/github/github.go b/github/github.go index 35ac16ac562..0a32a53c447 100644 --- a/github/github.go +++ b/github/github.go @@ -605,7 +605,7 @@ func (c *Client) Do(ctx context.Context, req *http.Request, v interface{}) (*Res switch v := v.(type) { case nil: case io.Writer: - _, _ = io.Copy(v, resp.Body) + _, err = io.Copy(v, resp.Body) default: decErr := json.NewDecoder(resp.Body).Decode(v) if decErr == io.EOF { From a58042e34e6d258b9ce3b1d7689e669c5836d23b Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Mon, 11 Jan 2021 14:21:11 +0100 Subject: [PATCH 5/6] add TestBareDo_returnsOpenBody test --- github/github_test.go | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/github/github_test.go b/github/github_test.go index e95c2d00b00..5d3a18cb78c 100644 --- a/github/github_test.go +++ b/github/github_test.go @@ -1560,3 +1560,38 @@ func TestAddOptions_QueryValues(t *testing.T) { t.Error("addOptions err = nil, want error") } } + +func TestBareDo_returnsOpenBody(t *testing.T) { + + client, mux, _, teardown := setup() + defer teardown() + + expectedBody := "Hello from the other side !" + + mux.HandleFunc("/test-url", func(w http.ResponseWriter, r *http.Request) { + testMethod(t, r, "GET") + fmt.Fprint(w, expectedBody) + }) + + ctx := context.Background() + req, err := client.NewRequest("GET", "test-url", nil) + if err != nil { + t.Fatalf("client.NewRequest returned error: %v", err) + } + + resp, err := client.BareDo(ctx, req) + if err != nil { + t.Fatalf("client.BareDo returned error: %v", err) + } + + got, err := ioutil.ReadAll(resp.Body) + if err != nil { + t.Fatalf("cioutil.ReadAll returned error: %v", err) + } + if string(got) != expectedBody { + t.Fatalf("Expected %q, got %q", expectedBody, string(got)) + } + if err := resp.Body.Close(); err != nil { + t.Fatalf("resp.Body.Close() returned error: %v", err) + } +} From 6d84976ec1fc4b7b606c04f19af87b8da17fb9fb Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Mon, 11 Jan 2021 15:16:07 +0100 Subject: [PATCH 6/6] Update github/github_test.go fix typo Co-authored-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com> --- github/github_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/github/github_test.go b/github/github_test.go index 5d3a18cb78c..db50b465555 100644 --- a/github/github_test.go +++ b/github/github_test.go @@ -1586,7 +1586,7 @@ func TestBareDo_returnsOpenBody(t *testing.T) { got, err := ioutil.ReadAll(resp.Body) if err != nil { - t.Fatalf("cioutil.ReadAll returned error: %v", err) + t.Fatalf("ioutil.ReadAll returned error: %v", err) } if string(got) != expectedBody { t.Fatalf("Expected %q, got %q", expectedBody, string(got))