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.
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.
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.
I think the underlying implementation of Proto() does a Clone() so original msg should be unmodified.
dnr
force-pushed
the
david/wrapped-errors
branch
from
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.
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.
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
Converthandle wrappedServiceErrorsWhy?
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.