Fix HTTP error handling in RESTClient.Request() method (#82)

- Ensure that `Request()` calls HandleHTTPError so that any JSON error body properly gets parsed; - Ensure that response body is closed after reading the error response.
cli · Oct 14, 2022 · c2fc965 · c2fc965
1 parent 2479ec8
commit c2fc965
Show file tree

Hide file tree

Showing 3 changed files with 19 additions and 21 deletions.
diff --git a/example_gh_test.go b/example_gh_test.go
@@ -77,10 +77,6 @@ func ExampleRESTClient_request() {
 	}
 	defer resp.Body.Close()
 
-	if resp.StatusCode > 299 {
-		log.Fatal("server error")
-	}
-
 	f, err := os.CreateTemp("", "*_checksums.txt")
 	if err != nil {
 		log.Fatal(err)

diff --git a/internal/api/rest_client.go b/internal/api/rest_client.go
@@ -33,16 +33,13 @@ func (c restClient) RequestWithContext(ctx context.Context, method string, path
 
 	resp, err := c.client.Do(req)
 	if err != nil {
-		return resp, err
+		return nil, err
 	}
 
 	success := resp.StatusCode >= 200 && resp.StatusCode < 300
 	if !success {
-		err = api.HTTPError{
-			Headers:    resp.Header,
-			RequestURL: resp.Request.URL,
-			StatusCode: resp.StatusCode,
-		}
+		defer resp.Body.Close()
+		return nil, api.HandleHTTPError(resp)
 	}
 
 	return resp, err
@@ -63,16 +60,17 @@ func (c restClient) DoWithContext(ctx context.Context, method string, path strin
 	if err != nil {
 		return err
 	}
-	defer resp.Body.Close()
 
 	success := resp.StatusCode >= 200 && resp.StatusCode < 300
 	if !success {
+		defer resp.Body.Close()
 		return api.HandleHTTPError(resp)
 	}
 
 	if resp.StatusCode == http.StatusNoContent {
 		return nil
 	}
+	defer resp.Body.Close()
 
 	b, err := io.ReadAll(resp.Body)
 	if err != nil {

diff --git a/internal/api/rest_client_test.go b/internal/api/rest_client_test.go
@@ -30,10 +30,9 @@ func TestRESTClientRequest(t *testing.T) {
 			httpMocks: func() {
 				gock.New("https://api.github.com").
 					Get("/some/test/path").
-					Reply(204).
-					JSON(`{}`)
+					Reply(204)
 			},
-			wantBody: `{}`,
+			wantBody: ``,
 		},
 		{
 			name: "success request non-empty response",
@@ -69,7 +68,7 @@ func TestRESTClientRequest(t *testing.T) {
 					JSON(`{"message": "OH NO"}`)
 			},
 			wantErr:    true,
-			wantErrMsg: "HTTP 422 (https://api.github.com/some/test/path)",
+			wantErrMsg: "HTTP 422: OH NO (https://api.github.com/some/test/path)",
 			wantBody:   `{"message": "OH NO"}`,
 		},
 		{
@@ -78,7 +77,7 @@ func TestRESTClientRequest(t *testing.T) {
 			httpMocks: func() {
 				gock.New("https://example.com").
 					Get("/someother/test/path").
-					Reply(204).
+					Reply(200).
 					JSON(`{}`)
 			},
 			wantBody: `{}`,
@@ -90,7 +89,7 @@ func TestRESTClientRequest(t *testing.T) {
 			httpMocks: func() {
 				gock.New("https://enterprise.com").
 					Get("/some/test/path").
-					Reply(204).
+					Reply(200).
 					JSON(`{}`)
 			},
 			wantBody: `{}`,
@@ -107,17 +106,22 @@ func TestRESTClientRequest(t *testing.T) {
 				tt.httpMocks()
 			}
 			client := NewRESTClient(tt.host, nil)
+
 			resp, err := client.Request("GET", tt.path, nil)
-			t.Cleanup(func() { resp.Body.Close() })
-			body, readErr := io.ReadAll(resp.Body)
-			assert.NoError(t, readErr)
 			if tt.wantErr {
 				assert.EqualError(t, err, tt.wantErrMsg)
 			} else {
 				assert.NoError(t, err)
 			}
+
+			if err == nil {
+				defer resp.Body.Close()
+				body, err := io.ReadAll(resp.Body)
+				assert.NoError(t, err)
+				assert.Equal(t, tt.wantBody, string(body))
+			}
+
 			assert.True(t, gock.IsDone(), printPendingMocks(gock.Pending()))
-			assert.Equal(t, tt.wantBody, string(body))
 		})
 	}
 }