-
Notifications
You must be signed in to change notification settings - Fork 1
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.
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
feat: use workspace client [IDE-195] #24
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,7 +18,8 @@ | |
package http | ||
|
||
import ( | ||
"errors" | ||
"bytes" | ||
"io" | ||
"net/http" | ||
"time" | ||
|
||
|
@@ -78,19 +79,14 @@ func (s *httpClient) Do(req *http.Request) (response *http.Response, err error) | |
return nil, err // no retries for errors | ||
} | ||
|
||
err = s.checkResponseCode(response) | ||
if err != nil { | ||
if retryErrorCodes[response.StatusCode] { | ||
s.logger.Debug().Err(err).Str("method", req.Method).Int("attempts done", i+1).Msg("retrying") | ||
if i < retryCount-1 { | ||
time.Sleep(5 * time.Second) | ||
continue | ||
} | ||
// return the error on last try | ||
return nil, err | ||
if retryErrorCodes[response.StatusCode] { | ||
s.logger.Debug().Err(err).Str("method", req.Method).Int("attempts done", i+1).Msg("retrying") | ||
if i < retryCount-1 { | ||
time.Sleep(5 * time.Second) | ||
continue | ||
} | ||
return nil, err | ||
} | ||
|
||
// no error, we can break the retry loop | ||
break | ||
} | ||
|
@@ -99,7 +95,18 @@ func (s *httpClient) Do(req *http.Request) (response *http.Response, err error) | |
|
||
func (s *httpClient) httpCall(req *http.Request) (*http.Response, error) { | ||
log := s.logger.With().Str("method", "http.httpCall").Logger() | ||
|
||
// store the request body so that after retrying it can be read again | ||
var copyReqBody io.ReadCloser | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is needed so that after every attempt we still have the request body. This code was refactored in #22 and before that change the body was built in every retry. I introduced a regression there so I decided to add a test as well to make sure this continues to work. |
||
if req.Body != nil { | ||
buf, _ := io.ReadAll(req.Body) | ||
reqBody := io.NopCloser(bytes.NewBuffer(buf)) | ||
copyReqBody = io.NopCloser(bytes.NewBuffer(buf)) | ||
req.Body = reqBody | ||
} | ||
response, err := s.clientFactory().Do(req) | ||
req.Body = copyReqBody | ||
|
||
if err != nil { | ||
log.Error().Err(err).Msg("got http error") | ||
s.errorReporter.CaptureError(err, observability.ErrorReporterOptions{ErrorDiagnosticPath: req.RequestURI}) | ||
|
@@ -108,10 +115,3 @@ func (s *httpClient) httpCall(req *http.Request) (*http.Response, error) { | |
|
||
return response, nil | ||
} | ||
|
||
func (s *httpClient) checkResponseCode(r *http.Response) error { | ||
if r.StatusCode >= 200 && r.StatusCode <= 299 { | ||
return nil | ||
} | ||
return errors.New("Unexpected response code: " + r.Status) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,7 +16,10 @@ | |
package http_test | ||
|
||
import ( | ||
"fmt" | ||
"io" | ||
"net/http" | ||
"strings" | ||
"testing" | ||
|
||
"github.com/golang/mock/gomock" | ||
|
@@ -36,8 +39,17 @@ type dummyTransport struct { | |
calls int | ||
} | ||
|
||
func (d *dummyTransport) RoundTrip(_ *http.Request) (*http.Response, error) { | ||
func (d *dummyTransport) RoundTrip(req *http.Request) (*http.Response, error) { | ||
d.calls++ | ||
if req.Body != nil { | ||
body, err := io.ReadAll(req.Body) | ||
if err != nil { | ||
return nil, err | ||
} | ||
if string(body) == "" { | ||
return nil, fmt.Errorf("body is empty") | ||
} | ||
} | ||
return &http.Response{ | ||
StatusCode: d.responseCode, | ||
Status: d.status, | ||
|
@@ -64,8 +76,35 @@ func TestSnykCodeBackendService_DoCall_shouldRetry(t *testing.T) { | |
require.NoError(t, err) | ||
|
||
s := codeClientHTTP.NewHTTPClient(newLogger(t), dummyClientFactory, mockInstrumentor, mockErrorReporter) | ||
_, err = s.Do(req) | ||
assert.Error(t, err) | ||
res, err := s.Do(req) | ||
assert.NoError(t, err) | ||
assert.NotNil(t, res) | ||
assert.Equal(t, 3, d.calls) | ||
} | ||
|
||
func TestSnykCodeBackendService_DoCall_shouldRetryWithARequestBody(t *testing.T) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the test that verifies that the request body is still populated after a retry attempt. |
||
d := &dummyTransport{responseCode: 502, status: "502 Bad Gateway"} | ||
dummyClientFactory := func() *http.Client { | ||
return &http.Client{ | ||
Transport: d, | ||
} | ||
} | ||
|
||
ctrl := gomock.NewController(t) | ||
mockSpan := mocks.NewMockSpan(ctrl) | ||
mockSpan.EXPECT().GetTraceId().AnyTimes() | ||
mockInstrumentor := mocks.NewMockInstrumentor(ctrl) | ||
mockInstrumentor.EXPECT().StartSpan(gomock.Any(), gomock.Any()).Return(mockSpan).Times(1) | ||
mockInstrumentor.EXPECT().Finish(gomock.Any()).Times(1) | ||
mockErrorReporter := mocks.NewMockErrorReporter(ctrl) | ||
|
||
req, err := http.NewRequest(http.MethodGet, "https://httpstat.us/500", io.NopCloser(strings.NewReader("body"))) | ||
require.NoError(t, err) | ||
|
||
s := codeClientHTTP.NewHTTPClient(newLogger(t), dummyClientFactory, mockInstrumentor, mockErrorReporter) | ||
res, err := s.Do(req) | ||
assert.NoError(t, err) | ||
assert.NotNil(t, res) | ||
assert.Equal(t, 3, d.calls) | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of throwing away the response (which may have useful information for the customer) and wrapping the status code in an error, we're going to return the original response and leave it up to the user of the HTTP client (deepcode vs workspace, for now) to decide what kind of error to return.