refactor(spanner): return APIError as wrapped error in spanner.Error struct #5169
Conversation
rahul2393
commented
Nov 26, 2021
rahul2393
commented
- Wrapping APIError in spanner.Error struct for making the error struct consistent across all GAPIC libraries
rahul2393
requested review from
hengfengli,
skuruppu and
a team
as code owners
google-cla
bot
added
the
cla: yes
spanner/errors.go
Outdated
| "google.golang.org/genproto/googleapis/rpc/errdetails" | ||
| "google.golang.org/grpc/codes" | ||
| "google.golang.org/grpc/status" | ||
| ) | ||
|
|
||
| // Error is the structured error returned by Cloud Spanner client. | ||
| // | ||
| // Deprecated: The APIError should be extracted from the wrapped error by |
There was a problem hiding this comment.
I think this deprecation warning could use more elaborate instructions on exactly what users should do. The end goal is that the client should return an APIError, and we want to instruct the users that they should also use APIError to get the details from the error that is returned by the client. The safest way that users can do that (at least that I can think of) would be the following:
// Unwrap any error that is returned by the Spanner client as an APIError
// to access the error details. Do not try to convert the error to the
// spanner.Error struct, as that struct may be removed in a future release.
//
// Example:
// var apiErr *apierror.APIError
// _, err := spanner.NewClient(context.Background())
// errors.As(err, &apiErr)
This example code will work both when the client returns instances of spanner.Error and when it starts to return apierror.APIError instances.
It might also be good to add a test to the client that verifies the above, so that we don't accidentally break this in the future.
| @@ -20,12 +20,18 @@ import ( | |||
| "context" | |||
| "fmt" | |||
|
|
|||
| "github.com/googleapis/gax-go/v2/apierror" | |||
There was a problem hiding this comment.
nit: Move this import to the import block below. Normally, the import section of a Go file should only have two sections: One for the internal Go libraries, and one for all the other libraries that we are importing.
spanner/errors.go
Outdated
| @@ -104,10 +110,13 @@ func (e *Error) decorate(info string) { | |||
| } | |||
|
|
|||
| // spannerErrorf generates a *spanner.Error with the given description and a | |||
There was a problem hiding this comment.
nit:
| // spannerErrorf generates a *spanner.Error with the given description and a | |
| // spannerErrorf generates a *spanner.Error with the given description and an |
spanner/errors.go
Outdated
| func spannerErrorf(code codes.Code, format string, args ...interface{}) error { | ||
| msg := fmt.Sprintf(format, args...) | ||
| wrapped := status.Error(code, msg) | ||
| if apiError, ok := apierror.FromError(wrapped); ok { |
There was a problem hiding this comment.
This check should really never fail, and if it would fail, we would start returning errors that are no longer in accordance with the documentation. We should therefore have a test that verifies that this does not fail, so we do not accidentally release a version that is not able to create an APIError from a status.Error.
Put another way: I think we should remove the if statement and just assume that it was ok, and have a test case that checks that the error that is returned from spannerErrorf always wraps an APIError.
spanner/errors.go
Outdated
| // errors for use in tests. The recommended way to create test errors is | ||
| // calling this method with a status error, e.g. | ||
| // ToSpannerError(status.New(codes.NotFound, "Table not found").Err()) | ||
| func ToSpannerError(err error) error { | ||
| return toSpannerErrorWithCommitInfo(err, false) | ||
| } | ||
|
|
||
| // ToAPIError converts a general Go error to *gax-go.APIError. If the given | ||
| // error is not convertible to *gax-go.APIError, the original error will be returned. | ||
| func ToAPIError(err error) error { |
There was a problem hiding this comment.
nit: Do not export this method.
There was a problem hiding this comment.
LGTM, but I am not Go expert. Suggest leave @noahdietz to take another look. :)
|
@noahdietz Can you please take a quick look at this PR? Thanks. |
