Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show additional error details when an rpc fails #379

Merged
merged 1 commit into from
Jun 22, 2023
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 22 additions & 7 deletions format.go
Original file line number Diff line number Diff line change
Expand Up @@ -136,10 +136,25 @@ type Formatter func(proto.Message) (string, error)
func NewJSONFormatter(emitDefaults bool, resolver jsonpb.AnyResolver) Formatter {
marshaler := jsonpb.Marshaler{
EmitDefaults: emitDefaults,
Indent: " ",
AnyResolver: resolver,
}
return marshaler.MarshalToString
// Workaround for indentation issue in jsonpb with Any messages.
// Bug was originally fixed in https://github.com/golang/protobuf/pull/834
// but later re-introduced before the module was deprecated and frozen.
// If jsonpb is ever replaced with google.golang.org/protobuf/encoding/protojson
// this workaround will no longer be needed.
formatter := func(message proto.Message) (string, error) {
output, err := marshaler.MarshalToString(message)
if err != nil {
return "", err
}
var buf bytes.Buffer
if err := json.Indent(&buf, []byte(output), "", " "); err != nil {
return "", err
}
return buf.String(), nil
}
return formatter
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jhump this seems reasonable to me, WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, this PR LGTM.

}

// NewTextFormatter returns a formatter that returns strings in the protobuf
Expand Down Expand Up @@ -274,11 +289,11 @@ func (r *anyResolver) Resolve(typeUrl string) (proto.Message, error) {
if !ok {
return nil, fmt.Errorf("unknown message: %s", typeUrl)
}
// populate any extensions for this message, too
if exts, err := r.source.AllExtensionsForType(mname); err != nil {
return nil, err
} else if err := r.er.AddExtension(exts...); err != nil {
return nil, err
// populate any extensions for this message, too (if there are any)
if exts, err := r.source.AllExtensionsForType(mname); err == nil {
if err := r.er.AddExtension(exts...); err != nil {
return nil, err
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's going on here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To give some extra context, we are trying to adopt the richer error model of grpc but grpcurl currently outputs the errors like this:

ERROR:
  Code: InvalidArgument
  Message: Request contains invalid fields
  Details:
  1)	{"@error":"google.rpc.BadRequest is not recognized; see @value for raw binary message data","@type":"type.googleapis.com/google.rpc.BadRequest","@value":"CiQKCnN0cmluZ19vbmUSFk5vIGVtcHR5IHZhbHVlIGFsbG93ZWQKJAoKc3RyaW5nX3R3bxIWTm8gZW1wdHkgdmFsdWUgYWxsb3dlZA=="}

The problem with the current code here is that when using service reflection to resolve google.rpc.BadRequest, it doesn't work since google.rpc.BadRequest doesn't have any protobuf extension.
You can see here that r.source.AllExtensionsForType(mname) returns an err github.com/jhump/protoreflect/grpcreflect.elementNotFoundError which then aborts the formatting.

image

With the change in this PR, the elementNotFoundError will be ignored and the formatting of the error will successfully be processed and printed:

ERROR:
  Code: InvalidArgument
  Message: Request contains invalid fields
  Details:
  1)	{
    	  "@type": "type.googleapis.com/google.rpc.BadRequest",
    	  "field_violations": [
    	    {
    	      "field": "string_one",
    	      "description": "No empty value allowed"
    	    },
    	    {
    	      "field": "string_two",
    	      "description": "No empty value allowed"
    	    }
    	  ]
    	}

This new code should add extensions (i.e. r.er.AddExtension(exts...)) only if extensions could be found with r.source.AllExtensionsForType(mname).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, that makes sense. Why not also ignore the error from r.er.AddExtension(exts...)?

}

if r.mf == nil {
Expand Down