Skip to content

Commit

Permalink
fixes CRDB datastore not reporting proper GRPC codes
Browse files Browse the repository at this point in the history
- duplicate relationship now reports gRPC code AlreadyExists
- retryable errors now return grpc code Unavailable
  • Loading branch information
vroldanbet committed Dec 19, 2023
1 parent 1bd9932 commit 302cd04
Show file tree
Hide file tree
Showing 3 changed files with 54 additions and 1 deletion.
11 changes: 10 additions & 1 deletion internal/datastore/crdb/crdb.go
Original file line number Diff line number Diff line change
Expand Up @@ -336,12 +336,21 @@ func (cds *crdbDatastore) ReadWriteTx(
return nil
})
if err != nil {
return datastore.NoRevision, err
return datastore.NoRevision, wrapError(err)
}

return commitTimestamp, nil
}

func wrapError(err error) error {
// If a unique constraint violation is returned, then its likely that the cause
// was an existing relationship.
if cerr := pgxcommon.ConvertToWriteConstraintError(livingTupleConstraint, err); cerr != nil {
return cerr
}
return err
}

func (cds *crdbDatastore) ReadyState(ctx context.Context) (datastore.ReadyState, error) {
headMigration, err := migrations.CRDBMigrations.HeadRevision()
if err != nil {
Expand Down
36 changes: 36 additions & 0 deletions internal/datastore/crdb/pool/slqerrors.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,12 @@ import (
"errors"
"fmt"

v1 "github.com/authzed/authzed-go/proto/authzed/api/v1"
"github.com/jackc/pgx/v5/pgconn"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"

"github.com/authzed/spicedb/pkg/spiceerrors"
)

const (
Expand Down Expand Up @@ -35,6 +40,15 @@ func (e *MaxRetryError) Error() string {

func (e *MaxRetryError) Unwrap() error { return e.LastErr }

func (e *MaxRetryError) GRPCStatus() *status.Status {
s, ok := status.FromError(e.Unwrap())
if !ok {
return nil
}

return s
}

// ResettableError is an error that we think may succeed if retried against a new connection.
type ResettableError struct {
Err error
Expand All @@ -43,6 +57,17 @@ type ResettableError struct {
func (e *ResettableError) Error() string { return "resettable error" + ": " + e.Err.Error() }
func (e *ResettableError) Unwrap() error { return e.Err }

func (e *ResettableError) GRPCStatus() *status.Status {
return spiceerrors.WithCodeAndDetails(
e.Unwrap(),
codes.Unavailable, // return unavailable so clients are know it's ok to retry
spiceerrors.ForReason(
v1.ErrorReason_ERROR_REASON_UNSPECIFIED, // FIXME provide a more specific reason
map[string]string{},
),
)
}

// RetryableError is an error that can be retried against the existing connection.
type RetryableError struct {
Err error
Expand All @@ -51,6 +76,17 @@ type RetryableError struct {
func (e *RetryableError) Error() string { return "retryable error" + ": " + e.Err.Error() }
func (e *RetryableError) Unwrap() error { return e.Err }

func (e *RetryableError) GRPCStatus() *status.Status {
return spiceerrors.WithCodeAndDetails(
e.Unwrap(),
codes.Unavailable, // return unavailable so clients are know it's ok to retry
spiceerrors.ForReason(
v1.ErrorReason_ERROR_REASON_UNSPECIFIED, // FIXME provide a more specific reason
map[string]string{},
),
)
}

// sqlErrorCode attempts to extract the crdb error code from the error state.
func sqlErrorCode(err error) string {
var pgerr *pgconn.PgError
Expand Down
8 changes: 8 additions & 0 deletions pkg/datastore/test/tuples.go
Original file line number Diff line number Diff line change
Expand Up @@ -566,6 +566,14 @@ func CreateAlreadyExistingTest(t *testing.T, tester DatastoreTester) {
require.ErrorAs(err, &common.CreateRelationshipExistsError{})
require.Contains(err.Error(), "could not CREATE relationship ")
grpcutil.RequireStatus(t, codes.AlreadyExists, err)

f := func(ctx context.Context, rwt datastore.ReadWriteTransaction) error {
_, err := rwt.BulkLoad(ctx, testfixtures.NewBulkTupleGenerator(testResourceNamespace, testReaderRelation, testUserNamespace, 1, t))
return err
}
_, _ = ds.ReadWriteTx(ctx, f)
_, err = ds.ReadWriteTx(ctx, f)
grpcutil.RequireStatus(t, codes.AlreadyExists, err)
}

// TouchAlreadyExistingTest tests touching a relationship twice.
Expand Down

0 comments on commit 302cd04

Please sign in to comment.