Skip to content

Commit 7332a2d

Browse files
josephschorrjosephschorr
authored
Merge pull request #2532 from josephschorr/improve-schema-diff-errors
Improve the errors returned from schema changes
2 parents c6760eb + 5868b49 commit 7332a2d

File tree

7 files changed

+81
-133
lines changed

7 files changed

+81
-133
lines changed

‎internal/services/shared/errors.go‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -41,15 +41,17 @@ func mustMakeStatusReadonly() error {
4141

4242
// NewSchemaWriteDataValidationError creates a new error representing that a schema write cannot be
4343
// completed due to existing data that would be left unreferenced.
44-
func NewSchemaWriteDataValidationError(message string, args ...any) SchemaWriteDataValidationError {
44+
func NewSchemaWriteDataValidationError(message string, args []any, metadata map[string]string) SchemaWriteDataValidationError {
4545
return SchemaWriteDataValidationError{
46-
error: fmt.Errorf(message, args...),
46+
error: fmt.Errorf(message, args...),
47+
metadata: metadata,
4748
}
4849
}
4950

5051
// SchemaWriteDataValidationError occurs when a schema cannot be applied due to leaving data unreferenced.
5152
type SchemaWriteDataValidationError struct {
5253
error
54+
metadata map[string]string
5355
}
5456

5557
// MarshalZerologObject implements zerolog object marshalling.
@@ -59,12 +61,15 @@ func (err SchemaWriteDataValidationError) MarshalZerologObject(e *zerolog.Event)
5961

6062
// GRPCStatus implements retrieving the gRPC status for the error.
6163
func (err SchemaWriteDataValidationError) GRPCStatus() *status.Status {
64+
if err.metadata == nil {
65+
err.metadata = map[string]string{}
66+
}
6267
return spiceerrors.WithCodeAndDetails(
6368
err,
6469
codes.InvalidArgument,
6570
spiceerrors.ForReason(
6671
v1.ErrorReason_ERROR_REASON_SCHEMA_TYPE_ERROR,
67-
map[string]string{},
72+
err.metadata,
6873
),
6974
)
7075
}

‎internal/services/shared/schema.go‎

Lines changed: 57 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package shared
22

33
import (
44
"context"
5+
"maps"
56

67
log "github.com/authzed/spicedb/internal/logging"
78
"github.com/authzed/spicedb/internal/namespace"
@@ -256,10 +257,18 @@ func sanityCheckCaveatChanges(
256257
for _, delta := range diff.Deltas() {
257258
switch delta.Type {
258259
case caveatdiff.RemovedParameter:
259-
return diff, NewSchemaWriteDataValidationError("cannot remove parameter `%s` on caveat `%s`", delta.ParameterName, caveatDef.Name)
260+
return diff, NewSchemaWriteDataValidationError("cannot remove parameter `%s` on caveat `%s`", []any{delta.ParameterName, caveatDef.Name}, map[string]string{
261+
"caveat_name": caveatDef.Name,
262+
"parameter_name": delta.ParameterName,
263+
"operation": "remove_parameter",
264+
})
260265

261266
case caveatdiff.ParameterTypeChanged:
262-
return diff, NewSchemaWriteDataValidationError("cannot change the type of parameter `%s` on caveat `%s`", delta.ParameterName, caveatDef.Name)
267+
return diff, NewSchemaWriteDataValidationError("cannot change the type of parameter `%s` on caveat `%s`", []any{delta.ParameterName, caveatDef.Name}, map[string]string{
268+
"caveat_name": caveatDef.Name,
269+
"parameter_name": delta.ParameterName,
270+
"operation": "change_parameter_type",
271+
})
263272
}
264273
}
265274

@@ -280,8 +289,12 @@ func ensureNoRelationshipsExistWithResourceType(ctx context.Context, rwt datasto
280289
ctx,
281290
qy,
282291
qyErr,
283-
"cannot delete object definition `%s`, as a relationship exists under it",
284-
namespaceName,
292+
"cannot delete object definition `%s`, as at least one relationship exists under it",
293+
[]any{namespaceName},
294+
map[string]string{
295+
"resource_type": namespaceName,
296+
"operation": "delete_object_definition",
297+
},
285298
)
286299
}
287300

@@ -344,7 +357,14 @@ func sanityCheckNamespaceChanges(
344357
ctx,
345358
qy,
346359
qyErr,
347-
"cannot delete relation `%s` in object definition `%s`, as a relationship exists under it", delta.RelationName, nsdef.Name)
360+
"cannot delete relation `%s` in object definition `%s`, as at least one relationship exists under it",
361+
[]any{delta.RelationName, nsdef.Name},
362+
map[string]string{
363+
"resource_type": nsdef.Name,
364+
"relation": delta.RelationName,
365+
"operation": "delete_relation",
366+
},
367+
)
348368
if err != nil {
349369
return diff, err
350370
}
@@ -364,7 +384,14 @@ func sanityCheckNamespaceChanges(
364384
ctx,
365385
qy,
366386
qyErr,
367-
"cannot delete relation `%s` in object definition `%s`, as a relationship references it", delta.RelationName, nsdef.Name)
387+
"cannot delete relation `%s` in object definition `%s`, as at least one relationship references it",
388+
[]any{delta.RelationName, nsdef.Name},
389+
map[string]string{
390+
"resource_type": nsdef.Name,
391+
"relation": delta.RelationName,
392+
"operation": "delete_relation_reverse_check",
393+
},
394+
)
368395
if err != nil {
369396
return diff, err
370397
}
@@ -410,7 +437,14 @@ func sanityCheckNamespaceChanges(
410437
qyr,
411438
qyrErr,
412439
"cannot remove allowed type `%s` from relation `%s` in object definition `%s`, as a relationship exists with it",
413-
schema.SourceForAllowedRelation(delta.AllowedType), delta.RelationName, nsdef.Name)
440+
[]any{schema.SourceForAllowedRelation(delta.AllowedType), delta.RelationName, nsdef.Name},
441+
map[string]string{
442+
"resource_type": nsdef.Name,
443+
"relation": delta.RelationName,
444+
"allowed_type": schema.SourceForAllowedRelation(delta.AllowedType),
445+
"operation": "remove_allowed_type",
446+
},
447+
)
414448
if err != nil {
415449
return diff, err
416450
}
@@ -430,16 +464,29 @@ func subjectRelationFilterForAllowedType(allowedType *core.AllowedRelation) data
430464

