From ce59396671e0e71242e6593cb7de45e1f96e0dda Mon Sep 17 00:00:00 2001 From: Renaud Hartert Date: Tue, 16 Jul 2024 13:56:07 +0200 Subject: [PATCH 1/5] Avoid loading the body twice in memory --- httpclient/response.go | 54 ++++++++++------- httpclient/response_test.go | 113 ++++++++++++++++++++++++++++++++---- 2 files changed, 135 insertions(+), 32 deletions(-) diff --git a/httpclient/response.go b/httpclient/response.go index c1aa29e99..577c7fb53 100644 --- a/httpclient/response.go +++ b/httpclient/response.go @@ -25,6 +25,13 @@ func WithResponseHeader(key string, value *string) DoOption { } } +// WithResponseUnmarshal unmarshals the response body into response. The +// supported response types are the following: +// - *bytes.Buffer, +// - *io.ReadCloser, +// - *[]byte, +// - a pointer to a struct with a Contents io.ReadCloser field, +// - a pointer to a struct representing a JSON object. func WithResponseUnmarshal(response any) DoOption { return DoOption{ in: func(r *http.Request) error { @@ -50,45 +57,50 @@ func WithResponseUnmarshal(response any) DoOption { if err != nil { return err } - // If the field contains a "Content" field of type bytes.Buffer, write the body over there and return. - if field, ok := findContentsField(response, body); ok { - // If so, set the value + + // If the response is a struct with a field "Contents" of type + // io.ReadCloser, set that field to the response body ReadCloser. + if field, ok := findContentsField(response); ok { field.Set(reflect.ValueOf(body.ReadCloser)) return nil } - // If the destination is bytes.Buffer, write the body over there - if raw, ok := response.(*io.ReadCloser); ok { - *raw = body.ReadCloser + // If the response is a ReadCloser, return it directly without + // reading the body. + if rc, ok := response.(*io.ReadCloser); ok { + *rc = body.ReadCloser return nil } + + // If the response is a bytes.Buffer, read the entire body directly + // into it. + if buffer, ok := response.(*bytes.Buffer); ok { + defer body.ReadCloser.Close() + _, err := buffer.ReadFrom(body.ReadCloser) + return err + } + + // At this point, fully read the content of the body and use it + // to populate the response object (whether it is a slice of bytes + // or a JSON object). defer body.ReadCloser.Close() - bs, err := io.ReadAll(body.ReadCloser) + bodyBytes, err := io.ReadAll(body.ReadCloser) if err != nil { return fmt.Errorf("failed to read response body: %w", err) } - if len(bs) == 0 { + if bs, ok := response.(*[]byte); ok { + *bs = bodyBytes return nil } - // If the destination is a byte slice or buffer, pass the body verbatim. - if raw, ok := response.(*[]byte); ok { - *raw = bs - return nil - } - if raw, ok := response.(*bytes.Buffer); ok { - _, err := raw.Write(bs) - return err - } - err = json.Unmarshal(bs, &response) - if err != nil { - return apierr.MakeUnexpectedError(body.Response, err, body.RequestBody.DebugBytes, bs) + if err = json.Unmarshal(bodyBytes, &response); err != nil { + return apierr.MakeUnexpectedError(body.Response, err, body.RequestBody.DebugBytes, bodyBytes) } return nil }, } } -func findContentsField(response any, body *common.ResponseWrapper) (*reflect.Value, bool) { +func findContentsField(response any) (*reflect.Value, bool) { value := reflect.ValueOf(response) value = reflect.Indirect(value) if value.Kind() != reflect.Struct { diff --git a/httpclient/response_test.go b/httpclient/response_test.go index e4aab5798..3bbc3e0ac 100644 --- a/httpclient/response_test.go +++ b/httpclient/response_test.go @@ -1,29 +1,120 @@ package httpclient import ( + "bytes" "context" "io" "net/http" "strings" "testing" + "github.com/google/go-cmp/cmp" "github.com/stretchr/testify/require" ) -func TestSimpleRequestRawResponse(t *testing.T) { - c := NewApiClient(ClientConfig{ +func make200Response(body string) *http.Response { + return &http.Response{ + StatusCode: 200, + Body: io.NopCloser(strings.NewReader(body)), + } +} + +func mockClient(resp *http.Response) *ApiClient { + return NewApiClient(ClientConfig{ Transport: hc(func(r *http.Request) (*http.Response, error) { - return &http.Response{ - StatusCode: 200, - Body: io.NopCloser(strings.NewReader("Hello, world!")), - Request: r, - }, nil + resp.Request = r + return resp, nil }), }) - var raw []byte - err := c.Do(context.Background(), "GET", "/a", WithResponseUnmarshal(&raw)) - require.NoError(t, err) - require.Equal(t, "Hello, world!", string(raw)) +} + +func TestWithResponseUnmarshal_structWithContent(t *testing.T) { + type structWithContents = struct { + Contents io.ReadCloser + } + want := structWithContents{ + Contents: io.NopCloser(strings.NewReader("foo bar")), + } + + var got structWithContents + c := mockClient(make200Response("foo bar")) + gotErr := c.Do(context.Background(), "GET", "/a", WithResponseUnmarshal(&got)) + + if gotErr != nil { + t.Errorf("WithResponseUnmarshal(): want no error, got: %s", gotErr) + } + + wantBytes, _ := io.ReadAll(want.Contents) + gotBytes, _ := io.ReadAll(got.Contents) + if diff := cmp.Diff(wantBytes, gotBytes); diff != "" { + t.Errorf("WithResponseUnmarshal(): want != got: (-want +got):\n%s", diff) + } +} + +func TestWithResponseUnmarshal_readCloser(t *testing.T) { + want := io.NopCloser(strings.NewReader("foo bar")) + + var got io.ReadCloser + c := mockClient(make200Response("foo bar")) + gotErr := c.Do(context.Background(), "GET", "/a", WithResponseUnmarshal(&got)) + + if gotErr != nil { + t.Errorf("WithResponseUnmarshal(): want no error, got: %s", gotErr) + } + + wantBytes, _ := io.ReadAll(want) + gotBytes, _ := io.ReadAll(got) + if diff := cmp.Diff(wantBytes, gotBytes); diff != "" { + t.Errorf("WithResponseUnmarshal(): want != got: (-want +got):\n%s", diff) + } +} + +func TestWithResponseUnmarshal_byteBuffer(t *testing.T) { + want := bytes.NewBuffer([]byte("foo bar")) + + var got bytes.Buffer + c := mockClient(make200Response("foo bar")) + gotErr := c.Do(context.Background(), "GET", "/a", WithResponseUnmarshal(&got)) + + if gotErr != nil { + t.Errorf("WithResponseUnmarshal(): want no error, got: %s", gotErr) + } + if diff := cmp.Diff(want.Bytes(), got.Bytes()); diff != "" { + t.Errorf("WithResponseUnmarshal(): want != got: (-want +got):\n%s", diff) + } +} + +func TestWithResponseUnmarshal_bytes(t *testing.T) { + want := []byte("foo bar") + + var got []byte + c := mockClient(make200Response("foo bar")) + gotErr := c.Do(context.Background(), "GET", "/a", WithResponseUnmarshal(&got)) + + if gotErr != nil { + t.Errorf("WithResponseUnmarshal(): want no error, got: %s", gotErr) + } + if diff := cmp.Diff(want, got); diff != "" { + t.Errorf("WithResponseUnmarshal(): want != got: (-want +got):\n%s", diff) + } +} + +func TestWithResponseUnmarshal_json(t *testing.T) { + type jsonStruct struct { + Foo string `json:"foo"` + } + want := jsonStruct{Foo: "bar"} + + var got jsonStruct + c := mockClient(make200Response(`{"foo": "bar"}`)) + gotErr := c.Do(context.Background(), "GET", "/a", WithResponseUnmarshal(&got)) + + if gotErr != nil { + t.Errorf("WithResponseUnmarshal(): want no error, got: %s", gotErr) + } + if diff := cmp.Diff(want, got); diff != "" { + t.Errorf("WithResponseUnmarshal(): want != got: (-want +got):\n%s", diff) + } } func TestWithResponseHeader(t *testing.T) { From 9990e66be3188a9b305b0521de6ae8f1f01f3756 Mon Sep 17 00:00:00 2001 From: Renaud Hartert Date: Tue, 16 Jul 2024 14:14:33 +0200 Subject: [PATCH 2/5] Rewording of comments --- httpclient/response.go | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/httpclient/response.go b/httpclient/response.go index 577c7fb53..8634a7758 100644 --- a/httpclient/response.go +++ b/httpclient/response.go @@ -32,6 +32,10 @@ func WithResponseHeader(key string, value *string) DoOption { // - *[]byte, // - a pointer to a struct with a Contents io.ReadCloser field, // - a pointer to a struct representing a JSON object. +// +// If response is a pointer to a io.ReadCloser or a struct with a io.ReadCloser +// field name "Contents", then the response io.ReadCloser is set to the value of +// the body's reader without actually reading it. func WithResponseUnmarshal(response any) DoOption { return DoOption{ in: func(r *http.Request) error { @@ -58,22 +62,14 @@ func WithResponseUnmarshal(response any) DoOption { return err } - // If the response is a struct with a field "Contents" of type - // io.ReadCloser, set that field to the response body ReadCloser. if field, ok := findContentsField(response); ok { field.Set(reflect.ValueOf(body.ReadCloser)) return nil } - - // If the response is a ReadCloser, return it directly without - // reading the body. - if rc, ok := response.(*io.ReadCloser); ok { - *rc = body.ReadCloser + if reader, ok := response.(*io.ReadCloser); ok { + *reader = body.ReadCloser return nil } - - // If the response is a bytes.Buffer, read the entire body directly - // into it. if buffer, ok := response.(*bytes.Buffer); ok { defer body.ReadCloser.Close() _, err := buffer.ReadFrom(body.ReadCloser) From ec4c38825edac599e70f99c26f9236bd0a6a8ebe Mon Sep 17 00:00:00 2001 From: Renaud Hartert Date: Tue, 16 Jul 2024 14:27:56 +0200 Subject: [PATCH 3/5] Fix json unmarshalling if empty response --- httpclient/response.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/httpclient/response.go b/httpclient/response.go index 8634a7758..4ece40e84 100644 --- a/httpclient/response.go +++ b/httpclient/response.go @@ -84,6 +84,9 @@ func WithResponseUnmarshal(response any) DoOption { if err != nil { return fmt.Errorf("failed to read response body: %w", err) } + if len(bodyBytes) == 0 { + return nil + } if bs, ok := response.(*[]byte); ok { *bs = bodyBytes return nil From 4d680121602a224de799cd782e02a4b4fba6d547 Mon Sep 17 00:00:00 2001 From: Renaud Hartert Date: Tue, 16 Jul 2024 20:43:15 +0200 Subject: [PATCH 4/5] Use testify in tests --- httpclient/response_test.go | 53 +++++++++++-------------------------- 1 file changed, 15 insertions(+), 38 deletions(-) diff --git a/httpclient/response_test.go b/httpclient/response_test.go index 3bbc3e0ac..cc29add13 100644 --- a/httpclient/response_test.go +++ b/httpclient/response_test.go @@ -8,7 +8,6 @@ import ( "strings" "testing" - "github.com/google/go-cmp/cmp" "github.com/stretchr/testify/require" ) @@ -29,6 +28,7 @@ func mockClient(resp *http.Response) *ApiClient { } func TestWithResponseUnmarshal_structWithContent(t *testing.T) { + c := mockClient(make200Response("foo bar")) type structWithContents = struct { Contents io.ReadCloser } @@ -37,84 +37,61 @@ func TestWithResponseUnmarshal_structWithContent(t *testing.T) { } var got structWithContents - c := mockClient(make200Response("foo bar")) gotErr := c.Do(context.Background(), "GET", "/a", WithResponseUnmarshal(&got)) - if gotErr != nil { - t.Errorf("WithResponseUnmarshal(): want no error, got: %s", gotErr) - } - + require.NoError(t, gotErr) wantBytes, _ := io.ReadAll(want.Contents) gotBytes, _ := io.ReadAll(got.Contents) - if diff := cmp.Diff(wantBytes, gotBytes); diff != "" { - t.Errorf("WithResponseUnmarshal(): want != got: (-want +got):\n%s", diff) - } + require.Equal(t, wantBytes, gotBytes) } func TestWithResponseUnmarshal_readCloser(t *testing.T) { + c := mockClient(make200Response("foo bar")) want := io.NopCloser(strings.NewReader("foo bar")) var got io.ReadCloser - c := mockClient(make200Response("foo bar")) gotErr := c.Do(context.Background(), "GET", "/a", WithResponseUnmarshal(&got)) - if gotErr != nil { - t.Errorf("WithResponseUnmarshal(): want no error, got: %s", gotErr) - } - + require.NoError(t, gotErr) wantBytes, _ := io.ReadAll(want) gotBytes, _ := io.ReadAll(got) - if diff := cmp.Diff(wantBytes, gotBytes); diff != "" { - t.Errorf("WithResponseUnmarshal(): want != got: (-want +got):\n%s", diff) - } + require.Equal(t, wantBytes, gotBytes) } func TestWithResponseUnmarshal_byteBuffer(t *testing.T) { + c := mockClient(make200Response("foo bar")) want := bytes.NewBuffer([]byte("foo bar")) var got bytes.Buffer - c := mockClient(make200Response("foo bar")) gotErr := c.Do(context.Background(), "GET", "/a", WithResponseUnmarshal(&got)) - if gotErr != nil { - t.Errorf("WithResponseUnmarshal(): want no error, got: %s", gotErr) - } - if diff := cmp.Diff(want.Bytes(), got.Bytes()); diff != "" { - t.Errorf("WithResponseUnmarshal(): want != got: (-want +got):\n%s", diff) - } + require.NoError(t, gotErr) + require.Equal(t, want.Bytes(), got.Bytes()) } func TestWithResponseUnmarshal_bytes(t *testing.T) { + c := mockClient(make200Response("foo bar")) want := []byte("foo bar") var got []byte - c := mockClient(make200Response("foo bar")) gotErr := c.Do(context.Background(), "GET", "/a", WithResponseUnmarshal(&got)) - if gotErr != nil { - t.Errorf("WithResponseUnmarshal(): want no error, got: %s", gotErr) - } - if diff := cmp.Diff(want, got); diff != "" { - t.Errorf("WithResponseUnmarshal(): want != got: (-want +got):\n%s", diff) - } + require.NoError(t, gotErr) + require.Equal(t, want, got) } func TestWithResponseUnmarshal_json(t *testing.T) { + c := mockClient(make200Response(`{"foo": "bar"}`)) type jsonStruct struct { Foo string `json:"foo"` } want := jsonStruct{Foo: "bar"} var got jsonStruct - c := mockClient(make200Response(`{"foo": "bar"}`)) gotErr := c.Do(context.Background(), "GET", "/a", WithResponseUnmarshal(&got)) - if gotErr != nil { - t.Errorf("WithResponseUnmarshal(): want no error, got: %s", gotErr) - } - if diff := cmp.Diff(want, got); diff != "" { - t.Errorf("WithResponseUnmarshal(): want != got: (-want +got):\n%s", diff) - } + require.NoError(t, gotErr) + require.Equal(t, want, got) } func TestWithResponseHeader(t *testing.T) { From 3d76a5b7e2febbfcc25b8c393a5063a4477acebb Mon Sep 17 00:00:00 2001 From: Renaud Hartert Date: Tue, 16 Jul 2024 21:12:46 +0200 Subject: [PATCH 5/5] Use assert instead of require --- httpclient/response_test.go | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/httpclient/response_test.go b/httpclient/response_test.go index cc29add13..ef36d2f32 100644 --- a/httpclient/response_test.go +++ b/httpclient/response_test.go @@ -8,7 +8,7 @@ import ( "strings" "testing" - "github.com/stretchr/testify/require" + "github.com/stretchr/testify/assert" ) func make200Response(body string) *http.Response { @@ -39,10 +39,10 @@ func TestWithResponseUnmarshal_structWithContent(t *testing.T) { var got structWithContents gotErr := c.Do(context.Background(), "GET", "/a", WithResponseUnmarshal(&got)) - require.NoError(t, gotErr) + assert.NoError(t, gotErr) wantBytes, _ := io.ReadAll(want.Contents) gotBytes, _ := io.ReadAll(got.Contents) - require.Equal(t, wantBytes, gotBytes) + assert.Equal(t, wantBytes, gotBytes) } func TestWithResponseUnmarshal_readCloser(t *testing.T) { @@ -52,10 +52,10 @@ func TestWithResponseUnmarshal_readCloser(t *testing.T) { var got io.ReadCloser gotErr := c.Do(context.Background(), "GET", "/a", WithResponseUnmarshal(&got)) - require.NoError(t, gotErr) + assert.NoError(t, gotErr) wantBytes, _ := io.ReadAll(want) gotBytes, _ := io.ReadAll(got) - require.Equal(t, wantBytes, gotBytes) + assert.Equal(t, wantBytes, gotBytes) } func TestWithResponseUnmarshal_byteBuffer(t *testing.T) { @@ -65,8 +65,8 @@ func TestWithResponseUnmarshal_byteBuffer(t *testing.T) { var got bytes.Buffer gotErr := c.Do(context.Background(), "GET", "/a", WithResponseUnmarshal(&got)) - require.NoError(t, gotErr) - require.Equal(t, want.Bytes(), got.Bytes()) + assert.NoError(t, gotErr) + assert.Equal(t, want.Bytes(), got.Bytes()) } func TestWithResponseUnmarshal_bytes(t *testing.T) { @@ -76,8 +76,8 @@ func TestWithResponseUnmarshal_bytes(t *testing.T) { var got []byte gotErr := c.Do(context.Background(), "GET", "/a", WithResponseUnmarshal(&got)) - require.NoError(t, gotErr) - require.Equal(t, want, got) + assert.NoError(t, gotErr) + assert.Equal(t, want, got) } func TestWithResponseUnmarshal_json(t *testing.T) { @@ -90,8 +90,8 @@ func TestWithResponseUnmarshal_json(t *testing.T) { var got jsonStruct gotErr := c.Do(context.Background(), "GET", "/a", WithResponseUnmarshal(&got)) - require.NoError(t, gotErr) - require.Equal(t, want, got) + assert.NoError(t, gotErr) + assert.Equal(t, want, got) } func TestWithResponseHeader(t *testing.T) { @@ -108,10 +108,9 @@ func TestWithResponseHeader(t *testing.T) { }), }) - var out string - ctx := context.Background() - err := client.Do(ctx, "GET", "abc", - WithResponseHeader("Foo", &out)) - require.NoError(t, err) - require.Equal(t, "some", out) + var got string + gotErr := client.Do(context.Background(), "GET", "abc", WithResponseHeader("Foo", &got)) + + assert.NoError(t, gotErr) + assert.Equal(t, "some", got) }