diff --git a/internal/datastore/mysql/datastore_test.go b/internal/datastore/mysql/datastore_test.go index 671749f595..fb197e2e58 100644 --- a/internal/datastore/mysql/datastore_test.go +++ b/internal/datastore/mysql/datastore_test.go @@ -106,6 +106,11 @@ func TestMySQL8Datastore(t *testing.T) { additionalMySQLTests(t, b) } +func TestMySQLRevisionTimestamps(t *testing.T) { + b := testdatastore.RunMySQLForTestingWithOptions(t, testdatastore.MySQLTesterOptions{MigrateForNewDatastore: true}, "") + t.Run("TransactionTimestamps", createDatastoreTest(b, TransactionTimestampsTest, defaultOptions...)) +} + func additionalMySQLTests(t *testing.T, b testdatastore.RunningEngineForTest) { reg := prometheus.NewRegistry() prometheus.DefaultGatherer = reg @@ -122,7 +127,6 @@ func additionalMySQLTests(t *testing.T, b testdatastore.RunningEngineForTest) { t.Run("ChunkedGarbageCollection", createDatastoreTest(b, ChunkedGarbageCollectionTest, defaultOptions...)) t.Run("EmptyGarbageCollection", createDatastoreTest(b, EmptyGarbageCollectionTest, defaultOptions...)) t.Run("NoRelationshipsGarbageCollection", createDatastoreTest(b, NoRelationshipsGarbageCollectionTest, defaultOptions...)) - t.Run("TransactionTimestamps", createDatastoreTest(b, TransactionTimestampsTest, defaultOptions...)) t.Run("QuantizedRevisions", func(t *testing.T) { QuantizedRevisionTest(t, b) }) diff --git a/internal/services/shared/schema.go b/internal/services/shared/schema.go index 7263ddcb57..219885bc6a 100644 --- a/internal/services/shared/schema.go +++ b/internal/services/shared/schema.go @@ -319,10 +319,46 @@ 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( ctx, diff --git a/internal/services/shared/schema_test.go b/internal/services/shared/schema_test.go index 78012466f4..007af287db 100644 --- a/internal/services/shared/schema_test.go +++ b/internal/services/shared/schema_test.go @@ -9,66 +9,323 @@ import ( "github.com/authzed/spicedb/internal/datastore/memdb" "github.com/authzed/spicedb/internal/testfixtures" "github.com/authzed/spicedb/pkg/datastore" + corev1 "github.com/authzed/spicedb/pkg/proto/core/v1" "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([]*corev1.RelationTuple, 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) + }) + } }