431465
// errorIfTupleIteratorReturnsTuples takes a tuple iterator and any error that was generated
432466
// when the original iterator was created, and returns an error if iterator contains any tuples.
433-
func errorIfTupleIteratorReturnsTuples(_ context.Context, qy datastore.RelationshipIterator, qyErr error, message string, args ...any) error {
467+
func errorIfTupleIteratorReturnsTuples(_ context.Context, qy datastore.RelationshipIterator, qyErr error, message string, args []any, metadata map[string]string) error {
434468
if qyErr != nil {
435469
return qyErr
436470
}
437471

438-
for _, err := range qy {
472+
for rel, err := range qy {
439473
if err != nil {
440474
return err
441475
}
442-
return NewSchemaWriteDataValidationError(message, args...)
476+
477+
strValue, err := tuple.String(rel)
478+
if err != nil {
479+
return err
480+
}
481+
482+
// Create metadata with relationship information
483+
fullMetadata := maps.Clone(metadata)
484+
if fullMetadata == nil {
485+
fullMetadata = make(map[string]string)
486+
}
487+
fullMetadata["relationship"] = strValue
488+
newArgs := append(args, strValue)
489+
return NewSchemaWriteDataValidationError(message+": %s", newArgs, fullMetadata)
443490
}
444491

445492
return nil

‎internal/services/shared/schema_test.go‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ func TestApplySchemaChanges(t *testing.T) {
8888
}
8989
9090
definition document {}`,
91-
expectedError: "cannot delete relation `viewer` in object definition `document`, as a relationship exists under it",
91+
expectedError: "cannot delete relation `viewer` in object definition `document`, as at least one relationship exists under it: document:somedoc#viewer@user:alice",
9292
},
9393
{
9494
name: "attempt to remove a relation with indirect relationships",
@@ -119,7 +119,7 @@ func TestApplySchemaChanges(t *testing.T) {
119119
}
120120
121121
definition document {}`,
122-
expectedError: "cannot delete relation `viewer` in object definition `document`, as a relationship exists under it",
122+
expectedError: "cannot delete relation `viewer` in object definition `document`, as at least one relationship exists under it: document:somedoc#viewer@group:somegroup#member",
123123
},
124124
{
125125
name: "attempt to remove a relation with other indirect relationships",
@@ -150,7 +150,7 @@ func TestApplySchemaChanges(t *testing.T) {
150150
}
151151
152152
definition document {}`,
153-
expectedError: "cannot delete relation `viewer` in object definition `document`, as a relationship exists under it",
153+
expectedError: "cannot delete relation `viewer` in object definition `document`, as at least one relationship exists under it: document:somedoc#viewer@org:someorg#admin",
154154
},
155155
{
156156
name: "attempt to remove a relation with wildcard",
@@ -165,7 +165,7 @@ func TestApplySchemaChanges(t *testing.T) {
165165
definition user {}
166166
167167
definition document {}`,
168-
expectedError: "cannot delete relation `viewer` in object definition `document`, as a relationship exists under it",
168+
expectedError: "cannot delete relation `viewer` in object definition `document`, as at least one relationship exists under it: document:somedoc#viewer@user:*",
169169
},
170170
{
171171
name: "attempt to remove a relation with only indirect relationships",
@@ -196,7 +196,7 @@ func TestApplySchemaChanges(t *testing.T) {
196196
}
197197
198198
definition document {}`,
199-
expectedError: "cannot delete relation `viewer` in object definition `document`, as a relationship exists under it",
199+
expectedError: "cannot delete relation `viewer` in object definition `document`, as at least one relationship exists under it: document:somedoc#viewer@org:someorg#admin",
200200
},
201201
{
202202
name: "remove a relation with no relationships",
@@ -287,7 +287,7 @@ func TestApplySchemaChanges(t *testing.T) {
287287
permission view = viewer
288288
}
289289
`,
290-
expectedError: "cannot remove allowed type `group#member` from relation `viewer` in object definition `document`, as a relationship exists with it",
290+
expectedError: "cannot remove allowed type `group#member` from relation `viewer` in object definition `document`, as a relationship exists with it: document:somedoc#viewer@group:somegroup#member",
291291
},
292292
{
293293
name: "attempt to remove non-caveated type when only caveated relationship exists",
@@ -361,7 +361,7 @@ func TestApplySchemaChanges(t *testing.T) {
361361
permission view = nil
362362
}
363363
`,
364-
expectedError: "cannot delete relation `reader` in object definition `document`, as a relationship exists under it",
364+
expectedError: "cannot delete relation `reader` in object definition `document`, as at least one relationship exists under it: document:firstdoc#reader@user:tom",
365365
},
366366
{
367367
name: "delete a subject type with relation but no data",
@@ -403,7 +403,7 @@ func TestApplySchemaChanges(t *testing.T) {
403403
permission view = reader
404404
}
405405
`,
406-
expectedError: "cannot remove allowed type `user` from relation `reader` in object definition `document`, as a relationship exists with it",
406+
expectedError: "cannot remove allowed type `user` from relation `reader` in object definition `document`, as a relationship exists with it: document:firstdoc#reader@user:tom",
407407
},
408408
{
409409
name: "delete a subject type while adding a replacement",
@@ -480,7 +480,7 @@ func TestApplySchemaChanges(t *testing.T) {
480480
permission view = reader
481481
}
482482
`,
483-
expectedError: "cannot remove allowed type `user` from relation `reader` in object definition `document`, as a relationship exists with it",
483+
expectedError: "cannot remove allowed type `user` from relation `reader` in object definition `document`, as a relationship exists with it: document:firstdoc#reader@user:tom",
484484
},
485485
{
486486
name: "attempt to delete an indirect subject type while direct remains",
@@ -505,7 +505,7 @@ func TestApplySchemaChanges(t *testing.T) {
505505
permission view = reader
506506
}
507507
`,
508-
expectedError: "cannot remove allowed type `user#foo` from relation `reader` in object definition `document`, as a relationship exists with it",
508+
expectedError: "cannot remove allowed type `user#foo` from relation `reader` in object definition `document`, as a relationship exists with it: document:firstdoc#reader@user:tom#foo",
509509
},
510510
{
511511
name: "delete an indirect subject type while direct remains",

‎internal/services/steelthreadtesting/steelresults/write-schema-removal-attempt-to-remove-the-user-relation-results.yaml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,2 @@
11
---
2-
error: 'failed to write schema: rpc error: code = InvalidArgument desc = cannot remove allowed type `user` from relation `viewer` in object definition `document`, as a relationship exists with it'
2+
error: 'failed to write schema: rpc error: code = InvalidArgument desc = cannot remove allowed type `user` from relation `viewer` in object definition `document`, as a relationship exists with it: document:somedoc#viewer@user:user-0'

‎internal/services/steelthreadtesting/steelresults/write-schema-removal-attempt-to-remove-the-wildcard-on-the-editor-results.yaml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,2 @@
11
---
2-
error: 'failed to write schema: rpc error: code = InvalidArgument desc = cannot remove allowed type `user2:*` from relation `editor` in object definition `document`, as a relationship exists with it'
2+
error: 'failed to write schema: rpc error: code = InvalidArgument desc = cannot remove allowed type `user2:*` from relation `editor` in object definition `document`, as a relationship exists with it: document:somedoc#editor@user2:*'

‎internal/services/steelthreadtesting/testdata/basic-schema-and-data.yaml‎

Lines changed: 0 additions & 104 deletions
Original file line numberDiff line numberDiff line change
@@ -16,109 +16,5 @@ schema: |+
1616
}
1717
1818
relationships: |
19-
// A single user2:* on a document.
2019
document:somedoc#editor@user2:*
21-
22-
// 100 direct users
23-
// for userid in range(0, 100)
24-
// document:somedoc#viewer@user:user-{userid}
2520
document:somedoc#viewer@user:user-0
26-
document:somedoc#viewer@user:user-1
27-
document:somedoc#viewer@user:user-2
28-
document:somedoc#viewer@user:user-3
29-
document:somedoc#viewer@user:user-4
30-
document:somedoc#viewer@user:user-5
31-
document:somedoc#viewer@user:user-6
32-
document:somedoc#viewer@user:user-7
33-
document:somedoc#viewer@user:user-8
34-
document:somedoc#viewer@user:user-9
35-
document:somedoc#viewer@user:user-10
36-
document:somedoc#viewer@user:user-11
37-
document:somedoc#viewer@user:user-12
38-
document:somedoc#viewer@user:user-13
39-
document:somedoc#viewer@user:user-14
40-
document:somedoc#viewer@user:user-15
41-
document:somedoc#viewer@user:user-16
42-
document:somedoc#viewer@user:user-17
43-
document:somedoc#viewer@user:user-18
44-
document:somedoc#viewer@user:user-19
45-
document:somedoc#viewer@user:user-20
46-
document:somedoc#viewer@user:user-21
47-
document:somedoc#viewer@user:user-22
48-
document:somedoc#viewer@user:user-23
49-
document:somedoc#viewer@user:user-24
50-
document:somedoc#viewer@user:user-25
51-
document:somedoc#viewer@user:user-26
52-
document:somedoc#viewer@user:user-27
53-
document:somedoc#viewer@user:user-28
54-
document:somedoc#viewer@user:user-29
55-
document:somedoc#viewer@user:user-30
56-
document:somedoc#viewer@user:user-31
57-
document:somedoc#viewer@user:user-32
58-
document:somedoc#viewer@user:user-33
59-
document:somedoc#viewer@user:user-34
60-
document:somedoc#viewer@user:user-35
61-
document:somedoc#viewer@user:user-36
62-
document:somedoc#viewer@user:user-37
63-
document:somedoc#viewer@user:user-38
64-
document:somedoc#viewer@user:user-39
65-
document:somedoc#viewer@user:user-40
66-
document:somedoc#viewer@user:user-41
67-
document:somedoc#viewer@user:user-42
68-
document:somedoc#viewer@user:user-43
69-
document:somedoc#viewer@user:user-44
70-
document:somedoc#viewer@user:user-45
71-
document:somedoc#viewer@user:user-46
72-
document:somedoc#viewer@user:user-47
73-
document:somedoc#viewer@user:user-48
74-
document:somedoc#viewer@user:user-49
75-
document:somedoc#viewer@user:user-50
76-
document:somedoc#viewer@user:user-51
77-
document:somedoc#viewer@user:user-52
78-
document:somedoc#viewer@user:user-53
79-
document:somedoc#viewer@user:user-54
80-
document:somedoc#viewer@user:user-55
81-
document:somedoc#viewer@user:user-56
82-
document:somedoc#viewer@user:user-57
83-
document:somedoc#viewer@user:user-58
84-
document:somedoc#viewer@user:user-59
85-
document:somedoc#viewer@user:user-60
86-
document:somedoc#viewer@user:user-61
87-
document:somedoc#viewer@user:user-62
88-
document:somedoc#viewer@user:user-63
89-
document:somedoc#viewer@user:user-64
90-
document:somedoc#viewer@user:user-65
91-
document:somedoc#viewer@user:user-66
92-
document:somedoc#viewer@user:user-67
93-
document:somedoc#viewer@user:user-68
94-
document:somedoc#viewer@user:user-69
95-
document:somedoc#viewer@user:user-70
96-
document:somedoc#viewer@user:user-71
97-
document:somedoc#viewer@user:user-72
98-
document:somedoc#viewer@user:user-73
99-
document:somedoc#viewer@user:user-74
100-
document:somedoc#viewer@user:user-75
101-
document:somedoc#viewer@user:user-76
102-
document:somedoc#viewer@user:user-77
103-
document:somedoc#viewer@user:user-78
104-
document:somedoc#viewer@user:user-79
105-
document:somedoc#viewer@user:user-80
106-
document:somedoc#viewer@user:user-81
107-
document:somedoc#viewer@user:user-82
108-
document:somedoc#viewer@user:user-83
109-
document:somedoc#viewer@user:user-84
110-
document:somedoc#viewer@user:user-85
111-
document:somedoc#viewer@user:user-86
112-
document:somedoc#viewer@user:user-87
113-
document:somedoc#viewer@user:user-88
114-
document:somedoc#viewer@user:user-89
115-
document:somedoc#viewer@user:user-90
116-
document:somedoc#viewer@user:user-91
117-
document:somedoc#viewer@user:user-92
118-
document:somedoc#viewer@user:user-93
119-
document:somedoc#viewer@user:user-94
120-
document:somedoc#viewer@user:user-95
121-
document:somedoc#viewer@user:user-96
122-
document:somedoc#viewer@user:user-97
123-
document:somedoc#viewer@user:user-98
124-
document:somedoc#viewer@user:user-99

0 commit comments

Comments
 (0)