Change namespace+relation validation to use a single DB lookup

Should reduce DB roundtrips and goroutine usage

Fixes #1333

internal/namespace/util.go

@@ -35,6 +35,69 @@ func ReadNamespaceAndRelation(
 	return nil, nil, NewRelationNotFoundErr(namespace, relation)
 }
 
+// TypeAndRelationToCheck is a single check of a namespace+relation pair.
+type TypeAndRelationToCheck struct {
+	// NamespaceName is the namespace name to ensure exists.
+	NamespaceName string
+
+	// RelationName is the relation name to ensure exists under the namespace.
+	RelationName string
+
+	// AllowEllipsis, if true, allows for the ellipsis as the RelationName.
+	AllowEllipsis bool
+}
+
+// CheckNamespaceAndRelations ensures that the given namespace+relation checks all succeed. If any fail, returns an error.
+//
+// Returns ErrNamespaceNotFound if the namespace cannot be found.
+// Returns ErrRelationNotFound if the relation was not found in the namespace.
+// Returns the direct downstream error for all other unknown error.
+func CheckNamespaceAndRelations(ctx context.Context, checks []TypeAndRelationToCheck, ds datastore.Reader) error {
+	nsNames := util.NewSet[string]()
+	for _, toCheck := range checks {
+		nsNames.Add(toCheck.NamespaceName)
+	}
+
+	if nsNames.IsEmpty() {
+		return nil
+	}
+
+	namespaces, err := ds.LookupNamespacesWithNames(ctx, nsNames.AsSlice())
+	if err != nil {
+		return err
+	}
+
+	mappedNamespaces := make(map[string]*core.NamespaceDefinition, len(namespaces))
+	for _, namespace := range namespaces {
+		mappedNamespaces[namespace.Definition.Name] = namespace.Definition
+	}
+
+	for _, toCheck := range checks {
+		nsDef, ok := mappedNamespaces[toCheck.NamespaceName]
+		if !ok {
+			return NewNamespaceNotFoundErr(toCheck.NamespaceName)
+		}
+
+		if toCheck.AllowEllipsis && toCheck.RelationName == datastore.Ellipsis {
+			continue
+		}
+
+		foundRelation := false
+		for _, rel := range nsDef.Relation {
+			if rel.Name == toCheck.RelationName {
+				foundRelation = true
+				break
+			}
+		}
+
+		if !foundRelation {
+			return NewRelationNotFoundErr(toCheck.NamespaceName, toCheck.RelationName)
+		}
+	}
+
+	return nil
+}
+
 // CheckNamespaceAndRelation checks that the specified namespace and relation exist in the
 // datastore.
 //

internal/namespace/util_test.go

@@ -1,14 +1,18 @@
-package namespace
+package namespace_test
 
 import (
+	"context"
 	"sort"
 	"testing"
 
-	"github.com/stretchr/testify/require"
+	"github.com/authzed/spicedb/internal/datastore/memdb"
 
-	core "github.com/authzed/spicedb/pkg/proto/core/v1"
+	"github.com/stretchr/testify/require"
 
+	"github.com/authzed/spicedb/internal/namespace"
+	"github.com/authzed/spicedb/internal/testfixtures"
 	ns "github.com/authzed/spicedb/pkg/namespace"
+	core "github.com/authzed/spicedb/pkg/proto/core/v1"
 )
 
 func TestListReferencedNamespaces(t *testing.T) {
@@ -59,11 +63,122 @@ func TestListReferencedNamespaces(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			require := require.New(t)
 
-			found := ListReferencedNamespaces(tc.toCheck)
+			found := namespace.ListReferencedNamespaces(tc.toCheck)
 			sort.Strings(found)
 			sort.Strings(tc.expectedNames)
 
 			require.Equal(tc.expectedNames, found)
 		})
 	}
 }
+
+func TestCheckNamespaceAndRelations(t *testing.T) {
+	tcs := []struct {
+		name          string
+		schema        string
+		checks        []namespace.TypeAndRelationToCheck
+		expectedError string
+	}{
+		{
+			"missing namespace",
+			`definition user {}`,
+			[]namespace.TypeAndRelationToCheck{
+				{
+					NamespaceName: "user",
+					RelationName:  "...",
+					AllowEllipsis: true,
+				},
+				{
+					NamespaceName: "resource",
+					RelationName:  "viewer",
+					AllowEllipsis: false,
+				},
+			},
+			"object definition `resource` not found",
+		},
+		{
+			"missing relation",
+			`definition resource {}`,
+			[]namespace.TypeAndRelationToCheck{
+				{
+					NamespaceName: "resource",
+					RelationName:  "viewer",
+					AllowEllipsis: false,
+				},
+			},
+			"relation/permission `viewer` not found",
+		},
+		{
+			"valid",
+			`definition user {}
+			
+			definition resource {
+				relation viewer: user
+			}
+			`,
+			[]namespace.TypeAndRelationToCheck{
+				{
+					NamespaceName: "user",
+					RelationName:  "...",
+					AllowEllipsis: true,
+				},
+				{
+					NamespaceName: "resource",
+					RelationName:  "viewer",
+					AllowEllipsis: false,
+				},
+			},
+			"",
+		},
+		{
+			"ellipsis not allowed",
+			`definition user {}`,
+			[]namespace.TypeAndRelationToCheck{
+				{
+					NamespaceName: "user",
+					RelationName:  "...",
+					AllowEllipsis: false,
+				},
+			},
+			"relation/permission `...` not found",
+		},
+		{
+			"another missing namespace",
+			`definition user {}`,
+			[]namespace.TypeAndRelationToCheck{
+				{
+					NamespaceName: "resource",
+					RelationName:  "viewer",
+					AllowEllipsis: false,
+				},
+				{
+					NamespaceName: "user",
+					RelationName:  "...",
+					AllowEllipsis: true,
+				},
+			},
+			"object definition `resource` not found",
+		},
+	}
+	for _, tc := range tcs {
+		t.Run(tc.name, func(t *testing.T) {
+			req := require.New(t)
+			rawDS, err := memdb.NewMemdbDatastore(0, 0, memdb.DisableGC)
+			req.NoError(err)
+
+			ds, _ := testfixtures.DatastoreFromSchemaAndTestRelationships(rawDS, tc.schema, nil, req)
+
+			rev, err := ds.HeadRevision(context.Background())
+			require.NoError(t, err)
+
+			reader := ds.SnapshotReader(rev)
+
+			err = namespace.CheckNamespaceAndRelations(context.Background(), tc.checks, reader)
+			if tc.expectedError == "" {
+				require.Nil(t, err)
+			} else {
+				require.ErrorContains(t, err, tc.expectedError)
+			}
+		})
+	}
+}

