From 9bd8d2f2a2d8fbb43e8bc740771737cea305f2c7 Mon Sep 17 00:00:00 2001 From: Devin Date: Fri, 6 Dec 2024 07:28:05 +0000 Subject: [PATCH 1/4] Move response creation earlier in BareDo and preserve response in URL errors - Move newResponse call earlier to avoid code duplication - Properly handle nil responses with var declaration - Return response object with URL errors to preserve HTTP information - Add test case using client.Do to verify behavior --- github/github.go | 11 +++++++---- github/github_test.go | 21 +++++++++++++++++++++ 2 files changed, 28 insertions(+), 4 deletions(-) diff --git a/github/github.go b/github/github.go index 6f476353473..29d59fad767 100644 --- a/github/github.go +++ b/github/github.go @@ -841,6 +841,11 @@ func (c *Client) BareDo(ctx context.Context, req *http.Request) (*Response, erro } resp, err := c.client.Do(req) + var response *Response + if resp != nil { + response = newResponse(resp) + } + if err != nil { // If we got an error, and the context has been canceled, // the context's error is probably more useful. @@ -854,15 +859,13 @@ func (c *Client) BareDo(ctx context.Context, req *http.Request) (*Response, erro if e, ok := err.(*url.Error); ok { if url, err := url.Parse(e.URL); err == nil { e.URL = sanitizeURL(url).String() - return nil, e } + return response, e } - return nil, err + return response, err } - response := newResponse(resp) - // Don't update the rate limits if this was a cached response. // X-From-Cache is set by https://github.com/gregjones/httpcache if response.Header.Get("X-From-Cache") == "" { diff --git a/github/github_test.go b/github/github_test.go index 73afa2d1968..4648facae8b 100644 --- a/github/github_test.go +++ b/github/github_test.go @@ -1106,6 +1106,27 @@ func TestDo_redirectLoop(t *testing.T) { } } +func TestDo_preservesResponseInURLError(t *testing.T) { + t.Parallel() + client, mux, _ := setup(t) + mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + http.Error(w, "Not Found", http.StatusNotFound) + }) + + req, _ := client.NewRequest("GET", ".", nil) + resp, err := client.Do(context.Background(), req, nil) + + if err == nil { + t.Fatal("Expected error to be returned") + } + if resp == nil { + t.Fatal("Expected response to be returned even with error") + } + if resp.StatusCode != http.StatusNotFound { + t.Errorf("Expected status code 404, got %d", resp.StatusCode) + } +} + // Test that an error caused by the internal http client's Do() function // does not leak the client secret. func TestDo_sanitizeURL(t *testing.T) { From d341400c40cf576069d9a078295e508756425fc0 Mon Sep 17 00:00:00 2001 From: Silas Alberti Date: Fri, 6 Dec 2024 00:37:04 -0700 Subject: [PATCH 2/4] fix stray diff --- github/github.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/github/github.go b/github/github.go index 29d59fad767..2856da7d322 100644 --- a/github/github.go +++ b/github/github.go @@ -859,8 +859,8 @@ func (c *Client) BareDo(ctx context.Context, req *http.Request) (*Response, erro if e, ok := err.(*url.Error); ok { if url, err := url.Parse(e.URL); err == nil { e.URL = sanitizeURL(url).String() + return response, e } - return response, e } return response, err From d959665a00466bc47eac28330c18e6b165c67334 Mon Sep 17 00:00:00 2001 From: Devin Date: Sat, 7 Dec 2024 03:42:06 +0000 Subject: [PATCH 3/4] Rename test to TestDo_preservesResponseInHTTPError for clarity --- 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 4648facae8b..58f24ea9dd5 100644 --- a/github/github_test.go +++ b/github/github_test.go @@ -1106,7 +1106,7 @@ func TestDo_redirectLoop(t *testing.T) { } } -func TestDo_preservesResponseInURLError(t *testing.T) { +func TestDo_preservesResponseInHTTPError(t *testing.T) { t.Parallel() client, mux, _ := setup(t) mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { From d9e59c11d717c643792418ce25173db80770376d Mon Sep 17 00:00:00 2001 From: Silas Alberti Date: Fri, 6 Dec 2024 20:49:31 -0700 Subject: [PATCH 4/4] recover devin's local changes --- github/github.go | 2 +- github/github_test.go | 34 +++++++++++++++++++++++++++++----- 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/github/github.go b/github/github.go index 2856da7d322..4f94f2481a4 100644 --- a/github/github.go +++ b/github/github.go @@ -851,7 +851,7 @@ func (c *Client) BareDo(ctx context.Context, req *http.Request) (*Response, erro // the context's error is probably more useful. select { case <-ctx.Done(): - return nil, ctx.Err() + return response, ctx.Err() default: } diff --git a/github/github_test.go b/github/github_test.go index 58f24ea9dd5..48ac2ab26e5 100644 --- a/github/github_test.go +++ b/github/github_test.go @@ -1109,21 +1109,45 @@ func TestDo_redirectLoop(t *testing.T) { func TestDo_preservesResponseInHTTPError(t *testing.T) { t.Parallel() client, mux, _ := setup(t) + mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { - http.Error(w, "Not Found", http.StatusNotFound) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusNotFound) + fmt.Fprintf(w, `{ + "message": "Resource not found", + "documentation_url": "https://docs.github.com/rest/reference/repos#get-a-repository" + }`) }) req, _ := client.NewRequest("GET", ".", nil) - resp, err := client.Do(context.Background(), req, nil) + var resp *Response + var data interface{} + resp, err := client.Do(context.Background(), req, &data) if err == nil { - t.Fatal("Expected error to be returned") + t.Fatal("Expected error response") + } + + // Verify error type and access to status code + errResp, ok := err.(*ErrorResponse) + if !ok { + t.Fatalf("Expected *ErrorResponse error, got %T", err) } + + // Verify status code is accessible from both Response and ErrorResponse if resp == nil { t.Fatal("Expected response to be returned even with error") } - if resp.StatusCode != http.StatusNotFound { - t.Errorf("Expected status code 404, got %d", resp.StatusCode) + if got, want := resp.StatusCode, http.StatusNotFound; got != want { + t.Errorf("Response status = %d, want %d", got, want) + } + if got, want := errResp.Response.StatusCode, http.StatusNotFound; got != want { + t.Errorf("Error response status = %d, want %d", got, want) + } + + // Verify error contains proper message + if !strings.Contains(errResp.Message, "Resource not found") { + t.Errorf("Error message = %q, want to contain 'Resource not found'", errResp.Message) } }