From e99b5bf49288c3b5ee81cc9b3e58adb2053af375 Mon Sep 17 00:00:00 2001 From: Chris Smith Date: Thu, 27 Oct 2022 17:51:10 -0600 Subject: [PATCH 1/4] feat(v2/apierror): add apierror.FromWrappingError --- v2/apierror/apierror.go | 46 +++++++++++++++++------ v2/apierror/apierror_test.go | 73 +++++++++++++++++++++++++++++++----- 2 files changed, 99 insertions(+), 20 deletions(-) diff --git a/v2/apierror/apierror.go b/v2/apierror/apierror.go index 787a619..0efaed0 100644 --- a/v2/apierror/apierror.go +++ b/v2/apierror/apierror.go @@ -233,30 +233,54 @@ func (a *APIError) Metadata() map[string]string { } -// FromError parses a Status error or a googleapi.Error and builds an APIError. -func FromError(err error) (*APIError, bool) { - if err == nil { - return nil, false - } - - ae := APIError{err: err} +// setDetailsFromError parses a Status error or a googleapi.Error +// and sets status and details or httpErr and details, respectively. +// It returns false if neither Status nor googleapi.Error can be parsed. +func (a *APIError) setDetailsFromError(err error) bool { st, isStatus := status.FromError(err) var herr *googleapi.Error isHTTPErr := errors.As(err, &herr) switch { case isStatus: - ae.status = st - ae.details = parseDetails(st.Details()) + a.status = st + a.details = parseDetails(st.Details()) case isHTTPErr: - ae.httpErr = herr - ae.details = parseHTTPDetails(herr) + a.httpErr = herr + a.details = parseHTTPDetails(herr) default: + return false + } + return true +} + +// FromError parses a Status error or a googleapi.Error and builds an APIError. +func FromError(err error) (*APIError, bool) { + if err == nil { return nil, false } + ae := APIError{err: err} + if !ae.setDetailsFromError(err) { + return nil, false + } return &ae, true +} + +// FromWrappingError parses a Status error or a googleapi.Error and +// builds an APIError, but to avoid a cycle when the provided error +// will externally wrap the new APIError, it does not set err in the +// new APIError. +func FromWrappingError(err error) (*APIError, bool) { + if err == nil { + return nil, false + } + ae := APIError{} + if !ae.setDetailsFromError(err) { + return nil, false + } + return &ae, true } // parseDetails accepts a slice of interface{} that should be backed by some diff --git a/v2/apierror/apierror_test.go b/v2/apierror/apierror_test.go index 358b918..52090be 100644 --- a/v2/apierror/apierror_test.go +++ b/v2/apierror/apierror_test.go @@ -363,21 +363,24 @@ func TestFromError(t *testing.T) { {&APIError{err: lS.Err(), status: lS, details: ErrDetails{LocalizedMessage: lo}}, true}, {&APIError{err: uS.Err(), status: uS, details: ErrDetails{Unknown: u}}, true}, {&APIError{err: hae, httpErr: hae, details: ErrDetails{ErrorInfo: httpErrInfo}}, true}, + {&APIError{err: errors.New("standard error")}, false}, } for _, tc := range tests { got, apiB := FromError(tc.apierr.err) if tc.b != apiB { - t.Errorf("got %v, want %v", apiB, tc.b) + t.Errorf("FromError(%s): got %v, want %v", tc.apierr.err, apiB, tc.b) } - if diff := cmp.Diff(got.details, tc.apierr.details, cmp.Comparer(proto.Equal)); diff != "" { - t.Errorf("got(-), want(+),: \n%s", diff) - } - if diff := cmp.Diff(got.status, tc.apierr.status, cmp.Comparer(proto.Equal), cmp.AllowUnexported(status.Status{})); diff != "" { - t.Errorf("got(-), want(+),: \n%s", diff) - } - if diff := cmp.Diff(got.err, tc.apierr.err, cmpopts.EquateErrors()); diff != "" { - t.Errorf("got(-), want(+),: \n%s", diff) + if tc.b { + if diff := cmp.Diff(got.details, tc.apierr.details, cmp.Comparer(proto.Equal)); diff != "" { + t.Errorf("got(-), want(+),: \n%s", diff) + } + if diff := cmp.Diff(got.status, tc.apierr.status, cmp.Comparer(proto.Equal), cmp.AllowUnexported(status.Status{})); diff != "" { + t.Errorf("got(-), want(+),: \n%s", diff) + } + if diff := cmp.Diff(got.err, tc.apierr.err, cmpopts.EquateErrors()); diff != "" { + t.Errorf("got(-), want(+),: \n%s", diff) + } } } if err, _ := FromError(nil); err != nil { @@ -389,6 +392,58 @@ func TestFromError(t *testing.T) { } } +func TestFromWrappingError(t *testing.T) { + httpErrInfo := &errdetails.ErrorInfo{Reason: "just because", Domain: "tests"} + any, err := anypb.New(httpErrInfo) + if err != nil { + t.Fatal(err) + } + e := &jsonerror.Error{Error: &jsonerror.Error_Status{Details: []*anypb.Any{any}}} + data, err := protojson.Marshal(e) + if err != nil { + t.Fatal(err) + } + hae := &googleapi.Error{ + Body: string(data), + } + + se := errors.New("standard error") + + tests := []struct { + source error + apierr *APIError + b bool + }{ + {hae, &APIError{httpErr: hae, details: ErrDetails{ErrorInfo: httpErrInfo}}, true}, + {se, &APIError{err: se}, false}, + } + + for _, tc := range tests { + got, apiB := FromWrappingError(tc.source) + if tc.b != apiB { + t.Errorf("FromWrappingError(%s): got %v, want %v", tc.apierr, apiB, tc.b) + } + if tc.b { + if diff := cmp.Diff(got.details, tc.apierr.details, cmp.Comparer(proto.Equal)); diff != "" { + t.Errorf("got(-), want(+),: \n%s", diff) + } + if diff := cmp.Diff(got.status, tc.apierr.status, cmp.Comparer(proto.Equal), cmp.AllowUnexported(status.Status{})); diff != "" { + t.Errorf("got(-), want(+),: \n%s", diff) + } + if got.err != nil { + t.Errorf("got %s, want nil", got.err) + } + } + } + if err, _ := FromWrappingError(nil); err != nil { + t.Errorf("got %s, want nil", err) + } + + if c, _ := FromWrappingError(context.DeadlineExceeded); c != nil { + t.Errorf("got %s, want nil", c) + } +} + func golden(name, got string) (string, error) { g := filepath.Join("testdata", name+".golden") if *update { From 56f61b1896a7da1d153cc22deaf6e3c8119e0d00 Mon Sep 17 00:00:00 2001 From: Chris Smith Date: Wed, 2 Nov 2022 11:22:46 -0600 Subject: [PATCH 2/4] rename FromWrappingError to ParseError --- v2/apierror/apierror.go | 25 ++++++++++--------------- v2/apierror/apierror_test.go | 11 ++++++----- 2 files changed, 16 insertions(+), 20 deletions(-) diff --git a/v2/apierror/apierror.go b/v2/apierror/apierror.go index 0efaed0..925005c 100644 --- a/v2/apierror/apierror.go +++ b/v2/apierror/apierror.go @@ -256,27 +256,22 @@ func (a *APIError) setDetailsFromError(err error) bool { // FromError parses a Status error or a googleapi.Error and builds an APIError. func FromError(err error) (*APIError, bool) { - if err == nil { - return nil, false - } - - ae := APIError{err: err} - if !ae.setDetailsFromError(err) { - return nil, false - } - return &ae, true + return ParseError(err, true) } -// FromWrappingError parses a Status error or a googleapi.Error and -// builds an APIError, but to avoid a cycle when the provided error -// will externally wrap the new APIError, it does not set err in the -// new APIError. -func FromWrappingError(err error) (*APIError, bool) { +// ParseError parses a Status error or a googleapi.Error and +// builds an APIError. If wrap is true, it sets err in the +// new APIError. If wrap is false, it does not set err, in order +// to avoid a cycle when the provided error will externally wrap +// the new APIError. +func ParseError(err error, wrap bool) (*APIError, bool) { if err == nil { return nil, false } - ae := APIError{} + if wrap { + ae = APIError{err: err} + } if !ae.setDetailsFromError(err) { return nil, false } diff --git a/v2/apierror/apierror_test.go b/v2/apierror/apierror_test.go index 52090be..ddd0419 100644 --- a/v2/apierror/apierror_test.go +++ b/v2/apierror/apierror_test.go @@ -392,7 +392,7 @@ func TestFromError(t *testing.T) { } } -func TestFromWrappingError(t *testing.T) { +func TestParseError(t *testing.T) { httpErrInfo := &errdetails.ErrorInfo{Reason: "just because", Domain: "tests"} any, err := anypb.New(httpErrInfo) if err != nil { @@ -419,9 +419,10 @@ func TestFromWrappingError(t *testing.T) { } for _, tc := range tests { - got, apiB := FromWrappingError(tc.source) + // ParseError with wrap = true is covered by TestFromError, above. + got, apiB := ParseError(tc.source, false) if tc.b != apiB { - t.Errorf("FromWrappingError(%s): got %v, want %v", tc.apierr, apiB, tc.b) + t.Errorf("ParseError(%s, false): got %v, want %v", tc.apierr, apiB, tc.b) } if tc.b { if diff := cmp.Diff(got.details, tc.apierr.details, cmp.Comparer(proto.Equal)); diff != "" { @@ -435,11 +436,11 @@ func TestFromWrappingError(t *testing.T) { } } } - if err, _ := FromWrappingError(nil); err != nil { + if err, _ := ParseError(nil, false); err != nil { t.Errorf("got %s, want nil", err) } - if c, _ := FromWrappingError(context.DeadlineExceeded); c != nil { + if c, _ := ParseError(context.DeadlineExceeded, false); c != nil { t.Errorf("got %s, want nil", c) } } From 51073f863303a7383fd2d352b0941697c3b626fe Mon Sep 17 00:00:00 2001 From: Chris Smith Date: Wed, 2 Nov 2022 12:29:13 -0600 Subject: [PATCH 3/4] update function docs --- v2/apierror/apierror.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/v2/apierror/apierror.go b/v2/apierror/apierror.go index 925005c..81462e4 100644 --- a/v2/apierror/apierror.go +++ b/v2/apierror/apierror.go @@ -261,9 +261,7 @@ func FromError(err error) (*APIError, bool) { // ParseError parses a Status error or a googleapi.Error and // builds an APIError. If wrap is true, it sets err in the -// new APIError. If wrap is false, it does not set err, in order -// to avoid a cycle when the provided error will externally wrap -// the new APIError. +// new APIError. func ParseError(err error, wrap bool) (*APIError, bool) { if err == nil { return nil, false From b7d7a156eadf1b778dc42e9e392fc45185439874 Mon Sep 17 00:00:00 2001 From: Chris Smith Date: Wed, 2 Nov 2022 13:18:05 -0600 Subject: [PATCH 4/4] update function docs 2 --- v2/apierror/apierror.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/v2/apierror/apierror.go b/v2/apierror/apierror.go index 81462e4..aa6be13 100644 --- a/v2/apierror/apierror.go +++ b/v2/apierror/apierror.go @@ -254,14 +254,16 @@ func (a *APIError) setDetailsFromError(err error) bool { return true } -// FromError parses a Status error or a googleapi.Error and builds an APIError. +// FromError parses a Status error or a googleapi.Error and builds an +// APIError, wrapping the provided error in the new APIError. It +// returns false if neither Status nor googleapi.Error can be parsed. func FromError(err error) (*APIError, bool) { return ParseError(err, true) } -// ParseError parses a Status error or a googleapi.Error and -// builds an APIError. If wrap is true, it sets err in the -// new APIError. +// ParseError parses a Status error or a googleapi.Error and builds an +// APIError. If wrap is true, it wraps the error in the new APIError. +// It returns false if neither Status nor googleapi.Error can be parsed. func ParseError(err error, wrap bool) (*APIError, bool) { if err == nil { return nil, false