From b285c20f7ca3aef7ba7c7c1c5a0f5e3b837c2996 Mon Sep 17 00:00:00 2001 From: Ravi Shekhar Jethani Date: Mon, 3 Jul 2023 03:44:14 +0200 Subject: [PATCH] Add response data streaming facility Here we add a new ResponseDecoder: ByteStreamer. This allows user to get a stream of bytes 'as is' i.e. without any decoding. This is useful for all those streaming use cases like downloading files, binary blobs etc. This is also useful when response data is just plain text for e.g. the 5XX responses from API gateways. Side changes: - The xml stuff in testing code no longer needed as this new decoder is used as non-default decoder. - Code reformatting as per gopls/gofumpt style. --- response.go | 31 +++++++++- sling_test.go | 152 +++++++++++++++++++++++++++++--------------------- 2 files changed, 118 insertions(+), 65 deletions(-) diff --git a/response.go b/response.go index c918ed1..39483ae 100644 --- a/response.go +++ b/response.go @@ -2,6 +2,8 @@ package sling import ( "encoding/json" + "fmt" + "io" "net/http" ) @@ -12,11 +14,36 @@ type ResponseDecoder interface { } // jsonDecoder decodes http response JSON into a JSON-tagged struct value. -type jsonDecoder struct { -} +type jsonDecoder struct{} // Decode decodes the Response Body into the value pointed to by v. // Caller must provide a non-nil v and close the resp.Body. func (d jsonDecoder) Decode(resp *http.Response, v interface{}) error { return json.NewDecoder(resp.Body).Decode(v) } + +// ByteStreamer is a [sling.ResponseDecoder] which simply forwards response data 'as is' rather than trying to deocde +// it. This is useful when 1/ response is actually just plain text (like 5XX response from API gateways). 2/ response +// data is a byte stream representing some file or a binary blob. +// +// It leverages existing facilities of automatic discarding and closing of response body so the user does not need to +// care about it. +type ByteStreamer struct{} + +// Decode simply tries to copy response data into v assuming its an [io.Writer] instance. Assuming so little about v +// gives consumers a lot of choice about consuming response data. They can wait for all data to be dumped into some +// buffer then act on it or they can read as soon as data gets written. +func (d ByteStreamer) Decode(resp *http.Response, v any) error { + var w io.Writer + w, ok := v.(io.Writer) + if !ok { + return fmt.Errorf("expected type: %T; got: %T", w, v) + } + + _, err := io.Copy(w, resp.Body) + if err != nil { + return fmt.Errorf("failed copying response data to v: %w", err) + } + + return nil +} diff --git a/sling_test.go b/sling_test.go index 8eab528..c93b6d5 100644 --- a/sling_test.go +++ b/sling_test.go @@ -3,7 +3,6 @@ package sling import ( "bytes" "context" - "encoding/xml" "errors" "fmt" "io" @@ -40,13 +39,6 @@ type FakeModel struct { var modelA = FakeModel{Text: "note", FavoriteCount: 12} -// Non-Json response decoder -type xmlResponseDecoder struct{} - -func (d xmlResponseDecoder) Decode(resp *http.Response, v interface{}) error { - return xml.NewDecoder(resp.Body).Decode(v) -} - func TestNew(t *testing.T) { sling := New() if sling.httpClient != http.DefaultClient { @@ -64,14 +56,14 @@ func TestSlingNew(t *testing.T) { fakeBodyProvider := jsonBodyProvider{FakeModel{}} cases := []*Sling{ - &Sling{httpClient: &http.Client{}, method: "GET", rawURL: "http://example.com"}, - &Sling{httpClient: nil, method: "", rawURL: "http://example.com"}, - &Sling{queryStructs: make([]interface{}, 0)}, - &Sling{queryStructs: []interface{}{paramsA}}, - &Sling{queryStructs: []interface{}{paramsA, paramsB}}, - &Sling{bodyProvider: fakeBodyProvider}, - &Sling{bodyProvider: fakeBodyProvider}, - &Sling{bodyProvider: nil}, + {httpClient: &http.Client{}, method: "GET", rawURL: "http://example.com"}, + {httpClient: nil, method: "", rawURL: "http://example.com"}, + {queryStructs: make([]interface{}, 0)}, + {queryStructs: []interface{}{paramsA}}, + {queryStructs: []interface{}{paramsA, paramsB}}, + {bodyProvider: fakeBodyProvider}, + {bodyProvider: fakeBodyProvider}, + {bodyProvider: nil}, New().Add("Content-Type", "application/json"), New().Add("A", "B").Add("a", "c").New(), New().Add("A", "B").New().Add("a", "c"), @@ -222,14 +214,14 @@ func TestAddHeader(t *testing.T) { sling *Sling expectedHeader map[string][]string }{ - {New().Add("authorization", "OAuth key=\"value\""), map[string][]string{"Authorization": []string{"OAuth key=\"value\""}}}, + {New().Add("authorization", "OAuth key=\"value\""), map[string][]string{"Authorization": {"OAuth key=\"value\""}}}, // header keys should be canonicalized - {New().Add("content-tYPE", "application/json").Add("User-AGENT", "sling"), map[string][]string{"Content-Type": []string{"application/json"}, "User-Agent": []string{"sling"}}}, + {New().Add("content-tYPE", "application/json").Add("User-AGENT", "sling"), map[string][]string{"Content-Type": {"application/json"}, "User-Agent": {"sling"}}}, // values for existing keys should be appended - {New().Add("A", "B").Add("a", "c"), map[string][]string{"A": []string{"B", "c"}}}, + {New().Add("A", "B").Add("a", "c"), map[string][]string{"A": {"B", "c"}}}, // Add should add to values for keys added by parent Slings - {New().Add("A", "B").Add("a", "c").New(), map[string][]string{"A": []string{"B", "c"}}}, - {New().Add("A", "B").New().Add("a", "c"), map[string][]string{"A": []string{"B", "c"}}}, + {New().Add("A", "B").Add("a", "c").New(), map[string][]string{"A": {"B", "c"}}}, + {New().Add("A", "B").New().Add("a", "c"), map[string][]string{"A": {"B", "c"}}}, } for _, c := range cases { // type conversion from header to alias'd map for deep equality comparison @@ -246,11 +238,11 @@ func TestSetHeader(t *testing.T) { expectedHeader map[string][]string }{ // should replace existing values associated with key - {New().Add("A", "B").Set("a", "c"), map[string][]string{"A": []string{"c"}}}, - {New().Set("content-type", "A").Set("Content-Type", "B"), map[string][]string{"Content-Type": []string{"B"}}}, + {New().Add("A", "B").Set("a", "c"), map[string][]string{"A": {"c"}}}, + {New().Set("content-type", "A").Set("Content-Type", "B"), map[string][]string{"Content-Type": {"B"}}}, // Set should replace values received by copying parent Slings - {New().Set("A", "B").Add("a", "c").New(), map[string][]string{"A": []string{"B", "c"}}}, - {New().Add("A", "B").New().Set("a", "c"), map[string][]string{"A": []string{"c"}}}, + {New().Set("A", "B").Add("a", "c").New(), map[string][]string{"A": {"B", "c"}}}, + {New().Add("A", "B").New().Set("a", "c"), map[string][]string{"A": {"c"}}}, } for _, c := range cases { // type conversion from Header to alias'd map for deep equality comparison @@ -555,20 +547,20 @@ func TestRequest_headers(t *testing.T) { sling *Sling expectedHeader map[string][]string }{ - {New().Add("authorization", "OAuth key=\"value\""), map[string][]string{"Authorization": []string{"OAuth key=\"value\""}}}, + {New().Add("authorization", "OAuth key=\"value\""), map[string][]string{"Authorization": {"OAuth key=\"value\""}}}, // header keys should be canonicalized - {New().Add("content-tYPE", "application/json").Add("User-AGENT", "sling"), map[string][]string{"Content-Type": []string{"application/json"}, "User-Agent": []string{"sling"}}}, + {New().Add("content-tYPE", "application/json").Add("User-AGENT", "sling"), map[string][]string{"Content-Type": {"application/json"}, "User-Agent": {"sling"}}}, // values for existing keys should be appended - {New().Add("A", "B").Add("a", "c"), map[string][]string{"A": []string{"B", "c"}}}, + {New().Add("A", "B").Add("a", "c"), map[string][]string{"A": {"B", "c"}}}, // Add should add to values for keys added by parent Slings - {New().Add("A", "B").Add("a", "c").New(), map[string][]string{"A": []string{"B", "c"}}}, - {New().Add("A", "B").New().Add("a", "c"), map[string][]string{"A": []string{"B", "c"}}}, + {New().Add("A", "B").Add("a", "c").New(), map[string][]string{"A": {"B", "c"}}}, + {New().Add("A", "B").New().Add("a", "c"), map[string][]string{"A": {"B", "c"}}}, // Add and Set - {New().Add("A", "B").Set("a", "c"), map[string][]string{"A": []string{"c"}}}, - {New().Set("content-type", "A").Set("Content-Type", "B"), map[string][]string{"Content-Type": []string{"B"}}}, + {New().Add("A", "B").Set("a", "c"), map[string][]string{"A": {"c"}}}, + {New().Set("content-type", "A").Set("Content-Type", "B"), map[string][]string{"Content-Type": {"B"}}}, // Set should replace values received by copying parent Slings - {New().Set("A", "B").Add("a", "c").New(), map[string][]string{"A": []string{"B", "c"}}}, - {New().Add("A", "B").New().Set("a", "c"), map[string][]string{"A": []string{"c"}}}, + {New().Set("A", "B").Add("a", "c").New(), map[string][]string{"A": {"B", "c"}}}, + {New().Add("A", "B").New().Set("a", "c"), map[string][]string{"A": {"c"}}}, } for _, c := range cases { req, _ := c.sling.Request() @@ -640,7 +632,6 @@ func TestDo_onSuccess(t *testing.T) { model := new(FakeModel) apiError := new(APIError) resp, err := sling.Do(req, model, apiError) - if err != nil { t.Errorf("expected nil, got %v", err) } @@ -668,7 +659,6 @@ func TestDo_onSuccessWithNilValue(t *testing.T) { apiError := new(APIError) resp, err := sling.Do(req, nil, apiError) - if err != nil { t.Errorf("expected nil, got %v", err) } @@ -694,7 +684,6 @@ func TestDo_noContent(t *testing.T) { model := new(FakeModel) apiError := new(APIError) resp, err := sling.Do(req, model, apiError) - if err != nil { t.Errorf("expected nil, got %v", err) } @@ -729,7 +718,6 @@ func TestDo_onFailure(t *testing.T) { model := new(FakeModel) apiError := new(APIError) resp, err := sling.Do(req, model, apiError) - if err != nil { t.Errorf("expected nil, got %v", err) } @@ -758,7 +746,6 @@ func TestDo_onFailureWithNilValue(t *testing.T) { model := new(FakeModel) resp, err := sling.Do(req, model, nil) - if err != nil { t.Errorf("expected nil, got %v", err) } @@ -771,39 +758,70 @@ func TestDo_onFailureWithNilValue(t *testing.T) { } } +func TestByteSreamer_failed(t *testing.T) { + tests := map[string]struct { + resp *http.Response + v any + }{ + "v is not io.Writer": { + &http.Response{ + Body: io.NopCloser(strings.NewReader("some response text")), + ContentLength: -1, + }, + &struct{}{}, + }, + "response body ready error": { + &http.Response{ + Body: io.NopCloser(&mockReader{ + ReadFn: func(b []byte) (int, error) { return 0, fmt.Errorf("some io error") }, + }), + ContentLength: -1, + }, + &bytes.Buffer{}, + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + err := ByteStreamer{}.Decode(test.resp, test.v) + if err == nil { + t.Errorf("expected non nil error") + } + }) + } +} + func TestReceive_success_nonDefaultDecoder(t *testing.T) { client, mux, server := testServer() defer server.Close() - mux.HandleFunc("/foo/submit", func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/xml") - data := ` - Some text - 24 - 10.5 - ` - fmt.Fprintf(w, xml.Header) - fmt.Fprint(w, data) + + expectedData := []byte{11, 22, 33, 44, 55} + + mux.HandleFunc("/foo/file", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/octet-stream") + w.Write(expectedData) }) - endpoint := New().Client(client).Base("http://example.com/").Path("foo/").Post("submit") + endpoint := New().Client(client).Base("http://example.com/").Path("foo/").Get("file") - model := new(FakeModel) - apiError := new(APIError) - resp, err := endpoint.New().ResponseDecoder(xmlResponseDecoder{}).Receive(model, apiError) + success := &bytes.Buffer{} + failure := &bytes.Buffer{} + resp, err := endpoint.New().ResponseDecoder(ByteStreamer{}).Receive(success, failure) if err != nil { t.Errorf("expected nil, got %v", err) } + if resp.StatusCode != 200 { - t.Errorf("expected %d, got %d", 200, resp.StatusCode) + t.Errorf("expected statu code: %d, got: %d", 200, resp.StatusCode) } - expectedModel := &FakeModel{Text: "Some text", FavoriteCount: 24, Temperature: 10.5} - if !reflect.DeepEqual(expectedModel, model) { - t.Errorf("expected %v, got %v", expectedModel, model) + + if failure.Len() != 0 { + t.Errorf("expected failure data to be empry; got: %v", failure.Bytes()) } - expectedAPIError := &APIError{} - if !reflect.DeepEqual(expectedAPIError, apiError) { - t.Errorf("failureV should be zero valued, exepcted %v, got %v", expectedAPIError, apiError) + + if !reflect.DeepEqual(expectedData, success.Bytes()) { + t.Errorf("expected response data: %v; got: %v", expectedData, success.Bytes()) } } @@ -824,7 +842,6 @@ func TestReceive_success(t *testing.T) { model := new(FakeModel) apiError := new(APIError) resp, err := endpoint.New().QueryStruct(params).BodyForm(params).Receive(model, apiError) - if err != nil { t.Errorf("expected nil, got %v", err) } @@ -856,7 +873,6 @@ func TestReceive_StatusOKNoContent(t *testing.T) { model := new(FakeModel) apiError := new(APIError) resp, err := endpoint.New().BodyForm(params).Receive(model, apiError) - if err != nil { t.Errorf("expected nil, got %v", err) } @@ -891,7 +907,6 @@ func TestReceive_failure(t *testing.T) { model := new(FakeModel) apiError := new(APIError) resp, err := endpoint.New().QueryStruct(params).BodyForm(params).Receive(model, apiError) - if err != nil { t.Errorf("expected nil, got %v", err) } @@ -918,7 +933,6 @@ func TestReceive_noContent(t *testing.T) { endpoint := New().Client(client).Base("http://example.com/").Path("foo/").Head("submit") resp, err := endpoint.New().Receive(nil, nil) - if err != nil { t.Errorf("expected nil, got %v", err) } @@ -1044,3 +1058,15 @@ func assertPostForm(t *testing.T, expected map[string]string, req *http.Request) t.Errorf("expected parameters %v, got %v", expected, req.PostForm) } } + +// mockReader implements [io.Reader] to create custom IO readers behavior in tests. +type mockReader struct { + ReadFn func([]byte) (int, error) +} + +func (mr *mockReader) Read(p []byte) (n int, err error) { + if mr.ReadFn != nil { + return mr.ReadFn(p) + } + return 0, nil +}