-
Notifications
You must be signed in to change notification settings - Fork 510
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
Conversation
Hello @dragonsinth could you have a look at this small PR when you have time? Thank you! |
if exts, err := r.source.AllExtensionsForType(mname); err == nil { | ||
if err := r.er.AddExtension(exts...); err != nil { | ||
return nil, err | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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)
.
There was a problem hiding this comment.
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...)
?
} | ||
return buf.String(), nil | ||
} | ||
return formatter |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, this PR LGTM.
Hello @dragonsinth @jhump can we have this PR merged? :) thx! |
@dragonsinth thank you for merging the PR, could you make a new release too? :) |
### Summary #### v1.8.9 - Disable CGO for improved compatibility across distros (fullstorydev/grpcurl#420) - Bump golang.org/x/net from 0.9.0 to 0.17.0 (fullstorydev/grpcurl#419) - SIGSEGV: panic: runtime error: invalid memory address or nil pointer dereference in protoreflect (fullstorydev/grpcurl#416) - Added alts credential option (fullstorydev/grpcurl#341) #### v1.8.8 - Update go.mod, goreleaser for v1.8.8 (fullstorydev/grpcurl#413) - Run tests on Go 1.21 (fullstorydev/grpcurl#408) - Update protoreflect v1.15.2 and grpc v1.57.0 (fullstorydev/grpcurl#406) - Use grpc.reflection.v1.ServerReflection (fullstorydev/grpcurl#407) - Bump google.golang.org/protobuf from 1.30.0 to 1.31.0 (fullstorydev/grpcurl#401) - Bump google.golang.org/grpc from 1.55.0 to 1.56.1 (fullstorydev/grpcurl#400) - Fix issues with error details (fullstorydev/grpcurl#379) - fix nil-dereference panic (fullstorydev/grpcurl#395) - Bump google.golang.org/grpc from 1.54.0 to 1.55.0 (fullstorydev/grpcurl#390) - Add "checkgenerate" make target to CI (fullstorydev/grpcurl#385) - Bump google.golang.org/grpc from 1.53.0 to 1.54.0 (fullstorydev/grpcurl#383) - Bump google.golang.org/protobuf from 1.29.1 to 1.30.0 (fullstorydev/grpcurl#378) - Bump google.golang.org/protobuf from 1.29.0 to 1.29.1 (fullstorydev/grpcurl#376) - Bump google.golang.org/protobuf from 1.28.1 to 1.29.0 (fullstorydev/grpcurl#375) - Bump github.com/golang/protobuf from 1.5.2 to 1.5.3 (fullstorydev/grpcurl#374) - Bump google.golang.org/grpc from 1.52.3 to 1.53.0 (fullstorydev/grpcurl#370) - Install the CodeSee workflow. Learn more at https://docs.codesee.io (fullstorydev/grpcurl#368) - Bump google.golang.org/grpc from 1.51.0 to 1.52.3 (fullstorydev/grpcurl#365) - Bump github.com/jhump/protoreflect from 1.14.0 to 1.14.1 (fullstorydev/grpcurl#361) - Bump google.golang.org/grpc from 1.50.1 to 1.51.0 (fullstorydev/grpcurl#348) - fix funcname in comment (fullstorydev/grpcurl#346) - Bump github.com/jhump/protoreflect from 1.13.0 to 1.14.0 (fullstorydev/grpcurl#343) - Bump google.golang.org/grpc from 1.50.0 to 1.50.1 (fullstorydev/grpcurl#338) - Bump google.golang.org/grpc from 1.49.0 to 1.50.0 (fullstorydev/grpcurl#336) - Bump github.com/jhump/protoreflect from 1.12.0 to 1.13.0 (fullstorydev/grpcurl#335) - Bump google.golang.org/grpc from 1.48.0 to 1.49.0 (fullstorydev/grpcurl#330) - fixup release process (fullstorydev/grpcurl#328) #### v1.8.7 - Unix sockets for windows - Lots of dependency version updates - Support for Go 1.18 - Add go 1.18 support; set Dockerfile to go 1.18 (fullstorydev/grpcurl#325) - build alpine base image (fullstorydev/grpcurl#311) - fix some typos (fullstorydev/grpcurl#314) - Bump google.golang.org/grpc from 1.47.0 to 1.48.0 (fullstorydev/grpcurl#324) - Adding power(ppc64le) arch support (fullstorydev/grpcurl#296) - Enable support for Unix sockets for Windows by enabling -unix flag for Windows builds. (fullstorydev/grpcurl#317) - Bump google.golang.org/grpc from 1.46.2 to 1.47.0 (fullstorydev/grpcurl#315) - Bump github.com/jhump/protoreflect from 1.10.3 to 1.12.0 (fullstorydev/grpcurl#294) - Bump google.golang.org/grpc from 1.44.0 to 1.46.2 (fullstorydev/grpcurl#310) - Bump google.golang.org/protobuf from 1.27.1 to 1.28.0 (fullstorydev/grpcurl#298) - use newer goreleaser (fullstorydev/grpcurl#293) - Restore support for linux/s390x for the next release. (fullstorydev/grpcurl#292) - Bump google.golang.org/protobuf from 1.26.0 to 1.27.1 (fullstorydev/grpcurl#288) #### v1.8.6 - Some bugs have been addressed in the library used to parse proto source files. Previously grpcurl would accept proto source files that could not actually be compiled with protoc. The converse could also happen: grpcurl could reject some proto source files that could successfully be compiled with protoc. More details can be found in the release notes for the changes to the protoparse library, versions v1.10.2 and v1.10.3. - Some implementations of the server reflection service have been observed to return multiple (even superfluous) file descriptors, in response to requests made by grpcurl. These extra files, if not returned in a particular order, would cause grpcurl to report an error that the service or method to be invoked could be not be resolved. The reflection client in grpcurl is now more robust to this condition and can handle responses with file descriptors in any order, so it should be interoperable with a larger variety of servers. - When a request message includes a field of type google.protobuf.Value and a value for that field that was a JSON array, grpcurl would incorrectly interpret the JSON array as if it were a single atomic value, the last value that was in the array. This has been fixed. - When a response message includes non-printable characters or code points outside the ASCII 7-bit range in the name of a field in a JSON object, it could be improperly encoded with escape characters that are not valid JSON. Standard tools/libraries could then fail to parse the JSON output from grpcurl. This has been fixed. Signed-off-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>
### Summary #### v1.8.9 - Disable CGO for improved compatibility across distros (fullstorydev/grpcurl#420) - Bump golang.org/x/net from 0.9.0 to 0.17.0 (fullstorydev/grpcurl#419) - SIGSEGV: panic: runtime error: invalid memory address or nil pointer dereference in protoreflect (fullstorydev/grpcurl#416) - Added alts credential option (fullstorydev/grpcurl#341) #### v1.8.8 - Update go.mod, goreleaser for v1.8.8 (fullstorydev/grpcurl#413) - Run tests on Go 1.21 (fullstorydev/grpcurl#408) - Update protoreflect v1.15.2 and grpc v1.57.0 (fullstorydev/grpcurl#406) - Use grpc.reflection.v1.ServerReflection (fullstorydev/grpcurl#407) - Bump google.golang.org/protobuf from 1.30.0 to 1.31.0 (fullstorydev/grpcurl#401) - Bump google.golang.org/grpc from 1.55.0 to 1.56.1 (fullstorydev/grpcurl#400) - Fix issues with error details (fullstorydev/grpcurl#379) - fix nil-dereference panic (fullstorydev/grpcurl#395) - Bump google.golang.org/grpc from 1.54.0 to 1.55.0 (fullstorydev/grpcurl#390) - Add "checkgenerate" make target to CI (fullstorydev/grpcurl#385) - Bump google.golang.org/grpc from 1.53.0 to 1.54.0 (fullstorydev/grpcurl#383) - Bump google.golang.org/protobuf from 1.29.1 to 1.30.0 (fullstorydev/grpcurl#378) - Bump google.golang.org/protobuf from 1.29.0 to 1.29.1 (fullstorydev/grpcurl#376) - Bump google.golang.org/protobuf from 1.28.1 to 1.29.0 (fullstorydev/grpcurl#375) - Bump github.com/golang/protobuf from 1.5.2 to 1.5.3 (fullstorydev/grpcurl#374) - Bump google.golang.org/grpc from 1.52.3 to 1.53.0 (fullstorydev/grpcurl#370) - Install the CodeSee workflow. Learn more at https://docs.codesee.io (fullstorydev/grpcurl#368) - Bump google.golang.org/grpc from 1.51.0 to 1.52.3 (fullstorydev/grpcurl#365) - Bump github.com/jhump/protoreflect from 1.14.0 to 1.14.1 (fullstorydev/grpcurl#361) - Bump google.golang.org/grpc from 1.50.1 to 1.51.0 (fullstorydev/grpcurl#348) - fix funcname in comment (fullstorydev/grpcurl#346) - Bump github.com/jhump/protoreflect from 1.13.0 to 1.14.0 (fullstorydev/grpcurl#343) - Bump google.golang.org/grpc from 1.50.0 to 1.50.1 (fullstorydev/grpcurl#338) - Bump google.golang.org/grpc from 1.49.0 to 1.50.0 (fullstorydev/grpcurl#336) - Bump github.com/jhump/protoreflect from 1.12.0 to 1.13.0 (fullstorydev/grpcurl#335) - Bump google.golang.org/grpc from 1.48.0 to 1.49.0 (fullstorydev/grpcurl#330) - fixup release process (fullstorydev/grpcurl#328) #### v1.8.7 - Unix sockets for windows - Lots of dependency version updates - Support for Go 1.18 - Add go 1.18 support; set Dockerfile to go 1.18 (fullstorydev/grpcurl#325) - build alpine base image (fullstorydev/grpcurl#311) - fix some typos (fullstorydev/grpcurl#314) - Bump google.golang.org/grpc from 1.47.0 to 1.48.0 (fullstorydev/grpcurl#324) - Adding power(ppc64le) arch support (fullstorydev/grpcurl#296) - Enable support for Unix sockets for Windows by enabling -unix flag for Windows builds. (fullstorydev/grpcurl#317) - Bump google.golang.org/grpc from 1.46.2 to 1.47.0 (fullstorydev/grpcurl#315) - Bump github.com/jhump/protoreflect from 1.10.3 to 1.12.0 (fullstorydev/grpcurl#294) - Bump google.golang.org/grpc from 1.44.0 to 1.46.2 (fullstorydev/grpcurl#310) - Bump google.golang.org/protobuf from 1.27.1 to 1.28.0 (fullstorydev/grpcurl#298) - use newer goreleaser (fullstorydev/grpcurl#293) - Restore support for linux/s390x for the next release. (fullstorydev/grpcurl#292) - Bump google.golang.org/protobuf from 1.26.0 to 1.27.1 (fullstorydev/grpcurl#288) #### v1.8.6 - Some bugs have been addressed in the library used to parse proto source files. Previously grpcurl would accept proto source files that could not actually be compiled with protoc. The converse could also happen: grpcurl could reject some proto source files that could successfully be compiled with protoc. More details can be found in the release notes for the changes to the protoparse library, versions v1.10.2 and v1.10.3. - Some implementations of the server reflection service have been observed to return multiple (even superfluous) file descriptors, in response to requests made by grpcurl. These extra files, if not returned in a particular order, would cause grpcurl to report an error that the service or method to be invoked could be not be resolved. The reflection client in grpcurl is now more robust to this condition and can handle responses with file descriptors in any order, so it should be interoperable with a larger variety of servers. - When a request message includes a field of type google.protobuf.Value and a value for that field that was a JSON array, grpcurl would incorrectly interpret the JSON array as if it were a single atomic value, the last value that was in the array. This has been fixed. - When a response message includes non-printable characters or code points outside the ASCII 7-bit range in the name of a field in a JSON object, it could be improperly encoded with escape characters that are not valid JSON. Standard tools/libraries could then fail to parse the JSON output from grpcurl. This has been fixed. Signed-off-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>
### Summary ### v1.9.1 - Update Dockerfile to use Go v1.21 (fullstorydev/grpcurl#455) - chore: fix some typos in comments (fullstorydev/grpcurl#454) ### v1.9.0 - Use latest protoreflect to fix some bugs (fullstorydev/grpcurl#453) - Brand name update (fullstorydev/grpcurl#452) - Bump github.com/golang/protobuf from 1.5.3 to 1.5.4 (fullstorydev/grpcurl#448) - goreleaser: bump version & add nfmp support fullstorydev/grpcurl#440) - Enable xDS credentials (fullstorydev/grpcurl#424) - Bump github.com/jhump/protoreflect from 1.15.5 to 1.15.6 (fullstorydev/grpcurl#446) - Use localhost for default unix domain socket authority (fullstorydev/grpcurl#445) - Bump github.com/jhump/protoreflect from 1.15.4 to 1.15.5 (fullstorydev/grpcurl#443) - Added initial support for -t flag to show timings (fullstorydev/grpcurl#428) - Expand the documentation of -max-time to clarify this sets the RPC timeout (fullstorydev/grpcurl#435) - Bump github.com/jhump/protoreflect from 1.15.3 to 1.15.4 (fullstorydev/grpcurl#436) - Bump google.golang.org/protobuf from 1.31.0 to 1.32.0 (fullstorydev/grpcurl#437) - indent: rip out old go 1.9 support - Bump golang.google.org/grpc to v1.57.1 (fullstorydev/grpcurl#427) - Update Tarball URL used by Homebrew (fullstorydev/grpcurl#421) #### v1.8.9 - Disable CGO for improved compatibility across distros (fullstorydev/grpcurl#420) - Bump golang.org/x/net from 0.9.0 to 0.17.0 (fullstorydev/grpcurl#419) - SIGSEGV: panic: runtime error: invalid memory address or nil pointer dereference in protoreflect (fullstorydev/grpcurl#416) - Added alts credential option (fullstorydev/grpcurl#341) #### v1.8.8 - Update go.mod, goreleaser for v1.8.8 (fullstorydev/grpcurl#413) - Run tests on Go 1.21 (fullstorydev/grpcurl#408) - Update protoreflect v1.15.2 and grpc v1.57.0 (fullstorydev/grpcurl#406) - Use grpc.reflection.v1.ServerReflection (fullstorydev/grpcurl#407) - Bump google.golang.org/protobuf from 1.30.0 to 1.31.0 (fullstorydev/grpcurl#401) - Bump google.golang.org/grpc from 1.55.0 to 1.56.1 (fullstorydev/grpcurl#400) - Fix issues with error details (fullstorydev/grpcurl#379) - fix nil-dereference panic (fullstorydev/grpcurl#395) - Bump google.golang.org/grpc from 1.54.0 to 1.55.0 (fullstorydev/grpcurl#390) - Add "checkgenerate" make target to CI (fullstorydev/grpcurl#385) - Bump google.golang.org/grpc from 1.53.0 to 1.54.0 (fullstorydev/grpcurl#383) - Bump google.golang.org/protobuf from 1.29.1 to 1.30.0 (fullstorydev/grpcurl#378) - Bump google.golang.org/protobuf from 1.29.0 to 1.29.1 (fullstorydev/grpcurl#376) - Bump google.golang.org/protobuf from 1.28.1 to 1.29.0 (fullstorydev/grpcurl#375) - Bump github.com/golang/protobuf from 1.5.2 to 1.5.3 (fullstorydev/grpcurl#374) - Bump google.golang.org/grpc from 1.52.3 to 1.53.0 (fullstorydev/grpcurl#370) - Install the CodeSee workflow. Learn more at https://docs.codesee.io (fullstorydev/grpcurl#368) - Bump google.golang.org/grpc from 1.51.0 to 1.52.3 (fullstorydev/grpcurl#365) - Bump github.com/jhump/protoreflect from 1.14.0 to 1.14.1 (fullstorydev/grpcurl#361) - Bump google.golang.org/grpc from 1.50.1 to 1.51.0 (fullstorydev/grpcurl#348) - fix funcname in comment (fullstorydev/grpcurl#346) - Bump github.com/jhump/protoreflect from 1.13.0 to 1.14.0 (fullstorydev/grpcurl#343) - Bump google.golang.org/grpc from 1.50.0 to 1.50.1 (fullstorydev/grpcurl#338) - Bump google.golang.org/grpc from 1.49.0 to 1.50.0 (fullstorydev/grpcurl#336) - Bump github.com/jhump/protoreflect from 1.12.0 to 1.13.0 (fullstorydev/grpcurl#335) - Bump google.golang.org/grpc from 1.48.0 to 1.49.0 (fullstorydev/grpcurl#330) - fixup release process (fullstorydev/grpcurl#328) #### v1.8.7 - Unix sockets for windows - Lots of dependency version updates - Support for Go 1.18 - Add go 1.18 support; set Dockerfile to go 1.18 (fullstorydev/grpcurl#325) - build alpine base image (fullstorydev/grpcurl#311) - fix some typos (fullstorydev/grpcurl#314) - Bump google.golang.org/grpc from 1.47.0 to 1.48.0 (fullstorydev/grpcurl#324) - Adding power(ppc64le) arch support (fullstorydev/grpcurl#296) - Enable support for Unix sockets for Windows by enabling -unix flag for Windows builds. (fullstorydev/grpcurl#317) - Bump google.golang.org/grpc from 1.46.2 to 1.47.0 (fullstorydev/grpcurl#315) - Bump github.com/jhump/protoreflect from 1.10.3 to 1.12.0 (fullstorydev/grpcurl#294) - Bump google.golang.org/grpc from 1.44.0 to 1.46.2 (fullstorydev/grpcurl#310) - Bump google.golang.org/protobuf from 1.27.1 to 1.28.0 (fullstorydev/grpcurl#298) - use newer goreleaser (fullstorydev/grpcurl#293) - Restore support for linux/s390x for the next release. (fullstorydev/grpcurl#292) - Bump google.golang.org/protobuf from 1.26.0 to 1.27.1 (fullstorydev/grpcurl#288) #### v1.8.6 - Some bugs have been addressed in the library used to parse proto source files. Previously grpcurl would accept proto source files that could not actually be compiled with protoc. The converse could also happen: grpcurl could reject some proto source files that could successfully be compiled with protoc. More details can be found in the release notes for the changes to the protoparse library, versions v1.10.2 and v1.10.3. - Some implementations of the server reflection service have been observed to return multiple (even superfluous) file descriptors, in response to requests made by grpcurl. These extra files, if not returned in a particular order, would cause grpcurl to report an error that the service or method to be invoked could be not be resolved. The reflection client in grpcurl is now more robust to this condition and can handle responses with file descriptors in any order, so it should be interoperable with a larger variety of servers. - When a request message includes a field of type google.protobuf.Value and a value for that field that was a JSON array, grpcurl would incorrectly interpret the JSON array as if it were a single atomic value, the last value that was in the array. This has been fixed. - When a response message includes non-printable characters or code points outside the ASCII 7-bit range in the name of a field in a JSON object, it could be improperly encoded with escape characters that are not valid JSON. Standard tools/libraries could then fail to parse the JSON output from grpcurl. This has been fixed. Signed-off-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>
Fixes the following issue: