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.

Sign up for GitHub

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

refactor(spanner): return APIError as wrapped error in spanner.Error struct #5169

Merged
merged 8 commits into from
Dec 2, 2021
Merged

refactor(spanner): return APIError as wrapped error in spanner.Error struct #5169

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1deea6d
refactor(spanner): return APIError as wrapped error in spanner.Error …
rahul2393 Nov 26, 2021
1b1a2a8
refactor(spanner): return APIError as wrapped error in spanner.Error …
rahul2393 Nov 29, 2021
75478f0
Merge branch 'main' into return-gax-apierror
rahul2393 Nov 29, 2021
5da2711
Merge branch 'main' into return-gax-apierror
rahul2393 Nov 29, 2021
5de2346
Merge branch 'main' into return-gax-apierror
rahul2393 Nov 30, 2021
dc6450e
Merge branch 'main' into return-gax-apierror
rahul2393 Dec 1, 2021
471cda3
Merge branch 'main' into return-gax-apierror
rahul2393 Dec 2, 2021
c86e6e3
Merge branch 'main' into return-gax-apierror
hengfengli Dec 2, 2021
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
28 changes: 23 additions & 5 deletions spanner/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,18 @@ import (
"context"
"fmt"

"github.com/googleapis/gax-go/v2/apierror"
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


"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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

// calling Unwrap(). This struct will be removed in a future
// release and APIError will be returned directly.
type Error struct {
// Code is the canonical error code for describing the nature of a
// particular error.
Expand Down Expand Up @@ -104,10 +110,13 @@ func (e *Error) decorate(info string) {
}

// spannerErrorf generates a *spanner.Error with the given description and a
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
// spannerErrorf generates a *spanner.Error with the given description and a
// spannerErrorf generates a *spanner.Error with the given description and an

// status error with the given error code as its wrapped error.
// APIError error having given error code as its status.
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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

wrapped = apiError
}
return &Error{
Code: code,
err: wrapped,
Expand All @@ -119,14 +128,23 @@ func spannerErrorf(code codes.Code, format string, args ...interface{}) error {
// error is already a *spanner.Error, the original error will be returned.
//
// Spanner Errors are normally created by the Spanner client library from the
// returned status of a RPC. This method can also be used to create Spanner
// returned APIError of a RPC. This method can also be used to create Spanner
// 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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Do not export this method.

if apiError, ok := apierror.FromError(err); ok {
return apiError
}
return err
}

// toSpannerErrorWithCommitInfo converts general Go error to *spanner.Error
// with additional information if the error occurred during a Commit request.
//
Expand All @@ -147,9 +165,9 @@ func toSpannerErrorWithCommitInfo(err error, errorDuringCommit bool) error {
desc = fmt.Sprintf("%s, %s", desc, transactionOutcomeUnknownMsg)
wrapped = &TransactionOutcomeUnknownError{err: wrapped}
}
return &Error{status.FromContextError(err).Code(), wrapped, desc, ""}
return &Error{status.FromContextError(err).Code(), ToAPIError(wrapped), desc, ""}
case status.Code(err) == codes.Unknown:
return &Error{codes.Unknown, err, err.Error(), ""}
return &Error{codes.Unknown, ToAPIError(err), err.Error(), ""}
default:
statusErr := status.Convert(err)
code, desc := statusErr.Code(), statusErr.Message()
Expand All @@ -158,7 +176,7 @@ func toSpannerErrorWithCommitInfo(err error, errorDuringCommit bool) error {
desc = fmt.Sprintf("%s, %s", desc, transactionOutcomeUnknownMsg)
wrapped = &TransactionOutcomeUnknownError{err: wrapped}
}
return &Error{code, wrapped, desc, ""}
return &Error{code, ToAPIError(wrapped), desc, ""}
}
}

Expand Down