From 036a050839e1dde083d35cc39f8ff4bc2e708c30 Mon Sep 17 00:00:00 2001 From: Chris Date: Fri, 12 Jan 2024 22:04:58 -0700 Subject: [PATCH 1/3] Test that retrying a POST request preserves the req.Body --- goshopify.go | 6 ++++- goshopify_test.go | 57 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 1 deletion(-) diff --git a/goshopify.go b/goshopify.go index 07626039..343a1b55 100644 --- a/goshopify.go +++ b/goshopify.go @@ -4,6 +4,7 @@ package goshopify import ( "bytes" "encoding/json" + "errors" "fmt" "io" "io/ioutil" @@ -439,7 +440,10 @@ func (c *Client) logBody(body *io.ReadCloser, format string) { if body == nil { return } - b, _ := ioutil.ReadAll(*body) + b, err := ioutil.ReadAll(*body) + if err != nil && !errors.Is(err, io.EOF) { + return + } if len(b) > 0 { c.log.Debugf(format, string(b)) } diff --git a/goshopify_test.go b/goshopify_test.go index 9c35d5fe..007ae175 100644 --- a/goshopify_test.go +++ b/goshopify_test.go @@ -350,6 +350,25 @@ func TestDo(t *testing.T) { } } +func TestDoErrBody(t *testing.T) { + shopUrl := "https://fooshop.myshopify.com" + httpmock.RegisterResponder("GET", shopUrl, httpmock.NewStringResponder(200, "")) + + req, err := http.NewRequest("GET", shopUrl, errReader{}) + if err != nil { + t.Error("error creating request: ", err) + } + + err = client.Do(req, nil) + if err == nil { + t.Errorf("Do(): expected error test-error, actual nil") + } + testErr := errors.New("test-error") + if !errors.As(err, &testErr) { + t.Errorf("Do(): expected ResponseDecodingError, actual %#v", err) + } +} + func TestRetry(t *testing.T) { setup() defer teardown() @@ -462,6 +481,44 @@ func TestRetry(t *testing.T) { } } +func TestRetryPost(t *testing.T) { + u := "foo/1" + responder := func(req *http.Request) (*http.Response, error) { + resp := httpmock.NewStringResponse(http.StatusTooManyRequests, `{"errors":"Exceeded 2 calls per second for api client. Reduce request rates to resume uninterrupted service."}`) + resp.Header.Add("Retry-After", "2.0") + body, err := ioutil.ReadAll(req.Body) + if err != nil { + return nil, err + } + if len(body) == 0 { + return nil, errors.New("empty body") + } + return resp, nil + } + expected := RateLimitError{ + RetryAfter: 2, + ResponseError: ResponseError{ + Status: 429, + Message: "Exceeded 2 calls per second for api client. Reduce request rates to resume uninterrupted service.", + }, + } + + testClient := NewClient(app, "fooshop", "abcd", WithRetry(2)) + httpmock.ActivateNonDefault(testClient.Client) + shopUrl := fmt.Sprintf("https://fooshop.myshopify.com/%v", u) + httpmock.RegisterResponder("POST", shopUrl, responder) + + testBody := []byte(`{"foo": "bar"}`) + req, err := testClient.NewRequest("POST", u, testBody, nil) + if err != nil { + t.Errorf("TestRetryPost(): errored %s", err) + } + err = testClient.Do(req, nil) + if !reflect.DeepEqual(err, expected) { + t.Errorf("Do(): expected error %#v, actual %#v", expected, err) + } +} + func TestClientDoAutoApiVersion(t *testing.T) { u := "foo/1" responder := func(req *http.Request) (*http.Response, error) { From 67009889a75369b1686d877538d3a4d807f159bb Mon Sep 17 00:00:00 2001 From: Chris Date: Fri, 5 Jan 2024 10:24:32 -0700 Subject: [PATCH 2/3] Don't re-use empty body on retry Retries on 429 (rate limit) were not working for me (except for GET requests). I would get these errors (retries result in a 400 Bad Request every time): [DEBUG] PUT: https://mydomain.myshopify.com/admin/api/2023-07/metafields/30029242335523.json [DEBUG] SENT: {"metafield":{"created_at":"2023-08-30T19:55:54-06:00","description":"The SKU of the product.","id":30029242335523,"key":"sku","namespace":"ns","owner_id":8557574947107,"own er_resource":"product","updated_at":"2023-08-30T19:55:54-06:00","value":"8bp-101009","type":"single_line_text_field","admin_graphql_api_id":"gid://shopify/Metafield/30029242335523"}} [DEBUG] Shopify X-Request-Id: 6676e531-ee1e-4efa-96b2-88682553f930 [DEBUG] RECV 429: 429 Too Many Requests [DEBUG] RESP: {"errors":"Exceeded 2 calls per second for api client. Reduce request rates to resume uninterrupted service."} [DEBUG] rate limited waiting 2s [DEBUG] Shopify X-Request-Id: [DEBUG] RECV 400: 400 Bad Request [DEBUG] RESP: 400 Bad Request

400 Bad Request

cloudflare
The problem is that on retry, the same request with an already-read Body was being sent. The solution that is working for me is to copy the Body into a []byte buffer before the first request, and then set the req.Body to that buffer before every request. --- goshopify.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/goshopify.go b/goshopify.go index 343a1b55..27cf3003 100644 --- a/goshopify.go +++ b/goshopify.go @@ -344,8 +344,19 @@ func (c *Client) doGetHeaders(req *http.Request, v interface{}) (http.Header, er c.attempts = 0 c.logRequest(req) + // copy request body so it can be re-used + var body []byte + if req.Body != nil { + body, err = ioutil.ReadAll(req.Body) + defer req.Body.Close() + if err != nil { + return nil, err + } + } + for { c.attempts++ + req.Body = ioutil.NopCloser(bytes.NewBuffer(body)) resp, err = c.Client.Do(req) c.logResponse(resp) if err != nil { From 339d4c91609d14cb1f0ca1a6b64bddc0ad7236cb Mon Sep 17 00:00:00 2001 From: Chris Date: Sat, 13 Jan 2024 21:02:08 -0700 Subject: [PATCH 3/3] Use package-level testErr to test for Read() errors --- goshopify_test.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/goshopify_test.go b/goshopify_test.go index 007ae175..b3bdd902 100644 --- a/goshopify_test.go +++ b/goshopify_test.go @@ -30,8 +30,10 @@ var ( // errReader can be used to simulate a failed call to response.Body.Read type errReader struct{} +var testErr = errors.New("test-error") + func (errReader) Read([]byte) (int, error) { - return 0, errors.New("test-error") + return 0, testErr } func (errReader) Close() error { @@ -363,8 +365,7 @@ func TestDoErrBody(t *testing.T) { if err == nil { t.Errorf("Do(): expected error test-error, actual nil") } - testErr := errors.New("test-error") - if !errors.As(err, &testErr) { + if !errors.Is(err, testErr) { t.Errorf("Do(): expected ResponseDecodingError, actual %#v", err) } } @@ -793,7 +794,7 @@ func TestCheckResponseError(t *testing.T) { }, { &http.Response{StatusCode: 400, Body: errReader{}}, - errors.New("test-error"), + testErr, }, { httpmock.NewStringResponse(422, `{"error": "Unprocessable Entity - ok"}`),