Skip to content

Commit

Permalink
status: fix panic when servers return a wrapped error with status OK (#…
Browse files Browse the repository at this point in the history
  • Loading branch information
atollena authored Jul 5, 2023
1 parent acbfcbb commit df3e021
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 4 deletions.
29 changes: 25 additions & 4 deletions status/status.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,11 +77,18 @@ func FromProto(s *spb.Status) *Status {
// FromError returns a Status representation of err.
//
// - If err was produced by this package or implements the method `GRPCStatus()
// *Status`, or if err wraps a type satisfying this, the appropriate Status is
// returned. For wrapped errors, the message returned contains the entire
// err.Error() text and not just the wrapped status.
// *Status` and `GRPCStatus()` does not return nil, or if err wraps a type
// satisfying this, the Status from `GRPCStatus()` is returned. For wrapped
// errors, the message returned contains the entire err.Error() text and not
// just the wrapped status. In that case, ok is true.
//
// - If err is nil, a Status is returned with codes.OK and no message.
// - If err is nil, a Status is returned with codes.OK and no message, and ok
// is true.
//
// - If err implements the method `GRPCStatus() *Status` and `GRPCStatus()`
// returns nil (which maps to Codes.OK), or if err wraps a type
// satisfying this, a Status is returned with codes.Unknown and err's
// Error() message, and ok is false.
//
// - Otherwise, err is an error not compatible with this package. In this
// case, a Status is returned with codes.Unknown and err's Error() message,
Expand All @@ -92,10 +99,24 @@ func FromError(err error) (s *Status, ok bool) {
}
type grpcstatus interface{ GRPCStatus() *Status }
if gs, ok := err.(grpcstatus); ok {
if gs.GRPCStatus() == nil {
// Error has status nil, which maps to codes.OK. There
// is no sensible behavior for this, so we turn it into
// an error with codes.Unknown and discard the existing
// status.
return New(codes.Unknown, err.Error()), false
}
return gs.GRPCStatus(), true
}
var gs grpcstatus
if errors.As(err, &gs) {
if gs.GRPCStatus() == nil {
// Error wraps an error that has status nil, which maps
// to codes.OK. There is no sensible behavior for this,
// so we turn it into an error with codes.Unknown and
// discard the existing status.
return New(codes.Unknown, err.Error()), false
}
p := gs.GRPCStatus().Proto()
p.Message = err.Error()
return status.FromProto(p), true
Expand Down
27 changes: 27 additions & 0 deletions status/status_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,33 @@ func (s) TestFromErrorWrapped(t *testing.T) {
}
}

type customErrorNilStatus struct {
}

func (c customErrorNilStatus) Error() string {
return "test"
}

func (c customErrorNilStatus) GRPCStatus() *Status {
return nil
}

func (s) TestFromErrorImplementsInterfaceReturnsOKStatus(t *testing.T) {
err := customErrorNilStatus{}
s, ok := FromError(err)
if ok || s.Code() != codes.Unknown || s.Message() != err.Error() {
t.Fatalf("FromError(%v) = %v, %v; want <Code()=%s, Message()=%q, Err()!=nil>, true", err, s, ok, codes.Unknown, err.Error())
}
}

func (s) TestFromErrorImplementsInterfaceReturnsOKStatusWrapped(t *testing.T) {
err := fmt.Errorf("wrapping: %w", customErrorNilStatus{})
s, ok := FromError(err)
if ok || s.Code() != codes.Unknown || s.Message() != err.Error() {
t.Fatalf("FromError(%v) = %v, %v; want <Code()=%s, Message()=%q, Err()!=nil>, true", err, s, ok, codes.Unknown, err.Error())
}
}

func (s) TestFromErrorImplementsInterfaceWrapped(t *testing.T) {
const code, message = codes.Internal, "test description"
err := fmt.Errorf("wrapped error: %w", customError{Code: code, Message: message})
Expand Down

0 comments on commit df3e021

Please sign in to comment.