fix(v2/apierror): return Unknown GRPCStatus when err source is HTTP (#…

…260)

* return Unknown status when googleapi.Error because gRPC treats nil as OK

closes: #254

v2/apierror/apierror.go

@@ -39,6 +39,7 @@ import (
 	jsonerror "github.com/googleapis/gax-go/v2/apierror/internal/proto"
 	"google.golang.org/api/googleapi"
 	"google.golang.org/genproto/googleapis/rpc/errdetails"
+	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 	"google.golang.org/protobuf/encoding/protojson"
 	"google.golang.org/protobuf/proto"
@@ -197,12 +198,12 @@ func (a *APIError) Unwrap() error {
 // Error returns a readable representation of the APIError.
 func (a *APIError) Error() string {
 	var msg string
-	if a.status != nil {
-		msg = a.err.Error()
-	} else if a.httpErr != nil {
+	if a.httpErr != nil {
 		// Truncate the googleapi.Error message because it dumps the Details in
 		// an ugly way.
 		msg = fmt.Sprintf("googleapi: Error %d: %s", a.httpErr.Code, a.httpErr.Message)
+	} else if a.status != nil {
+		msg = a.err.Error()
 	}
 	return strings.TrimSpace(fmt.Sprintf("%s\n%s", msg, a.details))
 }
@@ -236,6 +237,9 @@ func (a *APIError) Metadata() map[string]string {
 // 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.
+// When err is a googleapi.Error, the status of the returned error will
+// be set to an Unknown error, rather than nil, since a nil code is
+// interpreted as OK in the gRPC status package.
 func (a *APIError) setDetailsFromError(err error) bool {
 	st, isStatus := status.FromError(err)
 	var herr *googleapi.Error
@@ -248,6 +252,7 @@ func (a *APIError) setDetailsFromError(err error) bool {
 	case isHTTPErr:
 		a.httpErr = herr
 		a.details = parseHTTPDetails(herr)
+		a.status = status.New(codes.Unknown, herr.Message)
 	default:
 		return false
 	}

v2/apierror/apierror_test.go

@@ -183,6 +183,51 @@ func TestError(t *testing.T) {
 	}
 	rqS, _ := status.New(codes.Canceled, "Request cancelled by client").WithDetails(rq, ri, pf, br, qf)
 
+	rs := &errdetails.ResourceInfo{
+		ResourceType: "Foo",
+		ResourceName: "Bar",
+		Owner:        "Client",
+		Description:  "Directory not Found",
+	}
+	rS, _ := status.New(codes.NotFound, "rs").WithDetails(rs)
+
+	deb := &errdetails.DebugInfo{
+		StackEntries: []string{"Foo", "Bar"},
+		Detail:       "Stack",
+	}
+	dS, _ := status.New(codes.DataLoss, "Here is the debug info").WithDetails(deb)
+
+	hp := &errdetails.Help{
+		Links: []*errdetails.Help_Link{{Description: "Foo", Url: "Bar"}},
+	}
+	hS, _ := status.New(codes.Unimplemented, "Help Info").WithDetails(hp)
+
+	lo := &errdetails.LocalizedMessage{
+		Locale:  "Foo",
+		Message: "Bar",
+	}
+	lS, _ := status.New(codes.Unknown, "Localized Message").WithDetails(lo)
+
+	var uu []interface{}
+	uu = append(uu, "unknown detail 1")
+	uS := status.New(codes.Unknown, "Unknown")
+
+	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{
+		Message: "just because",
+		Body:    string(data),
+	}
+	haeS := status.New(codes.Unknown, "just because")
+
 	tests := []struct {
 		apierr *APIError
 		name   string
@@ -191,7 +236,13 @@ func TestError(t *testing.T) {
 		{&APIError{err: bS.Err(), status: bS, details: ErrDetails{BadRequest: br}}, "bad_request"},
 		{&APIError{err: rqS.Err(), status: rqS, details: ErrDetails{RequestInfo: rq, RetryInfo: ri,
 			PreconditionFailure: pf, QuotaFailure: qf, BadRequest: br}}, "multiple_info"},
+		{&APIError{err: bS.Err(), status: rS, details: ErrDetails{ResourceInfo: rs}}, "resource_info"},
+		{&APIError{err: bS.Err(), status: dS, details: ErrDetails{DebugInfo: deb}}, "debug_info"},
+		{&APIError{err: bS.Err(), status: hS, details: ErrDetails{Help: hp}}, "help"},
+		{&APIError{err: bS.Err(), status: lS, details: ErrDetails{LocalizedMessage: lo}}, "localized_message"},
+		{&APIError{err: bS.Err(), status: uS, details: ErrDetails{Unknown: uu}}, "unknown"},
 		{&APIError{err: bS.Err(), status: bS, details: ErrDetails{}}, "empty"},
+		{&APIError{err: hae, httpErr: hae, status: haeS, details: ErrDetails{ErrorInfo: httpErrInfo}}, "http_err"},
 	}
 	for _, tc := range tests {
 		t.Helper()
@@ -344,8 +395,10 @@ func TestFromError(t *testing.T) {
 		t.Fatal(err)
 	}
 	hae := &googleapi.Error{
-		Body: string(data),
+		Message: "just because",
+		Body:    string(data),
 	}
+	haeS := status.New(codes.Unknown, "just because")
 
 	tests := []struct {
 		apierr *APIError
@@ -362,7 +415,7 @@ func TestFromError(t *testing.T) {
 		{&APIError{err: hS.Err(), status: hS, details: ErrDetails{Help: hp}}, true},
 		{&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: hae, httpErr: hae, status: haeS, details: ErrDetails{ErrorInfo: httpErrInfo}}, true},
 		{&APIError{err: errors.New("standard error")}, false},
 	}
 
@@ -373,13 +426,13 @@ func TestFromError(t *testing.T) {
 		}
 		if tc.b {
 			if diff := cmp.Diff(got.details, tc.apierr.details, cmp.Comparer(proto.Equal)); diff != "" {
-				t.Errorf("got(-), want(+),: \n%s", diff)
+				t.Errorf("FromError(%s): got(-), want(+),: \n%s", tc.apierr.err, 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)
+				t.Errorf("FromError(%s): got(-), want(+),: \n%s", tc.apierr.err, diff)
 			}
 			if diff := cmp.Diff(got.err, tc.apierr.err, cmpopts.EquateErrors()); diff != "" {
-				t.Errorf("got(-), want(+),: \n%s", diff)
+				t.Errorf("FromError(%s): got(-), want(+),: \n%s", tc.apierr.err, diff)
 			}
 		}
 	}
@@ -404,8 +457,10 @@ func TestParseError(t *testing.T) {
 		t.Fatal(err)
 	}
 	hae := &googleapi.Error{
-		Body: string(data),
+		Message: "just because",
+		Body:    string(data),
 	}
+	haeS := status.New(codes.Unknown, "just because")
 
 	se := errors.New("standard error")
 
@@ -414,7 +469,7 @@ func TestParseError(t *testing.T) {
 		apierr *APIError
 		b      bool
 	}{
-		{hae, &APIError{httpErr: hae, details: ErrDetails{ErrorInfo: httpErrInfo}}, true},
+		{hae, &APIError{httpErr: hae, status: haeS, details: ErrDetails{ErrorInfo: httpErrInfo}}, true},
 		{se, &APIError{err: se}, false},
 	}
 

v2/apierror/testdata/debug_info.golden

@@ -0,0 +1,2 @@
+rpc error: code = InvalidArgument desc = br
+error details: name = DebugInfo detail = Stack stack = Foo Bar

v2/apierror/testdata/help.golden

@@ -0,0 +1,2 @@
+rpc error: code = InvalidArgument desc = br
+error details: name = Help desc = Foo url = Bar

v2/apierror/testdata/http_err.golden

@@ -0,0 +1,2 @@
+googleapi: Error 0: just because
+error details: name = ErrorInfo reason = just because domain = tests metadata = map[]

v2/apierror/testdata/localized_message.golden

@@ -0,0 +1,2 @@
+rpc error: code = InvalidArgument desc = br
+error details: name = LocalizedMessage locale = Foo msg = Bar

v2/apierror/testdata/resource_info.golden

@@ -0,0 +1,2 @@
+rpc error: code = InvalidArgument desc = br
+error details: name = ResourceInfo type = Foo resourcename = Bar owner = Client desc = Directory not Found

v2/apierror/testdata/unknown.golden

@@ -0,0 +1,2 @@
+rpc error: code = InvalidArgument desc = br
+error details: name = Unknown  desc = unknown detail 1