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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from Oct 14, 2022
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7030bd8
Fix HTTP error handling in `RESTClient.Request()` method
mislav Oct 13, 2022
19b0b80
Merge branch 'trunk' into request-http-error
mislav Oct 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 0 additions & 4 deletions example_gh_test.go
View file
Open in desktop
Expand Up @@ -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)
Expand Down
12 changes: 5 additions & 7 deletions internal/api/rest_client.go
View file
Open in desktop
Expand Up @@ -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
Expand All @@ -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()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we move lines 70-72 above line 64 we can remove one of the defer resp.Body.Close() lines.

Copy link
Contributor Author

@mislav mislav Oct 14, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case of HTTP 204 No Content, resp.Body will be nil. This is why I'm closing the body in two different places— it's being consumed in two different places too


b, err := io.ReadAll(resp.Body)
if err != nil {
Expand Down
24 changes: 14 additions & 10 deletions internal/api/rest_client_test.go
View file
Open in desktop
Expand Up @@ -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",
Expand Down Expand Up @@ -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"}`,
},
{
Expand All @@ -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: `{}`,
Expand All @@ -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: `{}`,
Expand All @@ -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))
})
}
}
Expand Down