Skip to content

Commit b59dc8c

Browse files
miparnisarimiparnisari
authored
fix: properly rewrite errors for watch api (#2640)
1 parent a79aceb commit b59dc8c

File tree

3 files changed

+231
-46
lines changed

3 files changed

+231
-46
lines changed

‎internal/services/shared/errors.go‎

Lines changed: 7 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -14,13 +14,11 @@ import (
1414
v1 "github.com/authzed/authzed-go/proto/authzed/api/v1"
1515

1616
"github.com/authzed/spicedb/internal/datastore/crdb/pool"
17-
"github.com/authzed/spicedb/internal/dispatch"
1817
"github.com/authzed/spicedb/internal/graph"
1918
log "github.com/authzed/spicedb/internal/logging"
2019
"github.com/authzed/spicedb/internal/sharederrors"
2120
"github.com/authzed/spicedb/pkg/cursor"
2221
"github.com/authzed/spicedb/pkg/datastore"
23-
dispatchv1 "github.com/authzed/spicedb/pkg/proto/dispatch/v1"
2422
"github.com/authzed/spicedb/pkg/schema"
2523
"github.com/authzed/spicedb/pkg/schemadsl/compiler"
2624
"github.com/authzed/spicedb/pkg/spiceerrors"
@@ -148,9 +146,8 @@ func rewriteError(ctx context.Context, err error, config *ConfigForErrors) error
148146
var relationNotFoundError sharederrors.UnknownRelationError
149147

150148
var compilerError compiler.BaseCompilerError
151-
var sourceError spiceerrors.WithSourceError
149+
var sourceError *spiceerrors.WithSourceError
152150
var typeError schema.TypeError
153-
var maxDepthError dispatch.MaxDepthExceededError
154151

155152
switch {
156153
case errors.As(err, &typeError):
@@ -168,14 +165,6 @@ func rewriteError(ctx context.Context, err error, config *ConfigForErrors) error
168165
case errors.As(err, &relationNotFoundError):
169166
return spiceerrors.WithCodeAndReason(err, codes.FailedPrecondition, v1.ErrorReason_ERROR_REASON_UNKNOWN_RELATION_OR_PERMISSION)
170167

171-
case errors.As(err, &maxDepthError):
172-
if config == nil {
173-
return spiceerrors.MustBugf("missing config for API error")
174-
}
175-
176-
_, isCheckRequest := maxDepthError.Request.(*dispatchv1.DispatchCheckRequest)
177-
return NewMaxDepthExceededError(config.MaximumAPIDepth, isCheckRequest)
178-
179168
case errors.As(err, &datastore.ReadOnlyError{}):
180169
return ErrServiceReadOnly
181170
case errors.As(err, &datastore.InvalidRevisionError{}):
@@ -184,6 +173,10 @@ func rewriteError(ctx context.Context, err error, config *ConfigForErrors) error
184173
return spiceerrors.WithCodeAndReason(err, codes.FailedPrecondition, v1.ErrorReason_ERROR_REASON_UNKNOWN_CAVEAT)
185174
case errors.As(err, &datastore.WatchDisabledError{}):
186175
return status.Errorf(codes.FailedPrecondition, "%s", err)
176+
case errors.As(err, &datastore.WatchCanceledError{}):
177+
return status.Errorf(codes.Canceled, "watch canceled by user: %s", err)
178+
case errors.As(err, &datastore.WatchDisconnectedError{}):
179+
return status.Errorf(codes.ResourceExhausted, "watch disconnected: %s", err)
187180
case errors.As(err, &datastore.CounterAlreadyRegisteredError{}):
188181
return spiceerrors.WithCodeAndReason(err, codes.FailedPrecondition, v1.ErrorReason_ERROR_REASON_COUNTER_ALREADY_REGISTERED)
189182
case errors.As(err, &datastore.CounterNotRegisteredError{}):
@@ -206,9 +199,10 @@ func rewriteError(ctx context.Context, err error, config *ConfigForErrors) error
206199
if _, ok := status.FromError(err); ok {
207200
return err
208201
}
202+
return status.Errorf(codes.Canceled, "%s", err)
209203
}
210204

211-
return status.Errorf(codes.Canceled, "%s", err)
205+
return status.Errorf(codes.Canceled, "context canceled")
212206
default:
213207
log.Ctx(ctx).Err(err).Msg("received unexpected error")
214208
return err

‎internal/services/shared/errors_test.go‎

Lines changed: 223 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -2,42 +2,241 @@ package shared
22

33
import (
44
"context"
5+
"fmt"
56
"testing"
6-
"time"
77

88
"github.com/stretchr/testify/require"
99
"google.golang.org/grpc/codes"
10+
"google.golang.org/grpc/status"
1011

1112
"github.com/authzed/grpcutil"
1213

1314
"github.com/authzed/spicedb/internal/datastore/crdb/pool"
1415
"github.com/authzed/spicedb/internal/dispatch"
16+
"github.com/authzed/spicedb/internal/graph"
17+
"github.com/authzed/spicedb/internal/namespace"
18+
"github.com/authzed/spicedb/pkg/cursor"
19+
"github.com/authzed/spicedb/pkg/datastore"
20+
corev1 "github.com/authzed/spicedb/pkg/proto/core/v1"
21+
"github.com/authzed/spicedb/pkg/spiceerrors"
1522
)
1623

17-
func TestRewriteCanceledError(t *testing.T) {
18-
ctx, cancelFunc := context.WithCancel(t.Context())
19-
cancelFunc()
20-
errorRewritten := RewriteError(ctx, ctx.Err(), nil)
21-
grpcutil.RequireStatus(t, codes.Canceled, errorRewritten)
22-
}
24+
func TestRewriteError(t *testing.T) {
25+
t.Parallel()
2326

24-
func TestRewriteDeadlineExceededError(t *testing.T) {
25-
ctx, cancelFunc := context.WithDeadline(t.Context(), time.Now())
26-
defer cancelFunc()
27-
errorRewritten := RewriteError(ctx, ctx.Err(), nil)
28-
grpcutil.RequireStatus(t, codes.DeadlineExceeded, errorRewritten)
29-
}
27+
ctxCancelWithCause, cancel := context.WithCancelCause(t.Context())
28+
cancel(fmt.Errorf("look at me i canceled this"))
3029

31-
func TestRewriteMaximumDepthExceededError(t *testing.T) {
32-
errorRewritten := RewriteError(t.Context(), dispatch.NewMaxDepthExceededError(nil), &ConfigForErrors{
33-
MaximumAPIDepth: 50,
34-
})
35-
require.ErrorContains(t, errorRewritten, "See: https://spicedb.dev/d/debug-max-depth")
36-
grpcutil.RequireStatus(t, codes.ResourceExhausted, errorRewritten)
37-
}
30+
ctxCancelWithCauseGrpc, cancel2 := context.WithCancelCause(t.Context())
31+
cancel2(status.Error(codes.AlreadyExists, "already exists"))
32+
33+
tests := []struct {
34+
name string
35+
inputCtx context.Context
36+
inputError error
37+
config *ConfigForErrors
38+
expectedCode codes.Code
39+
expectedContains string
40+
}{
41+
// context
42+
{
43+
name: "context canceled without cause",
44+
inputError: context.Canceled,
45+
config: nil,
46+
expectedCode: codes.Canceled,
47+
expectedContains: "context canceled",
48+
},
49+
{
50+
name: "context canceled with cause",
51+
inputCtx: ctxCancelWithCause,
52+
inputError: context.Canceled,
53+
config: nil,
54+
expectedCode: codes.Canceled,
55+
expectedContains: "look at me i canceled this",
56+
},
57+
{
58+
name: "context canceled with cause and embedded grpc error",
59+
inputCtx: ctxCancelWithCauseGrpc,
60+
inputError: context.Canceled,
61+
config: nil,
62+
expectedCode: codes.AlreadyExists,
63+
expectedContains: "already exists",
64+
},
65+
{
66+
name: "context deadline exceeded error",
67+
inputError: context.DeadlineExceeded,
68+
config: nil,
69+
expectedCode: codes.DeadlineExceeded,
70+
expectedContains: "context deadline exceeded",
71+
},
72+
// dispatch
73+
{
74+
name: "maximum depth exceeded error",
75+
inputError: dispatch.NewMaxDepthExceededError(nil),
76+
config: &ConfigForErrors{
77+
MaximumAPIDepth: 50,
78+
},
79+
expectedCode: codes.ResourceExhausted,
80+
expectedContains: "max depth exceeded: this usually indicates a recursive or too deep data dependency. See: https://spicedb.dev/d/debug-max-depth",
81+
},
82+
// pool
83+
{
84+
name: "error acquiring connections",
85+
inputError: pool.ErrAcquire,
86+
config: nil,
87+
expectedCode: codes.ResourceExhausted,
88+
expectedContains: "consider increasing write pool size and/or datastore capacity",
89+
},
90+
{
91+
name: "acquisition error",
92+
inputError: pool.ErrAcquire,
93+
config: nil,
94+
expectedCode: codes.ResourceExhausted,
95+
expectedContains: "failed to acquire in time: consider increasing write pool size and/or datastore capacity",
96+
},
97+
// datastore
98+
{
99+
name: "watch disconnected",
100+
inputError: datastore.NewWatchDisconnectedErr(),
101+
config: nil,
102+
expectedCode: codes.ResourceExhausted,
103+
expectedContains: "watch fell too far behind and was disconnected; consider increasing watch buffer size via the flag --datastore-watch-buffer-length",
104+
},
105+
{
106+
name: "watch canceled",
107+
inputError: datastore.NewWatchCanceledErr(),
108+
config: nil,
109+
expectedCode: codes.Canceled,
110+
expectedContains: "watch canceled by user: watch was canceled by the caller",
111+
},
112+
{
113+
name: "watch disabled",
114+
inputError: datastore.NewWatchDisabledErr("it was disabled"),
115+
config: nil,
116+
expectedCode: codes.FailedPrecondition,
117+
expectedContains: "watch is currently disabled: it was disabled",
118+
},
119+
{
120+
name: "counter already registered",
121+
inputError: datastore.NewCounterAlreadyRegisteredErr("somecounter", &corev1.RelationshipFilter{ResourceType: "doc"}),
122+
config: nil,
123+
expectedCode: codes.FailedPrecondition,
124+
expectedContains: "counter with name `somecounter` already registered",
125+
},
126+
{
127+
name: "counter not registered",
128+
inputError: datastore.NewCounterNotRegisteredErr("somecounter"),
129+
config: nil,
130+
expectedCode: codes.FailedPrecondition,
131+
expectedContains: "counter with name `somecounter` not found",
132+
},
133+
{
134+
name: "datastore readonly",
135+
inputError: datastore.NewReadonlyErr(),
136+
config: nil,
137+
expectedCode: codes.Unavailable,
138+
expectedContains: "service read-only",
139+
},
140+
{
141+
name: "datastore invalid revision",
142+
inputError: datastore.NewInvalidRevisionErr(datastore.NoRevision, datastore.RevisionStale),
143+
config: nil,
144+
expectedCode: codes.OutOfRange,
145+
expectedContains: "invalid zedtoken",
146+
},
147+
{
148+
name: "datastore caveat not found",
149+
inputError: datastore.NewCaveatNameNotFoundErr("somecaveat"),
150+
config: nil,
151+
expectedCode: codes.FailedPrecondition,
152+
expectedContains: "caveat with name `somecaveat` not found",
153+
},
154+
// graph
155+
{
156+
name: "graph unimplemented",
157+
inputError: graph.NewUnimplementedErr(fmt.Errorf("unimplemented")),
158+
config: nil,
159+
expectedCode: codes.Unimplemented,
160+
expectedContains: "unimplemented",
161+
},
162+
{
163+
name: "graph always fail",
164+
inputError: graph.NewAlwaysFailErr(),
165+
config: nil,
166+
expectedCode: codes.Internal,
167+
expectedContains: "internal error: always fail",
168+
},
169+
{
170+
name: "graph missing type info",
171+
inputError: graph.NewRelationMissingTypeInfoErr("document", "view"),
172+
config: nil,
173+
expectedCode: codes.FailedPrecondition,
174+
expectedContains: "failed precondition: relation/permission `view` under definition `document` is missing type information",
175+
},
176+
//{ TODO this doesn't pass
177+
// name: "compiler error",
178+
// inputError: &compiler.WithContextError{
179+
// BaseCompilerError: compiler.BaseCompilerError{
180+
// BaseMessage: "syntax error in schema",
181+
// },
182+
// },
183+
// config: nil,
184+
// expectedCode: codes.InvalidArgument,
185+
// expectedContains: "syntax error in schema",
186+
//},
187+
{
188+
name: "source error",
189+
inputError: spiceerrors.NewWithSourceError(
190+
fmt.Errorf("invalid schema definition"),
191+
"definition document {\n relation viewer: user\n}",
192+
1,
193+
1,
194+
),
195+
config: nil,
196+
expectedCode: codes.InvalidArgument,
197+
expectedContains: "invalid schema definition",
198+
},
199+
// cursor
200+
{
201+
name: "cursor hash mismatch",
202+
inputError: cursor.ErrHashMismatch,
203+
config: nil,
204+
expectedCode: codes.FailedPrecondition,
205+
expectedContains: "the cursor provided does not have the same arguments as the original API call",
206+
},
207+
// schema errors
208+
{
209+
name: "namespace not found",
210+
inputError: namespace.NewNamespaceNotFoundErr("document"),
211+
config: nil,
212+
expectedCode: codes.FailedPrecondition,
213+
expectedContains: "object definition `document` not found",
214+
},
215+
{
216+
name: "relation not found",
217+
inputError: namespace.NewRelationNotFoundErr("document", "viewer"),
218+
config: nil,
219+
expectedCode: codes.FailedPrecondition,
220+
expectedContains: "relation/permission `viewer` not found under definition `document`",
221+
},
222+
}
223+
224+
for _, tt := range tests {
225+
t.Run(tt.name, func(t *testing.T) {
226+
t.Parallel()
227+
228+
ctx := t.Context()
229+
if tt.inputCtx != nil {
230+
ctx = tt.inputCtx
231+
}
38232

39-
func TestRewriteAcquisitionError(t *testing.T) {
40-
errorRewritten := RewriteError(t.Context(), pool.ErrAcquire, nil)
41-
require.ErrorContains(t, errorRewritten, "failed to acquire in time: consider increasing write pool size and/or datastore capacity")
42-
grpcutil.RequireStatus(t, codes.ResourceExhausted, errorRewritten)
233+
errorRewritten := RewriteError(ctx, tt.inputError, tt.config)
234+
require.NotNil(t, errorRewritten)
235+
grpcutil.RequireStatus(t, tt.expectedCode, errorRewritten)
236+
t.Log(errorRewritten.Error())
237+
if tt.expectedContains != "" {
238+
require.ErrorContains(t, errorRewritten, tt.expectedContains)
239+
}
240+
})
241+
}
43242
}

‎internal/services/v1/watch.go‎

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ package v1
22

33
import (
44
"context"
5-
"errors"
65
"slices"
76
"time"
87

@@ -147,14 +146,7 @@ func (ws *watchServer) Watch(req *v1.WatchRequest, stream v1.WatchService_WatchS
147146
}
148147
}
149148
case err := <-errchan:
150-
switch {
151-
case errors.As(err, &datastore.WatchCanceledError{}):
152-
return status.Errorf(codes.Canceled, "watch canceled by user: %s", err)
153-
case errors.As(err, &datastore.WatchDisconnectedError{}):
154-
return status.Errorf(codes.ResourceExhausted, "watch disconnected: %s", err)
155-
default:
156-
return status.Errorf(codes.Internal, "watch error: %s", err)
157-
}
149+
return ws.rewriteError(ctx, err)
158150
}
159151
}
160152
}

0 commit comments

Comments
 (0)