internal/services/v1/permissions.go

@@ -10,7 +10,6 @@ import (
 	"github.com/authzed/authzed-go/pkg/responsemeta"
 	v1 "github.com/authzed/authzed-go/proto/authzed/api/v1"
 	"github.com/jzelinskie/stringz"
-	"golang.org/x/sync/errgroup"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/metadata"
 	"google.golang.org/grpc/status"
@@ -45,27 +44,19 @@ func (ps *permissionServer) CheckPermission(ctx context.Context, req *v1.CheckPe
 		return nil, shared.RewriteError(ctx, err)
 	}
 
-	// Perform our preflight checks in parallel
-	errG, checksCtx := errgroup.WithContext(ctx)
-	errG.Go(func() error {
-		return namespace.CheckNamespaceAndRelation(
-			checksCtx,
-			req.Resource.ObjectType,
-			req.Permission,
-			false,
-			ds,
-		)
-	})
-	errG.Go(func() error {
-		return namespace.CheckNamespaceAndRelation(
-			checksCtx,
-			req.Subject.Object.ObjectType,
-			normalizeSubjectRelation(req.Subject),
-			true,
-			ds,
-		)
-	})
-	if err := errG.Wait(); err != nil {
+	if err := namespace.CheckNamespaceAndRelations(ctx,
+		[]namespace.TypeAndRelationToCheck{
+			{
+				NamespaceName: req.Resource.ObjectType,
+				RelationName:  req.Permission,
+				AllowEllipsis: false,
+			},
+			{
+				NamespaceName: req.Subject.Object.ObjectType,
+				RelationName:  normalizeSubjectRelation(req.Subject),
+				AllowEllipsis: true,
+			},
+		}, ds); err != nil {
 		return nil, shared.RewriteError(ctx, err)
 	}
 
@@ -332,27 +323,19 @@ func (ps *permissionServer) LookupResources(req *v1.LookupResourcesRequest, resp
 
 	ds := datastoremw.MustFromContext(ctx).SnapshotReader(atRevision)
 
-	// Perform our preflight checks in parallel
-	errG, checksCtx := errgroup.WithContext(ctx)
-	errG.Go(func() error {
-		return namespace.CheckNamespaceAndRelation(
-			checksCtx,
-			req.Subject.Object.ObjectType,
-			normalizeSubjectRelation(req.Subject),
-			true,
-			ds,
-		)
-	})
-	errG.Go(func() error {
-		return namespace.CheckNamespaceAndRelation(
-			ctx,
-			req.ResourceObjectType,
-			req.Permission,
-			false,
-			ds,
-		)
-	})
-	if err := errG.Wait(); err != nil {
+	if err := namespace.CheckNamespaceAndRelations(ctx,
+		[]namespace.TypeAndRelationToCheck{
+			{
+				NamespaceName: req.ResourceObjectType,
+				RelationName:  req.Permission,
+				AllowEllipsis: false,
+			},
+			{
+				NamespaceName: req.Subject.Object.ObjectType,
+				RelationName:  normalizeSubjectRelation(req.Subject),
+				AllowEllipsis: true,
+			},
+		}, ds); err != nil {
 		return shared.RewriteError(ctx, err)
 	}
 
@@ -416,27 +399,19 @@ func (ps *permissionServer) LookupSubjects(req *v1.LookupSubjectsRequest, resp v
 		return shared.RewriteError(ctx, err)
 	}
 
-	// Perform our preflight checks in parallel
-	errG, checksCtx := errgroup.WithContext(ctx)
-	errG.Go(func() error {
-		return namespace.CheckNamespaceAndRelation(
-			checksCtx,
-			req.Resource.ObjectType,
-			req.Permission,
-			false,
-			ds,
-		)
-	})
-	errG.Go(func() error {
-		return namespace.CheckNamespaceAndRelation(
-			ctx,
-			req.SubjectObjectType,
-			stringz.DefaultEmpty(req.OptionalSubjectRelation, tuple.Ellipsis),
-			true,
-			ds,
-		)
-	})
-	if err := errG.Wait(); err != nil {
+	if err := namespace.CheckNamespaceAndRelations(ctx,
+		[]namespace.TypeAndRelationToCheck{
+			{
+				NamespaceName: req.Resource.ObjectType,
+				RelationName:  req.Permission,
+				AllowEllipsis: false,
+			},
+			{
+				NamespaceName: req.SubjectObjectType,
+				RelationName:  stringz.DefaultEmpty(req.OptionalSubjectRelation, tuple.Ellipsis),
+				AllowEllipsis: true,
+			},
+		}, ds); err != nil {
 		return shared.RewriteError(ctx, err)
 	}