From 812244d79fd05da59864908400ae65671f7d0a17 Mon Sep 17 00:00:00 2001 From: cthuang Date: Fri, 2 Oct 2020 14:40:23 +0100 Subject: [PATCH] TUN-3443: Decode as v4api response on non-200 status --- tunnelstore/client.go | 49 +++++++++++++++++++++++++------------- tunnelstore/client_test.go | 12 +++++----- 2 files changed, 38 insertions(+), 23 deletions(-) diff --git a/tunnelstore/client.go b/tunnelstore/client.go index 12582002..5ea2728b 100644 --- a/tunnelstore/client.go +++ b/tunnelstore/client.go @@ -376,16 +376,12 @@ func (r *RESTClient) sendRequest(method string, url url.URL, body interface{}) ( func parseResponse(reader io.Reader, data interface{}) error { // Schema for Tunnelstore responses in the v1 API. // Roughly, it's a wrapper around a particular result that adds failures/errors/etc - var result struct { - Result json.RawMessage `json:"result"` - Success bool `json:"success"` - Errors []string `json:"errors"` - } + var result response // First, parse the wrapper and check the API call succeeded if err := json.NewDecoder(reader).Decode(&result); err != nil { return errors.Wrap(err, "failed to decode response") } - if err := checkErrors(result.Errors); err != nil { + if err := result.checkErrors(); err != nil { return err } if !result.Success { @@ -402,24 +398,43 @@ func unmarshalTunnel(reader io.Reader) (*Tunnel, error) { return &tunnel, err } -func checkErrors(errs []string) error { - if len(errs) == 0 { +type response struct { + Success bool `json:"success,omitempty"` + Errors []apiErr `json:"errors,omitempty"` + Messages []string `json:"messages,omitempty"` + Result json.RawMessage `json:"result,omitempty"` +} + +func (r *response) checkErrors() error { + if len(r.Errors) == 0 { return nil } - if len(errs) == 1 { - return fmt.Errorf("API error: %s", errs[0]) + if len(r.Errors) == 1 { + return r.Errors[0] } - allErrs := strings.Join(errs, "; ") - return fmt.Errorf("API errors: %s", allErrs) + var messages string + for _, e := range r.Errors { + messages += fmt.Sprintf("%s; ", e) + } + return fmt.Errorf("API errors: %s", messages) +} + +type apiErr struct { + Code json.Number `json:"code,omitempty"` + Message string `json:"message,omitempty"` +} + +func (e apiErr) Error() string { + return fmt.Sprintf("code: %v, reason: %s", e.Code, e.Message) } func (r *RESTClient) statusCodeToError(op string, resp *http.Response) error { if resp.Header.Get("Content-Type") == "application/json" { - var errorsResp struct { - Error string `json:"error"` - } - if json.NewDecoder(resp.Body).Decode(&errorsResp) == nil && errorsResp.Error != "" { - return errors.Errorf("Failed to %s: %s", op, errorsResp.Error) + var errorsResp response + if json.NewDecoder(resp.Body).Decode(&errorsResp) == nil { + if err := errorsResp.checkErrors(); err != nil { + return errors.Errorf("Failed to %s: %s", op, err) + } } } diff --git a/tunnelstore/client_test.go b/tunnelstore/client_test.go index 708bc47c..60a14cb0 100644 --- a/tunnelstore/client_test.go +++ b/tunnelstore/client_test.go @@ -30,8 +30,8 @@ func TestDNSRouteUnmarshalResult(t *testing.T) { badJSON := []string{ `abc`, `{"success": false, "result": {"cname": "new"}}`, - `{"errors": ["foo"], "result": {"cname": "new"}}`, - `{"errors": ["foo", "bar"], "result": {"cname": "new"}}`, + `{"errors": [{"code": 1003, "message":"An A, AAAA or CNAME record already exists with that host"}], "result": {"cname": "new"}}`, + `{"errors": [{"code": 1003, "message":"An A, AAAA or CNAME record already exists with that host"}, {"code": 1004, "message":"Cannot use tunnel as origin for non-proxied load balancer"}], "result": {"cname": "new"}}`, `{"result": {"cname": "new"}}`, `{"result": {"cname": "new"}}`, } @@ -60,8 +60,8 @@ func TestLBRouteUnmarshalResult(t *testing.T) { badJSON := []string{ `abc`, `{"success": false, "result": {"pool": "unchanged", "load_balancer": "updated"}}`, - `{"errors": ["foo"], "result": {"pool": "unchanged", "load_balancer": "updated"}}`, - `{"errors": ["foo", "bar"], "result": {"pool": "unchanged", "load_balancer": "updated"}}`, + `{"errors": [{"code": 1003, "message":"An A, AAAA or CNAME record already exists with that host"}], "result": {"pool": "unchanged", "load_balancer": "updated"}}`, + `{"errors": [{"code": 1003, "message":"An A, AAAA or CNAME record already exists with that host"}, {"code": 1004, "message":"Cannot use tunnel as origin for non-proxied load balancer"}], "result": {"pool": "unchanged", "load_balancer": "updated"}}`, `{"result": {"pool": "unchanged", "load_balancer": "updated"}}`, } @@ -127,7 +127,7 @@ func Test_parseListTunnels(t *testing.T) { }, { name: "errors are present", - args: args{body: `{"errors": ["foo"], "result": []}`}, + args: args{body: `{"errors": [{"code": 1003, "message":"An A, AAAA or CNAME record already exists with that host"}], "result": []}`}, wantErr: true, }, { @@ -197,7 +197,7 @@ func TestUnmarshalTunnelErr(t *testing.T) { `abc`, `{"success": true, "result": abc}`, `{"success": false, "result": {"id": "00000000-0000-0000-0000-000000000000","name":"test","created_at":"0001-01-01T00:00:00Z","connections":[]}}}`, - `{"errors": ["foo"], "result": {"id": "00000000-0000-0000-0000-000000000000","name":"test","created_at":"0001-01-01T00:00:00Z","connections":[]}}}`, + `{"errors": [{"code": 1003, "message":"An A, AAAA or CNAME record already exists with that host"}], "result": {"id": "00000000-0000-0000-0000-000000000000","name":"test","created_at":"0001-01-01T00:00:00Z","connections":[]}}}`, } for i, test := range tests {