-
Notifications
You must be signed in to change notification settings - Fork 31
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
Handle wrapped ServiceErrors in Convert #128
Conversation
// This path does more copying so prefer to return a ServiceError directly if possible. | ||
var svcerr ServiceError | ||
if errors.As(err, &svcerr) { | ||
s := svcerr.Status().Proto() |
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.
Just double-checking, this does a copy right? In any case, can we verify in the test that the original message is unmodified?
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.
I'm not sure I would say it does a "copy". It asks the ServiceError to serialize itself into a google.golang.org/genproto/googleapis/rpc/status.Status (proto message). The it replaces one field in the proto message and constructs a google.golang.org/grpc/status.Status from that. Those both involve copying some fields, but we're moving between different types. Specifically, there's only ever one ServiceError.
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.
I think the underlying implementation of Proto() does a Clone() so original msg should be unmodified.
0e6e769
to
aa4be50
Compare
// This path does more copying so prefer to return a ServiceError directly if possible. | ||
var svcerr ServiceError | ||
if errors.As(err, &svcerr) { | ||
s := svcerr.Status().Proto() |
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.
I think the underlying implementation of Proto() does a Clone() so original msg should be unmodified.
// err does not implement ServiceError directly, but check if it wraps it. | ||
// This path does more allocation so prefer to return a ServiceError directly if possible. | ||
var svcerr ServiceError | ||
if errors.As(err, &svcerr) { |
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.
I support this change because this is what https://pkg.go.dev/google.golang.org/grpc/status#FromError does. But we should have never had a ServiceError
IMO because GRPCStatus()
should have been the method and existing gRPC status package should have been good enough. But I guess that ship sailed long ago.
Technically it's a backwards incompatible change, but from an SDK POV there are no obvious user-facing times where this will accidentally return a status where it wouldn't have otherwise because user-facing deserialized error hierarchy only doesn't contain this type at any depth (it's not a failure type).
What changed?
Make
Convert
handle wrappedServiceError
sWhy?
We shouldn't lose any more specific error info (i.e. grpc code) if it's present at all in the error.
How did you test it?
unit test
Potential risks
This code path is a little more expensive, if we end up using it a lot of the time that's a slight waste.