From e8ad9bcefe73e84de40e33c26815824dcfffff48 Mon Sep 17 00:00:00 2001 From: Joseph Schorr Date: Tue, 12 Nov 2024 17:23:22 -0500 Subject: [PATCH] Add subject filters in schema relation delete to force use of the index This should allow for much faster queries under the transaction. We observed some customers reporting timeouts when writting a new schema and dropping a relation because it ended up in full table scans. --- internal/services/shared/schema.go | 40 ++- internal/services/shared/schema_test.go | 366 ++++++++++++++++++++---- 2 files changed, 349 insertions(+), 57 deletions(-) diff --git a/internal/services/shared/schema.go b/internal/services/shared/schema.go index 5841e9d194..f4b7592e16 100644 --- a/internal/services/shared/schema.go +++ b/internal/services/shared/schema.go @@ -318,9 +318,45 @@ func sanityCheckNamespaceChanges( for _, delta := range diff.Deltas() { switch delta.Type { case nsdiff.RemovedRelation: + // NOTE: We add the subject filters here to ensure the reverse relationship index is used + // by the datastores. As there is no index that has {namespace, relation} directly, but there + // *is* an index that has {subject_namespace, subject_relation, namespace, relation}, we can + // force the datastore to use the reverse index by adding the subject filters. + var previousRelation *core.Relation + for _, relation := range existing.Relation { + if relation.Name == delta.RelationName { + previousRelation = relation + break + } + } + + if previousRelation == nil { + return nil, spiceerrors.MustBugf("relation `%s` not found in existing namespace definition", delta.RelationName) + } + + subjectSelectors := make([]datastore.SubjectsSelector, 0, len(previousRelation.TypeInformation.AllowedDirectRelations)) + for _, allowedType := range previousRelation.TypeInformation.AllowedDirectRelations { + if allowedType.GetRelation() == datastore.Ellipsis { + subjectSelectors = append(subjectSelectors, datastore.SubjectsSelector{ + OptionalSubjectType: allowedType.Namespace, + RelationFilter: datastore.SubjectRelationFilter{ + IncludeEllipsisRelation: true, + }, + }) + } else { + subjectSelectors = append(subjectSelectors, datastore.SubjectsSelector{ + OptionalSubjectType: allowedType.Namespace, + RelationFilter: datastore.SubjectRelationFilter{ + NonEllipsisRelation: allowedType.GetRelation(), + }, + }) + } + } + qy, qyErr := rwt.QueryRelationships(ctx, datastore.RelationshipsFilter{ - OptionalResourceType: nsdef.Name, - OptionalResourceRelation: delta.RelationName, + OptionalResourceType: nsdef.Name, + OptionalResourceRelation: delta.RelationName, + OptionalSubjectsSelectors: subjectSelectors, }, options.WithLimit(options.LimitOne)) err = errorIfTupleIteratorReturnsTuples( diff --git a/internal/services/shared/schema_test.go b/internal/services/shared/schema_test.go index 78012466f4..d2efcad058 100644 --- a/internal/services/shared/schema_test.go +++ b/internal/services/shared/schema_test.go @@ -11,64 +11,320 @@ import ( "github.com/authzed/spicedb/pkg/datastore" "github.com/authzed/spicedb/pkg/schemadsl/compiler" "github.com/authzed/spicedb/pkg/schemadsl/input" + "github.com/authzed/spicedb/pkg/tuple" ) func TestApplySchemaChanges(t *testing.T) { - require := require.New(t) - rawDS, err := memdb.NewMemdbDatastore(0, 0, memdb.DisableGC) - require.NoError(err) - - // Write the initial schema. - ds, _ := testfixtures.DatastoreFromSchemaAndTestRelationships(rawDS, ` - definition user {} - - definition document { - relation viewer: user - permission view = viewer - } - - caveat hasFortyTwo(value int) { - value == 42 - } - `, nil, require) - - // Update the schema and ensure it works. - compiled, err := compiler.Compile(compiler.InputSchema{ - Source: input.Source("schema"), - SchemaString: ` - definition user {} - - definition organization { - relation member: user - permission admin = member - } + tcs := []struct { + name string + startingSchema string + relationships []string + endingSchema string + expectedAppliedSchemaChanges AppliedSchemaChanges + expectedError string + }{ + { + name: "various changes", + startingSchema: ` + definition user {} + + definition document { + relation viewer: user + permission view = viewer + } + + caveat hasFortyTwo(value int) { + value == 42 + } + `, + endingSchema: ` + definition user {} + + definition organization { + relation member: user + permission admin = member + } + + caveat catchTwentyTwo(value int) { + value == 22 + } + `, + expectedAppliedSchemaChanges: AppliedSchemaChanges{ + TotalOperationCount: 5, + NewObjectDefNames: []string{"organization"}, + RemovedObjectDefNames: []string{"document"}, + NewCaveatDefNames: []string{"catchTwentyTwo"}, + RemovedCaveatDefNames: []string{"hasFortyTwo"}, + }, + }, + { + name: "attempt to remove a relation with relationships", + startingSchema: ` + definition user {} + + definition group { + relation member: user + } + + definition org { + relation admin: user + } + + definition document { + relation viewer: user | group#member | org#admin + }`, + relationships: []string{"document:somedoc#viewer@user:alice"}, + endingSchema: ` + definition user {} + + definition group { + relation member: user + } + + definition org { + relation admin: user + } + + definition document {}`, + expectedError: "cannot delete relation `viewer` in object definition `document`, as a relationship exists under it", + }, + { + name: "attempt to remove a relation with indirect relationships", + startingSchema: ` + definition user {} + + definition group { + relation member: user + } + + definition org { + relation admin: user + } + + definition document { + relation viewer: user | group#member | org#admin + }`, + relationships: []string{"document:somedoc#viewer@group:somegroup#member"}, + endingSchema: ` + definition user {} + + definition group { + relation member: user + } + + definition org { + relation admin: user + } + + definition document {}`, + expectedError: "cannot delete relation `viewer` in object definition `document`, as a relationship exists under it", + }, + { + name: "attempt to remove a relation with other indirect relationships", + startingSchema: ` + definition user {} + + definition group { + relation member: user + } + + definition org { + relation admin: user + } + + definition document { + relation viewer: user | group#member | org#admin + }`, + relationships: []string{"document:somedoc#viewer@org:someorg#admin"}, + endingSchema: ` + definition user {} + + definition group { + relation member: user + } + + definition org { + relation admin: user + } + + definition document {}`, + expectedError: "cannot delete relation `viewer` in object definition `document`, as a relationship exists under it", + }, + { + name: "attempt to remove a relation with wildcard", + startingSchema: ` + definition user {} + + definition document { + relation viewer: user:* | user + }`, + relationships: []string{"document:somedoc#viewer@user:*"}, + endingSchema: ` + definition user {} + + definition document {}`, + expectedError: "cannot delete relation `viewer` in object definition `document`, as a relationship exists under it", + }, + { + name: "attempt to remove a relation with only indirect relationships", + startingSchema: ` + definition user {} + + definition group { + relation member: user + } - caveat catchTwentyTwo(value int) { - value == 22 + definition org { + relation admin: user + } + + definition document { + relation viewer: group#member | org#admin + }`, + relationships: []string{"document:somedoc#viewer@org:someorg#admin"}, + endingSchema: ` + definition user {} + + definition group { + relation member: user + } + + definition org { + relation admin: user + } + + definition document {}`, + expectedError: "cannot delete relation `viewer` in object definition `document`, as a relationship exists under it", + }, + { + name: "remove a relation with no relationships", + startingSchema: ` + definition user {} + + definition group { + relation member: user + } + + definition org { + relation admin: user + } + + definition document { + relation viewer: user | group#member | org#admin + }`, + endingSchema: ` + definition user {} + + definition group { + relation member: user + } + + definition org { + relation admin: user + } + + definition document {}`, + expectedAppliedSchemaChanges: AppliedSchemaChanges{ + TotalOperationCount: 4, + }, + }, + { + name: "change the subject type allowed on a relation", + startingSchema: ` + definition user {} + + definition group { + relation member: user + } + + definition document { + relation viewer: user | group#member + permission view = viewer + } + `, + endingSchema: ` + definition user {} + + definition group { + relation member: user + } + + definition document { + relation viewer: user + permission view = viewer + } + `, + expectedAppliedSchemaChanges: AppliedSchemaChanges{ + TotalOperationCount: 3, + }, + }, + { + name: "attempt to change the subject type allowed on a relation with relationships", + startingSchema: ` + definition user {} + + definition group { + relation member: user + } + + definition document { + relation viewer: user | group#member + permission view = viewer + } + `, + relationships: []string{"document:somedoc#viewer@group:somegroup#member"}, + endingSchema: ` + definition user {} + + definition group { + relation member: user + } + + definition document { + relation viewer: user + permission view = viewer + } + `, + expectedError: "cannot remove allowed type `group#member` from relation `viewer` in object definition `document`, as a relationship exists with it", + }, + } + + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + require := require.New(t) + rawDS, err := memdb.NewMemdbDatastore(0, 0, memdb.DisableGC) + require.NoError(err) + + // Write the initial schema. + relationships := make([]tuple.Relationship, 0, len(tc.relationships)) + for _, rel := range tc.relationships { + relationships = append(relationships, tuple.MustParse(rel)) } - `, - }, compiler.AllowUnprefixedObjectType()) - require.NoError(err) - - validated, err := ValidateSchemaChanges(context.Background(), compiled, false) - require.NoError(err) - - _, err = ds.ReadWriteTx(context.Background(), func(ctx context.Context, rwt datastore.ReadWriteTransaction) error { - applied, err := ApplySchemaChanges(context.Background(), rwt, validated) - require.NoError(err) - - require.Equal(applied.NewObjectDefNames, []string{"organization"}) - require.Equal(applied.RemovedObjectDefNames, []string{"document"}) - require.Equal(applied.NewCaveatDefNames, []string{"catchTwentyTwo"}) - require.Equal(applied.RemovedCaveatDefNames, []string{"hasFortyTwo"}) - - orgDef, err := rwt.LookupNamespacesWithNames(ctx, []string{"organization"}) - require.NoError(err) - require.Len(orgDef, 1) - - require.NotEmpty(orgDef[0].Definition.Relation[0].CanonicalCacheKey) - require.NotEmpty(orgDef[0].Definition.Relation[1].CanonicalCacheKey) - return nil - }) - require.NoError(err) + + ds, _ := testfixtures.DatastoreFromSchemaAndTestRelationships(rawDS, tc.startingSchema, relationships, require) + + // Update the schema and ensure it works. + compiled, err := compiler.Compile(compiler.InputSchema{ + Source: input.Source("schema"), + SchemaString: tc.endingSchema, + }, compiler.AllowUnprefixedObjectType()) + require.NoError(err) + + validated, err := ValidateSchemaChanges(context.Background(), compiled, false) + require.NoError(err) + + _, err = ds.ReadWriteTx(context.Background(), func(ctx context.Context, rwt datastore.ReadWriteTransaction) error { + applied, err := ApplySchemaChanges(context.Background(), rwt, validated) + if tc.expectedError != "" { + require.EqualError(err, tc.expectedError) + return nil + } + + require.NoError(err) + require.Equal(tc.expectedAppliedSchemaChanges, *applied) + return nil + }) + require.NoError(err) + }) + } }